refactor(feature): decouples Feature from client (opendatahub-io#1234)

Including the client in the function signature makes its usage
explicit and more idiomatic. This approach clarifies that the
client is intended for use within the function. Decoupling the
client from Feature struct  enhances code readability.

It's a follow-up to PR
[opendatahub-io#1228](opendatahub-io#1228 (comment)).

(cherry picked from commit fde5d69)

components/kserve/kserve_config_handler.go

@@ -148,19 +148,19 @@ func (k *Kserve) configureServerless(ctx context.Context, cli client.Client, log
 			return dependOpsErrors
 		}
 
-		serverlessFeatures := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance))
+		serverlessFeatures := feature.ComponentFeaturesHandler(owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance))
 
-		if err := serverlessFeatures.Apply(ctx); err != nil {
+		if err := serverlessFeatures.Apply(ctx, cli); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
 func (k *Kserve) removeServerlessFeatures(ctx context.Context, cli client.Client, owner metav1.Object, instance *dsciv1.DSCInitializationSpec) error {
-	serverlessFeatures := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance))
+	serverlessFeatures := feature.ComponentFeaturesHandler(owner, k.GetComponentName(), instance.ApplicationsNamespace, k.configureServerlessFeatures(instance))
 
-	return serverlessFeatures.Delete(ctx)
+	return serverlessFeatures.Delete(ctx, cli)
 }
 
 func checkDependentOperators(ctx context.Context, cli client.Client) *multierror.Error {

components/kserve/servicemesh_setup.go

@@ -20,8 +20,8 @@ import (
 func (k *Kserve) configureServiceMesh(ctx context.Context, cli client.Client, owner metav1.Object, dscispec *dsciv1.DSCInitializationSpec) error {
 	if dscispec.ServiceMesh != nil {
 		if dscispec.ServiceMesh.ManagementState == operatorv1.Managed && k.GetManagementState() == operatorv1.Managed {
-			serviceMeshInitializer := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec))
-			return serviceMeshInitializer.Apply(ctx)
+			serviceMeshInitializer := feature.ComponentFeaturesHandler(owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec))
+			return serviceMeshInitializer.Apply(ctx, cli)
 		}
 		if dscispec.ServiceMesh.ManagementState == operatorv1.Unmanaged && k.GetManagementState() == operatorv1.Managed {
 			return nil
@@ -32,8 +32,8 @@ func (k *Kserve) configureServiceMesh(ctx context.Context, cli client.Client, ow
 }
 
 func (k *Kserve) removeServiceMeshConfigurations(ctx context.Context, cli client.Client, owner metav1.Object, dscispec *dsciv1.DSCInitializationSpec) error {
-	serviceMeshInitializer := feature.ComponentFeaturesHandler(cli, owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec))
-	return serviceMeshInitializer.Delete(ctx)
+	serviceMeshInitializer := feature.ComponentFeaturesHandler(owner, k.GetComponentName(), dscispec.ApplicationsNamespace, k.defineServiceMeshFeatures(ctx, cli, dscispec))
+	return serviceMeshInitializer.Delete(ctx, cli)
 }
 
 func (k *Kserve) defineServiceMeshFeatures(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) feature.FeaturesProvider {

controllers/dscinitialization/servicemesh_setup.go

@@ -8,6 +8,7 @@ import (
 	operatorv1 "github.com/openshift/api/operator/v1"
 	conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1"
 	corev1 "k8s.io/api/core/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1"
 	"github.com/opendatahub-io/opendatahub-operator/v2/controllers/status"
@@ -39,7 +40,7 @@ func (r *DSCInitializationReconciler) configureServiceMesh(ctx context.Context,
 		capabilities = append(capabilities, authzCapability)
 
 		for _, capability := range capabilities {
-			capabilityErr := capability.Apply(ctx)
+			capabilityErr := capability.Apply(ctx, r.Client)
 			if capabilityErr != nil {
 				r.Log.Error(capabilityErr, "failed applying service mesh resources")
 				r.Recorder.Eventf(instance, corev1.EventTypeWarning, "DSCInitializationReconcileError", "failed applying service mesh resources")
@@ -77,7 +78,7 @@ func (r *DSCInitializationReconciler) removeServiceMesh(ctx context.Context, ins
 		capabilities = append(capabilities, authzCapability)
 
 		for _, capability := range capabilities {
-			capabilityErr := capability.Delete(ctx)
+			capabilityErr := capability.Delete(ctx, r.Client)
 			if capabilityErr != nil {
 				r.Log.Error(capabilityErr, "failed deleting service mesh resources")
 				r.Recorder.Eventf(instance, corev1.EventTypeWarning, "DSCInitializationReconcileError", "failed deleting service mesh resources")
@@ -91,7 +92,7 @@ func (r *DSCInitializationReconciler) removeServiceMesh(ctx context.Context, ins
 
 func (r *DSCInitializationReconciler) serviceMeshCapability(instance *dsciv1.DSCInitialization, initialCondition *conditionsv1.Condition) *feature.HandlerWithReporter[*dsciv1.DSCInitialization] { //nolint:lll // Reason: generics are long
 	return feature.NewHandlerWithReporter(
-		feature.ClusterFeaturesHandler(r.Client, instance, r.serviceMeshCapabilityFeatures(instance)),
+		feature.ClusterFeaturesHandler(instance, r.serviceMeshCapabilityFeatures(instance)),
 		createCapabilityReporter(r.Client, instance, initialCondition),
 	)
 }
@@ -119,7 +120,7 @@ func (r *DSCInitializationReconciler) authorizationCapability(ctx context.Contex
 	}
 
 	return feature.NewHandlerWithReporter(
-		feature.ClusterFeaturesHandler(r.Client, instance, r.authorizationFeatures(instance)),
+		feature.ClusterFeaturesHandler(instance, r.authorizationFeatures(instance)),
 		createCapabilityReporter(r.Client, instance, condition),
 	), nil
 }
@@ -128,7 +129,7 @@ func (r *DSCInitializationReconciler) serviceMeshCapabilityFeatures(instance *ds
 	return func(registry feature.FeaturesRegistry) error {
 		controlPlaneSpec := instance.Spec.ServiceMesh.ControlPlane
 
-		meshMetricsCollection := func(_ context.Context, _ *feature.Feature) (bool, error) {
+		meshMetricsCollection := func(_ context.Context, _ client.Client, _ *feature.Feature) (bool, error) {
 			return controlPlaneSpec.MetricsCollection == "Istio", nil
 		}
 
@@ -221,13 +222,13 @@ func (r *DSCInitializationReconciler) authorizationFeatures(instance *dsciv1.DSC
 						Include(path.Join(Templates.AuthorinoDir, "deployment.injection.patch.tmpl.yaml")),
 				).
 				PreConditions(
-					func(ctx context.Context, f *feature.Feature) error {
+					func(ctx context.Context, cli client.Client, f *feature.Feature) error {
 						namespace, err := servicemesh.FeatureData.Authorization.Namespace.Extract(f)
 						if err != nil {
 							return fmt.Errorf("failed trying to resolve authorization provider namespace for feature '%s': %w", f.Name, err)
 						}
 
-						return feature.WaitForPodsToBeReady(namespace)(ctx, f)
+						return feature.WaitForPodsToBeReady(namespace)(ctx, cli, f)
 					},
 				).
 				WithData(servicemesh.FeatureData.ControlPlane.Define(&instance.Spec).AsAction()).

pkg/feature/builder.go

@@ -4,14 +4,8 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/hashicorp/go-multierror"
-	"github.com/pkg/errors"
-	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/rest"
-	"k8s.io/client-go/tools/clientcmd"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/client/config"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	featurev1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1"
@@ -27,8 +21,6 @@ type featureBuilder struct {
 	owner       metav1.Object
 	targetNs    string
 
-	client client.Client
-
 	builders []partialBuilder
 }
 
@@ -193,16 +185,10 @@ func (fb *featureBuilder) OnDelete(cleanups ...CleanupFunc) *featureBuilder {
 	return fb
 }
 
-// UsingClient allows to provide a custom client to the feature. If not called, a default client will be created.
-func (fb *featureBuilder) UsingClient(cli client.Client) *featureBuilder {
-	fb.client = cli
-	return fb
-}
-
 // Create creates a new Feature instance and add it to corresponding FeaturesHandler.
 // The actual feature creation in the cluster is not performed here.
 func (fb *featureBuilder) Create() (*Feature, error) {
-	alwaysEnabled := func(_ context.Context, _ *Feature) (bool, error) {
+	alwaysEnabled := func(_ context.Context, _ client.Client, _ *Feature) (bool, error) {
 		return true, nil
 	}
 
@@ -215,15 +201,6 @@ func (fb *featureBuilder) Create() (*Feature, error) {
 		owner:   fb.owner,
 	}
 
-	// UsingClient has not been called, so we need to create a new client
-	if fb.client == nil {
-		if err := createDefaultClient()(f); err != nil {
-			return nil, err
-		}
-	} else {
-		f.Client = fb.client
-	}
-
 	for i := range fb.builders {
 		if err := fb.builders[i](f); err != nil {
 			return nil, err
@@ -232,39 +209,3 @@ func (fb *featureBuilder) Create() (*Feature, error) {
 
 	return f, nil
 }
-
-func createDefaultClient() partialBuilder {
-	return func(f *Feature) error {
-		var err error
-
-		restCfg, err := config.GetConfig()
-		if errors.Is(err, rest.ErrNotInCluster) {
-			// rollback to local kubeconfig - this can be helpful when running the process locally i.e. while debugging
-			kubeconfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
-				&clientcmd.ClientConfigLoadingRules{ExplicitPath: clientcmd.RecommendedHomeFile},
-				&clientcmd.ConfigOverrides{},
-			)
-
-			restCfg, err = kubeconfig.ClientConfig()
-			if err != nil {
-				return err
-			}
-		} else if err != nil {
-			return err
-		}
-
-		f.Client, err = client.New(restCfg, client.Options{})
-		if err != nil {
-			return errors.WithStack(err)
-		}
-
-		var multiErr *multierror.Error
-		s := f.Client.Scheme()
-		multiErr = multierror.Append(multiErr,
-			featurev1.AddToScheme(s),
-			apiextv1.AddToScheme(s),
-		)
-
-		return multiErr.ErrorOrNil()
-	}
-}

pkg/feature/conditions.go

@@ -40,8 +40,8 @@ func (e *MissingOperatorError) Error() string {
 }
 
 func EnsureOperatorIsInstalled(operatorName string) Action {
-	return func(ctx context.Context, f *Feature) error {
-		if found, err := cluster.SubscriptionExists(ctx, f.Client, operatorName); !found || err != nil {
+	return func(ctx context.Context, cli client.Client, f *Feature) error {
+		if found, err := cluster.SubscriptionExists(ctx, cli, operatorName); !found || err != nil {
 			return fmt.Errorf(
 				"failed to find the pre-requisite operator subscription %q, please ensure operator is installed. %w",
 				operatorName,
@@ -53,13 +53,13 @@ func EnsureOperatorIsInstalled(operatorName string) Action {
 }
 
 func WaitForPodsToBeReady(namespace string) Action {
-	return func(ctx context.Context, f *Feature) error {
+	return func(ctx context.Context, cli client.Client, f *Feature) error {
 		f.Log.Info("waiting for pods to become ready", "namespace", namespace, "duration (s)", duration.Seconds())
 
 		return wait.PollUntilContextTimeout(ctx, interval, duration, false, func(ctx context.Context) (bool, error) {
 			var podList corev1.PodList
 
-			err := f.Client.List(ctx, &podList, client.InNamespace(namespace))
+			err := cli.List(ctx, &podList, client.InNamespace(namespace))
 			if err != nil {
 				return false, err
 			}
@@ -103,14 +103,14 @@ func WaitForPodsToBeReady(namespace string) Action {
 }
 
 func WaitForResourceToBeCreated(namespace string, gvk schema.GroupVersionKind) Action {
-	return func(ctx context.Context, f *Feature) error {
+	return func(ctx context.Context, cli client.Client, f *Feature) error {
 		f.Log.Info("waiting for resource to be created", "namespace", namespace, "resource", gvk)
 
 		return wait.PollUntilContextTimeout(ctx, interval, duration, false, func(ctx context.Context) (bool, error) {
 			list := &unstructured.UnstructuredList{}
 			list.SetGroupVersionKind(gvk)
 
-			err := f.Client.List(ctx, list, client.InNamespace(namespace), client.Limit(1))
+			err := cli.List(ctx, list, client.InNamespace(namespace), client.Limit(1))
 			if err != nil {
 				f.Log.Error(err, "failed waiting for resource", "namespace", namespace, "resource", gvk)
 

pkg/feature/feature.go

@@ -41,8 +41,7 @@ type Feature struct {
 	Enabled         EnabledFunc
 	Managed         bool
 
-	Client client.Client
-	Log    logr.Logger
+	Log logr.Logger
 
 	tracker *featurev1.FeatureTracker
 	source  *featurev1.Source
@@ -61,7 +60,7 @@ type Feature struct {
 
 // Action is a func type which can be used for different purposes during Feature's lifecycle
 // while having access to Feature struct.
-type Action func(ctx context.Context, f *Feature) error
+type Action func(ctx context.Context, cli client.Client, f *Feature) error
 
 // CleanupFunc defines how to clean up resources associated with a feature.
 // By default, all resources created by the feature are deleted when the feature is,
@@ -70,70 +69,70 @@ type Action func(ctx context.Context, f *Feature) error
 type CleanupFunc func(ctx context.Context, cli client.Client) error
 
 // EnabledFunc is a func type used to determine if a feature should be enabled.
-type EnabledFunc func(ctx context.Context, feature *Feature) (bool, error)
+type EnabledFunc func(ctx context.Context, cli client.Client, feature *Feature) (bool, error)
 
 // Apply applies the feature to the cluster.
 // It creates a FeatureTracker resource to establish ownership and reports the result of the operation as a condition.
-func (f *Feature) Apply(ctx context.Context) error {
+func (f *Feature) Apply(ctx context.Context, cli client.Client) error {
 	// If the feature is disabled, but the FeatureTracker exists in the cluster, ensure clean-up is triggered.
 	// This means that the feature was previously enabled, but now it is not anymore.
-	if enabled, err := f.Enabled(ctx, f); !enabled || err != nil {
+	if enabled, err := f.Enabled(ctx, cli, f); !enabled || err != nil {
 		if err != nil {
 			return err
 		}
 
-		return f.Cleanup(ctx)
+		return f.Cleanup(ctx, cli)
 	}
 
-	if trackerErr := createFeatureTracker(ctx, f); trackerErr != nil {
+	if trackerErr := createFeatureTracker(ctx, cli, f); trackerErr != nil {
 		return trackerErr
 	}
 
-	if _, updateErr := status.UpdateWithRetry(ctx, f.Client, f.tracker, func(saved *featurev1.FeatureTracker) {
+	if _, updateErr := status.UpdateWithRetry(ctx, cli, f.tracker, func(saved *featurev1.FeatureTracker) {
 		status.SetProgressingCondition(&saved.Status.Conditions, string(featurev1.ConditionReason.FeatureCreated), fmt.Sprintf("Applying feature [%s]", f.Name))
 		saved.Status.Phase = status.PhaseProgressing
 	}); updateErr != nil {
 		return updateErr
 	}
 
-	applyErr := f.applyFeature(ctx)
-	_, reportErr := createFeatureTrackerStatusReporter(f).ReportCondition(ctx, applyErr)
+	applyErr := f.applyFeature(ctx, cli)
+	_, reportErr := createFeatureTrackerStatusReporter(cli, f).ReportCondition(ctx, applyErr)
 
 	return multierror.Append(applyErr, reportErr).ErrorOrNil()
 }
 
-func (f *Feature) applyFeature(ctx context.Context) error {
+func (f *Feature) applyFeature(ctx context.Context, cli client.Client) error {
 	var multiErr *multierror.Error
 
 	for _, dataProvider := range f.dataProviders {
-		multiErr = multierror.Append(multiErr, dataProvider(ctx, f))
+		multiErr = multierror.Append(multiErr, dataProvider(ctx, cli, f))
 	}
 	if errDataLoad := multiErr.ErrorOrNil(); errDataLoad != nil {
 		return &withConditionReasonError{reason: featurev1.ConditionReason.LoadTemplateData, err: errDataLoad}
 	}
 
 	for _, precondition := range f.preconditions {
-		multiErr = multierror.Append(multiErr, precondition(ctx, f))
+		multiErr = multierror.Append(multiErr, precondition(ctx, cli, f))
 	}
 	if preconditionsErr := multiErr.ErrorOrNil(); preconditionsErr != nil {
 		return &withConditionReasonError{reason: featurev1.ConditionReason.PreConditions, err: preconditionsErr}
 	}
 
 	for _, clusterOperation := range f.clusterOperations {
-		if errClusterOperation := clusterOperation(ctx, f); errClusterOperation != nil {
+		if errClusterOperation := clusterOperation(ctx, cli, f); errClusterOperation != nil {
 			return &withConditionReasonError{reason: featurev1.ConditionReason.ResourceCreation, err: errClusterOperation}
 		}
 	}
 
 	for i := range f.appliers {
 		r := f.appliers[i]
-		if processErr := r.Apply(ctx, f.Client, f.data, DefaultMetaOptions(f)...); processErr != nil {
+		if processErr := r.Apply(ctx, cli, f.data, DefaultMetaOptions(f)...); processErr != nil {
 			return &withConditionReasonError{reason: featurev1.ConditionReason.ApplyManifests, err: processErr}
 		}
 	}
 
 	for _, postcondition := range f.postconditions {
-		multiErr = multierror.Append(multiErr, postcondition(ctx, f))
+		multiErr = multierror.Append(multiErr, postcondition(ctx, cli, f))
 	}
 	if postConditionErr := multiErr.ErrorOrNil(); postConditionErr != nil {
 		return &withConditionReasonError{reason: featurev1.ConditionReason.PostConditions, err: postConditionErr}
@@ -142,14 +141,14 @@ func (f *Feature) applyFeature(ctx context.Context) error {
 	return nil
 }
 
-func (f *Feature) Cleanup(ctx context.Context) error {
+func (f *Feature) Cleanup(ctx context.Context, cli client.Client) error {
 	// Ensure associated FeatureTracker instance has been removed as last one
 	// in the chain of cleanups.
 	f.addCleanup(removeFeatureTracker(f))
 
 	var cleanupErrors *multierror.Error
 	for _, cleanupFunc := range f.cleanups {
-		cleanupErrors = multierror.Append(cleanupErrors, cleanupFunc(ctx, f.Client))
+		cleanupErrors = multierror.Append(cleanupErrors, cleanupFunc(ctx, cli))
 	}
 
 	return cleanupErrors.ErrorOrNil()

pkg/feature/feature_data.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"fmt"
 
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
 	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/provider"
 )
 
@@ -13,8 +15,8 @@ import (
 //
 // If the value is static, consider using provider.ValueOf(variable).Get as passed provider function.
 func Entry[T any](key string, providerFunc provider.DataProviderFunc[T]) Action {
-	return func(ctx context.Context, f *Feature) error {
-		data, err := providerFunc(ctx, f.Client)
+	return func(ctx context.Context, cli client.Client, f *Feature) error {
+		data, err := providerFunc(ctx, cli)
 		if err != nil {
 			return err
 		}