From bbf79b104107a82f84ba73a3701c196fe27f289f Mon Sep 17 00:00:00 2001
From: Hidde Beydals <hello@hidde.co>
Date: Thu, 2 Dec 2021 16:07:03 +0100
Subject: [PATCH] controllers: use short SHA in chart SemVer meta

As the full version can be used as a label value, the full SHA from the
reference takes up too much space from the 63 characters available in
total.

To mitigate against this, we now take a "short" version of the first 12
characters, which was still unique for the Linux kernel in 2019 with
875.000 commits:
http://git-scm.com/book/en/v2/Git-Tools-Revision-Selection#Short-SHA-1

This should be sufficient to safely detect all changes within the
context of operations.

Signed-off-by: Hidde Beydals <hello@hidde.co>
---
 controllers/helmchart_controller.go      | 30 +++++++++++++++++++-----
 controllers/helmchart_controller_test.go |  2 +-
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go
index ddc149f45..4a7f5c801 100644
--- a/controllers/helmchart_controller.go
+++ b/controllers/helmchart_controller.go
@@ -443,14 +443,32 @@ func (r *HelmChartReconciler) fromTarballArtifact(ctx context.Context, source so
 		opts.CachedChart = artifact.Path
 	}
 
-	// Add revision metadata to chart build
+	// Configure revision metadata for chart build if we should react to revision changes
 	if c.Spec.ReconcileStrategy == sourcev1.ReconcileStrategyRevision {
-		// Isolate the commit SHA from GitRepository type artifacts by removing the branch/ prefix.
-		splitRev := strings.Split(source.Revision, "/")
-		opts.VersionMetadata = splitRev[len(splitRev)-1]
+		rev := source.Revision
+		if c.Kind == sourcev1.GitRepositoryKind {
+			// Split the reference by the `/` delimiter which may be present,
+			// and take the last entry which contains the SHA.
+			split := strings.Split(source.Revision, "/")
+			rev = split[len(split)-1]
+		}
+		if c.Kind == sourcev1.GitRepositoryKind || c.Kind == sourcev1.BucketKind {
+			// The SemVer from the metadata is at times used in e.g. the label metadata for a resource
+			// in a chart, which has a limited length of 63 characters.
+			// To not fill most of this space with a full length SHA hex (40 characters for SHA-1, and
+			// even more for SHA-2 for a chart from a Bucket), we shorten this to the first 12
+			// characters taken from the hex.
+			// For SHA-1, this has proven to be unique in the Linux kernel with over 875.000 commits
+			// (http://git-scm.com/book/en/v2/Git-Tools-Revision-Selection#Short-SHA-1).
+			// Note that for a collision to be problematic, it would need to happen right after the
+			// previous SHA for the artifact, which is highly unlikely, if not virtually impossible.
+			// Ref: https://en.wikipedia.org/wiki/Birthday_attack
+			rev = rev[0:12]
+		}
+		opts.VersionMetadata = rev
 	}
-	// Set the VersionMetadata to the object's Generation if ValuesFiles is defined
-	// This ensures changes can be noticed by the Artifact consumer
+	// Set the VersionMetadata to the object's Generation if ValuesFiles is defined,
+	// this ensures changes can be noticed by the Artifact consumer
 	if len(opts.GetValuesFiles()) > 0 {
 		if opts.VersionMetadata != "" {
 			opts.VersionMetadata += "."
diff --git a/controllers/helmchart_controller_test.go b/controllers/helmchart_controller_test.go
index 609a70280..cb9838b15 100644
--- a/controllers/helmchart_controller_test.go
+++ b/controllers/helmchart_controller_test.go
@@ -747,7 +747,7 @@ var _ = Describe("HelmChartReconciler", func() {
 						storage.ArtifactExist(*got.Status.Artifact)
 				}, timeout, interval).Should(BeTrue())
 				Expect(got.Status.Artifact.Revision).To(ContainSubstring(updated.Status.Artifact.Revision))
-				Expect(got.Status.Artifact.Revision).To(ContainSubstring(commit.String()))
+				Expect(got.Status.Artifact.Revision).To(ContainSubstring(commit.String()[0:12]))
 			})
 
 			When("Setting valid valuesFiles attribute", func() {