fix(feature): cleanup funcs should not rely on feature's data

controllers/dscinitialization/servicemesh_setup.go

@@ -203,7 +203,10 @@ func (r *DSCInitializationReconciler) authorizationFeatures(instance *dsciv1.DSC
 					feature.WaitForPodsToBeReady(serviceMeshSpec.ControlPlane.Namespace),
 				).
 				OnDelete(
-					servicemesh.RemoveExtensionProvider,
+					servicemesh.RemoveExtensionProvider(
+						instance.Spec.ServiceMesh.ControlPlane,
+						instance.Spec.ApplicationsNamespace+"-auth-provider",
+					),
 				),
 
 			// We do not have the control over deployment resource creation.

pkg/feature/builder.go

@@ -174,11 +174,7 @@ func (fb *featureBuilder) PostConditions(postconditions ...Action) *featureBuild
 }
 
 // OnDelete allow to add cleanup hooks that are executed when the feature is going to be deleted.
-// By default, all resources created by the feature are deleted when the feature is deleted, so there is no need to
-// explicitly add cleanup hooks for them.
-//
-// This is useful when you need to perform some additional cleanup actions such as removing effects of a patch operation.
-func (fb *featureBuilder) OnDelete(cleanups ...Action) *featureBuilder {
+func (fb *featureBuilder) OnDelete(cleanups ...CleanupFunc) *featureBuilder {
 	fb.builders = append(fb.builders, func(f *Feature) error {
 		f.addCleanup(cleanups...)
 

pkg/feature/feature.go

@@ -51,7 +51,7 @@ type Feature struct {
 
 	appliers []resource.Applier
 
-	cleanups          []Action
+	cleanups          []CleanupFunc
 	clusterOperations []Action
 	preconditions     []Action
 	postconditions    []Action
@@ -62,6 +62,12 @@ type Feature struct {
 // while having access to Feature struct.
 type Action func(ctx context.Context, 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,
+// so there is no need to explicitly add cleanup hooks for them.
+// This is useful when you need to perform some additional cleanup actions such as removing effects of a patch operation.
+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)
 
@@ -138,26 +144,17 @@ func (f *Feature) applyFeature(ctx context.Context) error {
 func (f *Feature) Cleanup(ctx context.Context) error {
 	// Ensure associated FeatureTracker instance has been removed as last one
 	// in the chain of cleanups.
-	f.addCleanup(removeFeatureTracker)
+	f.addCleanup(removeFeatureTracker(f))
 
 	var cleanupErrors *multierror.Error
-
-	for _, dataProvider := range f.dataProviders {
-		cleanupErrors = multierror.Append(cleanupErrors, dataProvider(ctx, f))
-	}
-
-	if dataLoadErr := cleanupErrors.ErrorOrNil(); dataLoadErr != nil {
-		return dataLoadErr
-	}
-
 	for _, cleanupFunc := range f.cleanups {
-		cleanupErrors = multierror.Append(cleanupErrors, cleanupFunc(ctx, f))
+		cleanupErrors = multierror.Append(cleanupErrors, cleanupFunc(ctx, f.Client))
 	}
 
 	return cleanupErrors.ErrorOrNil()
 }
 
-func (f *Feature) addCleanup(cleanupFuncs ...Action) {
+func (f *Feature) addCleanup(cleanupFuncs ...CleanupFunc) {
 	f.cleanups = append(f.cleanups, cleanupFuncs...)
 }
 

pkg/feature/feature_tracker_handler.go

@@ -30,7 +30,7 @@ func (e *withConditionReasonError) Error() string {
 // createFeatureTracker creates a FeatureTracker, persists it in the cluster,
 // and attaches it to the provided Feature instance.
 func createFeatureTracker(ctx context.Context, f *Feature) error {
-	tracker, errGet := getFeatureTracker(ctx, f)
+	tracker, errGet := getFeatureTracker(ctx, f.Client, f.Name, f.TargetNamespace)
 	if client.IgnoreNotFound(errGet) != nil {
 		return errGet
 	}
@@ -56,28 +56,30 @@ func createFeatureTracker(ctx context.Context, f *Feature) error {
 }
 
 // removeFeatureTracker removes the FeatureTracker associated with the provided Feature instance if one exists in the cluster.
-func removeFeatureTracker(ctx context.Context, f *Feature) error {
-	associatedTracker := f.tracker
-	if associatedTracker == nil {
-		// Check if it is persisted in the cluster, but Feature do not have it attached
-		if tracker, errGet := getFeatureTracker(ctx, f); client.IgnoreNotFound(errGet) != nil {
-			return errGet
-		} else {
-			associatedTracker = tracker
+func removeFeatureTracker(f *Feature) CleanupFunc {
+	return func(ctx context.Context, cli client.Client) error {
+		associatedTracker := f.tracker
+		if associatedTracker == nil {
+			// Check if it is persisted in the cluster, but Feature do not have it attached
+			if tracker, errGet := getFeatureTracker(ctx, cli, f.Name, f.TargetNamespace); client.IgnoreNotFound(errGet) != nil {
+				return errGet
+			} else {
+				associatedTracker = tracker
+			}
 		}
-	}
 
-	if associatedTracker != nil {
-		return client.IgnoreNotFound(f.Client.Delete(ctx, associatedTracker))
-	}
+		if associatedTracker != nil {
+			return client.IgnoreNotFound(f.Client.Delete(ctx, associatedTracker))
+		}
 
-	return nil
+		return nil
+	}
 }
 
-func getFeatureTracker(ctx context.Context, f *Feature) (*featurev1.FeatureTracker, error) {
-	tracker := featurev1.NewFeatureTracker(f.Name, f.TargetNamespace)
+func getFeatureTracker(ctx context.Context, cli client.Client, featureName, namespace string) (*featurev1.FeatureTracker, error) {
+	tracker := featurev1.NewFeatureTracker(featureName, namespace)
 
-	if errGet := f.Client.Get(ctx, client.ObjectKeyFromObject(tracker), tracker); errGet != nil {
+	if errGet := cli.Get(ctx, client.ObjectKeyFromObject(tracker), tracker); errGet != nil {
 		return nil, errGet
 	}
 

pkg/feature/servicemesh/cleanup.go

@@ -2,65 +2,62 @@ package servicemesh
 
 import (
 	"context"
-	"fmt"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	infrav1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/infrastructure/v1"
 	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk"
 	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature"
 )
 
-func RemoveExtensionProvider(ctx context.Context, f *feature.Feature) error {
-	extensionName, errExtName := FeatureData.Authorization.ExtensionProviderName.Extract(f)
-	if errExtName != nil {
-		return fmt.Errorf("failed to get extension name struct: %w", errExtName)
-	}
+func RemoveExtensionProvider(controlPlane infrav1.ControlPlaneSpec, extensionName string) feature.CleanupFunc {
+	return func(ctx context.Context, cli client.Client) error {
+		smcp := &unstructured.Unstructured{}
+		smcp.SetGroupVersionKind(gvk.ServiceMeshControlPlane)
 
-	controlPlane, err := FeatureData.ControlPlane.Extract(f)
-	if err != nil {
-		return fmt.Errorf("failed to get control plane struct: %w", err)
-	}
+		if err := cli.Get(ctx, client.ObjectKey{
+			Namespace: controlPlane.Namespace,
+			Name:      controlPlane.Name,
+		}, smcp); err != nil {
+			return client.IgnoreNotFound(err)
+		}
 
-	smcp := &unstructured.Unstructured{}
-	smcp.SetGroupVersionKind(gvk.ServiceMeshControlPlane)
+		extensionProviders, found, err := unstructured.NestedSlice(smcp.Object, "spec", "techPreview", "meshConfig", "extensionProviders")
+		if err != nil {
+			return err
+		}
+		if !found {
+			return nil
+		}
 
-	if err := f.Client.Get(ctx, client.ObjectKey{
-		Namespace: controlPlane.Namespace,
-		Name:      controlPlane.Name,
-	}, smcp); err != nil {
-		return client.IgnoreNotFound(err)
-	}
+		removed := false
 
-	extensionProviders, found, err := unstructured.NestedSlice(smcp.Object, "spec", "techPreview", "meshConfig", "extensionProviders")
-	if err != nil {
-		return err
-	}
-	if !found {
-		f.Log.Info("no extension providers found", "feature", f.Name, "control-plane", controlPlane.Name, "namespace", controlPlane.Namespace)
-		return nil
-	}
+		for i, v := range extensionProviders {
+			extensionProvider, ok := v.(map[string]interface{})
+			if !ok {
+				continue
+			}
 
-	for i, v := range extensionProviders {
-		extensionProvider, ok := v.(map[string]interface{})
-		if !ok {
-			f.Log.Info("WARN: Unexpected type for extensionProvider, it will not be removed")
-			continue
-		}
-		currentExtensionName, isString := extensionProvider["name"].(string)
-		if !isString {
-			f.Log.Info("WARN: Unexpected type for currentExtensionName, it will not be removed")
-			continue
-		}
-		if currentExtensionName == extensionName {
-			extensionProviders = append(extensionProviders[:i], extensionProviders[i+1:]...)
-			err = unstructured.SetNestedSlice(smcp.Object, extensionProviders, "spec", "techPreview", "meshConfig", "extensionProviders")
-			if err != nil {
-				return err
+			currentExtensionName, isString := extensionProvider["name"].(string)
+			if !isString {
+				continue
+			}
+			if currentExtensionName == extensionName {
+				extensionProviders = append(extensionProviders[:i], extensionProviders[i+1:]...)
+				err = unstructured.SetNestedSlice(smcp.Object, extensionProviders, "spec", "techPreview", "meshConfig", "extensionProviders")
+				if err != nil {
+					return err
+				}
+				removed = true
+				break
 			}
-			break
 		}
-	}
 
-	return f.Client.Update(ctx, smcp)
+		if removed {
+			return cli.Update(ctx, smcp)
+		}
+
+		return nil
+	}
 }

tests/integration/features/servicemesh_feature_test.go

@@ -212,7 +212,10 @@ var _ = Describe("Service Mesh setup", func() {
 								servicemesh.FeatureData.ControlPlane.Define(&dsci.Spec).AsAction(),
 							).
 							OnDelete(
-								servicemesh.RemoveExtensionProvider,
+								servicemesh.RemoveExtensionProvider(
+									dsci.Spec.ServiceMesh.ControlPlane,
+									dsci.Spec.ApplicationsNamespace+"-auth-provider",
+								),
 							))
 					})