Support AWS tags with spaces and valid special characters (#1374)

deliahu · web-flow · commit 5a8c4f72ce3d · 2020-09-22T21:40:31.000-07:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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{"kubernetes.io/", "k8s.io/", "eksctl.", "alpha.eksctl.", "beta.eksctl.", "aws:", "Aws:", "aWs:", "awS:", "aWS:", "AwS:", "aWS:", "AWS:"}
 )
 
 type Config struct {
@@ -165,16 +166,20 @@ 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,
+					DisallowLeadingWhitespace:  true,
+					DisallowTrailingWhitespace: true,
+					InvalidPrefixes:            _invalidTagPrefixes,
+					AWSTag:                     true,
 				},
 			},
 		},
@@ -547,6 +552,7 @@ func (cc *Config) SQSNamePrefix() string {
 	return SQSNamePrefix(cc.ClusterName)
 }
 
+// this validates the user-provided cluster config
 func (cc *Config) Validate(awsClient *aws.Client) error {
 	fmt.Print("verifying your configuration ...\n\n")
 
@@ -620,8 +626,10 @@ func (cc *Config) Validate(awsClient *aws.Client) error {
 		}
 	}
 
-	if cc.Tags[ClusterNameTag] != "" && cc.Tags[ClusterNameTag] != cc.ClusterName {
-		return ErrorCantOverrideDefaultTag()
+	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
 
diff --git a/pkg/types/clusterconfig/errors.go b/pkg/types/clusterconfig/errors.go
