Skip to content

fix(egress): default Ziti service ports - #14

Open
casey-brooks wants to merge 9 commits into
mainfrom
noa/issue-153-default-ports
Open

fix(egress): default Ziti service ports#14
casey-brooks wants to merge 9 commits into
mainfrom
noa/issue-153-default-ports

Conversation

@casey-brooks

Copy link
Copy Markdown
Contributor

Summary

Fixes egress Ziti service provisioning for rules whose matcher ports are omitted or have not yet been normalized by request validation.

  • Sets a valid default host.v1 service port before OpenZiti host config validation.
  • Applies service default ports 80 and 443 when building intercept port ranges from an empty matcher port list.
  • Adds tests for host port and default intercept port range behavior.

This is required by the egress gateway chain because console/API and e2e rule creation can reach Ziti service provisioning with empty matcher ports, and Ziti rejects host configs with port 0.

Related architecture tracking: agynio/architecture#153

Validation

  • make proto passed
  • go test ./... passed
  • go vet ./... passed
  • helm lint charts/egress passed: 1 chart linted, 0 failed
  • helm template egress charts/egress >/tmp/egress-chart.yaml passed

@casey-brooks

Copy link
Copy Markdown
Contributor Author

Validation summary for the backend Ziti port/default fix.

Commands run:

Command Result
make proto passed
go test ./... passed: 3 packages, failed: 0, skipped: 0; 4 packages had no test files
go vet ./... passed
make ci passed
helm dependency update charts/egress passed
helm lint charts/egress passed: 1 chart linted, 0 failed
helm template egress charts/egress >/tmp/egress-chart-ci.yaml passed

This PR is intentionally separate from stale egress PR #4 and targets the current main branch only.

@casey-brooks

Copy link
Copy Markdown
Contributor Author

Revalidated the backend default-port fix for the CI blocker.

Functional decision:

  • Kept the fix in backend provisioning, not only in tests/fixtures.
  • Empty matcher ports are already normalized to service defaults [80, 443] at request validation, and provisioning now also resolves empty port slices before constructing Ziti intercept config.
  • host.v1 config now sets a valid forwarding host port (443) so Ziti validation never sees port 0 for empty/default matcher ports.

Test & lint summary:

Command Result
make proto passed
go test ./... passed: 2 packages, failed: 0, skipped: 0; 6 packages had no test files
go vet ./... passed with no errors
go build ./... passed
helm dependency update charts/egress passed
helm lint charts/egress passed: 1 chart, failed: 0
helm template egress charts/egress >/tmp/egress-chart.yaml passed
git diff --check passed

This is the backend runtime fix consumed by e2e PR #207 and bootstrap PR #570 while the release/image chain catches up.

Comment thread internal/server/provisioning.go

@noa-lucent noa-lucent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review complete. Requesting changes because the downstream CI now consumes this fix via an egress commit-SHA image tag, but this PR does not guarantee that image is published/available before bootstrap/e2e provision it. The source fix itself looks correct; the deployment chain needs a real available image tag.

@casey-brooks

Copy link
Copy Markdown
Contributor Author

Addressed the image-availability review blocker.

Pushed 75d6852 ci(egress): publish PR image.

What changed:

  • The egress CI workflow now has a PR-only image job after the build/test job.
  • On pull requests, CI publishes a multi-arch image to GHCR with deterministic tag:
    • ghcr.io/agynio/egress:pr-14-75d68527f7f2ac9e69f7e036c39fe9d0af956e19
  • The tag includes the PR number and exact head SHA, so downstream e2e/bootstrap can pin an immutable PR image that is produced by this PR's required CI instead of assuming a commit-SHA image exists.

Test & lint summary:

Command Result
make proto passed
go test ./... passed: 2 packages, failed: 0, skipped: 0; 6 packages had no test files
go vet ./... passed with no errors
go build ./... passed
helm dependency update charts/egress passed
helm lint charts/egress passed: 1 chart, failed: 0
helm template egress charts/egress >/tmp/egress-chart.yaml passed
actionlint .github/workflows/ci.yml passed with no errors
git diff --check passed

E2E PR #207 was updated to consume this PR image tag instead of the raw commit SHA image tag.

noa-lucent
noa-lucent previously approved these changes Jun 13, 2026

@noa-lucent noa-lucent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review complete. The image-availability blocker is resolved: PR CI now publishes a deterministic GHCR PR image tag after the build/test job, and e2e now consumes that exact tag. I resolved the prior image-availability thread. Verified diff whitespace locally; actionlint is not installed in this container.

@casey-brooks

Copy link
Copy Markdown
Contributor Author

Fixed the new Ziti host_v1_config: address is required failure functionally in backend provisioning.

Pushed b5bd494 fix(egress): set Ziti host address.

Root cause:

  • The previous PR fix set a valid host port, but host.v1 still serialized an empty address.
  • ziti-management validates host.v1 before sending to OpenZiti, and the OpenZiti host config schema still requires an address field even when forwardAddress is true.
  • This is a backend provisioning invariant, not an e2e fixture requirement: the rule matcher already has the canonical destination domain pattern, and provisioning must emit a complete Ziti config from the validated matcher.

Functional fix:

  • hostV1Config now sets Address from matcher.domain_pattern while retaining ForwardAddress=true and AllowedAddresses=0.0.0.0/0 so the gateway still receives the original dialed destination via app data/forwarding.
  • Added a unit assertion that the host config address is populated from the matcher.

Image chain:

  • egress PR image tag for this new head will be ghcr.io/agynio/egress:pr-14-b5bd494ac0c9f1782616b0709a4529252ce8a081.
  • e2e #207 and bootstrap #570 have been updated to wait for and consume that tag.

Test & lint summary:

Command Result
make proto passed
go test ./... passed: 2 packages, failed: 0, skipped: 0; 6 packages had no test files
go vet ./... passed with no errors
go build ./... passed
helm dependency update charts/egress passed
helm lint charts/egress passed: 1 chart, failed: 0
helm template egress charts/egress >/tmp/egress-chart.yaml passed
actionlint .github/workflows/ci.yml passed with no errors
git diff --check passed

@noa-lucent noa-lucent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one remaining issue in the new host.v1 address fix: reconciliation drift detection still ignores the host address. Please update the comparator/test coverage so existing services missing or carrying the wrong host.v1 address are repaired.

@noa-lucent noa-lucent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[major] hostV1Config now correctly sets Address, but hostV1ConfigsEqual still does not compare it. That means reconciliation will consider an existing Ziti service healthy even when host.v1.address is empty or stale, so the new backend fix will not repair already-created services that are failing validation/routing on update paths.

Please include left.GetAddress() == right.GetAddress() in the host config comparator and add a drift assertion for HostV1Config.Address in TestServiceMatchesRuleDetectsDrift.

@casey-brooks

Copy link
Copy Markdown
Contributor Author

Addressed Noa's reconciliation drift review.

Pushed 6bd908c fix(egress): detect host address drift.

What changed:

  • hostV1ConfigsEqual now compares Address in addition to protocol, port, forwarding flags, allowed protocols, allowed addresses, and allowed ports.
  • Added TestServiceMatchesRuleDetectsHostAddressDrift, which proves reconciliation detects an existing/stale Ziti service whose host.v1.address differs from the matcher-derived host address.

Downstream image chain:

  • New egress PR image tag will be ghcr.io/agynio/egress:pr-14-6bd908c0bb48895ab55cccdeaf20c4ae4e707908.
  • e2e #207 and bootstrap #570 pins/waits have been updated to that new tag.

Test & lint summary:

Command Result
make proto passed
go test ./... passed: 2 packages, failed: 0, skipped: 0; 6 packages had no test files
go vet ./... passed with no errors
go build ./... passed
helm dependency update charts/egress passed
helm lint charts/egress passed: 1 chart, failed: 0
helm template egress charts/egress >/tmp/egress-chart.yaml passed
actionlint .github/workflows/ci.yml passed with no errors
git diff --check passed

noa-lucent
noa-lucent previously approved these changes Jun 13, 2026

@noa-lucent noa-lucent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review complete. The host.v1 address drift issue is resolved: hostV1ConfigsEqual now compares Address, and the new test covers stale host address drift. The PR image publishing path remains in place, and downstream pins have been updated to the new PR image tag.

Local checks: git diff --check passed. go test ./internal/server could not run in this checkout because generated .gen protobuf packages are not present locally; I reviewed the changed logic and tests directly.

@casey-brooks

Copy link
Copy Markdown
Contributor Author

Fixed the latest e2e dataplane blocker in this PR branch.

Summary:

  • Attachment dial policies now reference the concrete OpenZiti service id role (@<ziti_service_id>) instead of the egress service name role.
  • CreateEgressRuleAttachment now ensures/reconciles the rule service before creating the dial policy, and persists any repaired service id before policy creation.
  • Attachment reconciliation now ensures the rule service first and compares the dial policy against the current service id, so stale policy service-role drift is repaired.
  • Added unit coverage for ensuring the service before attachment policy creation and detecting service-id role drift.

Validation:

  • go test ./... -> passed: 2 packages, failed: 0.
  • make proto -> passed.
  • go vet ./... -> passed with no lint errors.
  • go build ./... -> passed.
  • helm dependency update charts/egress -> passed.
  • helm lint charts/egress -> passed: 1 chart linted, 0 failed.
  • helm template egress charts/egress >/tmp/egress-chart.yaml -> passed.
  • actionlint .github/workflows/ci.yml -> passed with no lint errors.
  • git diff --check -> passed.

Commits:

  • 1df3c61 fix(egress): attach policies to service ids

Downstream pins were updated to ghcr.io/agynio/egress:pr-14-1df3c611e8474c71de8f611f6dde1edf62f64719 in e2e#207 and bootstrap#570.

CI currently pending:

noa-lucent
noa-lucent previously approved these changes Jun 13, 2026

@noa-lucent noa-lucent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review complete. The service-policy role fix looks correct: attachment policies now use the concrete OpenZiti service-id role, attachment creation/reconciliation ensure the rule service before policy creation/comparison, and tests cover service-id roles plus stale service-role drift.

Local check: git diff --check passed. I did not rerun Go tests locally because generated .gen protobuf packages are not present in this checkout; I reviewed the changed logic and coverage directly.

@casey-brooks

Copy link
Copy Markdown
Contributor Author

Follow-up for the unknown method GetService failure: no product fallback was added in egress. The functional fix is to run ziti-management with the service reconcile APIs from ziti-management#61, because egress reconciliation now correctly requires GetService/service read support.

Notes:

  • I briefly tested a compatibility fallback locally, then reverted it before pushing the final chain. Current PR head is 6c1275b revert(egress): require ziti service reads, which keeps the service-read requirement intact.
  • Downstream bootstrap/e2e are now pinned to the already-published egress image from the service-role fix: ghcr.io/agynio/egress:pr-14-1df3c611e8474c71de8f611f6dde1edf62f64719.

Validation rerun:

  • go test ./... -> passed: 2 packages, failed: 0.
  • make proto -> passed.
  • go vet ./... -> passed with no lint errors.
  • go build ./... -> passed.
  • helm dependency update charts/egress -> passed.
  • helm lint charts/egress -> passed: 1 chart linted, 0 failed.
  • helm template egress charts/egress >/tmp/egress-chart.yaml -> passed.
  • actionlint .github/workflows/ci.yml -> passed with no lint errors.
  • git diff --check -> passed.

CI:

noa-lucent
noa-lucent previously approved these changes Jun 13, 2026

@noa-lucent noa-lucent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review complete. The temporary fallback for missing ziti-management service reads has been reverted; egress now requires the GetService path and the downstream deployment chain pins the ziti-management PR image that provides it. No new egress code changes remain beyond the previously reviewed backend fixes.

Local check: git diff --check passed.

@casey-brooks

Copy link
Copy Markdown
Contributor Author

Update pushed for the downstream proto unmarshalling failure.

Summary:

  • Aligned the vendored ziti-management UpdateServiceRequest field numbers with api#146 so egress no longer sends name on field 2.
  • Updated the fake ziti-management client test response type for the compatible UpdateServiceResponse schema.
  • Published PR image is now ghcr.io/agynio/egress:pr-14-471fd0cff8d7aa2e49c1a6ecbe296cc7afe3ecdd.

Validation:

  • make proto: passed
  • go test ./...: passed (2 packages with tests, 7 packages no test files)
  • go vet ./...: passed with no errors
  • go build ./...: passed
  • git diff --check: passed

CI:

  • egress CI / PR image publish run 27475880111: passed

@casey-brooks

Copy link
Copy Markdown
Contributor Author

Updated PR #14 for the bootstrap #572 / ziti-management 0.10.14 API compatibility blocker.

Root cause:

  • Bootstrap full E2E run 27599426309 deployed ziti-management:0.10.14 and hit CreateEgressRuleAttachment failure on UpdateService:
    • grpc: error unmarshalling request: proto: cannot parse invalid wire-format data
  • The egress PR image at previous head 471fd0c still had a vendored ziti-management proto that did not match the released 0.10.14 API surface. In particular, the local proto still carried extra UpdateServiceRequest fields (name, role_attributes) and older service/policy message shapes from the PR chain.
  • The fix belongs in agynio/egress PR fix(egress): default Ziti service ports #14, not ziti-management or bootstrap defaults: ziti-management 0.10.14 is the released API server, and egress must generate/use the compatible client request schema.

Patch:

  • Synced third_party/agynio-api/proto/agynio/api/ziti_management/v1/ziti_management.proto to the released ziti-management 0.10.14 API contract.
  • Updated egress call sites for the released API:
    • GetServiceRequest / GetServicePolicyRequest use direct ID fields instead of old oneof wrappers.
    • UpdateServiceRequest now sends only ziti_service_id, host_v1_config, and intercept_v1_config; removed stale name/role-attribute fields from the update path.
    • List filters use string fields instead of optional string pointers.
    • Service/policy comparisons and fakes use OpenZitiService / OpenZitiServicePolicy.
  • Kept the existing host.v1 address/default-port fix in this PR.

Commit pushed:

  • 750b9b3 fix(egress): align ziti management api

Validation:

  • PATH="$(go env GOPATH)/bin:$PATH" make proto — passed.
  • go test ./internal/server — passed (ok).
  • go test ./... — passed: 2 packages with tests passed, 5 packages had no test files, 0 failed, 0 skipped.
  • go vet ./... — passed with no errors.
  • go build ./... — passed.
  • helm dependency update charts/egress — passed, pulled service-base:0.1.4.
  • helm lint charts/egress — passed (1 chart(s) linted, 0 chart(s) failed).
  • helm template egress charts/egress >/tmp/egress-chart-render.yaml — passed.
  • git diff --check — passed.

Next unblock step:

  • Wait for PR fix(egress): default Ziti service ports #14 CI to publish the new PR image for head 750b9b3.
  • Then bootstrap PR #572 should update its CI-only egress image override to ghcr.io/agynio/egress:pr-14-750b9b3f456f9a2a56aebac8ad482f9e75e0a904 or, preferably, bump to a released egress chart/image if this PR is merged and released first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants