From ae7b69ee9c33c3d27e3b4b37463760215c75262b Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Mon, 3 Jun 2024 14:25:35 -0600 Subject: [PATCH 1/4] Restrict policies to non-duplicate routes 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 --- internal/mode/static/handler.go | 7 +- internal/mode/static/manager.go | 11 +- .../mode/static/nginx/config/generator.go | 25 +- .../mode/static/nginx/config/http/config.go | 25 +- .../mode/static/nginx/config/maps_template.go | 5 + .../mode/static/nginx/config/maps_test.go | 1 + .../policies/clientsettings/generator.go | 80 +++ .../policies/clientsettings/generator_test.go | 51 +- .../policies/clientsettings/validator.go | 2 +- .../policies/clientsettings/validator_test.go | 4 +- .../static/nginx/config/policies/generator.go | 85 +++ .../nginx/config/policies/generator_test.go | 82 +++ .../policies/observability/generator.go | 158 +++++ .../policies/observability/generator_test.go | 298 ++++++++++ .../policies/observability/validator.go | 7 +- .../policies/observability/validator_test.go | 6 +- .../config}/policies/policies_suite_test.go | 0 .../policies/policiesfakes/fake_generator.go | 279 +++++++++ .../policiesfakes/fake_object_kind.go | 0 .../policies/policiesfakes/fake_policy.go | 2 +- .../policies/policiesfakes/fake_validator.go | 2 +- .../{ => nginx/config}/policies/policy.go | 11 - .../config/policies/validator.go} | 36 +- .../config/policies/validator_test.go} | 29 +- internal/mode/static/nginx/config/servers.go | 338 +++++++---- .../static/nginx/config/servers_template.go | 16 +- .../mode/static/nginx/config/servers_test.go | 545 ++++++++++-------- .../config/validation/http_njs_match_test.go | 2 +- .../static/nginx/modules/src/httpmatches.js | 11 +- .../nginx/modules/test/httpmatches.test.js | 4 +- .../policies/clientsettings/generator.go | 44 -- .../policies/observability/generator.go | 73 --- .../policies/observability/generator_test.go | 214 ------- .../policiesfakes/fake_config_generator.go | 113 ---- .../mode/static/state/change_processor.go | 2 +- .../static/state/change_processor_test.go | 10 +- .../static/state/conditions/conditions.go | 15 + .../static/state/dataplane/configuration.go | 78 +-- .../state/dataplane/configuration_test.go | 170 ++---- internal/mode/static/state/dataplane/types.go | 21 +- internal/mode/static/state/graph/graph.go | 6 +- .../mode/static/state/graph/graph_test.go | 14 +- internal/mode/static/state/graph/httproute.go | 7 + .../mode/static/state/graph/httproute_test.go | 11 + internal/mode/static/state/graph/policies.go | 70 ++- .../mode/static/state/graph/policies_test.go | 306 +++++++++- .../mode/static/state/graph/route_common.go | 3 + internal/mode/static/state/store.go | 2 +- .../validationfakes/fake_policy_validator.go | 2 +- .../mode/static/state/validation/validator.go | 2 +- internal/mode/static/status/status_setters.go | 2 +- .../mode/static/status/status_setters_test.go | 2 +- site/content/how-to/monitoring/tracing.md | 4 - .../traffic-management/client-settings.md | 4 - site/content/overview/custom-policies.md | 3 +- tests/Makefile | 2 +- .../manifests/clientsettings/cafe-routes.yaml | 1 + tests/suite/manifests/hello-world/routes.yaml | 1 + 58 files changed, 2145 insertions(+), 1159 deletions(-) create mode 100644 internal/mode/static/nginx/config/policies/clientsettings/generator.go rename internal/mode/static/{ => nginx/config}/policies/clientsettings/generator_test.go (70%) rename internal/mode/static/{ => nginx/config}/policies/clientsettings/validator.go (99%) rename internal/mode/static/{ => nginx/config}/policies/clientsettings/validator_test.go (99%) create mode 100644 internal/mode/static/nginx/config/policies/generator.go create mode 100644 internal/mode/static/nginx/config/policies/generator_test.go create mode 100644 internal/mode/static/nginx/config/policies/observability/generator.go create mode 100644 internal/mode/static/nginx/config/policies/observability/generator_test.go rename internal/mode/static/{ => nginx/config}/policies/observability/validator.go (96%) rename internal/mode/static/{ => nginx/config}/policies/observability/validator_test.go (98%) rename internal/mode/static/{ => nginx/config}/policies/policies_suite_test.go (100%) create mode 100644 internal/mode/static/nginx/config/policies/policiesfakes/fake_generator.go rename internal/mode/static/{ => nginx/config}/policies/policiesfakes/fake_object_kind.go (100%) rename internal/mode/static/{ => nginx/config}/policies/policiesfakes/fake_policy.go (99%) rename internal/mode/static/{ => nginx/config}/policies/policiesfakes/fake_validator.go (99%) rename internal/mode/static/{ => nginx/config}/policies/policy.go (80%) rename internal/mode/static/{policies/manager.go => nginx/config/policies/validator.go} (62%) rename internal/mode/static/{policies/manager_test.go => nginx/config/policies/validator_test.go} (74%) delete mode 100644 internal/mode/static/policies/clientsettings/generator.go delete mode 100644 internal/mode/static/policies/observability/generator.go delete mode 100644 internal/mode/static/policies/observability/generator_test.go delete mode 100644 internal/mode/static/policies/policiesfakes/fake_config_generator.go diff --git a/internal/mode/static/handler.go b/internal/mode/static/handler.go index d90edc337..3c4d642bf 100644 --- a/internal/mode/static/handler.go +++ b/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) diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index 30b71f702..8a16a99eb 100644 --- a/internal/mode/static/manager.go +++ b/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, }, } diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index a0509194e..eac79be34 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/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,10 +135,13 @@ 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...) @@ -138,9 +149,9 @@ func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) []file.F } 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, diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index 9326ebb43..4ebab5c40 100644 --- a/internal/mode/static/nginx/config/http/config.go +++ b/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,19 +20,28 @@ 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 + Redirects bool } // Header defines an HTTP header to be passed to the proxied server. @@ -101,7 +112,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 +129,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 +} diff --git a/internal/mode/static/nginx/config/maps_template.go b/internal/mode/static/nginx/config/maps_template.go index 4b4407a1a..d11c3f887 100644 --- a/internal/mode/static/nginx/config/maps_template.go +++ b/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; +} ` diff --git a/internal/mode/static/nginx/config/maps_test.go b/internal/mode/static/nginx/config/maps_test.go index e903c1715..b9d038851 100644 --- a/internal/mode/static/nginx/config/maps_test.go +++ b/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) diff --git a/internal/mode/static/nginx/config/policies/clientsettings/generator.go b/internal/mode/static/nginx/config/policies/clientsettings/generator.go new file mode 100644 index 000000000..3eb4ea83b --- /dev/null +++ b/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) +} + +// GenerateForServer generates policy configuration for a normal location block. +func (g Generator) GenerateForLocation(pols []policies.Policy, _ http.Location) policies.GenerateResultFiles { + return generate(pols) +} + +// GenerateForServer generates policy configuration for a normal 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 +} diff --git a/internal/mode/static/policies/clientsettings/generator_test.go b/internal/mode/static/nginx/config/policies/clientsettings/generator_test.go similarity index 70% rename from internal/mode/static/policies/clientsettings/generator_test.go rename to internal/mode/static/nginx/config/policies/clientsettings/generator_test.go index 19b3b1de3..1fafea97d 100644 --- a/internal/mode/static/policies/clientsettings/generator_test.go +++ b/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) { @@ -153,21 +153,52 @@ func TestGenerate(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - cfgString := string(clientsettings.Generate(test.policy, nil)) + generator := clientsettings.NewGenerator() + + resFiles := generator.GenerateForServer([]policies.Policy{test.policy}, http.Server{}) + g.Expect(resFiles).To(HaveLen(1)) + + for _, str := range test.expStrings { + g.Expect(string(resFiles[0].Content)).To(ContainSubstring(str)) + } + + resFiles = generator.GenerateForLocation([]policies.Policy{test.policy}, http.Location{}) + g.Expect(resFiles).To(HaveLen(1)) for _, str := range test.expStrings { - g.Expect(cfgString).To(ContainSubstring(str)) + g.Expect(string(resFiles[0].Content)).To(ContainSubstring(str)) + } + + resFiles = generator.GenerateForInternalLocation([]policies.Policy{test.policy}) + g.Expect(resFiles).To(HaveLen(1)) + + for _, str := range test.expStrings { + g.Expect(string(resFiles[0].Content)).To(ContainSubstring(str)) } }) } } -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()) } diff --git a/internal/mode/static/policies/clientsettings/validator.go b/internal/mode/static/nginx/config/policies/clientsettings/validator.go similarity index 99% rename from internal/mode/static/policies/clientsettings/validator.go rename to internal/mode/static/nginx/config/policies/clientsettings/validator.go index bcc9f6ebf..60ce55c7a 100644 --- a/internal/mode/static/policies/clientsettings/validator.go +++ b/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" ) diff --git a/internal/mode/static/policies/clientsettings/validator_test.go b/internal/mode/static/nginx/config/policies/clientsettings/validator_test.go similarity index 99% rename from internal/mode/static/policies/clientsettings/validator_test.go rename to internal/mode/static/nginx/config/policies/clientsettings/validator_test.go index 8b9a7fcdd..9cb5884b6 100644 --- a/internal/mode/static/policies/clientsettings/validator_test.go +++ b/internal/mode/static/nginx/config/policies/clientsettings/validator_test.go @@ -12,9 +12,9 @@ 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/nginx/config/policies/clientsettings" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/validation" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/clientsettings" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" ) diff --git a/internal/mode/static/nginx/config/policies/generator.go b/internal/mode/static/nginx/config/policies/generator.go new file mode 100644 index 000000000..021b7e517 --- /dev/null +++ b/internal/mode/static/nginx/config/policies/generator.go @@ -0,0 +1,85 @@ +package policies + +import ( + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" +) + +// Generator defines an interface for a policy to implement its appropriate generator functions. +// +//counterfeiter:generate . Generator +type Generator interface { + // GenerateForServer generates policy configuration for the server block. + GenerateForServer(policies []Policy, server http.Server) GenerateResultFiles + // GenerateForServer generates policy configuration for a normal location block. + GenerateForLocation(policies []Policy, location http.Location) GenerateResultFiles + // GenerateForInternalLocation generates policy configuration for an internal location block. + GenerateForInternalLocation(policies []Policy) GenerateResultFiles +} + +// GenerateResultFiles is a list of files generated for inclusion by policy generators. +type GenerateResultFiles []File + +// File is the contents of the generated file. +type File struct { + Name string + Content []byte +} + +// CompositeGenerator contains all policy generators. +type CompositeGenerator struct { + generators []Generator +} + +// NewCompositeGenerator returns a new instance of a CompositeGenerator. +func NewCompositeGenerator(generators ...Generator) *CompositeGenerator { + return &CompositeGenerator{generators: generators} +} + +// GenerateForServer calls all policy generators for the server block. +func (g *CompositeGenerator) GenerateForServer(policies []Policy, server http.Server) GenerateResultFiles { + var compositeResult GenerateResultFiles + + for _, generator := range g.generators { + compositeResult = append(compositeResult, generator.GenerateForServer(policies, server)...) + } + + return compositeResult +} + +// GenerateForServer calls all policy generators for a normal location block. +func (g *CompositeGenerator) GenerateForLocation(policies []Policy, location http.Location) GenerateResultFiles { + var compositeResult GenerateResultFiles + + for _, generator := range g.generators { + compositeResult = append(compositeResult, generator.GenerateForLocation(policies, location)...) + } + + return compositeResult +} + +// GenerateForInternalLocation calls all policy generators for an internal location block. +func (g *CompositeGenerator) GenerateForInternalLocation(policies []Policy) GenerateResultFiles { + var compositeResult GenerateResultFiles + + for _, generator := range g.generators { + compositeResult = append(compositeResult, generator.GenerateForInternalLocation(policies)...) + } + + return compositeResult +} + +// UnimplementedGenerator can be inherited by any policy generator that may not need to implement all of +// possible generations, in order to satisfy the Generator interface. +type UnimplementedGenerator struct{} + +func (u UnimplementedGenerator) GenerateForServer(_ []Policy, _ http.Server) GenerateResultFiles { + return nil +} + +func (u UnimplementedGenerator) GenerateForLocation(_ []Policy, _ http.Location) GenerateResultFiles { + return nil +} + +func (u UnimplementedGenerator) GenerateForInternalLocation(_ []Policy) GenerateResultFiles { + return nil +} diff --git a/internal/mode/static/nginx/config/policies/generator_test.go b/internal/mode/static/nginx/config/policies/generator_test.go new file mode 100644 index 000000000..83a746d99 --- /dev/null +++ b/internal/mode/static/nginx/config/policies/generator_test.go @@ -0,0 +1,82 @@ +package policies_test + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "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/policiesfakes" +) + +var _ = Describe("Policy Generator", func() { + Context("Composite Generator", func() { + fakeGen1 := &policiesfakes.FakeGenerator{} + fakeGen2 := &policiesfakes.FakeGenerator{} + + fakeGen1.GenerateForServerReturns(policies.GenerateResultFiles{ + {Name: "gen1Server", Content: []byte("gen1Server-content")}, + }) + fakeGen1.GenerateForLocationReturns(policies.GenerateResultFiles{ + {Name: "gen1Location", Content: []byte("gen1Location-content")}, + }) + fakeGen1.GenerateForInternalLocationReturns(policies.GenerateResultFiles{ + {Name: "gen1IntLocation", Content: []byte("gen1IntLocation-content")}, + }) + + fakeGen2.GenerateForServerReturns(policies.GenerateResultFiles{ + {Name: "gen2Server", Content: []byte("gen2Server-content")}, + }) + fakeGen2.GenerateForLocationReturns(policies.GenerateResultFiles{ + {Name: "gen2Location", Content: []byte("gen2Location-content")}, + }) + fakeGen2.GenerateForInternalLocationReturns(policies.GenerateResultFiles{ + {Name: "gen2IntLocation", Content: []byte("gen2IntLocation-content")}, + }) + + generator := policies.NewCompositeGenerator(fakeGen1, fakeGen2) + + It("returns proper server content", func() { + expFiles := policies.GenerateResultFiles{ + {Name: "gen1Server", Content: []byte("gen1Server-content")}, + {Name: "gen2Server", Content: []byte("gen2Server-content")}, + } + + Expect(generator.GenerateForServer(nil, http.Server{})).To(BeEquivalentTo(expFiles)) + }) + + It("returns proper location content", func() { + expFiles := policies.GenerateResultFiles{ + {Name: "gen1Location", Content: []byte("gen1Location-content")}, + {Name: "gen2Location", Content: []byte("gen2Location-content")}, + } + + Expect(generator.GenerateForLocation(nil, http.Location{})).To(BeEquivalentTo(expFiles)) + }) + + It("returns proper internal location content", func() { + expFiles := policies.GenerateResultFiles{ + {Name: "gen1IntLocation", Content: []byte("gen1IntLocation-content")}, + {Name: "gen2IntLocation", Content: []byte("gen2IntLocation-content")}, + } + + Expect(generator.GenerateForInternalLocation(nil)).To(BeEquivalentTo(expFiles)) + }) + }) + + Context("Unimplemented Generator", func() { + generator := policies.UnimplementedGenerator{} + + It("returns nil for GenerateForServer", func() { + Expect(generator.GenerateForServer(nil, http.Server{})).To(BeNil()) + }) + + It("returns nil for GenerateForLocation", func() { + Expect(generator.GenerateForLocation(nil, http.Location{})).To(BeNil()) + }) + + It("returns nil for GenerateForInternalLocation", func() { + Expect(generator.GenerateForInternalLocation(nil)).To(BeNil()) + }) + }) +}) diff --git a/internal/mode/static/nginx/config/policies/observability/generator.go b/internal/mode/static/nginx/config/policies/observability/generator.go new file mode 100644 index 000000000..17ffe311c --- /dev/null +++ b/internal/mode/static/nginx/config/policies/observability/generator.go @@ -0,0 +1,158 @@ +package observability + +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" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" +) + +var ( + tmpl = template.Must(template.New("observability policy").Parse(observabilityTemplate)) + tmplInternal = template.Must(template.New("observability policy internal").Parse(internalTemplate)) + tmplExtRedirect = template.Must(template.New("observability policy ext redirect").Parse(externalRedirectTemplate)) +) + +const observabilityTemplate = ` +{{- if .Tracing }} +otel_trace {{ .Strategy }}; + {{- if .Tracing.Context }} +otel_trace_context {{ .Tracing.Context }}; + {{- end }} + {{- if .Tracing.SpanName }} +otel_span_name "{{ .Tracing.SpanName }}"; + {{- end }} + {{- range $attr := .Tracing.SpanAttributes }} +otel_span_attr "{{ $attr.Key }}" "{{ $attr.Value }}"; + {{- end }} + {{- range $attr := .GlobalSpanAttributes }} +otel_span_attr "{{ $attr.Key }}" "{{ $attr.Value }}"; + {{- end }} +{{- end }} +` + +const internalTemplate = ` +{{- if .Tracing }} + {{- if .Tracing.SpanName }} +otel_span_name "{{ .Tracing.SpanName }}"; + {{- else }} +otel_span_name $request_uri_path; + {{- end }} + {{- range $attr := .Tracing.SpanAttributes }} +otel_span_attr "{{ $attr.Key }}" "{{ $attr.Value }}"; + {{- end }} + {{- range $attr := .GlobalSpanAttributes }} +otel_span_attr "{{ $attr.Key }}" "{{ $attr.Value }}"; + {{- end }} +{{- end }} +` + +const externalRedirectTemplate = ` +{{- if .Tracing }} +otel_trace {{ .Strategy }}; + {{- if .Tracing.Context }} +otel_trace_context {{ .Tracing.Context }}; + {{- end }} +{{- end }} +` + +// Generator generates nginx configuration based on an observability policy. +type Generator struct { + policies.UnimplementedGenerator + + telemetryConf dataplane.Telemetry +} + +// NewGenerator returns a new instance of Generator. +func NewGenerator(telemetry dataplane.Telemetry) *Generator { + return &Generator{telemetryConf: telemetry} +} + +// GenerateForServer generates policy configuration for a normal location block. +func (g Generator) GenerateForLocation(pols []policies.Policy, location http.Location) policies.GenerateResultFiles { + buildTemplate := func( + tmplate *template.Template, + fileSuffix string, + includeGlobalAttrs bool, + ) policies.GenerateResultFiles { + for _, pol := range pols { + obs, ok := pol.(*ngfAPI.ObservabilityPolicy) + if !ok { + continue + } + + fields := map[string]interface{}{ + "Tracing": obs.Spec.Tracing, + "Strategy": getStrategy(obs), + } + if includeGlobalAttrs { + fields["GlobalSpanAttributes"] = g.telemetryConf.SpanAttributes + } + + return policies.GenerateResultFiles{ + { + Name: fmt.Sprintf("ObservabilityPolicy_%s_%s_%s.conf", obs.Namespace, obs.Name, fileSuffix), + Content: helpers.MustExecuteTemplate(tmplate, fields), + }, + } + } + return nil + } + + if location.Type == http.ExternalLocationType { + return buildTemplate(tmpl, "ext", true) + } + + return buildTemplate(tmplExtRedirect, "redirect", false) +} + +// GenerateForInternalLocation generates policy configuration for an internal location block. +func (g Generator) GenerateForInternalLocation(pols []policies.Policy) policies.GenerateResultFiles { + for _, pol := range pols { + obs, ok := pol.(*ngfAPI.ObservabilityPolicy) + if !ok { + continue + } + + fields := map[string]interface{}{ + "Tracing": obs.Spec.Tracing, + "GlobalSpanAttributes": g.telemetryConf.SpanAttributes, + } + + return policies.GenerateResultFiles{ + { + Name: fmt.Sprintf("ObservabilityPolicy_%s_%s_int.conf", obs.Namespace, obs.Name), + Content: helpers.MustExecuteTemplate(tmplInternal, fields), + }, + } + } + + return nil +} + +func getStrategy(obs *ngfAPI.ObservabilityPolicy) string { + var strategy string + if obs.Spec.Tracing != nil { + switch obs.Spec.Tracing.Strategy { + case ngfAPI.TraceStrategyParent: + strategy = "$otel_parent_sampled" + case ngfAPI.TraceStrategyRatio: + strategy = "on" + if obs.Spec.Tracing.Ratio != nil { + if *obs.Spec.Tracing.Ratio > 0 { + strategy = dataplane.CreateRatioVarName(*obs.Spec.Tracing.Ratio) + } else { + strategy = "off" + } + } + default: + strategy = "off" + } + } + + return strategy +} diff --git a/internal/mode/static/nginx/config/policies/observability/generator_test.go b/internal/mode/static/nginx/config/policies/observability/generator_test.go new file mode 100644 index 000000000..4faa880c0 --- /dev/null +++ b/internal/mode/static/nginx/config/policies/observability/generator_test.go @@ -0,0 +1,298 @@ +package observability_test + +import ( + "testing" + + . "github.com/onsi/gomega" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + 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" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/observability" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" +) + +func TestGenerate(t *testing.T) { + ratio := helpers.GetPointer[int32](25) + zeroRatio := helpers.GetPointer[int32](0) + context := helpers.GetPointer[ngfAPI.TraceContext](ngfAPI.TraceContextExtract) + spanName := helpers.GetPointer("my-span") + + tests := []struct { + name string + expExternalStrings []string + expRedirectStrings []string + expInternalStrings []string + policy policies.Policy + telemetryConf dataplane.Telemetry + }{ + { + name: "strategy set to default ratio", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyRatio, + }, + }, + }, + expExternalStrings: []string{ + "otel_trace on;", + }, + expRedirectStrings: []string{ + "otel_trace on;", + }, + expInternalStrings: []string{ + "otel_span_name $request_uri_path;", + }, + }, + { + name: "strategy set to custom ratio", + policy: &ngfAPI.ObservabilityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-policy", + Namespace: "test-namespace", + }, + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyRatio, + Ratio: ratio, + }, + }, + }, + expExternalStrings: []string{ + "otel_trace $otel_ratio_25;", + }, + expRedirectStrings: []string{ + "otel_trace $otel_ratio_25;", + }, + expInternalStrings: []string{ + "otel_span_name $request_uri_path;", + }, + }, + { + name: "strategy set to zero ratio", + policy: &ngfAPI.ObservabilityPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-policy", + Namespace: "test-namespace", + }, + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyRatio, + Ratio: zeroRatio, + }, + }, + }, + expExternalStrings: []string{ + "otel_trace off;", + }, + expRedirectStrings: []string{ + "otel_trace off;", + }, + expInternalStrings: []string{ + "otel_span_name $request_uri_path;", + }, + }, + { + name: "strategy set to parent", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyParent, + }, + }, + }, + expExternalStrings: []string{ + "otel_trace $otel_parent_sampled;", + }, + expRedirectStrings: []string{ + "otel_trace $otel_parent_sampled;", + }, + expInternalStrings: []string{ + "otel_span_name $request_uri_path;", + }, + }, + { + name: "context is set", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Context: context, + }, + }, + }, + expExternalStrings: []string{ + "otel_trace off;", + "otel_trace_context extract;", + }, + expRedirectStrings: []string{ + "otel_trace off;", + "otel_trace_context extract;", + }, + expInternalStrings: []string{ + "otel_span_name $request_uri_path;", + }, + }, + { + name: "spanName is set", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + SpanName: spanName, + }, + }, + }, + expExternalStrings: []string{ + "otel_trace off;", + "otel_span_name \"my-span\";", + }, + expRedirectStrings: []string{ + "otel_trace off;", + }, + expInternalStrings: []string{ + "otel_span_name \"my-span\";", + }, + }, + { + name: "span attributes set", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + SpanAttributes: []ngfAPI.SpanAttribute{ + {Key: "test-key", Value: "test-value"}, + }, + }, + }, + }, + expExternalStrings: []string{ + "otel_trace off;", + "otel_span_attr \"test-key\" \"test-value\";", + }, + expRedirectStrings: []string{ + "otel_trace off;", + }, + expInternalStrings: []string{ + "otel_span_name $request_uri_path;", + "otel_span_attr \"test-key\" \"test-value\";", + }, + }, + { + name: "global span attributes set", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{}, + }, + }, + telemetryConf: dataplane.Telemetry{ + SpanAttributes: []dataplane.SpanAttribute{ + {Key: "test-global-key", Value: "test-global-value"}, + }, + }, + expExternalStrings: []string{ + "otel_trace off;", + "otel_span_attr \"test-global-key\" \"test-global-value\";", + }, + expRedirectStrings: []string{ + "otel_trace off;", + }, + expInternalStrings: []string{ + "otel_span_name $request_uri_path;", + "otel_span_attr \"test-global-key\" \"test-global-value\";", + }, + }, + { + name: "all fields populated", + policy: &ngfAPI.ObservabilityPolicy{ + Spec: ngfAPI.ObservabilityPolicySpec{ + Tracing: &ngfAPI.Tracing{ + Strategy: ngfAPI.TraceStrategyRatio, + Context: context, + SpanName: spanName, + SpanAttributes: []ngfAPI.SpanAttribute{ + {Key: "test-key", Value: "test-value"}, + }, + }, + }, + }, + telemetryConf: dataplane.Telemetry{ + SpanAttributes: []dataplane.SpanAttribute{ + {Key: "test-global-key", Value: "test-global-value"}, + }, + }, + expExternalStrings: []string{ + "otel_trace on;", + "otel_trace_context extract;", + "otel_span_name \"my-span\";", + "otel_span_attr \"test-key\" \"test-value\";", + "otel_span_attr \"test-global-key\" \"test-global-value\";", + }, + expRedirectStrings: []string{ + "otel_trace on;", + "otel_trace_context extract;", + }, + expInternalStrings: []string{ + "otel_span_name \"my-span\";", + "otel_span_attr \"test-key\" \"test-value\";", + "otel_span_attr \"test-global-key\" \"test-global-value\";", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + generator := observability.NewGenerator(test.telemetryConf) + + for _, locType := range []http.LocationType{ + http.ExternalLocationType, http.RedirectLocationType, http.InternalLocationType, + } { + var expStrings []string + var resFiles policies.GenerateResultFiles + switch locType { + case http.ExternalLocationType: + expStrings = test.expExternalStrings + resFiles = generator.GenerateForLocation([]policies.Policy{test.policy}, http.Location{Type: locType}) + case http.RedirectLocationType: + expStrings = test.expRedirectStrings + resFiles = generator.GenerateForLocation([]policies.Policy{test.policy}, http.Location{Type: locType}) + case http.InternalLocationType: + expStrings = test.expInternalStrings + resFiles = generator.GenerateForInternalLocation([]policies.Policy{test.policy}) + } + + g.Expect(resFiles).To(HaveLen(1)) + + content := string(resFiles[0].Content) + + if len(expStrings) == 0 { + g.Expect(content).To(Equal("")) + } + + for _, str := range expStrings { + g.Expect(content).To(ContainSubstring(str)) + } + } + }) + } +} + +func TestGenerateNoPolicies(t *testing.T) { + g := NewWithT(t) + + generator := observability.NewGenerator(dataplane.Telemetry{}) + + resFiles := generator.GenerateForLocation([]policies.Policy{}, http.Location{}) + g.Expect(resFiles).To(BeEmpty()) + + resFiles = generator.GenerateForLocation([]policies.Policy{&ngfAPI.ClientSettingsPolicy{}}, http.Location{}) + g.Expect(resFiles).To(BeEmpty()) + + resFiles = generator.GenerateForInternalLocation([]policies.Policy{}) + g.Expect(resFiles).To(BeEmpty()) + + resFiles = generator.GenerateForInternalLocation([]policies.Policy{&ngfAPI.ClientSettingsPolicy{}}) + g.Expect(resFiles).To(BeEmpty()) +} diff --git a/internal/mode/static/policies/observability/validator.go b/internal/mode/static/nginx/config/policies/observability/validator.go similarity index 96% rename from internal/mode/static/policies/observability/validator.go rename to internal/mode/static/nginx/config/policies/observability/validator.go index 78d4e5d9d..3fa7ea228 100644 --- a/internal/mode/static/policies/observability/validator.go +++ b/internal/mode/static/nginx/config/policies/observability/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" ) @@ -112,7 +112,10 @@ func (v *Validator) validateSettings(spec ngfAPI.ObservabilityPolicySpec) error if spec.Tracing.SpanName != nil { if err := v.genericValidator.ValidateEscapedStringNoVarExpansion(*spec.Tracing.SpanName); err != nil { - allErrs = append(allErrs, field.Invalid(tracePath.Child("spanName"), *spec.Tracing.SpanName, err.Error())) + allErrs = append( + allErrs, + field.Invalid(tracePath.Child("spanName"), *spec.Tracing.SpanName, err.Error()), + ) } } diff --git a/internal/mode/static/policies/observability/validator_test.go b/internal/mode/static/nginx/config/policies/observability/validator_test.go similarity index 98% rename from internal/mode/static/policies/observability/validator_test.go rename to internal/mode/static/nginx/config/policies/observability/validator_test.go index 1e726b9fd..9f8976e4c 100644 --- a/internal/mode/static/policies/observability/validator_test.go +++ b/internal/mode/static/nginx/config/policies/observability/validator_test.go @@ -12,10 +12,10 @@ 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/nginx/config/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/observability" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/validation" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/observability" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" ) diff --git a/internal/mode/static/policies/policies_suite_test.go b/internal/mode/static/nginx/config/policies/policies_suite_test.go similarity index 100% rename from internal/mode/static/policies/policies_suite_test.go rename to internal/mode/static/nginx/config/policies/policies_suite_test.go diff --git a/internal/mode/static/nginx/config/policies/policiesfakes/fake_generator.go b/internal/mode/static/nginx/config/policies/policiesfakes/fake_generator.go new file mode 100644 index 000000000..f7cdb6537 --- /dev/null +++ b/internal/mode/static/nginx/config/policies/policiesfakes/fake_generator.go @@ -0,0 +1,279 @@ +// Code generated by counterfeiter. DO NOT EDIT. +package policiesfakes + +import ( + "sync" + + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" +) + +type FakeGenerator struct { + GenerateForInternalLocationStub func([]policies.Policy) policies.GenerateResultFiles + generateForInternalLocationMutex sync.RWMutex + generateForInternalLocationArgsForCall []struct { + arg1 []policies.Policy + } + generateForInternalLocationReturns struct { + result1 policies.GenerateResultFiles + } + generateForInternalLocationReturnsOnCall map[int]struct { + result1 policies.GenerateResultFiles + } + GenerateForLocationStub func([]policies.Policy, http.Location) policies.GenerateResultFiles + generateForLocationMutex sync.RWMutex + generateForLocationArgsForCall []struct { + arg1 []policies.Policy + arg2 http.Location + } + generateForLocationReturns struct { + result1 policies.GenerateResultFiles + } + generateForLocationReturnsOnCall map[int]struct { + result1 policies.GenerateResultFiles + } + GenerateForServerStub func([]policies.Policy, http.Server) policies.GenerateResultFiles + generateForServerMutex sync.RWMutex + generateForServerArgsForCall []struct { + arg1 []policies.Policy + arg2 http.Server + } + generateForServerReturns struct { + result1 policies.GenerateResultFiles + } + generateForServerReturnsOnCall map[int]struct { + result1 policies.GenerateResultFiles + } + invocations map[string][][]interface{} + invocationsMutex sync.RWMutex +} + +func (fake *FakeGenerator) GenerateForInternalLocation(arg1 []policies.Policy) policies.GenerateResultFiles { + var arg1Copy []policies.Policy + if arg1 != nil { + arg1Copy = make([]policies.Policy, len(arg1)) + copy(arg1Copy, arg1) + } + fake.generateForInternalLocationMutex.Lock() + ret, specificReturn := fake.generateForInternalLocationReturnsOnCall[len(fake.generateForInternalLocationArgsForCall)] + fake.generateForInternalLocationArgsForCall = append(fake.generateForInternalLocationArgsForCall, struct { + arg1 []policies.Policy + }{arg1Copy}) + stub := fake.GenerateForInternalLocationStub + fakeReturns := fake.generateForInternalLocationReturns + fake.recordInvocation("GenerateForInternalLocation", []interface{}{arg1Copy}) + fake.generateForInternalLocationMutex.Unlock() + if stub != nil { + return stub(arg1) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeGenerator) GenerateForInternalLocationCallCount() int { + fake.generateForInternalLocationMutex.RLock() + defer fake.generateForInternalLocationMutex.RUnlock() + return len(fake.generateForInternalLocationArgsForCall) +} + +func (fake *FakeGenerator) GenerateForInternalLocationCalls(stub func([]policies.Policy) policies.GenerateResultFiles) { + fake.generateForInternalLocationMutex.Lock() + defer fake.generateForInternalLocationMutex.Unlock() + fake.GenerateForInternalLocationStub = stub +} + +func (fake *FakeGenerator) GenerateForInternalLocationArgsForCall(i int) []policies.Policy { + fake.generateForInternalLocationMutex.RLock() + defer fake.generateForInternalLocationMutex.RUnlock() + argsForCall := fake.generateForInternalLocationArgsForCall[i] + return argsForCall.arg1 +} + +func (fake *FakeGenerator) GenerateForInternalLocationReturns(result1 policies.GenerateResultFiles) { + fake.generateForInternalLocationMutex.Lock() + defer fake.generateForInternalLocationMutex.Unlock() + fake.GenerateForInternalLocationStub = nil + fake.generateForInternalLocationReturns = struct { + result1 policies.GenerateResultFiles + }{result1} +} + +func (fake *FakeGenerator) GenerateForInternalLocationReturnsOnCall(i int, result1 policies.GenerateResultFiles) { + fake.generateForInternalLocationMutex.Lock() + defer fake.generateForInternalLocationMutex.Unlock() + fake.GenerateForInternalLocationStub = nil + if fake.generateForInternalLocationReturnsOnCall == nil { + fake.generateForInternalLocationReturnsOnCall = make(map[int]struct { + result1 policies.GenerateResultFiles + }) + } + fake.generateForInternalLocationReturnsOnCall[i] = struct { + result1 policies.GenerateResultFiles + }{result1} +} + +func (fake *FakeGenerator) GenerateForLocation(arg1 []policies.Policy, arg2 http.Location) policies.GenerateResultFiles { + var arg1Copy []policies.Policy + if arg1 != nil { + arg1Copy = make([]policies.Policy, len(arg1)) + copy(arg1Copy, arg1) + } + fake.generateForLocationMutex.Lock() + ret, specificReturn := fake.generateForLocationReturnsOnCall[len(fake.generateForLocationArgsForCall)] + fake.generateForLocationArgsForCall = append(fake.generateForLocationArgsForCall, struct { + arg1 []policies.Policy + arg2 http.Location + }{arg1Copy, arg2}) + stub := fake.GenerateForLocationStub + fakeReturns := fake.generateForLocationReturns + fake.recordInvocation("GenerateForLocation", []interface{}{arg1Copy, arg2}) + fake.generateForLocationMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeGenerator) GenerateForLocationCallCount() int { + fake.generateForLocationMutex.RLock() + defer fake.generateForLocationMutex.RUnlock() + return len(fake.generateForLocationArgsForCall) +} + +func (fake *FakeGenerator) GenerateForLocationCalls(stub func([]policies.Policy, http.Location) policies.GenerateResultFiles) { + fake.generateForLocationMutex.Lock() + defer fake.generateForLocationMutex.Unlock() + fake.GenerateForLocationStub = stub +} + +func (fake *FakeGenerator) GenerateForLocationArgsForCall(i int) ([]policies.Policy, http.Location) { + fake.generateForLocationMutex.RLock() + defer fake.generateForLocationMutex.RUnlock() + argsForCall := fake.generateForLocationArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeGenerator) GenerateForLocationReturns(result1 policies.GenerateResultFiles) { + fake.generateForLocationMutex.Lock() + defer fake.generateForLocationMutex.Unlock() + fake.GenerateForLocationStub = nil + fake.generateForLocationReturns = struct { + result1 policies.GenerateResultFiles + }{result1} +} + +func (fake *FakeGenerator) GenerateForLocationReturnsOnCall(i int, result1 policies.GenerateResultFiles) { + fake.generateForLocationMutex.Lock() + defer fake.generateForLocationMutex.Unlock() + fake.GenerateForLocationStub = nil + if fake.generateForLocationReturnsOnCall == nil { + fake.generateForLocationReturnsOnCall = make(map[int]struct { + result1 policies.GenerateResultFiles + }) + } + fake.generateForLocationReturnsOnCall[i] = struct { + result1 policies.GenerateResultFiles + }{result1} +} + +func (fake *FakeGenerator) GenerateForServer(arg1 []policies.Policy, arg2 http.Server) policies.GenerateResultFiles { + var arg1Copy []policies.Policy + if arg1 != nil { + arg1Copy = make([]policies.Policy, len(arg1)) + copy(arg1Copy, arg1) + } + fake.generateForServerMutex.Lock() + ret, specificReturn := fake.generateForServerReturnsOnCall[len(fake.generateForServerArgsForCall)] + fake.generateForServerArgsForCall = append(fake.generateForServerArgsForCall, struct { + arg1 []policies.Policy + arg2 http.Server + }{arg1Copy, arg2}) + stub := fake.GenerateForServerStub + fakeReturns := fake.generateForServerReturns + fake.recordInvocation("GenerateForServer", []interface{}{arg1Copy, arg2}) + fake.generateForServerMutex.Unlock() + if stub != nil { + return stub(arg1, arg2) + } + if specificReturn { + return ret.result1 + } + return fakeReturns.result1 +} + +func (fake *FakeGenerator) GenerateForServerCallCount() int { + fake.generateForServerMutex.RLock() + defer fake.generateForServerMutex.RUnlock() + return len(fake.generateForServerArgsForCall) +} + +func (fake *FakeGenerator) GenerateForServerCalls(stub func([]policies.Policy, http.Server) policies.GenerateResultFiles) { + fake.generateForServerMutex.Lock() + defer fake.generateForServerMutex.Unlock() + fake.GenerateForServerStub = stub +} + +func (fake *FakeGenerator) GenerateForServerArgsForCall(i int) ([]policies.Policy, http.Server) { + fake.generateForServerMutex.RLock() + defer fake.generateForServerMutex.RUnlock() + argsForCall := fake.generateForServerArgsForCall[i] + return argsForCall.arg1, argsForCall.arg2 +} + +func (fake *FakeGenerator) GenerateForServerReturns(result1 policies.GenerateResultFiles) { + fake.generateForServerMutex.Lock() + defer fake.generateForServerMutex.Unlock() + fake.GenerateForServerStub = nil + fake.generateForServerReturns = struct { + result1 policies.GenerateResultFiles + }{result1} +} + +func (fake *FakeGenerator) GenerateForServerReturnsOnCall(i int, result1 policies.GenerateResultFiles) { + fake.generateForServerMutex.Lock() + defer fake.generateForServerMutex.Unlock() + fake.GenerateForServerStub = nil + if fake.generateForServerReturnsOnCall == nil { + fake.generateForServerReturnsOnCall = make(map[int]struct { + result1 policies.GenerateResultFiles + }) + } + fake.generateForServerReturnsOnCall[i] = struct { + result1 policies.GenerateResultFiles + }{result1} +} + +func (fake *FakeGenerator) Invocations() map[string][][]interface{} { + fake.invocationsMutex.RLock() + defer fake.invocationsMutex.RUnlock() + fake.generateForInternalLocationMutex.RLock() + defer fake.generateForInternalLocationMutex.RUnlock() + fake.generateForLocationMutex.RLock() + defer fake.generateForLocationMutex.RUnlock() + fake.generateForServerMutex.RLock() + defer fake.generateForServerMutex.RUnlock() + copiedInvocations := map[string][][]interface{}{} + for key, value := range fake.invocations { + copiedInvocations[key] = value + } + return copiedInvocations +} + +func (fake *FakeGenerator) recordInvocation(key string, args []interface{}) { + fake.invocationsMutex.Lock() + defer fake.invocationsMutex.Unlock() + if fake.invocations == nil { + fake.invocations = map[string][][]interface{}{} + } + if fake.invocations[key] == nil { + fake.invocations[key] = [][]interface{}{} + } + fake.invocations[key] = append(fake.invocations[key], args) +} + +var _ policies.Generator = new(FakeGenerator) diff --git a/internal/mode/static/policies/policiesfakes/fake_object_kind.go b/internal/mode/static/nginx/config/policies/policiesfakes/fake_object_kind.go similarity index 100% rename from internal/mode/static/policies/policiesfakes/fake_object_kind.go rename to internal/mode/static/nginx/config/policies/policiesfakes/fake_object_kind.go diff --git a/internal/mode/static/policies/policiesfakes/fake_policy.go b/internal/mode/static/nginx/config/policies/policiesfakes/fake_policy.go similarity index 99% rename from internal/mode/static/policies/policiesfakes/fake_policy.go rename to internal/mode/static/nginx/config/policies/policiesfakes/fake_policy.go index 5fb03a937..3370e931a 100644 --- a/internal/mode/static/policies/policiesfakes/fake_policy.go +++ b/internal/mode/static/nginx/config/policies/policiesfakes/fake_policy.go @@ -4,7 +4,7 @@ package policiesfakes import ( "sync" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" diff --git a/internal/mode/static/policies/policiesfakes/fake_validator.go b/internal/mode/static/nginx/config/policies/policiesfakes/fake_validator.go similarity index 99% rename from internal/mode/static/policies/policiesfakes/fake_validator.go rename to internal/mode/static/nginx/config/policies/policiesfakes/fake_validator.go index 288ce3c57..05125b0d2 100644 --- a/internal/mode/static/policies/policiesfakes/fake_validator.go +++ b/internal/mode/static/nginx/config/policies/policiesfakes/fake_validator.go @@ -5,7 +5,7 @@ import ( "sync" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" ) type FakeValidator struct { diff --git a/internal/mode/static/policies/policy.go b/internal/mode/static/nginx/config/policies/policy.go similarity index 80% rename from internal/mode/static/policies/policy.go rename to internal/mode/static/nginx/config/policies/policy.go index 7072fe8d2..c26f3bec7 100644 --- a/internal/mode/static/policies/policy.go +++ b/internal/mode/static/nginx/config/policies/policy.go @@ -7,8 +7,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" "sigs.k8s.io/gateway-api/apis/v1alpha2" - - ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" ) //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate @@ -23,18 +21,9 @@ type Policy interface { client.Object } -// ConfigGenerator generates a slice of bytes containing the configuration from a Policy. -// -//counterfeiter:generate . ConfigGenerator -type ConfigGenerator interface { - Generate(policy Policy, globalSettings *GlobalSettings) []byte -} - // GlobalSettings contains global settings from the current state of the graph that may be // needed for policy validation or generation if certain policies rely on those global settings. type GlobalSettings struct { - // TracingSpanAttributes contain the attributes specified in the NginxProxy resource. - TracingSpanAttributes []ngfAPI.SpanAttribute // NginxProxyValid is whether or not the NginxProxy resource is valid. NginxProxyValid bool // TelemetryEnabled is whether or not telemetry is enabled in the NginxProxy resource. diff --git a/internal/mode/static/policies/manager.go b/internal/mode/static/nginx/config/policies/validator.go similarity index 62% rename from internal/mode/static/policies/manager.go rename to internal/mode/static/nginx/config/policies/validator.go index c7355720b..486028ea5 100644 --- a/internal/mode/static/policies/manager.go +++ b/internal/mode/static/nginx/config/policies/validator.go @@ -11,9 +11,6 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" ) -// GenerateFunc generates config as []byte for an NGF Policy. -type GenerateFunc func(policy Policy, globalSettings *GlobalSettings) []byte - // Validator validates an NGF Policy. // //counterfeiter:generate . Validator @@ -24,57 +21,40 @@ type Validator interface { Conflicts(a, b Policy) bool } -// Manager manages the validators and generators for NGF Policies. -type Manager struct { +// CompositeValidator manages the validators and generators for NGF Policies. +type CompositeValidator struct { validators map[schema.GroupVersionKind]Validator - generators map[schema.GroupVersionKind]GenerateFunc mustExtractGVK kinds.MustExtractGVK } -// ManagerConfig contains the config to register a Policy with the Manager. +// ManagerConfig contains the config to register a Policy with the CompositeValidator. type ManagerConfig struct { // Validator is the Validator for the Policy. Validator Validator - // Generate is the GenerateFunc for the Policy. - Generator GenerateFunc // GVK is the GroupVersionKind of the Policy. GVK schema.GroupVersionKind } -// NewManager returns a new Manager. +// NewManager returns a new CompositeValidator. // Implements dataplane.ConfigGenerator and validation.PolicyValidator. func NewManager( mustExtractGVK kinds.MustExtractGVK, configs ...ManagerConfig, -) *Manager { - v := &Manager{ +) *CompositeValidator { + v := &CompositeValidator{ validators: make(map[schema.GroupVersionKind]Validator), - generators: make(map[schema.GroupVersionKind]GenerateFunc), mustExtractGVK: mustExtractGVK, } for _, cfg := range configs { v.validators[cfg.GVK] = cfg.Validator - v.generators[cfg.GVK] = cfg.Generator } return v } -// Generate generates config for the policy as a byte array. -func (m *Manager) Generate(policy Policy, globalSettings *GlobalSettings) []byte { - gvk := m.mustExtractGVK(policy) - - generate, ok := m.generators[gvk] - if !ok { - panic(fmt.Sprintf("no generate function registered for policy %T", policy)) - } - - return generate(policy, globalSettings) -} - // Validate validates the policy. -func (m *Manager) Validate(policy Policy, globalSettings *GlobalSettings) []conditions.Condition { +func (m *CompositeValidator) Validate(policy Policy, globalSettings *GlobalSettings) []conditions.Condition { gvk := m.mustExtractGVK(policy) validator, ok := m.validators[gvk] @@ -86,7 +66,7 @@ func (m *Manager) Validate(policy Policy, globalSettings *GlobalSettings) []cond } // Conflicts returns true if the policies conflict. -func (m *Manager) Conflicts(polA, polB Policy) bool { +func (m *CompositeValidator) Conflicts(polA, polB Policy) bool { gvk := m.mustExtractGVK(polA) validator, ok := m.validators[gvk] diff --git a/internal/mode/static/policies/manager_test.go b/internal/mode/static/nginx/config/policies/validator_test.go similarity index 74% rename from internal/mode/static/policies/manager_test.go rename to internal/mode/static/nginx/config/policies/validator_test.go index 5699ac048..0ea940ce0 100644 --- a/internal/mode/static/policies/manager_test.go +++ b/internal/mode/static/nginx/config/policies/validator_test.go @@ -7,12 +7,12 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes" + policies "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" + policiesfakes "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" ) -var _ = Describe("Policy Manager", func() { +var _ = Describe("Policy CompositeValidator", func() { orangeGVK := schema.GroupVersionKind{Group: "fruit", Version: "1", Kind: "orange"} orangePolicy := &policiesfakes.FakePolicy{ GetNameStub: func() string { @@ -47,9 +47,6 @@ var _ = Describe("Policy Manager", func() { }, ConflictsStub: func(_ policies.Policy, _ policies.Policy) bool { return true }, }, - Generator: func(_ policies.Policy, _ *policies.GlobalSettings) []byte { - return []byte("apple") - }, GVK: appleGVK, }, policies.ManagerConfig{ @@ -59,9 +56,6 @@ var _ = Describe("Policy Manager", func() { }, ConflictsStub: func(_ policies.Policy, _ policies.Policy) bool { return false }, }, - Generator: func(_ policies.Policy, _ *policies.GlobalSettings) []byte { - return []byte("orange") - }, GVK: orangeGVK, }, ) @@ -99,21 +93,4 @@ var _ = Describe("Policy Manager", func() { }) }) }) - Context("Generation", func() { - When("Policy is registered with manager", func() { - It("Generates the configuration for the policy", func() { - Expect(mgr.Generate(applePolicy, nil)).To(Equal([]byte("apple"))) - Expect(mgr.Generate(orangePolicy, nil)).To(Equal([]byte("orange"))) - }) - }) - When("Policy is not registered with manager", func() { - It("Panics on generate", func() { - generate := func() { - _ = mgr.Generate(&policiesfakes.FakePolicy{}, nil) - } - - Expect(generate).To(Panic()) - }) - }) - }) }) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 3aeefa47c..727a0441d 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -10,6 +10,7 @@ import ( "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" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) @@ -57,8 +58,14 @@ var grpcBaseHeaders = []http.Header{ }, } -func executeServers(conf dataplane.Configuration) []executeResult { - servers, httpMatchPairs := createServers(conf.HTTPServers, conf.SSLServers) +func newExecuteServersFunc(generator policies.Generator) executeFunc { + return func(configuration dataplane.Configuration) []executeResult { + return executeServers(configuration, generator) + } +} + +func executeServers(conf dataplane.Configuration, generator policies.Generator) []executeResult { + servers, httpMatchPairs := createServers(conf.HTTPServers, conf.SSLServers, generator) serverConfig := http.ServerConfig{ Servers: servers, @@ -82,10 +89,10 @@ func executeServers(conf dataplane.Configuration) []executeResult { data: httpMatchConf, } - additionFileResults := createAdditionFileResults(conf) + includeFileResults := createIncludeFileResults(servers) - allResults := make([]executeResult, 0, len(additionFileResults)+2) - allResults = append(allResults, additionFileResults...) + allResults := make([]executeResult, 0, len(includeFileResults)+2) + allResults = append(allResults, includeFileResults...) allResults = append(allResults, serverResult, httpMatchResult) return allResults @@ -103,34 +110,24 @@ func getIPFamily(baseHTTPConfig dataplane.BaseHTTPConfig) http.IPFamily { return http.IPFamily{IPv4: true, IPv6: true} } -func createAdditionFileResults(conf dataplane.Configuration) []executeResult { - uniqueAdditions := make(map[string][]byte) +func createIncludeFileResults(servers []http.Server) []executeResult { + uniqueIncludes := make(map[string][]byte) - findUniqueAdditionsForServer := func(server dataplane.VirtualServer) { - for _, add := range server.Additions { - uniqueAdditions[createAdditionFileName(add)] = add.Bytes + for _, server := range servers { + for _, include := range server.Includes { + uniqueIncludes[include.Name] = include.Content } - for _, pr := range server.PathRules { - for _, mr := range pr.MatchRules { - for _, add := range mr.Additions { - uniqueAdditions[createAdditionFileName(add)] = add.Bytes - } + for _, loc := range server.Locations { + for _, include := range loc.Includes { + uniqueIncludes[include.Name] = include.Content } } } - for _, s := range conf.HTTPServers { - findUniqueAdditionsForServer(s) - } - - for _, s := range conf.SSLServers { - findUniqueAdditionsForServer(s) - } + results := make([]executeResult, 0, len(uniqueIncludes)) - results := make([]executeResult, 0, len(uniqueAdditions)) - - for filename, contents := range uniqueAdditions { + for filename, contents := range uniqueIncludes { results = append(results, executeResult{ dest: filename, data: contents, @@ -140,44 +137,33 @@ func createAdditionFileResults(conf dataplane.Configuration) []executeResult { return results } -func createAdditionFileName(addition dataplane.Addition) string { - return fmt.Sprintf("%s/%s.conf", includesFolder, addition.Identifier) -} - -func createIncludes(additions []dataplane.Addition) []string { - if len(additions) == 0 { - return nil - } - - includes := make([]string, 0, len(additions)) - - for _, addition := range additions { - includes = append(includes, createAdditionFileName(addition)) - } - - return includes -} - -func createServers(httpServers, sslServers []dataplane.VirtualServer) ([]http.Server, httpMatchPairs) { +func createServers( + httpServers, sslServers []dataplane.VirtualServer, + generator policies.Generator, +) ([]http.Server, httpMatchPairs) { servers := make([]http.Server, 0, len(httpServers)+len(sslServers)) finalMatchPairs := make(httpMatchPairs) for serverID, s := range httpServers { - httpServer, matchPairs := createServer(s, serverID) + httpServer, matchPairs := createServer(s, serverID, generator) servers = append(servers, httpServer) maps.Copy(finalMatchPairs, matchPairs) } for serverID, s := range sslServers { - sslServer, matchPair := createSSLServer(s, serverID) + sslServer, matchPairs := createSSLServer(s, serverID, generator) servers = append(servers, sslServer) - maps.Copy(finalMatchPairs, matchPair) + maps.Copy(finalMatchPairs, matchPairs) } return servers, finalMatchPairs } -func createSSLServer(virtualServer dataplane.VirtualServer, serverID int) (http.Server, httpMatchPairs) { +func createSSLServer( + virtualServer dataplane.VirtualServer, + serverIdx int, + generator policies.Generator, +) (http.Server, httpMatchPairs) { if virtualServer.IsDefault { return http.Server{ IsDefaultSSL: true, @@ -185,9 +171,10 @@ func createSSLServer(virtualServer dataplane.VirtualServer, serverID int) (http. }, nil } - locs, matchPairs, grpc := createLocations(&virtualServer, serverID) + serverID := fmt.Sprintf("SSL_%d", serverIdx) + locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator) - return http.Server{ + server := http.Server{ ServerName: virtualServer.Hostname, SSL: &http.SSL{ Certificate: generatePEMFileName(virtualServer.SSL.KeyPairID), @@ -196,11 +183,19 @@ func createSSLServer(virtualServer dataplane.VirtualServer, serverID int) (http. Locations: locs, Port: virtualServer.Port, GRPC: grpc, - Includes: createIncludes(virtualServer.Additions), - }, matchPairs + } + + server.Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForServer(virtualServer.Policies, server), + ) + return server, matchPairs } -func createServer(virtualServer dataplane.VirtualServer, serverID int) (http.Server, httpMatchPairs) { +func createServer( + virtualServer dataplane.VirtualServer, + serverIdx int, + generator policies.Generator, +) (http.Server, httpMatchPairs) { if virtualServer.IsDefault { return http.Server{ IsDefaultHTTP: true, @@ -208,30 +203,43 @@ func createServer(virtualServer dataplane.VirtualServer, serverID int) (http.Ser }, nil } - locs, matchPairs, grpc := createLocations(&virtualServer, serverID) + serverID := fmt.Sprintf("%d", serverIdx) + locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator) - return http.Server{ + server := http.Server{ ServerName: virtualServer.Hostname, Locations: locs, Port: virtualServer.Port, GRPC: grpc, - Includes: createIncludes(virtualServer.Additions), - }, matchPairs + } + + server.Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForServer(virtualServer.Policies, server), + ) + + return server, matchPairs } // rewriteConfig contains the configuration for a location to rewrite paths, // as specified in a URLRewrite filter. type rewriteConfig struct { - // Rewrite rewrites the original URI to the new URI (ex: /coffee -> /beans) - Rewrite string + // InternalRewrite rewrites an internal URI to the original URI (ex: /coffee_prefix_route0 -> /coffee) + InternalRewrite string + // MainRewrite rewrites the original URI to the new URI (ex: /coffee -> /beans) + MainRewrite string } type httpMatchPairs map[string][]routeMatch -func createLocations(server *dataplane.VirtualServer, serverID int) ([]http.Location, httpMatchPairs, bool) { +func createLocations( + server *dataplane.VirtualServer, + serverID string, + generator policies.Generator, +) ([]http.Location, httpMatchPairs, bool) { maxLocs, pathsAndTypes := getMaxLocationCountAndPathMap(server.PathRules) locs := make([]http.Location, 0, maxLocs) matchPairs := make(httpMatchPairs) + var rootPathExists bool var grpc bool @@ -248,41 +256,59 @@ func createLocations(server *dataplane.VirtualServer, serverID int) ([]http.Loca extLocations := initializeExternalLocations(rule, pathsAndTypes) - for matchRuleIdx, r := range rule.MatchRules { - buildLocations := extLocations - - if len(rule.MatchRules) != 1 || !isPathOnlyMatch(r.Match) { - intLocation, match := initializeInternalLocation(pathRuleIdx, matchRuleIdx, r.Match) - buildLocations = []http.Location{intLocation} - matches = append(matches, match) + if !needsInternalLocations(rule) { + for _, r := range rule.MatchRules { + extLocations = updateLocationsForFilters(r.Filters, extLocations, r, server.Port, rule.Path, rule.GRPC) } - includes := createIncludes(r.Additions) + for i := range extLocations { + extLocations[i].Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForLocation(rule.Policies, extLocations[i]), + ) + } - // buildLocations will either contain the extLocations OR the intLocation. - // If it contains the intLocation, the extLocations will be added to the final locations after we loop - // through all the MatchRules. - // It is safe to modify the buildLocations here by adding includes and filters. - buildLocations = updateLocationsForIncludes(buildLocations, includes) - buildLocations = updateLocationsForFilters(r.Filters, buildLocations, r, server.Port, rule.Path, rule.GRPC) - locs = append(locs, buildLocations...) + locs = append(locs, extLocations...) + continue } - if len(matches) > 0 { - for i := range extLocations { - // FIXME(sberman): De-dupe matches and associated locations - // so we don't need nginx/njs to perform unnecessary matching. - // https://github.com/nginxinc/nginx-gateway-fabric/issues/662 - var key string - if server.SSL != nil { - key = "SSL" - } - key += strconv.Itoa(serverID) + "_" + strconv.Itoa(pathRuleIdx) - extLocations[i].HTTPMatchKey = key - matchPairs[extLocations[i].HTTPMatchKey] = matches + internalLocations := make([]http.Location, 0, len(rule.MatchRules)) + + for matchRuleIdx, r := range rule.MatchRules { + if len(rule.MatchRules) != 1 || !isPathOnlyMatch(r.Match) { + intLocation, match := initializeInternalLocation(pathRuleIdx, matchRuleIdx, r.Match, grpc) + intLocation.Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForInternalLocation(rule.Policies), + ) + + intLocation = updateLocationForFilters( + r.Filters, + intLocation, + r, + server.Port, + rule.Path, + rule.GRPC, + ) + + internalLocations = append(internalLocations, intLocation) + matches = append(matches, match) } - locs = append(locs, extLocations...) } + + httpMatchKey := serverID + "_" + strconv.Itoa(pathRuleIdx) + for i := range extLocations { + // FIXME(sberman): De-dupe matches and associated locations + // so we don't need nginx/njs to perform unnecessary matching. + // https://github.com/nginxinc/nginx-gateway-fabric/issues/662 + extLocations[i].HTTPMatchKey = httpMatchKey + extLocations[i].Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForLocation(rule.Policies, extLocations[i]), + ) + + matchPairs[extLocations[i].HTTPMatchKey] = matches + } + + locs = append(locs, extLocations...) + locs = append(locs, internalLocations...) } if !rootPathExists { @@ -292,12 +318,27 @@ func createLocations(server *dataplane.VirtualServer, serverID int) ([]http.Loca return locs, matchPairs, grpc } -func updateLocationsForIncludes(locations []http.Location, includes []string) []http.Location { - for i := range locations { - locations[i].Includes = includes +func needsInternalLocations(rule dataplane.PathRule) bool { + if len(rule.MatchRules) > 1 { + return true + } + return len(rule.MatchRules) == 1 && !isPathOnlyMatch(rule.MatchRules[0].Match) +} + +func createIncludesFromPolicyGenerateResult(resFiles []policies.File) []http.Include { + if len(resFiles) == 0 { + return nil + } + + includes := make([]http.Include, 0, len(resFiles)) + for _, file := range resFiles { + includes = append(includes, http.Include{ + Name: includesFolder + "/" + file.Name, + Content: file.Content, + }) } - return locations + return includes } // pathAndTypeMap contains a map of paths and any path types defined for that path @@ -332,7 +373,9 @@ func initializeExternalLocations( pathsAndTypes pathAndTypeMap, ) []http.Location { extLocations := make([]http.Location, 0, 2) + locType := getLocationTypeForPathRule(rule) externalLocPath := createPath(rule) + // If the path type is Prefix and doesn't contain a trailing slash, then we need a second location // that handles the Exact prefix case (if it doesn't already exist), and the first location is updated // to handle the trailing slash prefix case (if it doesn't already exist) @@ -353,18 +396,21 @@ func initializeExternalLocations( if !trailingSlashPrefixPathExists { externalLocTrailing := http.Location{ Path: externalLocPath + "/", + Type: locType, } extLocations = append(extLocations, externalLocTrailing) } if !exactPathExists { externalLocExact := http.Location{ Path: exactPath(externalLocPath), + Type: locType, } extLocations = append(extLocations, externalLocExact) } } else { externalLoc := http.Location{ Path: externalLocPath, + Type: locType, } extLocations = []http.Location{externalLoc} } @@ -372,62 +418,88 @@ func initializeExternalLocations( return extLocations } +func getLocationTypeForPathRule(rule dataplane.PathRule) http.LocationType { + if needsInternalLocations(rule) { + return http.RedirectLocationType + } + + return http.ExternalLocationType +} + func initializeInternalLocation( pathruleIdx, matchRuleIdx int, match dataplane.Match, + grpc bool, ) (http.Location, routeMatch) { - path := fmt.Sprintf("@rule%d-route%d", pathruleIdx, matchRuleIdx) - return createMatchLocation(path), createRouteMatch(match, path) + path := fmt.Sprintf("%s-rule%d-route%d", http.InternalRoutePathPrefix, pathruleIdx, matchRuleIdx) + return createMatchLocation(path, grpc), createRouteMatch(match, path) } -// updateLocationsForFilters updates the existing locations with any relevant filters. -func updateLocationsForFilters( +func updateLocationForFilters( filters dataplane.HTTPFilters, - buildLocations []http.Location, + location http.Location, matchRule dataplane.MatchRule, listenerPort int32, path string, grpc bool, -) []http.Location { +) http.Location { if filters.InvalidFilter != nil { - for i := range buildLocations { - buildLocations[i].Return = &http.Return{Code: http.StatusInternalServerError} - } - return buildLocations + location.Return = &http.Return{Code: http.StatusInternalServerError} + return location } if filters.RequestRedirect != nil { ret := createReturnValForRedirectFilter(filters.RequestRedirect, listenerPort) - for i := range buildLocations { - buildLocations[i].Return = ret - } - return buildLocations + location.Return = ret + return location } rewrites := createRewritesValForRewriteFilter(filters.RequestURLRewrite, path) proxySetHeaders := generateProxySetHeaders(&matchRule.Filters, grpc) responseHeaders := generateResponseHeaders(&matchRule.Filters) - for i := range buildLocations { - if rewrites != nil { - if rewrites.Rewrite != "" { - buildLocations[i].Rewrites = append(buildLocations[i].Rewrites, rewrites.Rewrite) - } + + if rewrites != nil { + if location.Type == http.InternalLocationType && rewrites.InternalRewrite != "" { + location.Rewrites = append(location.Rewrites, rewrites.InternalRewrite) + } + if rewrites.MainRewrite != "" { + location.Rewrites = append(location.Rewrites, rewrites.MainRewrite) } - buildLocations[i].ProxySetHeaders = proxySetHeaders - buildLocations[i].ProxySSLVerify = createProxyTLSFromBackends(matchRule.BackendGroup.Backends) - proxyPass := createProxyPass( - matchRule.BackendGroup, - matchRule.Filters.RequestURLRewrite, - generateProtocolString(buildLocations[i].ProxySSLVerify, grpc), - grpc, - ) - buildLocations[i].ResponseHeaders = responseHeaders - buildLocations[i].ProxyPass = proxyPass - buildLocations[i].GRPC = grpc } - return buildLocations + location.ProxySetHeaders = proxySetHeaders + location.ProxySSLVerify = createProxyTLSFromBackends(matchRule.BackendGroup.Backends) + proxyPass := createProxyPass( + matchRule.BackendGroup, + matchRule.Filters.RequestURLRewrite, + generateProtocolString(location.ProxySSLVerify, grpc), + grpc, + ) + + location.ResponseHeaders = responseHeaders + location.ProxyPass = proxyPass + location.GRPC = grpc + + return location +} + +// updateLocationsForFilters updates the existing locations with any relevant filters. +func updateLocationsForFilters( + filters dataplane.HTTPFilters, + buildLocations []http.Location, + matchRule dataplane.MatchRule, + listenerPort int32, + path string, + grpc bool, +) []http.Location { + updatedLocations := make([]http.Location, len(buildLocations)) + + for i, loc := range buildLocations { + updatedLocations[i] = updateLocationForFilters(filters, loc, matchRule, listenerPort, path, grpc) + } + + return updatedLocations } func generateProtocolString(ssl *http.ProxySSLVerify, grpc bool) string { @@ -528,9 +600,10 @@ func createRewritesValForRewriteFilter(filter *dataplane.HTTPURLRewriteFilter, p rewrites := &rewriteConfig{} if filter.Path != nil { + rewrites.InternalRewrite = "^ $request_uri" switch filter.Path.Type { case dataplane.ReplaceFullPath: - rewrites.Rewrite = fmt.Sprintf("^ %s break", filter.Path.Replacement) + rewrites.MainRewrite = fmt.Sprintf("^ %s break", filter.Path.Replacement) case dataplane.ReplacePrefixMatch: filterPrefix := filter.Path.Replacement if filterPrefix == "" { @@ -555,7 +628,7 @@ func createRewritesValForRewriteFilter(filter *dataplane.HTTPURLRewriteFilter, p replacement = fmt.Sprintf("%s/$1", filterPrefix) } - rewrites.Rewrite = fmt.Sprintf("%s %s break", regex, replacement) + rewrites.MainRewrite = fmt.Sprintf("%s %s break", regex, replacement) } } @@ -662,10 +735,19 @@ func createProxyPass( return protocol + "://" + backendName + requestURI } -func createMatchLocation(path string) http.Location { - return http.Location{ - Path: path, +func createMatchLocation(path string, grpc bool) http.Location { + var rewrites []string + if grpc { + rewrites = []string{"^ $request_uri break"} } + + loc := http.Location{ + Path: path, + Rewrites: rewrites, + Type: http.InternalLocationType, + } + + return loc } func generateProxySetHeaders(filters *dataplane.HTTPFilters, grpc bool) []http.Header { diff --git a/internal/mode/static/nginx/config/servers_template.go b/internal/mode/static/nginx/config/servers_template.go index 4d8196b18..6d8bc0c36 100644 --- a/internal/mode/static/nginx/config/servers_template.go +++ b/internal/mode/static/nginx/config/servers_template.go @@ -52,14 +52,18 @@ server { server_name {{ $s.ServerName }}; - {{- range $i := $s.Includes }} - include {{ $i }}; - {{ end -}} + {{- range $i := $s.Includes }} + include {{ $i.Name }}; + {{ end -}} {{ range $l := $s.Locations }} location {{ $l.Path }} { + {{ if eq $l.Type "internal" -}} + internal; + {{ end }} + {{- range $i := $l.Includes }} - include {{ $i }}; + include {{ $i.Name }}; {{- end -}} {{ range $r := $l.Rewrites }} @@ -70,7 +74,7 @@ server { return {{ $l.Return.Code }} "{{ $l.Return.Body }}"; {{- end }} - {{- if $l.HTTPMatchKey }} + {{- if eq $l.Type "redirect" }} set $match_key {{ $l.HTTPMatchKey }}; js_content httpmatches.redirect; {{- end }} @@ -105,7 +109,7 @@ server { {{- end }} {{- end }} } - {{ end }} + {{- end }} {{- if $s.GRPC }} include /etc/nginx/grpc-error-locations.conf; diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index effb0099a..df517eebe 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -11,6 +11,8 @@ import ( "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" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" ) @@ -28,11 +30,8 @@ func TestExecuteServers(t *testing.T) { { Hostname: "cafe.example.com", Port: 8080, - Additions: []dataplane.Addition{ - { - Bytes: []byte("addition-1"), - Identifier: "addition-1", - }, + Policies: []policies.Policy{ + &policiesfakes.FakePolicy{}, }, }, }, @@ -80,15 +79,8 @@ func TestExecuteServers(t *testing.T) { }, }, }, - Additions: []dataplane.Addition{ - { - Bytes: []byte("addition-1"), - Identifier: "addition-1", // duplicate - }, - { - Bytes: []byte("addition-2"), - Identifier: "addition-2", - }, + Policies: []policies.Policy{ + &policiesfakes.FakePolicy{}, }, }, }, @@ -117,16 +109,28 @@ func TestExecuteServers(t *testing.T) { httpMatchVarsFile: func(g *WithT, data string) { g.Expect(data).To(Equal("{}")) }, - includesFolder + "/addition-1.conf": func(g *WithT, data string) { - g.Expect(data).To(Equal("addition-1")) + includesFolder + "/include-1.conf": func(g *WithT, data string) { + g.Expect(data).To(Equal("include-1")) }, - includesFolder + "/addition-2.conf": func(g *WithT, data string) { - g.Expect(data).To(Equal("addition-2")) + includesFolder + "/include-2.conf": func(g *WithT, data string) { + g.Expect(data).To(Equal("include-2")) }, } g := NewWithT(t) - results := executeServers(conf) + fakeGenerator := &policiesfakes.FakeGenerator{} + fakeGenerator.GenerateForServerReturns(policies.GenerateResultFiles{ + { + Name: "include-1.conf", + Content: []byte("include-1"), + }, + { + Name: "include-2.conf", + Content: []byte("include-2"), + }, + }) + + results := executeServers(conf, fakeGenerator) g.Expect(results).To(HaveLen(len(expectedResults))) for _, res := range results { @@ -235,7 +239,7 @@ func TestExecuteServersForIPFamily(t *testing.T) { for _, test := range tests { t.Run(test.msg, func(t *testing.T) { g := NewWithT(t) - results := executeServers(test.config) + results := executeServers(test.config, &policiesfakes.FakeGenerator{}) g.Expect(results).To(HaveLen(2)) serverConf := string(results[0].data) httpMatchConf := string(results[1].data) @@ -319,7 +323,7 @@ func TestExecuteForDefaultServers(t *testing.T) { t.Run(tc.msg, func(t *testing.T) { g := NewWithT(t) - serverResults := executeServers(tc.conf) + serverResults := executeServers(tc.conf, &policiesfakes.FakeGenerator{}) g.Expect(serverResults).To(HaveLen(2)) serverConf := string(serverResults[0].data) httpMatchConf := string(serverResults[1].data) @@ -695,36 +699,30 @@ func TestCreateServers(t *testing.T) { GRPC: true, }, { - Path: "/addition-path-only-match", + Path: "/include-path-only-match", PathType: dataplane.PathTypeExact, + Policies: []policies.Policy{ + &policiesfakes.FakePolicy{}, + }, MatchRules: []dataplane.MatchRule{ { Match: dataplane.Match{}, BackendGroup: fooGroup, - Additions: []dataplane.Addition{ - { - Bytes: []byte("path-only-match-addition"), - Identifier: "path-only-match-addition", - }, - }, }, }, }, { - Path: "/addition-header-match", + Path: "/include-header-match", PathType: dataplane.PathTypeExact, + Policies: []policies.Policy{ + &policiesfakes.FakePolicy{}, + }, MatchRules: []dataplane.MatchRule{ { Match: dataplane.Match{ Method: helpers.GetPointer("GET"), }, BackendGroup: fooGroup, - Additions: []dataplane.Addition{ - { - Bytes: []byte("match-addition"), - Identifier: "match-addition", - }, - }, }, }, }, @@ -739,15 +737,9 @@ func TestCreateServers(t *testing.T) { Hostname: "cafe.example.com", PathRules: cafePathRules, Port: 8080, - Additions: []dataplane.Addition{ - { - Bytes: []byte("server-addition-1"), - Identifier: "server-addition-1", - }, - { - Bytes: []byte("server-addition-2"), - Identifier: "server-addition-2", - }, + Policies: []policies.Policy{ + &policiesfakes.FakePolicy{}, + &policiesfakes.FakePolicy{}, }, }, } @@ -762,52 +754,46 @@ func TestCreateServers(t *testing.T) { SSL: &dataplane.SSL{KeyPairID: sslKeyPairID}, PathRules: cafePathRules, Port: 8443, - Additions: []dataplane.Addition{ - { - Bytes: []byte("server-addition-1"), - Identifier: "server-addition-1", - }, - { - Bytes: []byte("server-addition-3"), - Identifier: "server-addition-3", - }, + Policies: []policies.Policy{ + &policiesfakes.FakePolicy{}, + &policiesfakes.FakePolicy{}, }, }, } expMatchPairs := httpMatchPairs{ "1_0": { - {Method: "POST", RedirectPath: "@rule0-route0"}, - {Method: "PATCH", RedirectPath: "@rule0-route1"}, - {RedirectPath: "@rule0-route2", Any: true}, + {Method: "POST", RedirectPath: "/_ngf-internal-rule0-route0"}, + {Method: "PATCH", RedirectPath: "/_ngf-internal-rule0-route1"}, + {RedirectPath: "/_ngf-internal-rule0-route2", Any: true}, }, "1_1": { { Method: "GET", Headers: []string{"Version:V1", "test:foo", "my-header:my-value"}, QueryParams: []string{"GrEat=EXAMPLE", "test=foo=bar"}, - RedirectPath: "@rule1-route0", + RedirectPath: "/_ngf-internal-rule1-route0", }, }, "1_6": { - {RedirectPath: "@rule6-route0", Headers: []string{"redirect:this"}}, + {RedirectPath: "/_ngf-internal-rule6-route0", Headers: []string{"redirect:this"}}, }, "1_8": { { Headers: []string{"rewrite:this"}, - RedirectPath: "@rule8-route0", + RedirectPath: "/_ngf-internal-rule8-route0", }, }, "1_10": { { Headers: []string{"filter:this"}, - RedirectPath: "@rule10-route0", + RedirectPath: "/_ngf-internal-rule10-route0", }, }, "1_12": { { Method: "GET", - RedirectPath: "@rule12-route0", + RedirectPath: "/_ngf-internal-rule12-route0", Headers: nil, QueryParams: nil, Any: false, @@ -816,7 +802,7 @@ func TestCreateServers(t *testing.T) { "1_17": { { Method: "GET", - RedirectPath: "@rule17-route0", + RedirectPath: "/_ngf-internal-rule17-route0", }, }, } @@ -845,52 +831,75 @@ func TestCreateServers(t *testing.T) { }, } + externalIncludes := []http.Include{ + {Name: "/etc/nginx/includes/include-1.conf", Content: []byte("include-1")}, + } + internalIncludes := []http.Include{ + {Name: "/etc/nginx/includes/internal-include-1.conf", Content: []byte("include-1")}, + } + getExpectedLocations := func(isHTTPS bool) []http.Location { port := 8080 ssl := "" if isHTTPS { port = 8443 - ssl = "SSL" + ssl = "SSL_" } return []http.Location{ { - Path: "@rule0-route0", + Path: "/", + HTTPMatchKey: ssl + "1_0", + Type: http.RedirectLocationType, + Includes: externalIncludes, + }, + { + Path: "/_ngf-internal-rule0-route0", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.InternalLocationType, + Includes: internalIncludes, }, { - Path: "@rule0-route1", + Path: "/_ngf-internal-rule0-route1", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.InternalLocationType, + Includes: internalIncludes, }, { - Path: "@rule0-route2", + Path: "/_ngf-internal-rule0-route2", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.InternalLocationType, + Includes: internalIncludes, }, { - Path: "/", - HTTPMatchKey: ssl + "1_0", + Path: "/test/", + HTTPMatchKey: ssl + "1_1", + Type: http.RedirectLocationType, + Includes: externalIncludes, }, { - Path: "@rule1-route0", + Path: "/_ngf-internal-rule1-route0", ProxyPass: "http://$test__route1_rule1$request_uri", ProxySetHeaders: httpBaseHeaders, - }, - { - Path: "/test/", - HTTPMatchKey: ssl + "1_1", + Type: http.InternalLocationType, + Includes: internalIncludes, }, { Path: "/path-only/", ProxyPass: "http://invalid-backend-ref$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "= /path-only", ProxyPass: "http://invalid-backend-ref$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "/backend-tls-policy/", @@ -900,6 +909,8 @@ func TestCreateServers(t *testing.T) { Name: "test-btp.example.com", TrustedCertificate: "/etc/nginx/secrets/test-btp.crt", }, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "= /backend-tls-policy", @@ -909,6 +920,8 @@ func TestCreateServers(t *testing.T) { Name: "test-btp.example.com", TrustedCertificate: "/etc/nginx/secrets/test-btp.crt", }, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "/redirect-implicit-port/", @@ -916,6 +929,8 @@ func TestCreateServers(t *testing.T) { Code: 302, Body: fmt.Sprintf("$scheme://foo.example.com:%d$request_uri", port), }, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "= /redirect-implicit-port", @@ -923,6 +938,8 @@ func TestCreateServers(t *testing.T) { Code: 302, Body: fmt.Sprintf("$scheme://foo.example.com:%d$request_uri", port), }, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "/redirect-explicit-port/", @@ -930,6 +947,8 @@ func TestCreateServers(t *testing.T) { Code: 302, Body: "$scheme://bar.example.com:8080$request_uri", }, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "= /redirect-explicit-port", @@ -937,87 +956,121 @@ func TestCreateServers(t *testing.T) { Code: 302, Body: "$scheme://bar.example.com:8080$request_uri", }, - }, - { - Path: "@rule6-route0", - Return: &http.Return{ - Body: "$scheme://foo.example.com:8080$request_uri", - Code: 302, - }, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "/redirect-with-headers/", HTTPMatchKey: ssl + "1_6", + Type: http.RedirectLocationType, + Includes: externalIncludes, }, { Path: "= /redirect-with-headers", HTTPMatchKey: ssl + "1_6", + Type: http.RedirectLocationType, + Includes: externalIncludes, + }, + { + Path: "/_ngf-internal-rule6-route0", + Return: &http.Return{ + Body: "$scheme://foo.example.com:8080$request_uri", + Code: 302, + }, + Type: http.InternalLocationType, + Includes: internalIncludes, }, { Path: "/rewrite/", Rewrites: []string{"^ /replacement break"}, ProxyPass: "http://test_foo_80", ProxySetHeaders: rewriteProxySetHeaders, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "= /rewrite", Rewrites: []string{"^ /replacement break"}, ProxyPass: "http://test_foo_80", ProxySetHeaders: rewriteProxySetHeaders, - }, - { - Path: "@rule8-route0", - Rewrites: []string{"^/rewrite-with-headers(.*)$ /prefix-replacement$1 break"}, - ProxyPass: "http://test_foo_80", - ProxySetHeaders: rewriteProxySetHeaders, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "/rewrite-with-headers/", HTTPMatchKey: ssl + "1_8", + Type: http.RedirectLocationType, + Includes: externalIncludes, }, { Path: "= /rewrite-with-headers", HTTPMatchKey: ssl + "1_8", + Type: http.RedirectLocationType, + Includes: externalIncludes, }, { - Path: "/invalid-filter/", - Return: &http.Return{ - Code: http.StatusInternalServerError, - }, + Path: "/_ngf-internal-rule8-route0", + Rewrites: []string{"^ $request_uri", "^/rewrite-with-headers(.*)$ /prefix-replacement$1 break"}, + ProxyPass: "http://test_foo_80", + ProxySetHeaders: rewriteProxySetHeaders, + Type: http.InternalLocationType, + Includes: internalIncludes, }, { - Path: "= /invalid-filter", + Path: "/invalid-filter/", Return: &http.Return{ Code: http.StatusInternalServerError, }, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { - Path: "@rule10-route0", + Path: "= /invalid-filter", Return: &http.Return{ Code: http.StatusInternalServerError, }, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "/invalid-filter-with-headers/", HTTPMatchKey: ssl + "1_10", + Type: http.RedirectLocationType, + Includes: externalIncludes, }, { Path: "= /invalid-filter-with-headers", HTTPMatchKey: ssl + "1_10", + Type: http.RedirectLocationType, + Includes: externalIncludes, }, { - Path: "= /exact", - ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: httpBaseHeaders, + Path: "/_ngf-internal-rule10-route0", + Return: &http.Return{ + Code: http.StatusInternalServerError, + }, + Type: http.InternalLocationType, + Includes: internalIncludes, }, { - Path: "@rule12-route0", + Path: "= /exact", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "= /test", HTTPMatchKey: ssl + "1_12", + Type: http.RedirectLocationType, + Includes: externalIncludes, + }, + { + Path: "/_ngf-internal-rule12-route0", + ProxyPass: "http://test_foo_80$request_uri", + ProxySetHeaders: httpBaseHeaders, + Type: http.InternalLocationType, + Includes: internalIncludes, }, { Path: "/proxy-set-headers/", @@ -1054,6 +1107,8 @@ func TestCreateServers(t *testing.T) { Set: []http.Header{}, Remove: []string{}, }, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "= /proxy-set-headers", @@ -1090,12 +1145,16 @@ func TestCreateServers(t *testing.T) { Set: []http.Header{}, Remove: []string{}, }, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "= /grpc/method", ProxyPass: "grpc://test_foo_80", GRPC: true, ProxySetHeaders: grpcBaseHeaders, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { Path: "= /grpc-with-backend-tls-policy/method", @@ -1106,26 +1165,29 @@ func TestCreateServers(t *testing.T) { }, GRPC: true, ProxySetHeaders: grpcBaseHeaders, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { - Path: "= /addition-path-only-match", + Path: "= /include-path-only-match", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, - Includes: []string{ - includesFolder + "/path-only-match-addition.conf", - }, + Type: http.ExternalLocationType, + Includes: externalIncludes, }, { - Path: "@rule17-route0", - ProxyPass: "http://test_foo_80$request_uri", - ProxySetHeaders: httpBaseHeaders, - Includes: []string{ - includesFolder + "/match-addition.conf", - }, + Path: "= /include-header-match", + HTTPMatchKey: ssl + "1_17", + Type: http.RedirectLocationType, + Includes: externalIncludes, }, { - Path: "= /addition-header-match", - HTTPMatchKey: ssl + "1_17", + Path: "/_ngf-internal-rule17-route0", + ProxyPass: "http://test_foo_80$request_uri", + ProxySetHeaders: httpBaseHeaders, + Rewrites: []string{"^ $request_uri break"}, + Type: http.InternalLocationType, + Includes: internalIncludes, }, } } @@ -1142,10 +1204,6 @@ func TestCreateServers(t *testing.T) { Locations: getExpectedLocations(false), Port: 8080, GRPC: true, - Includes: []string{ - includesFolder + "/server-addition-1.conf", - includesFolder + "/server-addition-2.conf", - }, }, { IsDefaultSSL: true, @@ -1160,16 +1218,26 @@ func TestCreateServers(t *testing.T) { Locations: getExpectedLocations(true), Port: 8443, GRPC: true, - Includes: []string{ - includesFolder + "/server-addition-1.conf", - includesFolder + "/server-addition-3.conf", - }, }, } g := NewWithT(t) - result, httpMatchPair := createServers(httpServers, sslServers) + fakeGenerator := &policiesfakes.FakeGenerator{} + fakeGenerator.GenerateForLocationReturns(policies.GenerateResultFiles{ + { + Name: "include-1.conf", + Content: []byte("include-1"), + }, + }) + fakeGenerator.GenerateForInternalLocationReturns(policies.GenerateResultFiles{ + { + Name: "internal-include-1.conf", + Content: []byte("include-1"), + }, + }) + + result, httpMatchPair := createServers(httpServers, sslServers, fakeGenerator) g.Expect(httpMatchPair).To(Equal(allExpMatchPair)) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) @@ -1178,7 +1246,7 @@ func TestCreateServers(t *testing.T) { func modifyMatchPairs(matchPairs httpMatchPairs) httpMatchPairs { modified := make(httpMatchPairs) for k, v := range matchPairs { - modifiedKey := "SSL" + k + modifiedKey := "SSL_" + k modified[modifiedKey] = v } @@ -1254,11 +1322,13 @@ func TestCreateServersConflicts(t *testing.T) { Path: "/coffee/", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, }, { Path: "= /coffee", ProxyPass: "http://test_bar_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, }, createDefaultRootLocation(), }, @@ -1292,11 +1362,13 @@ func TestCreateServersConflicts(t *testing.T) { Path: "= /coffee", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, }, { Path: "/coffee/", ProxyPass: "http://test_bar_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, }, createDefaultRootLocation(), }, @@ -1340,11 +1412,13 @@ func TestCreateServersConflicts(t *testing.T) { Path: "/coffee/", ProxyPass: "http://test_bar_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, }, { Path: "= /coffee", ProxyPass: "http://test_baz_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, }, createDefaultRootLocation(), }, @@ -1378,7 +1452,7 @@ func TestCreateServersConflicts(t *testing.T) { g := NewWithT(t) - result, _ := createServers(httpServers, []dataplane.VirtualServer{}) + result, _ := createServers(httpServers, []dataplane.VirtualServer{}, &policiesfakes.FakeGenerator{}) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) }) } @@ -1463,11 +1537,13 @@ func TestCreateLocationsRootPath(t *testing.T) { Path: "/path-1", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, }, { Path: "/path-2", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, }, { Path: "/", @@ -1486,17 +1562,20 @@ func TestCreateLocationsRootPath(t *testing.T) { Path: "/path-1", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, }, { Path: "/path-2", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, }, { Path: "/grpc", ProxyPass: "grpc://test_foo_80", GRPC: true, ProxySetHeaders: grpcBaseHeaders, + Type: http.ExternalLocationType, }, { Path: "/", @@ -1514,16 +1593,19 @@ func TestCreateLocationsRootPath(t *testing.T) { Path: "/path-1", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, }, { Path: "/path-2", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, }, { Path: "/", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, + Type: http.ExternalLocationType, }, }, }, @@ -1548,7 +1630,7 @@ func TestCreateLocationsRootPath(t *testing.T) { locs, httpMatchPair, grpc := createLocations(&dataplane.VirtualServer{ PathRules: test.pathRules, Port: 80, - }, 1) + }, "1", &policiesfakes.FakeGenerator{}) g.Expect(locs).To(Equal(test.expLocations)) g.Expect(httpMatchPair).To(BeEmpty()) g.Expect(grpc).To(Equal(test.grpc)) @@ -1712,7 +1794,8 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) { }, }, expected: &rewriteConfig{ - Rewrite: "^ /full-path break", + InternalRewrite: "^ $request_uri", + MainRewrite: "^ /full-path break", }, msg: "full path", }, @@ -1725,7 +1808,8 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) { }, }, expected: &rewriteConfig{ - Rewrite: "^/original(.*)$ /prefix-path$1 break", + InternalRewrite: "^ $request_uri", + MainRewrite: "^/original(.*)$ /prefix-path$1 break", }, msg: "prefix path no trailing slashes", }, @@ -1738,7 +1822,8 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) { }, }, expected: &rewriteConfig{ - Rewrite: "^/original(?:/(.*))?$ /$1 break", + InternalRewrite: "^ $request_uri", + MainRewrite: "^/original(?:/(.*))?$ /$1 break", }, msg: "prefix path empty string", }, @@ -1751,7 +1836,8 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) { }, }, expected: &rewriteConfig{ - Rewrite: "^/original(?:/(.*))?$ /$1 break", + InternalRewrite: "^ $request_uri", + MainRewrite: "^/original(?:/(.*))?$ /$1 break", }, msg: "prefix path /", }, @@ -1764,7 +1850,8 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) { }, }, expected: &rewriteConfig{ - Rewrite: "^/original(?:/(.*))?$ /trailing/$1 break", + InternalRewrite: "^ $request_uri", + MainRewrite: "^/original(?:/(.*))?$ /trailing/$1 break", }, msg: "prefix path replacement with trailing /", }, @@ -1777,7 +1864,8 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) { }, }, expected: &rewriteConfig{ - Rewrite: "^/original/(.*)$ /prefix-path/$1 break", + InternalRewrite: "^ $request_uri", + MainRewrite: "^/original/(.*)$ /prefix-path/$1 break", }, msg: "prefix path original with trailing /", }, @@ -1790,7 +1878,8 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) { }, }, expected: &rewriteConfig{ - Rewrite: "^/original/(.*)$ /trailing/$1 break", + InternalRewrite: "^ $request_uri", + MainRewrite: "^/original/(.*)$ /trailing/$1 break", }, msg: "prefix path both with trailing slashes", }, @@ -2138,12 +2227,24 @@ func TestCreateProxyPass(t *testing.T) { func TestCreateMatchLocation(t *testing.T) { g := NewWithT(t) - expected := http.Location{ + expectedNoGRPC := http.Location{ Path: "/path", + Type: http.InternalLocationType, } - result := createMatchLocation("/path") - g.Expect(result).To(Equal(expected)) + grpc := false + result := createMatchLocation("/path", grpc) + g.Expect(result).To(Equal(expectedNoGRPC)) + + expectedWithGRPC := http.Location{ + Path: "/path", + Type: http.InternalLocationType, + Rewrites: []string{"^ $request_uri break"}, + } + + grpc = true + result = createMatchLocation("/path", grpc) + g.Expect(result).To(Equal(expectedWithGRPC)) } func TestGenerateProxySetHeaders(t *testing.T) { @@ -2457,37 +2558,46 @@ func TestGenerateResponseHeaders(t *testing.T) { } } -func TestCreateIncludes(t *testing.T) { +func TestCreateIncludesFromPolicyGenerateResult(t *testing.T) { tests := []struct { - name string - additions []dataplane.Addition - includes []string + name string + files []policies.File + includes []http.Include }{ { - name: "no additions", - additions: nil, - includes: nil, + name: "no files", + files: nil, + includes: nil, }, { name: "additions", - additions: []dataplane.Addition{ + files: []policies.File{ { - Bytes: []byte("one"), - Identifier: "one", + Content: []byte("one"), + Name: "one.conf", }, { - Bytes: []byte("two"), - Identifier: "two", + Content: []byte("two"), + Name: "two.conf", }, { - Bytes: []byte("three"), - Identifier: "three", + Content: []byte("three"), + Name: "three.conf", }, }, - includes: []string{ - includesFolder + "/one.conf", - includesFolder + "/two.conf", - includesFolder + "/three.conf", + includes: []http.Include{ + { + Content: []byte("one"), + Name: includesFolder + "/one.conf", + }, + { + Content: []byte("two"), + Name: includesFolder + "/two.conf", + }, + { + Content: []byte("three"), + Name: includesFolder + "/three.conf", + }, }, }, } @@ -2496,89 +2606,65 @@ func TestCreateIncludes(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - includes := createIncludes(test.additions) + includes := createIncludesFromPolicyGenerateResult(test.files) g.Expect(includes).To(Equal(test.includes)) }) } } -func TestCreateAdditionFileResults(t *testing.T) { - conf := dataplane.Configuration{ - HTTPServers: []dataplane.VirtualServer{ - { - Additions: []dataplane.Addition{ - { - Identifier: "include-1", - Bytes: []byte("include-1"), - }, - { - Identifier: "include-2", - Bytes: []byte("include-2"), - }, +func TestCreateIncludeFileResults(t *testing.T) { + servers := []http.Server{ + { + Includes: []http.Include{ + { + Name: "include-1.conf", + Content: []byte("include-1"), }, - PathRules: []dataplane.PathRule{ - { - MatchRules: []dataplane.MatchRule{ - { - Additions: []dataplane.Addition{ - { - Identifier: "include-3", - Bytes: []byte("include-3"), - }, - { - Identifier: "include-4", - Bytes: []byte("include-4"), - }, - }, - }, - }, - }, + { + Name: "include-2.conf", + Content: []byte("include-2"), }, }, - { - Additions: []dataplane.Addition{ - { - Identifier: "include-1", // dupe - Bytes: []byte("include-1"), - }, - { - Identifier: "include-2", // dupe - Bytes: []byte("include-2"), + Locations: []http.Location{ + { + Includes: []http.Include{ + { + Name: "include-3.conf", + Content: []byte("include-3"), + }, + { + Name: "include-4.conf", + Content: []byte("include-4"), + }, }, }, }, }, - SSLServers: []dataplane.VirtualServer{ - { - Additions: []dataplane.Addition{ - { - Identifier: "include-1", // dupe - Bytes: []byte("include-1"), - }, - { - Identifier: "include-2", // dupe - Bytes: []byte("include-2"), - }, + { + Includes: []http.Include{ + { + Name: "include-1.conf", // dupe + Content: []byte("include-1"), }, - PathRules: []dataplane.PathRule{ - { - MatchRules: []dataplane.MatchRule{ - { - Additions: []dataplane.Addition{ - { - Identifier: "include-3", - Bytes: []byte("include-3"), // dupe - }, - { - Identifier: "include-5", - Bytes: []byte("include-5"), // dupe - }, - { - Identifier: "include-6", - Bytes: []byte("include-6"), - }, - }, - }, + { + Name: "include-2.conf", // dupe + Content: []byte("include-2"), + }, + }, + Locations: []http.Location{ + { + Includes: []http.Include{ + { + Name: "include-3.conf", // dupe + Content: []byte("include-3"), + }, + { + Name: "include-4.conf", // dupe + Content: []byte("include-4"), + }, + { + Name: "include-5.conf", + Content: []byte("include-5"), }, }, }, @@ -2586,33 +2672,29 @@ func TestCreateAdditionFileResults(t *testing.T) { }, } - results := createAdditionFileResults(conf) + results := createIncludeFileResults(servers) expResults := []executeResult{ { - dest: includesFolder + "/" + "include-1.conf", + dest: "include-1.conf", data: []byte("include-1"), }, { - dest: includesFolder + "/" + "include-2.conf", + dest: "include-2.conf", data: []byte("include-2"), }, { - dest: includesFolder + "/" + "include-3.conf", + dest: "include-3.conf", data: []byte("include-3"), }, { - dest: includesFolder + "/" + "include-4.conf", + dest: "include-4.conf", data: []byte("include-4"), }, { - dest: includesFolder + "/" + "include-5.conf", + dest: "include-5.conf", data: []byte("include-5"), }, - { - dest: includesFolder + "/" + "include-6.conf", - data: []byte("include-6"), - }, } g := NewWithT(t) @@ -2620,13 +2702,6 @@ func TestCreateAdditionFileResults(t *testing.T) { g.Expect(results).To(ConsistOf(expResults)) } -func TestAdditionFilename(t *testing.T) { - g := NewWithT(t) - - name := createAdditionFileName(dataplane.Addition{Identifier: "my-addition"}) - g.Expect(name).To(Equal(includesFolder + "/" + "my-addition.conf")) -} - func TestGetIPFamily(t *testing.T) { test := []struct { msg string diff --git a/internal/mode/static/nginx/config/validation/http_njs_match_test.go b/internal/mode/static/nginx/config/validation/http_njs_match_test.go index 942d944af..6856dea98 100644 --- a/internal/mode/static/nginx/config/validation/http_njs_match_test.go +++ b/internal/mode/static/nginx/config/validation/http_njs_match_test.go @@ -13,7 +13,7 @@ func TestValidatePathInMatch(t *testing.T) { "/", "/path", "/path/subpath-123", - "/route0-rule0", + "/_ngf-internal-route0-rule0", ) testInvalidValuesForSimpleValidator( t, diff --git a/internal/mode/static/nginx/modules/src/httpmatches.js b/internal/mode/static/nginx/modules/src/httpmatches.js index 1b4eb0be3..bcee4d656 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/internal/mode/static/nginx/modules/src/httpmatches.js @@ -1,3 +1,5 @@ +import qs from 'querystring'; + const MATCHES_KEY = 'match_key'; const HTTP_CODES = { notFound: 404, @@ -71,7 +73,14 @@ function redirectForMatchList(r, matchList) { return; } - r.internalRedirect(match.redirectPath); + // If performing a rewrite, $request_uri won't be used, + // so we have to preserve args in the internal redirect. + let args = qs.stringify(r.args); + if (args) { + args = '?' + args; + } + + r.internalRedirect(match.redirectPath + args); } function verifyMatchList(matchList) { diff --git a/internal/mode/static/nginx/modules/test/httpmatches.test.js b/internal/mode/static/nginx/modules/test/httpmatches.test.js index d4f502ace..87dd10705 100644 --- a/internal/mode/static/nginx/modules/test/httpmatches.test.js +++ b/internal/mode/static/nginx/modules/test/httpmatches.test.js @@ -1,5 +1,5 @@ import { default as hm } from '../src/httpmatches.js'; -import { assert, describe, expect, it } from 'vitest'; +import { describe, expect, it } from 'vitest'; // Creates a NGINX HTTP Request Object for testing. // See documentation for all properties available: http://nginx.org/en/docs/njs/reference.html @@ -472,7 +472,7 @@ describe('redirectForMatchList', () => { params: { Arg1: 'value1', arg2: 'value2=SOME=other=value' }, }), matches: [testHeaderMatches, testQueryParamMatches, testAllMatchTypes, testAnyMatch], // request matches testAllMatchTypes and testAnyMatch. But first match should win. - expectedRedirect: '/a-match', + expectedRedirect: '/a-match?Arg1=value1&arg2=value2%3DSOME%3Dother%3Dvalue', }, ]; diff --git a/internal/mode/static/policies/clientsettings/generator.go b/internal/mode/static/policies/clientsettings/generator.go deleted file mode 100644 index 943c8e2c6..000000000 --- a/internal/mode/static/policies/clientsettings/generator.go +++ /dev/null @@ -1,44 +0,0 @@ -package clientsettings - -import ( - "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/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 }} -` - -// Generate generates configuration as []byte for a ClientSettingsPolicy. -func Generate(policy policies.Policy, _ *policies.GlobalSettings) []byte { - csp := helpers.MustCastObject[*ngfAPI.ClientSettingsPolicy](policy) - - return helpers.MustExecuteTemplate(tmpl, csp.Spec) -} diff --git a/internal/mode/static/policies/observability/generator.go b/internal/mode/static/policies/observability/generator.go deleted file mode 100644 index 046881ca6..000000000 --- a/internal/mode/static/policies/observability/generator.go +++ /dev/null @@ -1,73 +0,0 @@ -package observability - -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/policies" -) - -var tmpl = template.Must(template.New("observability policy").Parse(observabilityTemplate)) - -const observabilityTemplate = ` -{{- if .Tracing }} -otel_trace {{ .Strategy }}; - {{- if .Tracing.Context }} -otel_trace_context {{ .Tracing.Context }}; - {{- end }} - {{- if .Tracing.SpanName }} -otel_span_name "{{ .Tracing.SpanName }}"; - {{- end }} - {{- range $attr := .Tracing.SpanAttributes }} -otel_span_attr "{{ $attr.Key }}" "{{ $attr.Value }}"; - {{- end }} - {{- range $attr := .GlobalSpanAttributes }} -otel_span_attr "{{ $attr.Key }}" "{{ $attr.Value }}"; - {{- end }} -{{- end }} -` - -// Generate generates configuration as []byte for an ObservabilityPolicy. -func Generate(policy policies.Policy, globalSettings *policies.GlobalSettings) []byte { - obs := helpers.MustCastObject[*ngfAPI.ObservabilityPolicy](policy) - - var strategy string - if obs.Spec.Tracing != nil { - switch obs.Spec.Tracing.Strategy { - case ngfAPI.TraceStrategyParent: - strategy = "$otel_parent_sampled" - case ngfAPI.TraceStrategyRatio: - strategy = "on" - if obs.Spec.Tracing.Ratio != nil { - if *obs.Spec.Tracing.Ratio > 0 { - strategy = CreateRatioVarName(obs) - } else { - strategy = "off" - } - } - default: - strategy = "off" - } - } - - var spanAttributes []ngfAPI.SpanAttribute - if globalSettings != nil { - spanAttributes = globalSettings.TracingSpanAttributes - } - - fields := map[string]interface{}{ - "Tracing": obs.Spec.Tracing, - "Strategy": strategy, - "GlobalSpanAttributes": spanAttributes, - } - - return helpers.MustExecuteTemplate(tmpl, fields) -} - -// CreateRatioVarName builds a variable name for an ObservabilityPolicy to be used with -// ratio-based trace sampling. -func CreateRatioVarName(policy *ngfAPI.ObservabilityPolicy) string { - return fmt.Sprintf("$otel_ratio_%d", *policy.Spec.Tracing.Ratio) -} diff --git a/internal/mode/static/policies/observability/generator_test.go b/internal/mode/static/policies/observability/generator_test.go deleted file mode 100644 index 0a5d33d2e..000000000 --- a/internal/mode/static/policies/observability/generator_test.go +++ /dev/null @@ -1,214 +0,0 @@ -package observability_test - -import ( - "testing" - - . "github.com/onsi/gomega" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - 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/observability" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes" -) - -func TestGenerate(t *testing.T) { - ratio := helpers.GetPointer[int32](25) - zeroRatio := helpers.GetPointer[int32](0) - context := helpers.GetPointer[ngfAPI.TraceContext](ngfAPI.TraceContextExtract) - spanName := helpers.GetPointer("my-span") - - tests := []struct { - name string - policy policies.Policy - globalSettings *policies.GlobalSettings - expStrings []string - }{ - { - name: "strategy set to default ratio", - policy: &ngfAPI.ObservabilityPolicy{ - Spec: ngfAPI.ObservabilityPolicySpec{ - Tracing: &ngfAPI.Tracing{ - Strategy: ngfAPI.TraceStrategyRatio, - }, - }, - }, - expStrings: []string{ - "otel_trace on;", - }, - }, - { - name: "strategy set to custom ratio", - policy: &ngfAPI.ObservabilityPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-policy", - Namespace: "test-namespace", - }, - Spec: ngfAPI.ObservabilityPolicySpec{ - Tracing: &ngfAPI.Tracing{ - Strategy: ngfAPI.TraceStrategyRatio, - Ratio: ratio, - }, - }, - }, - expStrings: []string{ - "otel_trace $otel_ratio_25;", - }, - }, - { - name: "strategy set to zero ratio", - policy: &ngfAPI.ObservabilityPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-policy", - Namespace: "test-namespace", - }, - Spec: ngfAPI.ObservabilityPolicySpec{ - Tracing: &ngfAPI.Tracing{ - Strategy: ngfAPI.TraceStrategyRatio, - Ratio: zeroRatio, - }, - }, - }, - expStrings: []string{ - "otel_trace off;", - }, - }, - { - name: "strategy set to parent", - policy: &ngfAPI.ObservabilityPolicy{ - Spec: ngfAPI.ObservabilityPolicySpec{ - Tracing: &ngfAPI.Tracing{ - Strategy: ngfAPI.TraceStrategyParent, - }, - }, - }, - expStrings: []string{ - "otel_trace $otel_parent_sampled;", - }, - }, - { - name: "context is set", - policy: &ngfAPI.ObservabilityPolicy{ - Spec: ngfAPI.ObservabilityPolicySpec{ - Tracing: &ngfAPI.Tracing{ - Context: context, - }, - }, - }, - expStrings: []string{ - "otel_trace off;", - "otel_trace_context extract;", - }, - }, - { - name: "spanName is set", - policy: &ngfAPI.ObservabilityPolicy{ - Spec: ngfAPI.ObservabilityPolicySpec{ - Tracing: &ngfAPI.Tracing{ - SpanName: spanName, - }, - }, - }, - expStrings: []string{ - "otel_trace off;", - "otel_span_name \"my-span\";", - }, - }, - { - name: "span attributes set", - policy: &ngfAPI.ObservabilityPolicy{ - Spec: ngfAPI.ObservabilityPolicySpec{ - Tracing: &ngfAPI.Tracing{ - SpanAttributes: []ngfAPI.SpanAttribute{ - {Key: "test-key", Value: "test-value"}, - }, - }, - }, - }, - expStrings: []string{ - "otel_trace off;", - "otel_span_attr \"test-key\" \"test-value\";", - }, - }, - { - name: "global span attributes set", - policy: &ngfAPI.ObservabilityPolicy{ - Spec: ngfAPI.ObservabilityPolicySpec{ - Tracing: &ngfAPI.Tracing{}, - }, - }, - globalSettings: &policies.GlobalSettings{ - TracingSpanAttributes: []ngfAPI.SpanAttribute{ - {Key: "test-global-key", Value: "test-global-value"}, - }, - }, - expStrings: []string{ - "otel_trace off;", - "otel_span_attr \"test-global-key\" \"test-global-value\";", - }, - }, - { - name: "all fields populated", - policy: &ngfAPI.ObservabilityPolicy{ - Spec: ngfAPI.ObservabilityPolicySpec{ - Tracing: &ngfAPI.Tracing{ - Strategy: ngfAPI.TraceStrategyRatio, - Context: context, - SpanName: spanName, - SpanAttributes: []ngfAPI.SpanAttribute{ - {Key: "test-key", Value: "test-value"}, - }, - }, - }, - }, - globalSettings: &policies.GlobalSettings{ - TracingSpanAttributes: []ngfAPI.SpanAttribute{ - {Key: "test-global-key", Value: "test-global-value"}, - }, - }, - expStrings: []string{ - "otel_trace on;", - "otel_trace_context extract;", - "otel_span_name \"my-span\";", - "otel_span_attr \"test-key\" \"test-value\";", - "otel_span_attr \"test-global-key\" \"test-global-value\";", - }, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewWithT(t) - - cfgString := string(observability.Generate(test.policy, test.globalSettings)) - - for _, str := range test.expStrings { - g.Expect(cfgString).To(ContainSubstring(str)) - } - }) - } -} - -func TestGeneratePanics(t *testing.T) { - g := NewWithT(t) - - generate := func() { - observability.Generate(&policiesfakes.FakePolicy{}, nil) - } - - g.Expect(generate).To(Panic()) -} - -func TestCreateRatioVarName(t *testing.T) { - pol := &ngfAPI.ObservabilityPolicy{ - Spec: ngfAPI.ObservabilityPolicySpec{ - Tracing: &ngfAPI.Tracing{ - Ratio: helpers.GetPointer[int32](25), - }, - }, - } - - g := NewWithT(t) - g.Expect(observability.CreateRatioVarName(pol)).To(Equal("$otel_ratio_25")) -} diff --git a/internal/mode/static/policies/policiesfakes/fake_config_generator.go b/internal/mode/static/policies/policiesfakes/fake_config_generator.go deleted file mode 100644 index 17640f029..000000000 --- a/internal/mode/static/policies/policiesfakes/fake_config_generator.go +++ /dev/null @@ -1,113 +0,0 @@ -// Code generated by counterfeiter. DO NOT EDIT. -package policiesfakes - -import ( - "sync" - - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" -) - -type FakeConfigGenerator struct { - GenerateStub func(policies.Policy, *policies.GlobalSettings) []byte - generateMutex sync.RWMutex - generateArgsForCall []struct { - arg1 policies.Policy - arg2 *policies.GlobalSettings - } - generateReturns struct { - result1 []byte - } - generateReturnsOnCall map[int]struct { - result1 []byte - } - invocations map[string][][]interface{} - invocationsMutex sync.RWMutex -} - -func (fake *FakeConfigGenerator) Generate(arg1 policies.Policy, arg2 *policies.GlobalSettings) []byte { - fake.generateMutex.Lock() - ret, specificReturn := fake.generateReturnsOnCall[len(fake.generateArgsForCall)] - fake.generateArgsForCall = append(fake.generateArgsForCall, struct { - arg1 policies.Policy - arg2 *policies.GlobalSettings - }{arg1, arg2}) - stub := fake.GenerateStub - fakeReturns := fake.generateReturns - fake.recordInvocation("Generate", []interface{}{arg1, arg2}) - fake.generateMutex.Unlock() - if stub != nil { - return stub(arg1, arg2) - } - if specificReturn { - return ret.result1 - } - return fakeReturns.result1 -} - -func (fake *FakeConfigGenerator) GenerateCallCount() int { - fake.generateMutex.RLock() - defer fake.generateMutex.RUnlock() - return len(fake.generateArgsForCall) -} - -func (fake *FakeConfigGenerator) GenerateCalls(stub func(policies.Policy, *policies.GlobalSettings) []byte) { - fake.generateMutex.Lock() - defer fake.generateMutex.Unlock() - fake.GenerateStub = stub -} - -func (fake *FakeConfigGenerator) GenerateArgsForCall(i int) (policies.Policy, *policies.GlobalSettings) { - fake.generateMutex.RLock() - defer fake.generateMutex.RUnlock() - argsForCall := fake.generateArgsForCall[i] - return argsForCall.arg1, argsForCall.arg2 -} - -func (fake *FakeConfigGenerator) GenerateReturns(result1 []byte) { - fake.generateMutex.Lock() - defer fake.generateMutex.Unlock() - fake.GenerateStub = nil - fake.generateReturns = struct { - result1 []byte - }{result1} -} - -func (fake *FakeConfigGenerator) GenerateReturnsOnCall(i int, result1 []byte) { - fake.generateMutex.Lock() - defer fake.generateMutex.Unlock() - fake.GenerateStub = nil - if fake.generateReturnsOnCall == nil { - fake.generateReturnsOnCall = make(map[int]struct { - result1 []byte - }) - } - fake.generateReturnsOnCall[i] = struct { - result1 []byte - }{result1} -} - -func (fake *FakeConfigGenerator) Invocations() map[string][][]interface{} { - fake.invocationsMutex.RLock() - defer fake.invocationsMutex.RUnlock() - fake.generateMutex.RLock() - defer fake.generateMutex.RUnlock() - copiedInvocations := map[string][][]interface{}{} - for key, value := range fake.invocations { - copiedInvocations[key] = value - } - return copiedInvocations -} - -func (fake *FakeConfigGenerator) recordInvocation(key string, args []interface{}) { - fake.invocationsMutex.Lock() - defer fake.invocationsMutex.Unlock() - if fake.invocations == nil { - fake.invocations = map[string][][]interface{}{} - } - if fake.invocations[key] == nil { - fake.invocations[key] = [][]interface{}{} - } - fake.invocations[key] = append(fake.invocations[key], args) -} - -var _ policies.ConfigGenerator = new(FakeConfigGenerator) diff --git a/internal/mode/static/state/change_processor.go b/internal/mode/static/state/change_processor.go index 3fdcd55d9..40a3723df 100644 --- a/internal/mode/static/state/change_processor.go +++ b/internal/mode/static/state/change_processor.go @@ -19,7 +19,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/gatewayclass" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index 3e1455b2c..eff60594b 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -431,6 +431,7 @@ var _ = Describe("ChangeProcessor", func() { Attachment: &graph.ParentRefAttachmentStatus{ AcceptedHostnames: map[string][]string{"listener-80-1": {"foo.example.com"}}, Attached: true, + ListenerPort: 80, }, Gateway: types.NamespacedName{Namespace: "test", Name: "gateway-1"}, SectionName: hr1.Spec.ParentRefs[0].SectionName, @@ -439,6 +440,7 @@ var _ = Describe("ChangeProcessor", func() { Attachment: &graph.ParentRefAttachmentStatus{ AcceptedHostnames: map[string][]string{"listener-443-1": {"foo.example.com"}}, Attached: true, + ListenerPort: 443, }, Gateway: types.NamespacedName{Namespace: "test", Name: "gateway-1"}, Idx: 1, @@ -479,6 +481,7 @@ var _ = Describe("ChangeProcessor", func() { Attachment: &graph.ParentRefAttachmentStatus{ AcceptedHostnames: map[string][]string{"listener-80-1": {"bar.example.com"}}, Attached: true, + ListenerPort: 80, }, Gateway: types.NamespacedName{Namespace: "test", Name: "gateway-2"}, SectionName: hr2.Spec.ParentRefs[0].SectionName, @@ -487,6 +490,7 @@ var _ = Describe("ChangeProcessor", func() { Attachment: &graph.ParentRefAttachmentStatus{ AcceptedHostnames: map[string][]string{"listener-443-1": {"bar.example.com"}}, Attached: true, + ListenerPort: 443, }, Gateway: types.NamespacedName{Namespace: "test", Name: "gateway-2"}, Idx: 1, @@ -650,14 +654,16 @@ var _ = Describe("ChangeProcessor", func() { AcceptedHostnames: map[string][]string{ "listener-80-1": {"foo.example.com"}, }, - Attached: true, + Attached: true, + ListenerPort: 80, } expAttachment443 := &graph.ParentRefAttachmentStatus{ AcceptedHostnames: map[string][]string{ "listener-443-1": {"foo.example.com"}, }, - Attached: true, + Attached: true, + ListenerPort: 443, } listener80 := getListenerByName(expGraph.Gateway, "listener-80-1") diff --git a/internal/mode/static/state/conditions/conditions.go b/internal/mode/static/state/conditions/conditions.go index 5b075a644..457f9ad27 100644 --- a/internal/mode/static/state/conditions/conditions.go +++ b/internal/mode/static/state/conditions/conditions.go @@ -91,6 +91,10 @@ const ( // when telemetry is not enabled in the NginxProxy resource. PolicyMessageTelemetryNotEnabled = "Telemetry is not enabled in the NginxProxy resource" + // PolicyReasonTargetConflict is used with the "PolicyAccepted" condition when a Route that it targets + // has an overlapping hostname:port/path combination with another Route. + PolicyReasonTargetConflict v1alpha2.PolicyConditionReason = "TargetConflict" + // GatewayIgnoredReason is used with v1.RouteConditionAccepted when the route references a Gateway that is ignored // by NGF. GatewayIgnoredReason v1.RouteConditionReason = "GatewayIgnored" @@ -673,6 +677,17 @@ func NewPolicyTargetNotFound(msg string) conditions.Condition { } } +// 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) conditions.Condition { + return conditions.Condition{ + Type: string(v1alpha2.PolicyConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(PolicyReasonTargetConflict), + Message: msg, + } +} + // NewPolicyNotAcceptedNginxProxyNotSet returns a Condition that indicates that the Policy is not accepted // because it relies in the NginxProxy configuration which is missing or invalid. func NewPolicyNotAcceptedNginxProxyNotSet(msg string) conditions.Condition { diff --git a/internal/mode/static/state/dataplane/configuration.go b/internal/mode/static/state/dataplane/configuration.go index b81dbfe7a..39dc5d63f 100644 --- a/internal/mode/static/state/dataplane/configuration.go +++ b/internal/mode/static/state/dataplane/configuration.go @@ -15,8 +15,7 @@ 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/observability" + policies "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" ) @@ -31,7 +30,6 @@ func BuildConfiguration( ctx context.Context, g *graph.Graph, resolver resolver.ServiceResolver, - generator policies.ConfigGenerator, configVersion int, ) Configuration { if g.GatewayClass == nil || !g.GatewayClass.Valid { @@ -44,7 +42,7 @@ func BuildConfiguration( baseHTTPConfig := buildBaseHTTPConfig(g) upstreams := buildUpstreams(ctx, g.Gateway.Listeners, resolver, baseHTTPConfig.IPFamily) - httpServers, sslServers := buildServers(g, generator) + httpServers, sslServers := buildServers(g) backendGroups := buildBackendGroups(append(httpServers, sslServers...)) keyPairs := buildSSLKeyPairs(g.ReferencedSecrets, g.Gateway.Listeners) certBundles := buildCertBundles(g.ReferencedCaCertConfigMaps, backendGroups) @@ -205,7 +203,7 @@ func convertBackendTLS(btp *graph.BackendTLSPolicy) *VerifyTLS { return verify } -func buildServers(g *graph.Graph, generator policies.ConfigGenerator) (http, ssl []VirtualServer) { +func buildServers(g *graph.Graph) (http, ssl []VirtualServer) { rulesForProtocol := map[v1.ProtocolType]portPathRules{ v1.HTTPProtocolType: make(portPathRules), v1.HTTPSProtocolType: make(portPathRules), @@ -215,11 +213,11 @@ func buildServers(g *graph.Graph, generator policies.ConfigGenerator) (http, ssl if l.Valid { rules := rulesForProtocol[l.Source.Protocol][l.Source.Port] if rules == nil { - rules = newHostPathRules(generator) + rules = newHostPathRules() rulesForProtocol[l.Source.Protocol][l.Source.Port] = rules } - rules.upsertListener(l, g.GlobalSettings) + rules.upsertListener(l) } } @@ -228,14 +226,14 @@ func buildServers(g *graph.Graph, generator policies.ConfigGenerator) (http, ssl httpServers, sslServers := httpRules.buildServers(), sslRules.buildServers() - additions := buildAdditions(g.Gateway.Policies, g.GlobalSettings, generator) + pols := buildPolicies(g.Gateway.Policies) for i := range httpServers { - httpServers[i].Additions = additions + httpServers[i].Policies = pols } for i := range sslServers { - sslServers[i].Additions = additions + sslServers[i].Policies = pols } return httpServers, sslServers @@ -265,7 +263,6 @@ type pathAndType struct { } type hostPathRules struct { - generator policies.ConfigGenerator rulesPerHost map[string]map[pathAndType]PathRule listenersForHost map[string]*graph.Listener httpsListeners []*graph.Listener @@ -273,16 +270,15 @@ type hostPathRules struct { listenersExist bool } -func newHostPathRules(generator policies.ConfigGenerator) *hostPathRules { +func newHostPathRules() *hostPathRules { return &hostPathRules{ rulesPerHost: make(map[string]map[pathAndType]PathRule), listenersForHost: make(map[string]*graph.Listener), httpsListeners: make([]*graph.Listener, 0), - generator: generator, } } -func (hpr *hostPathRules) upsertListener(l *graph.Listener, globalSettings *policies.GlobalSettings) { +func (hpr *hostPathRules) upsertListener(l *graph.Listener) { hpr.listenersExist = true hpr.port = int32(l.Source.Port) @@ -295,14 +291,13 @@ func (hpr *hostPathRules) upsertListener(l *graph.Listener, globalSettings *poli continue } - hpr.upsertRoute(r, l, globalSettings) + hpr.upsertRoute(r, l) } } func (hpr *hostPathRules) upsertRoute( route *graph.L7Route, listener *graph.Listener, - globalSettings *policies.GlobalSettings, ) { var hostnames []string GRPC := route.RouteType == graph.RouteTypeGRPC @@ -350,7 +345,7 @@ func (hpr *hostPathRules) upsertRoute( } } - additions := buildAdditions(route.Policies, globalSettings, hpr.generator) + pols := buildPolicies(route.Policies) for _, h := range hostnames { for _, m := range rule.Matches { @@ -370,13 +365,13 @@ func (hpr *hostPathRules) upsertRoute( routeNsName := client.ObjectKeyFromObject(route.Source) hostRule.GRPC = GRPC + hostRule.Policies = append(hostRule.Policies, pols...) hostRule.MatchRules = append(hostRule.MatchRules, MatchRule{ Source: objectSrc, BackendGroup: newBackendGroup(rule.BackendRefs, routeNsName, i), Filters: filters, Match: convertMatch(m), - Additions: additions, }) hpr.rulesPerHost[h][key] = hostRule @@ -651,6 +646,8 @@ func buildTelemetry(g *graph.Graph) Telemetry { tel.Interval = string(*telemetry.Exporter.Interval) } + tel.SpanAttributes = setSpanAttributes(telemetry.SpanAttributes) + // FIXME(sberman): https://github.com/nginxinc/nginx-gateway-fabric/issues/2038 // Find a generic way to include relevant policy info at the http context so we don't need policy-specific // logic in this function @@ -658,7 +655,7 @@ func buildTelemetry(g *graph.Graph) Telemetry { for _, pol := range g.NGFPolicies { if obsPol, ok := pol.Source.(*ngfAPI.ObservabilityPolicy); ok { if obsPol.Spec.Tracing != nil && obsPol.Spec.Tracing.Ratio != nil && *obsPol.Spec.Tracing.Ratio > 0 { - ratioName := observability.CreateRatioVarName(obsPol) + ratioName := CreateRatioVarName(*obsPol.Spec.Tracing.Ratio) ratioMap[ratioName] = *obsPol.Spec.Tracing.Ratio } } @@ -672,6 +669,25 @@ func buildTelemetry(g *graph.Graph) Telemetry { return tel } +func setSpanAttributes(spanAttributes []ngfAPI.SpanAttribute) []SpanAttribute { + spanAttrs := make([]SpanAttribute, 0, len(spanAttributes)) + for _, spanAttr := range spanAttributes { + sa := SpanAttribute{ + Key: spanAttr.Key, + Value: spanAttr.Value, + } + spanAttrs = append(spanAttrs, sa) + } + + return spanAttrs +} + +// CreateRatioVarName builds a variable name for an ObservabilityPolicy to be used with +// ratio-based trace sampling. +func CreateRatioVarName(ratio int32) string { + return fmt.Sprintf("$otel_ratio_%d", ratio) +} + // buildBaseHTTPConfig generates the base http context config that should be applied to all servers. func buildBaseHTTPConfig(g *graph.Graph) BaseHTTPConfig { baseConfig := BaseHTTPConfig{ @@ -699,32 +715,20 @@ func buildBaseHTTPConfig(g *graph.Graph) BaseHTTPConfig { return baseConfig } -func buildAdditions( - policies []*graph.Policy, - globalSettings *policies.GlobalSettings, - generator policies.ConfigGenerator, -) []Addition { - if len(policies) == 0 { +func buildPolicies(graphPolicies []*graph.Policy) []policies.Policy { + if len(graphPolicies) == 0 { return nil } - additions := make([]Addition, 0, len(policies)) + finalPolicies := make([]policies.Policy, 0, len(graphPolicies)) - for _, policy := range policies { + for _, policy := range graphPolicies { if !policy.Valid { continue } - additions = append(additions, Addition{ - Bytes: generator.Generate(policy.Source, globalSettings), - Identifier: fmt.Sprintf( - "%s_%s_%s", - policy.Source.GetObjectKind().GroupVersionKind().Kind, - policy.Source.GetNamespace(), - policy.Source.GetName(), - ), - }) + finalPolicies = append(finalPolicies, policy.Source) } - return additions + return finalPolicies } diff --git a/internal/mode/static/state/dataplane/configuration_test.go b/internal/mode/static/state/dataplane/configuration_test.go index 83d5873ce..61e574260 100644 --- a/internal/mode/static/state/dataplane/configuration_test.go +++ b/internal/mode/static/state/dataplane/configuration_test.go @@ -20,8 +20,8 @@ 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/policiesfakes" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" + policiesfakes "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver/resolverfakes" @@ -39,10 +39,12 @@ func getNormalBackendRef() graph.BackendRef { func getExpectedConfiguration() Configuration { return Configuration{ BaseHTTPConfig: BaseHTTPConfig{HTTP2: true, IPFamily: Dual}, - HTTPServers: []VirtualServer{{ - IsDefault: true, - Port: 80, - }}, + HTTPServers: []VirtualServer{ + { + IsDefault: true, + Port: 80, + }, + }, SSLServers: []VirtualServer{ { IsDefault: true, @@ -1846,12 +1848,13 @@ func TestBuildConfiguration(t *testing.T) { conf.SSLServers = []VirtualServer{} conf.SSLKeyPairs = map[SSLKeyPairID]SSLKeyPair{} conf.Telemetry = Telemetry{ - Endpoint: "my-otel.svc:4563", - Interval: "5s", - BatchSize: 512, - BatchCount: 4, - ServiceName: "ngf:ns:gw:my-svc", - Ratios: []Ratio{}, + Endpoint: "my-otel.svc:4563", + Interval: "5s", + BatchSize: 512, + BatchCount: 4, + ServiceName: "ngf:ns:gw:my-svc", + Ratios: []Ratio{}, + SpanAttributes: []SpanAttribute{}, } conf.BaseHTTPConfig = BaseHTTPConfig{HTTP2: false, IPFamily: Dual} return conf @@ -1929,16 +1932,7 @@ func TestBuildConfiguration(t *testing.T) { { IsDefault: true, Port: 443, - Additions: []Addition{ - { - Bytes: []byte("apple"), - Identifier: "ApplePolicy_default_attach-gw", - }, - { - Bytes: []byte("orange"), - Identifier: "OrangePolicy_default_attach-gw", - }, - }, + Policies: []policies.Policy{gwPolicy1.Source, gwPolicy2.Source}, }, { Hostname: "policy.com", @@ -1950,59 +1944,27 @@ func TestBuildConfiguration(t *testing.T) { { BackendGroup: expHTTPSHRWithPolicyGroups[0], Source: &httpsHRWithPolicy.ObjectMeta, - Additions: []Addition{ - { - Bytes: []byte("lime"), - Identifier: "LimePolicy_default_attach-hr", - }, - }, }, }, + Policies: []policies.Policy{hrPolicy2.Source}, }, }, - SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, - Port: 443, - Additions: []Addition{ - { - Bytes: []byte("apple"), - Identifier: "ApplePolicy_default_attach-gw", - }, - { - Bytes: []byte("orange"), - Identifier: "OrangePolicy_default_attach-gw", - }, - }, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, + Port: 443, + Policies: []policies.Policy{gwPolicy1.Source, gwPolicy2.Source}, }, { Hostname: wildcardHostname, SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, Port: 443, - Additions: []Addition{ - { - Bytes: []byte("apple"), - Identifier: "ApplePolicy_default_attach-gw", - }, - { - Bytes: []byte("orange"), - Identifier: "OrangePolicy_default_attach-gw", - }, - }, + Policies: []policies.Policy{gwPolicy1.Source, gwPolicy2.Source}, }, } conf.HTTPServers = []VirtualServer{ { IsDefault: true, Port: 80, - Additions: []Addition{ - { - Bytes: []byte("apple"), - Identifier: "ApplePolicy_default_attach-gw", - }, - { - Bytes: []byte("orange"), - Identifier: "OrangePolicy_default_attach-gw", - }, - }, + Policies: []policies.Policy{gwPolicy1.Source, gwPolicy2.Source}, }, { Hostname: "policy.com", @@ -2014,27 +1976,13 @@ func TestBuildConfiguration(t *testing.T) { { Source: &hrWithPolicy.ObjectMeta, BackendGroup: expHRWithPolicyGroups[0], - Additions: []Addition{ - { - Bytes: []byte("lemon"), - Identifier: "LemonPolicy_default_attach-hr", - }, - }, }, }, + Policies: []policies.Policy{hrPolicy1.Source}, }, }, - Port: 80, - Additions: []Addition{ - { - Bytes: []byte("apple"), - Identifier: "ApplePolicy_default_attach-gw", - }, - { - Bytes: []byte("orange"), - Identifier: "OrangePolicy_default_attach-gw", - }, - }, + Port: 80, + Policies: []policies.Policy{gwPolicy1.Source, gwPolicy2.Source}, }, } conf.Upstreams = []Upstream{fooUpstream} @@ -2095,28 +2043,10 @@ func TestBuildConfiguration(t *testing.T) { t.Run(test.msg, func(t *testing.T) { g := NewWithT(t) - fakeGenerator := &policiesfakes.FakeConfigGenerator{ - GenerateStub: func(p policies.Policy, _ *policies.GlobalSettings) []byte { - switch kind := p.GetObjectKind().GroupVersionKind().Kind; kind { - case "ApplePolicy": - return []byte("apple") - case "OrangePolicy": - return []byte("orange") - case "LemonPolicy": - return []byte("lemon") - case "LimePolicy": - return []byte("lime") - default: - panic(fmt.Sprintf("unknown policy kind: %s", kind)) - } - }, - } - result := BuildConfiguration( context.TODO(), test.graph, fakeResolver, - fakeGenerator, 1, ) @@ -2891,6 +2821,9 @@ func TestBuildTelemetry(t *testing.T) { BatchSize: 512, BatchCount: 4, Ratios: []Ratio{}, + SpanAttributes: []SpanAttribute{ + {Key: "key", Value: "value"}, + }, } } @@ -3092,7 +3025,7 @@ func TestBuildTelemetry(t *testing.T) { } } -func TestBuildAdditions(t *testing.T) { +func TestBuildPolicies(t *testing.T) { getPolicy := func(kind, name string) policies.Policy { return &policiesfakes.FakePolicy{ GetNameStub: func() string { @@ -3114,14 +3047,14 @@ func TestBuildAdditions(t *testing.T) { } tests := []struct { - name string - policies []*graph.Policy - expAdditions []Addition + name string + policies []*graph.Policy + expPolicies []string }{ { - name: "nil policies", - policies: nil, - expAdditions: nil, + name: "nil policies", + policies: nil, + expPolicies: nil, }, { name: "mix of valid and invalid policies", @@ -3147,19 +3080,10 @@ func TestBuildAdditions(t *testing.T) { Valid: true, }, }, - expAdditions: []Addition{ - { - Identifier: "Kind1_test_valid1", - Bytes: []byte("valid1"), - }, - { - Identifier: "Kind2_test_valid2", - Bytes: []byte("valid2"), - }, - { - Identifier: "Kind3_test_valid3", - Bytes: []byte("valid3"), - }, + expPolicies: []string{ + "valid1", + "valid2", + "valid3", }, }, } @@ -3168,14 +3092,11 @@ func TestBuildAdditions(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - generator := &policiesfakes.FakeConfigGenerator{ - GenerateStub: func(policy policies.Policy, _ *policies.GlobalSettings) []byte { - return []byte(policy.GetName()) - }, + pols := buildPolicies(test.policies) + g.Expect(pols).To(HaveLen(len(test.expPolicies))) + for _, pol := range pols { + g.Expect(test.expPolicies).To(ContainElement(pol.GetName())) } - - additions := buildAdditions(test.policies, nil, generator) - g.Expect(additions).To(BeEquivalentTo(test.expAdditions)) }) } } @@ -3215,3 +3136,8 @@ func TestGetAllowedAddressType(t *testing.T) { }) } } + +func TestCreateRatioVarName(t *testing.T) { + g := NewWithT(t) + g.Expect(CreateRatioVarName(25)).To(Equal("$otel_ratio_25")) +} diff --git a/internal/mode/static/state/dataplane/types.go b/internal/mode/static/state/dataplane/types.go index d342ff3b0..c509dbc3c 100644 --- a/internal/mode/static/state/dataplane/types.go +++ b/internal/mode/static/state/dataplane/types.go @@ -6,6 +6,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" ) @@ -68,22 +69,14 @@ type VirtualServer struct { Hostname string // PathRules is a collection of routing rules. PathRules []PathRule - // Additions is a list of config additions for the server. - Additions []Addition + // Policies is a list of Policies that apply to the server. + Policies []policies.Policy // Port is the port of the server. Port int32 // IsDefault indicates whether the server is the default server. IsDefault bool } -// Addition holds additional configuration. -type Addition struct { - // Identifier is a unique identifier for the addition. - Identifier string - // Bytes contains the additional configuration. - Bytes []byte -} - // Upstream is a pool of endpoints to be load balanced. type Upstream struct { // Name is the name of the Upstream. Will be unique for each service/port combination. @@ -108,6 +101,8 @@ type PathRule struct { PathType PathType // MatchRules holds routing rules. MatchRules []MatchRule + // Policies contains the list of policies that are applied to this PathRule. + Policies []policies.Policy // GRPC indicates if this is a gRPC rule GRPC bool } @@ -214,8 +209,6 @@ type MatchRule struct { Source *metav1.ObjectMeta // Match holds the match for the rule. Match Match - // Additions holds the config additions for the rule. - Additions []Addition // BackendGroup is the group of Backends that the rule routes to. BackendGroup BackendGroup } @@ -270,7 +263,7 @@ type VerifyTLS struct { RootCAPath string } -// Telemetry represents Otel configuration for the dataplane. +// Telemetry represents global Otel configuration for the dataplane. type Telemetry struct { // Endpoint specifies the address of OTLP/gRPC endpoint that will accept telemetry data. Endpoint string @@ -280,6 +273,8 @@ type Telemetry struct { Interval string // Ratios is a list of tracing sampling ratios. Ratios []Ratio + // SpanAttributes are global custom key/value attributes that are added to each span. + SpanAttributes []SpanAttribute // BatchSize specifies the maximum number of spans to be sent in one batch per worker. BatchSize int32 // BatchCount specifies the number of pending batches per worker, spans exceeding the limit are dropped. diff --git a/internal/mode/static/state/graph/graph.go b/internal/mode/static/state/graph/graph.go index d874d5d2c..a954d0a89 100644 --- a/internal/mode/static/state/graph/graph.go +++ b/internal/mode/static/state/graph/graph.go @@ -16,7 +16,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/index" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) @@ -194,10 +194,6 @@ func BuildGraph( NginxProxyValid: npCfg.Valid, TelemetryEnabled: spec.Telemetry != nil && spec.Telemetry.Exporter != nil, } - - if spec.Telemetry != nil { - globalSettings.TracingSpanAttributes = spec.Telemetry.SpanAttributes - } } secretResolver := newSecretResolver(state.Secrets) diff --git a/internal/mode/static/state/graph/graph_test.go b/internal/mode/static/state/graph/graph_test.go index 15d1afde1..896aa231e 100644 --- a/internal/mode/static/state/graph/graph_test.go +++ b/internal/mode/static/state/graph/graph_test.go @@ -21,8 +21,8 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/controller/index" "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/policies/policiesfakes" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation/validationfakes" @@ -187,7 +187,7 @@ func TestBuildGraph(t *testing.T) { }, }, Hostnames: []gatewayv1.Hostname{ - "foo.example.com", + "bar.example.com", }, Rules: []gatewayv1.GRPCRouteRule{ { @@ -478,6 +478,7 @@ func TestBuildGraph(t *testing.T) { Attachment: &ParentRefAttachmentStatus{ Attached: true, AcceptedHostnames: map[string][]string{"listener-80-1": {"foo.example.com"}}, + ListenerPort: 80, }, }, }, @@ -500,7 +501,8 @@ func TestBuildGraph(t *testing.T) { SectionName: gr.Spec.ParentRefs[0].SectionName, Attachment: &ParentRefAttachmentStatus{ Attached: true, - AcceptedHostnames: map[string][]string{"listener-80-1": {"foo.example.com"}}, + AcceptedHostnames: map[string][]string{"listener-80-1": {"bar.example.com"}}, + ListenerPort: 80, }, }, }, @@ -525,6 +527,7 @@ func TestBuildGraph(t *testing.T) { Attachment: &ParentRefAttachmentStatus{ Attached: true, AcceptedHostnames: map[string][]string{"listener-443-1": {"foo.example.com"}}, + ListenerPort: 443, }, }, }, @@ -613,9 +616,6 @@ func TestBuildGraph(t *testing.T) { GlobalSettings: &policies.GlobalSettings{ NginxProxyValid: true, TelemetryEnabled: true, - TracingSpanAttributes: []ngfAPI.SpanAttribute{ - {Key: "key", Value: "value"}, - }, }, } } diff --git a/internal/mode/static/state/graph/httproute.go b/internal/mode/static/state/graph/httproute.go index cac85228c..79f05f9e3 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -2,11 +2,13 @@ package graph import ( "fmt" + "strings" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/validation/field" v1 "sigs.k8s.io/gateway-api/apis/v1" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) @@ -225,6 +227,11 @@ func validatePathMatch( return field.ErrorList{field.Required(fieldPath.Child("value"), "path value cannot be nil")} } + if strings.HasPrefix(*path.Value, http.InternalRoutePathPrefix) { + msg := fmt.Sprintf("path cannot start with %s", http.InternalRoutePathPrefix) + return field.ErrorList{field.Invalid(fieldPath.Child("value"), *path.Value, msg)} + } + if *path.Type != v1.PathMatchPathPrefix && *path.Type != v1.PathMatchExact { valErr := field.NotSupported(fieldPath.Child("type"), *path.Type, []string{string(v1.PathMatchExact), string(v1.PathMatchPathPrefix)}) diff --git a/internal/mode/static/state/graph/httproute_test.go b/internal/mode/static/state/graph/httproute_test.go index 4686908a2..68d58a40d 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -649,6 +649,17 @@ func TestValidateMatch(t *testing.T) { expectErrCount: 1, name: "wrong path type", }, + { + validator: createAllValidValidator(), + match: gatewayv1.HTTPRouteMatch{ + Path: &gatewayv1.HTTPPathMatch{ + Type: helpers.GetPointer(gatewayv1.PathMatchPathPrefix), + Value: helpers.GetPointer("/_ngf-internal-path"), + }, + }, + expectErrCount: 1, + name: "bad path prefix", + }, { validator: func() *validationfakes.FakeHTTPFieldsValidator { validator := createAllValidValidator() diff --git a/internal/mode/static/state/graph/policies.go b/internal/mode/static/state/graph/policies.go index bd4fa29a4..65eb05195 100644 --- a/internal/mode/static/state/graph/policies.go +++ b/internal/mode/static/state/graph/policies.go @@ -11,7 +11,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" "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" ngfsort "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/sort" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" @@ -167,7 +167,10 @@ func processPolicies( processedPolicies := make(map[PolicyKey]*Policy) for key, policy := range policies { + var conds []conditions.Condition + targetRefs := make([]PolicyTargetRef, 0, len(policy.GetTargetRefs())) + targetedRoutes := make(map[types.NamespacedName]*L7Route) for _, ref := range policy.GetTargetRefs() { refNsName := types.NamespacedName{Name: string(ref.Name), Namespace: policy.GetNamespace()} @@ -179,8 +182,10 @@ func processPolicies( continue } case hrGroupKind, grpcGroupKind: - if _, exists := routes[routeKeyForKind(ref.Kind, refNsName)]; !exists { + if route, exists := routes[routeKeyForKind(ref.Kind, refNsName)]; !exists { continue + } else { + targetedRoutes[client.ObjectKeyFromObject(route.Source)] = route } default: continue @@ -198,7 +203,24 @@ func processPolicies( continue } - conds := validator.Validate(policy, globalSettings) + for _, targetedRoute := range targetedRoutes { + // We need to check if this route referenced in the policy has an overlapping + // hostname:port/path with any other route that isn't referenced by this policy. + // If so, deny the policy. + hostPortPaths := buildHostPortPaths(targetedRoute) + + for _, route := range routes { + if _, ok := targetedRoutes[client.ObjectKeyFromObject(route.Source)]; ok { + continue + } + + if cond := checkForRouteOverlap(route, hostPortPaths); cond != nil { + conds = append(conds, *cond) + } + } + } + + conds = append(conds, validator.Validate(policy, globalSettings)...) processedPolicies[key] = &Policy{ Source: policy, @@ -214,6 +236,48 @@ func processPolicies( return processedPolicies } +// checkForRouteOverlap checks if any route references the same hostname:port/path combination +// as a route referenced in a policy. +func checkForRouteOverlap(route *L7Route, hostPortPaths map[string]string) *conditions.Condition { + for _, parentRef := range route.ParentRefs { + if parentRef.Attachment != nil { + port := parentRef.Attachment.ListenerPort + for _, hostname := range parentRef.Attachment.AcceptedHostnames { + for _, rule := range route.Spec.Rules { + for _, match := range rule.Matches { + if match.Path != nil && match.Path.Value != nil { + key := fmt.Sprintf("%s:%d%s", hostname, port, *match.Path.Value) + if val, ok := hostPortPaths[key]; !ok { + hostPortPaths[key] = fmt.Sprintf("%s/%s", route.Source.GetNamespace(), route.Source.GetName()) + } else { + conflictingRouteName := fmt.Sprintf("%s/%s", route.Source.GetNamespace(), route.Source.GetName()) + msg := fmt.Sprintf("Policy cannot be applied to target %q since another "+ + "Route %q shares a hostname:port/path combination with this target", val, conflictingRouteName) + cond := staticConds.NewPolicyNotAcceptedTargetConflict(msg) + + return &cond + } + } + } + } + } + } + } + + return nil +} + +// buildHostPortPaths uses the same logic as checkForRouteOverlap, except it's +// simply initializing the hostPortPaths map with the route that's referenced in the Policy, +// so it doesn't care about the return value. +func buildHostPortPaths(route *L7Route) map[string]string { + hostPortPaths := make(map[string]string) + + checkForRouteOverlap(route, hostPortPaths) + + return hostPortPaths +} + // markConflictedPolicies marks policies that conflict with a policy of greater precedence as invalid. // Policies are sorted by timestamp and then alphabetically. func markConflictedPolicies(policies map[PolicyKey]*Policy, validator validation.PolicyValidator) { diff --git a/internal/mode/static/state/graph/policies_test.go b/internal/mode/static/state/graph/policies_test.go index a555f4f4a..5851abcdc 100644 --- a/internal/mode/static/state/graph/policies_test.go +++ b/internal/mode/static/state/graph/policies_test.go @@ -14,8 +14,8 @@ 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/policies/policiesfakes" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" + policiesfakes "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) @@ -557,16 +557,16 @@ func TestProcessPolicies(t *testing.T) { gatewayWrongGroupRef := createTestRef(kinds.Gateway, "WrongGroup", "gw") nonNGFGatewayRef := createTestRef(kinds.Gateway, v1.GroupName, "not-ours") - pol1, pol1Key := createTestPolicyAndKey(policyGVK, hrRef, "pol1") - pol2, pol2Key := createTestPolicyAndKey(policyGVK, grpcRef, "pol2") - pol3, pol3Key := createTestPolicyAndKey(policyGVK, gatewayRef, "pol3") - pol4, pol4Key := createTestPolicyAndKey(policyGVK, ignoredGatewayRef, "pol4") - pol5, pol5Key := createTestPolicyAndKey(policyGVK, hrDoesNotExistRef, "pol5") - pol6, pol6Key := createTestPolicyAndKey(policyGVK, hrWrongGroup, "pol6") - pol7, pol7Key := createTestPolicyAndKey(policyGVK, gatewayWrongGroupRef, "pol7") - pol8, pol8Key := createTestPolicyAndKey(policyGVK, nonNGFGatewayRef, "pol8") + pol1, pol1Key := createTestPolicyAndKey(policyGVK, "pol1", hrRef) + pol2, pol2Key := createTestPolicyAndKey(policyGVK, "pol2", grpcRef) + pol3, pol3Key := createTestPolicyAndKey(policyGVK, "pol3", gatewayRef) + pol4, pol4Key := createTestPolicyAndKey(policyGVK, "pol4", ignoredGatewayRef) + pol5, pol5Key := createTestPolicyAndKey(policyGVK, "pol5", hrDoesNotExistRef) + pol6, pol6Key := createTestPolicyAndKey(policyGVK, "pol6", hrWrongGroup) + pol7, pol7Key := createTestPolicyAndKey(policyGVK, "pol7", gatewayWrongGroupRef) + pol8, pol8Key := createTestPolicyAndKey(policyGVK, "pol8", nonNGFGatewayRef) - pol1Conflict, pol1ConflictKey := createTestPolicyAndKey(policyGVK, hrRef, "pol1-conflict") + pol1Conflict, pol1ConflictKey := createTestPolicyAndKey(policyGVK, "pol1-conflict", hrRef) allValidValidator := &policiesfakes.FakeValidator{} @@ -753,8 +753,22 @@ func TestProcessPolicies(t *testing.T) { } routes := map[RouteKey]*L7Route{ - {RouteType: RouteTypeHTTP, NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr"}}: {}, - {RouteType: RouteTypeGRPC, NamespacedName: types.NamespacedName{Namespace: testNs, Name: "grpc"}}: {}, + {RouteType: RouteTypeHTTP, NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr"}}: { + Source: &v1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: "hr", + Namespace: testNs, + }, + }, + }, + {RouteType: RouteTypeGRPC, NamespacedName: types.NamespacedName{Namespace: testNs, Name: "grpc"}}: { + Source: &v1.GRPCRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: "grpc", + Namespace: testNs, + }, + }, + }, } for _, test := range tests { @@ -767,6 +781,205 @@ func TestProcessPolicies(t *testing.T) { } } +func TestProcessPolicies_RouteOverlap(t *testing.T) { + hrRefCoffee := createTestRef(kinds.HTTPRoute, v1.GroupName, "hr-coffee") + hrRefCoffeeTea := createTestRef(kinds.HTTPRoute, v1.GroupName, "hr-coffee-tea") + + policyGVK := schema.GroupVersionKind{Group: "Group", Version: "Version", Kind: "MyPolicy"} + pol1, pol1Key := createTestPolicyAndKey(policyGVK, "pol1", hrRefCoffee) + pol2, pol2Key := createTestPolicyAndKey(policyGVK, "pol2", hrRefCoffee, hrRefCoffeeTea) + + tests := []struct { + validator validation.PolicyValidator + policies map[PolicyKey]policies.Policy + routes map[RouteKey]*L7Route + expProcessedPolicies map[PolicyKey]*Policy + name string + }{ + { + name: "no overlap", + validator: &policiesfakes.FakeValidator{}, + policies: map[PolicyKey]policies.Policy{ + pol1Key: pol1, + }, + routes: map[RouteKey]*L7Route{ + { + RouteType: RouteTypeHTTP, + NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"}, + }: createTestRouteWithPaths("hr-coffee", "/coffee"), + { + RouteType: RouteTypeHTTP, + NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr2"}, + }: createTestRouteWithPaths("hr2", "/tea"), + }, + expProcessedPolicies: map[PolicyKey]*Policy{ + pol1Key: { + Source: pol1, + TargetRefs: []PolicyTargetRef{ + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"}, + Kind: kinds.HTTPRoute, + Group: v1.GroupName, + }, + }, + Ancestors: []PolicyAncestor{}, + Valid: true, + }, + }, + }, + { + name: "policy references route that overlaps a non-referenced route", + validator: &policiesfakes.FakeValidator{}, + policies: map[PolicyKey]policies.Policy{ + pol1Key: pol1, + }, + routes: map[RouteKey]*L7Route{ + { + RouteType: RouteTypeHTTP, + NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"}, + }: createTestRouteWithPaths("hr-coffee", "/coffee"), + { + RouteType: RouteTypeHTTP, + NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr2"}, + }: createTestRouteWithPaths("hr2", "/coffee"), + }, + expProcessedPolicies: map[PolicyKey]*Policy{ + pol1Key: { + Source: pol1, + TargetRefs: []PolicyTargetRef{ + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"}, + Kind: kinds.HTTPRoute, + Group: v1.GroupName, + }, + }, + Ancestors: []PolicyAncestor{}, + Conditions: []conditions.Condition{ + { + Type: "Accepted", + Status: "False", + Reason: "TargetConflict", + Message: "Policy cannot be applied to target \"test/hr-coffee\" since another Route " + + "\"test/hr2\" shares a hostname:port/path combination with this target", + }, + }, + Valid: false, + }, + }, + }, + { + name: "policy references 2 routes that overlap", + validator: &policiesfakes.FakeValidator{}, + policies: map[PolicyKey]policies.Policy{ + pol2Key: pol2, + }, + routes: map[RouteKey]*L7Route{ + { + RouteType: RouteTypeHTTP, + NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"}, + }: createTestRouteWithPaths("hr-coffee", "/coffee"), + { + RouteType: RouteTypeHTTP, + NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee-tea"}, + }: createTestRouteWithPaths("hr-coffee-tea", "/coffee", "/tea"), + }, + expProcessedPolicies: map[PolicyKey]*Policy{ + pol2Key: { + Source: pol2, + TargetRefs: []PolicyTargetRef{ + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"}, + Kind: kinds.HTTPRoute, + Group: v1.GroupName, + }, + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-coffee-tea"}, + Kind: kinds.HTTPRoute, + Group: v1.GroupName, + }, + }, + Ancestors: []PolicyAncestor{}, + Valid: true, + }, + }, + }, + { + name: "policy references 2 routes that overlap with non-referenced route", + validator: &policiesfakes.FakeValidator{}, + policies: map[PolicyKey]policies.Policy{ + pol2Key: pol2, + }, + routes: map[RouteKey]*L7Route{ + { + RouteType: RouteTypeHTTP, + NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"}, + }: createTestRouteWithPaths("hr-coffee", "/coffee"), + { + RouteType: RouteTypeHTTP, + NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee-tea"}, + }: createTestRouteWithPaths("hr-coffee-tea", "/coffee", "/tea"), + { + RouteType: RouteTypeHTTP, + NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee-latte"}, + }: createTestRouteWithPaths("hr-coffee-latte", "/coffee", "/latte"), + }, + expProcessedPolicies: map[PolicyKey]*Policy{ + pol2Key: { + Source: pol2, + TargetRefs: []PolicyTargetRef{ + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"}, + Kind: kinds.HTTPRoute, + Group: v1.GroupName, + }, + { + Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-coffee-tea"}, + Kind: kinds.HTTPRoute, + Group: v1.GroupName, + }, + }, + Ancestors: []PolicyAncestor{}, + Conditions: []conditions.Condition{ + { + Type: "Accepted", + Status: "False", + Reason: "TargetConflict", + Message: "Policy cannot be applied to target \"test/hr-coffee\" since another Route " + + "\"test/hr-coffee-latte\" shares a hostname:port/path combination with this target", + }, + { + Type: "Accepted", + Status: "False", + Reason: "TargetConflict", + Message: "Policy cannot be applied to target \"test/hr-coffee-tea\" since another Route " + + "\"test/hr-coffee-latte\" shares a hostname:port/path combination with this target", + }, + }, + Valid: false, + }, + }, + }, + } + + gateways := processedGateways{ + Winner: &v1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gw", + Namespace: testNs, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewWithT(t) + + processed := processPolicies(test.policies, test.validator, gateways, test.routes, nil) + g.Expect(processed).To(BeEquivalentTo(test.expProcessedPolicies)) + }) + } +} + func TestMarkConflictedPolicies(t *testing.T) { hrRef := createTestRef(kinds.HTTPRoute, v1.GroupName, "hr") hrTargetRef := PolicyTargetRef{ @@ -796,12 +1009,12 @@ func TestMarkConflictedPolicies(t *testing.T) { name: "different policy types can not conflict", policies: map[PolicyKey]*Policy{ createTestPolicyKey(orangeGVK, "orange"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange"), + Source: createTestPolicy(orangeGVK, "orange", hrRef), TargetRefs: []PolicyTargetRef{hrTargetRef}, Valid: true, }, createTestPolicyKey(appleGVK, "apple"): { - Source: createTestPolicy(appleGVK, hrRef, "apple"), + Source: createTestPolicy(appleGVK, "apple", hrRef), TargetRefs: []PolicyTargetRef{hrTargetRef}, Valid: true, }, @@ -813,12 +1026,12 @@ func TestMarkConflictedPolicies(t *testing.T) { name: "policies of the same type but with different target refs can not conflict", policies: map[PolicyKey]*Policy{ createTestPolicyKey(orangeGVK, "orange1"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange1"), + Source: createTestPolicy(orangeGVK, "orange1", hrRef), TargetRefs: []PolicyTargetRef{hrTargetRef}, Valid: true, }, createTestPolicyKey(orangeGVK, "orange2"): { - Source: createTestPolicy(orangeGVK, grpcRef, "orange2"), + Source: createTestPolicy(orangeGVK, "orange2", grpcRef), TargetRefs: []PolicyTargetRef{grpcTargetRef}, Valid: true, }, @@ -830,12 +1043,12 @@ func TestMarkConflictedPolicies(t *testing.T) { name: "invalid policies can not conflict", policies: map[PolicyKey]*Policy{ createTestPolicyKey(orangeGVK, "valid"): { - Source: createTestPolicy(orangeGVK, hrRef, "valid"), + Source: createTestPolicy(orangeGVK, "valid", hrRef), TargetRefs: []PolicyTargetRef{hrTargetRef}, Valid: true, }, createTestPolicyKey(orangeGVK, "invalid"): { - Source: createTestPolicy(orangeGVK, hrRef, "invalid"), + Source: createTestPolicy(orangeGVK, "invalid", hrRef), TargetRefs: []PolicyTargetRef{hrTargetRef}, Valid: false, }, @@ -848,27 +1061,27 @@ func TestMarkConflictedPolicies(t *testing.T) { " condition is added", policies: map[PolicyKey]*Policy{ createTestPolicyKey(orangeGVK, "orange1"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange1"), + Source: createTestPolicy(orangeGVK, "orange1", hrRef), TargetRefs: []PolicyTargetRef{hrTargetRef}, Valid: true, }, createTestPolicyKey(orangeGVK, "orange2"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange2"), + Source: createTestPolicy(orangeGVK, "orange2", hrRef), TargetRefs: []PolicyTargetRef{hrTargetRef}, Valid: true, }, createTestPolicyKey(orangeGVK, "orange3-conflicts-with-1"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange3-conflicts-with-1"), + Source: createTestPolicy(orangeGVK, "orange3-conflicts-with-1", hrRef), TargetRefs: []PolicyTargetRef{hrTargetRef}, Valid: true, }, createTestPolicyKey(orangeGVK, "orange4"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange4"), + Source: createTestPolicy(orangeGVK, "orange4", hrRef), TargetRefs: []PolicyTargetRef{hrTargetRef}, Valid: true, }, createTestPolicyKey(orangeGVK, "orange5-conflicts-with-4"): { - Source: createTestPolicy(orangeGVK, hrRef, "orange5-conflicts-with-4"), + Source: createTestPolicy(orangeGVK, "orange5-conflicts-with-4", hrRef), TargetRefs: []PolicyTargetRef{hrTargetRef}, Valid: true, }, @@ -935,10 +1148,10 @@ func createTestPolicyWithAncestors(numAncestors int) policies.Policy { func createTestPolicyAndKey( gvk schema.GroupVersionKind, - ref v1alpha2.LocalPolicyTargetReference, name string, + refs ...v1alpha2.LocalPolicyTargetReference, ) (policies.Policy, PolicyKey) { - pol := createTestPolicy(gvk, ref, name) + pol := createTestPolicy(gvk, name, refs...) key := createTestPolicyKey(gvk, name) return pol, key @@ -946,8 +1159,8 @@ func createTestPolicyAndKey( func createTestPolicy( gvk schema.GroupVersionKind, - ref v1alpha2.LocalPolicyTargetReference, name string, + refs ...v1alpha2.LocalPolicyTargetReference, ) policies.Policy { return &policiesfakes.FakePolicy{ GetNameStub: func() string { @@ -957,7 +1170,7 @@ func createTestPolicy( return testNs }, GetTargetRefsStub: func() []v1alpha2.LocalPolicyTargetReference { - return []v1alpha2.LocalPolicyTargetReference{ref} + return refs }, GetObjectKindStub: func() schema.ObjectKind { return &policiesfakes.FakeObjectKind{ @@ -983,3 +1196,40 @@ func createTestRef(kind v1.Kind, group v1.Group, name string) v1alpha2.LocalPoli Name: v1.ObjectName(name), } } + +func createTestRouteWithPaths(name string, paths ...string) *L7Route { + routeMatches := make([]v1.HTTPRouteMatch, 0, len(paths)) + + for _, path := range paths { + routeMatches = append(routeMatches, v1.HTTPRouteMatch{ + Path: &v1.HTTPPathMatch{ + Type: helpers.GetPointer(v1.PathMatchExact), + Value: helpers.GetPointer(path), + }, + }) + } + + route := &L7Route{ + Source: &v1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testNs, + }, + }, + Spec: L7RouteSpec{ + Rules: []RouteRule{ + {Matches: routeMatches}, + }, + }, + ParentRefs: []ParentRef{ + { + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{"listener-1": {"foo.example.com"}}, + ListenerPort: 80, + }, + }, + }, + } + + return route +} diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index f8fedd50d..c096f404c 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -42,6 +42,8 @@ type ParentRefAttachmentStatus struct { // FailedCondition is the condition that describes why the ParentRef is not attached to the Gateway. It is set // when Attached is false. FailedCondition conditions.Condition + // ListenerPort is the port on the Listener that the Route is attached to. + ListenerPort v1.PortNumber // Attached indicates if the ParentRef is attached to the Gateway. Attached bool } @@ -367,6 +369,7 @@ func tryToAttachRouteToListeners( return true, false } refStatus.AcceptedHostnames[string(l.Source.Name)] = hostnames + refStatus.ListenerPort = l.Source.Port l.Routes[rk] = route return true, true diff --git a/internal/mode/static/state/store.go b/internal/mode/static/state/store.go index 6309639ec..b517d2773 100644 --- a/internal/mode/static/state/store.go +++ b/internal/mode/static/state/store.go @@ -10,7 +10,7 @@ import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds" ngftypes "github.com/nginxinc/nginx-gateway-fabric/internal/framework/types" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" ) diff --git a/internal/mode/static/state/validation/validationfakes/fake_policy_validator.go b/internal/mode/static/state/validation/validationfakes/fake_policy_validator.go index 1eaa2745e..9f72257ec 100644 --- a/internal/mode/static/state/validation/validationfakes/fake_policy_validator.go +++ b/internal/mode/static/state/validation/validationfakes/fake_policy_validator.go @@ -5,7 +5,7 @@ import ( "sync" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation" ) diff --git a/internal/mode/static/state/validation/validator.go b/internal/mode/static/state/validation/validator.go index 9e0108428..c941ed170 100644 --- a/internal/mode/static/state/validation/validator.go +++ b/internal/mode/static/state/validation/validator.go @@ -4,7 +4,7 @@ package validation import ( "github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" ) // Validators include validators for API resources from the perspective of a data-plane. diff --git a/internal/mode/static/status/status_setters.go b/internal/mode/static/status/status_setters.go index 7a23f56f7..cda4ff601 100644 --- a/internal/mode/static/status/status_setters.go +++ b/internal/mode/static/status/status_setters.go @@ -11,7 +11,7 @@ import ( ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers" frameworkStatus "github.com/nginxinc/nginx-gateway-fabric/internal/framework/status" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies" ) func newNginxGatewayStatusSetter(status ngfAPI.NginxGatewayStatus) frameworkStatus.Setter { diff --git a/internal/mode/static/status/status_setters_test.go b/internal/mode/static/status/status_setters_test.go index a7b838342..821d7bcf9 100644 --- a/internal/mode/static/status/status_setters_test.go +++ b/internal/mode/static/status/status_setters_test.go @@ -12,7 +12,7 @@ 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/framework/kinds" - "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" ) func TestNewNginxGatewayStatusSetter(t *testing.T) { diff --git a/site/content/how-to/monitoring/tracing.md b/site/content/how-to/monitoring/tracing.md index a3a1dc806..739faa117 100644 --- a/site/content/how-to/monitoring/tracing.md +++ b/site/content/how-to/monitoring/tracing.md @@ -13,10 +13,6 @@ NGINX Gateway Fabric supports tracing using [OpenTelemetry](https://opentelemetr This guide explains how to enable tracing on HTTPRoutes using NGINX Gateway Fabric. It uses the OpenTelemetry Collector and Jaeger to process and collect the traces. -{{< important >}} -Tracing cannot be enabled for [HTTPRoute matches](https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteMatch) with `headers`, `params`, or `method` matchers defined. It will be added in a future release. -{{< /important >}} - ## Install the collectors The first step is to install the collectors. NGINX Gateway Fabric will be configured to export to the OpenTelemetry Collector, which is configured to export to Jaeger. This model allows the visualization collector (Jaeger) to be swapped with something else, or to add more collectors without needing to reconfigure NGINX Gateway Fabric. It is also possible to configure NGINX Gateway Fabric to export directly to Jaeger. diff --git a/site/content/how-to/traffic-management/client-settings.md b/site/content/how-to/traffic-management/client-settings.md index a61030866..7a8ccbba9 100644 --- a/site/content/how-to/traffic-management/client-settings.md +++ b/site/content/how-to/traffic-management/client-settings.md @@ -28,10 +28,6 @@ Settings applied to an HTTPRoute or GRPCRoute take precedence over settings appl This guide will show you how to use the `ClientSettingsPolicy` API to configure the client max body size for your applications. -{{< important >}} -The `ClientSettingsPolicy` API does not work with [HTTPRoute matches](https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteMatch) with `headers`, `params`, or `method` matchers defined. It will be added in a future release. -{{< /important >}} - For all the possible configuration options for `ClientSettingsPolicy`, see the [API reference]({{< relref "reference/api.md" >}}). ## Setup diff --git a/site/content/overview/custom-policies.md b/site/content/overview/custom-policies.md index ecac51b04..ca362a06b 100644 --- a/site/content/overview/custom-policies.md +++ b/site/content/overview/custom-policies.md @@ -22,9 +22,8 @@ The following table summarizes NGINX Gateway Fabric custom policies: {{}} - {{< important >}} -NGINX Gateway Fabric policies do not work with [HTTPRoute matches](https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io/v1.HTTPRouteMatch) with `headers`, `params`, or `method` matchers defined. It will be added in a future release. +If attaching a Policy to a Route, that Route must not share a hostname:port/path combination with any other Route that is not referenced by the same Policy. If it does, the Policy will be rejected. This is because the Policy would end up affecting other Routes that it is not attached to. {{< /important >}} ## Terminology diff --git a/tests/Makefile b/tests/Makefile index b75c2050d..d3f908d28 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -52,7 +52,7 @@ run-conformance-tests: ## Run conformance tests .PHONY: cleanup-conformance-tests cleanup-conformance-tests: ## Clean up conformance tests fixtures kubectl delete pod conformance - kubectl delete -f tests/conformance-rbac.yaml + kubectl delete -f conformance/conformance-rbac.yaml .PHONY: build build: generate-static-deployment diff --git a/tests/suite/manifests/clientsettings/cafe-routes.yaml b/tests/suite/manifests/clientsettings/cafe-routes.yaml index 2a7cac241..eb7751239 100644 --- a/tests/suite/manifests/clientsettings/cafe-routes.yaml +++ b/tests/suite/manifests/clientsettings/cafe-routes.yaml @@ -51,6 +51,7 @@ spec: - path: type: Exact value: /soda + method: POST # This results in an internal location, so we can verify that policy attachment works there backendRefs: - name: soda port: 80 diff --git a/tests/suite/manifests/hello-world/routes.yaml b/tests/suite/manifests/hello-world/routes.yaml index 9b7b456e9..dac81942d 100644 --- a/tests/suite/manifests/hello-world/routes.yaml +++ b/tests/suite/manifests/hello-world/routes.yaml @@ -28,6 +28,7 @@ spec: - path: type: Exact value: /world + method: GET # This results in an internal location, so we can verify that policy attachment works there backendRefs: - name: world port: 80 From 1312b3ba8e43630ef7f1ce8b1127661c6d1f2067 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Mon, 5 Aug 2024 11:42:31 -0600 Subject: [PATCH 2/4] Code review --- .../mode/static/nginx/config/http/config.go | 1 - .../policies/clientsettings/generator.go | 4 +- .../policies/clientsettings/generator_test.go | 32 ++++---- .../policies/observability/generator.go | 5 ++ internal/mode/static/nginx/config/servers.go | 73 +++++++++---------- internal/mode/static/state/graph/httproute.go | 3 +- 6 files changed, 57 insertions(+), 61 deletions(-) diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index 4ebab5c40..16954c4f3 100644 --- a/internal/mode/static/nginx/config/http/config.go +++ b/internal/mode/static/nginx/config/http/config.go @@ -41,7 +41,6 @@ type Location struct { Rewrites []string Includes []Include GRPC bool - Redirects bool } // Header defines an HTTP header to be passed to the proxied server. diff --git a/internal/mode/static/nginx/config/policies/clientsettings/generator.go b/internal/mode/static/nginx/config/policies/clientsettings/generator.go index 3eb4ea83b..753d87628 100644 --- a/internal/mode/static/nginx/config/policies/clientsettings/generator.go +++ b/internal/mode/static/nginx/config/policies/clientsettings/generator.go @@ -51,12 +51,12 @@ func (g Generator) GenerateForServer(pols []policies.Policy, _ http.Server) poli return generate(pols) } -// GenerateForServer generates policy configuration for a normal location block. +// GenerateForLocation generates policy configuration for a normal location block. func (g Generator) GenerateForLocation(pols []policies.Policy, _ http.Location) policies.GenerateResultFiles { return generate(pols) } -// GenerateForServer generates policy configuration for a normal location block. +// GenerateForInternalLocation generates policy configuration for an internal location block. func (g Generator) GenerateForInternalLocation(pols []policies.Policy) policies.GenerateResultFiles { return generate(pols) } diff --git a/internal/mode/static/nginx/config/policies/clientsettings/generator_test.go b/internal/mode/static/nginx/config/policies/clientsettings/generator_test.go index 1fafea97d..7d728fcfd 100644 --- a/internal/mode/static/nginx/config/policies/clientsettings/generator_test.go +++ b/internal/mode/static/nginx/config/policies/clientsettings/generator_test.go @@ -149,32 +149,28 @@ func TestGenerate(t *testing.T) { }, } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewWithT(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(_ *testing.T) { generator := clientsettings.NewGenerator() resFiles := generator.GenerateForServer([]policies.Policy{test.policy}, http.Server{}) - g.Expect(resFiles).To(HaveLen(1)) - - for _, str := range test.expStrings { - g.Expect(string(resFiles[0].Content)).To(ContainSubstring(str)) - } + checkResults(resFiles, test.expStrings) resFiles = generator.GenerateForLocation([]policies.Policy{test.policy}, http.Location{}) - g.Expect(resFiles).To(HaveLen(1)) - - for _, str := range test.expStrings { - g.Expect(string(resFiles[0].Content)).To(ContainSubstring(str)) - } + checkResults(resFiles, test.expStrings) resFiles = generator.GenerateForInternalLocation([]policies.Policy{test.policy}) - g.Expect(resFiles).To(HaveLen(1)) - - for _, str := range test.expStrings { - g.Expect(string(resFiles[0].Content)).To(ContainSubstring(str)) - } + checkResults(resFiles, test.expStrings) }) } } diff --git a/internal/mode/static/nginx/config/policies/observability/generator.go b/internal/mode/static/nginx/config/policies/observability/generator.go index 17ffe311c..dfb9f2349 100644 --- a/internal/mode/static/nginx/config/policies/observability/generator.go +++ b/internal/mode/static/nginx/config/policies/observability/generator.go @@ -73,6 +73,9 @@ func NewGenerator(telemetry dataplane.Telemetry) *Generator { } // GenerateForServer generates policy configuration for a normal location block. +// For a normal location, all directives are applied. +// When the configuration involves a normal location redirecting to an internal location, +// only otel_trace and otel_trace_context are applied to the normal location. func (g Generator) GenerateForLocation(pols []policies.Policy, location http.Location) policies.GenerateResultFiles { buildTemplate := func( tmplate *template.Template, @@ -111,6 +114,8 @@ func (g Generator) GenerateForLocation(pols []policies.Policy, location http.Loc } // GenerateForInternalLocation generates policy configuration for an internal location block. +// otel_span_attr and otel_span_name are set in the internal location, with otel_trace and otel_trace_context +// being specified in the external location that redirects to the internal location. func (g Generator) GenerateForInternalLocation(pols []policies.Policy) policies.GenerateResultFiles { for _, pol := range pols { obs, ok := pol.(*ngfAPI.ObservabilityPolicy) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 727a0441d..f6c666f8a 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -144,13 +144,15 @@ func createServers( servers := make([]http.Server, 0, len(httpServers)+len(sslServers)) finalMatchPairs := make(httpMatchPairs) - for serverID, s := range httpServers { + for idx, s := range httpServers { + serverID := fmt.Sprintf("%d", idx) httpServer, matchPairs := createServer(s, serverID, generator) servers = append(servers, httpServer) maps.Copy(finalMatchPairs, matchPairs) } - for serverID, s := range sslServers { + for idx, s := range sslServers { + serverID := fmt.Sprintf("SSL_%d", idx) sslServer, matchPairs := createSSLServer(s, serverID, generator) servers = append(servers, sslServer) maps.Copy(finalMatchPairs, matchPairs) @@ -161,7 +163,7 @@ func createServers( func createSSLServer( virtualServer dataplane.VirtualServer, - serverIdx int, + serverID string, generator policies.Generator, ) (http.Server, httpMatchPairs) { if virtualServer.IsDefault { @@ -171,7 +173,6 @@ func createSSLServer( }, nil } - serverID := fmt.Sprintf("SSL_%d", serverIdx) locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator) server := http.Server{ @@ -193,7 +194,7 @@ func createSSLServer( func createServer( virtualServer dataplane.VirtualServer, - serverIdx int, + serverID string, generator policies.Generator, ) (http.Server, httpMatchPairs) { if virtualServer.IsDefault { @@ -203,7 +204,6 @@ func createServer( }, nil } - serverID := fmt.Sprintf("%d", serverIdx) locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator) server := http.Server{ @@ -255,16 +255,15 @@ func createLocations( } extLocations := initializeExternalLocations(rule, pathsAndTypes) + for i := range extLocations { + extLocations[i].Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForLocation(rule.Policies, extLocations[i]), + ) + } if !needsInternalLocations(rule) { for _, r := range rule.MatchRules { - extLocations = updateLocationsForFilters(r.Filters, extLocations, r, server.Port, rule.Path, rule.GRPC) - } - - for i := range extLocations { - extLocations[i].Includes = createIncludesFromPolicyGenerateResult( - generator.GenerateForLocation(rule.Policies, extLocations[i]), - ) + extLocations = updateLocations(r.Filters, extLocations, r, server.Port, rule.Path, rule.GRPC) } locs = append(locs, extLocations...) @@ -274,24 +273,22 @@ func createLocations( internalLocations := make([]http.Location, 0, len(rule.MatchRules)) for matchRuleIdx, r := range rule.MatchRules { - if len(rule.MatchRules) != 1 || !isPathOnlyMatch(r.Match) { - intLocation, match := initializeInternalLocation(pathRuleIdx, matchRuleIdx, r.Match, grpc) - intLocation.Includes = createIncludesFromPolicyGenerateResult( - generator.GenerateForInternalLocation(rule.Policies), - ) - - intLocation = updateLocationForFilters( - r.Filters, - intLocation, - r, - server.Port, - rule.Path, - rule.GRPC, - ) - - internalLocations = append(internalLocations, intLocation) - matches = append(matches, match) - } + intLocation, match := initializeInternalLocation(pathRuleIdx, matchRuleIdx, r.Match, grpc) + intLocation.Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForInternalLocation(rule.Policies), + ) + + intLocation = updateLocation( + r.Filters, + intLocation, + r, + server.Port, + rule.Path, + rule.GRPC, + ) + + internalLocations = append(internalLocations, intLocation) + matches = append(matches, match) } httpMatchKey := serverID + "_" + strconv.Itoa(pathRuleIdx) @@ -300,10 +297,6 @@ func createLocations( // so we don't need nginx/njs to perform unnecessary matching. // https://github.com/nginxinc/nginx-gateway-fabric/issues/662 extLocations[i].HTTPMatchKey = httpMatchKey - extLocations[i].Includes = createIncludesFromPolicyGenerateResult( - generator.GenerateForLocation(rule.Policies, extLocations[i]), - ) - matchPairs[extLocations[i].HTTPMatchKey] = matches } @@ -436,7 +429,8 @@ func initializeInternalLocation( return createMatchLocation(path, grpc), createRouteMatch(match, path) } -func updateLocationForFilters( +// updateLocation updates a location with any relevant configurations, like proxy_pass, filters, tls settings, etc. +func updateLocation( filters dataplane.HTTPFilters, location http.Location, matchRule dataplane.MatchRule, @@ -484,8 +478,9 @@ func updateLocationForFilters( return location } -// updateLocationsForFilters updates the existing locations with any relevant filters. -func updateLocationsForFilters( +// updateLocations updates the existing locations with any relevant configurations, like proxy_pass, +// filters, tls settings, etc. +func updateLocations( filters dataplane.HTTPFilters, buildLocations []http.Location, matchRule dataplane.MatchRule, @@ -496,7 +491,7 @@ func updateLocationsForFilters( updatedLocations := make([]http.Location, len(buildLocations)) for i, loc := range buildLocations { - updatedLocations[i] = updateLocationForFilters(filters, loc, matchRule, listenerPort, path, grpc) + updatedLocations[i] = updateLocation(filters, loc, matchRule, listenerPort, path, grpc) } return updatedLocations diff --git a/internal/mode/static/state/graph/httproute.go b/internal/mode/static/state/graph/httproute.go index 79f05f9e3..1b5bdf367 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -228,7 +228,8 @@ func validatePathMatch( } if strings.HasPrefix(*path.Value, http.InternalRoutePathPrefix) { - msg := fmt.Sprintf("path cannot start with %s", http.InternalRoutePathPrefix) + msg := fmt.Sprintf("path cannot start with %s. This prefix is reserved for internal use", + http.InternalRoutePathPrefix) return field.ErrorList{field.Invalid(fieldPath.Child("value"), *path.Value, msg)} } From ce229e23160e64d56667b665a438f4de74b0327c Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Mon, 5 Aug 2024 12:05:26 -0600 Subject: [PATCH 3/4] Fix flaky test --- .../mode/static/state/graph/policies_test.go | 130 +++++------------- 1 file changed, 37 insertions(+), 93 deletions(-) diff --git a/internal/mode/static/state/graph/policies_test.go b/internal/mode/static/state/graph/policies_test.go index 5851abcdc..59ea1dad8 100644 --- a/internal/mode/static/state/graph/policies_test.go +++ b/internal/mode/static/state/graph/policies_test.go @@ -790,11 +790,12 @@ func TestProcessPolicies_RouteOverlap(t *testing.T) { pol2, pol2Key := createTestPolicyAndKey(policyGVK, "pol2", hrRefCoffee, hrRefCoffeeTea) tests := []struct { - validator validation.PolicyValidator - policies map[PolicyKey]policies.Policy - routes map[RouteKey]*L7Route - expProcessedPolicies map[PolicyKey]*Policy - name string + validator validation.PolicyValidator + policies map[PolicyKey]policies.Policy + routes map[RouteKey]*L7Route + name string + expConditions []conditions.Condition + valid bool }{ { name: "no overlap", @@ -812,20 +813,7 @@ func TestProcessPolicies_RouteOverlap(t *testing.T) { NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr2"}, }: createTestRouteWithPaths("hr2", "/tea"), }, - expProcessedPolicies: map[PolicyKey]*Policy{ - pol1Key: { - Source: pol1, - TargetRefs: []PolicyTargetRef{ - { - Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"}, - Kind: kinds.HTTPRoute, - Group: v1.GroupName, - }, - }, - Ancestors: []PolicyAncestor{}, - Valid: true, - }, - }, + valid: true, }, { name: "policy references route that overlaps a non-referenced route", @@ -843,27 +831,14 @@ func TestProcessPolicies_RouteOverlap(t *testing.T) { NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr2"}, }: createTestRouteWithPaths("hr2", "/coffee"), }, - expProcessedPolicies: map[PolicyKey]*Policy{ - pol1Key: { - Source: pol1, - TargetRefs: []PolicyTargetRef{ - { - Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"}, - Kind: kinds.HTTPRoute, - Group: v1.GroupName, - }, - }, - Ancestors: []PolicyAncestor{}, - Conditions: []conditions.Condition{ - { - Type: "Accepted", - Status: "False", - Reason: "TargetConflict", - Message: "Policy cannot be applied to target \"test/hr-coffee\" since another Route " + - "\"test/hr2\" shares a hostname:port/path combination with this target", - }, - }, - Valid: false, + valid: false, + expConditions: []conditions.Condition{ + { + Type: "Accepted", + Status: "False", + Reason: "TargetConflict", + Message: "Policy cannot be applied to target \"test/hr-coffee\" since another Route " + + "\"test/hr2\" shares a hostname:port/path combination with this target", }, }, }, @@ -883,25 +858,7 @@ func TestProcessPolicies_RouteOverlap(t *testing.T) { NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee-tea"}, }: createTestRouteWithPaths("hr-coffee-tea", "/coffee", "/tea"), }, - expProcessedPolicies: map[PolicyKey]*Policy{ - pol2Key: { - Source: pol2, - TargetRefs: []PolicyTargetRef{ - { - Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"}, - Kind: kinds.HTTPRoute, - Group: v1.GroupName, - }, - { - Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-coffee-tea"}, - Kind: kinds.HTTPRoute, - Group: v1.GroupName, - }, - }, - Ancestors: []PolicyAncestor{}, - Valid: true, - }, - }, + valid: true, }, { name: "policy references 2 routes that overlap with non-referenced route", @@ -923,39 +880,21 @@ func TestProcessPolicies_RouteOverlap(t *testing.T) { NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee-latte"}, }: createTestRouteWithPaths("hr-coffee-latte", "/coffee", "/latte"), }, - expProcessedPolicies: map[PolicyKey]*Policy{ - pol2Key: { - Source: pol2, - TargetRefs: []PolicyTargetRef{ - { - Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"}, - Kind: kinds.HTTPRoute, - Group: v1.GroupName, - }, - { - Nsname: types.NamespacedName{Namespace: testNs, Name: "hr-coffee-tea"}, - Kind: kinds.HTTPRoute, - Group: v1.GroupName, - }, - }, - Ancestors: []PolicyAncestor{}, - Conditions: []conditions.Condition{ - { - Type: "Accepted", - Status: "False", - Reason: "TargetConflict", - Message: "Policy cannot be applied to target \"test/hr-coffee\" since another Route " + - "\"test/hr-coffee-latte\" shares a hostname:port/path combination with this target", - }, - { - Type: "Accepted", - Status: "False", - Reason: "TargetConflict", - Message: "Policy cannot be applied to target \"test/hr-coffee-tea\" since another Route " + - "\"test/hr-coffee-latte\" shares a hostname:port/path combination with this target", - }, - }, - Valid: false, + valid: false, + expConditions: []conditions.Condition{ + { + Type: "Accepted", + Status: "False", + Reason: "TargetConflict", + Message: "Policy cannot be applied to target \"test/hr-coffee\" since another Route " + + "\"test/hr-coffee-latte\" shares a hostname:port/path combination with this target", + }, + { + Type: "Accepted", + Status: "False", + Reason: "TargetConflict", + Message: "Policy cannot be applied to target \"test/hr-coffee-tea\" since another Route " + + "\"test/hr-coffee-latte\" shares a hostname:port/path combination with this target", }, }, }, @@ -975,7 +914,12 @@ func TestProcessPolicies_RouteOverlap(t *testing.T) { g := NewWithT(t) processed := processPolicies(test.policies, test.validator, gateways, test.routes, nil) - g.Expect(processed).To(BeEquivalentTo(test.expProcessedPolicies)) + g.Expect(processed).To(HaveLen(1)) + + for _, pol := range processed { + g.Expect(pol.Valid).To(Equal(test.valid)) + g.Expect(pol.Conditions).To(Equal(test.expConditions)) + } }) } } From 8fdf3a46ebbacc93466760288549b0fe4a7add29 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Wed, 7 Aug 2024 09:55:36 -0600 Subject: [PATCH 4/4] Language fixups --- internal/mode/static/nginx/config/policies/generator.go | 2 +- .../static/nginx/config/policies/observability/generator.go | 2 +- internal/mode/static/nginx/config/policies/validator.go | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/mode/static/nginx/config/policies/generator.go b/internal/mode/static/nginx/config/policies/generator.go index 021b7e517..a1428a2f9 100644 --- a/internal/mode/static/nginx/config/policies/generator.go +++ b/internal/mode/static/nginx/config/policies/generator.go @@ -46,7 +46,7 @@ func (g *CompositeGenerator) GenerateForServer(policies []Policy, server http.Se return compositeResult } -// GenerateForServer calls all policy generators for a normal location block. +// GenerateForLocation calls all policy generators for a normal location block. func (g *CompositeGenerator) GenerateForLocation(policies []Policy, location http.Location) GenerateResultFiles { var compositeResult GenerateResultFiles diff --git a/internal/mode/static/nginx/config/policies/observability/generator.go b/internal/mode/static/nginx/config/policies/observability/generator.go index dfb9f2349..3e86827fe 100644 --- a/internal/mode/static/nginx/config/policies/observability/generator.go +++ b/internal/mode/static/nginx/config/policies/observability/generator.go @@ -72,7 +72,7 @@ func NewGenerator(telemetry dataplane.Telemetry) *Generator { return &Generator{telemetryConf: telemetry} } -// GenerateForServer generates policy configuration for a normal location block. +// GenerateForLocation generates policy configuration for a normal location block. // For a normal location, all directives are applied. // When the configuration involves a normal location redirecting to an internal location, // only otel_trace and otel_trace_context are applied to the normal location. diff --git a/internal/mode/static/nginx/config/policies/validator.go b/internal/mode/static/nginx/config/policies/validator.go index 486028ea5..80ffe1198 100644 --- a/internal/mode/static/nginx/config/policies/validator.go +++ b/internal/mode/static/nginx/config/policies/validator.go @@ -21,7 +21,7 @@ type Validator interface { Conflicts(a, b Policy) bool } -// CompositeValidator manages the validators and generators for NGF Policies. +// CompositeValidator manages the validators for NGF Policies. type CompositeValidator struct { validators map[schema.GroupVersionKind]Validator mustExtractGVK kinds.MustExtractGVK @@ -36,7 +36,7 @@ type ManagerConfig struct { } // NewManager returns a new CompositeValidator. -// Implements dataplane.ConfigGenerator and validation.PolicyValidator. +// Implements validation.PolicyValidator. func NewManager( mustExtractGVK kinds.MustExtractGVK, configs ...ManagerConfig,