From 591ef58c7cad06496c4121ae87e6c019632ae1a4 Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Wed, 9 Dec 2020 12:36:55 +0000 Subject: [PATCH 01/13] Add more early ingress annotation validations --- internal/k8s/validation.go | 14 ++++++ internal/k8s/validation_test.go | 78 +++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 0a6bd0c70e..665b26e870 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -18,6 +18,9 @@ const ( healthChecksMandatoryAnnotation = "nginx.com/health-checks-mandatory" healthChecksMandatoryQueueAnnotation = "nginx.com/health-checks-mandatory-queue" slowStartAnnotation = "nginx.com/slow-start" + serverTokensAnnotation = "nginx.org/server-tokens" + serverSnippetsAnnotation = "nginx.org/server-snippets" + locationSnippetsAnnotation = "nginx.org/location-snippets" ) type annotationValidationContext struct { @@ -57,6 +60,12 @@ var ( slowStartAnnotation: { validatePlusOnlyAnnotation, }, + serverTokensAnnotation: { + validateRequiredAnnotation, + validateBoolAnnotation, + }, + serverSnippetsAnnotation: {}, + locationSnippetsAnnotation: {}, } nginxAnnotationNames = sortedAnnotationNames(nginxAnnotationValidations) @@ -90,6 +99,11 @@ var ( validateRequiredAnnotation, validateTimeAnnotation, }, + serverTokensAnnotation: { + validateRequiredAnnotation, + }, + serverSnippetsAnnotation: {}, + locationSnippetsAnnotation: {}, } nginxPlusAnnotationNames = sortedAnnotationNames(nginxPlusAnnotationValidations) ) diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 3b479a230e..75831815c1 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -240,6 +240,46 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }, msg: "invalid nginx.com/slow-start annotation, nginx plus only", }, + + { + annotations: map[string]string{ + "nginx.org/server-tokens": "custom_setting", + }, + expectedErrors: []string{ + `annotations.nginx.org/server-tokens: Invalid value: "custom_setting": must be a valid boolean`, + }, + msg: "invalid nginx.org/server-tokens annotation, must be a boolean", + }, + + { + annotations: map[string]string{ + "nginx.org/server-snippets": "snippet-1", + }, + expectedErrors: nil, + msg: "valid nginx.org/server-snippets annotation, single-line", + }, + { + annotations: map[string]string{ + "nginx.org/server-snippets": "snippet-1\nsnippet-2\nsnippet-3", + }, + expectedErrors: nil, + msg: "valid nginx.org/server-snippets annotation, multi-line", + }, + + { + annotations: map[string]string{ + "nginx.org/location-snippets": "snippet-1", + }, + expectedErrors: nil, + msg: "valid nginx.org/location-snippets annotation", + }, + { + annotations: map[string]string{ + "nginx.org/location-snippets": "snippet-1\nsnippet-2\nsnippet-3", + }, + expectedErrors: nil, + msg: "valid nginx.org/location-snippets annotation, multi-line", + }, } for _, test := range tests { @@ -440,6 +480,44 @@ func TestValidateNginxPlusIngressAnnotations(t *testing.T) { }, msg: "invalid nginx.com/slow-start annotation", }, + + { + annotations: map[string]string{ + "nginx.org/server-tokens": "custom_setting", + }, + expectedErrors: nil, + msg: "valid nginx.org/server-tokens annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/server-snippets": "snippet-1", + }, + expectedErrors: nil, + msg: "valid nginx.org/server-snippets annotation, single-line", + }, + { + annotations: map[string]string{ + "nginx.org/server-snippets": "snippet-1\nsnippet-2\nsnippet-3", + }, + expectedErrors: nil, + msg: "valid nginx.org/server-snippets annotation, multi-line", + }, + + { + annotations: map[string]string{ + "nginx.org/location-snippets": "snippet-1", + }, + expectedErrors: nil, + msg: "valid nginx.org/location-snippets annotation", + }, + { + annotations: map[string]string{ + "nginx.org/location-snippets": "snippet-1\nsnippet-2\nsnippet-3", + }, + expectedErrors: nil, + msg: "valid nginx.org/location-snippets annotation, multi-line", + }, } for _, test := range tests { From 8dc390c8de3719909123943b28ae4457d728727e Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Wed, 9 Dec 2020 13:46:13 +0000 Subject: [PATCH 02/13] Add further validations --- internal/configs/annotations.go | 3 +- internal/k8s/validation.go | 53 ++++++- internal/k8s/validation_test.go | 242 ++++++++++++++++++++++++++++++-- 3 files changed, 285 insertions(+), 13 deletions(-) diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index 4afff52db8..df7254e938 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -114,8 +114,9 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool parsedSlowStart, err := ParseTime(slowStart) if err != nil { glog.Error(err) + } else { + cfgParams.SlowStart = parsedSlowStart } - 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 665b26e870..d7d62bfea1 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -21,6 +21,15 @@ const ( serverTokensAnnotation = "nginx.org/server-tokens" serverSnippetsAnnotation = "nginx.org/server-snippets" locationSnippetsAnnotation = "nginx.org/location-snippets" + proxyConnectTimeoutAnnotation = "nginx.org/proxy-connect-timeout" + proxyReadTimeoutAnnotation = "nginx.org/proxy-read-timeout" + proxySendTimeoutAnnotation = "nginx.org/proxy-send-timeout" + proxyHideHeadersAnnotation = "nginx.org/proxy-hide-headers" + proxyPassHeadersAnnotation = "nginx.org/proxy-pass-headers" + clientMaxBodySizeAnnotation = "nginx.org/client-max-body-size" + redirectToHTTPSAnnotation = "nginx.org/redirect-to-https" + sslRedirectAnnotation = "ingress.kubernetes.io/ssl-redirect" + proxyBufferingAnnotation = "nginx.org/proxy-buffering" ) type annotationValidationContext struct { @@ -64,8 +73,26 @@ var ( validateRequiredAnnotation, validateBoolAnnotation, }, - serverSnippetsAnnotation: {}, - locationSnippetsAnnotation: {}, + serverSnippetsAnnotation: {}, + locationSnippetsAnnotation: {}, + proxyConnectTimeoutAnnotation: {}, + proxyReadTimeoutAnnotation: {}, + proxySendTimeoutAnnotation: {}, + proxyHideHeadersAnnotation: {}, + proxyPassHeadersAnnotation: {}, + clientMaxBodySizeAnnotation: {}, + redirectToHTTPSAnnotation: { + validateRequiredAnnotation, + validateBoolAnnotation, + }, + sslRedirectAnnotation: { + validateRequiredAnnotation, + validateBoolAnnotation, + }, + proxyBufferingAnnotation: { + validateRequiredAnnotation, + validateBoolAnnotation, + }, } nginxAnnotationNames = sortedAnnotationNames(nginxAnnotationValidations) @@ -102,8 +129,26 @@ var ( serverTokensAnnotation: { validateRequiredAnnotation, }, - serverSnippetsAnnotation: {}, - locationSnippetsAnnotation: {}, + serverSnippetsAnnotation: {}, + locationSnippetsAnnotation: {}, + proxyConnectTimeoutAnnotation: {}, + proxyReadTimeoutAnnotation: {}, + proxySendTimeoutAnnotation: {}, + proxyHideHeadersAnnotation: {}, + proxyPassHeadersAnnotation: {}, + clientMaxBodySizeAnnotation: {}, + redirectToHTTPSAnnotation: { + validateRequiredAnnotation, + validateBoolAnnotation, + }, + sslRedirectAnnotation: { + validateRequiredAnnotation, + validateBoolAnnotation, + }, + proxyBufferingAnnotation: { + validateRequiredAnnotation, + validateBoolAnnotation, + }, } nginxPlusAnnotationNames = sortedAnnotationNames(nginxPlusAnnotationValidations) ) diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 75831815c1..d5361df011 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -256,14 +256,14 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/server-snippets": "snippet-1", }, expectedErrors: nil, - msg: "valid nginx.org/server-snippets annotation, single-line", + msg: "valid nginx.org/server-snippets annotation, single-value", }, { annotations: map[string]string{ "nginx.org/server-snippets": "snippet-1\nsnippet-2\nsnippet-3", }, expectedErrors: nil, - msg: "valid nginx.org/server-snippets annotation, multi-line", + msg: "valid nginx.org/server-snippets annotation, multi-value", }, { @@ -271,14 +271,127 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/location-snippets": "snippet-1", }, expectedErrors: nil, - msg: "valid nginx.org/location-snippets annotation", + msg: "valid nginx.org/location-snippets annotation, single-value", }, { annotations: map[string]string{ "nginx.org/location-snippets": "snippet-1\nsnippet-2\nsnippet-3", }, expectedErrors: nil, - msg: "valid nginx.org/location-snippets annotation, multi-line", + msg: "valid nginx.org/location-snippets annotation, multi-value", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-connect-timeout": "10s", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-connect-timeout annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-read-timeout": "10s", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-read-timeout annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-send-timeout": "10s", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-send-timeout annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-hide-headers": "header-1", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-hide-headers annotation, single-value", + }, + { + annotations: map[string]string{ + "nginx.org/proxy-hide-headers": "header-1,header-2,header-3", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-hide-headers annotation, multi-value", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-pass-headers": "header-1", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-pass-headers annotation, single-value", + }, + { + annotations: map[string]string{ + "nginx.org/proxy-pass-headers": "header-1,header-2,header-3", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-pass-headers annotation, multi-value", + }, + + { + annotations: map[string]string{ + "nginx.org/client-max-body-size": "16M", + }, + expectedErrors: nil, + msg: "valid nginx.org/client-max-body-size annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/redirect-to-https": "true", + }, + expectedErrors: nil, + msg: "valid nginx.org/redirect-to-https annotation", + }, + { + annotations: map[string]string{ + "nginx.org/redirect-to-https": "not_a_boolean", + }, + expectedErrors: []string{ + `annotations.nginx.org/redirect-to-https: Invalid value: "not_a_boolean": must be a valid boolean`, + }, + msg: "invalid nginx.org/redirect-to-https annotation", + }, + + { + annotations: map[string]string{ + "ingress.kubernetes.io/ssl-redirect": "true", + }, + expectedErrors: nil, + msg: "valid ingress.kubernetes.io/ssl-redirect annotation", + }, + { + annotations: map[string]string{ + "ingress.kubernetes.io/ssl-redirect": "not_a_boolean", + }, + expectedErrors: []string{ + `annotations.ingress.kubernetes.io/ssl-redirect: Invalid value: "not_a_boolean": must be a valid boolean`, + }, + msg: "invalid ingress.kubernetes.io/ssl-redirect annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-buffering": "true", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-buffering annotation", + }, + { + annotations: map[string]string{ + "nginx.org/proxy-buffering": "not_a_boolean", + }, + expectedErrors: []string{ + `annotations.nginx.org/proxy-buffering: Invalid value: "not_a_boolean": must be a valid boolean`, + }, + msg: "invalid nginx.org/proxy-buffering annotation", }, } @@ -494,14 +607,14 @@ func TestValidateNginxPlusIngressAnnotations(t *testing.T) { "nginx.org/server-snippets": "snippet-1", }, expectedErrors: nil, - msg: "valid nginx.org/server-snippets annotation, single-line", + msg: "valid nginx.org/server-snippets annotation, single-value", }, { annotations: map[string]string{ "nginx.org/server-snippets": "snippet-1\nsnippet-2\nsnippet-3", }, expectedErrors: nil, - msg: "valid nginx.org/server-snippets annotation, multi-line", + msg: "valid nginx.org/server-snippets annotation, multi-value", }, { @@ -509,14 +622,127 @@ func TestValidateNginxPlusIngressAnnotations(t *testing.T) { "nginx.org/location-snippets": "snippet-1", }, expectedErrors: nil, - msg: "valid nginx.org/location-snippets annotation", + msg: "valid nginx.org/location-snippets annotation, single-value", }, { annotations: map[string]string{ "nginx.org/location-snippets": "snippet-1\nsnippet-2\nsnippet-3", }, expectedErrors: nil, - msg: "valid nginx.org/location-snippets annotation, multi-line", + msg: "valid nginx.org/location-snippets annotation, multi-value", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-connect-timeout": "10s", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-connect-timeout annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-read-timeout": "10s", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-read-timeout annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-send-timeout": "10s", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-send-timeout annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-hide-headers": "header-1", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-hide-headers annotation, single-value", + }, + { + annotations: map[string]string{ + "nginx.org/proxy-hide-headers": "header-1,header-2,header-3", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-hide-headers annotation, multi-value", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-pass-headers": "header-1", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-pass-headers annotation, single-value", + }, + { + annotations: map[string]string{ + "nginx.org/proxy-pass-headers": "header-1,header-2,header-3", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-pass-headers annotation, multi-value", + }, + + { + annotations: map[string]string{ + "nginx.org/client-max-body-size": "16M", + }, + expectedErrors: nil, + msg: "valid nginx.org/client-max-body-size annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/redirect-to-https": "true", + }, + expectedErrors: nil, + msg: "valid nginx.org/redirect-to-https annotation", + }, + { + annotations: map[string]string{ + "nginx.org/redirect-to-https": "not_a_boolean", + }, + expectedErrors: []string{ + `annotations.nginx.org/redirect-to-https: Invalid value: "not_a_boolean": must be a valid boolean`, + }, + msg: "invalid nginx.org/redirect-to-https annotation", + }, + + { + annotations: map[string]string{ + "ingress.kubernetes.io/ssl-redirect": "true", + }, + expectedErrors: nil, + msg: "valid ingress.kubernetes.io/ssl-redirect annotation", + }, + { + annotations: map[string]string{ + "ingress.kubernetes.io/ssl-redirect": "not_a_boolean", + }, + expectedErrors: []string{ + `annotations.ingress.kubernetes.io/ssl-redirect: Invalid value: "not_a_boolean": must be a valid boolean`, + }, + msg: "invalid ingress.kubernetes.io/ssl-redirect annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-buffering": "true", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-buffering annotation", + }, + { + annotations: map[string]string{ + "nginx.org/proxy-buffering": "not_a_boolean", + }, + expectedErrors: []string{ + `annotations.nginx.org/proxy-buffering: Invalid value: "not_a_boolean": must be a valid boolean`, + }, + msg: "invalid nginx.org/proxy-buffering annotation", }, } From 3fcb93ee9f4cca46ba0ab90728b9eb347b8f0d80 Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Wed, 9 Dec 2020 15:55:14 +0000 Subject: [PATCH 03/13] Revert changes to annotations.go to better preserve existing functionality --- internal/configs/annotations.go | 37 +++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index df7254e938..d75ac83680 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -93,29 +93,40 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool if err != nil { glog.Error(err) } - cfgParams.HealthCheckEnabled = healthCheckEnabled + if isPlus { + cfgParams.HealthCheckEnabled = healthCheckEnabled + } else { + glog.Warning("Annotation 'nginx.com/health-checks' requires NGINX Plus") + } } - if healthCheckMandatory, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "nginx.com/health-checks-mandatory", ingEx.Ingress); exists { - if err != nil { - glog.Error(err) + 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 } - cfgParams.HealthCheckMandatory = healthCheckMandatory } - if healthCheckQueue, exists, err := GetMapKeyAsInt64(ingEx.Ingress.Annotations, "nginx.com/health-checks-mandatory-queue", ingEx.Ingress); exists { - if err != nil { - glog.Error(err) + 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 } - cfgParams.HealthCheckMandatoryQueue = healthCheckQueue } if slowStart, exists := ingEx.Ingress.Annotations["nginx.com/slow-start"]; exists { - parsedSlowStart, err := ParseTime(slowStart) - if err != nil { - glog.Error(err) + 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 { - cfgParams.SlowStart = parsedSlowStart + if isPlus { + cfgParams.SlowStart = parsedSlowStart + } else { + glog.Warning("Annotation 'nginx.com/slow-start' requires NGINX Plus") + } } } From 163355aa4f0486eecea7260328fbf8619e2af054 Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Thu, 10 Dec 2020 11:25:28 +0000 Subject: [PATCH 04/13] Add earlier HSTS annotation validation --- internal/k8s/validation.go | 58 ++++++- internal/k8s/validation_test.go | 284 ++++++++++++++++++++++++++++++-- 2 files changed, 327 insertions(+), 15 deletions(-) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index d7d62bfea1..68eb43ed04 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -30,6 +30,10 @@ const ( redirectToHTTPSAnnotation = "nginx.org/redirect-to-https" sslRedirectAnnotation = "ingress.kubernetes.io/ssl-redirect" proxyBufferingAnnotation = "nginx.org/proxy-buffering" + hstsAnnotation = "nginx.org/hsts" + hstsMaxAgeAnnotation = "nginx.org/hsts-max-age" + hstsIncludeSubdomainsAnnotation = "nginx.org/hsts-include-subdomains" + hstsBehindProxyAnnotation = "nginx.org/hsts-behind-proxy" ) type annotationValidationContext struct { @@ -93,6 +97,25 @@ var ( validateRequiredAnnotation, validateBoolAnnotation, }, + hstsAnnotation: { + validateRequiredAnnotation, + validateBoolAnnotation, + }, + hstsMaxAgeAnnotation: { + validateRelatedAnnotation(hstsAnnotation, validateIsTrue), + validateRequiredAnnotation, + validateInt64Annotation, + }, + hstsIncludeSubdomainsAnnotation: { + validateRelatedAnnotation(hstsAnnotation, validateIsTrue), + validateRequiredAnnotation, + validateBoolAnnotation, + }, + hstsBehindProxyAnnotation: { + validateRelatedAnnotation(hstsAnnotation, validateIsTrue), + validateRequiredAnnotation, + validateBoolAnnotation, + }, } nginxAnnotationNames = sortedAnnotationNames(nginxAnnotationValidations) @@ -120,7 +143,7 @@ var ( healthChecksMandatoryQueueAnnotation: { validateRelatedAnnotation(healthChecksMandatoryAnnotation, validateIsTrue), validateRequiredAnnotation, - validateNonNegativeIntAnnotation, + validateUint64Annotation, }, slowStartAnnotation: { validateRequiredAnnotation, @@ -149,6 +172,25 @@ var ( validateRequiredAnnotation, validateBoolAnnotation, }, + hstsAnnotation: { + validateRequiredAnnotation, + validateBoolAnnotation, + }, + hstsMaxAgeAnnotation: { + validateRelatedAnnotation(hstsAnnotation, validateIsTrue), + validateRequiredAnnotation, + validateInt64Annotation, + }, + hstsIncludeSubdomainsAnnotation: { + validateRelatedAnnotation(hstsAnnotation, validateIsTrue), + validateRequiredAnnotation, + validateBoolAnnotation, + }, + hstsBehindProxyAnnotation: { + validateRelatedAnnotation(hstsAnnotation, validateIsTrue), + validateRequiredAnnotation, + validateBoolAnnotation, + }, } nginxPlusAnnotationNames = sortedAnnotationNames(nginxPlusAnnotationValidations) ) @@ -285,7 +327,7 @@ func validatePlusOnlyAnnotation(context *annotationValidationContext) field.Erro 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 append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a boolean")) } return allErrs } @@ -293,12 +335,12 @@ func validateBoolAnnotation(context *annotationValidationContext) field.ErrorLis 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 append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a time")) } return allErrs } -func validateNonNegativeIntAnnotation(context *annotationValidationContext) field.ErrorList { +func validateUint64Annotation(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")) @@ -306,6 +348,14 @@ func validateNonNegativeIntAnnotation(context *annotationValidationContext) fiel return allErrs } +func validateInt64Annotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if _, err := configs.ParseInt64(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be an integer")) + } + return allErrs +} + func validateIsTrue(v string) error { b, err := configs.ParseBool(v) if err != nil { diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index d5361df011..937f6679db 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -246,7 +246,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/server-tokens": "custom_setting", }, expectedErrors: []string{ - `annotations.nginx.org/server-tokens: Invalid value: "custom_setting": must be a valid boolean`, + `annotations.nginx.org/server-tokens: Invalid value: "custom_setting": must be a boolean`, }, msg: "invalid nginx.org/server-tokens annotation, must be a boolean", }, @@ -355,7 +355,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/redirect-to-https": "not_a_boolean", }, expectedErrors: []string{ - `annotations.nginx.org/redirect-to-https: Invalid value: "not_a_boolean": must be a valid boolean`, + `annotations.nginx.org/redirect-to-https: Invalid value: "not_a_boolean": must be a boolean`, }, msg: "invalid nginx.org/redirect-to-https annotation", }, @@ -372,7 +372,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "ingress.kubernetes.io/ssl-redirect": "not_a_boolean", }, expectedErrors: []string{ - `annotations.ingress.kubernetes.io/ssl-redirect: Invalid value: "not_a_boolean": must be a valid boolean`, + `annotations.ingress.kubernetes.io/ssl-redirect: Invalid value: "not_a_boolean": must be a boolean`, }, msg: "invalid ingress.kubernetes.io/ssl-redirect annotation", }, @@ -389,10 +389,141 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/proxy-buffering": "not_a_boolean", }, expectedErrors: []string{ - `annotations.nginx.org/proxy-buffering: Invalid value: "not_a_boolean": must be a valid boolean`, + `annotations.nginx.org/proxy-buffering: Invalid value: "not_a_boolean": must be a boolean`, }, msg: "invalid nginx.org/proxy-buffering annotation", }, + + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + }, + expectedErrors: nil, + msg: "valid nginx.org/hsts annotation", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "not_a_boolean", + }, + expectedErrors: []string{ + `annotations.nginx.org/hsts: Invalid value: "not_a_boolean": must be a boolean`, + }, + msg: "invalid nginx.org/hsts annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + "nginx.org/hsts-max-age": "120", + }, + expectedErrors: nil, + msg: "valid nginx.org/hsts-max-age annotation", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + "nginx.org/hsts-max-age": "not_a_number", + }, + expectedErrors: []string{ + `annotations.nginx.org/hsts-max-age: Invalid value: "not_a_number": must be an integer`, + }, + msg: "invalid nginx.org/hsts-max-age, must be a number", + }, + { + annotations: map[string]string{ + "nginx.org/hsts-max-age": "true", + }, + expectedErrors: []string{ + "annotations.nginx.org/hsts-max-age: Forbidden: related annotation nginx.org/hsts: must be set", + }, + msg: "invalid nginx.org/hsts-max-age, related annotation nginx.org/hsts not set", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "false", + "nginx.org/hsts-max-age": "120", + }, + expectedErrors: []string{ + "annotations.nginx.org/hsts-max-age: Forbidden: related annotation nginx.org/hsts: must be true", + }, + msg: "invalid nginx.org/hsts-max-age nginx.org/hsts is not true", + }, + + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + "nginx.org/hsts-include-subdomains": "true", + }, + expectedErrors: nil, + msg: "valid nginx.org/hsts-include-subdomains annotation", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + "nginx.org/hsts-include-subdomains": "not_a_boolean", + }, + expectedErrors: []string{ + `annotations.nginx.org/hsts-include-subdomains: Invalid value: "not_a_boolean": must be a boolean`, + }, + msg: "invalid nginx.org/hsts-include-subdomains, must be a boolean", + }, + { + annotations: map[string]string{ + "nginx.org/hsts-include-subdomains": "true", + }, + expectedErrors: []string{ + "annotations.nginx.org/hsts-include-subdomains: Forbidden: related annotation nginx.org/hsts: must be set", + }, + msg: "invalid nginx.org/hsts-include-subdomains, related annotation nginx.org/hsts not set", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "false", + "nginx.org/hsts-include-subdomains": "true", + }, + expectedErrors: []string{ + "annotations.nginx.org/hsts-include-subdomains: Forbidden: related annotation nginx.org/hsts: must be true", + }, + msg: "invalid nginx.org/hsts-include-subdomains nginx.org/hsts is not true", + }, + + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + "nginx.org/hsts-behind-proxy": "true", + }, + expectedErrors: nil, + msg: "valid nginx.org/hsts-behind-proxy annotation", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + "nginx.org/hsts-behind-proxy": "not_a_boolean", + }, + expectedErrors: []string{ + `annotations.nginx.org/hsts-behind-proxy: Invalid value: "not_a_boolean": must be a boolean`, + }, + msg: "invalid nginx.org/hsts-behind-proxy, must be a boolean", + }, + { + annotations: map[string]string{ + "nginx.org/hsts-behind-proxy": "true", + }, + expectedErrors: []string{ + "annotations.nginx.org/hsts-behind-proxy: Forbidden: related annotation nginx.org/hsts: must be set", + }, + msg: "invalid nginx.org/hsts-behind-proxy, related annotation nginx.org/hsts not set", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "false", + "nginx.org/hsts-behind-proxy": "true", + }, + expectedErrors: []string{ + "annotations.nginx.org/hsts-behind-proxy: Forbidden: related annotation nginx.org/hsts: must be true", + }, + msg: "invalid nginx.org/hsts-behind-proxy nginx.org/hsts is not true", + }, } for _, test := range tests { @@ -425,7 +556,7 @@ func TestValidateNginxPlusIngressAnnotations(t *testing.T) { "nginx.com/health-checks": "not_a_boolean", }, 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 boolean`, `annotations.nginx.org/lb-method: Invalid value: "invalid_method": Invalid load balancing method: "invalid_method"`, }, msg: "invalid multiple annotations messages in alphabetical order", @@ -493,7 +624,7 @@ func TestValidateNginxPlusIngressAnnotations(t *testing.T) { "nginx.com/health-checks": "not_a_boolean", }, 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 boolean`, }, msg: "invalid nginx.com/health-checks annotation", }, @@ -512,7 +643,7 @@ func TestValidateNginxPlusIngressAnnotations(t *testing.T) { "nginx.com/health-checks-mandatory": "not_a_boolean", }, 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 boolean`, }, msg: "invalid nginx.com/health-checks-mandatory, must be a boolean", }, @@ -589,7 +720,7 @@ func TestValidateNginxPlusIngressAnnotations(t *testing.T) { "nginx.com/slow-start": "not_a_time", }, 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 time`, }, msg: "invalid nginx.com/slow-start annotation", }, @@ -706,7 +837,7 @@ func TestValidateNginxPlusIngressAnnotations(t *testing.T) { "nginx.org/redirect-to-https": "not_a_boolean", }, expectedErrors: []string{ - `annotations.nginx.org/redirect-to-https: Invalid value: "not_a_boolean": must be a valid boolean`, + `annotations.nginx.org/redirect-to-https: Invalid value: "not_a_boolean": must be a boolean`, }, msg: "invalid nginx.org/redirect-to-https annotation", }, @@ -723,7 +854,7 @@ func TestValidateNginxPlusIngressAnnotations(t *testing.T) { "ingress.kubernetes.io/ssl-redirect": "not_a_boolean", }, expectedErrors: []string{ - `annotations.ingress.kubernetes.io/ssl-redirect: Invalid value: "not_a_boolean": must be a valid boolean`, + `annotations.ingress.kubernetes.io/ssl-redirect: Invalid value: "not_a_boolean": must be a boolean`, }, msg: "invalid ingress.kubernetes.io/ssl-redirect annotation", }, @@ -740,10 +871,141 @@ func TestValidateNginxPlusIngressAnnotations(t *testing.T) { "nginx.org/proxy-buffering": "not_a_boolean", }, expectedErrors: []string{ - `annotations.nginx.org/proxy-buffering: Invalid value: "not_a_boolean": must be a valid boolean`, + `annotations.nginx.org/proxy-buffering: Invalid value: "not_a_boolean": must be a boolean`, }, msg: "invalid nginx.org/proxy-buffering annotation", }, + + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + }, + expectedErrors: nil, + msg: "valid nginx.org/hsts annotation", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "not_a_boolean", + }, + expectedErrors: []string{ + `annotations.nginx.org/hsts: Invalid value: "not_a_boolean": must be a boolean`, + }, + msg: "invalid nginx.org/hsts annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + "nginx.org/hsts-max-age": "120", + }, + expectedErrors: nil, + msg: "valid nginx.org/hsts-max-age annotation", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + "nginx.org/hsts-max-age": "not_a_number", + }, + expectedErrors: []string{ + `annotations.nginx.org/hsts-max-age: Invalid value: "not_a_number": must be an integer`, + }, + msg: "invalid nginx.org/hsts-max-age, must be a number", + }, + { + annotations: map[string]string{ + "nginx.org/hsts-max-age": "true", + }, + expectedErrors: []string{ + "annotations.nginx.org/hsts-max-age: Forbidden: related annotation nginx.org/hsts: must be set", + }, + msg: "invalid nginx.org/hsts-max-age, related annotation nginx.org/hsts not set", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "false", + "nginx.org/hsts-max-age": "120", + }, + expectedErrors: []string{ + "annotations.nginx.org/hsts-max-age: Forbidden: related annotation nginx.org/hsts: must be true", + }, + msg: "invalid nginx.org/hsts-max-age nginx.org/hsts is not true", + }, + + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + "nginx.org/hsts-include-subdomains": "true", + }, + expectedErrors: nil, + msg: "valid nginx.org/hsts-include-subdomains annotation", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + "nginx.org/hsts-include-subdomains": "not_a_boolean", + }, + expectedErrors: []string{ + `annotations.nginx.org/hsts-include-subdomains: Invalid value: "not_a_boolean": must be a boolean`, + }, + msg: "invalid nginx.org/hsts-include-subdomains, must be a boolean", + }, + { + annotations: map[string]string{ + "nginx.org/hsts-include-subdomains": "true", + }, + expectedErrors: []string{ + "annotations.nginx.org/hsts-include-subdomains: Forbidden: related annotation nginx.org/hsts: must be set", + }, + msg: "invalid nginx.org/hsts-include-subdomains, related annotation nginx.org/hsts not set", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "false", + "nginx.org/hsts-include-subdomains": "true", + }, + expectedErrors: []string{ + "annotations.nginx.org/hsts-include-subdomains: Forbidden: related annotation nginx.org/hsts: must be true", + }, + msg: "invalid nginx.org/hsts-include-subdomains nginx.org/hsts is not true", + }, + + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + "nginx.org/hsts-behind-proxy": "true", + }, + expectedErrors: nil, + msg: "valid nginx.org/hsts-behind-proxy annotation", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "true", + "nginx.org/hsts-behind-proxy": "not_a_boolean", + }, + expectedErrors: []string{ + `annotations.nginx.org/hsts-behind-proxy: Invalid value: "not_a_boolean": must be a boolean`, + }, + msg: "invalid nginx.org/hsts-behind-proxy, must be a boolean", + }, + { + annotations: map[string]string{ + "nginx.org/hsts-behind-proxy": "true", + }, + expectedErrors: []string{ + "annotations.nginx.org/hsts-behind-proxy: Forbidden: related annotation nginx.org/hsts: must be set", + }, + msg: "invalid nginx.org/hsts-behind-proxy, related annotation nginx.org/hsts not set", + }, + { + annotations: map[string]string{ + "nginx.org/hsts": "false", + "nginx.org/hsts-behind-proxy": "true", + }, + expectedErrors: []string{ + "annotations.nginx.org/hsts-behind-proxy: Forbidden: related annotation nginx.org/hsts: must be true", + }, + msg: "invalid nginx.org/hsts-behind-proxy nginx.org/hsts is not true", + }, } for _, test := range tests { From ad22af10bc55be396148eb97b965e122cbd43834 Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Thu, 10 Dec 2020 12:12:34 +0000 Subject: [PATCH 05/13] Add earlier jwt and proxy buffer annotation validation --- internal/k8s/validation.go | 32 ++++++++ internal/k8s/validation_test.go | 136 ++++++++++++++++++++++++++++++++ 2 files changed, 168 insertions(+) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 68eb43ed04..d997bb5af9 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -34,6 +34,14 @@ const ( hstsMaxAgeAnnotation = "nginx.org/hsts-max-age" hstsIncludeSubdomainsAnnotation = "nginx.org/hsts-include-subdomains" hstsBehindProxyAnnotation = "nginx.org/hsts-behind-proxy" + proxyBuffersAnnotation = "nginx.org/proxy-buffers" + proxyBufferSizeAnnotation = "nginx.org/proxy-buffer-size" + proxyMaxTempFileSizeAnnotation = "nginx.org/proxy-max-temp-file-size" + upstreamZoneSizeAnnotation = "nginx.org/upstream-zone-size" + jwtRealmAnnotation = "nginx.com/jwt-realm" + jwtKeyAnnotation = "nginx.com/jwt-key" + jwtTokenAnnotation = "nginx.com/jwt-token" + jwtLoginURLAnnotation = "nginx.com/jwt-login-url" ) type annotationValidationContext struct { @@ -116,6 +124,22 @@ var ( validateRequiredAnnotation, validateBoolAnnotation, }, + proxyBuffersAnnotation: {}, + proxyBufferSizeAnnotation: {}, + proxyMaxTempFileSizeAnnotation: {}, + upstreamZoneSizeAnnotation: {}, + jwtRealmAnnotation: { + validatePlusOnlyAnnotation, + }, + jwtKeyAnnotation: { + validatePlusOnlyAnnotation, + }, + jwtTokenAnnotation: { + validatePlusOnlyAnnotation, + }, + jwtLoginURLAnnotation: { + validatePlusOnlyAnnotation, + }, } nginxAnnotationNames = sortedAnnotationNames(nginxAnnotationValidations) @@ -191,6 +215,14 @@ var ( validateRequiredAnnotation, validateBoolAnnotation, }, + proxyBuffersAnnotation: {}, + proxyBufferSizeAnnotation: {}, + proxyMaxTempFileSizeAnnotation: {}, + upstreamZoneSizeAnnotation: {}, + jwtRealmAnnotation: {}, + jwtKeyAnnotation: {}, + jwtTokenAnnotation: {}, + jwtLoginURLAnnotation: {}, } nginxPlusAnnotationNames = sortedAnnotationNames(nginxPlusAnnotationValidations) ) diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 937f6679db..7798fb3be6 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -524,6 +524,78 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }, msg: "invalid nginx.org/hsts-behind-proxy nginx.org/hsts is not true", }, + + { + annotations: map[string]string{ + "nginx.org/proxy-buffers": "8 8k", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-buffers annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-buffer-size": "16k", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-buffer-size annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-max-temp-file-size": "128M", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-max-temp-file-size annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/upstream-zone-size": "512k", + }, + expectedErrors: nil, + msg: "valid nginx.org/upstream-zone-size annotation", + }, + + { + annotations: map[string]string{ + "nginx.com/jwt-realm": "true", + }, + expectedErrors: []string{ + "annotations.nginx.com/jwt-realm: Forbidden: annotation requires NGINX Plus", + }, + msg: "invalid nginx.com/jwt-realm annotation, nginx plus only", + }, + + { + annotations: map[string]string{ + "nginx.com/jwt-key": "true", + }, + expectedErrors: []string{ + "annotations.nginx.com/jwt-key: Forbidden: annotation requires NGINX Plus", + }, + msg: "invalid nginx.com/jwt-key annotation, nginx plus only", + }, + + { + annotations: map[string]string{ + "nginx.com/jwt-token": "true", + }, + expectedErrors: []string{ + "annotations.nginx.com/jwt-token: Forbidden: annotation requires NGINX Plus", + }, + msg: "invalid nginx.com/jwt-token annotation, nginx plus only", + }, + + { + annotations: map[string]string{ + "nginx.com/jwt-login-url": "true", + }, + expectedErrors: []string{ + "annotations.nginx.com/jwt-login-url: Forbidden: annotation requires NGINX Plus", + }, + msg: "invalid nginx.com/jwt-login-url annotation, nginx plus only", + }, } for _, test := range tests { @@ -1006,6 +1078,70 @@ func TestValidateNginxPlusIngressAnnotations(t *testing.T) { }, msg: "invalid nginx.org/hsts-behind-proxy nginx.org/hsts is not true", }, + + { + annotations: map[string]string{ + "nginx.org/proxy-buffers": "8 8k", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-buffers annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-buffer-size": "16k", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-buffer-size annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/proxy-max-temp-file-size": "128M", + }, + expectedErrors: nil, + msg: "valid nginx.org/proxy-max-temp-file-size annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/upstream-zone-size": "512k", + }, + expectedErrors: nil, + msg: "valid nginx.org/upstream-zone-size annotation", + }, + + { + annotations: map[string]string{ + "nginx.com/jwt-realm": "my-jwt-realm", + }, + expectedErrors: nil, + msg: "valid nginx.com/jwt-realm annotation", + }, + + { + annotations: map[string]string{ + "nginx.com/jwt-key": "my-jwk", + }, + expectedErrors: nil, + msg: "valid nginx.com/jwt-key annotation", + }, + + { + annotations: map[string]string{ + "nginx.com/jwt-token": "$cookie_auth_token", + }, + expectedErrors: nil, + msg: "valid nginx.com/jwt-token annotation", + }, + + { + annotations: map[string]string{ + "nginx.com/jwt-login-url": "https://login.example.com", + }, + expectedErrors: nil, + msg: "valid nginx.com/jwt-login-url annotation", + }, } for _, test := range tests { From bd30691aa6861f10b8013667a4274abcfa5da28a Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Thu, 10 Dec 2020 13:01:53 +0000 Subject: [PATCH 06/13] Add earlier listen ports annotation validation --- internal/configs/annotations.go | 56 --------- internal/configs/parsing_helpers.go | 61 +++++++++ internal/k8s/validation.go | 64 ++++++++++ internal/k8s/validation_test.go | 186 ++++++++++++++++++++++++++++ 4 files changed, 311 insertions(+), 56 deletions(-) diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index d75ac83680..660739a34b 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -1,8 +1,6 @@ package configs import ( - "fmt" - "strconv" "strings" "github.com/golang/glog" @@ -493,57 +491,3 @@ func mergeMasterAnnotationsIntoMinion(minionAnnotations map[string]string, maste } } } - -func parsePort(value string) (int, error) { - port, err := strconv.ParseInt(value, 10, 16) - if err != nil { - return 0, fmt.Errorf( - "Unable to parse port as integer: %s", - err, - ) - } - - if port <= 0 { - return 0, fmt.Errorf( - "Port number should be greater than zero: %q", - port, - ) - } - - return int(port), nil -} - -func parseStickyService(service string) (serviceName string, stickyCookie string, err error) { - parts := strings.SplitN(service, " ", 2) - - if len(parts) != 2 { - return "", "", fmt.Errorf("Invalid sticky-cookie service format: %s", service) - } - - svcNameParts := strings.Split(parts[0], "=") - if len(svcNameParts) != 2 { - return "", "", fmt.Errorf("Invalid sticky-cookie service format: %s", svcNameParts) - } - - return svcNameParts[1], parts[1], nil -} - -func parseRewrites(service string) (serviceName string, rewrite string, err error) { - parts := strings.SplitN(strings.TrimSpace(service), " ", 2) - - if len(parts) != 2 { - return "", "", fmt.Errorf("Invalid rewrite format: %s", service) - } - - svcNameParts := strings.Split(parts[0], "=") - if len(svcNameParts) != 2 { - return "", "", fmt.Errorf("Invalid rewrite format: %s", svcNameParts) - } - - rwPathParts := strings.Split(parts[1], "=") - if len(rwPathParts) != 2 { - return "", "", fmt.Errorf("Invalid rewrite format: %s", rwPathParts) - } - - return svcNameParts[1], rwPathParts[1], nil -} diff --git a/internal/configs/parsing_helpers.go b/internal/configs/parsing_helpers.go index 937486820d..f67339386b 100644 --- a/internal/configs/parsing_helpers.go +++ b/internal/configs/parsing_helpers.go @@ -225,6 +225,67 @@ func ParseSize(s string) (string, error) { return "", errors.New("Invalid size string") } +// ParsePortList ensures that the string is a comma-separated list of port numbers +func ParsePortList(s string) ([]int, error) { + ports := make([]int, 0) + for _, value := range strings.Split(s, ",") { + port, err := parsePort(value) + if err != nil { + return nil, err + } + ports = append(ports, port) + } + return ports, nil +} + +func parsePort(value string) (int, error) { + port, err := strconv.ParseInt(value, 10, 16) + if err != nil { + return 0, fmt.Errorf("Unable to parse port as integer: %s", err) + } + + if port <= 0 { + return 0, fmt.Errorf("Port number should be greater than zero: %q", port) + } + + return int(port), nil +} + +func parseStickyService(service string) (serviceName string, stickyCookie string, err error) { + parts := strings.SplitN(service, " ", 2) + + if len(parts) != 2 { + return "", "", fmt.Errorf("Invalid sticky-cookie service format: %s", service) + } + + svcNameParts := strings.Split(parts[0], "=") + if len(svcNameParts) != 2 { + return "", "", fmt.Errorf("Invalid sticky-cookie service format: %s", svcNameParts) + } + + return svcNameParts[1], parts[1], nil +} + +func parseRewrites(service string) (serviceName string, rewrite string, err error) { + parts := strings.SplitN(strings.TrimSpace(service), " ", 2) + + if len(parts) != 2 { + return "", "", fmt.Errorf("Invalid rewrite format: %s", service) + } + + svcNameParts := strings.Split(parts[0], "=") + if len(svcNameParts) != 2 { + return "", "", fmt.Errorf("Invalid rewrite format: %s", svcNameParts) + } + + rwPathParts := strings.Split(parts[1], "=") + if len(rwPathParts) != 2 { + return "", "", fmt.Errorf("Invalid rewrite format: %s", rwPathParts) + } + + return svcNameParts[1], rwPathParts[1], nil +} + 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/k8s/validation.go b/internal/k8s/validation.go index d997bb5af9..3fe95c6077 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -42,6 +42,12 @@ const ( jwtKeyAnnotation = "nginx.com/jwt-key" jwtTokenAnnotation = "nginx.com/jwt-token" jwtLoginURLAnnotation = "nginx.com/jwt-login-url" + listenPortsAnnotation = "nginx.org/listen-ports" + listenPortsSSLAnnotation = "nginx.org/listen-ports-ssl" + keepaliveAnnotation = "nginx.org/keepalive" + maxFailsAnnotation = "nginx.org/max-fails" + maxConnsAnnotation = "nginx.org/max-conns" + failTimeoutAnnotation = "nginx.org/fail-timeout" ) type annotationValidationContext struct { @@ -140,6 +146,27 @@ var ( jwtLoginURLAnnotation: { validatePlusOnlyAnnotation, }, + listenPortsAnnotation: { + validateRequiredAnnotation, + validatePortListAnnotation, + }, + listenPortsSSLAnnotation: { + validateRequiredAnnotation, + validatePortListAnnotation, + }, + keepaliveAnnotation: { + validateRequiredAnnotation, + validateIntAnnotation, + }, + maxFailsAnnotation: { + validateRequiredAnnotation, + validateIntAnnotation, + }, + maxConnsAnnotation: { + validateRequiredAnnotation, + validateIntAnnotation, + }, + failTimeoutAnnotation: {}, } nginxAnnotationNames = sortedAnnotationNames(nginxAnnotationValidations) @@ -223,6 +250,27 @@ var ( jwtKeyAnnotation: {}, jwtTokenAnnotation: {}, jwtLoginURLAnnotation: {}, + listenPortsAnnotation: { + validateRequiredAnnotation, + validatePortListAnnotation, + }, + listenPortsSSLAnnotation: { + validateRequiredAnnotation, + validatePortListAnnotation, + }, + keepaliveAnnotation: { + validateRequiredAnnotation, + validateIntAnnotation, + }, + maxFailsAnnotation: { + validateRequiredAnnotation, + validateIntAnnotation, + }, + maxConnsAnnotation: { + validateRequiredAnnotation, + validateIntAnnotation, + }, + failTimeoutAnnotation: {}, } nginxPlusAnnotationNames = sortedAnnotationNames(nginxPlusAnnotationValidations) ) @@ -388,6 +436,22 @@ func validateInt64Annotation(context *annotationValidationContext) field.ErrorLi return allErrs } +func validateIntAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if _, err := configs.ParseInt(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be an integer")) + } + return allErrs +} + +func validatePortListAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if _, err := configs.ParsePortList(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a comma-separated list of port numbers")) + } + return allErrs +} + func validateIsTrue(v string) error { b, err := configs.ParseBool(v) if err != nil { diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 7798fb3be6..0e20cb5b48 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -596,6 +596,99 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }, msg: "invalid nginx.com/jwt-login-url annotation, nginx plus only", }, + + { + annotations: map[string]string{ + "nginx.org/listen-ports": "80,8080,9090", + }, + expectedErrors: nil, + msg: "valid nginx.org/listen-ports annotation", + }, + { + annotations: map[string]string{ + "nginx.org/listen-ports": "not_a_port_list", + }, + expectedErrors: []string{ + `annotations.nginx.org/listen-ports: Invalid value: "not_a_port_list": must be a comma-separated list of port numbers`, + }, + msg: "invalid nginx.org/listen-ports annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/listen-ports-ssl": "443,8443", + }, + expectedErrors: nil, + msg: "valid nginx.org/listen-ports-ssl annotation", + }, + { + annotations: map[string]string{ + "nginx.org/listen-ports-ssl": "not_a_port_list", + }, + expectedErrors: []string{ + `annotations.nginx.org/listen-ports-ssl: Invalid value: "not_a_port_list": must be a comma-separated list of port numbers`, + }, + msg: "invalid nginx.org/listen-ports-ssl annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/keepalive": "1000", + }, + expectedErrors: nil, + msg: "valid nginx.org/keepalive annotation", + }, + { + annotations: map[string]string{ + "nginx.org/keepalive": "not_a_number", + }, + expectedErrors: []string{ + `annotations.nginx.org/keepalive: Invalid value: "not_a_number": must be an integer`, + }, + msg: "invalid nginx.org/keepalive annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/max-fails": "5", + }, + expectedErrors: nil, + msg: "valid nginx.org/max-fails annotation", + }, + { + annotations: map[string]string{ + "nginx.org/max-fails": "not_a_number", + }, + expectedErrors: []string{ + `annotations.nginx.org/max-fails: Invalid value: "not_a_number": must be an integer`, + }, + msg: "invalid nginx.org/max-fails annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/max-conns": "10", + }, + expectedErrors: nil, + msg: "valid nginx.org/max-conns annotation", + }, + { + annotations: map[string]string{ + "nginx.org/max-conns": "not_a_number", + }, + expectedErrors: []string{ + `annotations.nginx.org/max-conns: Invalid value: "not_a_number": must be an integer`, + }, + msg: "invalid nginx.org/max-conns annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/fail-timeout": "10s", + }, + expectedErrors: nil, + msg: "valid nginx.org/fail-timeout annotation", + }, } for _, test := range tests { @@ -1142,6 +1235,99 @@ func TestValidateNginxPlusIngressAnnotations(t *testing.T) { expectedErrors: nil, msg: "valid nginx.com/jwt-login-url annotation", }, + + { + annotations: map[string]string{ + "nginx.org/listen-ports": "80,8080,9090", + }, + expectedErrors: nil, + msg: "valid nginx.org/listen-ports annotation", + }, + { + annotations: map[string]string{ + "nginx.org/listen-ports": "not_a_port_list", + }, + expectedErrors: []string{ + `annotations.nginx.org/listen-ports: Invalid value: "not_a_port_list": must be a comma-separated list of port numbers`, + }, + msg: "invalid nginx.org/listen-ports annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/listen-ports-ssl": "443,8443", + }, + expectedErrors: nil, + msg: "valid nginx.org/listen-ports-ssl annotation", + }, + { + annotations: map[string]string{ + "nginx.org/listen-ports-ssl": "not_a_port_list", + }, + expectedErrors: []string{ + `annotations.nginx.org/listen-ports-ssl: Invalid value: "not_a_port_list": must be a comma-separated list of port numbers`, + }, + msg: "invalid nginx.org/listen-ports-ssl annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/keepalive": "1000", + }, + expectedErrors: nil, + msg: "valid nginx.org/keepalive annotation", + }, + { + annotations: map[string]string{ + "nginx.org/keepalive": "not_a_number", + }, + expectedErrors: []string{ + `annotations.nginx.org/keepalive: Invalid value: "not_a_number": must be an integer`, + }, + msg: "invalid nginx.org/keepalive annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/max-fails": "5", + }, + expectedErrors: nil, + msg: "valid nginx.org/max-fails annotation", + }, + { + annotations: map[string]string{ + "nginx.org/max-fails": "not_a_number", + }, + expectedErrors: []string{ + `annotations.nginx.org/max-fails: Invalid value: "not_a_number": must be an integer`, + }, + msg: "invalid nginx.org/max-fails annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/max-conns": "10", + }, + expectedErrors: nil, + msg: "valid nginx.org/max-conns annotation", + }, + { + annotations: map[string]string{ + "nginx.org/max-conns": "not_a_number", + }, + expectedErrors: []string{ + `annotations.nginx.org/max-conns: Invalid value: "not_a_number": must be an integer`, + }, + msg: "invalid nginx.org/max-conns annotation", + }, + + { + annotations: map[string]string{ + "nginx.org/fail-timeout": "10s", + }, + expectedErrors: nil, + msg: "valid nginx.org/fail-timeout annotation", + }, } for _, test := range tests { From 1b698532fbec0e2c0e63ff8999b1e87e07f7a2a4 Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Thu, 10 Dec 2020 14:23:52 +0000 Subject: [PATCH 07/13] Add earlier app protect and internal routes annotation validation --- internal/k8s/configuration.go | 16 +- internal/k8s/configuration_test.go | 12 +- internal/k8s/controller.go | 7 +- internal/k8s/validation.go | 168 ++-- internal/k8s/validation_test.go | 1206 ++++++++++++---------------- 5 files changed, 683 insertions(+), 726 deletions(-) diff --git a/internal/k8s/configuration.go b/internal/k8s/configuration.go index 971e65ef06..11f161f65b 100644 --- a/internal/k8s/configuration.go +++ b/internal/k8s/configuration.go @@ -307,13 +307,21 @@ type Configuration struct { appPolicyReferenceChecker *appProtectResourceReferenceChecker appLogConfReferenceChecker *appProtectResourceReferenceChecker - isPlus bool + isPlus bool + appProtectEnabled bool + internalRoutesEnabled bool lock sync.RWMutex } // NewConfiguration creates a new Configuration. -func NewConfiguration(hasCorrectIngressClass func(interface{}) bool, isPlus bool, virtualServerValidator *validation.VirtualServerValidator) *Configuration { +func NewConfiguration( + hasCorrectIngressClass func(interface{}) bool, + isPlus bool, + appProtectEnabled bool, + internalRoutesEnabled bool, + virtualServerValidator *validation.VirtualServerValidator, +) *Configuration { return &Configuration{ hosts: make(map[string]Resource), ingresses: make(map[string]*networking.Ingress), @@ -328,6 +336,8 @@ func NewConfiguration(hasCorrectIngressClass func(interface{}) bool, isPlus bool appPolicyReferenceChecker: newAppProtectResourceReferenceChecker(configs.AppProtectPolicyAnnotation), appLogConfReferenceChecker: newAppProtectResourceReferenceChecker(configs.AppProtectLogConfAnnotation), isPlus: isPlus, + appProtectEnabled: appProtectEnabled, + internalRoutesEnabled: internalRoutesEnabled, } } @@ -342,7 +352,7 @@ func (c *Configuration) AddOrUpdateIngress(ing *networking.Ingress) ([]ResourceC if !c.hasCorrectIngressClass(ing) { delete(c.ingresses, key) } else { - validationError = validateIngress(ing, c.isPlus).ToAggregate() + validationError = validateIngress(ing, c.isPlus, c.appProtectEnabled, c.internalRoutesEnabled).ToAggregate() if validationError != nil { delete(c.ingresses, key) } else { diff --git a/internal/k8s/configuration_test.go b/internal/k8s/configuration_test.go index 5f47c79baa..4481cd7b5b 100644 --- a/internal/k8s/configuration_test.go +++ b/internal/k8s/configuration_test.go @@ -16,8 +16,16 @@ func createTestConfiguration() *Configuration { ingressClass: "nginx", useIngressClassOnly: true, } - isPlus := false - return NewConfiguration(lbc.HasCorrectIngressClass, isPlus, validation.NewVirtualServerValidator(isPlus)) + var isPlus bool + var appProtectEnabled bool + var internalRoutesEnabled bool + return NewConfiguration( + lbc.HasCorrectIngressClass, + isPlus, + appProtectEnabled, + internalRoutesEnabled, + validation.NewVirtualServerValidator(isPlus), + ) } func TestAddIngressForRegularIngress(t *testing.T) { diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 8ad39fbb6e..20524f8ddf 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -289,7 +289,12 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc confClient: input.ConfClient, } - lbc.configuration = NewConfiguration(lbc.HasCorrectIngressClass, input.IsNginxPlus, input.VirtualServerValidator) + lbc.configuration = NewConfiguration( + lbc.HasCorrectIngressClass, + input.IsNginxPlus, + input.AppProtectEnabled, + input.InternalRoutesEnabled, + input.VirtualServerValidator) lbc.secretStore = secrets.NewLocalSecretStore(lbc.configurator) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 3fe95c6077..968bae4700 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -12,50 +12,55 @@ import ( ) 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" - slowStartAnnotation = "nginx.com/slow-start" - serverTokensAnnotation = "nginx.org/server-tokens" - serverSnippetsAnnotation = "nginx.org/server-snippets" - locationSnippetsAnnotation = "nginx.org/location-snippets" - proxyConnectTimeoutAnnotation = "nginx.org/proxy-connect-timeout" - proxyReadTimeoutAnnotation = "nginx.org/proxy-read-timeout" - proxySendTimeoutAnnotation = "nginx.org/proxy-send-timeout" - proxyHideHeadersAnnotation = "nginx.org/proxy-hide-headers" - proxyPassHeadersAnnotation = "nginx.org/proxy-pass-headers" - clientMaxBodySizeAnnotation = "nginx.org/client-max-body-size" - redirectToHTTPSAnnotation = "nginx.org/redirect-to-https" - sslRedirectAnnotation = "ingress.kubernetes.io/ssl-redirect" - proxyBufferingAnnotation = "nginx.org/proxy-buffering" - hstsAnnotation = "nginx.org/hsts" - hstsMaxAgeAnnotation = "nginx.org/hsts-max-age" - hstsIncludeSubdomainsAnnotation = "nginx.org/hsts-include-subdomains" - hstsBehindProxyAnnotation = "nginx.org/hsts-behind-proxy" - proxyBuffersAnnotation = "nginx.org/proxy-buffers" - proxyBufferSizeAnnotation = "nginx.org/proxy-buffer-size" - proxyMaxTempFileSizeAnnotation = "nginx.org/proxy-max-temp-file-size" - upstreamZoneSizeAnnotation = "nginx.org/upstream-zone-size" - jwtRealmAnnotation = "nginx.com/jwt-realm" - jwtKeyAnnotation = "nginx.com/jwt-key" - jwtTokenAnnotation = "nginx.com/jwt-token" - jwtLoginURLAnnotation = "nginx.com/jwt-login-url" - listenPortsAnnotation = "nginx.org/listen-ports" - listenPortsSSLAnnotation = "nginx.org/listen-ports-ssl" - keepaliveAnnotation = "nginx.org/keepalive" - maxFailsAnnotation = "nginx.org/max-fails" - maxConnsAnnotation = "nginx.org/max-conns" - failTimeoutAnnotation = "nginx.org/fail-timeout" + 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" + slowStartAnnotation = "nginx.com/slow-start" + serverTokensAnnotation = "nginx.org/server-tokens" + serverSnippetsAnnotation = "nginx.org/server-snippets" + locationSnippetsAnnotation = "nginx.org/location-snippets" + proxyConnectTimeoutAnnotation = "nginx.org/proxy-connect-timeout" + proxyReadTimeoutAnnotation = "nginx.org/proxy-read-timeout" + proxySendTimeoutAnnotation = "nginx.org/proxy-send-timeout" + proxyHideHeadersAnnotation = "nginx.org/proxy-hide-headers" + proxyPassHeadersAnnotation = "nginx.org/proxy-pass-headers" + clientMaxBodySizeAnnotation = "nginx.org/client-max-body-size" + redirectToHTTPSAnnotation = "nginx.org/redirect-to-https" + sslRedirectAnnotation = "ingress.kubernetes.io/ssl-redirect" + proxyBufferingAnnotation = "nginx.org/proxy-buffering" + hstsAnnotation = "nginx.org/hsts" + hstsMaxAgeAnnotation = "nginx.org/hsts-max-age" + hstsIncludeSubdomainsAnnotation = "nginx.org/hsts-include-subdomains" + hstsBehindProxyAnnotation = "nginx.org/hsts-behind-proxy" + proxyBuffersAnnotation = "nginx.org/proxy-buffers" + proxyBufferSizeAnnotation = "nginx.org/proxy-buffer-size" + proxyMaxTempFileSizeAnnotation = "nginx.org/proxy-max-temp-file-size" + upstreamZoneSizeAnnotation = "nginx.org/upstream-zone-size" + jwtRealmAnnotation = "nginx.com/jwt-realm" + jwtKeyAnnotation = "nginx.com/jwt-key" + jwtTokenAnnotation = "nginx.com/jwt-token" + jwtLoginURLAnnotation = "nginx.com/jwt-login-url" + listenPortsAnnotation = "nginx.org/listen-ports" + listenPortsSSLAnnotation = "nginx.org/listen-ports-ssl" + keepaliveAnnotation = "nginx.org/keepalive" + maxFailsAnnotation = "nginx.org/max-fails" + maxConnsAnnotation = "nginx.org/max-conns" + failTimeoutAnnotation = "nginx.org/fail-timeout" + appProtectEnableAnnotation = "appprotect.f5.com/app-protect-enable" + appProtectSecurityLogEnableAnnotation = "appprotect.f5.com/app-protect-security-log-enable" + internalRouteAnnotation = "nsm.nginx.com/internal-route" ) type annotationValidationContext struct { - annotations map[string]string - name string - value string - isPlus bool - fieldPath *field.Path + annotations map[string]string + name string + value string + isPlus bool + appProtectEnabled bool + internalRoutesEnabled bool + fieldPath *field.Path } type annotationValidationFunc func(context *annotationValidationContext) field.ErrorList @@ -167,6 +172,21 @@ var ( validateIntAnnotation, }, failTimeoutAnnotation: {}, + appProtectEnableAnnotation: { + validateAppProtectOnlyAnnotation, + validateRequiredAnnotation, + validateBoolAnnotation, + }, + appProtectSecurityLogEnableAnnotation: { + validateAppProtectOnlyAnnotation, + validateRequiredAnnotation, + validateBoolAnnotation, + }, + internalRouteAnnotation: { + validateInternalRoutesOnlyAnnotation, + validateRequiredAnnotation, + validateBoolAnnotation, + }, } nginxAnnotationNames = sortedAnnotationNames(nginxAnnotationValidations) @@ -271,6 +291,21 @@ var ( validateIntAnnotation, }, failTimeoutAnnotation: {}, + appProtectEnableAnnotation: { + validateAppProtectOnlyAnnotation, + validateRequiredAnnotation, + validateBoolAnnotation, + }, + appProtectSecurityLogEnableAnnotation: { + validateAppProtectOnlyAnnotation, + validateRequiredAnnotation, + validateBoolAnnotation, + }, + internalRouteAnnotation: { + validateInternalRoutesOnlyAnnotation, + validateRequiredAnnotation, + validateBoolAnnotation, + }, } nginxPlusAnnotationNames = sortedAnnotationNames(nginxPlusAnnotationValidations) ) @@ -286,9 +321,20 @@ func sortedAnnotationNames(annotationValidations annotationValidationConfig) []s // 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, isPlus bool) field.ErrorList { +func validateIngress( + ing *networking.Ingress, + isPlus bool, + appProtectEnabled bool, + internalRoutesEnabled bool, +) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, validateIngressAnnotations(ing.Annotations, isPlus, field.NewPath("annotations"))...) + allErrs = append(allErrs, validateIngressAnnotations( + ing.Annotations, + isPlus, + appProtectEnabled, + internalRoutesEnabled, + field.NewPath("annotations"), + )...) allErrs = append(allErrs, validateIngressSpec(&ing.Spec, field.NewPath("spec"))...) @@ -301,7 +347,13 @@ func validateIngress(ing *networking.Ingress, isPlus bool) field.ErrorList { return allErrs } -func validateIngressAnnotations(annotations map[string]string, isPlus bool, fieldPath *field.Path) field.ErrorList { +func validateIngressAnnotations( + annotations map[string]string, + isPlus bool, + appProtectEnabled bool, + internalRoutesEnabled bool, + fieldPath *field.Path, +) field.ErrorList { allErrs := field.ErrorList{} var annotationNames []string @@ -314,11 +366,13 @@ func validateIngressAnnotations(annotations map[string]string, isPlus bool, fiel for _, name := range annotationNames { if value, exists := annotations[name]; exists { context := &annotationValidationContext{ - annotations: annotations, - name: name, - value: value, - isPlus: isPlus, - fieldPath: fieldPath.Child(name), + annotations: annotations, + name: name, + value: value, + isPlus: isPlus, + appProtectEnabled: appProtectEnabled, + internalRoutesEnabled: internalRoutesEnabled, + fieldPath: fieldPath.Child(name), } allErrs = append(allErrs, validateIngressAnnotation(context)...) } @@ -404,6 +458,22 @@ func validatePlusOnlyAnnotation(context *annotationValidationContext) field.Erro return allErrs } +func validateAppProtectOnlyAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if !context.appProtectEnabled { + return append(allErrs, field.Forbidden(context.fieldPath, "annotation requires AppProtect")) + } + return allErrs +} + +func validateInternalRoutesOnlyAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if !context.internalRoutesEnabled { + return append(allErrs, field.Forbidden(context.fieldPath, "annotation requires Internal Routes enabled")) + } + return allErrs +} + func validateBoolAnnotation(context *annotationValidationContext) field.ErrorList { allErrs := field.ErrorList{} if _, err := configs.ParseBool(context.value); err != nil { diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 0e20cb5b48..9908351764 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -13,10 +13,12 @@ import ( func TestValidateIngress(t *testing.T) { tests := []struct { - ing *networking.Ingress - isPlus bool - expectedErrors []string - msg string + ing *networking.Ingress + isPlus bool + appProtectEnabled bool + internalRoutesEnabled bool + expectedErrors []string + msg string }{ { ing: &networking.Ingress{ @@ -28,9 +30,11 @@ func TestValidateIngress(t *testing.T) { }, }, }, - isPlus: false, - expectedErrors: nil, - msg: "valid input", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid input", }, { ing: &networking.Ingress{ @@ -47,7 +51,9 @@ func TestValidateIngress(t *testing.T) { }, }, }, - isPlus: false, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: 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", @@ -78,7 +84,9 @@ func TestValidateIngress(t *testing.T) { }, }, }, - isPlus: false, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "spec.rules[0].http.paths: Too many: 1: must have at most 0 items", }, @@ -100,7 +108,9 @@ func TestValidateIngress(t *testing.T) { }, }, }, - isPlus: false, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "spec.rules[0].http.paths: Required value: must include at least one path", }, @@ -109,7 +119,7 @@ func TestValidateIngress(t *testing.T) { } for _, test := range tests { - allErrs := validateIngress(test.ing, test.isPlus) + allErrs := validateIngress(test.ing, test.isPlus, test.appProtectEnabled, test.internalRoutesEnabled) assertion := assertErrors("validateIngress()", test.msg, allErrs, test.expectedErrors) if assertion != "" { t.Error(assertion) @@ -118,16 +128,21 @@ func TestValidateIngress(t *testing.T) { } func TestValidateNginxIngressAnnotations(t *testing.T) { - isPlus := false tests := []struct { - annotations map[string]string - expectedErrors []string - msg string + annotations map[string]string + isPlus bool + appProtectEnabled bool + internalRoutesEnabled bool + expectedErrors []string + msg string }{ { - annotations: map[string]string{}, - expectedErrors: nil, - msg: "valid no annotations", + annotations: map[string]string{}, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid no annotations", }, { @@ -135,6 +150,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/lb-method": "invalid_method", "nginx.org/mergeable-ingress-type": "invalid", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `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'`, @@ -146,20 +164,29 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "master", }, - expectedErrors: nil, - msg: "valid input with master annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + 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", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid input with minion annotation", }, { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "annotations.nginx.org/mergeable-ingress-type: Required value", }, @@ -169,6 +196,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "abc", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/mergeable-ingress-type: Invalid value: "abc": must be one of: 'master' or 'minion'`, }, @@ -179,13 +209,19 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/lb-method": "random", }, - expectedErrors: nil, - msg: "valid nginx.org/lb-method annotation, nginx normal", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/lb-method annotation, nginx normal", }, { annotations: map[string]string{ "nginx.org/lb-method": "least_time header", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/lb-method: Invalid value: "least_time header": Invalid load balancing method: "least_time header"`, }, @@ -195,6 +231,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/lb-method": "invalid_method", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/lb-method: Invalid value: "invalid_method": Invalid load balancing method: "invalid_method"`, }, @@ -205,46 +244,206 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/health-checks": "true", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: 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": "true", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.com/health-checks annotation", + }, + { + annotations: map[string]string{ + "nginx.com/health-checks": "not_a_boolean", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.nginx.com/health-checks: Invalid value: "not_a_boolean": must be a boolean`, + }, + msg: "invalid nginx.com/health-checks annotation", + }, { annotations: map[string]string{ "nginx.com/health-checks-mandatory": "true", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, 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": "true", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + 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, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.nginx.com/health-checks-mandatory: Invalid value: "not_a_boolean": must be a boolean`, + }, + msg: "invalid nginx.com/health-checks-mandatory, must be a boolean", + }, + { + annotations: map[string]string{ + "nginx.com/health-checks-mandatory": "true", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + 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, + appProtectEnabled: false, + internalRoutesEnabled: false, + 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-mandatory-queue": "true", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "annotations.nginx.com/health-checks-mandatory-queue: Forbidden: annotation requires NGINX Plus", }, msg: "invalid nginx.com/health-checks-mandatory-queue annotation, nginx plus only", }, + { + annotations: map[string]string{ + "nginx.com/health-checks": "true", + "nginx.com/health-checks-mandatory": "true", + "nginx.com/health-checks-mandatory-queue": "5", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + 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, + appProtectEnabled: false, + internalRoutesEnabled: false, + 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 number", + }, + { + annotations: map[string]string{ + "nginx.com/health-checks-mandatory-queue": "5", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + 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, + appProtectEnabled: false, + internalRoutesEnabled: false, + 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", + }, { annotations: map[string]string{ "nginx.com/slow-start": "true", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: 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": "60s", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.com/slow-start annotation", + }, + { + annotations: map[string]string{ + "nginx.com/slow-start": "not_a_time", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.nginx.com/slow-start: Invalid value: "not_a_time": must be a time`, + }, + msg: "invalid nginx.com/slow-start annotation", + }, { annotations: map[string]string{ "nginx.org/server-tokens": "custom_setting", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/server-tokens: Invalid value: "custom_setting": must be a boolean`, }, @@ -255,105 +454,147 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/server-snippets": "snippet-1", }, - expectedErrors: nil, - msg: "valid nginx.org/server-snippets annotation, single-value", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/server-snippets annotation, single-value", }, { annotations: map[string]string{ "nginx.org/server-snippets": "snippet-1\nsnippet-2\nsnippet-3", }, - expectedErrors: nil, - msg: "valid nginx.org/server-snippets annotation, multi-value", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/server-snippets annotation, multi-value", }, { annotations: map[string]string{ "nginx.org/location-snippets": "snippet-1", }, - expectedErrors: nil, - msg: "valid nginx.org/location-snippets annotation, single-value", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/location-snippets annotation, single-value", }, { annotations: map[string]string{ "nginx.org/location-snippets": "snippet-1\nsnippet-2\nsnippet-3", }, - expectedErrors: nil, - msg: "valid nginx.org/location-snippets annotation, multi-value", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/location-snippets annotation, multi-value", }, { annotations: map[string]string{ "nginx.org/proxy-connect-timeout": "10s", }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-connect-timeout annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/proxy-connect-timeout annotation", }, { annotations: map[string]string{ "nginx.org/proxy-read-timeout": "10s", }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-read-timeout annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/proxy-read-timeout annotation", }, { annotations: map[string]string{ "nginx.org/proxy-send-timeout": "10s", }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-send-timeout annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/proxy-send-timeout annotation", }, { annotations: map[string]string{ "nginx.org/proxy-hide-headers": "header-1", }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-hide-headers annotation, single-value", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/proxy-hide-headers annotation, single-value", }, { annotations: map[string]string{ "nginx.org/proxy-hide-headers": "header-1,header-2,header-3", }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-hide-headers annotation, multi-value", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/proxy-hide-headers annotation, multi-value", }, { annotations: map[string]string{ "nginx.org/proxy-pass-headers": "header-1", }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-pass-headers annotation, single-value", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/proxy-pass-headers annotation, single-value", }, { annotations: map[string]string{ "nginx.org/proxy-pass-headers": "header-1,header-2,header-3", }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-pass-headers annotation, multi-value", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/proxy-pass-headers annotation, multi-value", }, { annotations: map[string]string{ "nginx.org/client-max-body-size": "16M", }, - expectedErrors: nil, - msg: "valid nginx.org/client-max-body-size annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/client-max-body-size annotation", }, { annotations: map[string]string{ "nginx.org/redirect-to-https": "true", }, - expectedErrors: nil, - msg: "valid nginx.org/redirect-to-https annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/redirect-to-https annotation", }, { annotations: map[string]string{ "nginx.org/redirect-to-https": "not_a_boolean", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/redirect-to-https: Invalid value: "not_a_boolean": must be a boolean`, }, @@ -364,13 +605,19 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "ingress.kubernetes.io/ssl-redirect": "true", }, - expectedErrors: nil, - msg: "valid ingress.kubernetes.io/ssl-redirect annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid ingress.kubernetes.io/ssl-redirect annotation", }, { annotations: map[string]string{ "ingress.kubernetes.io/ssl-redirect": "not_a_boolean", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.ingress.kubernetes.io/ssl-redirect: Invalid value: "not_a_boolean": must be a boolean`, }, @@ -381,13 +628,19 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-buffering": "true", }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-buffering annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/proxy-buffering annotation", }, { annotations: map[string]string{ "nginx.org/proxy-buffering": "not_a_boolean", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/proxy-buffering: Invalid value: "not_a_boolean": must be a boolean`, }, @@ -398,13 +651,19 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/hsts": "true", }, - expectedErrors: nil, - msg: "valid nginx.org/hsts annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/hsts annotation", }, { annotations: map[string]string{ "nginx.org/hsts": "not_a_boolean", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/hsts: Invalid value: "not_a_boolean": must be a boolean`, }, @@ -416,14 +675,20 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "true", "nginx.org/hsts-max-age": "120", }, - expectedErrors: nil, - msg: "valid nginx.org/hsts-max-age annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/hsts-max-age annotation", }, { annotations: map[string]string{ "nginx.org/hsts": "true", "nginx.org/hsts-max-age": "not_a_number", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/hsts-max-age: Invalid value: "not_a_number": must be an integer`, }, @@ -433,6 +698,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/hsts-max-age": "true", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "annotations.nginx.org/hsts-max-age: Forbidden: related annotation nginx.org/hsts: must be set", }, @@ -443,6 +711,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "false", "nginx.org/hsts-max-age": "120", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "annotations.nginx.org/hsts-max-age: Forbidden: related annotation nginx.org/hsts: must be true", }, @@ -454,14 +725,20 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "true", "nginx.org/hsts-include-subdomains": "true", }, - expectedErrors: nil, - msg: "valid nginx.org/hsts-include-subdomains annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/hsts-include-subdomains annotation", }, { annotations: map[string]string{ "nginx.org/hsts": "true", "nginx.org/hsts-include-subdomains": "not_a_boolean", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/hsts-include-subdomains: Invalid value: "not_a_boolean": must be a boolean`, }, @@ -471,6 +748,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/hsts-include-subdomains": "true", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "annotations.nginx.org/hsts-include-subdomains: Forbidden: related annotation nginx.org/hsts: must be set", }, @@ -481,6 +761,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "false", "nginx.org/hsts-include-subdomains": "true", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "annotations.nginx.org/hsts-include-subdomains: Forbidden: related annotation nginx.org/hsts: must be true", }, @@ -492,14 +775,20 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "true", "nginx.org/hsts-behind-proxy": "true", }, - expectedErrors: nil, - msg: "valid nginx.org/hsts-behind-proxy annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/hsts-behind-proxy annotation", }, { annotations: map[string]string{ "nginx.org/hsts": "true", "nginx.org/hsts-behind-proxy": "not_a_boolean", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/hsts-behind-proxy: Invalid value: "not_a_boolean": must be a boolean`, }, @@ -509,6 +798,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/hsts-behind-proxy": "true", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "annotations.nginx.org/hsts-behind-proxy: Forbidden: related annotation nginx.org/hsts: must be set", }, @@ -519,6 +811,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "false", "nginx.org/hsts-behind-proxy": "true", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "annotations.nginx.org/hsts-behind-proxy: Forbidden: related annotation nginx.org/hsts: must be true", }, @@ -529,85 +824,155 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-buffers": "8 8k", }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-buffers annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/proxy-buffers annotation", }, { annotations: map[string]string{ "nginx.org/proxy-buffer-size": "16k", }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-buffer-size annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/proxy-buffer-size annotation", }, { annotations: map[string]string{ "nginx.org/proxy-max-temp-file-size": "128M", }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-max-temp-file-size annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/proxy-max-temp-file-size annotation", }, { annotations: map[string]string{ "nginx.org/upstream-zone-size": "512k", }, - expectedErrors: nil, - msg: "valid nginx.org/upstream-zone-size annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/upstream-zone-size annotation", }, { annotations: map[string]string{ "nginx.com/jwt-realm": "true", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "annotations.nginx.com/jwt-realm: Forbidden: annotation requires NGINX Plus", }, msg: "invalid nginx.com/jwt-realm annotation, nginx plus only", }, + { + annotations: map[string]string{ + "nginx.com/jwt-realm": "my-jwt-realm", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.com/jwt-realm annotation", + }, { annotations: map[string]string{ "nginx.com/jwt-key": "true", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "annotations.nginx.com/jwt-key: Forbidden: annotation requires NGINX Plus", }, msg: "invalid nginx.com/jwt-key annotation, nginx plus only", }, + { + annotations: map[string]string{ + "nginx.com/jwt-key": "my-jwk", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.com/jwt-key annotation", + }, { annotations: map[string]string{ "nginx.com/jwt-token": "true", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "annotations.nginx.com/jwt-token: Forbidden: annotation requires NGINX Plus", }, msg: "invalid nginx.com/jwt-token annotation, nginx plus only", }, + { + annotations: map[string]string{ + "nginx.com/jwt-token": "$cookie_auth_token", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.com/jwt-token annotation", + }, { annotations: map[string]string{ "nginx.com/jwt-login-url": "true", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ "annotations.nginx.com/jwt-login-url: Forbidden: annotation requires NGINX Plus", }, msg: "invalid nginx.com/jwt-login-url annotation, nginx plus only", }, + { + annotations: map[string]string{ + "nginx.com/jwt-login-url": "https://login.example.com", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.com/jwt-login-url annotation", + }, { annotations: map[string]string{ "nginx.org/listen-ports": "80,8080,9090", }, - expectedErrors: nil, - msg: "valid nginx.org/listen-ports annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/listen-ports annotation", }, { annotations: map[string]string{ "nginx.org/listen-ports": "not_a_port_list", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/listen-ports: Invalid value: "not_a_port_list": must be a comma-separated list of port numbers`, }, @@ -618,13 +983,19 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/listen-ports-ssl": "443,8443", }, - expectedErrors: nil, - msg: "valid nginx.org/listen-ports-ssl annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/listen-ports-ssl annotation", }, { annotations: map[string]string{ "nginx.org/listen-ports-ssl": "not_a_port_list", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/listen-ports-ssl: Invalid value: "not_a_port_list": must be a comma-separated list of port numbers`, }, @@ -635,13 +1006,19 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/keepalive": "1000", }, - expectedErrors: nil, - msg: "valid nginx.org/keepalive annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/keepalive annotation", }, { annotations: map[string]string{ "nginx.org/keepalive": "not_a_number", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/keepalive: Invalid value: "not_a_number": must be an integer`, }, @@ -652,13 +1029,19 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/max-fails": "5", }, - expectedErrors: nil, - msg: "valid nginx.org/max-fails annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/max-fails annotation", }, { annotations: map[string]string{ "nginx.org/max-fails": "not_a_number", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/max-fails: Invalid value: "not_a_number": must be an integer`, }, @@ -669,13 +1052,19 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/max-conns": "10", }, - expectedErrors: nil, - msg: "valid nginx.org/max-conns annotation", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/max-conns annotation", }, { annotations: map[string]string{ "nginx.org/max-conns": "not_a_number", }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ `annotations.nginx.org/max-conns: Invalid value: "not_a_number": must be an integer`, }, @@ -686,653 +1075,128 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/fail-timeout": "10s", }, - expectedErrors: nil, - msg: "valid nginx.org/fail-timeout annotation", - }, - } - - 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", + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/fail-timeout annotation", }, { annotations: map[string]string{ - "nginx.org/lb-method": "invalid_method", - "nginx.com/health-checks": "not_a_boolean", + "appprotect.f5.com/app-protect-enable": "true", }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ - `annotations.nginx.com/health-checks: Invalid value: "not_a_boolean": must be a boolean`, - `annotations.nginx.org/lb-method: Invalid value: "invalid_method": Invalid load balancing method: "invalid_method"`, - }, - 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", + "annotations.appprotect.f5.com/app-protect-enable: Forbidden: annotation requires AppProtect", }, - expectedErrors: nil, - msg: "valid input with minion annotation", + msg: "invalid appprotect.f5.com/app-protect-enable annotation, requires app protect", }, { annotations: map[string]string{ - "nginx.org/mergeable-ingress-type": "", - }, - expectedErrors: []string{ - "annotations.nginx.org/mergeable-ingress-type: Required value", + "appprotect.f5.com/app-protect-enable": "true", }, - msg: "invalid mergeable type annotation 1", + isPlus: true, + appProtectEnabled: true, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid appprotect.f5.com/app-protect-enable annotation", }, { annotations: map[string]string{ - "nginx.org/mergeable-ingress-type": "abc", + "appprotect.f5.com/app-protect-enable": "not_a_boolean", }, + isPlus: true, + appProtectEnabled: true, + internalRoutesEnabled: false, expectedErrors: []string{ - `annotations.nginx.org/mergeable-ingress-type: Invalid value: "abc": must be one of: 'master' or 'minion'`, + `annotations.appprotect.f5.com/app-protect-enable: Invalid value: "not_a_boolean": must be a boolean`, }, - msg: "invalid mergeable type annotation 2", + msg: "invalid appprotect.f5.com/app-protect-enable annotation", }, { 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", + "appprotect.f5.com/app-protect-security-log-enable": "true", }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ - `annotations.nginx.org/lb-method: Invalid value: "invalid_method": Invalid load balancing method: "invalid_method"`, + "annotations.appprotect.f5.com/app-protect-security-log-enable: Forbidden: annotation requires AppProtect", }, - msg: "invalid nginx.org/lb-method annotation, nginx", + msg: "invalid appprotect.f5.com/app-protect-security-log-enable annotation, requires app protect", }, - { annotations: map[string]string{ - "nginx.com/health-checks": "true", + "appprotect.f5.com/app-protect-security-log-enable": "true", }, - expectedErrors: nil, - msg: "valid nginx.com/health-checks annotation", + isPlus: true, + appProtectEnabled: true, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid appprotect.f5.com/app-protect-security-log-enable annotation", }, { annotations: map[string]string{ - "nginx.com/health-checks": "not_a_boolean", + "appprotect.f5.com/app-protect-security-log-enable": "not_a_boolean", }, + isPlus: true, + appProtectEnabled: true, + internalRoutesEnabled: false, expectedErrors: []string{ - `annotations.nginx.com/health-checks: Invalid value: "not_a_boolean": must be a boolean`, + `annotations.appprotect.f5.com/app-protect-security-log-enable: Invalid value: "not_a_boolean": must be a boolean`, }, - msg: "invalid nginx.com/health-checks annotation", + msg: "invalid appprotect.f5.com/app-protect-security-log-enable annotation", }, { annotations: map[string]string{ - "nginx.com/health-checks": "true", - "nginx.com/health-checks-mandatory": "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", + "nsm.nginx.com/internal-route": "true", }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, expectedErrors: []string{ - `annotations.nginx.com/health-checks-mandatory: Invalid value: "not_a_boolean": must be a boolean`, + "annotations.nsm.nginx.com/internal-route: Forbidden: annotation requires Internal Routes enabled", }, - msg: "invalid nginx.com/health-checks-mandatory, must be a boolean", + msg: "invalid nsm.nginx.com/internal-route annotation, requires internal routes", }, { annotations: map[string]string{ - "nginx.com/health-checks-mandatory": "true", - }, - expectedErrors: []string{ - "annotations.nginx.com/health-checks-mandatory: Forbidden: related annotation nginx.com/health-checks: must be set", + "nsm.nginx.com/internal-route": "true", }, - msg: "invalid nginx.com/health-checks-mandatory, related annotation nginx.com/health-checks not set", + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: true, + expectedErrors: nil, + msg: "valid nsm.nginx.com/internal-route annotation", }, { annotations: map[string]string{ - "nginx.com/health-checks": "false", - "nginx.com/health-checks-mandatory": "true", + "nsm.nginx.com/internal-route": "not_a_boolean", }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: 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", - }, - 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", + `annotations.nsm.nginx.com/internal-route: Invalid value: "not_a_boolean": must be a boolean`, }, - 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 number", - }, - { - annotations: map[string]string{ - "nginx.com/health-checks-mandatory-queue": "5", - }, - 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", - }, - 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", - }, - - { - annotations: map[string]string{ - "nginx.com/slow-start": "60s", - }, - expectedErrors: nil, - msg: "valid nginx.com/slow-start annotation", - }, - { - annotations: map[string]string{ - "nginx.com/slow-start": "not_a_time", - }, - expectedErrors: []string{ - `annotations.nginx.com/slow-start: Invalid value: "not_a_time": must be a time`, - }, - msg: "invalid nginx.com/slow-start annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/server-tokens": "custom_setting", - }, - expectedErrors: nil, - msg: "valid nginx.org/server-tokens annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/server-snippets": "snippet-1", - }, - expectedErrors: nil, - msg: "valid nginx.org/server-snippets annotation, single-value", - }, - { - annotations: map[string]string{ - "nginx.org/server-snippets": "snippet-1\nsnippet-2\nsnippet-3", - }, - expectedErrors: nil, - msg: "valid nginx.org/server-snippets annotation, multi-value", - }, - - { - annotations: map[string]string{ - "nginx.org/location-snippets": "snippet-1", - }, - expectedErrors: nil, - msg: "valid nginx.org/location-snippets annotation, single-value", - }, - { - annotations: map[string]string{ - "nginx.org/location-snippets": "snippet-1\nsnippet-2\nsnippet-3", - }, - expectedErrors: nil, - msg: "valid nginx.org/location-snippets annotation, multi-value", - }, - - { - annotations: map[string]string{ - "nginx.org/proxy-connect-timeout": "10s", - }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-connect-timeout annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/proxy-read-timeout": "10s", - }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-read-timeout annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/proxy-send-timeout": "10s", - }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-send-timeout annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/proxy-hide-headers": "header-1", - }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-hide-headers annotation, single-value", - }, - { - annotations: map[string]string{ - "nginx.org/proxy-hide-headers": "header-1,header-2,header-3", - }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-hide-headers annotation, multi-value", - }, - - { - annotations: map[string]string{ - "nginx.org/proxy-pass-headers": "header-1", - }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-pass-headers annotation, single-value", - }, - { - annotations: map[string]string{ - "nginx.org/proxy-pass-headers": "header-1,header-2,header-3", - }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-pass-headers annotation, multi-value", - }, - - { - annotations: map[string]string{ - "nginx.org/client-max-body-size": "16M", - }, - expectedErrors: nil, - msg: "valid nginx.org/client-max-body-size annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/redirect-to-https": "true", - }, - expectedErrors: nil, - msg: "valid nginx.org/redirect-to-https annotation", - }, - { - annotations: map[string]string{ - "nginx.org/redirect-to-https": "not_a_boolean", - }, - expectedErrors: []string{ - `annotations.nginx.org/redirect-to-https: Invalid value: "not_a_boolean": must be a boolean`, - }, - msg: "invalid nginx.org/redirect-to-https annotation", - }, - - { - annotations: map[string]string{ - "ingress.kubernetes.io/ssl-redirect": "true", - }, - expectedErrors: nil, - msg: "valid ingress.kubernetes.io/ssl-redirect annotation", - }, - { - annotations: map[string]string{ - "ingress.kubernetes.io/ssl-redirect": "not_a_boolean", - }, - expectedErrors: []string{ - `annotations.ingress.kubernetes.io/ssl-redirect: Invalid value: "not_a_boolean": must be a boolean`, - }, - msg: "invalid ingress.kubernetes.io/ssl-redirect annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/proxy-buffering": "true", - }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-buffering annotation", - }, - { - annotations: map[string]string{ - "nginx.org/proxy-buffering": "not_a_boolean", - }, - expectedErrors: []string{ - `annotations.nginx.org/proxy-buffering: Invalid value: "not_a_boolean": must be a boolean`, - }, - msg: "invalid nginx.org/proxy-buffering annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/hsts": "true", - }, - expectedErrors: nil, - msg: "valid nginx.org/hsts annotation", - }, - { - annotations: map[string]string{ - "nginx.org/hsts": "not_a_boolean", - }, - expectedErrors: []string{ - `annotations.nginx.org/hsts: Invalid value: "not_a_boolean": must be a boolean`, - }, - msg: "invalid nginx.org/hsts annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/hsts": "true", - "nginx.org/hsts-max-age": "120", - }, - expectedErrors: nil, - msg: "valid nginx.org/hsts-max-age annotation", - }, - { - annotations: map[string]string{ - "nginx.org/hsts": "true", - "nginx.org/hsts-max-age": "not_a_number", - }, - expectedErrors: []string{ - `annotations.nginx.org/hsts-max-age: Invalid value: "not_a_number": must be an integer`, - }, - msg: "invalid nginx.org/hsts-max-age, must be a number", - }, - { - annotations: map[string]string{ - "nginx.org/hsts-max-age": "true", - }, - expectedErrors: []string{ - "annotations.nginx.org/hsts-max-age: Forbidden: related annotation nginx.org/hsts: must be set", - }, - msg: "invalid nginx.org/hsts-max-age, related annotation nginx.org/hsts not set", - }, - { - annotations: map[string]string{ - "nginx.org/hsts": "false", - "nginx.org/hsts-max-age": "120", - }, - expectedErrors: []string{ - "annotations.nginx.org/hsts-max-age: Forbidden: related annotation nginx.org/hsts: must be true", - }, - msg: "invalid nginx.org/hsts-max-age nginx.org/hsts is not true", - }, - - { - annotations: map[string]string{ - "nginx.org/hsts": "true", - "nginx.org/hsts-include-subdomains": "true", - }, - expectedErrors: nil, - msg: "valid nginx.org/hsts-include-subdomains annotation", - }, - { - annotations: map[string]string{ - "nginx.org/hsts": "true", - "nginx.org/hsts-include-subdomains": "not_a_boolean", - }, - expectedErrors: []string{ - `annotations.nginx.org/hsts-include-subdomains: Invalid value: "not_a_boolean": must be a boolean`, - }, - msg: "invalid nginx.org/hsts-include-subdomains, must be a boolean", - }, - { - annotations: map[string]string{ - "nginx.org/hsts-include-subdomains": "true", - }, - expectedErrors: []string{ - "annotations.nginx.org/hsts-include-subdomains: Forbidden: related annotation nginx.org/hsts: must be set", - }, - msg: "invalid nginx.org/hsts-include-subdomains, related annotation nginx.org/hsts not set", - }, - { - annotations: map[string]string{ - "nginx.org/hsts": "false", - "nginx.org/hsts-include-subdomains": "true", - }, - expectedErrors: []string{ - "annotations.nginx.org/hsts-include-subdomains: Forbidden: related annotation nginx.org/hsts: must be true", - }, - msg: "invalid nginx.org/hsts-include-subdomains nginx.org/hsts is not true", - }, - - { - annotations: map[string]string{ - "nginx.org/hsts": "true", - "nginx.org/hsts-behind-proxy": "true", - }, - expectedErrors: nil, - msg: "valid nginx.org/hsts-behind-proxy annotation", - }, - { - annotations: map[string]string{ - "nginx.org/hsts": "true", - "nginx.org/hsts-behind-proxy": "not_a_boolean", - }, - expectedErrors: []string{ - `annotations.nginx.org/hsts-behind-proxy: Invalid value: "not_a_boolean": must be a boolean`, - }, - msg: "invalid nginx.org/hsts-behind-proxy, must be a boolean", - }, - { - annotations: map[string]string{ - "nginx.org/hsts-behind-proxy": "true", - }, - expectedErrors: []string{ - "annotations.nginx.org/hsts-behind-proxy: Forbidden: related annotation nginx.org/hsts: must be set", - }, - msg: "invalid nginx.org/hsts-behind-proxy, related annotation nginx.org/hsts not set", - }, - { - annotations: map[string]string{ - "nginx.org/hsts": "false", - "nginx.org/hsts-behind-proxy": "true", - }, - expectedErrors: []string{ - "annotations.nginx.org/hsts-behind-proxy: Forbidden: related annotation nginx.org/hsts: must be true", - }, - msg: "invalid nginx.org/hsts-behind-proxy nginx.org/hsts is not true", - }, - - { - annotations: map[string]string{ - "nginx.org/proxy-buffers": "8 8k", - }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-buffers annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/proxy-buffer-size": "16k", - }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-buffer-size annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/proxy-max-temp-file-size": "128M", - }, - expectedErrors: nil, - msg: "valid nginx.org/proxy-max-temp-file-size annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/upstream-zone-size": "512k", - }, - expectedErrors: nil, - msg: "valid nginx.org/upstream-zone-size annotation", - }, - - { - annotations: map[string]string{ - "nginx.com/jwt-realm": "my-jwt-realm", - }, - expectedErrors: nil, - msg: "valid nginx.com/jwt-realm annotation", - }, - - { - annotations: map[string]string{ - "nginx.com/jwt-key": "my-jwk", - }, - expectedErrors: nil, - msg: "valid nginx.com/jwt-key annotation", - }, - - { - annotations: map[string]string{ - "nginx.com/jwt-token": "$cookie_auth_token", - }, - expectedErrors: nil, - msg: "valid nginx.com/jwt-token annotation", - }, - - { - annotations: map[string]string{ - "nginx.com/jwt-login-url": "https://login.example.com", - }, - expectedErrors: nil, - msg: "valid nginx.com/jwt-login-url annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/listen-ports": "80,8080,9090", - }, - expectedErrors: nil, - msg: "valid nginx.org/listen-ports annotation", - }, - { - annotations: map[string]string{ - "nginx.org/listen-ports": "not_a_port_list", - }, - expectedErrors: []string{ - `annotations.nginx.org/listen-ports: Invalid value: "not_a_port_list": must be a comma-separated list of port numbers`, - }, - msg: "invalid nginx.org/listen-ports annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/listen-ports-ssl": "443,8443", - }, - expectedErrors: nil, - msg: "valid nginx.org/listen-ports-ssl annotation", - }, - { - annotations: map[string]string{ - "nginx.org/listen-ports-ssl": "not_a_port_list", - }, - expectedErrors: []string{ - `annotations.nginx.org/listen-ports-ssl: Invalid value: "not_a_port_list": must be a comma-separated list of port numbers`, - }, - msg: "invalid nginx.org/listen-ports-ssl annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/keepalive": "1000", - }, - expectedErrors: nil, - msg: "valid nginx.org/keepalive annotation", - }, - { - annotations: map[string]string{ - "nginx.org/keepalive": "not_a_number", - }, - expectedErrors: []string{ - `annotations.nginx.org/keepalive: Invalid value: "not_a_number": must be an integer`, - }, - msg: "invalid nginx.org/keepalive annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/max-fails": "5", - }, - expectedErrors: nil, - msg: "valid nginx.org/max-fails annotation", - }, - { - annotations: map[string]string{ - "nginx.org/max-fails": "not_a_number", - }, - expectedErrors: []string{ - `annotations.nginx.org/max-fails: Invalid value: "not_a_number": must be an integer`, - }, - msg: "invalid nginx.org/max-fails annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/max-conns": "10", - }, - expectedErrors: nil, - msg: "valid nginx.org/max-conns annotation", - }, - { - annotations: map[string]string{ - "nginx.org/max-conns": "not_a_number", - }, - expectedErrors: []string{ - `annotations.nginx.org/max-conns: Invalid value: "not_a_number": must be an integer`, - }, - msg: "invalid nginx.org/max-conns annotation", - }, - - { - annotations: map[string]string{ - "nginx.org/fail-timeout": "10s", - }, - expectedErrors: nil, - msg: "valid nginx.org/fail-timeout annotation", + msg: "invalid nsm.nginx.com/internal-route annotation", }, } for _, test := range tests { t.Run(test.msg, func(t *testing.T) { - allErrs := validateIngressAnnotations(test.annotations, isPlus, field.NewPath("annotations")) + allErrs := validateIngressAnnotations( + test.annotations, + test.isPlus, + test.appProtectEnabled, + test.internalRoutesEnabled, + field.NewPath("annotations"), + ) assertion := assertErrors("validateIngressAnnotations()", test.msg, allErrs, test.expectedErrors) if assertion != "" { t.Error(assertion) From f2bb43b7ee88253b9441ed24708c846572e0c177 Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Thu, 10 Dec 2020 15:16:42 +0000 Subject: [PATCH 08/13] Simplify annotation validation config --- internal/k8s/validation.go | 175 ++++++-------------------------- internal/k8s/validation_test.go | 20 ++++ 2 files changed, 49 insertions(+), 146 deletions(-) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 968bae4700..c03b6ab697 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -68,33 +68,42 @@ type annotationValidationConfig map[string][]annotationValidationFunc type validatorFunc func(val string) error var ( - // 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{ + // 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 = annotationValidationConfig{ mergeableIngressTypeAnnotation: { validateRequiredAnnotation, validateMergeableIngressTypeAnnotation, }, lbMethodAnnotation: { validateRequiredAnnotation, - validateNginxLBMethodAnnotation, + validateLBMethodAnnotation, }, healthChecksAnnotation: { validatePlusOnlyAnnotation, + validateRequiredAnnotation, + validateBoolAnnotation, }, healthChecksMandatoryAnnotation: { validatePlusOnlyAnnotation, + validateRelatedAnnotation(healthChecksAnnotation, validateIsTrue), + validateRequiredAnnotation, + validateBoolAnnotation, }, healthChecksMandatoryQueueAnnotation: { validatePlusOnlyAnnotation, + validateRelatedAnnotation(healthChecksMandatoryAnnotation, validateIsTrue), + validateRequiredAnnotation, + validateUint64Annotation, }, slowStartAnnotation: { validatePlusOnlyAnnotation, + validateRequiredAnnotation, + validateTimeAnnotation, }, serverTokensAnnotation: { validateRequiredAnnotation, - validateBoolAnnotation, + validateServerTokensAnnotation, }, serverSnippetsAnnotation: {}, locationSnippetsAnnotation: {}, @@ -188,126 +197,7 @@ var ( validateBoolAnnotation, }, } - 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: { - validateRelatedAnnotation(healthChecksAnnotation, validateIsTrue), - validateRequiredAnnotation, - validateBoolAnnotation, - }, - healthChecksMandatoryQueueAnnotation: { - validateRelatedAnnotation(healthChecksMandatoryAnnotation, validateIsTrue), - validateRequiredAnnotation, - validateUint64Annotation, - }, - slowStartAnnotation: { - validateRequiredAnnotation, - validateTimeAnnotation, - }, - serverTokensAnnotation: { - validateRequiredAnnotation, - }, - serverSnippetsAnnotation: {}, - locationSnippetsAnnotation: {}, - proxyConnectTimeoutAnnotation: {}, - proxyReadTimeoutAnnotation: {}, - proxySendTimeoutAnnotation: {}, - proxyHideHeadersAnnotation: {}, - proxyPassHeadersAnnotation: {}, - clientMaxBodySizeAnnotation: {}, - redirectToHTTPSAnnotation: { - validateRequiredAnnotation, - validateBoolAnnotation, - }, - sslRedirectAnnotation: { - validateRequiredAnnotation, - validateBoolAnnotation, - }, - proxyBufferingAnnotation: { - validateRequiredAnnotation, - validateBoolAnnotation, - }, - hstsAnnotation: { - validateRequiredAnnotation, - validateBoolAnnotation, - }, - hstsMaxAgeAnnotation: { - validateRelatedAnnotation(hstsAnnotation, validateIsTrue), - validateRequiredAnnotation, - validateInt64Annotation, - }, - hstsIncludeSubdomainsAnnotation: { - validateRelatedAnnotation(hstsAnnotation, validateIsTrue), - validateRequiredAnnotation, - validateBoolAnnotation, - }, - hstsBehindProxyAnnotation: { - validateRelatedAnnotation(hstsAnnotation, validateIsTrue), - validateRequiredAnnotation, - validateBoolAnnotation, - }, - proxyBuffersAnnotation: {}, - proxyBufferSizeAnnotation: {}, - proxyMaxTempFileSizeAnnotation: {}, - upstreamZoneSizeAnnotation: {}, - jwtRealmAnnotation: {}, - jwtKeyAnnotation: {}, - jwtTokenAnnotation: {}, - jwtLoginURLAnnotation: {}, - listenPortsAnnotation: { - validateRequiredAnnotation, - validatePortListAnnotation, - }, - listenPortsSSLAnnotation: { - validateRequiredAnnotation, - validatePortListAnnotation, - }, - keepaliveAnnotation: { - validateRequiredAnnotation, - validateIntAnnotation, - }, - maxFailsAnnotation: { - validateRequiredAnnotation, - validateIntAnnotation, - }, - maxConnsAnnotation: { - validateRequiredAnnotation, - validateIntAnnotation, - }, - failTimeoutAnnotation: {}, - appProtectEnableAnnotation: { - validateAppProtectOnlyAnnotation, - validateRequiredAnnotation, - validateBoolAnnotation, - }, - appProtectSecurityLogEnableAnnotation: { - validateAppProtectOnlyAnnotation, - validateRequiredAnnotation, - validateBoolAnnotation, - }, - internalRouteAnnotation: { - validateInternalRoutesOnlyAnnotation, - validateRequiredAnnotation, - validateBoolAnnotation, - }, - } - nginxPlusAnnotationNames = sortedAnnotationNames(nginxPlusAnnotationValidations) + annotationNames = sortedAnnotationNames(annotationValidations) ) func sortedAnnotationNames(annotationValidations annotationValidationConfig) []string { @@ -356,13 +246,6 @@ func validateIngressAnnotations( ) 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 { context := &annotationValidationContext{ @@ -383,14 +266,6 @@ func validateIngressAnnotations( 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) @@ -426,18 +301,26 @@ func validateMergeableIngressTypeAnnotation(context *annotationValidationContext return allErrs } -func validateNginxLBMethodAnnotation(context *annotationValidationContext) field.ErrorList { +func validateLBMethodAnnotation(context *annotationValidationContext) field.ErrorList { allErrs := field.ErrorList{} - if _, err := configs.ParseLBMethod(context.value); err != nil { + + parseFunc := configs.ParseLBMethod + if context.isPlus { + parseFunc = configs.ParseLBMethodForPlus + } + + if _, err := parseFunc(context.value); err != nil { return append(allErrs, field.Invalid(context.fieldPath, context.value, err.Error())) } return allErrs } -func validateNginxPlusLBMethodAnnotation(context *annotationValidationContext) field.ErrorList { +func validateServerTokensAnnotation(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())) + if !context.isPlus { + if _, err := configs.ParseBool(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a boolean")) + } } return allErrs } diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 9908351764..35a45d00e7 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -437,6 +437,26 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { msg: "invalid nginx.com/slow-start annotation", }, + { + annotations: map[string]string{ + "nginx.org/server-tokens": "true", + }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/server-tokens annotation, nginx", + }, + { + annotations: map[string]string{ + "nginx.org/server-tokens": "custom_setting", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/server-tokens annotation, nginx plus", + }, { annotations: map[string]string{ "nginx.org/server-tokens": "custom_setting", From 98749ddb85bb0c4333125d8197f7dfda9df259ca Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Thu, 10 Dec 2020 17:18:18 +0000 Subject: [PATCH 09/13] Add earlier sevices annotation validation --- internal/configs/annotations.go | 124 +++++++++--------------- internal/configs/parsing_helpers.go | 35 +++++++ internal/k8s/validation.go | 50 ++++++++++ internal/k8s/validation_test.go | 141 ++++++++++++++++++++++++++++ 4 files changed, 272 insertions(+), 78 deletions(-) diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index 660739a34b..152a216529 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -1,8 +1,6 @@ package configs import ( - "strings" - "github.com/golang/glog" ) @@ -285,13 +283,24 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool } } - ports, sslPorts := getServicesPorts(ingEx) - if len(ports) > 0 { - cfgParams.Ports = ports + if values, exists := ingEx.Ingress.Annotations["nginx.org/listen-ports"]; exists { + ports, err := ParsePortList(values) + if err != nil { + glog.Errorf("In %v nginx.org/listen-ports contains invalid declaration: %v, ignoring", ingEx.Ingress.Name, err) + } + if len(ports) > 0 { + cfgParams.Ports = ports + } } - if len(sslPorts) > 0 { - cfgParams.SSLPorts = sslPorts + if values, exists := ingEx.Ingress.Annotations["nginx.org/listen-ports"]; exists { + sslPorts, err := ParsePortList(values) + if err != nil { + glog.Errorf("In %v nginx.org/listen-ports-ssl contains invalid declaration: %v, ignoring", ingEx.Ingress.Name, err) + } + if len(sslPorts) > 0 { + cfgParams.SSLPorts = sslPorts + } } if keepalive, exists, err := GetMapKeyAsInt(ingEx.Ingress.Annotations, "nginx.org/keepalive", ingEx.Ingress); exists { @@ -361,99 +370,58 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool } func getWebsocketServices(ingEx *IngressEx) map[string]bool { - wsServices := make(map[string]bool) - - if services, exists := ingEx.Ingress.Annotations["nginx.org/websocket-services"]; exists { - for _, svc := range strings.Split(services, ",") { - wsServices[svc] = true + if value, exists := ingEx.Ingress.Annotations["nginx.org/websocket-services"]; exists { + services, err := ParseServiceList(value) + if err != nil { + glog.Error(err) } + return services } - - return wsServices + return make(map[string]bool) } func getRewrites(ingEx *IngressEx) map[string]string { - rewrites := make(map[string]string) - - if services, exists := ingEx.Ingress.Annotations["nginx.org/rewrites"]; exists { - for _, svc := range strings.Split(services, ";") { - if serviceName, rewrite, err := parseRewrites(svc); err != nil { - glog.Errorf("In %v nginx.org/rewrites contains invalid declaration: %v, ignoring", ingEx.Ingress.Name, err) - } else { - rewrites[serviceName] = rewrite - } + if value, exists := ingEx.Ingress.Annotations["nginx.org/rewrites"]; exists { + rewrites, err := ParseRewriteList(value) + if err != nil { + glog.Error(err) } + return rewrites } - - return rewrites + return make(map[string]string) } func getSSLServices(ingEx *IngressEx) map[string]bool { - sslServices := make(map[string]bool) - - if services, exists := ingEx.Ingress.Annotations["nginx.org/ssl-services"]; exists { - for _, svc := range strings.Split(services, ",") { - sslServices[svc] = true + if value, exists := ingEx.Ingress.Annotations["nginx.org/ssl-services"]; exists { + services, err := ParseServiceList(value) + if err != nil { + glog.Error(err) } + return services } - - return sslServices + return make(map[string]bool) } func getGrpcServices(ingEx *IngressEx) map[string]bool { - grpcServices := make(map[string]bool) - - if services, exists := ingEx.Ingress.Annotations["nginx.org/grpc-services"]; exists { - for _, svc := range strings.Split(services, ",") { - grpcServices[svc] = true + if value, exists := ingEx.Ingress.Annotations["nginx.org/grpc-services"]; exists { + services, err := ParseServiceList(value) + if err != nil { + glog.Error(err) } + return services } - - return grpcServices + return make(map[string]bool) } func getSessionPersistenceServices(ingEx *IngressEx) map[string]string { - spServices := make(map[string]string) - - if services, exists := ingEx.Ingress.Annotations["nginx.com/sticky-cookie-services"]; exists { - for _, svc := range strings.Split(services, ";") { - if serviceName, sticky, err := parseStickyService(svc); err != nil { - glog.Errorf("In %v nginx.com/sticky-cookie-services contains invalid declaration: %v, ignoring", ingEx.Ingress.Name, err) - } else { - spServices[serviceName] = sticky - } - } - } - - return spServices -} - -func getServicesPorts(ingEx *IngressEx) ([]int, []int) { - ports := map[string][]int{} - - annotations := []string{ - "nginx.org/listen-ports", - "nginx.org/listen-ports-ssl", - } - - for _, annotation := range annotations { - if values, exists := ingEx.Ingress.Annotations[annotation]; exists { - for _, value := range strings.Split(values, ",") { - if port, err := parsePort(value); err != nil { - glog.Errorf( - "In %v %s contains invalid declaration: %v, ignoring", - ingEx.Ingress.Name, - annotation, - err, - ) - } else { - ports[annotation] = append(ports[annotation], port) - } - } + if value, exists := ingEx.Ingress.Annotations["nginx.com/sticky-cookie-services"]; exists { + services, err := ParseStickyServiceList(value) + if err != nil { + glog.Error(err) } + return services } - - return ports[annotations[0]], ports[annotations[1]] + return make(map[string]string) } func filterMasterAnnotations(annotations map[string]string) []string { diff --git a/internal/configs/parsing_helpers.go b/internal/configs/parsing_helpers.go index f67339386b..f7fae19872 100644 --- a/internal/configs/parsing_helpers.go +++ b/internal/configs/parsing_helpers.go @@ -251,6 +251,41 @@ func parsePort(value string) (int, error) { return int(port), nil } +// ParseServiceList ensures that the string is a comma-separated list of services +func ParseServiceList(s string) (map[string]bool, error) { + services := make(map[string]bool) + for _, part := range strings.Split(s, ",") { + services[part] = true + } + return services, nil +} + +// ParseRewriteList ensures that the string is a semicolon-separated list of services +func ParseRewriteList(s string) (map[string]string, error) { + rewrites := make(map[string]string) + for _, part := range strings.Split(s, ";") { + serviceName, rewrite, err := parseRewrites(part) + if err != nil { + return nil, err + } + rewrites[serviceName] = rewrite + } + return rewrites, nil +} + +// ParseStickyServiceList ensures that the string is a semicolon-separated list of sticky services +func ParseStickyServiceList(s string) (map[string]string, error) { + services := make(map[string]string) + for _, part := range strings.Split(s, ";") { + serviceName, service, err := parseStickyService(part) + if err != nil { + return nil, err + } + services[serviceName] = service + } + return services, nil +} + func parseStickyService(service string) (serviceName string, stickyCookie string, err error) { parts := strings.SplitN(service, " ", 2) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index c03b6ab697..983699d378 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -51,6 +51,11 @@ const ( appProtectEnableAnnotation = "appprotect.f5.com/app-protect-enable" appProtectSecurityLogEnableAnnotation = "appprotect.f5.com/app-protect-security-log-enable" internalRouteAnnotation = "nsm.nginx.com/internal-route" + websocketServicesAnnotation = "nginx.org/websocket-services" + sslServicesAnnotation = "nginx.org/ssl-services" + grpcServicesAnnotation = "nginx.org/grpc-services" + rewritesAnnotation = "nginx.org/rewrites" + stickyCookieServicesAnnotation = "nginx.com/sticky-cookie-services" ) type annotationValidationContext struct { @@ -196,6 +201,27 @@ var ( validateRequiredAnnotation, validateBoolAnnotation, }, + websocketServicesAnnotation: { + validateRequiredAnnotation, + validateServiceListAnnotation, + }, + sslServicesAnnotation: { + validateRequiredAnnotation, + validateServiceListAnnotation, + }, + grpcServicesAnnotation: { + validateRequiredAnnotation, + validateServiceListAnnotation, + }, + rewritesAnnotation: { + validateRequiredAnnotation, + validateRewriteListAnnotation, + }, + stickyCookieServicesAnnotation: { + validatePlusOnlyAnnotation, + validateRequiredAnnotation, + validateStickyServiceListAnnotation, + }, } annotationNames = sortedAnnotationNames(annotationValidations) ) @@ -405,6 +431,30 @@ func validatePortListAnnotation(context *annotationValidationContext) field.Erro return allErrs } +func validateServiceListAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if _, err := configs.ParseServiceList(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a comma-separated list of services")) + } + return allErrs +} + +func validateStickyServiceListAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if _, err := configs.ParseStickyServiceList(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a semicolon-separated list of sticky services")) + } + return allErrs +} + +func validateRewriteListAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + if _, err := configs.ParseRewriteList(context.value); err != nil { + return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a semicolon-separated list of rewrites")) + } + return allErrs +} + func validateIsTrue(v string) error { b, err := configs.ParseBool(v) if err != nil { diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 35a45d00e7..1a46274b81 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -1206,6 +1206,147 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }, msg: "invalid nsm.nginx.com/internal-route annotation", }, + + { + annotations: map[string]string{ + "nginx.org/websocket-services": "service-1", + }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/websocket-services annotation, single-value", + }, + { + annotations: map[string]string{ + "nginx.org/websocket-services": "service-1,service-2", + }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/websocket-services annotation, multi-value", + }, + + { + annotations: map[string]string{ + "nginx.org/ssl-services": "service-1", + }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/ssl-services annotation, single-value", + }, + { + annotations: map[string]string{ + "nginx.org/ssl-services": "service-1,service-2", + }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/ssl-services annotation, multi-value", + }, + + { + annotations: map[string]string{ + "nginx.org/grpc-services": "service-1", + }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/grpc-services annotation, single-value", + }, + { + annotations: map[string]string{ + "nginx.org/grpc-services": "service-1,service-2", + }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/grpc-services annotation, multi-value", + }, + + { + annotations: map[string]string{ + "nginx.org/rewrites": "serviceName=service-1 rewrite=rewrite-1", + }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/rewrites annotation, single-value", + }, + { + annotations: map[string]string{ + "nginx.org/rewrites": "serviceName=service-1 rewrite=rewrite-1;serviceName=service-2 rewrite=rewrite-2", + }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/rewrites annotation, multi-value", + }, + { + annotations: map[string]string{ + "nginx.org/rewrites": "not_a_rewrite", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: true, + expectedErrors: []string{ + `annotations.nginx.org/rewrites: Invalid value: "not_a_rewrite": must be a semicolon-separated list of rewrites`, + }, + msg: "invalid nginx.org/rewrites annotation", + }, + + { + annotations: map[string]string{ + "nginx.com/sticky-cookie-services": "true", + }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + "annotations.nginx.com/sticky-cookie-services: Forbidden: annotation requires NGINX Plus", + }, + msg: "invalid nginx.com/sticky-cookie-services annotation, nginx plus only", + }, + { + annotations: map[string]string{ + "nginx.com/sticky-cookie-services": "serviceName=service-1 srv_id expires=1h path=/service-1", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.com/sticky-cookie-services annotation, single-value", + }, + { + annotations: map[string]string{ + "nginx.com/sticky-cookie-services": "serviceName=service-1 srv_id expires=1h path=/service-1;serviceName=service-2 srv_id expires=2h path=/service-2", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.com/sticky-cookie-services annotation, multi-value", + }, + { + annotations: map[string]string{ + "nginx.com/sticky-cookie-services": "not_a_rewrite", + }, + isPlus: true, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.nginx.com/sticky-cookie-services: Invalid value: "not_a_rewrite": must be a semicolon-separated list of sticky services`, + }, + msg: "invalid nginx.com/sticky-cookie-services annotation", + }, } for _, test := range tests { From b3b865263e7935eee0e13e8862dcb91c6c7d81a5 Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Fri, 11 Dec 2020 10:10:34 +0000 Subject: [PATCH 10/13] Change map and slice initialization to follow go idioms --- internal/configs/annotations.go | 12 ++++++------ internal/configs/parsing_helpers.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index 152a216529..db06960d7f 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -293,7 +293,7 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool } } - if values, exists := ingEx.Ingress.Annotations["nginx.org/listen-ports"]; exists { + if values, exists := ingEx.Ingress.Annotations["nginx.org/listen-ports-ssl"]; exists { sslPorts, err := ParsePortList(values) if err != nil { glog.Errorf("In %v nginx.org/listen-ports-ssl contains invalid declaration: %v, ignoring", ingEx.Ingress.Name, err) @@ -377,7 +377,7 @@ func getWebsocketServices(ingEx *IngressEx) map[string]bool { } return services } - return make(map[string]bool) + return nil } func getRewrites(ingEx *IngressEx) map[string]string { @@ -388,7 +388,7 @@ func getRewrites(ingEx *IngressEx) map[string]string { } return rewrites } - return make(map[string]string) + return nil } func getSSLServices(ingEx *IngressEx) map[string]bool { @@ -399,7 +399,7 @@ func getSSLServices(ingEx *IngressEx) map[string]bool { } return services } - return make(map[string]bool) + return nil } func getGrpcServices(ingEx *IngressEx) map[string]bool { @@ -410,7 +410,7 @@ func getGrpcServices(ingEx *IngressEx) map[string]bool { } return services } - return make(map[string]bool) + return nil } func getSessionPersistenceServices(ingEx *IngressEx) map[string]string { @@ -421,7 +421,7 @@ func getSessionPersistenceServices(ingEx *IngressEx) map[string]string { } return services } - return make(map[string]string) + return nil } func filterMasterAnnotations(annotations map[string]string) []string { diff --git a/internal/configs/parsing_helpers.go b/internal/configs/parsing_helpers.go index f7fae19872..dc9176db50 100644 --- a/internal/configs/parsing_helpers.go +++ b/internal/configs/parsing_helpers.go @@ -227,7 +227,7 @@ func ParseSize(s string) (string, error) { // ParsePortList ensures that the string is a comma-separated list of port numbers func ParsePortList(s string) ([]int, error) { - ports := make([]int, 0) + var ports []int for _, value := range strings.Split(s, ",") { port, err := parsePort(value) if err != nil { From b046dad43d929353619b97e0acc601b236825383 Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Fri, 11 Dec 2020 10:39:28 +0000 Subject: [PATCH 11/13] ParseServiceList no longer returns an error --- internal/configs/annotations.go | 18 +++--------------- internal/configs/parsing_helpers.go | 4 ++-- internal/k8s/validation.go | 8 ++------ 3 files changed, 7 insertions(+), 23 deletions(-) diff --git a/internal/configs/annotations.go b/internal/configs/annotations.go index db06960d7f..d568d8b667 100644 --- a/internal/configs/annotations.go +++ b/internal/configs/annotations.go @@ -371,11 +371,7 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool func getWebsocketServices(ingEx *IngressEx) map[string]bool { if value, exists := ingEx.Ingress.Annotations["nginx.org/websocket-services"]; exists { - services, err := ParseServiceList(value) - if err != nil { - glog.Error(err) - } - return services + return ParseServiceList(value) } return nil } @@ -393,22 +389,14 @@ func getRewrites(ingEx *IngressEx) map[string]string { func getSSLServices(ingEx *IngressEx) map[string]bool { if value, exists := ingEx.Ingress.Annotations["nginx.org/ssl-services"]; exists { - services, err := ParseServiceList(value) - if err != nil { - glog.Error(err) - } - return services + return ParseServiceList(value) } return nil } func getGrpcServices(ingEx *IngressEx) map[string]bool { if value, exists := ingEx.Ingress.Annotations["nginx.org/grpc-services"]; exists { - services, err := ParseServiceList(value) - if err != nil { - glog.Error(err) - } - return services + return ParseServiceList(value) } return nil } diff --git a/internal/configs/parsing_helpers.go b/internal/configs/parsing_helpers.go index dc9176db50..6be33c07d3 100644 --- a/internal/configs/parsing_helpers.go +++ b/internal/configs/parsing_helpers.go @@ -252,12 +252,12 @@ func parsePort(value string) (int, error) { } // ParseServiceList ensures that the string is a comma-separated list of services -func ParseServiceList(s string) (map[string]bool, error) { +func ParseServiceList(s string) map[string]bool { services := make(map[string]bool) for _, part := range strings.Split(s, ",") { services[part] = true } - return services, nil + return services } // ParseRewriteList ensures that the string is a semicolon-separated list of services diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 983699d378..94e677e240 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -431,12 +431,8 @@ func validatePortListAnnotation(context *annotationValidationContext) field.Erro return allErrs } -func validateServiceListAnnotation(context *annotationValidationContext) field.ErrorList { - allErrs := field.ErrorList{} - if _, err := configs.ParseServiceList(context.value); err != nil { - return append(allErrs, field.Invalid(context.fieldPath, context.value, "must be a comma-separated list of services")) - } - return allErrs +func validateServiceListAnnotation(_ *annotationValidationContext) field.ErrorList { + return field.ErrorList{} } func validateStickyServiceListAnnotation(context *annotationValidationContext) field.ErrorList { From 1c00f496bb6779c0b2755aedb843f2551b2d5d3e Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Fri, 11 Dec 2020 16:07:06 +0000 Subject: [PATCH 12/13] Validate services in ingress annotations --- internal/k8s/validation.go | 39 +++++++- internal/k8s/validation_test.go | 167 ++++++++++++++++++++++++++++++++ 2 files changed, 204 insertions(+), 2 deletions(-) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 94e677e240..8e60655142 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "sort" + "strings" "github.com/nginxinc/kubernetes-ingress/internal/configs" networking "k8s.io/api/networking/v1beta1" @@ -60,6 +61,7 @@ const ( type annotationValidationContext struct { annotations map[string]string + specServices map[string]bool name string value string isPlus bool @@ -246,6 +248,7 @@ func validateIngress( allErrs := field.ErrorList{} allErrs = append(allErrs, validateIngressAnnotations( ing.Annotations, + getSpecServices(ing.Spec), isPlus, appProtectEnabled, internalRoutesEnabled, @@ -265,6 +268,7 @@ func validateIngress( func validateIngressAnnotations( annotations map[string]string, + specServices map[string]bool, isPlus bool, appProtectEnabled bool, internalRoutesEnabled bool, @@ -276,6 +280,7 @@ func validateIngressAnnotations( if value, exists := annotations[name]; exists { context := &annotationValidationContext{ annotations: annotations, + specServices: specServices, name: name, value: value, isPlus: isPlus, @@ -431,8 +436,23 @@ func validatePortListAnnotation(context *annotationValidationContext) field.Erro return allErrs } -func validateServiceListAnnotation(_ *annotationValidationContext) field.ErrorList { - return field.ErrorList{} +func validateServiceListAnnotation(context *annotationValidationContext) field.ErrorList { + allErrs := field.ErrorList{} + var unknownServices []string + annotationServices := configs.ParseServiceList(context.value) + for svc := range annotationServices { + if _, exists := context.specServices[svc]; !exists { + unknownServices = append(unknownServices, svc) + } + } + if len(unknownServices) > 0 { + errorMsg := fmt.Sprintf( + "must be a comma-separated list of services. The following services were not found: %s", + strings.Join(unknownServices, ","), + ) + return append(allErrs, field.Invalid(context.fieldPath, context.value, errorMsg)) + } + return allErrs } func validateStickyServiceListAnnotation(context *annotationValidationContext) field.ErrorList { @@ -521,3 +541,18 @@ func validateMinionSpec(spec *networking.IngressSpec, fieldPath *field.Path) fie return allErrs } + +func getSpecServices(ingressSpec networking.IngressSpec) map[string]bool { + services := make(map[string]bool) + if ingressSpec.Backend != nil { + services[ingressSpec.Backend.ServiceName] = true + } + for _, rule := range ingressSpec.Rules { + if rule.HTTP != nil { + for _, path := range rule.HTTP.Paths { + services[path.Backend.ServiceName] = true + } + } + } + return services +} diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 1a46274b81..36950c4a25 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -130,6 +130,7 @@ func TestValidateIngress(t *testing.T) { func TestValidateNginxIngressAnnotations(t *testing.T) { tests := []struct { annotations map[string]string + specServices map[string]bool isPlus bool appProtectEnabled bool internalRoutesEnabled bool @@ -138,6 +139,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }{ { annotations: map[string]string{}, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -150,6 +152,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/lb-method": "invalid_method", "nginx.org/mergeable-ingress-type": "invalid", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -164,6 +167,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "master", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -174,6 +178,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "minion", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -184,6 +189,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -196,6 +202,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/mergeable-ingress-type": "abc", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -209,6 +216,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/lb-method": "random", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -219,6 +227,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/lb-method": "least_time header", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -231,6 +240,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/lb-method": "invalid_method", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -244,6 +254,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/health-checks": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -256,6 +267,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/health-checks": "true", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -266,6 +278,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/health-checks": "not_a_boolean", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -279,6 +292,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/health-checks-mandatory": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -292,6 +306,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.com/health-checks": "true", "nginx.com/health-checks-mandatory": "true", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -303,6 +318,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.com/health-checks": "true", "nginx.com/health-checks-mandatory": "not_a_boolean", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -315,6 +331,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/health-checks-mandatory": "true", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -328,6 +345,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.com/health-checks": "false", "nginx.com/health-checks-mandatory": "true", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -341,6 +359,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/health-checks-mandatory-queue": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -355,6 +374,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.com/health-checks-mandatory": "true", "nginx.com/health-checks-mandatory-queue": "5", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -367,6 +387,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.com/health-checks-mandatory": "true", "nginx.com/health-checks-mandatory-queue": "not_a_number", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -379,6 +400,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/health-checks-mandatory-queue": "5", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -393,6 +415,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.com/health-checks-mandatory": "false", "nginx.com/health-checks-mandatory-queue": "5", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -406,6 +429,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/slow-start": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -418,6 +442,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/slow-start": "60s", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -428,6 +453,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/slow-start": "not_a_time", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -441,6 +467,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/server-tokens": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -451,6 +478,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/server-tokens": "custom_setting", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -461,6 +489,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/server-tokens": "custom_setting", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -474,6 +503,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/server-snippets": "snippet-1", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -484,6 +514,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/server-snippets": "snippet-1\nsnippet-2\nsnippet-3", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -495,6 +526,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/location-snippets": "snippet-1", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -505,6 +537,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/location-snippets": "snippet-1\nsnippet-2\nsnippet-3", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -516,6 +549,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-connect-timeout": "10s", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -527,6 +561,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-read-timeout": "10s", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -538,6 +573,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-send-timeout": "10s", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -549,6 +585,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-hide-headers": "header-1", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -559,6 +596,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-hide-headers": "header-1,header-2,header-3", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -570,6 +608,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-pass-headers": "header-1", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -580,6 +619,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-pass-headers": "header-1,header-2,header-3", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -591,6 +631,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/client-max-body-size": "16M", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -602,6 +643,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/redirect-to-https": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -612,6 +654,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/redirect-to-https": "not_a_boolean", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -625,6 +668,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "ingress.kubernetes.io/ssl-redirect": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -635,6 +679,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "ingress.kubernetes.io/ssl-redirect": "not_a_boolean", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -648,6 +693,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-buffering": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -658,6 +704,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-buffering": "not_a_boolean", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -671,6 +718,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/hsts": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -681,6 +729,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/hsts": "not_a_boolean", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -695,6 +744,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "true", "nginx.org/hsts-max-age": "120", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -706,6 +756,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "true", "nginx.org/hsts-max-age": "not_a_number", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -718,6 +769,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/hsts-max-age": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -731,6 +783,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "false", "nginx.org/hsts-max-age": "120", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -745,6 +798,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "true", "nginx.org/hsts-include-subdomains": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -756,6 +810,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "true", "nginx.org/hsts-include-subdomains": "not_a_boolean", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -768,6 +823,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/hsts-include-subdomains": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -781,6 +837,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "false", "nginx.org/hsts-include-subdomains": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -795,6 +852,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "true", "nginx.org/hsts-behind-proxy": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -806,6 +864,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "true", "nginx.org/hsts-behind-proxy": "not_a_boolean", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -818,6 +877,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/hsts-behind-proxy": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -831,6 +891,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { "nginx.org/hsts": "false", "nginx.org/hsts-behind-proxy": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -844,6 +905,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-buffers": "8 8k", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -855,6 +917,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-buffer-size": "16k", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -866,6 +929,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/proxy-max-temp-file-size": "128M", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -877,6 +941,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/upstream-zone-size": "512k", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -888,6 +953,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/jwt-realm": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -900,6 +966,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/jwt-realm": "my-jwt-realm", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -911,6 +978,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/jwt-key": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -923,6 +991,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/jwt-key": "my-jwk", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -934,6 +1003,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/jwt-token": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -946,6 +1016,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/jwt-token": "$cookie_auth_token", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -957,6 +1028,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/jwt-login-url": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -969,6 +1041,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/jwt-login-url": "https://login.example.com", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -980,6 +1053,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/listen-ports": "80,8080,9090", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -990,6 +1064,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/listen-ports": "not_a_port_list", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1003,6 +1078,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/listen-ports-ssl": "443,8443", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1013,6 +1089,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/listen-ports-ssl": "not_a_port_list", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1026,6 +1103,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/keepalive": "1000", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1036,6 +1114,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/keepalive": "not_a_number", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1049,6 +1128,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/max-fails": "5", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1059,6 +1139,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/max-fails": "not_a_number", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1072,6 +1153,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/max-conns": "10", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1082,6 +1164,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/max-conns": "not_a_number", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1095,6 +1178,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/fail-timeout": "10s", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1106,6 +1190,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "appprotect.f5.com/app-protect-enable": "true", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1118,6 +1203,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "appprotect.f5.com/app-protect-enable": "true", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: true, internalRoutesEnabled: false, @@ -1128,6 +1214,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "appprotect.f5.com/app-protect-enable": "not_a_boolean", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: true, internalRoutesEnabled: false, @@ -1141,6 +1228,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "appprotect.f5.com/app-protect-security-log-enable": "true", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1153,6 +1241,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "appprotect.f5.com/app-protect-security-log-enable": "true", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: true, internalRoutesEnabled: false, @@ -1163,6 +1252,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "appprotect.f5.com/app-protect-security-log-enable": "not_a_boolean", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: true, internalRoutesEnabled: false, @@ -1176,6 +1266,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nsm.nginx.com/internal-route": "true", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1188,6 +1279,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nsm.nginx.com/internal-route": "true", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: true, @@ -1198,6 +1290,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nsm.nginx.com/internal-route": "not_a_boolean", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: true, @@ -1211,6 +1304,9 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/websocket-services": "service-1", }, + specServices: map[string]bool{ + "service-1": true, + }, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1221,17 +1317,39 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/websocket-services": "service-1,service-2", }, + specServices: map[string]bool{ + "service-1": true, + "service-2": true, + }, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, expectedErrors: nil, msg: "valid nginx.org/websocket-services annotation, multi-value", }, + { + annotations: map[string]string{ + "nginx.org/websocket-services": "service-1,service-2", + }, + specServices: map[string]bool{ + "service-1": true, + }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.nginx.org/websocket-services: Invalid value: "service-1,service-2": must be a comma-separated list of services. The following services were not found: service-2`, + }, + msg: "invalid nginx.org/websocket-services annotation, service does not exist", + }, { annotations: map[string]string{ "nginx.org/ssl-services": "service-1", }, + specServices: map[string]bool{ + "service-1": true, + }, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1242,17 +1360,39 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/ssl-services": "service-1,service-2", }, + specServices: map[string]bool{ + "service-1": true, + "service-2": true, + }, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, expectedErrors: nil, msg: "valid nginx.org/ssl-services annotation, multi-value", }, + { + annotations: map[string]string{ + "nginx.org/ssl-services": "service-1,service-2", + }, + specServices: map[string]bool{ + "service-1": true, + }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.nginx.org/ssl-services: Invalid value: "service-1,service-2": must be a comma-separated list of services. The following services were not found: service-2`, + }, + msg: "invalid nginx.org/ssl-services annotation, service does not exist", + }, { annotations: map[string]string{ "nginx.org/grpc-services": "service-1", }, + specServices: map[string]bool{ + "service-1": true, + }, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1263,17 +1403,37 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/grpc-services": "service-1,service-2", }, + specServices: map[string]bool{ + "service-1": true, + "service-2": true, + }, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, expectedErrors: nil, msg: "valid nginx.org/grpc-services annotation, multi-value", }, + { + annotations: map[string]string{ + "nginx.org/grpc-services": "service-1,service-2", + }, + specServices: map[string]bool{ + "service-1": true, + }, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: []string{ + `annotations.nginx.org/grpc-services: Invalid value: "service-1,service-2": must be a comma-separated list of services. The following services were not found: service-2`, + }, + msg: "invalid nginx.org/grpc-services annotation, service does not exist", + }, { annotations: map[string]string{ "nginx.org/rewrites": "serviceName=service-1 rewrite=rewrite-1", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1284,6 +1444,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/rewrites": "serviceName=service-1 rewrite=rewrite-1;serviceName=service-2 rewrite=rewrite-2", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1294,6 +1455,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.org/rewrites": "not_a_rewrite", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: true, @@ -1307,6 +1469,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/sticky-cookie-services": "true", }, + specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1319,6 +1482,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/sticky-cookie-services": "serviceName=service-1 srv_id expires=1h path=/service-1", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1329,6 +1493,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/sticky-cookie-services": "serviceName=service-1 srv_id expires=1h path=/service-1;serviceName=service-2 srv_id expires=2h path=/service-2", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1339,6 +1504,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { annotations: map[string]string{ "nginx.com/sticky-cookie-services": "not_a_rewrite", }, + specServices: map[string]bool{}, isPlus: true, appProtectEnabled: false, internalRoutesEnabled: false, @@ -1353,6 +1519,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { t.Run(test.msg, func(t *testing.T) { allErrs := validateIngressAnnotations( test.annotations, + test.specServices, test.isPlus, test.appProtectEnabled, test.internalRoutesEnabled, From 36dbc9194467a2abb000e14c949b5aeb48cb28bc Mon Sep 17 00:00:00 2001 From: Mike Stephen Date: Mon, 14 Dec 2020 12:43:55 +0000 Subject: [PATCH 13/13] hsts annotation validation has been relaxed to comform with existing behaviour --- internal/k8s/validation.go | 11 +++++-- internal/k8s/validation_test.go | 58 +++++++++++++++------------------ 2 files changed, 34 insertions(+), 35 deletions(-) diff --git a/internal/k8s/validation.go b/internal/k8s/validation.go index 8e60655142..198217d99b 100644 --- a/internal/k8s/validation.go +++ b/internal/k8s/validation.go @@ -137,17 +137,17 @@ var ( validateBoolAnnotation, }, hstsMaxAgeAnnotation: { - validateRelatedAnnotation(hstsAnnotation, validateIsTrue), + validateRelatedAnnotation(hstsAnnotation, validateIsBool), validateRequiredAnnotation, validateInt64Annotation, }, hstsIncludeSubdomainsAnnotation: { - validateRelatedAnnotation(hstsAnnotation, validateIsTrue), + validateRelatedAnnotation(hstsAnnotation, validateIsBool), validateRequiredAnnotation, validateBoolAnnotation, }, hstsBehindProxyAnnotation: { - validateRelatedAnnotation(hstsAnnotation, validateIsTrue), + validateRelatedAnnotation(hstsAnnotation, validateIsBool), validateRequiredAnnotation, validateBoolAnnotation, }, @@ -471,6 +471,11 @@ func validateRewriteListAnnotation(context *annotationValidationContext) field.E return allErrs } +func validateIsBool(v string) error { + _, err := configs.ParseBool(v) + return err +} + func validateIsTrue(v string) error { b, err := configs.ParseBool(v) if err != nil { diff --git a/internal/k8s/validation_test.go b/internal/k8s/validation_test.go index 36950c4a25..904e5de935 100644 --- a/internal/k8s/validation_test.go +++ b/internal/k8s/validation_test.go @@ -751,6 +751,18 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { expectedErrors: nil, msg: "valid nginx.org/hsts-max-age annotation", }, + { + annotations: map[string]string{ + "nginx.org/hsts": "false", + "nginx.org/hsts-max-age": "120", + }, + specServices: map[string]bool{}, + isPlus: false, + appProtectEnabled: false, + internalRoutesEnabled: false, + expectedErrors: nil, + msg: "valid nginx.org/hsts-max-age nginx.org/hsts can be false", + }, { annotations: map[string]string{ "nginx.org/hsts": "true", @@ -778,24 +790,22 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }, msg: "invalid nginx.org/hsts-max-age, related annotation nginx.org/hsts not set", }, + { annotations: map[string]string{ - "nginx.org/hsts": "false", - "nginx.org/hsts-max-age": "120", + "nginx.org/hsts": "true", + "nginx.org/hsts-include-subdomains": "true", }, specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, - expectedErrors: []string{ - "annotations.nginx.org/hsts-max-age: Forbidden: related annotation nginx.org/hsts: must be true", - }, - msg: "invalid nginx.org/hsts-max-age nginx.org/hsts is not true", + expectedErrors: nil, + msg: "valid nginx.org/hsts-include-subdomains annotation", }, - { annotations: map[string]string{ - "nginx.org/hsts": "true", + "nginx.org/hsts": "false", "nginx.org/hsts-include-subdomains": "true", }, specServices: map[string]bool{}, @@ -803,7 +813,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { appProtectEnabled: false, internalRoutesEnabled: false, expectedErrors: nil, - msg: "valid nginx.org/hsts-include-subdomains annotation", + msg: "valid nginx.org/hsts-include-subdomains, nginx.org/hsts can be false", }, { annotations: map[string]string{ @@ -832,24 +842,22 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }, msg: "invalid nginx.org/hsts-include-subdomains, related annotation nginx.org/hsts not set", }, + { annotations: map[string]string{ - "nginx.org/hsts": "false", - "nginx.org/hsts-include-subdomains": "true", + "nginx.org/hsts": "true", + "nginx.org/hsts-behind-proxy": "true", }, specServices: map[string]bool{}, isPlus: false, appProtectEnabled: false, internalRoutesEnabled: false, - expectedErrors: []string{ - "annotations.nginx.org/hsts-include-subdomains: Forbidden: related annotation nginx.org/hsts: must be true", - }, - msg: "invalid nginx.org/hsts-include-subdomains nginx.org/hsts is not true", + expectedErrors: nil, + msg: "valid nginx.org/hsts-behind-proxy annotation", }, - { annotations: map[string]string{ - "nginx.org/hsts": "true", + "nginx.org/hsts": "false", "nginx.org/hsts-behind-proxy": "true", }, specServices: map[string]bool{}, @@ -857,7 +865,7 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { appProtectEnabled: false, internalRoutesEnabled: false, expectedErrors: nil, - msg: "valid nginx.org/hsts-behind-proxy annotation", + msg: "valid nginx.org/hsts-behind-proxy, nginx.org/hsts can be false", }, { annotations: map[string]string{ @@ -886,20 +894,6 @@ func TestValidateNginxIngressAnnotations(t *testing.T) { }, msg: "invalid nginx.org/hsts-behind-proxy, related annotation nginx.org/hsts not set", }, - { - annotations: map[string]string{ - "nginx.org/hsts": "false", - "nginx.org/hsts-behind-proxy": "true", - }, - specServices: map[string]bool{}, - isPlus: false, - appProtectEnabled: false, - internalRoutesEnabled: false, - expectedErrors: []string{ - "annotations.nginx.org/hsts-behind-proxy: Forbidden: related annotation nginx.org/hsts: must be true", - }, - msg: "invalid nginx.org/hsts-behind-proxy nginx.org/hsts is not true", - }, { annotations: map[string]string{