add a catch-all route if needed (envoyproxy#2586)

* add a catch-all route if needed Signed-off-by: huabing zhao <[email protected]> * fix lint and gen Signed-off-by: huabing zhao <[email protected]> * make catch all route name unique across mulitple hosts Signed-off-by: huabing zhao <[email protected]> * Update internal/gatewayapi/translator.go Co-authored-by: Arko Dasgupta <[email protected]> Signed-off-by: Huabing Zhao <[email protected]> * address comments Signed-off-by: huabing zhao <[email protected]> --------- Signed-off-by: huabing zhao <[email protected]> Signed-off-by: Huabing Zhao <[email protected]> Co-authored-by: Arko Dasgupta <[email protected]>
zhaohuabing · Feb 15, 2024 · 35e646d · 35e646d
1 parent fe88e9a
commit 35e646d
Show file tree

Hide file tree

Showing 7 changed files with 140 additions and 8 deletions.
diff --git a/internal/gatewayapi/testdata/securitypolicy-with-basic-auth.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-basic-auth.out.yaml
@@ -155,3 +155,14 @@ 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
@@ -272,3 +272,25 @@ 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
@@ -62,15 +62,15 @@ httpRoutes:
     name: httproute-2
   spec:
     hostnames:
-    - www.example.com
+    - www.foo.com
     parentRefs:
     - namespace: envoy-gateway
       name: gateway-1
       sectionName: http
     rules:
     - matches:
       - path:
-          value: "/bar"
+          value: "/"
       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
@@ -86,7 +86,7 @@ httpRoutes:
     namespace: default
   spec:
     hostnames:
-    - www.example.com
+    - www.foo.com
     parentRefs:
     - name: gateway-1
       namespace: envoy-gateway
@@ -97,7 +97,7 @@ httpRoutes:
         port: 8080
       matches:
       - path:
-          value: /bar
+          value: /
   status:
     parents:
     - conditions:
@@ -256,8 +256,8 @@ xdsIR:
               port: 8080
             protocol: HTTP
             weight: 1
-        hostname: www.example.com
-        name: httproute/default/httproute-2/rule/0/match/0/www_example_com
+        hostname: www.foo.com
+        name: httproute/default/httproute-2/rule/0/match/0/www_foo_com
         oidc:
           clientID: client1.apps.googleusercontent.com
           clientSecret: Y2xpZW50MTpzZWNyZXQK
@@ -272,4 +272,15 @@ xdsIR:
         pathMatch:
           distinct: false
           name: ""
-          prefix: /bar
+          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: /
diff --git a/internal/gatewayapi/translator.go b/internal/gatewayapi/translator.go
@@ -6,8 +6,12 @@
 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"
@@ -203,12 +207,61 @@ 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
@@ -34,7 +34,11 @@ 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-hostname>/oauth2/callback`.
+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.
 
 After registering the application, you should have the following information:
 * Client ID: The client ID of the OIDC application.
@@ -73,6 +77,8 @@ 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
@@ -156,6 +156,35 @@ 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)
+			}
+		})
 	},
 }