From 05c1c42f49a29f00c2885b14dce7264d2fe8a114 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 13 Dec 2023 10:34:37 +0100 Subject: [PATCH] loader: log HTTP errors to provide faster feedback Signed-off-by: Hidde Beydals --- internal/controller/helmrelease_controller.go | 17 ++---- .../controller/helmrelease_controller_test.go | 6 -- internal/loader/client.go | 58 +++++++++++++++++++ 3 files changed, 62 insertions(+), 19 deletions(-) create mode 100644 internal/loader/client.go diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 82bebd6a1..653295783 100644 --- a/internal/controller/helmrelease_controller.go +++ b/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()) diff --git a/internal/controller/helmrelease_controller_test.go b/internal/controller/helmrelease_controller_test.go index a8ad0f1c9..08bd933c1 100644 --- a/internal/controller/helmrelease_controller_test.go +++ b/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) diff --git a/internal/loader/client.go b/internal/loader/client.go new file mode 100644 index 000000000..9ca2eb845 --- /dev/null +++ b/internal/loader/client.go @@ -0,0 +1,58 @@ +/* +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 = errorLogger(ctrl.LoggerFrom(ctx)) + return httpClient +} + +// errorLogger is a wrapper around logr.Logger that implements the +// retryablehttp.LeveledLogger interface while only logging errors. +type errorLogger logr.Logger + +func (l errorLogger) Error(msg string, keysAndValues ...interface{}) { + l.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. +}