api: add more timeout fields to ClientTimeout - #9315
Conversation
✅ 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
Additional details and impacted files@@ Coverage Diff @@
## main #9315 +/- ##
==========================================
- Coverage 76.17% 76.16% -0.02%
==========================================
Files 261 261
Lines 43528 43553 +25
==========================================
+ Hits 33159 33173 +14
- Misses 8165 8171 +6
- Partials 2204 2209 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Sure thing! |
4141323 to
96aec06
Compare
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)? |
|
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 |
|
@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".
There was a problem hiding this comment.
can you add a test case for xds translator to show the result of these new knobs?
There was a problem hiding this comment.
I have added xds translation tests but found out that we also have the same issues for filter chain specific settings. If for example two listeners with the same address / port have two different values for requestReceivedTimeout (already existing setting) then the resulting filter chain will only contain the settings of the first listener. I think it is the same issue with the handshake timeout.
There was a problem hiding this comment.
LGTM. Thanks!
It would be good to add xds test golden files as @zirain suggested.
|
/retest |
81d2d56 to
40cc34b
Compare
40cc34b to
245e193
Compare
245e193 to
a9c006e
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>
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>
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>
Co-authored-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com> Signed-off-by: kabbohus <hussein.kabbout@hotmail.com>
Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch>
Signed-off-by: kabbohus <hussein.kabbout@united-security-providers.ch>
a9c006e to
1c9779f
Compare
|
/retest |
5 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/.