fix: don't lose timeout settings that originate from the route when translating the backend traffic policy#4095
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4095 +/- ##
==========================================
+ Coverage 67.86% 67.94% +0.08%
==========================================
Files 187 187
Lines 23023 23018 -5
==========================================
+ Hits 15624 15640 +16
+ Misses 6277 6264 -13
+ Partials 1122 1114 -8 ☔ View full report in Codecov by Sentry. |
|
/retest |
1 similar comment
|
/retest |
There was a problem hiding this comment.
shouldnt this have worked as well ?
There was a problem hiding this comment.
The problem was an ordering problem. The timeout value was lost in the original ordering because r.Traffic (which contained the value collected from the HTTPRoute was overridden in line 392 (386 in the original file).
There was a problem hiding this comment.
why are we returning early here, if a route is non empty shouldnt we also compute route timeout set then and then return ?
There was a problem hiding this comment.
Returning early here because the policy doesn't contain any new timeout information. There's nothing to compute. The only thing that should happen is to not ignore whatever values are already in ir.Timeout due to how it was configured from the HTTPRoute resource.
There was a problem hiding this comment.
I think I understand the code flow now, we have two methods
- processTimeout - which builds out the
timeoutstruct by processing the HTTPRoute timeout section - buildTimeout - which builds out the
timeoutstruct by processing BTP
to improve readability, my suggestion would be to rename the functions to buildRouteTimeout and buildBTPTimeout and add a 3rd func called mergeTimeouts which copies over the route timeout fields if set, and gets called in the route section
There was a problem hiding this comment.
buildBTPTimeout is a little misleading. That function is used wherever there are ClusterSettings involved - also for things like ext-proc, ext-auth, tracing, and logging cluster definitions.
I'll make a few changes to improve readability though.
c0bb9e9 to
7344c83
Compare
|
/retest |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
arkodg
left a comment
There was a problem hiding this comment.
LGTM thanks for the refactor
going forward, we are probably going to hit such situations very often where a feature will exist in the extension as well as in the core gateway api API, so we should consider following this design pattern there as well to merge the values
translating the backend traffic policy Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
Signed-off-by: Lior Okman <lior.okman@sap.com>
e320f93 to
8cbe3e8
Compare
|
/retest |
|
cherry-picking this one back is non trivial due to the refactoring, so removing the cherry-pick label for this |
…ranslating the backend traffic policy (envoyproxy#4095) (#85) * Don't lose timeout settings that originate from the route when translating the backend traffic policy * Improve the readability of the timeout processing code. * Removed some logically unreachable code. --------- (cherry picked from commit 262e046) Signed-off-by: Lior Okman <lior.okman@sap.com> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Co-authored-by: Lior Okman <lior.okman@sap.com>
…he route when t… (#4450) fix: don't lose timeout settings that originate from the route when translating the backend traffic policy (#4095) (#85) * Don't lose timeout settings that originate from the route when translating the backend traffic policy * Improve the readability of the timeout processing code. * Removed some logically unreachable code. --------- (cherry picked from commit 262e046) Signed-off-by: Lior Okman <lior.okman@sap.com> Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Co-authored-by: Lior Okman <lior.okman@sap.com>
Which issue(s) this PR fixes:
Fixes #4091