From adcc176f1d227a99e682667b0ef8d2afb9942908 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Fri, 17 Nov 2023 10:28:10 +0100 Subject: [PATCH] controller: requeue on fixed interval on chart 404 Signed-off-by: Hidde Beydals --- internal/controller/helmrelease_controller.go | 9 ++++++++- .../controller/helmrelease_controller_test.go | 10 ++++++---- internal/loader/artifact_url.go | 7 ++++++- internal/loader/artifact_url_test.go | 16 +++++++++++++++- 4 files changed, 35 insertions(+), 7 deletions(-) diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 914d546aa..3bef30141 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -270,7 +270,14 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe // Load chart from artifact. loadedChart, err := loader.SecureLoadChartFromURL(r.httpClient, hc.GetArtifact().URL, hc.GetArtifact().Digest) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, err.Error()) + if errors.Is(err, loader.ErrFileNotFound) { + msg := fmt.Sprintf("Chart not ready: artifact not found. Retrying in %s", r.requeueDependency) + conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, msg) + log.Info(msg) + return ctrl.Result{RequeueAfter: r.requeueDependency}, nil + } + + conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, fmt.Sprintf("Could not load chart: %s", err.Error())) return ctrl.Result{}, err } diff --git a/internal/controller/helmrelease_controller_test.go b/internal/controller/helmrelease_controller_test.go index 4a9585bc9..47f07848d 100644 --- a/internal/controller/helmrelease_controller_test.go +++ b/internal/controller/helmrelease_controller_test.go @@ -406,7 +406,8 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { WithScheme(NewTestScheme()). WithObjects(chart). Build(), - httpClient: retryablehttp.NewClient(), + httpClient: retryablehttp.NewClient(), + requeueDependency: 10 * time.Second, } obj := &v2.HelmRelease{ @@ -419,12 +420,13 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) { }, } - _, err := r.reconcileRelease(context.TODO(), nil, obj) - g.Expect(err).To(HaveOccurred()) + res, err := r.reconcileRelease(context.TODO(), nil, obj) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(res.RequeueAfter).To(Equal(r.requeueDependency)) g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{ *conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, ""), - *conditions.FalseCondition(meta.ReadyCondition, v2.ArtifactFailedReason, "failed to download chart"), + *conditions.FalseCondition(meta.ReadyCondition, v2.ArtifactFailedReason, "Chart not ready"), })) }) diff --git a/internal/loader/artifact_url.go b/internal/loader/artifact_url.go index 92ba391d8..6b76416a4 100644 --- a/internal/loader/artifact_url.go +++ b/internal/loader/artifact_url.go @@ -33,6 +33,8 @@ import ( ) var ( + // ErrFileNotFound is an error type used to signal 404 HTTP status code responses. + ErrFileNotFound = errors.New("file not found") // ErrIntegrity signals a chart loader failed to verify the integrity of // a chart, for example due to a digest mismatch. ErrIntegrity = errors.New("integrity failure") @@ -54,7 +56,10 @@ func SecureLoadChartFromURL(client *retryablehttp.Client, URL, digest string) (* return nil, err } _ = resp.Body.Close() - return nil, fmt.Errorf("failed to download chart from '%s': %s", URL, resp.Status) + if resp.StatusCode == http.StatusNotFound { + return nil, fmt.Errorf("failed to download chart from '%s': %w", URL, ErrFileNotFound) + } + return nil, fmt.Errorf("failed to download chart from '%s' (status: %s)", URL, resp.Status) } var c bytes.Buffer diff --git a/internal/loader/artifact_url_test.go b/internal/loader/artifact_url_test.go index e60795d76..20f60ac22 100644 --- a/internal/loader/artifact_url_test.go +++ b/internal/loader/artifact_url_test.go @@ -39,12 +39,17 @@ func TestSecureLoadChartFromURL(t *testing.T) { digest := digestlib.SHA256.FromBytes(b) const chartPath = "/chart.tgz" + const notFoundPath = "/not-found.tgz" server := httptest.NewServer(http.HandlerFunc(func(res http.ResponseWriter, req *http.Request) { if req.URL.Path == chartPath { res.WriteHeader(http.StatusOK) _, _ = res.Write(b) return } + if req.URL.Path == notFoundPath { + res.WriteHeader(http.StatusNotFound) + return + } res.WriteHeader(http.StatusInternalServerError) })) t.Cleanup(func() { @@ -76,11 +81,20 @@ func TestSecureLoadChartFromURL(t *testing.T) { g.Expect(got).To(BeNil()) }) - t.Run("error on server error", func(t *testing.T) { + t.Run("file not found error on 404", func(t *testing.T) { + g := NewWithT(t) + + got, err := SecureLoadChartFromURL(client, server.URL+notFoundPath, digest.String()) + g.Expect(errors.Is(err, ErrFileNotFound)).To(BeTrue()) + g.Expect(got).To(BeNil()) + }) + + t.Run("error on HTTP request failure", func(t *testing.T) { g := NewWithT(t) got, err := SecureLoadChartFromURL(client, server.URL+"/invalid.tgz", digest.String()) g.Expect(err).To(HaveOccurred()) + g.Expect(errors.Is(err, ErrFileNotFound)).To(BeFalse()) g.Expect(got).To(BeNil()) }) }