Fix incorrect use of format strings with the conditions package.

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 <[email protected]>
fluxcd · Jul 12, 2024 · f97e3dd · f97e3dd
1 parent 655432b
commit f97e3dd
Show file tree

Hide file tree

Showing 9 changed files with 31 additions and 31 deletions.
diff --git 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
@@ -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", "%s", err)
 					return err
 				}
 				return err
@@ -250,7 +250,7 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error {
 			// 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
 			}

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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())