feat: allow updating initial_fetch_timeout in bootstrap #7962
Conversation
Signed-off-by: andrei <andrei@n2.games>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
please add a release notes. |
Signed-off-by: andrei <andrei@n2.games>
|
@andreik-n2 please fix the conflict. |
Signed-off-by: andreik-n2 <144345749+andreik-n2@users.noreply.github.com>
|
@andreik-n2 can we default to 0 ? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7962 +/- ##
==========================================
+ Coverage 72.81% 72.83% +0.01%
==========================================
Files 237 237
Lines 35475 35506 +31
==========================================
+ Hits 25832 25861 +29
- Misses 7803 7805 +2
Partials 1840 1840 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I don't think this PR change the default value from 15s to 0s, it make it patched. |
Hi @arkodg , are you sure you want set default to 0? I read that it could lead to having envoy proxy in not ready state indefinitely. Is it something you considered? |
yeah its better to fail hard for this case that configure partial config |
|
ptal @envoyproxy/gateway-maintainers |
…guration. Signed-off-by: andrei <andrei@n2.games>
| } | ||
|
|
||
| // Ensure dynamic resources config is same | ||
| // Ensure dynamic resources config is same except for initial_fetch_timeout |
There was a problem hiding this comment.
you mean the comment change or the code to skip validation for changed initial_fetch_timeout field in bootstrap?
- the second was needed to skip validation for changed initial_fetch_timeout, otherwise the gateway will fail.
Do you think after changing the default initial_fetch_timeout to 0, this is not needed? Also do you think it should not be allowed to patch initial_fetch_timeout in bootstrap?
There was a problem hiding this comment.
a user could update it if they wanted to
There was a problem hiding this comment.
sorry, I didn't get it. you can't modify bootstrap cds and lds configs as of now. Am I missing something? Can you elaborate please?
There was a problem hiding this comment.
my bad, i misunderstood this code, its been a while since I reviewed this, this diff allows a user to patch this value and allow overriding it, which is fine
3 participants
What type of PR is this?
fix: ignore initial_fetch_timeout in bootstrap validation to allow modifying this parameter. Set initial_fetch_timeout to 0s by default.
What this PR does / why we need it: This PR updates the bootstrap validation logic to ignore the initial_fetch_timeout field within cds_config and lds_config during comparison. Also this PR sets default initial_fetch_timeout to 0s in the bootstrap config.
Which issue(s) this PR fixes:
Fixes #5574
Release Notes: Yes