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 <[email protected]>
  • Loading branch information
@octo
octo committed Jul 12, 2024
1 parent 5203091 commit 25730b1
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 18 deletions.
16 changes: 8 additions & 8 deletions internal/controller/imageupdateautomation_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ func (r *ImageUpdateAutomationReconciler) reconcile(ctx context.Context, sp *pat
policies, err := getPolicies(ctx, r.Client, obj.Namespace, obj.Spec.PolicySelector)
if err != nil {
if errors.Is(err, errParsePolicySelector) {
conditions.MarkStalled(obj, imagev1.InvalidPolicySelectorReason, err.Error())
conditions.MarkStalled(obj, imagev1.InvalidPolicySelectorReason, "%s", err)
result, retErr = ctrl.Result{}, nil
return
}
Expand Down Expand Up @@ -343,17 +343,17 @@ func (r *ImageUpdateAutomationReconciler) reconcile(ctx context.Context, sp *pat
sm, err := source.NewSourceManager(ctx, r.Client, obj, smOpts...)
if err != nil {
if acl.IsAccessDenied(err) {
conditions.MarkStalled(obj, aclapi.AccessDeniedReason, err.Error())
conditions.MarkStalled(obj, aclapi.AccessDeniedReason, "%s", err)
result, retErr = ctrl.Result{}, nil
return
}
if errors.Is(err, source.ErrInvalidSourceConfiguration) {
conditions.MarkStalled(obj, imagev1.InvalidSourceConfigReason, err.Error())
conditions.MarkStalled(obj, imagev1.InvalidSourceConfigReason, "%s", err)
result, retErr = ctrl.Result{}, nil
return
}
e := fmt.Errorf("failed configuring source manager: %w", err)
conditions.MarkFalse(obj, meta.ReadyCondition, imagev1.SourceManagerFailedReason, e.Error())
conditions.MarkFalse(obj, meta.ReadyCondition, imagev1.SourceManagerFailedReason, "%s", e)
result, retErr = ctrl.Result{}, e
return
}
Expand Down Expand Up @@ -390,7 +390,7 @@ func (r *ImageUpdateAutomationReconciler) reconcile(ctx context.Context, sp *pat
commit, err := sm.CheckoutSource(ctx, checkoutOpts...)
if err != nil {
e := fmt.Errorf("failed to checkout source: %w", err)
conditions.MarkFalse(obj, meta.ReadyCondition, imagev1.GitOperationFailedReason, e.Error())
conditions.MarkFalse(obj, meta.ReadyCondition, imagev1.GitOperationFailedReason, "%s", e)
result, retErr = ctrl.Result{}, e
return
}
Expand Down Expand Up @@ -420,12 +420,12 @@ func (r *ImageUpdateAutomationReconciler) reconcile(ctx context.Context, sp *pat
policyResult, err := policy.ApplyPolicies(ctx, sm.WorkDirectory(), obj, policies)
if err != nil {
if errors.Is(err, policy.ErrNoUpdateStrategy) || errors.Is(err, policy.ErrUnsupportedUpdateStrategy) {
conditions.MarkStalled(obj, imagev1.InvalidUpdateStrategyReason, err.Error())
conditions.MarkStalled(obj, imagev1.InvalidUpdateStrategyReason, "%s", err)
result, retErr = ctrl.Result{}, nil
return
}
e := fmt.Errorf("failed to apply policies: %w", err)
conditions.MarkFalse(obj, meta.ReadyCondition, imagev1.UpdateFailedReason, e.Error())
conditions.MarkFalse(obj, meta.ReadyCondition, imagev1.UpdateFailedReason, "%s", e)
result, retErr = ctrl.Result{}, e
return
}
Expand Down Expand Up @@ -462,7 +462,7 @@ func (r *ImageUpdateAutomationReconciler) reconcile(ctx context.Context, sp *pat
pushResult, err = sm.CommitAndPush(ctx, obj, policyResult, pushCfg...)
if err != nil {
e := fmt.Errorf("failed to update source: %w", err)
conditions.MarkFalse(obj, meta.ReadyCondition, imagev1.GitOperationFailedReason, e.Error())
conditions.MarkFalse(obj, meta.ReadyCondition, imagev1.GitOperationFailedReason, "%s", e)
result, retErr = ctrl.Result{}, e
return
}
Expand Down
20 changes: 10 additions & 10 deletions internal/controller/update_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1356,7 +1356,7 @@ func TestImageUpdateAutomationReconciler_notify(t *testing.T) {
pushResult: nil,
syncNeeded: true,
newObjBeforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, readyMessage)
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "%s", readyMessage)
},
wantEvent: "Normal Succeeded repository up-to-date",
},
Expand All @@ -1365,10 +1365,10 @@ func TestImageUpdateAutomationReconciler_notify(t *testing.T) {
pushResult: nil,
syncNeeded: false,
oldObjBeforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, readyMessage)
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "%s", readyMessage)
},
newObjBeforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, readyMessage)
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "%s", readyMessage)
},
wantEvent: "Trace Succeeded no change since last reconciliation",
},
Expand All @@ -1377,10 +1377,10 @@ func TestImageUpdateAutomationReconciler_notify(t *testing.T) {
pushResult: nil,
syncNeeded: true,
oldObjBeforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, readyMessage)
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "%s", readyMessage)
},
newObjBeforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, readyMessage)
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "%s", readyMessage)
},
wantEvent: "Trace Succeeded repository up-to-date",
},
Expand All @@ -1389,10 +1389,10 @@ func TestImageUpdateAutomationReconciler_notify(t *testing.T) {
pushResult: testPushResult,
syncNeeded: true,
oldObjBeforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, readyMessage)
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "%s", readyMessage)
},
newObjBeforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, readyMessage)
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "%s", readyMessage)
},
wantEvent: "Normal Succeeded pushed commit 'rev' to branch 'branch'\ntest commit message",
},
Expand All @@ -1404,7 +1404,7 @@ func TestImageUpdateAutomationReconciler_notify(t *testing.T) {
conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, "failed to checkout source")
},
newObjBeforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, readyMessage)
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "%s", readyMessage)
},
wantEvent: "Normal Succeeded repository up-to-date",
},
Expand All @@ -1416,7 +1416,7 @@ func TestImageUpdateAutomationReconciler_notify(t *testing.T) {
conditions.MarkFalse(obj, meta.ReadyCondition, meta.FailedReason, "failed to checkout source")
},
newObjBeforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, readyMessage)
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "%s", readyMessage)
},
wantEvent: "Normal Succeeded pushed commit 'rev' to branch 'branch'\ntest commit message",
},
Expand All @@ -1425,7 +1425,7 @@ func TestImageUpdateAutomationReconciler_notify(t *testing.T) {
pushResult: nil,
syncNeeded: true,
oldObjBeforeFunc: func(obj conditions.Setter) {
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, readyMessage)
conditions.MarkTrue(obj, meta.ReadyCondition, meta.SucceededReason, "%s", readyMessage)
},
newObjBeforeFunc: func(obj conditions.Setter) {
conditions.MarkFalse(obj, meta.ReadyCondition, imagev1.GitOperationFailedReason, "failed to checkout source")
Expand Down

0 comments on commit 25730b1

Please sign in to comment.