Code review

Author: sjberman

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.

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)
 }

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)
 		})
 	}
 }

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)

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

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)}
 	}