Skip to content

Commit

Permalink
controller: requeue on fixed interval on chart 404
Browse files Browse the repository at this point in the history
Signed-off-by: Hidde Beydals <[email protected]>
  • Loading branch information
hiddeco committed Nov 17, 2023
1 parent 67f0eb9 commit adcc176
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 7 deletions.
9 changes: 8 additions & 1 deletion internal/controller/helmrelease_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
10 changes: 6 additions & 4 deletions internal/controller/helmrelease_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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"),
}))
})

Expand Down
7 changes: 6 additions & 1 deletion internal/loader/artifact_url.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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
Expand Down
16 changes: 15 additions & 1 deletion internal/loader/artifact_url_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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())
})
}
Expand Down

0 comments on commit adcc176

Please sign in to comment.