From c7e6e9bf50e3c615bd112f8b6d9a13d3749b69a6 Mon Sep 17 00:00:00 2001 From: ampant <52200332+ampant@users.noreply.github.com> Date: Wed, 28 Aug 2019 10:28:03 +0530 Subject: [PATCH] Addressed the PR comments Co-Authored-By: Michael Pleshakov --- docs/virtualserver-and-virtualserverroute.md | 20 +++++------ pkg/apis/configuration/v1alpha1/types.go | 2 +- .../configuration/validation/validation.go | 35 +++++++++++-------- .../validation/validation_test.go | 18 ++++++---- 4 files changed, 41 insertions(+), 34 deletions(-) diff --git a/docs/virtualserver-and-virtualserverroute.md b/docs/virtualserver-and-virtualserverroute.md index 0c02b101eb..d94bd31de5 100644 --- a/docs/virtualserver-and-virtualserverroute.md +++ b/docs/virtualserver-and-virtualserverroute.md @@ -18,6 +18,7 @@ This document is the reference documentation for the resources. To see additiona - [VirtualServerRoute.Subroute](#virtualserverroutesubroute) - [Common Parts of the VirtualServer and VirtualServerRoute](#common-parts-of-the-virtualserver-and-virtualserverroute) - [Upstream](#upstream) + - [Upstream.Buffers](#upstreambuffers) - [Upstream.TLS](#upstreamtls) - [Upstream.Healthcheck](#upstreamhealthcheck) - [Header](#header) @@ -218,27 +219,22 @@ tls: | `tls` | The TLS configuration for the Upstream. | [`tls`](#UpstreamTLS) | No | | `healthCheck` | The health check configuration for the Upstream. See the [health_check](http://nginx.org/en/docs/http/ngx_http_upstream_hc_module.html#health_check) directive. Note: this feature is supported only in NGINX Plus. | [`healthcheck`](#UpstreamHealthcheck) | No | | `buffering` | Enables buffering of responses from the upstream server. See the [proxy_buffering](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffering) directive. The default is set in the `proxy-buffering` ConfigMap key. | `boolean` | No | -| `buffers` | Configures the buffers used for reading a response from the upstream server for a single connection. The buffers field configures the buffers used for reading a response from the upstream server for a single connection: number: 4 size: 8K . See the [proxy_buffers](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffers) directive for additional information. | `buffers` | No | +| `buffers` | Configures the buffers used for reading a response from the upstream server for a single connection. | [`buffers`](#UpstreamBuffers) | No | | `buffer-size` | Sets the size of the buffer used for reading the first part of a response received from the upstream server. See the [proxy_buffer_size](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffer_size) directive. The default is set in the `proxy-buffer-size` ConfigMap key. | `string` | No | ### Upstream.Buffers -Buffers defines Buffer Configuration for an Upstream. The buffers field configures the buffers used for reading a response from the upstream server for a single connection. For example +The buffers field configures the buffers used for reading a response from the upstream server for a single connection: ```yaml -name: tea - service: tea-svc - port: 80 - buffering: true - buffers: - number: 4 - size: 8K - buffer-size: 16k +number: 4 +size: 8K ``` +See the [proxy_buffers](https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffers) directive for additional information. | Field | Description | Type | Required | | ----- | ----------- | ---- | -------- | -| `number` | Configures the number of buffers. The default is set in the `proxy-buffers` ConfigMap key. | `int` | Yes | -| `size` | Configures the size of a buffer. The default is set in the `proxy-buffers` ConfigMap key. | `string` | Yes | +| `number` | Configures the number of buffers. The default is set in the `proxy-buffers` ConfigMap key. | `int` | Yes | +| `size` | Configures the size of a buffer. The default is set in the `proxy-buffers` ConfigMap key. | `string` | Yes | ### Upstream.TLS diff --git a/pkg/apis/configuration/v1alpha1/types.go b/pkg/apis/configuration/v1alpha1/types.go index 212b8317f8..2d940551b9 100644 --- a/pkg/apis/configuration/v1alpha1/types.go +++ b/pkg/apis/configuration/v1alpha1/types.go @@ -47,7 +47,7 @@ type Upstream struct { HealthCheck *HealthCheck `json:"healthCheck"` } -//UpstreamBuffers defines Buffer Configuration for an Upstream +// UpstreamBuffers defines Buffer Configuration for an Upstream type UpstreamBuffers struct { Number int `json:"number"` Size string `json:"size"` diff --git a/pkg/apis/configuration/validation/validation.go b/pkg/apis/configuration/validation/validation.go index 9a985d3ae4..35a6ff3317 100644 --- a/pkg/apis/configuration/validation/validation.go +++ b/pkg/apis/configuration/validation/validation.go @@ -101,16 +101,16 @@ const offsetErrMsg = "must consist of numeric characters followed by a valid siz var offsetRegexp = regexp.MustCompile("^" + offsetFmt + "$") -func validateOffset(size string, fieldPath *field.Path) field.ErrorList { +func validateOffset(offset string, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - if size == "" { + if offset == "" { return allErrs } - if !offsetRegexp.MatchString(size) { + if !offsetRegexp.MatchString(offset) { msg := validation.RegexError(offsetErrMsg, offsetFmt, "16", "32k", "64M") - return append(allErrs, field.Invalid(fieldPath, size, msg)) + return append(allErrs, field.Invalid(fieldPath, offset, msg)) } return allErrs @@ -121,32 +121,37 @@ const sizeErrMsg = "must consist of numeric characters followed by a valid size var sizeRegexp = regexp.MustCompile("^" + sizeFmt + "$") -func validateBuffer(buff *v1alpha1.UpstreamBuffers, fieldPath *field.Path) field.ErrorList { +func validateSize(size string, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - if buff == nil { + if size == "" { return allErrs } - if buff.Number <= 0 { - return append(allErrs, field.Invalid(fieldPath, buff.Number, "must be positive")) + if !sizeRegexp.MatchString(size) { + msg := validation.RegexError(sizeErrMsg, sizeFmt, "16", "32k", "64M") + return append(allErrs, field.Invalid(fieldPath, size, msg)) } - - allErrs = validateSize(buff.Size, fieldPath) return allErrs } -func validateSize(size string, fieldPath *field.Path) field.ErrorList { +func validateBuffer(buff *v1alpha1.UpstreamBuffers, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - if size == "" { + if buff == nil { return allErrs } - if !sizeRegexp.MatchString(size) { - msg := validation.RegexError(sizeErrMsg, sizeFmt, "16", "32k", "64M") - return append(allErrs, field.Invalid(fieldPath, size, msg)) + if buff.Number <= 0 { + allErrs = append(allErrs, field.Invalid(fieldPath.Child("number"), buff.Number, "must be positive")) } + + if buff.Size == "" { + allErrs = append(allErrs, field.Required(fieldPath.Child("size"), "cannot be empty")) + } else { + allErrs = append(validateSize(buff.Size, fieldPath.Child("size"))) + } + return allErrs } diff --git a/pkg/apis/configuration/validation/validation_test.go b/pkg/apis/configuration/validation/validation_test.go index 4f5179e735..7f09c58141 100644 --- a/pkg/apis/configuration/validation/validation_test.go +++ b/pkg/apis/configuration/validation/validation_test.go @@ -1690,17 +1690,17 @@ func TestValidateTime(t *testing.T) { } func TestValidateOffset(t *testing.T) { - var validInput = []string{"", "1", "10k", "11m", "1K", "100M"} + var validInput = []string{"", "1", "10k", "11m", "1K", "100M", "5G"} for _, test := range validInput { - allErrs := validateOffset(test, field.NewPath("size-field")) + allErrs := validateOffset(test, field.NewPath("offset-field")) if len(allErrs) != 0 { t.Errorf("validateOffset(%q) returned an error for valid input", test) } } - var invalidInput = []string{"55mm", "2mG", "6kb", "-5k", "1L"} + var invalidInput = []string{"55mm", "2mG", "6kb", "-5k", "1L", "5Gb"} for _, test := range invalidInput { - allErrs := validateOffset(test, field.NewPath("size-field")) + allErrs := validateOffset(test, field.NewPath("offset-field")) if len(allErrs) == 0 { t.Errorf("validateOffset(%q) didn't return error for invalid input.", test) } @@ -1718,13 +1718,19 @@ func TestValidateBuffer(t *testing.T) { invalidbuff := &v1alpha1.UpstreamBuffers{Number: -8, Size: "15G"} allErrs = validateBuffer(invalidbuff, field.NewPath("buffers-field")) if len(allErrs) == 0 { - t.Errorf("validateBuffer didn't return error %v for invalid input %v.", allErrs, invalidbuff) + t.Errorf("validateBuffer didn't return error for invalid input %v.", invalidbuff) } invalidbuff = &v1alpha1.UpstreamBuffers{Number: 8, Size: "15G"} allErrs = validateBuffer(invalidbuff, field.NewPath("buffers-field")) if len(allErrs) == 0 { - t.Errorf("validateBuffer didn't return error %v for invalid input %v.", allErrs, invalidbuff) + t.Errorf("validateBuffer didn't return error for invalid input %v.", invalidbuff) + } + + invalidbuff = &v1alpha1.UpstreamBuffers{Number: 8, Size: ""} + allErrs = validateBuffer(invalidbuff, field.NewPath("buffers-field")) + if len(allErrs) == 0 { + t.Errorf("validateBuffer didn't return error for invalid input %v.", invalidbuff) } }