feat: add safe release name option to avoid random suffix in Helm rel… (#169)

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

* docs: add safe release name option (feature flag) to avoid random suffix in Helm release names

* docs: update Helm release name logic to enhance multi-tenancy support and character limit handling

* docs: clarify caution regarding disabling uniqueness for composition names in README

* refactor: remove unused event reasons from composition constants

Author: matteogastaldello

README.md

@@ -95,25 +95,57 @@ The **Composition Dynamic Controller (CDC)** is a specialized Kubernetes operato
 
 ## Helm Release Name Logic
 
+The logic used by the **`composition-dynamic-controller`** to determine the Helm release name has evolved to better handle multi-tenancy and character limits.
+
+---
+
 ### Prior Versions (<= 0.19.9)
 
-For versions up to 0.19.9, the **`composition-dynamic-controller`** used the following logic to determine the **Helm release name** associated with a composition resource:
+In these versions, the release name was determined by:
+
+1. The value of the **label** `krateo.io/release-name` (if set).
+2. The **Composition resource name** (as a fallback).
+
+---
+
+### Versions 0.20.0 to 0.20.2
+
+Starting with v0.20.0, the logic shifted to ensure uniqueness across different namespaces:
 
-1.  If the **label** `krateo.io/release-name` is set on the composition resource, its value is used as the Helm release name.
-2.  Otherwise, the **composition resource name** is used as the Helm release name.
+1. If the **annotation** `krateo.io/release-name` is set, its value is used.
+2. Otherwise, the name is generated as: `{composition.metadata.name}-{composition.metadata.uid[:8]}`.
+
+> [!IMPORTANT]
+> Because the UID suffix adds 9 characters (hyphen + 8-char UID) and Helm limits release names to **53 characters**, the `metadata.name` of a Composition cannot exceed **44 characters**.
 
 ---
 
-### Subsequent Versions (>= 0.20.0)
+### Versions 0.20.3+ (Configurable Logic)
+
+Starting from **v0.20.3**, the environment variable `COMPOSITION_CONTROLLER_SAFE_RELEASE_NAME` allows you to toggle between modern and legacy naming conventions.
+
+#### Default Behavior (`true`)
+
+The controller appends the **UID suffix** to ensure uniqueness across namespaces. This is the recommended setting to prevent Helm naming collisions when identical Composition names exist in different namespaces.
 
-Starting from version 0.20.0, the **`composition-dynamic-controller`** uses the following logic to determine the Helm release name associated with a composition resource:
+#### Legacy Behavior (`false`)
 
-1.  If the **annotation** `krateo.io/release-name` is set on the composition resource, its value is used as the Helm release name.
-2.  Otherwise, the release name is composed as follows: **`{composition.metadata.name}-{composition.metadata.uid[:8]}`**.
+The controller reverts to the logic used prior to v0.20.0. The release name is determined by:
+
+1. The value of the `krateo.io/release-name` **annotation** (if set).
+2. The **Composition resource name**.
+
+> [!CAUTION]
+> **Disabling this option is highly discouraged.** While it provides backward compatibility for charts with strict character length limits, it removes the uniqueness guarantee. Creating Compositions with the same name in different namespaces will cause release name collisions and failed Helm operations. It's up to the administrator to enforce a policy between users of composition name uniqueness to mitigate the risk.
+
+---
 
-**N.B.:** From this version onward, the `metadata.name` field of the composition cannot exceed **44 characters**. This is because the UID suffix adds **9 characters** to the release name (the hyphen `-` plus the 8 characters of the UID), and Helm release names cannot exceed **53 characters** in total.
+### Comparison Summary (v0.20.3+)
 
-This change was implemented to avoid conflicts when multiple resources belonging to the `composition.krateo.io` group with the same `metadata.name` are created in **different namespaces**.
+| `SAFE_RELEASE_NAME` | Source | UID Suffix | Collision Risk | Max Name Length |
+| --- | --- | --- | --- | --- |
+| **`true` (Default)** | Name + UID | Included | Negligible | 44 Characters |
+| **`false`** | Annotation/Name | Excluded | **High** | 53 Characters |
 
 ---
 
@@ -248,4 +280,5 @@ These enviroment varibles can be changed in the Deployment of the composition-dy
 | COMPOSITION_CONTROLLER_MAX_ERROR_RETRY_INTERVAL | The maximum interval between retries when an error occurs. This should be less than the half of the poll interval. |  60s |
 | COMPOSITION_CONTROLLER_MIN_ERROR_RETRY_INTERVAL | The minimum interval between retries when an error occurs. This should be less than max-error-retry-interval. | 1s |
 | COMPOSITION_CONTROLLER_MAX_ERROR_RETRIES | The maximum number of retries when an error occurs. Set to 0 to disable retries. | 5 |
-| COMPOSITION_CONTROLLER_METRICS_SERVER_PORT | The port where the metrics server will be listening. If not set, the metrics server is disabled. |  |
+| COMPOSITION_CONTROLLER_METRICS_SERVER_PORT | The port where the metrics server will be listening. If not set, the metrics server is disabled. |  |
+| COMPOSITION_CONTROLLER_SAFE_RELEASE_NAME | 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. | true |

internal/composition/composition.go

@@ -50,12 +50,9 @@ const (
 	reasonReconciliationGracefullyPaused event.Reason = "ReconciliationGracefullyPaused"
 
 	// Event reasons
-	reasonCreated   = "CompositionCreated"
-	reasonDeleted   = "CompositionDeleted"
-	reasonReady     = "CompositionReady"
-	reasonNotReady  = "CompositionNotReady"
-	reasonUpdated   = "CompositionUpdated"
-	reasonInstalled = "CompositionInstalled"
+	reasonCreated = "CompositionCreated"
+	reasonDeleted = "CompositionDeleted"
+	reasonUpdated = "CompositionUpdated"
 
 	// Environment variables
 	helmMaxHistoryEnvvar  = "HELM_MAX_HISTORY"
@@ -67,23 +64,29 @@ const (
 
 var _ controller.ExternalClient = (*handler)(nil)
 
-func NewHandler(cfg *rest.Config,
-	pig archive.Getter,
-	event event.APIRecorder,
-	pluralizer pluralizer.PluralizerInterface,
-	mapper apimeta.RESTMapper,
-	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
+	Mapper            apimeta.RESTMapper
+}
 
+func NewHandler(opts *HandlerOptions) controller.ExternalClient {
 	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,
+		mapper:            opts.Mapper,
 	}
 }
 
@@ -98,6 +101,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) {
@@ -122,8 +127,7 @@ func (h *handler) Observe(ctx context.Context, mg *unstructured.Unstructured) (c
 		DynamicClient: dyn,
 	}
 
-	compositionMeta.SetReleaseName(mg, compositionMeta.CalculateReleaseName(mg))
-	releaseName = compositionMeta.GetReleaseName(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."))
@@ -369,7 +373,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))
 	releaseName := compositionMeta.GetReleaseName(mg)
 	mg, err = tools.Update(ctx, mg, updateOpts)
 	if err != nil {

internal/composition/composition_test.go

@@ -195,10 +195,18 @@ func TestController(t *testing.T) {
 				t.Error("Creating REST mapper.", "error", err)
 				return ctx
 			}
-			handler = NewHandler(cfg.Client().RESTConfig(), pig, *event.NewAPIRecorder(rec), pluralizer, mapper, chartInspectorMockURL, "test-sa", altNamespace)
-
-			// handler = NewHandler(cfg.Client().RESTConfig(), log, pig, *event.NewAPIRecorder(rec), pluralizer, chartInspectorUrl, "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,
+				Mapper:            mapper,
+			})
 			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
 	}