fix: prevent policies targeting non-TLS listeners on the same port fr…

…om conflicting (#2786) * * Validate that multiple policies that affect listener configuration don't map to the same listener filter chain. * Change the XDS listener generation so that instead of defaultFilterChain for non-TLS routes, a filterChain with a destinationPort matcher is used. This allows multiple policies attached to non-TLS listeners that differ on the destination port to provide different policies without conflicting. Signed-off-by: Lior Okman <[email protected]> * Make hostname based routing work again for non-TLS listeners Signed-off-by: Lior Okman <[email protected]> * Fixed testdata for egctl Signed-off-by: Lior Okman <[email protected]> * Make the linter happy Signed-off-by: Lior Okman <[email protected]> * Added a unit-test Signed-off-by: Lior Okman <[email protected]> * Make the linter happy Signed-off-by: Lior Okman <[email protected]> * Update an e2e test with the new filterChain patch Signed-off-by: Lior Okman <[email protected]> * Revert changing the XDS translation, since a new listener is created anyways for each port. Signed-off-by: Lior Okman <[email protected]> * Also revert the xds change in the e2e test. Signed-off-by: Lior Okman <[email protected]> * Don't need to go over the full XDSIR map - just the current gateway. Signed-off-by: Lior Okman <[email protected]> * Refactored to separate the validation and the translation. Renamed the helper method to a more generic name. Signed-off-by: Lior Okman <[email protected]> --------- Signed-off-by: Lior Okman <[email protected]> Co-authored-by: Guy Daich <[email protected]>
envoyproxy · Mar 12, 2024 · 8d05fb5 · 8d05fb5
1 parent 01a123a
commit 8d05fb5
Show file tree

Hide file tree

Showing 5 changed files with 547 additions and 7 deletions.
diff --git a/internal/gatewayapi/clienttrafficpolicy.go b/internal/gatewayapi/clienttrafficpolicy.go
@@ -6,6 +6,7 @@
 package gatewayapi
 
 import (
+	"errors"
 	"fmt"
 	"sort"
 	"strings"
@@ -89,10 +90,18 @@ func (t *Translator) ProcessClientTrafficPolicies(resources *Resources,
 			policyMap[key].Insert(section)
 
 			// Translate for listener matching section name
+
 			var err error
 			for _, l := range gateway.listeners {
+				// Find IR
+				irKey := t.getIRKey(l.gateway)
+				// It must exist since we've already finished processing the gateways
+				gwXdsIR := xdsIR[irKey]
 				if string(l.Name) == section {
-					err = t.translateClientTrafficPolicyForListener(policy, l, xdsIR, infraIR, resources)
+					err = validatePortOverlapForClientTrafficPolicy(l, gwXdsIR)
+					if err == nil {
+						err = t.translateClientTrafficPolicyForListener(policy, l, xdsIR, infraIR, resources)
+					}
 					break
 				}
 			}
@@ -171,22 +180,30 @@ func (t *Translator) ProcessClientTrafficPolicies(resources *Resources,
 			policyMap[key].Insert(AllSections)
 
 			// Translate sections that have not yet been targeted
-			var err error
+			var errs error
 			for _, l := range gateway.listeners {
 				// Skip if section has already been targeted
 				if s != nil && s.Has(string(l.Name)) {
 					continue
 				}
 
-				err = t.translateClientTrafficPolicyForListener(policy, l, xdsIR, infraIR, resources)
+				// Find IR
+				irKey := t.getIRKey(l.gateway)
+				// It must exist since we've already finished processing the gateways
+				gwXdsIR := xdsIR[irKey]
+				if err := validatePortOverlapForClientTrafficPolicy(l, gwXdsIR); err != nil {
+					errs = errors.Join(errs, err)
+				} else if err := t.translateClientTrafficPolicyForListener(policy, l, xdsIR, infraIR, resources); err != nil {
+					errs = errors.Join(errs, err)
+				}
 			}
 
-			if err != nil {
+			if errs != nil {
 				status.SetClientTrafficPolicyCondition(policy,
 					gwv1a2.PolicyConditionAccepted,
 					metav1.ConditionFalse,
 					gwv1a2.PolicyReasonInvalid,
-					status.Error2ConditionMsg(err),
+					status.Error2ConditionMsg(errs),
 				)
 			} else {
 				// Set Accepted=True
@@ -269,6 +286,29 @@ func resolveCTPolicyTargetRef(policy *egv1a1.ClientTrafficPolicy, gateways []*Ga
 	return gateway
 }
 
+func validatePortOverlapForClientTrafficPolicy(l *ListenerContext, xds *ir.Xds) error {
+	// Find Listener IR
+	// TODO: Support TLSRoute and TCPRoute once
+	// https://github.com/envoyproxy/gateway/issues/1635 is completed
+
+	irListenerName := irHTTPListenerName(l)
+	var httpIR *ir.HTTPListener
+	for _, http := range xds.HTTP {
+		if http.Name == irListenerName {
+			httpIR = http
+			break
+		}
+	}
+
+	// IR must exist since we're past validation
+	if httpIR != nil {
+		if sameListeners := listenersWithSameHTTPPort(xds, httpIR); len(sameListeners) != 0 {
+			return fmt.Errorf("affects additional listeners: %s", strings.Join(sameListeners, ", "))
+		}
+	}
+	return nil
+}
+
 func (t *Translator) translateClientTrafficPolicyForListener(policy *egv1a1.ClientTrafficPolicy, l *ListenerContext,
 	xdsIR XdsIRMap, infraIR InfraIRMap, resources *Resources) error {
 	// Find IR

diff --git a/internal/gatewayapi/helpers.go b/internal/gatewayapi/helpers.go
@@ -420,3 +420,25 @@ func getAncestorRefForPolicy(gatewayNN types.NamespacedName, sectionName *v1alph
 		SectionName: sectionName,
 	}
 }
+
+// listenersWithSameHTTPPort returns a list of the names of all other HTTP listeners
+// that would share the same filter chain as the provided listener when translated
+// to XDS
+func listenersWithSameHTTPPort(xdsIR *ir.Xds, listener *ir.HTTPListener) []string {
+	// if TLS is enabled, the listener would have its own filterChain in Envoy, so
+	// no conflicts are possible
+	if listener.TLS != nil {
+		return nil
+	}
+	res := []string{}
+	for _, http := range xdsIR.HTTP {
+		if http == listener {
+			continue
+		}
+		// Non-TLS listeners can be distinguished by their ports
+		if http.Port == listener.Port {
+			res = append(res, http.Name)
+		}
+	}
+	return res
+}
diff --git a/internal/gatewayapi/securitypolicy.go b/internal/gatewayapi/securitypolicy.go
@@ -109,7 +109,10 @@ func (t *Translator) ProcessSecurityPolicies(securityPolicies []*egv1a1.Security
 				}
 			}
 
-			err := t.translateSecurityPolicyForRoute(policy, route, resources, xdsIR)
+			err := validatePortOverlapForSecurityPolicyRoute(xdsIR, route)
+			if err == nil {
+				err = t.translateSecurityPolicyForRoute(policy, route, resources, xdsIR)
+			}
 			if err != nil {
 				status.SetSecurityPolicyCondition(policy,
 					gwv1a2.PolicyConditionAccepted,
@@ -135,7 +138,13 @@ func (t *Translator) ProcessSecurityPolicies(securityPolicies []*egv1a1.Security
 				continue
 			}
 
-			err := t.translateSecurityPolicyForGateway(policy, gateway, resources, xdsIR)
+			irKey := t.getIRKey(gateway.Gateway)
+			// Should exist since we've validated this
+			xds := xdsIR[irKey]
+			err := validatePortOverlapForSecurityPolicyGateway(xds)
+			if err == nil {
+				err = t.translateSecurityPolicyForGateway(policy, gateway, resources, xdsIR)
+			}
 			if err != nil {
 				status.SetSecurityPolicyCondition(policy,
 					gwv1a2.PolicyConditionAccepted,
@@ -336,6 +345,7 @@ func (t *Translator) translateSecurityPolicyForRoute(
 				// TODO zhaohuabing: extract a utils function to check if an HTTP
 				// route is associated with a Gateway API xRoute
 				if strings.HasPrefix(r.Name, prefix) {
+					// This security policy matches the current route. It should only be accepted if it doesn't match any other route
 					r.CORS = cors
 					r.JWT = jwt
 					r.OIDC = oidc
@@ -348,6 +358,23 @@ func (t *Translator) translateSecurityPolicyForRoute(
 	return errs
 }
 
+func validatePortOverlapForSecurityPolicyRoute(xds XdsIRMap, route RouteContext) error {
+	var errs error
+	prefix := irRoutePrefix(route)
+	for _, ir := range xds {
+		for _, http := range ir.HTTP {
+			for _, r := range http.Routes {
+				if strings.HasPrefix(r.Name, prefix) {
+					if sameListeners := listenersWithSameHTTPPort(ir, http); len(sameListeners) != 0 {
+						errs = errors.Join(errs, fmt.Errorf("affects multiple listeners: %s", strings.Join(sameListeners, ", ")))
+					}
+				}
+			}
+		}
+	}
+	return errs
+}
+
 func (t *Translator) translateSecurityPolicyForGateway(
 	policy *egv1a1.SecurityPolicy, gateway *GatewayContext,
 	resources *Resources, xdsIR XdsIRMap) error {
@@ -439,6 +466,20 @@ func (t *Translator) translateSecurityPolicyForGateway(
 	return errs
 }
 
+func validatePortOverlapForSecurityPolicyGateway(xds *ir.Xds) error {
+	affectedListeners := []string{}
+	for _, http := range xds.HTTP {
+		if sameListeners := listenersWithSameHTTPPort(xds, http); len(sameListeners) != 0 {
+			affectedListeners = append(affectedListeners, sameListeners...)
+		}
+	}
+
+	if len(affectedListeners) > 0 {
+		return fmt.Errorf("affects multiple listeners: %s", strings.Join(affectedListeners, ", "))
+	}
+	return nil
+}
+
 func (t *Translator) buildCORS(cors *egv1a1.CORS) *ir.CORS {
 	var allowOrigins []*ir.StringMatch
 

diff --git a/internal/gatewayapi/testdata/conflicting-policies.in.yaml b/internal/gatewayapi/testdata/conflicting-policies.in.yaml
@@ -0,0 +1,119 @@
+envoyproxy:
+  apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: EnvoyProxy
+  metadata:
+    name: test
+    namespace: envoy-gateway-system
+  spec:
+    mergeGateways: true
+gateways:
+- apiVersion: gateway.networking.k8s.io/v1
+  kind: Gateway
+  metadata:
+    name: gateway-1
+    namespace: default
+  spec:
+    gatewayClassName: envoy-gateway-class
+    listeners:
+    - hostname: "*.192.168.0.15.nip.io"
+      name: http
+      protocol: HTTP
+      port: 80
+      allowedRoutes:
+        namespaces:
+          from: All
+- apiVersion: gateway.networking.k8s.io/v1
+  kind: Gateway
+  metadata:
+    name: mfqjpuycbgjrtdww
+    namespace: default
+  spec:
+    gatewayClassName: envoy-gateway-class
+    listeners:
+    - hostname: qccbahgo.qccbahgo
+      name: http
+      port: 80
+      protocol: HTTP
+httpRoutes:
+- apiVersion: gateway.networking.k8s.io/v1
+  kind: HTTPRoute
+  metadata:
+    name: bdkzlmibsivuiqav
+    namespace: default
+  spec:
+    hostnames:
+    - ntjxuedx.192.168.0.15.nip.io
+    parentRefs:
+    - group: gateway.networking.k8s.io
+      kind: Gateway
+      name: gateway-1
+      namespace: default
+      sectionName: http
+    rules:
+    - backendRefs:
+      - name: service-1
+        port: 8080
+      matches:
+      - path:
+          type: PathPrefix
+          value: /
+- apiVersion: gateway.networking.k8s.io/v1
+  kind: HTTPRoute
+  metadata:
+    name: mfqjpuycbgjrtdww
+    namespace: default
+  spec:
+    hostnames:
+    - qccbahgo.qccbahgo
+    parentRefs:
+    - group: gateway.networking.k8s.io
+      kind: Gateway
+      name: mfqjpuycbgjrtdww
+      namespace: default
+      sectionName: http
+    rules:
+    - backendRefs:
+      - name: service-1
+        port: 8080
+      matches:
+      - path:
+          type: PathPrefix
+          value: /
+securityPolicies:
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: SecurityPolicy
+  metadata:
+    name: cors-example
+    namespace: default
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: Gateway
+      name: mfqjpuycbgjrtdww
+    cors:
+      allowOrigins:
+      - "http://*.foo.com"
+      allowMethods:
+      - PUT
+      - GET
+      - POST
+      - DELETE
+      - PATCH
+      - OPTIONS
+      maxAge: 600s
+      allowCredentials: true
+clientTrafficPolicies:
+- apiVersion: gateway.envoyproxy.io/v1alpha1
+  kind: ClientTrafficPolicy
+  metadata:
+    namespace: default
+    name: target-gateway
+  spec:
+    targetRef:
+      group: gateway.networking.k8s.io
+      kind: Gateway
+      name: mfqjpuycbgjrtdww
+      namespace: default
+    timeout:
+      http:
+        requestReceivedTimeout: "5s"