Merge pull request #845 from fluxcd/retry-http-log-errors

loader: log HTTP errors to provide faster feedback

Author: hiddeco

internal/controller/helmrelease_controller.go

@@ -23,7 +23,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/hashicorp/go-retryablehttp"
 	corev1 "k8s.io/api/core/v1"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -90,8 +89,8 @@ type HelmReleaseReconciler struct {
 	FieldManager          string
 	DefaultServiceAccount string
 
-	httpClient        *retryablehttp.Client
-	requeueDependency time.Duration
+	requeueDependency    time.Duration
+	artifactFetchRetries int
 }
 
 type HelmReleaseReconcilerOptions struct {
@@ -122,15 +121,7 @@ func (r *HelmReleaseReconciler) SetupWithManager(ctx context.Context, mgr ctrl.M
 	}
 
 	r.requeueDependency = opts.DependencyRequeueInterval
-
-	// Configure the retryable http client used for fetching artifacts.
-	// By default, it retries 10 times within a 3.5 minutes window.
-	httpClient := retryablehttp.NewClient()
-	httpClient.RetryWaitMin = 5 * time.Second
-	httpClient.RetryWaitMax = 30 * time.Second
-	httpClient.RetryMax = opts.HTTPRetry
-	httpClient.Logger = nil
-	r.httpClient = httpClient
+	r.artifactFetchRetries = opts.HTTPRetry
 
 	return ctrl.NewControllerManagedBy(mgr).
 		For(&v2.HelmRelease{}, builder.WithPredicates(
@@ -294,7 +285,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
 	}
 
 	// Load chart from artifact.
-	loadedChart, err := loader.SecureLoadChartFromURL(r.httpClient, hc.GetArtifact().URL, hc.GetArtifact().Digest)
+	loadedChart, err := loader.SecureLoadChartFromURL(loader.NewRetryableHTTPClient(ctx, r.artifactFetchRetries), hc.GetArtifact().URL, hc.GetArtifact().Digest)
 	if err != nil {
 		if errors.Is(err, loader.ErrFileNotFound) {
 			msg := fmt.Sprintf("Chart not ready: artifact not found. Retrying in %s", r.requeueDependency.String())

internal/controller/helmrelease_controller_test.go

@@ -23,7 +23,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/hashicorp/go-retryablehttp"
 	. "github.com/onsi/gomega"
 	"github.com/opencontainers/go-digest"
 	helmrelease "helm.sh/helm/v3/pkg/release"
@@ -433,7 +432,6 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 				WithStatusSubresource(&v2.HelmRelease{}).
 				WithObjects(chart, obj).
 				Build(),
-			httpClient:        retryablehttp.NewClient(),
 			requeueDependency: 10 * time.Second,
 		}
 
@@ -526,7 +524,6 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 			Client:           c,
 			GetClusterConfig: GetTestClusterConfig,
 			EventRecorder:    record.NewFakeRecorder(32),
-			httpClient:       retryablehttp.NewClient(),
 		}
 
 		// Store the Helm release mock in the test namespace.
@@ -607,7 +604,6 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 			Client:           c,
 			GetClusterConfig: GetTestClusterConfig,
 			EventRecorder:    record.NewFakeRecorder(32),
-			httpClient:       retryablehttp.NewClient(),
 		}
 
 		res, err := r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, c), obj)
@@ -683,7 +679,6 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 			Client:           c,
 			GetClusterConfig: GetTestClusterConfig,
 			EventRecorder:    record.NewFakeRecorder(32),
-			httpClient:       retryablehttp.NewClient(),
 		}
 
 		_, err = r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, c), obj)
@@ -755,7 +750,6 @@ func TestHelmReleaseReconciler_reconcileRelease(t *testing.T) {
 			Client:           c,
 			GetClusterConfig: GetTestClusterConfig,
 			EventRecorder:    record.NewFakeRecorder(32),
-			httpClient:       retryablehttp.NewClient(),
 		}
 
 		_, err = r.reconcileRelease(context.TODO(), patch.NewSerialPatcher(obj, c), obj)

internal/loader/client.go

@@ -0,0 +1,64 @@
+/*
+Copyright 2023 The Flux authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package loader
+
+import (
+	"context"
+	"time"
+
+	"github.com/go-logr/logr"
+	"github.com/hashicorp/go-retryablehttp"
+	ctrl "sigs.k8s.io/controller-runtime"
+)
+
+// NewRetryableHTTPClient returns a new retrying HTTP client for loading
+// artifacts. The client will retry up to the given number of times before
+// giving up. The context is used to log errors.
+func NewRetryableHTTPClient(ctx context.Context, retries int) *retryablehttp.Client {
+	httpClient := retryablehttp.NewClient()
+	httpClient.RetryWaitMin = 5 * time.Second
+	httpClient.RetryWaitMax = 30 * time.Second
+	httpClient.RetryMax = retries
+	httpClient.Logger = newLoggerForContext(ctx)
+	return httpClient
+}
+
+func newLoggerForContext(ctx context.Context) retryablehttp.LeveledLogger {
+	return &errorLogger{log: ctrl.LoggerFrom(ctx)}
+}
+
+// errorLogger is a wrapper around logr.Logger that implements the
+// retryablehttp.LeveledLogger interface while only logging errors.
+type errorLogger struct {
+	log logr.Logger
+}
+
+func (l *errorLogger) Error(msg string, keysAndValues ...interface{}) {
+	l.log.Info(msg, keysAndValues...)
+}
+
+func (l *errorLogger) Info(msg string, keysAndValues ...interface{}) {
+	// Do nothing.
+}
+
+func (l *errorLogger) Debug(msg string, keysAndValues ...interface{}) {
+	// Do nothing.
+}
+
+func (l *errorLogger) Warn(msg string, keysAndValues ...interface{}) {
+	// Do nothing.
+}