feat(translator): Security policies for TLS gateways#9464
Conversation
Signed-off-by: asif.hasnain@sap.com <asif.hasnain@sap.com>
✅ 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: 0677eca60e
ℹ️ 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".
| refGrant("sp-tls-route", routeNS, resource.KindSecurityPolicy, policyNS, gwapiv1.GroupName, resource.KindTLSRoute), | ||
| }, | ||
| expectedNames: sets.New("sp-gateway", "sp-http-route", "sp-grpc-route", "sp-tcp-route"), | ||
| expectedNames: sets.New("sp-gateway", "sp-http-route", "sp-grpc-route", "sp-tcp-route", "sp-tls-route"), |
There was a problem hiding this comment.
This expectation cannot be satisfied by the current production allow-list: processPolicyTargetReferenceGrants still permits SecurityPolicy target grants only for Gateway/HTTPRoute/GRPCRoute/TCPRoute (internal/provider/kubernetes/controller.go:1681-1686). When a cross-namespace SecurityPolicy uses targetSelectors to select TLSRoutes, the TLSRoute ReferenceGrant is filtered out before translation, so the selector reference is treated as unpermitted and the policy never attaches even though the CRD now allows TLSRoute targets.
Useful? React with 👍 / 👎.
| irKey := t.getIRKey(gtwCtx.Gateway) | ||
| switch route.GetRouteType() { | ||
| case resource.KindTCPRoute: | ||
| case resource.KindTCPRoute, resource.KindTLSRoute: |
There was a problem hiding this comment.
Adding TLSRoute to this branch makes targetRefs.sectionName policies reach the TCP/TLS route path, but TLSRoute IR is currently built with Destination.Metadata: buildResourceMetadata(tlsRoute, nil) (internal/gatewayapi/route.go:1482), so r.Destination.Metadata.SectionName is always empty. A SecurityPolicy targeting a named TLSRoute rule passes HasRuleNames validation, then this path skips every route and leaves the policy accepted but with no authorization applied.
Useful? React with 👍 / 👎.
…-tls-gateways Signed-off-by: asif.hasnain@sap.com <asif.hasnain@sap.com>
There was a problem hiding this comment.
Pull request overview
This PR extends SecurityPolicy client-IP CIDR authorization support to TLSRoute (TLS passthrough), updating API validation/CRDs, translator logic, and adding conformance coverage.
Changes:
- Allow
SecurityPolicy.spec.targetRef(s).kind: TLSRoutevia API/CEL validations and regenerated CRDs/helm outputs. - Apply TCP-style (CIDR-only) authorization validation/translation to TLSRoute in
internal/gatewayapi/securitypolicy.go, with new xDS-IR golden testdata. - Add an e2e conformance test and manifests covering allow/deny-by-client-IP on TLSRoute passthrough.
Required fixes (highest severity first):
- Update CEL validation tests for the API error-message change:
api/v1alpha1/securitypolicy_types.go:49-52now includes/TLSRoutein validation messages, buttest/cel-validation/securitypolicy_test.gostill asserts the old message text (will break CEL validation test expectations). - E2E assertion correctness:
test/e2e/tests/tlsroute_authorization_client_ip.go:147-151currently treats an empty response as success for the “allowed” path, which can mask upstream connectivity failures. - Error-message consistency:
internal/gatewayapi/securitypolicy.go’s TCP/TLS validator should keep error strings consistently saying “TCP/TLS” (not “TCP”).
Release note risk:
- This is a user-facing feature addition; a release-note fragment under
release-notes/current/appears to be missing.
Reviewed changes
Copilot reviewed 12 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/helm/gateway-crds-helm/envoy-gateway-crds.out.yaml | Regenerated CRD output reflecting TLSRoute as an allowed SecurityPolicy target kind. |
| test/helm/gateway-crds-helm/e2e.out.yaml | Regenerated CRD output for e2e chart bundle reflecting TLSRoute validation changes. |
| test/helm/gateway-crds-helm/all.out.yaml | Regenerated aggregate CRD output reflecting TLSRoute validation changes. |
| test/e2e/tests/tlsroute_authorization_client_ip.go | Adds TLSRoute passthrough e2e conformance test for client-IP allow/deny authorization. |
| test/e2e/testdata/tlsroute-authorization-client-ip.yaml | Adds Gateway/TLSRoute/SecurityPolicy manifests used by the new e2e test. |
| site/content/en/latest/api/extension_types.md | Updates API docs to state SecurityPolicy can target TLSRoute and clarifies CIDR-only behavior. |
| internal/xds/translator/testdata/out/xds-ir/tls-route-authorization.routes.yaml | New xDS-IR golden (routes) for TLSRoute authorization scenario. |
| internal/xds/translator/testdata/out/xds-ir/tls-route-authorization.listeners.yaml | New xDS-IR golden (listeners) verifying RBAC + tcp_proxy wiring for SNI-based chains. |
| internal/xds/translator/testdata/out/xds-ir/tls-route-authorization.endpoints.yaml | New xDS-IR golden (endpoints) for the TLSRoute authorization scenario. |
| internal/xds/translator/testdata/out/xds-ir/tls-route-authorization.clusters.yaml | New xDS-IR golden (clusters) for the TLSRoute authorization scenario. |
| internal/xds/translator/testdata/in/xds-ir/tls-route-authorization.yaml | New xDS-IR input fixture representing TLSRoute routes with authorization rules. |
| internal/provider/kubernetes/controller_test.go | Updates controller test expectations to include SecurityPolicy reference grants for TLSRoute. |
| internal/gatewayapi/testdata/securitypolicy-with-merge-tcp-invalid.out.yaml | Updates expected status message text to “TCP/TLS” wording. |
| internal/gatewayapi/securitypolicy.go | Extends SecurityPolicy route processing/validation/translation to handle TLSRoute like TCPRoute (CIDR-only). |
| internal/gatewayapi/securitypolicy_test.go | Switches unit test to call the renamed TCP/TLS validator. |
| charts/gateway-helm/charts/crds/crds/generated/gateway.envoyproxy.io_securitypolicies.yaml | Regenerated CRD manifest enabling TLSRoute in SecurityPolicy target kind validation. |
| charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_securitypolicies.yaml | Regenerated CRD template enabling TLSRoute in SecurityPolicy target kind validation. |
| api/v1alpha1/securitypolicy_types.go | API docs + CEL validations updated to allow TLSRoute targets and mergeType use with TLSRoute. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return fmt.Errorf("rule %d: JWT not supported for TCP/TLS", i) | ||
| } | ||
| if len(rule.Principal.Headers) > 0 { | ||
| return fmt.Errorf("rule %d: headers not supported for TCP", i) | ||
| return fmt.Errorf("rule %d: headers not supported for TCP/TLS", i) |
| // +kubebuilder:validation:XValidation:rule="has(self.targetRef) ? self.targetRef.kind in ['Gateway', 'ListenerSet', 'HTTPRoute', 'GRPCRoute', 'TCPRoute', 'TLSRoute'] : true", message="this policy can only have a targetRef.kind of Gateway/ListenerSet/HTTPRoute/GRPCRoute/TCPRoute/TLSRoute" | ||
| // +kubebuilder:validation:XValidation:rule="has(self.targetRefs) ? self.targetRefs.all(ref, ref.group == 'gateway.networking.k8s.io') : true ", message="this policy can only have a targetRefs[*].group of gateway.networking.k8s.io" | ||
| // +kubebuilder:validation:XValidation:rule="has(self.targetRefs) ? self.targetRefs.all(ref, ref.kind in ['Gateway', 'ListenerSet', 'HTTPRoute', 'GRPCRoute', 'TCPRoute']) : true ", message="this policy can only have a targetRefs[*].kind of Gateway/ListenerSet/HTTPRoute/GRPCRoute/TCPRoute" | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.mergeType) || ((!has(self.targetRef) || self.targetRef.kind in ['HTTPRoute', 'GRPCRoute', 'TCPRoute']) && (!has(self.targetRefs) || self.targetRefs.all(ref, ref.kind in ['HTTPRoute', 'GRPCRoute', 'TCPRoute'])) && (!has(self.targetSelectors) || self.targetSelectors.all(sel, sel.kind in ['HTTPRoute', 'GRPCRoute', 'TCPRoute'])))", message="mergeType can only be used with xRoute targets" | ||
| // +kubebuilder:validation:XValidation:rule="has(self.targetRefs) ? self.targetRefs.all(ref, ref.kind in ['Gateway', 'ListenerSet', 'HTTPRoute', 'GRPCRoute', 'TCPRoute', 'TLSRoute']) : true ", message="this policy can only have a targetRefs[*].kind of Gateway/ListenerSet/HTTPRoute/GRPCRoute/TCPRoute/TLSRoute" | ||
| // +kubebuilder:validation:XValidation:rule="!has(self.mergeType) || ((!has(self.targetRef) || self.targetRef.kind in ['HTTPRoute', 'GRPCRoute', 'TCPRoute', 'TLSRoute']) && (!has(self.targetRefs) || self.targetRefs.all(ref, ref.kind in ['HTTPRoute', 'GRPCRoute', 'TCPRoute', 'TLSRoute'])) && (!has(self.targetSelectors) || self.targetSelectors.all(sel, sel.kind in ['HTTPRoute', 'GRPCRoute', 'TCPRoute', 'TLSRoute'])))", message="mergeType can only be used with xRoute targets" |
| if n > 0 { | ||
| t.Logf("Successfully received response: %s", string(buf[:n])) | ||
| } else { | ||
| t.Log("Connection allowed and completed successfully") | ||
| } |
guydc
left a comment
guydc
left a comment
There was a problem hiding this comment.
Thanks @asHasnain for picking this up, it's a useful enhancement of security policy for additional TCP/TLS Scenerios.
Can we add some yaml tests in the GW-API layer covering translation of more complex scenarios like multiple rules in a single TLS route and multiple TLS routes in a listener?
TCP routes usually have a simpler xds shape than TLS routes, so I just want to make sure that reusing the existing translation of security policy for TCP covers all cases.
| // | ||
| // NOTE: SecurityPolicy can target Gateway, ListenerSet, HTTPRoute, GRPCRoute, and TCPRoute. | ||
| // When a SecurityPolicy targets a TCPRoute, only client-IP CIDR based authorization | ||
| // NOTE: SecurityPolicy can target Gateway, ListenerSet, HTTPRoute, GRPCRoute, and TCPRoute, and TLSRoute. |
There was a problem hiding this comment.
Can you please add a release note as well for this new feautre?
| }, | ||
| } | ||
|
|
||
| func testTLSRouteConnectionBlocked(t *testing.T, gwAddr, hostname string) { |
There was a problem hiding this comment.
Please enhance the test to also confirm that the RBAC filter actually blocked/allowed the traffic, using the RBAC stats and prometheus test utils: https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/rbac_filter#statistics
There was a problem hiding this comment.
A function is added to verify it via RBAC metrics envoy_rbac_denied and envoy_rbac_allowed.
Signed-off-by: asif.hasnain@sap.com <asif.hasnain@sap.com>
asHasnain
force-pushed
the
security-policy-for-tls-gateways
branch
from
94cabf1 to
0c9e464
Compare
| }, | ||
| } | ||
|
|
||
| func testTLSRouteConnectionBlocked(t *testing.T, gwAddr, hostname string) { |
There was a problem hiding this comment.
A function is added to verify it via RBAC metrics envoy_rbac_denied and envoy_rbac_allowed.
| // | ||
| // NOTE: SecurityPolicy can target Gateway, ListenerSet, HTTPRoute, GRPCRoute, and TCPRoute. | ||
| // When a SecurityPolicy targets a TCPRoute, only client-IP CIDR based authorization | ||
| // NOTE: SecurityPolicy can target Gateway, ListenerSet, HTTPRoute, GRPCRoute, and TCPRoute, and TLSRoute. |
What this PR does / why we need it:
Adds SecurityPolicy support for TLSRoute with client IP-based authorization. Currently, SecurityPolicies support Gateway, HTTPRoute, GRPCRoute, and TCPRoute, but not TLSRoute. This PR extends authorization support to TLSRoute (passthrough mode), allowing users to apply client IP allow/deny lists using SecurityPolicy.
Which issue(s) this PR fixes:
Fixes #6704
PR Checklist
git commit -s). See DCO: Sign your work./api), the API was discussed and agreed before the implementation. The API change can be in a separate PR, or in the same PR, but the API must be agreed before implementation. N/A if this PR does not contain API changes.make generate gen-check,make lint, and the unit-test/coverage build pass. (Flaky e2e failures are not considered breakages, butgen-check,lint, and coverage MUST pass.)release-notes/current/<section>/<pr-number>-<slug>.md(seerelease-notes/current/README.mdfor sections and naming). N/A if this PR does not contain non-trivial changes.make gen-checkand committed the result if API/helm charts/modules changed.release-notes/current/breaking_changes/.