Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a confusing reconcile failure in GatewayNamespace mode where a pre-existing, unmanaged Deployment/DaemonSet with the same name as the Gateway could trigger the selector-migration (“illegal change in a custom label”) error before the ownership guard ran. The ownership check is now evaluated earlier so same-name collisions surface a clear ownership-conflict error and the pre-existing resource is left untouched.
Changes:
- Run
checkOwnershipimmediately after a successfulGet(and before selector-migration logic) for Deployments and DaemonSets. - Add unit regression coverage for unmanaged same-name collisions (Deployment + DaemonSet) and a positive “EG-owned reconcile still works” case.
- Add an e2e subtest for a pre-existing unmanaged Deployment name collision, plus a release note fragment.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
internal/infrastructure/kubernetes/infra_resource.go |
Moves the ownership guard earlier in Deployment/DaemonSet reconcile paths to return a clear ownership-conflict error before selector-migration logic. |
internal/infrastructure/kubernetes/infra_resource_test.go |
Adds regression unit tests for unmanaged same-name Deployment/DaemonSet collisions and validates EG-owned reconcile still succeeds. |
test/e2e/tests/gateway_namespace_ownership.go |
Adds e2e coverage ensuring a pre-existing unmanaged Deployment is not adopted or garbage-collected in GatewayNamespace mode. |
release-notes/current/bug_fixes/9132-gateway-namespace-unmanaged-resource-conflict.md |
Documents the bug fix and the improved error surfacing for unmanaged same-name infra resources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2be9ab1f36
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9470 +/- ##
=======================================
Coverage 81.42% 81.43%
=======================================
Files 266 266
Lines 41300 41323 +23
=======================================
+ Hits 33630 33650 +20
- Misses 7669 7672 +3
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…yNamespace mode Address review feedback on envoyproxy#9470 (Codex P2 + Copilot). The production controller cache filters DaemonSets (and HPA/PDB) by the envoy managed-label selector, so an unmanaged same-name DaemonSet is absent from the cache: checkOwnership's cached Get returned NotFound and the ownership guard never ran, so applyIfOwned proceeded to server-side apply and users still got an API/immutability error or adoption instead of the ownership-conflict error. The earlier unit tests used an unfiltered fake client and so missed this. Wire the uncached API reader (mgr.GetAPIReader()) through KubernetesClientHolder -> Provider.GetAPIReader -> Infra.apiReader, and use it for the existence read in checkOwnership (falling back to the cached client when no API reader is configured, e.g. in unit tests). This makes the ownership-conflict detection reliable for all resource kinds, including the label-filtered ones, matching the intent claimed by envoyproxy#9215. Also set replicas=0 on the e2e pre-existing unmanaged Deployment to keep it inert (no scheduling/image-pull), per Copilot review — the collision behavior under test needs no Pods to run. Tests: - TestCheckOwnership_UsesAPIReaderWhenCacheMisses: cached Get returns NotFound (simulating the filtered cache) while the API reader sees the unmanaged resource; asserts the ownership error is surfaced. Verified to FAIL on the pre-API-reader code. - TestCheckOwnership_FallsBackToCachedClientWhenNoAPIReader: preserves the cached-client path used by the existing tests. - Existing unit + e2e tests still pass. Signed-off-by: liuhy <liuhongyu@apache.org> Co-Authored-By: Claude <noreply@anthropic.com>
Review feedback addressed (d2da3f1)Thanks @copilot and @codex for the review. Both points addressed: Codex P2 — use an unfiltered read for DaemonSet ownership conflicts (real bug) Fix: wire the uncached API reader ( Copilot — keep the e2e Deployment inert Checks
Note on the 3 failing e2e jobs from the first runThey failed on |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…nt/DaemonSet in GatewayNamespace mode In GatewayNamespace mode, when a pre-existing, unmanaged Deployment or DaemonSet shares the Gateway's name (e.g. an unrelated nginx Deployment), the reconcile surfaced a misleading "illegal change in a custom label of EnvoyProxy" error instead of a clear ownership-conflict error. This happened because createOrUpdateDeployment/createOrUpdateDaemonSet ran the selector-migration workaround before the ownership check: an unmanaged same-name resource with a different selector hit the selector-migration error and never reached the ownership guard added by envoyproxy#9215. Move checkOwnership to run right after the existing Get succeeds and before the selector-migration block, so unmanaged collisions return "already exists and is not owned by this Gateway" and the pre-existing resource is left untouched. Non-GatewayNamespace mode is unaffected (checkOwnership returns nil immediately), so the legitimate custom-label migration error path is preserved. Fixes envoyproxy#9132 Signed-off-by: liuhy <liuhongyu@apache.org> Co-Authored-By: Claude <noreply@anthropic.com>
…yNamespace mode Address review feedback on envoyproxy#9470 (Codex P2 + Copilot). The production controller cache filters DaemonSets (and HPA/PDB) by the envoy managed-label selector, so an unmanaged same-name DaemonSet is absent from the cache: checkOwnership's cached Get returned NotFound and the ownership guard never ran, so applyIfOwned proceeded to server-side apply and users still got an API/immutability error or adoption instead of the ownership-conflict error. The earlier unit tests used an unfiltered fake client and so missed this. Wire the uncached API reader (mgr.GetAPIReader()) through KubernetesClientHolder -> Provider.GetAPIReader -> Infra.apiReader, and use it for the existence read in checkOwnership (falling back to the cached client when no API reader is configured, e.g. in unit tests). This makes the ownership-conflict detection reliable for all resource kinds, including the label-filtered ones, matching the intent claimed by envoyproxy#9215. Also set replicas=0 on the e2e pre-existing unmanaged Deployment to keep it inert (no scheduling/image-pull), per Copilot review — the collision behavior under test needs no Pods to run. Tests: - TestCheckOwnership_UsesAPIReaderWhenCacheMisses: cached Get returns NotFound (simulating the filtered cache) while the API reader sees the unmanaged resource; asserts the ownership error is surfaced. Verified to FAIL on the pre-API-reader code. - TestCheckOwnership_FallsBackToCachedClientWhenNoAPIReader: preserves the cached-client path used by the existing tests. - Existing unit + e2e tests still pass. Signed-off-by: liuhy <liuhongyu@apache.org> Co-Authored-By: Claude <noreply@anthropic.com>
CI lint failed on gateway_namespace_ownership.go:183 because forbidigo (tools/linter/golangci-lint/.golangci.yml) forbids ptr.To from k8s.io/utils/ptr in favor of the new() builtin. ptr.To[int32](0) and new(int32) are equivalent (both yield a *int32 to zero), so switch to new(int32) and drop the now-unused import. The APIReader cache-miss fix is unchanged. Signed-off-by: liuhy <liuhongyu@apache.org> Co-Authored-By: Claude <noreply@anthropic.com>
|
/retest |
Signed-off-by: liuhy <liuhongyu@apache.org>
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions! |
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a52b18f82
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: liuhy <liuhongyu@apache.org>
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions! |
Signed-off-by: liuhy <liuhongyu@apache.org>
What this PR does / why we need it:
In
GatewayNamespacemode, when a pre-existing, unmanaged Deployment or DaemonSet shares the Gateway's name (e.g. an unrelatednginxDeployment, as in #9132), the reconcile surfaced a misleading error:This is really a same-name resource conflict, not a custom-label migration. It happened because
createOrUpdateDeployment/createOrUpdateDaemonSetran the selector-migration workaround before the ownership check: an unmanaged same-name resource with a different selector hit the selector-migration error and never reached the ownership guard that #9215 added for SA/ConfigMap/Service/PDB/HPA.This PR moves
checkOwnershipto run right after the existingGetsucceeds and before the selector-migration block, in both the Deployment and DaemonSet paths. Now an unmanaged same-name collision returns a clear error (Deployment ns1/gateway-1 already exists and is not owned by this Gateway) and the pre-existing resource is left untouched.Non-
GatewayNamespacemode is unaffected —checkOwnershipreturns nil immediately there — so the legitimate custom-label migration error path (and its existing tests) is preserved.Which issue(s) this PR fixes:
Fixes #9132
PR Checklist
git commit -s). See DCO: Sign your work.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.) — unit tests pass locally (go test ./internal/infrastructure/kubernetes/...);gen-check/lint/coverage to be confirmed by CI.GatewayNamespaceOwnershipe2e test. The two collision tests were verified to FAIL on the unfixed code (they reproduce the exact misleading error) and PASS with the fix; all existing selector-migration tests still pass.release-notes/current/bug_fixes/9132-gateway-namespace-unmanaged-resource-conflict.md.🤖 Generated with Claude Code