feat(translator): extension server should fail close (#4936)

* Start of work on fail-close for extension server

Signed-off-by: Lior Okman <[email protected]>

* Override hook targets if the hooks return an error for HTTP listeners
and HTTP routes.

Signed-off-by: Lior Okman <[email protected]>

* Added support for setting the extension server's mode to fail-open.

Signed-off-by: Lior Okman <[email protected]>

* Treat connection errors in the same way as hook errors.

Signed-off-by: Lior Okman <[email protected]>

* Added a note in the release notes

Signed-off-by: Lior Okman <[email protected]>

* Some more tests

Signed-off-by: Lior Okman <[email protected]>

* Updated release notes and comments as requested in the review.

Signed-off-by: Lior Okman <[email protected]>

* Removed a trailing space in the release notes.

Signed-off-by: Lior Okman <[email protected]>

---------

Signed-off-by: Lior Okman <[email protected]>

api/v1alpha1/envoygateway_types.go

@@ -500,6 +500,18 @@ type ExtensionManager struct {
 	//
 	// +kubebuilder:validation:Required
 	Service *ExtensionService `json:"service,omitempty"`
+
+	// FailOpen defines if Envoy Gateway should ignore errors returned from the Extension Service hooks.
+	// The default is false, which means Envoy Gateway will fail closed if the Extension Service returns an error.
+	//
+	// Fail-close means that if the Extension Service hooks return an error, the relevant route/listener/resource
+	// will be replaced with a default configuration returning Internal Server Error (HTTP 500).
+	//
+	// Fail-open means that if the Extension Service hooks return an error, no changes will be applied to the
+	// source of the configuration which was sent to the extension server.
+	//
+	// +optional
+	FailOpen bool `json:"failOpen,omitempty"`
 }
 
 // ExtensionHooks defines extension hooks across all supported runners

internal/extension/registry/extension_manager.go

@@ -107,6 +107,11 @@ func NewInMemoryManager(cfg egv1a1.ExtensionManager, server extension.EnvoyGatew
 	}, c, nil
 }
 
+// FailOpen returns true if the extension manager is configured to fail open, and false otherwise.
+func (m *Manager) FailOpen() bool {
+	return m.extension.FailOpen
+}
+
 // HasExtension checks to see whether a given Group and Kind has an
 // associated extension registered for it.
 func (m *Manager) HasExtension(g gwapiv1.Group, k gwapiv1.Kind) bool {
@@ -139,15 +144,15 @@ func getExtensionServerAddress(service *egv1a1.ExtensionService) string {
 // that are used to generate an xDS resource.
 // If the extension makes use of the hook then the XDS Hook Client is returned. If it does not support
 // the hook type then nil is returned
-func (m *Manager) GetPreXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) extTypes.XDSHookClient {
+func (m *Manager) GetPreXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) (extTypes.XDSHookClient, error) {
 	ctx := context.Background()
 	ext := m.extension
 
 	if ext.Hooks == nil {
-		return nil
+		return nil, nil
 	}
 	if ext.Hooks.XDSTranslator == nil {
-		return nil
+		return nil, nil
 	}
 
 	hookUsed := false
@@ -158,20 +163,20 @@ func (m *Manager) GetPreXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) extT
 		}
 	}
 	if !hookUsed {
-		return nil
+		return nil, nil
 	}
 
 	if m.extensionConnCache == nil {
 		serverAddr := getExtensionServerAddress(ext.Service)
 
 		opts, err := setupGRPCOpts(ctx, m.k8sClient, &ext, m.namespace)
 		if err != nil {
-			return nil
+			return nil, err
 		}
 
 		conn, err := grpc.Dial(serverAddr, opts...)
 		if err != nil {
-			return nil
+			return nil, err
 		}
 
 		m.extensionConnCache = conn
@@ -181,22 +186,22 @@ func (m *Manager) GetPreXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) extT
 	xdsHookClient := &XDSHook{
 		grpcClient: client,
 	}
-	return xdsHookClient
+	return xdsHookClient, nil
 }
 
 // GetPostXDSHookClient checks if the registered extension makes use of a particular hook type that modifies
 // xDS resources after they are generated by Envoy Gateway.
 // If the extension makes use of the hook then the XDS Hook Client is returned. If it does not support
 // the hook type then nil is returned
-func (m *Manager) GetPostXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) extTypes.XDSHookClient {
+func (m *Manager) GetPostXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) (extTypes.XDSHookClient, error) {
 	ctx := context.Background()
 	ext := m.extension
 
 	if ext.Hooks == nil {
-		return nil
+		return nil, nil
 	}
 	if ext.Hooks.XDSTranslator == nil {
-		return nil
+		return nil, nil
 	}
 
 	hookUsed := false
@@ -207,20 +212,20 @@ func (m *Manager) GetPostXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) ext
 		}
 	}
 	if !hookUsed {
-		return nil
+		return nil, nil
 	}
 
 	if m.extensionConnCache == nil {
 		serverAddr := getExtensionServerAddress(ext.Service)
 
 		opts, err := setupGRPCOpts(ctx, m.k8sClient, &ext, m.namespace)
 		if err != nil {
-			return nil
+			return nil, err
 		}
 
 		conn, err := grpc.Dial(serverAddr, opts...)
 		if err != nil {
-			return nil
+			return nil, err
 		}
 
 		m.extensionConnCache = conn
@@ -230,7 +235,7 @@ func (m *Manager) GetPostXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) ext
 	xdsHookClient := &XDSHook{
 		grpcClient: client,
 	}
-	return xdsHookClient
+	return xdsHookClient, nil
 }
 
 func (m *Manager) CleanupHookConns() {

internal/extension/types/manager.go

@@ -25,11 +25,14 @@ type Manager interface {
 	// that are used to generate an xDS resource.
 	// If the extension makes use of the hook then the XDS Hook Client is returned. If it does not support
 	// the hook type then nil is returned
-	GetPreXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) XDSHookClient
+	GetPreXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) (XDSHookClient, error)
 
 	// GetPostXDSHookClient checks if the registered extension makes use of a particular hook type that modifies
 	// xDS resources after they are generated by Envoy Gateway.
 	// If the extension makes use of the hook then the XDS Hook Client is returned. If it does not support
 	// the hook type then nil is returned
-	GetPostXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) XDSHookClient
+	GetPostXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) (XDSHookClient, error)
+
+	// FailOpen returns true if the extension manager is configured to fail open, and false otherwise.
+	FailOpen() bool
 }

internal/xds/translator/extension.go

@@ -35,7 +35,10 @@ func processExtensionPostRouteHook(route *routev3.Route, vHost *routev3.VirtualH
 
 	// Check if an extension want to modify the route that was just configured/created
 	extManager := *em
-	extRouteHookClient := extManager.GetPostXDSHookClient(egv1a1.XDSRoute)
+	extRouteHookClient, err := extManager.GetPostXDSHookClient(egv1a1.XDSRoute)
+	if err != nil {
+		return err
+	}
 	if extRouteHookClient == nil {
 		return nil
 	}
@@ -71,7 +74,10 @@ func processExtensionPostVHostHook(vHost *routev3.VirtualHost, em *extensionType
 
 	// Check if an extension want to modify the route that was just configured/created
 	extManager := *em
-	extVHHookClient := extManager.GetPostXDSHookClient(egv1a1.XDSVirtualHost)
+	extVHHookClient, err := extManager.GetPostXDSHookClient(egv1a1.XDSVirtualHost)
+	if err != nil {
+		return err
+	}
 	if extVHHookClient == nil {
 		return nil
 	}
@@ -100,7 +106,10 @@ func processExtensionPostListenerHook(tCtx *types.ResourceVersionTable, xdsListe
 
 	// Check if an extension want to modify the listener that was just configured/created
 	extManager := *em
-	extListenerHookClient := extManager.GetPostXDSHookClient(egv1a1.XDSHTTPListener)
+	extListenerHookClient, err := extManager.GetPostXDSHookClient(egv1a1.XDSHTTPListener)
+	if err != nil {
+		return err
+	}
 	if extListenerHookClient != nil {
 		unstructuredResources := make([]*unstructured.Unstructured, len(extensionRefs))
 		for refIdx, ref := range extensionRefs {
@@ -140,7 +149,10 @@ func processExtensionPostTranslationHook(tCtx *types.ResourceVersionTable, em *e
 	// that is non-static. If a cluster definition is unlikely to change over the course of an extension's lifetime then the custom bootstrap config
 	// is the preferred way of adding it.
 	extManager := *em
-	extensionInsertHookClient := extManager.GetPostXDSHookClient(egv1a1.XDSTranslation)
+	extensionInsertHookClient, err := extManager.GetPostXDSHookClient(egv1a1.XDSTranslation)
+	if err != nil {
+		return err
+	}
 	if extensionInsertHookClient == nil {
 		return nil
 	}

internal/xds/translator/extensionserver_test.go

@@ -202,6 +202,18 @@ 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 {
+		// This simulates an extension server that returns an error. It allows verifying that fail-close is working.
+		if edsConfig := cluster.GetEdsClusterConfig(); edsConfig != nil {
+			if strings.Contains(edsConfig.ServiceName, "fail-close-error") {
+				return &pb.PostTranslateModifyResponse{
+					Clusters: req.Clusters,
+					Secrets:  req.Secrets,
+				}, fmt.Errorf("cluster hook resource error: %s", edsConfig.ServiceName)
+			}
+		}
+	}
+
 	extensionSvcEndpoint := &endpointV3.LbEndpoint_Endpoint{
 		Endpoint: &endpointV3.Endpoint{
 			Address: &coreV3.Address{

internal/xds/translator/runner/runner_test.go

@@ -174,12 +174,16 @@ type extManagerMock struct {
 	types.Manager
 }
 
-func (m *extManagerMock) GetPostXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) types.XDSHookClient {
+func (m *extManagerMock) GetPostXDSHookClient(xdsHookType egv1a1.XDSTranslatorHook) (types.XDSHookClient, error) {
 	if xdsHookType == egv1a1.XDSHTTPListener {
-		return &xdsHookClientMock{}
+		return &xdsHookClientMock{}, nil
 	}
 
-	return nil
+	return nil, nil
+}
+
+func (m *extManagerMock) FailOpen() bool {
+	return false
 }
 
 type xdsHookClientMock struct {

internal/xds/translator/testdata/in/extension-xds-ir/http-route-extension-translate-error.yaml

@@ -0,0 +1,20 @@
+http:
+- name: "extension-post-xdstranslate-hook-error"
+  address: "0.0.0.0"
+  port: 10080
+  hostnames:
+  - "*"
+  path:
+    mergeSlashes: true
+    escapedSlashesAction: UnescapeAndRedirect
+  routes:
+  - name: "first-route"
+    hostname: "*"
+    pathMatch:
+      prefix: "/"
+    destination:
+      name: "fail-close-error"
+      settings:
+      - endpoints:
+        - host: "1.2.3.4"
+          port: 50000

internal/xds/translator/testdata/out/extension-xds-ir/http-route-extension-listener-error.clusters.yaml

@@ -0,0 +1,27 @@
+- circuitBreakers:
+    thresholds:
+    - maxRetries: 1024
+  commonLbConfig:
+    localityWeightedLbConfig: {}
+  connectTimeout: 10s
+  dnsLookupFamily: V4_PREFERRED
+  edsClusterConfig:
+    edsConfig:
+      ads: {}
+      resourceApiVersion: V3
+    serviceName: first-route-dest
+  ignoreHealthOnHostRemoval: true
+  lbPolicy: LEAST_REQUEST
+  name: first-route-dest
+  perConnectionBufferLimitBytes: 32768
+  type: EDS
+- loadAssignment:
+    clusterName: mock-extension-injected-cluster
+    endpoints:
+    - lbEndpoints:
+      - endpoint:
+          address:
+            socketAddress:
+              address: exampleservice.examplenamespace.svc.cluster.local
+              portValue: 5000
+  name: mock-extension-injected-cluster

internal/xds/translator/testdata/out/extension-xds-ir/http-route-extension-listener-error.endpoints.yaml

@@ -0,0 +1,12 @@
+- clusterName: first-route-dest
+  endpoints:
+  - lbEndpoints:
+    - endpoint:
+        address:
+          socketAddress:
+            address: 1.2.3.4
+            portValue: 50000
+      loadBalancingWeight: 1
+    loadBalancingWeight: 1
+    locality:
+      region: first-route-dest/backend/0

internal/xds/translator/testdata/out/extension-xds-ir/http-route-extension-listener-error.listeners.yaml

@@ -0,0 +1,41 @@
+- address:
+    socketAddress:
+      address: 0.0.0.0
+      portValue: 10080
+  defaultFilterChain:
+    filters:
+    - name: envoy.filters.network.http_connection_manager
+      typedConfig:
+        '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
+        commonHttpProtocolOptions:
+          headersWithUnderscoresAction: REJECT_REQUEST
+        http2ProtocolOptions:
+          initialConnectionWindowSize: 1048576
+          initialStreamWindowSize: 65536
+          maxConcurrentStreams: 100
+        httpFilters:
+        - name: envoy.filters.http.router
+          typedConfig:
+            '@type': type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
+            suppressEnvoyHeaders: true
+        mergeSlashes: true
+        normalizePath: true
+        pathWithEscapedSlashesAction: UNESCAPE_AND_REDIRECT
+        routeConfig:
+          name: error_route_configuration
+          virtualHosts:
+          - domains:
+            - '*'
+            name: error_vhost
+            routes:
+            - directResponse:
+                status: 500
+              match:
+                prefix: /
+              name: error_route
+        serverHeaderTransformation: PASS_THROUGH
+        statPrefix: http-10080
+        useRemoteAddress: true
+    name: extension-post-xdslistener-hook-error
+  name: extension-post-xdslistener-hook-error
+  perConnectionBufferLimitBytes: 32768

internal/xds/translator/testdata/out/extension-xds-ir/http-route-extension-listener-error.routes.yaml

@@ -0,0 +1,14 @@
+- ignorePortInHostMatching: true
+  name: extension-post-xdslistener-hook-error
+  virtualHosts:
+  - domains:
+    - '*'
+    name: extension-post-xdslistener-hook-error/*
+    routes:
+    - match:
+        prefix: /
+      name: first-route
+      route:
+        cluster: first-route-dest
+        upgradeConfigs:
+        - upgradeType: websocket

internal/xds/translator/testdata/out/extension-xds-ir/http-route-extension-listener-error.secrets.yaml

@@ -0,0 +1,4 @@
+- genericSecret:
+    secret:
+      inlineString: super-secret-extension-secret
+  name: mock-extension-injected-secret

internal/xds/translator/testdata/out/extension-xds-ir/http-route-extension-route-error.clusters.yaml

@@ -0,0 +1,27 @@
+- circuitBreakers:
+    thresholds:
+    - maxRetries: 1024
+  commonLbConfig:
+    localityWeightedLbConfig: {}
+  connectTimeout: 10s
+  dnsLookupFamily: V4_PREFERRED
+  edsClusterConfig:
+    edsConfig:
+      ads: {}
+      resourceApiVersion: V3
+    serviceName: extension-post-xdsroute-hook-error-dest
+  ignoreHealthOnHostRemoval: true
+  lbPolicy: LEAST_REQUEST
+  name: extension-post-xdsroute-hook-error-dest
+  perConnectionBufferLimitBytes: 32768
+  type: EDS
+- loadAssignment:
+    clusterName: mock-extension-injected-cluster
+    endpoints:
+    - lbEndpoints:
+      - endpoint:
+          address:
+            socketAddress:
+              address: exampleservice.examplenamespace.svc.cluster.local
+              portValue: 5000
+  name: mock-extension-injected-cluster