use gwapiv1a2.PolicyStatus for SecurityPolicy Status

Signed-off-by: huabing zhao <[email protected]>

api/v1alpha1/securitypolicy_types.go

@@ -31,7 +31,7 @@ type SecurityPolicy struct {
 	Spec SecurityPolicySpec `json:"spec"`
 
 	// Status defines the current status of SecurityPolicy.
-	Status SecurityPolicyStatus `json:"status,omitempty"`
+	Status gwapiv1a2.PolicyStatus `json:"status,omitempty"`
 }
 
 // SecurityPolicySpec defines the desired state of SecurityPolicy.

internal/gatewayapi/securitypolicy.go

@@ -80,20 +80,27 @@ func (t *Translator) ProcessSecurityPolicies(securityPolicies []*egv1a1.Security
 	// Process the policies targeting xRoutes
 	for _, policy := range securityPolicies {
 		if policy.Spec.TargetRef.Kind != KindGateway {
-			policy := policy.DeepCopy()
+			var (
+				policy         = policy.DeepCopy()
+				targetedRoute  RouteContext
+				parentGateways []gwv1a2.ParentReference
+				err            error
+			)
+
 			res = append(res, policy)
 
 			// Negative statuses have already been assigned so its safe to skip
-			route := resolveSecurityPolicyRouteTargetRef(policy, routeMap)
-			if route == nil {
+			targetedRoute, err = resolveSecurityPolicyRouteTargetRef(policy, routeMap)
+			if targetedRoute == nil && err == nil {
 				continue
 			}
 
-			// Find the Gateway that the route belongs to and add it to the
-			// gatewayRouteMap, which will be used to check policy overrides
-			for _, p := range GetParentReferences(route) {
+			// Find the parent Gateways for the route and add it to the
+			// gatewayRouteMap, which will be used to check policy override.
+			// The parent gateways are also used to set the status of the policy.
+			for _, p := range GetParentReferences(targetedRoute) {
 				if p.Kind == nil || *p.Kind == KindGateway {
-					namespace := route.GetNamespace()
+					namespace := targetedRoute.GetNamespace()
 					if p.Namespace != nil {
 						namespace = string(*p.Namespace)
 					}
@@ -105,65 +112,127 @@ func (t *Translator) ProcessSecurityPolicies(securityPolicies []*egv1a1.Security
 					if _, ok := gatewayRouteMap[gw]; !ok {
 						gatewayRouteMap[gw] = make(sets.Set[string])
 					}
-					gatewayRouteMap[gw].Insert(utils.NamespacedName(route).String())
+					gatewayRouteMap[gw].Insert(utils.NamespacedName(targetedRoute).String())
+					parentGateways = append(parentGateways, gwv1a2.ParentReference{
+						Group:       ptr.To(gwapiv1.Group(gwapiv1.GroupName)),
+						Kind:        ptr.To(gwapiv1.Kind(KindGateway)),
+						Namespace:   ptr.To(gwapiv1.Namespace(namespace)),
+						Name:        p.Name,
+						SectionName: p.SectionName,
+					})
 				}
 			}
 
-			err := t.translateSecurityPolicyForRoute(policy, route, resources, xdsIR)
 			if err != nil {
-				status.SetSecurityPolicyCondition(policy,
+				status.SetSecurityPolicyCondition(
+					policy,
+					parentGateways,
+					gwapiv1.GatewayController(t.GatewayControllerName),
 					gwv1a2.PolicyConditionAccepted,
 					metav1.ConditionFalse,
 					gwv1a2.PolicyReasonInvalid,
-					status.Error2ConditionMsg(err),
-				)
+					status.Error2ConditionMsg(err))
+				continue
+			}
+
+			err = t.translateSecurityPolicyForRoute(policy, targetedRoute, resources, xdsIR)
+			if err != nil {
+				status.SetSecurityPolicyCondition(
+					policy,
+					parentGateways,
+					gwapiv1.GatewayController(t.GatewayControllerName),
+					gwv1a2.PolicyConditionAccepted,
+					metav1.ConditionFalse,
+					gwv1a2.PolicyReasonInvalid,
+					status.Error2ConditionMsg(err))
 			} else {
-				message := "SecurityPolicy has been accepted."
-				status.SetSecurityPolicyAccepted(&policy.Status, message)
+				status.SetSecurityPolicyCondition(
+					policy,
+					parentGateways,
+					gwapiv1.GatewayController(t.GatewayControllerName),
+					gwv1a2.PolicyConditionAccepted,
+					metav1.ConditionTrue,
+					gwv1a2.PolicyReasonAccepted,
+					"SecurityPolicy has been accepted.")
 			}
 		}
 	}
+
 	// Process the policies targeting Gateways
 	for _, policy := range securityPolicies {
 		if policy.Spec.TargetRef.Kind == KindGateway {
-			policy := policy.DeepCopy()
+			var (
+				policy          = policy.DeepCopy()
+				targetedGateway *GatewayContext
+				err             error
+			)
+
 			res = append(res, policy)
 
 			// Negative statuses have already been assigned so its safe to skip
-			gateway := resolveSecurityPolicyGatewayTargetRef(policy, gatewayMap)
-			if gateway == nil {
+			targetedGateway, err = resolveSecurityPolicyGatewayTargetRef(policy, gatewayMap)
+			if err == nil && targetedGateway == nil {
 				continue
 			}
+			parentGateways := []gwv1a2.ParentReference{
+				{
+					Group:     ptr.To(gwapiv1.Group(gwapiv1.GroupName)),
+					Kind:      ptr.To(gwapiv1.Kind(KindGateway)),
+					Namespace: ptr.To(gwapiv1.Namespace(targetedGateway.Namespace)),
+					Name:      gwapiv1.ObjectName(targetedGateway.Name),
+				},
+			}
 
-			err := t.translateSecurityPolicyForGateway(policy, gateway, resources, xdsIR)
 			if err != nil {
-				status.SetSecurityPolicyCondition(policy,
+				status.SetSecurityPolicyCondition(
+					policy,
+					parentGateways,
+					gwapiv1.GatewayController(t.GatewayControllerName),
 					gwv1a2.PolicyConditionAccepted,
 					metav1.ConditionFalse,
 					gwv1a2.PolicyReasonInvalid,
-					status.Error2ConditionMsg(err),
-				)
+					status.Error2ConditionMsg(err))
+			}
+
+			err = t.translateSecurityPolicyForGateway(policy, targetedGateway, resources, xdsIR)
+			if err != nil {
+				status.SetSecurityPolicyCondition(
+					policy,
+					parentGateways,
+					gwapiv1.GatewayController(t.GatewayControllerName),
+					gwv1a2.PolicyConditionAccepted,
+					metav1.ConditionFalse,
+					gwv1a2.PolicyReasonInvalid,
+					status.Error2ConditionMsg(err))
 			} else {
-				message := "SecurityPolicy has been accepted."
-				status.SetSecurityPolicyAccepted(&policy.Status, message)
+				status.SetSecurityPolicyCondition(
+					policy,
+					parentGateways,
+					gwapiv1.GatewayController(t.GatewayControllerName),
+					gwv1a2.PolicyConditionAccepted,
+					metav1.ConditionTrue,
+					gwv1a2.PolicyReasonAccepted,
+					"SecurityPolicy has been accepted.")
 			}
 
 			// Check if this policy is overridden by other policies targeting
 			// at route level
-			gw := utils.NamespacedName(gateway).String()
+			gw := utils.NamespacedName(targetedGateway).String()
 			if r, ok := gatewayRouteMap[gw]; ok {
 				// Maintain order here to ensure status/string does not change with the same data
 				routes := r.UnsortedList()
 				sort.Strings(routes)
 				message := fmt.Sprintf(
 					"This policy is being overridden by other securityPolicies for these routes: %v",
 					routes)
-				status.SetSecurityPolicyCondition(policy,
+				status.SetSecurityPolicyCondition(
+					policy,
+					parentGateways,
+					gwapiv1.GatewayController(t.GatewayControllerName),
 					egv1a1.PolicyConditionOverridden,
 					metav1.ConditionTrue,
 					egv1a1.PolicyReasonOverridden,
-					message,
-				)
+					message)
 			}
 		}
 	}
@@ -173,7 +242,7 @@ func (t *Translator) ProcessSecurityPolicies(securityPolicies []*egv1a1.Security
 
 func resolveSecurityPolicyGatewayTargetRef(
 	policy *egv1a1.SecurityPolicy,
-	gateways map[types.NamespacedName]*policyGatewayTargetContext) *GatewayContext {
+	gateways map[types.NamespacedName]*policyGatewayTargetContext) (*GatewayContext, error) {
 	targetNs := policy.Spec.TargetRef.Namespace
 	// If empty, default to namespace of policy
 	if targetNs == nil {
@@ -182,17 +251,8 @@ func resolveSecurityPolicyGatewayTargetRef(
 
 	// Ensure Policy and target are in the same namespace
 	if policy.Namespace != string(*targetNs) {
-		message := fmt.Sprintf(
-			"Namespace:%s TargetRef.Namespace:%s, SecurityPolicy can only target a resource in the same namespace.",
-			policy.Namespace, *targetNs)
-
-		status.SetSecurityPolicyCondition(policy,
-			gwv1a2.PolicyConditionAccepted,
-			metav1.ConditionFalse,
-			gwv1a2.PolicyReasonInvalid,
-			message,
-		)
-		return nil
+		//TODO zhaohuabing use CEL to validate cross-namespace reference
+		return nil, nil
 	}
 
 	// Find the Gateway
@@ -203,52 +263,38 @@ func resolveSecurityPolicyGatewayTargetRef(
 	gateway, ok := gateways[key]
 
 	// Gateway not found
+	// It's not an error if the gateway is not found because the SecurityPolicy
+	// may be reconciled by multiple controllers, and the gateway may not be managed
+	// by this controller.
 	if !ok {
-		return nil
+		return nil, nil
 	}
 
 	// Check if another policy targeting the same Gateway exists
 	if gateway.attached {
-		message := "Unable to target Gateway, another SecurityPolicy has already attached to it"
-
-		status.SetSecurityPolicyCondition(policy,
-			gwv1a2.PolicyConditionAccepted,
-			metav1.ConditionFalse,
-			gwv1a2.PolicyReasonConflicted,
-			message,
-		)
-		return nil
+		return gateway.GatewayContext, errors.New("unable to target Gateway, another SecurityPolicy has already attached to it")
 	}
 
 	// Set context and save
 	gateway.attached = true
 	gateways[key] = gateway
 
-	return gateway.GatewayContext
+	return gateway.GatewayContext, nil
 }
 
 func resolveSecurityPolicyRouteTargetRef(
 	policy *egv1a1.SecurityPolicy,
-	routes map[policyTargetRouteKey]*policyRouteTargetContext) RouteContext {
+	routes map[policyTargetRouteKey]*policyRouteTargetContext) (RouteContext, error) {
 	targetNs := policy.Spec.TargetRef.Namespace
 	// If empty, default to namespace of policy
 	if targetNs == nil {
 		targetNs = ptr.To(gwv1b1.Namespace(policy.Namespace))
 	}
 
 	// Ensure Policy and target are in the same namespace
+	// TODO zhaohuabing use CEL to validate cross-namespace reference
 	if policy.Namespace != string(*targetNs) {
-		message := fmt.Sprintf(
-			"Namespace:%s TargetRef.Namespace:%s, SecurityPolicy can only target a resource in the same namespace.",
-			policy.Namespace, *targetNs)
-
-		status.SetSecurityPolicyCondition(policy,
-			gwv1a2.PolicyConditionAccepted,
-			metav1.ConditionFalse,
-			gwv1a2.PolicyReasonInvalid,
-			message,
-		)
-		return nil
+		return nil, nil
 	}
 
 	// Check if the route exists
@@ -260,30 +306,25 @@ func resolveSecurityPolicyRouteTargetRef(
 	route, ok := routes[key]
 
 	// Route not found
+	// It's not an error if the gateway is not found because the SecurityPolicy
+	// may be reconciled by multiple controllers, and the gateway may not be managed
+	// by this controller.
 	if !ok {
-		return nil
+		return nil, nil
 	}
 
 	// Check if another policy targeting the same xRoute exists
 	if route.attached {
-		message := fmt.Sprintf(
-			"Unable to target %s, another SecurityPolicy has already attached to it",
+		return route.RouteContext, fmt.Errorf(
+			"unable to target %s, another SecurityPolicy has already attached to it",
 			string(policy.Spec.TargetRef.Kind))
-
-		status.SetSecurityPolicyCondition(policy,
-			gwv1a2.PolicyConditionAccepted,
-			metav1.ConditionFalse,
-			gwv1a2.PolicyReasonConflicted,
-			message,
-		)
-		return nil
 	}
 
 	// Set context and save
 	route.attached = true
 	routes[key] = route
 
-	return route.RouteContext
+	return route.RouteContext, nil
 }
 
 func (t *Translator) translateSecurityPolicyForRoute(

internal/gatewayapi/testdata/merge-with-isolated-policies.out.yaml

@@ -263,12 +263,19 @@ securityPolicies:
       name: gateway-1
       namespace: default
   status:
-    conditions:
-    - lastTransitionTime: null
-      message: SecurityPolicy has been accepted.
-      reason: Accepted
-      status: "True"
-      type: Accepted
+    ancestors:
+    - ancestorRef:
+        group: gateway.networking.k8s.io
+        kind: Gateway
+        name: gateway-1
+        namespace: default
+      conditions:
+      - lastTransitionTime: null
+        message: SecurityPolicy has been accepted.
+        reason: Accepted
+        status: "True"
+        type: Accepted
+      controllerName: gateway.envoyproxy.io/gatewayclass-controller
 xdsIR:
   envoy-gateway-class:
     accessLog:

internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.out.yaml

@@ -12,7 +12,8 @@ securityPolicies:
       kind: HTTPRoute
       name: unknown-httproute
       namespace: envoy-gateway
-  status: {}
+  status:
+    ancestors: null
 - apiVersion: gateway.envoyproxy.io/v1alpha1
   kind: SecurityPolicy
   metadata:
@@ -25,5 +26,6 @@ securityPolicies:
       kind: Gateway
       name: unknown-gateway
       namespace: envoy-gateway
-  status: {}
+  status:
+    ancestors: null
 xdsIR: {}