Add fail-timeout and max-fails support to vs/vsr

Author: Rulox

docs/virtualserver-and-virtualserverroute.md

@@ -176,7 +176,9 @@ The upstream defines a destination for the routing configuration. For example:
 name: tea
 service: tea-svc
 port: 80
-lb-method: "round_robin"
+lb-method: round_robin
+fail-timeout: 10s
+max-fails: 1
 ``` 
 
 | Field | Description | Type | Required |
@@ -185,7 +187,8 @@ lb-method: "round_robin"
 | `service` | The name of a [service](https://kubernetes.io/docs/concepts/services-networking/service/). The service must belong to the same namespace as the resource. If the service doesn't exist, NGINX will assume the service has zero endpoints and return a `502` response for requests for this upstream. | `string` | Yes |
 | `port` | The port of the service. If the service doesn't define that port, NGINX will assume the service has zero endpoints and return a `502` response for requests for this upstream. The port must fall into the range `1..65553`. | `uint16` | Yes |
 | `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 |
 
 ### Split
 

internal/configs/annotations.go

@@ -1,9 +1,7 @@
 package configs
 
 import (
-	"errors"
 	"fmt"
-	"regexp"
 	"strconv"
 	"strings"
 
@@ -102,7 +100,7 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool
 	}
 
 	if slowStart, exists := ingEx.Ingress.Annotations["nginx.com/slow-start"]; exists {
-		if parsedSlowStart, err := parseSlowStart(slowStart); err != nil {
+		if parsedSlowStart, err := ParseTime(slowStart); err != nil {
 			glog.Errorf("Ingress %s/%s: Invalid value nginx.org/slow-start: got %q: %v", ingEx.Ingress.GetNamespace(), ingEx.Ingress.GetName(), slowStart, err)
 		} else {
 			if isPlus {
@@ -426,31 +424,6 @@ func mergeMasterAnnotationsIntoMinion(minionAnnotations map[string]string, maste
 	}
 }
 
-// http://nginx.org/en/docs/syntax.html
-var validTimeSuffixes = []string{
-	"ms",
-	"s",
-	"m",
-	"h",
-	"d",
-	"w",
-	"M",
-	"y",
-}
-
-var durationEscaped = strings.Join(validTimeSuffixes, "|")
-var validNginxTime = regexp.MustCompile(`^([0-9]+([` + durationEscaped + `]?){0,1} *)+$`)
-
-// parseSlowStart ensures that the slow_start value in the annotation is valid.
-func parseSlowStart(s string) (string, error) {
-	s = strings.TrimSpace(s)
-
-	if validNginxTime.MatchString(s) {
-		return s, nil
-	}
-	return "", errors.New("Invalid time string")
-}
-
 func parsePort(value string) (int, error) {
 	port, err := strconv.ParseInt(value, 10, 16)
 	if err != nil {
@@ -504,4 +477,3 @@ func parseRewrites(service string) (serviceName string, rewrite string, err erro
 
 	return svcNameParts[1], rwPathParts[1], nil
 }
-

internal/configs/annotations_test.go

@@ -151,23 +151,3 @@ func TestMergeMasterAnnotationsIntoMinion(t *testing.T) {
 		t.Errorf("mergeMasterAnnotationsIntoMinion returned %v, but expected %v", minionAnnotations, expectedMergedAnnotations)
 	}
 }
-
-func TestParseSlowStart(t *testing.T) {
-	var testsWithValidInput = []string{"1", "1m10s", "11 11", "5m 30s", "1s", "100m", "5w", "15m", "11M", "3h", "100y", "600"}
-	var invalidInput = []string{"ss", "rM", "m0m", "s1s", "-5s", "", "1L"}
-	for _, test := range testsWithValidInput {
-		result, err := parseSlowStart(test)
-		if err != nil {
-			t.Errorf("TestParseSlowStart(%q) returned an error for valid input", test)
-		}
-		if test != result {
-			t.Errorf("TestParseSlowStart(%q) returned %q expected %q", test, result, test)
-		}
-	}
-	for _, test := range invalidInput {
-		result, err := parseSlowStart(test)
-		if err == nil {
-			t.Errorf("TestParseSlowStart(%q) didn't return error. Returned: %q", test, result)
-		}
-	}
-}

internal/configs/parsing_helpers.go

@@ -1,7 +1,9 @@
 package configs
 
 import (
+	"errors"
 	"fmt"
+	"regexp"
 	"strconv"
 	"strings"
 
@@ -159,3 +161,28 @@ func validateHashLBMethod(method string) (string, error) {
 
 	return "", fmt.Errorf("Invalid load balancing method: %q", method)
 }
+
+// http://nginx.org/en/docs/syntax.html
+var validTimeSuffixes = []string{
+	"ms",
+	"s",
+	"m",
+	"h",
+	"d",
+	"w",
+	"M",
+	"y",
+}
+
+var durationEscaped = strings.Join(validTimeSuffixes, "|")
+var validNginxTime = regexp.MustCompile(`^([0-9]+([` + durationEscaped + `]?){0,1} *)+$`)
+
+// ParseTime ensures that the string value in the annotation is a valid time.
+func ParseTime(s string) (string, error) {
+	s = strings.TrimSpace(s)
+
+	if validNginxTime.MatchString(s) {
+		return s, nil
+	}
+	return "", errors.New("Invalid time string")
+}

internal/configs/parsing_helpers_test.go

@@ -4,7 +4,7 @@ import (
 	"reflect"
 	"testing"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/api/extensions/v1beta1"
 	meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
@@ -362,3 +362,23 @@ func TestParseLBMethodForPlus(t *testing.T) {
 		}
 	}
 }
+
+func TestParseTime(t *testing.T) {
+	var testsWithValidInput = []string{"1", "1m10s", "11 11", "5m 30s", "1s", "100m", "5w", "15m", "11M", "3h", "100y", "600"}
+	var invalidInput = []string{"ss", "rM", "m0m", "s1s", "-5s", "", "1L"}
+	for _, test := range testsWithValidInput {
+		result, err := ParseTime(test)
+		if err != nil {
+			t.Errorf("TestparseTime(%q) returned an error for valid input", test)
+		}
+		if test != result {
+			t.Errorf("TestparseTime(%q) returned %q expected %q", test, result, test)
+		}
+	}
+	for _, test := range invalidInput {
+		result, err := ParseTime(test)
+		if err == nil {
+			t.Errorf("TestparseTime(%q) didn't return error. Returned: %q", test, result)
+		}
+	}
+}

internal/configs/virtualserver.go

@@ -201,17 +201,17 @@ func generateUpstream(upstreamName string, upstream conf_v1alpha1.Upstream, endp
 	for _, e := range endpoints {
 		s := version2.UpstreamServer{
 			Address:     e,
-			MaxFails:    cfgParams.MaxFails,
-			FailTimeout: cfgParams.FailTimeout,
+			MaxFails:    generatePositiveIntFromPointer(upstream.MaxFails, cfgParams.MaxFails),
+			FailTimeout: generateTime(upstream.FailTimeout, cfgParams.FailTimeout),
 		}
 		upsServers = append(upsServers, s)
 	}
 
 	if !isPlus && len(upsServers) == 0 {
 		s := version2.UpstreamServer{
 			Address:     nginx502Server,
-			MaxFails:    cfgParams.MaxFails,
-			FailTimeout: cfgParams.FailTimeout,
+			MaxFails:    generatePositiveIntFromPointer(upstream.MaxFails, cfgParams.MaxFails),
+			FailTimeout: generateTime(upstream.FailTimeout, cfgParams.FailTimeout),
 		}
 		upsServers = append(upsServers, s)
 	}
@@ -235,6 +235,20 @@ func generateLBMethod(method string, defaultMethod string) string {
 	return method
 }
 
+func generateTime(time string, defaultTime string) string {
+	if time == "" {
+		return defaultTime
+	}
+	return time
+}
+
+func generatePositiveIntFromPointer(n *int, defaultN int) int {
+	if n == nil {
+		return defaultN
+	}
+	return *n
+}
+
 func generateLocation(path string, upstreamName string, cfgParams *ConfigParams) version2.Location {
 	loc := version2.Location{
 		Path:                 path,

pkg/apis/configuration/v1alpha1/types.go

@@ -25,10 +25,12 @@ 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"`
+	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"`
 }
 
 // Route defines a route.

pkg/apis/configuration/v1alpha1/zz_generated.deepcopy.go

@@ -57,6 +57,33 @@ func validateTLS(tls *v1alpha1.TLS, fieldPath *field.Path) field.ErrorList {
 	return validateSecretName(tls.Secret, fieldPath.Child("secret"))
 }
 
+func validatePositiveIntOrZero(n *int, fieldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	if n == nil {
+		return allErrs
+	}
+
+	if *n < 0 {
+		return append(allErrs, field.Invalid(fieldPath, n, "must be positive or zero"))
+	}
+
+	return allErrs
+}
+
+func validateTime(time string, fieldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+
+	if time == "" {
+		return allErrs
+	}
+
+	if _, err := configs.ParseTime(time); err != nil {
+		return append(allErrs, field.Invalid(fieldPath, time, err.Error()))
+	}
+
+	return allErrs
+}
+
 func validateUpstreamLBMethod(lBMethod string, fieldPath *field.Path, isPlus bool) field.ErrorList {
 	allErrs := field.ErrorList{}
 	if lBMethod == "" {
@@ -112,6 +139,8 @@ func validateUpstreams(upstreams []v1alpha1.Upstream, fieldPath *field.Path, isP
 
 		allErrs = append(allErrs, validateServiceName(u.Service, idxPath.Child("service"))...)
 		allErrs = append(allErrs, validateUpstreamLBMethod(u.LBMethod, idxPath.Child("lb-method"), isPlus)...)
+		allErrs = append(allErrs, validateTime(u.FailTimeout, idxPath.Child("fail-timeout"))...)
+		allErrs = append(allErrs, validatePositiveIntOrZero(u.MaxFails, idxPath.Child("max-fails"))...)
 
 		for _, msg := range validation.IsValidPortNum(int(u.Port)) {
 			allErrs = append(allErrs, field.Invalid(idxPath.Child("port"), u.Port, msg))

pkg/apis/configuration/validation/validation.go

@@ -1437,3 +1437,63 @@ func TestValidateUpstreamLBMethodFails(t *testing.T) {
 		}
 	}
 }
+
+func createPointerFromInt(n int) *int {
+	return &n
+}
+
+func TestValidatePositiveIntOrZero(t *testing.T) {
+	tests := []struct {
+		number *int
+		msg    string
+	}{
+		{
+			number: nil,
+			msg:    "valid (nil)",
+		},
+		{
+			number: createPointerFromInt(0),
+			msg:    "valid (0)",
+		},
+		{
+			number: createPointerFromInt(1),
+			msg:    "valid (1)",
+		},
+	}
+
+	for _, test := range tests {
+		allErrs := validatePositiveIntOrZero(test.number, field.NewPath("int-field"))
+
+		if len(allErrs) != 0 {
+			t.Errorf("validatePositiveInt returned errors for case: %v", test.msg)
+		}
+	}
+}
+
+func TestValidatePositiveIntOrZeroFails(t *testing.T) {
+	number := createPointerFromInt(-1)
+	allErrs := validatePositiveIntOrZero(number, field.NewPath("int-field"))
+
+	if len(allErrs) == 0 {
+		t.Error("validatePositiveInt returned no errors for case: invalid (-1)")
+	}
+
+}
+
+func TestValidateTime(t *testing.T) {
+	time := "1h 2s"
+	allErrs := validateTime(time, field.NewPath("time-field"))
+
+	if len(allErrs) != 0 {
+		t.Errorf("validateTime returned errors %v valid input %v", allErrs, time)
+	}
+}
+
+func TestValidateTimeFails(t *testing.T) {
+	time := "invalid"
+	allErrs := validateTime(time, field.NewPath("time-field"))
+
+	if len(allErrs) == 0 {
+		t.Errorf("validateTime returned no errors for invalid input %v", time)
+	}
+}