Merge pull request #1515 from mynameiswhm/master

Allow usage of non_idempotent option in proxy_next_upstream
kubernetes · Oct 18, 2017 · 99a355f · 99a355f
2 parents fb79164 + 3d07b97
commit 99a355f
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 11 deletions.
diff --git a/docs/user-guide/annotations.md b/docs/user-guide/annotations.md
@@ -28,6 +28,7 @@ The following annotations are supported:
 |[ingress.kubernetes.io/proxy-connect-timeout](#custom-timeouts)|number|
 |[ingress.kubernetes.io/proxy-send-timeout](#custom-timeouts)|number|
 |[ingress.kubernetes.io/proxy-read-timeout](#custom-timeouts)|number|
+|[ingress.kubernetes.io/proxy-next-upstream](#custom-timeouts)|string|
 |[ingress.kubernetes.io/proxy-request-buffering](#custom-timeouts)|string|
 |[ingress.kubernetes.io/rewrite-target](#rewrite)|URI|
 |[ingress.kubernetes.io/secure-backends](#secure-backends)|true or false|
@@ -313,6 +314,7 @@ In some scenarios is required to have different values. To allow this we provide
 - `ingress.kubernetes.io/proxy-connect-timeout`
 - `ingress.kubernetes.io/proxy-send-timeout`
 - `ingress.kubernetes.io/proxy-read-timeout`
+- `ingress.kubernetes.io/proxy-next-upstream`
 - `ingress.kubernetes.io/proxy-request-buffering`
 
 ### Custom max body size

diff --git a/pkg/nginx/template/template.go b/pkg/nginx/template/template.go
@@ -44,6 +44,7 @@ import (
 
 const (
 	slash         = "/"
+	nonIdempotent = "non_idempotent"
 	defBufferSize = 65535
 )
 
@@ -548,20 +549,30 @@ func isSticky(host string, loc *ingress.Location, stickyLocations map[string][]s
 	return false
 }
 
-func buildNextUpstream(input interface{}) string {
-	nextUpstream, ok := input.(string)
+func buildNextUpstream(i, r interface{}) string {
+	nextUpstream, ok := i.(string)
 	if !ok {
-		glog.Errorf("expected a 'string' type but %T was returned", input)
+		glog.Errorf("expected a 'string' type but %T was returned", i)
 		return ""
 	}
 
+	retryNonIdempotent := r.(bool)
+
 	parts := strings.Split(nextUpstream, " ")
 
 	nextUpstreamCodes := make([]string, 0, len(parts))
 	for _, v := range parts {
-		if v != "" && v != "non_idempotent" {
+		if v != "" && v != nonIdempotent {
 			nextUpstreamCodes = append(nextUpstreamCodes, v)
 		}
+
+		if v == nonIdempotent {
+			retryNonIdempotent = true
+		}
+	}
+
+	if retryNonIdempotent {
+		nextUpstreamCodes = append(nextUpstreamCodes, nonIdempotent)
 	}
 
 	return strings.Join(nextUpstreamCodes, " ")

diff --git a/pkg/nginx/template/template_test.go b/pkg/nginx/template/template_test.go
@@ -311,13 +311,40 @@ func TestBuildResolvers(t *testing.T) {
 }
 
 func TestBuildNextUpstream(t *testing.T) {
-	nextUpstream := "timeout http_500 http_502 non_idempotent"
-	validNextUpstream := "timeout http_500 http_502"
-
-	buildNextUpstream := buildNextUpstream(nextUpstream)
+	cases := map[string]struct {
+		NextUpstream  string
+		NonIdempotent bool
+		Output        string
+	}{
+		"default": {
+			"timeout http_500 http_502",
+			false,
+			"timeout http_500 http_502",
+		},
+		"global": {
+			"timeout http_500 http_502",
+			true,
+			"timeout http_500 http_502 non_idempotent",
+		},
+		"local": {
+			"timeout http_500 http_502 non_idempotent",
+			false,
+			"timeout http_500 http_502 non_idempotent",
+		},
+	}
 
-	if buildNextUpstream != validNextUpstream {
-		t.Errorf("Expected '%v' but returned '%v'", validNextUpstream, buildNextUpstream)
+	for k, tc := range cases {
+		nextUpstream := buildNextUpstream(tc.NextUpstream, tc.NonIdempotent)
+		if nextUpstream != tc.Output {
+			t.Errorf(
+				"%s: called buildNextUpstream('%s', %v); expected '%v' but returned '%v'",
+				k,
+				tc.NextUpstream,
+				tc.NonIdempotent,
+				tc.Output,
+				nextUpstream,
+			)
+		}
 	}
 }
 

diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl
@@ -788,7 +788,7 @@ stream {
             proxy_cookie_path                       {{ $location.Proxy.CookiePath }};
 
             # In case of errors try the next upstream server before returning an error
-            proxy_next_upstream                     {{ buildNextUpstream $location.Proxy.NextUpstream }}{{ if $all.Cfg.RetryNonIdempotent }} non_idempotent{{ end }};
+            proxy_next_upstream                     {{ buildNextUpstream $location.Proxy.NextUpstream $all.Cfg.RetryNonIdempotent }};
 
             {{/* rewrite only works if the content is not compressed */}}
             {{ if $location.Rewrite.AddBaseURL }}