diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index 4917a80cac..6127ffa094 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/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,33 @@ 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 { + fileBytes := make(map[string][]byte) + for _, execute := range g.getExecuteFuncs() { - c = append(c, execute(conf)...) + results := execute(conf) + for _, res := range results { + fileBytes[res.dest] = append(fileBytes[res.dest], res.data...) + } } - return file.File{ - Content: c, - Path: httpConfigFile, - Type: file.TypeRegular, + files := make([]file.File, 0, len(fileBytes)) + 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, } } diff --git a/internal/mode/static/nginx/config/generator_test.go b/internal/mode/static/nginx/config/generator_test.go index 30fd4befc8..a4e334ea74 100644 --- a/internal/mode/static/nginx/config/generator_test.go +++ b/internal/mode/static/nginx/config/generator_test.go @@ -2,6 +2,7 @@ package config_test import ( "fmt" + "sort" "testing" . "github.com/onsi/gomega" @@ -70,13 +71,16 @@ func TestGenerate(t *testing.T) { files := generator.Generate(conf) - g.Expect(files).To(HaveLen(4)) + g.Expect(files).To(HaveLen(5)) + arrange := func(i, j int) bool { + return files[i].Path < files[j].Path + } + sort.Slice(files, arrange) - g.Expect(files[0]).To(Equal(file.File{ - Type: file.TypeSecret, - Path: "/etc/nginx/secrets/test-keypair.pem", - Content: []byte("test-cert\ntest-key"), - })) + g.Expect(files[0].Type).To(Equal(file.TypeRegular)) + g.Expect(files[0].Path).To(Equal("/etc/nginx/conf.d/config-version.conf")) + configVersion := string(files[0].Content) + g.Expect(configVersion).To(ContainSubstring(fmt.Sprintf("return 200 %d", conf.Version))) g.Expect(files[1].Type).To(Equal(file.TypeRegular)) g.Expect(files[1].Path).To(Equal("/etc/nginx/conf.d/http.conf")) @@ -88,12 +92,18 @@ 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) - g.Expect(configVersion).To(ContainSubstring(fmt.Sprintf("return 200 %d", conf.Version))) + expString := "{}" + g.Expect(string(files[2].Content)).To(Equal(expString)) g.Expect(files[3].Path).To(Equal("/etc/nginx/secrets/test-certbundle.crt")) certBundle := string(files[3].Content) g.Expect(certBundle).To(Equal("test-cert")) + + g.Expect(files[4]).To(Equal(file.File{ + Type: file.TypeSecret, + Path: "/etc/nginx/secrets/test-keypair.pem", + Content: []byte("test-cert\ntest-key"), + })) } diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index 8486b7d464..5db1e061fe 100644 --- a/internal/mode/static/nginx/config/http/config.go +++ b/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. diff --git a/internal/mode/static/nginx/config/maps.go b/internal/mode/static/nginx/config/maps.go index e3cb7d78a5..a4d6c419b6 100644 --- a/internal/mode/static/nginx/config/maps.go +++ b/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 { diff --git a/internal/mode/static/nginx/config/maps_test.go b/internal/mode/static/nginx/config/maps_test.go index 1e6f1a30a9..e903c17151 100644 --- a/internal/mode/static/nginx/config/maps_test.go +++ b/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) + g.Expect(mapResult).To(HaveLen(1)) + maps := string(mapResult[0].data) + g.Expect(mapResult[0].dest).To(Equal(httpConfigFile)) for expSubStr, expCount := range expSubStrings { g.Expect(expCount).To(Equal(strings.Count(maps, expSubStr))) } diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index a80de123f7..58c1602c24 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -3,6 +3,8 @@ package config import ( "encoding/json" "fmt" + "maps" + "strconv" "strings" gotemplate "text/template" @@ -38,58 +40,84 @@ var baseHeaders = []http.Header{ }, } -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 @@ type rewriteConfig struct { 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 @@ func createLocations(pathRules []dataplane.PathRule, listenerPort int32) []http. 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 @@ func createLocations(pathRules []dataplane.PathRule, listenerPort int32) []http. 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 @@ func initializeInternalLocation( 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 @@ func createRewritesValForRewriteFilter(filter *dataplane.HTTPURLRewriteFilter, p 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. -// If the request satisfies the httpMatch, NGINX will redirect the request to the location RedirectPath. -type httpMatch struct { +// 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 routeMatch. +// If the request satisfies the routeMatch, 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. @@ -410,8 +449,8 @@ type httpMatch struct { 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 @@ func convertSetHeaders(headers []dataplane.HTTPHeader) []http.Header { 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) } diff --git a/internal/mode/static/nginx/config/servers_template.go b/internal/mode/static/nginx/config/servers_template.go index dbf37575ae..8d272e05a7 100644 --- a/internal/mode/static/nginx/config/servers_template.go +++ b/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/matches.json; {{- range $s := . -}} {{ if $s.IsDefaultSSL -}} server { @@ -41,8 +42,8 @@ server { return {{ $l.Return.Code }} "{{ $l.Return.Body }}"; {{- end }} - {{- if $l.HTTPMatchVar }} - set $http_matches {{ $l.HTTPMatchVar | printf "%q" }}; + {{- if $l.HTTPMatchKey }} + set $match_key {{ $l.HTTPMatchKey }}; js_content httpmatches.redirect; {{- end }} diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 218e59d9d3..3e931fe1e4 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -1,8 +1,8 @@ package config import ( - "encoding/json" "fmt" + "maps" "strings" "testing" @@ -63,9 +63,13 @@ func TestExecuteServers(t *testing.T) { "ssl_certificate_key /etc/nginx/secrets/test-keypair.pem;": 2, } g := NewWithT(t) - servers := string(executeServers(conf)) + serverResults := executeServers(conf) + g.Expect(serverResults).To(HaveLen(2)) + serverConf := string(serverResults[0].data) + httpMatchConf := string(serverResults[1].data) + g.Expect(httpMatchConf).To(Equal("{}")) for expSubStr, expCount := range expSubStrings { - g.Expect(strings.Count(servers, expSubStr)).To(Equal(expCount)) + g.Expect(strings.Count(serverConf, expSubStr)).To(Equal(expCount)) } } @@ -140,14 +144,18 @@ func TestExecuteForDefaultServers(t *testing.T) { t.Run(tc.msg, func(t *testing.T) { g := NewWithT(t) - cfg := string(executeServers(tc.conf)) + serverResults := executeServers(tc.conf) + g.Expect(serverResults).To(HaveLen(2)) + serverConf := string(serverResults[0].data) + httpMatchConf := string(serverResults[1].data) + g.Expect(httpMatchConf).To(Equal("{}")) for _, expPort := range tc.httpPorts { - g.Expect(cfg).To(ContainSubstring(fmt.Sprintf(httpDefaultFmt, expPort))) + g.Expect(serverConf).To(ContainSubstring(fmt.Sprintf(httpDefaultFmt, expPort))) } for _, expPort := range tc.sslPorts { - g.Expect(cfg).To(ContainSubstring(fmt.Sprintf(sslDefaultFmt, expPort))) + g.Expect(serverConf).To(ContainSubstring(fmt.Sprintf(sslDefaultFmt, expPort))) } }) } @@ -508,44 +516,51 @@ func TestCreateServers(t *testing.T) { }, } - expectedMatchString := func(m []httpMatch) string { - g := NewWithT(t) - b, err := json.Marshal(m) - g.Expect(err).ToNot(HaveOccurred()) - return string(b) - } - - slashMatches := []httpMatch{ - {Method: "POST", RedirectPath: "@rule0-route0"}, - {Method: "PATCH", RedirectPath: "@rule0-route1"}, - {Any: true, RedirectPath: "@rule0-route2"}, - } - testMatches := []httpMatch{ - { - Method: "GET", - Headers: []string{"Version:V1", "test:foo", "my-header:my-value"}, - QueryParams: []string{"GrEat=EXAMPLE", "test=foo=bar"}, - RedirectPath: "@rule1-route0", + expMatchPairs := httpMatchPairs{ + "1_0": { + {Method: "POST", RedirectPath: "@rule0-route0"}, + {Method: "PATCH", RedirectPath: "@rule0-route1"}, + {RedirectPath: "@rule0-route2", Any: true}, }, - } - exactMatches := []httpMatch{ - { - Method: "GET", - RedirectPath: "@rule12-route0", + "1_1": { + { + Method: "GET", + Headers: []string{"Version:V1", "test:foo", "my-header:my-value"}, + QueryParams: []string{"GrEat=EXAMPLE", "test=foo=bar"}, + RedirectPath: "@rule1-route0", + }, }, - } - redirectHeaderMatches := []httpMatch{ - { - Headers: []string{"redirect:this"}, - RedirectPath: "@rule6-route0", + "1_6": { + {RedirectPath: "@rule6-route0", Headers: []string{"redirect:this"}}, }, - } - rewriteHeaderMatches := []httpMatch{ - { - Headers: []string{"rewrite:this"}, - RedirectPath: "@rule8-route0", + "1_8": { + { + Headers: []string{"rewrite:this"}, + RedirectPath: "@rule8-route0", + }, + }, + "1_10": { + { + Headers: []string{"filter:this"}, + RedirectPath: "@rule10-route0", + }, + }, + "1_12": { + { + Method: "GET", + RedirectPath: "@rule12-route0", + Headers: nil, + QueryParams: nil, + Any: false, + }, }, } + + allExpMatchPair := make(httpMatchPairs) + maps.Copy(allExpMatchPair, expMatchPairs) + modifiedMatchPairs := modifyMatchPairs(expMatchPairs) + maps.Copy(allExpMatchPair, modifiedMatchPairs) + rewriteProxySetHeaders := []http.Header{ { Name: "Host", @@ -564,17 +579,13 @@ func TestCreateServers(t *testing.T) { Value: "$connection_upgrade", }, } - invalidFilterHeaderMatches := []httpMatch{ - { - Headers: []string{"filter:this"}, - RedirectPath: "@rule10-route0", - }, - } getExpectedLocations := func(isHTTPS bool) []http.Location { port := 8080 + ssl := "" if isHTTPS { port = 8443 + ssl = "SSL" } return []http.Location{ @@ -595,7 +606,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "/", - HTTPMatchVar: expectedMatchString(slashMatches), + HTTPMatchKey: ssl + "1_0", }, { Path: "@rule1-route0", @@ -604,7 +615,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "/test/", - HTTPMatchVar: expectedMatchString(testMatches), + HTTPMatchKey: ssl + "1_1", }, { Path: "/path-only/", @@ -671,11 +682,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/redirect-with-headers/", - HTTPMatchVar: expectedMatchString(redirectHeaderMatches), + HTTPMatchKey: ssl + "1_6", }, { Path: "= /redirect-with-headers", - HTTPMatchVar: expectedMatchString(redirectHeaderMatches), + HTTPMatchKey: ssl + "1_6", }, { Path: "/rewrite/", @@ -697,11 +708,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/rewrite-with-headers/", - HTTPMatchVar: expectedMatchString(rewriteHeaderMatches), + HTTPMatchKey: ssl + "1_8", }, { Path: "= /rewrite-with-headers", - HTTPMatchVar: expectedMatchString(rewriteHeaderMatches), + HTTPMatchKey: ssl + "1_8", }, { Path: "/invalid-filter/", @@ -723,11 +734,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/invalid-filter-with-headers/", - HTTPMatchVar: expectedMatchString(invalidFilterHeaderMatches), + HTTPMatchKey: ssl + "1_10", }, { Path: "= /invalid-filter-with-headers", - HTTPMatchVar: expectedMatchString(invalidFilterHeaderMatches), + HTTPMatchKey: ssl + "1_10", }, { Path: "= /exact", @@ -741,7 +752,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "= /test", - HTTPMatchVar: expectedMatchString(exactMatches), + HTTPMatchKey: ssl + "1_12", }, { Path: "/proxy-set-headers/", @@ -827,10 +838,22 @@ func TestCreateServers(t *testing.T) { g := NewWithT(t) - result := createServers(httpServers, sslServers) + result, httpMatchPair := createServers(httpServers, sslServers) + + g.Expect(httpMatchPair).To(Equal(allExpMatchPair)) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) } +func modifyMatchPairs(matchPairs httpMatchPairs) httpMatchPairs { + modified := make(httpMatchPairs) + for k, v := range matchPairs { + modifiedKey := "SSL" + k + modified[modifiedKey] = v + } + + return modified +} + func TestCreateServersConflicts(t *testing.T) { fooGroup := dataplane.BackendGroup{ Source: types.NamespacedName{Namespace: "test", Name: "route"}, @@ -1024,7 +1047,7 @@ func TestCreateServersConflicts(t *testing.T) { g := NewWithT(t) - result := createServers(httpServers, []dataplane.VirtualServer{}) + result, _ := createServers(httpServers, []dataplane.VirtualServer{}) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) }) } @@ -1148,8 +1171,12 @@ func TestCreateLocationsRootPath(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - locs := createLocations(test.pathRules, 80) + locs, httpMatchPair := createLocations(&dataplane.VirtualServer{ + PathRules: test.pathRules, + Port: 80, + }, 1) g.Expect(locs).To(Equal(test.expLocations)) + g.Expect(httpMatchPair).To(BeEmpty()) }) } } @@ -1404,7 +1431,7 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) { } } -func TestCreateHTTPMatch(t *testing.T) { +func TestCreateRouteMatch(t *testing.T) { testPath := "/internal_loc" testMethodMatch := helpers.GetPointer("PUT") @@ -1452,11 +1479,11 @@ func TestCreateHTTPMatch(t *testing.T) { tests := []struct { match dataplane.Match msg string - expected httpMatch + expected routeMatch }{ { match: dataplane.Match{}, - expected: httpMatch{ + expected: routeMatch{ Any: true, RedirectPath: testPath, }, @@ -1466,7 +1493,7 @@ func TestCreateHTTPMatch(t *testing.T) { match: dataplane.Match{ Method: testMethodMatch, // A path match with a method should not set the Any field to true }, - expected: httpMatch{ + expected: routeMatch{ Method: "PUT", RedirectPath: testPath, }, @@ -1476,7 +1503,7 @@ func TestCreateHTTPMatch(t *testing.T) { match: dataplane.Match{ Headers: testHeaderMatches, }, - expected: httpMatch{ + expected: routeMatch{ RedirectPath: testPath, Headers: expectedHeaders, }, @@ -1486,7 +1513,7 @@ func TestCreateHTTPMatch(t *testing.T) { match: dataplane.Match{ QueryParams: testQueryParamMatches, }, - expected: httpMatch{ + expected: routeMatch{ QueryParams: expectedArgs, RedirectPath: testPath, }, @@ -1497,7 +1524,7 @@ func TestCreateHTTPMatch(t *testing.T) { Method: testMethodMatch, QueryParams: testQueryParamMatches, }, - expected: httpMatch{ + expected: routeMatch{ Method: "PUT", QueryParams: expectedArgs, RedirectPath: testPath, @@ -1509,7 +1536,7 @@ func TestCreateHTTPMatch(t *testing.T) { Method: testMethodMatch, Headers: testHeaderMatches, }, - expected: httpMatch{ + expected: routeMatch{ Method: "PUT", Headers: expectedHeaders, RedirectPath: testPath, @@ -1521,7 +1548,7 @@ func TestCreateHTTPMatch(t *testing.T) { QueryParams: testQueryParamMatches, Headers: testHeaderMatches, }, - expected: httpMatch{ + expected: routeMatch{ QueryParams: expectedArgs, Headers: expectedHeaders, RedirectPath: testPath, @@ -1534,7 +1561,7 @@ func TestCreateHTTPMatch(t *testing.T) { QueryParams: testQueryParamMatches, Method: testMethodMatch, }, - expected: httpMatch{ + expected: routeMatch{ Method: "PUT", Headers: expectedHeaders, QueryParams: expectedArgs, @@ -1546,7 +1573,7 @@ func TestCreateHTTPMatch(t *testing.T) { match: dataplane.Match{ Headers: testDuplicateHeaders, }, - expected: httpMatch{ + expected: routeMatch{ Headers: expectedHeaders, RedirectPath: testPath, }, @@ -1557,7 +1584,7 @@ func TestCreateHTTPMatch(t *testing.T) { t.Run(tc.msg, func(t *testing.T) { g := NewWithT(t) - result := createHTTPMatch(tc.match, testPath) + result := createRouteMatch(tc.match, testPath) g.Expect(helpers.Diff(result, tc.expected)).To(BeEmpty()) }) } diff --git a/internal/mode/static/nginx/config/split_clients.go b/internal/mode/static/nginx/config/split_clients.go index 61d63aaec0..7d74ad18be 100644 --- a/internal/mode/static/nginx/config/split_clients.go +++ b/internal/mode/static/nginx/config/split_clients.go @@ -11,10 +11,15 @@ import ( var splitClientsTemplate = gotemplate.Must(gotemplate.New("split_clients").Parse(splitClientsTemplateText)) -func executeSplitClients(conf dataplane.Configuration) []byte { +func executeSplitClients(conf dataplane.Configuration) []executeResult { splitClients := createSplitClients(conf.BackendGroups) - return execute(splitClientsTemplate, splitClients) + result := executeResult{ + dest: httpConfigFile, + data: execute(splitClientsTemplate, splitClients), + } + + return []executeResult{result} } func createSplitClients(backendGroups []dataplane.BackendGroup) []http.SplitClient { diff --git a/internal/mode/static/nginx/config/split_clients_test.go b/internal/mode/static/nginx/config/split_clients_test.go index 31e0ed445b..9ee6785ca9 100644 --- a/internal/mode/static/nginx/config/split_clients_test.go +++ b/internal/mode/static/nginx/config/split_clients_test.go @@ -98,7 +98,10 @@ func TestExecuteSplitClients(t *testing.T) { for _, test := range tests { t.Run(test.msg, func(t *testing.T) { g := NewWithT(t) - sc := string(executeSplitClients(dataplane.Configuration{BackendGroups: test.backendGroups})) + splitResults := executeSplitClients(dataplane.Configuration{BackendGroups: test.backendGroups}) + g.Expect(splitResults).To(HaveLen(1)) + sc := string(splitResults[0].data) + g.Expect(splitResults[0].dest).To(Equal(httpConfigFile)) for _, expSubString := range test.expStrings { g.Expect(sc).To(ContainSubstring(expSubString)) diff --git a/internal/mode/static/nginx/config/upstreams.go b/internal/mode/static/nginx/config/upstreams.go index 5d941890f3..f2ff54a90d 100644 --- a/internal/mode/static/nginx/config/upstreams.go +++ b/internal/mode/static/nginx/config/upstreams.go @@ -25,10 +25,14 @@ const ( invalidBackendZoneSize = "32k" ) -func (g GeneratorImpl) executeUpstreams(conf dataplane.Configuration) []byte { +func (g GeneratorImpl) executeUpstreams(conf dataplane.Configuration) []executeResult { upstreams := g.createUpstreams(conf.Upstreams) - return execute(upstreamsTemplate, upstreams) + result := executeResult{ + dest: httpConfigFile, + data: execute(upstreamsTemplate, upstreams), + } + return []executeResult{result} } func (g GeneratorImpl) createUpstreams(upstreams []dataplane.Upstream) []http.Upstream { diff --git a/internal/mode/static/nginx/config/upstreams_test.go b/internal/mode/static/nginx/config/upstreams_test.go index 851dd776b1..8e98e4d794 100644 --- a/internal/mode/static/nginx/config/upstreams_test.go +++ b/internal/mode/static/nginx/config/upstreams_test.go @@ -47,8 +47,12 @@ func TestExecuteUpstreams(t *testing.T) { "server unix:/var/lib/nginx/nginx-502-server.sock;", } - upstreams := string(gen.executeUpstreams(dataplane.Configuration{Upstreams: stateUpstreams})) + upstreamResults := gen.executeUpstreams(dataplane.Configuration{Upstreams: stateUpstreams}) g := NewWithT(t) + g.Expect(upstreamResults).To(HaveLen(1)) + upstreams := string(upstreamResults[0].data) + + g.Expect(upstreamResults[0].dest).To(Equal(httpConfigFile)) for _, expSubString := range expectedSubStrings { g.Expect(upstreams).To(ContainSubstring(expSubString)) } diff --git a/internal/mode/static/nginx/modules/src/httpmatches.js b/internal/mode/static/nginx/modules/src/httpmatches.js index 6478b12991..1b4eb0be36 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/internal/mode/static/nginx/modules/src/httpmatches.js @@ -1,30 +1,55 @@ -import qs from 'querystring'; - -const MATCHES_VARIABLE = 'http_matches'; +const MATCHES_KEY = 'match_key'; const HTTP_CODES = { notFound: 404, internalServerError: 500, }; function redirect(r) { - let matches; - + let matchList; try { - matches = extractMatchesFromRequest(r); + matchList = extractMatchesFromRequest(r, matches); } catch (e) { r.error(e.message); r.return(HTTP_CODES.internalServerError); return; } + redirectForMatchList(r, matchList); +} + +function extractMatchesFromRequest(r, matches) { + let matchList; + if (!r.variables[MATCHES_KEY]) { + throw Error( + `cannot redirect the request; the ${MATCHES_KEY} is not defined on the request object`, + ); + } - // Matches is a list of http matches in order of precedence. + let key = r.variables[MATCHES_KEY]; + if (!matches[key]) { + throw Error( + `cannot redirect the request; the key ${key} is not defined on the matches object`, + ); + } + + // matchList is a list of http matches in order of precedence. // We will accept the first match that the request satisfies. // If there's a match, redirect request to internal location block. // If an exception occurs, return 500. // If no matches are found, return 404. + matchList = matches[key]; + try { + verifyMatchList(matchList); + } catch (e) { + throw Error(`cannot redirect the request; ${matchList} matches list is not valid: ${e}`); + } + + return matchList; +} + +function redirectForMatchList(r, matchList) { let match; try { - match = findWinningMatch(r, matches); + match = findWinningMatch(r, matchList); } catch (e) { r.error(e.message); r.return(HTTP_CODES.internalServerError); @@ -49,32 +74,16 @@ function redirect(r) { r.internalRedirect(match.redirectPath); } -function extractMatchesFromRequest(r) { - if (!r.variables[MATCHES_VARIABLE]) { - throw Error( - `cannot redirect the request; the variable ${MATCHES_VARIABLE} is not defined on the request object`, - ); +function verifyMatchList(matchList) { + if (!Array.isArray(matchList)) { + throw Error(`cannot redirect the request; expected a list of matches, got ${matchList}`); } - let matches; - - try { - matches = JSON.parse(r.variables[MATCHES_VARIABLE]); - } catch (e) { - throw Error( - `cannot redirect the request; error parsing ${r.variables[MATCHES_VARIABLE]} into a JSON object: ${e}`, - ); - } - - if (!Array.isArray(matches)) { - throw Error(`cannot redirect the request; expected a list of matches, got ${matches}`); - } - - if (matches.length === 0) { + if (matchList.length === 0) { throw Error(`cannot redirect the request; matches is an empty list`); } - return matches; + return; } function findWinningMatch(r, matches) { @@ -199,11 +208,13 @@ function paramsMatch(requestParams, params) { export default { redirect, + redirectForMatchList, + extractMatchesFromRequest, + MATCHES_KEY, + verifyMatchList, testMatch, findWinningMatch, headersMatch, paramsMatch, - extractMatchesFromRequest, HTTP_CODES, - MATCHES_VARIABLE, }; diff --git a/internal/mode/static/nginx/modules/test/httpmatches.test.js b/internal/mode/static/nginx/modules/test/httpmatches.test.js index 0a1ca26aba..d4f502ace9 100644 --- a/internal/mode/static/nginx/modules/test/httpmatches.test.js +++ b/internal/mode/static/nginx/modules/test/httpmatches.test.js @@ -3,7 +3,7 @@ import { assert, 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 -function createRequest({ method = '', headers = {}, params = {}, matches = '' } = {}) { +function createRequest({ method = '', headers = {}, params = {}, matchKey = '' } = {}) { let r = { // Test mocks return(statusCode) { @@ -30,8 +30,8 @@ function createRequest({ method = '', headers = {}, params = {}, matches = '' } r.args = params; } - if (matches) { - r.variables[hm.MATCHES_VARIABLE] = matches; + if (matchKey) { + r.variables[hm.MATCHES_KEY] = matchKey; } return r; @@ -40,43 +40,76 @@ function createRequest({ method = '', headers = {}, params = {}, matches = '' } describe('extractMatchesFromRequest', () => { const tests = [ { - name: 'throws if matches variable does not exist on request', - request: createRequest(), + name: 'throws if match_key variable does not exist on request', + request: createRequest({}), + matchesObject: {}, + expectThrow: true, + errSubstring: 'match_key is not defined', + }, + { + name: 'throws if key does not exist on matches object', + request: createRequest({ matchKey: 'test' }), + matchesObject: {}, expectThrow: true, - errSubstring: 'http_matches is not defined', + errSubstring: 'the key test is not defined on the matches object', }, { - name: 'throws if matches variable is not JSON', - request: createRequest({ matches: 'not-JSON' }), + name: 'throws an error if matchList is not valid', + request: createRequest({ matchKey: 'test' }), + matchesObject: { test: {} }, expectThrow: true, - errSubstring: 'error parsing', + errSubstring: + 'matches list is not valid: Error: cannot redirect the request; expected a list of matches', }, + { + name: 'does not throw if matches key is present & expected matchList is returned', + request: createRequest({ matchKey: 'test' }), + matchesObject: { test: [{ match: 'value' }] }, + expectThrow: false, + expected: [{ match: 'value' }], + }, + ]; + tests.forEach((test) => { + it(test.name, () => { + if (test.expectThrow) { + expect(() => + hm.extractMatchesFromRequest(test.request, test.matchesObject), + ).to.throw(test.errSubstring); + } else { + expect( + hm.extractMatchesFromRequest(test.request, test.matchesObject), + ).to.deep.equal(test.expected); + } + }); + }); +}); + +describe('verifyMatchList', () => { + const tests = [ { name: 'throws if matches variable is not an array', - request: createRequest({ matches: '{}' }), + matchList: '{}', expectThrow: true, errSubstring: 'expected a list of matches', }, { name: 'throws if the length of the matches variable is zero', - request: createRequest({ matches: '[]' }), + matchList: [], expectThrow: true, errSubstring: 'matches is an empty list', }, { name: 'does not throw if matches variable is expected list of matches', - request: createRequest({ matches: '[{"any":true}]' }), + matchList: '[{"any":true}]', expectThrow: false, }, ]; tests.forEach((test) => { it(test.name, () => { if (test.expectThrow) { - expect(() => hm.extractMatchesFromRequest(test.request)).to.throw( - test.errSubstring, - ); + expect(() => hm.verifyMatchList(test.matchList)).to.throw(test.errSubstring); } else { - expect(() => hm.extractMatchesFromRequest(test.request).to.not.throw()); + expect(() => hm.verifyMatchList(test.matchList).to.not.throw()); } }); }); @@ -195,10 +228,6 @@ describe('findWinningMatch', () => { tests.forEach((test) => { it(test.name, () => { - test.request.variables = { - http_matches: JSON.stringify(test.matches), - }; - if (test.expectThrow) { expect(() => hm.findWinningMatch(test.request, test.matches)).to.throw( test.errSubstring, @@ -393,7 +422,7 @@ describe('paramsMatch', () => { }); }); -describe('redirect', () => { +describe('redirectForMatchList', () => { const testAnyMatch = { any: true, redirectPath: '/any' }; const testHeaderMatches = { headers: ['header1:VALUE1', 'header2:value2', 'header3:value3'], @@ -412,13 +441,13 @@ describe('redirect', () => { const tests = [ { - name: 'returns Internal Server Error status code if http_matches variable is not set', + name: 'returns Internal Server Error status code if match variable is not set', request: createRequest(), matches: null, expectedReturn: hm.HTTP_CODES.internalServerError, }, { - name: 'returns Internal Server Error status code if http_matches contains malformed match', + name: 'returns Internal Server Error status code if matchList contains malformed match', request: createRequest(), matches: [{ headers: ['malformedheader'] }], expectedReturn: hm.HTTP_CODES.internalServerError, @@ -449,14 +478,7 @@ describe('redirect', () => { tests.forEach((test) => { it(test.name, () => { - if (test.matches) { - // set http_matches variable - test.request.variables = { - http_matches: JSON.stringify(test.matches), - }; - } - - hm.redirect(test.request); + hm.redirectForMatchList(test.request, test.matches); if (test.expectedReturn) { expect(test.request.testReturned).to.equal(test.expectedReturn); } else if (test.expectedRedirect) {