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

…tahub-io#1184)

Initially, `Cleanup` functions did not rely on loading Feature's
data for invocation, as certain data was held as fields of the Feature
struct, making it implicitly available. With the refactoring of Feature
to become a "glorified map" container, this behavior has changed,
leading to errors under certain circumstances.

With `DSCI.ServiceMesh` now being a pointer to a struct, there is a corner
case where the (now needed) ServiceMesh spec is `nil`, leading to a panic.

When the reconciliation of DSC is triggered, it is invoked for each
component, even if it is never defined or changed. In a simple
case where DSCI does not have a specified ServiceMesh (`nil` instead of the default),
and DSC has a single `Managed` component that is not KServe/Serverless,
the reconciliation will trigger the removal of KServe resources regardless.
This, in turn, will call for the removal of Serverless features, which rely on
SMCP config when they are applied. With data now being loaded in the cleanup
step, this leads to an error, as there is no SMCP config to read from.

By default, the cleanup logic removes the owner FeatureTracker for each Feature,
and this does not require loading Feature's data, as FeatureTracker is used
as `OwnerReference` for each resources created for a given Feature.

The API allows defining custom functions that can be invoked in
addition. There is one custom cleanup function requiring the Service Mesh
control plane spec to remove part of the config introduced by its patch.
The code in question only worked because it had default values to work
on and was not failing if patched object was not found.

The refactoring wrongly enforced loading of Feature data during cleanup for
this single case, and this cascaded to other Feature sets where it was
unnecessary exposing this faulty behaviour.

With this change, no reference to Feature is enforced at the API
level by introducing the `CleanupFunc(ctx, client)` type for defining
custom cleanup logic. The custom patch function has been reworked
to comply with this.

(cherry picked from commit a88cef0)

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",
+								),
 							))
 					})