Add a test when switching from chart template to chartRef

The test case successfully upgrade with the same chart because version
is not computed the same way (12 digits of digest appended for
OCIRepository source).

Signed-off-by: Soule BA <bah.soule@gmail.com>

docs/spec/v2beta2/helmreleases.md

@@ -210,25 +210,27 @@ from which to fetch the Helm chart. The chart is fetched by the controller with
 information provided by `.status.artifact` of the OCIRepository.
 
 The chart version of the last release attempt is reported in `.status.lastAttemptedRevision`.
-The version is in the format <version>+<digest[0:12]>. The digest of the artifact is
-appended to the version to ensure that a change in the artifact content triggers a new release.
-The controller will automatically perform a Helm release when the OCIRepository produces a new artifact.
+The version is in the format `<version>+<digest[0:12]>`. The digest of the OCI artifact
+is appended to the version to ensure that a change in the artifact content triggers
+a new release. The controller will automatically perform a Helm upgrade when the
+OCIRepository detects a new digest in the OCI artifact stored in registry, even if
+the version inside `Chart.yaml` is unchanged.
 
 **Warning:** One of `.spec.chart` or `.spec.chartRef` must be set, but not both.
-If switching from `.spec.chart` to `.spec.chartRef`, the HelmRelease will uninstall
-the previous release before installing the new one.
+When switching from `.spec.chart` to `.spec.chartRef`, the controller will perform
+an Helm upgrade and will garbage collect the old HelmChart object.
 
 **Note:** On multi-tenant clusters, platform admins can disable cross-namespace
-references with the `--no-cross-namespace-refs=true` flag. When this flag is
-set, the HelmRelease can only refer to Sources in the same namespace as the
+references with the `--no-cross-namespace-refs=true` controller flag. When this flag is
+set, the HelmRelease can only refer to OCIRepositories in the same namespace as the
 HelmRelease object.
 
 ```yaml
----
 apiVersion: source.toolkit.fluxcd.io/v1beta2
 kind: OCIRepository
 metadata:
-  name: podinfo-ocirepo
+  name: podinfo
+  namespace: default
 spec:
   interval: 30s
   url: oci://ghcr.io/stefanprodan/charts/podinfo

internal/controller/helmrelease_controller.go

@@ -891,17 +891,23 @@ func mutateChartWithSourceRevision(chart *chart.Chart, source source.Source) (st
 		}
 		// algotithm are sha256, sha384, sha512 with the canonical being sha256
 		// So every digest starts with a sha algorithm and a colon
-		sha := strings.Split(tagD[1], ":")[1]
+		sha, err := extractDigetString(tagD[1])
+		if err != nil {
+			return "", err
+		}
 		// add the digest to the chart version to make sure mutable tags are detected
-		*ver, err = ver.SetMetadata(sha[0:12])
+		*ver, err = ver.SetMetadata(sha)
 		if err != nil {
 			return "", err
 		}
 		ociDigest = tagD[1]
 	default:
 		// default to the digest
-		sha := strings.Split(revision, ":")[1]
-		*ver, err = ver.SetMetadata(sha[0:12])
+		sha, err := extractDigetString(revision)
+		if err != nil {
+			return "", err
+		}
+		*ver, err = ver.SetMetadata(sha)
 		if err != nil {
 			return "", err
 		}
@@ -911,3 +917,16 @@ func mutateChartWithSourceRevision(chart *chart.Chart, source source.Source) (st
 	chart.Metadata.Version = ver.String()
 	return ociDigest, nil
 }
+
+func extractDigetString(revision string) (string, error) {
+	var sha string
+	if pair := strings.Split(revision, ":"); len(pair) != 2 {
+		return "", fmt.Errorf("invalid artifact revision %s", revision)
+	} else {
+		sha = pair[1]
+	}
+	if len(sha) < 12 {
+		return "", fmt.Errorf("invalid artifact revision %s", revision)
+	}
+	return sha[0:12], nil
+}

internal/controller/helmrelease_controller_test.go

@@ -1299,11 +1299,149 @@ func TestHelmReleaseReconciler_reconcileReleaseFromOCIRepositorySource(t *testin
 
 		// Verify attempted values are set.
 		g.Expect(obj.Status.LastAttemptedGeneration).To(Equal(obj.Generation))
-		dig2 := strings.Split(chartArtifact.Revision, ":")[1][0:12]
-		g.Expect(obj.Status.LastAttemptedRevision).To(Equal(chartMock.Metadata.Version + "+" + dig2))
+		dig = strings.Split(chartArtifact.Revision, ":")[1][0:12]
+		g.Expect(obj.Status.LastAttemptedRevision).To(Equal(chartMock.Metadata.Version + "+" + dig))
 		g.Expect(obj.Status.LastAttemptedConfigDigest).To(Equal("sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"))
 		g.Expect(obj.Status.LastAttemptedValuesChecksum).To(BeEmpty())
+	})
+
+	t.Run("upgrade by switching from existing HelmChat to chartRef", func(t *testing.T) {
+		g := NewWithT(t)
+
+		// Create HelmChart mock.
+		chartMock := testutil.BuildChart()
+		chartArtifact, err := testutil.SaveChartAsArtifact(chartMock, digest.SHA256, testServer.URL(), testServer.Root())
+		g.Expect(err).ToNot(HaveOccurred())
+		// copy the artifact to mutate the revision
+		ociArtifact := chartArtifact.DeepCopy()
+		ociArtifact.Revision += "@" + chartArtifact.Digest
+
+		ns, err := testEnv.CreateNamespace(context.TODO(), "mock")
+		g.Expect(err).ToNot(HaveOccurred())
+		t.Cleanup(func() {
+			_ = testEnv.Delete(context.TODO(), ns)
+		})
+
+		// hc is the HelmChart object created by the HelmRelease object.
+		hc := &sourcev1b2.HelmChart{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:       "chart",
+				Namespace:  ns.Name,
+				Generation: 1,
+			},
+			Spec: sourcev1b2.HelmChartSpec{
+				Chart:   "testdata/test-helmrepo",
+				Version: "0.1.0",
+				SourceRef: sourcev1b2.LocalHelmChartSourceReference{
+					Kind: sourcev1b2.HelmRepositoryKind,
+					Name: "test-helmrepo",
+				},
+			},
+			Status: sourcev1b2.HelmChartStatus{
+				ObservedGeneration: 1,
+				Artifact:           chartArtifact,
+				Conditions: []metav1.Condition{
+					{
+						Type:   meta.ReadyCondition,
+						Status: metav1.ConditionTrue,
+					},
+				},
+			},
+		}
+
+		// ocirepo is the chartRef object to switch to.
+		ocirepo := &sourcev1b2.OCIRepository{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:       "ocirepo",
+				Namespace:  ns.Name,
+				Generation: 1,
+			},
+			Spec: sourcev1b2.OCIRepositorySpec{
+				URL:      "oci://test-example.com",
+				Interval: metav1.Duration{Duration: 1 * time.Second},
+			},
+			Status: sourcev1b2.OCIRepositoryStatus{
+				ObservedGeneration: 1,
+				Artifact:           ociArtifact,
+				Conditions: []metav1.Condition{
+					{
+						Type:   meta.ReadyCondition,
+						Status: metav1.ConditionTrue,
+					},
+				},
+			},
+		}
 
+		// Create a test Helm release storage mock.
+		rls := testutil.BuildRelease(&helmrelease.MockReleaseOptions{
+			Name:      "release",
+			Namespace: ns.Name,
+			Version:   1,
+			Chart:     chartMock,
+			Status:    helmrelease.StatusDeployed,
+		})
+
+		obj := &v2.HelmRelease{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "release",
+				Namespace: ns.Name,
+			},
+			Spec: v2.HelmReleaseSpec{
+				ChartRef: &v2.CrossNamespaceSourceReference{
+					Kind: "OCIRepository",
+					Name: "ocirepo",
+				},
+			},
+			Status: v2.HelmReleaseStatus{
+				StorageNamespace: ns.Name,
+				History: v2.Snapshots{
+					release.ObservedToSnapshot(release.ObserveRelease(rls)),
+				},
+				HelmChart: hc.Namespace + "/" + hc.Name,
+			},
+		}
+
+		c := fake.NewClientBuilder().
+			WithScheme(NewTestScheme()).
+			WithStatusSubresource(&v2.HelmRelease{}).
+			WithObjects(hc, ocirepo, obj).
+			Build()
+
+		r := &HelmReleaseReconciler{
+			Client:           c,
+			GetClusterConfig: GetTestClusterConfig,
+			EventRecorder:    record.NewFakeRecorder(32),
+		}
+
+		// Store the Helm release mock in the test namespace.
+		getter, err := r.buildRESTClientGetter(context.TODO(), obj)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		cfg, err := action.NewConfigFactory(getter, action.WithStorage(helmdriver.SecretsDriverName, obj.Status.StorageNamespace))
+		g.Expect(err).ToNot(HaveOccurred())
+
+		store := helmstorage.Init(cfg.Driver)
+		g.Expect(store.Create(rls)).To(Succeed())
+
+		_, err = r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj)
+		g.Expect(err).ToNot(HaveOccurred())
+
+		// Verify attempted values are set.
+		g.Expect(obj.Status.LastAttemptedGeneration).To(Equal(obj.Generation))
+		dig := strings.Split(ociArtifact.Revision, ":")[1][0:12]
+		g.Expect(obj.Status.LastAttemptedRevision).To(Equal(chartMock.Metadata.Version + "+" + dig))
+		g.Expect(obj.Status.LastAttemptedConfigDigest).To(Equal("sha256:e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"))
+		g.Expect(obj.Status.LastAttemptedValuesChecksum).To(BeEmpty())
+
+		// verify upgrade succeeded
+		g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
+			*conditions.TrueCondition(meta.ReadyCondition, v2.UpgradeSucceededReason, "Helm upgrade succeeded for release %s with chart %s",
+				fmt.Sprintf("%s/%s.v%d", rls.Namespace, rls.Name, rls.Version+1), fmt.Sprintf("%s@%s", chartMock.Name(),
+					fmt.Sprintf("%s+%s", chartMock.Metadata.Version, dig))),
+			*conditions.TrueCondition(v2.ReleasedCondition, v2.UpgradeSucceededReason, "Helm upgrade succeeded for release %s with chart %s",
+				fmt.Sprintf("%s/%s.v%d", rls.Namespace, rls.Name, rls.Version+1), fmt.Sprintf("%s@%s", chartMock.Name(),
+					fmt.Sprintf("%s+%s", chartMock.Metadata.Version, dig))),
+		}))
 	})
 }