Skip to content

Commit e7a3213

Browse files
sjbermansjberman
committed
Simplify code
1 parent e9f9058 commit e7a3213

File tree

2 files changed

+112
-134
lines changed

2 files changed

+112
-134
lines changed

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

Lines changed: 24 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,8 @@ func createLocations(pathRules []dataplane.PathRule, listenerPort int32) []http.
9292
m := r.GetMatch()
9393

9494
buildLocations := extLocations
95-
intLocation, match, intLocationExists := initializeInternalLocation(rule, matchRuleIdx, m)
96-
if intLocationExists {
95+
if len(rule.MatchRules) != 1 || !isPathOnlyMatch(m) {
96+
intLocation, match := initializeInternalLocation(rule, matchRuleIdx, m)
9797
buildLocations = []http.Location{intLocation}
9898
matches = append(matches, match)
9999
}
@@ -138,17 +138,9 @@ func createLocations(pathRules []dataplane.PathRule, listenerPort int32) []http.
138138
}
139139

140140
if len(matches) > 0 {
141-
// FIXME(sberman): De-dupe matches and associated locations
142-
// so we don't need nginx/njs to perform unnecessary matching.
143-
// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/662
144-
b, err := json.Marshal(matches)
145-
if err != nil {
146-
// panic is safe here because we should never fail to marshal the match unless we constructed it incorrectly.
147-
panic(fmt.Errorf("could not marshal http match: %w", err))
148-
}
149-
141+
matchesStr := convertMatchesToString(matches)
150142
for i := range extLocations {
151-
extLocations[i].HTTPMatchVar = string(b)
143+
extLocations[i].HTTPMatchVar = matchesStr
152144
}
153145
locs = append(locs, extLocations...)
154146
}
@@ -198,7 +190,9 @@ func initializeExternalLocations(
198190
// that handles the Exact prefix case (if it doesn't already exist), and the first location is updated
199191
// to handle the trailing slash prefix case (if it doesn't already exist)
200192
if isNonSlashedPrefixPath(rule.PathType, externalLocPath) {
201-
// if Exact path and trailing slash Prefix path already exist, then we don't need to build anything
193+
// if Exact path and/or trailing slash Prefix path already exists, this means some routing rule
194+
// configures it. The routing rule location has priority over this location, so we don't try to
195+
// overwrite it and we don't add a duplicate location to NGINX because that will cause an NGINX config error.
202196
_, exactPathExists := pathsAndTypes[rule.Path][dataplane.PathTypeExact]
203197
var trailingSlashPrefixPathExists bool
204198
if pathTypes, exists := pathsAndTypes[rule.Path+"/"]; exists {
@@ -235,17 +229,9 @@ func initializeInternalLocation(
235229
rule dataplane.PathRule,
236230
matchRuleIdx int,
237231
match v1beta1.HTTPRouteMatch,
238-
) (http.Location, httpMatch, bool) {
239-
var intLocation http.Location
240-
var hm httpMatch
241-
intLocationNeeded := len(rule.MatchRules) != 1 || !isPathOnlyMatch(match)
242-
if intLocationNeeded {
243-
path := createPathForMatch(rule.Path, rule.PathType, matchRuleIdx)
244-
hm = createHTTPMatch(match, path)
245-
intLocation = createMatchLocation(path)
246-
}
247-
248-
return intLocation, hm, intLocationNeeded
232+
) (http.Location, httpMatch) {
233+
path := createPathForMatch(rule.Path, rule.PathType, matchRuleIdx)
234+
return createMatchLocation(path), createHTTPMatch(match, path)
249235
}
250236

251237
func createReturnValForRedirectFilter(filter *v1beta1.HTTPRequestRedirectFilter, listenerPort int32) *http.Return {
@@ -438,6 +424,19 @@ func convertSetHeaders(headers []dataplane.HTTPHeader) []http.Header {
438424
return locHeaders
439425
}
440426

427+
func convertMatchesToString(matches []httpMatch) string {
428+
// FIXME(sberman): De-dupe matches and associated locations
429+
// so we don't need nginx/njs to perform unnecessary matching.
430+
// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/662
431+
b, err := json.Marshal(matches)
432+
if err != nil {
433+
// panic is safe here because we should never fail to marshal the match unless we constructed it incorrectly.
434+
panic(fmt.Errorf("could not marshal http match: %w", err))
435+
}
436+
437+
return string(b)
438+
}
439+
441440
func exactPath(path string) string {
442441
return fmt.Sprintf("= %s", path)
443442
}
@@ -463,7 +462,7 @@ func createDefaultRootLocation() http.Location {
463462
}
464463
}
465464

466-
// returns whether or not a path is of type Prefix and does not contain a trailing slash
465+
// isNonSlashedPrefixPath returns whether or not a path is of type Prefix and does not contain a trailing slash
467466
func isNonSlashedPrefixPath(pathType dataplane.PathType, path string) bool {
468467
return pathType == dataplane.PathTypePrefix && !strings.HasSuffix(path, "/")
469468
}

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

Lines changed: 88 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -892,6 +892,41 @@ func TestCreateServersConflicts(t *testing.T) {
892892
return hr
893893
}
894894

895+
hr1 := createHR([]pathAndType{
896+
{
897+
path: "/coffee",
898+
pathType: v1beta1.PathMatchPathPrefix,
899+
},
900+
{
901+
path: "/coffee",
902+
pathType: v1beta1.PathMatchExact,
903+
},
904+
})
905+
hr2 := createHR([]pathAndType{
906+
{
907+
path: "/coffee",
908+
pathType: v1beta1.PathMatchPathPrefix,
909+
},
910+
{
911+
path: "/coffee/",
912+
pathType: v1beta1.PathMatchPathPrefix,
913+
},
914+
})
915+
hr3 := createHR([]pathAndType{
916+
{
917+
path: "/coffee",
918+
pathType: v1beta1.PathMatchPathPrefix,
919+
},
920+
{
921+
path: "/coffee/",
922+
pathType: v1beta1.PathMatchPathPrefix,
923+
},
924+
{
925+
path: "/coffee",
926+
pathType: v1beta1.PathMatchExact,
927+
},
928+
})
929+
895930
fooGroup := dataplane.BackendGroup{
896931
Source: types.NamespacedName{Namespace: "test", Name: "route"},
897932
RuleIdx: 0,
@@ -939,18 +974,9 @@ func TestCreateServersConflicts(t *testing.T) {
939974
PathType: dataplane.PathTypePrefix,
940975
MatchRules: []dataplane.MatchRule{
941976
{
942-
MatchIdx: 0,
943-
RuleIdx: 0,
944-
Source: createHR([]pathAndType{
945-
{
946-
path: "/coffee",
947-
pathType: v1beta1.PathMatchPathPrefix,
948-
},
949-
{
950-
path: "/coffee",
951-
pathType: v1beta1.PathMatchExact,
952-
},
953-
}),
977+
MatchIdx: 0,
978+
RuleIdx: 0,
979+
Source: hr1,
954980
BackendGroup: fooGroup,
955981
},
956982
},
@@ -960,18 +986,9 @@ func TestCreateServersConflicts(t *testing.T) {
960986
PathType: dataplane.PathTypeExact,
961987
MatchRules: []dataplane.MatchRule{
962988
{
963-
MatchIdx: 0,
964-
RuleIdx: 0,
965-
Source: createHR([]pathAndType{
966-
{
967-
path: "/coffee",
968-
pathType: v1beta1.PathMatchPathPrefix,
969-
},
970-
{
971-
path: "/coffee",
972-
pathType: v1beta1.PathMatchExact,
973-
},
974-
}),
989+
MatchIdx: 0,
990+
RuleIdx: 0,
991+
Source: hr1,
975992
BackendGroup: barGroup,
976993
},
977994
},
@@ -997,18 +1014,9 @@ func TestCreateServersConflicts(t *testing.T) {
9971014
PathType: dataplane.PathTypePrefix,
9981015
MatchRules: []dataplane.MatchRule{
9991016
{
1000-
MatchIdx: 0,
1001-
RuleIdx: 0,
1002-
Source: createHR([]pathAndType{
1003-
{
1004-
path: "/coffee",
1005-
pathType: v1beta1.PathMatchPathPrefix,
1006-
},
1007-
{
1008-
path: "/coffee/",
1009-
pathType: v1beta1.PathMatchPathPrefix,
1010-
},
1011-
}),
1017+
MatchIdx: 0,
1018+
RuleIdx: 0,
1019+
Source: hr2,
10121020
BackendGroup: fooGroup,
10131021
},
10141022
},
@@ -1018,18 +1026,9 @@ func TestCreateServersConflicts(t *testing.T) {
10181026
PathType: dataplane.PathTypePrefix,
10191027
MatchRules: []dataplane.MatchRule{
10201028
{
1021-
MatchIdx: 0,
1022-
RuleIdx: 1,
1023-
Source: createHR([]pathAndType{
1024-
{
1025-
path: "/coffee",
1026-
pathType: v1beta1.PathMatchPathPrefix,
1027-
},
1028-
{
1029-
path: "/coffee/",
1030-
pathType: v1beta1.PathMatchPathPrefix,
1031-
},
1032-
}),
1029+
MatchIdx: 0,
1030+
RuleIdx: 1,
1031+
Source: hr2,
10331032
BackendGroup: barGroup,
10341033
},
10351034
},
@@ -1055,22 +1054,9 @@ func TestCreateServersConflicts(t *testing.T) {
10551054
PathType: dataplane.PathTypePrefix,
10561055
MatchRules: []dataplane.MatchRule{
10571056
{
1058-
MatchIdx: 0,
1059-
RuleIdx: 0,
1060-
Source: createHR([]pathAndType{
1061-
{
1062-
path: "/coffee",
1063-
pathType: v1beta1.PathMatchPathPrefix,
1064-
},
1065-
{
1066-
path: "/coffee/",
1067-
pathType: v1beta1.PathMatchPathPrefix,
1068-
},
1069-
{
1070-
path: "/coffee",
1071-
pathType: v1beta1.PathMatchExact,
1072-
},
1073-
}),
1057+
MatchIdx: 0,
1058+
RuleIdx: 0,
1059+
Source: hr3,
10741060
BackendGroup: fooGroup,
10751061
},
10761062
},
@@ -1080,22 +1066,9 @@ func TestCreateServersConflicts(t *testing.T) {
10801066
PathType: dataplane.PathTypePrefix,
10811067
MatchRules: []dataplane.MatchRule{
10821068
{
1083-
MatchIdx: 0,
1084-
RuleIdx: 1,
1085-
Source: createHR([]pathAndType{
1086-
{
1087-
path: "/coffee",
1088-
pathType: v1beta1.PathMatchPathPrefix,
1089-
},
1090-
{
1091-
path: "/coffee/",
1092-
pathType: v1beta1.PathMatchPathPrefix,
1093-
},
1094-
{
1095-
path: "/coffee",
1096-
pathType: v1beta1.PathMatchExact,
1097-
},
1098-
}),
1069+
MatchIdx: 0,
1070+
RuleIdx: 1,
1071+
Source: hr3,
10991072
BackendGroup: barGroup,
11001073
},
11011074
},
@@ -1762,40 +1735,46 @@ func TestIsPathOnlyMatch(t *testing.T) {
17621735
func TestCreateProxyPass(t *testing.T) {
17631736
g := NewGomegaWithT(t)
17641737

1765-
expected := "http://10.0.0.1:80"
1766-
1767-
grp := dataplane.BackendGroup{
1768-
Backends: []dataplane.Backend{
1769-
{
1770-
UpstreamName: "10.0.0.1:80",
1771-
Valid: true,
1772-
Weight: 1,
1738+
tests := []struct {
1739+
expected string
1740+
grp dataplane.BackendGroup
1741+
}{
1742+
{
1743+
expected: "http://10.0.0.1:80",
1744+
grp: dataplane.BackendGroup{
1745+
Backends: []dataplane.Backend{
1746+
{
1747+
UpstreamName: "10.0.0.1:80",
1748+
Valid: true,
1749+
Weight: 1,
1750+
},
1751+
},
17731752
},
17741753
},
1775-
}
1776-
1777-
result := createProxyPass(grp)
1778-
g.Expect(result).To(Equal(expected))
1779-
1780-
expected = "http://$ns1__bg_rule0"
1781-
1782-
grp = dataplane.BackendGroup{
1783-
Source: types.NamespacedName{Namespace: "ns1", Name: "bg"},
1784-
Backends: []dataplane.Backend{
1785-
{
1786-
UpstreamName: "my-variable",
1787-
Valid: true,
1788-
Weight: 1,
1789-
},
1790-
{
1791-
UpstreamName: "my-variable2",
1792-
Valid: true,
1793-
Weight: 1,
1754+
{
1755+
expected: "http://$ns1__bg_rule0",
1756+
grp: dataplane.BackendGroup{
1757+
Source: types.NamespacedName{Namespace: "ns1", Name: "bg"},
1758+
Backends: []dataplane.Backend{
1759+
{
1760+
UpstreamName: "my-variable",
1761+
Valid: true,
1762+
Weight: 1,
1763+
},
1764+
{
1765+
UpstreamName: "my-variable2",
1766+
Valid: true,
1767+
Weight: 1,
1768+
},
1769+
},
17941770
},
17951771
},
17961772
}
1797-
result = createProxyPass(grp)
1798-
g.Expect(result).To(Equal(expected))
1773+
1774+
for _, tc := range tests {
1775+
result := createProxyPass(tc.grp)
1776+
g.Expect(result).To(Equal(tc.expected))
1777+
}
17991778
}
18001779

18011780
func TestCreateMatchLocation(t *testing.T) {

0 commit comments

Comments
 (0)