Enhance SnippetsPolicy validation and configuration generation

Author: fabian4

apis/v1alpha1/snippetspolicy_types.go

@@ -56,6 +56,7 @@ type SnippetsPolicySpec struct {
 	// +kubebuilder:validation:MinItems=1
 	// +kubebuilder:validation:MaxItems=16
 	// +kubebuilder:validation:XValidation:message="TargetRefs Kind must be Gateway",rule="self.all(t, t.kind == 'Gateway')"
+	// +kubebuilder:validation:XValidation:message="TargetRefs Name must be unique",rule="self.all(p1, self.exists_one(p2, (p1.name == p2.name)))"
 	// +kubebuilder:validation:XValidation:message="TargetRefs Group must be gateway.networking.k8s.io",rule="self.all(t, t.group == 'gateway.networking.k8s.io')"
 	//nolint:lll
 	TargetRefs []gatewayv1.LocalPolicyTargetReference `json:"targetRefs"`
@@ -64,5 +65,7 @@ type SnippetsPolicySpec struct {
 	// +kubebuilder:validation:MaxItems=4
 	// +kubebuilder:validation:XValidation:message="Only one snippet allowed per context",rule="self.all(s1, self.exists_one(s2, s1.context == s2.context))"
 	//nolint:lll
-	Snippets []Snippet `json:"snippets"`
+	//
+	// +optional
+	Snippets []Snippet `json:"snippets,omitempty"`
 }

config/crd/bases/gateway.nginx.org_snippetspolicies.yaml

@@ -116,10 +116,11 @@ spec:
                 x-kubernetes-validations:
                 - message: TargetRefs Kind must be Gateway
                   rule: self.all(t, t.kind == 'Gateway')
+                - message: TargetRefs Name must be unique
+                  rule: self.all(p1, self.exists_one(p2, (p1.name == p2.name)))
                 - message: TargetRefs Group must be gateway.networking.k8s.io
                   rule: self.all(t, t.group == 'gateway.networking.k8s.io')
             required:
-            - snippets
             - targetRefs
             type: object
           status:

internal/controller/nginx/config/policies/snippetspolicy/generator.go

@@ -18,7 +18,6 @@ package snippetspolicy
 
 import (
 	"fmt"
-	"path/filepath"
 
 	"github.com/nginx/nginx-gateway-fabric/v2/apis/v1alpha1"
 	"github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/config/http"
@@ -27,19 +26,19 @@ import (
 
 const (
 	mainTemplate = `
-# SnippetsPolicy %s for Gateway %s main context
+# SnippetsPolicy %s main context
 %s
 `
 	httpTemplate = `
-# SnippetsPolicy %s for Gateway %s http context
+# SnippetsPolicy %s http context
 %s
 `
 	serverTemplate = `
-# SnippetsPolicy %s for Gateway %s server context in listener %s
+# SnippetsPolicy %s server context
 %s
 `
 	locationTemplate = `
-# SnippetsPolicy %s for Gateway %s location context with match key %s
+# SnippetsPolicy %s location context
 %s
 `
 )
@@ -56,28 +55,27 @@ func NewGenerator() *Generator {
 
 // GenerateForMain generates policy configuration for the main block.
 func (g *Generator) GenerateForMain(pols []policies.Policy) policies.GenerateResultFiles {
-	return g.generate(pols, v1alpha1.NginxContextMain, "")
+	return g.generate(pols, v1alpha1.NginxContextMain)
 }
 
 // GenerateForHTTP generates policy configuration for the http block.
 func (g *Generator) GenerateForHTTP(pols []policies.Policy) policies.GenerateResultFiles {
-	return g.generate(pols, v1alpha1.NginxContextHTTP, "")
+	return g.generate(pols, v1alpha1.NginxContextHTTP)
 }
 
 // GenerateForServer generates policy configuration for the server block.
-func (g *Generator) GenerateForServer(pols []policies.Policy, server http.Server) policies.GenerateResultFiles {
-	return g.generate(pols, v1alpha1.NginxContextHTTPServer, server.Listen)
+func (g *Generator) GenerateForServer(pols []policies.Policy, _ http.Server) policies.GenerateResultFiles {
+	return g.generate(pols, v1alpha1.NginxContextHTTPServer)
 }
 
 // GenerateForLocation generates policy configuration for the location block.
-func (g *Generator) GenerateForLocation(pols []policies.Policy, location http.Location) policies.GenerateResultFiles {
-	return g.generate(pols, v1alpha1.NginxContextHTTPServerLocation, location.HTTPMatchKey)
+func (g *Generator) GenerateForLocation(pols []policies.Policy, _ http.Location) policies.GenerateResultFiles {
+	return g.generate(pols, v1alpha1.NginxContextHTTPServerLocation)
 }
 
 func (g *Generator) generate(
 	pols []policies.Policy,
 	context v1alpha1.NginxContext,
-	id string,
 ) policies.GenerateResultFiles {
 	var files policies.GenerateResultFiles
 
@@ -92,48 +90,32 @@ func (g *Generator) generate(
 				continue
 			}
 
-			for _, gwRef := range sp.Spec.TargetRefs {
-				// TargetRef info for file path and comment
-				gwNsName := fmt.Sprintf("%s-%s", sp.GetNamespace(), gwRef.Name) // Assuming local target ref implies same namespace
-
-				policyNsName := fmt.Sprintf("%s/%s", sp.GetNamespace(), sp.GetName())
-
-				// Build content and filename
-				var content string
-				var filename string
-
-				switch context {
-				case v1alpha1.NginxContextMain:
-					content = fmt.Sprintf(mainTemplate, policyNsName, gwNsName, snippet.Value)
-					filename = filepath.Join(
-						"includes", "policy", gwNsName,
-						fmt.Sprintf("SnippetsPolicy_main_%s.conf", sp.GetName()),
-					)
-				case v1alpha1.NginxContextHTTP:
-					content = fmt.Sprintf(httpTemplate, policyNsName, gwNsName, snippet.Value)
-					filename = filepath.Join(
-						"includes", "policy", gwNsName,
-						fmt.Sprintf("SnippetsPolicy_http_%s.conf", sp.GetName()),
-					)
-				case v1alpha1.NginxContextHTTPServer:
-					content = fmt.Sprintf(serverTemplate, policyNsName, gwNsName, id, snippet.Value)
-					filename = filepath.Join(
-						"includes", "policy", gwNsName, id,
-						fmt.Sprintf("SnippetsPolicy_server_%s.conf", sp.GetName()),
-					)
-				case v1alpha1.NginxContextHTTPServerLocation:
-					content = fmt.Sprintf(locationTemplate, policyNsName, gwNsName, id, snippet.Value)
-					filename = filepath.Join(
-						"includes", "policy", gwNsName, id,
-						fmt.Sprintf("SnippetsPolicy_location_%s.conf", sp.GetName()),
-					)
-				}
-
-				files = append(files, policies.File{
-					Name:    filename,
-					Content: []byte(content),
-				})
+			policyNsName := fmt.Sprintf("%s/%s", sp.GetNamespace(), sp.GetName())
+			policyFileID := fmt.Sprintf("%s-%s", sp.GetNamespace(), sp.GetName())
+
+			// Build content and filename
+			var content string
+			var filename string
+
+			switch context {
+			case v1alpha1.NginxContextMain:
+				content = fmt.Sprintf(mainTemplate, policyNsName, snippet.Value)
+				filename = fmt.Sprintf("SnippetsPolicy_main_%s.conf", policyFileID)
+			case v1alpha1.NginxContextHTTP:
+				content = fmt.Sprintf(httpTemplate, policyNsName, snippet.Value)
+				filename = fmt.Sprintf("SnippetsPolicy_http_%s.conf", policyFileID)
+			case v1alpha1.NginxContextHTTPServer:
+				content = fmt.Sprintf(serverTemplate, policyNsName, snippet.Value)
+				filename = fmt.Sprintf("SnippetsPolicy_server_%s.conf", policyFileID)
+			case v1alpha1.NginxContextHTTPServerLocation:
+				content = fmt.Sprintf(locationTemplate, policyNsName, snippet.Value)
+				filename = fmt.Sprintf("SnippetsPolicy_location_%s.conf", policyFileID)
 			}
+
+			files = append(files, policies.File{
+				Name:    filename,
+				Content: []byte(content),
+			})
 		}
 	}
 	return files

internal/controller/nginx/config/policies/snippetspolicy/generator_test.go

@@ -57,15 +57,15 @@ func TestGenerator(t *testing.T) {
 		gWithT := NewWithT(t)
 		files := g.GenerateForMain(pols)
 		gWithT.Expect(files).To(HaveLen(1))
-		gWithT.Expect(files[0].Name).To(Equal("includes/policy/default-gateway-1/SnippetsPolicy_main_policy-1.conf"))
+		gWithT.Expect(files[0].Name).To(Equal("SnippetsPolicy_main_default-policy-1.conf"))
 		gWithT.Expect(string(files[0].Content)).To(ContainSubstring("worker_processes 1;"))
 	})
 
 	t.Run("GenerateForHTTP", func(t *testing.T) {
 		gWithT := NewWithT(t)
 		files := g.GenerateForHTTP(pols)
 		gWithT.Expect(files).To(HaveLen(1))
-		gWithT.Expect(files[0].Name).To(Equal("includes/policy/default-gateway-1/SnippetsPolicy_http_policy-1.conf"))
+		gWithT.Expect(files[0].Name).To(Equal("SnippetsPolicy_http_default-policy-1.conf"))
 		gWithT.Expect(string(files[0].Content)).To(ContainSubstring("log_format custom '...';"))
 	})
 
@@ -76,7 +76,7 @@ func TestGenerator(t *testing.T) {
 		}
 		files := g.GenerateForServer(pols, server)
 		gWithT.Expect(files).To(HaveLen(1))
-		gWithT.Expect(files[0].Name).To(Equal("includes/policy/default-gateway-1/80/SnippetsPolicy_server_policy-1.conf"))
+		gWithT.Expect(files[0].Name).To(Equal("SnippetsPolicy_server_default-policy-1.conf"))
 		gWithT.Expect(string(files[0].Content)).To(ContainSubstring("client_max_body_size 10m;"))
 	})
 
@@ -88,11 +88,29 @@ func TestGenerator(t *testing.T) {
 		files := g.GenerateForLocation(pols, location)
 		gWithT.Expect(files).To(HaveLen(1))
 		gWithT.Expect(files[0].Name).To(Equal(
-			"includes/policy/default-gateway-1/12345/SnippetsPolicy_location_policy-1.conf",
+			"SnippetsPolicy_location_default-policy-1.conf",
 		))
 		gWithT.Expect(string(files[0].Content)).To(ContainSubstring("location_snippet;"))
 	})
 
+	t.Run("GenerateForMain with empty snippets", func(t *testing.T) {
+		gWithT := NewWithT(t)
+		policy := &v1alpha1.SnippetsPolicy{
+			ObjectMeta: metav1.ObjectMeta{Name: "p-empty", Namespace: "default"},
+			Spec: v1alpha1.SnippetsPolicySpec{
+				TargetRefs: []gatewayv1.LocalPolicyTargetReference{
+					{
+						Name: "gw",
+					},
+				},
+				Snippets: nil,
+			},
+		}
+
+		files := g.GenerateForMain([]policies.Policy{policy})
+		gWithT.Expect(files).To(BeEmpty())
+	})
+
 	t.Run("GenerateForMain with multiple policies", func(t *testing.T) {
 		gWithT := NewWithT(t)
 		policy1 := &v1alpha1.SnippetsPolicy{
@@ -148,12 +166,8 @@ func TestGenerator(t *testing.T) {
 		}
 
 		files := g.GenerateForMain([]policies.Policy{policy})
-		gWithT.Expect(files).To(HaveLen(2))
-		gWithT.Expect(files[0].Name).To(ContainSubstring("gw1"))
-		gWithT.Expect(files[1].Name).To(ContainSubstring("gw2"))
+		gWithT.Expect(files).To(HaveLen(1))
+		gWithT.Expect(files[0].Name).To(Equal("SnippetsPolicy_main_default-policy-multi.conf"))
 		gWithT.Expect(string(files[0].Content)).To(ContainSubstring("data;"))
-		gWithT.Expect(string(files[1].Content)).To(ContainSubstring("data;"))
-		gWithT.Expect(string(files[0].Content)).To(ContainSubstring("gw1"))
-		gWithT.Expect(string(files[1].Content)).To(ContainSubstring("gw2"))
 	})
 }

internal/controller/nginx/config/policies/snippetspolicy/validator.go

@@ -47,6 +47,7 @@ func (v *Validator) Validate(policy policies.Policy) []conditions.Condition {
 	supportedGroups := []gatewayv1.Group{gatewayv1.GroupName}
 
 	// Validate TargetRef
+	seenTargetRefs := make(map[string]struct{})
 	for i, targetRef := range sp.Spec.TargetRefs {
 		if err := policies.ValidateTargetRef(
 			targetRef,
@@ -56,6 +57,12 @@ func (v *Validator) Validate(policy policies.Policy) []conditions.Condition {
 		); err != nil {
 			return []conditions.Condition{conditions.NewPolicyInvalid(err.Error())}
 		}
+
+		if _, exists := seenTargetRefs[string(targetRef.Name)]; exists {
+			msg := fmt.Sprintf("duplicate targetRef name %q", targetRef.Name)
+			return []conditions.Condition{conditions.NewPolicyInvalid(msg)}
+		}
+		seenTargetRefs[string(targetRef.Name)] = struct{}{}
 	}
 
 	// Validate Snippets

internal/controller/nginx/config/policies/snippetspolicy/validator_test.go

@@ -119,6 +119,28 @@ func TestValidator_Validate(t *testing.T) {
 				conditions.NewPolicyInvalid("duplicate context \"main\""),
 			},
 		},
+		{
+			name: "duplicate target ref name",
+			policy: createModifiedPolicy(func(p *ngfAPI.SnippetsPolicy) *ngfAPI.SnippetsPolicy {
+				p.Spec.TargetRefs = append(p.Spec.TargetRefs, gatewayv1.LocalPolicyTargetReference{
+					Group: gatewayv1.GroupName,
+					Kind:  kinds.Gateway,
+					Name:  "test-gateway",
+				})
+				return p
+			}),
+			expConditions: []conditions.Condition{
+				conditions.NewPolicyInvalid("duplicate targetRef name \"test-gateway\""),
+			},
+		},
+		{
+			name: "valid policy with empty snippets",
+			policy: createModifiedPolicy(func(p *ngfAPI.SnippetsPolicy) *ngfAPI.SnippetsPolicy {
+				p.Spec.Snippets = nil
+				return p
+			}),
+			expConditions: nil,
+		},
 	}
 
 	v := snippetspolicy.NewValidator()

internal/controller/state/graph/policies.go

@@ -125,7 +125,7 @@ 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, logger)
+				attachPolicyToGateway(policy, ref, g.Gateways, g.Routes, ctlrName, logger)
 			case kinds.HTTPRoute, kinds.GRPCRoute:
 				route, exists := g.Routes[routeKeyForKind(ref.Kind, ref.Nsname)]
 				if !exists {
@@ -292,6 +292,7 @@ func attachPolicyToGateway(
 	policy *Policy,
 	ref PolicyTargetRef,
 	gateways map[types.NamespacedName]*Gateway,
+	routes map[RouteKey]*L7Route,
 	ctlrName string,
 	logger logr.Logger,
 ) {
@@ -306,6 +307,7 @@ func attachPolicyToGateway(
 		// 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)
+			propagateSnippetsPolicyToRoutes(policy, gw, routes)
 		}
 		return
 	}
@@ -350,6 +352,40 @@ func attachPolicyToGateway(
 
 	policy.Ancestors = append(policy.Ancestors, ancestor)
 	gw.Policies = append(gw.Policies, policy)
+	propagateSnippetsPolicyToRoutes(policy, gw, routes)
+}
+
+func propagateSnippetsPolicyToRoutes(
+	policy *Policy,
+	gw *Gateway,
+	routes map[RouteKey]*L7Route,
+) {
+	// Only SnippetsPolicy supports propagation from Gateway to Routes
+	if getPolicyKind(policy.Source) != kinds.SnippetsPolicy {
+		return
+	}
+
+	gwNsName := client.ObjectKeyFromObject(gw.Source)
+
+	for _, route := range routes {
+		for _, parentRef := range route.ParentRefs {
+			// Check if the route is attached to this specific gateway
+			if parentRef.Gateway != nil && parentRef.Gateway.NamespacedName == gwNsName {
+				// Avoid duplicate attachment if logic runs multiple times (though graph build is single pass)
+				// or if policy targets both.
+				alreadyAttached := false
+				for _, p := range route.Policies {
+					if p == policy {
+						alreadyAttached = true
+						break
+					}
+				}
+				if !alreadyAttached {
+					route.Policies = append(route.Policies, policy)
+				}
+			}
+		}
+	}
 }
 
 func processPolicies(

internal/controller/state/graph/policies_test.go

@@ -644,7 +644,7 @@ func TestAttachPolicyToGateway(t *testing.T) {
 			t.Parallel()
 			g := NewWithT(t)
 
-			attachPolicyToGateway(test.policy, test.policy.TargetRefs[0], test.gws, "nginx-gateway", logr.Discard())
+			attachPolicyToGateway(test.policy, test.policy.TargetRefs[0], test.gws, nil, "nginx-gateway", logr.Discard())
 
 			if test.expAttached {
 				for _, gw := range test.gws {