Skip to content

Commit

Permalink
Fix incorrect use of format strings with the conditions package.
Browse files Browse the repository at this point in the history 
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 <fforster@gitlab.com>
  • Loading branch information
@octo
octo committed Jul 12, 2024
1 parent 655432b commit 02132ec
Show file tree
Hide file tree
Showing 9 changed files with 32 additions and 32 deletions.
28 changes: 14 additions & 14 deletions internal/controller/helmrelease_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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())

Expand All @@ -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.
Expand All @@ -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
Expand All @@ -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
}
Expand All @@ -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
}
Expand All @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
}

Expand Down
8 changes: 4 additions & 4 deletions internal/reconcile/atomic_release.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand All @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions internal/reconcile/install.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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())
Expand Down
4 changes: 2 additions & 2 deletions internal/reconcile/rollback_remediation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions internal/reconcile/test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions internal/reconcile/uninstall.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down
4 changes: 2 additions & 2 deletions internal/reconcile/uninstall_remediation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions internal/reconcile/unlock.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down
4 changes: 2 additions & 2 deletions internal/reconcile/upgrade.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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())
Expand Down

0 comments on commit 02132ec

Please sign in to comment.