feat: add safe release name option to avoid random suffix in Helm release names

Author: matteogastaldello

internal/composition/composition.go

@@ -65,7 +65,18 @@ const (
 
 var _ controller.ExternalClient = (*handler)(nil)
 
-func NewHandler(cfg *rest.Config, pig archive.Getter, event event.APIRecorder, pluralizer pluralizer.PluralizerInterface, chartInspectorUrl string, saName string, saNamespace string) controller.ExternalClient {
+type HandlerOptions struct {
+	Kubeconfig        *rest.Config
+	PackageInfoGetter archive.Getter
+	EventRecorder     event.APIRecorder
+	Pluralizer        pluralizer.PluralizerInterface
+	ChartInspectorUrl string
+	SaName            string
+	SaNamespace       string
+	SafeReleaseName   bool
+}
+
+func NewHandler(opts *HandlerOptions) controller.ExternalClient {
 	val, ok := os.LookupEnv(helmRegistryConfigPathEnvVar)
 	if ok {
 		helmRegistryConfigPath = val
@@ -74,13 +85,14 @@ func NewHandler(cfg *rest.Config, pig archive.Getter, event event.APIRecorder, p
 	helmRegistryConfigFile = filepath.Join(helmRegistryConfigPath, registry.CredentialsFileBasename)
 
 	return &handler{
-		kubeconfig:        cfg,
-		pluralizer:        pluralizer,
-		packageInfoGetter: pig,
-		eventRecorder:     event,
-		chartInspectorUrl: chartInspectorUrl,
-		saName:            saName,
-		saNamespace:       saNamespace,
+		kubeconfig:        opts.Kubeconfig,
+		pluralizer:        opts.Pluralizer,
+		packageInfoGetter: opts.PackageInfoGetter,
+		eventRecorder:     opts.EventRecorder,
+		chartInspectorUrl: opts.ChartInspectorUrl,
+		saName:            opts.SaName,
+		saNamespace:       opts.SaNamespace,
+		safeReleaseName:   opts.SafeReleaseName,
 	}
 }
 
@@ -93,6 +105,8 @@ type handler struct {
 	chartInspectorUrl string
 	saName            string
 	saNamespace       string
+	// Feature flag to disable random suffix in Helm release names. This is highly discouraged as it can lead to release name collisions, but it can be useful for certain complex charts that have issues with long release names.
+	safeReleaseName bool
 }
 
 func (h *handler) Observe(ctx context.Context, mg *unstructured.Unstructured) (controller.ExternalObservation, error) {
@@ -116,7 +130,7 @@ func (h *handler) Observe(ctx context.Context, mg *unstructured.Unstructured) (c
 		DynamicClient: dyn,
 	}
 
-	compositionMeta.SetReleaseName(mg, compositionMeta.CalculateReleaseName(mg))
+	compositionMeta.SetReleaseName(mg, compositionMeta.CalculateReleaseName(mg, h.safeReleaseName))
 	if _, p := compositionMeta.GetGracefullyPausedTime(mg); p && compositionMeta.IsGracefullyPaused(mg) {
 		log.Debug("Composition is gracefully paused, skipping observe.")
 		h.eventRecorder.Event(mg, event.Normal(reasonReconciliationGracefullyPaused, "Observe", "Reconciliation is paused via the gracefully paused annotation."))
@@ -354,7 +368,7 @@ func (h *handler) Create(ctx context.Context, mg *unstructured.Unstructured) err
 		return nil
 	}
 
-	compositionMeta.SetReleaseName(mg, compositionMeta.CalculateReleaseName(mg))
+	compositionMeta.SetReleaseName(mg, compositionMeta.CalculateReleaseName(mg, h.safeReleaseName))
 	mg, err = tools.Update(ctx, mg, updateOpts)
 	if err != nil {
 		return fmt.Errorf("updating cr with values: %w", err)

internal/composition/composition_test.go

@@ -189,10 +189,18 @@ func TestController(t *testing.T) {
 				return ctx
 			}
 
-			handler = NewHandler(cfg.Client().RESTConfig(), pig, *event.NewAPIRecorder(rec), pluralizer, chartInspectorMockURL, "test-sa", altNamespace)
-
-			// handler = NewHandler(cfg.Client().RESTConfig(), log, pig, *event.NewAPIRecorder(rec), pluralizer, chartInspectorUrl, "test-sa", altNamespace)
-
+			// handler = NewHandler(cfg.Client().RESTConfig(), pig, *event.NewAPIRecorder(rec), pluralizer, chartInspectorMockURL, "test-sa", altNamespace)
+
+			handler = NewHandler(&HandlerOptions{
+				Kubeconfig:        cfg.Client().RESTConfig(),
+				PackageInfoGetter: pig,
+				EventRecorder:     *event.NewAPIRecorder(rec),
+				Pluralizer:        pluralizer,
+				ChartInspectorUrl: chartInspectorMockURL,
+				SaName:            "test-sa",
+				SaNamespace:       altNamespace,
+				SafeReleaseName:   true,
+			})
 			resli, err := decoder.DecodeAllFiles(ctx, os.DirFS(filepath.Join(testdataPath, "compositiondefinitions")), "*.yaml")
 			if err != nil {
 				t.Log("Error decoding CRDs: ", err)

internal/meta/meta.go

@@ -33,15 +33,20 @@ const (
 	AnnotationKeyReconciliationGracefullyPausedTime = "krateo.io/gracefully-paused-time"
 )
 
-func CalculateReleaseName(o runtime.Object) string {
+// CalculateReleaseName generates a release name for the Helm chart based on the name and UID of the resource.
+// If safeReleaseName is false, it will not append a random suffix to the release name, which can lead to collisions but is necessary for certain complex charts that have issues with long release names.
+func CalculateReleaseName(o runtime.Object, safeReleaseName bool) string {
 	obj := o.(metav1.Object)
-	uid := obj.GetUID()
-	if uid == "" {
-		// Generate random string if UID is not set
-		return fmt.Sprintf("%s-%s", obj.GetName(), rand.SafeEncodeString(rand.String(8)))
+	if safeReleaseName {
+		uid := obj.GetUID()
+		if uid == "" {
+			// Generate random string if UID is not set
+			return fmt.Sprintf("%s-%s", obj.GetName(), rand.SafeEncodeString(rand.String(8)))
+		}
+		hashstr := rand.SafeEncodeString(string(obj.GetUID())[:8])
+		return fmt.Sprintf("%s-%s", obj.GetName(), hashstr)
 	}
-	hashstr := rand.SafeEncodeString(string(obj.GetUID())[:8])
-	return fmt.Sprintf("%s-%s", obj.GetName(), hashstr)
+	return obj.GetName()
 }
 
 func GetReleaseName(o metav1.Object) string {
@@ -55,6 +60,7 @@ func SetReleaseName(o metav1.Object, name string) {
 	if mglabels == nil {
 		mglabels = make(map[string]string)
 	}
+
 	if _, ok := mglabels[ReleaseNameLabel]; !ok {
 		mglabels[ReleaseNameLabel] = name
 	}

internal/meta/meta_test.go

@@ -1,7 +1,6 @@
 package meta
 
 import (
-	"fmt"
 	"strings"
 	"testing"
 	"time"
@@ -406,102 +405,80 @@ func TestGetGracefullyPausedTime(t *testing.T) {
 		})
 	}
 }
-func TestCalculateReleaseName_Deterministic(t *testing.T) {
-	obj := unstructured.Unstructured{}
-	obj.SetName("my-resource")
-	obj.SetGroupVersionKind(schema.GroupVersionKind{
-		Group:   "test.group",
-		Version: "v1",
-		Kind:    "MyKind",
-	})
-	obj.SetUID(types.UID("5a47edd9-c710-4b4b-b5ea-b6cdf9fc1f58"))
-
-	r1 := CalculateReleaseName(&obj)
-	r2 := CalculateReleaseName(&obj)
-	fmt.Println(r1)
-	fmt.Println(r2)
-
-	if r1 == "" {
-		t.Fatalf("CalculateReleaseName returned empty string")
-	}
-	if r1 != r2 {
-		t.Fatalf("CalculateReleaseName not deterministic: %q vs %q", r1, r2)
-	}
-	if !strings.HasPrefix(r1, "my-resource-") {
-		t.Fatalf("CalculateReleaseName result %q does not have expected prefix %q", r1, "my-resource-")
-	}
-}
-
-func TestCalculateReleaseName_DifferentGVKProducesDifferentHash(t *testing.T) {
-	name := "same-name"
-
-	obj1 := unstructured.Unstructured{}
-	obj1.SetName(name)
-	obj1.SetGroupVersionKind(schema.GroupVersionKind{
-		Group:   "group.one",
-		Version: "v1",
-		Kind:    "KindA",
-	})
-	obj1.SetUID(types.UID("5a47edd9-c710-4b4b-b5ea-b6cdf9fc1f58"))
-
-	obj2 := unstructured.Unstructured{}
-	obj2.SetName(name)
-	obj2.SetGroupVersionKind(schema.GroupVersionKind{
-		Group:   "group.two",
-		Version: "v1",
-		Kind:    "KindA",
-	})
-	obj2.SetUID(types.UID("b3d6f4e2-1c2d-4e5f-9a6b-7c8d9e0f1a2b"))
-
-	r1 := CalculateReleaseName(&obj1)
-	r2 := CalculateReleaseName(&obj2)
-
-	if r1 == r2 {
-		t.Fatalf("Expected different release names for different GVKs but got same: %q", r1)
+func TestCalculateReleaseName(t *testing.T) {
+	tests := []struct {
+		name            string
+		objName         string
+		uid             string
+		safeReleaseName bool
+		expectedPrefix  string
+		isDeterministic bool
+	}{
+		{
+			name:            "Deterministic with UID and safeReleaseName",
+			objName:         "my-resource",
+			uid:             "5a47edd9-c710-4b4b-b5ea-b6cdf9fc1f58",
+			safeReleaseName: true,
+			expectedPrefix:  "my-resource-",
+			isDeterministic: true,
+		},
+		{
+			name:            "Deterministic without safeReleaseName",
+			objName:         "simple-name",
+			uid:             "5a47edd9-c710-4b4b-b5ea-b6cdf9fc1f58",
+			safeReleaseName: false,
+			expectedPrefix:  "simple-name",
+			isDeterministic: true,
+		},
+		{
+			name:            "Non-deterministic when UID is missing and safeReleaseName is true",
+			objName:         "no-uid-resource",
+			uid:             "",
+			safeReleaseName: true,
+			expectedPrefix:  "no-uid-resource-",
+			isDeterministic: false,
+		},
 	}
-}
-
-func TestCalculateReleaseName_NameIncludedAndUniqueForDifferentNames(t *testing.T) {
-	gvk := schema.GroupVersionKind{Group: "example.io", Version: "v1", Kind: "Example"}
-	objA := unstructured.Unstructured{}
-	objA.SetName("alpha")
-	objA.SetGroupVersionKind(gvk)
-	objA.SetUID(types.UID("aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"))
 
-	objB := unstructured.Unstructured{}
-	objB.SetName("beta")
-	objB.SetGroupVersionKind(gvk)
-	objB.SetUID(types.UID("bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"))
-
-	ra := CalculateReleaseName(&objA)
-	rb := CalculateReleaseName(&objB)
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			obj := &unstructured.Unstructured{}
+			obj.SetName(tt.objName)
+			if tt.uid != "" {
+				obj.SetUID(types.UID(tt.uid))
+			}
 
-	if !strings.HasPrefix(ra, "alpha-") {
-		t.Fatalf("Release name %q does not start with expected prefix %q", ra, "alpha-")
-	}
-	if !strings.HasPrefix(rb, "beta-") {
-		t.Fatalf("Release name %q does not start with expected prefix %q", rb, "beta-")
-	}
-	if ra == rb {
-		t.Fatalf("Expected different release names for different resource names but got same: %q", ra)
-	}
-}
+			// First execution
+			r1 := CalculateReleaseName(obj, tt.safeReleaseName)
+			// Second execution to check determinism
+			r2 := CalculateReleaseName(obj, tt.safeReleaseName)
 
-func TestCalculateReleaseName_UIDNotFound(t *testing.T) {
-	obj := unstructured.Unstructured{}
-	obj.SetName("no-uid-resource")
-	obj.SetGroupVersionKind(schema.GroupVersionKind{
-		Group:   "test.group",
-		Version: "v1",
-		Kind:    "NoUIDKind",
-	})
-	// Not setting UID
+			// 1. Basic validation
+			if r1 == "" {
+				t.Fatal("CalculateReleaseName returned an empty string")
+			}
 
-	releaseName := CalculateReleaseName(&obj)
+			// 2. Prefix validation
+			if !strings.HasPrefix(r1, tt.expectedPrefix) {
+				t.Errorf("Expected prefix %q, got %q", tt.expectedPrefix, r1)
+			}
 
-	fmt.Println(releaseName)
+			// 3. Determinism validation
+			if tt.isDeterministic {
+				if r1 != r2 {
+					t.Errorf("Result should be deterministic but changed: %q vs %q", r1, r2)
+				}
+			} else {
+				// For non-deterministic cases, we expect them to be different
+				if r1 == r2 {
+					t.Errorf("Result should be random but was identical: %q", r1)
+				}
+			}
 
-	if !strings.HasPrefix(releaseName, "no-uid-resource-") {
-		t.Fatalf("CalculateReleaseName result %q does not have expected prefix %q", releaseName, "no-uid-resource-")
+			// 4. Special case: safeReleaseName = false should return EXACTLY the name
+			if !tt.safeReleaseName && r1 != tt.objName {
+				t.Errorf("Expected exact name %q, got %q", tt.objName, r1)
+			}
+		})
 	}
 }

main.go

@@ -75,6 +75,8 @@ func main() {
 		env.Int("COMPOSITION_CONTROLLER_MAX_ERROR_RETRIES", 5), "How many times to retry the processing of a resource when an error occurs before giving up and dropping the resource.")
 	metricsServerPort := flag.Int("metrics-server-port",
 		env.Int("COMPOSITION_CONTROLLER_METRICS_SERVER_PORT", 0), "The address to bind the metrics server to. If empty, metrics server is disabled.")
+	safeReleaseName := flag.Bool("safe-release-name",
+		env.Bool("COMPOSITION_CONTROLLER_SAFE_RELEASE_NAME", true), "If disabled the randmom suffix is not appended in the Helm release name. This can be useful for avoid having problems with complex helm charts. The use of this option is highly discouraged, as it can lead to release name collisions.")
 
 	flag.Usage = func() {
 		fmt.Fprintln(flag.CommandLine.Output(), "Flags:")
@@ -155,6 +157,7 @@ func main() {
 		WithValues("maxErrorRetryInterval", *maxErrorRetryInterval).
 		WithValues("maxErrorRetry", *maxErrorRetry).
 		WithValues("metricsServerPort", *metricsServerPort).
+		WithValues("safeReleaseName", *safeReleaseName).
 		Info("Starting composition dynamic controller.")
 
 	// Create a label requirement for the composition version
@@ -165,7 +168,22 @@ func main() {
 	}
 	labelselector := labels.NewSelector().Add(*labelreq)
 
-	handler := composition.NewHandler(cfg, pig, *event.NewAPIRecorder(rec), *pluralizer, *urlChartInspector, *saName, *saNamespace)
+	apiRecorder := event.NewAPIRecorder(rec)
+	if apiRecorder == nil {
+		log.Error(fmt.Errorf("creating API recorder"), "API recorder is nil, events will not be recorded.")
+		os.Exit(1)
+	}
+
+	handler := composition.NewHandler(&composition.HandlerOptions{
+		Kubeconfig:        cfg,
+		Pluralizer:        pluralizer,
+		PackageInfoGetter: pig,
+		EventRecorder:     *apiRecorder,
+		ChartInspectorUrl: *urlChartInspector,
+		SaName:            *saName,
+		SaNamespace:       *saNamespace,
+		SafeReleaseName:   *safeReleaseName,
+	})
 
 	opts := []builder.FuncOption{
 		builder.WithLogger(log),