Skip to content

Commit 8df5af2

Browse files
salonichf5salonichf5
committed
update based on reviews
1 parent 06f2968 commit 8df5af2

14 files changed

+254
-205
lines changed

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

+25-26
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
package config
22

33
import (
4-
"encoding/json"
5-
"fmt"
64
"path/filepath"
75

86
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file"
@@ -57,8 +55,13 @@ func NewGeneratorImpl(plus bool) GeneratorImpl {
5755
return GeneratorImpl{plus: plus}
5856
}
5957

58+
type executeResult struct {
59+
dest string
60+
data []byte
61+
}
62+
6063
// executeFunc is a function that generates NGINX configuration from internal representation.
61-
type executeFunc func(configuration dataplane.Configuration) []byte
64+
type executeFunc func(configuration dataplane.Configuration) []executeResult
6265

6366
// Generate generates NGINX configuration files from internal representation.
6467
// 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 {
113116
}
114117

115118
func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) []file.File {
119+
files := make([]file.File, 0)
116120
var c []byte
121+
117122
for _, execute := range g.getExecuteFuncs() {
118-
c = append(c, execute(conf)...)
123+
results := execute(conf)
124+
for _, res := range results {
125+
if res.dest == httpConfigFile {
126+
c = append(c, res.data...)
127+
} else {
128+
files = append(files, file.File{
129+
Path: res.dest,
130+
Content: res.data,
131+
Type: file.TypeRegular,
132+
})
133+
}
134+
}
119135
}
120136

121-
servers, httpMatchPairs := executeServers(conf)
122-
123-
// create server conf
124-
serverConf := execute(serversTemplate, servers)
125-
c = append(c, serverConf...)
126-
127-
httpConf := file.File{
128-
Content: c,
137+
files = append(files, file.File{
129138
Path: httpConfigFile,
139+
Content: c,
130140
Type: file.TypeRegular,
131-
}
132-
133-
// create httpMatchPair conf
134-
b, err := json.Marshal(httpMatchPairs)
135-
if err != nil {
136-
// panic is safe here because we should never fail to marshal the match unless we constructed it incorrectly.
137-
panic(fmt.Errorf("could not marshal http match pairs: %w", err))
138-
}
139-
matchConf := file.File{
140-
Content: b,
141-
Path: httpMatchVarsFile,
142-
Type: file.TypeRegular,
143-
}
141+
})
144142

145-
return []file.File{httpConf, matchConf}
143+
return files
146144
}
147145

148146
func (g GeneratorImpl) getExecuteFuncs() []executeFunc {
149147
return []executeFunc{
148+
executeServers,
150149
g.executeUpstreams,
151150
executeSplitClients,
152151
executeMaps,

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

+7-5
Original file line numberDiff line numberDiff line change
@@ -78,19 +78,21 @@ func TestGenerate(t *testing.T) {
7878
Content: []byte("test-cert\ntest-key"),
7979
}))
8080

81+
g.Expect(files[1].Path).To(Equal("/etc/nginx/conf.d/match.json"))
8182
g.Expect(files[1].Type).To(Equal(file.TypeRegular))
82-
g.Expect(files[1].Path).To(Equal("/etc/nginx/conf.d/http.conf"))
83-
httpCfg := string(files[1].Content) // converting to string so that on failure gomega prints strings not byte arrays
83+
expString := "{}"
84+
g.Expect(string(files[1].Content)).To(Equal(expString))
85+
86+
g.Expect(files[2].Type).To(Equal(file.TypeRegular))
87+
g.Expect(files[2].Path).To(Equal("/etc/nginx/conf.d/http.conf"))
88+
httpCfg := string(files[2].Content) // converting to string so that on failure gomega prints strings not byte arrays
8489
// Note: this only verifies that Generate() returns a byte array with upstream, server, and split_client blocks.
8590
// It does not test the correctness of those blocks. That functionality is covered by other tests in this package.
8691
g.Expect(httpCfg).To(ContainSubstring("listen 80"))
8792
g.Expect(httpCfg).To(ContainSubstring("listen 443"))
8893
g.Expect(httpCfg).To(ContainSubstring("upstream"))
8994
g.Expect(httpCfg).To(ContainSubstring("split_clients"))
9095

91-
g.Expect(files[2].Path).To(Equal("/etc/nginx/conf.d/match.json"))
92-
g.Expect(files[2].Type).To(Equal(file.TypeRegular))
93-
9496
g.Expect(files[3].Type).To(Equal(file.TypeRegular))
9597
g.Expect(files[3].Path).To(Equal("/etc/nginx/conf.d/config-version.conf"))
9698
configVersion := string(files[3].Content)

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

-18
Original file line numberDiff line numberDiff line change
@@ -93,21 +93,3 @@ type ProxySSLVerify struct {
9393
TrustedCertificate string
9494
Name string
9595
}
96-
97-
// httpMatch is an internal representation of an HTTPRouteMatch.
98-
// This struct is marshaled into a string and stored as a variable in the nginx location block for the route's path.
99-
// The NJS httpmatches module will look up this variable on the request object and compare the request against the
100-
// Method, Headers, and QueryParams contained in httpMatch.
101-
// If the request satisfies the httpMatch, NGINX will redirect the request to the location RedirectPath.
102-
type RouteMatch struct {
103-
// Method is the HTTPMethod of the HTTPRouteMatch.
104-
Method string `json:"method,omitempty"`
105-
// RedirectPath is the path to redirect the request to if the request satisfies the match conditions.
106-
RedirectPath string `json:"redirectPath,omitempty"`
107-
// Headers is a list of HTTPHeaders name value pairs with the format "{name}:{value}".
108-
Headers []string `json:"headers,omitempty"`
109-
// QueryParams is a list of HTTPQueryParams name value pairs with the format "{name}={value}".
110-
QueryParams []string `json:"params,omitempty"`
111-
// Any represents a match with no match conditions.
112-
Any bool `json:"any,omitempty"`
113-
}

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

+6-2
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,13 @@ import (
1010

1111
var mapsTemplate = gotemplate.Must(gotemplate.New("maps").Parse(mapsTemplateText))
1212

13-
func executeMaps(conf dataplane.Configuration) []byte {
13+
func executeMaps(conf dataplane.Configuration) []executeResult {
1414
maps := buildAddHeaderMaps(append(conf.HTTPServers, conf.SSLServers...))
15-
return execute(mapsTemplate, maps)
15+
result := executeResult{
16+
dest: httpConfigFile,
17+
data: execute(mapsTemplate, maps),
18+
}
19+
return []executeResult{result}
1620
}
1721

1822
func buildAddHeaderMaps(servers []dataplane.VirtualServer) []http.Map {

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

+2-1
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,8 @@ func TestExecuteMaps(t *testing.T) {
8888
"map $http_upgrade $connection_upgrade {": 1,
8989
}
9090

91-
maps := string(executeMaps(conf))
91+
mapResult := executeMaps(conf)
92+
maps := string(mapResult[0].data)
9293
for expSubStr, expCount := range expSubStrings {
9394
g.Expect(expCount).To(Equal(strings.Count(maps, expSubStr)))
9495
}

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

+70-49
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
package config
22

33
import (
4+
"encoding/json"
45
"fmt"
5-
"regexp"
6+
"maps"
67
"strconv"
78
"strings"
89
gotemplate "text/template"
@@ -39,52 +40,57 @@ var baseHeaders = []http.Header{
3940
},
4041
}
4142

42-
func executeServers(conf dataplane.Configuration) ([]http.Server, map[string][]http.RouteMatch) {
43+
func executeServers(conf dataplane.Configuration) []executeResult {
4344
servers, httpMatchPairs := createServers(conf.HTTPServers, conf.SSLServers)
4445

45-
return servers, httpMatchPairs
46+
serverResult := executeResult{
47+
dest: httpConfigFile,
48+
data: execute(serversTemplate, servers),
49+
}
50+
51+
// create httpMatchPair conf
52+
httpMatchConf, err := json.Marshal(httpMatchPairs)
53+
if err != nil {
54+
// panic is safe here because we should never fail to marshal the match unless we constructed it incorrectly.
55+
panic(fmt.Errorf("could not marshal http match pairs: %w", err))
56+
}
57+
58+
httpMatchResult := executeResult{
59+
dest: httpMatchVarsFile,
60+
data: httpMatchConf,
61+
}
62+
63+
return []executeResult{serverResult, httpMatchResult}
4664
}
4765

48-
func createServers(httpServers, sslServers []dataplane.VirtualServer) (
49-
[]http.Server,
50-
map[string][]http.RouteMatch,
51-
) {
66+
func createServers(httpServers, sslServers []dataplane.VirtualServer) ([]http.Server, httpMatchPairs) {
5267
servers := make([]http.Server, 0, len(httpServers)+len(sslServers))
53-
finalMatchPairs := make(map[string][]http.RouteMatch)
68+
finalMatchPairs := make(httpMatchPairs)
5469

55-
for _, s := range httpServers {
56-
httpServer, matchPair := createServer(s)
70+
for serverID, s := range httpServers {
71+
httpServer, matchPair := createServer(s, serverID)
5772
servers = append(servers, httpServer)
58-
59-
for key, val := range matchPair {
60-
finalMatchPairs[key] = val
61-
}
73+
maps.Copy(finalMatchPairs, matchPair)
6274
}
6375

64-
for _, s := range sslServers {
65-
sslServer, matchPair := createSSLServer(s)
76+
for serverID, s := range sslServers {
77+
sslServer, matchPair := createSSLServer(s, serverID)
6678
servers = append(servers, sslServer)
67-
68-
for key, val := range matchPair {
69-
finalMatchPairs[key] = val
70-
}
79+
maps.Copy(finalMatchPairs, matchPair)
7180
}
7281

7382
return servers, finalMatchPairs
7483
}
7584

76-
func createSSLServer(virtualServer dataplane.VirtualServer) (
77-
http.Server,
78-
map[string][]http.RouteMatch,
79-
) {
85+
func createSSLServer(virtualServer dataplane.VirtualServer, serverID int) (http.Server, httpMatchPairs) {
8086
if virtualServer.IsDefault {
8187
return http.Server{
8288
IsDefaultSSL: true,
8389
Port: virtualServer.Port,
8490
}, nil
8591
}
8692

87-
locs, matchPairs := createLocations(virtualServer)
93+
locs, matchPairs := createLocations(&virtualServer, serverID)
8894

8995
return http.Server{
9096
ServerName: virtualServer.Hostname,
@@ -97,18 +103,15 @@ func createSSLServer(virtualServer dataplane.VirtualServer) (
97103
}, matchPairs
98104
}
99105

100-
func createServer(virtualServer dataplane.VirtualServer) (
101-
http.Server,
102-
map[string][]http.RouteMatch,
103-
) {
106+
func createServer(virtualServer dataplane.VirtualServer, serverID int) (http.Server, httpMatchPairs) {
104107
if virtualServer.IsDefault {
105108
return http.Server{
106109
IsDefaultHTTP: true,
107110
Port: virtualServer.Port,
108111
}, nil
109112
}
110113

111-
locs, matchPairs := createLocations(virtualServer)
114+
locs, matchPairs := createLocations(&virtualServer, serverID)
112115

113116
return http.Server{
114117
ServerName: virtualServer.Hostname,
@@ -124,19 +127,16 @@ type rewriteConfig struct {
124127
Rewrite string
125128
}
126129

127-
type httpMatchPairs map[string][]http.RouteMatch
130+
type httpMatchPairs map[string][]RouteMatch
128131

129-
func createLocations(server dataplane.VirtualServer) (
130-
[]http.Location,
131-
map[string][]http.RouteMatch,
132-
) {
132+
func createLocations(server *dataplane.VirtualServer, serverID int) ([]http.Location, httpMatchPairs) {
133133
maxLocs, pathsAndTypes := getMaxLocationCountAndPathMap(server.PathRules)
134134
locs := make([]http.Location, 0, maxLocs)
135135
matchPairs := make(httpMatchPairs)
136136
var rootPathExists bool
137137

138138
for pathRuleIdx, rule := range server.PathRules {
139-
matches := make([]http.RouteMatch, 0, len(rule.MatchRules))
139+
matches := make([]RouteMatch, 0, len(rule.MatchRules))
140140

141141
if rule.Path == rootPath {
142142
rootPathExists = true
@@ -157,9 +157,19 @@ func createLocations(server dataplane.VirtualServer) (
157157
}
158158

159159
if len(matches) > 0 {
160-
for i := range extLocations {
161-
key := server.Hostname + extLocations[i].Path + strconv.Itoa(int(server.Port))
162-
extLocations[i].HTTPMatchKey = sanitizeKey(key)
160+
for i, loc := range extLocations {
161+
// FIXME(sberman): De-dupe matches and associated locations
162+
// so we don't need nginx/njs to perform unnecessary matching.
163+
// https://github.com/nginxinc/nginx-gateway-fabric/issues/662
164+
var key string
165+
if server.SSL != nil {
166+
key += "SSL"
167+
}
168+
key += strconv.Itoa(serverID) + "_" + strconv.Itoa(pathRuleIdx)
169+
if strings.Contains(loc.Path, "= /") {
170+
key += "EXACT"
171+
}
172+
extLocations[i].HTTPMatchKey = key
163173
matchPairs[extLocations[i].HTTPMatchKey] = matches
164174
}
165175
locs = append(locs, extLocations...)
@@ -173,13 +183,6 @@ func createLocations(server dataplane.VirtualServer) (
173183
return locs, matchPairs
174184
}
175185

176-
// removeSpecialCharacters removes '/', '.' from key and replaces '= ' with 'EXACT',
177-
// to avoid compilation issues with NJS and NGINX Conf.
178-
func sanitizeKey(input string) string {
179-
s := regexp.MustCompile("[./]").ReplaceAllString(input, "")
180-
return regexp.MustCompile("= ").ReplaceAllString(s, "EXACT")
181-
}
182-
183186
// pathAndTypeMap contains a map of paths and any path types defined for that path
184187
// for example, {/foo: {exact: {}, prefix: {}}}
185188
type pathAndTypeMap map[string]map[dataplane.PathType]struct{}
@@ -256,7 +259,7 @@ func initializeInternalLocation(
256259
pathruleIdx,
257260
matchRuleIdx int,
258261
match dataplane.Match,
259-
) (http.Location, http.RouteMatch) {
262+
) (http.Location, RouteMatch) {
260263
path := fmt.Sprintf("@rule%d-route%d", pathruleIdx, matchRuleIdx)
261264
return createMatchLocation(path), createRouteMatch(match, path)
262265
}
@@ -431,8 +434,26 @@ func createRewritesValForRewriteFilter(filter *dataplane.HTTPURLRewriteFilter, p
431434
return rewrites
432435
}
433436

434-
func createRouteMatch(match dataplane.Match, redirectPath string) http.RouteMatch {
435-
hm := http.RouteMatch{
437+
// httpMatch is an internal representation of an HTTPRouteMatch.
438+
// This struct is marshaled into a string and stored as a variable in the nginx location block for the route's path.
439+
// The NJS httpmatches module will look up this variable on the request object and compare the request against the
440+
// Method, Headers, and QueryParams contained in httpMatch.
441+
// If the request satisfies the httpMatch, NGINX will redirect the request to the location RedirectPath.
442+
type RouteMatch struct {
443+
// Method is the HTTPMethod of the HTTPRouteMatch.
444+
Method string `json:"method,omitempty"`
445+
// RedirectPath is the path to redirect the request to if the request satisfies the match conditions.
446+
RedirectPath string `json:"redirectPath,omitempty"`
447+
// Headers is a list of HTTPHeaders name value pairs with the format "{name}:{value}".
448+
Headers []string `json:"headers,omitempty"`
449+
// QueryParams is a list of HTTPQueryParams name value pairs with the format "{name}={value}".
450+
QueryParams []string `json:"params,omitempty"`
451+
// Any represents a match with no match conditions.
452+
Any bool `json:"any,omitempty"`
453+
}
454+
455+
func createRouteMatch(match dataplane.Match, redirectPath string) RouteMatch {
456+
hm := RouteMatch{
436457
RedirectPath: redirectPath,
437458
}
438459

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package config
22

33
var serversTemplateText = `
4+
js_preload_object matches from /etc/nginx/conf.d/match.json;
45
{{- range $s := . -}}
56
{{ if $s.IsDefaultSSL -}}
67
server {
@@ -17,7 +18,6 @@ server {
1718
}
1819
{{- else }}
1920
server {
20-
js_preload_object matches from /etc/nginx/conf.d/match.json;
2121
{{- if $s.SSL }}
2222
listen {{ $s.Port }} ssl;
2323
ssl_certificate {{ $s.SSL.Certificate }};

0 commit comments

Comments
 (0)