api: add maxRequestHeaderBytes and host normalization to ClientTrafficPolicy - #8825
api: add maxRequestHeaderBytes and host normalization to ClientTrafficPolicy#8825albsga4 wants to merge 9 commits into
Conversation
…cPolicy Add maxRequestHeaderBytes to ClientTrafficPolicy.headers for configuring maximum request header size (Envoy max_request_headers_kb). Values use resource.Quantity format (e.g. 80Ki) and are validated to be at least 1Ki. Add host.stripPortMode to ClientTrafficPolicy for stripping ports from the Host/Authority header before route matching. Supports Any (unconditional) and Matching (only when port matches listener port). Fixes envoyproxy#5368 Signed-off-by: asalvador <asalvador@newrelic.com>
✅ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0174fe5350
ℹ️ 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".
Guard against uint32 overflow by rejecting values where bytes/1024 exceeds math.MaxUint32. Signed-off-by: asalvador <asalvador@newrelic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52c7f869ce
ℹ️ 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".
Run docs-api-gen to include HostSettings, StripPortMode, and MaxRequestHeaderBytes in the generated API reference. Signed-off-by: asalvador <asalvador@newrelic.com>
Add stripTrailingHostDot bool to the host section. Maps to Envoy's strip_trailing_host_dot on the HCM. Without this, requests with Host "example.com." don't match routes for "example.com". Fixes envoyproxy#8832 Signed-off-by: asalvador <asalvador@newrelic.com>
Signed-off-by: asalvador <asalvador@newrelic.com>
Signed-off-by: asalvador <asalvador@newrelic.com>
…t/ctp-max-request-headers-strip-port
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8825 +/- ##
==========================================
+ Coverage 74.57% 74.65% +0.07%
==========================================
Files 250 251 +1
Lines 39751 40292 +541
==========================================
+ Hits 29644 30079 +435
- Misses 8056 8142 +86
- Partials 2051 2071 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- gocritic/ifElseChain: rewrite if-else to switch for MaxRequestHeaderBytes validation - unparam: translateHostSettings always returned nil, remove error return Signed-off-by: asalvador <asalvador@newrelic.com>
| } | ||
|
|
||
| // HostSettings provides configuration options for the Host/Authority header on the listener. | ||
| type HostSettings struct { |
There was a problem hiding this comment.
can we limit this PR to maxRequestHeaderBytes ?
There was a problem hiding this comment.
Both features are HCM-level listener settings that share the same translation path in listener.go — maxRequestHeaderBytes sets the header size limit, and host normalization (StripPortMode, StripTrailingHostDot) configures how the Host/Authority header is processed before routing.
That said, they are conceptually separate — one is about size limits, the other about normalization. Happy to split into two PRs if you prefer.
There was a problem hiding this comment.
thanks, lets scope this PR to maxRequestHeaderBytes
| // +optional | ||
| // +kubebuilder:validation:XIntOrString | ||
| // +kubebuilder:validation:Pattern="^[1-9]+[0-9]*([EPTGMK]i|[EPTGMk])?$" | ||
| MaxRequestHeaderBytes *resource.Quantity `json:"maxRequestHeaderBytes,omitempty"` |
There was a problem hiding this comment.
based on the style of other fields in the APIs, should this be MaxRequestHeaderLimit ?
There was a problem hiding this comment.
I went with MaxRequestHeaderBytes to stay close to Envoy's own field name (max_request_headers_kb) — the field represents a byte quantity via resource.Quantity (e.g. 60Ki, 1Mi), so Bytes felt natural.
I see the convention though — BufferLimit, SocketBufferLimit, ConnectionLimit all use *Limit for caps. Happy to rename to MaxRequestHeaderLimit if that's the preferred style.
There was a problem hiding this comment.
yeah, prefer the limit suffix instead for the user facing Envoy Gateway API, thanks
|
FTR, as this PR is going to be trimmed from host settings part, I created a second PR from it #9115 |
Add a new `host` section to `ClientTrafficPolicy` to normalize the Host/Authority header on the listener, parallel to the existing `path` section: - `host.stripTrailingHostDot` maps to Envoy `strip_trailing_host_dot`, so requests with `Host: example.com.` match routes for `example.com` (NGINX-parity behavior). - `host.stripPortMode` (`Any`/`Matching`) maps to Envoy `strip_any_host_port` / `strip_matching_host_port`. Both were previously only achievable via fragile EnvoyPatchPolicy. This change is split out of envoyproxy#8825 per maintainer review, which scoped that PR to `maxRequestHeaderBytes`. Fixes envoyproxy#8832 Co-authored-by: albsga4 <asalvador@newrelic.com> Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, when it's ready. Thank you for your contributions! |
Add a new `host` section to `ClientTrafficPolicy` to normalize the Host/Authority header on the listener, parallel to the existing `path` section: - `host.stripTrailingHostDot` maps to Envoy `strip_trailing_host_dot`, so requests with `Host: example.com.` match routes for `example.com` (NGINX-parity behavior). - `host.stripPortMode` (`Any`/`Matching`) maps to Envoy `strip_any_host_port` / `strip_matching_host_port`. Both were previously only achievable via fragile EnvoyPatchPolicy. This change is split out of envoyproxy#8825 per maintainer review, which scoped that PR to `maxRequestHeaderBytes`. Fixes envoyproxy#8832 Co-authored-by: albsga4 <asalvador@newrelic.com> Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
Add a new `host` section to `ClientTrafficPolicy` to normalize the Host/Authority header on the listener, parallel to the existing `path` section: - `host.stripTrailingHostDot` maps to Envoy `strip_trailing_host_dot`, so requests with `Host: example.com.` match routes for `example.com` (NGINX-parity behavior). - `host.stripPortMode` (`Any`/`Matching`) maps to Envoy `strip_any_host_port` / `strip_matching_host_port`. Both were previously only achievable via fragile EnvoyPatchPolicy. This change is split out of envoyproxy#8825 per maintainer review, which scoped that PR to `maxRequestHeaderBytes`. Fixes envoyproxy#8832 Co-authored-by: albsga4 <asalvador@newrelic.com> Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
Add a new `host` section to `ClientTrafficPolicy` to normalize the Host/Authority header on the listener, parallel to the existing `path` section: - `host.stripTrailingHostDot` maps to Envoy `strip_trailing_host_dot`, so requests with `Host: example.com.` match routes for `example.com` (NGINX-parity behavior). - `host.stripPortMode` (`Any`/`Matching`) maps to Envoy `strip_any_host_port` / `strip_matching_host_port`. Both were previously only achievable via fragile EnvoyPatchPolicy. This change is split out of envoyproxy#8825 per maintainer review, which scoped that PR to `maxRequestHeaderBytes`. Fixes envoyproxy#8832 Co-authored-by: albsga4 <asalvador@newrelic.com> Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
* feat(api): add host normalization to ClientTrafficPolicy Add a new `host` section to `ClientTrafficPolicy` to normalize the Host/Authority header on the listener, parallel to the existing `path` section: - `host.stripTrailingHostDot` maps to Envoy `strip_trailing_host_dot`, so requests with `Host: example.com.` match routes for `example.com` (NGINX-parity behavior). Both were previously only achievable via fragile EnvoyPatchPolicy. This change is split out of #8825 per maintainer review, which scoped that PR to `maxRequestHeaderBytes`. Fixes #8832 Co-authored-by: albsga4 <asalvador@newrelic.com> Signed-off-by: Salim Boulkour <salim.boulkour@algolia.com>
What type of PR is this?
api: addmaxRequestHeaderBytesandhost.stripPortModefields toClientTrafficPolicyWhat this PR does / why we need it:
Two common
HttpConnectionManagersettings currently requireEnvoyPatchPolicywith fragile jsonPath expressions to configure:max_request_headers_kb— the default 60 KiB limit is too small for many production use cases (large Authorization headers, cookies, etc.)strip_port_mode— clients sendingHost: example.com:443don't match routes defined without portsThis PR adds native
ClientTrafficPolicyfields for both:headers.maxRequestHeaderBytes(resource.Quantity)max_request_headers_kbon the HCM80Ki,100Ki,1Mi, or raw bytes like81920)1Ki(1024 bytes) — values below return an errorconnection.socketBufferLimit(Quantity in API, uint32 in IR)host.stripPortMode(Enum: Any | Matching)hostsection parallel to the existingpathsection (path normalizes path, host normalizes host)Anymaps to Envoystrip_any_host_port(strips port unconditionally)Matchingmaps to Envoystrip_matching_host_port(strips only when port matches listener)Changes:
api/v1alpha1/clienttrafficpolicy_types.go—MaxRequestHeaderBytesinHeaderSettings, newHostSettingsstruct withStripPortModeenuminternal/ir/xds.go—MaxRequestHeadersKB *uint32inHeaderSettings,HostSettingswith typedStripPortMode+ constantsinternal/gatewayapi/clienttrafficpolicy.go— Quantity-to-uint32 conversion with validation,translateHostSettingsinternal/xds/translator/listener.go— xDS translation (typed-nil safe pattern forstrip_port_modeoneof, standalone bool forstrip_matching_host_port)request-header-size.md+host-port-stripping.mdWhich issue(s) this PR fixes:
Fixes #5368
Release Notes: Yes
/cc @arkodg