diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index b1157cd56a..bf63b1afe1 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/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, diff --git a/internal/mode/static/nginx/config/generator_test.go b/internal/mode/static/nginx/config/generator_test.go index 50bfc85463..648fe341bb 100644 --- a/internal/mode/static/nginx/config/generator_test.go +++ b/internal/mode/static/nginx/config/generator_test.go @@ -78,9 +78,14 @@ 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")) @@ -88,9 +93,6 @@ 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/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) diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index a4acc953b9..5db1e061fe 100644 --- a/internal/mode/static/nginx/config/http/config.go +++ b/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"` -} 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..20c914b9b8 100644 --- a/internal/mode/static/nginx/config/maps_test.go +++ b/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))) } diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 162bad2d33..746a6b582d 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/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,44 +40,49 @@ 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, @@ -84,7 +90,7 @@ func createSSLServer(virtualServer dataplane.VirtualServer) ( }, nil } - locs, matchPairs := createLocations(virtualServer) + locs, matchPairs := createLocations(&virtualServer, serverID) return http.Server{ ServerName: virtualServer.Hostname, @@ -97,10 +103,7 @@ 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, @@ -108,7 +111,7 @@ func createServer(virtualServer dataplane.VirtualServer) ( }, 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, } diff --git a/internal/mode/static/nginx/config/servers_template.go b/internal/mode/static/nginx/config/servers_template.go index 664c7d4245..6364303f70 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/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 }}; diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 7d50f67b3e..450e0bd882 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -2,7 +2,6 @@ package config import ( "fmt" - "strconv" "strings" "testing" @@ -63,8 +62,10 @@ func TestExecuteServers(t *testing.T) { "ssl_certificate_key /etc/nginx/secrets/test-keypair.pem;": 2, } g := NewWithT(t) - servers, _ := executeServers(conf) - serverConf := string(execute(serversTemplate, servers)) + serverResults := executeServers(conf) + serverConf := string(serverResults[0].data) + httpMatchConf := string(serverResults[1].data) + g.Expect(httpMatchConf).To(Equal("{}")) for expSubStr, expCount := range expSubStrings { g.Expect(strings.Count(serverConf, expSubStr)).To(Equal(expCount)) } @@ -141,8 +142,10 @@ func TestExecuteForDefaultServers(t *testing.T) { t.Run(tc.msg, func(t *testing.T) { g := NewWithT(t) - servers, _ := executeServers(tc.conf) - serverConf := string(execute(serversTemplate, servers)) + serverResults := executeServers(tc.conf) + serverConf := string(serverResults[0].data) + httpMatchConf := string(serverResults[1].data) + g.Expect(httpMatchConf).To(Equal("{}")) for _, expPort := range tc.httpPorts { g.Expect(serverConf).To(ContainSubstring(fmt.Sprintf(httpDefaultFmt, expPort))) @@ -510,13 +513,13 @@ func TestCreateServers(t *testing.T) { }, } - expMatchPairs := map[string][]http.RouteMatch{ - "cafeexamplecom8080": { + expMatchPairs := map[string][]RouteMatch{ + "1_0": { {Method: "POST", RedirectPath: "@rule0-route0"}, {Method: "PATCH", RedirectPath: "@rule0-route1"}, - {Any: true, RedirectPath: "@rule0-route2"}, + {RedirectPath: "@rule0-route2", Any: true}, }, - "cafeexamplecomtest8080": { + "1_1": { { Method: "GET", Headers: []string{"Version:V1", "test:foo", "my-header:my-value"}, @@ -524,51 +527,45 @@ func TestCreateServers(t *testing.T) { RedirectPath: "@rule1-route0", }, }, - "cafeexamplecomEXACTredirect-with-headers8080": { + "1_6": { {RedirectPath: "@rule6-route0", Headers: []string{"redirect:this"}}, }, - "cafeexamplecomredirect-with-headers8080": { + "1_6EXACT": { { - Headers: []string{"redirect:this"}, RedirectPath: "@rule6-route0", + Headers: []string{"redirect:this"}, }, }, - "cafeexamplecomEXACTrewrite-with-headers8080": { + "1_8": { { Headers: []string{"rewrite:this"}, RedirectPath: "@rule8-route0", }, }, - "cafeexamplecomrewrite-with-headers8080": { + "1_8EXACT": { { - Headers: []string{"rewrite:this"}, RedirectPath: "@rule8-route0", + Headers: []string{"rewrite:this"}, }, }, - "cafeexamplecomEXACTinvalid-filter-with-headers8080": { + "1_10": { { Headers: []string{"filter:this"}, RedirectPath: "@rule10-route0", }, }, - "cafeexamplecominvalid-filter-with-headers8080": { + "1_10EXACT": { { Headers: []string{"filter:this"}, RedirectPath: "@rule10-route0", }, }, - "cafeexamplecomEXACTtest8080": { - { - Method: "GET", - RedirectPath: "@rule12-route0", - }, - }, - "cafeexamplecom8443": { + "SSL1_0": { {Method: "POST", RedirectPath: "@rule0-route0"}, {Method: "PATCH", RedirectPath: "@rule0-route1"}, - {RedirectPath: "@rule0-route2", Any: true}, + {Any: true, RedirectPath: "@rule0-route2"}, }, - "cafeexamplecomtest8443": { + "SSL1_1": { { Method: "GET", RedirectPath: "@rule1-route0", @@ -576,43 +573,46 @@ func TestCreateServers(t *testing.T) { QueryParams: []string{"GrEat=EXAMPLE", "test=foo=bar"}, }, }, - "cafeexamplecomEXACTredirect-with-headers8443": { + "SSL1_10": { { - RedirectPath: "@rule6-route0", - Headers: []string{"redirect:this"}, + RedirectPath: "@rule10-route0", + Headers: []string{"filter:this"}, }, }, - "cafeexamplecomrewrite-with-headers8443": { + "SSL1_10EXACT": { { - RedirectPath: "@rule8-route0", - Headers: []string{"rewrite:this"}, + RedirectPath: "@rule10-route0", + Headers: []string{"filter:this"}, }, }, - "cafeexamplecomEXACTrewrite-with-headers8443": { + "SSL1_12EXACT": { { - RedirectPath: "@rule8-route0", - Headers: []string{"rewrite:this"}, + Method: "GET", + RedirectPath: "@rule12-route0", }, }, - "cafeexamplecomredirect-with-headers8443": { + "1_12EXACT": {{Method: "GET", RedirectPath: "@rule12-route0"}}, + "SSL1_6": { + {RedirectPath: "@rule6-route0", Headers: []string{"redirect:this"}}, + }, + "SSL1_6EXACT": { { - RedirectPath: "@rule6-route0", Headers: []string{"redirect:this"}, + RedirectPath: "@rule6-route0", }, }, - "cafeexamplecominvalid-filter-with-headers8443": { + "SSL1_8": { { - RedirectPath: "@rule10-route0", - Headers: []string{"filter:this"}, + Headers: []string{"rewrite:this"}, + RedirectPath: "@rule8-route0", }, }, - "cafeexamplecomEXACTinvalid-filter-with-headers8443": { + "SSL1_8EXACT": { { - RedirectPath: "@rule10-route0", - Headers: []string{"filter:this"}, + RedirectPath: "@rule8-route0", + Headers: []string{"rewrite:this"}, }, }, - "cafeexamplecomEXACTtest8443": {{Method: "GET", RedirectPath: "@rule12-route0"}}, } rewriteProxySetHeaders := []http.Header{ @@ -635,9 +635,11 @@ func TestCreateServers(t *testing.T) { } getExpectedLocations := func(isHTTPS bool) []http.Location { - port := 8080 + port := "8080" + ssl := "" if isHTTPS { - port = 8443 + port = "8443" + ssl = "SSL" } return []http.Location{ @@ -658,7 +660,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "/", - HTTPMatchKey: "cafeexamplecom" + strconv.Itoa(port), + HTTPMatchKey: ssl + "1" + port, }, { Path: "@rule1-route0", @@ -667,7 +669,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "/test/", - HTTPMatchKey: "cafeexamplecomtest" + strconv.Itoa(port), + HTTPMatchKey: ssl + "2" + port, }, { Path: "/path-only/", @@ -701,14 +703,14 @@ func TestCreateServers(t *testing.T) { Path: "/redirect-implicit-port/", Return: &http.Return{ Code: 302, - Body: fmt.Sprintf("$scheme://foo.example.com:%d$request_uri", port), + Body: "$scheme://foo.example.com:%d$request_uri" + port, }, }, { Path: "= /redirect-implicit-port", Return: &http.Return{ Code: 302, - Body: fmt.Sprintf("$scheme://foo.example.com:%d$request_uri", port), + Body: "$scheme://foo.example.com:%d$request_uri" + port, }, }, { @@ -734,11 +736,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/redirect-with-headers/", - HTTPMatchKey: "cafeexamplecomredirect-with-headers" + strconv.Itoa(port), + HTTPMatchKey: ssl + "3" + port, }, { Path: "= /redirect-with-headers", - HTTPMatchKey: "cafeexamplecomEXACTredirect-with-headers" + strconv.Itoa(port), + HTTPMatchKey: ssl + "4" + port + "EXACT", }, { Path: "/rewrite/", @@ -760,11 +762,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/rewrite-with-headers/", - HTTPMatchKey: "cafeexamplecomrewrite-with-headers" + strconv.Itoa(port), + HTTPMatchKey: ssl + "5" + port, }, { Path: "= /rewrite-with-headers", - HTTPMatchKey: "cafeexamplecomEXACTrewrite-with-headers" + strconv.Itoa(port), + HTTPMatchKey: ssl + "6" + port + "EXACT", }, { Path: "/invalid-filter/", @@ -786,11 +788,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/invalid-filter-with-headers/", - HTTPMatchKey: "cafeexamplecominvalid-filter-with-headers" + strconv.Itoa(port), + HTTPMatchKey: ssl + "7" + port, }, { Path: "= /invalid-filter-with-headers", - HTTPMatchKey: "cafeexamplecomEXACTinvalid-filter-with-headers" + strconv.Itoa(port), + HTTPMatchKey: ssl + "8" + port + "EXACT", }, { Path: "= /exact", @@ -804,7 +806,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "= /test", - HTTPMatchKey: "cafeexamplecomEXACTtest" + strconv.Itoa(port), + HTTPMatchKey: ssl + "11" + port + "EXACT", }, { Path: "/proxy-set-headers/", @@ -891,9 +893,8 @@ func TestCreateServers(t *testing.T) { g := NewWithT(t) result, httpMatchPair := createServers(httpServers, sslServers) - if httpMatchPair != nil { - g.Expect(helpers.Diff(httpMatchPair, expMatchPairs)).To(BeEmpty()) - } + + g.Expect(helpers.Diff(expMatchPairs, httpMatchPair)).To(BeEmpty()) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) } @@ -1214,10 +1215,10 @@ func TestCreateLocationsRootPath(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - locs, httpMatchPair := createLocations(dataplane.VirtualServer{ + locs, httpMatchPair := createLocations(&dataplane.VirtualServer{ PathRules: test.pathRules, Port: 80, - }) + }, 1) g.Expect(locs).To(Equal(test.expLocations)) g.Expect(httpMatchPair).To(BeEmpty()) }) @@ -1522,11 +1523,11 @@ func TestCreateRouteMatch(t *testing.T) { tests := []struct { match dataplane.Match msg string - expected http.RouteMatch + expected RouteMatch }{ { match: dataplane.Match{}, - expected: http.RouteMatch{ + expected: RouteMatch{ Any: true, RedirectPath: testPath, }, @@ -1536,7 +1537,7 @@ func TestCreateRouteMatch(t *testing.T) { match: dataplane.Match{ Method: testMethodMatch, // A path match with a method should not set the Any field to true }, - expected: http.RouteMatch{ + expected: RouteMatch{ Method: "PUT", RedirectPath: testPath, }, @@ -1546,7 +1547,7 @@ func TestCreateRouteMatch(t *testing.T) { match: dataplane.Match{ Headers: testHeaderMatches, }, - expected: http.RouteMatch{ + expected: RouteMatch{ RedirectPath: testPath, Headers: expectedHeaders, }, @@ -1556,7 +1557,7 @@ func TestCreateRouteMatch(t *testing.T) { match: dataplane.Match{ QueryParams: testQueryParamMatches, }, - expected: http.RouteMatch{ + expected: RouteMatch{ QueryParams: expectedArgs, RedirectPath: testPath, }, @@ -1567,7 +1568,7 @@ func TestCreateRouteMatch(t *testing.T) { Method: testMethodMatch, QueryParams: testQueryParamMatches, }, - expected: http.RouteMatch{ + expected: RouteMatch{ Method: "PUT", QueryParams: expectedArgs, RedirectPath: testPath, @@ -1579,7 +1580,7 @@ func TestCreateRouteMatch(t *testing.T) { Method: testMethodMatch, Headers: testHeaderMatches, }, - expected: http.RouteMatch{ + expected: RouteMatch{ Method: "PUT", Headers: expectedHeaders, RedirectPath: testPath, @@ -1591,7 +1592,7 @@ func TestCreateRouteMatch(t *testing.T) { QueryParams: testQueryParamMatches, Headers: testHeaderMatches, }, - expected: http.RouteMatch{ + expected: RouteMatch{ QueryParams: expectedArgs, Headers: expectedHeaders, RedirectPath: testPath, @@ -1604,7 +1605,7 @@ func TestCreateRouteMatch(t *testing.T) { QueryParams: testQueryParamMatches, Method: testMethodMatch, }, - expected: http.RouteMatch{ + expected: RouteMatch{ Method: "PUT", Headers: expectedHeaders, QueryParams: expectedArgs, @@ -1616,7 +1617,7 @@ func TestCreateRouteMatch(t *testing.T) { match: dataplane.Match{ Headers: testDuplicateHeaders, }, - expected: http.RouteMatch{ + expected: RouteMatch{ Headers: expectedHeaders, RedirectPath: testPath, }, 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..77422fa7e7 100644 --- a/internal/mode/static/nginx/config/split_clients_test.go +++ b/internal/mode/static/nginx/config/split_clients_test.go @@ -98,7 +98,8 @@ 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}) + sc := string(splitResults[0].data) 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..e96057dd5b 100644 --- a/internal/mode/static/nginx/config/upstreams_test.go +++ b/internal/mode/static/nginx/config/upstreams_test.go @@ -47,7 +47,8 @@ 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}) + upstreams := string(upstreamResults[0].data) g := NewWithT(t) 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 56b96d936f..0f3d138936 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/internal/mode/static/nginx/modules/src/httpmatches.js @@ -16,36 +16,12 @@ function redirect(r) { return; } - // Matches 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. - let match; try { - match = findWinningMatch(r, matchList); + internalRedirect(r, matchList); } catch (e) { r.error(e.message); r.return(HTTP_CODES.internalServerError); - return; } - - if (!match) { - r.return(HTTP_CODES.notFound); - return; - } - - if (!match.redirectPath) { - r.error( - `cannot redirect the request; the match ${JSON.stringify( - match, - )} does not have a redirectPath set`, - ); - r.return(HTTP_CODES.internalServerError); - return; - } - - r.internalRedirect(match.redirectPath); } function internalRedirect(r, matchList) { @@ -54,6 +30,14 @@ function internalRedirect(r, matchList) { // If there's a match, redirect request to internal location block. // If an exception occurs, return 500. // If no matches are found, return 404. + try { + verifyMatchList(matchList); + } catch (e) { + r.error(e.message); + r.return(HTTP_CODES.internalServerError); + return; + } + let match; try { match = findWinningMatch(r, matchList); @@ -81,6 +65,18 @@ function internalRedirect(r, matchList) { r.internalRedirect(match.redirectPath); } +function verifyMatchList(matchList) { + if (!Array.isArray(matchList)) { + throw Error(`cannot redirect the request; expected a list of matches, got ${matchList}`); + } + + if (matchList.length === 0) { + throw Error(`cannot redirect the request; matches is an empty list`); + } + + return; +} + function findWinningMatch(r, matches) { for (let i = 0; i < matches.length; i++) { try { @@ -204,6 +200,7 @@ function paramsMatch(requestParams, params) { export default { redirect, internalRedirect, + verifyMatchList, testMatch, findWinningMatch, headersMatch, diff --git a/internal/mode/static/nginx/modules/test/httpmatches.test.js b/internal/mode/static/nginx/modules/test/httpmatches.test.js index a256717308..9ac253e08e 100644 --- a/internal/mode/static/nginx/modules/test/httpmatches.test.js +++ b/internal/mode/static/nginx/modules/test/httpmatches.test.js @@ -33,6 +33,37 @@ function createRequest({ method = '', headers = {}, params = {} } = {}) { return r; } +describe('verifyMatchList', () => { + const tests = [ + { + name: 'throws if matches variable is not an array', + matchList: '{}', + expectThrow: true, + errSubstring: 'expected a list of matches', + }, + { + name: 'throws if the length of the matches variable is zero', + matchList: '[]', + expectThrow: true, + errSubstring: 'expected a list of matches', + }, + { + name: 'does not throw if matches variable is expected list of matches', + matchList: '[{"any":true}]', + expectThrow: false, + }, + ]; + tests.forEach((test) => { + it(test.name, () => { + if (test.expectThrow) { + expect(() => hm.verifyMatchList(test.matchList)).to.throw(test.errSubstring); + } else { + expect(() => hm.verifyMatchList(test.matchList).to.not.throw()); + } + }); + }); +}); + describe('testMatch', () => { const tests = [ {