refactor: group xds security features for security policy (envoyproxy…

…#3019) * group security features for security policy Signed-off-by: shawnh2 <shawnhxh@outlook.com> * change SP xds Any to Empty, return deepcopy for SP Printable method Signed-off-by: shawnh2 <shawnhxh@outlook.com> * fix deepcopy and test data Signed-off-by: shawnh2 <shawnhxh@outlook.com> --------- Signed-off-by: shawnh2 <shawnhxh@outlook.com>
eitansuez · Apr 12, 2024 · 231b3b4 · 231b3b4
1 parent d383ba5
commit 231b3b4
Show file tree

Hide file tree

Showing 36 changed files with 889 additions and 778 deletions.
diff --git a/internal/gatewayapi/securitypolicy.go b/internal/gatewayapi/securitypolicy.go
@@ -380,19 +380,21 @@ func (t *Translator) translateSecurityPolicyForRoute(
 	// Note: there are multiple features in a security policy, even if some of them
 	// are invalid, we still want to apply the valid ones.
 	prefix := irRoutePrefix(route)
-	for _, ir := range xdsIR {
-		for _, http := range ir.HTTP {
-			for _, r := range http.Routes {
+	for _, x := range xdsIR {
+		for _, h := range x.HTTP {
+			for _, r := range h.Routes {
 				// Apply if there is a match
-				// 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
-					r.BasicAuth = basicAuth
-					r.ExtAuth = extAuth
+					// This security policy matches the current route.
+					// It should only be accepted if it doesn't match any other route
+					r.Security = &ir.SecurityFeatures{
+						CORS:      cors,
+						JWT:       jwt,
+						OIDC:      oidc,
+						BasicAuth: basicAuth,
+						ExtAuth:   extAuth,
+					}
 				}
 			}
 		}
@@ -454,44 +456,32 @@ func (t *Translator) translateSecurityPolicyForGateway(
 	// are invalid, we still want to apply the valid ones.
 	irKey := t.getIRKey(gateway.Gateway)
 	// Should exist since we've validated this
-	ir := xdsIR[irKey]
+	x := xdsIR[irKey]
 
 	policyTarget := irStringKey(
 		string(ptr.Deref(policy.Spec.TargetRef.Namespace, gwv1a2.Namespace(policy.Namespace))),
 		string(policy.Spec.TargetRef.Name),
 	)
-	for _, http := range ir.HTTP {
-		gatewayName := http.Name[0:strings.LastIndex(http.Name, "/")]
+	for _, h := range x.HTTP {
+		gatewayName := h.Name[0:strings.LastIndex(h.Name, "/")]
 		if t.MergeGateways && gatewayName != policyTarget {
 			continue
 		}
 		// A Policy targeting the most specific scope(xRoute) wins over a policy
 		// targeting a lesser specific scope(Gateway).
-		for _, r := range http.Routes {
+		for _, r := range h.Routes {
 			// If any of the features are already set, it means that a more specific
 			// policy(targeting xRoute) has already set it, so we skip it.
-			// TODO: zhaohuabing group the features into a struct and check if all of them are set
-			if r.CORS != nil ||
-				r.JWT != nil ||
-				r.OIDC != nil ||
-				r.BasicAuth != nil ||
-				r.ExtAuth != nil {
+			if !r.Security.Empty() {
 				continue
 			}
-			if r.CORS == nil {
-				r.CORS = cors
-			}
-			if r.JWT == nil {
-				r.JWT = jwt
-			}
-			if r.OIDC == nil {
-				r.OIDC = oidc
-			}
-			if r.BasicAuth == nil {
-				r.BasicAuth = basicAuth
-			}
-			if r.ExtAuth == nil {
-				r.ExtAuth = extAuth
+
+			r.Security = &ir.SecurityFeatures{
+				CORS:      cors,
+				JWT:       jwt,
+				OIDC:      oidc,
+				BasicAuth: basicAuth,
+				ExtAuth:   extAuth,
 			}
 		}
 	}

diff --git a/internal/gatewayapi/testdata/conflicting-policies.out.yaml b/internal/gatewayapi/testdata/conflicting-policies.out.yaml
@@ -308,20 +308,6 @@ xdsIR:
       - backendWeights:
           invalid: 0
           valid: 0
-        cors:
-          allowCredentials: true
-          allowMethods:
-          - PUT
-          - GET
-          - POST
-          - DELETE
-          - PATCH
-          - OPTIONS
-          allowOrigins:
-          - distinct: false
-            name: ""
-            safeRegex: http://.*\.foo\.com
-          maxAge: 10m0s
         destination:
           name: httproute/default/mfqjpuycbgjrtdww/rule/0
           settings:
@@ -338,3 +324,18 @@ xdsIR:
           distinct: false
           name: ""
           prefix: /
+        security:
+          cors:
+            allowCredentials: true
+            allowMethods:
+            - PUT
+            - GET
+            - POST
+            - DELETE
+            - PATCH
+            - OPTIONS
+            allowOrigins:
+            - distinct: false
+              name: ""
+              safeRegex: http://.*\.foo\.com
+            maxAge: 10m0s
diff --git a/internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml b/internal/gatewayapi/testdata/merge-with-isolated-policies-2.out.yaml
@@ -519,21 +519,6 @@ xdsIR:
       - backendWeights:
           invalid: 0
           valid: 0
-        cors:
-          allowHeaders:
-          - x-header-5
-          - x-header-6
-          allowMethods:
-          - GET
-          - POST
-          allowOrigins:
-          - distinct: false
-            name: ""
-            safeRegex: .*
-          exposeHeaders:
-          - x-header-7
-          - x-header-8
-          maxAge: 33m20s
         destination:
           name: httproute/default/httproute-1/rule/0
           settings:
@@ -550,6 +535,22 @@ xdsIR:
           distinct: false
           name: ""
           prefix: /
+        security:
+          cors:
+            allowHeaders:
+            - x-header-5
+            - x-header-6
+            allowMethods:
+            - GET
+            - POST
+            allowOrigins:
+            - distinct: false
+              name: ""
+              safeRegex: .*
+            exposeHeaders:
+            - x-header-7
+            - x-header-8
+            maxAge: 33m20s
         tcpKeepalive:
           idleTime: 1200
           interval: 60
@@ -570,21 +571,6 @@ xdsIR:
       - backendWeights:
           invalid: 0
           valid: 0
-        cors:
-          allowHeaders:
-          - x-header-5
-          - x-header-6
-          allowMethods:
-          - GET
-          - POST
-          allowOrigins:
-          - distinct: false
-            name: ""
-            safeRegex: .*
-          exposeHeaders:
-          - x-header-7
-          - x-header-8
-          maxAge: 33m20s
         destination:
           name: httproute/default/httproute-2/rule/0
           settings:
@@ -601,6 +587,22 @@ xdsIR:
           distinct: false
           name: ""
           prefix: /
+        security:
+          cors:
+            allowHeaders:
+            - x-header-5
+            - x-header-6
+            allowMethods:
+            - GET
+            - POST
+            allowOrigins:
+            - distinct: false
+              name: ""
+              safeRegex: .*
+            exposeHeaders:
+            - x-header-7
+            - x-header-8
+            maxAge: 33m20s
         tcpKeepalive:
           idleTime: 1200
           interval: 60
@@ -621,21 +623,6 @@ xdsIR:
       - backendWeights:
           invalid: 0
           valid: 0
-        cors:
-          allowHeaders:
-          - x-header-5
-          - x-header-6
-          allowMethods:
-          - GET
-          - POST
-          allowOrigins:
-          - distinct: false
-            name: ""
-            safeRegex: .*
-          exposeHeaders:
-          - x-header-7
-          - x-header-8
-          maxAge: 33m20s
         destination:
           name: httproute/default/httproute-3/rule/0
           settings:
@@ -652,6 +639,22 @@ xdsIR:
           distinct: false
           name: ""
           prefix: /
+        security:
+          cors:
+            allowHeaders:
+            - x-header-5
+            - x-header-6
+            allowMethods:
+            - GET
+            - POST
+            allowOrigins:
+            - distinct: false
+              name: ""
+              safeRegex: .*
+            exposeHeaders:
+            - x-header-7
+            - x-header-8
+            maxAge: 33m20s
     - address: 0.0.0.0
       hostnames:
       - foo.example.com

diff --git a/internal/gatewayapi/testdata/merge-with-isolated-policies.out.yaml b/internal/gatewayapi/testdata/merge-with-isolated-policies.out.yaml
@@ -309,21 +309,6 @@ xdsIR:
       - backendWeights:
           invalid: 0
           valid: 0
-        cors:
-          allowHeaders:
-          - x-header-5
-          - x-header-6
-          allowMethods:
-          - GET
-          - POST
-          allowOrigins:
-          - distinct: false
-            name: ""
-            safeRegex: .*
-          exposeHeaders:
-          - x-header-7
-          - x-header-8
-          maxAge: 33m20s
         destination:
           name: httproute/default/httproute-1/rule/0
           settings:
@@ -340,6 +325,22 @@ xdsIR:
           distinct: false
           name: ""
           prefix: /
+        security:
+          cors:
+            allowHeaders:
+            - x-header-5
+            - x-header-6
+            allowMethods:
+            - GET
+            - POST
+            allowOrigins:
+            - distinct: false
+              name: ""
+              safeRegex: .*
+            exposeHeaders:
+            - x-header-7
+            - x-header-8
+            maxAge: 33m20s
         tcpKeepalive:
           idleTime: 1200
           interval: 60