fix(egress): default Ziti service ports - #14
Conversation
|
Validation summary for the backend Ziti port/default fix. Commands run:
This PR is intentionally separate from stale egress PR #4 and targets the current |
|
Revalidated the backend default-port fix for the CI blocker. Functional decision:
Test & lint summary:
This is the backend runtime fix consumed by e2e PR #207 and bootstrap PR #570 while the release/image chain catches up. |
noa-lucent
left a comment
There was a problem hiding this comment.
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.
|
Addressed the image-availability review blocker. Pushed What changed:
Test & lint summary:
E2E PR #207 was updated to consume this PR image tag instead of the raw commit SHA image tag. |
noa-lucent
left a comment
There was a problem hiding this comment.
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.
|
Fixed the new Ziti Pushed Root cause:
Functional fix:
Image chain:
Test & lint summary:
|
noa-lucent
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
[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.
|
Addressed Noa's reconciliation drift review. Pushed What changed:
Downstream image chain:
Test & lint summary:
|
noa-lucent
left a comment
There was a problem hiding this comment.
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.
|
Fixed the latest e2e dataplane blocker in this PR branch. Summary:
Validation:
Commits:
Downstream pins were updated to CI currently pending:
|
noa-lucent
left a comment
There was a problem hiding this comment.
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.
|
Follow-up for the Notes:
Validation rerun:
CI:
|
noa-lucent
left a comment
There was a problem hiding this comment.
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.
|
Update pushed for the downstream proto unmarshalling failure. Summary:
Validation:
CI:
|
|
Updated PR #14 for the bootstrap #572 / ziti-management 0.10.14 API compatibility blocker. Root cause:
Patch:
Commit pushed:
Validation:
Next unblock step:
|
Summary
Fixes egress Ziti service provisioning for rules whose matcher ports are omitted or have not yet been normalized by request validation.
host.v1service port before OpenZiti host config validation.80and443when building intercept port ranges from an empty matcher port list.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 protopassedgo test ./...passedgo vet ./...passedhelm lint charts/egresspassed: 1 chart linted, 0 failedhelm template egress charts/egress >/tmp/egress-chart.yamlpassed