Merge pull request #1639 from fluxcd/backport-1638-to-release/v1.7.x

[release/v1.7.x] Requeue on artifact digest mismatch

Author: matheuscscp

internal/controller/helmrelease_controller.go

@@ -336,8 +336,14 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context,
 	loadedChart, err := loader.SecureLoadChartFromURL(ctx, source.GetArtifact().URL, source.GetArtifact().Digest,
 		r.ArtifactFetchRetries, r.ArtifactFetchTimeout, r.ArtifactServerTLSConfig)
 	if err != nil {
-		if errors.Is(err, loader.ErrFileNotFound) {
-			msg := fmt.Sprintf("Source not ready: artifact not found. Retrying in %s", r.DependencyRequeueInterval.String())
+		// Requeue at the dependency interval if the artifact is not found or
+		// its digest doesn't match, as the source status may not be updated yet.
+		if errors.Is(err, loader.ErrFileNotFound) || errors.Is(err, loader.ErrDigestMismatch) {
+			cause := "artifact not found"
+			if errors.Is(err, loader.ErrDigestMismatch) {
+				cause = "artifact digest changed"
+			}
+			msg := fmt.Sprintf("Source not ready: %s. Retrying in %s", cause, r.DependencyRequeueInterval.String())
 			conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, "%s", msg)
 			log.Info(msg)
 			return ctrl.Result{RequeueAfter: r.DependencyRequeueInterval}, errWaitForDependency

internal/controller/helmrelease_controller_test.go

@@ -413,6 +413,67 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 		}))
 	})
 
+	t.Run("requeues on Helm chart digest mismatch", func(t *testing.T) {
+		g := NewWithT(t)
+
+		chartMock := testutil.BuildChart()
+		chartArtifact, err := testutil.SaveChartAsArtifact(chartMock, digest.SHA256, testServer.URL(), testServer.Root())
+		g.Expect(err).ToNot(HaveOccurred())
+		// Advertise a digest that differs from the served artifact.
+		chartArtifact.Digest = digest.SHA256.FromString("rebuilt").String()
+
+		chart := &sourcev1.HelmChart{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:       "chart",
+				Namespace:  "mock",
+				Generation: 1,
+			},
+			Status: sourcev1.HelmChartStatus{
+				ObservedGeneration: 1,
+				Artifact:           chartArtifact,
+				Conditions: []metav1.Condition{
+					{
+						Type:   meta.ReadyCondition,
+						Status: metav1.ConditionTrue,
+					},
+				},
+			},
+		}
+
+		obj := &v2.HelmRelease{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "release",
+				Namespace: "mock",
+			},
+			Status: v2.HelmReleaseStatus{
+				HelmChart: "mock/chart",
+			},
+		}
+
+		recorder := events.NewFakeRecorder(32, false)
+		r := &HelmReleaseReconciler{
+			Client: fake.NewClientBuilder().
+				WithScheme(NewTestScheme()).
+				WithStatusSubresource(&v2.HelmRelease{}).
+				WithObjects(chart, obj).
+				Build(),
+			Recorder:                  recorder,
+			DependencyRequeueInterval: 10 * time.Second,
+		}
+		r.APIReader = r.Client
+
+		res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, r.Client), obj, nil)
+		g.Expect(err).To(Equal(errWaitForDependency))
+		g.Expect(res.RequeueAfter).To(Equal(r.DependencyRequeueInterval))
+
+		g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
+			*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, ""),
+			*conditions.FalseCondition(meta.ReadyCondition, v2.ArtifactFailedReason,
+				"Source not ready: artifact digest changed. Retrying in 10s"),
+		}))
+		g.Expect(recorder.Events).To(BeEmpty())
+	})
+
 	t.Run("uninstalls HelmRelease if target has changed", func(t *testing.T) {
 		g := NewWithT(t)
 

internal/loader/artifact_url.go

@@ -47,11 +47,16 @@ const (
 var (
 	// ErrFileNotFound is an error type used to signal 404 HTTP status code responses.
 	ErrFileNotFound = errors.New("file not found")
+
+	// ErrDigestMismatch is an error type used to signal that the downloaded
+	// artifact does not match the provided digest.
+	ErrDigestMismatch = fluxfetch.ErrDigestMismatch
 )
 
 // SecureLoadChartFromURL attempts to download a Helm chart from the given URL,
 // verifies it against the given digest and loads it. The error may be of type
-// ErrFileNotFound if the artifact does not exist.
+// ErrFileNotFound if the artifact does not exist, or ErrDigestMismatch if the
+// downloaded artifact does not match the given digest.
 func SecureLoadChartFromURL(ctx context.Context, URL, digest string, retries int, timeout time.Duration, tlsConfig *tls.Config) (*chart.Chart, error) {
 	sourceLocalhost := os.Getenv(envSourceControllerLocalhost)
 	if strings.Contains(URL, "//source-watcher") {
@@ -76,6 +81,9 @@ func SecureLoadChartFromURL(ctx context.Context, URL, digest string, retries int
 		if errors.Is(err, fluxfetch.ErrFileNotFound) {
 			return nil, fmt.Errorf("failed to download chart from '%s': %w", URL, ErrFileNotFound)
 		}
+		if errors.Is(err, fluxfetch.ErrDigestMismatch) {
+			return nil, fmt.Errorf("failed to download chart from '%s': %w", URL, err)
+		}
 		return nil, err
 	}
 

internal/loader/artifact_url_test.go

@@ -104,8 +104,13 @@ func TestSecureLoadChartFromURL(t *testing.T) {
 	t.Run("error on chart data digest mismatch", func(t *testing.T) {
 		g := NewWithT(t)
 
-		got, err := SecureLoadChartFromURL(context.Background(), chartURL, digestlib.SHA256.FromString("invalid").String(), 2, 5*time.Second, nil)
+		wrongDigest := digestlib.SHA256.FromString("invalid").String()
+		got, err := SecureLoadChartFromURL(context.Background(), chartURL, wrongDigest, 2, 5*time.Second, nil)
 		g.Expect(err).To(HaveOccurred())
+		g.Expect(errors.Is(err, ErrDigestMismatch)).To(BeTrue())
+		g.Expect(errors.Is(err, ErrFileNotFound)).To(BeFalse())
+		g.Expect(err.Error()).To(ContainSubstring(chartURL))
+		g.Expect(err.Error()).To(ContainSubstring("computed digest doesn't match provided '" + wrongDigest + "'"))
 		g.Expect(got).To(BeNil())
 	})
 
@@ -114,6 +119,7 @@ func TestSecureLoadChartFromURL(t *testing.T) {
 
 		got, err := SecureLoadChartFromURL(context.Background(), server.URL+notFoundPath, digest.String(), 2, 5*time.Second, nil)
 		g.Expect(errors.Is(err, ErrFileNotFound)).To(BeTrue())
+		g.Expect(errors.Is(err, ErrDigestMismatch)).To(BeFalse())
 		g.Expect(got).To(BeNil())
 	})
 
@@ -123,6 +129,7 @@ func TestSecureLoadChartFromURL(t *testing.T) {
 		got, err := SecureLoadChartFromURL(context.Background(), server.URL+"/invalid.tgz", digest.String(), 2, 5*time.Second, nil)
 		g.Expect(err).To(HaveOccurred())
 		g.Expect(errors.Is(err, ErrFileNotFound)).To(BeFalse())
+		g.Expect(errors.Is(err, ErrDigestMismatch)).To(BeFalse())
 		g.Expect(got).To(BeNil())
 	})
 
@@ -131,6 +138,7 @@ func TestSecureLoadChartFromURL(t *testing.T) {
 
 		got, err := SecureLoadChartFromURL(context.Background(), server.URL+slowPath, digest.String(), 0, 10*time.Millisecond, nil)
 		g.Expect(err).To(HaveOccurred())
+		g.Expect(errors.Is(err, ErrDigestMismatch)).To(BeFalse())
 		g.Expect(got).To(BeNil())
 	})
 }