diff --git a/internal/mode/static/handler.go b/internal/mode/static/handler.go index d90edc337e..3c4d642bf7 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 30b71f7026..8a16a99eb3 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 a0509194e0..eac79be34e 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 9326ebb439..fa01b467a5 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 4b4407a1a4..d11c3f8870 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 e903c17151..b9d0388512 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 0000000000..3eb4ea83b6 --- /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 92% rename from internal/mode/static/policies/clientsettings/generator_test.go rename to internal/mode/static/nginx/config/policies/clientsettings/generator_test.go index 19b3b1de3b..fe09695197 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) { @@ -166,7 +166,15 @@ func TestGeneratePanics(t *testing.T) { g := NewWithT(t) generate := func() { - clientsettings.Generate(&policiesfakes.FakePolicy{}, nil) + generator := clientsettings.NewGenerator() + generator.GenerateForServer([]policies.Policy{}, http.Server{}) + } + + g.Expect(generate).To(Panic()) + + generate = func() { + generator := clientsettings.NewGenerator() + generator.GenerateForLocation([]policies.Policy{}, http.Location{}) } g.Expect(generate).To(Panic()) 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 bcc9f6ebfc..60ce55c7a9 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 8b9a7fcdd4..9cb5884b6e 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 0000000000..021b7e517a --- /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/observability/generator.go b/internal/mode/static/nginx/config/policies/observability/generator.go new file mode 100644 index 0000000000..77e6927f9b --- /dev/null +++ b/internal/mode/static/nginx/config/policies/observability/generator.go @@ -0,0 +1,159 @@ +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 }} +` + +// TODO(sberman): (add unit test for span name) +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/policies/observability/generator_test.go b/internal/mode/static/nginx/config/policies/observability/generator_test.go similarity index 93% rename from internal/mode/static/policies/observability/generator_test.go rename to internal/mode/static/nginx/config/policies/observability/generator_test.go index 0a5d33d2e5..49019ef8ea 100644 --- a/internal/mode/static/policies/observability/generator_test.go +++ b/internal/mode/static/nginx/config/policies/observability/generator_test.go @@ -8,9 +8,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/observability" - "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/observability" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes" ) func TestGenerate(t *testing.T) { @@ -199,16 +199,3 @@ func TestGeneratePanics(t *testing.T) { 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/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 78d4e5d9df..3fa7ea2289 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 1e726b9fd4..9f8976e4c2 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 0000000000..f7cdb65375 --- /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 5fb03a937a..3370e931ab 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 288ce3c57f..05125b0d27 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 7072fe8d2d..c26f3bec7b 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 c7355720b1..486028ea56 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 92% rename from internal/mode/static/policies/manager_test.go rename to internal/mode/static/nginx/config/policies/validator_test.go index 5699ac0480..3f36d2bcd8 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 { diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 3aeefa47c7..7f954de1d4 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,60 @@ func createLocations(server *dataplane.VirtualServer, serverID int) ([]http.Loca extLocations := initializeExternalLocations(rule, pathsAndTypes) - for matchRuleIdx, r := range rule.MatchRules { - buildLocations := extLocations + 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]), + ) + } + + locs = append(locs, extLocations...) + continue + } + + httpMatchKey := serverID + "_" + strconv.Itoa(pathRuleIdx) + 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) - buildLocations = []http.Location{intLocation} + intLocation, match := initializeInternalLocation(pathRuleIdx, matchRuleIdx, r.Match, grpc) + intLocation.HTTPMatchKey = httpMatchKey + 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) } + } - includes := createIncludes(r.Additions) + 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]), + ) - // 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...) + matchPairs[extLocations[i].HTTPMatchKey] = matches } - 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 - } - locs = append(locs, extLocations...) - } + locs = append(locs, extLocations...) + locs = append(locs, internalLocations...) } if !rootPathExists { @@ -292,12 +319,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) +} - return locations +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 includes } // pathAndTypeMap contains a map of paths and any path types defined for that path @@ -332,7 +374,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 +397,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 +419,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 +601,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 +629,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 +736,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 4d8196b180..6d8bc0c362 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 effb0099ab..3f46f87b68 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -777,37 +777,37 @@ func TestCreateServers(t *testing.T) { 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 +816,7 @@ func TestCreateServers(t *testing.T) { "1_17": { { Method: "GET", - RedirectPath: "@rule17-route0", + RedirectPath: "/_ngf-internal-rule17-route0", }, }, } @@ -855,17 +855,20 @@ func TestCreateServers(t *testing.T) { return []http.Location{ { - Path: "@rule0-route0", + Internal: true, + Path: "/_ngf-internal-rule0-route0", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, }, { - Path: "@rule0-route1", + Internal: true, + Path: "/_ngf-internal-rule0-route1", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, }, { - Path: "@rule0-route2", + Internal: true, + Path: "/_ngf-internal-rule0-route2", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, }, @@ -874,7 +877,8 @@ func TestCreateServers(t *testing.T) { HTTPMatchKey: ssl + "1_0", }, { - Path: "@rule1-route0", + Internal: true, + Path: "/_ngf-internal-rule1-route0", ProxyPass: "http://$test__route1_rule1$request_uri", ProxySetHeaders: httpBaseHeaders, }, @@ -939,7 +943,8 @@ func TestCreateServers(t *testing.T) { }, }, { - Path: "@rule6-route0", + Internal: true, + Path: "/_ngf-internal-rule6-route0", Return: &http.Return{ Body: "$scheme://foo.example.com:8080$request_uri", Code: 302, @@ -966,8 +971,9 @@ func TestCreateServers(t *testing.T) { ProxySetHeaders: rewriteProxySetHeaders, }, { - Path: "@rule8-route0", - Rewrites: []string{"^/rewrite-with-headers(.*)$ /prefix-replacement$1 break"}, + Internal: true, + Path: "/_ngf-internal-rule8-route0", + Rewrites: []string{"^ $request_uri", "^/rewrite-with-headers(.*)$ /prefix-replacement$1 break"}, ProxyPass: "http://test_foo_80", ProxySetHeaders: rewriteProxySetHeaders, }, @@ -992,7 +998,8 @@ func TestCreateServers(t *testing.T) { }, }, { - Path: "@rule10-route0", + Internal: true, + Path: "/_ngf-internal-rule10-route0", Return: &http.Return{ Code: http.StatusInternalServerError, }, @@ -1011,7 +1018,8 @@ func TestCreateServers(t *testing.T) { ProxySetHeaders: httpBaseHeaders, }, { - Path: "@rule12-route0", + Internal: true, + Path: "/_ngf-internal-rule12-route0", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, }, @@ -1116,12 +1124,14 @@ func TestCreateServers(t *testing.T) { }, }, { - Path: "@rule17-route0", + Internal: true, + Path: "/_ngf-internal-rule17-route0", ProxyPass: "http://test_foo_80$request_uri", ProxySetHeaders: httpBaseHeaders, Includes: []string{ includesFolder + "/match-addition.conf", }, + Rewrites: []string{"^ $request_uri break"}, }, { Path: "= /addition-header-match", @@ -1712,7 +1722,8 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) { }, }, expected: &rewriteConfig{ - Rewrite: "^ /full-path break", + InternalRewrite: "^ $request_uri", + MainRewrite: "^ /full-path break", }, msg: "full path", }, @@ -1725,7 +1736,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 +1750,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 +1764,8 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) { }, }, expected: &rewriteConfig{ - Rewrite: "^/original(?:/(.*))?$ /$1 break", + InternalRewrite: "^ $request_uri", + MainRewrite: "^/original(?:/(.*))?$ /$1 break", }, msg: "prefix path /", }, @@ -1764,7 +1778,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 +1792,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 +1806,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 +2155,24 @@ func TestCreateProxyPass(t *testing.T) { func TestCreateMatchLocation(t *testing.T) { g := NewWithT(t) - expected := http.Location{ - Path: "/path", + expectedNoGRPC := http.Location{ + Path: "/path", + Internal: true, } - 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", + Internal: true, + Rewrites: []string{"^ $request_uri break"}, + } + + grpc = true + result = createMatchLocation("/path", grpc) + g.Expect(result).To(Equal(expectedWithGRPC)) } func TestGenerateProxySetHeaders(t *testing.T) { @@ -2586,7 +2615,7 @@ func TestCreateAdditionFileResults(t *testing.T) { }, } - results := createAdditionFileResults(conf) + results := createIncludeFileResults(conf) expResults := []executeResult{ { 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 942d944af8..6856dea988 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 1b4eb0be36..bcee4d6569 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 d4f502ace9..87dd107055 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 943c8e2c62..0000000000 --- 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 046881ca6c..0000000000 --- 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/policiesfakes/fake_config_generator.go b/internal/mode/static/policies/policiesfakes/fake_config_generator.go deleted file mode 100644 index 17640f029b..0000000000 --- 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 3fdcd55d97..40a3723dff 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/conditions/conditions.go b/internal/mode/static/state/conditions/conditions.go index 5b075a6446..0ef990500d 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 b81dbfe7a0..a093e63605 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,16 @@ func buildTelemetry(g *graph.Graph) Telemetry { tel.Interval = string(*telemetry.Exporter.Interval) } + spanAttrs := make([]SpanAttribute, 0, len(telemetry.SpanAttributes)) + for _, spanAttr := range telemetry.SpanAttributes { + sa := SpanAttribute{ + Key: spanAttr.Key, + Value: spanAttr.Value, + } + spanAttrs = append(spanAttrs, sa) + } + tel.SpanAttributes = spanAttrs + // 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 +663,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 +677,12 @@ func buildTelemetry(g *graph.Graph) Telemetry { return tel } +// 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 +710,18 @@ func buildBaseHTTPConfig(g *graph.Graph) BaseHTTPConfig { return baseConfig } -func buildAdditions( - policies []*graph.Policy, - globalSettings *policies.GlobalSettings, - generator policies.ConfigGenerator, -) []Addition { - if len(policies) == 0 { - return nil - } +func buildPolicies( + graphPolicies []*graph.Policy, +) []policies.Policy { + finalPolicies := make([]policies.Policy, 0, len(graphPolicies)) - additions := make([]Addition, 0, len(policies)) - - 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 83d5873ce0..04b76769c7 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" @@ -3215,3 +3215,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 d342ff3b0c..20facb87a2 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. @@ -110,6 +103,8 @@ type PathRule struct { MatchRules []MatchRule // GRPC indicates if this is a gRPC rule GRPC bool + // Policies contains the list of policies that are applied to this PathRule. + Policies []policies.Policy } // InvalidHTTPFilter is a special filter for handling the case when configured filters are invalid. @@ -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 @@ -284,6 +277,8 @@ type Telemetry struct { BatchSize int32 // BatchCount specifies the number of pending batches per worker, spans exceeding the limit are dropped. BatchCount int32 + // SpanAttributes are global custom key/value attributes that are added to each span. + SpanAttributes []SpanAttribute } // SpanAttribute is a key value pair to be added to a tracing span. diff --git a/internal/mode/static/state/graph/graph.go b/internal/mode/static/state/graph/graph.go index d874d5d2c7..a954d0a898 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 2081fd881c..d944c110ef 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" @@ -608,9 +608,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 cac85228c6..79f05f9e3e 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 4686908a26..68d58a40d7 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 bd4fa29a4f..4b1158c4ec 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" @@ -166,6 +166,8 @@ func processPolicies( processedPolicies := make(map[PolicyKey]*Policy) + var conds []conditions.Condition + for key, policy := range policies { targetRefs := make([]PolicyTargetRef, 0, len(policy.GetTargetRefs())) @@ -179,8 +181,24 @@ 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 { + // We need to check if this route referenced in the policy has an overlapping + // hostname:port/path with any other route. If so, deny the policy. + hostPortPaths := make(map[string]string) + + registerRouteTarget(route, hostPortPaths) + + for _, r := range routes { + if client.ObjectKeyFromObject(r.Source) == client.ObjectKeyFromObject(route.Source) { + continue + } + + if cond := checkForRouteOverlap(r, hostPortPaths); cond != nil { + conds = append(conds, *cond) + } + } } default: continue @@ -198,7 +216,7 @@ func processPolicies( continue } - conds := validator.Validate(policy, globalSettings) + conds = append(conds, validator.Validate(policy, globalSettings)...) processedPolicies[key] = &Policy{ Source: policy, @@ -214,6 +232,43 @@ 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 +} + +// registerRouteTarget 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 (since it's guaranteed to be nil). +func registerRouteTarget(route *L7Route, hostPortPaths map[string]string) { + checkForRouteOverlap(route, 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 a555f4f4a4..069f1c4ea9 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" ) diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index 8fe32a96c4..dc83354056 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 } @@ -363,6 +365,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 6309639ecd..b517d27732 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 1eaa2745e1..9f72257ec7 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 9e01084287..c941ed1700 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 7a23f56f73..cda4ff6012 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 a7b838342d..821d7bcf94 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/tests/Makefile b/tests/Makefile index 6876242697..4b140eee64 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 2a7cac241d..eb77512392 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 9b7b456e98..dac81942d4 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