Remove duplicated http filters for ExtAuth (#2893)

* remove duplicated http filters Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * fix test Signed-off-by: huabing zhao <zhaohuabing@gmail.com> * address comments Signed-off-by: huabing zhao <zhaohuabing@gmail.com> --------- Signed-off-by: huabing zhao <zhaohuabing@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com>
envoyproxy · Mar 15, 2024 · 5679e41 · 5679e41
1 parent 62ecd29
commit 5679e41
Show file tree

Hide file tree

Showing 16 changed files with 155 additions and 38 deletions.
diff --git a/internal/gatewayapi/securitypolicy.go b/internal/gatewayapi/securitypolicy.go
@@ -378,7 +378,10 @@ func (t *Translator) translateSecurityPolicyForRoute(
 	}
 
 	if policy.Spec.ExtAuth != nil {
-		if extAuth, err = t.buildExtAuth(policy, resources); err != nil {
+		if extAuth, err = t.buildExtAuth(
+			utils.NamespacedName(route).String(),
+			policy,
+			resources); err != nil {
 			errs = errors.Join(errs, err)
 		}
 	}
@@ -458,7 +461,10 @@ func (t *Translator) translateSecurityPolicyForGateway(
 	}
 
 	if policy.Spec.ExtAuth != nil {
-		if extAuth, err = t.buildExtAuth(policy, resources); err != nil {
+		if extAuth, err = t.buildExtAuth(
+			utils.NamespacedName(gateway).String(),
+			policy,
+			resources); err != nil {
 			errs = errors.Join(errs, err)
 		}
 	}
@@ -793,6 +799,7 @@ func (t *Translator) buildBasicAuth(
 }
 
 func (t *Translator) buildExtAuth(
+	name string,
 	policy *egv1a1.SecurityPolicy,
 	resources *Resources) (*ir.ExtAuth, error) {
 	var (
@@ -845,6 +852,7 @@ func (t *Translator) buildExtAuth(
 	}
 
 	extAuth := &ir.ExtAuth{
+		Name:             name,
 		HeadersToExtAuth: policy.Spec.ExtAuth.HeadersToExtAuth,
 	}
 

diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth-with-backendtlspolicy.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth-with-backendtlspolicy.out.yaml
@@ -324,6 +324,7 @@ xdsIR:
           headersToExtAuth:
           - header1
           - header2
+          name: default/httproute-1
         hostname: www.foo.com
         isHTTP2: false
         name: httproute/default/httproute-1/rule/0/match/0/www_foo_com
@@ -364,6 +365,7 @@ xdsIR:
             - header1
             - header2
             path: /auth
+          name: default/gateway-1
         hostname: www.bar.com
         isHTTP2: false
         name: httproute/default/httproute-2/rule/0/match/0/www_bar_com

diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth.in.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth.in.yaml
@@ -29,10 +29,16 @@ httpRoutes:
       rules:
         - matches:
             - path:
-                value: /foo
+                value: /foo1
           backendRefs:
             - name: service-1
               port: 8080
+        - matches:
+            - path:
+                value: /foo2
+          backendRefs:
+            - name: service-2
+              port: 8080
   - apiVersion: gateway.networking.k8s.io/v1
     kind: HTTPRoute
     metadata:
@@ -50,7 +56,7 @@ httpRoutes:
             - path:
                 value: /bar
           backendRefs:
-            - name: service-1
+            - name: service-3
               port: 8080
 services:
   - apiVersion: v1

diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth.out.yaml
@@ -59,7 +59,13 @@ httpRoutes:
         port: 8080
       matches:
       - path:
-          value: /foo
+          value: /foo1
+    - backendRefs:
+      - name: service-2
+        port: 8080
+      matches:
+      - path:
+          value: /foo2
   status:
     parents:
     - conditions:
@@ -93,7 +99,7 @@ httpRoutes:
       sectionName: http
     rules:
     - backendRefs:
-      - name: service-1
+      - name: service-3
         port: 8080
       matches:
       - path:
@@ -253,13 +259,49 @@ xdsIR:
           headersToExtAuth:
           - header1
           - header2
+          name: default/httproute-1
         hostname: www.foo.com
         isHTTP2: false
         name: httproute/default/httproute-1/rule/0/match/0/www_foo_com
         pathMatch:
           distinct: false
           name: ""
-          prefix: /foo
+          prefix: /foo1
+      - backendWeights:
+          invalid: 0
+          valid: 0
+        destination:
+          name: httproute/default/httproute-1/rule/1
+          settings:
+          - addressType: IP
+            endpoints:
+            - host: 7.7.7.7
+              port: 8080
+            protocol: HTTP
+            weight: 1
+        extAuth:
+          grpc:
+            authority: grpc-backend.default:9000
+            destination:
+              name: securitypolicy/default/policy-for-http-route/grpc-backend
+              settings:
+              - addressType: IP
+                endpoints:
+                - host: 8.8.8.8
+                  port: 9000
+                protocol: GRPC
+                weight: 1
+          headersToExtAuth:
+          - header1
+          - header2
+          name: default/httproute-1
+        hostname: www.foo.com
+        isHTTP2: false
+        name: httproute/default/httproute-1/rule/1/match/0/www_foo_com
+        pathMatch:
+          distinct: false
+          name: ""
+          prefix: /foo2
       - backendWeights:
           invalid: 0
           valid: 0
@@ -288,6 +330,7 @@ xdsIR:
             - header1
             - header2
             path: /auth
+          name: default/gateway-1
         hostname: www.bar.com
         isHTTP2: false
         name: httproute/default/httproute-2/rule/0/match/0/www_bar_com

diff --git a/internal/ir/xds.go b/internal/ir/xds.go
@@ -575,6 +575,10 @@ type BasicAuth struct {
 //
 // +k8s:deepcopy-gen=true
 type ExtAuth struct {
+	// Name is a unique name for an ExtAuth configuration.
+	// The xds translator only generates one external authorization filter for each unique name.
+	Name string `json:"name" yaml:"name"`
+
 	// GRPC defines the gRPC External Authorization service.
 	// Only one of GRPCService or HTTPService may be specified.
 	GRPC *GRPCExtAuthService `json:"grpc,omitempty"`

diff --git a/internal/xds/translator/basicauth.go b/internal/xds/translator/basicauth.go
@@ -129,7 +129,8 @@ func (*basicAuth) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error
 	if irRoute.BasicAuth == nil {
 		return nil
 	}
-	if err := enableFilterOnRoute(basicAuthFilter, route, irRoute); err != nil {
+	filterName := basicAuthFilterName(irRoute)
+	if err := enableFilterOnRoute(route, filterName); err != nil {
 		return err
 	}
 	return nil

diff --git a/internal/xds/translator/extauth.go b/internal/xds/translator/extauth.go
@@ -55,7 +55,14 @@ func (*extAuth) patchHCM(mgr *hcmv3.HttpConnectionManager, irListener *ir.HTTPLi
 			continue
 		}
 
-		filter, err := buildHCMExtAuthFilter(route)
+		// Only generates one OAuth2 Envoy filter for each unique name.
+		// For example, if there are two routes under the same gateway with the
+		// same OIDC config, only one OAuth2 filter will be generated.
+		if hcmContainsFilter(mgr, extAuthFilterName(route.ExtAuth)) {
+			continue
+		}
+
+		filter, err := buildHCMExtAuthFilter(route.ExtAuth)
 		if err != nil {
 			errs = errors.Join(errs, err)
 			continue
@@ -68,8 +75,8 @@ func (*extAuth) patchHCM(mgr *hcmv3.HttpConnectionManager, irListener *ir.HTTPLi
 }
 
 // buildHCMExtAuthFilter returns an ext_authz HTTP filter from the provided IR HTTPRoute.
-func buildHCMExtAuthFilter(route *ir.HTTPRoute) (*hcmv3.HttpFilter, error) {
-	extAuthProto := extAuthConfig(route.ExtAuth)
+func buildHCMExtAuthFilter(extAuth *ir.ExtAuth) (*hcmv3.HttpFilter, error) {
+	extAuthProto := extAuthConfig(extAuth)
 	if err := extAuthProto.ValidateAll(); err != nil {
 		return nil, err
 	}
@@ -80,16 +87,16 @@ func buildHCMExtAuthFilter(route *ir.HTTPRoute) (*hcmv3.HttpFilter, error) {
 	}
 
 	return &hcmv3.HttpFilter{
-		Name:     extAuthFilterName(route),
+		Name:     extAuthFilterName(extAuth),
 		Disabled: true,
 		ConfigType: &hcmv3.HttpFilter_TypedConfig{
 			TypedConfig: extAuthAny,
 		},
 	}, nil
 }
 
-func extAuthFilterName(route *ir.HTTPRoute) string {
-	return perRouteFilterName(extAuthFilter, route.Name)
+func extAuthFilterName(extAuth *ir.ExtAuth) string {
+	return perRouteFilterName(extAuthFilter, extAuth.Name)
 }
 
 func extAuthConfig(extAuth *ir.ExtAuth) *extauthv3.ExtAuthz {
@@ -276,7 +283,8 @@ func (*extAuth) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
 	if irRoute.ExtAuth == nil {
 		return nil
 	}
-	if err := enableFilterOnRoute(extAuthFilter, route, irRoute); err != nil {
+	filterName := extAuthFilterName(irRoute.ExtAuth)
+	if err := enableFilterOnRoute(route, filterName); err != nil {
 		return err
 	}
 	return nil

diff --git a/internal/xds/translator/oidc.go b/internal/xds/translator/oidc.go
@@ -94,7 +94,7 @@ func buildHCMOAuth2Filter(route *ir.HTTPRoute) (*hcmv3.HttpFilter, error) {
 }
 
 func oauth2FilterName(route *ir.HTTPRoute) string {
-	return fmt.Sprintf("%s_%s", oauth2Filter, route.Name)
+	return perRouteFilterName(oauth2Filter, route.Name)
 }
 
 func oauth2Config(route *ir.HTTPRoute) (*oauth2v3.OAuth2, error) {
@@ -340,8 +340,8 @@ func (*oidc) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
 	if irRoute.OIDC == nil {
 		return nil
 	}
-
-	if err := enableFilterOnRoute(oauth2Filter, route, irRoute); err != nil {
+	filterName := oauth2FilterName(irRoute)
+	if err := enableFilterOnRoute(route, filterName); err != nil {
 		return err
 	}
 	return nil

diff --git a/internal/xds/translator/testdata/in/xds-ir/ext-auth.yaml b/internal/xds/translator/testdata/in/xds-ir/ext-auth.yaml
@@ -19,6 +19,34 @@ http:
         - host: "10.0.0.1"
           port: 50000
     extAuth:
+      name: default/httproute-1
+      http:
+        authority: http-backend.envoy-gateway:80
+        headersToBackend:
+        - header1
+        - header2
+        path: /auth
+        destination:
+          name: securitypolicy/default/policy-for-first-route/http-backend
+          settings:
+          - addressType: IP
+            endpoints:
+            - host: 7.7.7.7
+              port: 80
+            protocol: HTTP
+            weight: 1
+  - name: httproute/default/httproute-1/rule/1/match/0/www_example_com
+    hostname: "*"
+    pathMatch:
+      exact: "foo"
+    destination:
+      name: httproute/default/httproute-1/rule/0
+      settings:
+      - endpoints:
+        - host: "10.0.0.1"
+          port: 50000
+    extAuth:
+      name: default/httproute-1
       http:
         authority: http-backend.envoy-gateway:80
         headersToBackend:
@@ -45,6 +73,7 @@ http:
         - host: "10.0.0.2"
           port: 60000
     extAuth:
+      name: default/gateway-1
       grpc:
         authority: grpc-backend.default:9000
         destination:

diff --git a/internal/xds/translator/testdata/out/xds-ir/basic-auth.listeners.yaml b/internal/xds/translator/testdata/out/xds-ir/basic-auth.listeners.yaml
@@ -15,7 +15,7 @@
           maxConcurrentStreams: 100
         httpFilters:
         - disabled: true
-          name: envoy.filters.http.basic_auth_first-route
+          name: envoy.filters.http.basic_auth/first-route
           typedConfig:
             '@type': type.googleapis.com/envoy.extensions.filters.http.basic_auth.v3.BasicAuth
             users:

diff --git a/internal/xds/translator/testdata/out/xds-ir/basic-auth.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/basic-auth.routes.yaml
@@ -13,6 +13,6 @@
         upgradeConfigs:
         - upgradeType: websocket
       typedPerFilterConfig:
-        envoy.filters.http.basic_auth_first-route:
+        envoy.filters.http.basic_auth/first-route:
           '@type': type.googleapis.com/envoy.config.route.v3.FilterConfig
           config: {}
diff --git a/internal/xds/translator/testdata/out/xds-ir/ext-auth.listeners.yaml b/internal/xds/translator/testdata/out/xds-ir/ext-auth.listeners.yaml
@@ -15,7 +15,7 @@
           maxConcurrentStreams: 100
         httpFilters:
         - disabled: true
-          name: envoy.filters.http.ext_authz_httproute/default/httproute-1/rule/0/match/0/www_example_com
+          name: envoy.filters.http.ext_authz/default/httproute-1
           typedConfig:
             '@type': type.googleapis.com/envoy.extensions.filters.http.ext_authz.v3.ExtAuthz
             httpService:
@@ -30,7 +30,7 @@
                 uri: http://http-backend.envoy-gateway:80/auth
             transportApiVersion: V3
         - disabled: true
-          name: envoy.filters.http.ext_authz_httproute/default/httproute-2/rule/0/match/0/www_example_com
+          name: envoy.filters.http.ext_authz/default/gateway-1
           typedConfig:
             '@type': type.googleapis.com/envoy.extensions.filters.http.ext_authz.v3.ExtAuthz
             allowedHeaders:

diff --git a/internal/xds/translator/testdata/out/xds-ir/ext-auth.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/ext-auth.routes.yaml
@@ -13,7 +13,18 @@
         upgradeConfigs:
         - upgradeType: websocket
       typedPerFilterConfig:
-        envoy.filters.http.ext_authz_httproute/default/httproute-1/rule/0/match/0/www_example_com:
+        envoy.filters.http.ext_authz/default/httproute-1:
+          '@type': type.googleapis.com/envoy.config.route.v3.FilterConfig
+          config: {}
+    - match:
+        path: foo
+      name: httproute/default/httproute-1/rule/1/match/0/www_example_com
+      route:
+        cluster: httproute/default/httproute-1/rule/0
+        upgradeConfigs:
+        - upgradeType: websocket
+      typedPerFilterConfig:
+        envoy.filters.http.ext_authz/default/httproute-1:
           '@type': type.googleapis.com/envoy.config.route.v3.FilterConfig
           config: {}
     - match:
@@ -24,6 +35,6 @@
         upgradeConfigs:
         - upgradeType: websocket
       typedPerFilterConfig:
-        envoy.filters.http.ext_authz_httproute/default/httproute-2/rule/0/match/0/www_example_com:
+        envoy.filters.http.ext_authz/default/gateway-1:
           '@type': type.googleapis.com/envoy.config.route.v3.FilterConfig
           config: {}
diff --git a/internal/xds/translator/testdata/out/xds-ir/oidc.listeners.yaml b/internal/xds/translator/testdata/out/xds-ir/oidc.listeners.yaml
@@ -15,7 +15,7 @@
           maxConcurrentStreams: 100
         httpFilters:
         - disabled: true
-          name: envoy.filters.http.oauth2_first-route
+          name: envoy.filters.http.oauth2/first-route
           typedConfig:
             '@type': type.googleapis.com/envoy.extensions.filters.http.oauth2.v3.OAuth2
             config:
@@ -56,7 +56,7 @@
                 timeout: 10s
                 uri: https://oauth.foo.com/token
         - disabled: true
-          name: envoy.filters.http.oauth2_second-route
+          name: envoy.filters.http.oauth2/second-route
           typedConfig:
             '@type': type.googleapis.com/envoy.extensions.filters.http.oauth2.v3.OAuth2
             config: