From 25730b122d5e67c3be18760799816bdd19047b4e Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Fri, 12 Jul 2024 09:24: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 --- .../imageupdateautomation_controller.go | 16 +++++++-------- internal/controller/update_test.go | 20 +++++++++---------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/internal/controller/imageupdateautomation_controller.go b/internal/controller/imageupdateautomation_controller.go index d7ccdcea..958bdbc1 100644 --- a/internal/controller/imageupdateautomation_controller.go +++ b/internal/controller/imageupdateautomation_controller.go @@ -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 } @@ -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 } @@ -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 } @@ -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 } @@ -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 } diff --git a/internal/controller/update_test.go b/internal/controller/update_test.go index d1e370f1..6469cdd8 100644 --- a/internal/controller/update_test.go +++ b/internal/controller/update_test.go @@ -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", }, @@ -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", }, @@ -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", }, @@ -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", }, @@ -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", }, @@ -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", }, @@ -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")