Repository: rancher/fleet
Pull request #1356 (2023-03-01 12:45:56)

[v0.5] ensure valid release name for agent
Backport of https://github.com/rancher/fleet/pull/1353

Refers to https://github.com/rancher/rancher/issues/40684


Patch:
diff --git a/.github/workflows/aks.yml b/.github/workflows/aks.yml
index d3c8808f..6ecffbc4 100644
--- a/.github/workflows/aks.yml
+++ b/.github/workflows/aks.yml
@@ -17,7 +17,7 @@ on:
 env:
   GOARCH: amd64
   CGO_ENABLED: 0
-  SETUP_GO_VERSION: '^1.18'
+  SETUP_GO_VERSION: '1.18.*'
   GINKGO_NODES: 1
   FLAKE_ATTEMPTS: 1
   AWS_REGION: 'us-east-2'
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 25602cc9..dda5ed59 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -11,7 +11,7 @@ on:
 env:
   GOARCH: amd64
   CGO_ENABLED: 0
-  SETUP_GO_VERSION: '^1.18'
+  SETUP_GO_VERSION: '1.18.*'
 
 jobs:
   unit-test:
diff --git a/.github/workflows/eks.yml b/.github/workflows/eks.yml
index e0a11042..1ba43ca8 100644
--- a/.github/workflows/eks.yml
+++ b/.github/workflows/eks.yml
@@ -25,7 +25,7 @@ on:
 env:
   GOARCH: amd64
   CGO_ENABLED: 0
-  SETUP_GO_VERSION: '^1.18'
+  SETUP_GO_VERSION: '1.18.*'
   GINKGO_NODES: 1
   FLAKE_ATTEMPTS: 1
   AWS_REGION: 'us-east-2'
diff --git a/.github/workflows/gke.yml b/.github/workflows/gke.yml
index 8206c7ed..9c410ccd 100644
--- a/.github/workflows/gke.yml
+++ b/.github/workflows/gke.yml
@@ -16,7 +16,7 @@ on:
 env:
   GOARCH: amd64
   CGO_ENABLED: 0
-  SETUP_GO_VERSION: '^1.18'
+  SETUP_GO_VERSION: '1.18.*'
   GINKGO_NODES: 1
   FLAKE_ATTEMPTS: 1
   GKE_ZONE: 'europe-west1-b'
diff --git a/.github/workflows/golangci-lint.yml b/.github/workflows/golangci-lint.yml
index 19eba799..fd123ab3 100644
--- a/.github/workflows/golangci-lint.yml
+++ b/.github/workflows/golangci-lint.yml
@@ -1,7 +1,7 @@
 name: golangci-lint
 
 env:
-  SETUP_GO_VERSION: '^1.18'
+  SETUP_GO_VERSION: '1.18.*'
 
 on:
   schedule:
diff --git a/.github/workflows/rancher-integration.yml b/.github/workflows/rancher-integration.yml
index b73d4927..88d669ad 100644
--- a/.github/workflows/rancher-integration.yml
+++ b/.github/workflows/rancher-integration.yml
@@ -23,7 +23,7 @@ on:
 env:
   GOARCH: amd64
   CGO_ENABLED: 0
-  SETUP_GO_VERSION: '^1.18'
+  SETUP_GO_VERSION: '1.18.*'
 
 jobs:
   rancher-integration:
diff --git a/modules/cli/apply/apply.go b/modules/cli/apply/apply.go
index bd1c2cc7..82a5bca6 100644
--- a/modules/cli/apply/apply.go
+++ b/modules/cli/apply/apply.go
@@ -9,7 +9,6 @@ import (
 	"io"
 	"os"
 	"path/filepath"
-	"regexp"
 	"strings"
 
 	"github.com/sirupsen/logrus"
@@ -18,8 +17,8 @@ import (
 	fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
 	"github.com/rancher/fleet/pkg/bundle"
 	"github.com/rancher/fleet/pkg/bundleyaml"
+	name2 "github.com/rancher/fleet/pkg/name"
 
-	name2 "github.com/rancher/wrangler/pkg/name"
 	"github.com/rancher/wrangler/pkg/yaml"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -29,11 +28,7 @@ import (
 )
 
 var (
-	disallowedChars = regexp.MustCompile("[^a-zA-Z0-9-]+")
-	helmReleaseName = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`)
-	dnsLabelSafe    = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
-	multiDash       = regexp.MustCompile("-+")
-	ErrNoResources  = errors.New("no resources found to deploy")
+	ErrNoResources = errors.New("no resources found to deploy")
 )
 
 type Options struct {
@@ -178,58 +173,19 @@ func readBundle(ctx context.Context, name, baseDir string, opts *Options) (*bund
 	})
 }
 
-// createName uses the bundle name + the path to the bundle to create a unique
-// name. The resulting name is DNS label safe (RFC1123) and complies with
-// Helm's regex for release names.
+// Dir reads a bundle and image scans from a directory and writes runtime objects to the selected output.
 //
 // name: the gitrepo name, passed to 'fleet apply' on the cli
 // basedir: the path from the walk func in Dir, []baseDirs
-func createName(name, baseDir string) string {
-	needHex := false
-
-	str := filepath.Join(name, baseDir)
-	str = strings.ReplaceAll(str, "/", "-")
-
-	// avoid collision from different case
-	if str != strings.ToLower(str) {
-		needHex = true
-	}
-
-	// avoid collision from disallowed characters
-	if disallowedChars.MatchString(str) {
-		needHex = true
-	}
-
-	if needHex {
-		// append checksum before cleaning up the string
-		str = fmt.Sprintf("%s-%s", str, name2.Hex(str, 8))
-	}
-
-	// clean up new name
-	str = strings.ToLower(str)
-	str = disallowedChars.ReplaceAllLiteralString(str, "-")
-	str = multiDash.ReplaceAllString(str, "-")
-	str = strings.TrimLeft(str, "-")
-	str = strings.TrimRight(str, "-")
-
-	// shorten name to 53 characters, the limit for helm release names
-	if helmReleaseName.MatchString(str) && dnsLabelSafe.MatchString(str) {
-		// name2.Limit will add another checksum if the name is too long
-		logrus.Debugf("shorten bundle name %v to %v\n", str, name2.Limit(str, 53))
-		return name2.Limit(str, 53)
-	}
-
-	// if the string ends up empty or otherwise invalid, fall back to just
-	// a checksum of the original input
-	logrus.Debugf("couldn't derive a valid bundle name, using checksum instead for '%s'", str)
-	return name2.Hex(filepath.Join(name, baseDir), 24)
-}
-
 func Dir(ctx context.Context, client *client.Getter, name, baseDir string, opts *Options, gitRepoBundlesMap map[string]bool) error {
 	if opts == nil {
 		opts = &Options{}
 	}
-	bundle, err := readBundle(ctx, createName(name, baseDir), baseDir, opts)
+	// the bundleID is a valid helm release name, it's used as a default if a release name is not specified in helm options
+	bundleID := filepath.Join(name, baseDir)
+	bundleID = name2.HelmReleaseName(bundleID)
+
+	bundle, err := readBundle(ctx, bundleID, baseDir, opts)
 	if err != nil {
 		return err
 	}
diff --git a/pkg/apis/fleet.cattle.io/v1alpha1/bundle.go b/pkg/apis/fleet.cattle.io/v1alpha1/bundle.go
index 8e91f765..898e3c60 100644
--- a/pkg/apis/fleet.cattle.io/v1alpha1/bundle.go
+++ b/pkg/apis/fleet.cattle.io/v1alpha1/bundle.go
@@ -31,6 +31,10 @@ var (
 	}
 )
 
+// MaxHelmReleaseNameLen is the maximum length of a Helm release name.
+// See https://github.com/helm/helm/blob/293b50c65d4d56187cd4e2f390f0ada46b4c4737/pkg/chartutil/validate_name.go#L54-L61
+const MaxHelmReleaseNameLen = 53
+
 type BundleState string
 
 // +genclient
diff --git a/pkg/controllers/cluster/import.go b/pkg/controllers/cluster/import.go
index 0263b682..4a642034 100644
--- a/pkg/controllers/cluster/import.go
+++ b/pkg/controllers/cluster/import.go
@@ -211,6 +211,7 @@ func (i *importHandler) importCluster(cluster *fleet.Cluster, status fleet.Clust
 	}
 
 	if err := connection.SmokeTestKubeClientConnection(kc); err != nil {
+		logrus.Errorf("Cluster import for '%s/%s'. Smoke test failed: %v", cluster.Namespace, cluster.Name, err)
 		return status, err
 	}
 
@@ -221,13 +222,14 @@ func (i *importHandler) importCluster(cluster *fleet.Cluster, status fleet.Clust
 	setID := helmdeployer.GetSetID(config.AgentBootstrapConfigName, "", cluster.Spec.AgentNamespace)
 	apply = apply.WithDynamicLookup().WithSetID(setID).WithNoDeleteGVK(fleetns.GVK())
 
-	token, err := i.tokens.Get(cluster.Namespace, ImportTokenPrefix+cluster.Name)
+	tokenName := name.SafeConcatName(ImportTokenPrefix + cluster.Name)
+	token, err := i.tokens.Get(cluster.Namespace, tokenName)
 	if err != nil {
 		// ignore error
-		_, _ = i.tokenClient.Create(&fleet.ClusterRegistrationToken{
+		_, err = i.tokenClient.Create(&fleet.ClusterRegistrationToken{
 			ObjectMeta: metav1.ObjectMeta{
 				Namespace: cluster.Namespace,
-				Name:      ImportTokenPrefix + cluster.Name,
+				Name:      tokenName,
 				OwnerReferences: []metav1.OwnerReference{
 					{
 						APIVersion: fleet.SchemeGroupVersion.String(),
@@ -241,6 +243,7 @@ func (i *importHandler) importCluster(cluster *fleet.Cluster, status fleet.Clust
 				TTL: &metav1.Duration{Duration: ImportTokenTTL},
 			},
 		})
+		logrus.Debugf("Failed to create ClusterRegistrationToken for cluster %s/%s: %v (requeueing)", cluster.Namespace, cluster.Name, err)
 		i.clusters.EnqueueAfter(cluster.Namespace, cluster.Name, 2*time.Second)
 		return status, nil
 	}
@@ -298,6 +301,7 @@ func (i *importHandler) importCluster(cluster *fleet.Cluster, status fleet.Clust
 	}
 
 	if err := apply.ApplyObjects(obj...); err != nil {
+		logrus.Errorf("Failed cluster import for '%s/%s'. Cannot create agent deployment", cluster.Namespace, cluster.Name)
 		return status, err
 	}
 	logrus.Infof("Deployed new agent for cluster %s/%s", cluster.Namespace, cluster.Name)
diff --git a/pkg/controllers/manageagent/manageagent.go b/pkg/controllers/manageagent/manageagent.go
index b087e638..fdefea48 100644
--- a/pkg/controllers/manageagent/manageagent.go
+++ b/pkg/controllers/manageagent/manageagent.go
@@ -125,9 +125,10 @@ func (h *handler) OnNamespace(key string, namespace *corev1.Namespace) (*corev1.
 	var objs []runtime.Object
 
 	for _, cluster := range clusters {
-		logrus.Infof("Updated agent for cluster %s/%s", cluster.Namespace, cluster.Name)
+		logrus.Infof("Update agent bundle for cluster %s/%s", cluster.Namespace, cluster.Name)
 		bundle, err := h.getAgentBundle(namespace.Name, cluster)
 		if err != nil {
+			logrus.Errorf("Failed to update agent bundle for cluster %s/%s", cluster.Namespace, cluster.Name)
 			return nil, err
 		}
 		objs = append(objs, bundle)
diff --git a/pkg/crd/crds.go b/pkg/crd/crds.go
index c5497860..3c3d6927 100644
--- a/pkg/crd/crds.go
+++ b/pkg/crd/crds.go
@@ -195,7 +195,7 @@ func releaseNameValidation() apiextv1.JSONSchemaProps {
 	return apiextv1.JSONSchemaProps{
 		Type:      "string",
 		Pattern:   `^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`,
-		MaxLength: &[]int64{53}[0],
+		MaxLength: &[]int64{fleet.MaxHelmReleaseNameLen}[0],
 		Nullable:  true,
 	}
 }
diff --git a/pkg/helmdeployer/deployer.go b/pkg/helmdeployer/deployer.go
index 6a0d104e..fedc153b 100644
--- a/pkg/helmdeployer/deployer.go
+++ b/pkg/helmdeployer/deployer.go
@@ -19,8 +19,10 @@ import (
 	fleet "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
 	"github.com/rancher/fleet/pkg/kustomize"
 	"github.com/rancher/fleet/pkg/manifest"
+	name2 "github.com/rancher/fleet/pkg/name"
 	"github.com/rancher/fleet/pkg/rawyaml"
 	"github.com/rancher/fleet/pkg/render"
+
 	"github.com/rancher/wrangler/pkg/apply"
 	corecontrollers "github.com/rancher/wrangler/pkg/generated/controllers/core/v1"
 	"github.com/rancher/wrangler/pkg/kv"
@@ -246,9 +248,9 @@ func (h *Helm) getOpts(bundleID string, options fleet.BundleDeploymentOptions) (
 	}
 
 	// releaseName has a limit of 53 in helm https://github.com/helm/helm/blob/main/pkg/action/install.go#L58
-	// apply already makes sure that the bundle.Name/bundleID is not longer
-	// than 53 characters
-	return timeout, ns, bundleID
+	// fleet apply already produces valid names, but we need to make sure
+	// that bundles from other sources are valid
+	return timeout, ns, name2.HelmReleaseName(bundleID)
 }
 
 func (h *Helm) getCfg(namespace, serviceAccountName string) (action.Configuration, error) {
diff --git a/pkg/name/name.go b/pkg/name/name.go
new file mode 100644
index 00000000..3fb5cb6a
--- /dev/null
+++ b/pkg/name/name.go
@@ -0,0 +1,87 @@
+// Package name provides functions for truncating and hashing strings and for generating valid k8s resource names.
+package name
+
+import (
+	"crypto/md5" // nolint:gosec // Non-crypto use
+	"encoding/hex"
+	"fmt"
+	"regexp"
+	"strings"
+
+	"github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
+	"github.com/sirupsen/logrus"
+)
+
+var (
+	disallowedChars = regexp.MustCompile(`[^a-zA-Z0-9-]+`)
+	helmReleaseName = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`)
+	dnsLabelSafe    = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`)
+	multiDash       = regexp.MustCompile("-+")
+)
+
+// Limit the length of a string to count characters. If the string's length is
+// greater than count, it will be truncated and a hash will be appended to the
+// end.
+// If count is too small to include the shortened hash the string is simply
+// truncated.
+func Limit(s string, count int) string {
+	if len(s) <= count {
+		return s
+	}
+
+	if count <= 6 {
+		return s[:count]
+	}
+	return fmt.Sprintf("%s-%s", s[:count-6], Hex(s, 5))
+}
+
+// Hex returns a hex-encoded hash of the string and truncates it to length.
+// Warning: truncating the 32 character hash makes collisions more likely.
+func Hex(s string, length int) string {
+	h := md5.Sum([]byte(s)) // nolint:gosec // Non-crypto use
+	d := hex.EncodeToString(h[:])
+	return d[:length]
+}
+
+// HelmReleaseName uses the provided string to create a unique name. The
+// resulting name is DNS label safe (RFC1123) and complies with Helm's regex
+// for release names.
+func HelmReleaseName(str string) string {
+	needHex := false
+	bak := str
+
+	str = strings.ReplaceAll(str, "/", "-")
+
+	// avoid collision from different case
+	if str != strings.ToLower(str) {
+		needHex = true
+	}
+
+	// avoid collision from disallowed characters
+	if disallowedChars.MatchString(str) {
+		needHex = true
+	}
+
+	if needHex {
+		// append checksum before cleaning up the string
+		str = fmt.Sprintf("%s-%s", str, Hex(str, 8))
+	}
+
+	// clean up new name
+	str = strings.ToLower(str)
+	str = disallowedChars.ReplaceAllLiteralString(str, "-")
+	str = multiDash.ReplaceAllString(str, "-")
+	str = strings.TrimLeft(str, "-")
+	str = strings.TrimRight(str, "-")
+
+	// shorten name to 53 characters, the limit for helm release names
+	if helmReleaseName.MatchString(str) && dnsLabelSafe.MatchString(str) {
+		logrus.Debugf("shorten bundle name %v to %v", str, Limit(str, v1alpha1.MaxHelmReleaseNameLen))
+		return Limit(str, v1alpha1.MaxHelmReleaseNameLen)
+	}
+
+	// if the string ends up empty or otherwise invalid, fall back to just
+	// a checksum of the original input
+	logrus.Debugf("couldn't derive a valid bundle name, using checksum instead for '%s'", str)
+	return Hex(bak, 24)
+}


Test patch:
diff --git a/.github/workflows/e2e-ci.yml b/.github/workflows/e2e-ci.yml
index 1ee04875..737e517f 100644
--- a/.github/workflows/e2e-ci.yml
+++ b/.github/workflows/e2e-ci.yml
@@ -18,7 +18,7 @@ on:
 env:
   GOARCH: amd64
   CGO_ENABLED: 0
-  SETUP_GO_VERSION: '^1.18'
+  SETUP_GO_VERSION: '1.18.*'
 
 jobs:
   e2e-fleet-test:
diff --git a/.github/workflows/e2e-multicluster-ci.yml b/.github/workflows/e2e-multicluster-ci.yml
index 3e4cb539..a89c01d7 100644
--- a/.github/workflows/e2e-multicluster-ci.yml
+++ b/.github/workflows/e2e-multicluster-ci.yml
@@ -21,7 +21,7 @@ on:
 env:
   GOARCH: amd64
   CGO_ENABLED: 0
-  SETUP_GO_VERSION: '^1.18'
+  SETUP_GO_VERSION: '1.18.*'
 
 jobs:
   e2e-fleet-mc-test:
diff --git a/pkg/name/name_test.go b/pkg/name/name_test.go
new file mode 100644
index 00000000..a0a77084
--- /dev/null
+++ b/pkg/name/name_test.go
@@ -0,0 +1,63 @@
+package name_test
+
+import (
+	"fmt"
+
+	"github.com/rancher/fleet/pkg/name"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Name", func() {
+	type test struct {
+		arg    string
+		result string
+		n      int
+	}
+
+	const (
+		str50 = "1234567890" + "1234567890" + "1234567890" + "1234567890" + "1234567890"
+		str53 = str50 + "123"
+		str63 = str50 + "1234567890" + "123"
+	)
+
+	Context("Limit", func() {
+		tests := []test{
+			{arg: "1234567", n: 5, result: "12345"},
+			{arg: "1234567", n: 6, result: "123456"},
+			{arg: "1234567", n: 7, result: "1234567"},
+			{arg: "1234567", n: 8, result: "1234567"},
+			{arg: "12345678", n: 8, result: "12345678"},
+			{arg: "12345678", n: 7, result: "1-25d55"},
+			{arg: "123456789", n: 8, result: "12-25f9e"},
+		}
+
+		It("matches expected results", func() {
+			for _, t := range tests {
+				r := name.Limit(t.arg, t.n)
+				Expect(r).To(Equal(t.result), fmt.Sprintf("%#v", t))
+			}
+		})
+	})
+
+	Context("HelmReleaseName", func() {
+		tests := []test{
+			{arg: str53, result: str53},
+			{arg: str53 + "a", result: "12345678901234567890123456789012345678901234567-fdba4"},
+			{arg: str63 + "a", result: "12345678901234567890123456789012345678901234567-eb12d"},
+			{arg: "long-name-test-shortpath-with@char", result: "long-name-test-shortpath-with-char-031bab5e"},
+			{arg: "long-name-test-shortpath-with+char", result: "long-name-test-shortpath-with-char-21c88393"},
+			{arg: "long-name-test-0.App_ ", result: "long-name-test-0-app-5bf6b3fb"},
+			{arg: "long-name-test--App_-_12.factor", result: "long-name-test-app-12-factor-0efbac37"},
+			{arg: "bundle.name.example.com", result: "bundle-name-example-com-645ef821"},
+		}
+
+		It("matches expected results", func() {
+			for _, t := range tests {
+				r := name.HelmReleaseName(t.arg)
+				Expect(r).To(Equal(t.result), fmt.Sprintf("%#v", t))
+			}
+		})
+	})
+})
diff --git a/pkg/name/suite_test.go b/pkg/name/suite_test.go
new file mode 100644
index 00000000..7db1aa7d
--- /dev/null
+++ b/pkg/name/suite_test.go
@@ -0,0 +1,13 @@
+package name_test
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestNames(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Name Suite")
+}