diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index 4ebab5c409..16954c4f3b 100644 --- a/internal/mode/static/nginx/config/http/config.go +++ b/internal/mode/static/nginx/config/http/config.go @@ -41,7 +41,6 @@ type Location struct { Rewrites []string Includes []Include GRPC bool - Redirects bool } // Header defines an HTTP header to be passed to the proxied server. diff --git a/internal/mode/static/nginx/config/policies/clientsettings/generator.go b/internal/mode/static/nginx/config/policies/clientsettings/generator.go index 3eb4ea83b6..753d876284 100644 --- a/internal/mode/static/nginx/config/policies/clientsettings/generator.go +++ b/internal/mode/static/nginx/config/policies/clientsettings/generator.go @@ -51,12 +51,12 @@ func (g Generator) GenerateForServer(pols []policies.Policy, _ http.Server) poli return generate(pols) } -// GenerateForServer generates policy configuration for a normal location block. +// GenerateForLocation generates policy configuration for a normal location block. func (g Generator) GenerateForLocation(pols []policies.Policy, _ http.Location) policies.GenerateResultFiles { return generate(pols) } -// GenerateForServer generates policy configuration for a normal location block. +// GenerateForInternalLocation generates policy configuration for an internal location block. func (g Generator) GenerateForInternalLocation(pols []policies.Policy) policies.GenerateResultFiles { return generate(pols) } diff --git a/internal/mode/static/nginx/config/policies/clientsettings/generator_test.go b/internal/mode/static/nginx/config/policies/clientsettings/generator_test.go index 1fafea97d8..7d728fcfd2 100644 --- a/internal/mode/static/nginx/config/policies/clientsettings/generator_test.go +++ b/internal/mode/static/nginx/config/policies/clientsettings/generator_test.go @@ -149,32 +149,28 @@ func TestGenerate(t *testing.T) { }, } - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewWithT(t) + g := NewWithT(t) + + checkResults := func(resFiles policies.GenerateResultFiles, expStrings []string) { + g.Expect(resFiles).To(HaveLen(1)) + + for _, str := range expStrings { + g.Expect(string(resFiles[0].Content)).To(ContainSubstring(str)) + } + } + for _, test := range tests { + t.Run(test.name, func(_ *testing.T) { generator := clientsettings.NewGenerator() resFiles := generator.GenerateForServer([]policies.Policy{test.policy}, http.Server{}) - g.Expect(resFiles).To(HaveLen(1)) - - for _, str := range test.expStrings { - g.Expect(string(resFiles[0].Content)).To(ContainSubstring(str)) - } + checkResults(resFiles, test.expStrings) resFiles = generator.GenerateForLocation([]policies.Policy{test.policy}, http.Location{}) - g.Expect(resFiles).To(HaveLen(1)) - - for _, str := range test.expStrings { - g.Expect(string(resFiles[0].Content)).To(ContainSubstring(str)) - } + checkResults(resFiles, test.expStrings) resFiles = generator.GenerateForInternalLocation([]policies.Policy{test.policy}) - g.Expect(resFiles).To(HaveLen(1)) - - for _, str := range test.expStrings { - g.Expect(string(resFiles[0].Content)).To(ContainSubstring(str)) - } + checkResults(resFiles, test.expStrings) }) } } diff --git a/internal/mode/static/nginx/config/policies/observability/generator.go b/internal/mode/static/nginx/config/policies/observability/generator.go index 17ffe311c2..dfb9f2349c 100644 --- a/internal/mode/static/nginx/config/policies/observability/generator.go +++ b/internal/mode/static/nginx/config/policies/observability/generator.go @@ -73,6 +73,9 @@ func NewGenerator(telemetry dataplane.Telemetry) *Generator { } // GenerateForServer generates policy configuration for a normal location block. +// For a normal location, all directives are applied. +// When the configuration involves a normal location redirecting to an internal location, +// only otel_trace and otel_trace_context are applied to the normal location. func (g Generator) GenerateForLocation(pols []policies.Policy, location http.Location) policies.GenerateResultFiles { buildTemplate := func( tmplate *template.Template, @@ -111,6 +114,8 @@ func (g Generator) GenerateForLocation(pols []policies.Policy, location http.Loc } // GenerateForInternalLocation generates policy configuration for an internal location block. +// otel_span_attr and otel_span_name are set in the internal location, with otel_trace and otel_trace_context +// being specified in the external location that redirects to the internal location. func (g Generator) GenerateForInternalLocation(pols []policies.Policy) policies.GenerateResultFiles { for _, pol := range pols { obs, ok := pol.(*ngfAPI.ObservabilityPolicy) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 727a0441d1..f6c666f8a3 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -144,13 +144,15 @@ func createServers( servers := make([]http.Server, 0, len(httpServers)+len(sslServers)) finalMatchPairs := make(httpMatchPairs) - for serverID, s := range httpServers { + for idx, s := range httpServers { + serverID := fmt.Sprintf("%d", idx) httpServer, matchPairs := createServer(s, serverID, generator) servers = append(servers, httpServer) maps.Copy(finalMatchPairs, matchPairs) } - for serverID, s := range sslServers { + for idx, s := range sslServers { + serverID := fmt.Sprintf("SSL_%d", idx) sslServer, matchPairs := createSSLServer(s, serverID, generator) servers = append(servers, sslServer) maps.Copy(finalMatchPairs, matchPairs) @@ -161,7 +163,7 @@ func createServers( func createSSLServer( virtualServer dataplane.VirtualServer, - serverIdx int, + serverID string, generator policies.Generator, ) (http.Server, httpMatchPairs) { if virtualServer.IsDefault { @@ -171,7 +173,6 @@ func createSSLServer( }, nil } - serverID := fmt.Sprintf("SSL_%d", serverIdx) locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator) server := http.Server{ @@ -193,7 +194,7 @@ func createSSLServer( func createServer( virtualServer dataplane.VirtualServer, - serverIdx int, + serverID string, generator policies.Generator, ) (http.Server, httpMatchPairs) { if virtualServer.IsDefault { @@ -203,7 +204,6 @@ func createServer( }, nil } - serverID := fmt.Sprintf("%d", serverIdx) locs, matchPairs, grpc := createLocations(&virtualServer, serverID, generator) server := http.Server{ @@ -255,16 +255,15 @@ func createLocations( } extLocations := initializeExternalLocations(rule, pathsAndTypes) + for i := range extLocations { + extLocations[i].Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForLocation(rule.Policies, extLocations[i]), + ) + } if !needsInternalLocations(rule) { for _, r := range rule.MatchRules { - extLocations = updateLocationsForFilters(r.Filters, extLocations, r, server.Port, rule.Path, rule.GRPC) - } - - for i := range extLocations { - extLocations[i].Includes = createIncludesFromPolicyGenerateResult( - generator.GenerateForLocation(rule.Policies, extLocations[i]), - ) + extLocations = updateLocations(r.Filters, extLocations, r, server.Port, rule.Path, rule.GRPC) } locs = append(locs, extLocations...) @@ -274,24 +273,22 @@ func createLocations( internalLocations := make([]http.Location, 0, len(rule.MatchRules)) for matchRuleIdx, r := range rule.MatchRules { - if len(rule.MatchRules) != 1 || !isPathOnlyMatch(r.Match) { - intLocation, match := initializeInternalLocation(pathRuleIdx, matchRuleIdx, r.Match, grpc) - intLocation.Includes = createIncludesFromPolicyGenerateResult( - generator.GenerateForInternalLocation(rule.Policies), - ) - - intLocation = updateLocationForFilters( - r.Filters, - intLocation, - r, - server.Port, - rule.Path, - rule.GRPC, - ) - - internalLocations = append(internalLocations, intLocation) - matches = append(matches, match) - } + intLocation, match := initializeInternalLocation(pathRuleIdx, matchRuleIdx, r.Match, grpc) + intLocation.Includes = createIncludesFromPolicyGenerateResult( + generator.GenerateForInternalLocation(rule.Policies), + ) + + intLocation = updateLocation( + r.Filters, + intLocation, + r, + server.Port, + rule.Path, + rule.GRPC, + ) + + internalLocations = append(internalLocations, intLocation) + matches = append(matches, match) } httpMatchKey := serverID + "_" + strconv.Itoa(pathRuleIdx) @@ -300,10 +297,6 @@ func createLocations( // so we don't need nginx/njs to perform unnecessary matching. // https://github.com/nginxinc/nginx-gateway-fabric/issues/662 extLocations[i].HTTPMatchKey = httpMatchKey - extLocations[i].Includes = createIncludesFromPolicyGenerateResult( - generator.GenerateForLocation(rule.Policies, extLocations[i]), - ) - matchPairs[extLocations[i].HTTPMatchKey] = matches } @@ -436,7 +429,8 @@ func initializeInternalLocation( return createMatchLocation(path, grpc), createRouteMatch(match, path) } -func updateLocationForFilters( +// updateLocation updates a location with any relevant configurations, like proxy_pass, filters, tls settings, etc. +func updateLocation( filters dataplane.HTTPFilters, location http.Location, matchRule dataplane.MatchRule, @@ -484,8 +478,9 @@ func updateLocationForFilters( return location } -// updateLocationsForFilters updates the existing locations with any relevant filters. -func updateLocationsForFilters( +// updateLocations updates the existing locations with any relevant configurations, like proxy_pass, +// filters, tls settings, etc. +func updateLocations( filters dataplane.HTTPFilters, buildLocations []http.Location, matchRule dataplane.MatchRule, @@ -496,7 +491,7 @@ func updateLocationsForFilters( updatedLocations := make([]http.Location, len(buildLocations)) for i, loc := range buildLocations { - updatedLocations[i] = updateLocationForFilters(filters, loc, matchRule, listenerPort, path, grpc) + updatedLocations[i] = updateLocation(filters, loc, matchRule, listenerPort, path, grpc) } return updatedLocations diff --git a/internal/mode/static/state/graph/httproute.go b/internal/mode/static/state/graph/httproute.go index 79f05f9e3e..1b5bdf3676 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -228,7 +228,8 @@ func validatePathMatch( } if strings.HasPrefix(*path.Value, http.InternalRoutePathPrefix) { - msg := fmt.Sprintf("path cannot start with %s", http.InternalRoutePathPrefix) + msg := fmt.Sprintf("path cannot start with %s. This prefix is reserved for internal use", + http.InternalRoutePathPrefix) return field.ErrorList{field.Invalid(fieldPath.Child("value"), *path.Value, msg)} }