From 90f19e0954fa62a5c2730176758aba4e3b13a124 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Wed, 17 Jul 2019 12:17:14 +0100 Subject: [PATCH 01/15] add support for retires --- docs/virtualserver-and-virtualserverroute.md | 12 ++++-- .../cafe-virtual-server.yaml | 6 +++ internal/configs/version2/config.go | 27 ++++++------ .../version2/nginx-plus.virtualserver.tmpl | 3 ++ .../configs/version2/nginx.virtualserver.tmpl | 3 ++ internal/configs/virtualserver.go | 42 ++++++++++++------- pkg/apis/configuration/v1alpha1/types.go | 25 ++++++----- 7 files changed, 78 insertions(+), 40 deletions(-) diff --git a/docs/virtualserver-and-virtualserverroute.md b/docs/virtualserver-and-virtualserverroute.md index 846b09a769..adfbddccb4 100644 --- a/docs/virtualserver-and-virtualserverroute.md +++ b/docs/virtualserver-and-virtualserverroute.md @@ -184,6 +184,9 @@ keepalive: 32 connect-timeout: 30s read-timeout: 30s send-timeout: 30s +next-upstream: "error timeout non_idempotent" +next-upstream-timeout: 5s +next-upstreeam-tries: 10 tls: enable: True ``` @@ -197,9 +200,12 @@ tls: | `fail-timeout` | The time during which the specified number of unsuccessful attempts to communicate with an upstream server should happen to consider the server unavailable. See the [fail_timeout](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#fail_timeout) parameter of the server directive. The default is set in the `fail-timeout` ConfigMap key. | `string` | No | | `max-fails` | The number of unsuccessful attempts to communicate with an upstream server that should happen in the duration set by the `fail-timeout` to consider the server unavailable. See the [max_fails](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#max_fails) parameter of the server directive. The default is set in the `max-fails` ConfgMap key. | `int` | No | | `keepalive` | Configures the cache for connections to upstream servers. The value `0` disables the cache. See the [keepalive](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive) directive. The default is set in the `keepalive` ConfigMap key. | `int` | No -`connect-timeout` | The timeout for establishing a connection with an upstream server. See the [proxy_connect_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_connect_timeout) directive. The default is specified in the `proxy-connect-timeout` ConfigMap key. | `string` | No -`read-timeout` | The timeout for reading a response from an upstream server. See the [proxy_read_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout) directive. The default is specified in the `proxy-read-timeout` ConfigMap key. | `string` | No -`send-timeout` | The timeout for transmitting a request to an upstream server. See the [proxy_send_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_send_timeout) directive. The default is specified in the `proxy-send-timeout` ConfigMap key. | `string` | No +| `connect-timeout` | The timeout for establishing a connection with an upstream server. See the [proxy_connect_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_connect_timeout) directive. The default is specified in the `proxy-connect-timeout` ConfigMap key. | `string` | No +| `read-timeout` | The timeout for reading a response from an upstream server. See the [proxy_read_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout) directive. The default is specified in the `proxy-read-timeout` ConfigMap key. | `string` | No +| `send-timeout` | The timeout for transmitting a request to an upstream server. See the [proxy_send_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_send_timeout) directive. The default is specified in the `proxy-send-timeout` ConfigMap key. | `string` | No +| `next-upstream` | Specifies in which cases a request should be passed to the next upstream server. See the proxy_next_upstream directive. The default is `error timeout` | `string` | No +| `next-upstream-timeout` | The time during which a request can be passed to the next upstream server. See the proxy_next_upstream_timeout directive. The `0` value turns off the time limit. The default is `0`. | `string` | No +| `next-upstream-tries` | The number of possible tries for passing a request to the next upstream server. See the proxy_next_upstream_tries directive. The `0` value turns off this limit. The default is `0`. | `int` | No | `tls` | The TLS configuration for the Upstream. | [`tls`](#UpstreamTLS) | No | ### Upstream.TLS diff --git a/examples-of-custom-resources/basic-configuration/cafe-virtual-server.yaml b/examples-of-custom-resources/basic-configuration/cafe-virtual-server.yaml index 192d3123a5..2c25671a30 100644 --- a/examples-of-custom-resources/basic-configuration/cafe-virtual-server.yaml +++ b/examples-of-custom-resources/basic-configuration/cafe-virtual-server.yaml @@ -10,9 +10,15 @@ spec: - name: tea service: tea-svc port: 80 + next-upstream: "error timeout HTTP_404 non_idempotent" + next-upstream-timeout: 5s + next-upstream-tries: 10 - name: coffee service: coffee-svc port: 80 + next-upstream: "error timeout HTTP_404 non_idempotent" + next-upstream-timeout: 5s + next-upstream-tries: 10 routes: - path: /tea upstream: tea diff --git a/internal/configs/version2/config.go b/internal/configs/version2/config.go index e0c1f324db..ad4c7aebb1 100644 --- a/internal/configs/version2/config.go +++ b/internal/configs/version2/config.go @@ -49,18 +49,21 @@ type SSL struct { // Location defines a location. type Location struct { - Path string - Snippets []string - ProxyConnectTimeout string - ProxyReadTimeout string - ProxySendTimeout string - ClientMaxBodySize string - ProxyMaxTempFileSize string - ProxyBuffering bool - ProxyBuffers string - ProxyBufferSize string - ProxyPass string - HasKeepalive bool + Path string + Snippets []string + ProxyConnectTimeout string + ProxyReadTimeout string + ProxySendTimeout string + ClientMaxBodySize string + ProxyMaxTempFileSize string + ProxyBuffering bool + ProxyBuffers string + ProxyBufferSize string + ProxyPass string + ProxyNextUpstream string + ProxyNextUpstreamTimeout string + ProxyNextUpstreamTries int + HasKeepalive bool } // SplitClient defines a split_clients. diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 9cb108e25f..fcff628645 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -119,6 +119,9 @@ server { proxy_set_header X-Forwarded-Proto $scheme; proxy_pass {{ $l.ProxyPass }}; + proxy_next_upstream {{ $l.ProxyNextUpstream }}; + proxy_next_upstream_timeout {{ $l.ProxyNextUpstreamTimeout }}; + proxy_next_upstream_tries {{ $l.ProxyNextUpstreamTries }}; } {{ end }} } \ No newline at end of file diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index 9cb108e25f..fcff628645 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -119,6 +119,9 @@ server { proxy_set_header X-Forwarded-Proto $scheme; proxy_pass {{ $l.ProxyPass }}; + proxy_next_upstream {{ $l.ProxyNextUpstream }}; + proxy_next_upstream_timeout {{ $l.ProxyNextUpstreamTimeout }}; + proxy_next_upstream_tries {{ $l.ProxyNextUpstreamTries }}; } {{ end }} } \ No newline at end of file diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 0aa2c77da3..7160458195 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -265,20 +265,34 @@ func generateProxyPassProtocol(upstream conf_v1alpha1.Upstream) string { } func generateLocation(path string, upstreamName string, upstream conf_v1alpha1.Upstream, cfgParams *ConfigParams) version2.Location { - return version2.Location{ - Path: path, - Snippets: cfgParams.LocationSnippets, - ProxyConnectTimeout: generateTime(upstream.ProxyConnectTimeout, cfgParams.ProxyConnectTimeout), - ProxyReadTimeout: generateTime(upstream.ProxyReadTimeout, cfgParams.ProxyReadTimeout), - ProxySendTimeout: generateTime(upstream.ProxySendTimeout, cfgParams.ProxySendTimeout), - ClientMaxBodySize: cfgParams.ClientMaxBodySize, - ProxyMaxTempFileSize: cfgParams.ProxyMaxTempFileSize, - ProxyBuffering: cfgParams.ProxyBuffering, - ProxyBuffers: cfgParams.ProxyBuffers, - ProxyBufferSize: cfgParams.ProxyBufferSize, - ProxyPass: fmt.Sprintf("%v://%v", generateProxyPassProtocol(upstream), upstreamName), - HasKeepalive: upstreamHasKeepalive(upstream, cfgParams), - } + loc := version2.Location{ + Path: path, + Snippets: cfgParams.LocationSnippets, + ProxyConnectTimeout: generateTime(upstream.ProxyConnectTimeout, cfgParams.ProxyConnectTimeout), + ProxyReadTimeout: generateTime(upstream.ProxyReadTimeout, cfgParams.ProxyReadTimeout), + ProxySendTimeout: generateTime(upstream.ProxySendTimeout, cfgParams.ProxySendTimeout), + ClientMaxBodySize: cfgParams.ClientMaxBodySize, + ProxyMaxTempFileSize: cfgParams.ProxyMaxTempFileSize, + ProxyBuffering: cfgParams.ProxyBuffering, + ProxyBuffers: cfgParams.ProxyBuffers, + ProxyBufferSize: cfgParams.ProxyBufferSize, + ProxyPass: fmt.Sprintf("%v://%v", generateProxyPassProtocol(upstream), upstreamName), + ProxyNextUpstream: "", + ProxyNextUpstreamTimeout: "", + ProxyNextUpstreamTries: 0, + HasKeepalive: upstreamHasKeepalive(upstream, cfgParams), + } + + if upstream.ProxyNextUpstream != "" { + loc.ProxyNextUpstream = upstream.ProxyNextUpstream + } + if upstream.ProxyNextUpstreamTimeout != "" { + loc.ProxyNextUpstreamTimeout = upstream.ProxyNextUpstreamTimeout + } + if upstream.ProxyNextUpstreamTries != 0 { + loc.ProxyNextUpstreamTries = upstream.ProxyNextUpstreamTries + } + return loc } type splitRouteCfg struct { diff --git a/pkg/apis/configuration/v1alpha1/types.go b/pkg/apis/configuration/v1alpha1/types.go index a4a60279eb..d1237e8c2e 100644 --- a/pkg/apis/configuration/v1alpha1/types.go +++ b/pkg/apis/configuration/v1alpha1/types.go @@ -25,17 +25,20 @@ type VirtualServerSpec struct { // Upstream defines an upstream. type Upstream struct { - Name string `json:"name"` - Service string `json:"service"` - Port uint16 `json:"port"` - LBMethod string `json:"lb-method"` - FailTimeout string `json:"fail-timeout"` - MaxFails *int `json:"max-fails"` - Keepalive *int `json:"keepalive"` - ProxyConnectTimeout string `json:"connect-timeout"` - ProxyReadTimeout string `json:"read-timeout"` - ProxySendTimeout string `json:"send-timeout"` - TLS UpstreamTLS `json:"tls"` + Name string `json:"name"` + Service string `json:"service"` + Port uint16 `json:"port"` + LBMethod string `json:"lb-method"` + FailTimeout string `json:"fail-timeout"` + MaxFails *int `json:"max-fails"` + Keepalive *int `json:"keepalive"` + ProxyConnectTimeout string `json:"connect-timeout"` + ProxyReadTimeout string `json:"read-timeout"` + ProxySendTimeout string `json:"send-timeout"` + ProxyNextUpstream string `json:"next-upstream"` + ProxyNextUpstreamTimeout string `json:"next-upstream-timeout"` + ProxyNextUpstreamTries int `json:"next-upstream-tries"` + TLS UpstreamTLS `json:"tls"` } // UpstreamTLS defines a TLS configuration for an Upstream. From 56d68275d1718ff9f11ed18017e7652afcaab876 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Thu, 18 Jul 2019 14:37:15 +0100 Subject: [PATCH 02/15] add retries support for vs/vsr --- internal/configs/virtualserver.go | 26 ++++++++++-------------- pkg/apis/configuration/v1alpha1/types.go | 2 +- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 7160458195..8002564656 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -264,8 +264,15 @@ func generateProxyPassProtocol(upstream conf_v1alpha1.Upstream) string { return "http" } +func generateStringOrDefault(s string, defaultS string) string { + if s == "" { + return defaultS + } + return s +} + func generateLocation(path string, upstreamName string, upstream conf_v1alpha1.Upstream, cfgParams *ConfigParams) version2.Location { - loc := version2.Location{ + return version2.Location{ Path: path, Snippets: cfgParams.LocationSnippets, ProxyConnectTimeout: generateTime(upstream.ProxyConnectTimeout, cfgParams.ProxyConnectTimeout), @@ -277,22 +284,11 @@ func generateLocation(path string, upstreamName string, upstream conf_v1alpha1.U ProxyBuffers: cfgParams.ProxyBuffers, ProxyBufferSize: cfgParams.ProxyBufferSize, ProxyPass: fmt.Sprintf("%v://%v", generateProxyPassProtocol(upstream), upstreamName), - ProxyNextUpstream: "", - ProxyNextUpstreamTimeout: "", - ProxyNextUpstreamTries: 0, + ProxyNextUpstream: generateStringOrDefault(upstream.ProxyNextUpstream, "error timeout"), + ProxyNextUpstreamTimeout: generateStringOrDefault(upstream.ProxyNextUpstreamTimeout, "0s"), + ProxyNextUpstreamTries: generateIntFromPointer(upstream.ProxyNextUpstreamTries, 0), HasKeepalive: upstreamHasKeepalive(upstream, cfgParams), } - - if upstream.ProxyNextUpstream != "" { - loc.ProxyNextUpstream = upstream.ProxyNextUpstream - } - if upstream.ProxyNextUpstreamTimeout != "" { - loc.ProxyNextUpstreamTimeout = upstream.ProxyNextUpstreamTimeout - } - if upstream.ProxyNextUpstreamTries != 0 { - loc.ProxyNextUpstreamTries = upstream.ProxyNextUpstreamTries - } - return loc } type splitRouteCfg struct { diff --git a/pkg/apis/configuration/v1alpha1/types.go b/pkg/apis/configuration/v1alpha1/types.go index d1237e8c2e..decdb440bb 100644 --- a/pkg/apis/configuration/v1alpha1/types.go +++ b/pkg/apis/configuration/v1alpha1/types.go @@ -37,7 +37,7 @@ type Upstream struct { ProxySendTimeout string `json:"send-timeout"` ProxyNextUpstream string `json:"next-upstream"` ProxyNextUpstreamTimeout string `json:"next-upstream-timeout"` - ProxyNextUpstreamTries int `json:"next-upstream-tries"` + ProxyNextUpstreamTries *int `json:"next-upstream-tries"` TLS UpstreamTLS `json:"tls"` } From 875d4546d8a4cfd510ff37dc4401ec276030d3da Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Mon, 22 Jul 2019 10:57:52 +0100 Subject: [PATCH 03/15] Add tests for retries --- internal/configs/virtualserver_test.go | 134 +++++++++++++++++-------- 1 file changed, 91 insertions(+), 43 deletions(-) diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index d1431957c1..a580fbaf00 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -263,14 +263,20 @@ func TestGenerateVirtualServerConfig(t *testing.T) { Snippets: []string{"# server snippet"}, Locations: []version2.Location{ { - Path: "/tea", - ProxyPass: "http://vs_default_cafe_tea", - HasKeepalive: true, + Path: "/tea", + ProxyPass: "http://vs_default_cafe_tea", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, + HasKeepalive: true, }, { - Path: "/coffee", - ProxyPass: "http://vs_default_cafe_vsr_default_coffee_coffee", - HasKeepalive: true, + Path: "/coffee", + ProxyPass: "http://vs_default_cafe_vsr_default_coffee_coffee", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, + HasKeepalive: true, }, }, }, @@ -460,20 +466,32 @@ func TestGenerateVirtualServerConfigForVirtualServerWithSplits(t *testing.T) { }, Locations: []version2.Location{ { - Path: "@splits_0_split_0", - ProxyPass: "http://vs_default_cafe_tea-v1", + Path: "@splits_0_split_0", + ProxyPass: "http://vs_default_cafe_tea-v1", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, }, { - Path: "@splits_0_split_1", - ProxyPass: "http://vs_default_cafe_tea-v2", + Path: "@splits_0_split_1", + ProxyPass: "http://vs_default_cafe_tea-v2", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, }, { - Path: "@splits_1_split_0", - ProxyPass: "http://vs_default_cafe_vsr_default_coffee_coffee-v1", + Path: "@splits_1_split_0", + ProxyPass: "http://vs_default_cafe_vsr_default_coffee_coffee-v1", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, }, { - Path: "@splits_1_split_1", - ProxyPass: "http://vs_default_cafe_vsr_default_coffee_coffee-v2", + Path: "@splits_1_split_1", + ProxyPass: "http://vs_default_cafe_vsr_default_coffee_coffee-v2", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, }, }, }, @@ -704,20 +722,32 @@ func TestGenerateVirtualServerConfigForVirtualServerWithRules(t *testing.T) { }, Locations: []version2.Location{ { - Path: "@rules_0_match_0", - ProxyPass: "http://vs_default_cafe_tea-v2", + Path: "@rules_0_match_0", + ProxyPass: "http://vs_default_cafe_tea-v2", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, }, { - Path: "@rules_0_default", - ProxyPass: "http://vs_default_cafe_tea-v1", + Path: "@rules_0_default", + ProxyPass: "http://vs_default_cafe_tea-v1", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, }, { - Path: "@rules_1_match_0", - ProxyPass: "http://vs_default_cafe_vsr_default_coffee_coffee-v2", + Path: "@rules_1_match_0", + ProxyPass: "http://vs_default_cafe_vsr_default_coffee_coffee-v2", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, }, { - Path: "@rules_1_default", - ProxyPass: "http://vs_default_cafe_vsr_default_coffee_coffee-v1", + Path: "@rules_1_default", + ProxyPass: "http://vs_default_cafe_vsr_default_coffee_coffee-v1", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, }, }, }, @@ -911,17 +941,20 @@ func TestGenerateLocation(t *testing.T) { upstreamName := "test-upstream" expected := version2.Location{ - Path: "/", - Snippets: []string{"# location snippet"}, - ProxyConnectTimeout: "30s", - ProxyReadTimeout: "31s", - ProxySendTimeout: "32s", - ClientMaxBodySize: "1m", - ProxyMaxTempFileSize: "1024m", - ProxyBuffering: true, - ProxyBuffers: "8 4k", - ProxyBufferSize: "4k", - ProxyPass: "http://test-upstream", + Path: "/", + Snippets: []string{"# location snippet"}, + ProxyConnectTimeout: "30s", + ProxyReadTimeout: "31s", + ProxySendTimeout: "32s", + ClientMaxBodySize: "1m", + ProxyMaxTempFileSize: "1024m", + ProxyBuffering: true, + ProxyBuffers: "8 4k", + ProxyBufferSize: "4k", + ProxyPass: "http://test-upstream", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, } result := generateLocation(path, upstreamName, conf_v1alpha1.Upstream{}, &cfgParams) @@ -1156,12 +1189,18 @@ func TestGenerateSplitRouteConfig(t *testing.T) { }, Locations: []version2.Location{ { - Path: "@splits_1_split_0", - ProxyPass: "http://vs_default_cafe_coffee-v1", + Path: "@splits_1_split_0", + ProxyPass: "http://vs_default_cafe_coffee-v1", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, }, { - Path: "@splits_1_split_1", - ProxyPass: "http://vs_default_cafe_coffee-v2", + Path: "@splits_1_split_1", + ProxyPass: "http://vs_default_cafe_coffee-v2", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, }, }, InternalRedirectLocation: version2.InternalRedirectLocation{ @@ -1364,16 +1403,25 @@ func TestGenerateRulesRouteConfig(t *testing.T) { }, Locations: []version2.Location{ { - Path: "@rules_1_match_0", - ProxyPass: "http://vs_default_cafe_coffee-v1", + Path: "@rules_1_match_0", + ProxyPass: "http://vs_default_cafe_coffee-v1", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, }, { - Path: "@rules_1_match_1", - ProxyPass: "http://vs_default_cafe_coffee-v2", + Path: "@rules_1_match_1", + ProxyPass: "http://vs_default_cafe_coffee-v2", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, }, { - Path: "@rules_1_default", - ProxyPass: "http://vs_default_cafe_tea", + Path: "@rules_1_default", + ProxyPass: "http://vs_default_cafe_tea", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, }, }, InternalRedirectLocation: version2.InternalRedirectLocation{ From ba13b9acec792549b31ec386c40d352bff2ca8b5 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Tue, 23 Jul 2019 12:21:33 +0100 Subject: [PATCH 04/15] Resolve PR feedback --- docs/virtualserver-and-virtualserverroute.md | 6 +++--- .../basic-configuration/cafe-virtual-server.yaml | 6 ------ internal/configs/virtualserver.go | 6 +++--- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/docs/virtualserver-and-virtualserverroute.md b/docs/virtualserver-and-virtualserverroute.md index adfbddccb4..4980d5ed41 100644 --- a/docs/virtualserver-and-virtualserverroute.md +++ b/docs/virtualserver-and-virtualserverroute.md @@ -203,9 +203,9 @@ tls: | `connect-timeout` | The timeout for establishing a connection with an upstream server. See the [proxy_connect_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_connect_timeout) directive. The default is specified in the `proxy-connect-timeout` ConfigMap key. | `string` | No | `read-timeout` | The timeout for reading a response from an upstream server. See the [proxy_read_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout) directive. The default is specified in the `proxy-read-timeout` ConfigMap key. | `string` | No | `send-timeout` | The timeout for transmitting a request to an upstream server. See the [proxy_send_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_send_timeout) directive. The default is specified in the `proxy-send-timeout` ConfigMap key. | `string` | No -| `next-upstream` | Specifies in which cases a request should be passed to the next upstream server. See the proxy_next_upstream directive. The default is `error timeout` | `string` | No -| `next-upstream-timeout` | The time during which a request can be passed to the next upstream server. See the proxy_next_upstream_timeout directive. The `0` value turns off the time limit. The default is `0`. | `string` | No -| `next-upstream-tries` | The number of possible tries for passing a request to the next upstream server. See the proxy_next_upstream_tries directive. The `0` value turns off this limit. The default is `0`. | `int` | No +| `next-upstream` | Specifies in which cases a request should be passed to the next upstream server. See the [proxy_next_upstream](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_next_upstream) directive. The default is `error timeout`. | `string` | No +| `next-upstream-timeout` | The time during which a request can be passed to the next upstream server. See the [proxy_next_upstream_timeout](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_next_upstream_timeout) directive. The `0` value turns off the time limit. The default is `0`. | `string` | No +| `next-upstream-tries` | The number of possible tries for passing a request to the next upstream server. See the [proxy_next_upstream_tries](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_next_upstream_tries) directive. The `0` value turns off this limit. The default is `0`. | `int` | No | `tls` | The TLS configuration for the Upstream. | [`tls`](#UpstreamTLS) | No | ### Upstream.TLS diff --git a/examples-of-custom-resources/basic-configuration/cafe-virtual-server.yaml b/examples-of-custom-resources/basic-configuration/cafe-virtual-server.yaml index 2c25671a30..192d3123a5 100644 --- a/examples-of-custom-resources/basic-configuration/cafe-virtual-server.yaml +++ b/examples-of-custom-resources/basic-configuration/cafe-virtual-server.yaml @@ -10,15 +10,9 @@ spec: - name: tea service: tea-svc port: 80 - next-upstream: "error timeout HTTP_404 non_idempotent" - next-upstream-timeout: 5s - next-upstream-tries: 10 - name: coffee service: coffee-svc port: 80 - next-upstream: "error timeout HTTP_404 non_idempotent" - next-upstream-timeout: 5s - next-upstream-tries: 10 routes: - path: /tea upstream: tea diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 8002564656..64e4e58afc 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -264,7 +264,7 @@ func generateProxyPassProtocol(upstream conf_v1alpha1.Upstream) string { return "http" } -func generateStringOrDefault(s string, defaultS string) string { +func generateString(s string, defaultS string) string { if s == "" { return defaultS } @@ -284,8 +284,8 @@ func generateLocation(path string, upstreamName string, upstream conf_v1alpha1.U ProxyBuffers: cfgParams.ProxyBuffers, ProxyBufferSize: cfgParams.ProxyBufferSize, ProxyPass: fmt.Sprintf("%v://%v", generateProxyPassProtocol(upstream), upstreamName), - ProxyNextUpstream: generateStringOrDefault(upstream.ProxyNextUpstream, "error timeout"), - ProxyNextUpstreamTimeout: generateStringOrDefault(upstream.ProxyNextUpstreamTimeout, "0s"), + ProxyNextUpstream: generateString(upstream.ProxyNextUpstream, "error timeout"), + ProxyNextUpstreamTimeout: generateString(upstream.ProxyNextUpstreamTimeout, "0s"), ProxyNextUpstreamTries: generateIntFromPointer(upstream.ProxyNextUpstreamTries, 0), HasKeepalive: upstreamHasKeepalive(upstream, cfgParams), } From a749e727df4ed74ef693e586d006f9112b367844 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Tue, 23 Jul 2019 12:22:40 +0100 Subject: [PATCH 05/15] Regen the automatic code --- pkg/apis/configuration/v1alpha1/zz_generated.deepcopy.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/apis/configuration/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/configuration/v1alpha1/zz_generated.deepcopy.go index fbce6db46f..cfdabd0717 100644 --- a/pkg/apis/configuration/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/configuration/v1alpha1/zz_generated.deepcopy.go @@ -144,6 +144,11 @@ func (in *Upstream) DeepCopyInto(out *Upstream) { *out = new(int) **out = **in } + if in.ProxyNextUpstreamTries != nil { + in, out := &in.ProxyNextUpstreamTries, &out.ProxyNextUpstreamTries + *out = new(int) + **out = **in + } out.TLS = in.TLS return } From 3d3dea27f9f1c1217943e5e3439a6faf26cb887e Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Tue, 23 Jul 2019 12:36:02 +0100 Subject: [PATCH 06/15] Add unit test --- internal/configs/virtualserver_test.go | 27 ++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index a580fbaf00..c283efe9b7 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -925,6 +925,33 @@ func TestGenerateProxyPassProtocol(t *testing.T) { } } +func TestGenerateString(t *testing.T) { + tests := []struct { + nextUpstream string + expected string + }{ + { + nextUpstream: "error timeout", + expected: "error timeout", + }, + { + nextUpstream: "", + expected: "error timeout", + }, + { + nextUpstream: "http_404", + expected: "http_404", + }, + } + + for _, test := range tests { + result := generateString(test.nextUpstream, "error timeout") + if result != test.expected { + t.Errorf("generateString() return %v but expected %v", result, test.expected) + } + } +} + func TestGenerateLocation(t *testing.T) { cfgParams := ConfigParams{ ProxyConnectTimeout: "30s", From 084e8531ad2cea7671dc4d9faf69891d30934704 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Tue, 23 Jul 2019 12:37:38 +0100 Subject: [PATCH 07/15] Remove generateTime and replaced with generateString --- internal/configs/virtualserver.go | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 64e4e58afc..0b90182d0a 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -205,7 +205,7 @@ func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, endp s := version2.UpstreamServer{ Address: e, MaxFails: generateIntFromPointer(upstream.MaxFails, cfgParams.MaxFails), - FailTimeout: generateTime(upstream.FailTimeout, cfgParams.FailTimeout), + FailTimeout: generateString(upstream.FailTimeout, cfgParams.FailTimeout), } upsServers = append(upsServers, s) } @@ -214,7 +214,7 @@ func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, endp s := version2.UpstreamServer{ Address: nginx502Server, MaxFails: generateIntFromPointer(upstream.MaxFails, cfgParams.MaxFails), - FailTimeout: generateTime(upstream.FailTimeout, cfgParams.FailTimeout), + FailTimeout: generateString(upstream.FailTimeout, cfgParams.FailTimeout), } upsServers = append(upsServers, s) } @@ -236,13 +236,6 @@ func generateLBMethod(method string, defaultMethod string) string { return method } -func generateTime(time string, defaultTime string) string { - if time == "" { - return defaultTime - } - return time -} - func generateIntFromPointer(n *int, defaultN int) int { if n == nil { return defaultN @@ -275,9 +268,9 @@ func generateLocation(path string, upstreamName string, upstream conf_v1alpha1.U return version2.Location{ Path: path, Snippets: cfgParams.LocationSnippets, - ProxyConnectTimeout: generateTime(upstream.ProxyConnectTimeout, cfgParams.ProxyConnectTimeout), - ProxyReadTimeout: generateTime(upstream.ProxyReadTimeout, cfgParams.ProxyReadTimeout), - ProxySendTimeout: generateTime(upstream.ProxySendTimeout, cfgParams.ProxySendTimeout), + ProxyConnectTimeout: generateString(upstream.ProxyConnectTimeout, cfgParams.ProxyConnectTimeout), + ProxyReadTimeout: generateString(upstream.ProxyReadTimeout, cfgParams.ProxyReadTimeout), + ProxySendTimeout: generateString(upstream.ProxySendTimeout, cfgParams.ProxySendTimeout), ClientMaxBodySize: cfgParams.ClientMaxBodySize, ProxyMaxTempFileSize: cfgParams.ProxyMaxTempFileSize, ProxyBuffering: cfgParams.ProxyBuffering, From 3f2fa62642d7e74f1683f177046253165cce6387 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Tue, 23 Jul 2019 15:29:14 +0100 Subject: [PATCH 08/15] Add validation for upstream fields --- .../configuration/validation/validation.go | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/pkg/apis/configuration/validation/validation.go b/pkg/apis/configuration/validation/validation.go index 1a004702fb..1d4784464e 100644 --- a/pkg/apis/configuration/validation/validation.go +++ b/pkg/apis/configuration/validation/validation.go @@ -142,6 +142,9 @@ func validateUpstreams(upstreams []v1alpha1.Upstream, fieldPath *field.Path, isP allErrs = append(allErrs, validateTime(u.ProxyConnectTimeout, idxPath.Child("connect-timeout"))...) allErrs = append(allErrs, validateTime(u.ProxyReadTimeout, idxPath.Child("read-timeout"))...) allErrs = append(allErrs, validateTime(u.ProxySendTimeout, idxPath.Child("send-timeout"))...) + allErrs = append(allErrs, validateNextUpstream(u.ProxyNextUpstream, idxPath.Child("next-upstream"))...) + allErrs = append(allErrs, validateTime(u.ProxyNextUpstream, idxPath.Child("next-upstream-timeout"))...) + allErrs = append(allErrs, validatePositiveIntOrZero(u.ProxyNextUpstreamTries, idxPath.Child("next-upstream-tries"))...) allErrs = append(allErrs, validateUpstreamLBMethod(u.LBMethod, idxPath.Child("lb-method"), isPlus)...) allErrs = append(allErrs, validateTime(u.FailTimeout, idxPath.Child("fail-timeout"))...) @@ -156,6 +159,32 @@ func validateUpstreams(upstreams []v1alpha1.Upstream, fieldPath *field.Path, isP return allErrs, upstreamNames } +// ValidateNextUpstream checks the values given for passing queries to a upstream +func validateNextUpstream(nextUpstream string, fieldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + validParams := map[string]bool{ + "error": true, + "timeout": true, + "invalid_header": true, + "http_500": true, + "http_502": true, + "http_503": true, + "http_504": true, + "http_403": true, + "http_404": true, + "http_429": true, + "non_idempotent": true, + "off": true, + } + params := strings.Fields(nextUpstream) + for _, para := range params { + if !validParams[para] { + allErrs = append(allErrs, field.Invalid(fieldPath, para, "not a valid parameter")) + } + } + return allErrs +} + // validateUpstreamName checks is an upstream name is valid. // The rules for NGINX upstream names are less strict than IsDNS1035Label. // However, it is convenient to enforce IsDNS1035Label in the yaml for From 4d47357bac6aff27501ba257cebac88455782bea Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Tue, 23 Jul 2019 16:12:49 +0100 Subject: [PATCH 09/15] Add test for validation --- .../configuration/validation/validation.go | 2 +- .../validation/validation_test.go | 111 ++++++++++++++---- 2 files changed, 91 insertions(+), 22 deletions(-) diff --git a/pkg/apis/configuration/validation/validation.go b/pkg/apis/configuration/validation/validation.go index 1d4784464e..71386bc01b 100644 --- a/pkg/apis/configuration/validation/validation.go +++ b/pkg/apis/configuration/validation/validation.go @@ -143,7 +143,7 @@ func validateUpstreams(upstreams []v1alpha1.Upstream, fieldPath *field.Path, isP allErrs = append(allErrs, validateTime(u.ProxyReadTimeout, idxPath.Child("read-timeout"))...) allErrs = append(allErrs, validateTime(u.ProxySendTimeout, idxPath.Child("send-timeout"))...) allErrs = append(allErrs, validateNextUpstream(u.ProxyNextUpstream, idxPath.Child("next-upstream"))...) - allErrs = append(allErrs, validateTime(u.ProxyNextUpstream, idxPath.Child("next-upstream-timeout"))...) + allErrs = append(allErrs, validateTime(u.ProxyNextUpstreamTimeout, idxPath.Child("next-upstream-timeout"))...) allErrs = append(allErrs, validatePositiveIntOrZero(u.ProxyNextUpstreamTries, idxPath.Child("next-upstream-tries"))...) allErrs = append(allErrs, validateUpstreamLBMethod(u.LBMethod, idxPath.Child("lb-method"), isPlus)...) diff --git a/pkg/apis/configuration/validation/validation_test.go b/pkg/apis/configuration/validation/validation_test.go index 8846382f06..6156fc0d33 100644 --- a/pkg/apis/configuration/validation/validation_test.go +++ b/pkg/apis/configuration/validation/validation_test.go @@ -133,14 +133,20 @@ func TestValidateUpstreams(t *testing.T) { { upstreams: []v1alpha1.Upstream{ { - Name: "upstream1", - Service: "test-1", - Port: 80, + Name: "upstream1", + Service: "test-1", + Port: 80, + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "10s", + ProxyNextUpstreamTries: createPointerFromInt(5), }, { - Name: "upstream2", - Service: "test-2", - Port: 80, + Name: "upstream2", + Service: "test-2", + Port: 80, + ProxyNextUpstream: "http_404", + ProxyNextUpstreamTimeout: "3s", + ProxyNextUpstreamTries: createPointerFromInt(15), }, }, expectedUpstreamNames: map[string]sets.Empty{ @@ -171,9 +177,12 @@ func TestValidateUpstreamsFails(t *testing.T) { { upstreams: []v1alpha1.Upstream{ { - Name: "@upstream1", - Service: "test-1", - Port: 80, + Name: "@upstream1", + Service: "test-1", + Port: 80, + ProxyNextUpstream: "http_502", + ProxyNextUpstreamTimeout: "10s", + ProxyNextUpstreamTries: createPointerFromInt(5), }, }, expectedUpstreamNames: sets.String{}, @@ -182,9 +191,12 @@ func TestValidateUpstreamsFails(t *testing.T) { { upstreams: []v1alpha1.Upstream{ { - Name: "upstream1", - Service: "@test-1", - Port: 80, + Name: "upstream1", + Service: "@test-1", + Port: 80, + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "10s", + ProxyNextUpstreamTries: createPointerFromInt(5), }, }, expectedUpstreamNames: map[string]sets.Empty{ @@ -195,9 +207,12 @@ func TestValidateUpstreamsFails(t *testing.T) { { upstreams: []v1alpha1.Upstream{ { - Name: "upstream1", - Service: "test-1", - Port: 0, + Name: "upstream1", + Service: "test-1", + Port: 0, + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "10s", + ProxyNextUpstreamTries: createPointerFromInt(5), }, }, expectedUpstreamNames: map[string]sets.Empty{ @@ -208,14 +223,20 @@ func TestValidateUpstreamsFails(t *testing.T) { { upstreams: []v1alpha1.Upstream{ { - Name: "upstream1", - Service: "test-1", - Port: 80, + Name: "upstream1", + Service: "test-1", + Port: 80, + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "10s", + ProxyNextUpstreamTries: createPointerFromInt(5), }, { - Name: "upstream1", - Service: "test-2", - Port: 80, + Name: "upstream1", + Service: "test-2", + Port: 80, + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "10s", + ProxyNextUpstreamTries: createPointerFromInt(5), }, }, expectedUpstreamNames: map[string]sets.Empty{ @@ -223,6 +244,54 @@ func TestValidateUpstreamsFails(t *testing.T) { }, msg: "duplicated upstreams", }, + { + upstreams: []v1alpha1.Upstream{ + { + Name: "upstream1", + Service: "test-1", + Port: 80, + ProxyNextUpstream: "https_504", + ProxyNextUpstreamTimeout: "10s", + ProxyNextUpstreamTries: createPointerFromInt(5), + }, + }, + expectedUpstreamNames: map[string]sets.Empty{ + "upstream1": sets.Empty{}, + }, + msg: "invalid next upstream syntax", + }, + { + upstreams: []v1alpha1.Upstream{ + { + Name: "upstream1", + Service: "test-1", + Port: 80, + ProxyNextUpstream: "http_504", + ProxyNextUpstreamTimeout: "-2s", + ProxyNextUpstreamTries: createPointerFromInt(5), + }, + }, + expectedUpstreamNames: map[string]sets.Empty{ + "upstream1": sets.Empty{}, + }, + msg: "invalid upstream timeout value", + }, + { + upstreams: []v1alpha1.Upstream{ + { + Name: "upstream1", + Service: "test-1", + Port: 80, + ProxyNextUpstream: "https_504", + ProxyNextUpstreamTimeout: "10s", + ProxyNextUpstreamTries: createPointerFromInt(-1), + }, + }, + expectedUpstreamNames: map[string]sets.Empty{ + "upstream1": sets.Empty{}, + }, + msg: "invalid upstream tries value", + }, } isPlus := false From 9ebcc9b5f6720c46605c99cafd2f518d98b30d3d Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Thu, 25 Jul 2019 11:21:30 +0100 Subject: [PATCH 10/15] Address feedback from PR --- docs/virtualserver-and-virtualserverroute.md | 14 ++--- internal/configs/virtualserver_test.go | 18 +++--- .../configuration/validation/validation.go | 39 ++++++++----- .../validation/validation_test.go | 56 ++++++++++++++++++- 4 files changed, 93 insertions(+), 34 deletions(-) diff --git a/docs/virtualserver-and-virtualserverroute.md b/docs/virtualserver-and-virtualserverroute.md index 4980d5ed41..732ecc1643 100644 --- a/docs/virtualserver-and-virtualserverroute.md +++ b/docs/virtualserver-and-virtualserverroute.md @@ -199,13 +199,13 @@ tls: | `lb-method` | The load [balancing method](https://docs.nginx.com/nginx/admin-guide/load-balancer/http-load-balancer/#choosing-a-load-balancing-method). To use the round-robin method, specify `round_robin`. The default is specified in the `lb-method` ConfigMap key. | `string` | No | | `fail-timeout` | The time during which the specified number of unsuccessful attempts to communicate with an upstream server should happen to consider the server unavailable. See the [fail_timeout](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#fail_timeout) parameter of the server directive. The default is set in the `fail-timeout` ConfigMap key. | `string` | No | | `max-fails` | The number of unsuccessful attempts to communicate with an upstream server that should happen in the duration set by the `fail-timeout` to consider the server unavailable. See the [max_fails](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#max_fails) parameter of the server directive. The default is set in the `max-fails` ConfgMap key. | `int` | No | -| `keepalive` | Configures the cache for connections to upstream servers. The value `0` disables the cache. See the [keepalive](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive) directive. The default is set in the `keepalive` ConfigMap key. | `int` | No -| `connect-timeout` | The timeout for establishing a connection with an upstream server. See the [proxy_connect_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_connect_timeout) directive. The default is specified in the `proxy-connect-timeout` ConfigMap key. | `string` | No -| `read-timeout` | The timeout for reading a response from an upstream server. See the [proxy_read_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout) directive. The default is specified in the `proxy-read-timeout` ConfigMap key. | `string` | No -| `send-timeout` | The timeout for transmitting a request to an upstream server. See the [proxy_send_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_send_timeout) directive. The default is specified in the `proxy-send-timeout` ConfigMap key. | `string` | No -| `next-upstream` | Specifies in which cases a request should be passed to the next upstream server. See the [proxy_next_upstream](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_next_upstream) directive. The default is `error timeout`. | `string` | No -| `next-upstream-timeout` | The time during which a request can be passed to the next upstream server. See the [proxy_next_upstream_timeout](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_next_upstream_timeout) directive. The `0` value turns off the time limit. The default is `0`. | `string` | No -| `next-upstream-tries` | The number of possible tries for passing a request to the next upstream server. See the [proxy_next_upstream_tries](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_next_upstream_tries) directive. The `0` value turns off this limit. The default is `0`. | `int` | No +| `keepalive` | Configures the cache for connections to upstream servers. The value `0` disables the cache. See the [keepalive](https://nginx.org/en/docs/http/ngx_http_upstream_module.html#keepalive) directive. The default is set in the `keepalive` ConfigMap key. | `int` | No | +| `connect-timeout` | The timeout for establishing a connection with an upstream server. See the [proxy_connect_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_connect_timeout) directive. The default is specified in the `proxy-connect-timeout` ConfigMap key. | `string` | No | +| `read-timeout` | The timeout for reading a response from an upstream server. See the [proxy_read_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_read_timeout) directive. The default is specified in the `proxy-read-timeout` ConfigMap key. | `string` | No | +| `send-timeout` | The timeout for transmitting a request to an upstream server. See the [proxy_send_timeout](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_send_timeout) directive. The default is specified in the `proxy-send-timeout` ConfigMap key. | `string` | No | +| `next-upstream` | Specifies in which cases a request should be passed to the next upstream server. See the [proxy_next_upstream](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_next_upstream) directive. The default is `error timeout`. | `string` | No | +| `next-upstream-timeout` | The time during which a request can be passed to the next upstream server. See the [proxy_next_upstream_timeout](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_next_upstream_timeout) directive. The `0` value turns off the time limit. The default is `0`. | `string` | No | +| `next-upstream-tries` | The number of possible tries for passing a request to the next upstream server. See the [proxy_next_upstream_tries](http://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_next_upstream_tries) directive. The `0` value turns off this limit. The default is `0`. | `int` | No | | `tls` | The TLS configuration for the Upstream. | [`tls`](#UpstreamTLS) | No | ### Upstream.TLS diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index c283efe9b7..7c0ffa4c15 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -927,25 +927,25 @@ func TestGenerateProxyPassProtocol(t *testing.T) { func TestGenerateString(t *testing.T) { tests := []struct { - nextUpstream string - expected string + inputS string + expected string }{ { - nextUpstream: "error timeout", - expected: "error timeout", + inputS: "error timeout", + expected: "error timeout", }, { - nextUpstream: "", - expected: "error timeout", + inputS: "", + expected: "error timeout", }, { - nextUpstream: "http_404", - expected: "http_404", + inputS: "http_404", + expected: "http_404", }, } for _, test := range tests { - result := generateString(test.nextUpstream, "error timeout") + result := generateString(test.inputS, "error timeout") if result != test.expected { t.Errorf("generateString() return %v but expected %v", result, test.expected) } diff --git a/pkg/apis/configuration/validation/validation.go b/pkg/apis/configuration/validation/validation.go index 71386bc01b..3126984b04 100644 --- a/pkg/apis/configuration/validation/validation.go +++ b/pkg/apis/configuration/validation/validation.go @@ -159,28 +159,37 @@ func validateUpstreams(upstreams []v1alpha1.Upstream, fieldPath *field.Path, isP return allErrs, upstreamNames } +var validVariableParams = map[string]bool{ + "error": true, + "timeout": true, + "invalid_header": true, + "http_500": true, + "http_502": true, + "http_503": true, + "http_504": true, + "http_403": true, + "http_404": true, + "http_429": true, + "non_idempotent": true, + "off": true, +} + // ValidateNextUpstream checks the values given for passing queries to a upstream func validateNextUpstream(nextUpstream string, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - validParams := map[string]bool{ - "error": true, - "timeout": true, - "invalid_header": true, - "http_500": true, - "http_502": true, - "http_503": true, - "http_504": true, - "http_403": true, - "http_404": true, - "http_429": true, - "non_idempotent": true, - "off": true, - } + var occur int params := strings.Fields(nextUpstream) for _, para := range params { - if !validParams[para] { + if !validVariableParams[para] { allErrs = append(allErrs, field.Invalid(fieldPath, para, "not a valid parameter")) } + for _, p := range params { + if p == para { + occur++ + } else if occur > 1 { + allErrs = append(allErrs, field.Invalid(fieldPath, para, "can not have recurring parameters")) + } + } } return allErrs } diff --git a/pkg/apis/configuration/validation/validation_test.go b/pkg/apis/configuration/validation/validation_test.go index 6156fc0d33..af255a3455 100644 --- a/pkg/apis/configuration/validation/validation_test.go +++ b/pkg/apis/configuration/validation/validation_test.go @@ -144,9 +144,9 @@ func TestValidateUpstreams(t *testing.T) { Name: "upstream2", Service: "test-2", Port: 80, - ProxyNextUpstream: "http_404", - ProxyNextUpstreamTimeout: "3s", - ProxyNextUpstreamTries: createPointerFromInt(15), + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "10s", + ProxyNextUpstreamTries: createPointerFromInt(5), }, }, expectedUpstreamNames: map[string]sets.Empty{ @@ -306,6 +306,56 @@ func TestValidateUpstreamsFails(t *testing.T) { } } +func TestValidateNextUpstream(t *testing.T) { + tests := []struct { + inputS string + expected bool + }{ + { + inputS: "error timeout", + expected: true, + }, + { + + inputS: "http_404 timeout", + expected: true, + }, + } + for _, test := range tests { + allErrs := validateNextUpstream(test.inputS, field.NewPath("next-upstreams")) + if len(allErrs) > 0 { + t.Errorf("validateNextUpstream() returned errors %v for valid input for the case of %s", allErrs, test.inputS) + } + } +} + +func TestValidateNextUpstreamFails(t *testing.T) { + tests := []struct { + inputS string + expected bool + }{ + { + inputS: "error error", + expected: false, + }, + { + + inputS: "", + expected: false, + }, + { + inputS: "https_404", + expected: false, + }, + } + for _, test := range tests { + allErrs := validateNextUpstream(test.inputS, field.NewPath("next-upstreams")) + if len(allErrs) < 0 { + t.Errorf("validateNextUpstream() didn't return errors %v for invalid input for the case of %s", allErrs, test.inputS) + } + } +} + func TestValidateDNS1035Label(t *testing.T) { validNames := []string{ "test", From bfa535c06c94b82e549d5d94fa1089397bdabe26 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Thu, 25 Jul 2019 12:27:06 +0100 Subject: [PATCH 11/15] Address feedback from PR --- pkg/apis/configuration/v1alpha1/types.go | 2 +- .../v1alpha1/zz_generated.deepcopy.go | 5 ----- .../configuration/validation/validation.go | 2 +- .../validation/validation_test.go | 22 ++++++------------- 4 files changed, 9 insertions(+), 22 deletions(-) diff --git a/pkg/apis/configuration/v1alpha1/types.go b/pkg/apis/configuration/v1alpha1/types.go index decdb440bb..d1237e8c2e 100644 --- a/pkg/apis/configuration/v1alpha1/types.go +++ b/pkg/apis/configuration/v1alpha1/types.go @@ -37,7 +37,7 @@ type Upstream struct { ProxySendTimeout string `json:"send-timeout"` ProxyNextUpstream string `json:"next-upstream"` ProxyNextUpstreamTimeout string `json:"next-upstream-timeout"` - ProxyNextUpstreamTries *int `json:"next-upstream-tries"` + ProxyNextUpstreamTries int `json:"next-upstream-tries"` TLS UpstreamTLS `json:"tls"` } diff --git a/pkg/apis/configuration/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/configuration/v1alpha1/zz_generated.deepcopy.go index cfdabd0717..fbce6db46f 100644 --- a/pkg/apis/configuration/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/configuration/v1alpha1/zz_generated.deepcopy.go @@ -144,11 +144,6 @@ func (in *Upstream) DeepCopyInto(out *Upstream) { *out = new(int) **out = **in } - if in.ProxyNextUpstreamTries != nil { - in, out := &in.ProxyNextUpstreamTries, &out.ProxyNextUpstreamTries - *out = new(int) - **out = **in - } out.TLS = in.TLS return } diff --git a/pkg/apis/configuration/validation/validation.go b/pkg/apis/configuration/validation/validation.go index 3126984b04..2afba6cace 100644 --- a/pkg/apis/configuration/validation/validation.go +++ b/pkg/apis/configuration/validation/validation.go @@ -174,7 +174,7 @@ var validVariableParams = map[string]bool{ "off": true, } -// ValidateNextUpstream checks the values given for passing queries to a upstream +// validateNextUpstream checks the values given for passing queries to a upstream func validateNextUpstream(nextUpstream string, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} var occur int diff --git a/pkg/apis/configuration/validation/validation_test.go b/pkg/apis/configuration/validation/validation_test.go index af255a3455..96ebae4eac 100644 --- a/pkg/apis/configuration/validation/validation_test.go +++ b/pkg/apis/configuration/validation/validation_test.go @@ -308,17 +308,14 @@ func TestValidateUpstreamsFails(t *testing.T) { func TestValidateNextUpstream(t *testing.T) { tests := []struct { - inputS string - expected bool + inputS string }{ { - inputS: "error timeout", - expected: true, + inputS: "error timeout", }, { - inputS: "http_404 timeout", - expected: true, + inputS: "http_404 timeout", }, } for _, test := range tests { @@ -331,21 +328,16 @@ func TestValidateNextUpstream(t *testing.T) { func TestValidateNextUpstreamFails(t *testing.T) { tests := []struct { - inputS string - expected bool + inputS strings }{ { - inputS: "error error", - expected: false, + inputS: "error error", }, { - - inputS: "", - expected: false, + inputS: "", }, { - inputS: "https_404", - expected: false, + inputS: "https_404", }, } for _, test := range tests { From e6d5cf47761f31d1218c688314b02880ed3c7e3b Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Thu, 25 Jul 2019 12:38:52 +0100 Subject: [PATCH 12/15] Fix errors for tests --- internal/configs/virtualserver.go | 2 +- .../configuration/validation/validation.go | 2 +- .../validation/validation_test.go | 22 +++++++++---------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 0b90182d0a..298bb7f3ab 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -279,7 +279,7 @@ func generateLocation(path string, upstreamName string, upstream conf_v1alpha1.U ProxyPass: fmt.Sprintf("%v://%v", generateProxyPassProtocol(upstream), upstreamName), ProxyNextUpstream: generateString(upstream.ProxyNextUpstream, "error timeout"), ProxyNextUpstreamTimeout: generateString(upstream.ProxyNextUpstreamTimeout, "0s"), - ProxyNextUpstreamTries: generateIntFromPointer(upstream.ProxyNextUpstreamTries, 0), + ProxyNextUpstreamTries: upstream.ProxyNextUpstreamTries, HasKeepalive: upstreamHasKeepalive(upstream, cfgParams), } } diff --git a/pkg/apis/configuration/validation/validation.go b/pkg/apis/configuration/validation/validation.go index 2afba6cace..31f680a5f3 100644 --- a/pkg/apis/configuration/validation/validation.go +++ b/pkg/apis/configuration/validation/validation.go @@ -144,7 +144,7 @@ func validateUpstreams(upstreams []v1alpha1.Upstream, fieldPath *field.Path, isP allErrs = append(allErrs, validateTime(u.ProxySendTimeout, idxPath.Child("send-timeout"))...) allErrs = append(allErrs, validateNextUpstream(u.ProxyNextUpstream, idxPath.Child("next-upstream"))...) allErrs = append(allErrs, validateTime(u.ProxyNextUpstreamTimeout, idxPath.Child("next-upstream-timeout"))...) - allErrs = append(allErrs, validatePositiveIntOrZero(u.ProxyNextUpstreamTries, idxPath.Child("next-upstream-tries"))...) + allErrs = append(allErrs, validatePositiveIntOrZero(&u.ProxyNextUpstreamTries, idxPath.Child("next-upstream-tries"))...) allErrs = append(allErrs, validateUpstreamLBMethod(u.LBMethod, idxPath.Child("lb-method"), isPlus)...) allErrs = append(allErrs, validateTime(u.FailTimeout, idxPath.Child("fail-timeout"))...) diff --git a/pkg/apis/configuration/validation/validation_test.go b/pkg/apis/configuration/validation/validation_test.go index 96ebae4eac..06e56405ee 100644 --- a/pkg/apis/configuration/validation/validation_test.go +++ b/pkg/apis/configuration/validation/validation_test.go @@ -138,7 +138,7 @@ func TestValidateUpstreams(t *testing.T) { Port: 80, ProxyNextUpstream: "error timeout", ProxyNextUpstreamTimeout: "10s", - ProxyNextUpstreamTries: createPointerFromInt(5), + ProxyNextUpstreamTries: 5, }, { Name: "upstream2", @@ -146,7 +146,7 @@ func TestValidateUpstreams(t *testing.T) { Port: 80, ProxyNextUpstream: "error timeout", ProxyNextUpstreamTimeout: "10s", - ProxyNextUpstreamTries: createPointerFromInt(5), + ProxyNextUpstreamTries: 5, }, }, expectedUpstreamNames: map[string]sets.Empty{ @@ -182,7 +182,7 @@ func TestValidateUpstreamsFails(t *testing.T) { Port: 80, ProxyNextUpstream: "http_502", ProxyNextUpstreamTimeout: "10s", - ProxyNextUpstreamTries: createPointerFromInt(5), + ProxyNextUpstreamTries: 5, }, }, expectedUpstreamNames: sets.String{}, @@ -196,7 +196,7 @@ func TestValidateUpstreamsFails(t *testing.T) { Port: 80, ProxyNextUpstream: "error timeout", ProxyNextUpstreamTimeout: "10s", - ProxyNextUpstreamTries: createPointerFromInt(5), + ProxyNextUpstreamTries: 5, }, }, expectedUpstreamNames: map[string]sets.Empty{ @@ -212,7 +212,7 @@ func TestValidateUpstreamsFails(t *testing.T) { Port: 0, ProxyNextUpstream: "error timeout", ProxyNextUpstreamTimeout: "10s", - ProxyNextUpstreamTries: createPointerFromInt(5), + ProxyNextUpstreamTries: 5, }, }, expectedUpstreamNames: map[string]sets.Empty{ @@ -228,7 +228,7 @@ func TestValidateUpstreamsFails(t *testing.T) { Port: 80, ProxyNextUpstream: "error timeout", ProxyNextUpstreamTimeout: "10s", - ProxyNextUpstreamTries: createPointerFromInt(5), + ProxyNextUpstreamTries: 5, }, { Name: "upstream1", @@ -236,7 +236,7 @@ func TestValidateUpstreamsFails(t *testing.T) { Port: 80, ProxyNextUpstream: "error timeout", ProxyNextUpstreamTimeout: "10s", - ProxyNextUpstreamTries: createPointerFromInt(5), + ProxyNextUpstreamTries: 5, }, }, expectedUpstreamNames: map[string]sets.Empty{ @@ -252,7 +252,7 @@ func TestValidateUpstreamsFails(t *testing.T) { Port: 80, ProxyNextUpstream: "https_504", ProxyNextUpstreamTimeout: "10s", - ProxyNextUpstreamTries: createPointerFromInt(5), + ProxyNextUpstreamTries: 5, }, }, expectedUpstreamNames: map[string]sets.Empty{ @@ -268,7 +268,7 @@ func TestValidateUpstreamsFails(t *testing.T) { Port: 80, ProxyNextUpstream: "http_504", ProxyNextUpstreamTimeout: "-2s", - ProxyNextUpstreamTries: createPointerFromInt(5), + ProxyNextUpstreamTries: 5, }, }, expectedUpstreamNames: map[string]sets.Empty{ @@ -284,7 +284,7 @@ func TestValidateUpstreamsFails(t *testing.T) { Port: 80, ProxyNextUpstream: "https_504", ProxyNextUpstreamTimeout: "10s", - ProxyNextUpstreamTries: createPointerFromInt(-1), + ProxyNextUpstreamTries: -1, }, }, expectedUpstreamNames: map[string]sets.Empty{ @@ -328,7 +328,7 @@ func TestValidateNextUpstream(t *testing.T) { func TestValidateNextUpstreamFails(t *testing.T) { tests := []struct { - inputS strings + inputS string }{ { inputS: "error error", From ba63652b35cd27cd381a9607f8d9b155b10d49a8 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Thu, 25 Jul 2019 16:17:54 +0100 Subject: [PATCH 13/15] Resolve feedback from review --- internal/configs/virtualserver_test.go | 4 ---- pkg/apis/configuration/validation/validation.go | 17 ++++++++--------- .../configuration/validation/validation_test.go | 8 ++------ 3 files changed, 10 insertions(+), 19 deletions(-) diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 7c0ffa4c15..5256a574f3 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -938,10 +938,6 @@ func TestGenerateString(t *testing.T) { inputS: "", expected: "error timeout", }, - { - inputS: "http_404", - expected: "http_404", - }, } for _, test := range tests { diff --git a/pkg/apis/configuration/validation/validation.go b/pkg/apis/configuration/validation/validation.go index 31f680a5f3..4d647c5c3c 100644 --- a/pkg/apis/configuration/validation/validation.go +++ b/pkg/apis/configuration/validation/validation.go @@ -159,7 +159,7 @@ func validateUpstreams(upstreams []v1alpha1.Upstream, fieldPath *field.Path, isP return allErrs, upstreamNames } -var validVariableParams = map[string]bool{ +var validNextUpstreamParams = map[string]bool{ "error": true, "timeout": true, "invalid_header": true, @@ -172,23 +172,22 @@ var validVariableParams = map[string]bool{ "http_429": true, "non_idempotent": true, "off": true, + "": true, } // validateNextUpstream checks the values given for passing queries to a upstream func validateNextUpstream(nextUpstream string, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - var occur int + allParams := sets.String{} params := strings.Fields(nextUpstream) for _, para := range params { - if !validVariableParams[para] { + if !validNextUpstreamParams[para] { allErrs = append(allErrs, field.Invalid(fieldPath, para, "not a valid parameter")) } - for _, p := range params { - if p == para { - occur++ - } else if occur > 1 { - allErrs = append(allErrs, field.Invalid(fieldPath, para, "can not have recurring parameters")) - } + if allParams.Has(para) { + allErrs = append(allErrs, field.Invalid(fieldPath, para, "can not have duplicate parameters")) + } else { + allParams.Insert(para) } } return allErrs diff --git a/pkg/apis/configuration/validation/validation_test.go b/pkg/apis/configuration/validation/validation_test.go index 06e56405ee..e035352a8d 100644 --- a/pkg/apis/configuration/validation/validation_test.go +++ b/pkg/apis/configuration/validation/validation_test.go @@ -314,14 +314,13 @@ func TestValidateNextUpstream(t *testing.T) { inputS: "error timeout", }, { - inputS: "http_404 timeout", }, } for _, test := range tests { allErrs := validateNextUpstream(test.inputS, field.NewPath("next-upstreams")) if len(allErrs) > 0 { - t.Errorf("validateNextUpstream() returned errors %v for valid input for the case of %s", allErrs, test.inputS) + t.Errorf("validateNextUpstream(%q) returned errors %v for valid input for the case of %s", test.inputS, allErrs, test.inputS) } } } @@ -333,9 +332,6 @@ func TestValidateNextUpstreamFails(t *testing.T) { { inputS: "error error", }, - { - inputS: "", - }, { inputS: "https_404", }, @@ -343,7 +339,7 @@ func TestValidateNextUpstreamFails(t *testing.T) { for _, test := range tests { allErrs := validateNextUpstream(test.inputS, field.NewPath("next-upstreams")) if len(allErrs) < 0 { - t.Errorf("validateNextUpstream() didn't return errors %v for invalid input for the case of %s", allErrs, test.inputS) + t.Errorf("validateNextUpstream(%q) didn't return errors %v for invalid input for the case of %s", test.inputS, allErrs, test.inputS) } } } From d9f88740990e27bf12fe44e0fbf9a1e20cd64e59 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Thu, 25 Jul 2019 16:21:33 +0100 Subject: [PATCH 14/15] Update tests --- internal/configs/virtualserver_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 5256a574f3..93c7dc3395 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -931,8 +931,8 @@ func TestGenerateString(t *testing.T) { expected string }{ { - inputS: "error timeout", - expected: "error timeout", + inputS: "http_404", + expected: "http_404", }, { inputS: "", From 02ed166d243494a2a4bb0c35c9cd7c39610e1e26 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Fri, 26 Jul 2019 10:15:31 +0100 Subject: [PATCH 15/15] Add final changes from feedback --- pkg/apis/configuration/validation/validation.go | 3 +++ pkg/apis/configuration/validation/validation_test.go | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/pkg/apis/configuration/validation/validation.go b/pkg/apis/configuration/validation/validation.go index 4d647c5c3c..47cb7bfeb2 100644 --- a/pkg/apis/configuration/validation/validation.go +++ b/pkg/apis/configuration/validation/validation.go @@ -179,6 +179,9 @@ var validNextUpstreamParams = map[string]bool{ func validateNextUpstream(nextUpstream string, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} allParams := sets.String{} + if nextUpstream == "" { + return allErrs + } params := strings.Fields(nextUpstream) for _, para := range params { if !validNextUpstreamParams[para] { diff --git a/pkg/apis/configuration/validation/validation_test.go b/pkg/apis/configuration/validation/validation_test.go index e035352a8d..626e20100b 100644 --- a/pkg/apis/configuration/validation/validation_test.go +++ b/pkg/apis/configuration/validation/validation_test.go @@ -320,7 +320,7 @@ func TestValidateNextUpstream(t *testing.T) { for _, test := range tests { allErrs := validateNextUpstream(test.inputS, field.NewPath("next-upstreams")) if len(allErrs) > 0 { - t.Errorf("validateNextUpstream(%q) returned errors %v for valid input for the case of %s", test.inputS, allErrs, test.inputS) + t.Errorf("validateNextUpstream(%q) returned errors %v for valid input.", test.inputS, allErrs) } } } @@ -339,7 +339,7 @@ func TestValidateNextUpstreamFails(t *testing.T) { for _, test := range tests { allErrs := validateNextUpstream(test.inputS, field.NewPath("next-upstreams")) if len(allErrs) < 0 { - t.Errorf("validateNextUpstream(%q) didn't return errors %v for invalid input for the case of %s", test.inputS, allErrs, test.inputS) + t.Errorf("validateNextUpstream(%q) didn't return errors %v for invalid input.", test.inputS, allErrs) } } }