code review feedback

openshift · Apr 22, 2021 · b6dcb74 · b6dcb74
1 parent 48c4bd7
commit b6dcb74
Show file tree

Hide file tree

Showing 3 changed files with 83 additions and 7 deletions.
diff --git a/pkg/asset/manifests/infrastructure.go b/pkg/asset/manifests/infrastructure.go
@@ -5,11 +5,10 @@ import (
 	"sort"
 
 	"github.com/ghodss/yaml"
+	configv1 "github.com/openshift/api/config/v1"
 	"github.com/pkg/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
-	configv1 "github.com/openshift/api/config/v1"
-
 	"github.com/openshift/installer/pkg/asset"
 	"github.com/openshift/installer/pkg/asset/installconfig"
 	gcpmanifests "github.com/openshift/installer/pkg/asset/manifests/gcp"

diff --git a/pkg/types/aws/validation/platform.go b/pkg/types/aws/validation/platform.go
@@ -11,13 +11,13 @@ import (
 	"github.com/openshift/installer/pkg/types/aws"
 )
 
-// tagRegex is used to check that the keys and values of a tag contain only valid characters
+// tagRegex is used to check that the keys and values of a tag contain only valid characters.
 var tagRegex = regexp.MustCompile(`^[0-9A-Za-z_.:/=+-@]*$`)
 
-// kubernetesNamespaceRegex is used to check that a tag key is not in the kubernetes.io namespace
+// kubernetesNamespaceRegex is used to check that a tag key is not in the kubernetes.io namespace.
 var kubernetesNamespaceRegex = regexp.MustCompile(`^([^/]*\.)?kubernetes.io/`)
 
-// openshiftNamespaceRegex is used to check that a tag key is not in the openshift.io namespace
+// openshiftNamespaceRegex is used to check that a tag key is not in the openshift.io namespace.
 var openshiftNamespaceRegex = regexp.MustCompile(`^([^/]*\.)?openshift.io/`)
 
 // ValidatePlatform checks that the specified platform is valid.
@@ -43,7 +43,7 @@ func ValidatePlatform(p *aws.Platform, fldPath *field.Path) field.ErrorList {
 	return allErrs
 }
 
-func validateUserTags(tags map[string]string, usingTagProgagation bool, fldPath *field.Path) field.ErrorList {
+func validateUserTags(tags map[string]string, propagatingTags bool, fldPath *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
 	if len(tags) == 0 {
 		return allErrs
@@ -52,7 +52,7 @@ func validateUserTags(tags map[string]string, usingTagProgagation bool, fldPath
 		if strings.EqualFold(key, "Name") {
 			allErrs = append(allErrs, field.Invalid(fldPath.Key(key), tags[key], "Name key is not allowed for user defined tags"))
 		}
-		if usingTagProgagation {
+		if propagatingTags {
 			if err := validateTag(key, value); err != nil {
 				allErrs = append(allErrs, field.Invalid(fldPath.Key(key), value, err.Error()))
 			}

diff --git a/pkg/types/aws/validation/platform_test.go b/pkg/types/aws/validation/platform_test.go
@@ -1,6 +1,7 @@
 package validation
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -174,3 +175,79 @@ func TestValidatePlatform(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateTag(t *testing.T) {
+	cases := []struct {
+		name      string
+		key       string
+		value     string
+		expectErr bool
+	}{{
+		name:  "valid",
+		key:   "test-key",
+		value: "test-value",
+	}, {
+		name:      "invalid characters in key",
+		key:       "bad-key***",
+		value:     "test-value",
+		expectErr: true,
+	}, {
+		name:      "invalid characters in value",
+		key:       "test-key",
+		value:     "bad-value***",
+		expectErr: true,
+	}, {
+		name:      "empty key",
+		key:       "",
+		value:     "test-value",
+		expectErr: true,
+	}, {
+		name:      "empty value",
+		key:       "test-key",
+		value:     "",
+		expectErr: true,
+	}, {
+		name:      "key too long",
+		key:       strings.Repeat("a", 129),
+		value:     "test-value",
+		expectErr: true,
+	}, {
+		name:      "value too long",
+		key:       "test-key",
+		value:     strings.Repeat("a", 257),
+		expectErr: true,
+	}, {
+		name:      "key in kubernetes.io namespace",
+		key:       "kubernetes.io/cluster/some-cluster",
+		value:     "owned",
+		expectErr: true,
+	}, {
+		name:      "key in openshift.io namespace",
+		key:       "openshift.io/some-key",
+		value:     "some-value",
+		expectErr: true,
+	}, {
+		name:      "key in openshift.io subdomain namespace",
+		key:       "other.openshift.io/some-key",
+		value:     "some-value",
+		expectErr: true,
+	}, {
+		name:  "key in namespace similar to openshift.io",
+		key:   "otheropenshift.io/some-key",
+		value: "some-value",
+	}, {
+		name:  "key with openshift.io in path",
+		key:   "some-domain/openshift.io/some-key",
+		value: "some-value",
+	}}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			err := validateTag(tc.key, tc.value)
+			if tc.expectErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
+}