Addressed the PR comments

Co-Authored-By: Michael Pleshakov <[email protected]>

Author: ampant

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
 

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"`

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
 }
 

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)
 	}
 }