From 44f78e0434b020b71974e8f155f40e8959a0ac89 Mon Sep 17 00:00:00 2001 From: huabing zhao <zhaohuabing@gmail.com> Date: Tue, 20 Feb 2024 20:05:57 +0800 Subject: [PATCH] Revert "add a catch-all route if needed (#2586)" This reverts commit 35e646d943b119ff55446e2788d88bc1cf5f3ce9. --- .../securitypolicy-with-basic-auth.out.yaml | 11 ---- .../securitypolicy-with-extauth.out.yaml | 22 -------- .../testdata/securitypolicy-with-oidc.in.yaml | 4 +- .../securitypolicy-with-oidc.out.yaml | 21 ++------ internal/gatewayapi/translator.go | 53 ------------------- site/content/en/latest/user/oidc.md | 8 +-- test/e2e/tests/basic-auth.go | 29 ---------- 7 files changed, 8 insertions(+), 140 deletions(-) diff --git a/internal/gatewayapi/testdata/securitypolicy-with-basic-auth.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-basic-auth.out.yaml index 2356d6a4db8..96c93cd3e93 100644 --- a/internal/gatewayapi/testdata/securitypolicy-with-basic-auth.out.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-with-basic-auth.out.yaml @@ -155,14 +155,3 @@ xdsIR: distinct: false name: "" prefix: /foo - - backendWeights: - invalid: 0 - valid: 0 - directResponse: - statusCode: 404 - hostname: gateway.envoyproxy.io - name: gateway_envoyproxy_io/catch-all-return-404 - pathMatch: - distinct: false - name: "" - prefix: / diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth.out.yaml index 9076ad5e136..412c5643809 100755 --- a/internal/gatewayapi/testdata/securitypolicy-with-extauth.out.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-with-extauth.out.yaml @@ -272,25 +272,3 @@ xdsIR: distinct: false name: "" prefix: /bar - - backendWeights: - invalid: 0 - valid: 0 - directResponse: - statusCode: 404 - hostname: www.foo.com - name: www_foo_com/catch-all-return-404 - pathMatch: - distinct: false - name: "" - prefix: / - - backendWeights: - invalid: 0 - valid: 0 - directResponse: - statusCode: 404 - hostname: www.bar.com - name: www_bar_com/catch-all-return-404 - pathMatch: - distinct: false - name: "" - prefix: / diff --git a/internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml b/internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml index f443b090369..91fae31ce82 100644 --- a/internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-with-oidc.in.yaml @@ -62,7 +62,7 @@ httpRoutes: name: httproute-2 spec: hostnames: - - www.foo.com + - www.example.com parentRefs: - namespace: envoy-gateway name: gateway-1 @@ -70,7 +70,7 @@ httpRoutes: rules: - matches: - path: - value: "/" + value: "/bar" backendRefs: - name: service-1 port: 8080 diff --git a/internal/gatewayapi/testdata/securitypolicy-with-oidc.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-oidc.out.yaml index 034461fa4b6..bd2496ecb77 100644 --- a/internal/gatewayapi/testdata/securitypolicy-with-oidc.out.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-with-oidc.out.yaml @@ -86,7 +86,7 @@ httpRoutes: namespace: default spec: hostnames: - - www.foo.com + - www.example.com parentRefs: - name: gateway-1 namespace: envoy-gateway @@ -97,7 +97,7 @@ httpRoutes: port: 8080 matches: - path: - value: / + value: /bar status: parents: - conditions: @@ -256,8 +256,8 @@ xdsIR: port: 8080 protocol: HTTP weight: 1 - hostname: www.foo.com - name: httproute/default/httproute-2/rule/0/match/0/www_foo_com + hostname: www.example.com + name: httproute/default/httproute-2/rule/0/match/0/www_example_com oidc: clientID: client1.apps.googleusercontent.com clientSecret: Y2xpZW50MTpzZWNyZXQK @@ -272,15 +272,4 @@ xdsIR: pathMatch: distinct: false name: "" - prefix: / - - backendWeights: - invalid: 0 - valid: 0 - directResponse: - statusCode: 404 - hostname: www.example.com - name: www_example_com/catch-all-return-404 - pathMatch: - distinct: false - name: "" - prefix: / + prefix: /bar diff --git a/internal/gatewayapi/translator.go b/internal/gatewayapi/translator.go index 87aa5746c04..5163deca337 100644 --- a/internal/gatewayapi/translator.go +++ b/internal/gatewayapi/translator.go @@ -6,12 +6,8 @@ package gatewayapi import ( - "fmt" - "strings" - "golang.org/x/exp/maps" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/utils/ptr" gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" @@ -207,61 +203,12 @@ func (t *Translator) Translate(resources *Resources) *TranslateResult { // Sort xdsIR based on the Gateway API spec sortXdsIRMap(xdsIR) - // Add a catch-all route for each HTTP listener if needed - addCatchAllRoute(xdsIR) - return newTranslateResult(gateways, httpRoutes, grpcRoutes, tlsRoutes, tcpRoutes, udpRoutes, clientTrafficPolicies, backendTrafficPolicies, securityPolicies, xdsIR, infraIR) } -// For filters without native per-route support, we need to add a catch-all route -// to ensure that these filters are disabled for non-matching requests. -// https://github.com/envoyproxy/gateway/issues/2507 -func addCatchAllRoute(xdsIR map[string]*ir.Xds) { - for _, i := range xdsIR { - for _, http := range i.HTTP { - var needCatchAllRoutePerHost = make(map[string]bool) - for _, r := range http.Routes { - if r.ExtAuth != nil || r.BasicAuth != nil || r.OIDC != nil { - needCatchAllRoutePerHost[r.Hostname] = true - } - } - - // skip if there is already a catch-all route - for host := range needCatchAllRoutePerHost { - for _, r := range http.Routes { - if (r.Hostname == host && - r.PathMatch != nil && - r.PathMatch.Prefix != nil && - *r.PathMatch.Prefix == "/") && - len(r.HeaderMatches) == 0 && - len(r.QueryParamMatches) == 0 { - delete(needCatchAllRoutePerHost, host) - } - } - } - - for host, needCatchAllRoute := range needCatchAllRoutePerHost { - if needCatchAllRoute { - underscoredHost := strings.ReplaceAll(host, ".", "_") - http.Routes = append(http.Routes, &ir.HTTPRoute{ - Name: fmt.Sprintf("%s/catch-all-return-404", underscoredHost), - PathMatch: &ir.StringMatch{ - Prefix: ptr.To("/"), - }, - DirectResponse: &ir.DirectResponse{ - StatusCode: 404, - }, - Hostname: host, - }) - } - } - } - } -} - // GetRelevantGateways returns GatewayContexts, containing a copy of the original // Gateway with the Listener statuses reset. func (t *Translator) GetRelevantGateways(gateways []*gwapiv1.Gateway) []*GatewayContext { diff --git a/site/content/en/latest/user/oidc.md b/site/content/en/latest/user/oidc.md index 41926603355..034d39b5c04 100644 --- a/site/content/en/latest/user/oidc.md +++ b/site/content/en/latest/user/oidc.md @@ -34,11 +34,7 @@ providers, including Auth0, Azure AD, Keycloak, Okta, OneLogin, Salesforce, UAA, Follow the steps in the [Google OIDC documentation][google-oidc] to register an OIDC application. Please make sure the redirect URL is set to the one you configured in the SecurityPolicy that you will create in the step below. If you don't -specify a redirect URL in the SecurityPolicy, the default redirect URL is `https://${GATEWAY_HOST}/oauth2/callback`. -Please notice that the `redirectURL` and `logoutPath` must be caught by the target HTTPRoute. For example, if the -HTTPRoute is configured to match the host `www.example.com` and the path `/foo`, the `redirectURL` must -be prefixed with `https://www.example.com/foo`, and `logoutPath` must be prefixed with`/foo`, for example, -`https://www.example.com/foo/oauth2/callback` and `/foo/logout`, otherwise the OIDC authentication will fail. +specify a redirect URL in the SecurityPolicy, the default redirect URL is `https://<gateway-hostname>/oauth2/callback`. After registering the application, you should have the following information: * Client ID: The client ID of the OIDC application. @@ -77,8 +73,6 @@ spec: clientID: "${CLIENT_ID}.apps.googleusercontent.com" clientSecret: name: "my-app-client-secret" - redirectURI: "https://www.example.com/oauth2/callback" - logoutPath: "/logout" EOF ``` diff --git a/test/e2e/tests/basic-auth.go b/test/e2e/tests/basic-auth.go index e5b16e6c307..cecac930216 100644 --- a/test/e2e/tests/basic-auth.go +++ b/test/e2e/tests/basic-auth.go @@ -156,35 +156,6 @@ var BasicAuthTest = suite.ConformanceTest{ t.Errorf("failed to compare request and response: %v", err) } }) - - // https://github.com/envoyproxy/gateway/issues/2507 - t.Run("request without matching routes ", func(t *testing.T) { - ns := "gateway-conformance-infra" - routeNN := types.NamespacedName{Name: "http-with-basic-auth-1", Namespace: ns} - gwNN := types.NamespacedName{Name: "same-namespace", Namespace: ns} - gwAddr := kubernetes.GatewayAndHTTPRoutesMustBeAccepted(t, suite.Client, suite.TimeoutConfig, suite.ControllerName, kubernetes.NewGatewayRef(gwNN), routeNN) - SecurityPolicyMustBeAccepted(t, suite.Client, types.NamespacedName{Name: "basic-auth-1", Namespace: ns}) - // TODO: We should wait for the `programmed` condition to be true before sending traffic. - expectedResponse := http.ExpectedResponse{ - Request: http.Request{ - Path: "/not-matching-route", - }, - Response: http.Response{ - StatusCode: 404, - }, - Namespace: ns, - } - - req := http.MakeRequest(t, &expectedResponse, gwAddr, "HTTP", "http") - cReq, cResp, err := suite.RoundTripper.CaptureRoundTrip(req) - if err != nil { - t.Errorf("failed to get expected response: %v", err) - } - - if err := http.CompareRequest(t, &req, cReq, cResp, expectedResponse); err != nil { - t.Errorf("failed to compare request and response: %v", err) - } - }) }, }