feat(clienttrafficpolicy): support forward_proto_config to infer x-forwarded-proto from PROXY protocol#9469
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR exposes Envoy HCM’s forward_proto_config through ClientTrafficPolicy.spec.proxyProtocol.forwardProtoConfig, enabling Envoy to infer x-forwarded-proto from the PROXY protocol destination port (useful for L4 LB TLS termination scenarios).
Changes:
- Add
ForwardProtoConfigAPI type underProxyProtocolSettingswith CEL + per-item port validations; regenerate CRDs and API docs. - Translate
forwardProtoConfiginto IR and then into Envoy HCMForwardProtoConfig; add unit + golden coverage. - Add an e2e conformance-style test and release note fragment for the new feature.
Findings (ordered by severity)
Required fixes
internal/gatewayapi/clienttrafficpolicy.go:810-818:buildIRForwardProtoConfigcurrently allocates slices unconditionally, which converts “unset” lists into empty (non-nil) slices in IR and can lead to unnecessary allocations / less-stable rendered output. Recommend only allocating when the corresponding input list is non-empty, and defensively returningnilif both lists are empty.
Optional follow-ups
test/e2e/testdata/forward-proto-config.yaml:113: Minor grammar in an inline comment (“on gateway namespace mode” → “in … mode”).
Reviewed changes
Copilot reviewed 12 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
api/v1alpha1/clienttrafficpolicy_types.go |
Adds ForwardProtoConfig API type under ProxyProtocolSettings with validations. |
api/v1alpha1/zz_generated.deepcopy.go |
Updates generated deep-copies for the new API type. |
charts/gateway-helm/charts/crds/crds/generated/gateway.envoyproxy.io_clienttrafficpolicies.yaml |
Regenerated CRD schema for the new field/validations (helm chart copy). |
charts/gateway-crds-helm/templates/generated/gateway.envoyproxy.io_clienttrafficpolicies.yaml |
Regenerated CRD schema for the new field/validations (crds chart copy). |
site/content/en/latest/api/extension_types.md |
Regenerated API docs including ForwardProtoConfig. |
internal/gatewayapi/clienttrafficpolicy.go |
Translates API forwardProtoConfig into IR. |
internal/gatewayapi/testdata/clienttrafficpolicy-proxyprotocol-forwardproto.in.yaml |
New golden input for proxyProtocol + forwardProtoConfig. |
internal/gatewayapi/testdata/clienttrafficpolicy-proxyprotocol-forwardproto.out.yaml |
New golden output verifying IR translation includes forwardProtoConfig. |
internal/ir/xds.go |
Adds IR representation of ForwardProtoConfig. |
internal/ir/zz_generated.deepcopy.go |
Updates generated deep-copies to properly copy the new nested pointer type. |
internal/xds/translator/listener.go |
Sets Envoy HCM ForwardProtoConfig when configured in IR proxyProtocol. |
internal/xds/translator/proxy_protocol_test.go |
Adds unit test verifying HCM forward proto config is populated as expected. |
test/cel-validation/clienttrafficpolicy_test.go |
Adds CEL validation test cases (valid + invalid empty config). |
test/e2e/testdata/forward-proto-config.yaml |
Adds e2e manifests to exercise forward proto inference behavior. |
test/e2e/tests/forward_proto_config.go |
Adds e2e test asserting backend receives X-Forwarded-Proto: https. |
release-notes/current/new_features/9219-forward-proto-config-clienttrafficpolicy.md |
Release note fragment for the new feature. |
Files not reviewed (2)
- api/v1alpha1/zz_generated.deepcopy.go: Generated file
- internal/ir/zz_generated.deepcopy.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 60ed3701a1
ℹ️ 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".
60ed370 to
f68cad8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9469 +/- ##
==========================================
+ Coverage 75.40% 75.43% +0.02%
==========================================
Files 252 252
Lines 41617 41638 +21
==========================================
+ Hits 31381 31408 +27
+ Misses 8103 8097 -6
Partials 2133 2133 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f68cad8236
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
…rwarded-proto from PROXY protocol Add a forwardProtoConfig field to proxyProtocol in ClientTrafficPolicy that exposes Envoy's HttpConnectionManager.forward_proto_config feature. When PROXY protocol is enabled on a listener, this infers the x-forwarded-proto header from the PROXY protocol destination port, so the correct scheme is set for requests forwarded to backends. This is useful when a Layer 4 load balancer (such as AWS NLB) terminates TLS and forwards traffic to Envoy over PROXY protocol, where Envoy would otherwise not know whether the original client request was HTTP or HTTPS. The new field nests httpsDestinationPorts and httpDestinationPorts (each a list of ports in [1, 65535]). At least one of the two lists must be set (enforced via a CEL XValidation). forward_proto_config is an HCM field, so it applies to HTTP listeners only. Closes envoyproxy#9219 Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: liuhy <liuhongyu@apache.org>
…nation ports Add a CEL validation requiring httpsDestinationPorts and httpDestinationPorts to be disjoint, so a port cannot be mapped to both http and https. Add a CEL test covering the ambiguous case. Signed-off-by: liuhy <liuhongyu@apache.org>
The disjoint XValidation added in 1380f1d was regenerated into the source CRD charts (charts/.../generated/gateway.envoyproxy.io_clienttrafficpolicies.yaml) but the test/helm/gateway-crds-helm/*.out.yaml snapshots were not re-rendered, so they still only carried the "at least one port list" rule. This made CI gen-check fail. Re-render the snapshots via `make helm-template.gateway-crds-helm` so all three (all.out.yaml, e2e.out.yaml, envoy-gateway-crds.out.yaml) now include the "httpsDestinationPorts and httpDestinationPorts must be disjoint" validation, matching the generated CRDs. Signed-off-by: liuhy <liuhongyu@apache.org>
Co-authored-by: zirain <zirain2009@gmail.com> Signed-off-by: aias00 <liuhongyu@apache.org>
Signed-off-by: liuhy <liuhongyu@apache.org>
f86a08f to
3e41fd1
Compare
zirain
left a comment
zirain
left a comment
There was a problem hiding this comment.
LGTM, derfer to other maintainers for the naming.
What this PR does / why we need it:
Exposes Envoy's
forward_proto_configfeature in the Gateway API. When PROXY protocol is enabled on a listener, this infers thex-forwarded-protoheader from the PROXY protocol destination port, so the correct scheme (http/https) is set for requests forwarded to backends.This is useful when a Layer 4 load balancer (such as AWS NLB) terminates TLS and forwards traffic to Envoy over PROXY protocol, where Envoy would otherwise not know whether the original client request was HTTP or HTTPS.
The new field nests under
proxyProtocolinClientTrafficPolicy(sinceforward_proto_configreads the local address restored by theproxy_protocollistener filter):httpsDestinationPorts/httpDestinationPorts: lists of destination ports (each in[1, 65535]).XValidation).forward_proto_configis an HCM field, so it applies to HTTP listeners only; the TCP listener path carries the config but does not consume it.proxyProtocol).Which issue(s) this PR fixes:
Fixes #9219
PR Checklist
git commit -s).ClientTrafficPolicy.spec.proxyProtocol). The API has not been pre-discussed/agreed in a separate issue — opening this PR to gather API feedback; happy to split the API into its own PR if maintainers prefer.make generate/CRD regen,make lint(no new issues), and unit/coverage builds pass locally. Note:gen-check'shelm-generateandgenerate-gwapi-manifestssteps fail in my sandbox due to blocked chart-repo/curl network access, unrelated to this change; the EG CRD, deepcopy, and API docs were regenerated directly withcontroller-gen/crd-ref-docsand verified stable.clienttrafficpolicy-proxyprotocol-forwardproto), 3 CEL validation cases (2 valid, 1 rejecting empty config), an xDS translator unit test (TestHCMForwardProtoConfig), and an e2e test assertingx-forwarded-proto: httpsis inferred from the PROXY protocol destination port.site/content/en/latest/api/extension_types.md).release-notes/current/new_features/9219-forward-proto-config-clienttrafficpolicy.md.🤖 Generated with Claude Code