Merge pull request #2340 from Shopify/sticky-upstream-name

Fix buildupstream name to work with dynamic session affinity

internal/ingress/controller/template/template.go

@@ -629,8 +629,7 @@ func buildDenyVariable(a interface{}) string {
 	return fmt.Sprintf("$deny_%v", denyPathSlugMap[l])
 }
 
-// TODO: Needs Unit Tests
+func buildUpstreamName(host string, b interface{}, loc interface{}, dynamicConfigurationEnabled bool) string {
-func buildUpstreamName(host string, b interface{}, loc interface{}) string {
 
 	backends, ok := b.([]*ingress.Backend)
 	if !ok {
@@ -646,14 +645,16 @@ func buildUpstreamName(host string, b interface{}, loc interface{}) string {
 
 	upstreamName := location.Backend
 
-	for _, backend := range backends {
+	if !dynamicConfigurationEnabled {
-		if backend.Name == location.Backend {
+		for _, backend := range backends {
-			if backend.SessionAffinity.AffinityType == "cookie" &&
+			if backend.Name == location.Backend {
-				isSticky(host, location, backend.SessionAffinity.CookieSessionAffinity.Locations) {
+				if backend.SessionAffinity.AffinityType == "cookie" &&
-				upstreamName = fmt.Sprintf("sticky-%v", upstreamName)
+					isSticky(host, location, backend.SessionAffinity.CookieSessionAffinity.Locations) {
-			}
+					upstreamName = fmt.Sprintf("sticky-%v", upstreamName)
+				}
 
-			break
+				break
+			}
 		}
 	}
 

internal/ingress/controller/template/template_test.go

@@ -709,3 +709,46 @@ func TestIsLocationInLocationList(t *testing.T) {
 		}
 	}
 }
+
+func TestBuildUpstreamName(t *testing.T) {
+	defaultBackend := "upstream-name"
+	defaultHost := "example.com"
+
+	for k, tc := range tmplFuncTestcases {
+		loc := &ingress.Location{
+			Path:             tc.Path,
+			Rewrite:          rewrite.Config{Target: tc.Target, AddBaseURL: tc.AddBaseURL, BaseURLScheme: tc.BaseURLScheme},
+			Backend:          defaultBackend,
+			XForwardedPrefix: tc.XForwardedPrefix,
+		}
+
+		backend := &ingress.Backend{
+			Name:   defaultBackend,
+			Secure: tc.SecureBackend,
+		}
+
+		expected := defaultBackend
+
+		if tc.Sticky {
+			if !tc.DynamicConfigurationEnabled {
+				expected = fmt.Sprintf("sticky-" + expected)
+			}
+
+			backend.SessionAffinity = ingress.SessionAffinityConfig{
+				AffinityType: "cookie",
+				CookieSessionAffinity: ingress.CookieSessionAffinity{
+					Locations: map[string][]string{
+						defaultHost: {tc.Path},
+					},
+				},
+			}
+		}
+
+		backends := []*ingress.Backend{backend}
+
+		pp := buildUpstreamName(defaultHost, backends, loc, tc.DynamicConfigurationEnabled)
+		if !strings.EqualFold(expected, pp) {
+			t.Errorf("%s: expected \n'%v'\nbut returned \n'%v'", k, expected, pp)
+		}
+	}
+}

rootfs/etc/nginx/template/nginx.tmpl

@@ -772,7 +772,7 @@ stream {
             # ngx_auth_request module overrides variables in the parent request,
             # therefore we have to explicitly set this variable again so that when the parent request
             # resumes it has the correct value set for this variable so that Lua can pick backend correctly
-            set $proxy_upstream_name "{{ buildUpstreamName $server.Hostname $all.Backends $location }}";
+            set $proxy_upstream_name "{{ buildUpstreamName $server.Hostname $all.Backends $location $all.DynamicConfigurationEnabled }}";
 
             proxy_pass_request_body     off;
             proxy_set_header            Content-Length "";
@@ -886,7 +886,7 @@ stream {
 
             {{ if $all.Cfg.EnableVtsStatus }}{{ if $location.VtsFilterKey }} vhost_traffic_status_filter_by_set_key {{ $location.VtsFilterKey }};{{ end }}{{ end }}
 
-            set $proxy_upstream_name "{{ buildUpstreamName $server.Hostname $all.Backends $location }}";
+            set $proxy_upstream_name "{{ buildUpstreamName $server.Hostname $all.Backends $location $all.DynamicConfigurationEnabled }}";
 
             {{ $ing := (getIngressInformation $location.Ingress $location.Path) }}
             {{/* $ing.Metadata contains the Ingress metadata */}}