Use separate state type for exchanging data between reconcile steps

Author: pedjak

internal/operator-controller/controllers/boxcutter_reconcile_steps.go

@@ -81,15 +81,15 @@ func (d *BoxcutterRevisionStatesGetter) GetRevisionStates(ctx context.Context, e
 }
 
 func MigrateStorage(m StorageMigrator) ReconcileStepFunc {
-	return func(ctx context.Context, ext *ocv1.ClusterExtension) (context.Context, *ctrl.Result, error) {
+	return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
 		objLbls := map[string]string{
 			labels.OwnerKindKey: ocv1.ClusterExtensionKind,
 			labels.OwnerNameKey: ext.GetName(),
 		}
 
 		if err := m.Migrate(ctx, ext, objLbls); err != nil {
-			return ctx, nil, fmt.Errorf("migrating storage: %w", err)
+			return nil, fmt.Errorf("migrating storage: %w", err)
 		}
-		return ctx, nil, nil
+		return nil, nil
 	}
 }

internal/operator-controller/controllers/clusterextension_controller.go

@@ -58,11 +58,17 @@ const (
 	ClusterExtensionCleanupContentManagerCacheFinalizer = "olm.operatorframework.io/cleanup-contentmanager-cache"
 )
 
+type reconcileState struct {
+	revisionStates           *RevisionStates
+	resolvedRevisionMetadata *RevisionMetadata
+	imageFS                  fs.FS
+}
+
 // ReconcileStepFunc represents a single step in the ClusterExtension reconciliation process.
-// It takes a context and ClusterExtension object as input and returns:
-// - A potentially modified context for the next step
+// It takes a context, state and ClusterExtension object as input and returns:
 // - Any error that occurred during reconciliation, which will be returned to the caller
-type ReconcileStepFunc func(context.Context, *ocv1.ClusterExtension) (context.Context, *ctrl.Result, error)
+// - A ctrl.Result that indicates whether reconciliation should complete immediately or be retried later
+type ReconcileStepFunc func(context.Context, *reconcileState, *ocv1.ClusterExtension) (*ctrl.Result, error)
 
 // ReconcileSteps is an ordered collection of reconciliation steps that are executed sequentially.
 // Each step receives the output context from the previous step, allowing data to flow through the pipeline.
@@ -77,18 +83,15 @@ type ReconcileSteps []ReconcileStepFunc
 func (steps *ReconcileSteps) Reconcile(ctx context.Context, ext *ocv1.ClusterExtension) (ctrl.Result, error) {
 	var res *ctrl.Result
 	var err error
-	nextStepCtx := ctx
+	s := &reconcileState{}
 	for _, step := range *steps {
-		nextStepCtx, res, err = step(nextStepCtx, ext)
+		res, err = step(ctx, s, ext)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
 		if res != nil {
 			return *res, nil
 		}
-		if nextStepCtx == nil {
-			return ctrl.Result{}, nil
-		}
 	}
 	return ctrl.Result{}, nil
 }

internal/operator-controller/controllers/clusterextension_reconcile_steps.go

@@ -19,8 +19,6 @@ package controllers
 import (
 	"context"
 	"errors"
-	"fmt"
-	"io/fs"
 
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -36,55 +34,35 @@ import (
 	imageutil "github.com/operator-framework/operator-controller/internal/shared/util/image"
 )
 
-type revisionStatesKey struct{}
-type resolvedRevisionMetadataKey struct{}
-type imageFSKey struct{}
-
-func valueFromContext[T any](ctx context.Context, key any) (*T, error) {
-	v := ctx.Value(key)
-	if v == nil {
-		return nil, fmt.Errorf("value not found in context under key %v", key)
-	}
-	ptv, ok := v.(*T)
-	if !ok {
-		tv, ok := v.(T)
-		if !ok {
-			return nil, fmt.Errorf("value found in context under key %v is not of type %T or pointer to it", key, v)
-		}
-		return &tv, nil
-	}
-	return ptv, nil
-}
-
 func HandleFinalizers(f finalizer.Finalizer) ReconcileStepFunc {
-	return func(ctx context.Context, ext *ocv1.ClusterExtension) (context.Context, *ctrl.Result, error) {
+	return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
 		l := log.FromContext(ctx)
 
 		l.Info("handling finalizers")
 		finalizeResult, err := f.Finalize(ctx, ext)
 		if err != nil {
 			setStatusProgressing(ext, err)
-			return ctx, nil, err
+			return nil, err
 		}
 		if finalizeResult.Updated || finalizeResult.StatusUpdated {
 			// On create: make sure the finalizer is applied before we do anything
 			// On delete: make sure we do nothing after the finalizer is removed
-			return ctx, &ctrl.Result{}, nil
+			return &ctrl.Result{}, nil
 		}
 
 		if ext.GetDeletionTimestamp() != nil {
 			// If we've gotten here, that means the cluster extension is being deleted, we've handled all of
 			// _our_ finalizers (above), but the cluster extension is still present in the cluster, likely
 			// because there are _other_ finalizers that other controllers need to handle, (e.g. the orphan
 			// deletion finalizer).
-			return nil, nil, nil
+			return &ctrl.Result{}, nil
 		}
-		return ctx, nil, nil
+		return nil, nil
 	}
 }
 
 func RetrieveRevisionStates(r RevisionStatesGetter) ReconcileStepFunc {
-	return func(ctx context.Context, ext *ocv1.ClusterExtension) (context.Context, *ctrl.Result, error) {
+	return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
 		l := log.FromContext(ctx)
 		l.Info("getting installed bundle")
 		revisionStates, err := r.GetRevisionStates(ctx, ext)
@@ -94,37 +72,34 @@ func RetrieveRevisionStates(r RevisionStatesGetter) ReconcileStepFunc {
 			if errors.As(err, &saerr) {
 				setInstalledStatusConditionUnknown(ext, saerr.Error())
 				setStatusProgressing(ext, errors.New("installation cannot proceed due to missing ServiceAccount"))
-				return ctx, nil, err
+				return nil, err
 			}
 			setInstalledStatusConditionUnknown(ext, err.Error())
 			setStatusProgressing(ext, errors.New("retrying to get installed bundle"))
-			return ctx, nil, err
+			return nil, err
 		}
-		return context.WithValue(ctx, revisionStatesKey{}, revisionStates), nil, nil
+		state.revisionStates = revisionStates
+		return nil, nil
 	}
 }
 
 func RetrieveRevisionMetadata(r resolve.Resolver) ReconcileStepFunc {
-	return func(ctx context.Context, ext *ocv1.ClusterExtension) (context.Context, *ctrl.Result, error) {
+	return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
 		l := log.FromContext(ctx)
-		revisionStates, err := valueFromContext[RevisionStates](ctx, revisionStatesKey{})
-		if err != nil {
-			return ctx, nil, err
-		}
 		var resolvedRevisionMetadata *RevisionMetadata
-		if len(revisionStates.RollingOut) == 0 {
+		if len(state.revisionStates.RollingOut) == 0 {
 			l.Info("resolving bundle")
 			var bm *ocv1.BundleMetadata
-			if revisionStates.Installed != nil {
-				bm = &revisionStates.Installed.BundleMetadata
+			if state.revisionStates.Installed != nil {
+				bm = &state.revisionStates.Installed.BundleMetadata
 			}
 			resolvedBundle, resolvedBundleVersion, resolvedDeprecation, err := r.Resolve(ctx, ext, bm)
 			if err != nil {
 				// Note: We don't distinguish between resolution-specific errors and generic errors
 				setStatusProgressing(ext, err)
-				setInstalledStatusFromRevisionStates(ext, revisionStates)
+				setInstalledStatusFromRevisionStates(ext, state.revisionStates)
 				ensureAllConditionsWithReason(ext, ocv1.ReasonFailed, err.Error())
-				return ctx, nil, err
+				return nil, err
 			}
 
 			// set deprecation status after _successful_ resolution
@@ -148,54 +123,39 @@ func RetrieveRevisionMetadata(r resolve.Resolver) ReconcileStepFunc {
 				BundleMetadata: bundleutil.MetadataFor(resolvedBundle.Name, *resolvedBundleVersion),
 			}
 		} else {
-			resolvedRevisionMetadata = revisionStates.RollingOut[0]
+			resolvedRevisionMetadata = state.revisionStates.RollingOut[0]
 		}
-		return context.WithValue(ctx, resolvedRevisionMetadataKey{}, resolvedRevisionMetadata), nil, nil
+		state.resolvedRevisionMetadata = resolvedRevisionMetadata
+		return nil, nil
 	}
 }
 
 func UnpackBundle(i imageutil.Puller, cache imageutil.Cache) ReconcileStepFunc {
-	return func(ctx context.Context, ext *ocv1.ClusterExtension) (context.Context, *ctrl.Result, error) {
+	return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
 		l := log.FromContext(ctx)
 		l.Info("unpacking resolved bundle")
-		resolvedRevisionMetadata, err := valueFromContext[RevisionMetadata](ctx, resolvedRevisionMetadataKey{})
+		imageFS, _, _, err := i.Pull(ctx, ext.GetName(), state.resolvedRevisionMetadata.Image, cache)
 		if err != nil {
-			return ctx, nil, err
-		}
-		imageFS, _, _, err := i.Pull(ctx, ext.GetName(), resolvedRevisionMetadata.Image, cache)
-		if err != nil {
-			revisionStates := ctx.Value(revisionStatesKey{}).(*RevisionStates)
 			// Wrap the error passed to this with the resolution information until we have successfully
 			// installed since we intend for the progressing condition to replace the resolved condition
 			// and will be removing the .status.resolution field from the ClusterExtension status API
-			setStatusProgressing(ext, wrapErrorWithResolutionInfo(resolvedRevisionMetadata.BundleMetadata, err))
-			setInstalledStatusFromRevisionStates(ext, revisionStates)
-			return ctx, nil, err
+			setStatusProgressing(ext, wrapErrorWithResolutionInfo(state.resolvedRevisionMetadata.BundleMetadata, err))
+			setInstalledStatusFromRevisionStates(ext, state.revisionStates)
+			return nil, err
 		}
-		return context.WithValue(ctx, imageFSKey{}, imageFS), nil, nil
+		state.imageFS = imageFS
+		return nil, nil
 	}
 }
 
 func ApplyBundle(a Applier) ReconcileStepFunc {
-	return func(ctx context.Context, ext *ocv1.ClusterExtension) (context.Context, *ctrl.Result, error) {
+	return func(ctx context.Context, state *reconcileState, ext *ocv1.ClusterExtension) (*ctrl.Result, error) {
 		l := log.FromContext(ctx)
-		resolvedRevisionMetadata, err := valueFromContext[RevisionMetadata](ctx, resolvedRevisionMetadataKey{})
-		if err != nil {
-			return ctx, nil, err
-		}
-		imageFS, err := valueFromContext[fs.FS](ctx, imageFSKey{})
-		if err != nil {
-			return ctx, nil, err
-		}
-		revisionStates, err := valueFromContext[RevisionStates](ctx, revisionStatesKey{})
-		if err != nil {
-			return ctx, nil, err
-		}
 		storeLbls := map[string]string{
-			labels.BundleNameKey:      resolvedRevisionMetadata.Name,
-			labels.PackageNameKey:     resolvedRevisionMetadata.Package,
-			labels.BundleVersionKey:   resolvedRevisionMetadata.Version,
-			labels.BundleReferenceKey: resolvedRevisionMetadata.Image,
+			labels.BundleNameKey:      state.resolvedRevisionMetadata.Name,
+			labels.PackageNameKey:     state.resolvedRevisionMetadata.Package,
+			labels.BundleVersionKey:   state.resolvedRevisionMetadata.Version,
+			labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image,
 		}
 		objLbls := map[string]string{
 			labels.OwnerKindKey: ocv1.ClusterExtensionKind,
@@ -212,21 +172,21 @@ func ApplyBundle(a Applier) ReconcileStepFunc {
 		// to ensure exponential backoff can occur:
 		//   - Permission errors (it is not possible to watch changes to permissions.
 		//     The only way to eventually recover from permission errors is to keep retrying).
-		rolloutSucceeded, rolloutStatus, err := a.Apply(ctx, *imageFS, ext, objLbls, storeLbls)
+		rolloutSucceeded, rolloutStatus, err := a.Apply(ctx, state.imageFS, ext, objLbls, storeLbls)
 
 		// Set installed status
 		if rolloutSucceeded {
-			revisionStates = &RevisionStates{Installed: resolvedRevisionMetadata}
-		} else if err == nil && revisionStates.Installed == nil && len(revisionStates.RollingOut) == 0 {
-			revisionStates = &RevisionStates{RollingOut: []*RevisionMetadata{resolvedRevisionMetadata}}
+			state.revisionStates = &RevisionStates{Installed: state.resolvedRevisionMetadata}
+		} else if err == nil && state.revisionStates.Installed == nil && len(state.revisionStates.RollingOut) == 0 {
+			state.revisionStates = &RevisionStates{RollingOut: []*RevisionMetadata{state.resolvedRevisionMetadata}}
 		}
-		setInstalledStatusFromRevisionStates(ext, revisionStates)
+		setInstalledStatusFromRevisionStates(ext, state.revisionStates)
 
 		// If there was an error applying the resolved bundle,
 		// report the error via the Progressing condition.
 		if err != nil {
-			setStatusProgressing(ext, wrapErrorWithResolutionInfo(resolvedRevisionMetadata.BundleMetadata, err))
-			return ctx, nil, err
+			setStatusProgressing(ext, wrapErrorWithResolutionInfo(state.resolvedRevisionMetadata.BundleMetadata, err))
+			return nil, err
 		} else if !rolloutSucceeded {
 			apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
 				Type:               ocv1.TypeProgressing,
@@ -238,6 +198,6 @@ func ApplyBundle(a Applier) ReconcileStepFunc {
 		} else {
 			setStatusProgressing(ext, nil)
 		}
-		return ctx, nil, nil
+		return nil, nil
 	}
 }