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..9dcf829020 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 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:" ) // 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(policyType string, policyName string) Condition { + return Condition{ + Type: string(v1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(PolicyReasonAncestorLimitReached), + Message: fmt.Sprintf("%s %s %s", PolicyMessageAncestorLimitReached, policyType, policyName), + } +} + // 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/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..11a77c87c2 100644 --- a/internal/controller/state/graph/backend_tls_policy.go +++ b/internal/controller/state/graph/backend_tls_policy.go @@ -3,7 +3,9 @@ package graph import ( "fmt" "slices" + "strings" + "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" @@ -18,7 +20,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 @@ -34,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 { @@ -45,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{ @@ -68,17 +70,10 @@ func validateBackendTLSPolicy( backendTLSPolicy *v1alpha3.BackendTLSPolicy, configMapResolver *configMapResolver, secretResolver *secretResolver, - ctlrName 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 - } - if err := validateBackendTLSHostname(backendTLSPolicy); err != nil { valid = false conds = append(conds, conditions.NewPolicyInvalid(fmt.Sprintf("invalid hostname: %s", err.Error()))) @@ -183,31 +178,148 @@ func validateBackendTLSWellKnownCACerts(btp *v1alpha3.BackendTLSPolicy) error { return nil } +// 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 { + for i, condition := range conds { + if condition.Reason == string(conditions.PolicyReasonAncestorLimitReached) { + if !strings.Contains(condition.Message, policyName) { + conds[i].Message = fmt.Sprintf("%s, %s %s", condition.Message, policyType, policyName) + } + return conds + } + } + + newCondition := conditions.NewPolicyAncestorLimitReached(policyType, policyName) + return append(conds, newCondition) +} + +// collectOrderedGateways collects gateways in spec order (services) then creation time order (gateways within service). +func collectOrderedGateways( + policy *v1alpha3.BackendTLSPolicy, + services map[types.NamespacedName]*ReferencedService, + gateways map[types.NamespacedName]*Gateway, + existingNGFGatewayAncestors map[types.NamespacedName]struct{}, +) []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 { + if refs.Kind != kinds.Service { + continue + } + + svcNsName := types.NamespacedName{ + Namespace: policy.Namespace, + Name: string(refs.Name), + } + + referencedService, exists := services[svcNsName] + if !exists { + continue + } + + // 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) + } + } + } + + sortGatewaysByCreationTime(existingGateways, gateways) + sortGatewaysByCreationTime(newGateways, gateways) + + return append(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 +} + 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 { - gateways := make(map[types.NamespacedName]struct{}) + existingNGFGatewayAncestors := extractExistingNGFGatewayAncestors(backendTLSPolicy.Source, ctlrName) + orderedGateways := collectOrderedGateways( + backendTLSPolicy.Source, + services, + gateways, + existingNGFGatewayAncestors, + ) - for _, refs := range backendTLSPolicy.Source.Spec.TargetRefs { - if refs.Kind != kinds.Service { - continue - } + ancestorCount := countNonNGFAncestors(backendTLSPolicy.Source, ctlrName) - for svcNsName, referencedServices := range services { - if svcNsName.Name != string(refs.Name) { - continue - } + // 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) - for gateway := range referencedServices.GatewayNsNames { - gateways[gateway] = struct{}{} + if gateway, ok := gateways[gatewayNsName]; ok { + gateway.Conditions = addPolicyAncestorLimitCondition(gateway.Conditions, policyName, kinds.BackendTLSPolicy) + } else { + // 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 } - } - for gateway := range gateways { - backendTLSPolicy.Gateways = append(backendTLSPolicy.Gateways, gateway) + ancestorCount++ + + 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..20567860b7 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" ) @@ -77,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)) }) @@ -399,7 +402,7 @@ func TestValidateBackendTLSPolicy(t *testing.T) { }, }, { - name: "invalid case with too many ancestors", + name: "valid case with many ancestors", tlsPolicy: &v1alpha3.BackendTLSPolicy{ ObjectMeta: metav1.ObjectMeta{ Name: "tls-policy", @@ -416,7 +419,7 @@ func TestValidateBackendTLSPolicy(t *testing.T) { Ancestors: ancestors, }, }, - ignored: true, + isValid: true, }, } @@ -474,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)) @@ -660,8 +663,148 @@ 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", 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(&testNGFLogSink{buffer: &logBuf}) + + // Helper function to create ancestor references + 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")) + + // 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 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/graph.go b/internal/controller/state/graph/graph.go index 52cb047ae2..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" @@ -198,6 +199,7 @@ func BuildGraph( gcName string, plusSecrets map[types.NamespacedName][]PlusSecretFile, validators validation.Validators, + logger logr.Logger, ) *Graph { processedGwClasses, gcExists := processGatewayClasses(state.GatewayClasses, gcName, controllerName) if gcExists && processedGwClasses.Winner == nil { @@ -236,7 +238,6 @@ func BuildGraph( state.BackendTLSPolicies, configMapResolver, secretResolver, - controllerName, gws, ) @@ -269,7 +270,7 @@ func BuildGraph( referencedServices := buildReferencedServices(routes, l4routes, gws) - addGatewaysForBackendTLSPolicies(processedBackendTLSPolicies, referencedServices) + 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( @@ -302,7 +303,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/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..c4b135672b 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" @@ -22,6 +23,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 @@ -69,7 +71,52 @@ 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) { +// 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{}, +) []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 append(existingGateways, newGateways...) +} + +func (g *Graph) attachPolicies(validator validation.PolicyValidator, ctlrName string, logger logr.Logger) { if len(g.Gateways) == 0 { return } @@ -78,21 +125,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) } } } @@ -103,58 +150,107 @@ func attachPolicyToService( svc *ReferencedService, gws map[types.NamespacedName]*Gateway, ctlrName string, + logger logr.Logger, ) { - if ngfPolicyAncestorsFull(policy, ctlrName) { - return - } + var attachedToAnyGateway bool + + // Extract existing NGF gateway ancestors from policy status + existingNGFGatewayAncestors := extractExistingNGFGatewayAncestorsForPolicy(policy, ctlrName) + + // Collect and order gateways with existing gateway prioritization + orderedGateways := collectOrderedGatewaysForService(svc, gws, existingNGFGatewayAncestors) - var validForAGateway bool - for gwNsName, gw := range gws { - if _, belongsToGw := svc.GatewayNsNames[gwNsName]; !belongsToGw { + for _, gwNsName := range orderedGateways { + gw := gws[gwNsName] + + if gw == nil || gw.Source == nil { 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 + } + + // 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 = addPolicyAncestorLimitCondition(gw.Conditions, policyName, policyKind) + logAncestorLimitReached(logger, policyName, policyKind, gwNsName.String()) + + // Mark this gateway as invalid for the policy due to ancestor limits + policy.InvalidForGateways[gwNsName] = struct{}{} + continue } 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 } - 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 - } - +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 } 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 = addPolicyAncestorLimitCondition(route.Conditions, policyName, policyKind) + logAncestorLimitReached(logger, 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 +259,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 +273,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( @@ -191,17 +293,44 @@ func attachPolicyToGateway( ref PolicyTargetRef, gateways map[types.NamespacedName]*Gateway, ctlrName string, + logger logr.Logger, ) { - 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 = 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. + logger.Info("Gateway target not found and ancestors slice is full.", "policy", policyName, "ancestor", ancestorName) + } + logAncestorLimitReached(logger, 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 +346,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..e84bbd6b56 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)) @@ -575,7 +577,6 @@ func TestAttachPolicyToGateway(t *testing.T) { gws: newGatewayMap(true, []types.NamespacedName{gatewayNsName}), expAncestors: []PolicyAncestor{ {Ancestor: getGatewayParentRef(gatewayNsName)}, - {Ancestor: getGatewayParentRef(gatewayNsName)}, }, expAttached: true, }, @@ -644,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 { @@ -729,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{ @@ -831,7 +872,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 { @@ -1964,3 +2005,393 @@ 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 + currentAncestors []v1alpha2.PolicyAncestorStatus + updatedAncestorsLen int + expectFull bool + }{ + { + name: "empty current ancestors, no updated ancestors", + currentAncestors: []v1alpha2.PolicyAncestorStatus{}, + updatedAncestorsLen: 0, + expectFull: false, + }, + { + name: "less than 16 total (current + updated)", + currentAncestors: []v1alpha2.PolicyAncestorStatus{ + getAncestorRef("other-controller", "gateway1"), + getAncestorRef("other-controller", "gateway2"), + }, + updatedAncestorsLen: 2, + expectFull: 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 + expectFull: 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, + 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)", + currentAncestors: func() []v1alpha2.PolicyAncestorStatus { + ancestors := make([]v1alpha2.PolicyAncestorStatus, 10) + for i := range 10 { + ancestors[i] = getAncestorRef("other-controller", "gateway") + } + return ancestors + }(), + updatedAncestorsLen: 7, + expectFull: 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, + expectFull: 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, "nginx-gateway") + g.Expect(result).To(Equal(test.expectFull)) + }) + } +} + +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"} + + // Helper function to create ancestor references + 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")) + + // 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 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")) +} + +// 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 +} + +// createPolicyWithExistingGatewayStatus creates a fake policy with a gateway in its status ancestors. +func createPolicyWithExistingGatewayStatus(gatewayNsName types.NamespacedName, controllerName string) policies.Policy { + 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{} + }, + GetTargetRefsStub: func() []v1alpha2.LocalPolicyTargetReference { + return []v1alpha2.LocalPolicyTargetReference{} + }, + } +} + +// 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 +} diff --git a/internal/controller/state/graph/policy_ancestor.go b/internal/controller/state/graph/policy_ancestor.go index 698b887511..4ab2e970e4 100644 --- a/internal/controller/state/graph/policy_ancestor.go +++ b/internal/controller/state/graph/policy_ancestor.go @@ -1,34 +1,21 @@ 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/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/policies" "github.com/nginx/nginx-gateway-fabric/v2/internal/framework/helpers" ) const maxAncestors = 16 -// 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 +// logAncestorLimitReached logs when a policy ancestor limit is reached. +func logAncestorLimitReached(logger logr.Logger, policyName, policyKind, ancestorName string) { + 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 @@ -95,3 +82,53 @@ func parentRefEqual(ref1, ref2 v1.ParentReference) bool { return true } + +// 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 { + ancestorName = string(*ancestorRef.Namespace) + "/" + ancestorName + } + return ancestorName +} + +// 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 or "Policy" if GetObjectKind() returns nil. +func getPolicyKind(policy policies.Policy) string { + policyKind := "Policy" + if objKind := policy.GetObjectKind(); objKind != nil { + policyKind = objKind.GroupVersionKind().Kind + } + 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/controller/state/graph/policy_ancestor_test.go b/internal/controller/state/graph/policy_ancestor_test.go index 49903cf0df..42a34fcd4f 100644 --- a/internal/controller/state/graph/policy_ancestor_test.go +++ b/internal/controller/state/graph/policy_ancestor_test.go @@ -2,63 +2,22 @@ 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/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/controller/nginx/config/policies/policiesfakes" "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 { @@ -279,3 +238,190 @@ 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)) + }) + } +} + +func TestGetPolicyName(t *testing.T) { + t.Parallel() + g := NewWithT(t) + policy := &policiesfakes.FakePolicy{} + policy.GetNameReturns("test-policy") + policy.GetNamespaceReturns("test-ns") + g.Expect(getPolicyName(policy)).To(Equal("test-ns/test-policy")) +} + +func TestGetPolicyKind(t *testing.T) { + t.Parallel() + tests := []struct { + name string + setup func() policies.Policy + expected string + }{ + { + 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", + setup: func() policies.Policy { + policy := &policiesfakes.FakePolicy{} + policy.GetObjectKindReturns(nil) + return policy + }, + expected: "Policy", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + policy := test.setup() + g.Expect(getPolicyKind(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)) + }) + } +} 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.