fix: Fix reload errors due long matching conditions

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

@@ -23,6 +23,9 @@ const (
 
 	// configVersionFile is the path to the config version configuration file.
 	configVersionFile = httpFolder + "/config-version.conf"
+
+	// httpMatchVarsFile is the path to the http_match pairs configuration file.
+	httpMatchVarsFile = httpFolder + "/matches.json"
 )
 
 // ConfigFolders is a list of folders where NGINX configuration files are stored.
@@ -52,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.
@@ -66,7 +74,7 @@ func (g GeneratorImpl) Generate(conf dataplane.Configuration) []file.File {
 		files = append(files, generatePEM(id, pair.Cert, pair.Key))
 	}
 
-	files = append(files, g.generateHTTPConfig(conf))
+	files = append(files, g.generateHTTPConfig(conf)...)
 
 	files = append(files, generateConfigVersion(conf.Version))
 
@@ -106,24 +114,39 @@ func generateCertBundleFileName(id dataplane.CertBundleID) string {
 	return filepath.Join(secretsFolder, string(id)+".crt")
 }
 
-func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) file.File {
-	var c []byte
+func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) []file.File {
+	files := make([]file.File, 0)
+	fileBytes := make(map[string][]byte)
+
 	for _, execute := range g.getExecuteFuncs() {
-		c = append(c, execute(conf)...)
+		results := execute(conf)
+		for _, res := range results {
+			b, ok := fileBytes[res.dest]
+			if ok {
+				b = append(b, res.data...)
+				fileBytes[res.dest] = b
+			} else {
+				fileBytes[res.dest] = res.data
+			}
+		}
 	}
 
-	return file.File{
-		Content: c,
-		Path:    httpConfigFile,
-		Type:    file.TypeRegular,
+	for filepath, bytes := range fileBytes {
+		files = append(files, file.File{
+			Path:    filepath,
+			Content: bytes,
+			Type:    file.TypeRegular,
+		})
 	}
+
+	return files
 }
 
 func (g GeneratorImpl) getExecuteFuncs() []executeFunc {
 	return []executeFunc{
+		executeServers,
 		g.executeUpstreams,
 		executeSplitClients,
-		executeServers,
 		executeMaps,
 	}
 }

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

@@ -70,7 +70,7 @@ func TestGenerate(t *testing.T) {
 
 	files := generator.Generate(conf)
 
-	g.Expect(files).To(HaveLen(4))
+	g.Expect(files).To(HaveLen(5))
 
 	g.Expect(files[0]).To(Equal(file.File{
 		Type:    file.TypeSecret,
@@ -88,12 +88,17 @@ func TestGenerate(t *testing.T) {
 	g.Expect(httpCfg).To(ContainSubstring("upstream"))
 	g.Expect(httpCfg).To(ContainSubstring("split_clients"))
 
+	g.Expect(files[2].Path).To(Equal("/etc/nginx/conf.d/matches.json"))
 	g.Expect(files[2].Type).To(Equal(file.TypeRegular))
-	g.Expect(files[2].Path).To(Equal("/etc/nginx/conf.d/config-version.conf"))
-	configVersion := string(files[2].Content)
+	expString := "{}"
+	g.Expect(string(files[2].Content)).To(Equal(expString))
+
+	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)
 	g.Expect(configVersion).To(ContainSubstring(fmt.Sprintf("return 200 %d", conf.Version)))
 
-	g.Expect(files[3].Path).To(Equal("/etc/nginx/secrets/test-certbundle.crt"))
-	certBundle := string(files[3].Content)
+	g.Expect(files[4].Path).To(Equal("/etc/nginx/secrets/test-certbundle.crt"))
+	certBundle := string(files[4].Content)
 	g.Expect(certBundle).To(Equal("test-cert"))
 }

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

@@ -12,13 +12,13 @@ type Server struct {
 
 // Location holds all configuration for an HTTP location.
 type Location struct {
-	Return          *Return
-	ProxySSLVerify  *ProxySSLVerify
 	Path            string
 	ProxyPass       string
-	HTTPMatchVar    string
-	Rewrites        []string
+	HTTPMatchKey    string
 	ProxySetHeaders []Header
+	ProxySSLVerify  *ProxySSLVerify
+	Return          *Return
+	Rewrites        []string
 }
 
 // Header defines a HTTP header to be passed to the proxied server.

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,10 @@ func TestExecuteMaps(t *testing.T) {
 		"map $http_upgrade $connection_upgrade {":                             1,
 	}
 
-	maps := string(executeMaps(conf))
+	mapResult := executeMaps(conf)
+	maps := string(mapResult[0].data)
+	g.Expect(mapResult).To(HaveLen(1))
+	g.Expect(mapResult[0].dest).To(Equal(httpConfigFile))
 	for expSubStr, expCount := range expSubStrings {
 		g.Expect(expCount).To(Equal(strings.Count(maps, expSubStr)))
 	}

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

@@ -3,6 +3,8 @@
 import (
 	"encoding/json"
 	"fmt"
+	"maps"
+	"strconv"
 	"strings"
 	gotemplate "text/template"
 
@@ -38,58 +40,84 @@
 	},
 }
 
-func executeServers(conf dataplane.Configuration) []byte {
-	servers := createServers(conf.HTTPServers, conf.SSLServers)
+func executeServers(conf dataplane.Configuration) []executeResult {
+	servers, httpMatchPairs := createServers(conf.HTTPServers, conf.SSLServers)
 
-	return execute(serversTemplate, servers)
+	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 {
+func createServers(httpServers, sslServers []dataplane.VirtualServer) ([]http.Server, httpMatchPairs) {
 	servers := make([]http.Server, 0, len(httpServers)+len(sslServers))
+	finalMatchPairs := make(httpMatchPairs)
 
-	for _, s := range httpServers {
-		servers = append(servers, createServer(s))
+	for serverID, s := range httpServers {
+		httpServer, matchPairs := createServer(s, serverID)
+		servers = append(servers, httpServer)
+		maps.Copy(finalMatchPairs, matchPairs)
 	}
 
-	for _, s := range sslServers {
-		servers = append(servers, createSSLServer(s))
+	for serverID, s := range sslServers {
+		sslServer, matchPair := createSSLServer(s, serverID)
+		servers = append(servers, sslServer)
+		maps.Copy(finalMatchPairs, matchPair)
 	}
 
-	return servers
+	return servers, finalMatchPairs
 }
 
-func createSSLServer(virtualServer dataplane.VirtualServer) http.Server {
+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, serverID)
+
 	return http.Server{
 		ServerName: virtualServer.Hostname,
 		SSL: &http.SSL{
 			Certificate:    generatePEMFileName(virtualServer.SSL.KeyPairID),
 			CertificateKey: generatePEMFileName(virtualServer.SSL.KeyPairID),
 		},
-		Locations: createLocations(virtualServer.PathRules, virtualServer.Port),
+		Locations: locs,
 		Port:      virtualServer.Port,
-	}
+	}, matchPairs
 }
 
-func createServer(virtualServer dataplane.VirtualServer) http.Server {
+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, serverID)
+
 	return http.Server{
 		ServerName: virtualServer.Hostname,
-		Locations:  createLocations(virtualServer.PathRules, virtualServer.Port),
+		Locations:  locs,
 		Port:       virtualServer.Port,
-	}
+	}, matchPairs
 }
 
 // rewriteConfig contains the configuration for a location to rewrite paths,
@@ -99,13 +127,16 @@
 	Rewrite string
 }
 
-func createLocations(pathRules []dataplane.PathRule, listenerPort int32) []http.Location {
-	maxLocs, pathsAndTypes := getMaxLocationCountAndPathMap(pathRules)
+type httpMatchPairs map[string][]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 pathRules {
-		matches := make([]httpMatch, 0, len(rule.MatchRules))
+	for pathRuleIdx, rule := range server.PathRules {
+		matches := make([]routeMatch, 0, len(rule.MatchRules))
 
 		if rule.Path == rootPath {
 			rootPathExists = true
@@ -121,14 +152,22 @@
 				matches = append(matches, match)
 			}
 
-			buildLocations = updateLocationsForFilters(r.Filters, buildLocations, r, listenerPort, rule.Path)
+			buildLocations = updateLocationsForFilters(r.Filters, buildLocations, r, server.Port, rule.Path)
 			locs = append(locs, buildLocations...)
 		}
 
 		if len(matches) > 0 {
-			matchesStr := convertMatchesToString(matches)
 			for i := range extLocations {
-				extLocations[i].HTTPMatchVar = matchesStr
+				// 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...)
 		}
@@ -138,7 +177,7 @@
 		locs = append(locs, createDefaultRootLocation())
 	}
 
-	return locs
+	return locs, matchPairs
 }
 
 // pathAndTypeMap contains a map of paths and any path types defined for that path
@@ -217,9 +256,9 @@
 	pathruleIdx,
 	matchRuleIdx int,
 	match dataplane.Match,
-) (http.Location, httpMatch) {
+) (http.Location, routeMatch) {
 	path := fmt.Sprintf("@rule%d-route%d", pathruleIdx, matchRuleIdx)
-	return createMatchLocation(path), createHTTPMatch(match, path)
+	return createMatchLocation(path), createRouteMatch(match, path)
 }
 
 // updateLocationsForFilters updates the existing locations with any relevant filters.
@@ -392,12 +431,12 @@
 	return rewrites
 }
 
-// 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.
+// routeMatch is an internal representation of an HTTPRouteMatch.
+// This struct is stored as a key-value pair in /etc/nginx/conf.d/matches.json with a key for the route's path.
+// The NJS httpmatches module will look up key specified in the nginx location 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 httpMatch struct {
+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.
@@ -410,8 +449,8 @@
 	Any bool `json:"any,omitempty"`
 }
 
-func createHTTPMatch(match dataplane.Match, redirectPath string) httpMatch {
-	hm := httpMatch{
+func createRouteMatch(match dataplane.Match, redirectPath string) routeMatch {
+	hm := routeMatch{
 		RedirectPath: redirectPath,
 	}
 
@@ -558,19 +597,6 @@
 	return locHeaders
 }
 
-func convertMatchesToString(matches []httpMatch) string {
-	// 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
-	b, err := json.Marshal(matches)
-	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: %w", err))
-	}
-
-	return string(b)
-}
-
 func exactPath(path string) string {
 	return fmt.Sprintf("= %s", path)
 }