Repository: rancher/fleet
Pull request #1353 (2023-02-28 15:38:15)

Ensure fleet-agent's helm release name has valid length
Refers to https://github.com/rancher/rancher/issues/40710


The cluster resource name is used when creating the bundle resource, however the helm release name is not shortened to 53 chars.

Enforcement of proper release name lengths was introduced with https://github.com/rancher/fleet/pull/964 to avoid reoccurring deletion of bundles. Later the fix was removed from the helmdeployer https://github.com/rancher/fleet/issues/1272 because `fleet apply` would produce valid bundles. However, the `fleet-agent` code does not use `fleet apply` and was no longer protected.

This PR keeps the bundle name intact and specifies the helm release name only.
We also switch away from wrangler's name package and use a "stable" limiter that [doesn't modify a string if its length is less or equal](https://github.com/rancher/wrangler/pull/279).




Patch:
diff --git a/modules/cli/apply/apply.go b/modules/cli/apply/apply.go
index 548e2210..9f0a328c 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/bundlereader"
 	"github.com/rancher/fleet/pkg/fleetyaml"
+	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) (*flee
 	})
 }
 
-// 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, scans, 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, scans, 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 7cfb9dee..9ca8ef00 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/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 499150f6..815bf013 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"
@@ -261,9 +263,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..88e58ab8
--- /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\n", 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/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")
+}