Repository navigation
controller: honor apiKey.secretRef.namespace on MCPRoute with a ReferenceGrant - #2907
Open
abdulaziz-dev-lab wants to merge 19 commits into
Open
abdulaziz-dev-lab wants to merge 19 commits into
abdulaziz-dev-lab wants to merge 19 commits into
Conversation
…enceGrant MCPRoute accepts a namespace on securityPolicy.apiKey.secretRef (it is a Gateway API SecretObjectReference, and the CRD even carries the ReferenceGrant wording), but the controller never read it: readAPIKey always fetched the Secret from the route's own namespace. A reference to shared/token therefore injected the value of a same-named Secret in the route namespace when one existed, with the route marked Accepted, or failed with "secrets ... not found" in the wrong namespace when it did not. The Secret index already keyed routes by the referenced namespace, so Secret updates woke the route while the credential came from elsewhere. Resolve the Secret namespace from secretRef.namespace, defaulting to the route namespace, and require a ReferenceGrant in the Secret's namespace that allows MCPRoute from the route's namespace before reading a Secret elsewhere, the same rule BackendSecurityPolicy applies since theagentrouter#2783. The grant check runs before the Secret is read, so an ungranted reference never reaches the API server. Without a grant the MCPRoute is NotAccepted with the usual "is not permitted" message. The ReferenceGrant controller now also enqueues MCPRoutes that reference a Secret in the grant's namespace, so creating or revoking a grant takes effect without an unrelated reconcile. Document the grant on the API field (both versions) and in the MCP capabilities page. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
…ferenceGrant is revoked When the ReferenceGrant that allowed an MCPRoute backend's cross-namespace apiKey.secretRef was deleted or narrowed, the reconcile returned at the grant check and left the backend's generated HTTPRoute, HTTPRouteFilter and credential Secret from the last granted reconcile in place. The data plane kept injecting the copied credential (on the query-parameter path the key sits in the HTTPRoute's URL rewrite); only the status flipped. Check every cross-namespace secretRef against its ReferenceGrant before the per-backend loop. A denied backend's three generated objects are deleted so it converges to the never-granted state, the rest of the route (other backends, main HTTPRoute, security policy, gateways) is still synced, and the route is NotAccepted with the "is not permitted" message, one line per denied backend. The check runs before the loop so a persistent error on an earlier backend cannot keep a later backend's revoked credential alive; the deletes are idempotent, so the re-enqueue from the owned HTTPRoute's delete event and a controller restart are no-ops. Every other error keeps the previous behaviour: the objects of the last successful reconcile stay until the next one. Covered by unit tests at the Reconcile level (header and query-parameter paths, mixed routes, non-grant errors, restart), an envtest case that wires the MCPRoute and ReferenceGrant controllers through a manager, and an e2e test on kind. The docs paragraph on Secrets in another namespace describes the behaviour. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
✅ Deploy Preview for theagentrouter ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
abdulaziz-dev-lab
marked this pull request as draft
October 21, 2026 22:05
… lookup Drop the unit cases that repeat another test without adding coverage: TestMCPRouteController_Reconcile_SecretRefNamespaceNotPermitted (the first two steps of TestMCPRouteController_Reconcile_SecretRefGrantRevoked assert the same), the two rule-level cases for a grant with another route namespace or Secret name (covered by TestReferenceGrantValidator_ValidateMCPRouteSecretReference and the narrowed-grant step), the narrowing by from.namespace (the same path as the narrowing by to.name), the finalizer-held grant step (TestReferenceGrantValidator_TerminatingGrant and the envtest case), the same-namespace Secret deletion (asserted by TestMCPRouteController_Reconcile_SecretRefGrantRevokedAfterFailingBackend) and the helper-level deletePerBackendResources test (idempotence and the user's Secrets are asserted at the Reconcile level). Add the one error path of revokeDeniedSecretRefs that had no test: a failed ReferenceGrant lookup keeps the backend's objects, marks the route NotAccepted with that error and recovers on the next reconcile. In the e2e test, match the MCP proxy's 500 by its status text alone. The MCP Go SDK reports a rejected initialize with http.StatusText, so the error never carries the digits "500"; the only place they could appear is the port of the forwarded address in a transport error, so a connection error on a port such as 35001 could have satisfied the old check. Reuse toolNames for the tool list while here. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
…etRef The "Secrets in another namespace" subsection showed the ReferenceGrant but never the field that makes it necessary: no snippet on the page sets secretRef.namespace. Add the backend fragment next to the grant and say that the rule is about a backend's apiKey.secretRef. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
Contributor
Author
|
/retest |
1 similar comment
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
abdulaziz-dev-lab
marked this pull request as ready for review
October 22, 2026 20:09
abdulaziz-dev-lab
marked this pull request as draft
October 26, 2026 17:31
Contributor
Author
|
@nacx heads up this is not ready for merge yet after testing, apologies for premature review request, I changed it back to draft. |
Per-backend object names derive from "<route>-<backend>", so route "github-mcp" with backend "prod" and route "github" with backend "mcp-prod" share the same HTTPRoute, HTTPRouteFilter and credential Secret names. revokeDeniedSecretRefs deleted those objects by name, so a denied backend removed another MCPRoute's live objects on every retry and that route recreated them. deletePerBackendResources now reads each object first, keeps it when another object controls it and deletes it with a UID precondition. Objects without a controller reference are still deleted, so leftovers of a controller that stopped mid-revocation are cleaned up as before. The orphan cleanup shares the helper and gains the same guard for the filter and the credential Secret, which it also deleted by name. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
…p the list revokeDeniedSecretRefs deletes a denied backend's generated objects before the per-backend loop, but syncMCPRoute returned the denial only when every later step succeeded. Any later error replaced it, so the status never said why the denied backend's objects were gone. syncMCPRoute now joins the denials with the error of the remaining sync, and revokeDeniedSecretRefs returns one value instead of two adjacent errors. The joined denials also grow with the number of denied backends: about 290 bytes each with short names and up to about 1 KiB with maximum-length names, while the CRD caps a condition message at 32768 bytes and a route has up to 256 backends. Past that size the status update was rejected and the route kept reporting Accepted after a revocation had removed every backend. The status now lists at most ten denials and counts the rest. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
NewReferenceGrantController takes four positional channels and the constructor test checked three of them. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
Say where the referenced value ends up, what deleting the Secret does, and that a controller started with --watchNamespaces must watch the Secret's namespace to see the ReferenceGrant. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
…rols Per-backend object names derive from "<route>-<backend>" and collide across MCPRoutes. The deletes were guarded, but the write paths were not: ensureCredentialSecret updated the other route's credential Secret in place, and ensureMCPBackendRefHTTPFilter deleted its credential Secret as "stale" and rewrote its filter spec, before the colliding HTTPRoute create failed. The other route then injected the wrong key, or none, while reporting Accepted, and after a revocation the ownership guard kept the overwritten value. Both writers now return an error that names the collision when the existing object is controlled by another MCPRoute, so the colliding backend fails closed and the first route's objects and credential survive every retry. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
…ep fails later revokeDeniedSecretRefs returned nothing but the error when a later backend's grant lookup or cleanup failed, so the denials of earlier backends, whose HTTPRoute, HTTPRouteFilter and credential Secret the same sweep had just deleted, were missing from the status for as long as the later error persisted. The partial denials are now returned with the error and joined into it. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
joinDenials summarised as soon as the denials exceeded the limit, so a route with eleven denied backends listed ten and ended with "1 more backends have a Secret reference that is not permitted", hiding one name to save nothing. The sweep now records one denial more than the limit and only counts the rest, so the count line stands for at least two backends and the extra wrapping of denials that would never be listed is gone. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
… one place The owned-HTTPRoute index and the per-backend cleanup each compared a controller reference's kind and name with their own literal, and neither looked at the API group, so a same-named MCPRoute of another group would have been treated as this route. controllingMCPRouteName now holds the identity for both. The cleanup derives the kind label from the object instead of taking it as a parameter, shares the "gone or replaced" classification of delete errors, and the tests cover another MCPRoute, a same-named MCPRoute of another group, another kind, and the UID precondition that keeps a replaced object. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
The CRD requires exactly one of apiKey.inline and apiKey.secretRef but does not require inline to be non-empty. readAPIKey treats an empty inline value as "read the Secret" and dereferenced the nil secretRef on the injectionPolicy IfNotPresent and queryParam paths; the reconcile panicked, was recovered by controller-runtime and requeued forever without a status. Return an error instead, so the route is NotAccepted with the reason. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
…h as an error validateReference listed ReferenceGrants from the manager's cache with the target-kind index only. On a controller started with --watchNamespaces, a grant in a namespace outside that list is invisible, the list came back empty, and the reference was denied as if no grant existed. For an MCPRoute that denial now deletes the backend's generated HTTPRoute, HTTPRouteFilter and credential Secret, so a working route lost its backend on a controller restart with a narrower watch list, with a status that named a grant that does exist. Listing in the target namespace as well makes such a cache return its "unknown namespace" error instead of an empty list. The reference is then reported as a lookup error, nothing is deleted, and the next reconcile retries. Same-namespace references are unaffected, and so are caches that watch every namespace. Covered by an envtest case on a namespace-scoped cache. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
…ection path The query-parameter path writes the value into the generated HTTPRoute and creates no managed Secret; the previous sentence listed the Secret for both paths. Also say that the controller does not watch for the referenced Secret's deletion, and that an unwatched grant namespace is reported as an error. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
…ady checks it The helper-test rewrite in the previous commits also removed TestMCPRouteController_Reconcile_DenialReportedWithLaterError, which repeated TestMCPRouteController_Reconcile_SecretRefGrantRevokedAfterFailingBackend with one extra check: that the joined error still unwraps to a ReferenceNotPermittedError. That check moves into the remaining test, which now builds its expected denial text with the shared helper. Signed-off-by: Abdulaziz Alharbi <calamitygaming64@gmail.com>
abdulaziz-dev-lab
marked this pull request as ready for review
October 27, 2026 17:52
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
MCPRouteignoredapiKey.secretRef.namespaceand always read the Secretfrom the route's namespace, silently using a same-named Secret or failing.
This reads the Secret from
secretRef.namespaceand requires aReferenceGrantin that namespace, matchingBackendSecurityPolicy(#2783).Without a grant the route is
NotAcceptedand no Secret is read; with one thevalue is copied into the managed credential Secret (or, for
queryParam, thegenerated HTTPRoute), as for a same-namespace Secret. Revoking a grant deletes
the backend's generated HTTPRoute, HTTPRouteFilter and credential Secret. The
ReferenceGrant controller now also wakes affected MCPRoutes.
Behavior change (upgrade notes)
secretRefnow needs a ReferenceGrant; create grantsbefore upgrading or routes become
NotAccepted.keeps the last copied value.
--watchNamespaces, the Secret's namespace must be watched. Thisapplies to all ReferenceGrant checks (BackendSecurityPolicy,
AIGatewayRoute, QuotaPolicy, Gateway, extension server): an unwatched
namespace is now an error, not a denial, and a QuotaPolicy in that case
reports
ReconciliationFailedinstead ofRefNotPermitted.apiKey.inlinewithIfNotPresent/queryParamnow errors insteadof panicking.
Related: Fixes #2905. Sibling of #2783. Related: #2385.
Notes for reviewers
errors can't keep a revoked credential alive; status lists ≤10 denials.
closed by an ownership check instead of overwriting, and deletes carry a
UID precondition.
NewReferenceGrantControllergains a nil-tolerantmcpRouteChan.(revocation, unwatched namespace), e2e
TestMCPRouteCrossNamespaceSecret.kind before/after