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 <[email protected]>
…els must match Signed-off-by: hai.yue <[email protected]>
…pre-existing and user-managed. we must not block the apply or add an ownerReference to it. Signed-off-by: hai.yue <[email protected]>
Signed-off-by: hai.yue <[email protected]>
yuehaii
force-pushed
the
GatewayNamespace-mode
branch
from
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 <[email protected]>
Signed-off-by: Hai <[email protected]>
Signed-off-by: hai.yue <[email protected]>
|
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 <[email protected]>
Signed-off-by: hai.yue <[email protected]>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6237ba9ee5
ℹ️ 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".
Signed-off-by: hai.yue <[email protected]>
…eaking Signed-off-by: hai.yue <[email protected]>
|
@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 <[email protected]>
Signed-off-by: hai.yue <[email protected]>
|
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 <[email protected]>
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 <[email protected]>
| // 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.
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