feat(translator): extension server should fail close #4936
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4936 +/- ##
==========================================
- Coverage 66.80% 66.75% -0.06%
==========================================
Files 209 209
Lines 32264 32367 +103
==========================================
+ Hits 21554 21606 +52
- Misses 9419 9455 +36
- Partials 1291 1306 +15 ☔ View full report in Codecov by Sentry. |
|
/retest |
1 similar comment
|
/retest |
| for _, filter := range listener.FilterChains { | ||
| hcm, err := findHCMinFilterChain(filter) | ||
| if err != nil { | ||
| // no HCM found, skip |
There was a problem hiding this comment.
Should we also cover use cases where tcp_proxy is found instead of HCM? e.g. for filter chains generated for TCP/TLSRoute. Is it currently possible to mutate these filter chains in the extension manager using one of the hooks?
There was a problem hiding this comment.
The listener hook will be called for non-HTTP(s) listeners.
But for non-HTTP(s) listeners, it's not possible to return an HTTP 500 response. What would "failing-close" look like here?
There was a problem hiding this comment.
Maybe we can remove the filter chain such that envoy resets connections? Anyway, not blocking IMHO.
There was a problem hiding this comment.
OK, I'll do something like that.
|
thanks @liorokman , overall LGTM, added some non blocking comments around adding comments |
Signed-off-by: Lior Okman <lior.okman@sap.com>
and HTTP routes. 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>
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>
liorokman
force-pushed
the
ext-server-fail-close
branch
from
2357622 to
0ffd03c
Compare
Signed-off-by: Lior Okman <lior.okman@sap.com>
|
/retest |
This PR changes the default behavior for extension servers to fail-close instead of fail open.
Which issue(s) this PR fixes:
Fixes #4155
Release Notes: Yes