Skip to content

Commit

Permalink
Address PR feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@LorcanMcVeigh
LorcanMcVeigh committed Jul 24, 2019
1 parent 4af171d commit db6c954
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 45 deletions.
14 changes: 7 additions & 7 deletions docs/virtualserver-and-virtualserverroute.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 7 additions & 11 deletions internal/configs/virtualserver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -927,25 +927,21 @@ 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",
},
{
nextUpstream: "",
expected: "error timeout",
inputS: "error timeout",
expected: "error timeout",
},
{
nextUpstream: "http_404",
expected: "http_404",
inputS: "",
expected: "error timeout",
},
}

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)
}
Expand Down
39 changes: 24 additions & 15 deletions pkg/apis/configuration/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
68 changes: 56 additions & 12 deletions pkg/apis/configuration/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,20 +133,14 @@ func TestValidateUpstreams(t *testing.T) {
{
upstreams: []v1alpha1.Upstream{
{
Name: "upstream1",
Service: "test-1",
Port: 80,
ProxyNextUpstream: "error timeout",
ProxyNextUpstreamTimeout: "10s",
ProxyNextUpstreamTries: createPointerFromInt(5),
Name: "upstream1",
Service: "test-1",
Port: 80,
},
{
Name: "upstream2",
Service: "test-2",
Port: 80,
ProxyNextUpstream: "http_404",
ProxyNextUpstreamTimeout: "3s",
ProxyNextUpstreamTries: createPointerFromInt(15),
Name: "upstream2",
Service: "test-2",
Port: 80,
},
},
expectedUpstreamNames: map[string]sets.Empty{
Expand All @@ -168,6 +162,56 @@ func TestValidateUpstreams(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 TestValidateUpstreamsFails(t *testing.T) {
tests := []struct {
upstreams []v1alpha1.Upstream
Expand Down

0 comments on commit db6c954

Please sign in to comment.