Add more unit tests

Author: Sarthak Agrawal

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
 			}

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
+}

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
 }