From f4abacc8f177dfe458202df6b699bb5857d2657b Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Mon, 7 Dec 2020 15:17:29 +0000 Subject: [PATCH 1/9] Ingress annotation validation earlier in k8s lifecycle Minimal validation was performed at the stage when errors can be show to the user in k8s status messages. Most was done later, when errors could only be written to the controller's log file. This change moves some of the validation to the earlier phase, more will follow. --- internal/configs/annotations.go | 26 +-- internal/configs/parsing_helpers.go | 64 +++++- internal/configs/parsing_helpers_test.go | 189 ++++++++++++++++++ internal/k8s/configuration.go | 5 +- internal/k8s/validation.go | 149 +++++++++++++- internal/k8s/validation_test.go | 189 +++++++++++++++++- pkg/apis/configuration/validation/common.go | 43 +++- .../configuration/validation/common_test.go | 44 +++- pkg/apis/configuration/validation/policy.go | 2 +- .../configuration/validation/virtualserver.go | 67 ++----- .../validation/virtualserver_test.go | 36 ---- 11 files changed, 678 insertions(+), 136 deletions(-) diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index f925a37129..4cf9de9789 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -93,29 +93,21 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool if err != nil { glog.Error(err) } - if isPlus { - cfgParams.HealthCheckEnabled = healthCheckEnabled - } else { - glog.Warning("Annotation 'nginx.com/health-checks' requires NGINX Plus") - } + cfgParams.HealthCheckEnabled = healthCheckEnabled } - if cfgParams.HealthCheckEnabled { - if healthCheckMandatory, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.com/health-checks-mandatory", ingEx.Ingress); exists { - if err != nil { - glog.Error(err) - } - cfgParams.HealthCheckMandatory = healthCheckMandatory + if healthCheckMandatory, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.com/health-checks-mandatory", ingEx.Ingress); exists { + if err != nil { + glog.Error(err) } + cfgParams.HealthCheckMandatory = healthCheckMandatory } - if cfgParams.HealthCheckMandatory { - if healthCheckQueue, exists, err := GetMapKeyAsInt64(ingEx.Ingress.Annotations, "nginx.com/health-checks-mandatory-queue", ingEx.Ingress); exists { - if err != nil { - glog.Error(err) - } - cfgParams.HealthCheckMandatoryQueue = healthCheckQueue + if healthCheckQueue, exists, err := GetMapKeyAsInt64(ingEx.Ingress.Annotations, "nginx.com/health-checks-mandatory-queue", ingEx.Ingress); exists { + if err != nil { + glog.Error(err) } + cfgParams.HealthCheckMandatoryQueue = healthCheckQueue } if slowStart, exists := ingEx.Ingress.Annotations["nginx.com/slow-start"]; exists { diff --git a/internal/configs/parsing_helpers.go b/internal/configs/parsing_helpers.go index 18a4ef7bef..aaf47a6790 100644 --- a/internal/configs/parsing_helpers.go +++ b/internal/configs/parsing_helpers.go @@ -21,7 +21,7 @@ type apiObject interface { // GetMapKeyAsBool searches the map for the given key and parses the key as bool. func GetMapKeyAsBool(m map[string]string, key string, context apiObject) (bool, bool, error) { if str, exists := m[key]; exists { - b, err := strconv.ParseBool(str) + b, err := ParseBool(str) if err != nil { return false, exists, fmt.Errorf("%s %v/%v '%s' contains invalid bool: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err) } @@ -35,7 +35,7 @@ func GetMapKeyAsBool(m map[string]string, key string, context apiObject) (bool, // GetMapKeyAsInt tries to find and parse a key in a map as int. func GetMapKeyAsInt(m map[string]string, key string, context apiObject) (int, bool, error) { if str, exists := m[key]; exists { - i, err := strconv.Atoi(str) + i, err := ParseInt(str) if err != nil { return 0, exists, fmt.Errorf("%s %v/%v '%s' contains invalid integer: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err) } @@ -49,7 +49,7 @@ func GetMapKeyAsInt(m map[string]string, key string, context apiObject) (int, bo // GetMapKeyAsInt64 tries to find and parse a key in a map as int64. func GetMapKeyAsInt64(m map[string]string, key string, context apiObject) (int64, bool, error) { if str, exists := m[key]; exists { - i, err := strconv.ParseInt(str, 10, 64) + i, err := ParseInt64(str) if err != nil { return 0, exists, fmt.Errorf("%s %v/%v '%s' contains invalid integer: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err) } @@ -63,7 +63,7 @@ func GetMapKeyAsInt64(m map[string]string, key string, context apiObject) (int64 // GetMapKeyAsUint64 tries to find and parse a key in a map as uint64. func GetMapKeyAsUint64(m map[string]string, key string, context apiObject, nonZero bool) (uint64, bool, error) { if str, exists := m[key]; exists { - i, err := strconv.ParseUint(str, 10, 64) + i, err := ParseUint64(str) if err != nil { return 0, exists, fmt.Errorf("%s %v/%v '%s' contains invalid uint64: %v, ignoring", context.GetObjectKind().GroupVersionKind().Kind, context.GetNamespace(), context.GetName(), key, err) } @@ -162,6 +162,23 @@ func validateHashLBMethod(method string) (string, error) { return "", fmt.Errorf("Invalid load balancing method: %q", method) } +// ParseBool ensures that the string value in the annotation is a valid bool +func ParseBool(s string) (bool, error) { + return strconv.ParseBool(s) +} + +func ParseInt(s string) (int, error) { + return strconv.Atoi(s) +} + +func ParseInt64(s string) (int64, error) { + return strconv.ParseInt(s, 10, 64) +} + +func ParseUint64(s string) (uint64, error) { + return strconv.ParseUint(s, 10, 64) +} + // http://nginx.org/en/docs/syntax.html var validTimeSuffixes = []string{ "ms", @@ -187,6 +204,45 @@ func ParseTime(s string) (string, error) { return "", errors.New("Invalid time string") } +// http://nginx.org/en/docs/syntax.html +const OffsetFmt = `\d+[kKmMgG]?` + +var offsetRegexp = regexp.MustCompile("^" + OffsetFmt + "$") + +func ParseOffset(s string) (string, error) { + s = strings.TrimSpace(s) + + if offsetRegexp.MatchString(s) { + return s, nil + } + return "", errors.New("Invalid offset string") +} + +// http://nginx.org/en/docs/syntax.html +const SizeFmt = `\d+[kKmM]?` + +var sizeRegexp = regexp.MustCompile("^" + SizeFmt + "$") + +func ParseSize(s string) (string, error) { + s = strings.TrimSpace(s) + + if sizeRegexp.MatchString(s) { + return s, nil + } + return "", errors.New("Invalid size string") +} + +var proxyBuffersRegexp = regexp.MustCompile(`^\d+ \d+[kKmM]?$`) + +func ParseProxyBuffers(s string) (string, error) { + s = strings.TrimSpace(s) + + if proxyBuffersRegexp.MatchString(s) { + return s, nil + } + return "", errors.New("must be a valid proxy buffers spec as specified here: https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffers, e.g. \"8 4k\"") +} + var threshEx = regexp.MustCompile(`high=([1-9]|[1-9][0-9]|100) low=([1-9]|[1-9][0-9]|100)\b`) var threshExR = regexp.MustCompile(`low=([1-9]|[1-9][0-9]|100) high=([1-9]|[1-9][0-9]|100)\b`) diff --git a/internal/configs/parsing_helpers_test.go b/internal/configs/parsing_helpers_test.go index 94846f47bf..ae0917caa2 100644 --- a/internal/configs/parsing_helpers_test.go +++ b/internal/configs/parsing_helpers_test.go @@ -383,6 +383,46 @@ func TestParseTime(t *testing.T) { } } +func TestParseOffset(t *testing.T) { + var testsWithValidInput = []string{"1", "2k", "2K", "3m", "3M", "4g", "4G"} + var invalidInput = []string{"-1", "", "blah"} + for _, test := range testsWithValidInput { + result, err := ParseOffset(test) + if err != nil { + t.Errorf("TestParseOffset(%q) returned an error for valid input", test) + } + if test != result { + t.Errorf("TestParseOffset(%q) returned %q expected %q", test, result, test) + } + } + for _, test := range invalidInput { + result, err := ParseOffset(test) + if err == nil { + t.Errorf("TestParseOffset(%q) didn't return error. Returned: %q", test, result) + } + } +} + +func TestParseSize(t *testing.T) { + var testsWithValidInput = []string{"1", "2k", "2K", "3m", "3M"} + var invalidInput = []string{"-1", "", "blah", "4g", "4G"} + for _, test := range testsWithValidInput { + result, err := ParseSize(test) + if err != nil { + t.Errorf("TestParseSize(%q) returned an error for valid input", test) + } + if test != result { + t.Errorf("TestParseSize(%q) returned %q expected %q", test, result, test) + } + } + for _, test := range invalidInput { + result, err := ParseSize(test) + if err == nil { + t.Errorf("TestParseSize(%q) didn't return error. Returned: %q", test, result) + } + } +} + func TestVerifyThresholds(t *testing.T) { validInput := []string{ "high=3 low=1", @@ -416,3 +456,152 @@ func TestVerifyThresholds(t *testing.T) { } } } + +func TestParseBool(t *testing.T) { + var testsWithValidInput = []struct { + input string + expected bool + }{ + {"0", false}, + {"1", true}, + {"true", true}, + {"false", false}, + } + + var invalidInput = []string{ + "", + "blablah", + "-100", + "-1", + } + + for _, test := range testsWithValidInput { + result, err := ParseBool(test.input) + if err != nil { + t.Errorf("TestParseBool(%q) returned an error for valid input", test.input) + } + + if result != test.expected { + t.Errorf("TestParseBool(%q) returned %t expected %t", test.input, result, test.expected) + } + } + + for _, input := range invalidInput { + _, err := ParseBool(input) + if err == nil { + t.Errorf("TestParseBool(%q) does not return an error for invalid input", input) + } + } +} + +func TestParseInt(t *testing.T) { + var testsWithValidInput = []struct { + input string + expected int + }{ + {"0", 0}, + {"1", 1}, + {"-100", -100}, + {"123456789", 123456789}, + } + + var invalidInput = []string{ + "", + "blablah", + "10000000000000000000000000000000000000000000000000000000000000000", + "1,000", + } + + for _, test := range testsWithValidInput { + result, err := ParseInt(test.input) + if err != nil { + t.Errorf("TestParseBool(%q) returned an error for valid input", test.input) + } + + if result != test.expected { + t.Errorf("TestParseBool(%q) returned %d expected %d", test.input, result, test.expected) + } + } + + for _, input := range invalidInput { + _, err := ParseInt(input) + if err == nil { + t.Errorf("TestParseBool(%q) does not return an error for invalid input", input) + } + } +} + +func TestParseInt64(t *testing.T) { + var testsWithValidInput = []struct { + input string + expected int64 + }{ + {"0", 0}, + {"1", 1}, + {"-100", -100}, + {"123456789", 123456789}, + } + + var invalidInput = []string{ + "", + "blablah", + "10000000000000000000000000000000000000000000000000000000000000000", + "1,000", + } + + for _, test := range testsWithValidInput { + result, err := ParseInt64(test.input) + if err != nil { + t.Errorf("TestParseBool(%q) returned an error for valid input", test.input) + } + + if result != test.expected { + t.Errorf("TestParseBool(%q) returned %d expected %d", test.input, result, test.expected) + } + } + + for _, input := range invalidInput { + _, err := ParseInt64(input) + if err == nil { + t.Errorf("TestParseBool(%q) does not return an error for invalid input", input) + } + } +} + +func TestParseUint64(t *testing.T) { + var testsWithValidInput = []struct { + input string + expected uint64 + }{ + {"0", 0}, + {"1", 1}, + {"100", 100}, + {"123456789", 123456789}, + } + + var invalidInput = []string{ + "", + "blablah", + "10000000000000000000000000000000000000000000000000000000000000000", + "1,000", + "-1023", + } + + for _, test := range testsWithValidInput { + result, err := ParseUint64(test.input) + if err != nil { + t.Errorf("TestParseBool(%q) returned an error for valid input", test.input) + } + + if result != test.expected { + t.Errorf("TestParseBool(%q) returned %d expected %d", test.input, result, test.expected) + } + } + + for _, input := range invalidInput { + _, err := ParseUint64(input) + if err == nil { + t.Errorf("TestParseBool(%q) does not return an error for invalid input", input) + } + } +} diff --git a/internal/k8s/configuration.go b/internal/k8s/configuration.go index 621256dbf7..971e65ef06 100644 --- a/internal/k8s/configuration.go +++ b/internal/k8s/configuration.go @@ -307,6 +307,8 @@ type Configuration struct { appPolicyReferenceChecker *appProtectResourceReferenceChecker appLogConfReferenceChecker *appProtectResourceReferenceChecker + isPlus bool + lock sync.RWMutex } @@ -325,6 +327,7 @@ func NewConfiguration(hasCorrectIngressClass func(interface{}) bool, isPlus bool policyReferenceChecker: newPolicyReferenceChecker(), appPolicyReferenceChecker: newAppProtectResourceReferenceChecker(configs.AppProtectPolicyAnnotation), appLogConfReferenceChecker: newAppProtectResourceReferenceChecker(configs.AppProtectLogConfAnnotation), + isPlus: isPlus, } } @@ -339,7 +342,7 @@ func (c *Configuration) AddOrUpdateIngress(ing *networking.Ingress) ([]ResourceC if !c.hasCorrectIngressClass(ing) { delete(c.ingresses, key) } else { - validationError = validateIngress(ing).ToAggregate() + validationError = validateIngress(ing, c.isPlus).ToAggregate() if validationError != nil { delete(c.ingresses, key) } else { diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 1b5a7358a8..ffbf13c19c 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -1,19 +1,66 @@ package k8s import ( + "errors" + "fmt" + + "github.com/nginxinc/kubernetes-ingress/internal/configs" networking "k8s.io/api/networking/v1beta1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" ) -const mergeableIngressTypeAnnotationKey = "nginx.org/mergeable-ingress-type" +const ( + mergeableIngressTypeAnnotation = "nginx.org/mergeable-ingress-type" + lbMethodAnnotation = "nginx.org/lb-method" + healthChecksAnnotation = "nginx.com/health-checks" + healthChecksMandatoryAnnotation = "nginx.com/health-checks-mandatory" + healthChecksMandatoryQueueAnnotation = "nginx.com/health-checks-mandatory-queue" +) + +type annotationValidationContext struct { + annotations map[string]string + name string + value string + isPlus bool + fieldPath *field.Path +} + +type annotationValidationFunc func(context *annotationValidationContext) field.ErrorList +type validatorFunc func(val string) error + +var annotationValidations = map[string][]annotationValidationFunc{ + mergeableIngressTypeAnnotation: { + validateRequiredAnnotation, + validateMergeableIngressTypeAnnotation, + }, + lbMethodAnnotation: { + validateRequiredAnnotation, + validateLBMethodAnnotation, + }, + healthChecksAnnotation: { + validateRequiredAnnotation, + validatePlusOnlyAnnotation, + validateBoolAnnotation, + }, + healthChecksMandatoryAnnotation: { + validateRelatedAnnotation(healthChecksAnnotation, validateIsTrue), + validateRequiredAnnotation, + validateBoolAnnotation, + }, + healthChecksMandatoryQueueAnnotation: { + validateRelatedAnnotation(healthChecksMandatoryAnnotation, validateIsTrue), + validateRequiredAnnotation, + validateNonNegativeIntAnnotation, + }, +} // validateIngress validate an Ingress resource with rules that our Ingress Controller enforces. // Note that the full validation of Ingress resources is done by Kubernetes. -func validateIngress(ing *networking.Ingress) field.ErrorList { +func validateIngress(ing *networking.Ingress, isPlus bool) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, validateIngressAnnotations(ing.Annotations, field.NewPath("annotations"))...) + allErrs = append(allErrs, validateIngressAnnotations(ing.Annotations, isPlus, field.NewPath("annotations"))...) allErrs = append(allErrs, validateIngressSpec(&ing.Spec, field.NewPath("spec"))...) @@ -26,30 +73,110 @@ func validateIngress(ing *networking.Ingress) field.ErrorList { return allErrs } -func validateIngressAnnotations(annotations map[string]string, fieldPath *field.Path) field.ErrorList { +func validateIngressAnnotations(annotations map[string]string, isPlus bool, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - if value, exists := annotations[mergeableIngressTypeAnnotationKey]; exists { - allErrs = append(allErrs, validateMergeableIngressTypeAnnotation(value, fieldPath.Child(mergeableIngressTypeAnnotationKey))...) + for name, validationFuncs := range annotationValidations { + if value, exists := annotations[name]; exists { + for _, validationFunc := range validationFuncs { + valErrors := validationFunc(&annotationValidationContext{ + annotations: annotations, + name: name, + value: value, + isPlus: isPlus, + fieldPath: fieldPath.Child(name), + }) + if len(valErrors) > 0 { + allErrs = append(allErrs, valErrors...) + break + } + } + } + } + + return allErrs +} + +func validateRelatedAnnotation(name string, validator validatorFunc) annotationValidationFunc { + return func(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + val, exists := context.annotations[name] + if !exists { + return append(allErrs, field.Forbidden(context.fieldPath, fmt.Sprintf("related annotation %s: must be set", name))) + } + + if err := validator(val); err != nil { + return append(allErrs, field.Forbidden(context.fieldPath, fmt.Sprintf("related annotation %s: %s", name, err.Error()))) + } + return allErrs + } +} + +func validateMergeableIngressTypeAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if context.value != "master" && context.value != "minion" { + return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be one of: 'master' or 'minion'")) } + return allErrs +} +func validateLBMethodAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if context.isPlus { + if _, err := configs.ParseLBMethodForPlus(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, err.Error())) + } + } else { + if _, err := configs.ParseLBMethod(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, err.Error())) + } + } return allErrs } -func validateMergeableIngressTypeAnnotation(value string, fieldPath *field.Path) field.ErrorList { +func validateRequiredAnnotation(context *annotationValidationContext) field.ErrorList { allErrs := field.ErrorList{} + if context.value == "" { + return append(allErrs, field.Required(context.fieldPath, "")) + } + return allErrs +} - if value == "" { - return append(allErrs, field.Required(fieldPath, "")) +func validatePlusOnlyAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if !context.isPlus { + return append(allErrs, field.Forbidden(context.fieldPath, "annotation requires NGINX Plus")) } + return allErrs +} - if value != "master" && value != "minion" { - return append(allErrs, field.Invalid(fieldPath, value, "must be one of: 'master' or 'minion'")) +func validateBoolAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if _, err := configs.ParseBool(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a valid boolean")) } + return allErrs +} +func validateNonNegativeIntAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if _, err := configs.ParseUint64(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a non-negative integer")) + } return allErrs } +func validateIsTrue(v string) error { + b, err := configs.ParseBool(v) + if err != nil { + return err + } + if !b { + return errors.New("must be true") + } + return nil +} + func validateIngressSpec(spec *networking.IngressSpec, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 9e7d43afd4..3cb2b1977f 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -14,6 +14,7 @@ import ( func TestValidateIngress(t *testing.T) { tests := []struct { ing *networking.Ingress + isPlus bool expectedErrors []string msg string }{ @@ -27,6 +28,7 @@ func TestValidateIngress(t *testing.T) { }, }, }, + isPlus: false, expectedErrors: nil, msg: "valid input", }, @@ -45,6 +47,7 @@ func TestValidateIngress(t *testing.T) { }, }, }, + isPlus: false, expectedErrors: []string{ `annotations.nginx.org/mergeable-ingress-type: Invalid value: "invalid": must be one of: 'master' or 'minion'`, "spec.rules[0].host: Required value", @@ -75,6 +78,7 @@ func TestValidateIngress(t *testing.T) { }, }, }, + isPlus: false, expectedErrors: []string{ "spec.rules[0].http.paths: Too many: 1: must have at most 0 items", }, @@ -96,6 +100,7 @@ func TestValidateIngress(t *testing.T) { }, }, }, + isPlus: false, expectedErrors: []string{ "spec.rules[0].http.paths: Required value: must include at least one path", }, @@ -104,7 +109,7 @@ func TestValidateIngress(t *testing.T) { } for _, test := range tests { - allErrs := validateIngress(test.ing) + allErrs := validateIngress(test.ing, test.isPlus) assertion := assertErrors("validateIngress()", test.msg, allErrs, test.expectedErrors) if assertion != "" { t.Error(assertion) @@ -115,18 +120,22 @@ func TestValidateIngress(t *testing.T) { func TestValidateIngressAnnotations(t *testing.T) { tests := []struct { annotations map[string]string + isPlus bool expectedErrors []string msg string }{ { annotations: map[string]string{}, + isPlus: true, expectedErrors: nil, msg: "valid input", }, + { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "master", }, + isPlus: false, expectedErrors: nil, msg: "valid input with master annotation", }, @@ -134,6 +143,7 @@ func TestValidateIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "minion", }, + isPlus: false, expectedErrors: nil, msg: "valid input with minion annotation", }, @@ -141,6 +151,7 @@ func TestValidateIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "", }, + isPlus: false, expectedErrors: []string{ "annotations.nginx.org/mergeable-ingress-type: Required value", }, @@ -150,19 +161,185 @@ func TestValidateIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "abc", }, + isPlus: false, expectedErrors: []string{ `annotations.nginx.org/mergeable-ingress-type: Invalid value: "abc": must be one of: 'master' or 'minion'`, }, msg: "invalid mergeable type annotation 2", }, + + { + annotations: map[string]string{ + "nginx.org/lb-method": "least_time header", + }, + isPlus: true, + expectedErrors: nil, + msg: "valid nginx.org/lb-method annotation, nginx plus", + }, + { + annotations: map[string]string{ + "nginx.org/lb-method": "random", + }, + isPlus: false, + expectedErrors: nil, + msg: "valid nginx.org/lb-method annotation, nginx normal", + }, + { + annotations: map[string]string{ + "nginx.org/lb-method": "invalid_method", + }, + isPlus: true, + expectedErrors: []string{ + "annotations.nginx.org/lb-method: Invalid value: \"invalid_method\": Invalid load balancing method: \"invalid_method\"", + }, + msg: "invalid nginx.org/lb-method annotation, nginx", + }, + { + annotations: map[string]string{ + "nginx.org/lb-method": "least_time header", + }, + isPlus: false, + expectedErrors: []string{ + "annotations.nginx.org/lb-method: Invalid value: \"least_time header\": Invalid load balancing method: \"least_time header\"", + }, + msg: "invalid nginx.org/lb-method annotation, nginx plus", + }, + + { + annotations: map[string]string{ + "nginx.com/health-checks": "true", + }, + isPlus: true, + expectedErrors: nil, + msg: "valid nginx.com/health-checks annotation", + }, + { + annotations: map[string]string{ + "nginx.com/health-checks": "true", + }, + isPlus: false, + expectedErrors: []string{ + "annotations.nginx.com/health-checks: Forbidden: annotation requires NGINX Plus", + }, + msg: "invalid nginx.com/health-checks annotation, nginx plus only", + }, + { + annotations: map[string]string{ + "nginx.com/health-checks": "not_a_boolean", + }, + isPlus: true, + expectedErrors: []string{ + "annotations.nginx.com/health-checks: Invalid value: \"not_a_boolean\": must be a valid boolean", + }, + msg: "invalid nginx.com/health-checks annotation", + }, + + { + annotations: map[string]string{ + "nginx.com/health-checks": "not_a_boolean", + }, + isPlus: false, + expectedErrors: []string{ + "annotations.nginx.com/health-checks: Forbidden: annotation requires NGINX Plus", + }, + msg: "invalid nginx.com/health-checks annotation", + }, + { + annotations: map[string]string{ + "nginx.com/health-checks": "true", + "nginx.com/health-checks-mandatory": "true", + }, + isPlus: true, + expectedErrors: nil, + msg: "valid nginx.com/health-checks-mandatory annotation", + }, + { + annotations: map[string]string{ + "nginx.com/health-checks": "true", + "nginx.com/health-checks-mandatory": "not_a_boolean", + }, + isPlus: true, + expectedErrors: []string{ + "annotations.nginx.com/health-checks-mandatory: Invalid value: \"not_a_boolean\": must be a valid boolean", + }, + msg: "invalid nginx.com/health-checks-mandatory, must be a boolean", + }, + { + annotations: map[string]string{ + "nginx.com/health-checks-mandatory": "true", + }, + isPlus: true, + expectedErrors: []string{ + "annotations.nginx.com/health-checks-mandatory: Forbidden: related annotation nginx.com/health-checks: must be set", + }, + msg: "invalid nginx.com/health-checks-mandatory, related annotation nginx.com/health-checks not set", + }, + { + annotations: map[string]string{ + "nginx.com/health-checks": "false", + "nginx.com/health-checks-mandatory": "true", + }, + isPlus: true, + expectedErrors: []string{ + "annotations.nginx.com/health-checks-mandatory: Forbidden: related annotation nginx.com/health-checks: must be true", + }, + msg: "invalid nginx.com/health-checks-mandatory nginx.com/health-checks is not true", + }, + + { + annotations: map[string]string{ + "nginx.com/health-checks": "true", + "nginx.com/health-checks-mandatory": "true", + "nginx.com/health-checks-mandatory-queue": "5", + }, + isPlus: true, + expectedErrors: nil, + msg: "valid nginx.com/health-checks-mandatory-queue annotation", + }, + { + annotations: map[string]string{ + "nginx.com/health-checks": "true", + "nginx.com/health-checks-mandatory": "true", + "nginx.com/health-checks-mandatory-queue": "not_a_number", + }, + isPlus: true, + expectedErrors: []string{ + "annotations.nginx.com/health-checks-mandatory-queue: Invalid value: \"not_a_number\": must be a non-negative integer", + }, + msg: "invalid nginx.com/health-checks-mandatory-queue, must be a boolean", + }, + { + annotations: map[string]string{ + "nginx.com/health-checks-mandatory-queue": "5", + }, + isPlus: true, + expectedErrors: []string{ + "annotations.nginx.com/health-checks-mandatory-queue: Forbidden: related annotation nginx.com/health-checks-mandatory: must be set", + }, + msg: "invalid nginx.com/health-checks-mandatory-queue, related annotation nginx.com/health-checks-mandatory not set", + }, + { + annotations: map[string]string{ + "nginx.com/health-checks": "true", + "nginx.com/health-checks-mandatory": "false", + "nginx.com/health-checks-mandatory-queue": "5", + }, + isPlus: true, + expectedErrors: []string{ + "annotations.nginx.com/health-checks-mandatory-queue: Forbidden: related annotation nginx.com/health-checks-mandatory: must be true", + }, + msg: "invalid nginx.com/health-checks-mandatory-queue nginx.com/health-checks-mandatory is not true", + }, } for _, test := range tests { - allErrs := validateIngressAnnotations(test.annotations, field.NewPath("annotations")) - assertion := assertErrors("validateIngressAnnotations()", test.msg, allErrs, test.expectedErrors) - if assertion != "" { - t.Error(assertion) - } + t.Run(test.msg, func(t *testing.T) { + allErrs := validateIngressAnnotations(test.annotations, test.isPlus, field.NewPath("annotations")) + assertion := assertErrors("validateIngressAnnotations()", test.msg, allErrs, test.expectedErrors) + if assertion != "" { + t.Error(assertion) + } + }) } } diff --git a/pkg/apis/configuration/validation/common.go b/pkg/apis/configuration/validation/common.go index 8d03e6dd2c..7fbf552e52 100644 --- a/pkg/apis/configuration/validation/common.go +++ b/pkg/apis/configuration/validation/common.go @@ -5,6 +5,7 @@ import ( "regexp" "strings" + "github.com/nginxinc/kubernetes-ingress/internal/configs" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" ) @@ -135,20 +136,52 @@ func validateStringWithVariables(str string, fieldPath *field.Path, specialVars return allErrs } -const sizeFmt = `\d+[kKmM]?` +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 +} + +// http://nginx.org/en/docs/syntax.html +const offsetErrMsg = "must consist of numeric characters followed by a valid size suffix. 'k|K|m|M|g|G" + +func ValidateOffset(offset string, fieldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + if offset == "" { + return allErrs + } + + if _, err := configs.ParseOffset(offset); err != nil { + msg := validation.RegexError(offsetErrMsg, configs.OffsetFmt, "16", "32k", "64M", "2G") + return append(allErrs, field.Invalid(fieldPath, offset, msg)) + } + + return allErrs +} + +// http://nginx.org/en/docs/syntax.html const sizeErrMsg = "must consist of numeric characters followed by a valid size suffix. 'k|K|m|M" -var sizeRegexp = regexp.MustCompile("^" + sizeFmt + "$") +var sizeRegexp = regexp.MustCompile("^" + configs.SizeFmt + "$") -func validateSize(size string, fieldPath *field.Path) field.ErrorList { +func ValidateSize(size string, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if size == "" { return allErrs } - if !sizeRegexp.MatchString(size) { - msg := validation.RegexError(sizeErrMsg, sizeFmt, "16", "32k", "64M") + if _, err := configs.ParseSize(size); err != nil { + msg := validation.RegexError(sizeErrMsg, configs.SizeFmt, "16", "32k", "64M") return append(allErrs, field.Invalid(fieldPath, size, msg)) } return allErrs diff --git a/pkg/apis/configuration/validation/common_test.go b/pkg/apis/configuration/validation/common_test.go index dcaa0a05c5..79ab3b43c9 100644 --- a/pkg/apis/configuration/validation/common_test.go +++ b/pkg/apis/configuration/validation/common_test.go @@ -273,17 +273,53 @@ func TestValidateStringWithVariablesFail(t *testing.T) { func TestValidateSize(t *testing.T) { var validInput = []string{"", "4k", "8K", "16m", "32M"} for _, test := range validInput { - allErrs := validateSize(test, field.NewPath("size-field")) + allErrs := ValidateSize(test, field.NewPath("size-field")) if len(allErrs) != 0 { - t.Errorf("validateSize(%q) returned an error for valid input", test) + t.Errorf("ValidateSize(%q) returned an error for valid input", test) } } var invalidInput = []string{"55mm", "2mG", "6kb", "-5k", "1L", "5G"} for _, test := range invalidInput { - allErrs := validateSize(test, field.NewPath("size-field")) + allErrs := ValidateSize(test, field.NewPath("size-field")) if len(allErrs) == 0 { - t.Errorf("validateSize(%q) didn't return error for invalid input.", test) + t.Errorf("ValidateSize(%q) didn't return error for invalid input.", test) + } + } +} + +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) + } +} + +func TestValidateOffset(t *testing.T) { + var validInput = []string{"", "1", "10k", "11m", "1K", "100M", "5G"} + for _, test := range validInput { + 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", "5Gb"} + for _, test := range invalidInput { + allErrs := ValidateOffset(test, field.NewPath("offset-field")) + if len(allErrs) == 0 { + t.Errorf("ValidateOffset(%q) didn't return error for invalid input.", test) } } } diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go index 92f79d1eed..d7492d6f30 100644 --- a/pkg/apis/configuration/validation/policy.go +++ b/pkg/apis/configuration/validation/policy.go @@ -221,7 +221,7 @@ func validateRateLimitZoneSize(zoneSize string, fieldPath *field.Path) field.Err return append(allErrs, field.Required(fieldPath, "")) } - allErrs = append(allErrs, validateSize(zoneSize, fieldPath)...) + allErrs = append(allErrs, ValidateSize(zoneSize, fieldPath)...) kbZoneSize := strings.TrimSuffix(strings.ToLower(zoneSize), "k") kbZoneSizeNum, err := strconv.Atoi(kbZoneSize) diff --git a/pkg/apis/configuration/validation/virtualserver.go b/pkg/apis/configuration/validation/virtualserver.go index 2fc6751b39..f8c165e9bc 100644 --- a/pkg/apis/configuration/validation/virtualserver.go +++ b/pkg/apis/configuration/validation/virtualserver.go @@ -172,41 +172,6 @@ func validatePositiveIntOrZeroFromPointer(n *int, fieldPath *field.Path) field.E 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 -} - -// http://nginx.org/en/docs/syntax.html -const offsetFmt = `\d+[kKmMgG]?` -const offsetErrMsg = "must consist of numeric characters followed by a valid size suffix. 'k|K|m|M|g|G" - -var offsetRegexp = regexp.MustCompile("^" + offsetFmt + "$") - -func validateOffset(offset string, fieldPath *field.Path) field.ErrorList { - allErrs := field.ErrorList{} - - if offset == "" { - return allErrs - } - - if !offsetRegexp.MatchString(offset) { - msg := validation.RegexError(offsetErrMsg, offsetFmt, "16", "32k", "64M") - return append(allErrs, field.Invalid(fieldPath, offset, msg)) - } - - return allErrs -} - func validateBuffer(buff *v1.UpstreamBuffers, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} @@ -221,7 +186,7 @@ func validateBuffer(buff *v1.UpstreamBuffers, fieldPath *field.Path) field.Error if buff.Size == "" { allErrs = append(allErrs, field.Required(fieldPath.Child("size"), "cannot be empty")) } else { - allErrs = append(allErrs, validateSize(buff.Size, fieldPath.Child("size"))...) + allErrs = append(allErrs, ValidateSize(buff.Size, fieldPath.Child("size"))...) } return allErrs @@ -259,13 +224,13 @@ func validateUpstreamHealthCheck(hc *v1.HealthCheck, fieldPath *field.Path) fiel allErrs = append(allErrs, validatePath(hc.Path, fieldPath.Child("path"))...) } - allErrs = append(allErrs, validateTime(hc.Interval, fieldPath.Child("interval"))...) - allErrs = append(allErrs, validateTime(hc.Jitter, fieldPath.Child("jitter"))...) + allErrs = append(allErrs, ValidateTime(hc.Interval, fieldPath.Child("interval"))...) + allErrs = append(allErrs, ValidateTime(hc.Jitter, fieldPath.Child("jitter"))...) allErrs = append(allErrs, validatePositiveIntOrZero(hc.Fails, fieldPath.Child("fails"))...) allErrs = append(allErrs, validatePositiveIntOrZero(hc.Passes, fieldPath.Child("passes"))...) - allErrs = append(allErrs, validateTime(hc.ConnectTimeout, fieldPath.Child("connect-timeout"))...) - allErrs = append(allErrs, validateTime(hc.ReadTimeout, fieldPath.Child("read-timeout"))...) - allErrs = append(allErrs, validateTime(hc.SendTimeout, fieldPath.Child("send-timeout"))...) + allErrs = append(allErrs, ValidateTime(hc.ConnectTimeout, fieldPath.Child("connect-timeout"))...) + allErrs = append(allErrs, ValidateTime(hc.ReadTimeout, fieldPath.Child("read-timeout"))...) + allErrs = append(allErrs, ValidateTime(hc.SendTimeout, fieldPath.Child("send-timeout"))...) allErrs = append(allErrs, validateStatusMatch(hc.StatusMatch, fieldPath.Child("statusMatch"))...) for i, header := range hc.Headers { @@ -302,7 +267,7 @@ func validateSessionCookie(sc *v1.SessionCookie, fieldPath *field.Path) field.Er } if sc.Expires != "max" { - allErrs = append(allErrs, validateTime(sc.Expires, fieldPath.Child("expires"))...) + allErrs = append(allErrs, ValidateTime(sc.Expires, fieldPath.Child("expires"))...) } if sc.Domain != "" { @@ -447,22 +412,22 @@ func (vsv *VirtualServerValidator) validateUpstreams(upstreams []v1.Upstream, fi allErrs = append(allErrs, validateServiceName(u.Service, idxPath.Child("service"))...) allErrs = append(allErrs, validateLabels(u.Subselector, idxPath.Child("subselector"))...) - 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, 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.ProxyNextUpstreamTimeout, idxPath.Child("next-upstream-timeout"))...) + allErrs = append(allErrs, ValidateTime(u.ProxyNextUpstreamTimeout, idxPath.Child("next-upstream-timeout"))...) allErrs = append(allErrs, validatePositiveIntOrZeroFromPointer(&u.ProxyNextUpstreamTries, idxPath.Child("next-upstream-tries"))...) allErrs = append(allErrs, validateUpstreamLBMethod(u.LBMethod, idxPath.Child("lb-method"), vsv.isPlus)...) - allErrs = append(allErrs, validateTime(u.FailTimeout, idxPath.Child("fail-timeout"))...) + allErrs = append(allErrs, ValidateTime(u.FailTimeout, idxPath.Child("fail-timeout"))...) allErrs = append(allErrs, validatePositiveIntOrZeroFromPointer(u.MaxFails, idxPath.Child("max-fails"))...) allErrs = append(allErrs, validatePositiveIntOrZeroFromPointer(u.Keepalive, idxPath.Child("keepalive"))...) allErrs = append(allErrs, validatePositiveIntOrZeroFromPointer(u.MaxConns, idxPath.Child("max-conns"))...) - allErrs = append(allErrs, validateOffset(u.ClientMaxBodySize, idxPath.Child("client-max-body-size"))...) + allErrs = append(allErrs, ValidateOffset(u.ClientMaxBodySize, idxPath.Child("client-max-body-size"))...) allErrs = append(allErrs, validateUpstreamHealthCheck(u.HealthCheck, idxPath.Child("healthCheck"))...) - allErrs = append(allErrs, validateTime(u.SlowStart, idxPath.Child("slow-start"))...) + allErrs = append(allErrs, ValidateTime(u.SlowStart, idxPath.Child("slow-start"))...) allErrs = append(allErrs, validateBuffer(u.ProxyBuffers, idxPath.Child("buffers"))...) - allErrs = append(allErrs, validateSize(u.ProxyBufferSize, idxPath.Child("buffer-size"))...) + allErrs = append(allErrs, ValidateSize(u.ProxyBufferSize, idxPath.Child("buffer-size"))...) allErrs = append(allErrs, validateQueue(u.Queue, idxPath.Child("queue"))...) allErrs = append(allErrs, validateSessionCookie(u.SessionCookie, idxPath.Child("sessionCookie"))...) @@ -1452,7 +1417,7 @@ func validateQueue(queue *v1.UpstreamQueue, fieldPath *field.Path) field.ErrorLi return allErrs } - allErrs = append(allErrs, validateTime(queue.Timeout, fieldPath.Child("timeout"))...) + allErrs = append(allErrs, ValidateTime(queue.Timeout, fieldPath.Child("timeout"))...) if queue.Size <= 0 { allErrs = append(allErrs, field.Required(fieldPath.Child("size"), "must be positive")) } diff --git a/pkg/apis/configuration/validation/virtualserver_test.go b/pkg/apis/configuration/validation/virtualserver_test.go index de9daba1af..828a688320 100644 --- a/pkg/apis/configuration/validation/virtualserver_test.go +++ b/pkg/apis/configuration/validation/virtualserver_test.go @@ -2252,33 +2252,6 @@ func TestValidatePositiveIntOrZeroFails(t *testing.T) { } } -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 TestValidateOffset(t *testing.T) { - var validInput = []string{"", "1", "10k", "11m", "1K", "100M", "5G"} - for _, test := range validInput { - 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", "5Gb"} - for _, test := range invalidInput { - allErrs := validateOffset(test, field.NewPath("offset-field")) - if len(allErrs) == 0 { - t.Errorf("validateOffset(%q) didn't return error for invalid input.", test) - } - } -} - func TestValidateBuffer(t *testing.T) { validbuff := &v1.UpstreamBuffers{Number: 8, Size: "8k"} allErrs := validateBuffer(validbuff, field.NewPath("buffers-field")) @@ -2309,15 +2282,6 @@ func TestValidateBuffer(t *testing.T) { } } -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) - } -} - func TestValidateUpstreamHealthCheck(t *testing.T) { hc := &v1.HealthCheck{ Enable: true, From 8de2ac564151b2daaaecf23e6fbed9c592941e39 Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Mon, 7 Dec 2020 16:53:32 +0000 Subject: [PATCH 2/9] Add a comment describing how annotation validations are run --- internal/k8s/validation.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index ffbf13c19c..8fd1897fb7 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -29,6 +29,8 @@ type annotationValidationContext struct { type annotationValidationFunc func(context *annotationValidationContext) field.ErrorList type validatorFunc func(val string) error +// annotationValidations defines the various validations which will be applied in order to each ingress annotation. +// If any specified validation fails, the remaining validations for that annotation will not be run. var annotationValidations = map[string][]annotationValidationFunc{ mergeableIngressTypeAnnotation: { validateRequiredAnnotation, From 1e79a56047bd5e261f732141124dc1033523999e Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Tue, 8 Dec 2020 10:07:41 +0000 Subject: [PATCH 3/9] Correct output messages from parsing helper tests --- internal/configs/parsing_helpers_test.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/configs/parsing_helpers_test.go b/internal/configs/parsing_helpers_test.go index ae0917caa2..408fcb4da7 100644 --- a/internal/configs/parsing_helpers_test.go +++ b/internal/configs/parsing_helpers_test.go @@ -515,18 +515,18 @@ func TestParseInt(t *testing.T) { for _, test := range testsWithValidInput { result, err := ParseInt(test.input) if err != nil { - t.Errorf("TestParseBool(%q) returned an error for valid input", test.input) + t.Errorf("TestParseInt(%q) returned an error for valid input", test.input) } if result != test.expected { - t.Errorf("TestParseBool(%q) returned %d expected %d", test.input, result, test.expected) + t.Errorf("TestParseInt(%q) returned %d expected %d", test.input, result, test.expected) } } for _, input := range invalidInput { _, err := ParseInt(input) if err == nil { - t.Errorf("TestParseBool(%q) does not return an error for invalid input", input) + t.Errorf("TestParseInt(%q) does not return an error for invalid input", input) } } } @@ -552,18 +552,18 @@ func TestParseInt64(t *testing.T) { for _, test := range testsWithValidInput { result, err := ParseInt64(test.input) if err != nil { - t.Errorf("TestParseBool(%q) returned an error for valid input", test.input) + t.Errorf("TestParseInt64(%q) returned an error for valid input", test.input) } if result != test.expected { - t.Errorf("TestParseBool(%q) returned %d expected %d", test.input, result, test.expected) + t.Errorf("TestParseInt64(%q) returned %d expected %d", test.input, result, test.expected) } } for _, input := range invalidInput { _, err := ParseInt64(input) if err == nil { - t.Errorf("TestParseBool(%q) does not return an error for invalid input", input) + t.Errorf("TestParseInt64(%q) does not return an error for invalid input", input) } } } @@ -590,18 +590,18 @@ func TestParseUint64(t *testing.T) { for _, test := range testsWithValidInput { result, err := ParseUint64(test.input) if err != nil { - t.Errorf("TestParseBool(%q) returned an error for valid input", test.input) + t.Errorf("TestParseUint64(%q) returned an error for valid input", test.input) } if result != test.expected { - t.Errorf("TestParseBool(%q) returned %d expected %d", test.input, result, test.expected) + t.Errorf("TestParseUint64(%q) returned %d expected %d", test.input, result, test.expected) } } for _, input := range invalidInput { _, err := ParseUint64(input) if err == nil { - t.Errorf("TestParseBool(%q) does not return an error for invalid input", input) + t.Errorf("TestParseUint64(%q) does not return an error for invalid input", input) } } } From 0a5c8e24fcfa43440b44c26f3e16a63e4e7c5f41 Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Tue, 8 Dec 2020 11:43:27 +0000 Subject: [PATCH 4/9] Sort ingress annotation validation errors alphabetically --- internal/k8s/validation.go | 93 +++++++++++++++++++-------------- internal/k8s/validation_test.go | 15 +++++- 2 files changed, 68 insertions(+), 40 deletions(-) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 8fd1897fb7..fb1887f3f5 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -3,6 +3,7 @@ package k8s import ( "errors" "fmt" + "sort" "github.com/nginxinc/kubernetes-ingress/internal/configs" networking "k8s.io/api/networking/v1beta1" @@ -29,32 +30,44 @@ type annotationValidationContext struct { type annotationValidationFunc func(context *annotationValidationContext) field.ErrorList type validatorFunc func(val string) error -// annotationValidations defines the various validations which will be applied in order to each ingress annotation. -// If any specified validation fails, the remaining validations for that annotation will not be run. -var annotationValidations = map[string][]annotationValidationFunc{ - mergeableIngressTypeAnnotation: { - validateRequiredAnnotation, - validateMergeableIngressTypeAnnotation, - }, - lbMethodAnnotation: { - validateRequiredAnnotation, - validateLBMethodAnnotation, - }, - healthChecksAnnotation: { - validateRequiredAnnotation, - validatePlusOnlyAnnotation, - validateBoolAnnotation, - }, - healthChecksMandatoryAnnotation: { - validateRelatedAnnotation(healthChecksAnnotation, validateIsTrue), - validateRequiredAnnotation, - validateBoolAnnotation, - }, - healthChecksMandatoryQueueAnnotation: { - validateRelatedAnnotation(healthChecksMandatoryAnnotation, validateIsTrue), - validateRequiredAnnotation, - validateNonNegativeIntAnnotation, - }, +var ( + // annotationValidations defines the various validations which will be applied in order to each ingress annotation. + // If any specified validation fails, the remaining validations for that annotation will not be run. + annotationValidations = map[string][]annotationValidationFunc{ + mergeableIngressTypeAnnotation: { + validateRequiredAnnotation, + validateMergeableIngressTypeAnnotation, + }, + lbMethodAnnotation: { + validateRequiredAnnotation, + validateLBMethodAnnotation, + }, + healthChecksAnnotation: { + validateRequiredAnnotation, + validatePlusOnlyAnnotation, + validateBoolAnnotation, + }, + healthChecksMandatoryAnnotation: { + validateRelatedAnnotation(healthChecksAnnotation, validateIsTrue), + validateRequiredAnnotation, + validateBoolAnnotation, + }, + healthChecksMandatoryQueueAnnotation: { + validateRelatedAnnotation(healthChecksMandatoryAnnotation, validateIsTrue), + validateRequiredAnnotation, + validateNonNegativeIntAnnotation, + }, + } + annotationNames = sortedAnnotationNames(annotationValidations) +) + +func sortedAnnotationNames(annotationValidations map[string][]annotationValidationFunc) []string { + sortedNames := make([]string, 0) + for annotationName := range annotationValidations { + sortedNames = append(sortedNames, annotationName) + } + sort.Strings(sortedNames) + return sortedNames } // validateIngress validate an Ingress resource with rules that our Ingress Controller enforces. @@ -78,19 +91,21 @@ func validateIngress(ing *networking.Ingress, isPlus bool) field.ErrorList { func validateIngressAnnotations(annotations map[string]string, isPlus bool, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - for name, validationFuncs := range annotationValidations { - if value, exists := annotations[name]; exists { - for _, validationFunc := range validationFuncs { - valErrors := validationFunc(&annotationValidationContext{ - annotations: annotations, - name: name, - value: value, - isPlus: isPlus, - fieldPath: fieldPath.Child(name), - }) - if len(valErrors) > 0 { - allErrs = append(allErrs, valErrors...) - break + for _, name := range annotationNames { + if value, nameExists := annotations[name]; nameExists { + if validationFuncs, validationExists := annotationValidations[name]; validationExists { + for _, validationFunc := range validationFuncs { + valErrors := validationFunc(&annotationValidationContext{ + annotations: annotations, + name: name, + value: value, + isPlus: isPlus, + fieldPath: fieldPath.Child(name), + }) + if len(valErrors) > 0 { + allErrs = append(allErrs, valErrors...) + break + } } } } diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 3cb2b1977f..e50c81688f 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -128,7 +128,20 @@ func TestValidateIngressAnnotations(t *testing.T) { annotations: map[string]string{}, isPlus: true, expectedErrors: nil, - msg: "valid input", + msg: "valid no annotations", + }, + + { + annotations: map[string]string{ + "nginx.org/lb-method": "invalid_method", + "nginx.com/health-checks": "not_a_boolean", + }, + isPlus: true, + expectedErrors: []string{ + "annotations.nginx.com/health-checks: Invalid value: \"not_a_boolean\": must be a valid boolean", + "annotations.nginx.org/lb-method: Invalid value: \"invalid_method\": Invalid load balancing method: \"invalid_method\"", + }, + msg: "invalid multiple annotations messages in alphabetical order", }, { From d3e6b92095d8888567b41948e0d788ea7501f23b Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Tue, 8 Dec 2020 11:57:02 +0000 Subject: [PATCH 5/9] Refactor validateIngressAnnotations func --- internal/k8s/validation.go | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index fb1887f3f5..47d3913f94 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -90,27 +90,31 @@ func validateIngress(ing *networking.Ingress, isPlus bool) field.ErrorList { func validateIngressAnnotations(annotations map[string]string, isPlus bool, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - for _, name := range annotationNames { - if value, nameExists := annotations[name]; nameExists { - if validationFuncs, validationExists := annotationValidations[name]; validationExists { - for _, validationFunc := range validationFuncs { - valErrors := validationFunc(&annotationValidationContext{ - annotations: annotations, - name: name, - value: value, - isPlus: isPlus, - fieldPath: fieldPath.Child(name), - }) - if len(valErrors) > 0 { - allErrs = append(allErrs, valErrors...) - break - } - } - } + if value, exists := annotations[name]; exists { + allErrs = append(allErrs, validateIngressAnnotation(&annotationValidationContext{ + annotations: annotations, + name: name, + value: value, + isPlus: isPlus, + fieldPath: fieldPath.Child(name), + })...) } } + return allErrs +} +func validateIngressAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if validationFuncs, exists := annotationValidations[context.name]; exists { + for _, validationFunc := range validationFuncs { + valErrors := validationFunc(context) + if len(valErrors) > 0 { + allErrs = append(allErrs, valErrors...) + break + } + } + } return allErrs } From d16827ad82233059dad28a815155763517950f48 Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Tue, 8 Dec 2020 12:04:22 +0000 Subject: [PATCH 6/9] Change unexported functions back to private --- pkg/apis/configuration/validation/common.go | 8 ++--- .../configuration/validation/common_test.go | 24 +++++++------- pkg/apis/configuration/validation/policy.go | 2 +- .../configuration/validation/virtualserver.go | 32 +++++++++---------- 4 files changed, 32 insertions(+), 34 deletions(-) diff --git a/pkg/apis/configuration/validation/common.go b/pkg/apis/configuration/validation/common.go index 7fbf552e52..860c320b64 100644 --- a/pkg/apis/configuration/validation/common.go +++ b/pkg/apis/configuration/validation/common.go @@ -136,7 +136,7 @@ func validateStringWithVariables(str string, fieldPath *field.Path, specialVars return allErrs } -func ValidateTime(time string, fieldPath *field.Path) field.ErrorList { +func validateTime(time string, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if time == "" { @@ -153,7 +153,7 @@ func ValidateTime(time string, fieldPath *field.Path) field.ErrorList { // http://nginx.org/en/docs/syntax.html const offsetErrMsg = "must consist of numeric characters followed by a valid size suffix. 'k|K|m|M|g|G" -func ValidateOffset(offset string, fieldPath *field.Path) field.ErrorList { +func validateOffset(offset string, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if offset == "" { @@ -171,9 +171,7 @@ func ValidateOffset(offset string, fieldPath *field.Path) field.ErrorList { // http://nginx.org/en/docs/syntax.html const sizeErrMsg = "must consist of numeric characters followed by a valid size suffix. 'k|K|m|M" -var sizeRegexp = regexp.MustCompile("^" + configs.SizeFmt + "$") - -func ValidateSize(size string, fieldPath *field.Path) field.ErrorList { +func validateSize(size string, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if size == "" { diff --git a/pkg/apis/configuration/validation/common_test.go b/pkg/apis/configuration/validation/common_test.go index 79ab3b43c9..d7afd11b82 100644 --- a/pkg/apis/configuration/validation/common_test.go +++ b/pkg/apis/configuration/validation/common_test.go @@ -273,53 +273,53 @@ func TestValidateStringWithVariablesFail(t *testing.T) { func TestValidateSize(t *testing.T) { var validInput = []string{"", "4k", "8K", "16m", "32M"} for _, test := range validInput { - allErrs := ValidateSize(test, field.NewPath("size-field")) + allErrs := validateSize(test, field.NewPath("size-field")) if len(allErrs) != 0 { - t.Errorf("ValidateSize(%q) returned an error for valid input", test) + t.Errorf("validateSize(%q) returned an error for valid input", test) } } var invalidInput = []string{"55mm", "2mG", "6kb", "-5k", "1L", "5G"} for _, test := range invalidInput { - allErrs := ValidateSize(test, field.NewPath("size-field")) + allErrs := validateSize(test, field.NewPath("size-field")) if len(allErrs) == 0 { - t.Errorf("ValidateSize(%q) didn't return error for invalid input.", test) + t.Errorf("validateSize(%q) didn't return error for invalid input.", test) } } } func TestValidateTime(t *testing.T) { time := "1h 2s" - allErrs := ValidateTime(time, field.NewPath("time-field")) + allErrs := validateTime(time, field.NewPath("time-field")) if len(allErrs) != 0 { - t.Errorf("ValidateTime returned errors %v valid input %v", allErrs, time) + 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")) + allErrs := validateTime(time, field.NewPath("time-field")) if len(allErrs) == 0 { - t.Errorf("ValidateTime returned no errors for invalid input %v", time) + t.Errorf("validateTime returned no errors for invalid input %v", time) } } func TestValidateOffset(t *testing.T) { var validInput = []string{"", "1", "10k", "11m", "1K", "100M", "5G"} for _, test := range validInput { - allErrs := ValidateOffset(test, field.NewPath("offset-field")) + allErrs := validateOffset(test, field.NewPath("offset-field")) if len(allErrs) != 0 { - t.Errorf("ValidateOffset(%q) returned an error for valid input", test) + t.Errorf("validateOffset(%q) returned an error for valid input", test) } } var invalidInput = []string{"55mm", "2mG", "6kb", "-5k", "1L", "5Gb"} for _, test := range invalidInput { - allErrs := ValidateOffset(test, field.NewPath("offset-field")) + allErrs := validateOffset(test, field.NewPath("offset-field")) if len(allErrs) == 0 { - t.Errorf("ValidateOffset(%q) didn't return error for invalid input.", test) + t.Errorf("validateOffset(%q) didn't return error for invalid input.", test) } } } diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go index d7492d6f30..92f79d1eed 100644 --- a/pkg/apis/configuration/validation/policy.go +++ b/pkg/apis/configuration/validation/policy.go @@ -221,7 +221,7 @@ func validateRateLimitZoneSize(zoneSize string, fieldPath *field.Path) field.Err return append(allErrs, field.Required(fieldPath, "")) } - allErrs = append(allErrs, ValidateSize(zoneSize, fieldPath)...) + allErrs = append(allErrs, validateSize(zoneSize, fieldPath)...) kbZoneSize := strings.TrimSuffix(strings.ToLower(zoneSize), "k") kbZoneSizeNum, err := strconv.Atoi(kbZoneSize) diff --git a/pkg/apis/configuration/validation/virtualserver.go b/pkg/apis/configuration/validation/virtualserver.go index f8c165e9bc..da1f419ff9 100644 --- a/pkg/apis/configuration/validation/virtualserver.go +++ b/pkg/apis/configuration/validation/virtualserver.go @@ -186,7 +186,7 @@ func validateBuffer(buff *v1.UpstreamBuffers, fieldPath *field.Path) field.Error if buff.Size == "" { allErrs = append(allErrs, field.Required(fieldPath.Child("size"), "cannot be empty")) } else { - allErrs = append(allErrs, ValidateSize(buff.Size, fieldPath.Child("size"))...) + allErrs = append(allErrs, validateSize(buff.Size, fieldPath.Child("size"))...) } return allErrs @@ -224,13 +224,13 @@ func validateUpstreamHealthCheck(hc *v1.HealthCheck, fieldPath *field.Path) fiel allErrs = append(allErrs, validatePath(hc.Path, fieldPath.Child("path"))...) } - allErrs = append(allErrs, ValidateTime(hc.Interval, fieldPath.Child("interval"))...) - allErrs = append(allErrs, ValidateTime(hc.Jitter, fieldPath.Child("jitter"))...) + allErrs = append(allErrs, validateTime(hc.Interval, fieldPath.Child("interval"))...) + allErrs = append(allErrs, validateTime(hc.Jitter, fieldPath.Child("jitter"))...) allErrs = append(allErrs, validatePositiveIntOrZero(hc.Fails, fieldPath.Child("fails"))...) allErrs = append(allErrs, validatePositiveIntOrZero(hc.Passes, fieldPath.Child("passes"))...) - allErrs = append(allErrs, ValidateTime(hc.ConnectTimeout, fieldPath.Child("connect-timeout"))...) - allErrs = append(allErrs, ValidateTime(hc.ReadTimeout, fieldPath.Child("read-timeout"))...) - allErrs = append(allErrs, ValidateTime(hc.SendTimeout, fieldPath.Child("send-timeout"))...) + allErrs = append(allErrs, validateTime(hc.ConnectTimeout, fieldPath.Child("connect-timeout"))...) + allErrs = append(allErrs, validateTime(hc.ReadTimeout, fieldPath.Child("read-timeout"))...) + allErrs = append(allErrs, validateTime(hc.SendTimeout, fieldPath.Child("send-timeout"))...) allErrs = append(allErrs, validateStatusMatch(hc.StatusMatch, fieldPath.Child("statusMatch"))...) for i, header := range hc.Headers { @@ -267,7 +267,7 @@ func validateSessionCookie(sc *v1.SessionCookie, fieldPath *field.Path) field.Er } if sc.Expires != "max" { - allErrs = append(allErrs, ValidateTime(sc.Expires, fieldPath.Child("expires"))...) + allErrs = append(allErrs, validateTime(sc.Expires, fieldPath.Child("expires"))...) } if sc.Domain != "" { @@ -412,22 +412,22 @@ func (vsv *VirtualServerValidator) validateUpstreams(upstreams []v1.Upstream, fi allErrs = append(allErrs, validateServiceName(u.Service, idxPath.Child("service"))...) allErrs = append(allErrs, validateLabels(u.Subselector, idxPath.Child("subselector"))...) - 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, 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.ProxyNextUpstreamTimeout, idxPath.Child("next-upstream-timeout"))...) + allErrs = append(allErrs, validateTime(u.ProxyNextUpstreamTimeout, idxPath.Child("next-upstream-timeout"))...) allErrs = append(allErrs, validatePositiveIntOrZeroFromPointer(&u.ProxyNextUpstreamTries, idxPath.Child("next-upstream-tries"))...) allErrs = append(allErrs, validateUpstreamLBMethod(u.LBMethod, idxPath.Child("lb-method"), vsv.isPlus)...) - allErrs = append(allErrs, ValidateTime(u.FailTimeout, idxPath.Child("fail-timeout"))...) + allErrs = append(allErrs, validateTime(u.FailTimeout, idxPath.Child("fail-timeout"))...) allErrs = append(allErrs, validatePositiveIntOrZeroFromPointer(u.MaxFails, idxPath.Child("max-fails"))...) allErrs = append(allErrs, validatePositiveIntOrZeroFromPointer(u.Keepalive, idxPath.Child("keepalive"))...) allErrs = append(allErrs, validatePositiveIntOrZeroFromPointer(u.MaxConns, idxPath.Child("max-conns"))...) - allErrs = append(allErrs, ValidateOffset(u.ClientMaxBodySize, idxPath.Child("client-max-body-size"))...) + allErrs = append(allErrs, validateOffset(u.ClientMaxBodySize, idxPath.Child("client-max-body-size"))...) allErrs = append(allErrs, validateUpstreamHealthCheck(u.HealthCheck, idxPath.Child("healthCheck"))...) - allErrs = append(allErrs, ValidateTime(u.SlowStart, idxPath.Child("slow-start"))...) + allErrs = append(allErrs, validateTime(u.SlowStart, idxPath.Child("slow-start"))...) allErrs = append(allErrs, validateBuffer(u.ProxyBuffers, idxPath.Child("buffers"))...) - allErrs = append(allErrs, ValidateSize(u.ProxyBufferSize, idxPath.Child("buffer-size"))...) + allErrs = append(allErrs, validateSize(u.ProxyBufferSize, idxPath.Child("buffer-size"))...) allErrs = append(allErrs, validateQueue(u.Queue, idxPath.Child("queue"))...) allErrs = append(allErrs, validateSessionCookie(u.SessionCookie, idxPath.Child("sessionCookie"))...) @@ -1417,7 +1417,7 @@ func validateQueue(queue *v1.UpstreamQueue, fieldPath *field.Path) field.ErrorLi return allErrs } - allErrs = append(allErrs, ValidateTime(queue.Timeout, fieldPath.Child("timeout"))...) + allErrs = append(allErrs, validateTime(queue.Timeout, fieldPath.Child("timeout"))...) if queue.Size <= 0 { allErrs = append(allErrs, field.Required(fieldPath.Child("size"), "must be positive")) } From 9d2ee77dc59bfbf069b268964698b897e0c4c000 Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Tue, 8 Dec 2020 12:30:28 +0000 Subject: [PATCH 7/9] Add earlier validation of nginx.com/slow-start annotation --- internal/configs/annotations.go | 12 ++++-------- internal/k8s/validation.go | 14 ++++++++++++++ internal/k8s/validation_test.go | 29 +++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 8 deletions(-) diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index 4cf9de9789..9a87845cab 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -111,15 +111,11 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool } if slowStart, exists := ingEx.Ingress.Annotations["nginx.com/slow-start"]; exists { - 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 { - cfgParams.SlowStart = parsedSlowStart - } else { - glog.Warning("Annotation 'nginx.com/slow-start' requires NGINX Plus") - } + parsedSlowStart, err := ParseTime(slowStart) + if err != nil { + glog.Error(err) } + cfgParams.SlowStart = parsedSlowStart } if serverTokens, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.org/server-tokens", ingEx.Ingress); exists { diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 47d3913f94..2bf1a5641c 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -17,6 +17,7 @@ const ( healthChecksAnnotation = "nginx.com/health-checks" healthChecksMandatoryAnnotation = "nginx.com/health-checks-mandatory" healthChecksMandatoryQueueAnnotation = "nginx.com/health-checks-mandatory-queue" + slowStartAnnotation = "nginx.com/slow-start" ) type annotationValidationContext struct { @@ -57,6 +58,11 @@ var ( validateRequiredAnnotation, validateNonNegativeIntAnnotation, }, + slowStartAnnotation: { + validateRequiredAnnotation, + validatePlusOnlyAnnotation, + validateTimeAnnotation, + }, } annotationNames = sortedAnnotationNames(annotationValidations) ) @@ -179,6 +185,14 @@ func validateBoolAnnotation(context *annotationValidationContext) field.ErrorLis return allErrs } +func validateTimeAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if _, err := configs.ParseTime(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a valid time")) + } + return allErrs +} + func validateNonNegativeIntAnnotation(context *annotationValidationContext) field.ErrorList { allErrs := field.ErrorList{} if _, err := configs.ParseUint64(context.value); err != nil { diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index e50c81688f..beae5de0ee 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -343,6 +343,35 @@ func TestValidateIngressAnnotations(t *testing.T) { }, msg: "invalid nginx.com/health-checks-mandatory-queue nginx.com/health-checks-mandatory is not true", }, + + { + annotations: map[string]string{ + "nginx.com/slow-start": "60s", + }, + isPlus: true, + expectedErrors: nil, + msg: "valid nginx.com/slow-start annotation", + }, + { + annotations: map[string]string{ + "nginx.com/slow-start": "true", + }, + isPlus: false, + expectedErrors: []string{ + "annotations.nginx.com/slow-start: Forbidden: annotation requires NGINX Plus", + }, + msg: "invalid nginx.com/slow-start annotation, nginx plus only", + }, + { + annotations: map[string]string{ + "nginx.com/slow-start": "not_a_time", + }, + isPlus: true, + expectedErrors: []string{ + "annotations.nginx.com/slow-start: Invalid value: \"not_a_time\": must be a valid time", + }, + msg: "invalid nginx.com/slow-start annotation", + }, } for _, test := range tests { From 1925a4b5258017e5600413bda799bb87a0e8ff09 Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Tue, 8 Dec 2020 15:18:32 +0000 Subject: [PATCH 8/9] Separate out nginx and nginx plus validation --- internal/k8s/validation.go | 86 ++++++++++---- internal/k8s/validation_test.go | 196 +++++++++++++++++++++----------- 2 files changed, 198 insertions(+), 84 deletions(-) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 2bf1a5641c..0a6bd0c70e 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -29,23 +29,51 @@ type annotationValidationContext struct { } type annotationValidationFunc func(context *annotationValidationContext) field.ErrorList +type annotationValidationConfig map[string][]annotationValidationFunc type validatorFunc func(val string) error var ( - // annotationValidations defines the various validations which will be applied in order to each ingress annotation. - // If any specified validation fails, the remaining validations for that annotation will not be run. - annotationValidations = map[string][]annotationValidationFunc{ + // nginxAnnotationValidations defines the various validations which will be applied in order to each ingress + // annotation for nginx. If any specified validation fails, the remaining validations for that annotation will not + // be run. + nginxAnnotationValidations = annotationValidationConfig{ mergeableIngressTypeAnnotation: { validateRequiredAnnotation, validateMergeableIngressTypeAnnotation, }, lbMethodAnnotation: { validateRequiredAnnotation, - validateLBMethodAnnotation, + validateNginxLBMethodAnnotation, }, healthChecksAnnotation: { - validateRequiredAnnotation, validatePlusOnlyAnnotation, + }, + healthChecksMandatoryAnnotation: { + validatePlusOnlyAnnotation, + }, + healthChecksMandatoryQueueAnnotation: { + validatePlusOnlyAnnotation, + }, + slowStartAnnotation: { + validatePlusOnlyAnnotation, + }, + } + nginxAnnotationNames = sortedAnnotationNames(nginxAnnotationValidations) + + // nginxPlusAnnotationValidations defines the various validations which will be applied in order to each ingress + // annotation for nginx plus. If any specified validation fails, the remaining validations for that annotation will + // not be run. + nginxPlusAnnotationValidations = annotationValidationConfig{ + mergeableIngressTypeAnnotation: { + validateRequiredAnnotation, + validateMergeableIngressTypeAnnotation, + }, + lbMethodAnnotation: { + validateRequiredAnnotation, + validateNginxPlusLBMethodAnnotation, + }, + healthChecksAnnotation: { + validateRequiredAnnotation, validateBoolAnnotation, }, healthChecksMandatoryAnnotation: { @@ -60,14 +88,13 @@ var ( }, slowStartAnnotation: { validateRequiredAnnotation, - validatePlusOnlyAnnotation, validateTimeAnnotation, }, } - annotationNames = sortedAnnotationNames(annotationValidations) + nginxPlusAnnotationNames = sortedAnnotationNames(nginxPlusAnnotationValidations) ) -func sortedAnnotationNames(annotationValidations map[string][]annotationValidationFunc) []string { +func sortedAnnotationNames(annotationValidations annotationValidationConfig) []string { sortedNames := make([]string, 0) for annotationName := range annotationValidations { sortedNames = append(sortedNames, annotationName) @@ -80,7 +107,6 @@ func sortedAnnotationNames(annotationValidations map[string][]annotationValidati // Note that the full validation of Ingress resources is done by Kubernetes. func validateIngress(ing *networking.Ingress, isPlus bool) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, validateIngressAnnotations(ing.Annotations, isPlus, field.NewPath("annotations"))...) allErrs = append(allErrs, validateIngressSpec(&ing.Spec, field.NewPath("spec"))...) @@ -96,22 +122,40 @@ func validateIngress(ing *networking.Ingress, isPlus bool) field.ErrorList { func validateIngressAnnotations(annotations map[string]string, isPlus bool, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} + + var annotationNames []string + if isPlus { + annotationNames = nginxPlusAnnotationNames + } else { + annotationNames = nginxAnnotationNames + } + for _, name := range annotationNames { if value, exists := annotations[name]; exists { - allErrs = append(allErrs, validateIngressAnnotation(&annotationValidationContext{ + context := &annotationValidationContext{ annotations: annotations, name: name, value: value, isPlus: isPlus, fieldPath: fieldPath.Child(name), - })...) + } + allErrs = append(allErrs, validateIngressAnnotation(context)...) } } + return allErrs } func validateIngressAnnotation(context *annotationValidationContext) field.ErrorList { allErrs := field.ErrorList{} + + var annotationValidations annotationValidationConfig + if context.isPlus { + annotationValidations = nginxPlusAnnotationValidations + } else { + annotationValidations = nginxAnnotationValidations + } + if validationFuncs, exists := annotationValidations[context.name]; exists { for _, validationFunc := range validationFuncs { valErrors := validationFunc(context) @@ -147,16 +191,18 @@ func validateMergeableIngressTypeAnnotation(context *annotationValidationContext return allErrs } -func validateLBMethodAnnotation(context *annotationValidationContext) field.ErrorList { +func validateNginxLBMethodAnnotation(context *annotationValidationContext) field.ErrorList { allErrs := field.ErrorList{} - if context.isPlus { - if _, err := configs.ParseLBMethodForPlus(context.value); err != nil { - return append(allErrs, field.Invalid(context.fieldPath, context.value, err.Error())) - } - } else { - if _, err := configs.ParseLBMethod(context.value); err != nil { - return append(allErrs, field.Invalid(context.fieldPath, context.value, err.Error())) - } + if _, err := configs.ParseLBMethod(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, err.Error())) + } + return allErrs +} + +func validateNginxPlusLBMethodAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if _, err := configs.ParseLBMethodForPlus(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, err.Error())) } return allErrs } diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index beae5de0ee..3b479a230e 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -117,29 +117,27 @@ func TestValidateIngress(t *testing.T) { } } -func TestValidateIngressAnnotations(t *testing.T) { +func TestValidateNginxIngressAnnotations(t *testing.T) { + isPlus := false tests := []struct { annotations map[string]string - isPlus bool expectedErrors []string msg string }{ { annotations: map[string]string{}, - isPlus: true, expectedErrors: nil, msg: "valid no annotations", }, { annotations: map[string]string{ - "nginx.org/lb-method": "invalid_method", - "nginx.com/health-checks": "not_a_boolean", + "nginx.org/lb-method": "invalid_method", + "nginx.org/mergeable-ingress-type": "invalid", }, - isPlus: true, expectedErrors: []string{ - "annotations.nginx.com/health-checks: Invalid value: \"not_a_boolean\": must be a valid boolean", - "annotations.nginx.org/lb-method: Invalid value: \"invalid_method\": Invalid load balancing method: \"invalid_method\"", + `annotations.nginx.org/lb-method: Invalid value: "invalid_method": Invalid load balancing method: "invalid_method"`, + `annotations.nginx.org/mergeable-ingress-type: Invalid value: "invalid": must be one of: 'master' or 'minion'`, }, msg: "invalid multiple annotations messages in alphabetical order", }, @@ -148,7 +146,6 @@ func TestValidateIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "master", }, - isPlus: false, expectedErrors: nil, msg: "valid input with master annotation", }, @@ -156,7 +153,6 @@ func TestValidateIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "minion", }, - isPlus: false, expectedErrors: nil, msg: "valid input with minion annotation", }, @@ -164,7 +160,6 @@ func TestValidateIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "", }, - isPlus: false, expectedErrors: []string{ "annotations.nginx.org/mergeable-ingress-type: Required value", }, @@ -174,7 +169,6 @@ func TestValidateIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "abc", }, - isPlus: false, expectedErrors: []string{ `annotations.nginx.org/mergeable-ingress-type: Invalid value: "abc": must be one of: 'master' or 'minion'`, }, @@ -183,86 +177,179 @@ func TestValidateIngressAnnotations(t *testing.T) { { annotations: map[string]string{ - "nginx.org/lb-method": "least_time header", + "nginx.org/lb-method": "random", }, - isPlus: true, expectedErrors: nil, - msg: "valid nginx.org/lb-method annotation, nginx plus", + msg: "valid nginx.org/lb-method annotation, nginx normal", }, { annotations: map[string]string{ - "nginx.org/lb-method": "random", + "nginx.org/lb-method": "least_time header", }, - isPlus: false, - expectedErrors: nil, - msg: "valid nginx.org/lb-method annotation, nginx normal", + expectedErrors: []string{ + `annotations.nginx.org/lb-method: Invalid value: "least_time header": Invalid load balancing method: "least_time header"`, + }, + msg: "invalid nginx.org/lb-method annotation, nginx plus only", }, { annotations: map[string]string{ "nginx.org/lb-method": "invalid_method", }, - isPlus: true, expectedErrors: []string{ - "annotations.nginx.org/lb-method: Invalid value: \"invalid_method\": Invalid load balancing method: \"invalid_method\"", + `annotations.nginx.org/lb-method: Invalid value: "invalid_method": Invalid load balancing method: "invalid_method"`, }, - msg: "invalid nginx.org/lb-method annotation, nginx", + msg: "invalid nginx.org/lb-method annotation", }, + { annotations: map[string]string{ - "nginx.org/lb-method": "least_time header", + "nginx.com/health-checks": "true", }, - isPlus: false, expectedErrors: []string{ - "annotations.nginx.org/lb-method: Invalid value: \"least_time header\": Invalid load balancing method: \"least_time header\"", + "annotations.nginx.com/health-checks: Forbidden: annotation requires NGINX Plus", }, - msg: "invalid nginx.org/lb-method annotation, nginx plus", + msg: "invalid nginx.com/health-checks annotation, nginx plus only", }, { annotations: map[string]string{ - "nginx.com/health-checks": "true", + "nginx.com/health-checks-mandatory": "true", }, - isPlus: true, - expectedErrors: nil, - msg: "valid nginx.com/health-checks annotation", + expectedErrors: []string{ + "annotations.nginx.com/health-checks-mandatory: Forbidden: annotation requires NGINX Plus", + }, + msg: "invalid nginx.com/health-checks-mandatory annotation, nginx plus only", }, + { annotations: map[string]string{ - "nginx.com/health-checks": "true", + "nginx.com/health-checks-mandatory-queue": "true", }, - isPlus: false, expectedErrors: []string{ - "annotations.nginx.com/health-checks: Forbidden: annotation requires NGINX Plus", + "annotations.nginx.com/health-checks-mandatory-queue: Forbidden: annotation requires NGINX Plus", }, - msg: "invalid nginx.com/health-checks annotation, nginx plus only", + msg: "invalid nginx.com/health-checks-mandatory-queue annotation, nginx plus only", }, + { annotations: map[string]string{ + "nginx.com/slow-start": "true", + }, + expectedErrors: []string{ + "annotations.nginx.com/slow-start: Forbidden: annotation requires NGINX Plus", + }, + msg: "invalid nginx.com/slow-start annotation, nginx plus only", + }, + } + + for _, test := range tests { + t.Run(test.msg, func(t *testing.T) { + allErrs := validateIngressAnnotations(test.annotations, isPlus, field.NewPath("annotations")) + assertion := assertErrors("validateIngressAnnotations()", test.msg, allErrs, test.expectedErrors) + if assertion != "" { + t.Error(assertion) + } + }) + } +} + +func TestValidateNginxPlusIngressAnnotations(t *testing.T) { + isPlus := true + tests := []struct { + annotations map[string]string + expectedErrors []string + msg string + }{ + { + annotations: map[string]string{}, + expectedErrors: nil, + msg: "valid no annotations", + }, + + { + annotations: map[string]string{ + "nginx.org/lb-method": "invalid_method", "nginx.com/health-checks": "not_a_boolean", }, - isPlus: true, expectedErrors: []string{ - "annotations.nginx.com/health-checks: Invalid value: \"not_a_boolean\": must be a valid boolean", + `annotations.nginx.com/health-checks: Invalid value: "not_a_boolean": must be a valid boolean`, + `annotations.nginx.org/lb-method: Invalid value: "invalid_method": Invalid load balancing method: "invalid_method"`, }, - msg: "invalid nginx.com/health-checks annotation", + msg: "invalid multiple annotations messages in alphabetical order", + }, + + { + annotations: map[string]string{ + "nginx.org/mergeable-ingress-type": "master", + }, + expectedErrors: nil, + msg: "valid input with master annotation", + }, + { + annotations: map[string]string{ + "nginx.org/mergeable-ingress-type": "minion", + }, + expectedErrors: nil, + msg: "valid input with minion annotation", + }, + { + annotations: map[string]string{ + "nginx.org/mergeable-ingress-type": "", + }, + expectedErrors: []string{ + "annotations.nginx.org/mergeable-ingress-type: Required value", + }, + msg: "invalid mergeable type annotation 1", + }, + { + annotations: map[string]string{ + "nginx.org/mergeable-ingress-type": "abc", + }, + expectedErrors: []string{ + `annotations.nginx.org/mergeable-ingress-type: Invalid value: "abc": must be one of: 'master' or 'minion'`, + }, + msg: "invalid mergeable type annotation 2", }, + { + annotations: map[string]string{ + "nginx.org/lb-method": "least_time header", + }, + expectedErrors: nil, + msg: "valid nginx.org/lb-method annotation", + }, + { + annotations: map[string]string{ + "nginx.org/lb-method": "invalid_method", + }, + expectedErrors: []string{ + `annotations.nginx.org/lb-method: Invalid value: "invalid_method": Invalid load balancing method: "invalid_method"`, + }, + msg: "invalid nginx.org/lb-method annotation, nginx", + }, + + { + annotations: map[string]string{ + "nginx.com/health-checks": "true", + }, + expectedErrors: nil, + msg: "valid nginx.com/health-checks annotation", + }, { annotations: map[string]string{ "nginx.com/health-checks": "not_a_boolean", }, - isPlus: false, expectedErrors: []string{ - "annotations.nginx.com/health-checks: Forbidden: annotation requires NGINX Plus", + `annotations.nginx.com/health-checks: Invalid value: "not_a_boolean": must be a valid boolean`, }, msg: "invalid nginx.com/health-checks annotation", }, + { annotations: map[string]string{ "nginx.com/health-checks": "true", "nginx.com/health-checks-mandatory": "true", }, - isPlus: true, expectedErrors: nil, msg: "valid nginx.com/health-checks-mandatory annotation", }, @@ -271,9 +358,8 @@ func TestValidateIngressAnnotations(t *testing.T) { "nginx.com/health-checks": "true", "nginx.com/health-checks-mandatory": "not_a_boolean", }, - isPlus: true, expectedErrors: []string{ - "annotations.nginx.com/health-checks-mandatory: Invalid value: \"not_a_boolean\": must be a valid boolean", + `annotations.nginx.com/health-checks-mandatory: Invalid value: "not_a_boolean": must be a valid boolean`, }, msg: "invalid nginx.com/health-checks-mandatory, must be a boolean", }, @@ -281,7 +367,6 @@ func TestValidateIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/health-checks-mandatory": "true", }, - isPlus: true, expectedErrors: []string{ "annotations.nginx.com/health-checks-mandatory: Forbidden: related annotation nginx.com/health-checks: must be set", }, @@ -292,7 +377,6 @@ func TestValidateIngressAnnotations(t *testing.T) { "nginx.com/health-checks": "false", "nginx.com/health-checks-mandatory": "true", }, - isPlus: true, expectedErrors: []string{ "annotations.nginx.com/health-checks-mandatory: Forbidden: related annotation nginx.com/health-checks: must be true", }, @@ -305,7 +389,6 @@ func TestValidateIngressAnnotations(t *testing.T) { "nginx.com/health-checks-mandatory": "true", "nginx.com/health-checks-mandatory-queue": "5", }, - isPlus: true, expectedErrors: nil, msg: "valid nginx.com/health-checks-mandatory-queue annotation", }, @@ -315,17 +398,15 @@ func TestValidateIngressAnnotations(t *testing.T) { "nginx.com/health-checks-mandatory": "true", "nginx.com/health-checks-mandatory-queue": "not_a_number", }, - isPlus: true, expectedErrors: []string{ - "annotations.nginx.com/health-checks-mandatory-queue: Invalid value: \"not_a_number\": must be a non-negative integer", + `annotations.nginx.com/health-checks-mandatory-queue: Invalid value: "not_a_number": must be a non-negative integer`, }, - msg: "invalid nginx.com/health-checks-mandatory-queue, must be a boolean", + msg: "invalid nginx.com/health-checks-mandatory-queue, must be a number", }, { annotations: map[string]string{ "nginx.com/health-checks-mandatory-queue": "5", }, - isPlus: true, expectedErrors: []string{ "annotations.nginx.com/health-checks-mandatory-queue: Forbidden: related annotation nginx.com/health-checks-mandatory: must be set", }, @@ -337,7 +418,6 @@ func TestValidateIngressAnnotations(t *testing.T) { "nginx.com/health-checks-mandatory": "false", "nginx.com/health-checks-mandatory-queue": "5", }, - isPlus: true, expectedErrors: []string{ "annotations.nginx.com/health-checks-mandatory-queue: Forbidden: related annotation nginx.com/health-checks-mandatory: must be true", }, @@ -348,27 +428,15 @@ func TestValidateIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/slow-start": "60s", }, - isPlus: true, expectedErrors: nil, msg: "valid nginx.com/slow-start annotation", }, - { - annotations: map[string]string{ - "nginx.com/slow-start": "true", - }, - isPlus: false, - expectedErrors: []string{ - "annotations.nginx.com/slow-start: Forbidden: annotation requires NGINX Plus", - }, - msg: "invalid nginx.com/slow-start annotation, nginx plus only", - }, { annotations: map[string]string{ "nginx.com/slow-start": "not_a_time", }, - isPlus: true, expectedErrors: []string{ - "annotations.nginx.com/slow-start: Invalid value: \"not_a_time\": must be a valid time", + `annotations.nginx.com/slow-start: Invalid value: "not_a_time": must be a valid time`, }, msg: "invalid nginx.com/slow-start annotation", }, @@ -376,7 +444,7 @@ func TestValidateIngressAnnotations(t *testing.T) { for _, test := range tests { t.Run(test.msg, func(t *testing.T) { - allErrs := validateIngressAnnotations(test.annotations, test.isPlus, field.NewPath("annotations")) + allErrs := validateIngressAnnotations(test.annotations, isPlus, field.NewPath("annotations")) assertion := assertErrors("validateIngressAnnotations()", test.msg, allErrs, test.expectedErrors) if assertion != "" { t.Error(assertion) From 255f29c8e50007f603b3dc762b817f972cd0da5f Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Tue, 8 Dec 2020 15:45:40 +0000 Subject: [PATCH 9/9] Fix golint issues in parsing_helpers.go --- internal/configs/parsing_helpers.go | 40 ++++++++--------------------- 1 file changed, 11 insertions(+), 29 deletions(-) diff --git a/internal/configs/parsing_helpers.go b/internal/configs/parsing_helpers.go index aaf47a6790..937486820d 100644 --- a/internal/configs/parsing_helpers.go +++ b/internal/configs/parsing_helpers.go @@ -162,53 +162,45 @@ func validateHashLBMethod(method string) (string, error) { return "", fmt.Errorf("Invalid load balancing method: %q", method) } -// ParseBool ensures that the string value in the annotation is a valid bool +// ParseBool ensures that the string value is a valid bool func ParseBool(s string) (bool, error) { return strconv.ParseBool(s) } +// ParseInt ensures that the string value is a valid int func ParseInt(s string) (int, error) { return strconv.Atoi(s) } +// ParseInt64 ensures that the string value is a valid int64 func ParseInt64(s string) (int64, error) { return strconv.ParseInt(s, 10, 64) } +// ParseUint64 ensures that the string value is a valid uint64 func ParseUint64(s string) (uint64, error) { return strconv.ParseUint(s, 10, 64) } -// 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} *)+$`) +// timeRegexp http://nginx.org/en/docs/syntax.html +var timeRegexp = regexp.MustCompile(`^([0-9]+([ms|s|m|h|d|w|M|y]?){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) { + if timeRegexp.MatchString(s) { return s, nil } return "", errors.New("Invalid time string") } -// http://nginx.org/en/docs/syntax.html +// OffsetFmt http://nginx.org/en/docs/syntax.html const OffsetFmt = `\d+[kKmMgG]?` var offsetRegexp = regexp.MustCompile("^" + OffsetFmt + "$") +// ParseOffset ensures that the string value is a valid offset func ParseOffset(s string) (string, error) { s = strings.TrimSpace(s) @@ -218,11 +210,12 @@ func ParseOffset(s string) (string, error) { return "", errors.New("Invalid offset string") } -// http://nginx.org/en/docs/syntax.html +// SizeFmt http://nginx.org/en/docs/syntax.html const SizeFmt = `\d+[kKmM]?` var sizeRegexp = regexp.MustCompile("^" + SizeFmt + "$") +// ParseSize ensures that the string value is a valid size func ParseSize(s string) (string, error) { s = strings.TrimSpace(s) @@ -232,17 +225,6 @@ func ParseSize(s string) (string, error) { return "", errors.New("Invalid size string") } -var proxyBuffersRegexp = regexp.MustCompile(`^\d+ \d+[kKmM]?$`) - -func ParseProxyBuffers(s string) (string, error) { - s = strings.TrimSpace(s) - - if proxyBuffersRegexp.MatchString(s) { - return s, nil - } - return "", errors.New("must be a valid proxy buffers spec as specified here: https://nginx.org/en/docs/http/ngx_http_proxy_module.html#proxy_buffers, e.g. \"8 4k\"") -} - var threshEx = regexp.MustCompile(`high=([1-9]|[1-9][0-9]|100) low=([1-9]|[1-9][0-9]|100)\b`) var threshExR = regexp.MustCompile(`low=([1-9]|[1-9][0-9]|100) high=([1-9]|[1-9][0-9]|100)\b`)