Add support for path-regex annotation in Ingress Master-Minion (#4200)

nginx · Aug 11, 2023 · cace410 · cace410
1 parent 514b275
commit cace410
Show file tree

Hide file tree

Showing 6 changed files with 1,340 additions and 522 deletions.
diff --git a/internal/configs/version1/nginx-plus.ingress.tmpl b/internal/configs/version1/nginx-plus.ingress.tmpl
@@ -177,7 +177,7 @@ server {
 	{{end -}}
 
 	{{range $location := $server.Locations}}
-	location {{  makeLocationPath $location.Path $.Ingress.Annotations | printf }} {
+	location {{  makeLocationPath $location $.Ingress.Annotations | printf }} {
 		set $service "{{$location.ServiceName}}";
 		status_zone "{{ $location.ServiceName }}";
 		{{with $location.MinionIngress}}

diff --git a/internal/configs/version1/nginx.ingress.tmpl b/internal/configs/version1/nginx.ingress.tmpl
@@ -102,7 +102,7 @@ server {
 	{{- end}}
 
 	{{range $location := $server.Locations}}
-	location {{  makeLocationPath $location.Path $.Ingress.Annotations | printf }} {
+	location {{  makeLocationPath $location $.Ingress.Annotations | printf }} {
 		set $service "{{$location.ServiceName}}";
 		{{with $location.MinionIngress}}
 		# location for minion {{$location.MinionIngress.Namespace}}/{{$location.MinionIngress.Name}}

diff --git a/internal/configs/version1/template_helper.go b/internal/configs/version1/template_helper.go
@@ -14,18 +14,42 @@ func trim(s string) string {
 	return strings.TrimSpace(s)
 }
 
-// makeLocationPath takes a string representing a location path
-// and a map representing Ingress annotations.
+// makeLocationPath takes location and Ingress annotations and returns
+// modified location path with added regex modifier or the original path
+// if no path-regex annotation is present in ingressAnnotations
+// or in Location's Ingress.
+//
+// Annotation 'path-regex' set on a Minion Ingress will take priority over
+// the annotation set on the master (in Master-Minion Ingress setup).
+// If no annotation 'path-regex' is set on Minion and only on Ingress (including master),
+// all location paths will be updated using provided regex modifier types.
+func makeLocationPath(loc *Location, ingressAnnotations map[string]string) string {
+	if loc.MinionIngress != nil {
+		// Case when annotation 'path-regex' set on Location's Minion.
+		_, isMinion := loc.MinionIngress.Annotations["nginx.org/mergeable-ingress-type"]
+		regexType, hasRegex := loc.MinionIngress.Annotations["nginx.org/path-regex"]
+
+		if isMinion && hasRegex {
+			return makePathWithRegex(loc.Path, regexType)
+		}
+	}
+
+	// Case when annotation 'path-regex' set on Ingress (including Master).
+	regexType, ok := ingressAnnotations["nginx.org/path-regex"]
+	if !ok {
+		return loc.Path
+	}
+	return makePathWithRegex(loc.Path, regexType)
+}
+
+// makePathWithRegex takes a path representing a location and a regexType
+// (one of `case_sensitive`, `case_insensitive` or `exact`).
 // It returns a location path with added regular expression modifier.
 // See [Location Directive].
 //
 // [Location Directive]: https://nginx.org/en/docs/http/ngx_http_core_module.html#location
-func makeLocationPath(path string, annotations map[string]string) string {
-	p, ok := annotations["nginx.org/path-regex"]
-	if !ok {
-		return path
-	}
-	switch p {
+func makePathWithRegex(path, regexType string) string {
+	switch regexType {
 	case "case_sensitive":
 		return fmt.Sprintf("~ \"^%s\"", path)
 	case "case_insensitive":

diff --git a/internal/configs/version1/template_helper_test.go b/internal/configs/version1/template_helper_test.go
@@ -6,110 +6,273 @@ import (
 	"text/template"
 )
 
-func TestWithPathRegex_MatchesCaseSensitiveModifier(t *testing.T) {
+func TestMakeLocationPath_WithRegexCaseSensitiveModifier(t *testing.T) {
 	t.Parallel()
 
 	want := "~ \"^/coffee/[A-Z0-9]{3}\""
-	got := makeLocationPath("/coffee/[A-Z0-9]{3}", map[string]string{"nginx.org/path-regex": "case_sensitive"})
+	got := makeLocationPath(
+		&Location{Path: "/coffee/[A-Z0-9]{3}"},
+		map[string]string{"nginx.org/path-regex": "case_sensitive"},
+	)
 	if got != want {
 		t.Errorf("got: %s, want: %s", got, want)
 	}
 }
 
-func TestWithPathRegex_MatchesCaseInsensitiveModifier(t *testing.T) {
+func TestMakeLocationPath_WithRegexCaseInsensitiveModifier(t *testing.T) {
 	t.Parallel()
 
 	want := "~* \"^/coffee/[A-Z0-9]{3}\""
-	got := makeLocationPath("/coffee/[A-Z0-9]{3}", map[string]string{"nginx.org/path-regex": "case_insensitive"})
+	got := makeLocationPath(
+		&Location{Path: "/coffee/[A-Z0-9]{3}"},
+		map[string]string{"nginx.org/path-regex": "case_insensitive"},
+	)
 	if got != want {
 		t.Errorf("got: %s, want: %s", got, want)
 	}
 }
 
-func TestWithPathReqex_MatchesExactModifier(t *testing.T) {
+func TestMakeLocationPath_WithRegexExactModifier(t *testing.T) {
 	t.Parallel()
 
 	want := "= \"/coffee\""
-	got := makeLocationPath("/coffee", map[string]string{"nginx.org/path-regex": "exact"})
+	got := makeLocationPath(
+		&Location{Path: "/coffee"},
+		map[string]string{"nginx.org/path-regex": "exact"},
+	)
 	if got != want {
 		t.Errorf("got: %s, want: %s", got, want)
 	}
 }
 
-func TestWithPathReqex_DoesNotMatchModifier(t *testing.T) {
+func TestMakeLocationPath_WithBogusRegexModifier(t *testing.T) {
 	t.Parallel()
 
 	want := "/coffee"
-	got := makeLocationPath("/coffee", map[string]string{"nginx.org/path-regex": "bogus"})
+	got := makeLocationPath(
+		&Location{Path: "/coffee"},
+		map[string]string{"nginx.org/path-regex": "bogus"},
+	)
 	if got != want {
 		t.Errorf("got: %s, want: %s", got, want)
 	}
 }
 
-func TestWithPathReqex_DoesNotMatchEmptyModifier(t *testing.T) {
+func TestMakeLocationPath_WithEmptyRegexModifier(t *testing.T) {
 	t.Parallel()
 
 	want := "/coffee"
-	got := makeLocationPath("/coffee", map[string]string{"nginx.org/path-regex": ""})
+	got := makeLocationPath(
+		&Location{Path: "/coffee"},
+		map[string]string{"nginx.org/path-regex": ""},
+	)
 	if got != want {
 		t.Errorf("got: %s, want: %s", got, want)
 	}
 }
 
-func TestWithPathReqex_DoesNotMatchBogusAnnotationName(t *testing.T) {
+func TestMakeLocationPath_WithBogusAnnotationName(t *testing.T) {
 	t.Parallel()
 
 	want := "/coffee"
-	got := makeLocationPath("/coffee", map[string]string{"nginx.org/bogus-annotation": ""})
+	got := makeLocationPath(
+		&Location{Path: "/coffee"},
+		map[string]string{"nginx.org/bogus-annotation": ""},
+	)
 	if got != want {
 		t.Errorf("got: %s, want: %s", got, want)
 	}
 }
 
-func TestSplitHelperFunction(t *testing.T) {
+func TestMakeLocationPath_ForIngressWithoutPathRegex(t *testing.T) {
 	t.Parallel()
-	const tpl = `{{range $n := split . ","}}{{$n}} {{end}}`
 
-	tmpl, err := template.New("testTemplate").Funcs(helperFunctions).Parse(tpl)
-	if err != nil {
-		t.Fatalf("Failed to parse template: %v", err)
+	want := "/coffee"
+	got := makeLocationPath(
+		&Location{Path: "/coffee"},
+		map[string]string{},
+	)
+	if got != want {
+		t.Errorf("got %q, want %q", got, want)
+	}
+}
+
+func TestMakeLocationPath_ForIngressWithPathRegexCaseSensitive(t *testing.T) {
+	t.Parallel()
+
+	want := "~ \"^/coffee\""
+	got := makeLocationPath(
+		&Location{Path: "/coffee"},
+		map[string]string{
+			"nginx.org/path-regex": "case_sensitive",
+		},
+	)
+	if got != want {
+		t.Errorf("got %q, want %q", got, want)
+	}
+}
+
+func TestMakeLocationPath_ForIngressWithPathRegexSetOnMinion(t *testing.T) {
+	t.Parallel()
+
+	want := "~ \"^/coffee\""
+	got := makeLocationPath(
+		&Location{
+			Path: "/coffee",
+			MinionIngress: &Ingress{
+				Name:      "cafe-ingress-coffee-minion",
+				Namespace: "default",
+				Annotations: map[string]string{
+					"nginx.org/mergeable-ingress-type": "minion",
+					"nginx.org/path-regex":             "case_sensitive",
+				},
+			},
+		},
+		map[string]string{
+			"nginx.org/mergeable-ingress-type": "master",
+		},
+	)
+
+	if got != want {
+		t.Errorf("got %q, want %q", got, want)
+	}
+}
+
+func TestMakeLocationPath_ForIngressWithPathRegexSetOnMaster(t *testing.T) {
+	t.Parallel()
+
+	want := "~ \"^/coffee\""
+	got := makeLocationPath(
+		&Location{
+			Path: "/coffee",
+			MinionIngress: &Ingress{
+				Name:      "cafe-ingress-coffee-minion",
+				Namespace: "default",
+				Annotations: map[string]string{
+					"nginx.org/mergeable-ingress-type": "minion",
+				},
+			},
+		},
+		map[string]string{
+			"nginx.org/mergeable-ingress-type": "master",
+			"nginx.org/path-regex":             "case_sensitive",
+		},
+	)
+
+	if got != want {
+		t.Errorf("got %q, want %q", got, want)
+	}
+}
+
+func TestMakeLocationPath_SetOnMinionTakesPrecedenceOverMaster(t *testing.T) {
+	t.Parallel()
+
+	want := "= \"/coffee\""
+	got := makeLocationPath(
+		&Location{
+			Path: "/coffee",
+			MinionIngress: &Ingress{
+				Name:      "cafe-ingress-coffee-minion",
+				Namespace: "default",
+				Annotations: map[string]string{
+					"nginx.org/mergeable-ingress-type": "minion",
+					"nginx.org/path-regex":             "exact",
+				},
+			},
+		},
+		map[string]string{
+			"nginx.org/mergeable-ingress-type": "master",
+			"nginx.org/path-regex":             "case_sensitive",
+		},
+	)
+
+	if got != want {
+		t.Errorf("got %q, want %q", got, want)
 	}
+}
+
+func TestMakeLocationPath_PathRegexSetOnMaster(t *testing.T) {
+	t.Parallel()
+
+	want := "= \"/coffee\""
+	got := makeLocationPath(
+		&Location{
+			Path: "/coffee",
+			MinionIngress: &Ingress{
+				Name:      "cafe-ingress-coffee-minion",
+				Namespace: "default",
+				Annotations: map[string]string{
+					"nginx.org/mergeable-ingress-type": "minion",
+				},
+			},
+		},
+		map[string]string{
+			"nginx.org/mergeable-ingress-type": "master",
+			"nginx.org/path-regex":             "exact",
+		},
+	)
 
+	if got != want {
+		t.Errorf("got %q, want %q", got, want)
+	}
+}
+
+func TestSplitInputString(t *testing.T) {
+	t.Parallel()
+
+	tmpl := newSplitTemplate(t)
 	var buf bytes.Buffer
 
 	input := "foo,bar"
 	expected := "foo bar "
 
-	err = tmpl.Execute(&buf, input)
+	err := tmpl.Execute(&buf, input)
 	if err != nil {
 		t.Fatalf("Failed to execute the template %v", err)
 	}
-
 	if buf.String() != expected {
-		t.Fatalf("Template generated wrong config, got %v but expected %v.", buf.String(), expected)
+		t.Errorf("Template generated wrong config, got %v but expected %v.", buf.String(), expected)
 	}
 }
 
-func TestTrimHelperFunction(t *testing.T) {
+func TestTrimWhiteSpaceFromInputString(t *testing.T) {
 	t.Parallel()
-	const tpl = `{{trim .}}`
 
-	tmpl, err := template.New("testTemplate").Funcs(helperFunctions).Parse(tpl)
-	if err != nil {
-		t.Fatalf("Failed to parse template: %v", err)
+	tmpl := newTrimTemplate(t)
+	inputs := []string{
+		"  foobar     ",
+		"foobar   ",
+		"   foobar",
+		"foobar",
 	}
-
-	var buf bytes.Buffer
-
-	input := "  foobar     "
 	expected := "foobar"
 
-	err = tmpl.Execute(&buf, input)
+	for _, i := range inputs {
+		var buf bytes.Buffer
+		err := tmpl.Execute(&buf, i)
+		if err != nil {
+			t.Fatalf("Failed to execute the template %v", err)
+		}
+		if buf.String() != expected {
+			t.Errorf("Template generated wrong config, got %v but expected %v.", buf.String(), expected)
+		}
+	}
+}
+
+func newSplitTemplate(t *testing.T) *template.Template {
+	t.Helper()
+	tmpl, err := template.New("testTemplate").Funcs(helperFunctions).Parse(`{{range $n := split . ","}}{{$n}} {{end}}`)
 	if err != nil {
-		t.Fatalf("Failed to execute the template %v", err)
+		t.Fatalf("Failed to parse template: %v", err)
 	}
+	return tmpl
+}
 
-	if buf.String() != expected {
-		t.Fatalf("Template generated wrong config, got %v but expected %v.", buf.String(), expected)
+func newTrimTemplate(t *testing.T) *template.Template {
+	t.Helper()
+	tmpl, err := template.New("testTemplate").Funcs(helperFunctions).Parse(`{{trim .}}`)
+	if err != nil {
+		t.Fatalf("Failed to parse template: %v", err)
 	}
+	return tmpl
 }