From 975b7fe474737bcf0eed2a4834828571802a8b41 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Wed, 6 Aug 2025 14:52:55 -0700 Subject: [PATCH 01/18] Fix policy attachment when ancestors slice is full --- internal/controller/state/change_processor.go | 1 + .../controller/state/conditions/conditions.go | 20 +++ .../state/conditions/conditions_test.go | 14 +++ .../state/dataplane/configuration.go | 9 +- .../state/dataplane/configuration_test.go | 21 +++- .../state/graph/backend_tls_policy.go | 41 +++++-- .../state/graph/backend_tls_policy_test.go | 6 +- internal/controller/state/graph/graph.go | 7 +- internal/controller/state/graph/graph_test.go | 2 + .../state/graph/multiple_gateways_test.go | 3 + internal/controller/state/graph/policies.go | 114 ++++++++++++++---- .../controller/state/graph/policies_test.go | 1 - .../controller/state/graph/policy_ancestor.go | 43 +++++++ 13 files changed, 237 insertions(+), 45 deletions(-) diff --git a/internal/controller/state/change_processor.go b/internal/controller/state/change_processor.go index a89d956198..f3184adde8 100644 --- a/internal/controller/state/change_processor.go +++ b/internal/controller/state/change_processor.go @@ -268,6 +268,7 @@ func (c *ChangeProcessorImpl) Process() *graph.Graph { c.cfg.GatewayClassName, c.cfg.PlusSecrets, c.cfg.Validators, + c.cfg.Logger, ) return c.latestGraph diff --git a/internal/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go index a9d418847e..feca03c30a 100644 --- a/internal/controller/state/conditions/conditions.go +++ b/internal/controller/state/conditions/conditions.go @@ -138,6 +138,15 @@ const ( // GatewayReasonParamsRefInvalid is used with the "GatewayResolvedRefs" condition when the // parametersRef resource is invalid. GatewayReasonParamsRefInvalid v1.GatewayConditionReason = "ParametersRefInvalid" + + // PolicyReasonAncestorLimitReached is used with the "PolicyAccepted" condition when a policy + // cannot be applied because the ancestor status list has reached the maximum size of 16. + PolicyReasonAncestorLimitReached v1alpha2.PolicyConditionReason = "AncestorLimitReached" + + // 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" ) // Condition defines a condition to be reported in the status of resources. @@ -968,6 +977,17 @@ 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 { + return Condition{ + Type: string(v1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(PolicyReasonAncestorLimitReached), + Message: PolicyMessageAncestorLimitReached, + } +} + // NewPolicyNotAcceptedTargetConflict returns a Condition that indicates that the Policy is not accepted // because the target resource has a conflict with another resource when attempting to apply this policy. func NewPolicyNotAcceptedTargetConflict(msg string) Condition { diff --git a/internal/controller/state/conditions/conditions_test.go b/internal/controller/state/conditions/conditions_test.go index 45ebcc5808..adc9a00834 100644 --- a/internal/controller/state/conditions/conditions_test.go +++ b/internal/controller/state/conditions/conditions_test.go @@ -5,6 +5,7 @@ import ( . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/gateway-api/apis/v1alpha2" ) func TestDeduplicateConditions(t *testing.T) { @@ -145,3 +146,16 @@ func TestHasMatchingCondition(t *testing.T) { }) } } + +func TestNewPolicyAncestorLimitReached(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + policyName := "test-policy" + condition := NewPolicyAncestorLimitReached(policyName) + + g.Expect(condition.Type).To(Equal(string(v1alpha2.PolicyConditionAccepted))) + g.Expect(condition.Status).To(Equal(metav1.ConditionFalse)) + g.Expect(condition.Reason).To(Equal(string(PolicyReasonAncestorLimitReached))) + g.Expect(condition.Message).To(Equal(PolicyMessageAncestorLimitReached)) +} diff --git a/internal/controller/state/dataplane/configuration.go b/internal/controller/state/dataplane/configuration.go index 29ba14d15e..94f5f0819a 100644 --- a/internal/controller/state/dataplane/configuration.go +++ b/internal/controller/state/dataplane/configuration.go @@ -390,7 +390,7 @@ func newBackendGroup( UpstreamName: ref.ServicePortReference(), Weight: ref.Weight, Valid: valid, - VerifyTLS: convertBackendTLS(ref.BackendTLSPolicy), + VerifyTLS: convertBackendTLS(ref.BackendTLSPolicy, gatewayName), }) } @@ -401,10 +401,15 @@ func newBackendGroup( } } -func convertBackendTLS(btp *graph.BackendTLSPolicy) *VerifyTLS { +func convertBackendTLS(btp *graph.BackendTLSPolicy, gwNsName types.NamespacedName) *VerifyTLS { if btp == nil || !btp.Valid { return nil } + + if !slices.Contains(btp.Gateways, gwNsName) { + return nil + } + verify := &VerifyTLS{} if btp.CaCertRef.Name != "" { verify.CertBundleID = generateCertBundleID(btp.CaCertRef) diff --git a/internal/controller/state/dataplane/configuration_test.go b/internal/controller/state/dataplane/configuration_test.go index 9b3b190759..75fefc982a 100644 --- a/internal/controller/state/dataplane/configuration_test.go +++ b/internal/controller/state/dataplane/configuration_test.go @@ -607,6 +607,7 @@ func TestBuildConfiguration(t *testing.T) { }, CaCertRef: types.NamespacedName{Namespace: "test", Name: "configmap-1"}, Valid: true, + Gateways: []types.NamespacedName{gatewayNsName}, } expHTTPSHR8Groups[0].Backends[0].VerifyTLS = &VerifyTLS{ @@ -661,6 +662,7 @@ func TestBuildConfiguration(t *testing.T) { }, CaCertRef: types.NamespacedName{Namespace: "test", Name: "configmap-2"}, Valid: true, + Gateways: []types.NamespacedName{gatewayNsName}, } expHTTPSHR9Groups[0].Backends[0].VerifyTLS = &VerifyTLS{ @@ -3569,6 +3571,9 @@ func TestHostnameMoreSpecific(t *testing.T) { func TestConvertBackendTLS(t *testing.T) { t.Parallel() + + testGateway := types.NamespacedName{Namespace: "test", Name: "gateway"} + btpCaCertRefs := &graph.BackendTLSPolicy{ Source: &v1alpha3.BackendTLSPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -3588,6 +3593,7 @@ func TestConvertBackendTLS(t *testing.T) { }, Valid: true, CaCertRef: types.NamespacedName{Namespace: "test", Name: "ca-cert"}, + Gateways: []types.NamespacedName{testGateway}, } btpWellKnownCerts := &graph.BackendTLSPolicy{ @@ -3598,7 +3604,8 @@ func TestConvertBackendTLS(t *testing.T) { }, }, }, - Valid: true, + Valid: true, + Gateways: []types.NamespacedName{testGateway}, } expectedWithCertPath := &VerifyTLS{ @@ -3615,31 +3622,41 @@ func TestConvertBackendTLS(t *testing.T) { tests := []struct { btp *graph.BackendTLSPolicy + gwNsName types.NamespacedName expected *VerifyTLS msg string }{ { btp: nil, + gwNsName: testGateway, expected: nil, msg: "nil backend tls policy", }, { btp: btpCaCertRefs, + gwNsName: testGateway, expected: expectedWithCertPath, msg: "normal case with cert path", }, { btp: btpWellKnownCerts, + gwNsName: testGateway, expected: expectedWithWellKnownCerts, msg: "normal case no cert path", }, + { + btp: btpCaCertRefs, + gwNsName: types.NamespacedName{Namespace: "test", Name: "unsupported-gateway"}, + expected: nil, + msg: "gateway not supported by policy", + }, } for _, tc := range tests { t.Run(tc.msg, func(t *testing.T) { t.Parallel() g := NewWithT(t) - g.Expect(convertBackendTLS(tc.btp)).To(Equal(tc.expected)) + g.Expect(convertBackendTLS(tc.btp, tc.gwNsName)).To(Equal(tc.expected)) }) } } diff --git a/internal/controller/state/graph/backend_tls_policy.go b/internal/controller/state/graph/backend_tls_policy.go index a0e4eb67d3..f391019c53 100644 --- a/internal/controller/state/graph/backend_tls_policy.go +++ b/internal/controller/state/graph/backend_tls_policy.go @@ -18,7 +18,8 @@ type BackendTLSPolicy struct { Source *v1alpha3.BackendTLSPolicy // CaCertRef is the name of the ConfigMap that contains the CA certificate. CaCertRef types.NamespacedName - // Gateways are the names of the Gateways that are being checked for this BackendTLSPolicy. + // Gateways are the names of the Gateways for which this BackendTLSPolicy is effectively applied. + // Only contains gateways where the policy can be applied (not limited by ancestor status). Gateways []types.NamespacedName // Conditions include Conditions for the BackendTLSPolicy. Conditions []conditions.Condition @@ -68,16 +69,13 @@ func validateBackendTLSPolicy( backendTLSPolicy *v1alpha3.BackendTLSPolicy, configMapResolver *configMapResolver, secretResolver *secretResolver, - ctlrName string, + _ string, ) (valid, ignored bool, conds []conditions.Condition) { valid = true ignored = false - // FIXME (kate-osborn): https://github.com/nginx/nginx-gateway-fabric/issues/1987 - if backendTLSPolicyAncestorsFull(backendTLSPolicy.Status.Ancestors, ctlrName) { - valid = false - ignored = true - } + // 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) if err := validateBackendTLSHostname(backendTLSPolicy); err != nil { valid = false @@ -186,10 +184,12 @@ func validateBackendTLSWellKnownCACerts(btp *v1alpha3.BackendTLSPolicy) error { func addGatewaysForBackendTLSPolicies( backendTLSPolicies map[types.NamespacedName]*BackendTLSPolicy, services map[types.NamespacedName]*ReferencedService, + ctlrName string, ) { for _, backendTLSPolicy := range backendTLSPolicies { - gateways := make(map[types.NamespacedName]struct{}) + potentialGateways := make(map[types.NamespacedName]struct{}) + // First, collect all potential gateways for this policy for _, refs := range backendTLSPolicy.Source.Spec.TargetRefs { if refs.Kind != kinds.Service { continue @@ -201,13 +201,32 @@ func addGatewaysForBackendTLSPolicies( } for gateway := range referencedServices.GatewayNsNames { - gateways[gateway] = struct{}{} + potentialGateways[gateway] = struct{}{} } } } - for gateway := range gateways { - backendTLSPolicy.Gateways = append(backendTLSPolicy.Gateways, gateway) + // 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) + + // Check ancestor limit for BackendTLS policy + isFull := backendTLSPolicyAncestorsFull( + backendTLSPolicy.Source.Status.Ancestors, + ctlrName, + ) + + if isFull { + policyName := backendTLSPolicy.Source.Namespace + "/" + backendTLSPolicy.Source.Name + gatewayName := getAncestorName(proposedAncestor) + LogAncestorLimitReached(policyName, "BackendTLSPolicy", gatewayName) + + continue + } + + // Gateway can be effectively used by this policy + backendTLSPolicy.Gateways = append(backendTLSPolicy.Gateways, gatewayNsName) } } } diff --git a/internal/controller/state/graph/backend_tls_policy_test.go b/internal/controller/state/graph/backend_tls_policy_test.go index 0adca7ba67..0136e08fc6 100644 --- a/internal/controller/state/graph/backend_tls_policy_test.go +++ b/internal/controller/state/graph/backend_tls_policy_test.go @@ -399,7 +399,7 @@ func TestValidateBackendTLSPolicy(t *testing.T) { }, }, { - name: "invalid case with too many ancestors", + name: "valid case with many ancestors (ancestor limit now handled during gateway assignment)", tlsPolicy: &v1alpha3.BackendTLSPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: "tls-policy", @@ -416,7 +416,7 @@ func TestValidateBackendTLSPolicy(t *testing.T) { Ancestors: ancestors, }, }, - ignored: true, + isValid: true, }, } @@ -660,7 +660,7 @@ func TestAddGatewaysForBackendTLSPolicies(t *testing.T) { g := NewWithT(t) t.Run(test.name, func(t *testing.T) { t.Parallel() - addGatewaysForBackendTLSPolicies(test.backendTLSPolicies, test.services) + addGatewaysForBackendTLSPolicies(test.backendTLSPolicies, test.services, "nginx-gateway") g.Expect(helpers.Diff(test.backendTLSPolicies, test.expected)).To(BeEmpty()) }) } diff --git a/internal/controller/state/graph/graph.go b/internal/controller/state/graph/graph.go index 52cb047ae2..54e30f1149 100644 --- a/internal/controller/state/graph/graph.go +++ b/internal/controller/state/graph/graph.go @@ -13,6 +13,8 @@ import ( "sigs.k8s.io/gateway-api/apis/v1alpha3" "sigs.k8s.io/gateway-api/apis/v1beta1" + "github.com/go-logr/logr" + ngfAPIv1alpha1 "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1" ngfAPIv1alpha2 "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha2" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies" @@ -198,7 +200,10 @@ func BuildGraph( gcName string, plusSecrets map[types.NamespacedName][]PlusSecretFile, validators validation.Validators, + logger logr.Logger, ) *Graph { + // Set the logger for the graph package + SetLogger(logger) processedGwClasses, gcExists := processGatewayClasses(state.GatewayClasses, gcName, controllerName) if gcExists && processedGwClasses.Winner == nil { // configured GatewayClass does not reference this controller @@ -269,7 +274,7 @@ func BuildGraph( referencedServices := buildReferencedServices(routes, l4routes, gws) - addGatewaysForBackendTLSPolicies(processedBackendTLSPolicies, referencedServices) + addGatewaysForBackendTLSPolicies(processedBackendTLSPolicies, referencedServices, controllerName) // policies must be processed last because they rely on the state of the other resources in the graph processedPolicies := processPolicies( diff --git a/internal/controller/state/graph/graph_test.go b/internal/controller/state/graph/graph_test.go index 8d51926a26..46802f3391 100644 --- a/internal/controller/state/graph/graph_test.go +++ b/internal/controller/state/graph/graph_test.go @@ -3,6 +3,7 @@ package graph import ( "testing" + "github.com/go-logr/logr" . "github.com/onsi/gomega" "github.com/onsi/gomega/format" v1 "k8s.io/api/core/v1" @@ -1309,6 +1310,7 @@ func TestBuildGraph(t *testing.T) { GenericValidator: &validationfakes.FakeGenericValidator{}, PolicyValidator: fakePolicyValidator, }, + logr.Discard(), ) g.Expect(helpers.Diff(test.expected, result)).To(BeEmpty()) diff --git a/internal/controller/state/graph/multiple_gateways_test.go b/internal/controller/state/graph/multiple_gateways_test.go index a17f5c95da..80f4a10b04 100644 --- a/internal/controller/state/graph/multiple_gateways_test.go +++ b/internal/controller/state/graph/multiple_gateways_test.go @@ -3,6 +3,7 @@ package graph import ( "testing" + "github.com/go-logr/logr" . "github.com/onsi/gomega" "github.com/onsi/gomega/format" v1 "k8s.io/api/core/v1" @@ -398,6 +399,7 @@ func Test_MultipleGateways_WithNginxProxy(t *testing.T) { GenericValidator: &validationfakes.FakeGenericValidator{}, PolicyValidator: fakePolicyValidator, }, + logr.Discard(), ) g.Expect(helpers.Diff(test.expGraph, result)).To(BeEmpty()) @@ -886,6 +888,7 @@ func Test_MultipleGateways_WithListeners(t *testing.T) { GenericValidator: &validationfakes.FakeGenericValidator{}, PolicyValidator: fakePolicyValidator, }, + logr.Discard(), ) g.Expect(helpers.Diff(test.expGraph, result)).To(BeEmpty()) diff --git a/internal/controller/state/graph/policies.go b/internal/controller/state/graph/policies.go index b5fbc0834b..b5b639eeed 100644 --- a/internal/controller/state/graph/policies.go +++ b/internal/controller/state/graph/policies.go @@ -22,6 +22,7 @@ type Policy struct { Source policies.Policy // InvalidForGateways is a map of Gateways for which this Policy is invalid for. Certain NginxProxy // configurations may result in a policy not being valid for some Gateways, but not others. + // This includes gateways that cannot accept the policy due to ancestor status limits. InvalidForGateways map[types.NamespacedName]struct{} // Ancestors is a list of ancestor objects of the Policy. Used in status. Ancestors []PolicyAncestor @@ -104,18 +105,38 @@ func attachPolicyToService( gws map[types.NamespacedName]*Gateway, ctlrName string, ) { - if ngfPolicyAncestorsFull(policy, ctlrName) { - return - } + var attachedToAnyGateway bool - var validForAGateway bool for gwNsName, gw := range gws { if _, belongsToGw := svc.GatewayNsNames[gwNsName]; !belongsToGw { continue } + ancestorRef := createParentReference(v1.GroupName, kinds.Gateway, client.ObjectKeyFromObject(gw.Source)) ancestor := PolicyAncestor{ - Ancestor: createParentReference(v1.GroupName, kinds.Gateway, client.ObjectKeyFromObject(gw.Source)), + Ancestor: ancestorRef, + } + + if _, ok := policy.InvalidForGateways[gwNsName]; ok { + continue + } + + if ancestorsContainsAncestorRef(policy.Ancestors, ancestor.Ancestor) { + // Ancestor already exists, but we should still consider this gateway as attached + attachedToAnyGateway = true + continue + } + + if ngfPolicyAncestorsFull(policy, ctlrName) { + policyName := getPolicyName(policy.Source) + policyKind := getPolicyKind(policy.Source) + + gw.Conditions = append(gw.Conditions, conditions.NewPolicyAncestorLimitReached(policyName)) + LogAncestorLimitReached(policyName, policyKind, gwNsName.String()) + + // Mark this gateway as invalid for the policy due to ancestor limits + policy.InvalidForGateways[gwNsName] = struct{}{} + continue } if !gw.Valid { @@ -129,32 +150,41 @@ func attachPolicyToService( continue } - if !ancestorsContainsAncestorRef(policy.Ancestors, ancestor.Ancestor) { - policy.Ancestors = append(policy.Ancestors, ancestor) - } - validForAGateway = true + // Gateway is valid, add ancestor and mark as attached + policy.Ancestors = append(policy.Ancestors, ancestor) + attachedToAnyGateway = true } - if validForAGateway { + // Attach policy to service if effective for at least one gateway + if attachedToAnyGateway { svc.Policies = append(svc.Policies, policy) } } func attachPolicyToRoute(policy *Policy, route *L7Route, validator validation.PolicyValidator, ctlrName string) { - if ngfPolicyAncestorsFull(policy, ctlrName) { - // FIXME (kate-osborn): https://github.com/nginx/nginx-gateway-fabric/issues/1987 - return - } - kind := v1.Kind(kinds.HTTPRoute) if route.RouteType == RouteTypeGRPC { kind = kinds.GRPCRoute } routeNsName := types.NamespacedName{Namespace: route.Source.GetNamespace(), Name: route.Source.GetName()} + ancestorRef := createParentReference(v1.GroupName, kind, routeNsName) + + // Check ancestor limit + isFull := ngfPolicyAncestorsFull(policy, ctlrName) + if isFull { + policyName := getPolicyName(policy.Source) + policyKind := getPolicyKind(policy.Source) + routeName := getAncestorName(ancestorRef) + + route.Conditions = append(route.Conditions, conditions.NewPolicyAncestorLimitReached(policyName)) + LogAncestorLimitReached(policyName, policyKind, routeName) + + return + } ancestor := PolicyAncestor{ - Ancestor: createParentReference(v1.GroupName, kind, routeNsName), + Ancestor: ancestorRef, } if !route.Valid || !route.Attachable || len(route.ParentRefs) == 0 { @@ -163,6 +193,9 @@ func attachPolicyToRoute(policy *Policy, route *L7Route, validator validation.Po return } + // Track which gateways the policy is effective for + var effectiveGateways []types.NamespacedName + // as of now, ObservabilityPolicy is the only policy that needs this check, and it only attaches to Routes for _, parentRef := range route.ParentRefs { if parentRef.Gateway != nil && parentRef.Gateway.EffectiveNginxProxy != nil { @@ -174,16 +207,19 @@ func attachPolicyToRoute(policy *Policy, route *L7Route, validator validation.Po if conds := validator.ValidateGlobalSettings(policy.Source, globalSettings); len(conds) > 0 { policy.InvalidForGateways[gw.NamespacedName] = struct{}{} ancestor.Conditions = append(ancestor.Conditions, conds...) + } else { + // Policy is effective for this gateway (not adding to InvalidForGateways) + effectiveGateways = append(effectiveGateways, gw.NamespacedName) } } } policy.Ancestors = append(policy.Ancestors, ancestor) - if len(policy.InvalidForGateways) == len(route.ParentRefs) { - return - } - route.Policies = append(route.Policies, policy) + // Only attach policy to route if it's effective for at least one gateway + if len(effectiveGateways) > 0 || len(policy.InvalidForGateways) < len(route.ParentRefs) { + route.Policies = append(route.Policies, policy) + } } func attachPolicyToGateway( @@ -192,16 +228,42 @@ func attachPolicyToGateway( gateways map[types.NamespacedName]*Gateway, ctlrName string, ) { - if ngfPolicyAncestorsFull(policy, ctlrName) { - // FIXME (kate-osborn): https://github.com/nginx/nginx-gateway-fabric/issues/1987 + ancestorRef := createParentReference(v1.GroupName, kinds.Gateway, ref.Nsname) + gw, exists := gateways[ref.Nsname] + + if _, ok := policy.InvalidForGateways[ref.Nsname]; ok { return } - ancestor := PolicyAncestor{ - Ancestor: createParentReference(v1.GroupName, kinds.Gateway, ref.Nsname), + if ancestorsContainsAncestorRef(policy.Ancestors, ancestorRef) { + // Ancestor already exists, but still attach policy to gateway if it's valid + if exists && gw != nil && gw.Valid && gw.Source != nil { + gw.Policies = append(gw.Policies, policy) + } + return } + isFull := ngfPolicyAncestorsFull(policy, ctlrName) + if isFull { + ancestorName := getAncestorName(ancestorRef) + policyName := getPolicyName(policy.Source) + policyKind := getPolicyKind(policy.Source) + + if exists { + gw.Conditions = append(gw.Conditions, conditions.NewPolicyAncestorLimitReached(policyName)) + } 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. + logger.Info("Gateway target not found and ancestors slice is full.", "policy", policyName, "ancestor", ancestorName) + } + LogAncestorLimitReached(policyName, policyKind, ancestorName) - gw, exists := gateways[ref.Nsname] + policy.InvalidForGateways[ref.Nsname] = struct{}{} + return + } + + ancestor := PolicyAncestor{ + Ancestor: ancestorRef, + } if !exists || (gw != nil && gw.Source == nil) { policy.InvalidForGateways[ref.Nsname] = struct{}{} @@ -217,6 +279,8 @@ func attachPolicyToGateway( return } + // Policy is effective for this gateway (not adding to InvalidForGateways) + policy.Ancestors = append(policy.Ancestors, ancestor) gw.Policies = append(gw.Policies, policy) } diff --git a/internal/controller/state/graph/policies_test.go b/internal/controller/state/graph/policies_test.go index 6108420042..e426bcdf6c 100644 --- a/internal/controller/state/graph/policies_test.go +++ b/internal/controller/state/graph/policies_test.go @@ -575,7 +575,6 @@ func TestAttachPolicyToGateway(t *testing.T) { gws: newGatewayMap(true, []types.NamespacedName{gatewayNsName}), expAncestors: []PolicyAncestor{ {Ancestor: getGatewayParentRef(gatewayNsName)}, - {Ancestor: getGatewayParentRef(gatewayNsName)}, }, expAttached: true, }, diff --git a/internal/controller/state/graph/policy_ancestor.go b/internal/controller/state/graph/policy_ancestor.go index 698b887511..89340baa44 100644 --- a/internal/controller/state/graph/policy_ancestor.go +++ b/internal/controller/state/graph/policy_ancestor.go @@ -5,11 +5,27 @@ import ( v1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" + "github.com/go-logr/logr" + + "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" ) const maxAncestors = 16 +// Package-level logger. +var logger logr.Logger + +// SetLogger sets the logger for the graph package. +func SetLogger(l logr.Logger) { + logger = l +} + +// LogAncestorLimitReached logs when a policy ancestor limit is reached. +func LogAncestorLimitReached(policyName, policyKind, ancestorName string) { + logger.Info("Policy ancestor limit reached", "policy", policyName, "policyKind", policyKind, "ancestor", ancestorName) +} + // backendTLSPolicyAncestorsFull returns whether or not an ancestor list is full. A list is not full when: // - the number of current ancestors is less than the maximum allowed // - an entry for an NGF managed resource already exists in the ancestor list. This means that we are overwriting @@ -95,3 +111,30 @@ func parentRefEqual(ref1, ref2 v1.ParentReference) bool { return true } + +// Helper functions to eliminate code duplication + +// getAncestorName generates a human-readable name for an ancestor from a ParentReference. +// Returns namespace/name format if namespace is specified, otherwise just name. +func getAncestorName(ancestorRef v1.ParentReference) string { + ancestorName := string(ancestorRef.Name) + if ancestorRef.Namespace != nil { + ancestorName = string(*ancestorRef.Namespace) + "/" + ancestorName + } + return ancestorName +} + +// getPolicyName generates a human-readable name for a policy in namespace/name format. +func getPolicyName(policy policies.Policy) string { + return policy.GetNamespace() + "/" + policy.GetName() +} + +// getPolicyKind returns the policy kind with defensive programming for test environments. +// Returns "Policy" as fallback if GetObjectKind() returns nil. +func getPolicyKind(policy policies.Policy) string { + policyKind := "Policy" // Default fallback + if objKind := policy.GetObjectKind(); objKind != nil { + policyKind = objKind.GroupVersionKind().Kind + } + return policyKind +} From b8e5c5c241888a2eb37d41d42b637db99177ae99 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Wed, 6 Aug 2025 15:25:58 -0700 Subject: [PATCH 02/18] Add more unit tests --- .../state/graph/backend_tls_policy.go | 13 +- .../state/graph/backend_tls_policy_test.go | 288 +++++++++++++- internal/controller/state/graph/graph.go | 6 +- internal/controller/state/graph/policies.go | 25 +- .../controller/state/graph/policies_test.go | 365 +++++++++++++++++- .../controller/state/graph/policy_ancestor.go | 10 +- 6 files changed, 680 insertions(+), 27 deletions(-) diff --git a/internal/controller/state/graph/backend_tls_policy.go b/internal/controller/state/graph/backend_tls_policy.go index f391019c53..e6d55fea21 100644 --- a/internal/controller/state/graph/backend_tls_policy.go +++ b/internal/controller/state/graph/backend_tls_policy.go @@ -4,6 +4,7 @@ import ( "fmt" "slices" + "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" v1 "sigs.k8s.io/gateway-api/apis/v1" @@ -185,6 +186,8 @@ 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 { potentialGateways := make(map[types.NamespacedName]struct{}) @@ -220,7 +223,15 @@ func addGatewaysForBackendTLSPolicies( if isFull { policyName := backendTLSPolicy.Source.Namespace + "/" + backendTLSPolicy.Source.Name gatewayName := getAncestorName(proposedAncestor) - LogAncestorLimitReached(policyName, "BackendTLSPolicy", gatewayName) + gateway, ok := gateways[gatewayNsName] + if ok { + gateway.Conditions = append(gateway.Conditions, conditions.NewPolicyAncestorLimitReached(policyName)) + } 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) + } + + LogAncestorLimitReached(logger, policyName, "BackendTLSPolicy", gatewayName) continue } diff --git a/internal/controller/state/graph/backend_tls_policy_test.go b/internal/controller/state/graph/backend_tls_policy_test.go index 0136e08fc6..444f64a63c 100644 --- a/internal/controller/state/graph/backend_tls_policy_test.go +++ b/internal/controller/state/graph/backend_tls_policy_test.go @@ -1,8 +1,10 @@ package graph import ( + "bytes" "testing" + "github.com/go-logr/logr" . "github.com/onsi/gomega" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -11,6 +13,7 @@ import ( "sigs.k8s.io/gateway-api/apis/v1alpha2" "sigs.k8s.io/gateway-api/apis/v1alpha3" + "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/conditions" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/kinds" ) @@ -660,8 +663,291 @@ func TestAddGatewaysForBackendTLSPolicies(t *testing.T) { g := NewWithT(t) t.Run(test.name, func(t *testing.T) { t.Parallel() - addGatewaysForBackendTLSPolicies(test.backendTLSPolicies, test.services, "nginx-gateway") + addGatewaysForBackendTLSPolicies(test.backendTLSPolicies, test.services, "nginx-gateway", nil, logr.Discard()) g.Expect(helpers.Diff(test.backendTLSPolicies, test.expected)).To(BeEmpty()) }) } } + +func TestAddGatewaysForBackendTLSPoliciesAncestorLimit(t *testing.T) { + t.Parallel() + + // Create a test logger that captures log output + var logBuf bytes.Buffer + testLogger := logr.New(&testLogSink{buffer: &logBuf}) + + // Create BackendTLSPolicy with 16 ancestors (full) + getAncestorRef := func(ctlrName, parentName string) v1alpha2.PolicyAncestorStatus { + return v1alpha2.PolicyAncestorStatus{ + ControllerName: gatewayv1.GatewayController(ctlrName), + AncestorRef: gatewayv1.ParentReference{ + Name: gatewayv1.ObjectName(parentName), + Namespace: helpers.GetPointer(gatewayv1.Namespace("test")), + Group: helpers.GetPointer[gatewayv1.Group](gatewayv1.GroupName), + Kind: helpers.GetPointer[gatewayv1.Kind](kinds.Gateway), + }, + } + } + + // Create 16 ancestors from different controllers to simulate full list + fullAncestors := make([]v1alpha2.PolicyAncestorStatus, 16) + for i := range 16 { + fullAncestors[i] = getAncestorRef("other-controller", "other-gateway") + } + + btpWithFullAncestors := &BackendTLSPolicy{ + Source: &v1alpha3.BackendTLSPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "btp-full-ancestors", + Namespace: "test", + }, + Spec: v1alpha3.BackendTLSPolicySpec{ + TargetRefs: []v1alpha2.LocalPolicyTargetReferenceWithSectionName{ + { + LocalPolicyTargetReference: v1alpha2.LocalPolicyTargetReference{ + Kind: "Service", + Name: "service1", + }, + }, + }, + }, + Status: v1alpha2.PolicyStatus{ + Ancestors: fullAncestors, + }, + }, + } + + btpNormal := &BackendTLSPolicy{ + Source: &v1alpha3.BackendTLSPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "btp-normal", + Namespace: "test", + }, + Spec: v1alpha3.BackendTLSPolicySpec{ + TargetRefs: []v1alpha2.LocalPolicyTargetReferenceWithSectionName{ + { + LocalPolicyTargetReference: v1alpha2.LocalPolicyTargetReference{ + Kind: "Service", + Name: "service2", + }, + }, + }, + }, + Status: v1alpha2.PolicyStatus{ + Ancestors: []v1alpha2.PolicyAncestorStatus{}, // Empty ancestors list + }, + }, + } + + services := map[types.NamespacedName]*ReferencedService{ + {Namespace: "test", Name: "service1"}: { + GatewayNsNames: map[types.NamespacedName]struct{}{ + {Namespace: "test", Name: "gateway1"}: {}, + }, + }, + {Namespace: "test", Name: "service2"}: { + GatewayNsNames: map[types.NamespacedName]struct{}{ + {Namespace: "test", Name: "gateway2"}: {}, + }, + }, + } + + // Create gateways - one will receive ancestor limit condition + gateways := map[types.NamespacedName]*Gateway{ + {Namespace: "test", Name: "gateway1"}: { + Source: &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Name: "gateway1", Namespace: "test"}, + }, + Conditions: []conditions.Condition{}, // Start with empty conditions + }, + {Namespace: "test", Name: "gateway2"}: { + Source: &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Name: "gateway2", Namespace: "test"}, + }, + Conditions: []conditions.Condition{}, // Start with empty conditions + }, + } + + backendTLSPolicies := map[types.NamespacedName]*BackendTLSPolicy{ + {Namespace: "test", Name: "btp-full-ancestors"}: btpWithFullAncestors, + {Namespace: "test", Name: "btp-normal"}: btpNormal, + } + + g := NewWithT(t) + + // Execute the function + addGatewaysForBackendTLSPolicies(backendTLSPolicies, services, "nginx-gateway", gateways, testLogger) + + // Verify that the policy with full ancestors doesn't get any gateways assigned + g.Expect(btpWithFullAncestors.Gateways).To(BeEmpty(), "Policy with full ancestors should not get gateways assigned") + + // Verify that the normal policy gets its gateway assigned + g.Expect(btpNormal.Gateways).To(HaveLen(1)) + g.Expect(btpNormal.Gateways[0]).To(Equal(types.NamespacedName{Namespace: "test", Name: "gateway2"})) + + // Verify that gateway1 received the ancestor limit condition + gateway1 := gateways[types.NamespacedName{Namespace: "test", Name: "gateway1"}] + g.Expect(gateway1.Conditions).To(HaveLen(1), "Gateway should have received ancestor limit condition") + + condition := gateway1.Conditions[0] + g.Expect(condition.Type).To(Equal(string(v1alpha2.PolicyConditionAccepted))) + g.Expect(condition.Status).To(Equal(metav1.ConditionFalse)) + g.Expect(condition.Reason).To(Equal(string(conditions.PolicyReasonAncestorLimitReached))) + g.Expect(condition.Message).To(ContainSubstring("ancestor status list has reached the maximum size of 16")) + + // Verify that gateway2 did not receive any conditions (normal case) + gateway2 := gateways[types.NamespacedName{Namespace: "test", Name: "gateway2"}] + g.Expect(gateway2.Conditions).To(BeEmpty(), "Normal gateway should not have conditions") + + // Verify logging function works - test the logging function directly + LogAncestorLimitReached(testLogger, "test/btp-full-ancestors", "BackendTLSPolicy", "test/gateway1") + logOutput := logBuf.String() + + g.Expect(logOutput).To(ContainSubstring("Policy ancestor limit reached")) + g.Expect(logOutput).To(ContainSubstring("policy=test/btp-full-ancestors")) + g.Expect(logOutput).To(ContainSubstring("policyKind=BackendTLSPolicy")) + g.Expect(logOutput).To(ContainSubstring("ancestor=test/gateway1")) +} + +func TestBackendTLSPolicyAncestorsFullFunc(t *testing.T) { + t.Parallel() + + getAncestorRef := func(ctlrName, parentName string) v1alpha2.PolicyAncestorStatus { + return v1alpha2.PolicyAncestorStatus{ + ControllerName: gatewayv1.GatewayController(ctlrName), + AncestorRef: gatewayv1.ParentReference{ + Name: gatewayv1.ObjectName(parentName), + Namespace: helpers.GetPointer(gatewayv1.Namespace("test")), + Group: helpers.GetPointer[gatewayv1.Group](gatewayv1.GroupName), + Kind: helpers.GetPointer[gatewayv1.Kind](kinds.Gateway), + }, + } + } + + tests := []struct { + name string + ctlrName string + ancestors []v1alpha2.PolicyAncestorStatus + expected bool + }{ + { + name: "empty ancestors list", + ancestors: []v1alpha2.PolicyAncestorStatus{}, + ctlrName: "nginx-gateway", + expected: false, + }, + { + name: "less than 16 ancestors", + ancestors: []v1alpha2.PolicyAncestorStatus{ + getAncestorRef("other-controller", "gateway1"), + getAncestorRef("other-controller", "gateway2"), + }, + ctlrName: "nginx-gateway", + expected: false, + }, + { + name: "exactly 16 ancestors, none from our controller", + ancestors: func() []v1alpha2.PolicyAncestorStatus { + ancestors := make([]v1alpha2.PolicyAncestorStatus, 16) + for i := range 16 { + ancestors[i] = getAncestorRef("other-controller", "gateway") + } + return ancestors + }(), + ctlrName: "nginx-gateway", + expected: true, + }, + { + name: "exactly 16 ancestors, one from our controller", + ancestors: func() []v1alpha2.PolicyAncestorStatus { + ancestors := make([]v1alpha2.PolicyAncestorStatus, 16) + for i := range 15 { + ancestors[i] = getAncestorRef("other-controller", "gateway") + } + ancestors[15] = getAncestorRef("nginx-gateway", "our-gateway") + return ancestors + }(), + ctlrName: "nginx-gateway", + expected: false, // Not full because we can overwrite our own entry + }, + { + name: "more than 16 ancestors, none from our controller", + ancestors: func() []v1alpha2.PolicyAncestorStatus { + ancestors := make([]v1alpha2.PolicyAncestorStatus, 20) + for i := range 20 { + ancestors[i] = getAncestorRef("other-controller", "gateway") + } + return ancestors + }(), + ctlrName: "nginx-gateway", + expected: true, + }, + { + name: "more than 16 ancestors, one from our controller", + ancestors: func() []v1alpha2.PolicyAncestorStatus { + ancestors := make([]v1alpha2.PolicyAncestorStatus, 20) + for i := range 19 { + ancestors[i] = getAncestorRef("other-controller", "gateway") + } + ancestors[19] = getAncestorRef("nginx-gateway", "our-gateway") + return ancestors + }(), + ctlrName: "nginx-gateway", + expected: false, // Not full because we can overwrite our own entry + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + result := backendTLSPolicyAncestorsFull(test.ancestors, test.ctlrName) + g.Expect(result).To(Equal(test.expected)) + }) + } +} + +// testLogSink implements logr.LogSink for testing. +type testLogSink struct { + buffer *bytes.Buffer +} + +func (s *testLogSink) Init(_ logr.RuntimeInfo) {} + +func (s *testLogSink) Enabled(_ int) bool { + return true +} + +func (s *testLogSink) Info(_ int, msg string, keysAndValues ...interface{}) { + s.buffer.WriteString(msg) + for i := 0; i < len(keysAndValues); i += 2 { + if i+1 < len(keysAndValues) { + s.buffer.WriteString(" ") + if key, ok := keysAndValues[i].(string); ok { + s.buffer.WriteString(key) + } + s.buffer.WriteString("=") + if value, ok := keysAndValues[i+1].(string); ok { + s.buffer.WriteString(value) + } + } + } + s.buffer.WriteString("\n") +} + +func (s *testLogSink) Error(err error, msg string, _ ...interface{}) { + s.buffer.WriteString("ERROR: ") + s.buffer.WriteString(msg) + s.buffer.WriteString(" error=") + s.buffer.WriteString(err.Error()) + s.buffer.WriteString("\n") +} + +func (s *testLogSink) WithValues(_ ...interface{}) logr.LogSink { + return s +} + +func (s *testLogSink) WithName(_ string) logr.LogSink { + return s +} diff --git a/internal/controller/state/graph/graph.go b/internal/controller/state/graph/graph.go index 54e30f1149..7c928d185d 100644 --- a/internal/controller/state/graph/graph.go +++ b/internal/controller/state/graph/graph.go @@ -202,8 +202,6 @@ func BuildGraph( validators validation.Validators, logger logr.Logger, ) *Graph { - // Set the logger for the graph package - SetLogger(logger) processedGwClasses, gcExists := processGatewayClasses(state.GatewayClasses, gcName, controllerName) if gcExists && processedGwClasses.Winner == nil { // configured GatewayClass does not reference this controller @@ -274,7 +272,7 @@ func BuildGraph( referencedServices := buildReferencedServices(routes, l4routes, gws) - addGatewaysForBackendTLSPolicies(processedBackendTLSPolicies, referencedServices, controllerName) + addGatewaysForBackendTLSPolicies(processedBackendTLSPolicies, referencedServices, controllerName, gws, logger) // policies must be processed last because they rely on the state of the other resources in the graph processedPolicies := processPolicies( @@ -307,7 +305,7 @@ func BuildGraph( PlusSecrets: plusSecrets, } - g.attachPolicies(validators.PolicyValidator, controllerName) + g.attachPolicies(validators.PolicyValidator, controllerName, logger) return g } diff --git a/internal/controller/state/graph/policies.go b/internal/controller/state/graph/policies.go index b5b639eeed..46b94f3c2b 100644 --- a/internal/controller/state/graph/policies.go +++ b/internal/controller/state/graph/policies.go @@ -4,6 +4,7 @@ import ( "fmt" "sort" + "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -70,7 +71,7 @@ const ( ) // attachPolicies attaches the graph's processed policies to the resources they target. It modifies the graph in place. -func (g *Graph) attachPolicies(validator validation.PolicyValidator, ctlrName string) { +func (g *Graph) attachPolicies(validator validation.PolicyValidator, ctlrName string, logger logr.Logger) { if len(g.Gateways) == 0 { return } @@ -79,21 +80,21 @@ func (g *Graph) attachPolicies(validator validation.PolicyValidator, ctlrName st for _, ref := range policy.TargetRefs { switch ref.Kind { case kinds.Gateway: - attachPolicyToGateway(policy, ref, g.Gateways, ctlrName) + attachPolicyToGateway(policy, ref, g.Gateways, ctlrName, logger) case kinds.HTTPRoute, kinds.GRPCRoute: route, exists := g.Routes[routeKeyForKind(ref.Kind, ref.Nsname)] if !exists { continue } - attachPolicyToRoute(policy, route, validator, ctlrName) + attachPolicyToRoute(policy, route, validator, ctlrName, logger) case kinds.Service: svc, exists := g.ReferencedServices[ref.Nsname] if !exists { continue } - attachPolicyToService(policy, svc, g.Gateways, ctlrName) + attachPolicyToService(policy, svc, g.Gateways, ctlrName, logger) } } } @@ -104,6 +105,7 @@ func attachPolicyToService( svc *ReferencedService, gws map[types.NamespacedName]*Gateway, ctlrName string, + logger logr.Logger, ) { var attachedToAnyGateway bool @@ -132,7 +134,7 @@ func attachPolicyToService( policyKind := getPolicyKind(policy.Source) gw.Conditions = append(gw.Conditions, conditions.NewPolicyAncestorLimitReached(policyName)) - LogAncestorLimitReached(policyName, policyKind, gwNsName.String()) + LogAncestorLimitReached(logger, policyName, policyKind, gwNsName.String()) // Mark this gateway as invalid for the policy due to ancestor limits policy.InvalidForGateways[gwNsName] = struct{}{} @@ -161,7 +163,13 @@ func attachPolicyToService( } } -func attachPolicyToRoute(policy *Policy, route *L7Route, validator validation.PolicyValidator, ctlrName string) { +func attachPolicyToRoute( + policy *Policy, + route *L7Route, + validator validation.PolicyValidator, + ctlrName string, + logger logr.Logger, +) { kind := v1.Kind(kinds.HTTPRoute) if route.RouteType == RouteTypeGRPC { kind = kinds.GRPCRoute @@ -178,7 +186,7 @@ func attachPolicyToRoute(policy *Policy, route *L7Route, validator validation.Po routeName := getAncestorName(ancestorRef) route.Conditions = append(route.Conditions, conditions.NewPolicyAncestorLimitReached(policyName)) - LogAncestorLimitReached(policyName, policyKind, routeName) + LogAncestorLimitReached(logger, policyName, policyKind, routeName) return } @@ -227,6 +235,7 @@ func attachPolicyToGateway( ref PolicyTargetRef, gateways map[types.NamespacedName]*Gateway, ctlrName string, + logger logr.Logger, ) { ancestorRef := createParentReference(v1.GroupName, kinds.Gateway, ref.Nsname) gw, exists := gateways[ref.Nsname] @@ -255,7 +264,7 @@ func attachPolicyToGateway( // Log in the controller log. logger.Info("Gateway target not found and ancestors slice is full.", "policy", policyName, "ancestor", ancestorName) } - LogAncestorLimitReached(policyName, policyKind, ancestorName) + LogAncestorLimitReached(logger, policyName, policyKind, ancestorName) policy.InvalidForGateways[ref.Nsname] = struct{}{} return diff --git a/internal/controller/state/graph/policies_test.go b/internal/controller/state/graph/policies_test.go index e426bcdf6c..82044dba36 100644 --- a/internal/controller/state/graph/policies_test.go +++ b/internal/controller/state/graph/policies_test.go @@ -1,9 +1,11 @@ package graph import ( + "bytes" "slices" "testing" + "github.com/go-logr/logr" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -241,7 +243,7 @@ func TestAttachPolicies(t *testing.T) { NGFPolicies: test.ngfPolicies, } - graph.attachPolicies(nil, "nginx-gateway") + graph.attachPolicies(nil, "nginx-gateway", logr.Discard()) for _, expect := range test.expects { expect(g, graph) } @@ -498,7 +500,7 @@ func TestAttachPolicyToRoute(t *testing.T) { t.Parallel() g := NewWithT(t) - attachPolicyToRoute(test.policy, test.route, test.validator, "nginx-gateway") + attachPolicyToRoute(test.policy, test.route, test.validator, "nginx-gateway", logr.Discard()) if test.expAttached { g.Expect(test.route.Policies).To(HaveLen(1)) @@ -643,7 +645,7 @@ func TestAttachPolicyToGateway(t *testing.T) { t.Parallel() g := NewWithT(t) - attachPolicyToGateway(test.policy, test.policy.TargetRefs[0], test.gws, "nginx-gateway") + attachPolicyToGateway(test.policy, test.policy.TargetRefs[0], test.gws, "nginx-gateway", logr.Discard()) if test.expAttached { for _, gw := range test.gws { @@ -830,7 +832,7 @@ func TestAttachPolicyToService(t *testing.T) { t.Parallel() g := NewWithT(t) - attachPolicyToService(test.policy, test.svc, test.gws, "ctlr") + attachPolicyToService(test.policy, test.svc, test.gws, "ctlr", logr.Discard()) if test.expAttached { g.Expect(test.svc.Policies).To(HaveLen(1)) } else { @@ -1963,3 +1965,358 @@ func TestAddStatusToTargetRefs(t *testing.T) { addStatusToTargetRefs(policyKind, nil) }).ToNot(Panic()) } + +func TestNGFPolicyAncestorsFullFunc(t *testing.T) { + t.Parallel() + + createPolicyWithAncestors := func(ancestors []v1alpha2.PolicyAncestorStatus) *Policy { + fakePolicy := &policiesfakes.FakePolicy{ + GetPolicyStatusStub: func() v1alpha2.PolicyStatus { + return v1alpha2.PolicyStatus{ + Ancestors: ancestors, + } + }, + } + return &Policy{ + Source: fakePolicy, + Ancestors: []PolicyAncestor{}, // Updated ancestors list (starts empty) + } + } + + getAncestorRef := func(ctlrName, parentName string) v1alpha2.PolicyAncestorStatus { + return v1alpha2.PolicyAncestorStatus{ + ControllerName: v1.GatewayController(ctlrName), + AncestorRef: v1.ParentReference{ + Name: v1.ObjectName(parentName), + Namespace: helpers.GetPointer(v1.Namespace("test")), + Group: helpers.GetPointer[v1.Group](v1.GroupName), + Kind: helpers.GetPointer[v1.Kind](kinds.Gateway), + }, + } + } + + tests := []struct { + name string + ctlrName string + currentAncestors []v1alpha2.PolicyAncestorStatus + updatedAncestorsLen int + expected bool + }{ + { + name: "empty current ancestors, no updated ancestors", + currentAncestors: []v1alpha2.PolicyAncestorStatus{}, + updatedAncestorsLen: 0, + ctlrName: "nginx-gateway", + expected: false, + }, + { + name: "less than 16 total (current + updated)", + currentAncestors: []v1alpha2.PolicyAncestorStatus{ + getAncestorRef("other-controller", "gateway1"), + getAncestorRef("other-controller", "gateway2"), + }, + updatedAncestorsLen: 2, + ctlrName: "nginx-gateway", + expected: false, + }, + { + name: "exactly 16 non-NGF ancestors, no updated ancestors", + currentAncestors: func() []v1alpha2.PolicyAncestorStatus { + ancestors := make([]v1alpha2.PolicyAncestorStatus, 16) + for i := range 16 { + ancestors[i] = getAncestorRef("other-controller", "gateway") + } + return ancestors + }(), + updatedAncestorsLen: 1, // Trying to add 1 NGF ancestor + ctlrName: "nginx-gateway", + expected: true, + }, + { + name: "15 non-NGF + 1 NGF ancestor, adding 1 more NGF ancestor", + currentAncestors: func() []v1alpha2.PolicyAncestorStatus { + ancestors := make([]v1alpha2.PolicyAncestorStatus, 16) + for i := range 15 { + ancestors[i] = getAncestorRef("other-controller", "gateway") + } + ancestors[15] = getAncestorRef("nginx-gateway", "our-gateway") + return ancestors + }(), + updatedAncestorsLen: 1, + ctlrName: "nginx-gateway", + expected: true, // Full because 15 non-NGF + 1 new NGF = 16 which is the limit + }, + { + name: "10 non-NGF ancestors, trying to add 7 NGF ancestors (would exceed 16)", + currentAncestors: func() []v1alpha2.PolicyAncestorStatus { + ancestors := make([]v1alpha2.PolicyAncestorStatus, 10) + for i := range 10 { + ancestors[i] = getAncestorRef("other-controller", "gateway") + } + return ancestors + }(), + updatedAncestorsLen: 7, + ctlrName: "nginx-gateway", + expected: true, + }, + { + name: "5 non-NGF + 5 NGF ancestors, trying to add 6 more NGF ancestors", + currentAncestors: func() []v1alpha2.PolicyAncestorStatus { + ancestors := make([]v1alpha2.PolicyAncestorStatus, 10) + for i := range 5 { + ancestors[i] = getAncestorRef("other-controller", "gateway") + } + for i := 5; i < 10; i++ { + ancestors[i] = getAncestorRef("nginx-gateway", "our-gateway") + } + return ancestors + }(), + updatedAncestorsLen: 6, + ctlrName: "nginx-gateway", + expected: false, // 5 non-NGF + 6 new NGF = 11 total (within limit) + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + policy := createPolicyWithAncestors(test.currentAncestors) + // Simulate the updated ancestors list + for range test.updatedAncestorsLen { + policy.Ancestors = append(policy.Ancestors, PolicyAncestor{ + Ancestor: createParentReference(v1.GroupName, kinds.Gateway, + types.NamespacedName{Namespace: "test", Name: "new-gateway"}), + }) + } + + result := ngfPolicyAncestorsFull(policy, test.ctlrName) + g.Expect(result).To(Equal(test.expected)) + }) + } +} + +func TestNGFPolicyAncestorLimitHandling(t *testing.T) { + t.Parallel() + + // Create a test logger that captures log output + var logBuf bytes.Buffer + testLogger := logr.New(&testNGFLogSink{buffer: &logBuf}) + + policyGVK := schema.GroupVersionKind{Group: "Group", Version: "Version", Kind: "TestPolicy"} + + // Create a policy with 16 non-NGF ancestors (ancestor limit reached) + getAncestorRef := func(ctlrName, parentName string) v1alpha2.PolicyAncestorStatus { + return v1alpha2.PolicyAncestorStatus{ + ControllerName: v1.GatewayController(ctlrName), + AncestorRef: v1.ParentReference{ + Name: v1.ObjectName(parentName), + Namespace: helpers.GetPointer(v1.Namespace("test")), + Group: helpers.GetPointer[v1.Group](v1.GroupName), + Kind: helpers.GetPointer[v1.Kind](kinds.Gateway), + }, + } + } + + // Create 16 ancestors from different controllers to simulate full list + fullAncestors := make([]v1alpha2.PolicyAncestorStatus, 16) + for i := range 16 { + fullAncestors[i] = getAncestorRef("other-controller", "other-gateway") + } + + policyWithFullAncestors := &policiesfakes.FakePolicy{ + GetNameStub: func() string { + return "policy-full-ancestors" + }, + GetNamespaceStub: func() string { + return "test" + }, + GetPolicyStatusStub: func() v1alpha2.PolicyStatus { + return v1alpha2.PolicyStatus{ + Ancestors: fullAncestors, + } + }, + GetObjectKindStub: func() schema.ObjectKind { + return &policiesfakes.FakeObjectKind{ + GroupVersionKindStub: func() schema.GroupVersionKind { + return policyGVK + }, + } + }, + GetTargetRefsStub: func() []v1alpha2.LocalPolicyTargetReference { + return []v1alpha2.LocalPolicyTargetReference{ + { + Group: v1.GroupName, + Kind: kinds.Gateway, + Name: v1.ObjectName("gateway1"), + }, + } + }, + } + + // Create a policy with fewer ancestors (normal case) + normalPolicy := &policiesfakes.FakePolicy{ + GetNameStub: func() string { + return "policy-normal" + }, + GetNamespaceStub: func() string { + return "test" + }, + GetPolicyStatusStub: func() v1alpha2.PolicyStatus { + return v1alpha2.PolicyStatus{ + Ancestors: []v1alpha2.PolicyAncestorStatus{}, // Empty ancestors list + } + }, + GetObjectKindStub: func() schema.ObjectKind { + return &policiesfakes.FakeObjectKind{ + GroupVersionKindStub: func() schema.GroupVersionKind { + return policyGVK + }, + } + }, + GetTargetRefsStub: func() []v1alpha2.LocalPolicyTargetReference { + return []v1alpha2.LocalPolicyTargetReference{ + { + Group: v1.GroupName, + Kind: kinds.Gateway, + Name: v1.ObjectName("gateway2"), + }, + } + }, + } + + // Create gateways + gateways := map[types.NamespacedName]*Gateway{ + {Namespace: "test", Name: "gateway1"}: { + Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Name: "gateway1", Namespace: "test"}, + }, + Conditions: []conditions.Condition{}, // Start with empty conditions + }, + {Namespace: "test", Name: "gateway2"}: { + Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{Name: "gateway2", Namespace: "test"}, + }, + Conditions: []conditions.Condition{}, // Start with empty conditions + }, + } + + // Create test policies map + testPolicies := map[PolicyKey]policies.Policy{ + { + NsName: types.NamespacedName{Namespace: "test", Name: "policy-full-ancestors"}, + GVK: policyGVK, + }: policyWithFullAncestors, + { + NsName: types.NamespacedName{Namespace: "test", Name: "policy-normal"}, + GVK: policyGVK, + }: normalPolicy, + } + + // Create fake validator + validator := &policiesfakes.FakeValidator{ + ValidateStub: func(_ policies.Policy) []conditions.Condition { + return nil + }, + ConflictsStub: func(_, _ policies.Policy) bool { + return false + }, + } + + // Create empty routes and services for the test + routes := map[RouteKey]*L7Route{} + referencedServices := map[types.NamespacedName]*ReferencedService{} + + g := NewWithT(t) + + // Process policies which should trigger ancestor limit handling + processedPolicies := processPolicies(testPolicies, validator, routes, referencedServices, gateways) + + // Create a graph and attach policies to trigger ancestor limit handling + graph := &Graph{ + Gateways: gateways, + NGFPolicies: processedPolicies, + } + + // Call attachPolicies to trigger the ancestor limit logic + graph.attachPolicies(validator, "nginx-gateway", testLogger) + + // Verify that the policy with full ancestors has no actual ancestors assigned + policyFullKey := PolicyKey{ + NsName: types.NamespacedName{Namespace: "test", Name: "policy-full-ancestors"}, + GVK: policyGVK, + } + policyFull := graph.NGFPolicies[policyFullKey] + g.Expect(policyFull.Ancestors).To(BeEmpty(), "Policy with full ancestors should have no ancestors assigned") + + // Verify that the normal policy gets its ancestor assigned + policyNormalKey := PolicyKey{NsName: types.NamespacedName{Namespace: "test", Name: "policy-normal"}, GVK: policyGVK} + policyNormal := graph.NGFPolicies[policyNormalKey] + g.Expect(policyNormal.Ancestors).To(HaveLen(1), "Normal policy should have ancestor assigned") + + // Verify that gateway1 received the ancestor limit condition + gateway1 := gateways[types.NamespacedName{Namespace: "test", Name: "gateway1"}] + g.Expect(gateway1.Conditions).To(HaveLen(1), "Gateway should have received ancestor limit condition") + + condition := gateway1.Conditions[0] + g.Expect(condition.Type).To(Equal(string(v1alpha2.PolicyConditionAccepted))) + g.Expect(condition.Status).To(Equal(metav1.ConditionFalse)) + g.Expect(condition.Reason).To(Equal(string(conditions.PolicyReasonAncestorLimitReached))) + g.Expect(condition.Message).To(ContainSubstring("ancestor status list has reached the maximum size of 16")) + + // Verify that gateway2 did not receive any conditions (normal case) + gateway2 := gateways[types.NamespacedName{Namespace: "test", Name: "gateway2"}] + g.Expect(gateway2.Conditions).To(BeEmpty(), "Normal gateway should not have conditions") + + // Verify logging occurred + logOutput := logBuf.String() + g.Expect(logOutput).To(ContainSubstring("Policy ancestor limit reached")) + g.Expect(logOutput).To(ContainSubstring("policy=test/policy-full-ancestors")) + g.Expect(logOutput).To(ContainSubstring("policyKind=TestPolicy")) + g.Expect(logOutput).To(ContainSubstring("ancestor=test/gateway1")) +} + +// testNGFLogSink implements logr.LogSink for testing NGF policies. +type testNGFLogSink struct { + buffer *bytes.Buffer +} + +func (s *testNGFLogSink) Init(_ logr.RuntimeInfo) {} + +func (s *testNGFLogSink) Enabled(_ int) bool { + return true +} + +func (s *testNGFLogSink) Info(_ int, msg string, keysAndValues ...interface{}) { + s.buffer.WriteString(msg) + for i := 0; i < len(keysAndValues); i += 2 { + if i+1 < len(keysAndValues) { + s.buffer.WriteString(" ") + if key, ok := keysAndValues[i].(string); ok { + s.buffer.WriteString(key) + } + s.buffer.WriteString("=") + if value, ok := keysAndValues[i+1].(string); ok { + s.buffer.WriteString(value) + } + } + } + s.buffer.WriteString("\n") +} + +func (s *testNGFLogSink) Error(err error, msg string, _ ...interface{}) { + s.buffer.WriteString("ERROR: ") + s.buffer.WriteString(msg) + s.buffer.WriteString(" error=") + s.buffer.WriteString(err.Error()) + s.buffer.WriteString("\n") +} + +func (s *testNGFLogSink) WithValues(_ ...interface{}) logr.LogSink { + return s +} + +func (s *testNGFLogSink) WithName(_ string) logr.LogSink { + return s +} diff --git a/internal/controller/state/graph/policy_ancestor.go b/internal/controller/state/graph/policy_ancestor.go index 89340baa44..2c4aaed5b9 100644 --- a/internal/controller/state/graph/policy_ancestor.go +++ b/internal/controller/state/graph/policy_ancestor.go @@ -13,16 +13,8 @@ import ( const maxAncestors = 16 -// Package-level logger. -var logger logr.Logger - -// SetLogger sets the logger for the graph package. -func SetLogger(l logr.Logger) { - logger = l -} - // LogAncestorLimitReached logs when a policy ancestor limit is reached. -func LogAncestorLimitReached(policyName, policyKind, ancestorName string) { +func LogAncestorLimitReached(logger logr.Logger, policyName, policyKind, ancestorName string) { logger.Info("Policy ancestor limit reached", "policy", policyName, "policyKind", policyKind, "ancestor", ancestorName) } From 73a5af27b37c7c1be32dc6f8692281f68b562784 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Mon, 11 Aug 2025 12:40:48 -0700 Subject: [PATCH 03/18] fix a lot --- .../controller/state/conditions/conditions.go | 6 +- .../state/conditions/conditions_test.go | 3 +- .../state/graph/backend_tls_policy.go | 166 ++++++++++++++---- internal/controller/state/graph/policies.go | 80 ++++++++- .../controller/state/graph/policies_test.go | 62 +++++++ .../controller/state/graph/policy_ancestor.go | 32 +++- internal/framework/kinds/kinds.go | 2 + 7 files changed, 298 insertions(+), 53 deletions(-) diff --git a/internal/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go index feca03c30a..00814ef341 100644 --- a/internal/controller/state/conditions/conditions.go +++ b/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, diff --git a/internal/controller/state/conditions/conditions_test.go b/internal/controller/state/conditions/conditions_test.go index adc9a00834..0a079bfe14 100644 --- a/internal/controller/state/conditions/conditions_test.go +++ b/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)) diff --git a/internal/controller/state/graph/backend_tls_policy.go b/internal/controller/state/graph/backend_tls_policy.go index e6d55fea21..7eb47e571e 100644 --- a/internal/controller/state/graph/backend_tls_policy.go +++ b/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) } } diff --git a/internal/controller/state/graph/policies.go b/internal/controller/state/graph/policies.go index 46b94f3c2b..ca416fdf31 100644 --- a/internal/controller/state/graph/policies.go +++ b/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. diff --git a/internal/controller/state/graph/policies_test.go b/internal/controller/state/graph/policies_test.go index 82044dba36..60e5a51283 100644 --- a/internal/controller/state/graph/policies_test.go +++ b/internal/controller/state/graph/policies_test.go @@ -730,6 +730,46 @@ func TestAttachPolicyToService(t *testing.T) { }, }, }, + { + name: "attachment; existing gateway from policy status processed first", + policy: &Policy{ + Source: createPolicyWithExistingGatewayStatus(gwNsname, "ctlr"), + InvalidForGateways: map[types.NamespacedName]struct{}{}, + }, + svc: &ReferencedService{ + GatewayNsNames: map[types.NamespacedName]struct{}{ + gwNsname: {}, // This gateway exists in policy status (existing) + gw2Nsname: {}, // This gateway is new + }, + }, + gws: map[types.NamespacedName]*Gateway{ + gwNsname: { + Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: gwNsname.Name, + Namespace: gwNsname.Namespace, + }, + }, + Valid: true, + }, + gw2Nsname: { + Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: gw2Nsname.Name, + Namespace: gw2Nsname.Namespace, + }, + }, + Valid: true, + }, + }, + expAttached: true, + // Only new gateway should be added to ancestors, existing one already exists in policy status + expAncestors: []PolicyAncestor{ + { + Ancestor: getGatewayParentRef(gw2Nsname), // Only new gateway gets added + }, + }, + }, { name: "attachment; ancestor doesn't exist so add it", policy: &Policy{ @@ -2320,3 +2360,25 @@ func (s *testNGFLogSink) WithValues(_ ...interface{}) logr.LogSink { func (s *testNGFLogSink) WithName(_ string) logr.LogSink { return s } + +// createPolicyWithExistingGatewayStatus creates a fake policy with a gateway in its status ancestors. +func createPolicyWithExistingGatewayStatus(gatewayNsName types.NamespacedName, controllerName string) policies.Policy { + fakePolicy := &policiesfakes.FakePolicy{ + GetPolicyStatusStub: func() v1alpha2.PolicyStatus { + return v1alpha2.PolicyStatus{ + Ancestors: []v1alpha2.PolicyAncestorStatus{ + { + ControllerName: v1.GatewayController(controllerName), + AncestorRef: v1.ParentReference{ + Group: helpers.GetPointer[v1.Group](v1.GroupName), + Kind: helpers.GetPointer[v1.Kind](kinds.Gateway), + Namespace: (*v1.Namespace)(&gatewayNsName.Namespace), + Name: v1.ObjectName(gatewayNsName.Name), + }, + }, + }, + } + }, + } + return fakePolicy +} diff --git a/internal/controller/state/graph/policy_ancestor.go b/internal/controller/state/graph/policy_ancestor.go index 2c4aaed5b9..bddb18522e 100644 --- a/internal/controller/state/graph/policy_ancestor.go +++ b/internal/controller/state/graph/policy_ancestor.go @@ -1,12 +1,13 @@ package graph import ( + "sort" + + "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/types" v1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" - "github.com/go-logr/logr" - "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" ) @@ -130,3 +131,30 @@ func getPolicyKind(policy policies.Policy) string { } return policyKind } + +// compareNamespacedNames compares two NamespacedName objects lexicographically. +func compareNamespacedNames(a, b types.NamespacedName) bool { + if a.Namespace == b.Namespace { + return a.Name < b.Name + } + return a.Namespace < b.Namespace +} + +// sortGatewaysByCreationTime sorts gateways by creation timestamp, falling back to namespace/name for determinism. +func sortGatewaysByCreationTime(gatewayNames []types.NamespacedName, gateways map[types.NamespacedName]*Gateway) { + sort.SliceStable(gatewayNames, func(i, j int) bool { + gi := gateways[gatewayNames[i]] + gj := gateways[gatewayNames[j]] + + if gi == nil || gj == nil { + return compareNamespacedNames(gatewayNames[i], gatewayNames[j]) + } + + cti := gi.Source.CreationTimestamp.Time + ctj := gj.Source.CreationTimestamp.Time + if cti.Equal(ctj) { + return compareNamespacedNames(gatewayNames[i], gatewayNames[j]) + } + return cti.Before(ctj) + }) +} diff --git a/internal/framework/kinds/kinds.go b/internal/framework/kinds/kinds.go index baeda6f9ee..35ca8e2b00 100644 --- a/internal/framework/kinds/kinds.go +++ b/internal/framework/kinds/kinds.go @@ -21,6 +21,8 @@ const ( GRPCRoute = "GRPCRoute" // TLSRoute is the TLSRoute kind. TLSRoute = "TLSRoute" + // BackendTLSPolicy is the BackendTLSPolicy kind. + BackendTLSPolicy = "BackendTLSPolicy" ) // Core API Kinds. From 3ee449d644007b4956299a534a89e3b3a198dda5 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Mon, 11 Aug 2025 15:00:24 -0700 Subject: [PATCH 04/18] remove old code --- .../state/graph/backend_tls_policy.go | 13 +-- .../state/graph/backend_tls_policy_test.go | 103 +----------------- internal/controller/state/graph/graph.go | 1 - .../controller/state/graph/policy_ancestor.go | 22 ---- .../state/graph/policy_ancestor_test.go | 47 -------- 5 files changed, 4 insertions(+), 182 deletions(-) diff --git a/internal/controller/state/graph/backend_tls_policy.go b/internal/controller/state/graph/backend_tls_policy.go index 7eb47e571e..3a3955b75e 100644 --- a/internal/controller/state/graph/backend_tls_policy.go +++ b/internal/controller/state/graph/backend_tls_policy.go @@ -37,7 +37,6 @@ func processBackendTLSPolicies( backendTLSPolicies map[types.NamespacedName]*v1alpha3.BackendTLSPolicy, configMapResolver *configMapResolver, secretResolver *secretResolver, - ctlrName string, gateways map[types.NamespacedName]*Gateway, ) map[types.NamespacedName]*BackendTLSPolicy { if len(backendTLSPolicies) == 0 || len(gateways) == 0 { @@ -48,7 +47,7 @@ func processBackendTLSPolicies( for nsname, backendTLSPolicy := range backendTLSPolicies { var caCertRef types.NamespacedName - valid, ignored, conds := validateBackendTLSPolicy(backendTLSPolicy, configMapResolver, secretResolver, ctlrName) + valid, ignored, conds := validateBackendTLSPolicy(backendTLSPolicy, configMapResolver, secretResolver) if valid && !ignored && backendTLSPolicy.Spec.Validation.CACertificateRefs != nil { caCertRef = types.NamespacedName{ @@ -71,21 +70,13 @@ func validateBackendTLSPolicy( backendTLSPolicy *v1alpha3.BackendTLSPolicy, configMapResolver *configMapResolver, secretResolver *secretResolver, - _ string, ) (valid, ignored bool, conds []conditions.Condition) { valid = true ignored = false - // 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(err.Error())) + conds = append(conds, conditions.NewPolicyInvalid(fmt.Sprintf("invalid hostname: %s", err.Error()))) } caCertRefs := backendTLSPolicy.Spec.Validation.CACertificateRefs diff --git a/internal/controller/state/graph/backend_tls_policy_test.go b/internal/controller/state/graph/backend_tls_policy_test.go index 444f64a63c..c2d5685516 100644 --- a/internal/controller/state/graph/backend_tls_policy_test.go +++ b/internal/controller/state/graph/backend_tls_policy_test.go @@ -80,7 +80,7 @@ func TestProcessBackendTLSPoliciesEmpty(t *testing.T) { t.Parallel() g := NewWithT(t) - processed := processBackendTLSPolicies(test.backendTLSPolicies, nil, nil, "test", test.gateways) + processed := processBackendTLSPolicies(test.backendTLSPolicies, nil, nil, test.gateways) g.Expect(processed).To(Equal(test.expected)) }) @@ -477,7 +477,7 @@ func TestValidateBackendTLSPolicy(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - valid, ignored, conds := validateBackendTLSPolicy(test.tlsPolicy, configMapResolver, secretMapResolver, "test") + valid, ignored, conds := validateBackendTLSPolicy(test.tlsPolicy, configMapResolver, secretMapResolver) if !test.isValid && !test.ignored { g.Expect(conds).To(HaveLen(1)) @@ -809,105 +809,6 @@ func TestAddGatewaysForBackendTLSPoliciesAncestorLimit(t *testing.T) { g.Expect(logOutput).To(ContainSubstring("ancestor=test/gateway1")) } -func TestBackendTLSPolicyAncestorsFullFunc(t *testing.T) { - t.Parallel() - - getAncestorRef := func(ctlrName, parentName string) v1alpha2.PolicyAncestorStatus { - return v1alpha2.PolicyAncestorStatus{ - ControllerName: gatewayv1.GatewayController(ctlrName), - AncestorRef: gatewayv1.ParentReference{ - Name: gatewayv1.ObjectName(parentName), - Namespace: helpers.GetPointer(gatewayv1.Namespace("test")), - Group: helpers.GetPointer[gatewayv1.Group](gatewayv1.GroupName), - Kind: helpers.GetPointer[gatewayv1.Kind](kinds.Gateway), - }, - } - } - - tests := []struct { - name string - ctlrName string - ancestors []v1alpha2.PolicyAncestorStatus - expected bool - }{ - { - name: "empty ancestors list", - ancestors: []v1alpha2.PolicyAncestorStatus{}, - ctlrName: "nginx-gateway", - expected: false, - }, - { - name: "less than 16 ancestors", - ancestors: []v1alpha2.PolicyAncestorStatus{ - getAncestorRef("other-controller", "gateway1"), - getAncestorRef("other-controller", "gateway2"), - }, - ctlrName: "nginx-gateway", - expected: false, - }, - { - name: "exactly 16 ancestors, none from our controller", - ancestors: func() []v1alpha2.PolicyAncestorStatus { - ancestors := make([]v1alpha2.PolicyAncestorStatus, 16) - for i := range 16 { - ancestors[i] = getAncestorRef("other-controller", "gateway") - } - return ancestors - }(), - ctlrName: "nginx-gateway", - expected: true, - }, - { - name: "exactly 16 ancestors, one from our controller", - ancestors: func() []v1alpha2.PolicyAncestorStatus { - ancestors := make([]v1alpha2.PolicyAncestorStatus, 16) - for i := range 15 { - ancestors[i] = getAncestorRef("other-controller", "gateway") - } - ancestors[15] = getAncestorRef("nginx-gateway", "our-gateway") - return ancestors - }(), - ctlrName: "nginx-gateway", - expected: false, // Not full because we can overwrite our own entry - }, - { - name: "more than 16 ancestors, none from our controller", - ancestors: func() []v1alpha2.PolicyAncestorStatus { - ancestors := make([]v1alpha2.PolicyAncestorStatus, 20) - for i := range 20 { - ancestors[i] = getAncestorRef("other-controller", "gateway") - } - return ancestors - }(), - ctlrName: "nginx-gateway", - expected: true, - }, - { - name: "more than 16 ancestors, one from our controller", - ancestors: func() []v1alpha2.PolicyAncestorStatus { - ancestors := make([]v1alpha2.PolicyAncestorStatus, 20) - for i := range 19 { - ancestors[i] = getAncestorRef("other-controller", "gateway") - } - ancestors[19] = getAncestorRef("nginx-gateway", "our-gateway") - return ancestors - }(), - ctlrName: "nginx-gateway", - expected: false, // Not full because we can overwrite our own entry - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - g := NewWithT(t) - - result := backendTLSPolicyAncestorsFull(test.ancestors, test.ctlrName) - g.Expect(result).To(Equal(test.expected)) - }) - } -} - // testLogSink implements logr.LogSink for testing. type testLogSink struct { buffer *bytes.Buffer diff --git a/internal/controller/state/graph/graph.go b/internal/controller/state/graph/graph.go index 7c928d185d..ec2c9e1589 100644 --- a/internal/controller/state/graph/graph.go +++ b/internal/controller/state/graph/graph.go @@ -239,7 +239,6 @@ func BuildGraph( state.BackendTLSPolicies, configMapResolver, secretResolver, - controllerName, gws, ) diff --git a/internal/controller/state/graph/policy_ancestor.go b/internal/controller/state/graph/policy_ancestor.go index bddb18522e..2e944209c3 100644 --- a/internal/controller/state/graph/policy_ancestor.go +++ b/internal/controller/state/graph/policy_ancestor.go @@ -6,7 +6,6 @@ import ( "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/types" v1 "sigs.k8s.io/gateway-api/apis/v1" - "sigs.k8s.io/gateway-api/apis/v1alpha2" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" @@ -19,27 +18,6 @@ func LogAncestorLimitReached(logger logr.Logger, policyName, policyKind, ancesto logger.Info("Policy ancestor limit reached", "policy", policyName, "policyKind", policyKind, "ancestor", ancestorName) } -// backendTLSPolicyAncestorsFull returns whether or not an ancestor list is full. A list is not full when: -// - the number of current ancestors is less than the maximum allowed -// - an entry for an NGF managed resource already exists in the ancestor list. This means that we are overwriting -// that status entry with the current status entry, since there is only one ancestor (Gateway) for this policy. -func backendTLSPolicyAncestorsFull( - ancestors []v1alpha2.PolicyAncestorStatus, - ctlrName string, -) bool { - if len(ancestors) < maxAncestors { - return false - } - - for _, ancestor := range ancestors { - if string(ancestor.ControllerName) == ctlrName { - return false - } - } - - return true -} - // ngfPolicyAncestorsFull returns whether or not an ancestor list is full. A list is full when // the sum of the following is greater than or equal to the maximum allowed: // - number of non-NGF managed ancestors diff --git a/internal/controller/state/graph/policy_ancestor_test.go b/internal/controller/state/graph/policy_ancestor_test.go index 49903cf0df..2cb9239114 100644 --- a/internal/controller/state/graph/policy_ancestor_test.go +++ b/internal/controller/state/graph/policy_ancestor_test.go @@ -12,53 +12,6 @@ import ( "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/kinds" ) -func TestBackendTLSPolicyAncestorsFull(t *testing.T) { - t.Parallel() - createCurStatus := func(numAncestors int, ctlrName string) []v1alpha2.PolicyAncestorStatus { - statuses := make([]v1alpha2.PolicyAncestorStatus, 0, numAncestors) - - for range numAncestors { - statuses = append(statuses, v1alpha2.PolicyAncestorStatus{ - ControllerName: v1.GatewayController(ctlrName), - }) - } - - return statuses - } - - tests := []struct { - name string - curStatus []v1alpha2.PolicyAncestorStatus - expFull bool - }{ - { - name: "not full", - curStatus: createCurStatus(15, "controller"), - expFull: false, - }, - { - name: "full; ancestor does not exist in current status", - curStatus: createCurStatus(16, "controller"), - expFull: true, - }, - { - name: "full, but ancestor does exist in current status", - curStatus: createCurStatus(16, "nginx-gateway"), - expFull: false, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - t.Parallel() - g := NewWithT(t) - - full := backendTLSPolicyAncestorsFull(test.curStatus, "nginx-gateway") - g.Expect(full).To(Equal(test.expFull)) - }) - } -} - func TestNGFPolicyAncestorsFull(t *testing.T) { t.Parallel() type ancestorConfig struct { From 1bfb81b8b807d677fbc1501a5dca6be5490d8ab9 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Tue, 12 Aug 2025 15:25:33 -0700 Subject: [PATCH 05/18] remove specific mention of ancestor limit number --- internal/controller/state/conditions/conditions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go index 00814ef341..be9fcdcc64 100644 --- a/internal/controller/state/conditions/conditions.go +++ b/internal/controller/state/conditions/conditions.go @@ -146,7 +146,7 @@ const ( // PolicyMessageAncestorLimitReached is a message used with PolicyReasonAncestorLimitReached // when a policy cannot be applied due to the 16 ancestor limit being reached. PolicyMessageAncestorLimitReached = "Policies cannot be applied because the ancestor status list " + - "has reached the maximum size of 16. The following policies have been ignored:" + "has reached the maximum size. The following policies have been ignored:" ) // Condition defines a condition to be reported in the status of resources. From 85441df3cd53415686d4ab43fe7a66677a0c6f6f Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Tue, 12 Aug 2025 15:27:23 -0700 Subject: [PATCH 06/18] move import --- internal/controller/state/graph/graph.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/controller/state/graph/graph.go b/internal/controller/state/graph/graph.go index ec2c9e1589..b833d38da4 100644 --- a/internal/controller/state/graph/graph.go +++ b/internal/controller/state/graph/graph.go @@ -3,6 +3,7 @@ package graph import ( "fmt" + "github.com/go-logr/logr" v1 "k8s.io/api/core/v1" discoveryV1 "k8s.io/api/discovery/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -13,8 +14,6 @@ import ( "sigs.k8s.io/gateway-api/apis/v1alpha3" "sigs.k8s.io/gateway-api/apis/v1beta1" - "github.com/go-logr/logr" - ngfAPIv1alpha1 "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1" ngfAPIv1alpha2 "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha2" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies" From bd097ece51722abd437e15d984010d212bf45d49 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Tue, 12 Aug 2025 15:32:25 -0700 Subject: [PATCH 07/18] fix condition reason checking --- internal/controller/state/graph/backend_tls_policy.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/controller/state/graph/backend_tls_policy.go b/internal/controller/state/graph/backend_tls_policy.go index 3a3955b75e..028d52b366 100644 --- a/internal/controller/state/graph/backend_tls_policy.go +++ b/internal/controller/state/graph/backend_tls_policy.go @@ -195,10 +195,8 @@ func addPolicyAncestorLimitCondition( policyName string, policyType string, ) []conditions.Condition { - const policyAncestorLimitReachedType = "PolicyAncestorLimitReached" - for i, condition := range conds { - if condition.Type == policyAncestorLimitReachedType { + if condition.Reason == string(conditions.PolicyReasonAncestorLimitReached) { if !strings.Contains(condition.Message, policyName) { conds[i].Message = fmt.Sprintf("%s, %s %s", condition.Message, policyType, policyName) } From d146ad5665c2c5d98c348d8393ab9b52b3fb8ef1 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Tue, 12 Aug 2025 15:33:36 -0700 Subject: [PATCH 08/18] clean collectOrderedGateways function --- .../state/graph/backend_tls_policy.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/internal/controller/state/graph/backend_tls_policy.go b/internal/controller/state/graph/backend_tls_policy.go index 028d52b366..7bfedb21e4 100644 --- a/internal/controller/state/graph/backend_tls_policy.go +++ b/internal/controller/state/graph/backend_tls_policy.go @@ -216,8 +216,10 @@ func collectOrderedGateways( services map[types.NamespacedName]*ReferencedService, gateways map[types.NamespacedName]*Gateway, existingNGFGatewayAncestors map[types.NamespacedName]struct{}, -) (existingGateways []types.NamespacedName, newGateways []types.NamespacedName) { +) []types.NamespacedName { seenGateways := make(map[types.NamespacedName]struct{}) + existingGateways := make([]types.NamespacedName, 0) + newGateways := make([]types.NamespacedName, 0) // Process services in spec order to maintain deterministic gateway ordering for _, refs := range policy.Spec.TargetRefs { @@ -252,7 +254,8 @@ func collectOrderedGateways( sortGatewaysByCreationTime(existingGateways, gateways) sortGatewaysByCreationTime(newGateways, gateways) - return existingGateways, newGateways + existingGateways = append(existingGateways, newGateways...) + return existingGateways } func extractExistingNGFGatewayAncestors( @@ -288,11 +291,12 @@ func addGatewaysForBackendTLSPolicies( ) { for _, backendTLSPolicy := range backendTLSPolicies { existingNGFGatewayAncestors := extractExistingNGFGatewayAncestors(backendTLSPolicy.Source, ctlrName) - existingGateways, newGateways := collectOrderedGateways( - backendTLSPolicy.Source, services, gateways, existingNGFGatewayAncestors) - - existingGateways = append(existingGateways, newGateways...) - orderedGateways := existingGateways + orderedGateways := collectOrderedGateways( + backendTLSPolicy.Source, + services, + gateways, + existingNGFGatewayAncestors, + ) ancestorCount := countNonNGFAncestors(backendTLSPolicy.Source, ctlrName) From 51a714f9a5f73cb6e4d24f4dfa6751585fc776a6 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Tue, 12 Aug 2025 15:34:50 -0700 Subject: [PATCH 09/18] remove export log function --- internal/controller/state/graph/backend_tls_policy.go | 2 +- internal/controller/state/graph/backend_tls_policy_test.go | 2 +- internal/controller/state/graph/policies.go | 6 +++--- internal/controller/state/graph/policy_ancestor.go | 4 ++-- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/controller/state/graph/backend_tls_policy.go b/internal/controller/state/graph/backend_tls_policy.go index 7bfedb21e4..045c0d4a07 100644 --- a/internal/controller/state/graph/backend_tls_policy.go +++ b/internal/controller/state/graph/backend_tls_policy.go @@ -316,7 +316,7 @@ func addGatewaysForBackendTLSPolicies( "Gateway not found in the graph", "policy", policyName, "ancestor", gatewayName) } - LogAncestorLimitReached(logger, policyName, "BackendTLSPolicy", gatewayName) + logAncestorLimitReached(logger, policyName, "BackendTLSPolicy", gatewayName) continue } diff --git a/internal/controller/state/graph/backend_tls_policy_test.go b/internal/controller/state/graph/backend_tls_policy_test.go index c2d5685516..e83e74965d 100644 --- a/internal/controller/state/graph/backend_tls_policy_test.go +++ b/internal/controller/state/graph/backend_tls_policy_test.go @@ -800,7 +800,7 @@ func TestAddGatewaysForBackendTLSPoliciesAncestorLimit(t *testing.T) { g.Expect(gateway2.Conditions).To(BeEmpty(), "Normal gateway should not have conditions") // Verify logging function works - test the logging function directly - LogAncestorLimitReached(testLogger, "test/btp-full-ancestors", "BackendTLSPolicy", "test/gateway1") + logAncestorLimitReached(testLogger, "test/btp-full-ancestors", "BackendTLSPolicy", "test/gateway1") logOutput := logBuf.String() g.Expect(logOutput).To(ContainSubstring("Policy ancestor limit reached")) diff --git a/internal/controller/state/graph/policies.go b/internal/controller/state/graph/policies.go index ca416fdf31..43a720a4b0 100644 --- a/internal/controller/state/graph/policies.go +++ b/internal/controller/state/graph/policies.go @@ -200,7 +200,7 @@ func attachPolicyToService( policyKind := getPolicyKind(policy.Source) gw.Conditions = addPolicyAncestorLimitCondition(gw.Conditions, policyName, policyKind) - LogAncestorLimitReached(logger, policyName, policyKind, gwNsName.String()) + logAncestorLimitReached(logger, policyName, policyKind, gwNsName.String()) // Mark this gateway as invalid for the policy due to ancestor limits policy.InvalidForGateways[gwNsName] = struct{}{} @@ -248,7 +248,7 @@ func attachPolicyToRoute( routeName := getAncestorName(ancestorRef) route.Conditions = addPolicyAncestorLimitCondition(route.Conditions, policyName, policyKind) - LogAncestorLimitReached(logger, policyName, policyKind, routeName) + logAncestorLimitReached(logger, policyName, policyKind, routeName) return } @@ -326,7 +326,7 @@ func attachPolicyToGateway( // Log in the controller log. logger.Info("Gateway target not found and ancestors slice is full.", "policy", policyName, "ancestor", ancestorName) } - LogAncestorLimitReached(logger, policyName, policyKind, ancestorName) + logAncestorLimitReached(logger, policyName, policyKind, ancestorName) policy.InvalidForGateways[ref.Nsname] = struct{}{} return diff --git a/internal/controller/state/graph/policy_ancestor.go b/internal/controller/state/graph/policy_ancestor.go index 2e944209c3..fb5539143a 100644 --- a/internal/controller/state/graph/policy_ancestor.go +++ b/internal/controller/state/graph/policy_ancestor.go @@ -13,8 +13,8 @@ import ( const maxAncestors = 16 -// LogAncestorLimitReached logs when a policy ancestor limit is reached. -func LogAncestorLimitReached(logger logr.Logger, policyName, policyKind, ancestorName string) { +// logAncestorLimitReached logs when a policy ancestor limit is reached. +func logAncestorLimitReached(logger logr.Logger, policyName, policyKind, ancestorName string) { logger.Info("Policy ancestor limit reached", "policy", policyName, "policyKind", policyKind, "ancestor", ancestorName) } From 901a6c4ad884cde390db59f00b000ff3a229a294 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Tue, 12 Aug 2025 15:50:25 -0700 Subject: [PATCH 10/18] add unit tests for helpers and simplify collectOrderedGateways --- .../state/graph/backend_tls_policy_test.go | 2 +- internal/controller/state/graph/policies.go | 15 +- .../controller/state/graph/policy_ancestor.go | 12 +- .../state/graph/policy_ancestor_test.go | 234 ++++++++++++++++++ 4 files changed, 245 insertions(+), 18 deletions(-) diff --git a/internal/controller/state/graph/backend_tls_policy_test.go b/internal/controller/state/graph/backend_tls_policy_test.go index e83e74965d..80beede45c 100644 --- a/internal/controller/state/graph/backend_tls_policy_test.go +++ b/internal/controller/state/graph/backend_tls_policy_test.go @@ -402,7 +402,7 @@ func TestValidateBackendTLSPolicy(t *testing.T) { }, }, { - name: "valid case with many ancestors (ancestor limit now handled during gateway assignment)", + name: "valid case with many ancestors", tlsPolicy: &v1alpha3.BackendTLSPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: "tls-policy", diff --git a/internal/controller/state/graph/policies.go b/internal/controller/state/graph/policies.go index 43a720a4b0..2f947c743d 100644 --- a/internal/controller/state/graph/policies.go +++ b/internal/controller/state/graph/policies.go @@ -98,9 +98,9 @@ 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)) +) []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 { @@ -113,7 +113,8 @@ func collectOrderedGatewaysForService( sortGatewaysByCreationTime(existingGateways, gateways) sortGatewaysByCreationTime(newGateways, gateways) - return existingGateways, newGateways + existingGateways = append(existingGateways, newGateways...) + return existingGateways } func (g *Graph) attachPolicies(validator validation.PolicyValidator, ctlrName string, logger logr.Logger) { @@ -158,11 +159,7 @@ func attachPolicyToService( existingNGFGatewayAncestors := extractExistingNGFGatewayAncestorsForPolicy(policy, ctlrName) // Collect and order gateways with existing gateway prioritization - existingGateways, newGateways := collectOrderedGatewaysForService( - svc, gws, existingNGFGatewayAncestors) - - existingGateways = append(existingGateways, newGateways...) - orderedGateways := existingGateways + orderedGateways := collectOrderedGatewaysForService(svc, gws, existingNGFGatewayAncestors) for _, gwNsName := range orderedGateways { gw := gws[gwNsName] diff --git a/internal/controller/state/graph/policy_ancestor.go b/internal/controller/state/graph/policy_ancestor.go index fb5539143a..d2c056201a 100644 --- a/internal/controller/state/graph/policy_ancestor.go +++ b/internal/controller/state/graph/policy_ancestor.go @@ -83,10 +83,7 @@ func parentRefEqual(ref1, ref2 v1.ParentReference) bool { return true } -// Helper functions to eliminate code duplication - -// getAncestorName generates a human-readable name for an ancestor from a ParentReference. -// Returns namespace/name format if namespace is specified, otherwise just name. +// getAncestorName returns namespace/name format if namespace is specified, otherwise just name. func getAncestorName(ancestorRef v1.ParentReference) string { ancestorName := string(ancestorRef.Name) if ancestorRef.Namespace != nil { @@ -95,15 +92,14 @@ func getAncestorName(ancestorRef v1.ParentReference) string { return ancestorName } -// getPolicyName generates a human-readable name for a policy in namespace/name format. +// getPolicyName returns a human-readable name for a policy in namespace/name format. func getPolicyName(policy policies.Policy) string { return policy.GetNamespace() + "/" + policy.GetName() } -// getPolicyKind returns the policy kind with defensive programming for test environments. -// Returns "Policy" as fallback if GetObjectKind() returns nil. +// getPolicyKind returns the policy kind or "Policy" if GetObjectKind() returns nil. func getPolicyKind(policy policies.Policy) string { - policyKind := "Policy" // Default fallback + policyKind := "Policy" if objKind := policy.GetObjectKind(); objKind != nil { policyKind = objKind.GroupVersionKind().Kind } diff --git a/internal/controller/state/graph/policy_ancestor_test.go b/internal/controller/state/graph/policy_ancestor_test.go index 2cb9239114..5a987c552c 100644 --- a/internal/controller/state/graph/policy_ancestor_test.go +++ b/internal/controller/state/graph/policy_ancestor_test.go @@ -2,13 +2,19 @@ package graph import ( "testing" + "time" + "github.com/go-logr/logr/testr" . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" v1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" ngfAPIv1alpha2 "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha2" + "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/kinds" ) @@ -232,3 +238,231 @@ func TestParentRefEqual(t *testing.T) { }) } } + +func TestLogAncestorLimitReached(t *testing.T) { + t.Parallel() + logger := testr.New(t) + logAncestorLimitReached(logger, "test-policy", "TestPolicy", "test-ancestor") +} + +func TestGetAncestorName(t *testing.T) { + t.Parallel() + tests := []struct { + name string + ref v1.ParentReference + expected string + }{ + { + name: "with namespace", + ref: v1.ParentReference{ + Name: "test-gw", + Namespace: func() *v1.Namespace { ns := v1.Namespace("test-ns"); return &ns }(), + }, + expected: "test-ns/test-gw", + }, + { + name: "without namespace", + ref: v1.ParentReference{ + Name: "test-gw", + }, + expected: "test-gw", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + g.Expect(getAncestorName(test.ref)).To(Equal(test.expected)) + }) + } +} + +type mockPolicy struct { + name string + namespace string + kind string +} + +func (m *mockPolicy) GetName() string { return m.name } +func (m *mockPolicy) SetName(name string) { m.name = name } +func (m *mockPolicy) GetNamespace() string { return m.namespace } +func (m *mockPolicy) SetNamespace(namespace string) { m.namespace = namespace } +func (m *mockPolicy) GetObjectKind() schema.ObjectKind { + if m.kind == "" { + return nil + } + return &mockObjectKind{kind: m.kind} +} +func (m *mockPolicy) GetTargetRefs() []v1alpha2.LocalPolicyTargetReference { return nil } +func (m *mockPolicy) GetPolicyStatus() v1alpha2.PolicyStatus { return v1alpha2.PolicyStatus{} } +func (m *mockPolicy) SetPolicyStatus(_ v1alpha2.PolicyStatus) {} +func (m *mockPolicy) DeepCopyObject() runtime.Object { return m } +func (m *mockPolicy) GetUID() types.UID { return "" } +func (m *mockPolicy) GetResourceVersion() string { return "" } +func (m *mockPolicy) SetUID(_ types.UID) {} +func (m *mockPolicy) SetResourceVersion(_ string) {} +func (m *mockPolicy) GetGeneration() int64 { return 0 } +func (m *mockPolicy) SetGeneration(_ int64) {} +func (m *mockPolicy) GetCreationTimestamp() metav1.Time { return metav1.Time{} } +func (m *mockPolicy) SetCreationTimestamp(_ metav1.Time) {} +func (m *mockPolicy) GetDeletionTimestamp() *metav1.Time { return nil } +func (m *mockPolicy) SetDeletionTimestamp(_ *metav1.Time) {} +func (m *mockPolicy) GetDeletionGracePeriodSeconds() *int64 { return nil } +func (m *mockPolicy) SetDeletionGracePeriodSeconds(*int64) {} +func (m *mockPolicy) GetLabels() map[string]string { return nil } +func (m *mockPolicy) SetLabels(_ map[string]string) {} +func (m *mockPolicy) GetAnnotations() map[string]string { return nil } +func (m *mockPolicy) SetAnnotations(_ map[string]string) {} +func (m *mockPolicy) GetFinalizers() []string { return nil } +func (m *mockPolicy) SetFinalizers(_ []string) {} +func (m *mockPolicy) GetOwnerReferences() []metav1.OwnerReference { return nil } +func (m *mockPolicy) SetOwnerReferences([]metav1.OwnerReference) {} +func (m *mockPolicy) GetManagedFields() []metav1.ManagedFieldsEntry { return nil } +func (m *mockPolicy) SetManagedFields(_ []metav1.ManagedFieldsEntry) {} +func (m *mockPolicy) GetGenerateName() string { return "" } +func (m *mockPolicy) SetGenerateName(_ string) {} +func (m *mockPolicy) GetSelfLink() string { return "" } +func (m *mockPolicy) SetSelfLink(_ string) {} + +type mockObjectKind struct{ kind string } + +func (m *mockObjectKind) GroupVersionKind() schema.GroupVersionKind { + return schema.GroupVersionKind{Kind: m.kind} +} +func (m *mockObjectKind) SetGroupVersionKind(_ schema.GroupVersionKind) {} + +func TestGetPolicyName(t *testing.T) { + t.Parallel() + g := NewWithT(t) + policy := &mockPolicy{name: "test-policy", namespace: "test-ns"} + g.Expect(getPolicyName(policy)).To(Equal("test-ns/test-policy")) +} + +func TestGetPolicyKind(t *testing.T) { + t.Parallel() + tests := []struct { + name string + policy policies.Policy + expected string + }{ + { + name: "with kind", + policy: &mockPolicy{kind: "TestPolicy"}, + expected: "TestPolicy", + }, + { + name: "without kind", + policy: &mockPolicy{}, + expected: "Policy", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + g.Expect(getPolicyKind(test.policy)).To(Equal(test.expected)) + }) + } +} + +func TestCompareNamespacedNames(t *testing.T) { + t.Parallel() + tests := []struct { + name string + a, b types.NamespacedName + expected bool + }{ + { + name: "same namespace, a name < b name", + a: types.NamespacedName{Namespace: "ns", Name: "a"}, + b: types.NamespacedName{Namespace: "ns", Name: "b"}, + expected: true, + }, + { + name: "same namespace, a name > b name", + a: types.NamespacedName{Namespace: "ns", Name: "b"}, + b: types.NamespacedName{Namespace: "ns", Name: "a"}, + expected: false, + }, + { + name: "a namespace < b namespace", + a: types.NamespacedName{Namespace: "a", Name: "z"}, + b: types.NamespacedName{Namespace: "b", Name: "a"}, + expected: true, + }, + { + name: "a namespace > b namespace", + a: types.NamespacedName{Namespace: "b", Name: "a"}, + b: types.NamespacedName{Namespace: "a", Name: "z"}, + expected: false, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + g.Expect(compareNamespacedNames(test.a, test.b)).To(Equal(test.expected)) + }) + } +} + +func TestSortGatewaysByCreationTime(t *testing.T) { + t.Parallel() + now := time.Now() + gw1Name := types.NamespacedName{Namespace: "test", Name: "gw1"} + gw2Name := types.NamespacedName{Namespace: "test", Name: "gw2"} + + tests := []struct { + name string + gateways map[types.NamespacedName]*Gateway + names []types.NamespacedName + expected []types.NamespacedName + }{ + { + name: "sort by creation time", + gateways: map[types.NamespacedName]*Gateway{ + gw1Name: {Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{CreationTimestamp: metav1.NewTime(now.Add(time.Hour))}, + }}, + gw2Name: {Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{CreationTimestamp: metav1.NewTime(now)}, + }}, + }, + names: []types.NamespacedName{gw1Name, gw2Name}, + expected: []types.NamespacedName{gw2Name, gw1Name}, + }, + { + name: "same creation time, sort by namespace/name", + gateways: map[types.NamespacedName]*Gateway{ + gw2Name: {Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{CreationTimestamp: metav1.NewTime(now)}, + }}, + gw1Name: {Source: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{CreationTimestamp: metav1.NewTime(now)}, + }}, + }, + names: []types.NamespacedName{gw2Name, gw1Name}, + expected: []types.NamespacedName{gw1Name, gw2Name}, + }, + { + name: "nil gateway fallback to namespace/name", + gateways: map[types.NamespacedName]*Gateway{gw1Name: nil}, + names: []types.NamespacedName{gw2Name, gw1Name}, + expected: []types.NamespacedName{gw1Name, gw2Name}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + names := make([]types.NamespacedName, len(test.names)) + copy(names, test.names) + sortGatewaysByCreationTime(names, test.gateways) + g.Expect(names).To(Equal(test.expected)) + }) + } +} From 48ac29d35f058493223d147360c2c81062b75db3 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Tue, 12 Aug 2025 15:56:50 -0700 Subject: [PATCH 11/18] remove duplicate log sink --- .../state/graph/backend_tls_policy_test.go | 50 ++----------------- .../controller/state/graph/policies_test.go | 2 +- 2 files changed, 4 insertions(+), 48 deletions(-) diff --git a/internal/controller/state/graph/backend_tls_policy_test.go b/internal/controller/state/graph/backend_tls_policy_test.go index 80beede45c..a244bab679 100644 --- a/internal/controller/state/graph/backend_tls_policy_test.go +++ b/internal/controller/state/graph/backend_tls_policy_test.go @@ -674,9 +674,9 @@ func TestAddGatewaysForBackendTLSPoliciesAncestorLimit(t *testing.T) { // Create a test logger that captures log output var logBuf bytes.Buffer - testLogger := logr.New(&testLogSink{buffer: &logBuf}) + testLogger := logr.New(&testNGFLogSink{buffer: &logBuf}) - // Create BackendTLSPolicy with 16 ancestors (full) + // Helper function to create ancestor references getAncestorRef := func(ctlrName, parentName string) v1alpha2.PolicyAncestorStatus { return v1alpha2.PolicyAncestorStatus{ ControllerName: gatewayv1.GatewayController(ctlrName), @@ -793,7 +793,7 @@ func TestAddGatewaysForBackendTLSPoliciesAncestorLimit(t *testing.T) { g.Expect(condition.Type).To(Equal(string(v1alpha2.PolicyConditionAccepted))) g.Expect(condition.Status).To(Equal(metav1.ConditionFalse)) g.Expect(condition.Reason).To(Equal(string(conditions.PolicyReasonAncestorLimitReached))) - g.Expect(condition.Message).To(ContainSubstring("ancestor status list has reached the maximum size of 16")) + g.Expect(condition.Message).To(ContainSubstring("ancestor status list has reached the maximum size")) // Verify that gateway2 did not receive any conditions (normal case) gateway2 := gateways[types.NamespacedName{Namespace: "test", Name: "gateway2"}] @@ -808,47 +808,3 @@ func TestAddGatewaysForBackendTLSPoliciesAncestorLimit(t *testing.T) { g.Expect(logOutput).To(ContainSubstring("policyKind=BackendTLSPolicy")) g.Expect(logOutput).To(ContainSubstring("ancestor=test/gateway1")) } - -// testLogSink implements logr.LogSink for testing. -type testLogSink struct { - buffer *bytes.Buffer -} - -func (s *testLogSink) Init(_ logr.RuntimeInfo) {} - -func (s *testLogSink) Enabled(_ int) bool { - return true -} - -func (s *testLogSink) Info(_ int, msg string, keysAndValues ...interface{}) { - s.buffer.WriteString(msg) - for i := 0; i < len(keysAndValues); i += 2 { - if i+1 < len(keysAndValues) { - s.buffer.WriteString(" ") - if key, ok := keysAndValues[i].(string); ok { - s.buffer.WriteString(key) - } - s.buffer.WriteString("=") - if value, ok := keysAndValues[i+1].(string); ok { - s.buffer.WriteString(value) - } - } - } - s.buffer.WriteString("\n") -} - -func (s *testLogSink) Error(err error, msg string, _ ...interface{}) { - s.buffer.WriteString("ERROR: ") - s.buffer.WriteString(msg) - s.buffer.WriteString(" error=") - s.buffer.WriteString(err.Error()) - s.buffer.WriteString("\n") -} - -func (s *testLogSink) WithValues(_ ...interface{}) logr.LogSink { - return s -} - -func (s *testLogSink) WithName(_ string) logr.LogSink { - return s -} diff --git a/internal/controller/state/graph/policies_test.go b/internal/controller/state/graph/policies_test.go index 60e5a51283..5faacb3d53 100644 --- a/internal/controller/state/graph/policies_test.go +++ b/internal/controller/state/graph/policies_test.go @@ -2303,7 +2303,7 @@ func TestNGFPolicyAncestorLimitHandling(t *testing.T) { g.Expect(condition.Type).To(Equal(string(v1alpha2.PolicyConditionAccepted))) g.Expect(condition.Status).To(Equal(metav1.ConditionFalse)) g.Expect(condition.Reason).To(Equal(string(conditions.PolicyReasonAncestorLimitReached))) - g.Expect(condition.Message).To(ContainSubstring("ancestor status list has reached the maximum size of 16")) + g.Expect(condition.Message).To(ContainSubstring("ancestor status list has reached the maximum size")) // Verify that gateway2 did not receive any conditions (normal case) gateway2 := gateways[types.NamespacedName{Namespace: "test", Name: "gateway2"}] From ce118bc965ebeac389cfb214877b002dfca72da6 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Tue, 12 Aug 2025 16:02:58 -0700 Subject: [PATCH 12/18] fix test readability --- .../controller/state/graph/policies_test.go | 27 +++++++------------ 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/internal/controller/state/graph/policies_test.go b/internal/controller/state/graph/policies_test.go index 5faacb3d53..eeab0c33fd 100644 --- a/internal/controller/state/graph/policies_test.go +++ b/internal/controller/state/graph/policies_test.go @@ -2037,17 +2037,15 @@ func TestNGFPolicyAncestorsFullFunc(t *testing.T) { tests := []struct { name string - ctlrName string currentAncestors []v1alpha2.PolicyAncestorStatus updatedAncestorsLen int - expected bool + expectFull bool }{ { name: "empty current ancestors, no updated ancestors", currentAncestors: []v1alpha2.PolicyAncestorStatus{}, updatedAncestorsLen: 0, - ctlrName: "nginx-gateway", - expected: false, + expectFull: false, }, { name: "less than 16 total (current + updated)", @@ -2056,8 +2054,7 @@ func TestNGFPolicyAncestorsFullFunc(t *testing.T) { getAncestorRef("other-controller", "gateway2"), }, updatedAncestorsLen: 2, - ctlrName: "nginx-gateway", - expected: false, + expectFull: false, }, { name: "exactly 16 non-NGF ancestors, no updated ancestors", @@ -2069,8 +2066,7 @@ func TestNGFPolicyAncestorsFullFunc(t *testing.T) { return ancestors }(), updatedAncestorsLen: 1, // Trying to add 1 NGF ancestor - ctlrName: "nginx-gateway", - expected: true, + expectFull: true, }, { name: "15 non-NGF + 1 NGF ancestor, adding 1 more NGF ancestor", @@ -2083,8 +2079,7 @@ func TestNGFPolicyAncestorsFullFunc(t *testing.T) { return ancestors }(), updatedAncestorsLen: 1, - ctlrName: "nginx-gateway", - expected: true, // Full because 15 non-NGF + 1 new NGF = 16 which is the limit + expectFull: true, // Full because 15 non-NGF + 1 new NGF = 16 which is the limit }, { name: "10 non-NGF ancestors, trying to add 7 NGF ancestors (would exceed 16)", @@ -2096,8 +2091,7 @@ func TestNGFPolicyAncestorsFullFunc(t *testing.T) { return ancestors }(), updatedAncestorsLen: 7, - ctlrName: "nginx-gateway", - expected: true, + expectFull: true, }, { name: "5 non-NGF + 5 NGF ancestors, trying to add 6 more NGF ancestors", @@ -2112,8 +2106,7 @@ func TestNGFPolicyAncestorsFullFunc(t *testing.T) { return ancestors }(), updatedAncestorsLen: 6, - ctlrName: "nginx-gateway", - expected: false, // 5 non-NGF + 6 new NGF = 11 total (within limit) + expectFull: false, // 5 non-NGF + 6 new NGF = 11 total (within limit) }, } @@ -2131,8 +2124,8 @@ func TestNGFPolicyAncestorsFullFunc(t *testing.T) { }) } - result := ngfPolicyAncestorsFull(policy, test.ctlrName) - g.Expect(result).To(Equal(test.expected)) + result := ngfPolicyAncestorsFull(policy, "nginx-gateway") + g.Expect(result).To(Equal(test.expectFull)) }) } } @@ -2146,7 +2139,7 @@ func TestNGFPolicyAncestorLimitHandling(t *testing.T) { policyGVK := schema.GroupVersionKind{Group: "Group", Version: "Version", Kind: "TestPolicy"} - // Create a policy with 16 non-NGF ancestors (ancestor limit reached) + // Helper function to create ancestor references getAncestorRef := func(ctlrName, parentName string) v1alpha2.PolicyAncestorStatus { return v1alpha2.PolicyAncestorStatus{ ControllerName: v1.GatewayController(ctlrName), From ef5d3d28ab30935931b8a4134e8765d778211434 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Tue, 12 Aug 2025 16:15:23 -0700 Subject: [PATCH 13/18] use fake policy --- .../controller/state/graph/policies_test.go | 50 +++++++++++++------ 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/internal/controller/state/graph/policies_test.go b/internal/controller/state/graph/policies_test.go index eeab0c33fd..9ccd5c7b29 100644 --- a/internal/controller/state/graph/policies_test.go +++ b/internal/controller/state/graph/policies_test.go @@ -2356,22 +2356,42 @@ func (s *testNGFLogSink) WithName(_ string) logr.LogSink { // createPolicyWithExistingGatewayStatus creates a fake policy with a gateway in its status ancestors. func createPolicyWithExistingGatewayStatus(gatewayNsName types.NamespacedName, controllerName string) policies.Policy { - fakePolicy := &policiesfakes.FakePolicy{ + ancestors := []v1alpha2.PolicyAncestorStatus{ + { + ControllerName: v1.GatewayController(controllerName), + AncestorRef: v1.ParentReference{ + Group: helpers.GetPointer[v1.Group](v1.GroupName), + Kind: helpers.GetPointer[v1.Kind](kinds.Gateway), + Namespace: (*v1.Namespace)(&gatewayNsName.Namespace), + Name: v1.ObjectName(gatewayNsName.Name), + }, + }, + } + return createFakePolicyWithAncestors("test-policy", "test", ancestors) +} + +// createFakePolicy creates a basic fake policy with common defaults. +func createFakePolicy(name, namespace string) *policiesfakes.FakePolicy { + return &policiesfakes.FakePolicy{ + GetNameStub: func() string { return name }, + GetNamespaceStub: func() string { return namespace }, GetPolicyStatusStub: func() v1alpha2.PolicyStatus { - return v1alpha2.PolicyStatus{ - Ancestors: []v1alpha2.PolicyAncestorStatus{ - { - ControllerName: v1.GatewayController(controllerName), - AncestorRef: v1.ParentReference{ - Group: helpers.GetPointer[v1.Group](v1.GroupName), - Kind: helpers.GetPointer[v1.Kind](kinds.Gateway), - Namespace: (*v1.Namespace)(&gatewayNsName.Namespace), - Name: v1.ObjectName(gatewayNsName.Name), - }, - }, - }, - } + return v1alpha2.PolicyStatus{} + }, + GetTargetRefsStub: func() []v1alpha2.LocalPolicyTargetReference { + return []v1alpha2.LocalPolicyTargetReference{} }, } - return fakePolicy +} + +// createFakePolicyWithAncestors creates a fake policy with specific ancestors. +func createFakePolicyWithAncestors( + name, namespace string, + ancestors []v1alpha2.PolicyAncestorStatus, +) *policiesfakes.FakePolicy { + policy := createFakePolicy(name, namespace) + policy.GetPolicyStatusStub = func() v1alpha2.PolicyStatus { + return v1alpha2.PolicyStatus{Ancestors: ancestors} + } + return policy } From 314a724f45a2918d4f548d342e229bfa92f395e2 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Thu, 14 Aug 2025 09:40:06 -0700 Subject: [PATCH 14/18] use generated fakes --- .../state/graph/policy_ancestor_test.go | 83 +++++-------------- 1 file changed, 21 insertions(+), 62 deletions(-) diff --git a/internal/controller/state/graph/policy_ancestor_test.go b/internal/controller/state/graph/policy_ancestor_test.go index 5a987c552c..42a34fcd4f 100644 --- a/internal/controller/state/graph/policy_ancestor_test.go +++ b/internal/controller/state/graph/policy_ancestor_test.go @@ -7,7 +7,6 @@ import ( "github.com/go-logr/logr/testr" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" v1 "sigs.k8s.io/gateway-api/apis/v1" @@ -15,6 +14,7 @@ import ( ngfAPIv1alpha2 "github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha2" "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies" + "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies/policiesfakes" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/kinds" ) @@ -278,64 +278,12 @@ func TestGetAncestorName(t *testing.T) { } } -type mockPolicy struct { - name string - namespace string - kind string -} - -func (m *mockPolicy) GetName() string { return m.name } -func (m *mockPolicy) SetName(name string) { m.name = name } -func (m *mockPolicy) GetNamespace() string { return m.namespace } -func (m *mockPolicy) SetNamespace(namespace string) { m.namespace = namespace } -func (m *mockPolicy) GetObjectKind() schema.ObjectKind { - if m.kind == "" { - return nil - } - return &mockObjectKind{kind: m.kind} -} -func (m *mockPolicy) GetTargetRefs() []v1alpha2.LocalPolicyTargetReference { return nil } -func (m *mockPolicy) GetPolicyStatus() v1alpha2.PolicyStatus { return v1alpha2.PolicyStatus{} } -func (m *mockPolicy) SetPolicyStatus(_ v1alpha2.PolicyStatus) {} -func (m *mockPolicy) DeepCopyObject() runtime.Object { return m } -func (m *mockPolicy) GetUID() types.UID { return "" } -func (m *mockPolicy) GetResourceVersion() string { return "" } -func (m *mockPolicy) SetUID(_ types.UID) {} -func (m *mockPolicy) SetResourceVersion(_ string) {} -func (m *mockPolicy) GetGeneration() int64 { return 0 } -func (m *mockPolicy) SetGeneration(_ int64) {} -func (m *mockPolicy) GetCreationTimestamp() metav1.Time { return metav1.Time{} } -func (m *mockPolicy) SetCreationTimestamp(_ metav1.Time) {} -func (m *mockPolicy) GetDeletionTimestamp() *metav1.Time { return nil } -func (m *mockPolicy) SetDeletionTimestamp(_ *metav1.Time) {} -func (m *mockPolicy) GetDeletionGracePeriodSeconds() *int64 { return nil } -func (m *mockPolicy) SetDeletionGracePeriodSeconds(*int64) {} -func (m *mockPolicy) GetLabels() map[string]string { return nil } -func (m *mockPolicy) SetLabels(_ map[string]string) {} -func (m *mockPolicy) GetAnnotations() map[string]string { return nil } -func (m *mockPolicy) SetAnnotations(_ map[string]string) {} -func (m *mockPolicy) GetFinalizers() []string { return nil } -func (m *mockPolicy) SetFinalizers(_ []string) {} -func (m *mockPolicy) GetOwnerReferences() []metav1.OwnerReference { return nil } -func (m *mockPolicy) SetOwnerReferences([]metav1.OwnerReference) {} -func (m *mockPolicy) GetManagedFields() []metav1.ManagedFieldsEntry { return nil } -func (m *mockPolicy) SetManagedFields(_ []metav1.ManagedFieldsEntry) {} -func (m *mockPolicy) GetGenerateName() string { return "" } -func (m *mockPolicy) SetGenerateName(_ string) {} -func (m *mockPolicy) GetSelfLink() string { return "" } -func (m *mockPolicy) SetSelfLink(_ string) {} - -type mockObjectKind struct{ kind string } - -func (m *mockObjectKind) GroupVersionKind() schema.GroupVersionKind { - return schema.GroupVersionKind{Kind: m.kind} -} -func (m *mockObjectKind) SetGroupVersionKind(_ schema.GroupVersionKind) {} - func TestGetPolicyName(t *testing.T) { t.Parallel() g := NewWithT(t) - policy := &mockPolicy{name: "test-policy", namespace: "test-ns"} + policy := &policiesfakes.FakePolicy{} + policy.GetNameReturns("test-policy") + policy.GetNamespaceReturns("test-ns") g.Expect(getPolicyName(policy)).To(Equal("test-ns/test-policy")) } @@ -343,17 +291,27 @@ func TestGetPolicyKind(t *testing.T) { t.Parallel() tests := []struct { name string - policy policies.Policy + setup func() policies.Policy expected string }{ { - name: "with kind", - policy: &mockPolicy{kind: "TestPolicy"}, + name: "with kind", + setup: func() policies.Policy { + policy := &policiesfakes.FakePolicy{} + objectKind := &policiesfakes.FakeObjectKind{} + objectKind.GroupVersionKindReturns(schema.GroupVersionKind{Kind: "TestPolicy"}) + policy.GetObjectKindReturns(objectKind) + return policy + }, expected: "TestPolicy", }, { - name: "without kind", - policy: &mockPolicy{}, + name: "without kind", + setup: func() policies.Policy { + policy := &policiesfakes.FakePolicy{} + policy.GetObjectKindReturns(nil) + return policy + }, expected: "Policy", }, } @@ -362,7 +320,8 @@ func TestGetPolicyKind(t *testing.T) { t.Run(test.name, func(t *testing.T) { t.Parallel() g := NewWithT(t) - g.Expect(getPolicyKind(test.policy)).To(Equal(test.expected)) + policy := test.setup() + g.Expect(getPolicyKind(policy)).To(Equal(test.expected)) }) } } From 127f29189c7fe90109e1d9035cc863f8cd834d26 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Thu, 14 Aug 2025 09:41:38 -0700 Subject: [PATCH 15/18] remove 16 ancestor limit from comment --- internal/controller/state/conditions/conditions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go index be9fcdcc64..83d2dd9b09 100644 --- a/internal/controller/state/conditions/conditions.go +++ b/internal/controller/state/conditions/conditions.go @@ -144,7 +144,7 @@ const ( PolicyReasonAncestorLimitReached v1alpha2.PolicyConditionReason = "AncestorLimitReached" // PolicyMessageAncestorLimitReached is a message used with PolicyReasonAncestorLimitReached - // when a policy cannot be applied due to the 16 ancestor limit being reached. + // when a policy cannot be applied due to the ancestor limit being reached. PolicyMessageAncestorLimitReached = "Policies cannot be applied because the ancestor status list " + "has reached the maximum size. The following policies have been ignored:" ) From 744caddc779a17783706c41897a56790b597e813 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Thu, 14 Aug 2025 11:33:09 -0700 Subject: [PATCH 16/18] remove extra var assignment --- internal/controller/state/graph/backend_tls_policy.go | 6 ++---- internal/controller/state/graph/policies.go | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/internal/controller/state/graph/backend_tls_policy.go b/internal/controller/state/graph/backend_tls_policy.go index 045c0d4a07..063b17880e 100644 --- a/internal/controller/state/graph/backend_tls_policy.go +++ b/internal/controller/state/graph/backend_tls_policy.go @@ -206,8 +206,7 @@ func addPolicyAncestorLimitCondition( newCondition := conditions.NewPolicyAncestorLimitReached() newCondition.Message = fmt.Sprintf("%s %s %s", newCondition.Message, policyType, policyName) - conds = append(conds, newCondition) - return conds + return append(conds, newCondition) } // collectOrderedGateways collects gateways in spec order (services) then creation time order (gateways within service). @@ -254,8 +253,7 @@ func collectOrderedGateways( sortGatewaysByCreationTime(existingGateways, gateways) sortGatewaysByCreationTime(newGateways, gateways) - existingGateways = append(existingGateways, newGateways...) - return existingGateways + return append(existingGateways, newGateways...) } func extractExistingNGFGatewayAncestors( diff --git a/internal/controller/state/graph/policies.go b/internal/controller/state/graph/policies.go index 2f947c743d..c4b135672b 100644 --- a/internal/controller/state/graph/policies.go +++ b/internal/controller/state/graph/policies.go @@ -113,8 +113,7 @@ func collectOrderedGatewaysForService( sortGatewaysByCreationTime(existingGateways, gateways) sortGatewaysByCreationTime(newGateways, gateways) - existingGateways = append(existingGateways, newGateways...) - return existingGateways + return append(existingGateways, newGateways...) } func (g *Graph) attachPolicies(validator validation.PolicyValidator, ctlrName string, logger logr.Logger) { From 7f7e8bddc0c96f5dc67f1f4516ca3bd5df572ef9 Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Thu, 14 Aug 2025 11:36:46 -0700 Subject: [PATCH 17/18] remove unecessary test case --- .../controller/state/conditions/conditions_test.go | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/internal/controller/state/conditions/conditions_test.go b/internal/controller/state/conditions/conditions_test.go index 0a079bfe14..45ebcc5808 100644 --- a/internal/controller/state/conditions/conditions_test.go +++ b/internal/controller/state/conditions/conditions_test.go @@ -5,7 +5,6 @@ import ( . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/gateway-api/apis/v1alpha2" ) func TestDeduplicateConditions(t *testing.T) { @@ -146,15 +145,3 @@ func TestHasMatchingCondition(t *testing.T) { }) } } - -func TestNewPolicyAncestorLimitReached(t *testing.T) { - t.Parallel() - g := NewWithT(t) - - condition := NewPolicyAncestorLimitReached() - - g.Expect(condition.Type).To(Equal(string(v1alpha2.PolicyConditionAccepted))) - g.Expect(condition.Status).To(Equal(metav1.ConditionFalse)) - g.Expect(condition.Reason).To(Equal(string(PolicyReasonAncestorLimitReached))) - g.Expect(condition.Message).To(Equal(PolicyMessageAncestorLimitReached)) -} From 0ae3ca855ba5f9debe95d88b3e67af0db4c31c6f Mon Sep 17 00:00:00 2001 From: Sarthak Agrawal Date: Thu, 14 Aug 2025 13:09:46 -0700 Subject: [PATCH 18/18] fix log and condition --- internal/controller/state/conditions/conditions.go | 4 ++-- internal/controller/state/graph/backend_tls_policy.go | 3 +-- internal/controller/state/graph/backend_tls_policy_test.go | 4 ++-- internal/controller/state/graph/policies_test.go | 4 ++-- internal/controller/state/graph/policy_ancestor.go | 2 +- 5 files changed, 8 insertions(+), 9 deletions(-) diff --git a/internal/controller/state/conditions/conditions.go b/internal/controller/state/conditions/conditions.go index 83d2dd9b09..9dcf829020 100644 --- a/internal/controller/state/conditions/conditions.go +++ b/internal/controller/state/conditions/conditions.go @@ -979,12 +979,12 @@ 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() Condition { +func NewPolicyAncestorLimitReached(policyType string, policyName string) Condition { return Condition{ Type: string(v1alpha2.PolicyConditionAccepted), Status: metav1.ConditionFalse, Reason: string(PolicyReasonAncestorLimitReached), - Message: PolicyMessageAncestorLimitReached, + Message: fmt.Sprintf("%s %s %s", PolicyMessageAncestorLimitReached, policyType, policyName), } } diff --git a/internal/controller/state/graph/backend_tls_policy.go b/internal/controller/state/graph/backend_tls_policy.go index 063b17880e..11a77c87c2 100644 --- a/internal/controller/state/graph/backend_tls_policy.go +++ b/internal/controller/state/graph/backend_tls_policy.go @@ -204,8 +204,7 @@ func addPolicyAncestorLimitCondition( } } - newCondition := conditions.NewPolicyAncestorLimitReached() - newCondition.Message = fmt.Sprintf("%s %s %s", newCondition.Message, policyType, policyName) + newCondition := conditions.NewPolicyAncestorLimitReached(policyType, policyName) return append(conds, newCondition) } diff --git a/internal/controller/state/graph/backend_tls_policy_test.go b/internal/controller/state/graph/backend_tls_policy_test.go index a244bab679..20567860b7 100644 --- a/internal/controller/state/graph/backend_tls_policy_test.go +++ b/internal/controller/state/graph/backend_tls_policy_test.go @@ -803,8 +803,8 @@ func TestAddGatewaysForBackendTLSPoliciesAncestorLimit(t *testing.T) { logAncestorLimitReached(testLogger, "test/btp-full-ancestors", "BackendTLSPolicy", "test/gateway1") logOutput := logBuf.String() - g.Expect(logOutput).To(ContainSubstring("Policy ancestor limit reached")) - g.Expect(logOutput).To(ContainSubstring("policy=test/btp-full-ancestors")) + g.Expect(logOutput).To(ContainSubstring("Policy ancestor limit reached for test/btp-full-ancestors")) + g.Expect(logOutput).To(ContainSubstring("test/btp-full-ancestors")) g.Expect(logOutput).To(ContainSubstring("policyKind=BackendTLSPolicy")) g.Expect(logOutput).To(ContainSubstring("ancestor=test/gateway1")) } diff --git a/internal/controller/state/graph/policies_test.go b/internal/controller/state/graph/policies_test.go index 9ccd5c7b29..e84bbd6b56 100644 --- a/internal/controller/state/graph/policies_test.go +++ b/internal/controller/state/graph/policies_test.go @@ -2304,8 +2304,8 @@ func TestNGFPolicyAncestorLimitHandling(t *testing.T) { // Verify logging occurred logOutput := logBuf.String() - g.Expect(logOutput).To(ContainSubstring("Policy ancestor limit reached")) - g.Expect(logOutput).To(ContainSubstring("policy=test/policy-full-ancestors")) + g.Expect(logOutput).To(ContainSubstring("Policy ancestor limit reached for test/policy-full-ancestors")) + g.Expect(logOutput).To(ContainSubstring("test/policy-full-ancestors")) g.Expect(logOutput).To(ContainSubstring("policyKind=TestPolicy")) g.Expect(logOutput).To(ContainSubstring("ancestor=test/gateway1")) } diff --git a/internal/controller/state/graph/policy_ancestor.go b/internal/controller/state/graph/policy_ancestor.go index d2c056201a..4ab2e970e4 100644 --- a/internal/controller/state/graph/policy_ancestor.go +++ b/internal/controller/state/graph/policy_ancestor.go @@ -15,7 +15,7 @@ const maxAncestors = 16 // logAncestorLimitReached logs when a policy ancestor limit is reached. func logAncestorLimitReached(logger logr.Logger, policyName, policyKind, ancestorName string) { - logger.Info("Policy ancestor limit reached", "policy", policyName, "policyKind", policyKind, "ancestor", ancestorName) + logger.Info("Policy ancestor limit reached for "+policyName, "policyKind", policyKind, "ancestor", ancestorName) } // ngfPolicyAncestorsFull returns whether or not an ancestor list is full. A list is full when