fix: prevent GatewayNamespace mode from hijacking pre-existing ServiceAccounts and ConfigMaps - #9215
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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: 4486073b16
ℹ️ 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 #9215 +/- ##
==========================================
+ Coverage 75.34% 75.38% +0.04%
==========================================
Files 252 252
Lines 41434 41471 +37
==========================================
+ Hits 31218 31263 +45
+ Misses 8104 8098 -6
+ Partials 2112 2110 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Can you check the failed e2e test? |
Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
…els must match Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
…pre-existing and user-managed. we must not block the apply or add an ownerReference to it. Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
18d0712 to
8a319e5
Compare
|
hi @zirain , good morning. I have fixed the failed test cases. Could you please approve those workflows for verification? |
|
can you add e2e and release notes for this? |
sure. I will add e2e test cases and release notes. |
Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
Signed-off-by: Hai <20416005+yuehaii@users.noreply.github.com>
Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
|
those e2e test failure have no relation with current fix for cm/sa ownership. I will retest the failure cases.
|
|
hi @zirain , good day. I have added the e2e test case and release note. please help review the pr when it is convenient. thanks. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec3265bdf1
ℹ️ 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".
| err := i.Client.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, existing) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| return nil |
There was a problem hiding this comment.
This ownership guard is using the infrastructure client's normal Get, and the Kubernetes provider wires that client from mgr.GetClient() (internal/provider/kubernetes/kubernetes.go:402-405), so reads for cached types such as ServiceAccounts and ConfigMaps can be stale. If a tenant creates the colliding ServiceAccount/ConfigMap shortly before creating the Gateway, for example in the same apply, the Gateway reconcile can hit this IsNotFound path before the cache observes the pre-existing object and still proceed to server-side apply with force ownership, reintroducing the hijack this change is meant to prevent. Use an uncached/APIReader-style live read for this conflict check before applying.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think this might be acceptable as this edge case is very rare and unlikely happens in production.
a tenant creates the colliding ServiceAccount/ConfigMap shortly before creating the Gateway, for example in the same apply, the Gateway reconcile can hit this IsNotFound path before the cache observes the pre-existing object and still proceed to server-side apply with force ownership
the name collision is possible for those resources. there might be rare cases having a pre-existing same name for those resources in the same namespace. please let me know if you want me to implement similar fix for Deployment, DaemonSet, PDB, HPA, and Service. |
I’d prefer to handle all resources the same way for consistency and maintainability. |
|
Seems like we got these resource names as gateway names in #6135. |
the name collisions should be carefully considered previously: "since its more intuitive for end users as well as safe to do (no collisions)". |
Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
…eaking Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72217c06a2
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…heck before applying" This reverts commit 999a349. Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
|
hi @zhaohuabing , good day. I have solved the issues. could you please review the changes when it is convenient. thanks. |
Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
Thanks for the explanation. Could we make this clearer by using a method like isRatelimit() and adding some comment to explain this behavior for better maintenance? |
Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com>
| // noGatewayIdentityLabels reports whether labels carry none of the three | ||
| // gateway ownership identity labels. Resources in GatewayNamespace mode | ||
| // with no identity labels (such as ratelimit resources) are controller-scoped | ||
| // and need no collision guard. |
There was a problem hiding this comment.
I think we need collision guard for all EG created resources - but it's not blocking for this PR.
There was a problem hiding this comment.
if needed, I can replace
if !i.EnvoyGateway.GatewayNamespaceMode() || noGatewayIdentityLabels(obj.GetLabels()) {
with
if !i.EnvoyGateway.GatewayNamespaceMode() || obj.GetNamespace() == i.ControllerNamespace
|
/retest |
| # custom sa for gateway namespace mode | ||
| apiVersion: v1 | ||
| automountServiceAccountToken: false | ||
| kind: ServiceAccount | ||
| metadata: | ||
| name: custom-sa | ||
| namespace: gateway-conformance-infra | ||
| labels: | ||
| app.kubernetes.io/managed-by: envoy-gateway | ||
| gateway.envoyproxy.io/owning-gateway-namespace: gateway-conformance-infra | ||
| gateway.envoyproxy.io/owning-gateway-name: eg-deployment |
There was a problem hiding this comment.
I cannot recall why this's put here, but it should be fine to remove this directly.
There was a problem hiding this comment.
it is added to fix the "TestE2E/EnvoyProxyCustomName" and "TestE2E/EnvoyProxyCustomName/Deployment" test cases failure. please refer this thread.
There was a problem hiding this comment.
I think we could remove the SA entirely, let's give a try later.
…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>
…eAccounts and ConfigMaps (envoyproxy#9215) * check ownership before apply Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * both the generic managed-by label and the owning-gateway identity labels must match Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * if supplies a custom SA name via EnvoyServiceAccount.Name, the SA is pre-existing and user-managed. we must not block the apply or add an ownerReference to it. Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * add test cases Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * add e2e test case and release note. Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * fix lint format warning Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * use an uncached/APIReader-style live read for this conflict check before applying Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * add same fix and test cases for Deployment, DaemonSet, optional PDB, optional HPA, and Service Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * add missing release note Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * sync the release notes with main Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * restore previous added content in current release Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * rename the release file name to lower cases Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * solve the issues detected by codex Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * adjust the test cases and remove the gate. fix lint format warning. Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * replace with GatewayNamespaceMode gate Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * fix previous dirty context caused HTTPRouteMixedBackends test case breaking Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * Revert "use an uncached/APIReader-style live read for this conflict check before applying" This reverts commit 999a349. Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * review and adjust code suggested by copilot and codex. Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * refactor code to make it more readable. Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> * refactor code to be more readable. Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> --------- Signed-off-by: hai.yue <20416005+yuehaii@users.noreply.github.com> Signed-off-by: Hai <20416005+yuehaii@users.noreply.github.com> Co-authored-by: zirain <zirain2009@gmail.com> Signed-off-by: Andrea Abellonio <andrea.abellonio@servescale.ai>
…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>
What type of PR is this?
fix: prevent GatewayNamespace mode from hijacking pre-existing ServiceAccounts and ConfigMaps
What this PR does / why we need it:
In GatewayNamespace mode, when a Gateway is created, Envoy Gateway provisions a ServiceAccount and ConfigMap in the same namespace as the Gateway, using the Gateway's own name as the resource name.
If a resource with that name already existed in the namespace, the reconciler would silently overwrite it via ServerSideApply with client.ForceOwnership, stealing field ownership and injecting a new ownerReference pointing to the Gateway object. When the Gateway was later deleted, Kubernetes garbage collection followed the ownerReference and deleted the pre-existing resource, causing unrelated components to break.
Fixes #9136
Release Notes: Yes