api: add more timeout fields to ClientTimeout - #9315
Conversation
Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
As instructed, I have created a PR to propose the new API changes before implementing anything. |
![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: 7417ab4412
ℹ️ 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".
|
@HusseinKabbout can you run |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (48.14%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #9315 +/- ##
==========================================
- Coverage 76.05% 76.03% -0.02%
==========================================
Files 260 260
Lines 43367 43392 +25
==========================================
+ Hits 32984 32995 +11
- Misses 8184 8191 +7
- Partials 2199 2206 +7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch>
Sure thing! |
Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch>
4141323 to
96aec06
Compare
Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch>
b169c44 to
041b9b9
Compare
Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch>
Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch>
Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch>
4805145 to
f8799b4
Compare
|
Based on the discussion, I think the API looks OK now. I can still rename the attributes but I think their place in the API is settled. I went ahead and implemented the timouts. One open question I have is how does EG handle defaults? Should I explicitly set default values when they make sense (they do here)? |
Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch>
|
Regarding e2e tests, I did not find any EG timeout tests, only a conformance test related to Gateway API timeouts. |
|
@arkodg Do you think this can get in v1.9.0-rc.1 Release or is it maybe too late? |
Hi @HusseinKabbout could you please rename |
Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4a8a31eee
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if timeout != nil && timeout.TCP != nil && timeout.TCP.ConnectionInspectionTimeout != nil { | ||
| listener.ListenerFiltersTimeout = durationpb.New(timeout.TCP.ConnectionInspectionTimeout.Duration) |
There was a problem hiding this comment.
When multiple HTTPS or TLS listeners share an address and port, only the first IR listener creates the xDS listener; subsequent listeners reuse it without revisiting ListenerFiltersTimeout. Consequently, a ClientTrafficPolicy targeting a later listener has its connectionInspectionTimeout silently ignored, while a timeout on the first listener affects every filter chain on that socket. The existing overlap validation only covers non-HTTPS listeners, so this global setting must either be reconciled across shared listeners or conflicting policies must be rejected.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not sure this makes sense. ClientTrafficPolicy is per listener and EG tracks which listeners have already been attached to and errors out for that policy.
When multiple HTTPS or TLS listeners share an address and port, only the first IR listener creates the xDS listener
Since they share the same xDS listener then we can't actually have a different ListenerFiltersTimeout anyway, right? Am I missing something here?
There was a problem hiding this comment.
You're right that we can't have different values per listener — listener_filters_timeout is a Listener field, not a FilterChain field, so "reconciling" isn't an option.
The conflict is still real though, just framed differently. The tracking you mention is one-CTP-per-section, and the port-overlap check skips TLS listeners (it assumes each gets its own filter chain, which is true for filter-chain-scoped settings but not listener-scoped ones). So with two HTTPS listeners on :443 and a CTP per section, both are Accepted, the first-processed value wins for both, and the other is silently dropped.
That said, this isn't introduced by your PR — the same applies to connection.bufferLimit, tcpKeepalive, and proxy protocol (see the existing TODO in translator.go). A proper fix is a cross-listener consistency check covering all of them, which belongs in its own PR.
Non-blocking. Just worth a sentence in the ConnectionInspectionTimeout godoc that it applies to the whole listener socket, so users aren't surprised when a shared address:port picks one value. A follow-up issue would be nice too.
Co-authored-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com> Signed-off-by: kabbohus <hussein.kabbout@hotmail.com>
4 participants
What type of PR is this?
Feature + API (non-breaking)
What this PR does / why we need it:
This PR exposes new Envoy proxy timeout settings in
ClientTrafficPolicywhen dealing with untrusted downstreams. By tweaking these new exposed settings, DoS protection can be improved depending on the use case.The settings are:
ConnectionInspectionTimeoutRequestHeadersReceivedTimeoutHandshakeTimeoutWhich issue(s) this PR fixes:
Fixes #9254
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/.