Repository navigation
Conversation
…ials bspToFilterAPIBackendAuth read the Secret for APIKey, AzureAPIKey, AnthropicAPIKey and AWS credentialsFile policies from the policy's own namespace. It ignored secretRef.namespace and never checked a ReferenceGrant, while the Secret watch index resolves the referenced namespace with backendSecurityPolicySecretRef. A cross-namespace reference failed with "not found", or silently used a same-named Secret from the policy's namespace. Resolve the Secret with backendSecurityPolicySecretRef and check validateSecretReference before reading, as the rotation paths have done since theagentrouter#2746. Without a grant the read fails and the backend is skipped. getSecretData errors now include the namespace, and the connect-providers docs no longer pin secretRef examples to the default namespace. Fixes theagentrouter#2778 Signed-off-by: Abdulaziz Alharbi <246710009+abdulaziz-dev-lab@users.noreply.github.com>
✅ Deploy Preview for theagentrouter ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
nacx
left a comment
There was a problem hiding this comment.
should static types also set NotAccepted on the policy status, or keep that for a separate PR?
Yes, please. Make the change aprt of this PR as well.
| func (c *GatewayController) getBSPSecretRefData(ctx context.Context, bsp *aigv1b1.BackendSecurityPolicy, dataKey string) (string, error) { | ||
| name, namespace, ok := backendSecurityPolicySecretRef(bsp) | ||
| if !ok { | ||
| return "", fmt.Errorf("secretRef is not set for policy %s", bsp.Name) |
There was a problem hiding this comment.
Worth namespacing this message like the two you just changed above. With policies of the same name in several namespaces, 'secretRef is not set for policy bsp' does not identify the object.
| return "", fmt.Errorf("secretRef is not set for policy %s", bsp.Name) | |
| return "", fmt.Errorf("secretRef is not set for policy %s/%s", bsp.Namespace, bsp.Name) |
There was a problem hiding this comment.
Done in b02a549, along with the test that checks this message.
| if !ok { | ||
| return "", fmt.Errorf("secretRef is not set for policy %s", bsp.Name) | ||
| } | ||
| if err := c.referenceGrantValidator.validateSecretReference(ctx, bsp.Namespace, namespace, name); err != nil { |
There was a problem hiding this comment.
The standalone aigw does not support ReferenceGrants, and this change makes it more evident. In standalone mode, this method will use the fake client, that has no ReferenceGrants populated, and cross-secrets in the aigw config will silently drop.
I know this is a gap that was already there, but it will become more evident now. Worth fixing in this PR?
It should be a small shange. Something like adding somethign like case "ReferenceGrant": mustExtractAndAppend(obj, &referenceGrants) to collectObjects and create the collected grants in the fake client in translateCustomResourceObjects, alongside the user-defined Secrets.
There was a problem hiding this comment.
Done in d6cc9fa. collectObjects now returns ReferenceGrants and still writes them to the Envoy Gateway resources, since EG needs them for HTTPRoute→Backend refs. translateCustomResourceObjects creates them in the controller-runtime fake client before anything is reconciled, because that is where the validator lists grants from (the user Secrets are in the client-go fake clientset). TestRunCmdContext_writeEnvoyResourcesAndRunExtProc_crossNamespaceSecret covers it with and without the grant.
With the NotAccepted change (5898814), an ungranted cross-namespace secretRef in an aigw config now makes aigw panic at startup with the "not permitted" error, like any other reconcile error there, instead of silently dropping the backend. Let me know if you'd rather it only log in standalone mode.
Not changed: collectObjects dedups on kind/name, so two objects with the same name in different namespaces (e.g. two grants named allow-bsp) collapse into one. I can fix that here or in a follow-up.
| apiKey: | ||
| secretRef: | ||
| name: openai-secret | ||
| namespace: default |
There was a problem hiding this comment.
Dropping namespace: default here is right, but the same pattern is still in the repo's own manifests: examples/basic/openai.yaml:52, anthropic.yaml:52, cohere.yaml:53, tars.yaml:54, typesafe.yaml:53 and tests/e2e/testdata/testupstream.yaml:152. They work today only because those policies also live in default; applied into any other namespace they become grant-requiring cross-namespace references under the new rule. Since this PR is behaviour-changing for exactly that shape, it would be worth sweeping them in the same change, and adding a sentence here saying that a secretRef.namespace different from the policy's namespace now requires a ReferenceGrant in the Secret's namespace.
There was a problem hiding this comment.
Done in 0c3ef3a. Besides the six files you listed, I removed the same pin from examples/basic/bitdeer.yaml, examples/basic/azure_openai.yaml (clientSecretRef) and the two aigw translate test inputs. I kept the OIDC clientSecret.namespace pins (the GCP Workload Identity Federation example on this page and the crdcel gcp_oidc fixture), because the OIDC token provider errors when that field is unset. versioned_docs are untouched. The new "Secrets in another namespace" section under "Authentication Types" explains the ReferenceGrant, with the same example as the v1.2 upgrade guidance.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Policies with the same name can exist in several namespaces, so the name alone does not identify the object. Also cover the error for a Secret that lacks the expected key. Signed-off-by: Abdulaziz Alharbi <246710009+abdulaziz-dev-lab@users.noreply.github.com>
The controllers check cross-namespace references against ReferenceGrants from the controller-runtime client, but standalone aigw never created any there. A BackendSecurityPolicy whose Secret is in another namespace was therefore left out of the filter config even when the config file contained a matching grant. collectObjects now collects ReferenceGrants, and still writes them to the Envoy Gateway resources, which need them for their own cross-namespace references. translateCustomResourceObjects creates them in the fake client before reconciling anything. Signed-off-by: Abdulaziz Alharbi <246710009+abdulaziz-dev-lab@users.noreply.github.com>
…s grant is missing The Gateway controller already leaves out a backend whose APIKey, AzureAPIKey, AnthropicAPIKey or AWS credentialsFile Secret is in another namespace without a ReferenceGrant, but the policy still reported Accepted. The BackendSecurityPolicy controller now makes the same check, so the policy is marked NotAccepted with the "not permitted" error, as the rotation-based types are. The targeted backends are still synced before the error is returned, like AIGatewayRoute does for a rejected backendRef. A policy changed to point at a Secret it isn't granted then stops publishing the credential it had before. In standalone aigw the error stops startup, as other reconcile errors do, instead of the backend being dropped silently. Signed-off-by: Abdulaziz Alharbi <246710009+abdulaziz-dev-lab@users.noreply.github.com>
The examples and test manifests set namespace: default on secretRef and clientSecretRef next to policies that are also in default. Applied in any other namespace, those become cross-namespace references that need a ReferenceGrant. Leave the namespace out so the Secret is looked up next to the policy. The OIDC clientSecret pins stay, because the OIDC token provider requires that namespace. Document that a Secret in another namespace needs a ReferenceGrant in the Secret's namespace, with an example. Signed-off-by: Abdulaziz Alharbi <246710009+abdulaziz-dev-lab@users.noreply.github.com>
Thanks for the review @nacx. Changes since your review:
I updated the PR description to match. |
|
/retest |
1 similar comment
|
/retest |
|
/retest |
| kind: BackendSecurityPolicy | ||
| namespace: default | ||
| to: | ||
| - group: "" |
There was a problem hiding this comment.
nit: add the specific secret name (realistically we don't want all secrets from being accessible)
There was a problem hiding this comment.
Good point. Since this PR is already merged, I'll scope it to shared-openai-apikey in a follow-up PR. The example in the new "Secrets in another namespace" section of connect-providers.md has the same grant, so I'll name the Secret there too.
|
/retest |
Description
For the static credential types (APIKey, AzureAPIKey, AnthropicAPIKey, and AWSCredentials with
credentialsFile),bspToFilterAPIBackendAuthread the Secret from the BackendSecurityPolicy's own namespace. It ignoredsecretRef.namespaceand never checked a ReferenceGrant. Meanwhile the Secret watch index andReferenceGrantControllerresolve the namespace withbackendSecurityPolicySecretRef. A cross-namespace reference therefore failed with "not found", or silently used a same-named Secret from the policy's namespace.The Gateway controller now resolves the Secret with
backendSecurityPolicySecretReftoo, and callsvalidateSecretReferencebefore reading it, as the rotation paths have done since #2746. Without a matching ReferenceGrant the read fails and the backend is left out of the filter config. The BackendSecurityPolicy controller makes the same check for these types, so the policy is marked NotAccepted with the "not permitted" error. It still syncs the targeted backends before returning that error, as AIGatewayRoute has done for a rejected backendRef since #2750, so a policy changed to point at a Secret it isn't granted stops publishing the credential it had before. Same-namespace references are unchanged and need no grant.Standalone
aigwnow loads ReferenceGrants from its configuration into the fake client the controllers read, and still writes them to the Envoy Gateway resources. A cross-namespacesecretRefworks there when the configuration includes the grant. Without one, aigw panics at startup with the "not permitted" error, as it does for other reconcile errors, instead of silently leaving the backend out.Error messages from
getSecretDataand for an unsetsecretRefnow include the namespace. The examples, the connect-providers docs, and the e2e and aigw test manifests no longer pinsecretRef.namespace: default, so they work in whatever namespace they are applied. The OIDCclientSecret.namespacepins stay, because the OIDC token provider requires that field. The connect-providers docs now explain that a Secret in another namespace needs a ReferenceGrant in the Secret's namespace.Tests:
TestGatewayController_bspToFilterAPIBackendAuth_CrossNamespaceSecretruns the four static types with the namespace unset and set to the policy's own. It also covers cross-namespace refs with no grant, a matching grant, and grants with the wrong from-kind or from-namespace. Every case has a same-named Secret with a different value in the policy's namespace, and the denied cases assert that no Secret was read.TestBackendSecurityPolicyController_Reconcile_StaticCredentialCrossNamespacechecks, for each static type, NotAccepted without a grant and Accepted with one, and that the targeted backend is synced in both cases.TestRunCmdContext_writeEnvoyResourcesAndRunExtProc_crossNamespaceSecretruns an aigw configuration with a cross-namespace API key Secret. With the grant, the filter config carries the key and Envoy Gateway still receives the grant. Without it, aigw fails with the "not permitted" error.Related Issues/PRs (if applicable)
Fixes #2778
Special notes for reviewers (if applicable)
Like #2746, this changes behavior for some existing configs and may be worth a release note. A static policy whose
secretRef.namespacenames another namespace, but whose Secret is in the policy's namespace, only worked because the namespace was ignored. It now needs the Secret in the referenced namespace and a ReferenceGrant there, or thenamespacefield removed.