fix: translator reports errors for existing clusters and secretes (en…

…voyproxy#4707) * fix: existing clusters and secretes Signed-off-by: Huabing Zhao <[email protected]> * fix cluster index for SP Signed-off-by: Huabing Zhao <[email protected]> * minor change Signed-off-by: Huabing Zhao <[email protected]> * minor change Signed-off-by: Huabing Zhao <[email protected]> * minor change Signed-off-by: Huabing Zhao <[email protected]> * minor change Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> * add comment Signed-off-by: Huabing Zhao <[email protected]> * remove index Signed-off-by: Huabing Zhao <[email protected]> * fix lint Signed-off-by: Huabing Zhao <[email protected]> --------- Signed-off-by: Huabing Zhao <[email protected]>
zhaohuabing · Nov 22, 2024 · 067efab · 067efab
1 parent 0d3d455
commit 067efab
Show file tree

Hide file tree

Showing 29 changed files with 403 additions and 90 deletions.
diff --git a/internal/gatewayapi/envoyextensionpolicy.go b/internal/gatewayapi/envoyextensionpolicy.go
@@ -434,7 +434,7 @@ func (t *Translator) buildExtProc(
 		err       error
 	)
 
-	if rd, err = t.translateExtServiceBackendRefs(policy, extProc.BackendRefs, ir.GRPC, resources, envoyProxy, extProcIdx); err != nil {
+	if rd, err = t.translateExtServiceBackendRefs(policy, extProc.BackendRefs, ir.GRPC, resources, envoyProxy, "extproc", extProcIdx); err != nil {
 		return nil, err
 	}
 

diff --git a/internal/gatewayapi/ext_service.go b/internal/gatewayapi/ext_service.go
@@ -29,6 +29,7 @@ func (t *Translator) translateExtServiceBackendRefs(
 	protocol ir.AppProtocol,
 	resources *resource.Resources,
 	envoyProxy *egv1a1.EnvoyProxy,
+	configType string,
 	index int, // index is used to differentiate between multiple external services in the same policy
 ) (*ir.RouteDestination, error) {
 	var (
@@ -66,7 +67,7 @@ func (t *Translator) translateExtServiceBackendRefs(
 	}
 
 	rs = &ir.RouteDestination{
-		Name:     irIndexedExtServiceDestinationName(pnn, policy.GetObjectKind().GroupVersionKind().Kind, index),
+		Name:     irIndexedExtServiceDestinationName(pnn, policy.GetObjectKind().GroupVersionKind().Kind, configType, index),
 		Settings: ds,
 	}
 	return rs, nil
@@ -139,12 +140,12 @@ func (t *Translator) processExtServiceDestination(
 	return ds, nil
 }
 
-// TODO: also refer to extension type, as Wasm may also introduce destinations
-func irIndexedExtServiceDestinationName(policyNamespacedName types.NamespacedName, policyKind string, idx int) string {
+func irIndexedExtServiceDestinationName(policyNamespacedName types.NamespacedName, policyKind string, configType string, idx int) string {
 	return strings.ToLower(fmt.Sprintf(
-		"%s/%s/%s/%d",
+		"%s/%s/%s/%s/%d",
 		policyKind,
 		policyNamespacedName.Namespace,
 		policyNamespacedName.Name,
+		configType,
 		idx))
 }
diff --git a/internal/gatewayapi/securitypolicy.go b/internal/gatewayapi/securitypolicy.go
@@ -373,8 +373,7 @@ func (t *Translator) translateSecurityPolicyForRoute(
 			if extAuth, err = t.buildExtAuth(
 				policy,
 				resources,
-				gtwCtx.envoyProxy,
-			); err != nil {
+				gtwCtx.envoyProxy); err != nil {
 				err = perr.WithMessage(err, "ExtAuth")
 				errs = errors.Join(errs, err)
 			}
@@ -385,7 +384,7 @@ func (t *Translator) translateSecurityPolicyForRoute(
 			if oidc, err = t.buildOIDC(
 				policy,
 				resources,
-				gtwCtx.envoyProxy); err != nil {
+				gtwCtx.envoyProxy); err != nil { // TODO zhaohuabing: Only the last EnvoyProxy is used
 				err = perr.WithMessage(err, "OIDC")
 				errs = errors.Join(errs, err)
 			}
@@ -468,8 +467,7 @@ func (t *Translator) translateSecurityPolicyForGateway(
 		if extAuth, err = t.buildExtAuth(
 			policy,
 			resources,
-			gateway.envoyProxy,
-		); err != nil {
+			gateway.envoyProxy); err != nil {
 			err = perr.WithMessage(err, "ExtAuth")
 			errs = errors.Join(errs, err)
 		}
@@ -705,7 +703,7 @@ func (t *Translator) buildOIDCProvider(policy *egv1a1.SecurityPolicy, resources
 	}
 
 	if len(provider.BackendRefs) > 0 {
-		if rd, err = t.translateExtServiceBackendRefs(policy, provider.BackendRefs, protocol, resources, envoyProxy, 0); err != nil {
+		if rd, err = t.translateExtServiceBackendRefs(policy, provider.BackendRefs, protocol, resources, envoyProxy, "oidc", 0); err != nil {
 			return nil, err
 		}
 	}
@@ -839,7 +837,11 @@ func (t *Translator) buildBasicAuth(
 	}, nil
 }
 
-func (t *Translator) buildExtAuth(policy *egv1a1.SecurityPolicy, resources *resource.Resources, envoyProxy *egv1a1.EnvoyProxy) (*ir.ExtAuth, error) {
+func (t *Translator) buildExtAuth(
+	policy *egv1a1.SecurityPolicy,
+	resources *resource.Resources,
+	envoyProxy *egv1a1.EnvoyProxy,
+) (*ir.ExtAuth, error) {
 	var (
 		http            = policy.Spec.ExtAuth.HTTP
 		grpc            = policy.Spec.ExtAuth.GRPC
@@ -893,7 +895,7 @@ func (t *Translator) buildExtAuth(policy *egv1a1.SecurityPolicy, resources *reso
 		}
 	}
 
-	if rd, err = t.translateExtServiceBackendRefs(policy, backendRefs, protocol, resources, envoyProxy, 0); err != nil {
+	if rd, err = t.translateExtServiceBackendRefs(policy, backendRefs, protocol, resources, envoyProxy, "extauth", 0); err != nil {
 		return nil, err
 	}
 

diff --git a/internal/gatewayapi/testdata/envoyextensionpolicy-override-replace.out.yaml b/internal/gatewayapi/testdata/envoyextensionpolicy-override-replace.out.yaml
@@ -296,7 +296,7 @@ xdsIR:
           extProcs:
           - authority: grpc-backend-2.default:8000
             destination:
-              name: envoyextensionpolicy/default/policy-for-route-1/0
+              name: envoyextensionpolicy/default/policy-for-route-1/extproc/0
               settings:
               - protocol: GRPC
                 weight: 1
@@ -325,7 +325,7 @@ xdsIR:
           extProcs:
           - authority: grpc-backend.envoy-gateway:9000
             destination:
-              name: envoyextensionpolicy/envoy-gateway/policy-for-gateway-1/0
+              name: envoyextensionpolicy/envoy-gateway/policy-for-gateway-1/extproc/0
               settings:
               - protocol: GRPC
                 weight: 1

diff --git a/...rnal/gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-backendtlspolicy.out.yaml b/...rnal/gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-backendtlspolicy.out.yaml
@@ -308,7 +308,7 @@ xdsIR:
           extProcs:
           - authority: grpc-backend-2.default:9000
             destination:
-              name: envoyextensionpolicy/default/policy-for-http-route/0
+              name: envoyextensionpolicy/default/policy-for-http-route/extproc/0
               settings:
               - addressType: IP
                 endpoints:
@@ -349,7 +349,7 @@ xdsIR:
           extProcs:
           - authority: grpc-backend.envoy-gateway:8000
             destination:
-              name: envoyextensionpolicy/default/policy-for-gateway/0
+              name: envoyextensionpolicy/default/policy-for-gateway/extproc/0
               settings:
               - addressType: IP
                 protocol: GRPC

diff --git a/.../gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-multiple-backendrefs.out.yaml b/.../gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-multiple-backendrefs.out.yaml
@@ -308,7 +308,7 @@ xdsIR:
           extProcs:
           - authority: grpc-backend.envoy-gateway:8000
             destination:
-              name: envoyextensionpolicy/default/policy-for-http-route/0
+              name: envoyextensionpolicy/default/policy-for-http-route/extproc/0
               settings:
               - addressType: IP
                 protocol: GRPC

diff --git a/...rnal/gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-traffic-features.out.yaml b/...rnal/gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-traffic-features.out.yaml
@@ -337,7 +337,7 @@ xdsIR:
           extProcs:
           - authority: grpc-backend.envoy-gateway:8000
             destination:
-              name: envoyextensionpolicy/default/policy-for-http-route/0
+              name: envoyextensionpolicy/default/policy-for-http-route/extproc/0
               settings:
               - addressType: IP
                 protocol: GRPC

diff --git a/internal/gatewayapi/testdata/envoyproxy-priority-backend.out.yaml b/internal/gatewayapi/testdata/envoyproxy-priority-backend.out.yaml
@@ -311,7 +311,7 @@ xdsIR:
           extProcs:
           - authority: grpc-backend.envoy-gateway:8000
             destination:
-              name: envoyextensionpolicy/default/policy-for-http-route/0
+              name: envoyextensionpolicy/default/policy-for-http-route/extproc/0
               settings:
               - addressType: IP
                 protocol: GRPC

diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth-backend.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth-backend.out.yaml
@@ -390,7 +390,7 @@ xdsIR:
             grpc:
               authority: service-2.default:8080
               destination:
-                name: securitypolicy/default/policy-for-http-route-1/0
+                name: securitypolicy/default/policy-for-http-route-1/extauth/0
                 settings:
                 - addressType: IP
                   endpoints:
@@ -434,7 +434,7 @@ xdsIR:
             grpc:
               authority: service-2.default:8080
               destination:
-                name: securitypolicy/default/policy-for-http-route-1/0
+                name: securitypolicy/default/policy-for-http-route-1/extauth/0
                 settings:
                 - addressType: IP
                   endpoints:
@@ -498,7 +498,7 @@ xdsIR:
             grpc:
               authority: service-2.default:8080
               destination:
-                name: securitypolicy/default/policy-for-http-route-3--grpc-backendref/0
+                name: securitypolicy/default/policy-for-http-route-3--grpc-backendref/extauth/0
                 settings:
                 - addressType: IP
                   endpoints:
@@ -532,7 +532,7 @@ xdsIR:
             http:
               authority: primary.foo.com:3000
               destination:
-                name: securitypolicy/default/policy-for-http-route-3-http-backendref/0
+                name: securitypolicy/default/policy-for-http-route-3-http-backendref/extauth/0
                 settings:
                 - addressType: FQDN
                   endpoints:

diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth-backendref.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth-backendref.out.yaml
@@ -263,7 +263,7 @@ xdsIR:
             grpc:
               authority: grpc-backend.default:9000
               destination:
-                name: securitypolicy/default/policy-for-http-route-1/0
+                name: securitypolicy/default/policy-for-http-route-1/extauth/0
                 settings:
                 - addressType: IP
                   endpoints:
@@ -301,7 +301,7 @@ xdsIR:
             grpc:
               authority: grpc-backend.default:9000
               destination:
-                name: securitypolicy/default/policy-for-http-route-1/0
+                name: securitypolicy/default/policy-for-http-route-1/extauth/0
                 settings:
                 - addressType: IP
                   endpoints:
@@ -339,7 +339,7 @@ xdsIR:
             http:
               authority: http-backend.envoy-gateway:80
               destination:
-                name: securitypolicy/default/policy-for-gateway-1/0
+                name: securitypolicy/default/policy-for-gateway-1/extauth/0
                 settings:
                 - addressType: IP
                   endpoints:

diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth-recomputation.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth-recomputation.out.yaml
@@ -246,7 +246,7 @@ xdsIR:
             grpc:
               authority: service-2.default:8080
               destination:
-                name: securitypolicy/default/policy-for-http-route-1/0
+                name: securitypolicy/default/policy-for-http-route-1/extauth/0
                 settings:
                 - addressType: IP
                   endpoints:
@@ -291,7 +291,7 @@ xdsIR:
             grpc:
               authority: service-2.default:8080
               destination:
-                name: securitypolicy/default/policy-for-http-route-1/0
+                name: securitypolicy/default/policy-for-http-route-1/extauth/0
                 settings:
                 - addressType: IP
                   endpoints:

diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth-with-backendtlspolicy.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth-with-backendtlspolicy.out.yaml
@@ -322,7 +322,7 @@ xdsIR:
             grpc:
               authority: grpc-backend.default:9000
               destination:
-                name: securitypolicy/default/policy-for-http-route/0
+                name: securitypolicy/default/policy-for-http-route/extauth/0
                 settings:
                 - addressType: IP
                   endpoints:
@@ -366,7 +366,7 @@ xdsIR:
             http:
               authority: http-backend.envoy-gateway:80
               destination:
-                name: securitypolicy/default/policy-for-gateway/0
+                name: securitypolicy/default/policy-for-gateway/extauth/0
                 settings:
                 - addressType: IP
                   endpoints:

diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth.out.yaml
@@ -263,7 +263,7 @@ xdsIR:
             grpc:
               authority: grpc-backend.default:9000
               destination:
-                name: securitypolicy/default/policy-for-http-route-1/0
+                name: securitypolicy/default/policy-for-http-route-1/extauth/0
                 settings:
                 - addressType: IP
                   endpoints:
@@ -301,7 +301,7 @@ xdsIR:
             grpc:
               authority: grpc-backend.default:9000
               destination:
-                name: securitypolicy/default/policy-for-http-route-1/0
+                name: securitypolicy/default/policy-for-http-route-1/extauth/0
                 settings:
                 - addressType: IP
                   endpoints:
@@ -339,7 +339,7 @@ xdsIR:
             http:
               authority: http-backend.envoy-gateway:80
               destination:
-                name: securitypolicy/default/policy-for-gateway-1/0
+                name: securitypolicy/default/policy-for-gateway-1/extauth/0
                 settings:
                 - addressType: IP
                   endpoints:

diff --git a/internal/gatewayapi/testdata/securitypolicy-with-oidc-backendcluster.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-oidc-backendcluster.out.yaml
@@ -228,7 +228,7 @@ xdsIR:
             provider:
               authorizationEndpoint: https://oauth.foo.com/oauth2/v2/auth
               destination:
-                name: securitypolicy/envoy-gateway/policy-for-gateway/0
+                name: securitypolicy/envoy-gateway/policy-for-gateway/oidc/0
                 settings:
                 - addressType: FQDN
                   endpoints:

diff --git a/internal/xds/translator/accesslog.go b/internal/xds/translator/accesslog.go
@@ -6,7 +6,6 @@
 package translator
 
 import (
-	"errors"
 	"sort"
 	"strings"
 
@@ -545,7 +544,7 @@ func processClusterForAccessLog(tCtx *types.ResourceVersionTable, al *ir.AccessL
 			backendConnection: traffic.BackendConnection,
 			dns:               traffic.DNS,
 			http2Settings:     traffic.HTTP2,
-		}); err != nil && !errors.Is(err, ErrXdsClusterExists) {
+		}); err != nil {
 			return err
 		}
 	}
@@ -573,7 +572,7 @@ func processClusterForAccessLog(tCtx *types.ResourceVersionTable, al *ir.AccessL
 			backendConnection: traffic.BackendConnection,
 			dns:               traffic.DNS,
 			http2Settings:     traffic.HTTP2,
-		}); err != nil && !errors.Is(err, ErrXdsClusterExists) {
+		}); err != nil {
 			return err
 		}
 	}

diff --git a/internal/xds/translator/extauth.go b/internal/xds/translator/extauth.go
@@ -226,14 +226,12 @@ func (*extAuth) patchResources(tCtx *types.ResourceVersionTable,
 		}
 		if route.Security.ExtAuth.HTTP != nil {
 			if err := createExtServiceXDSCluster(
-				&route.Security.ExtAuth.HTTP.Destination, route.Security.ExtAuth.Traffic, tCtx); err != nil && !errors.Is(
-				err, ErrXdsClusterExists) {
+				&route.Security.ExtAuth.HTTP.Destination, route.Security.ExtAuth.Traffic, tCtx); err != nil {
 				errs = errors.Join(errs, err)
 			}
 		} else {
 			if err := createExtServiceXDSCluster(
-				&route.Security.ExtAuth.GRPC.Destination, route.Security.ExtAuth.Traffic, tCtx); err != nil && !errors.Is(
-				err, ErrXdsClusterExists) {
+				&route.Security.ExtAuth.GRPC.Destination, route.Security.ExtAuth.Traffic, tCtx); err != nil {
 				errs = errors.Join(errs, err)
 			}
 		}

diff --git a/internal/xds/translator/extproc.go b/internal/xds/translator/extproc.go
@@ -173,8 +173,7 @@ func (*extProc) patchResources(tCtx *types.ResourceVersionTable,
 		for i := range route.EnvoyExtensions.ExtProcs {
 			ep := route.EnvoyExtensions.ExtProcs[i]
 			if err := createExtServiceXDSCluster(
-				&ep.Destination, ep.Traffic, tCtx); err != nil && !errors.Is(
-				err, ErrXdsClusterExists) {
+				&ep.Destination, ep.Traffic, tCtx); err != nil {
 				errs = errors.Join(errs, err)
 			}
 		}

diff --git a/internal/xds/translator/oidc.go b/internal/xds/translator/oidc.go
@@ -310,8 +310,7 @@ func createOAuthServerClusters(tCtx *types.ResourceVersionTable,
 		// If the OIDC provider has a destination, use it.
 		if oidc.Provider.Destination != nil && len(oidc.Provider.Destination.Settings) > 0 {
 			if err := createExtServiceXDSCluster(
-				oidc.Provider.Destination, oidc.Provider.Traffic, tCtx); err != nil && !errors.Is(
-				err, ErrXdsClusterExists) {
+				oidc.Provider.Destination, oidc.Provider.Traffic, tCtx); err != nil {
 				errs = errors.Join(errs, err)
 			}
 		} else {
@@ -372,11 +371,7 @@ func createOAuth2TokenEndpointCluster(tCtx *types.ResourceVersionTable,
 		clusterArgs.tSocket = tSocket
 	}
 
-	if err = addXdsCluster(tCtx, clusterArgs); err != nil && !errors.Is(err, ErrXdsClusterExists) {
-		return err
-	}
-
-	return err
+	return addXdsCluster(tCtx, clusterArgs)
 }
 
 // createOAuth2Secrets creates OAuth2 client and HMAC secrets from the provided