update based on reviews

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

@@ -1,8 +1,6 @@
 package config
 
 import (
-	"encoding/json"
-	"fmt"
 	"path/filepath"
 
 	"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file"
@@ -57,8 +55,13 @@ func NewGeneratorImpl(plus bool) GeneratorImpl {
 	return GeneratorImpl{plus: plus}
 }
 
+type executeResult struct {
+	dest string
+	data []byte
+}
+
 // executeFunc is a function that generates NGINX configuration from internal representation.
-type executeFunc func(configuration dataplane.Configuration) []byte
+type executeFunc func(configuration dataplane.Configuration) []executeResult
 
 // Generate generates NGINX configuration files from internal representation.
 // It is the responsibility of the caller to validate the configuration before calling this function.
@@ -113,40 +116,36 @@ func generateCertBundleFileName(id dataplane.CertBundleID) string {
 }
 
 func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) []file.File {
+	files := make([]file.File, 0)
 	var c []byte
+
 	for _, execute := range g.getExecuteFuncs() {
-		c = append(c, execute(conf)...)
+		results := execute(conf)
+		for _, res := range results {
+			if res.dest == httpConfigFile {
+				c = append(c, res.data...)
+			} else {
+				files = append(files, file.File{
+					Path:    res.dest,
+					Content: res.data,
+					Type:    file.TypeRegular,
+				})
+			}
+		}
 	}
 
-	servers, httpMatchPairs := executeServers(conf)
-
-	// create server conf
-	serverConf := execute(serversTemplate, servers)
-	c = append(c, serverConf...)
-
-	httpConf := file.File{
-		Content: c,
+	files = append(files, file.File{
 		Path:    httpConfigFile,
+		Content: c,
 		Type:    file.TypeRegular,
-	}
-
-	// create httpMatchPair conf
-	b, err := json.Marshal(httpMatchPairs)
-	if err != nil {
-		// panic is safe here because we should never fail to marshal the match unless we constructed it incorrectly.
-		panic(fmt.Errorf("could not marshal http match pairs: %w", err))
-	}
-	matchConf := file.File{
-		Content: b,
-		Path:    httpMatchVarsFile,
-		Type:    file.TypeRegular,
-	}
+	})
 
-	return []file.File{httpConf, matchConf}
+	return files
 }
 
 func (g GeneratorImpl) getExecuteFuncs() []executeFunc {
 	return []executeFunc{
+		executeServers,
 		g.executeUpstreams,
 		executeSplitClients,
 		executeMaps,

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

@@ -78,19 +78,21 @@ func TestGenerate(t *testing.T) {
 		Content: []byte("test-cert\ntest-key"),
 	}))
 
+	g.Expect(files[1].Path).To(Equal("/etc/nginx/conf.d/match.json"))
 	g.Expect(files[1].Type).To(Equal(file.TypeRegular))
-	g.Expect(files[1].Path).To(Equal("/etc/nginx/conf.d/http.conf"))
-	httpCfg := string(files[1].Content) // converting to string so that on failure gomega prints strings not byte arrays
+	expString := "{}"
+	g.Expect(string(files[1].Content)).To(Equal(expString))
+
+	g.Expect(files[2].Type).To(Equal(file.TypeRegular))
+	g.Expect(files[2].Path).To(Equal("/etc/nginx/conf.d/http.conf"))
+	httpCfg := string(files[2].Content) // converting to string so that on failure gomega prints strings not byte arrays
 	// Note: this only verifies that Generate() returns a byte array with upstream, server, and split_client blocks.
 	// It does not test the correctness of those blocks. That functionality is covered by other tests in this package.
 	g.Expect(httpCfg).To(ContainSubstring("listen 80"))
 	g.Expect(httpCfg).To(ContainSubstring("listen 443"))
 	g.Expect(httpCfg).To(ContainSubstring("upstream"))
 	g.Expect(httpCfg).To(ContainSubstring("split_clients"))
 
-	g.Expect(files[2].Path).To(Equal("/etc/nginx/conf.d/match.json"))
-	g.Expect(files[2].Type).To(Equal(file.TypeRegular))
-
 	g.Expect(files[3].Type).To(Equal(file.TypeRegular))
 	g.Expect(files[3].Path).To(Equal("/etc/nginx/conf.d/config-version.conf"))
 	configVersion := string(files[3].Content)

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

@@ -93,21 +93,3 @@ type ProxySSLVerify struct {
 	TrustedCertificate string
 	Name               string
 }
-
-// httpMatch is an internal representation of an HTTPRouteMatch.
-// This struct is marshaled into a string and stored as a variable in the nginx location block for the route's path.
-// The NJS httpmatches module will look up this variable on the request object and compare the request against the
-// Method, Headers, and QueryParams contained in httpMatch.
-// If the request satisfies the httpMatch, NGINX will redirect the request to the location RedirectPath.
-type RouteMatch struct {
-	// Method is the HTTPMethod of the HTTPRouteMatch.
-	Method string `json:"method,omitempty"`
-	// RedirectPath is the path to redirect the request to if the request satisfies the match conditions.
-	RedirectPath string `json:"redirectPath,omitempty"`
-	// Headers is a list of HTTPHeaders name value pairs with the format "{name}:{value}".
-	Headers []string `json:"headers,omitempty"`
-	// QueryParams is a list of HTTPQueryParams name value pairs with the format "{name}={value}".
-	QueryParams []string `json:"params,omitempty"`
-	// Any represents a match with no match conditions.
-	Any bool `json:"any,omitempty"`
-}

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

@@ -10,9 +10,13 @@ import (
 
 var mapsTemplate = gotemplate.Must(gotemplate.New("maps").Parse(mapsTemplateText))
 
-func executeMaps(conf dataplane.Configuration) []byte {
+func executeMaps(conf dataplane.Configuration) []executeResult {
 	maps := buildAddHeaderMaps(append(conf.HTTPServers, conf.SSLServers...))
-	return execute(mapsTemplate, maps)
+	result := executeResult{
+		dest: httpConfigFile,
+		data: execute(mapsTemplate, maps),
+	}
+	return []executeResult{result}
 }
 
 func buildAddHeaderMaps(servers []dataplane.VirtualServer) []http.Map {

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

@@ -88,7 +88,8 @@ func TestExecuteMaps(t *testing.T) {
 		"map $http_upgrade $connection_upgrade {":                             1,
 	}
 
-	maps := string(executeMaps(conf))
+	mapResult := executeMaps(conf)
+	maps := string(mapResult[0].data)
 	for expSubStr, expCount := range expSubStrings {
 		g.Expect(expCount).To(Equal(strings.Count(maps, expSubStr)))
 	}

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

@@ -1,8 +1,9 @@
 package config
 
 import (
+	"encoding/json"
 	"fmt"
-	"regexp"
+	"maps"
 	"strconv"
 	"strings"
 	gotemplate "text/template"
@@ -39,52 +40,57 @@ var baseHeaders = []http.Header{
 	},
 }
 
-func executeServers(conf dataplane.Configuration) ([]http.Server, map[string][]http.RouteMatch) {
+func executeServers(conf dataplane.Configuration) []executeResult {
 	servers, httpMatchPairs := createServers(conf.HTTPServers, conf.SSLServers)
 
-	return servers, httpMatchPairs
+	serverResult := executeResult{
+		dest: httpConfigFile,
+		data: execute(serversTemplate, servers),
+	}
+
+	// create httpMatchPair conf
+	httpMatchConf, err := json.Marshal(httpMatchPairs)
+	if err != nil {
+		// panic is safe here because we should never fail to marshal the match unless we constructed it incorrectly.
+		panic(fmt.Errorf("could not marshal http match pairs: %w", err))
+	}
+
+	httpMatchResult := executeResult{
+		dest: httpMatchVarsFile,
+		data: httpMatchConf,
+	}
+
+	return []executeResult{serverResult, httpMatchResult}
 }
 
-func createServers(httpServers, sslServers []dataplane.VirtualServer) (
-	[]http.Server,
-	map[string][]http.RouteMatch,
-) {
+func createServers(httpServers, sslServers []dataplane.VirtualServer) ([]http.Server, httpMatchPairs) {
 	servers := make([]http.Server, 0, len(httpServers)+len(sslServers))
-	finalMatchPairs := make(map[string][]http.RouteMatch)
+	finalMatchPairs := make(httpMatchPairs)
 
-	for _, s := range httpServers {
-		httpServer, matchPair := createServer(s)
+	for serverID, s := range httpServers {
+		httpServer, matchPair := createServer(s, serverID)
 		servers = append(servers, httpServer)
-
-		for key, val := range matchPair {
-			finalMatchPairs[key] = val
-		}
+		maps.Copy(finalMatchPairs, matchPair)
 	}
 
-	for _, s := range sslServers {
-		sslServer, matchPair := createSSLServer(s)
+	for serverID, s := range sslServers {
+		sslServer, matchPair := createSSLServer(s, serverID)
 		servers = append(servers, sslServer)
-
-		for key, val := range matchPair {
-			finalMatchPairs[key] = val
-		}
+		maps.Copy(finalMatchPairs, matchPair)
 	}
 
 	return servers, finalMatchPairs
 }
 
-func createSSLServer(virtualServer dataplane.VirtualServer) (
-	http.Server,
-	map[string][]http.RouteMatch,
-) {
+func createSSLServer(virtualServer dataplane.VirtualServer, serverID int) (http.Server, httpMatchPairs) {
 	if virtualServer.IsDefault {
 		return http.Server{
 			IsDefaultSSL: true,
 			Port:         virtualServer.Port,
 		}, nil
 	}
 
-	locs, matchPairs := createLocations(virtualServer)
+	locs, matchPairs := createLocations(&virtualServer, serverID)
 
 	return http.Server{
 		ServerName: virtualServer.Hostname,
@@ -97,18 +103,15 @@ func createSSLServer(virtualServer dataplane.VirtualServer) (
 	}, matchPairs
 }
 
-func createServer(virtualServer dataplane.VirtualServer) (
-	http.Server,
-	map[string][]http.RouteMatch,
-) {
+func createServer(virtualServer dataplane.VirtualServer, serverID int) (http.Server, httpMatchPairs) {
 	if virtualServer.IsDefault {
 		return http.Server{
 			IsDefaultHTTP: true,
 			Port:          virtualServer.Port,
 		}, nil
 	}
 
-	locs, matchPairs := createLocations(virtualServer)
+	locs, matchPairs := createLocations(&virtualServer, serverID)
 
 	return http.Server{
 		ServerName: virtualServer.Hostname,
@@ -124,19 +127,16 @@ type rewriteConfig struct {
 	Rewrite string
 }
 
-type httpMatchPairs map[string][]http.RouteMatch
+type httpMatchPairs map[string][]RouteMatch
 
-func createLocations(server dataplane.VirtualServer) (
-	[]http.Location,
-	map[string][]http.RouteMatch,
-) {
+func createLocations(server *dataplane.VirtualServer, serverID int) ([]http.Location, httpMatchPairs) {
 	maxLocs, pathsAndTypes := getMaxLocationCountAndPathMap(server.PathRules)
 	locs := make([]http.Location, 0, maxLocs)
 	matchPairs := make(httpMatchPairs)
 	var rootPathExists bool
 
 	for pathRuleIdx, rule := range server.PathRules {
-		matches := make([]http.RouteMatch, 0, len(rule.MatchRules))
+		matches := make([]RouteMatch, 0, len(rule.MatchRules))
 
 		if rule.Path == rootPath {
 			rootPathExists = true
@@ -157,9 +157,19 @@ func createLocations(server dataplane.VirtualServer) (
 		}
 
 		if len(matches) > 0 {
-			for i := range extLocations {
-				key := server.Hostname + extLocations[i].Path + strconv.Itoa(int(server.Port))
-				extLocations[i].HTTPMatchKey = sanitizeKey(key)
+			for i, loc := 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)
+				if strings.Contains(loc.Path, "= /") {
+					key += "EXACT"
+				}
+				extLocations[i].HTTPMatchKey = key
 				matchPairs[extLocations[i].HTTPMatchKey] = matches
 			}
 			locs = append(locs, extLocations...)
@@ -173,13 +183,6 @@ func createLocations(server dataplane.VirtualServer) (
 	return locs, matchPairs
 }
 
-// removeSpecialCharacters removes '/', '.' from key and replaces '= ' with 'EXACT',
-// to avoid compilation issues with NJS and NGINX Conf.
-func sanitizeKey(input string) string {
-	s := regexp.MustCompile("[./]").ReplaceAllString(input, "")
-	return regexp.MustCompile("= ").ReplaceAllString(s, "EXACT")
-}
-
 // pathAndTypeMap contains a map of paths and any path types defined for that path
 // for example, {/foo: {exact: {}, prefix: {}}}
 type pathAndTypeMap map[string]map[dataplane.PathType]struct{}
@@ -256,7 +259,7 @@ func initializeInternalLocation(
 	pathruleIdx,
 	matchRuleIdx int,
 	match dataplane.Match,
-) (http.Location, http.RouteMatch) {
+) (http.Location, RouteMatch) {
 	path := fmt.Sprintf("@rule%d-route%d", pathruleIdx, matchRuleIdx)
 	return createMatchLocation(path), createRouteMatch(match, path)
 }
@@ -431,8 +434,26 @@ func createRewritesValForRewriteFilter(filter *dataplane.HTTPURLRewriteFilter, p
 	return rewrites
 }
 
-func createRouteMatch(match dataplane.Match, redirectPath string) http.RouteMatch {
-	hm := http.RouteMatch{
+// httpMatch is an internal representation of an HTTPRouteMatch.
+// This struct is marshaled into a string and stored as a variable in the nginx location block for the route's path.
+// The NJS httpmatches module will look up this variable on the request object and compare the request against the
+// Method, Headers, and QueryParams contained in httpMatch.
+// If the request satisfies the httpMatch, NGINX will redirect the request to the location RedirectPath.
+type RouteMatch struct {
+	// Method is the HTTPMethod of the HTTPRouteMatch.
+	Method string `json:"method,omitempty"`
+	// RedirectPath is the path to redirect the request to if the request satisfies the match conditions.
+	RedirectPath string `json:"redirectPath,omitempty"`
+	// Headers is a list of HTTPHeaders name value pairs with the format "{name}:{value}".
+	Headers []string `json:"headers,omitempty"`
+	// QueryParams is a list of HTTPQueryParams name value pairs with the format "{name}={value}".
+	QueryParams []string `json:"params,omitempty"`
+	// Any represents a match with no match conditions.
+	Any bool `json:"any,omitempty"`
+}
+
+func createRouteMatch(match dataplane.Match, redirectPath string) RouteMatch {
+	hm := RouteMatch{
 		RedirectPath: redirectPath,
 	}
 

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

@@ -1,6 +1,7 @@
 package config
 
 var serversTemplateText = `
+js_preload_object matches from /etc/nginx/conf.d/match.json;
 {{- range $s := . -}}
     {{ if $s.IsDefaultSSL -}}
 server {
@@ -17,7 +18,6 @@ server {
 }
     {{- else }}
 server {
-    js_preload_object matches from /etc/nginx/conf.d/match.json;
         {{- if $s.SSL }}
     listen {{ $s.Port }} ssl;
     ssl_certificate {{ $s.SSL.Certificate }};