fix a lot

Author: Sarthak Agrawal

internal/controller/state/conditions/conditions.go

@@ -145,8 +145,8 @@ const (
 
 	// PolicyMessageAncestorLimitReached is a message used with PolicyReasonAncestorLimitReached
 	// when a policy cannot be applied due to the 16 ancestor limit being reached.
-	PolicyMessageAncestorLimitReached = "Policy cannot be applied because the ancestor status list " +
-		"has reached the maximum size of 16"
+	PolicyMessageAncestorLimitReached = "Policies cannot be applied because the ancestor status list " +
+		"has reached the maximum size of 16. The following policies have been ignored:"
 )
 
 // Condition defines a condition to be reported in the status of resources.
@@ -979,7 +979,7 @@ func NewPolicyTargetNotFound(msg string) Condition {
 
 // NewPolicyAncestorLimitReached returns a Condition that indicates that the Policy is not accepted because
 // the ancestor status list has reached the maximum size of 16.
-func NewPolicyAncestorLimitReached(_ string) Condition {
+func NewPolicyAncestorLimitReached() Condition {
 	return Condition{
 		Type:    string(v1alpha2.PolicyConditionAccepted),
 		Status:  metav1.ConditionFalse,

internal/controller/state/conditions/conditions_test.go

@@ -151,8 +151,7 @@ func TestNewPolicyAncestorLimitReached(t *testing.T) {
 	t.Parallel()
 	g := NewWithT(t)
 
-	policyName := "test-policy"
-	condition := NewPolicyAncestorLimitReached(policyName)
+	condition := NewPolicyAncestorLimitReached()
 
 	g.Expect(condition.Type).To(Equal(string(v1alpha2.PolicyConditionAccepted)))
 	g.Expect(condition.Status).To(Equal(metav1.ConditionFalse))

internal/controller/state/graph/backend_tls_policy.go

@@ -3,6 +3,7 @@ package graph
 import (
 	"fmt"
 	"slices"
+	"strings"
 
 	"github.com/go-logr/logr"
 	"k8s.io/apimachinery/pkg/types"
@@ -75,12 +76,16 @@ func validateBackendTLSPolicy(
 	valid = true
 	ignored = false
 
-	// Note: Ancestor limit checking moved to addGatewaysForBackendTLSPolicies for per-gateway effectiveness tracking
-	// The policy may be partially effective (work for some gateways but not others due to ancestor limits)
+	// Ancestor limit handling is now done during gateway assignment phase, not validation
+	// if backendTLSPolicyAncestorsFull(backendTLSPolicy.Status.Ancestors, ctlrName) {
+	//	valid = false
+	//	ignored = true
+	//	return
+	// }
 
 	if err := validateBackendTLSHostname(backendTLSPolicy); err != nil {
 		valid = false
-		conds = append(conds, conditions.NewPolicyInvalid(fmt.Sprintf("invalid hostname: %s", err.Error())))
+		conds = append(conds, conditions.NewPolicyInvalid(err.Error()))
 	}
 
 	caCertRefs := backendTLSPolicy.Spec.Validation.CACertificateRefs
@@ -182,61 +187,148 @@ func validateBackendTLSWellKnownCACerts(btp *v1alpha3.BackendTLSPolicy) error {
 	return nil
 }
 
-func addGatewaysForBackendTLSPolicies(
-	backendTLSPolicies map[types.NamespacedName]*BackendTLSPolicy,
+// countNonNGFAncestors counts the number of non-NGF ancestors in policy status.
+func countNonNGFAncestors(policy *v1alpha3.BackendTLSPolicy, ctlrName string) int {
+	nonNGFCount := 0
+	for _, ancestor := range policy.Status.Ancestors {
+		if string(ancestor.ControllerName) != ctlrName {
+			nonNGFCount++
+		}
+	}
+	return nonNGFCount
+}
+
+// addPolicyAncestorLimitCondition adds or updates a PolicyAncestorLimitReached condition.
+func addPolicyAncestorLimitCondition(
+	conds []conditions.Condition,
+	policyName string,
+	policyType string,
+) []conditions.Condition {
+	const policyAncestorLimitReachedType = "PolicyAncestorLimitReached"
+
+	for i, condition := range conds {
+		if condition.Type == policyAncestorLimitReachedType {
+			if !strings.Contains(condition.Message, policyName) {
+				conds[i].Message = fmt.Sprintf("%s, %s %s", condition.Message, policyType, policyName)
+			}
+			return conds
+		}
+	}
+
+	newCondition := conditions.NewPolicyAncestorLimitReached()
+	newCondition.Message = fmt.Sprintf("%s %s %s", newCondition.Message, policyType, policyName)
+	conds = append(conds, newCondition)
+	return conds
+}
+
+// collectOrderedGateways collects gateways in spec order (services) then creation time order (gateways within service).
+func collectOrderedGateways(
+	policy *v1alpha3.BackendTLSPolicy,
 	services map[types.NamespacedName]*ReferencedService,
-	ctlrName string,
 	gateways map[types.NamespacedName]*Gateway,
-	logger logr.Logger,
-) {
-	for _, backendTLSPolicy := range backendTLSPolicies {
-		potentialGateways := make(map[types.NamespacedName]struct{})
+	existingNGFGatewayAncestors map[types.NamespacedName]struct{},
+) (existingGateways []types.NamespacedName, newGateways []types.NamespacedName) {
+	seenGateways := make(map[types.NamespacedName]struct{})
+
+	// Process services in spec order to maintain deterministic gateway ordering
+	for _, refs := range policy.Spec.TargetRefs {
+		if refs.Kind != kinds.Service {
+			continue
+		}
+
+		svcNsName := types.NamespacedName{
+			Namespace: policy.Namespace,
+			Name:      string(refs.Name),
+		}
+
+		referencedService, exists := services[svcNsName]
+		if !exists {
+			continue
+		}
 
-		// First, collect all potential gateways for this policy
-		for _, refs := range backendTLSPolicy.Source.Spec.TargetRefs {
-			if refs.Kind != kinds.Service {
+		// Add to ordered lists, categorizing existing vs new, skipping duplicates
+		for gateway := range referencedService.GatewayNsNames {
+			if _, seen := seenGateways[gateway]; seen {
 				continue
 			}
+			seenGateways[gateway] = struct{}{}
+			if _, exists := existingNGFGatewayAncestors[gateway]; exists {
+				existingGateways = append(existingGateways, gateway)
+			} else {
+				newGateways = append(newGateways, gateway)
+			}
+		}
+	}
 
-			for svcNsName, referencedServices := range services {
-				if svcNsName.Name != string(refs.Name) {
-					continue
-				}
+	sortGatewaysByCreationTime(existingGateways, gateways)
+	sortGatewaysByCreationTime(newGateways, gateways)
 
-				for gateway := range referencedServices.GatewayNsNames {
-					potentialGateways[gateway] = struct{}{}
-				}
+	return existingGateways, newGateways
+}
+
+func extractExistingNGFGatewayAncestors(
+	backendTLSPolicy *v1alpha3.BackendTLSPolicy,
+	ctlrName string,
+) map[types.NamespacedName]struct{} {
+	existingNGFGatewayAncestors := make(map[types.NamespacedName]struct{})
+
+	for _, ancestor := range backendTLSPolicy.Status.Ancestors {
+		if string(ancestor.ControllerName) != ctlrName {
+			continue
+		}
+
+		if ancestor.AncestorRef.Kind != nil && *ancestor.AncestorRef.Kind == v1.Kind(kinds.Gateway) &&
+			ancestor.AncestorRef.Namespace != nil {
+			gatewayNsName := types.NamespacedName{
+				Namespace: string(*ancestor.AncestorRef.Namespace),
+				Name:      string(ancestor.AncestorRef.Name),
 			}
+			existingNGFGatewayAncestors[gatewayNsName] = struct{}{}
 		}
+	}
+
+	return existingNGFGatewayAncestors
+}
 
-		// Now check each potential gateway against ancestor limits
-		for gatewayNsName := range potentialGateways {
-			// Create a proposed ancestor reference for this gateway
-			proposedAncestor := createParentReference(v1.GroupName, kinds.Gateway, gatewayNsName)
+func addGatewaysForBackendTLSPolicies(
+	backendTLSPolicies map[types.NamespacedName]*BackendTLSPolicy,
+	services map[types.NamespacedName]*ReferencedService,
+	ctlrName string,
+	gateways map[types.NamespacedName]*Gateway,
+	logger logr.Logger,
+) {
+	for _, backendTLSPolicy := range backendTLSPolicies {
+		existingNGFGatewayAncestors := extractExistingNGFGatewayAncestors(backendTLSPolicy.Source, ctlrName)
+		existingGateways, newGateways := collectOrderedGateways(
+			backendTLSPolicy.Source, services, gateways, existingNGFGatewayAncestors)
+
+		existingGateways = append(existingGateways, newGateways...)
+		orderedGateways := existingGateways
 
-			// Check ancestor limit for BackendTLS policy
-			isFull := backendTLSPolicyAncestorsFull(
-				backendTLSPolicy.Source.Status.Ancestors,
-				ctlrName,
-			)
+		ancestorCount := countNonNGFAncestors(backendTLSPolicy.Source, ctlrName)
 
-			if isFull {
+		// Process each gateway, respecting ancestor limits
+		for _, gatewayNsName := range orderedGateways {
+			// Check if adding this gateway would exceed the ancestor limit
+			if ancestorCount >= maxAncestors {
 				policyName := backendTLSPolicy.Source.Namespace + "/" + backendTLSPolicy.Source.Name
+				proposedAncestor := createParentReference(v1.GroupName, kinds.Gateway, gatewayNsName)
 				gatewayName := getAncestorName(proposedAncestor)
-				gateway, ok := gateways[gatewayNsName]
-				if ok {
-					gateway.Conditions = append(gateway.Conditions, conditions.NewPolicyAncestorLimitReached(policyName))
+
+				if gateway, ok := gateways[gatewayNsName]; ok {
+					gateway.Conditions = addPolicyAncestorLimitCondition(gateway.Conditions, policyName, kinds.BackendTLSPolicy)
 				} else {
-					// Not found in the graph, log the issue. I don't think this should happen.
-					logger.Info("Gateway not found in the graph", "policy", policyName, "ancestor", gatewayName)
+					// This should never happen, but we'll log it if it does
+					logger.Error(fmt.Errorf("gateway not found in the graph"),
+						"Gateway not found in the graph", "policy", policyName, "ancestor", gatewayName)
 				}
 
 				LogAncestorLimitReached(logger, policyName, "BackendTLSPolicy", gatewayName)
-
 				continue
 			}
 
-			// Gateway can be effectively used by this policy
+			ancestorCount++
+
 			backendTLSPolicy.Gateways = append(backendTLSPolicy.Gateways, gatewayNsName)
 		}
 	}

internal/controller/state/graph/policies.go

@@ -71,6 +71,51 @@ const (
 )
 
 // attachPolicies attaches the graph's processed policies to the resources they target. It modifies the graph in place.
+// extractExistingNGFGatewayAncestorsForPolicy extracts existing NGF gateway ancestors from policy status.
+func extractExistingNGFGatewayAncestorsForPolicy(policy *Policy, ctlrName string) map[types.NamespacedName]struct{} {
+	existingNGFGatewayAncestors := make(map[types.NamespacedName]struct{})
+
+	for _, ancestor := range policy.Source.GetPolicyStatus().Ancestors {
+		if string(ancestor.ControllerName) != ctlrName {
+			continue
+		}
+
+		if ancestor.AncestorRef.Kind != nil && *ancestor.AncestorRef.Kind == v1.Kind(kinds.Gateway) &&
+			ancestor.AncestorRef.Namespace != nil {
+			gatewayNsName := types.NamespacedName{
+				Namespace: string(*ancestor.AncestorRef.Namespace),
+				Name:      string(ancestor.AncestorRef.Name),
+			}
+			existingNGFGatewayAncestors[gatewayNsName] = struct{}{}
+		}
+	}
+
+	return existingNGFGatewayAncestors
+}
+
+// collectOrderedGatewaysForService collects gateways for a service with existing gateway prioritization.
+func collectOrderedGatewaysForService(
+	svc *ReferencedService,
+	gateways map[types.NamespacedName]*Gateway,
+	existingNGFGatewayAncestors map[types.NamespacedName]struct{},
+) (existingGateways []types.NamespacedName, newGateways []types.NamespacedName) {
+	existingGateways = make([]types.NamespacedName, 0, len(svc.GatewayNsNames))
+	newGateways = make([]types.NamespacedName, 0, len(svc.GatewayNsNames))
+
+	for gwNsName := range svc.GatewayNsNames {
+		if _, exists := existingNGFGatewayAncestors[gwNsName]; exists {
+			existingGateways = append(existingGateways, gwNsName)
+		} else {
+			newGateways = append(newGateways, gwNsName)
+		}
+	}
+
+	sortGatewaysByCreationTime(existingGateways, gateways)
+	sortGatewaysByCreationTime(newGateways, gateways)
+
+	return existingGateways, newGateways
+}
+
 func (g *Graph) attachPolicies(validator validation.PolicyValidator, ctlrName string, logger logr.Logger) {
 	if len(g.Gateways) == 0 {
 		return
@@ -109,8 +154,20 @@ func attachPolicyToService(
 ) {
 	var attachedToAnyGateway bool
 
-	for gwNsName, gw := range gws {
-		if _, belongsToGw := svc.GatewayNsNames[gwNsName]; !belongsToGw {
+	// Extract existing NGF gateway ancestors from policy status
+	existingNGFGatewayAncestors := extractExistingNGFGatewayAncestorsForPolicy(policy, ctlrName)
+
+	// Collect and order gateways with existing gateway prioritization
+	existingGateways, newGateways := collectOrderedGatewaysForService(
+		svc, gws, existingNGFGatewayAncestors)
+
+	existingGateways = append(existingGateways, newGateways...)
+	orderedGateways := existingGateways
+
+	for _, gwNsName := range orderedGateways {
+		gw := gws[gwNsName]
+
+		if gw == nil || gw.Source == nil {
 			continue
 		}
 
@@ -129,11 +186,20 @@ func attachPolicyToService(
 			continue
 		}
 
+		// Check if this is an existing gateway from policy status
+		_, isExistingGateway := existingNGFGatewayAncestors[gwNsName]
+
+		if isExistingGateway {
+			// Existing gateway from policy status - mark as attached but don't add to ancestors
+			attachedToAnyGateway = true
+			continue
+		}
+
 		if ngfPolicyAncestorsFull(policy, ctlrName) {
 			policyName := getPolicyName(policy.Source)
 			policyKind := getPolicyKind(policy.Source)
 
-			gw.Conditions = append(gw.Conditions, conditions.NewPolicyAncestorLimitReached(policyName))
+			gw.Conditions = addPolicyAncestorLimitCondition(gw.Conditions, policyName, policyKind)
 			LogAncestorLimitReached(logger, policyName, policyKind, gwNsName.String())
 
 			// Mark this gateway as invalid for the policy due to ancestor limits
@@ -144,10 +210,6 @@ func attachPolicyToService(
 		if !gw.Valid {
 			policy.InvalidForGateways[gwNsName] = struct{}{}
 			ancestor.Conditions = []conditions.Condition{conditions.NewPolicyTargetNotFound("Parent Gateway is invalid")}
-			if ancestorsContainsAncestorRef(policy.Ancestors, ancestor.Ancestor) {
-				continue
-			}
-
 			policy.Ancestors = append(policy.Ancestors, ancestor)
 			continue
 		}
@@ -185,7 +247,7 @@ func attachPolicyToRoute(
 		policyKind := getPolicyKind(policy.Source)
 		routeName := getAncestorName(ancestorRef)
 
-		route.Conditions = append(route.Conditions, conditions.NewPolicyAncestorLimitReached(policyName))
+		route.Conditions = addPolicyAncestorLimitCondition(route.Conditions, policyName, policyKind)
 		LogAncestorLimitReached(logger, policyName, policyKind, routeName)
 
 		return
@@ -258,7 +320,7 @@ func attachPolicyToGateway(
 		policyKind := getPolicyKind(policy.Source)
 
 		if exists {
-			gw.Conditions = append(gw.Conditions, conditions.NewPolicyAncestorLimitReached(policyName))
+			gw.Conditions = addPolicyAncestorLimitCondition(gw.Conditions, policyName, policyKind)
 		} else {
 			// Situation where gateway target is not found and the ancestors slice is full so I cannot add the condition.
 			// Log in the controller log.