From 067efaba4663773d118765222154e2785e15b06a Mon Sep 17 00:00:00 2001 From: Huabing Zhao Date: Wed, 20 Nov 2024 10:24:31 +0800 Subject: [PATCH] fix: translator reports errors for existing clusters and secretes (#4707) * fix: existing clusters and secretes Signed-off-by: Huabing Zhao * fix cluster index for SP Signed-off-by: Huabing Zhao * minor change Signed-off-by: Huabing Zhao * minor change Signed-off-by: Huabing Zhao * minor change Signed-off-by: Huabing Zhao * minor change Signed-off-by: Huabing Zhao * fix lint Signed-off-by: Huabing Zhao * add comment Signed-off-by: Huabing Zhao * remove index Signed-off-by: Huabing Zhao * fix lint Signed-off-by: Huabing Zhao --------- Signed-off-by: Huabing Zhao --- internal/gatewayapi/envoyextensionpolicy.go | 2 +- internal/gatewayapi/ext_service.go | 9 +- internal/gatewayapi/securitypolicy.go | 18 +-- ...yextensionpolicy-override-replace.out.yaml | 4 +- ...ith-extproc-with-backendtlspolicy.out.yaml | 4 +- ...extproc-with-multiple-backendrefs.out.yaml | 2 +- ...ith-extproc-with-traffic-features.out.yaml | 2 +- .../envoyproxy-priority-backend.out.yaml | 2 +- ...curitypolicy-with-extauth-backend.out.yaml | 8 +- ...itypolicy-with-extauth-backendref.out.yaml | 6 +- ...policy-with-extauth-recomputation.out.yaml | 4 +- ...ith-extauth-with-backendtlspolicy.out.yaml | 4 +- .../securitypolicy-with-extauth.out.yaml | 6 +- ...typolicy-with-oidc-backendcluster.out.yaml | 2 +- internal/xds/translator/accesslog.go | 5 +- internal/xds/translator/extauth.go | 6 +- internal/xds/translator/extproc.go | 3 +- internal/xds/translator/oidc.go | 9 +- internal/xds/translator/ratelimit.go | 9 +- .../securitypolicy-with-oidc-jwt-authz.yaml | 80 +++++++++++++ ...typolicy-with-oidc-jwt-authz.clusters.yaml | 54 +++++++++ ...ypolicy-with-oidc-jwt-authz.endpoints.yaml | 12 ++ ...ypolicy-with-oidc-jwt-authz.listeners.yaml | 107 ++++++++++++++++++ ...ritypolicy-with-oidc-jwt-authz.routes.yaml | 74 ++++++++++++ ...itypolicy-with-oidc-jwt-authz.secrets.yaml | 8 ++ internal/xds/translator/tracing.go | 8 +- internal/xds/translator/translator.go | 31 +++-- internal/xds/translator/utils.go | 13 +-- release-notes/current.yaml | 1 + 29 files changed, 403 insertions(+), 90 deletions(-) create mode 100644 internal/xds/translator/testdata/in/xds-ir/securitypolicy-with-oidc-jwt-authz.yaml create mode 100644 internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.clusters.yaml create mode 100644 internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.endpoints.yaml create mode 100644 internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.listeners.yaml create mode 100644 internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.routes.yaml create mode 100644 internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.secrets.yaml diff --git a/internal/gatewayapi/envoyextensionpolicy.go b/internal/gatewayapi/envoyextensionpolicy.go index 9ba561f1b5d..baf0c529d93 100644 --- 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 index e7ab19036ee..39bd5aebe47 100644 --- 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 index 8635d216457..3219f816da5 100644 --- 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 index 4f055e7bc4d..2c6b006af93 100644 --- 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/internal/gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-backendtlspolicy.out.yaml b/internal/gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-backendtlspolicy.out.yaml index 6b9ad5ee281..a1d7beec90b 100644 --- a/internal/gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-backendtlspolicy.out.yaml +++ b/internal/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/internal/gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-multiple-backendrefs.out.yaml b/internal/gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-multiple-backendrefs.out.yaml index 021ae6a2cd5..a81a7cd4410 100644 --- a/internal/gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-multiple-backendrefs.out.yaml +++ b/internal/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/internal/gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-traffic-features.out.yaml b/internal/gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-traffic-features.out.yaml index 4edde355292..21fb5de6103 100644 --- a/internal/gatewayapi/testdata/envoyextensionpolicy-with-extproc-with-traffic-features.out.yaml +++ b/internal/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 index fda9d4ccca9..426268f6340 100644 --- 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 index d304f6c13eb..ccdb2458370 100644 --- 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 index 905b81b3cba..d72cd182896 100644 --- 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 index 94012ec739a..350fc8e908b 100644 --- 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 index 7c4b2ce2739..b87c7992c90 100644 --- 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 index 905b81b3cba..d72cd182896 100644 --- 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 index d6f0c4dbc47..0fc57e6735a 100644 --- 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 index 076eb659d83..265e3ed8a9c 100644 --- 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 index 7d7cc6a7227..2f8766fe91c 100644 --- 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 index 2bc6c4b6ba6..57cc9634d09 100644 --- 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 index a706cae662f..c51bbd75499 100644 --- 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 diff --git a/internal/xds/translator/ratelimit.go b/internal/xds/translator/ratelimit.go index 06b37bc4589..eb6a1c4a2cd 100644 --- a/internal/xds/translator/ratelimit.go +++ b/internal/xds/translator/ratelimit.go @@ -7,7 +7,6 @@ package translator import ( "bytes" - "errors" "net/url" "strconv" "strings" @@ -492,17 +491,13 @@ func (t *Translator) createRateLimitServiceCluster(tCtx *types.ResourceVersionTa return err } - if err := addXdsCluster(tCtx, &xdsClusterArgs{ + return addXdsCluster(tCtx, &xdsClusterArgs{ name: clusterName, settings: []*ir.DestinationSetting{ds}, tSocket: tSocket, endpointType: EndpointTypeDNS, metrics: metrics, - }); err != nil && !errors.Is(err, ErrXdsClusterExists) { - return err - } - - return nil + }) } func getRouteRuleDescriptor(ruleIndex, matchIndex int) string { diff --git a/internal/xds/translator/testdata/in/xds-ir/securitypolicy-with-oidc-jwt-authz.yaml b/internal/xds/translator/testdata/in/xds-ir/securitypolicy-with-oidc-jwt-authz.yaml new file mode 100644 index 00000000000..0c0d820db63 --- /dev/null +++ b/internal/xds/translator/testdata/in/xds-ir/securitypolicy-with-oidc-jwt-authz.yaml @@ -0,0 +1,80 @@ +http: +- name: "envoy-gateway/gateway-1/http" + address: 0.0.0.0 + hostnames: + - '*' + isHTTP2: false + metadata: + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + sectionName: http + path: + escapedSlashesAction: UnescapeAndRedirect + mergeSlashes: true + port: 10080 + routes: + - destination: + name: httproute/default/httproute-1/rule/0 + settings: + - addressType: IP + endpoints: + - host: 7.7.7.7 + port: 8080 + protocol: HTTP + weight: 1 + hostname: www.example.com + isHTTP2: false + metadata: + kind: HTTPRoute + name: httproute-1 + namespace: default + name: httproute/default/httproute-1/rule/0/match/0/www_example_com + pathMatch: + distinct: false + name: "" + prefix: /foo + security: + authorization: + defaultAction: Deny + rules: + - action: Allow + name: allow + principal: + jwt: + claims: + - name: groups + valueType: StringArray + values: + - foobar + provider: exjwt + jwt: + providers: + - claimToHeaders: + - claim: email + header: x-user-email + extractFrom: + cookies: + - IdToken + issuer: https://oidc.example.com/auth/realms/example + name: exjwt + remoteJWKS: + uri: https://oidc.example.com/auth/realms/example/protocol/openid-connect/certs + oidc: + clientID: prometheus + clientSecret: 'qrOYACHXoe7UEDI/raOjNSx+Z9ufXSc/22C3T6X/zPY=' + cookieNameOverrides: + idToken: IdToken + cookieSuffix: 5f93c2e4 + hmacSecret: 'qrOYACHXoe7UEDI/raOjNSx+Z9ufXSc/22C3T6X/zPY=' + logoutPath: /logout + name: securitypolicy/default/policy-for-http-route + provider: + authorizationEndpoint: https://oidc.example.com/authorize + tokenEndpoint: https://oidc.example.com/oauth/token + redirectPath: /oauth2/callback + redirectURL: '%REQ(x-forwarded-proto)%://%REQ(:authority)%/oauth2/callback' + scopes: + - openid + - email + - profile diff --git a/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.clusters.yaml b/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.clusters.yaml new file mode 100644 index 00000000000..1535201f87b --- /dev/null +++ b/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.clusters.yaml @@ -0,0 +1,54 @@ +- circuitBreakers: + thresholds: + - maxRetries: 1024 + commonLbConfig: + localityWeightedLbConfig: {} + connectTimeout: 10s + dnsLookupFamily: V4_ONLY + edsClusterConfig: + edsConfig: + ads: {} + resourceApiVersion: V3 + serviceName: httproute/default/httproute-1/rule/0 + ignoreHealthOnHostRemoval: true + lbPolicy: LEAST_REQUEST + name: httproute/default/httproute-1/rule/0 + outlierDetection: {} + perConnectionBufferLimitBytes: 32768 + type: EDS +- circuitBreakers: + thresholds: + - maxRetries: 1024 + commonLbConfig: + localityWeightedLbConfig: {} + connectTimeout: 10s + dnsLookupFamily: V4_ONLY + dnsRefreshRate: 30s + lbPolicy: LEAST_REQUEST + loadAssignment: + clusterName: oidc_example_com_443 + endpoints: + - lbEndpoints: + - endpoint: + address: + socketAddress: + address: oidc.example.com + portValue: 443 + loadBalancingWeight: 1 + loadBalancingWeight: 1 + locality: + region: oidc_example_com_443/backend/0 + name: oidc_example_com_443 + outlierDetection: {} + perConnectionBufferLimitBytes: 32768 + respectDnsTtl: true + transportSocket: + name: envoy.transport_sockets.tls + typedConfig: + '@type': type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext + commonTlsContext: + validationContext: + trustedCa: + filename: /etc/ssl/certs/ca-certificates.crt + sni: oidc.example.com + type: STRICT_DNS diff --git a/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.endpoints.yaml b/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.endpoints.yaml new file mode 100644 index 00000000000..29bb6b4e444 --- /dev/null +++ b/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.endpoints.yaml @@ -0,0 +1,12 @@ +- clusterName: httproute/default/httproute-1/rule/0 + endpoints: + - lbEndpoints: + - endpoint: + address: + socketAddress: + address: 7.7.7.7 + portValue: 8080 + loadBalancingWeight: 1 + loadBalancingWeight: 1 + locality: + region: httproute/default/httproute-1/rule/0/backend/0 diff --git a/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.listeners.yaml b/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.listeners.yaml new file mode 100644 index 00000000000..ada9749df63 --- /dev/null +++ b/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.listeners.yaml @@ -0,0 +1,107 @@ +- 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: + - disabled: true + name: envoy.filters.http.oauth2/securitypolicy/default/policy-for-http-route + typedConfig: + '@type': type.googleapis.com/envoy.extensions.filters.http.oauth2.v3.OAuth2 + config: + authScopes: + - openid + - email + - profile + authType: BASIC_AUTH + authorizationEndpoint: https://oidc.example.com/authorize + credentials: + clientId: prometheus + cookieNames: + bearerToken: AccessToken-5f93c2e4 + idToken: IdToken + oauthExpires: OauthExpires-5f93c2e4 + oauthHmac: OauthHMAC-5f93c2e4 + oauthNonce: OauthNonce-5f93c2e4 + refreshToken: RefreshToken-5f93c2e4 + hmacSecret: + name: oauth2/hmac_secret/securitypolicy/default/policy-for-http-route + sdsConfig: + ads: {} + resourceApiVersion: V3 + tokenSecret: + name: oauth2/client_secret/securitypolicy/default/policy-for-http-route + sdsConfig: + ads: {} + resourceApiVersion: V3 + preserveAuthorizationHeader: true + redirectPathMatcher: + path: + exact: /oauth2/callback + redirectUri: '%REQ(x-forwarded-proto)%://%REQ(:authority)%/oauth2/callback' + signoutPath: + path: + exact: /logout + tokenEndpoint: + cluster: oidc_example_com_443 + timeout: 10s + uri: https://oidc.example.com/oauth/token + useRefreshToken: false + - name: envoy.filters.http.jwt_authn + typedConfig: + '@type': type.googleapis.com/envoy.extensions.filters.http.jwt_authn.v3.JwtAuthentication + providers: + httproute/default/httproute-1/rule/0/match/0/www_example_com/exjwt: + claimToHeaders: + - claimName: email + headerName: x-user-email + forward: true + fromCookies: + - IdToken + issuer: https://oidc.example.com/auth/realms/example + normalizePayloadInMetadata: + spaceDelimitedClaims: + - scope + payloadInMetadata: exjwt + remoteJwks: + asyncFetch: {} + cacheDuration: 300s + httpUri: + cluster: oidc_example_com_443 + timeout: 10s + uri: https://oidc.example.com/auth/realms/example/protocol/openid-connect/certs + retryPolicy: {} + requirementMap: + httproute/default/httproute-1/rule/0/match/0/www_example_com: + providerName: httproute/default/httproute-1/rule/0/match/0/www_example_com/exjwt + - name: envoy.filters.http.rbac + typedConfig: + '@type': type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBAC + - 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 + rds: + configSource: + ads: {} + resourceApiVersion: V3 + routeConfigName: envoy-gateway/gateway-1/http + serverHeaderTransformation: PASS_THROUGH + statPrefix: http-10080 + useRemoteAddress: true + name: envoy-gateway/gateway-1/http + name: envoy-gateway/gateway-1/http + perConnectionBufferLimitBytes: 32768 diff --git a/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.routes.yaml new file mode 100644 index 00000000000..9c66aad8e61 --- /dev/null +++ b/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.routes.yaml @@ -0,0 +1,74 @@ +- ignorePortInHostMatching: true + name: envoy-gateway/gateway-1/http + virtualHosts: + - domains: + - www.example.com + metadata: + filterMetadata: + envoy-gateway: + resources: + - kind: Gateway + name: gateway-1 + namespace: envoy-gateway + sectionName: http + name: envoy-gateway/gateway-1/http/www_example_com + routes: + - match: + pathSeparatedPrefix: /foo + metadata: + filterMetadata: + envoy-gateway: + resources: + - kind: HTTPRoute + name: httproute-1 + namespace: default + name: httproute/default/httproute-1/rule/0/match/0/www_example_com + route: + cluster: httproute/default/httproute-1/rule/0 + upgradeConfigs: + - upgradeType: websocket + typedPerFilterConfig: + envoy.filters.http.jwt_authn: + '@type': type.googleapis.com/envoy.extensions.filters.http.jwt_authn.v3.PerRouteConfig + requirementName: httproute/default/httproute-1/rule/0/match/0/www_example_com + envoy.filters.http.oauth2/securitypolicy/default/policy-for-http-route: + '@type': type.googleapis.com/envoy.config.route.v3.FilterConfig + config: {} + envoy.filters.http.rbac: + '@type': type.googleapis.com/envoy.extensions.filters.http.rbac.v3.RBACPerRoute + rbac: + matcher: + matcherList: + matchers: + - onMatch: + action: + name: allow + typedConfig: + '@type': type.googleapis.com/envoy.config.rbac.v3.Action + name: ALLOW + predicate: + singlePredicate: + customMatch: + name: claim_matcher + typedConfig: + '@type': type.googleapis.com/envoy.extensions.matching.input_matchers.metadata.v3.Metadata + value: + listMatch: + oneOf: + stringMatch: + exact: foobar + input: + name: claim + typedConfig: + '@type': type.googleapis.com/envoy.extensions.matching.common_inputs.network.v3.DynamicMetadataInput + filter: envoy.filters.http.jwt_authn + path: + - key: exjwt + - key: groups + onNoMatch: + action: + name: default + typedConfig: + '@type': type.googleapis.com/envoy.config.rbac.v3.Action + action: DENY + name: DENY diff --git a/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.secrets.yaml b/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.secrets.yaml new file mode 100644 index 00000000000..b60c4fc7500 --- /dev/null +++ b/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.secrets.yaml @@ -0,0 +1,8 @@ +- genericSecret: + secret: + inlineBytes: qrOYACHXoe7UEDI/raOjNSx+Z9ufXSc/22C3T6X/zPY= + name: oauth2/client_secret/securitypolicy/default/policy-for-http-route +- genericSecret: + secret: + inlineBytes: qrOYACHXoe7UEDI/raOjNSx+Z9ufXSc/22C3T6X/zPY= + name: oauth2/hmac_secret/securitypolicy/default/policy-for-http-route diff --git a/internal/xds/translator/tracing.go b/internal/xds/translator/tracing.go index 3e817bad1bf..ee3f4f5e907 100644 --- a/internal/xds/translator/tracing.go +++ b/internal/xds/translator/tracing.go @@ -6,7 +6,6 @@ package translator import ( - "errors" "fmt" "sort" @@ -176,7 +175,7 @@ func processClusterForTracing(tCtx *types.ResourceVersionTable, tracing *ir.Trac if traffic == nil { traffic = &ir.TrafficFeatures{} } - if err := addXdsCluster(tCtx, &xdsClusterArgs{ + return addXdsCluster(tCtx, &xdsClusterArgs{ name: tracing.Destination.Name, settings: tracing.Destination.Settings, tSocket: nil, @@ -191,8 +190,5 @@ func processClusterForTracing(tCtx *types.ResourceVersionTable, tracing *ir.Trac backendConnection: traffic.BackendConnection, dns: traffic.DNS, http2Settings: traffic.HTTP2, - }); err != nil && !errors.Is(err, ErrXdsClusterExists) { - return err - } - return nil + }) } diff --git a/internal/xds/translator/translator.go b/internal/xds/translator/translator.go index c06c7195d7c..29bc7d2f5ff 100644 --- a/internal/xds/translator/translator.go +++ b/internal/xds/translator/translator.go @@ -34,11 +34,6 @@ import ( "github.com/envoyproxy/gateway/internal/xds/types" ) -var ( - ErrXdsClusterExists = errors.New("xds cluster exists") - ErrXdsSecretExists = errors.New("xds secret exists") -) - const ( AuthorityHeaderKey = ":authority" // The dummy cluster for TCP listeners that have no routes @@ -491,7 +486,7 @@ func (t *Translator) addRouteToRouteConfig( tSocket: nil, endpointType: EndpointTypeStatic, metrics: metrics, - }); err != nil && !errors.Is(err, ErrXdsClusterExists) { + }); err != nil { errs = errors.Join(errs, err) } } @@ -602,7 +597,7 @@ func (t *Translator) processTCPListenerXdsTranslation( patchProxyProtocolFilter(xdsListener, tcpListener.EnableProxyProtocol) for _, route := range tcpListener.Routes { - if err := processXdsCluster(tCtx, &TCPRouteTranslator{route}, &ExtraArgs{metrics: metrics}); err != nil && !errors.Is(err, ErrXdsClusterExists) { + if err := processXdsCluster(tCtx, &TCPRouteTranslator{route}, &ExtraArgs{metrics: metrics}); err != nil { errs = errors.Join(errs, err) } if route.TLS != nil && route.TLS.Terminate != nil { @@ -689,7 +684,7 @@ func processUDPListenerXdsTranslation( } // 1:1 between IR UDPRoute and xDS Cluster - if err := processXdsCluster(tCtx, &UDPRouteTranslator{route}, &ExtraArgs{metrics: metrics}); err != nil && !errors.Is(err, ErrXdsClusterExists) { + if err := processXdsCluster(tCtx, &UDPRouteTranslator{route}, &ExtraArgs{metrics: metrics}); err != nil { errs = errors.Join(errs, err) } } @@ -783,10 +778,7 @@ func findXdsEndpoint(tCtx *types.ResourceVersionTable, name string) *endpointv3. // processXdsCluster processes xds cluster with args per route. func processXdsCluster(tCtx *types.ResourceVersionTable, route clusterArgs, extras *ExtraArgs) error { - if err := addXdsCluster(tCtx, route.asClusterArgs(extras)); err != nil && !errors.Is(err, ErrXdsClusterExists) { - return err - } - return nil + return addXdsCluster(tCtx, route.asClusterArgs(extras)) } // findXdsSecret finds a xds secret with the same name, and returns nil if there is no match. @@ -805,10 +797,12 @@ func findXdsSecret(tCtx *types.ResourceVersionTable, name string) *tlsv3.Secret return nil } +// addXdsSecret adds a xds secret with args. +// If the secret already exists, it skips adding the secret and returns nil func addXdsSecret(tCtx *types.ResourceVersionTable, secret *tlsv3.Secret) error { - // Return early if cluster with the same name exists + // Return early if secret with the same name exists if c := findXdsSecret(tCtx, secret.Name); c != nil { - return ErrXdsSecretExists + return nil } if err := tCtx.AddXdsResource(resourcev3.SecretType, secret); err != nil { @@ -817,10 +811,15 @@ func addXdsSecret(tCtx *types.ResourceVersionTable, secret *tlsv3.Secret) error return nil } +// addXdsCluster adds a xds cluster with args. +// If the cluster already exists, it skips adding the cluster and returns nil. func addXdsCluster(tCtx *types.ResourceVersionTable, args *xdsClusterArgs) error { - // Return early if cluster with the same name exists + // Return early if cluster with the same name exists. + // All the current callers can all safely assume the xdsClusterArgs is the same for the clusters with the same name. + // If this assumption changes, the callers should call findXdsCluster first to check if the cluster already exists + // before calling addXdsCluster. if c := findXdsCluster(tCtx, args.name); c != nil { - return ErrXdsClusterExists + return nil } xdsCluster := buildXdsCluster(args) diff --git a/internal/xds/translator/utils.go b/internal/xds/translator/utils.go index 23d455edd9c..882d9b1e926 100644 --- a/internal/xds/translator/utils.go +++ b/internal/xds/translator/utils.go @@ -133,7 +133,6 @@ func createExtServiceXDSCluster(rd *ir.RouteDestination, traffic *ir.TrafficFeat var ( endpointType EndpointType tSocket *corev3.TransportSocket - err error ) // Make sure that there are safe defaults for the traffic @@ -148,7 +147,7 @@ func createExtServiceXDSCluster(rd *ir.RouteDestination, traffic *ir.TrafficFeat } else { endpointType = EndpointTypeStatic } - if err = addXdsCluster(tCtx, &xdsClusterArgs{ + return addXdsCluster(tCtx, &xdsClusterArgs{ name: rd.Name, settings: rd.Settings, tSocket: tSocket, @@ -162,10 +161,7 @@ func createExtServiceXDSCluster(rd *ir.RouteDestination, traffic *ir.TrafficFeat endpointType: endpointType, dns: traffic.DNS, http2Settings: traffic.HTTP2, - }); err != nil && !errors.Is(err, ErrXdsClusterExists) { - return err - } - return nil + }) } // addClusterFromURL adds a cluster to the resource version table from the provided URL. @@ -198,8 +194,5 @@ func addClusterFromURL(url string, tCtx *types.ResourceVersionTable) error { clusterArgs.tSocket = tSocket } - if err = addXdsCluster(tCtx, clusterArgs); err != nil && !errors.Is(err, ErrXdsClusterExists) { - return err - } - return nil + return addXdsCluster(tCtx, clusterArgs) } diff --git a/release-notes/current.yaml b/release-notes/current.yaml index 6023fba94a1..144eae5a683 100644 --- a/release-notes/current.yaml +++ b/release-notes/current.yaml @@ -22,6 +22,7 @@ bug fixes: | Helm chart fails for Flux HelmRelease Fixed Envoy rejecting TCP Listeners that have no attached TCPRoutes Fixed failed to update SecurityPolicy resources with the `backendRef` field specified + Fixed xDS translation failed when oidc tokenEndpoint and jwt remoteJWKS are specified in the same SecurityPolicy and using the same hostname # Enhancements that improve performance. performance improvements: |