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".
| 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.
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>
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. |
Signed-off-by: asif.hasnain@sap.com <asif.hasnain@sap.com>
|
cc @zhaohuabing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9464 +/- ##
==========================================
+ Coverage 75.28% 75.44% +0.16%
==========================================
Files 252 252
Lines 41536 41648 +112
==========================================
+ Hits 31269 31422 +153
+ Misses 8143 8094 -49
- Partials 2124 2132 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @asHasnain TestE2E/TLSRouteAuthzWithClientIP/allowed_client_IP_can_connect_to_allowed.example.co is failing. Could you please fix it? |
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/.