CR-12777 cluster add flow improvoments (#467)

* cluster name suffix
cluster resources suffix wip

* add job name suffix

* job backoffLimit: 1

* start of unitest

* test

* unit tests

* wip

* bump

* small fix + test

* regex fix

* - changes + comment

* small fix

* bump

Author: rotem-codefresh

Makefile

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

cmd/commands/cluster.go

@@ -18,14 +18,17 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"regexp"
 	"sort"
 	"strings"
+	"time"
 
 	"github.com/codefresh-io/cli-v2/pkg/log"
 	"github.com/codefresh-io/cli-v2/pkg/store"
 	"github.com/codefresh-io/cli-v2/pkg/util"
 	kubeutil "github.com/codefresh-io/cli-v2/pkg/util/kube"
 	kustutil "github.com/codefresh-io/cli-v2/pkg/util/kust"
+	"github.com/codefresh-io/go-sdk/pkg/codefresh/model"
 
 	"github.com/Masterminds/semver/v3"
 	"github.com/argoproj-labs/argocd-autopilot/pkg/kube"
@@ -102,8 +105,7 @@ func newClusterAddCommand() *cobra.Command {
 				return err
 			}
 
-			setClusterName(&opts)
-			err = validateClusterName(opts.clusterName)
+			err = setClusterName(cmd.Context(), &opts)
 
 			return err
 		},
@@ -145,7 +147,7 @@ func runClusterAdd(ctx context.Context, opts *ClusterAddOptions) error {
 	}
 
 	csdpToken := cfConfig.GetCurrentContext().Token
-	k := createAddClusterKustomization(ingressUrl, opts.clusterName, server, csdpToken, *runtime.RuntimeVersion)
+	k, nameSuffix := createAddClusterKustomization(ingressUrl, opts.clusterName, server, csdpToken, *runtime.RuntimeVersion)
 
 	manifests, err := kustutil.BuildKustomization(k)
 	if err != nil {
@@ -162,24 +164,83 @@ func runClusterAdd(ctx context.Context, opts *ClusterAddOptions) error {
 		return fmt.Errorf("failed applying manifests to cluster: %w", err)
 	}
 
-	return kubeutil.WaitForJob(ctx, opts.kubeFactory, "kube-system", store.Get().AddClusterJobName)
+	return kubeutil.WaitForJob(ctx, opts.kubeFactory, "kube-system", fmt.Sprintf("%s%s", store.Get().AddClusterJobName, nameSuffix))
 }
 
-func setClusterName(opts *ClusterAddOptions) {
+func setClusterName(ctx context.Context, opts *ClusterAddOptions) error {
 	if opts.clusterName != "" {
-		return
+		return validateClusterName(opts.clusterName)
 	}
-	opts.clusterName = opts.kubeContext
+
+	var err error
+	sanitizedName := sanitizeClusterName(opts.kubeContext)
+	opts.clusterName, err = ensureNoClusterNameDuplicates(ctx, sanitizedName, opts.runtimeName)
+
+	return err
 }
 
 func validateClusterName(name string) error {
-	if strings.ContainsAny(name, "%`") {
-		return fmt.Errorf("cluster name '%s' is invalid. '%%' and '`' are not allowed", name)
+	maxDNSNameLength := 253
+	if len(name) > maxDNSNameLength {
+		return fmt.Errorf("cluster name can contain no more than 253 characters")
+	}
+
+	match, err := regexp.MatchString("^[a-z\\d]([-a-z\\d\\.]{0,251}[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 nil
 }
 
-func createAddClusterKustomization(ingressUrl, contextName, server, csdpToken, version string) *kusttypes.Kustomization {
+// 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
+
+	name = strings.ToLower(name)
+	name = invalidDNSNameChars.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)]
+	}
+
+	return strings.Trim(name, "-.")
+}
+
+func ensureNoClusterNameDuplicates(ctx context.Context, name string, runtimeName string) (string, error) {
+	clusters, err := cfConfig.NewClient().V2().Cluster().List(ctx, runtimeName)
+	if err != nil {
+		return "", fmt.Errorf("failed to get clusters list: %w", err)
+	}
+	
+	suffix := getSuffixToClusterName(clusters, name, name, 0)
+	if suffix != 0 {
+		return fmt.Sprintf("%s-%d", name, suffix), nil
+	}
+
+	return name, nil
+}
+
+func getSuffixToClusterName(clusters []model.Cluster, name string, tempName string, counter int) int {
+	for _, cluster := range clusters {
+		if cluster.Metadata.Name == tempName {
+			counter++
+			tempName = fmt.Sprintf("%s-%d", name, counter)
+			counter = getSuffixToClusterName(clusters, name, tempName, counter)
+			break
+		}
+	}
+
+	return counter
+}
+
+func createAddClusterKustomization(ingressUrl, contextName, server, csdpToken, version string) (*kusttypes.Kustomization, string) {
+	nameSuffix := getClusterResourcesNameSuffix()
 	resourceUrl := store.AddClusterDefURL
 	if strings.HasPrefix(resourceUrl, "http") {
 		resourceUrl = fmt.Sprintf("%s?ref=v%s", resourceUrl, version)
@@ -219,10 +280,16 @@ func createAddClusterKustomization(ingressUrl, contextName, server, csdpToken, v
 		Resources: []string{
 			resourceUrl,
 		},
+		NameSuffix: nameSuffix,
 	}
 	k.FixKustomizationPostUnmarshalling()
 	util.Die(k.FixKustomizationPreMarshalling())
-	return k
+	return k, nameSuffix
+}
+
+func getClusterResourcesNameSuffix() string {
+	now := time.Now()
+	return fmt.Sprintf("-%d", now.UnixMilli())
 }
 
 func newClusterRemoveCommand() *cobra.Command {

cmd/commands/cluster_test.go

@@ -0,0 +1,174 @@
+// Copyright 2022 The Codefresh Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package commands
+
+import (
+	"testing"
+
+	"github.com/codefresh-io/go-sdk/pkg/codefresh/model"
+)
+
+func Test_getSuffixToClusterName(t *testing.T) {
+	cluster1 := getEmptyClusterEntity()
+	cluster2 := getEmptyClusterEntity()
+	cluster3 := getEmptyClusterEntity()
+
+	cluster1.Metadata.Name = "test-cluster"
+	cluster2.Metadata.Name = "test-cluster-1"
+	cluster3.Metadata.Name = "test-cluster-2"
+
+	clusters := []model.Cluster{
+		cluster1,
+		cluster2,
+		cluster3,
+	}
+
+	type args struct {
+		clusters []model.Cluster
+		name     string
+		tempName string
+		counter  int
+	}
+	tests := []struct {
+		name string
+		args args
+		want int
+	}{
+		{
+			name: "should return 3",
+			args: args{
+				clusters: clusters,
+				name:     "test-cluster",
+				tempName: "test-cluster",
+				counter:  0,
+			},
+			want: 3,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := getSuffixToClusterName(tt.args.clusters, tt.args.name, tt.args.tempName, tt.args.counter); got != tt.want {
+				t.Errorf("getSuffixToClusterName() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func Test_sanitizeClusterName(t *testing.T) {
+	type args struct {
+		name string
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{
+		{
+			name: "should return sanitized string",
+			args: args{
+				name: "^-.test!@-:cluster&*`;')test.cluster(-12_3=+::±§.",
+			},
+			want: "test----cluster------test.cluster--12-3",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := sanitizeClusterName(tt.args.name); got != tt.want {
+				t.Errorf("sanitizeClusterName() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func Test_validateClusterName(t *testing.T) {
+	type args struct {
+		name string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		wantErr bool
+	}{
+		{
+			name: "name should be valid",
+			args: args{
+				name: "1test-cluster.test.cluster123z",
+			},
+			wantErr: false,
+		},
+		{
+			name: "name should not be valid",
+			args: args{
+				name: ".test-cluster",
+			},
+			wantErr: true,
+		},
+		{
+			name: "name should not be valid",
+			args: args{
+				name: "test-cluster.",
+			},
+			wantErr: true,
+		},
+		{
+			name: "name should not be valid",
+			args: args{
+				name: "Test-cluster",
+			},
+			wantErr: true,
+		},
+		{
+			name: "name should not be valid",
+			args: args{
+				name: "test-cluster:test/cluster",
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := validateClusterName(tt.args.name); (err != nil) != tt.wantErr {
+				t.Errorf("validateClusterName() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}
+
+func getEmptyClusterEntity() model.Cluster {
+	empty := ""
+	return model.Cluster{
+		Metadata: &model.ObjectMeta{
+			Group:       "",
+			Version:     "",
+			Kind:        "",
+			Name:        "",
+			Description: &empty,
+			Namespace:   &empty,
+			Runtime:     "",
+			Cluster:     &empty,
+			Account:     "",
+			Labels:      nil,
+			Annotations: nil,
+			LastUpdated: &empty,
+			Created:     &empty,
+			UID:         &empty,
+		},
+		Errors:       []model.Error{},
+		ReferencedBy: []model.BaseEntity{},
+		References:   []model.BaseEntity{},
+		Server:       "",
+		Namespaces:   []string{},
+	}
+}

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.416/cf-linux-amd64.tar.gz | tar zx
+curl -L --output - https://github.com/codefresh-io/cli-v2/releases/download/v0.0.417/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.416/cf-darwin-amd64.tar.gz | tar zx
+curl -L --output - https://github.com/codefresh-io/cli-v2/releases/download/v0.0.417/cf-darwin-amd64.tar.gz | tar zx
 
 # move the binary to your $PATH
 mv ./cf-darwin-amd64 /usr/local/bin/cf

manifests/add-cluster/kustomize/job.yaml

@@ -3,6 +3,7 @@ kind: Job
 metadata:
   name: csdp-add-cluster-job
 spec:
+  backoffLimit: 1
   ttlSecondsAfterFinished: 600  # stick around for 10m
   template:
     metadata:

manifests/runtime.yaml

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

pkg/store/store.go

@@ -155,7 +155,7 @@ func Get() *Store {
 }
 
 func init() {
-	s.AddClusterJobName = "csdp-add-cluster-job"
+	s.AddClusterJobName = "csdp-add-cluster-job-"
 	s.ArgoCDServerName = "argocd-server"
 	s.ArgoCDTokenKey = "token"
 	s.ArgoCDTokenSecret = "argocd-token"