From 301b4070f982753e7926e6904c5424f44022c7ee Mon Sep 17 00:00:00 2001 From: David Eliahu Date: Tue, 22 Sep 2020 16:06:09 -0700 Subject: [PATCH 1/4] Support tags with spaces and other special characters --- manager/cluster_config_env.py | 2 +- manager/install.sh | 2 +- manager/manifests/fluentd.yaml | 2 +- manager/manifests/istio-values.yaml | 4 +- pkg/lib/configreader/errors.go | 18 ++- pkg/lib/configreader/string.go | 14 ++ pkg/lib/configreader/string_ptr.go | 4 + pkg/lib/regex/regex.go | 17 ++- pkg/lib/regex/regex_test.go | 166 +++++++++++++++++++++++ pkg/types/clusterconfig/clusterconfig.go | 23 ++-- 10 files changed, 233 insertions(+), 19 deletions(-) diff --git a/manager/cluster_config_env.py b/manager/cluster_config_env.py index bd50bb563a..e9af68048a 100644 --- a/manager/cluster_config_env.py +++ b/manager/cluster_config_env.py @@ -20,7 +20,7 @@ def export(base_key, value): if base_key.lower().startswith("cortex_tags"): inlined_tags = ",".join([f"{k}={v}" for k, v in value.items()]) - print(f"export CORTEX_TAGS={inlined_tags}") + print(f"export CORTEX_TAGS='{inlined_tags}'") print(f"export CORTEX_TAGS_JSON='{json.dumps(value)}'") return diff --git a/manager/install.sh b/manager/install.sh index d2dfa16948..1d1cb02ae3 100755 --- a/manager/install.sh +++ b/manager/install.sh @@ -189,7 +189,7 @@ function main() { fi # create VPC Link - create_vpc_link_output=$(aws apigatewayv2 create-vpc-link --region $CORTEX_REGION --tags $CORTEX_TAGS --name $CORTEX_CLUSTER_NAME --subnet-ids $private_subnets --security-group-ids $default_security_group) + create_vpc_link_output=$(aws apigatewayv2 create-vpc-link --region $CORTEX_REGION --tags "$CORTEX_TAGS_JSON" --name $CORTEX_CLUSTER_NAME --subnet-ids $private_subnets --security-group-ids $default_security_group) vpc_link_id=$(echo $create_vpc_link_output | jq .VpcLinkId | tr -d '"') if [ "$vpc_link_id" = "" ] || [ "$vpc_link_id" = "null" ]; then echo -e "unable to extract vpc link ID from create-vpc-link output:\n$create_vpc_link_output" diff --git a/manager/manifests/fluentd.yaml b/manager/manifests/fluentd.yaml index a5b814bef4..e7bd3aa425 100644 --- a/manager/manifests/fluentd.yaml +++ b/manager/manifests/fluentd.yaml @@ -145,7 +145,7 @@ data: region "#{ENV['AWS_REGION']}" log_group_name_key group_name log_stream_name_key stream_name - log_group_aws_tags ${CORTEX_TAGS_JSON} + log_group_aws_tags "${CORTEX_TAGS_JSON}" remove_log_stream_name_key true remove_log_group_name_key true auto_create_stream true diff --git a/manager/manifests/istio-values.yaml b/manager/manifests/istio-values.yaml index 09c35aae9e..b2ae8397f1 100644 --- a/manager/manifests/istio-values.yaml +++ b/manager/manifests/istio-values.yaml @@ -41,7 +41,7 @@ gateways: serviceAnnotations: ${CORTEX_OPERATOR_LOAD_BALANCER_ANNOTATION} service.beta.kubernetes.io/aws-load-balancer-type: "nlb" - service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags: ${CORTEX_TAGS} + service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags: "${CORTEX_TAGS}" type: LoadBalancer externalTrafficPolicy: Local # https://medium.com/pablo-perez/k8s-externaltrafficpolicy-local-or-cluster-40b259a19404, https://www.asykim.com/blog/deep-dive-into-kubernetes-external-traffic-policies ports: @@ -84,7 +84,7 @@ gateways: serviceAnnotations: ${CORTEX_API_LOAD_BALANCER_ANNOTATION} service.beta.kubernetes.io/aws-load-balancer-type: "nlb" - service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags: ${CORTEX_TAGS} + service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags: "${CORTEX_TAGS}" ${CORTEX_SSL_CERTIFICATE_ANNOTATION} service.beta.kubernetes.io/aws-load-balancer-ssl-ports: "443" type: LoadBalancer diff --git a/pkg/lib/configreader/errors.go b/pkg/lib/configreader/errors.go index 527e2dc2bd..51f30c3779 100644 --- a/pkg/lib/configreader/errors.go +++ b/pkg/lib/configreader/errors.go @@ -32,6 +32,8 @@ const ( ErrInvalidYAML = "configreader.invalid_yaml" ErrTooLong = "configreader.too_long" ErrTooShort = "configreader.too_short" + ErrLeadingWhitespace = "configreader.leading_whitespace" + ErrTrailingWhitespace = "configreader.trailing_whitespace" ErrAlphaNumericDashUnderscore = "configreader.alpha_numeric_dash_underscore" ErrAlphaNumericDashDotUnderscore = "configreader.alpha_numeric_dash_dot_underscore" ErrInvalidAWSTag = "configreader.invalid_aws_tag" @@ -114,6 +116,20 @@ func ErrorTooShort(provided string, minLen int) error { }) } +func ErrorLeadingWhitespace(provided string) error { + return errors.WithStack(&errors.Error{ + Kind: ErrLeadingWhitespace, + Message: fmt.Sprintf("%s cannot start with whitespace", s.UserStr(provided)), + }) +} + +func ErrorTrailingWhitespace(provided string) error { + return errors.WithStack(&errors.Error{ + Kind: ErrTrailingWhitespace, + Message: fmt.Sprintf("%s cannot end with whitespace", s.UserStr(provided)), + }) +} + func ErrorAlphaNumericDashUnderscore(provided string) error { return errors.WithStack(&errors.Error{ Kind: ErrAlphaNumericDashUnderscore, @@ -131,7 +147,7 @@ func ErrorAlphaNumericDashDotUnderscore(provided string) error { func ErrorInvalidAWSTag(provided string) error { return errors.WithStack(&errors.Error{ Kind: ErrInvalidAWSTag, - Message: fmt.Sprintf("%s must contain only letters, numbers, spaces, and the following characters: _ . : / = + - @", s.UserStr(provided)), + Message: fmt.Sprintf("%s must contain only letters, numbers, spaces, and the following characters: _ . : / + - @", s.UserStr(provided)), }) } diff --git a/pkg/lib/configreader/string.go b/pkg/lib/configreader/string.go index b825460d9e..9a9a2ab6d8 100644 --- a/pkg/lib/configreader/string.go +++ b/pkg/lib/configreader/string.go @@ -42,6 +42,8 @@ type StringValidation struct { InvalidPrefixes []string MaxLength int MinLength int + DisallowLeadingWhitespace bool + DisallowTrailingWhitespace bool AlphaNumericDashDotUnderscoreOrEmpty bool AlphaNumericDashDotUnderscore bool AlphaNumericDashUnderscore bool @@ -257,6 +259,18 @@ func ValidateStringVal(val string, v *StringValidation) error { } } + if v.DisallowLeadingWhitespace { + if regex.HasLeadingWhitespace(val) { + return ErrorLeadingWhitespace(val) + } + } + + if v.DisallowTrailingWhitespace { + if regex.HasTrailingWhitespace(val) { + return ErrorTrailingWhitespace(val) + } + } + if v.AlphaNumericDashDotUnderscore { if !regex.IsAlphaNumericDashDotUnderscore(val) { return ErrorAlphaNumericDashDotUnderscore(val) diff --git a/pkg/lib/configreader/string_ptr.go b/pkg/lib/configreader/string_ptr.go index e0830bf071..b882c0c641 100644 --- a/pkg/lib/configreader/string_ptr.go +++ b/pkg/lib/configreader/string_ptr.go @@ -35,6 +35,8 @@ type StringPtrValidation struct { InvalidPrefixes []string MaxLength int MinLength int + DisallowLeadingWhitespace bool + DisallowTrailingWhitespace bool AlphaNumericDashDotUnderscoreOrEmpty bool AlphaNumericDashDotUnderscore bool AlphaNumericDashUnderscore bool @@ -59,6 +61,8 @@ func makeStringValValidation(v *StringPtrValidation) *StringValidation { InvalidPrefixes: v.InvalidPrefixes, MaxLength: v.MaxLength, MinLength: v.MinLength, + DisallowLeadingWhitespace: v.DisallowLeadingWhitespace, + DisallowTrailingWhitespace: v.DisallowTrailingWhitespace, AlphaNumericDashDotUnderscoreOrEmpty: v.AlphaNumericDashDotUnderscoreOrEmpty, AlphaNumericDashDotUnderscore: v.AlphaNumericDashDotUnderscore, AlphaNumericDashUnderscore: v.AlphaNumericDashUnderscore, diff --git a/pkg/lib/regex/regex.go b/pkg/lib/regex/regex.go index 451e1b584e..e2b583d702 100644 --- a/pkg/lib/regex/regex.go +++ b/pkg/lib/regex/regex.go @@ -29,8 +29,21 @@ func MatchAnyRegex(s string, regexes []*regexp.Regexp) bool { return false } -// letters, numbers, spaces representable in UTF-8, and the following characters: _ . : / = + - @ -var _awsTagRegex = regexp.MustCompile(`^[\sa-zA-Z0-9_\-\.:/=+@]+$`) +var _leadingWhitespaceRegex = regexp.MustCompile(`^\s+`) + +func HasLeadingWhitespace(s string) bool { + return _leadingWhitespaceRegex.MatchString(s) +} + +var _trailingWhitespaceRegex = regexp.MustCompile(`\s+$`) + +func HasTrailingWhitespace(s string) bool { + return _trailingWhitespaceRegex.MatchString(s) +} + +// letters, numbers, spaces representable in UTF-8, and the following characters: _ . : / + - @ +// = is not supported because it doesn't propagate to the NLB correctly (via the k8s service annotation) +var _awsTagRegex = regexp.MustCompile(`^[\sa-zA-Z0-9_\-\.:/+@]+$`) func IsValidAWSTag(s string) bool { return _awsTagRegex.MatchString(s) diff --git a/pkg/lib/regex/regex_test.go b/pkg/lib/regex/regex_test.go index 777ab19cb4..643780c3b7 100644 --- a/pkg/lib/regex/regex_test.go +++ b/pkg/lib/regex/regex_test.go @@ -19,6 +19,8 @@ package regex import ( "strings" "testing" + + "github.com/stretchr/testify/assert" ) type regexpMatch struct { @@ -27,6 +29,170 @@ type regexpMatch struct { subs []string } +func TestHasLeadingWhitespace(t *testing.T) { + testcases := []regexpMatch{ + { + input: " test", + match: true, + }, + { + input: " test", + match: true, + }, + { + input: "\ntest", + match: true, + }, + { + input: " test ", + match: true, + }, + { + input: " test ", + match: true, + }, + { + input: "\ntest\n", + match: true, + }, + { + input: "test", + match: false, + }, + { + input: "te st", + match: false, + }, + { + input: "te st", + match: false, + }, + { + input: "te\nst", + match: false, + }, + { + input: "_", + match: false, + }, + { + input: "test ", + match: false, + }, + { + input: "test ", + match: false, + }, + { + input: "test\n", + match: false, + }, + { + input: " ", + match: true, + }, + { + input: " ", + match: true, + }, + { + input: "\n", + match: true, + }, + { + input: "", + match: false, + }, + } + + for i := range testcases { + match := HasLeadingWhitespace(testcases[i].input) + assert.Equal(t, testcases[i].match, match, "input: "+testcases[i].input) + } +} + +func TestHasTrailingWhitespace(t *testing.T) { + testcases := []regexpMatch{ + { + input: " test", + match: false, + }, + { + input: " test", + match: false, + }, + { + input: "\ntest", + match: false, + }, + { + input: " test ", + match: true, + }, + { + input: " test ", + match: true, + }, + { + input: "\ntest\n", + match: true, + }, + { + input: "test", + match: false, + }, + { + input: "te st", + match: false, + }, + { + input: "te st", + match: false, + }, + { + input: "te\nst", + match: false, + }, + { + input: "_", + match: false, + }, + { + input: "test ", + match: true, + }, + { + input: "test ", + match: true, + }, + { + input: "test\n", + match: true, + }, + { + input: " ", + match: true, + }, + { + input: " ", + match: true, + }, + { + input: "\n", + match: true, + }, + { + input: "", + match: false, + }, + } + + for i := range testcases { + match := HasTrailingWhitespace(testcases[i].input) + assert.Equal(t, testcases[i].match, match, "input: "+testcases[i].input) + } +} + func TestAlphaNumericDashDotUnderscoreRegex(t *testing.T) { testcases := []regexpMatch{ { diff --git a/pkg/types/clusterconfig/clusterconfig.go b/pkg/types/clusterconfig/clusterconfig.go index 36326bbda8..c22f152d04 100644 --- a/pkg/types/clusterconfig/clusterconfig.go +++ b/pkg/types/clusterconfig/clusterconfig.go @@ -49,6 +49,7 @@ var ( _cachedCNISupportedInstances *string // This regex is stricter than the actual S3 rules _strictS3BucketRegex = regexp.MustCompile(`^([a-z0-9])+(-[a-z0-9]+)*$`) + _invalidTagPrefixes = []string{"cortex.dev/", "kubernetes.io/", "k8s.io/", "eksctl.", "alpha.eksctl.", "beta.eksctl.", "aws:", "Aws:", "aWs:", "awS:", "aWS:", "AwS:", "aWS:", "AWS:"} ) type Config struct { @@ -165,16 +166,19 @@ var UserValidation = &cr.StructValidation{ AllowEmpty: true, ConvertNullToEmpty: true, KeyStringValidator: &cr.StringValidation{ - MinLength: 1, - MaxLength: 127, - InvalidPrefixes: []string{"aws:", "Aws:", "aWs:", "awS:", "aWS:", "AwS:", "aWS:", "AWS:"}, - AWSTag: true, + MinLength: 1, + MaxLength: 127, + DisallowLeadingWhitespace: true, + DisallowTrailingWhitespace: true, + InvalidPrefixes: _invalidTagPrefixes, + AWSTag: true, }, ValueStringValidator: &cr.StringValidation{ - MinLength: 1, - MaxLength: 255, - InvalidPrefixes: []string{"aws:", "Aws:", "aWs:", "awS:", "aWS:", "AwS:", "aWS:", "AWS:"}, - AWSTag: true, + MinLength: 1, + MaxLength: 255, + DisallowTrailingWhitespace: true, + InvalidPrefixes: _invalidTagPrefixes, + AWSTag: true, }, }, }, @@ -620,9 +624,6 @@ func (cc *Config) Validate(awsClient *aws.Client) error { } } - if cc.Tags[ClusterNameTag] != "" && cc.Tags[ClusterNameTag] != cc.ClusterName { - return ErrorCantOverrideDefaultTag() - } cc.Tags[ClusterNameTag] = cc.ClusterName if err := cc.validateAvailabilityZones(awsClient); err != nil { From ce7ec16a62274b11f813938f264f815338a8a699 Mon Sep 17 00:00:00 2001 From: David Eliahu Date: Tue, 22 Sep 2020 16:33:19 -0700 Subject: [PATCH 2/4] Update cortex.dev tag validation --- pkg/types/clusterconfig/clusterconfig.go | 8 +++++++- pkg/types/clusterconfig/errors.go | 8 -------- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/pkg/types/clusterconfig/clusterconfig.go b/pkg/types/clusterconfig/clusterconfig.go index c22f152d04..c3587b23fd 100644 --- a/pkg/types/clusterconfig/clusterconfig.go +++ b/pkg/types/clusterconfig/clusterconfig.go @@ -49,7 +49,7 @@ var ( _cachedCNISupportedInstances *string // This regex is stricter than the actual S3 rules _strictS3BucketRegex = regexp.MustCompile(`^([a-z0-9])+(-[a-z0-9]+)*$`) - _invalidTagPrefixes = []string{"cortex.dev/", "kubernetes.io/", "k8s.io/", "eksctl.", "alpha.eksctl.", "beta.eksctl.", "aws:", "Aws:", "aWs:", "awS:", "aWS:", "AwS:", "aWS:", "AWS:"} + _invalidTagPrefixes = []string{"kubernetes.io/", "k8s.io/", "eksctl.", "alpha.eksctl.", "beta.eksctl.", "aws:", "Aws:", "aWs:", "awS:", "aWS:", "AwS:", "aWS:", "AWS:"} ) type Config struct { @@ -551,6 +551,7 @@ func (cc *Config) SQSNamePrefix() string { return SQSNamePrefix(cc.ClusterName) } +// this validates the user-provided cluster conifg func (cc *Config) Validate(awsClient *aws.Client) error { fmt.Print("verifying your configuration ...\n\n") @@ -624,6 +625,11 @@ func (cc *Config) Validate(awsClient *aws.Client) error { } } + for tagName := range cc.Tags { + if strings.HasPrefix(tagName, "cortex.dev/") { + return errors.Wrap(cr.ErrorCantHavePrefix(tagName, "cortex.dev/"), TagsKey) + } + } cc.Tags[ClusterNameTag] = cc.ClusterName if err := cc.validateAvailabilityZones(awsClient); err != nil { diff --git a/pkg/types/clusterconfig/errors.go b/pkg/types/clusterconfig/errors.go index 866e17afb6..a88b6036b9 100644 --- a/pkg/types/clusterconfig/errors.go +++ b/pkg/types/clusterconfig/errors.go @@ -52,7 +52,6 @@ const ( ErrInvalidInstanceType = "clusterconfig.invalid_instance_type" ErrIOPSNotSupported = "clusterconfig.iops_not_supported" ErrIOPSTooLarge = "clusterconfig.iops_too_large" - ErrCantOverrideDefaultTag = "clusterconfig.cant_override_default_tag" ErrSSLCertificateARNNotFound = "clusterconfig.ssl_certificate_arn_not_found" ) @@ -236,13 +235,6 @@ func ErrorIOPSTooLarge(iops int64, volumeSize int64) error { }) } -func ErrorCantOverrideDefaultTag() error { - return errors.WithStack(&errors.Error{ - Kind: ErrCantOverrideDefaultTag, - Message: fmt.Sprintf("the \"%s\" tag cannot be overridden (it is set by default, and it must always be equal to your cluster name)", ClusterNameTag), - }) -} - func ErrorSSLCertificateARNNotFound(sslCertificateARN string, region string) error { return errors.WithStack(&errors.Error{ Kind: ErrSSLCertificateARNNotFound, From a1c7b7f95741aafb70c2bee2b1e884297a164141 Mon Sep 17 00:00:00 2001 From: David Eliahu Date: Tue, 22 Sep 2020 17:04:39 -0700 Subject: [PATCH 3/4] Update clusterconfig.go --- pkg/types/clusterconfig/clusterconfig.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/types/clusterconfig/clusterconfig.go b/pkg/types/clusterconfig/clusterconfig.go index c3587b23fd..eb4b8ee4f1 100644 --- a/pkg/types/clusterconfig/clusterconfig.go +++ b/pkg/types/clusterconfig/clusterconfig.go @@ -176,6 +176,7 @@ var UserValidation = &cr.StructValidation{ ValueStringValidator: &cr.StringValidation{ MinLength: 1, MaxLength: 255, + DisallowLeadingWhitespace: true, DisallowTrailingWhitespace: true, InvalidPrefixes: _invalidTagPrefixes, AWSTag: true, From a44718d0dfa6256a041b6ce90db2142ef0ff2ba5 Mon Sep 17 00:00:00 2001 From: David Eliahu Date: Tue, 22 Sep 2020 21:37:21 -0700 Subject: [PATCH 4/4] Address PR comment --- pkg/types/clusterconfig/clusterconfig.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/types/clusterconfig/clusterconfig.go b/pkg/types/clusterconfig/clusterconfig.go index eb4b8ee4f1..aa8bf547e2 100644 --- a/pkg/types/clusterconfig/clusterconfig.go +++ b/pkg/types/clusterconfig/clusterconfig.go @@ -552,7 +552,7 @@ func (cc *Config) SQSNamePrefix() string { return SQSNamePrefix(cc.ClusterName) } -// this validates the user-provided cluster conifg +// this validates the user-provided cluster config func (cc *Config) Validate(awsClient *aws.Client) error { fmt.Print("verifying your configuration ...\n\n")