From 3cbc1ec136be8db09892a659776ea0315c155722 Mon Sep 17 00:00:00 2001 From: Saloni Date: Mon, 8 Apr 2024 20:48:51 -0600 Subject: [PATCH 01/27] fix: fix reload errors due long matching conditions --- .../mode/static/nginx/config/generator.go | 33 ++- .../static/nginx/config/generator_test.go | 13 +- .../mode/static/nginx/config/http/config.go | 26 +- internal/mode/static/nginx/config/servers.go | 126 +++++----- .../static/nginx/config/servers_template.go | 5 +- .../mode/static/nginx/config/servers_test.go | 208 ++++++++++----- .../static/nginx/modules/src/httpmatches.js | 125 ++++------ .../nginx/modules/test/httpmatches.test.js | 236 +++++++----------- 8 files changed, 403 insertions(+), 369 deletions(-) diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index 4917a80cac..b1157cd56a 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -1,6 +1,8 @@ package config import ( + "encoding/json" + "fmt" "path/filepath" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file" @@ -23,6 +25,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 + "/match.json" ) // ConfigFolders is a list of folders where NGINX configuration files are stored. @@ -66,7 +71,8 @@ func (g GeneratorImpl) Generate(conf dataplane.Configuration) []file.File { files = append(files, generatePEM(id, pair.Cert, pair.Key)) } - files = append(files, g.generateHTTPConfig(conf)) + httpFiles := g.generateHTTPConfig(conf) + files = append(files, httpFiles...) files = append(files, generateConfigVersion(conf.Version)) @@ -106,24 +112,43 @@ func generateCertBundleFileName(id dataplane.CertBundleID) string { return filepath.Join(secretsFolder, string(id)+".crt") } -func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) file.File { +func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) []file.File { var c []byte for _, execute := range g.getExecuteFuncs() { c = append(c, execute(conf)...) } - return file.File{ + servers, httpMatchPairs := executeServers(conf) + + // create server conf + serverConf := execute(serversTemplate, servers) + c = append(c, serverConf...) + + httpConf := file.File{ Content: c, Path: httpConfigFile, 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} } func (g GeneratorImpl) getExecuteFuncs() []executeFunc { return []executeFunc{ 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..50bfc85463 100644 --- a/internal/mode/static/nginx/config/generator_test.go +++ b/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,15 @@ 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[2].Path).To(Equal("/etc/nginx/conf.d/config-version.conf")) - configVersion := string(files[2].Content) + + 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")) } diff --git a/internal/mode/static/nginx/config/http/config.go b/internal/mode/static/nginx/config/http/config.go index 8486b7d464..a4acc953b9 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. @@ -93,3 +93,21 @@ 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/servers.go b/internal/mode/static/nginx/config/servers.go index a80de123f7..162bad2d33 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" + "strconv" "strings" gotemplate "text/template" @@ -38,58 +39,82 @@ var baseHeaders = []http.Header{ }, } -func executeServers(conf dataplane.Configuration) []byte { - servers := createServers(conf.HTTPServers, conf.SSLServers) +func executeServers(conf dataplane.Configuration) ([]http.Server, map[string][]http.RouteMatch) { + servers, httpMatchPairs := createServers(conf.HTTPServers, conf.SSLServers) - return execute(serversTemplate, servers) + return servers, httpMatchPairs } -func createServers(httpServers, sslServers []dataplane.VirtualServer) []http.Server { +func createServers(httpServers, sslServers []dataplane.VirtualServer) ( + []http.Server, + map[string][]http.RouteMatch, +) { servers := make([]http.Server, 0, len(httpServers)+len(sslServers)) + finalMatchPairs := make(map[string][]http.RouteMatch) for _, s := range httpServers { - servers = append(servers, createServer(s)) + httpServer, matchPair := createServer(s) + servers = append(servers, httpServer) + + for key, val := range matchPair { + finalMatchPairs[key] = val + } } for _, s := range sslServers { - servers = append(servers, createSSLServer(s)) + sslServer, matchPair := createSSLServer(s) + servers = append(servers, sslServer) + + for key, val := range matchPair { + finalMatchPairs[key] = val + } } - return servers + return servers, finalMatchPairs } -func createSSLServer(virtualServer dataplane.VirtualServer) http.Server { +func createSSLServer(virtualServer dataplane.VirtualServer) ( + http.Server, + map[string][]http.RouteMatch, +) { if virtualServer.IsDefault { return http.Server{ IsDefaultSSL: true, Port: virtualServer.Port, - } + }, nil } + locs, matchPairs := createLocations(virtualServer) + 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) ( + http.Server, + map[string][]http.RouteMatch, +) { if virtualServer.IsDefault { return http.Server{ IsDefaultHTTP: true, Port: virtualServer.Port, - } + }, nil } + locs, matchPairs := createLocations(virtualServer) + 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 +124,19 @@ type rewriteConfig struct { Rewrite string } -func createLocations(pathRules []dataplane.PathRule, listenerPort int32) []http.Location { - maxLocs, pathsAndTypes := getMaxLocationCountAndPathMap(pathRules) +type httpMatchPairs map[string][]http.RouteMatch + +func createLocations(server dataplane.VirtualServer) ( + []http.Location, + map[string][]http.RouteMatch, +) { + 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([]http.RouteMatch, 0, len(rule.MatchRules)) if rule.Path == rootPath { rootPathExists = true @@ -121,14 +152,15 @@ 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 + key := server.Hostname + extLocations[i].Path + strconv.Itoa(int(server.Port)) + extLocations[i].HTTPMatchKey = sanitizeKey(key) + matchPairs[extLocations[i].HTTPMatchKey] = matches } locs = append(locs, extLocations...) } @@ -138,7 +170,14 @@ func createLocations(pathRules []dataplane.PathRule, listenerPort int32) []http. locs = append(locs, createDefaultRootLocation()) } - return locs + 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 @@ -217,9 +256,9 @@ func initializeInternalLocation( pathruleIdx, matchRuleIdx int, match dataplane.Match, -) (http.Location, httpMatch) { +) (http.Location, http.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,26 +431,8 @@ 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 { - // 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 createHTTPMatch(match dataplane.Match, redirectPath string) httpMatch { - hm := httpMatch{ +func createRouteMatch(match dataplane.Match, redirectPath string) http.RouteMatch { + hm := http.RouteMatch{ RedirectPath: redirectPath, } @@ -558,19 +579,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..664c7d4245 100644 --- a/internal/mode/static/nginx/config/servers_template.go +++ b/internal/mode/static/nginx/config/servers_template.go @@ -17,6 +17,7 @@ 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 }}; @@ -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..7d50f67b3e 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" + "strconv" "strings" "testing" @@ -63,9 +63,10 @@ func TestExecuteServers(t *testing.T) { "ssl_certificate_key /etc/nginx/secrets/test-keypair.pem;": 2, } g := NewWithT(t) - servers := string(executeServers(conf)) + servers, _ := executeServers(conf) + serverConf := string(execute(serversTemplate, servers)) 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 +141,15 @@ func TestExecuteForDefaultServers(t *testing.T) { t.Run(tc.msg, func(t *testing.T) { g := NewWithT(t) - cfg := string(executeServers(tc.conf)) + servers, _ := executeServers(tc.conf) + serverConf := string(execute(serversTemplate, servers)) 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 +510,111 @@ 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 := map[string][]http.RouteMatch{ + "cafeexamplecom8080": { + {Method: "POST", RedirectPath: "@rule0-route0"}, + {Method: "PATCH", RedirectPath: "@rule0-route1"}, + {Any: true, RedirectPath: "@rule0-route2"}, }, - } - exactMatches := []httpMatch{ - { - Method: "GET", - RedirectPath: "@rule12-route0", + "cafeexamplecomtest8080": { + { + 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", + "cafeexamplecomEXACTredirect-with-headers8080": { + {RedirectPath: "@rule6-route0", Headers: []string{"redirect:this"}}, }, - } - rewriteHeaderMatches := []httpMatch{ - { - Headers: []string{"rewrite:this"}, - RedirectPath: "@rule8-route0", + "cafeexamplecomredirect-with-headers8080": { + { + Headers: []string{"redirect:this"}, + RedirectPath: "@rule6-route0", + }, + }, + "cafeexamplecomEXACTrewrite-with-headers8080": { + { + Headers: []string{"rewrite:this"}, + RedirectPath: "@rule8-route0", + }, + }, + "cafeexamplecomrewrite-with-headers8080": { + { + Headers: []string{"rewrite:this"}, + RedirectPath: "@rule8-route0", + }, + }, + "cafeexamplecomEXACTinvalid-filter-with-headers8080": { + { + Headers: []string{"filter:this"}, + RedirectPath: "@rule10-route0", + }, + }, + "cafeexamplecominvalid-filter-with-headers8080": { + { + Headers: []string{"filter:this"}, + RedirectPath: "@rule10-route0", + }, + }, + "cafeexamplecomEXACTtest8080": { + { + Method: "GET", + RedirectPath: "@rule12-route0", + }, }, + "cafeexamplecom8443": { + {Method: "POST", RedirectPath: "@rule0-route0"}, + {Method: "PATCH", RedirectPath: "@rule0-route1"}, + {RedirectPath: "@rule0-route2", Any: true}, + }, + "cafeexamplecomtest8443": { + { + Method: "GET", + RedirectPath: "@rule1-route0", + Headers: []string{"Version:V1", "test:foo", "my-header:my-value"}, + QueryParams: []string{"GrEat=EXAMPLE", "test=foo=bar"}, + }, + }, + "cafeexamplecomEXACTredirect-with-headers8443": { + { + RedirectPath: "@rule6-route0", + Headers: []string{"redirect:this"}, + }, + }, + "cafeexamplecomrewrite-with-headers8443": { + { + RedirectPath: "@rule8-route0", + Headers: []string{"rewrite:this"}, + }, + }, + "cafeexamplecomEXACTrewrite-with-headers8443": { + { + RedirectPath: "@rule8-route0", + Headers: []string{"rewrite:this"}, + }, + }, + "cafeexamplecomredirect-with-headers8443": { + { + RedirectPath: "@rule6-route0", + Headers: []string{"redirect:this"}, + }, + }, + "cafeexamplecominvalid-filter-with-headers8443": { + { + RedirectPath: "@rule10-route0", + Headers: []string{"filter:this"}, + }, + }, + "cafeexamplecomEXACTinvalid-filter-with-headers8443": { + { + RedirectPath: "@rule10-route0", + Headers: []string{"filter:this"}, + }, + }, + "cafeexamplecomEXACTtest8443": {{Method: "GET", RedirectPath: "@rule12-route0"}}, } + rewriteProxySetHeaders := []http.Header{ { Name: "Host", @@ -564,12 +633,6 @@ 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 @@ -595,7 +658,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "/", - HTTPMatchVar: expectedMatchString(slashMatches), + HTTPMatchKey: "cafeexamplecom" + strconv.Itoa(port), }, { Path: "@rule1-route0", @@ -604,7 +667,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "/test/", - HTTPMatchVar: expectedMatchString(testMatches), + HTTPMatchKey: "cafeexamplecomtest" + strconv.Itoa(port), }, { Path: "/path-only/", @@ -671,11 +734,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/redirect-with-headers/", - HTTPMatchVar: expectedMatchString(redirectHeaderMatches), + HTTPMatchKey: "cafeexamplecomredirect-with-headers" + strconv.Itoa(port), }, { Path: "= /redirect-with-headers", - HTTPMatchVar: expectedMatchString(redirectHeaderMatches), + HTTPMatchKey: "cafeexamplecomEXACTredirect-with-headers" + strconv.Itoa(port), }, { Path: "/rewrite/", @@ -697,11 +760,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/rewrite-with-headers/", - HTTPMatchVar: expectedMatchString(rewriteHeaderMatches), + HTTPMatchKey: "cafeexamplecomrewrite-with-headers" + strconv.Itoa(port), }, { Path: "= /rewrite-with-headers", - HTTPMatchVar: expectedMatchString(rewriteHeaderMatches), + HTTPMatchKey: "cafeexamplecomEXACTrewrite-with-headers" + strconv.Itoa(port), }, { Path: "/invalid-filter/", @@ -723,11 +786,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/invalid-filter-with-headers/", - HTTPMatchVar: expectedMatchString(invalidFilterHeaderMatches), + HTTPMatchKey: "cafeexamplecominvalid-filter-with-headers" + strconv.Itoa(port), }, { Path: "= /invalid-filter-with-headers", - HTTPMatchVar: expectedMatchString(invalidFilterHeaderMatches), + HTTPMatchKey: "cafeexamplecomEXACTinvalid-filter-with-headers" + strconv.Itoa(port), }, { Path: "= /exact", @@ -741,7 +804,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "= /test", - HTTPMatchVar: expectedMatchString(exactMatches), + HTTPMatchKey: "cafeexamplecomEXACTtest" + strconv.Itoa(port), }, { Path: "/proxy-set-headers/", @@ -827,7 +890,10 @@ func TestCreateServers(t *testing.T) { g := NewWithT(t) - result := createServers(httpServers, sslServers) + result, httpMatchPair := createServers(httpServers, sslServers) + if httpMatchPair != nil { + g.Expect(helpers.Diff(httpMatchPair, expMatchPairs)).To(BeEmpty()) + } g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) } @@ -1024,7 +1090,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 +1214,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, + }) g.Expect(locs).To(Equal(test.expLocations)) + g.Expect(httpMatchPair).To(BeEmpty()) }) } } @@ -1404,7 +1474,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 +1522,11 @@ func TestCreateHTTPMatch(t *testing.T) { tests := []struct { match dataplane.Match msg string - expected httpMatch + expected http.RouteMatch }{ { match: dataplane.Match{}, - expected: httpMatch{ + expected: http.RouteMatch{ Any: true, RedirectPath: testPath, }, @@ -1466,7 +1536,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: http.RouteMatch{ Method: "PUT", RedirectPath: testPath, }, @@ -1476,7 +1546,7 @@ func TestCreateHTTPMatch(t *testing.T) { match: dataplane.Match{ Headers: testHeaderMatches, }, - expected: httpMatch{ + expected: http.RouteMatch{ RedirectPath: testPath, Headers: expectedHeaders, }, @@ -1486,7 +1556,7 @@ func TestCreateHTTPMatch(t *testing.T) { match: dataplane.Match{ QueryParams: testQueryParamMatches, }, - expected: httpMatch{ + expected: http.RouteMatch{ QueryParams: expectedArgs, RedirectPath: testPath, }, @@ -1497,7 +1567,7 @@ func TestCreateHTTPMatch(t *testing.T) { Method: testMethodMatch, QueryParams: testQueryParamMatches, }, - expected: httpMatch{ + expected: http.RouteMatch{ Method: "PUT", QueryParams: expectedArgs, RedirectPath: testPath, @@ -1509,7 +1579,7 @@ func TestCreateHTTPMatch(t *testing.T) { Method: testMethodMatch, Headers: testHeaderMatches, }, - expected: httpMatch{ + expected: http.RouteMatch{ Method: "PUT", Headers: expectedHeaders, RedirectPath: testPath, @@ -1521,7 +1591,7 @@ func TestCreateHTTPMatch(t *testing.T) { QueryParams: testQueryParamMatches, Headers: testHeaderMatches, }, - expected: httpMatch{ + expected: http.RouteMatch{ QueryParams: expectedArgs, Headers: expectedHeaders, RedirectPath: testPath, @@ -1534,7 +1604,7 @@ func TestCreateHTTPMatch(t *testing.T) { QueryParams: testQueryParamMatches, Method: testMethodMatch, }, - expected: httpMatch{ + expected: http.RouteMatch{ Method: "PUT", Headers: expectedHeaders, QueryParams: expectedArgs, @@ -1546,7 +1616,7 @@ func TestCreateHTTPMatch(t *testing.T) { match: dataplane.Match{ Headers: testDuplicateHeaders, }, - expected: httpMatch{ + expected: http.RouteMatch{ Headers: expectedHeaders, RedirectPath: testPath, }, @@ -1557,7 +1627,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/modules/src/httpmatches.js b/internal/mode/static/nginx/modules/src/httpmatches.js index 6478b12991..b16daa32d3 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/internal/mode/static/nginx/modules/src/httpmatches.js @@ -1,80 +1,53 @@ -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; - - try { - matches = extractMatchesFromRequest(r); - } catch (e) { - r.error(e.message); - r.return(HTTP_CODES.internalServerError); - 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, matches); - } 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); + let matchList; + + try { + let key = r.variables[MATCHES_KEY]; + matchList = matches[key]; + } catch (e) { + r.error(e.message); + r.return(HTTP_CODES.internalServerError); + return; + } } -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`, - ); - } - - 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) { - throw Error(`cannot redirect the request; matches is an empty list`); - } - - return matches; +function internalRedirect(r, matchList) { + // 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. + let match; + try { + match = findWinningMatch(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 findWinningMatch(r, matches) { @@ -198,12 +171,10 @@ function paramsMatch(requestParams, params) { } export default { - redirect, - testMatch, - findWinningMatch, - headersMatch, - paramsMatch, - extractMatchesFromRequest, - HTTP_CODES, - MATCHES_VARIABLE, + internalRedirect, + testMatch, + findWinningMatch, + headersMatch, + paramsMatch, + HTTP_CODES, }; diff --git a/internal/mode/static/nginx/modules/test/httpmatches.test.js b/internal/mode/static/nginx/modules/test/httpmatches.test.js index 0a1ca26aba..63278b69cb 100644 --- a/internal/mode/static/nginx/modules/test/httpmatches.test.js +++ b/internal/mode/static/nginx/modules/test/httpmatches.test.js @@ -3,20 +3,20 @@ 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 = '' } = {}) { - let r = { - // Test mocks - return(statusCode) { - r.testReturned = statusCode; - }, - internalRedirect(redirectPath) { - r.testRedirectedTo = redirectPath; - }, - error(msg) { - console.log('\tngx_error:', msg); - }, - variables: {}, - }; +function createRequest({ method = '', headers = {}, params = {} } = {}) { + let r = { + // Test mocks + return(statusCode) { + r.testReturned = statusCode; + }, + internalRedirect(redirectPath) { + r.testRedirectedTo = redirectPath; + }, + error(msg) { + console.log('\tngx_error:', msg); + }, + variables: {}, + }; if (method) { r.method = method; @@ -30,58 +30,9 @@ function createRequest({ method = '', headers = {}, params = {}, matches = '' } r.args = params; } - if (matches) { - r.variables[hm.MATCHES_VARIABLE] = matches; - } - - return r; + return r; } -describe('extractMatchesFromRequest', () => { - const tests = [ - { - name: 'throws if matches variable does not exist on request', - request: createRequest(), - expectThrow: true, - errSubstring: 'http_matches is not defined', - }, - { - name: 'throws if matches variable is not JSON', - request: createRequest({ matches: 'not-JSON' }), - expectThrow: true, - errSubstring: 'error parsing', - }, - { - name: 'throws if matches variable is not an array', - request: createRequest({ matches: '{}' }), - expectThrow: true, - errSubstring: 'expected a list of matches', - }, - { - name: 'throws if the length of the matches variable is zero', - request: createRequest({ matches: '[]' }), - 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}]' }), - expectThrow: false, - }, - ]; - tests.forEach((test) => { - it(test.name, () => { - if (test.expectThrow) { - expect(() => hm.extractMatchesFromRequest(test.request)).to.throw( - test.errSubstring, - ); - } else { - expect(() => hm.extractMatchesFromRequest(test.request).to.not.throw()); - } - }); - }); -}); - describe('testMatch', () => { const tests = [ { @@ -193,22 +144,16 @@ 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, - ); - } else { - const result = hm.findWinningMatch(test.request, test.matches); - expect(result).to.equal(test.expected); - } - }); - }); + tests.forEach((test) => { + it(test.name, () => { + if (test.expectThrow) { + expect(() => hm.findWinningMatch(test.request, test.matches)).to.throw(test.errSubstring); + } else { + const result = hm.findWinningMatch(test.request, test.matches); + expect(result).to.equal(test.expected); + } + }); + }); }); describe('headersMatch', () => { @@ -393,75 +338,68 @@ describe('paramsMatch', () => { }); }); -describe('redirect', () => { - const testAnyMatch = { any: true, redirectPath: '/any' }; - const testHeaderMatches = { - headers: ['header1:VALUE1', 'header2:value2', 'header3:value3'], - redirectPath: '/headers', - }; - const testQueryParamMatches = { - params: ['Arg1=value1', 'arg2=value2=SOME=other=value', 'arg3===value3&*1(*+'], - redirectPath: '/params', - }; - const testAllMatchTypes = { - method: 'GET', - headers: ['header1:value1', 'header2:value2'], - params: ['Arg1=value1', 'arg2=value2=SOME=other=value'], - redirectPath: '/a-match', - }; - - const tests = [ - { - name: 'returns Internal Server Error status code if http_matches 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', - request: createRequest(), - matches: [{ headers: ['malformedheader'] }], - expectedReturn: hm.HTTP_CODES.internalServerError, - }, - { - name: 'returns Not Found status code if request does not satisfy any match', - request: createRequest({ method: 'GET' }), - matches: [{ method: 'POST' }], - expectedReturn: hm.HTTP_CODES.notFound, - }, - { - name: 'returns Internal Server Error status code if request satisfies match, but the redirectPath is missing', - request: createRequest({ method: 'GET' }), - matches: [{ method: 'GET' }], - expectedReturn: hm.HTTP_CODES.internalServerError, - }, - { - name: 'redirects to the redirectPath of the first match the request satisfies', - request: createRequest({ - method: 'GET', - headers: { header1: 'value1', header2: 'value2' }, - params: { Arg1: 'value1', arg2: 'value2=SOME=other=value' }, - }), - matches: [testHeaderMatches, testQueryParamMatches, testAllMatchTypes, testAnyMatch], // request matches testAllMatchTypes and testAnyMatch. But first match should win. - expectedRedirect: '/a-match', - }, - ]; +describe('internalRedirect', () => { + const testAnyMatch = { any: true, redirectPath: '/any' }; + const testHeaderMatches = { + headers: ['header1:VALUE1', 'header2:value2', 'header3:value3'], + redirectPath: '/headers', + }; + const testQueryParamMatches = { + params: ['Arg1=value1', 'arg2=value2=SOME=other=value', 'arg3===value3&*1(*+'], + redirectPath: '/params', + }; + const testAllMatchTypes = { + method: 'GET', + headers: ['header1:value1', 'header2:value2'], + params: ['Arg1=value1', 'arg2=value2=SOME=other=value'], + redirectPath: '/a-match', + }; - tests.forEach((test) => { - it(test.name, () => { - if (test.matches) { - // set http_matches variable - test.request.variables = { - http_matches: JSON.stringify(test.matches), - }; - } + const tests = [ + { + 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 matchList contains malformed match', + request: createRequest(), + matches: [{ headers: ['malformedheader'] }], + expectedReturn: hm.HTTP_CODES.internalServerError, + }, + { + name: 'returns Not Found status code if request does not satisfy any match', + request: createRequest({ method: 'GET' }), + matches: [{ method: 'POST' }], + expectedReturn: hm.HTTP_CODES.notFound, + }, + { + name: 'returns Internal Server Error status code if request satisfies match, but the redirectPath is missing', + request: createRequest({ method: 'GET' }), + matches: [{ method: 'GET' }], + expectedReturn: hm.HTTP_CODES.internalServerError, + }, + { + name: 'redirects to the redirectPath of the first match the request satisfies', + request: createRequest({ + method: 'GET', + headers: { header1: 'value1', header2: 'value2' }, + params: { Arg1: 'value1', arg2: 'value2=SOME=other=value' }, + }), + matches: [testHeaderMatches, testQueryParamMatches, testAllMatchTypes, testAnyMatch], // request matches testAllMatchTypes and testAnyMatch. But first match should win. + expectedRedirect: '/a-match', + }, + ]; - hm.redirect(test.request); - if (test.expectedReturn) { - expect(test.request.testReturned).to.equal(test.expectedReturn); - } else if (test.expectedRedirect) { - expect(test.request.testRedirectedTo).to.equal(test.expectedRedirect); - } - }); - }); + tests.forEach((test) => { + it(test.name, () => { + hm.internalRedirect(test.request, test.matches); + if (test.expectedReturn) { + expect(test.request.testReturned).to.equal(test.expectedReturn); + } else if (test.expectedRedirect) { + expect(test.request.testRedirectedTo).to.equal(test.expectedRedirect); + } + }); + }); }); From 26d766ab3e0ae6438a9c8e37a2755119af17411e Mon Sep 17 00:00:00 2001 From: Saloni Date: Wed, 17 Apr 2024 11:01:40 -0600 Subject: [PATCH 02/27] add redirect to export list --- internal/mode/static/nginx/modules/src/httpmatches.js | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/mode/static/nginx/modules/src/httpmatches.js b/internal/mode/static/nginx/modules/src/httpmatches.js index b16daa32d3..da82f57898 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/internal/mode/static/nginx/modules/src/httpmatches.js @@ -171,6 +171,7 @@ function paramsMatch(requestParams, params) { } export default { + redirect, internalRedirect, testMatch, findWinningMatch, From f0da36dbf255ef7fed326de02e6e00ecb006fb3e Mon Sep 17 00:00:00 2001 From: Saloni Date: Mon, 22 Apr 2024 09:51:47 -0600 Subject: [PATCH 03/27] update based on reviews --- .../mode/static/nginx/config/generator.go | 51 +++--- .../static/nginx/config/generator_test.go | 12 +- .../mode/static/nginx/config/http/config.go | 18 --- internal/mode/static/nginx/config/maps.go | 8 +- .../mode/static/nginx/config/maps_test.go | 3 +- internal/mode/static/nginx/config/servers.go | 119 ++++++++------ .../static/nginx/config/servers_template.go | 2 +- .../mode/static/nginx/config/servers_test.go | 145 +++++++++--------- .../mode/static/nginx/config/split_clients.go | 9 +- .../static/nginx/config/split_clients_test.go | 3 +- .../mode/static/nginx/config/upstreams.go | 8 +- .../static/nginx/config/upstreams_test.go | 3 +- .../static/nginx/modules/src/httpmatches.js | 28 ++++ .../nginx/modules/test/httpmatches.test.js | 31 ++++ 14 files changed, 260 insertions(+), 180 deletions(-) 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 da82f57898..ca3cba4326 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/internal/mode/static/nginx/modules/src/httpmatches.js @@ -15,6 +15,13 @@ function redirect(r) { r.return(HTTP_CODES.internalServerError); return; } + + try { + internalRedirect(r, matchList); + } catch (e) { + r.error(e.message); + r.return(HTTP_CODES.internalServerError); + } } function internalRedirect(r, matchList) { @@ -23,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); @@ -50,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 { @@ -173,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 63278b69cb..f04ed9df49 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 = [ { From a0e31b0f9f6fecd9a62db52ca271022f06206e12 Mon Sep 17 00:00:00 2001 From: Saloni Date: Mon, 22 Apr 2024 11:19:17 -0600 Subject: [PATCH 04/27] remove print statements --- internal/mode/static/nginx/config/servers.go | 1 + .../mode/static/nginx/config/servers_test.go | 58 ++++++++++--------- 2 files changed, 31 insertions(+), 28 deletions(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 746a6b582d..b1eae50602 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -170,6 +170,7 @@ func createLocations(server *dataplane.VirtualServer, serverID int) ([]http.Loca key += "EXACT" } extLocations[i].HTTPMatchKey = key + fmt.Println("key", key, loc.Path) matchPairs[extLocations[i].HTTPMatchKey] = matches } locs = append(locs, extLocations...) diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 450e0bd882..3afc5bf3af 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -560,6 +560,9 @@ func TestCreateServers(t *testing.T) { RedirectPath: "@rule10-route0", }, }, + "1_12EXACT": { + {Method: "GET", RedirectPath: "@rule12-route0"}, + }, "SSL1_0": { {Method: "POST", RedirectPath: "@rule0-route0"}, {Method: "PATCH", RedirectPath: "@rule0-route1"}, @@ -573,25 +576,6 @@ func TestCreateServers(t *testing.T) { QueryParams: []string{"GrEat=EXAMPLE", "test=foo=bar"}, }, }, - "SSL1_10": { - { - RedirectPath: "@rule10-route0", - Headers: []string{"filter:this"}, - }, - }, - "SSL1_10EXACT": { - { - RedirectPath: "@rule10-route0", - Headers: []string{"filter:this"}, - }, - }, - "SSL1_12EXACT": { - { - Method: "GET", - RedirectPath: "@rule12-route0", - }, - }, - "1_12EXACT": {{Method: "GET", RedirectPath: "@rule12-route0"}}, "SSL1_6": { {RedirectPath: "@rule6-route0", Headers: []string{"redirect:this"}}, }, @@ -613,6 +597,24 @@ func TestCreateServers(t *testing.T) { Headers: []string{"rewrite:this"}, }, }, + "SSL1_10": { + { + RedirectPath: "@rule10-route0", + Headers: []string{"filter:this"}, + }, + }, + "SSL1_10EXACT": { + { + RedirectPath: "@rule10-route0", + Headers: []string{"filter:this"}, + }, + }, + "SSL1_12EXACT": { + { + Method: "GET", + RedirectPath: "@rule12-route0", + }, + }, } rewriteProxySetHeaders := []http.Header{ @@ -660,7 +662,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "/", - HTTPMatchKey: ssl + "1" + port, + HTTPMatchKey: ssl + "1_0" + port, }, { Path: "@rule1-route0", @@ -669,7 +671,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "/test/", - HTTPMatchKey: ssl + "2" + port, + HTTPMatchKey: ssl + "1_1" + port, }, { Path: "/path-only/", @@ -736,11 +738,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/redirect-with-headers/", - HTTPMatchKey: ssl + "3" + port, + HTTPMatchKey: ssl + "1_6" + port, }, { Path: "= /redirect-with-headers", - HTTPMatchKey: ssl + "4" + port + "EXACT", + HTTPMatchKey: ssl + "1_6" + port + "EXACT", }, { Path: "/rewrite/", @@ -762,11 +764,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/rewrite-with-headers/", - HTTPMatchKey: ssl + "5" + port, + HTTPMatchKey: ssl + "1_8" + port, }, { Path: "= /rewrite-with-headers", - HTTPMatchKey: ssl + "6" + port + "EXACT", + HTTPMatchKey: ssl + "1_8" + port + "EXACT", }, { Path: "/invalid-filter/", @@ -788,11 +790,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/invalid-filter-with-headers/", - HTTPMatchKey: ssl + "7" + port, + HTTPMatchKey: ssl + "1_10" + port, }, { Path: "= /invalid-filter-with-headers", - HTTPMatchKey: ssl + "8" + port + "EXACT", + HTTPMatchKey: ssl + "1_10" + port + "EXACT", }, { Path: "= /exact", @@ -806,7 +808,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "= /test", - HTTPMatchKey: ssl + "11" + port + "EXACT", + HTTPMatchKey: ssl + "1_12" + port + "EXACT", }, { Path: "/proxy-set-headers/", From 44c632da39e9e16976361cb144188be5efa08561 Mon Sep 17 00:00:00 2001 From: Saloni Date: Mon, 22 Apr 2024 11:31:04 -0600 Subject: [PATCH 05/27] fix unit test --- internal/mode/static/nginx/config/servers.go | 1 - .../mode/static/nginx/config/servers_test.go | 32 +++++++++++-------- 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index b1eae50602..746a6b582d 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -170,7 +170,6 @@ func createLocations(server *dataplane.VirtualServer, serverID int) ([]http.Loca key += "EXACT" } extLocations[i].HTTPMatchKey = key - fmt.Println("key", key, loc.Path) matchPairs[extLocations[i].HTTPMatchKey] = matches } locs = append(locs, extLocations...) diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 3afc5bf3af..54e3e142db 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -637,10 +637,10 @@ func TestCreateServers(t *testing.T) { } getExpectedLocations := func(isHTTPS bool) []http.Location { - port := "8080" + port := 8080 ssl := "" if isHTTPS { - port = "8443" + port = 8443 ssl = "SSL" } @@ -662,7 +662,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "/", - HTTPMatchKey: ssl + "1_0" + port, + HTTPMatchKey: ssl + "1_0", }, { Path: "@rule1-route0", @@ -671,7 +671,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "/test/", - HTTPMatchKey: ssl + "1_1" + port, + HTTPMatchKey: ssl + "1_1", }, { Path: "/path-only/", @@ -705,14 +705,14 @@ func TestCreateServers(t *testing.T) { Path: "/redirect-implicit-port/", Return: &http.Return{ Code: 302, - Body: "$scheme://foo.example.com:%d$request_uri" + port, + Body: fmt.Sprintf("$scheme://foo.example.com:%d$request_uri", port), }, }, { Path: "= /redirect-implicit-port", Return: &http.Return{ Code: 302, - Body: "$scheme://foo.example.com:%d$request_uri" + port, + Body: fmt.Sprintf("$scheme://foo.example.com:%d$request_uri", port), }, }, { @@ -738,11 +738,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/redirect-with-headers/", - HTTPMatchKey: ssl + "1_6" + port, + HTTPMatchKey: ssl + "1_6", }, { Path: "= /redirect-with-headers", - HTTPMatchKey: ssl + "1_6" + port + "EXACT", + HTTPMatchKey: ssl + "1_6" + "EXACT", }, { Path: "/rewrite/", @@ -764,11 +764,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/rewrite-with-headers/", - HTTPMatchKey: ssl + "1_8" + port, + HTTPMatchKey: ssl + "1_8", }, { Path: "= /rewrite-with-headers", - HTTPMatchKey: ssl + "1_8" + port + "EXACT", + HTTPMatchKey: ssl + "1_8" + "EXACT", }, { Path: "/invalid-filter/", @@ -790,11 +790,11 @@ func TestCreateServers(t *testing.T) { }, { Path: "/invalid-filter-with-headers/", - HTTPMatchKey: ssl + "1_10" + port, + HTTPMatchKey: ssl + "1_10", }, { Path: "= /invalid-filter-with-headers", - HTTPMatchKey: ssl + "1_10" + port + "EXACT", + HTTPMatchKey: ssl + "1_10" + "EXACT", }, { Path: "= /exact", @@ -808,7 +808,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "= /test", - HTTPMatchKey: ssl + "1_12" + port + "EXACT", + HTTPMatchKey: ssl + "1_12" + "EXACT", }, { Path: "/proxy-set-headers/", @@ -896,7 +896,11 @@ func TestCreateServers(t *testing.T) { result, httpMatchPair := createServers(httpServers, sslServers) - g.Expect(helpers.Diff(expMatchPairs, httpMatchPair)).To(BeEmpty()) + for key, val := range httpMatchPair { + expVal, ok := expMatchPairs[key] + g.Expect(ok).To(BeTrue()) + g.Expect(val).To(Equal(expVal)) + } g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) } From e40a5cce7936018d2b0952a2b4015d8a09ff58d2 Mon Sep 17 00:00:00 2001 From: Saloni Date: Mon, 22 Apr 2024 12:02:02 -0600 Subject: [PATCH 06/27] update unit tests --- internal/mode/static/nginx/config/servers_test.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 54e3e142db..9ed3e8d5a6 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -513,7 +513,7 @@ func TestCreateServers(t *testing.T) { }, } - expMatchPairs := map[string][]RouteMatch{ + expMatchPairs := httpMatchPairs{ "1_0": { {Method: "POST", RedirectPath: "@rule0-route0"}, {Method: "PATCH", RedirectPath: "@rule0-route1"}, @@ -896,11 +896,7 @@ func TestCreateServers(t *testing.T) { result, httpMatchPair := createServers(httpServers, sslServers) - for key, val := range httpMatchPair { - expVal, ok := expMatchPairs[key] - g.Expect(ok).To(BeTrue()) - g.Expect(val).To(Equal(expVal)) - } + g.Expect(httpMatchPair).To(Equal(expMatchPairs)) g.Expect(helpers.Diff(expectedServers, result)).To(BeEmpty()) } From d96fcaa5dfdfe9bb4643fe77336c290cbae1207f Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Mon, 22 Apr 2024 16:35:48 -0600 Subject: [PATCH 07/27] Update internal/mode/static/nginx/config/generator.go Co-authored-by: Saylor Berman --- internal/mode/static/nginx/config/generator.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index bf63b1afe1..98a9fb32b2 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -74,8 +74,7 @@ func (g GeneratorImpl) Generate(conf dataplane.Configuration) []file.File { files = append(files, generatePEM(id, pair.Cert, pair.Key)) } - httpFiles := g.generateHTTPConfig(conf) - files = append(files, httpFiles...) + files = append(files, g.generateHTTPConfig(conf)...) files = append(files, generateConfigVersion(conf.Version)) From 7897819fc8a42269e260065f6616b359b488508d Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Mon, 22 Apr 2024 16:36:16 -0600 Subject: [PATCH 08/27] Update internal/mode/static/nginx/config/servers.go Co-authored-by: Saylor Berman --- internal/mode/static/nginx/config/servers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 746a6b582d..78369b5040 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -163,7 +163,7 @@ func createLocations(server *dataplane.VirtualServer, serverID int) ([]http.Loca // https://github.com/nginxinc/nginx-gateway-fabric/issues/662 var key string if server.SSL != nil { - key += "SSL" + key = "SSL" } key += strconv.Itoa(serverID) + "_" + strconv.Itoa(pathRuleIdx) if strings.Contains(loc.Path, "= /") { From 81b56555d5945bb3d86d1974f346e91b43cd0c34 Mon Sep 17 00:00:00 2001 From: Saloni Date: Mon, 22 Apr 2024 17:03:21 -0600 Subject: [PATCH 09/27] update key for match.json --- internal/mode/static/nginx/config/servers.go | 5 +- .../mode/static/nginx/config/servers_test.go | 55 +++++-------------- 2 files changed, 15 insertions(+), 45 deletions(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 78369b5040..a651cf2f2f 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -157,7 +157,7 @@ func createLocations(server *dataplane.VirtualServer, serverID int) ([]http.Loca } if len(matches) > 0 { - for i, loc := range extLocations { + for i := 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 @@ -166,9 +166,6 @@ func createLocations(server *dataplane.VirtualServer, serverID int) ([]http.Loca 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 } diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 9ed3e8d5a6..cde7d3dd4a 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -530,39 +530,18 @@ func TestCreateServers(t *testing.T) { "1_6": { {RedirectPath: "@rule6-route0", Headers: []string{"redirect:this"}}, }, - "1_6EXACT": { - { - RedirectPath: "@rule6-route0", - Headers: []string{"redirect:this"}, - }, - }, "1_8": { { Headers: []string{"rewrite:this"}, RedirectPath: "@rule8-route0", }, }, - "1_8EXACT": { - { - RedirectPath: "@rule8-route0", - Headers: []string{"rewrite:this"}, - }, - }, "1_10": { { Headers: []string{"filter:this"}, RedirectPath: "@rule10-route0", }, }, - "1_10EXACT": { - { - Headers: []string{"filter:this"}, - RedirectPath: "@rule10-route0", - }, - }, - "1_12EXACT": { - {Method: "GET", RedirectPath: "@rule12-route0"}, - }, "SSL1_0": { {Method: "POST", RedirectPath: "@rule0-route0"}, {Method: "PATCH", RedirectPath: "@rule0-route1"}, @@ -579,40 +558,34 @@ func TestCreateServers(t *testing.T) { "SSL1_6": { {RedirectPath: "@rule6-route0", Headers: []string{"redirect:this"}}, }, - "SSL1_6EXACT": { - { - Headers: []string{"redirect:this"}, - RedirectPath: "@rule6-route0", - }, - }, "SSL1_8": { { Headers: []string{"rewrite:this"}, RedirectPath: "@rule8-route0", }, }, - "SSL1_8EXACT": { - { - RedirectPath: "@rule8-route0", - Headers: []string{"rewrite:this"}, - }, - }, "SSL1_10": { { RedirectPath: "@rule10-route0", Headers: []string{"filter:this"}, }, }, - "SSL1_10EXACT": { + "1_12": { { - RedirectPath: "@rule10-route0", - Headers: []string{"filter:this"}, + Method: "GET", + RedirectPath: "@rule12-route0", + Headers: nil, + QueryParams: nil, + Any: false, }, }, - "SSL1_12EXACT": { + "SSL1_12": { { Method: "GET", RedirectPath: "@rule12-route0", + Headers: nil, + QueryParams: nil, + Any: false, }, }, } @@ -742,7 +715,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "= /redirect-with-headers", - HTTPMatchKey: ssl + "1_6" + "EXACT", + HTTPMatchKey: ssl + "1_6", }, { Path: "/rewrite/", @@ -768,7 +741,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "= /rewrite-with-headers", - HTTPMatchKey: ssl + "1_8" + "EXACT", + HTTPMatchKey: ssl + "1_8", }, { Path: "/invalid-filter/", @@ -794,7 +767,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "= /invalid-filter-with-headers", - HTTPMatchKey: ssl + "1_10" + "EXACT", + HTTPMatchKey: ssl + "1_10", }, { Path: "= /exact", @@ -808,7 +781,7 @@ func TestCreateServers(t *testing.T) { }, { Path: "= /test", - HTTPMatchKey: ssl + "1_12" + "EXACT", + HTTPMatchKey: ssl + "1_12", }, { Path: "/proxy-set-headers/", From 6153ddb27713275e35cc2dd46b056dc77c42d75f Mon Sep 17 00:00:00 2001 From: Saloni Date: Wed, 24 Apr 2024 12:32:40 -0600 Subject: [PATCH 10/27] format njs code --- .../static/nginx/modules/src/httpmatches.js | 140 +++++------ .../nginx/modules/test/httpmatches.test.js | 228 +++++++++--------- .../nginx/modules/test/vitest.config.ts | 10 +- 3 files changed, 190 insertions(+), 188 deletions(-) diff --git a/internal/mode/static/nginx/modules/src/httpmatches.js b/internal/mode/static/nginx/modules/src/httpmatches.js index ca3cba4326..d09f652723 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/internal/mode/static/nginx/modules/src/httpmatches.js @@ -5,76 +5,76 @@ const HTTP_CODES = { }; function redirect(r) { - let matchList; - - try { - let key = r.variables[MATCHES_KEY]; - matchList = matches[key]; - } catch (e) { - r.error(e.message); - r.return(HTTP_CODES.internalServerError); - return; - } - - try { - internalRedirect(r, matchList); - } catch (e) { - r.error(e.message); - r.return(HTTP_CODES.internalServerError); - } + let matchList; + + try { + let key = r.variables[MATCHES_KEY]; + matchList = matches[key]; + } catch (e) { + r.error(e.message); + r.return(HTTP_CODES.internalServerError); + return; + } + + try { + internalRedirect(r, matchList); + } catch (e) { + r.error(e.message); + r.return(HTTP_CODES.internalServerError); + } } function internalRedirect(r, matchList) { - // 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. - try { - verifyMatchList(matchList); - } catch (e) { - r.error(e.message); - r.return(HTTP_CODES.internalServerError); - return; - } - - let match; - try { - match = findWinningMatch(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); + // 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. + try { + verifyMatchList(matchList); + } catch (e) { + r.error(e.message); + r.return(HTTP_CODES.internalServerError); + return; + } + + let match; + try { + match = findWinningMatch(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 verifyMatchList(matchList) { - if (!Array.isArray(matchList)) { - throw Error(`cannot redirect the request; expected a list of matches, got ${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`); - } + if (matchList.length === 0) { + throw Error(`cannot redirect the request; matches is an empty list`); + } - return; + return; } function findWinningMatch(r, matches) { @@ -198,12 +198,12 @@ function paramsMatch(requestParams, params) { } export default { - redirect, - internalRedirect, - verifyMatchList, - testMatch, - findWinningMatch, - headersMatch, - paramsMatch, - HTTP_CODES, + redirect, + internalRedirect, + verifyMatchList, + testMatch, + findWinningMatch, + headersMatch, + paramsMatch, + HTTP_CODES, }; diff --git a/internal/mode/static/nginx/modules/test/httpmatches.test.js b/internal/mode/static/nginx/modules/test/httpmatches.test.js index f04ed9df49..fa05de6209 100644 --- a/internal/mode/static/nginx/modules/test/httpmatches.test.js +++ b/internal/mode/static/nginx/modules/test/httpmatches.test.js @@ -4,19 +4,19 @@ 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 = {} } = {}) { - let r = { - // Test mocks - return(statusCode) { - r.testReturned = statusCode; - }, - internalRedirect(redirectPath) { - r.testRedirectedTo = redirectPath; - }, - error(msg) { - console.log('\tngx_error:', msg); - }, - variables: {}, - }; + let r = { + // Test mocks + return(statusCode) { + r.testReturned = statusCode; + }, + internalRedirect(redirectPath) { + r.testRedirectedTo = redirectPath; + }, + error(msg) { + console.log('\tngx_error:', msg); + }, + variables: {}, + }; if (method) { r.method = method; @@ -30,38 +30,38 @@ function createRequest({ method = '', headers = {}, params = {} } = {}) { r.args = params; } - return r; + 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()); - } - }); - }); + 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', () => { @@ -175,16 +175,18 @@ describe('findWinningMatch', () => { }, ]; - tests.forEach((test) => { - it(test.name, () => { - if (test.expectThrow) { - expect(() => hm.findWinningMatch(test.request, test.matches)).to.throw(test.errSubstring); - } else { - const result = hm.findWinningMatch(test.request, test.matches); - expect(result).to.equal(test.expected); - } - }); - }); + tests.forEach((test) => { + it(test.name, () => { + if (test.expectThrow) { + expect(() => hm.findWinningMatch(test.request, test.matches)).to.throw( + test.errSubstring, + ); + } else { + const result = hm.findWinningMatch(test.request, test.matches); + expect(result).to.equal(test.expected); + } + }); + }); }); describe('headersMatch', () => { @@ -370,67 +372,67 @@ describe('paramsMatch', () => { }); describe('internalRedirect', () => { - const testAnyMatch = { any: true, redirectPath: '/any' }; - const testHeaderMatches = { - headers: ['header1:VALUE1', 'header2:value2', 'header3:value3'], - redirectPath: '/headers', - }; - const testQueryParamMatches = { - params: ['Arg1=value1', 'arg2=value2=SOME=other=value', 'arg3===value3&*1(*+'], - redirectPath: '/params', - }; - const testAllMatchTypes = { - method: 'GET', - headers: ['header1:value1', 'header2:value2'], - params: ['Arg1=value1', 'arg2=value2=SOME=other=value'], - redirectPath: '/a-match', - }; + const testAnyMatch = { any: true, redirectPath: '/any' }; + const testHeaderMatches = { + headers: ['header1:VALUE1', 'header2:value2', 'header3:value3'], + redirectPath: '/headers', + }; + const testQueryParamMatches = { + params: ['Arg1=value1', 'arg2=value2=SOME=other=value', 'arg3===value3&*1(*+'], + redirectPath: '/params', + }; + const testAllMatchTypes = { + method: 'GET', + headers: ['header1:value1', 'header2:value2'], + params: ['Arg1=value1', 'arg2=value2=SOME=other=value'], + redirectPath: '/a-match', + }; - const tests = [ - { - 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 matchList contains malformed match', - request: createRequest(), - matches: [{ headers: ['malformedheader'] }], - expectedReturn: hm.HTTP_CODES.internalServerError, - }, - { - name: 'returns Not Found status code if request does not satisfy any match', - request: createRequest({ method: 'GET' }), - matches: [{ method: 'POST' }], - expectedReturn: hm.HTTP_CODES.notFound, - }, - { - name: 'returns Internal Server Error status code if request satisfies match, but the redirectPath is missing', - request: createRequest({ method: 'GET' }), - matches: [{ method: 'GET' }], - expectedReturn: hm.HTTP_CODES.internalServerError, - }, - { - name: 'redirects to the redirectPath of the first match the request satisfies', - request: createRequest({ - method: 'GET', - headers: { header1: 'value1', header2: 'value2' }, - params: { Arg1: 'value1', arg2: 'value2=SOME=other=value' }, - }), - matches: [testHeaderMatches, testQueryParamMatches, testAllMatchTypes, testAnyMatch], // request matches testAllMatchTypes and testAnyMatch. But first match should win. - expectedRedirect: '/a-match', - }, - ]; + const tests = [ + { + 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 matchList contains malformed match', + request: createRequest(), + matches: [{ headers: ['malformedheader'] }], + expectedReturn: hm.HTTP_CODES.internalServerError, + }, + { + name: 'returns Not Found status code if request does not satisfy any match', + request: createRequest({ method: 'GET' }), + matches: [{ method: 'POST' }], + expectedReturn: hm.HTTP_CODES.notFound, + }, + { + name: 'returns Internal Server Error status code if request satisfies match, but the redirectPath is missing', + request: createRequest({ method: 'GET' }), + matches: [{ method: 'GET' }], + expectedReturn: hm.HTTP_CODES.internalServerError, + }, + { + name: 'redirects to the redirectPath of the first match the request satisfies', + request: createRequest({ + method: 'GET', + headers: { header1: 'value1', header2: 'value2' }, + params: { Arg1: 'value1', arg2: 'value2=SOME=other=value' }, + }), + matches: [testHeaderMatches, testQueryParamMatches, testAllMatchTypes, testAnyMatch], // request matches testAllMatchTypes and testAnyMatch. But first match should win. + expectedRedirect: '/a-match', + }, + ]; - tests.forEach((test) => { - it(test.name, () => { - hm.internalRedirect(test.request, test.matches); - if (test.expectedReturn) { - expect(test.request.testReturned).to.equal(test.expectedReturn); - } else if (test.expectedRedirect) { - expect(test.request.testRedirectedTo).to.equal(test.expectedRedirect); - } - }); - }); + tests.forEach((test) => { + it(test.name, () => { + hm.internalRedirect(test.request, test.matches); + if (test.expectedReturn) { + expect(test.request.testReturned).to.equal(test.expectedReturn); + } else if (test.expectedRedirect) { + expect(test.request.testRedirectedTo).to.equal(test.expectedRedirect); + } + }); + }); }); diff --git a/internal/mode/static/nginx/modules/test/vitest.config.ts b/internal/mode/static/nginx/modules/test/vitest.config.ts index b951a942f7..8de8c069fa 100644 --- a/internal/mode/static/nginx/modules/test/vitest.config.ts +++ b/internal/mode/static/nginx/modules/test/vitest.config.ts @@ -1,9 +1,9 @@ import { defineConfig } from 'vitest/config'; export default defineConfig({ - test: { - coverage: { - reporter: ['text', 'json', 'html'], - }, - }, + test: { + coverage: { + reporter: ['text', 'json', 'html'], + }, + }, }); From 75cdb8fc772f70f6c3505bd323ec55a38bdb8c8c Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Wed, 24 Apr 2024 13:15:02 -0600 Subject: [PATCH 11/27] Update internal/mode/static/nginx/config/generator.go Co-authored-by: Michael Pleshakov --- internal/mode/static/nginx/config/generator.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index 98a9fb32b2..fa452b59ce 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -25,7 +25,7 @@ const ( configVersionFile = httpFolder + "/config-version.conf" // httpMatchVarsFile is the path to the http_match pairs configuration file. - httpMatchVarsFile = httpFolder + "/match.json" + httpMatchVarsFile = httpFolder + "/matches.json" ) // ConfigFolders is a list of folders where NGINX configuration files are stored. From 4ad2de16898818588822a77918aec55ce5602a18 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Wed, 24 Apr 2024 13:15:44 -0600 Subject: [PATCH 12/27] Update internal/mode/static/nginx/config/servers.go Co-authored-by: Michael Pleshakov --- internal/mode/static/nginx/config/servers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index a651cf2f2f..5ccaefb8e9 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -68,7 +68,7 @@ func createServers(httpServers, sslServers []dataplane.VirtualServer) ([]http.Se finalMatchPairs := make(httpMatchPairs) for serverID, s := range httpServers { - httpServer, matchPair := createServer(s, serverID) + httpServer, matchPairs := createServer(s, serverID) servers = append(servers, httpServer) maps.Copy(finalMatchPairs, matchPair) } From d0afa69331d509070faad6e2933141b6fb8123de Mon Sep 17 00:00:00 2001 From: Saloni Date: Wed, 24 Apr 2024 13:37:05 -0600 Subject: [PATCH 13/27] review comments --- .../mode/static/nginx/config/maps_test.go | 2 + internal/mode/static/nginx/config/servers.go | 26 +++--- .../static/nginx/config/servers_template.go | 2 +- .../mode/static/nginx/config/servers_test.go | 79 +++++++------------ .../static/nginx/config/split_clients_test.go | 2 + .../static/nginx/config/upstreams_test.go | 3 + .../static/nginx/modules/src/httpmatches.js | 11 +-- 7 files changed, 53 insertions(+), 72 deletions(-) diff --git a/internal/mode/static/nginx/config/maps_test.go b/internal/mode/static/nginx/config/maps_test.go index 20c914b9b8..5c510210af 100644 --- a/internal/mode/static/nginx/config/maps_test.go +++ b/internal/mode/static/nginx/config/maps_test.go @@ -90,6 +90,8 @@ func TestExecuteMaps(t *testing.T) { 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))) } diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 5ccaefb8e9..a8935f270c 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -70,7 +70,7 @@ func createServers(httpServers, sslServers []dataplane.VirtualServer) ([]http.Se for serverID, s := range httpServers { httpServer, matchPairs := createServer(s, serverID) servers = append(servers, httpServer) - maps.Copy(finalMatchPairs, matchPair) + maps.Copy(finalMatchPairs, matchPairs) } for serverID, s := range sslServers { @@ -127,7 +127,7 @@ type rewriteConfig struct { Rewrite string } -type httpMatchPairs map[string][]RouteMatch +type httpMatchPairs map[string][]routeMatch func createLocations(server *dataplane.VirtualServer, serverID int) ([]http.Location, httpMatchPairs) { maxLocs, pathsAndTypes := getMaxLocationCountAndPathMap(server.PathRules) @@ -136,7 +136,7 @@ func createLocations(server *dataplane.VirtualServer, serverID int) ([]http.Loca var rootPathExists bool for pathRuleIdx, rule := range server.PathRules { - matches := make([]RouteMatch, 0, len(rule.MatchRules)) + matches := make([]routeMatch, 0, len(rule.MatchRules)) if rule.Path == rootPath { rootPathExists = true @@ -256,9 +256,9 @@ func initializeInternalLocation( pathruleIdx, matchRuleIdx int, match dataplane.Match, -) (http.Location, RouteMatch) { +) (http.Location, routeMatch) { path := fmt.Sprintf("@rule%d-route%d", pathruleIdx, matchRuleIdx) - return createMatchLocation(path), createRouteMatch(match, path) + return createMatchLocation(path), createrouteMatch(match, path) } // updateLocationsForFilters updates the existing locations with any relevant filters. @@ -431,13 +431,13 @@ 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. +// httpMatch 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 RouteMatch struct { - // Method is the HTTPMethod of the HTTPRouteMatch. +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"` @@ -449,8 +449,8 @@ type RouteMatch struct { Any bool `json:"any,omitempty"` } -func createRouteMatch(match dataplane.Match, redirectPath string) RouteMatch { - hm := RouteMatch{ +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 6364303f70..8d272e05a7 100644 --- a/internal/mode/static/nginx/config/servers_template.go +++ b/internal/mode/static/nginx/config/servers_template.go @@ -1,7 +1,7 @@ package config var serversTemplateText = ` -js_preload_object matches from /etc/nginx/conf.d/match.json; +js_preload_object matches from /etc/nginx/conf.d/matches.json; {{- range $s := . -}} {{ if $s.IsDefaultSSL -}} server { diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index cde7d3dd4a..5f6549efed 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -2,6 +2,7 @@ package config import ( "fmt" + "maps" "strings" "testing" @@ -542,34 +543,6 @@ func TestCreateServers(t *testing.T) { RedirectPath: "@rule10-route0", }, }, - "SSL1_0": { - {Method: "POST", RedirectPath: "@rule0-route0"}, - {Method: "PATCH", RedirectPath: "@rule0-route1"}, - {Any: true, RedirectPath: "@rule0-route2"}, - }, - "SSL1_1": { - { - Method: "GET", - RedirectPath: "@rule1-route0", - Headers: []string{"Version:V1", "test:foo", "my-header:my-value"}, - QueryParams: []string{"GrEat=EXAMPLE", "test=foo=bar"}, - }, - }, - "SSL1_6": { - {RedirectPath: "@rule6-route0", Headers: []string{"redirect:this"}}, - }, - "SSL1_8": { - { - Headers: []string{"rewrite:this"}, - RedirectPath: "@rule8-route0", - }, - }, - "SSL1_10": { - { - RedirectPath: "@rule10-route0", - Headers: []string{"filter:this"}, - }, - }, "1_12": { { Method: "GET", @@ -579,17 +552,13 @@ func TestCreateServers(t *testing.T) { Any: false, }, }, - "SSL1_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", @@ -869,10 +838,20 @@ func TestCreateServers(t *testing.T) { result, httpMatchPair := createServers(httpServers, sslServers) - g.Expect(httpMatchPair).To(Equal(expMatchPairs)) + 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"}, @@ -1450,7 +1429,7 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) { } } -func TestCreateRouteMatch(t *testing.T) { +func TestCreaterouteMatch(t *testing.T) { testPath := "/internal_loc" testMethodMatch := helpers.GetPointer("PUT") @@ -1498,11 +1477,11 @@ func TestCreateRouteMatch(t *testing.T) { tests := []struct { match dataplane.Match msg string - expected RouteMatch + expected routeMatch }{ { match: dataplane.Match{}, - expected: RouteMatch{ + expected: routeMatch{ Any: true, RedirectPath: testPath, }, @@ -1512,7 +1491,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: RouteMatch{ + expected: routeMatch{ Method: "PUT", RedirectPath: testPath, }, @@ -1522,7 +1501,7 @@ func TestCreateRouteMatch(t *testing.T) { match: dataplane.Match{ Headers: testHeaderMatches, }, - expected: RouteMatch{ + expected: routeMatch{ RedirectPath: testPath, Headers: expectedHeaders, }, @@ -1532,7 +1511,7 @@ func TestCreateRouteMatch(t *testing.T) { match: dataplane.Match{ QueryParams: testQueryParamMatches, }, - expected: RouteMatch{ + expected: routeMatch{ QueryParams: expectedArgs, RedirectPath: testPath, }, @@ -1543,7 +1522,7 @@ func TestCreateRouteMatch(t *testing.T) { Method: testMethodMatch, QueryParams: testQueryParamMatches, }, - expected: RouteMatch{ + expected: routeMatch{ Method: "PUT", QueryParams: expectedArgs, RedirectPath: testPath, @@ -1555,7 +1534,7 @@ func TestCreateRouteMatch(t *testing.T) { Method: testMethodMatch, Headers: testHeaderMatches, }, - expected: RouteMatch{ + expected: routeMatch{ Method: "PUT", Headers: expectedHeaders, RedirectPath: testPath, @@ -1567,7 +1546,7 @@ func TestCreateRouteMatch(t *testing.T) { QueryParams: testQueryParamMatches, Headers: testHeaderMatches, }, - expected: RouteMatch{ + expected: routeMatch{ QueryParams: expectedArgs, Headers: expectedHeaders, RedirectPath: testPath, @@ -1580,7 +1559,7 @@ func TestCreateRouteMatch(t *testing.T) { QueryParams: testQueryParamMatches, Method: testMethodMatch, }, - expected: RouteMatch{ + expected: routeMatch{ Method: "PUT", Headers: expectedHeaders, QueryParams: expectedArgs, @@ -1592,7 +1571,7 @@ func TestCreateRouteMatch(t *testing.T) { match: dataplane.Match{ Headers: testDuplicateHeaders, }, - expected: RouteMatch{ + expected: routeMatch{ Headers: expectedHeaders, RedirectPath: testPath, }, @@ -1603,7 +1582,7 @@ func TestCreateRouteMatch(t *testing.T) { t.Run(tc.msg, func(t *testing.T) { g := NewWithT(t) - result := createRouteMatch(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_test.go b/internal/mode/static/nginx/config/split_clients_test.go index 77422fa7e7..4f81ab4f52 100644 --- a/internal/mode/static/nginx/config/split_clients_test.go +++ b/internal/mode/static/nginx/config/split_clients_test.go @@ -100,6 +100,8 @@ func TestExecuteSplitClients(t *testing.T) { g := NewWithT(t) splitResults := executeSplitClients(dataplane.Configuration{BackendGroups: test.backendGroups}) sc := string(splitResults[0].data) + g.Expect(splitResults).To(HaveLen(1)) + 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_test.go b/internal/mode/static/nginx/config/upstreams_test.go index e96057dd5b..29bcbb6f11 100644 --- a/internal/mode/static/nginx/config/upstreams_test.go +++ b/internal/mode/static/nginx/config/upstreams_test.go @@ -49,7 +49,10 @@ func TestExecuteUpstreams(t *testing.T) { upstreamResults := gen.executeUpstreams(dataplane.Configuration{Upstreams: stateUpstreams}) upstreams := string(upstreamResults[0].data) + g := NewWithT(t) + g.Expect(upstreamResults).To(HaveLen(1)) + 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 d09f652723..91a7cf7bd5 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/internal/mode/static/nginx/modules/src/httpmatches.js @@ -16,15 +16,10 @@ function redirect(r) { return; } - try { - internalRedirect(r, matchList); - } catch (e) { - r.error(e.message); - r.return(HTTP_CODES.internalServerError); - } + redirectForMatchList(r, matchList); } -function internalRedirect(r, matchList) { +function redirectForMatchList(r, matchList) { // 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. @@ -199,7 +194,7 @@ function paramsMatch(requestParams, params) { export default { redirect, - internalRedirect, + redirectForMatchList, verifyMatchList, testMatch, findWinningMatch, From 8f117c7e9e687521db5eb13cb524bb9a4b2835df Mon Sep 17 00:00:00 2001 From: Saloni Date: Wed, 24 Apr 2024 18:44:24 -0600 Subject: [PATCH 14/27] improve httpmatches.js --- .../static/nginx/modules/src/httpmatches.js | 26 ++++++++-- .../nginx/modules/test/httpmatches.test.js | 48 +++++++++++++++++-- 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/internal/mode/static/nginx/modules/src/httpmatches.js b/internal/mode/static/nginx/modules/src/httpmatches.js index 91a7cf7bd5..cea893739e 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/internal/mode/static/nginx/modules/src/httpmatches.js @@ -8,8 +8,7 @@ function redirect(r) { let matchList; try { - let key = r.variables[MATCHES_KEY]; - matchList = matches[key]; + matchList = extractMatchesFromRequest(r, matches); } catch (e) { r.error(e.message); r.return(HTTP_CODES.internalServerError); @@ -19,12 +18,27 @@ function redirect(r) { redirectForMatchList(r, matchList); } -function 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`, + ); + } + + 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) { @@ -33,6 +47,10 @@ function redirectForMatchList(r, matchList) { return; } + return matchList; +} + +function redirectForMatchList(r, matchList) { let match; try { match = findWinningMatch(r, matchList); @@ -195,6 +213,8 @@ function paramsMatch(requestParams, params) { export default { redirect, redirectForMatchList, + extractMatchesFromRequest, + MATCHES_KEY, verifyMatchList, testMatch, findWinningMatch, diff --git a/internal/mode/static/nginx/modules/test/httpmatches.test.js b/internal/mode/static/nginx/modules/test/httpmatches.test.js index fa05de6209..1f56bdf8de 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 = {} } = {}) { +function createRequest({ method = '', headers = {}, params = {}, matchKey = '' } = {}) { let r = { // Test mocks return(statusCode) { @@ -30,9 +30,51 @@ function createRequest({ method = '', headers = {}, params = {} } = {}) { r.args = params; } + if (matchKey) { + r.variables[hm.MATCHES_KEY] = matchKey; + } + return r; } +describe('extractMatchesFromRequest', () => { + const tests = [ + { + 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 request', + request: createRequest({}), + matchesObject: { test: '["any": "true"]' }, + expectThrow: true, + errSubstring: 'match_key is not defined', + }, + { + name: 'does not throw if matches key is present & expected matchList is returned', + request: createRequest({ matchKey: 'test' }), + matchesObject: { test: '[]' }, + expectThrow: false, + }, + ]; + 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.not.throw(), + ); + } + }); + }); +}); + describe('verifyMatchList', () => { const tests = [ { @@ -371,7 +413,7 @@ describe('paramsMatch', () => { }); }); -describe('internalRedirect', () => { +describe('redirectForMatchList', () => { const testAnyMatch = { any: true, redirectPath: '/any' }; const testHeaderMatches = { headers: ['header1:VALUE1', 'header2:value2', 'header3:value3'], @@ -427,7 +469,7 @@ describe('internalRedirect', () => { tests.forEach((test) => { it(test.name, () => { - hm.internalRedirect(test.request, test.matches); + hm.redirectForMatchList(test.request, test.matches); if (test.expectedReturn) { expect(test.request.testReturned).to.equal(test.expectedReturn); } else if (test.expectedRedirect) { From 5ca49f78e78eab20440bec584d1d353b32c87995 Mon Sep 17 00:00:00 2001 From: Saloni Date: Wed, 24 Apr 2024 20:06:14 -0600 Subject: [PATCH 15/27] address reviews --- .../mode/static/nginx/config/generator.go | 26 +++++++++---------- .../static/nginx/config/generator_test.go | 14 +++++----- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index fa452b59ce..28646bc98c 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -116,28 +116,28 @@ func generateCertBundleFileName(id dataplane.CertBundleID) string { func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) []file.File { files := make([]file.File, 0) - var c []byte + fileBytes := make(map[string][]byte) for _, execute := range g.getExecuteFuncs() { results := execute(conf) for _, res := range results { - if res.dest == httpConfigFile { - c = append(c, res.data...) + b, ok := fileBytes[res.dest] + if ok { + b = append(b, res.data...) + fileBytes[res.dest] = b } else { - files = append(files, file.File{ - Path: res.dest, - Content: res.data, - Type: file.TypeRegular, - }) + fileBytes[res.dest] = res.data } } } - files = append(files, file.File{ - Path: httpConfigFile, - Content: c, - Type: file.TypeRegular, - }) + for filepath, bytes := range fileBytes { + files = append(files, file.File{ + Path: filepath, + Content: bytes, + Type: file.TypeRegular, + }) + } return files } diff --git a/internal/mode/static/nginx/config/generator_test.go b/internal/mode/static/nginx/config/generator_test.go index 648fe341bb..3097bd1676 100644 --- a/internal/mode/static/nginx/config/generator_test.go +++ b/internal/mode/static/nginx/config/generator_test.go @@ -78,14 +78,9 @@ 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)) - 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 + 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 // 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")) @@ -93,6 +88,11 @@ 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)) + 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) From 3683dd70751d97045ca86cc4caeb8b3a3cda624c Mon Sep 17 00:00:00 2001 From: Saloni Date: Thu, 25 Apr 2024 09:19:04 -0600 Subject: [PATCH 16/27] add more unit test coverage for njs unit tests --- .../static/nginx/modules/src/httpmatches.js | 6 +--- .../nginx/modules/test/httpmatches.test.js | 28 ++++++++++++------- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/internal/mode/static/nginx/modules/src/httpmatches.js b/internal/mode/static/nginx/modules/src/httpmatches.js index cea893739e..1a7d575cd3 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/internal/mode/static/nginx/modules/src/httpmatches.js @@ -6,7 +6,6 @@ const HTTP_CODES = { function redirect(r) { let matchList; - try { matchList = extractMatchesFromRequest(r, matches); } catch (e) { @@ -14,7 +13,6 @@ function redirect(r) { r.return(HTTP_CODES.internalServerError); return; } - redirectForMatchList(r, matchList); } @@ -42,9 +40,7 @@ function extractMatchesFromRequest(r, matches) { try { verifyMatchList(matchList); } catch (e) { - r.error(e.message); - r.return(HTTP_CODES.internalServerError); - return; + throw Error(`cannot redirect the request; ${matchList} matches list is not valid`); } return matchList; diff --git a/internal/mode/static/nginx/modules/test/httpmatches.test.js b/internal/mode/static/nginx/modules/test/httpmatches.test.js index 1f56bdf8de..4372fa6df1 100644 --- a/internal/mode/static/nginx/modules/test/httpmatches.test.js +++ b/internal/mode/static/nginx/modules/test/httpmatches.test.js @@ -47,17 +47,25 @@ describe('extractMatchesFromRequest', () => { errSubstring: 'match_key is not defined', }, { - name: 'throws if key does not exist on matches request', - request: createRequest({}), - matchesObject: { test: '["any": "true"]' }, + name: 'throws if key does not exist on matches object', + request: createRequest({ matchKey: 'test' }), + matchesObject: {}, expectThrow: true, - errSubstring: 'match_key is not defined', + errSubstring: 'the key test is not defined on the matches object', + }, + { + name: 'throws an error if matchList is not valid', + request: createRequest({ matchKey: 'test' }), + matchesObject: { test: {} }, + expectThrow: true, + errSubstring: 'matches list is not valid', }, { name: 'does not throw if matches key is present & expected matchList is returned', request: createRequest({ matchKey: 'test' }), - matchesObject: { test: '[]' }, + matchesObject: { test: [{ match: 'value' }] }, expectThrow: false, + expected: [{ match: 'value' }], }, ]; tests.forEach((test) => { @@ -67,9 +75,9 @@ describe('extractMatchesFromRequest', () => { hm.extractMatchesFromRequest(test.request, test.matchesObject), ).to.throw(test.errSubstring); } else { - expect(() => - hm.extractMatchesFromRequest(test.request, test.matchesObject).to.not.throw(), - ); + expect( + hm.extractMatchesFromRequest(test.request, test.matchesObject), + ).to.deep.equal(test.expected); } }); }); @@ -85,9 +93,9 @@ describe('verifyMatchList', () => { }, { name: 'throws if the length of the matches variable is zero', - matchList: '[]', + matchList: [], expectThrow: true, - errSubstring: 'expected a list of matches', + errSubstring: 'matches is an empty list', }, { name: 'does not throw if matches variable is expected list of matches', From 6e7e79e5e903b8c5f84e91be7c79af06ca76e074 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Thu, 25 Apr 2024 09:23:03 -0600 Subject: [PATCH 17/27] Update internal/mode/static/nginx/config/servers.go Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com> --- internal/mode/static/nginx/config/servers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index a8935f270c..47d1324b42 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -449,7 +449,7 @@ type routeMatch struct { Any bool `json:"any,omitempty"` } -func createrouteMatch(match dataplane.Match, redirectPath string) routeMatch { +func createRouteMatch(match dataplane.Match, redirectPath string) routeMatch { hm := routeMatch{ RedirectPath: redirectPath, } From b45920591fb97100f8fe11e260b33d6c2ac591ce Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Thu, 25 Apr 2024 09:23:10 -0600 Subject: [PATCH 18/27] Update internal/mode/static/nginx/config/servers.go Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com> --- internal/mode/static/nginx/config/servers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 47d1324b42..9d20160a1c 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -437,7 +437,7 @@ func createRewritesValForRewriteFilter(filter *dataplane.HTTPURLRewriteFilter, p // 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 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"` From 02653cef64624cb841459073a69209234d80a55f Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Thu, 25 Apr 2024 09:23:26 -0600 Subject: [PATCH 19/27] Update internal/mode/static/nginx/config/servers_test.go Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com> --- internal/mode/static/nginx/config/servers_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 5f6549efed..1031bfb8cc 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -1582,7 +1582,7 @@ func TestCreaterouteMatch(t *testing.T) { t.Run(tc.msg, func(t *testing.T) { g := NewWithT(t) - result := createrouteMatch(tc.match, testPath) + result := createRouteMatch(tc.match, testPath) g.Expect(helpers.Diff(result, tc.expected)).To(BeEmpty()) }) } From 7bf61f3c074bbbe43dffef8381f182bab6e6680d Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Thu, 25 Apr 2024 09:25:55 -0600 Subject: [PATCH 20/27] Update internal/mode/static/nginx/config/servers_test.go Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com> --- internal/mode/static/nginx/config/servers_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/config/servers_test.go b/internal/mode/static/nginx/config/servers_test.go index 1031bfb8cc..27fb3a762c 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -1429,7 +1429,7 @@ func TestCreateRewritesValForRewriteFilter(t *testing.T) { } } -func TestCreaterouteMatch(t *testing.T) { +func TestCreateRouteMatch(t *testing.T) { testPath := "/internal_loc" testMethodMatch := helpers.GetPointer("PUT") From 0a4a7ac6c4fb8848bb33ac77718997ce97ad627b Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Thu, 25 Apr 2024 09:26:02 -0600 Subject: [PATCH 21/27] Update internal/mode/static/nginx/config/servers.go Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com> --- internal/mode/static/nginx/config/servers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 9d20160a1c..62b98e5851 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -258,7 +258,7 @@ func initializeInternalLocation( match dataplane.Match, ) (http.Location, routeMatch) { path := fmt.Sprintf("@rule%d-route%d", pathruleIdx, matchRuleIdx) - return createMatchLocation(path), createrouteMatch(match, path) + return createMatchLocation(path), createRouteMatch(match, path) } // updateLocationsForFilters updates the existing locations with any relevant filters. From f63f317a90d1f27e601168cb5d799ab06a2e023f Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Thu, 25 Apr 2024 09:26:16 -0600 Subject: [PATCH 22/27] Update internal/mode/static/nginx/config/servers.go Co-authored-by: Ciara Stacke <18287516+ciarams87@users.noreply.github.com> --- internal/mode/static/nginx/config/servers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/nginx/config/servers.go b/internal/mode/static/nginx/config/servers.go index 62b98e5851..f0747ba38c 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -431,7 +431,7 @@ func createRewritesValForRewriteFilter(filter *dataplane.HTTPURLRewriteFilter, p return rewrites } -// httpMatch is an internal representation of an HTTProuteMatch. +// 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. From cc9a19ea5abcda7acf38942a636696cec74acf24 Mon Sep 17 00:00:00 2001 From: Saloni Date: Thu, 25 Apr 2024 12:51:58 -0600 Subject: [PATCH 23/27] update based on reviews --- .../mode/static/nginx/config/generator.go | 7 ++--- .../static/nginx/config/generator_test.go | 29 +++++++++++-------- internal/mode/static/nginx/config/servers.go | 4 +-- .../static/nginx/modules/src/httpmatches.js | 5 +++- .../nginx/modules/test/vitest.config.ts | 10 +++---- 5 files changed, 31 insertions(+), 24 deletions(-) diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index 28646bc98c..7a3b0f9fe3 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -115,22 +115,21 @@ func generateCertBundleFileName(id dataplane.CertBundleID) string { } func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) []file.File { - files := make([]file.File, 0) fileBytes := make(map[string][]byte) for _, execute := range g.getExecuteFuncs() { results := execute(conf) for _, res := range results { - b, ok := fileBytes[res.dest] + _, ok := fileBytes[res.dest] if ok { - b = append(b, res.data...) - fileBytes[res.dest] = b + fileBytes[res.dest] = append(fileBytes[res.dest], res.data...) } else { fileBytes[res.dest] = res.data } } } + files := make([]file.File, 0, len(fileBytes)) for filepath, bytes := range fileBytes { files = append(files, file.File{ Path: filepath, diff --git a/internal/mode/static/nginx/config/generator_test.go b/internal/mode/static/nginx/config/generator_test.go index 3097bd1676..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" @@ -71,12 +72,15 @@ func TestGenerate(t *testing.T) { files := generator.Generate(conf) 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")) @@ -93,12 +97,13 @@ func TestGenerate(t *testing.T) { 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[4].Path).To(Equal("/etc/nginx/secrets/test-certbundle.crt")) - certBundle := string(files[4].Content) + 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/servers.go b/internal/mode/static/nginx/config/servers.go index f0747ba38c..58c1602c24 100644 --- a/internal/mode/static/nginx/config/servers.go +++ b/internal/mode/static/nginx/config/servers.go @@ -434,8 +434,8 @@ func createRewritesValForRewriteFilter(filter *dataplane.HTTPURLRewriteFilter, p // 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. +// 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"` diff --git a/internal/mode/static/nginx/modules/src/httpmatches.js b/internal/mode/static/nginx/modules/src/httpmatches.js index 1a7d575cd3..ec9c87d18b 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/internal/mode/static/nginx/modules/src/httpmatches.js @@ -40,7 +40,10 @@ function extractMatchesFromRequest(r, matches) { try { verifyMatchList(matchList); } catch (e) { - throw Error(`cannot redirect the request; ${matchList} matches list is not valid`); + throw Error( + `cannot redirect the request; ${matchList} matches list is not valid`, + e.message, + ); } return matchList; diff --git a/internal/mode/static/nginx/modules/test/vitest.config.ts b/internal/mode/static/nginx/modules/test/vitest.config.ts index 8de8c069fa..b951a942f7 100644 --- a/internal/mode/static/nginx/modules/test/vitest.config.ts +++ b/internal/mode/static/nginx/modules/test/vitest.config.ts @@ -1,9 +1,9 @@ import { defineConfig } from 'vitest/config'; export default defineConfig({ - test: { - coverage: { - reporter: ['text', 'json', 'html'], - }, - }, + test: { + coverage: { + reporter: ['text', 'json', 'html'], + }, + }, }); From 27f9e531e7af7097a90d354d433f2a14f8c4965b Mon Sep 17 00:00:00 2001 From: Saloni Date: Thu, 25 Apr 2024 13:22:06 -0600 Subject: [PATCH 24/27] rearrange expect statements to avoid panic --- internal/mode/static/nginx/config/maps_test.go | 2 +- internal/mode/static/nginx/config/servers_test.go | 2 ++ internal/mode/static/nginx/config/split_clients_test.go | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/internal/mode/static/nginx/config/maps_test.go b/internal/mode/static/nginx/config/maps_test.go index 5c510210af..e903c17151 100644 --- a/internal/mode/static/nginx/config/maps_test.go +++ b/internal/mode/static/nginx/config/maps_test.go @@ -89,8 +89,8 @@ func TestExecuteMaps(t *testing.T) { } mapResult := executeMaps(conf) - maps := string(mapResult[0].data) 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_test.go b/internal/mode/static/nginx/config/servers_test.go index 27fb3a762c..3e931fe1e4 100644 --- a/internal/mode/static/nginx/config/servers_test.go +++ b/internal/mode/static/nginx/config/servers_test.go @@ -64,6 +64,7 @@ func TestExecuteServers(t *testing.T) { } g := NewWithT(t) serverResults := executeServers(conf) + g.Expect(serverResults).To(HaveLen(2)) serverConf := string(serverResults[0].data) httpMatchConf := string(serverResults[1].data) g.Expect(httpMatchConf).To(Equal("{}")) @@ -144,6 +145,7 @@ func TestExecuteForDefaultServers(t *testing.T) { g := NewWithT(t) 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("{}")) diff --git a/internal/mode/static/nginx/config/split_clients_test.go b/internal/mode/static/nginx/config/split_clients_test.go index 4f81ab4f52..9ee6785ca9 100644 --- a/internal/mode/static/nginx/config/split_clients_test.go +++ b/internal/mode/static/nginx/config/split_clients_test.go @@ -99,8 +99,8 @@ func TestExecuteSplitClients(t *testing.T) { t.Run(test.msg, func(t *testing.T) { g := NewWithT(t) splitResults := executeSplitClients(dataplane.Configuration{BackendGroups: test.backendGroups}) - sc := string(splitResults[0].data) g.Expect(splitResults).To(HaveLen(1)) + sc := string(splitResults[0].data) g.Expect(splitResults[0].dest).To(Equal(httpConfigFile)) for _, expSubString := range test.expStrings { From c637c6d2b67a8f2ebb3ed88cb2b233cf7c0a1c2e Mon Sep 17 00:00:00 2001 From: Saloni Date: Thu, 25 Apr 2024 14:31:54 -0600 Subject: [PATCH 25/27] fix unit test --- internal/mode/static/nginx/config/upstreams_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/mode/static/nginx/config/upstreams_test.go b/internal/mode/static/nginx/config/upstreams_test.go index 29bcbb6f11..8e98e4d794 100644 --- a/internal/mode/static/nginx/config/upstreams_test.go +++ b/internal/mode/static/nginx/config/upstreams_test.go @@ -48,10 +48,10 @@ func TestExecuteUpstreams(t *testing.T) { } upstreamResults := gen.executeUpstreams(dataplane.Configuration{Upstreams: stateUpstreams}) - upstreams := string(upstreamResults[0].data) - 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)) From 07fbf393898da7c57c71941bc1c74c663dcaf3bf Mon Sep 17 00:00:00 2001 From: Saloni Date: Mon, 29 Apr 2024 09:32:00 -0600 Subject: [PATCH 26/27] fix httpmatches --- internal/mode/static/nginx/config/generator.go | 7 +------ internal/mode/static/nginx/modules/src/httpmatches.js | 3 +-- .../mode/static/nginx/modules/test/httpmatches.test.js | 2 +- 3 files changed, 3 insertions(+), 9 deletions(-) diff --git a/internal/mode/static/nginx/config/generator.go b/internal/mode/static/nginx/config/generator.go index 7a3b0f9fe3..6127ffa094 100644 --- a/internal/mode/static/nginx/config/generator.go +++ b/internal/mode/static/nginx/config/generator.go @@ -120,12 +120,7 @@ func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) []file.F for _, execute := range g.getExecuteFuncs() { results := execute(conf) for _, res := range results { - _, ok := fileBytes[res.dest] - if ok { - fileBytes[res.dest] = append(fileBytes[res.dest], res.data...) - } else { - fileBytes[res.dest] = res.data - } + fileBytes[res.dest] = append(fileBytes[res.dest], res.data...) } } diff --git a/internal/mode/static/nginx/modules/src/httpmatches.js b/internal/mode/static/nginx/modules/src/httpmatches.js index ec9c87d18b..f2a493c7b9 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/internal/mode/static/nginx/modules/src/httpmatches.js @@ -41,8 +41,7 @@ function extractMatchesFromRequest(r, matches) { verifyMatchList(matchList); } catch (e) { throw Error( - `cannot redirect the request; ${matchList} matches list is not valid`, - e.message, + `cannot redirect the request; error parsing ${r.variables[MATCHES_KEY]} into a JSON object: ${e}`, ); } diff --git a/internal/mode/static/nginx/modules/test/httpmatches.test.js b/internal/mode/static/nginx/modules/test/httpmatches.test.js index 4372fa6df1..d1978c91ab 100644 --- a/internal/mode/static/nginx/modules/test/httpmatches.test.js +++ b/internal/mode/static/nginx/modules/test/httpmatches.test.js @@ -58,7 +58,7 @@ describe('extractMatchesFromRequest', () => { request: createRequest({ matchKey: 'test' }), matchesObject: { test: {} }, expectThrow: true, - errSubstring: 'matches list is not valid', + errSubstring: 'cannot redirect the request; expected a list of matches', }, { name: 'does not throw if matches key is present & expected matchList is returned', From 2dcb7c4ac8c0341e1349b578a223a1149ce42d43 Mon Sep 17 00:00:00 2001 From: Saloni Date: Mon, 29 Apr 2024 10:30:43 -0600 Subject: [PATCH 27/27] update error message --- internal/mode/static/nginx/modules/src/httpmatches.js | 4 +--- internal/mode/static/nginx/modules/test/httpmatches.test.js | 3 ++- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/mode/static/nginx/modules/src/httpmatches.js b/internal/mode/static/nginx/modules/src/httpmatches.js index f2a493c7b9..1b4eb0be36 100644 --- a/internal/mode/static/nginx/modules/src/httpmatches.js +++ b/internal/mode/static/nginx/modules/src/httpmatches.js @@ -40,9 +40,7 @@ function extractMatchesFromRequest(r, matches) { try { verifyMatchList(matchList); } catch (e) { - throw Error( - `cannot redirect the request; error parsing ${r.variables[MATCHES_KEY]} into a JSON object: ${e}`, - ); + throw Error(`cannot redirect the request; ${matchList} matches list is not valid: ${e}`); } return matchList; diff --git a/internal/mode/static/nginx/modules/test/httpmatches.test.js b/internal/mode/static/nginx/modules/test/httpmatches.test.js index d1978c91ab..d4f502ace9 100644 --- a/internal/mode/static/nginx/modules/test/httpmatches.test.js +++ b/internal/mode/static/nginx/modules/test/httpmatches.test.js @@ -58,7 +58,8 @@ describe('extractMatchesFromRequest', () => { request: createRequest({ matchKey: 'test' }), matchesObject: { test: {} }, expectThrow: true, - errSubstring: 'cannot redirect the request; expected a list of matches', + 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',