Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix reload errors for NGINX when large num of match conditions are used #1799

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion internal/mode/static/nginx/config/http/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ type Location struct {
ProxySSLVerify *ProxySSLVerify
Path string
ProxyPass string
HTTPMatchVar string
HTTPMatchVar []string
Rewrites []string
ProxySetHeaders []Header
}
Expand Down
45 changes: 39 additions & 6 deletions internal/mode/static/nginx/config/servers.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package config

import (
"bytes"
"encoding/json"
"fmt"
"strings"
Expand All @@ -16,6 +17,7 @@ const (
// HeaderMatchSeparator is the separator for constructing header-based match for NJS.
HeaderMatchSeparator = ":"
rootPath = "/"
matchMaxLength = 2000 // TODO: account for character escaping when mapped to config
)

// baseHeaders contains the constant headers set in each server block
Expand Down Expand Up @@ -558,17 +560,48 @@ func convertSetHeaders(headers []dataplane.HTTPHeader) []http.Header {
return locHeaders
}

func convertMatchesToString(matches []httpMatch) string {
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))

var matchList []string

var buffer bytes.Buffer
for _, match := range matches {
matchBytes, err := json.Marshal(match)
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))
}
matchLen := len(matchBytes)
if buffer.Len()+matchLen+4 > matchMaxLength {
// Flush buffer if adding current match exceeds the maximum length
buffer.WriteString("]")
matchList = append(matchList, buffer.String())
buffer.Reset()
buffer.WriteString("[")
} else {
if buffer.Len() > 1 {
// Add comma if not first match in the buffer
buffer.WriteString(",")
} else {
// Add opening braces if the first match in the buffer
buffer.WriteString("[")
}
}
buffer.Write(matchBytes)
}

if buffer.Len() > 0 {
buffer.WriteString("]")
matchList = append(matchList, buffer.String())
}

return string(b)
for _, m := range matchList {
fmt.Println("match --> ", m)
}
return matchList
}

func exactPath(path string) string {
Expand Down
6 changes: 3 additions & 3 deletions internal/mode/static/nginx/config/servers_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ server {
return {{ $l.Return.Code }} "{{ $l.Return.Body }}";
{{- end }}

{{- if $l.HTTPMatchVar }}
set $http_matches {{ $l.HTTPMatchVar | printf "%q" }};
js_content httpmatches.redirect;
{{ range $i, $h := $l.HTTPMatchVar }}
set $http_matches_{{ $i }} {{ $h | printf "%q" }};
{{- end }}
js_content httpmatches.redirect;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This directive shouldn't exist if there are no HTTPMatchVars


{{- if $l.ProxyPass -}}
{{ range $h := $l.ProxySetHeaders }}
Expand Down
5 changes: 3 additions & 2 deletions internal/mode/static/nginx/config/servers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -508,11 +508,12 @@ func TestCreateServers(t *testing.T) {
},
}

expectedMatchString := func(m []httpMatch) string {
expectedMatchString := func(m []httpMatch) []string {
g := NewWithT(t)
b, err := json.Marshal(m)
g.Expect(err).ToNot(HaveOccurred())
return string(b)
str := string(b)
return []string{str}
}

slashMatches := []httpMatch{
Expand Down
28 changes: 16 additions & 12 deletions internal/mode/static/nginx/modules/src/httpmatches.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,22 +50,26 @@ function redirect(r) {
}

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 index = 0;
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}`,
);
while (true) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this loop just overwriting the matches var every time? So when you look for a match, you're only looking at the last iteration.

const MATCHES_VARIABLE = `http_matches_${index}`;
if (!r.variables[MATCHES_VARIABLE]) {
break;
}

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}`,
);
}
}

console.log('matches in js', matches);

if (!Array.isArray(matches)) {
throw Error(`cannot redirect the request; expected a list of matches, got ${matches}`);
}
Expand Down
Loading