From 72b81a73e1d0b0d429acf92e4375c69901c3cb53 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Mon, 14 Aug 2023 18:32:48 +0100 Subject: [PATCH] Path-Regex annotation on master and minions are set independently --- internal/configs/version1/template_helper.go | 7 +- .../configs/version1/template_helper_test.go | 25 ++++- internal/configs/version1/template_test.go | 99 ++++++++++++++++++- 3 files changed, 122 insertions(+), 9 deletions(-) diff --git a/internal/configs/version1/template_helper.go b/internal/configs/version1/template_helper.go index dbb7125a92..9f33064aae 100644 --- a/internal/configs/version1/template_helper.go +++ b/internal/configs/version1/template_helper.go @@ -26,12 +26,15 @@ func trim(s string) string { 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"] + ingressType, isMergeable := loc.MinionIngress.Annotations["nginx.org/mergeable-ingress-type"] regexType, hasRegex := loc.MinionIngress.Annotations["nginx.org/path-regex"] - if isMinion && hasRegex { + if isMergeable && ingressType == "minion" && hasRegex { return makePathWithRegex(loc.Path, regexType) } + if isMergeable && ingressType == "minion" && !hasRegex { + return loc.Path + } } // Case when annotation 'path-regex' set on Ingress (including Master). diff --git a/internal/configs/version1/template_helper_test.go b/internal/configs/version1/template_helper_test.go index 3df1887401..7e0ee01226 100644 --- a/internal/configs/version1/template_helper_test.go +++ b/internal/configs/version1/template_helper_test.go @@ -148,9 +148,6 @@ func TestMakeLocationPath_ForIngressWithPathRegexSetOnMaster(t *testing.T) { MinionIngress: &Ingress{ Name: "cafe-ingress-coffee-minion", Namespace: "default", - Annotations: map[string]string{ - "nginx.org/mergeable-ingress-type": "minion", - }, }, }, map[string]string{ @@ -191,10 +188,10 @@ func TestMakeLocationPath_SetOnMinionTakesPrecedenceOverMaster(t *testing.T) { } } -func TestMakeLocationPath_PathRegexSetOnMaster(t *testing.T) { +func TestMakeLocationPath_PathRegexSetOnMasterDoesNotModifyMinionWithoutPathRegexAnnotation(t *testing.T) { t.Parallel() - want := "= \"/coffee\"" + want := "/coffee" got := makeLocationPath( &Location{ Path: "/coffee", @@ -217,6 +214,24 @@ func TestMakeLocationPath_PathRegexSetOnMaster(t *testing.T) { } } +func TestMakeLocationPath_ForIngress(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 TestSplitInputString(t *testing.T) { t.Parallel() diff --git a/internal/configs/version1/template_test.go b/internal/configs/version1/template_test.go index 67b942903c..3f77862145 100644 --- a/internal/configs/version1/template_test.go +++ b/internal/configs/version1/template_test.go @@ -152,6 +152,27 @@ func TestExecuteTemplate_ForMergeableIngressForNGINXPlus(t *testing.T) { } } +func TestExecuteTemplate_ForMergeableIngressForNGINXPlusWithMasterPathRegex(t *testing.T) { + t.Parallel() + + tmpl := newNGINXPlusIngressTmpl(t) + buf := &bytes.Buffer{} + + err := tmpl.Execute(buf, ingressCfgMasterMinionNGINXPlusMasterMinions) + t.Log(buf.String()) + if err != nil { + t.Fatal(err) + } + want := "location /coffee {" + if !strings.Contains(buf.String(), want) { + t.Errorf("want %q in generated config", want) + } + want = "location /tea {" + if !strings.Contains(buf.String(), want) { + t.Errorf("want %q in generated config", want) + } +} + func TestExecuteTemplate_ForMergeableIngressWithOneMinionWithPathRegexAnnotation(t *testing.T) { t.Parallel() @@ -210,11 +231,11 @@ func TestExecuteTemplate_ForMergeableIngressForNGINXPlusWithPathRegexAnnotationO t.Fatal(err) } - want := "location ~ \"^/coffee\" {" + want := "location /coffee {" if !strings.Contains(buf.String(), want) { t.Errorf("want %q in generated config", want) } - want = "location ~ \"^/tea\" {" + want = "location /tea {" if !strings.Contains(buf.String(), want) { t.Errorf("want %q in generated config", want) } @@ -915,6 +936,80 @@ var ( }, } + // ingressCfgMasterMinionNGINXPlusMasterMinions holds data to test the following scenario: + // + // Ingress Master - Minion + // - Master: with `path-regex` annotation + // - Minion 1 (cafe-ingress-coffee-minion): without `path-regex` annotation + // - Minion 2 (cafe-ingress-tea-minion): without `path-regex` annotation + ingressCfgMasterMinionNGINXPlusMasterMinions = IngressNginxConfig{ + Upstreams: []Upstream{ + coffeeUpstreamNginxPlus, + teaUpstreamNGINXPlus, + }, + Servers: []Server{ + { + Name: "cafe.example.com", + ServerTokens: "on", + Locations: []Location{ + { + Path: "/coffee", + ServiceName: "coffee-svc", + Upstream: coffeeUpstreamNginxPlus, + ProxyConnectTimeout: "60s", + ProxyReadTimeout: "60s", + ProxySendTimeout: "60s", + ClientMaxBodySize: "1m", + ProxyBuffering: true, + MinionIngress: &Ingress{ + Name: "cafe-ingress-coffee-minion", + Namespace: "default", + Annotations: map[string]string{ + "nginx.org/mergeable-ingress-type": "minion", + }, + }, + ProxySSLName: "coffee-svc.default.svc", + }, + { + Path: "/tea", + ServiceName: "tea-svc", + Upstream: teaUpstreamNGINXPlus, + ProxyConnectTimeout: "60s", + ProxyReadTimeout: "60s", + ProxySendTimeout: "60s", + ClientMaxBodySize: "1m", + ProxyBuffering: true, + MinionIngress: &Ingress{ + Name: "cafe-ingress-tea-minion", + Namespace: "default", + Annotations: map[string]string{ + "nginx.org/mergeable-ingress-type": "minion", + }, + }, + ProxySSLName: "tea-svc.default.svc", + }, + }, + SSL: true, + SSLCertificate: "/etc/nginx/secrets/default-cafe-secret", + SSLCertificateKey: "/etc/nginx/secrets/default-cafe-secret", + StatusZone: "cafe.example.com", + HSTSMaxAge: 2592000, + Ports: []int{80}, + SSLPorts: []int{443}, + SSLRedirect: true, + HealthChecks: make(map[string]HealthCheck), + }, + }, + Ingress: Ingress{ + Name: "cafe-ingress-master", + Namespace: "default", + Annotations: map[string]string{ + "nginx.org/mergeable-ingress-type": "master", + "nginx.org/path-regex": "case_sensitive", + }, + }, + } + // ingressCfgMasterMinionNGINXPlusMinionWithPathRegexAnnotation holds data to test the following scenario: // // Ingress Master - Minion