Restrict policies to non-duplicate routes (#2318)

Problem: Some NGINX directives are not applied or enforced when configured in an internal location. This occurs when redirecting or rewriting a request from an external location to an internal location.

Solution: Only accept a policy if the Route it targets is the only Route that matches the hostname, port, and path combination. If other Routes overlap, the policy will be rejected.

This allows us to apply policy configuration to the external location instead of the internal locations. We would limit the policies we accept rather than limiting which Routes we accept.

This is possible because, with the policy restriction, a policy cannot be applied to a Route that shares an external location with another Route.

However, for the otel module, we still require some internal location directives to be specified, so the policy generator has been refactored to account for this.

Finally, revert named locations back to internal locations. As part of this process, we've learned that named locations do not behave as expected.

Co-authored-by: Kate Osborn <[email protected]>

---------

Co-authored-by: Kate Osborn <[email protected]>

Author: sjberman

internal/mode/static/handler.go

@@ -24,7 +24,6 @@ import (
 	ngxConfig "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime"
-	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph"
@@ -65,8 +64,6 @@ type eventHandlerConfig struct {
 	serviceResolver resolver.ServiceResolver
 	// generator is the nginx config generator.
 	generator ngxConfig.Generator
-	// policyConfigGenerator is the config generator for NGF policies.
-	policyConfigGenerator policies.ConfigGenerator
 	// k8sClient is a Kubernetes API client
 	k8sClient client.Client
 	// logLevelSetter is used to update the logging level.
@@ -196,7 +193,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
 		return
 	case state.EndpointsOnlyChange:
 		h.version++
-		cfg := dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver, h.cfg.policyConfigGenerator, h.version)
+		cfg := dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver, h.version)
 
 		h.setLatestConfiguration(&cfg)
 
@@ -207,7 +204,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
 		)
 	case state.ClusterStateChange:
 		h.version++
-		cfg := dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver, h.cfg.policyConfigGenerator, h.version)
+		cfg := dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver, h.version)
 
 		h.setLatestConfiguration(&cfg)
 

internal/mode/static/manager.go

@@ -49,12 +49,12 @@ import (
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/metrics/collectors"
 	ngxcfg "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/clientsettings"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/observability"
 	ngxvalidation "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/validation"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file"
 	ngxruntime "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime"
-	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies"
-	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/clientsettings"
-	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/observability"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation"
@@ -232,7 +232,6 @@ func StartManager(cfg config.Config) error {
 		usageSecret:                   usageSecret,
 		gatewayCtlrName:               cfg.GatewayCtlrName,
 		updateGatewayClassStatus:      cfg.UpdateGatewayClassStatus,
-		policyConfigGenerator:         policyManager,
 	})
 
 	objects, objectLists := prepareFirstEventBatchPreparerArgs(
@@ -287,17 +286,15 @@ func StartManager(cfg config.Config) error {
 func createPolicyManager(
 	mustExtractGVK kinds.MustExtractGVK,
 	validator validation.GenericValidator,
-) *policies.Manager {
+) *policies.CompositeValidator {
 	cfgs := []policies.ManagerConfig{
 		{
 			GVK:       mustExtractGVK(&ngfAPI.ClientSettingsPolicy{}),
 			Validator: clientsettings.NewValidator(validator),
-			Generator: clientsettings.Generate,
 		},
 		{
 			GVK:       mustExtractGVK(&ngfAPI.ObservabilityPolicy{}),
 			Validator: observability.NewValidator(validator),
-			Generator: observability.Generate,
 		},
 	}
 

internal/mode/static/nginx/config/generator.go

@@ -3,6 +3,9 @@ package config
 import (
 	"path/filepath"
 
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/clientsettings"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/observability"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane"
 )
@@ -85,7 +88,12 @@ func (g GeneratorImpl) Generate(conf dataplane.Configuration) []file.File {
 		files = append(files, generatePEM(id, pair.Cert, pair.Key))
 	}
 
-	files = append(files, g.generateHTTPConfig(conf)...)
+	policyGenerator := policies.NewCompositeGenerator(
+		clientsettings.NewGenerator(),
+		observability.NewGenerator(conf.Telemetry),
+	)
+
+	files = append(files, g.generateHTTPConfig(conf, policyGenerator)...)
 
 	files = append(files, generateConfigVersion(conf.Version))
 
@@ -127,20 +135,23 @@ func generateCertBundleFileName(id dataplane.CertBundleID) string {
 	return filepath.Join(secretsFolder, string(id)+".crt")
 }
 
-func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) []file.File {
+func (g GeneratorImpl) generateHTTPConfig(
+	conf dataplane.Configuration,
+	generator policies.Generator,
+) []file.File {
 	fileBytes := make(map[string][]byte)
 
-	for _, execute := range g.getExecuteFuncs() {
+	for _, execute := range g.getExecuteFuncs(generator) {
 		results := execute(conf)
 		for _, res := range results {
 			fileBytes[res.dest] = append(fileBytes[res.dest], res.data...)
 		}
 	}
 
 	files := make([]file.File, 0, len(fileBytes))
-	for filepath, bytes := range fileBytes {
+	for fp, bytes := range fileBytes {
 		files = append(files, file.File{
-			Path:    filepath,
+			Path:    fp,
 			Content: bytes,
 			Type:    file.TypeRegular,
 		})
@@ -149,10 +160,10 @@ func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) []file.F
 	return files
 }
 
-func (g GeneratorImpl) getExecuteFuncs() []executeFunc {
+func (g GeneratorImpl) getExecuteFuncs(generator policies.Generator) []executeFunc {
 	return []executeFunc{
 		executeBaseHTTPConfig,
-		executeServers,
+		newExecuteServersFunc(generator),
 		g.executeUpstreams,
 		executeSplitClients,
 		executeMaps,

internal/mode/static/nginx/config/http/config.go

@@ -1,11 +1,13 @@
 package http
 
+const InternalRoutePathPrefix = "/_ngf-internal"
+
 // Server holds all configuration for an HTTP server.
 type Server struct {
 	SSL           *SSL
 	ServerName    string
 	Locations     []Location
-	Includes      []string
+	Includes      []Include
 	Port          int32
 	IsDefaultHTTP bool
 	IsDefaultSSL  bool
@@ -18,18 +20,26 @@ type IPFamily struct {
 	IPv6 bool
 }
 
+type LocationType string
+
+const (
+	InternalLocationType LocationType = "internal"
+	ExternalLocationType LocationType = "external"
+	RedirectLocationType LocationType = "redirect"
+)
+
 // Location holds all configuration for an HTTP location.
 type Location struct {
 	Path            string
 	ProxyPass       string
 	HTTPMatchKey    string
-	HTTPMatchVar    string
+	Type            LocationType
 	ProxySetHeaders []Header
 	ProxySSLVerify  *ProxySSLVerify
 	Return          *Return
 	ResponseHeaders ResponseHeaders
 	Rewrites        []string
-	Includes        []string
+	Includes        []Include
 	GRPC            bool
 }
 
@@ -101,7 +111,7 @@ type Map struct {
 	Parameters []MapParameter
 }
 
-// Parameter defines a Value and Result pair in a Map.
+// MapParameter defines a Value and Result pair in a Map.
 type MapParameter struct {
 	Value  string
 	Result string
@@ -118,3 +128,9 @@ type ServerConfig struct {
 	Servers  []Server
 	IPFamily IPFamily
 }
+
+// Include defines a file that's included via the include directive.
+type Include struct {
+	Name    string
+	Content []byte
+}

internal/mode/static/nginx/config/maps_template.go

@@ -24,4 +24,9 @@ map $http_upgrade $connection_upgrade {
     default upgrade;
     '' close;
 }
+
+## Returns just the path from the original request URI.
+map $request_uri $request_uri_path {
+  "~^(?P<path>[^?]*)(\?.*)?$"  $path;
+}
 `

internal/mode/static/nginx/config/maps_test.go

@@ -86,6 +86,7 @@ func TestExecuteMaps(t *testing.T) {
 		"map ${http_my_set_header} $my_set_header_header_var {":               0,
 		"map $http_host $gw_api_compliant_host {":                             1,
 		"map $http_upgrade $connection_upgrade {":                             1,
+		"map $request_uri $request_uri_path {":                                1,
 	}
 
 	mapResult := executeMaps(conf)

internal/mode/static/nginx/config/policies/clientsettings/generator.go

@@ -0,0 +1,80 @@
+package clientsettings
+
+import (
+	"fmt"
+	"text/template"
+
+	ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies"
+)
+
+var tmpl = template.Must(template.New("client settings policy").Parse(clientSettingsTemplate))
+
+const clientSettingsTemplate = `
+{{- if .Body }}
+	{{- if .Body.MaxSize }}
+client_max_body_size {{ .Body.MaxSize }};
+	{{- end }}
+	{{- if .Body.Timeout }}
+client_body_timeout {{ .Body.Timeout }};
+	{{- end }}
+{{- end }}
+{{- if .KeepAlive }}
+	{{- if .KeepAlive.Requests }}
+keepalive_requests {{ .KeepAlive.Requests }};
+	{{- end }}
+	{{- if .KeepAlive.Time }}
+keepalive_time {{ .KeepAlive.Time }};
+	{{- end }}
+    {{- if .KeepAlive.Timeout }}
+        {{- if and .KeepAlive.Timeout.Server .KeepAlive.Timeout.Header }}
+keepalive_timeout {{ .KeepAlive.Timeout.Server }} {{ .KeepAlive.Timeout.Header }};
+        {{- else if .KeepAlive.Timeout.Server }}
+keepalive_timeout {{ .KeepAlive.Timeout.Server }};
+        {{- end }}
+    {{- end }}
+{{- end }}
+`
+
+// Generator generates nginx configuration based on a clientsettings policy.
+type Generator struct{}
+
+// NewGenerator returns a new instance of Generator.
+func NewGenerator() *Generator {
+	return &Generator{}
+}
+
+// GenerateForServer generates policy configuration for the server block.
+func (g Generator) GenerateForServer(pols []policies.Policy, _ http.Server) policies.GenerateResultFiles {
+	return generate(pols)
+}
+
+// GenerateForLocation generates policy configuration for a normal location block.
+func (g Generator) GenerateForLocation(pols []policies.Policy, _ http.Location) policies.GenerateResultFiles {
+	return generate(pols)
+}
+
+// GenerateForInternalLocation generates policy configuration for an internal location block.
+func (g Generator) GenerateForInternalLocation(pols []policies.Policy) policies.GenerateResultFiles {
+	return generate(pols)
+}
+
+func generate(pols []policies.Policy) policies.GenerateResultFiles {
+	files := make(policies.GenerateResultFiles, 0, len(pols))
+
+	for _, pol := range pols {
+		csp, ok := pol.(*ngfAPI.ClientSettingsPolicy)
+		if !ok {
+			continue
+		}
+
+		files = append(files, policies.File{
+			Name:    fmt.Sprintf("ClientSettingsPolicy_%s_%s.conf", csp.Namespace, csp.Name),
+			Content: helpers.MustExecuteTemplate(tmpl, csp.Spec),
+		})
+	}
+
+	return files
+}

internal/mode/static/policies/clientsettings/generator_test.go renamed to internal/mode/static/nginx/config/policies/clientsettings/generator_test.go

@@ -7,9 +7,9 @@ import (
 
 	ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
-	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies"
-	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/clientsettings"
-	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/clientsettings"
 )
 
 func TestGenerate(t *testing.T) {
@@ -149,25 +149,52 @@ func TestGenerate(t *testing.T) {
 		},
 	}
 
+	g := NewWithT(t)
+
+	checkResults := func(resFiles policies.GenerateResultFiles, expStrings []string) {
+		g.Expect(resFiles).To(HaveLen(1))
+
+		for _, str := range expStrings {
+			g.Expect(string(resFiles[0].Content)).To(ContainSubstring(str))
+		}
+	}
+
 	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			g := NewWithT(t)
+		t.Run(test.name, func(_ *testing.T) {
+			generator := clientsettings.NewGenerator()
+
+			resFiles := generator.GenerateForServer([]policies.Policy{test.policy}, http.Server{})
+			checkResults(resFiles, test.expStrings)
 
-			cfgString := string(clientsettings.Generate(test.policy, nil))
+			resFiles = generator.GenerateForLocation([]policies.Policy{test.policy}, http.Location{})
+			checkResults(resFiles, test.expStrings)
 
-			for _, str := range test.expStrings {
-				g.Expect(cfgString).To(ContainSubstring(str))
-			}
+			resFiles = generator.GenerateForInternalLocation([]policies.Policy{test.policy})
+			checkResults(resFiles, test.expStrings)
 		})
 	}
 }
 
-func TestGeneratePanics(t *testing.T) {
+func TestGenerateNoPolicies(t *testing.T) {
 	g := NewWithT(t)
 
-	generate := func() {
-		clientsettings.Generate(&policiesfakes.FakePolicy{}, nil)
-	}
+	generator := clientsettings.NewGenerator()
+
+	resFiles := generator.GenerateForServer([]policies.Policy{}, http.Server{})
+	g.Expect(resFiles).To(BeEmpty())
+
+	resFiles = generator.GenerateForServer([]policies.Policy{&ngfAPI.ObservabilityPolicy{}}, http.Server{})
+	g.Expect(resFiles).To(BeEmpty())
+
+	resFiles = generator.GenerateForLocation([]policies.Policy{}, http.Location{})
+	g.Expect(resFiles).To(BeEmpty())
+
+	resFiles = generator.GenerateForLocation([]policies.Policy{&ngfAPI.ObservabilityPolicy{}}, http.Location{})
+	g.Expect(resFiles).To(BeEmpty())
+
+	resFiles = generator.GenerateForInternalLocation([]policies.Policy{})
+	g.Expect(resFiles).To(BeEmpty())
 
-	g.Expect(generate).To(Panic())
+	resFiles = generator.GenerateForInternalLocation([]policies.Policy{&ngfAPI.ObservabilityPolicy{}})
+	g.Expect(resFiles).To(BeEmpty())
 }

internal/mode/static/policies/clientsettings/validator.go renamed to internal/mode/static/nginx/config/policies/clientsettings/validator.go

@@ -8,7 +8,7 @@ import (
 	"github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
-	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies"
+	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies"
 	staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions"
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation"
 )