From c94eb8ec212974eb4c6a80ddc51e5d5d46dd6e21 Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Fri, 12 Jul 2024 08:56:37 +0200 Subject: [PATCH] Fix incorrect use of format strings with the `conditions` package. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `Mark…` functions in the `conditions` package accept a format string and (optional) arguments, just like `fmt.Printf` and friends. In many places, the code passed an error message as the format string, causing it to be interpreted as a format string by the `fmt` package. This leads to issues when the message contains percent signs, e.g. URL-encoded values. This PR adds a format string and shortens `err.Error()` to `err`, which yields the same output. This change is identical in principle to fluxcd/source-controller#1529. Signed-off-by: Florian Forster --- internal/controller/helmrelease_controller.go | 28 +++++++++---------- internal/reconcile/atomic_release.go | 14 +++++----- internal/reconcile/install.go | 4 +-- internal/reconcile/rollback_remediation.go | 4 +-- internal/reconcile/test.go | 4 +-- internal/reconcile/uninstall.go | 4 +-- internal/reconcile/uninstall_remediation.go | 4 +-- internal/reconcile/unlock.go | 4 +-- internal/reconcile/upgrade.go | 4 +-- 9 files changed, 35 insertions(+), 35 deletions(-) diff --git a/internal/controller/helmrelease_controller.go b/internal/controller/helmrelease_controller.go index 8b7cd38c8..c3a142a10 100644 --- a/internal/controller/helmrelease_controller.go +++ b/internal/controller/helmrelease_controller.go @@ -252,7 +252,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe if err := r.checkDependencies(ctx, obj); err != nil { msg := fmt.Sprintf("dependencies do not meet ready condition (%s): retrying in %s", err.Error(), r.requeueDependency.String()) - conditions.MarkFalse(obj, meta.ReadyCondition, v2.DependencyNotReadyReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, v2.DependencyNotReadyReason, "%s", err) r.Eventf(obj, corev1.EventTypeNormal, v2.DependencyNotReadyReason, err.Error()) log.Info(msg) @@ -272,8 +272,8 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe source, err := r.getSource(ctx, obj) if err != nil { if acl.IsAccessDenied(err) { - conditions.MarkStalled(obj, aclv1.AccessDeniedReason, err.Error()) - conditions.MarkFalse(obj, meta.ReadyCondition, aclv1.AccessDeniedReason, err.Error()) + conditions.MarkStalled(obj, aclv1.AccessDeniedReason, "%s", err) + conditions.MarkFalse(obj, meta.ReadyCondition, aclv1.AccessDeniedReason, "%s", err) conditions.Delete(obj, meta.ReconcilingCondition) r.Eventf(obj, corev1.EventTypeWarning, aclv1.AccessDeniedReason, err.Error()) @@ -284,7 +284,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe } msg := fmt.Sprintf("could not get Source object: %s", err.Error()) - conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, msg) + conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, "%s", msg) return ctrl.Result{}, err } // Remove any stale corresponding Ready=False condition with Unknown. @@ -295,7 +295,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe // Check if the source is ready. if ready, msg := isSourceReady(source); !ready { log.Info(msg) - conditions.MarkFalse(obj, meta.ReadyCondition, "SourceNotReady", msg) + conditions.MarkFalse(obj, meta.ReadyCondition, "SourceNotReady", "%s", msg) // Do not requeue immediately, when the artifact is created // the watcher should trigger a reconciliation. return jitter.JitteredRequeueInterval(ctrl.Result{RequeueAfter: obj.GetRequeueAfter()}), errWaitForChart @@ -308,7 +308,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe // Compose values based from the spec and references. values, err := chartutil.ChartValuesFromReferences(ctx, r.Client, obj.Namespace, obj.GetValues(), obj.Spec.ValuesFrom...) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, "ValuesError", err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, "ValuesError", "%s", err) r.Eventf(obj, corev1.EventTypeWarning, "ValuesError", err.Error()) return ctrl.Result{}, err } @@ -322,12 +322,12 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe if err != nil { if errors.Is(err, loader.ErrFileNotFound) { msg := fmt.Sprintf("Source not ready: artifact not found. Retrying in %s", r.requeueDependency.String()) - conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, msg) + conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, "%s", msg) log.Info(msg) return ctrl.Result{RequeueAfter: r.requeueDependency}, errWaitForDependency } - conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, fmt.Sprintf("Could not load chart: %s", err.Error())) + conditions.MarkFalse(obj, meta.ReadyCondition, v2.ArtifactFailedReason, "Could not load chart: %s", err) r.Eventf(obj, corev1.EventTypeWarning, v2.ArtifactFailedReason, err.Error()) return ctrl.Result{}, err } @@ -338,14 +338,14 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe ociDigest, err := mutateChartWithSourceRevision(loadedChart, source) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, "ChartMutateError", err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, "ChartMutateError", "%s", err) return ctrl.Result{}, err } // Build the REST client getter. getter, err := r.buildRESTClientGetter(ctx, obj) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, "RESTClientError", err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, "RESTClientError", "%s", err) return ctrl.Result{}, err } // Remove any stale corresponding Ready=False condition with Unknown. @@ -403,7 +403,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe action.WithStorageLog(action.NewDebugLog(ctrl.LoggerFrom(ctx).V(logger.TraceLevel))), ) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, "FactoryError", err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, "FactoryError", "%s", err) return ctrl.Result{}, err } // Remove any stale corresponding Ready=False condition with Unknown. @@ -491,7 +491,7 @@ func (r *HelmReleaseReconciler) reconcileReleaseDeletion(ctx context.Context, ob } conditions.MarkFalse(obj, meta.ReadyCondition, v2.UninstallFailedReason, - "failed to build REST client getter to uninstall release: %s", err.Error()) + "failed to build REST client getter to uninstall release: %s", err) return err } @@ -525,7 +525,7 @@ func (r *HelmReleaseReconciler) reconcileReleaseDeletion(ctx context.Context, ob } conditions.MarkFalse(obj, meta.ReadyCondition, v2.UninstallFailedReason, - "failed to confirm ServiceAccount '%s' can be used to uninstall release: %s", serviceAccount, err.Error()) + "failed to confirm ServiceAccount '%s' can be used to uninstall release: %s", serviceAccount, err) return err } } @@ -562,7 +562,7 @@ func (r *HelmReleaseReconciler) reconcileUninstall(ctx context.Context, getter g action.WithStorageLog(action.NewDebugLog(ctrl.LoggerFrom(ctx).V(logger.TraceLevel))), ) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, "ConfigFactoryErr", err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, "ConfigFactoryErr", "%s", err) return err } diff --git a/internal/reconcile/atomic_release.go b/internal/reconcile/atomic_release.go index a57ab8334..ec825e510 100644 --- a/internal/reconcile/atomic_release.go +++ b/internal/reconcile/atomic_release.go @@ -185,7 +185,7 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error { log.V(logger.DebugLevel).Info("determining current state of Helm release") state, err := DetermineReleaseState(ctx, r.configFactory, req) if err != nil { - conditions.MarkFalse(req.Object, meta.ReadyCondition, "StateError", fmt.Sprintf("Could not determine release state: %s", err.Error())) + conditions.MarkFalse(req.Object, meta.ReadyCondition, "StateError", "Could not determine release state: %s", err) return fmt.Errorf("cannot determine release state: %w", err) } @@ -198,7 +198,7 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error { return err } if errors.Is(err, ErrMissingRollbackTarget) { - conditions.MarkStalled(req.Object, "MissingRollbackTarget", "Failed to perform remediation: %s", err.Error()) + conditions.MarkStalled(req.Object, "MissingRollbackTarget", "Failed to perform remediation: %s", err) return err } return err @@ -231,7 +231,7 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error { ) if remediation := req.Object.GetActiveRemediation(); remediation == nil || !remediation.RetriesExhausted(req.Object) { - conditions.MarkReconciling(req.Object, meta.ProgressingWithRetryReason, conditions.GetMessage(req.Object, meta.ReadyCondition)) + conditions.MarkReconciling(req.Object, meta.ProgressingWithRetryReason, "%s", conditions.GetMessage(req.Object, meta.ReadyCondition)) return ErrMustRequeue } @@ -243,14 +243,14 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error { // This to show continuous progress, as Helm actions can be long-running. reconcilingMsg := fmt.Sprintf("Running '%s' action with timeout of %s", next.Name(), timeoutForAction(next, req.Object).String()) - conditions.MarkReconciling(req.Object, meta.ProgressingReason, reconcilingMsg) + conditions.MarkReconciling(req.Object, meta.ProgressingReason, "%s", reconcilingMsg) // If the next action is a release action, we can mark the release // as progressing in terms of readiness as well. Doing this for any // other action type is not useful, as it would potentially // overwrite more important failure state from an earlier action. if next.Type() == ReconcilerTypeRelease { - conditions.MarkUnknown(req.Object, meta.ReadyCondition, meta.ProgressingReason, reconcilingMsg) + conditions.MarkUnknown(req.Object, meta.ReadyCondition, meta.ProgressingReason, "%s", reconcilingMsg) } // Patch the object to reflect the new condition. @@ -262,7 +262,7 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error { log.Info(fmt.Sprintf("running '%s' action with timeout of %s", next.Name(), timeoutForAction(next, req.Object).String())) if err = next.Reconcile(ctx, req); err != nil { if conditions.IsReady(req.Object) { - conditions.MarkFalse(req.Object, meta.ReadyCondition, "ReconcileError", err.Error()) + conditions.MarkFalse(req.Object, meta.ReadyCondition, "ReconcileError", "%s", err) } return err } @@ -275,7 +275,7 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error { remediation := req.Object.GetActiveRemediation() if remediation == nil || !remediation.RetriesExhausted(req.Object) { - conditions.MarkReconciling(req.Object, meta.ProgressingWithRetryReason, conditions.GetMessage(req.Object, meta.ReadyCondition)) + conditions.MarkReconciling(req.Object, meta.ProgressingWithRetryReason, "%s", conditions.GetMessage(req.Object, meta.ReadyCondition)) return ErrMustRequeue } // Check if retries have exhausted after remediation for early diff --git a/internal/reconcile/install.go b/internal/reconcile/install.go index 6917bbbef..50747d8ba 100644 --- a/internal/reconcile/install.go +++ b/internal/reconcile/install.go @@ -148,7 +148,7 @@ func (r *Install) failure(req *Request, buffer *action.LogBuffer, err error) { // Mark install failure on object. req.Object.Status.Failures++ - conditions.MarkFalse(req.Object, v2.ReleasedCondition, v2.InstallFailedReason, msg) + conditions.MarkFalse(req.Object, v2.ReleasedCondition, v2.InstallFailedReason, "%s", msg) // Record warning event, this message contains more data than the // Condition summary. @@ -173,7 +173,7 @@ func (r *Install) success(req *Request) { msg := fmt.Sprintf(fmtInstallSuccess, cur.FullReleaseName(), cur.VersionedChartName()) // Mark install success on object. - conditions.MarkTrue(req.Object, v2.ReleasedCondition, v2.InstallSucceededReason, msg) + conditions.MarkTrue(req.Object, v2.ReleasedCondition, v2.InstallSucceededReason, "%s", msg) if req.Object.GetTest().Enable && !cur.HasBeenTested() { conditions.MarkUnknown(req.Object, v2.TestSuccessCondition, "AwaitingTests", fmtTestPending, cur.FullReleaseName(), cur.VersionedChartName()) diff --git a/internal/reconcile/rollback_remediation.go b/internal/reconcile/rollback_remediation.go index 9e9fe567a..ecf1a72d8 100644 --- a/internal/reconcile/rollback_remediation.go +++ b/internal/reconcile/rollback_remediation.go @@ -137,7 +137,7 @@ func (r *RollbackRemediation) failure(req *Request, prev *v2.Snapshot, buffer *a // Mark remediation failure on object. req.Object.Status.Failures++ - conditions.MarkFalse(req.Object, v2.RemediatedCondition, v2.RollbackFailedReason, msg) + conditions.MarkFalse(req.Object, v2.RemediatedCondition, v2.RollbackFailedReason, "%s", msg) // Record warning event, this message contains more data than the // Condition summary. @@ -158,7 +158,7 @@ func (r *RollbackRemediation) success(req *Request, prev *v2.Snapshot) { msg := fmt.Sprintf(fmtRollbackRemediationSuccess, prev.FullReleaseName(), prev.VersionedChartName()) // Mark remediation success on object. - conditions.MarkTrue(req.Object, v2.RemediatedCondition, v2.RollbackSucceededReason, msg) + conditions.MarkTrue(req.Object, v2.RemediatedCondition, v2.RollbackSucceededReason, "%s", msg) // Record event. r.eventRecorder.AnnotatedEventf( diff --git a/internal/reconcile/test.go b/internal/reconcile/test.go index 423e87239..db28a75e0 100644 --- a/internal/reconcile/test.go +++ b/internal/reconcile/test.go @@ -139,7 +139,7 @@ func (r *Test) failure(req *Request, err error) { // Mark test failure on object. req.Object.Status.Failures++ - conditions.MarkFalse(req.Object, v2.TestSuccessCondition, v2.TestFailedReason, msg) + conditions.MarkFalse(req.Object, v2.TestSuccessCondition, v2.TestFailedReason, "%s", msg) // Record warning event, this message contains more data than the // Condition summary. @@ -176,7 +176,7 @@ func (r *Test) success(req *Request) { msg := fmt.Sprintf(fmtTestSuccess, cur.FullReleaseName(), cur.VersionedChartName(), hookMsg) // Mark test success on object. - conditions.MarkTrue(req.Object, v2.TestSuccessCondition, v2.TestSucceededReason, msg) + conditions.MarkTrue(req.Object, v2.TestSuccessCondition, v2.TestSucceededReason, "%s", msg) // Record event. r.eventRecorder.AnnotatedEventf( diff --git a/internal/reconcile/uninstall.go b/internal/reconcile/uninstall.go index 7873019e3..5391ba72b 100644 --- a/internal/reconcile/uninstall.go +++ b/internal/reconcile/uninstall.go @@ -174,7 +174,7 @@ func (r *Uninstall) failure(req *Request, buffer *action.LogBuffer, err error) { // Mark remediation failure on object. req.Object.Status.Failures++ - conditions.MarkFalse(req.Object, v2.ReleasedCondition, v2.UninstallFailedReason, msg) + conditions.MarkFalse(req.Object, v2.ReleasedCondition, v2.UninstallFailedReason, "%s", msg) // Record warning event, this message contains more data than the // Condition summary. @@ -195,7 +195,7 @@ func (r *Uninstall) success(req *Request) { msg := fmt.Sprintf(fmtUninstallSuccess, cur.FullReleaseName(), cur.VersionedChartName()) // Mark remediation success on object. - conditions.MarkFalse(req.Object, v2.ReleasedCondition, v2.UninstallSucceededReason, msg) + conditions.MarkFalse(req.Object, v2.ReleasedCondition, v2.UninstallSucceededReason, "%s", msg) // Record warning event, this message contains more data than the // Condition summary. diff --git a/internal/reconcile/uninstall_remediation.go b/internal/reconcile/uninstall_remediation.go index 360eae418..c0a01e645 100644 --- a/internal/reconcile/uninstall_remediation.go +++ b/internal/reconcile/uninstall_remediation.go @@ -148,7 +148,7 @@ func (r *UninstallRemediation) failure(req *Request, buffer *action.LogBuffer, e // Mark uninstall failure on object. req.Object.Status.Failures++ - conditions.MarkFalse(req.Object, v2.RemediatedCondition, v2.UninstallFailedReason, msg) + conditions.MarkFalse(req.Object, v2.RemediatedCondition, v2.UninstallFailedReason, "%s", msg) // Record warning event, this message contains more data than the // Condition summary. @@ -170,7 +170,7 @@ func (r *UninstallRemediation) success(req *Request) { msg := fmt.Sprintf(fmtUninstallRemediationSuccess, cur.FullReleaseName(), cur.VersionedChartName()) // Mark remediation success on object. - conditions.MarkTrue(req.Object, v2.RemediatedCondition, v2.UninstallSucceededReason, msg) + conditions.MarkTrue(req.Object, v2.RemediatedCondition, v2.UninstallSucceededReason, "%s", msg) // Record event. r.eventRecorder.AnnotatedEventf( diff --git a/internal/reconcile/unlock.go b/internal/reconcile/unlock.go index c76f96d6e..af32724f6 100644 --- a/internal/reconcile/unlock.go +++ b/internal/reconcile/unlock.go @@ -114,7 +114,7 @@ func (r *Unlock) failure(req *Request, cur *v2.Snapshot, status helmrelease.Stat // Mark unlock failure on object. req.Object.Status.Failures++ - conditions.MarkFalse(req.Object, v2.ReleasedCondition, "PendingRelease", msg) + conditions.MarkFalse(req.Object, v2.ReleasedCondition, "PendingRelease", "%s", msg) // Record warning event. r.eventRecorder.AnnotatedEventf( @@ -133,7 +133,7 @@ func (r *Unlock) success(req *Request, cur *v2.Snapshot, status helmrelease.Stat msg := fmt.Sprintf(fmtUnlockSuccess, cur.FullReleaseName(), cur.VersionedChartName(), status.String()) // Mark unlock success on object. - conditions.MarkFalse(req.Object, v2.ReleasedCondition, "PendingRelease", msg) + conditions.MarkFalse(req.Object, v2.ReleasedCondition, "PendingRelease", "%s", msg) // Record event. r.eventRecorder.AnnotatedEventf( diff --git a/internal/reconcile/upgrade.go b/internal/reconcile/upgrade.go index 6d7949036..fba330d9e 100644 --- a/internal/reconcile/upgrade.go +++ b/internal/reconcile/upgrade.go @@ -138,7 +138,7 @@ func (r *Upgrade) failure(req *Request, buffer *action.LogBuffer, err error) { // Mark upgrade failure on object. req.Object.Status.Failures++ - conditions.MarkFalse(req.Object, v2.ReleasedCondition, v2.UpgradeFailedReason, msg) + conditions.MarkFalse(req.Object, v2.ReleasedCondition, v2.UpgradeFailedReason, "%s", msg) // Record warning event, this message contains more data than the // Condition summary. @@ -163,7 +163,7 @@ func (r *Upgrade) success(req *Request) { msg := fmt.Sprintf(fmtUpgradeSuccess, cur.FullReleaseName(), cur.VersionedChartName()) // Mark upgrade success on object. - conditions.MarkTrue(req.Object, v2.ReleasedCondition, v2.UpgradeSucceededReason, msg) + conditions.MarkTrue(req.Object, v2.ReleasedCondition, v2.UpgradeSucceededReason, "%s", msg) if req.Object.GetTest().Enable && !cur.HasBeenTested() { conditions.MarkUnknown(req.Object, v2.TestSuccessCondition, "AwaitingTests", fmtTestPending, cur.FullReleaseName(), cur.VersionedChartName())