fix: translator reports errors for existing clusters and secretes by zhaohuabing · Pull Request #4707 · envoyproxy/gateway

zhaohuabing · 2024-11-12T06:54:43Z

Fixes #4706 xDS translation failed when oidc tokenEndpoint and jwt remoteJWKS are specified within the same security policy and using the same hostname

Refactor: skips adding the cluster/secrets and returns nil to make the code cleaner and easier to maintain. It's safe to remove ErrXdsClusterExists and ErrXdsSecretsExists as they don't need to be handled in any places.

Release Notes: Yes

Test before the fix:

--- FAIL: TestTranslateXds (0.43s)
    --- FAIL: TestTranslateXds/securitypolicy-with-oidc-jwt-authz (0.00s)
        translator_test.go:142: securitypolicy-with-oidc-jwt-authz
        translator_test.go:143: 
                Error Trace:    /home/ubuntu/gateway/internal/xds/translator/translator_test.go:143
                Error:          Received unexpected error:
                                xds cluster exists
                Test:           TestTranslateXds/securitypolicy-with-oidc-jwt-authz
FAIL
FAIL    github.com/envoyproxy/gateway/internal/xds/translator   0.673s
FAIL

After:

ok      github.com/envoyproxy/gateway/internal/xds/translator   0.251s

codecov · 2024-11-12T07:02:34Z

Codecov Report

Attention: Patch coverage is 72.22222% with 10 lines in your changes missing coverage. Please review.

Project coverage is 65.65%. Comparing base (f99c36c) to head (2759831).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/xds/translator/translator.go	72.72%	1 Missing and 2 partials ⚠️
internal/gatewayapi/securitypolicy.go	66.66%	0 Missing and 2 partials ⚠️
internal/xds/translator/extauth.go	0.00%	0 Missing and 2 partials ⚠️
internal/xds/translator/accesslog.go	50.00%	0 Missing and 1 partial ⚠️
internal/xds/translator/extproc.go	0.00%	0 Missing and 1 partial ⚠️
internal/xds/translator/oidc.go	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4707      +/-   ##
==========================================
+ Coverage   65.63%   65.65%   +0.02%     
==========================================
  Files         211      211              
  Lines       32017    31996      -21     
==========================================
- Hits        21014    21008       -6     
+ Misses       9761     9751      -10     
+ Partials     1242     1237       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

arkodg · 2024-11-13T02:39:49Z

hey @zhaohuabing is the issue that we are generating the same name for the jwks and oidc clusters if both are set in the policy ? shouldnt we be using an additional string value to differentiate them ?

zhaohuabing · 2024-11-14T14:29:51Z

hey @zhaohuabing is the issue that we are generating the same name for the jwks and oidc clusters if both are set in the policy ? shouldnt we be using an additional string value to differentiate them ?

For the cluster generated by a url, EG uses the host and port for the cluster name to avoid creating duplicated clusters for the same host+port combination.

jwt: 
 providers:
   - remoteJWKS:
       uri: https://oidc.example.com/auth/realms/example/protocol/openid-connect/cert

oidc:
 provider:
   tokenEndpoint: https://oidc.example.com/oauth/token

The name of the generated cluster: oidc_example_com_443.

We could change this logic to generate an unique name for each single oidc or jwt configuration. However, we should also ensure that the translator shouldn't throw error if the cluster already exists.

arkodg · 2024-11-14T15:12:39Z

hey @zhaohuabing is the issue that we are generating the same name for the jwks and oidc clusters if both are set in the policy ? shouldnt we be using an additional string value to differentiate them ?

For the cluster generated by a url, EG uses the host and port for the cluster name to avoid creating duplicated clusters for the same host+port combination.
jwt: 
 providers:
   - remoteJWKS:
       uri: https://oidc.example.com/auth/realms/example/protocol/openid-connect/cert

oidc:
 provider:
   tokenEndpoint: https://oidc.example.com/oauth/token
The name of the generated cluster: oidc_example_com_443.

We could change this logic to generate an unique name for each single oidc or jwt configuration. However, we should also ensure that the translator shouldn't throw error if the cluster already exists.

is it safe to reuse the same cluster configuration ? is the naming different when use the backendRefs field ?

zhaohuabing · 2024-11-14T15:32:57Z

is it safe to reuse the same cluster configuration ? is the naming different when use the backendRefs field ?

It's safe to reuse the asme cluster for ulr generated cluster as the cluster confiugration is identical for the same host+port combination.

For OIDC provider with backendRefs, EG generate an unique cluster name like securitypolicy/envoy-gateway/policy-for-gateway/0

internal/xds/translator/translator.go

zhaohuabing · 2024-11-14T15:48:11Z

For OIDC provider with backendRefs, EG generate an unique cluster name like securitypolicy/envoy-gateway/policy-for-gateway/0.

Ha, there is also a bug here, the index is always 0, which generates duplicated name for different clusters if there're both ext auth and oidc whithin a SecurityPolicy. Will fix it in this PR as well.

arkodg · 2024-11-14T15:51:13Z

For OIDC provider with backendRefs, EG generate an unique cluster name like securitypolicy/envoy-gateway/policy-for-gateway/0.

Ha, there is also a bug here, the index is always 0, which generates duplicated name for different clusters if there're both ext auth and oidc whithin a SecurityPolicy. Will fix it in this PR as well.

nice catch, prob needs another prefix like jwt, oidc after policy name

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

arkodg · 2024-11-19T01:23:01Z

internal/gatewayapi/securitypolicy.go

 	prefix := irRoutePrefix(route)
 	parentRefs := GetParentReferences(route)
+	var (
+		extAutClusterIndex = 0


why is the index needed ?

A cluster is created for each parent, so an index is added to the cluster name to avoid name conflict.

var ( extAutClusterIndex = 0 oidcClusterIndex = 0 ) for _, p := range parentRefs { parentRefCtx := GetRouteParentContext(route, p) gtwCtx := parentRefCtx.GetGateway() if gtwCtx == nil { continue } var extAuth *ir.ExtAuth if policy.Spec.ExtAuth != nil { if extAuth, err = t.buildExtAuth( policy, resources, gtwCtx.envoyProxy, extAutClusterIndex, ); err != nil { err = perr.WithMessage(err, "ExtAuth") errs = errors.Join(errs, err) } extAutClusterIndex++ } var oidc *ir.OIDC if policy.Spec.OIDC != nil { if oidc, err = t.buildOIDC( policy, resources, gtwCtx.envoyProxy, // TODO zhaohuabing: Only the last EnvoyProxy will be used as the OIDC name doesn't include the cluster index oidcClusterIndex, ); err != nil { err = perr.WithMessage(err, "OIDC") errs = errors.Join(errs, err) } oidcClusterIndex++ } ... } return errs }

It's the same policy though, can't the clusters be reused ?

It's because the EnvoyProxy configuration is different for each parent.

There's still an issue here, only the last one will be applied as the OIDC name is identical, I've added a TODO and will address this later in a follow-up PR.

#4707 (comment)

if its due to envoy proxy resource attributes that make the cluster unique, then lets append /<ns>-<name>/ of EP instead to maximize reuse ?

or listener name, the index here is not used to debug and is not useful for reuse

Rethinking on this, since EnvoyProxy is an optional parameter, its name may not be suitable to be used as part of the generated cluster name. We can always use EndpointRoutingType or ServiceRoutingType for OIDC, ExtProc, and other none-route backend clusters and don't need to use the RoutingType in the EnvoyProxy.

@arkodg Could we address this in a follow-up PR for #4720? This is a minor issue and won't break OIDC. Addressing it in a separate PR will make cherry-picking to v1.2.2 easier.

@zhaohuabing I'd prefer if we removed the index altogether and raised another GH issue for dealing with policies targeting routes linked to multiple envoy proxies that are impacting the upstream TLS.

internal/xds/translator/translator.go

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

arkodg

LGTM thanks !

…voyproxy#4707) * fix: existing clusters and secretes Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix cluster index for SP Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * minor change Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * minor change Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * minor change Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * minor change Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * add comment Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * remove index Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

* fix: tcp listener is rejected when no route attached (#4681) * fix: tcp listener is rejected when no route attached Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * change cluter name Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix listener connection limit test Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix listener connetcp keepalive test Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix tcp endpoint stats test Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix tcp-route-enable-req-resp-sizes-stats Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix extensionpolicy-tcp-udp-http test Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> (cherry picked from commit f99c36c) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix: remove backendrefs validation (#4705) * remove backendrefs validation Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * add tests Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * add tests Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com> (cherry picked from commit 5068698) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix: translator reports errors for existing clusters and secretes (#4707) * fix: existing clusters and secretes Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix cluster index for SP Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * minor change Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * minor change Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * minor change Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * minor change Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * add comment Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * remove index Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * xds: always use `::` and `IPv4Compact` for dynamic listener (#4743) * enable IPv4Compact Signed-off-by: zirain <zirain2009@gmail.com> * fix xds test Signed-off-by: zirain <zirain2009@gmail.com> * release-notes Signed-off-by: zirain <zirain2009@gmail.com> * nit Signed-off-by: zirain <zirain2009@gmail.com> * gen Signed-off-by: zirain <zirain2009@gmail.com> --------- Signed-off-by: zirain <zirain2009@gmail.com> (cherry picked from commit 78da42c) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * Fix: frequent 503 errors when connecting to a Service experiencing high Pod churn (#4754) * Revert "fix: some status updates are discarded by the status updater (#4337)" This reverts commit 14830c7. Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * store update events and process it later Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * rename method Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * add release note Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * xds: use V4_PREFERRED dnsLookupFamily by default (#4745) * use Cluster_V4_PREFERRED Signed-off-by: zirain <zirain2009@gmail.com> * release notes Signed-off-by: zirain <zirain2009@gmail.com> --------- Signed-off-by: zirain <zirain2009@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: zirain <zirain2009@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com>

) * fix: tcp listener is rejected when no route attached (envoyproxy#4681) * fix: tcp listener is rejected when no route attached Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * change cluter name Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix listener connection limit test Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix listener connetcp keepalive test Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix tcp endpoint stats test Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix tcp-route-enable-req-resp-sizes-stats Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix extensionpolicy-tcp-udp-http test Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> (cherry picked from commit f99c36c) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix: remove backendrefs validation (envoyproxy#4705) * remove backendrefs validation Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * add tests Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * add tests Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com> (cherry picked from commit 5068698) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix: translator reports errors for existing clusters and secretes (envoyproxy#4707) * fix: existing clusters and secretes Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix cluster index for SP Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * minor change Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * minor change Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * minor change Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * minor change Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * add comment Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * remove index Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * fix lint Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * xds: always use `::` and `IPv4Compact` for dynamic listener (envoyproxy#4743) * enable IPv4Compact Signed-off-by: zirain <zirain2009@gmail.com> * fix xds test Signed-off-by: zirain <zirain2009@gmail.com> * release-notes Signed-off-by: zirain <zirain2009@gmail.com> * nit Signed-off-by: zirain <zirain2009@gmail.com> * gen Signed-off-by: zirain <zirain2009@gmail.com> --------- Signed-off-by: zirain <zirain2009@gmail.com> (cherry picked from commit 78da42c) Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * Fix: frequent 503 errors when connecting to a Service experiencing high Pod churn (envoyproxy#4754) * Revert "fix: some status updates are discarded by the status updater (envoyproxy#4337)" This reverts commit 14830c7. Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * store update events and process it later Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * rename method Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * add release note Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> * xds: use V4_PREFERRED dnsLookupFamily by default (envoyproxy#4745) * use Cluster_V4_PREFERRED Signed-off-by: zirain <zirain2009@gmail.com> * release notes Signed-off-by: zirain <zirain2009@gmail.com> --------- Signed-off-by: zirain <zirain2009@gmail.com> --------- Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com> Signed-off-by: zirain <zirain2009@gmail.com> Co-authored-by: zirain <zirain2009@gmail.com>

zhaohuabing added the cherrypick/release-v1.2 label Nov 12, 2024

zhaohuabing requested a review from a team as a code owner November 12, 2024 06:54

zhaohuabing marked this pull request as draft November 12, 2024 06:55

zhaohuabing added cherrypick/release-v1.2.2 and removed cherrypick/release-v1.2 labels Nov 12, 2024

zhaohuabing force-pushed the fix-existing-cluster branch from 87b5867 to 15264f6 Compare November 12, 2024 07:06

zhaohuabing marked this pull request as ready for review November 12, 2024 07:10

zhaohuabing force-pushed the fix-existing-cluster branch from 15264f6 to bda736c Compare November 12, 2024 07:15

zhaohuabing marked this pull request as draft November 12, 2024 07:15

zhaohuabing changed the title ~~fix: existing clusters and secretes~~ fix: translator reports errors for existing clusters and secretes Nov 12, 2024

zhaohuabing force-pushed the fix-existing-cluster branch 6 times, most recently from 21c1901 to a7d7e6b Compare November 13, 2024 00:01

zhaohuabing marked this pull request as ready for review November 13, 2024 00:07

fix: existing clusters and secretes

bd01257

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing force-pushed the fix-existing-cluster branch from a7d7e6b to bd01257 Compare November 13, 2024 00:12

Merge branch 'main' into fix-existing-cluster

ab2a47c

arkodg reviewed Nov 14, 2024

View reviewed changes

internal/xds/translator/translator.go Show resolved Hide resolved

fix cluster index for SP

4fe8508

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing marked this pull request as draft November 14, 2024 16:57

zhaohuabing marked this pull request as ready for review November 14, 2024 17:26

zhaohuabing requested a review from arkodg November 14, 2024 17:31

zhaohuabing added 3 commits November 15, 2024 14:31

Merge remote-tracking branch 'origin/main' into fix-existing-cluster

6c22930

add comment

2f22fbb

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

Merge branch 'main' into fix-existing-cluster

5cdea4d

zhaohuabing mentioned this pull request Nov 18, 2024

OIDC filter randomly failing in v1.2 due to missing information in token request #4718

Closed

arkodg reviewed Nov 19, 2024

View reviewed changes

internal/xds/translator/translator.go Show resolved Hide resolved

zhaohuabing added 3 commits November 18, 2024 18:39

Merge branch 'main' into fix-existing-cluster

1c2709e

Merge branch 'main' into fix-existing-cluster

09a3bef

remove index

4a63519

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

zhaohuabing requested a review from arkodg November 19, 2024 14:19

zhaohuabing added 2 commits November 19, 2024 22:23

Merge branch 'main' into fix-existing-cluster

7f04c87

fix lint

d57bf68

Signed-off-by: Huabing Zhao <zhaohuabing@gmail.com>

guydc previously approved these changes Nov 20, 2024

View reviewed changes

arkodg previously approved these changes Nov 20, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into fix-existing-cluster

2759831

zhaohuabing dismissed stale reviews from arkodg and guydc via 2759831 November 20, 2024 01:45

zhaohuabing requested review from arkodg and guydc November 20, 2024 01:45

arkodg approved these changes Nov 20, 2024

View reviewed changes

zirain approved these changes Nov 20, 2024

View reviewed changes

zhaohuabing merged commit 86d750a into envoyproxy:main Nov 20, 2024

zhaohuabing deleted the fix-existing-cluster branch November 20, 2024 02:24

zhaohuabing mentioned this pull request Nov 22, 2024

[release/v1.2] cherry pick bug fixes and IPv6 to v1.2.2 #4765

Merged

Conversation

zhaohuabing commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

arkodg commented Nov 13, 2024

Uh oh!

zhaohuabing commented Nov 14, 2024

Uh oh!

arkodg commented Nov 14, 2024

Uh oh!

zhaohuabing commented Nov 14, 2024

Uh oh!

Uh oh!

zhaohuabing commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arkodg commented Nov 14, 2024

Uh oh!

arkodg Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

zhaohuabing Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

zhaohuabing Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

arkodg Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

zhaohuabing Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arkodg Nov 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arkodg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zhaohuabing commented Nov 12, 2024 •

edited

Loading

codecov bot commented Nov 12, 2024 •

edited

Loading

zhaohuabing commented Nov 14, 2024 •

edited

Loading

zhaohuabing Nov 19, 2024 •

edited

Loading

zhaohuabing Nov 19, 2024 •

edited

Loading

zhaohuabing Nov 19, 2024 •

edited

Loading