From ad38b1cb84a3b9e7e2298f083a9773f2c9dee3fc Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Mon, 8 Jul 2024 08:27:13 +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 https://github.com/fluxcd/source-controller/pull/1529. Signed-off-by: Florian Forster --- .../controller/kustomization_controller.go | 56 +++++++++---------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/internal/controller/kustomization_controller.go b/internal/controller/kustomization_controller.go index 1b2aa218..20a030d7 100644 --- a/internal/controller/kustomization_controller.go +++ b/internal/controller/kustomization_controller.go @@ -223,7 +223,7 @@ func (r *KustomizationReconciler) Reconcile(ctx context.Context, req ctrl.Reques // Resolve the source reference and requeue the reconciliation if the source is not found. artifactSource, err := r.getSource(ctx, obj) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.ArtifactFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.ArtifactFailedReason, "%s", err) if apierrors.IsNotFound(err) { msg := fmt.Sprintf("Source '%s' not found", obj.Spec.SourceRef.String()) @@ -232,7 +232,7 @@ func (r *KustomizationReconciler) Reconcile(ctx context.Context, req ctrl.Reques } if acl.IsAccessDenied(err) { - conditions.MarkFalse(obj, meta.ReadyCondition, apiacl.AccessDeniedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, apiacl.AccessDeniedReason, "%s", err) log.Error(err, "Access denied to cross-namespace source") r.event(obj, "unknown", eventv1.EventSeverityError, err.Error(), nil) return ctrl.Result{RequeueAfter: obj.GetRetryInterval()}, nil @@ -245,7 +245,7 @@ func (r *KustomizationReconciler) Reconcile(ctx context.Context, req ctrl.Reques // Requeue the reconciliation if the source artifact is not found. if artifactSource.GetArtifact() == nil { msg := fmt.Sprintf("Source artifact not found, retrying in %s", r.requeueDependency.String()) - conditions.MarkFalse(obj, meta.ReadyCondition, meta.ArtifactFailedReason, msg) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.ArtifactFailedReason, "%s", msg) log.Info(msg) return ctrl.Result{RequeueAfter: r.requeueDependency}, nil } @@ -253,7 +253,7 @@ func (r *KustomizationReconciler) Reconcile(ctx context.Context, req ctrl.Reques // Check dependencies and requeue the reconciliation if the check fails. if len(obj.Spec.DependsOn) > 0 { if err := r.checkDependencies(ctx, obj, artifactSource); err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.DependencyNotReadyReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.DependencyNotReadyReason, "%s", err) msg := fmt.Sprintf("Dependencies do not meet ready condition, retrying in %s", r.requeueDependency.String()) log.Info(msg) r.event(obj, artifactSource.GetArtifact().Revision, eventv1.EventSeverityInfo, msg, nil) @@ -268,7 +268,7 @@ func (r *KustomizationReconciler) Reconcile(ctx context.Context, req ctrl.Reques // Requeue at the specified retry interval if the artifact tarball is not found. if errors.Is(reconcileErr, fetch.ErrFileNotFound) { msg := fmt.Sprintf("Source is not ready, artifact not found, retrying in %s", r.requeueDependency.String()) - conditions.MarkFalse(obj, meta.ReadyCondition, meta.ArtifactFailedReason, msg) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.ArtifactFailedReason, "%s", msg) log.Info(msg) return ctrl.Result{RequeueAfter: r.requeueDependency}, nil } @@ -299,8 +299,8 @@ func (r *KustomizationReconciler) reconcile( // Update status with the reconciliation progress. revision := src.GetArtifact().Revision progressingMsg := fmt.Sprintf("Fetching manifests for revision %s with a timeout of %s", revision, obj.GetTimeout().String()) - conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "Reconciliation in progress") - conditions.MarkReconciling(obj, meta.ProgressingReason, progressingMsg) + conditions.MarkUnknown(obj, meta.ReadyCondition, meta.ProgressingReason, "%s", "Reconciliation in progress") + conditions.MarkReconciling(obj, meta.ProgressingReason, "%s", progressingMsg) if err := r.patch(ctx, obj, patcher); err != nil { return fmt.Errorf("failed to update status: %w", err) } @@ -315,7 +315,7 @@ func (r *KustomizationReconciler) reconcile( tmpDir, err := MkdirTempAbs("", "kustomization-") if err != nil { err = fmt.Errorf("tmp dir error: %w", err) - conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.DirCreationFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, sourcev1.DirCreationFailedReason, "%s", err) return err } @@ -333,27 +333,27 @@ func (r *KustomizationReconciler) reconcile( os.Getenv("SOURCE_CONTROLLER_LOCALHOST"), ctrl.LoggerFrom(ctx), ).Fetch(src.GetArtifact().URL, src.GetArtifact().Digest, tmpDir); err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.ArtifactFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.ArtifactFailedReason, "%s", err) return err } // check build path exists dirPath, err := securejoin.SecureJoin(tmpDir, obj.Spec.Path) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.ArtifactFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.ArtifactFailedReason, "%s", err) return err } if _, err := os.Stat(dirPath); err != nil { err = fmt.Errorf("kustomization path not found: %w", err) - conditions.MarkFalse(obj, meta.ReadyCondition, meta.ArtifactFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.ArtifactFailedReason, "%s", err) return err } // Report progress and set last attempted revision in status. obj.Status.LastAttemptedRevision = revision progressingMsg = fmt.Sprintf("Building manifests for revision %s with a timeout of %s", revision, obj.GetTimeout().String()) - conditions.MarkReconciling(obj, meta.ProgressingReason, progressingMsg) + conditions.MarkReconciling(obj, meta.ProgressingReason, "%s", progressingMsg) if err := r.patch(ctx, obj, patcher); err != nil { return fmt.Errorf("failed to update status: %w", err) } @@ -373,33 +373,33 @@ func (r *KustomizationReconciler) reconcile( // Create the Kubernetes client that runs under impersonation. kubeClient, statusPoller, err := impersonation.GetClient(ctx) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.ReconciliationFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.ReconciliationFailedReason, "%s", err) return fmt.Errorf("failed to build kube client: %w", err) } // Generate kustomization.yaml if needed. k, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.BuildFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.BuildFailedReason, "%s", err) return err } err = r.generate(unstructured.Unstructured{Object: k}, tmpDir, dirPath) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.BuildFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.BuildFailedReason, "%s", err) return err } // Build the Kustomize overlay and decrypt secrets if needed. resources, err := r.build(ctx, obj, unstructured.Unstructured{Object: k}, tmpDir, dirPath) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.BuildFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.BuildFailedReason, "%s", err) return err } // Convert the build result into Kubernetes unstructured objects. objects, err := ssautil.ReadObjects(bytes.NewReader(resources)) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.BuildFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.BuildFailedReason, "%s", err) return err } @@ -413,7 +413,7 @@ func (r *KustomizationReconciler) reconcile( // Update status with the reconciliation progress. progressingMsg = fmt.Sprintf("Detecting drift for revision %s with a timeout of %s", revision, obj.GetTimeout().String()) - conditions.MarkReconciling(obj, meta.ProgressingReason, progressingMsg) + conditions.MarkReconciling(obj, meta.ProgressingReason, "%s", progressingMsg) if err := r.patch(ctx, obj, patcher); err != nil { return fmt.Errorf("failed to update status: %w", err) } @@ -421,7 +421,7 @@ func (r *KustomizationReconciler) reconcile( // Validate and apply resources in stages. drifted, changeSet, err := r.apply(ctx, resourceManager, obj, revision, objects) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.ReconciliationFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.ReconciliationFailedReason, "%s", err) return err } @@ -429,7 +429,7 @@ func (r *KustomizationReconciler) reconcile( newInventory := inventory.New() err = inventory.AddChangeSet(newInventory, changeSet) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.ReconciliationFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.ReconciliationFailedReason, "%s", err) return err } @@ -439,13 +439,13 @@ func (r *KustomizationReconciler) reconcile( // Detect stale resources which are subject to garbage collection. staleObjects, err := inventory.Diff(oldInventory, newInventory) if err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.ReconciliationFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.ReconciliationFailedReason, "%s", err) return err } // Run garbage collection for stale resources that do not have pruning disabled. if _, err := r.prune(ctx, resourceManager, obj, revision, staleObjects); err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.PruneFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.PruneFailedReason, "%s", err) return err } @@ -459,7 +459,7 @@ func (r *KustomizationReconciler) reconcile( isNewRevision, drifted, changeSet.ToObjMetadataSet()); err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.HealthCheckFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.HealthCheckFailedReason, "%s", err) return err } @@ -890,8 +890,8 @@ func (r *KustomizationReconciler) checkHealth(ctx context.Context, // Update status with the reconciliation progress. message := fmt.Sprintf("Running health checks for revision %s with a timeout of %s", revision, obj.GetTimeout().String()) - conditions.MarkReconciling(obj, meta.ProgressingReason, message) - conditions.MarkUnknown(obj, meta.HealthyCondition, meta.ProgressingReason, message) + conditions.MarkReconciling(obj, meta.ProgressingReason, "%s", message) + conditions.MarkUnknown(obj, meta.HealthyCondition, meta.ProgressingReason, "%s", message) if err := r.patch(ctx, obj, patcher); err != nil { return fmt.Errorf("unable to update the healthy status to progressing: %w", err) } @@ -902,8 +902,8 @@ func (r *KustomizationReconciler) checkHealth(ctx context.Context, Timeout: obj.GetTimeout(), FailFast: r.FailFast, }); err != nil { - conditions.MarkFalse(obj, meta.ReadyCondition, meta.HealthCheckFailedReason, err.Error()) - conditions.MarkFalse(obj, meta.HealthyCondition, meta.HealthCheckFailedReason, err.Error()) + conditions.MarkFalse(obj, meta.ReadyCondition, meta.HealthCheckFailedReason, "%s", err) + conditions.MarkFalse(obj, meta.HealthyCondition, meta.HealthCheckFailedReason, "%s", err) return fmt.Errorf("health check failed after %s: %w", time.Since(checkStart).String(), err) } @@ -913,7 +913,7 @@ func (r *KustomizationReconciler) checkHealth(ctx context.Context, r.event(obj, revision, eventv1.EventSeverityInfo, msg, nil) } - conditions.MarkTrue(obj, meta.HealthyCondition, meta.SucceededReason, msg) + conditions.MarkTrue(obj, meta.HealthyCondition, meta.SucceededReason, "%s", msg) if err := r.patch(ctx, obj, patcher); err != nil { return fmt.Errorf("unable to update the healthy status to progressing: %w", err) }