Fix: requests not matching defined routes trigger per-route filters (#…

…2663) * Revert "add a catch-all route if needed (#2586)" This reverts commit 35e646d. * disable per-route filters at the http_filters Signed-off-by: huabing zhao <zhaohuabing@gmail.com> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com>
envoyproxy · Feb 20, 2024 · 7ab3da6 · 7ab3da6
1 parent 5e78542
commit 7ab3da6
Show file tree

Hide file tree

Showing 21 changed files with 34 additions and 321 deletions.
diff --git a/internal/gatewayapi/testdata/securitypolicy-with-basic-auth.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-basic-auth.out.yaml
@@ -155,14 +155,3 @@ xdsIR:
           distinct: false
           name: ""
           prefix: /foo
-      - backendWeights:
-          invalid: 0
-          valid: 0
-        directResponse:
-          statusCode: 404
-        hostname: gateway.envoyproxy.io
-        name: gateway_envoyproxy_io/catch-all-return-404
-        pathMatch:
-          distinct: false
-          name: ""
-          prefix: /
diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth.out.yaml
@@ -272,25 +272,3 @@ xdsIR:
           distinct: false
           name: ""
           prefix: /bar
-      - backendWeights:
-          invalid: 0
-          valid: 0
-        directResponse:
-          statusCode: 404
-        hostname: www.foo.com
-        name: www_foo_com/catch-all-return-404
-        pathMatch:
-          distinct: false
-          name: ""
-          prefix: /
-      - backendWeights:
-          invalid: 0
-          valid: 0
-        directResponse:
-          statusCode: 404
-        hostname: www.bar.com
-        name: www_bar_com/catch-all-return-404
-        pathMatch:
-          distinct: false
-          name: ""
-          prefix: /
diff --git a/internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml b/internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml
@@ -62,15 +62,15 @@ httpRoutes:
     name: httproute-2
   spec:
     hostnames:
-    - www.foo.com
+    - www.example.com
     parentRefs:
     - namespace: envoy-gateway
       name: gateway-1
       sectionName: http
     rules:
     - matches:
       - path:
-          value: "/"
+          value: "/bar"
       backendRefs:
       - name: service-1
         port: 8080

diff --git a/internal/gatewayapi/testdata/securitypolicy-with-oidc.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-oidc.out.yaml
@@ -86,7 +86,7 @@ httpRoutes:
     namespace: default
   spec:
     hostnames:
-    - www.foo.com
+    - www.example.com
     parentRefs:
     - name: gateway-1
       namespace: envoy-gateway
@@ -97,7 +97,7 @@ httpRoutes:
         port: 8080
       matches:
       - path:
-          value: /
+          value: /bar
   status:
     parents:
     - conditions:
@@ -256,8 +256,8 @@ xdsIR:
               port: 8080
             protocol: HTTP
             weight: 1
-        hostname: www.foo.com
-        name: httproute/default/httproute-2/rule/0/match/0/www_foo_com
+        hostname: www.example.com
+        name: httproute/default/httproute-2/rule/0/match/0/www_example_com
         oidc:
           clientID: client1.apps.googleusercontent.com
           clientSecret: Y2xpZW50MTpzZWNyZXQK
@@ -272,15 +272,4 @@ xdsIR:
         pathMatch:
           distinct: false
           name: ""
-          prefix: /
-      - backendWeights:
-          invalid: 0
-          valid: 0
-        directResponse:
-          statusCode: 404
-        hostname: www.example.com
-        name: www_example_com/catch-all-return-404
-        pathMatch:
-          distinct: false
-          name: ""
-          prefix: /
+          prefix: /bar
diff --git a/internal/gatewayapi/translator.go b/internal/gatewayapi/translator.go
@@ -6,12 +6,8 @@
 package gatewayapi
 
 import (
-	"fmt"
-	"strings"
-
 	"golang.org/x/exp/maps"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	"k8s.io/utils/ptr"
 	gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"
 
 	egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
@@ -207,61 +203,12 @@ func (t *Translator) Translate(resources *Resources) *TranslateResult {
 	// Sort xdsIR based on the Gateway API spec
 	sortXdsIRMap(xdsIR)
 
-	// Add a catch-all route for each HTTP listener if needed
-	addCatchAllRoute(xdsIR)
-
 	return newTranslateResult(gateways, httpRoutes, grpcRoutes, tlsRoutes,
 		tcpRoutes, udpRoutes, clientTrafficPolicies, backendTrafficPolicies,
 		securityPolicies, xdsIR, infraIR)
 
 }
 
-// For filters without native per-route support, we need to add a catch-all route
-// to ensure that these filters are disabled for non-matching requests.
-// https://github.com/envoyproxy/gateway/issues/2507
-func addCatchAllRoute(xdsIR map[string]*ir.Xds) {
-	for _, i := range xdsIR {
-		for _, http := range i.HTTP {
-			var needCatchAllRoutePerHost = make(map[string]bool)
-			for _, r := range http.Routes {
-				if r.ExtAuth != nil || r.BasicAuth != nil || r.OIDC != nil {
-					needCatchAllRoutePerHost[r.Hostname] = true
-				}
-			}
-
-			// skip if there is already a catch-all route
-			for host := range needCatchAllRoutePerHost {
-				for _, r := range http.Routes {
-					if (r.Hostname == host &&
-						r.PathMatch != nil &&
-						r.PathMatch.Prefix != nil &&
-						*r.PathMatch.Prefix == "/") &&
-						len(r.HeaderMatches) == 0 &&
-						len(r.QueryParamMatches) == 0 {
-						delete(needCatchAllRoutePerHost, host)
-					}
-				}
-			}
-
-			for host, needCatchAllRoute := range needCatchAllRoutePerHost {
-				if needCatchAllRoute {
-					underscoredHost := strings.ReplaceAll(host, ".", "_")
-					http.Routes = append(http.Routes, &ir.HTTPRoute{
-						Name: fmt.Sprintf("%s/catch-all-return-404", underscoredHost),
-						PathMatch: &ir.StringMatch{
-							Prefix: ptr.To("/"),
-						},
-						DirectResponse: &ir.DirectResponse{
-							StatusCode: 404,
-						},
-						Hostname: host,
-					})
-				}
-			}
-		}
-	}
-}
-
 // GetRelevantGateways returns GatewayContexts, containing a copy of the original
 // Gateway with the Listener statuses reset.
 func (t *Translator) GetRelevantGateways(gateways []*gwapiv1.Gateway) []*GatewayContext {

diff --git a/internal/xds/translator/basicauth.go b/internal/xds/translator/basicauth.go
@@ -7,7 +7,6 @@ package translator
 
 import (
 	"errors"
-	"fmt"
 
 	corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
 	routev3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
@@ -35,6 +34,7 @@ var _ httpFilter = &basicAuth{}
 // patchHCM builds and appends the basic_auth Filters to the HTTP Connection Manager
 // if applicable, and it does not already exist.
 // Note: this method creates an basic_auth filter for each route that contains an BasicAuth config.
+// The filter is disabled by default. It is enabled on the route level.
 func (*basicAuth) patchHCM(mgr *hcmv3.HttpConnectionManager, irListener *ir.HTTPListener) error {
 	var errs error
 
@@ -77,7 +77,8 @@ func buildHCMBasicAuthFilter(route *ir.HTTPRoute) (*hcmv3.HttpFilter, error) {
 	}
 
 	return &hcmv3.HttpFilter{
-		Name: basicAuthFilterName(route),
+		Name:     basicAuthFilterName(route),
+		Disabled: true,
 		ConfigType: &hcmv3.HttpFilter_TypedConfig{
 			TypedConfig: basicAuthAny,
 		},
@@ -116,52 +117,6 @@ func (*basicAuth) patchResources(*types.ResourceVersionTable, []*ir.HTTPRoute) e
 	return nil
 }
 
-// patchRouteCfg patches the provided route configuration with the basicAuth filter
-// if applicable.
-// Note: this method disables all the basicAuth filters by default. The filter will
-// be enabled per-route in the typePerFilterConfig of the route.
-func (*basicAuth) patchRouteConfig(routeCfg *routev3.RouteConfiguration, irListener *ir.HTTPListener) error {
-	if routeCfg == nil {
-		return errors.New("route configuration is nil")
-	}
-	if irListener == nil {
-		return errors.New("ir listener is nil")
-	}
-
-	var errs error
-	for _, route := range irListener.Routes {
-		if !routeContainsBasicAuth(route) {
-			continue
-		}
-
-		filterName := basicAuthFilterName(route)
-		filterCfg := routeCfg.TypedPerFilterConfig
-
-		if _, ok := filterCfg[filterName]; ok {
-			// This should not happen since this is the only place where the basicAuth
-			// filter is added in a route.
-			errs = errors.Join(errs, fmt.Errorf(
-				"route config already contains basicAuth config: %+v", route))
-			continue
-		}
-
-		// Disable all the filters by default. The filter will be enabled
-		// per-route in the typePerFilterConfig of the route.
-		routeCfgAny, err := anypb.New(&routev3.FilterConfig{Disabled: true})
-		if err != nil {
-			errs = errors.Join(errs, err)
-			continue
-		}
-
-		if filterCfg == nil {
-			routeCfg.TypedPerFilterConfig = make(map[string]*anypb.Any)
-		}
-
-		routeCfg.TypedPerFilterConfig[filterName] = routeCfgAny
-	}
-	return errs
-}
-
 // patchRoute patches the provided route with the basicAuth config if applicable.
 // Note: this method enables the corresponding basicAuth filter for the provided route.
 func (*basicAuth) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {

diff --git a/internal/xds/translator/cors.go b/internal/xds/translator/cors.go
@@ -165,10 +165,6 @@ func (*cors) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
 	return nil
 }
 
-func (*cors) patchRouteConfig(*routev3.RouteConfiguration, *ir.HTTPListener) error {
-	return nil
-}
-
 func (c *cors) patchResources(*types.ResourceVersionTable, []*ir.HTTPRoute) error {
 	return nil
 }
diff --git a/internal/xds/translator/extauth.go b/internal/xds/translator/extauth.go
@@ -7,7 +7,6 @@ package translator
 
 import (
 	"errors"
-	"fmt"
 	"net/url"
 
 	corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
@@ -38,6 +37,7 @@ var _ httpFilter = &extAuth{}
 // patchHCM builds and appends the ext_authz Filters to the HTTP Connection Manager
 // if applicable, and it does not already exist.
 // Note: this method creates an ext_authz filter for each route that contains an ExtAuthz config.
+// The filter is disabled by default. It is enabled on the route level.
 // TODO: zhaohuabing avoid duplicated HTTP filters
 func (*extAuth) patchHCM(mgr *hcmv3.HttpConnectionManager, irListener *ir.HTTPListener) error {
 	var errs error
@@ -80,7 +80,8 @@ func buildHCMExtAuthFilter(route *ir.HTTPRoute) (*hcmv3.HttpFilter, error) {
 	}
 
 	return &hcmv3.HttpFilter{
-		Name: extAuthFilterName(route),
+		Name:     extAuthFilterName(route),
+		Disabled: true,
 		ConfigType: &hcmv3.HttpFilter_TypedConfig{
 			TypedConfig: extAuthAny,
 		},
@@ -244,52 +245,6 @@ func createExtServiceXDSCluster(rd *ir.RouteDestination, tCtx *types.ResourceVer
 	return nil
 }
 
-// patchRouteCfg patches the provided route configuration with the extAuth filter
-// if applicable.
-// Note: this method disables all the extAuth filters by default. The filter will
-// be enabled per-route in the typePerFilterConfig of the route.
-func (*extAuth) patchRouteConfig(routeCfg *routev3.RouteConfiguration, irListener *ir.HTTPListener) error {
-	if routeCfg == nil {
-		return errors.New("route configuration is nil")
-	}
-	if irListener == nil {
-		return errors.New("ir listener is nil")
-	}
-
-	var errs error
-	for _, route := range irListener.Routes {
-		if !routeContainsExtAuth(route) {
-			continue
-		}
-
-		filterName := extAuthFilterName(route)
-		filterCfg := routeCfg.TypedPerFilterConfig
-
-		if _, ok := filterCfg[filterName]; ok {
-			// This should not happen since this is the only place where the extAuth
-			// filter is added in a route.
-			errs = errors.Join(errs, fmt.Errorf(
-				"route config already contains extAuth config: %+v", route))
-			continue
-		}
-
-		// Disable all the filters by default. The filter will be enabled
-		// per-route in the typePerFilterConfig of the route.
-		routeCfgAny, err := anypb.New(&routev3.FilterConfig{Disabled: true})
-		if err != nil {
-			errs = errors.Join(errs, err)
-			continue
-		}
-
-		if filterCfg == nil {
-			routeCfg.TypedPerFilterConfig = make(map[string]*anypb.Any)
-		}
-
-		routeCfg.TypedPerFilterConfig[filterName] = routeCfgAny
-	}
-	return errs
-}
-
 // patchRoute patches the provided route with the extAuth config if applicable.
 // Note: this method enables the corresponding extAuth filter for the provided route.
 func (*extAuth) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {

diff --git a/internal/xds/translator/fault.go b/internal/xds/translator/fault.go
@@ -121,10 +121,6 @@ func (*fault) patchResources(*types.ResourceVersionTable, []*ir.HTTPRoute) error
 	return nil
 }
 
-func (*fault) patchRouteConfig(routeCfg *routev3.RouteConfiguration, irListener *ir.HTTPListener) error {
-	return nil
-}
-
 // patchRoute patches the provided route with the fault config if applicable.
 // Note: this method enables the corresponding fault filter for the provided route.
 func (*fault) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {