fix: report clear ownership conflict for unmanaged same-name Deployment/DaemonSet in GatewayNamespace mode#9470
fix: report clear ownership conflict for unmanaged same-name Deployment/DaemonSet in GatewayNamespace mode#9470Aias00 wants to merge 4 commits into
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.
| Spec: appsv1.DeploymentSpec{ | ||
| Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "pre-existing"}}, | ||
| Template: corev1.PodTemplateSpec{ |
There was a problem hiding this comment.
Good catch — done in d2da3f1: the pre-existing unmanaged Deployment now sets replicas=0 so it stays inert (no scheduling/image-pull). The ownership-collision behavior under test needs no Pods to run.
![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".
| if err := i.checkOwnership(ctx, daemonSet); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
When GatewayNamespace mode is running an EnvoyProxy as a DaemonSet and a same-name unmanaged DaemonSet lacks EG's managed labels, the production manager cache will not contain it because DaemonSets are registered with the managedSelector label filter in internal/provider/kubernetes/kubernetes.go. The earlier i.Client.Get then returns NotFound and this newly added ownership check is never reached, so applyIfOwned proceeds to server-side apply and users still get an API/immutability error or possible adoption instead of the promised ownership-conflict error. The new unit test uses an unfiltered fake client, so it does not cover the production cache behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Excellent catch — this was a real gap. The production cache filters DaemonSets (and HPA/PDB) by the envoy managed-label selector (internal/provider/kubernetes/kubernetes.go), so an unmanaged same-name DaemonSet is absent from the cache and the cached Get returned NotFound before the guard ran. The earlier unit tests used an unfiltered fake client and missed it.
Fixed in d2da3f1: the uncached API reader (mgr.GetAPIReader()) is now wired through KubernetesClientHolder → Provider.GetAPIReader → Infra.apiReader, and checkOwnership uses it for the existence read (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, matching the intent #9215 claimed.
Added TestCheckOwnership_UsesAPIReaderWhenCacheMisses: the cached Get returns NotFound (simulating the filtered cache) while the API reader sees the unmanaged resource; it asserts the ownership error is surfaced. Verified the test FAILs on the pre-API-reader code. Note this also closes the same latent gap for PDB/HPA from #9215.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9470 +/- ##
==========================================
+ Coverage 75.47% 75.48% +0.01%
==========================================
Files 252 252
Lines 41662 41687 +25
==========================================
+ Hits 31444 31468 +24
- Misses 8088 8091 +3
+ Partials 2130 2128 -2 ☔ 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>
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