From 45071978a8d837f48e88655e8d0d3d0154576171 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Wed, 17 Jul 2019 12:17:14 +0100 Subject: [PATCH 1/9] 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 ae556cd9daedbbd6a41ba6bb105b391af33ca85a Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Thu, 18 Jul 2019 14:37:15 +0100 Subject: [PATCH 2/9] 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 e8b74dd939b062c7b0b269bc65596c37ca43895a Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Mon, 22 Jul 2019 10:57:52 +0100 Subject: [PATCH 3/9] 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 2d39589b107721e89f44112896a8c82b263ad1a6 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Tue, 23 Jul 2019 12:21:33 +0100 Subject: [PATCH 4/9] 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 8d54e70789c11a3030f7f85c5f05b4e53a324d75 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Tue, 23 Jul 2019 12:22:40 +0100 Subject: [PATCH 5/9] 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 5f9f971d26e927f8f207108780bf11bd1f1cc946 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Tue, 23 Jul 2019 12:36:02 +0100 Subject: [PATCH 6/9] 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 13e5b552825aca21cc5371afdb1e387f7bf6cc5d Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Tue, 23 Jul 2019 12:37:38 +0100 Subject: [PATCH 7/9] 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 742676a1c0a6a899c3f73aaa569f3f91f01687e0 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Tue, 23 Jul 2019 15:29:14 +0100 Subject: [PATCH 8/9] 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 e22d7974018929304473e92b40780cd237e4d935 Mon Sep 17 00:00:00 2001 From: LorcanMcVeigh Date: Tue, 23 Jul 2019 16:12:49 +0100 Subject: [PATCH 9/9] 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