fix: isHTTPRouteFilterReferencingSecret always returns true#9340
fix: isHTTPRouteFilterReferencingSecret always returns true#9340emilien-puget wants to merge 1 commit into
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: 13a9c25b03
ℹ️ 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".
| } | ||
|
|
||
| return true | ||
| return false |
There was a problem hiding this comment.
When an HTTPRouteFilter credentialInjection valueRef.namespace points at a Secret in another namespace, this new false path skips reconciliation for that Secret because secretRouteFilterIndexFunc indexes HTTPRouteFilters under filter.Namespace instead of ValueRef.Namespace. The generated CRD explicitly allows cross-namespace valueRef.namespace with a ReferenceGrant, and the translator calls validateSecretRef(true, ...), so rotating that cross-namespace credential will no longer enqueue a reconcile and Envoy can keep using stale credentials until some unrelated event occurs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
the modification in internal/provider/kubernetes/indexers.go should take care of that cross namespace stuff, is it enough ?
emilien-puget
force-pushed
the
fix/httproutefilter-secret-predicate
branch
2 times, most recently
from
5759e1a to
b1f2401
Compare
…-namespace index Two bugs in the HTTPRouteFilter secret predicate path: 1. predicates.go: isHTTPRouteFilterReferencingSecret had 'return true' where 'return false' was intended when no HTTPRouteFilter references the given Secret. This caused validateSecretForReconcile to always return true when hrfCRDExists=true, triggering unnecessary reconciliations on every Secret change. 2. indexers.go: secretRouteFilterIndexFunc indexed the Secret reference under filter.Namespace instead of the Secret's actual namespace. When ValueRef.Namespace points to a different namespace (allowed by the CRD with a ReferenceGrant), the index key was wrong and cross-namespace credential rotation would not trigger reconciliation. Fixed by using gatewayapi.NamespaceDerefOr to resolve the correct namespace, matching the pattern used by secretSecurityPolicyIndexFunc and secretEnvoyExtensionPolicyIndexFunc. Added unit tests covering: - Secret not referenced by any HTTPRouteFilter (expect false) - Secret referenced by HTTPRouteFilter CredentialInjection, same namespace (expect true) - Secret in another namespace referenced via ValueRef.Namespace (expect true) Signed-off-by: Emilien Puget <emilien.puget@numspot.com>
emilien-puget
force-pushed
the
fix/httproutefilter-secret-predicate
branch
from
b1f2401 to
38c877f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38c877fb3e
ℹ️ 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".
| secretReferences = append(secretReferences, | ||
| types.NamespacedName{ | ||
| Namespace: filter.Namespace, | ||
| Namespace: gatewayapi.NamespaceDerefOr(filter.Spec.CredentialInjection.Credential.ValueRef.Namespace, filter.Namespace), |
There was a problem hiding this comment.
When CredentialInjection.Credential.ValueRef.Namespace is set, this index now makes the remote Secret pass validateSecretForReconcile, but the reconcile path still fetches the credential from the HTTPRouteFilter namespace in internal/provider/kubernetes/filters.go:112 and only inserts that namespace into the resource tree. As a result, a cross-namespace credential Secret change is enqueued but the translator still cannot see the remote Secret, so the configuration remains stale or invalid despite the new cross-namespace test expectation.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9340 +/- ##
==========================================
+ Coverage 75.17% 75.22% +0.04%
==========================================
Files 252 252
Lines 41049 41049
==========================================
+ Hits 30858 30878 +20
+ Misses 8094 8075 -19
+ Partials 2097 2096 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
The isHTTPRouteFilterReferencingSecret function in predicates.go had 'return true' where 'return false' was intended when no HTTPRouteFilter references the given Secret. This caused validateSecretForReconcile to always return true when hrfCRDExists=true, triggering unnecessary reconciliations on every Secret change.
Added unit tests covering:
Fixes https://github.com/envoyproxy/gateway/issues/9339`