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?
release-notes/current.yaml
Outdated
| @@ -7,6 +7,7 @@ breaking changes: | | |||
| An empty TLS ALPNProtocols list is now treated as user-defined disablement of the TLS ALPN extension. | |||
| Outlier detection (passive health check) is now disabled by default. | |||
| refer to https://gateway.envoyproxy.io/docs/api/extension_types/#backendtrafficpolicy for working with passive health checks. | |||
| Envoy Gateway treats errors in calls to an extension service as fail-closed by default. The previous behavior can be enabled by setting the `failOpen` field to `true` in the extension service configuration. | |||
There was a problem hiding this comment.
Maybe we can elaborate a bit here on what fail closed means. Basically, translation/connectivity errors would lead to replacement of proxy routing config with immediate responses.
| @@ -202,6 +202,17 @@ func (t *testingExtensionServer) PostHTTPListenerModify(_ context.Context, req * | |||
|
|
|||
| // PostTranslateModifyHook inserts and overrides some clusters/secrets | |||
| func (t *testingExtensionServer) PostTranslateModify(_ context.Context, req *pb.PostTranslateModifyRequest) (*pb.PostTranslateModifyResponse, error) { | |||
| for _, cluster := range req.Clusters { | |||
| if edsConfig := cluster.GetEdsClusterConfig(); edsConfig != nil { | |||
| if strings.Contains(edsConfig.ServiceName, "fail-close-error") { | |||
There was a problem hiding this comment.
would be good to have some comments in here explaining what this is simulating
| @@ -500,6 +572,21 @@ func (t *Translator) addRouteToRouteConfig( | |||
| // If no extension exists (or it doesn't subscribe to this hook) then this is a quick no-op. | |||
| if err = processExtensionPostVHostHook(vHost, t.ExtensionManager); err != nil { | |||
| errs = errors.Join(errs, err) | |||
| if t.ExtensionManager != nil && !(*t.ExtensionManager).FailOpen() { | |||
| vHost.Routes = []*routev3.Route{ | |||
There was a problem hiding this comment.
would be good to have some comments in here explaining what this setting means, and why its added here, similar comment would be great for other snippets where xds is being modified to return a 500
|
thanks @liorokman , overall LGTM, added some non blocking comments around adding comments |
Signed-off-by: Lior Okman <[email protected]>
and HTTP routes. Signed-off-by: Lior Okman <[email protected]>
Signed-off-by: Lior Okman <[email protected]>
Signed-off-by: Lior Okman <[email protected]>
Signed-off-by: Lior Okman <[email protected]>
Signed-off-by: Lior Okman <[email protected]>
Signed-off-by: Lior Okman <[email protected]>
liorokman
force-pushed
the
ext-server-fail-close
branch
from
2357622 to
0ffd03c
Compare
Signed-off-by: Lior Okman <[email protected]>
|
/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