Review feedback

Author: ciarams87

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

@@ -10,6 +10,8 @@ import (
 	"github.com/nginx/nginx-gateway-fabric/internal/framework/helpers"
 )
 
+const appProtectBundleFolder = "/etc/app_protect/bundles"
+
 var tmpl = template.Must(template.New("waf policy").Parse(wafTemplate))
 
 const wafTemplate = `
@@ -63,7 +65,7 @@ func generate(pols []policies.Policy) policies.GenerateResultFiles {
 		if wp.Spec.PolicySource != nil && wp.Spec.PolicySource.FileLocation != "" {
 			fileLocation := wp.Spec.PolicySource.FileLocation
 			bundleName := helpers.ToSafeFileName(fileLocation)
-			bundlePath := fmt.Sprintf("%s/%s.tgz", "/etc/app_protect/bundles", bundleName)
+			bundlePath := fmt.Sprintf("%s/%s.tgz", appProtectBundleFolder, bundleName)
 			fields["BundlePath"] = bundlePath
 		}
 
@@ -79,7 +81,7 @@ func generate(pols []policies.Policy) policies.GenerateResultFiles {
 
 				if secLog.LogProfileBundle != nil && secLog.LogProfileBundle.FileLocation != "" {
 					bundleName := helpers.ToSafeFileName(secLog.LogProfileBundle.FileLocation)
-					bundlePath := fmt.Sprintf("%s/%s.tgz", "/etc/app_protect/bundles", bundleName)
+					bundlePath := fmt.Sprintf("%s/%s.tgz", appProtectBundleFolder, bundleName)
 					logEntry["LogProfileBundlePath"] = bundlePath
 				}
 

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

@@ -1,6 +1,7 @@
 package waf_test
 
 import (
+	"fmt"
 	"testing"
 
 	. "github.com/onsi/gomega"
@@ -11,10 +12,16 @@ import (
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/config/http"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/config/policies"
 	"github.com/nginx/nginx-gateway-fabric/internal/controller/nginx/config/policies/waf"
+	"github.com/nginx/nginx-gateway-fabric/internal/framework/helpers"
 )
 
 func TestGenerate(t *testing.T) {
 	t.Parallel()
+
+	apDirBase := "app_protect_policy_file \"/etc/app_protect/bundles"
+	apFileDirective := fmt.Sprintf("%s/%s", apDirBase, helpers.ToSafeFileName("http://example.com/policy.tgz"))
+	apSecLogBase := "app_protect_security_log \"/etc/app_protect/bundles"
+	apSecLogDirective := fmt.Sprintf("%s/%s", apSecLogBase, helpers.ToSafeFileName("http://example.com/custom-log.tgz"))
 	tests := []struct {
 		name       string
 		policy     policies.Policy
@@ -35,7 +42,7 @@ func TestGenerate(t *testing.T) {
 			},
 			expStrings: []string{
 				"app_protect_enable on;",
-				"app_protect_policy_file \"/etc/app_protect/bundles/",
+				apFileDirective,
 			},
 		},
 		{
@@ -64,7 +71,7 @@ func TestGenerate(t *testing.T) {
 			},
 			expStrings: []string{
 				"app_protect_enable on;",
-				"app_protect_policy_file \"/etc/app_protect/bundles/",
+				apFileDirective,
 				"app_protect_security_log_enable on;",
 				"app_protect_security_log \"log_default\" stderr;",
 			},
@@ -94,7 +101,7 @@ func TestGenerate(t *testing.T) {
 			},
 			expStrings: []string{
 				"app_protect_security_log_enable on;",
-				"app_protect_security_log \"/etc/app_protect/bundles/",
+				apSecLogDirective,
 				"/var/log/nginx/security.log;",
 			},
 		},
@@ -165,7 +172,7 @@ func TestGenerate(t *testing.T) {
 			},
 			expStrings: []string{
 				"app_protect_enable on;",
-				"app_protect_policy_file \"/etc/app_protect/bundles/",
+				apFileDirective,
 				"app_protect_security_log_enable on;",
 				"app_protect_security_log \"log_all\" stderr;",
 				"app_protect_security_log \"log_blocked\" /var/log/blocked.log;",

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

@@ -67,9 +67,7 @@ func (v *Validator) ValidateGlobalSettings(
 }
 
 // Conflicts returns false as we don't allow merging for WAFPolicies.
-func (v Validator) Conflicts(polA, polB policies.Policy) bool {
-	_ = helpers.MustCastObject[*ngfAPI.WAFPolicy](polA)
-	_ = helpers.MustCastObject[*ngfAPI.WAFPolicy](polB)
+func (v Validator) Conflicts(_, _ policies.Policy) bool {
 	return false
 }
 

internal/controller/state/graph/policies.go

@@ -493,11 +493,7 @@ func fetchWAFPolicyBundleData(
 	refPolicyBundles := make(map[WAFBundleKey]*WAFBundleData)
 
 	for policyKey, policy := range processedPolicies {
-		if policyKey.GVK != wafPolicyGVK {
-			continue
-		}
-
-		if !policy.Valid {
+		if policyKey.GVK != wafPolicyGVK || !policy.Valid {
 			continue
 		}