CR-13046 cluster name bug (#485)

* fix cluster name validation + tests

* small test change

* fixes

* small fix + bump

Author: rotem-codefresh

Makefile

@@ -1,4 +1,4 @@
-VERSION=v0.0.430
+VERSION=v0.0.431
 
 OUT_DIR=dist
 YEAR?=$(shell date +"%Y")

cmd/commands/cluster.go

@@ -193,43 +193,57 @@ func setClusterName(ctx context.Context, opts *ClusterAddOptions) error {
 	}
 
 	var err error
-	sanitizedName := sanitizeClusterName(opts.kubeContext)
+	sanitizedName, err := sanitizeClusterName(opts.kubeContext)
+	if err != nil {
+		return err
+	}
+
 	opts.clusterName, err = ensureNoClusterNameDuplicates(ctx, sanitizedName, opts.runtimeName)
 
 	return err
 }
 
 func validateClusterName(name string) error {
-	maxDNSNameLength := 253
-	if len(name) > maxDNSNameLength {
-		return fmt.Errorf("cluster name can contain no more than 253 characters")
+	maxNameLength := 63
+	if len(name) > maxNameLength {
+		return fmt.Errorf("cluster name can contain no more than 63 characters")
 	}
 
-	match, err := regexp.MatchString("^[a-z\\d]([-a-z\\d\\.]{0,251}[a-z\\d])?$", name)
+	match, err := regexp.MatchString("^[a-z]([-a-z\\d]{0,61}[a-z\\d])?$", name)
 	if err != nil {
 		return err
 	}
 
 	if !match {
-		return fmt.Errorf("cluster name must be according to k8s resource naming rules")
+		return fmt.Errorf("cluster name must be according to k8s RFC 1035 label names rules")
 	}
 
 	return nil
 }
 
-// copied from https://github.com/argoproj/argo-cd/blob/master/applicationset/generators/cluster.go#L214
-func sanitizeClusterName(name string) string {
-	invalidDNSNameChars := regexp.MustCompile("[^-a-z0-9.]")
-	maxDNSNameLength := 253
+// partially copied from https://github.com/argoproj/argo-cd/blob/master/applicationset/generators/cluster.go#L214
+func sanitizeClusterName(name string) (string, error) {
+	nameKeeper := name
+	invalidNameChars := regexp.MustCompile("[^-a-z0-9]")
+	maxNameLength := 63
 
 	name = strings.ToLower(name)
-	name = invalidDNSNameChars.ReplaceAllString(name, "-")
+	name = invalidNameChars.ReplaceAllString(name, "-")
 	// saving space for 2 chars in case a cluster with the sanitized name already exists
-	if len(name) > (maxDNSNameLength - 2) {
-		name = name[:(maxDNSNameLength - 2)]
+	if len(name) > (maxNameLength - 2) {
+		name = name[:(maxNameLength - 2)]
 	}
 
-	return strings.Trim(name, "-.")
+	name = strings.Trim(name, "-.")
+	
+	beginsWithNum := regexp.MustCompile(`^\d+`)
+	name = beginsWithNum.ReplaceAllString(name, "")
+
+	if name == "" {
+		return "", fmt.Errorf("failed sanitizing cluster name \"%s\". please use --name flag manually", nameKeeper)
+	}
+
+	return name, nil
 }
 
 func ensureNoClusterNameDuplicates(ctx context.Context, name string, runtimeName string) (string, error) {

cmd/commands/cluster_test.go

@@ -74,18 +74,41 @@ func Test_sanitizeClusterName(t *testing.T) {
 		name string
 		args args
 		want string
+		wantErr bool
 	}{
 		{
 			name: "should return sanitized string",
 			args: args{
-				name: "^-.test!@-:cluster&*`;')test.cluster(-12_3=+::±§.",
+				name: "^-.Test!@-:cluster&*`;')test.cluster(-12_3=+::±§.",
 			},
-			want: "test----cluster------test.cluster--12-3",
+			want: "test----cluster------test-cluster--12-3",
+			wantErr: false,
+		},
+		{
+			name: "should return sanitized string",
+			args: args{
+				name: "^-.123test!@-:cluster&*`;')test.cluster(-12_3=+::±§.",
+			},
+			want: "test----cluster------test-cluster--12-3",
+			wantErr: false,
+		},
+		{
+			name: "should return error of sanitization failed",
+			args: args{
+				name: "12345",
+			},
+			want: "",
+			wantErr: true,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := sanitizeClusterName(tt.args.name); got != tt.want {
+			got, err := sanitizeClusterName(tt.args.name)
+
+			if (err != nil) != tt.wantErr {
+				t.Errorf("sanitizeClusterName() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			if got != tt.want {
 				t.Errorf("sanitizeClusterName() = %v, want %v", got, tt.want)
 			}
 		})
@@ -104,35 +127,35 @@ func Test_validateClusterName(t *testing.T) {
 		{
 			name: "name should be valid",
 			args: args{
-				name: "1test-cluster.test.cluster123z",
+				name: "test-cluster-123",
 			},
 			wantErr: false,
 		},
 		{
-			name: "name should not be valid",
+			name: "name should not be valid (contains uppercase)",
 			args: args{
-				name: ".test-cluster",
+				name: "Test-cluster",
 			},
 			wantErr: true,
 		},
 		{
-			name: "name should not be valid",
+			name: "name should not be valid (contains invalid chars)",
 			args: args{
-				name: "test-cluster.",
+				name: "test-cluster:test/cluster.123#",
 			},
 			wantErr: true,
 		},
 		{
-			name: "name should not be valid",
+			name: "name should not be valid (begins with numeric char)",
 			args: args{
-				name: "Test-cluster",
+				name: "2test-cluster",
 			},
 			wantErr: true,
 		},
 		{
-			name: "name should not be valid",
+			name: "name should not be valid (too long)",
 			args: args{
-				name: "test-cluster:test/cluster",
+				name: "this-cluster-name-is-too-long-1-this-cluster-name-is-too-long-1-this-cluster-name-is-too-long-1-123",
 			},
 			wantErr: true,
 		},

docs/releases/release_notes.md

@@ -23,7 +23,7 @@ cf version
 
 ```bash
 # download and extract the binary
-curl -L --output - https://github.com/codefresh-io/cli-v2/releases/download/v0.0.430/cf-linux-amd64.tar.gz | tar zx
+curl -L --output - https://github.com/codefresh-io/cli-v2/releases/download/v0.0.431/cf-linux-amd64.tar.gz | tar zx
 
 # move the binary to your $PATH
 mv ./cf-linux-amd64 /usr/local/bin/cf
@@ -36,7 +36,7 @@ cf version
 
 ```bash
 # download and extract the binary
-curl -L --output - https://github.com/codefresh-io/cli-v2/releases/download/v0.0.430/cf-darwin-amd64.tar.gz | tar zx
+curl -L --output - https://github.com/codefresh-io/cli-v2/releases/download/v0.0.431/cf-darwin-amd64.tar.gz | tar zx
 
 # move the binary to your $PATH
 mv ./cf-darwin-amd64 /usr/local/bin/cf

manifests/runtime.yaml

@@ -5,7 +5,7 @@ metadata:
   namespace: "{{ namespace }}"
 spec:
   defVersion: 1.0.1
-  version: 0.0.430
+  version: 0.0.431
   bootstrapSpecifier: github.com/codefresh-io/cli-v2/manifests/argo-cd
   components:
     - name: events