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 8, 2024
1 parent e70e5b3 commit ad38b1c
Showing 1 changed file with 28 additions and 28 deletions.
56 changes: 28 additions & 28 deletions internal/controller/kustomization_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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
Expand All @@ -245,15 +245,15 @@ 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
}

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

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

Expand All @@ -413,23 +413,23 @@ 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)
}

// 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
}

// Create an inventory from the reconciled resources.
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
}

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

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

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

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

0 comments on commit ad38b1c

Please sign in to comment.