feat: adds EnabledWhen to the feature builder (opendatahub-io#1057)

* rebase on top of latest inc * fix linter * rename test func * Update pkg/feature/builder.go Co-authored-by: Bartosz Majsak <[email protected]> * Update pkg/feature/builder.go Co-authored-by: Bartosz Majsak <[email protected]> * fix err var name * Update tests/integration/features/cleanup_int_test.go Co-authored-by: Bartosz Majsak <[email protected]> * fix linter, add ctx * re-remove pesky whitespace * move logic to apply and improve tests * ensure feature tracker is also removed * Update tests/integration/features/cleanup_int_test.go Co-authored-by: Bartosz Majsak <[email protected]> --------- Co-authored-by: Bartosz Majsak <[email protected]> (cherry picked from commit bff2e6a)
VaishnaviHire · Jul 24, 2024 · b10ab4a · b10ab4a
1 parent a255bf7
commit b10ab4a
Show file tree

Hide file tree

Showing 3 changed files with 155 additions and 14 deletions.
diff --git a/pkg/feature/builder.go b/pkg/feature/builder.go
@@ -125,6 +125,20 @@ func (fb *featureBuilder) WithData(loader ...Action) *featureBuilder {
 	return fb
 }
 
+// EnabledWhen determines if a Feature should be loaded and applied based on specified criteria.
+// The criteria are supplied as a function.
+//
+// Note: The function passed should consistently return true while the feature is needed.
+// If the function returns false at any point, the feature's contents might be removed during the reconciliation process.
+func (fb *featureBuilder) EnabledWhen(enabled EnabledFunc) *featureBuilder {
+	fb.builders = append(fb.builders, func(f *Feature) error {
+		f.Enabled = enabled
+
+		return nil
+	})
+	return fb
+}
+
 // WithResources allows to define programmatically which resources should be created when applying defined Feature.
 func (fb *featureBuilder) WithResources(resources ...Action) *featureBuilder {
 	fb.builders = append(fb.builders, func(f *Feature) error {

diff --git a/pkg/feature/feature.go b/pkg/feature/feature.go
@@ -21,7 +21,7 @@ import (
 type Feature struct {
 	Name    string
 	Spec    *Spec
-	Enabled bool
+	Enabled EnabledFunc
 	Managed bool
 	Tracker *featurev1.FeatureTracker
 
@@ -41,18 +41,33 @@ type Feature struct {
 
 func newFeature(name string) *Feature {
 	return &Feature{
-		Name:    name,
-		Enabled: true,
-		Log:     ctrlLog.Log.WithName("features").WithValues("feature", name),
+		Name: name,
+		Enabled: func(_ context.Context, feature *Feature) (bool, error) {
+			return true, nil
+		},
+		Log: ctrlLog.Log.WithName("features").WithValues("feature", name),
 	}
 }
 
 // Action is a func type which can be used for different purposes while having access to Feature struct.
 type Action func(ctx context.Context, feature *Feature) 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)
+
 func (f *Feature) Apply(ctx context.Context) error {
-	if !f.Enabled {
-		return nil
+	// 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 err != nil {
+			return err
+		}
+		errGet := getFeatureTrackerIfAbsent(ctx, f)
+		if errGet == nil {
+			return f.Cleanup(ctx)
+		}
+
+		return client.IgnoreNotFound(errGet)
 	}
 
 	if trackerErr := f.createFeatureTracker(ctx); trackerErr != nil {
@@ -125,10 +140,6 @@ func (f *Feature) applyFeature(ctx context.Context) error {
 }
 
 func (f *Feature) Cleanup(ctx context.Context) error {
-	if !f.Enabled {
-		return nil
-	}
-
 	// Ensure associated FeatureTracker instance has been removed as last one
 	// in the chain of cleanups.
 	f.addCleanup(removeFeatureTracker)

diff --git a/tests/integration/features/cleanup_int_test.go b/tests/integration/features/cleanup_int_test.go
@@ -3,6 +3,7 @@ package features_test
 import (
 	"context"
 
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -58,7 +59,7 @@ var _ = Describe("feature cleanup", func() {
 			Expect(featuresHandler.Apply(ctx)).Should(Succeed())
 
 			// then
-			Eventually(createdSecretHasOwnerReferenceToOwningFeature(namespace, secretName)).
+			Eventually(createdSecretHasOwnerReferenceToOwningFeature(namespace, featureName)).
 				WithContext(ctx).
 				WithTimeout(fixtures.Timeout).
 				WithPolling(fixtures.Interval).
@@ -70,7 +71,7 @@ var _ = Describe("feature cleanup", func() {
 			Expect(featuresHandler.Delete(ctx)).To(Succeed())
 
 			// then
-			Eventually(createdSecretHasOwnerReferenceToOwningFeature(namespace, secretName)).
+			Consistently(createdSecretHasOwnerReferenceToOwningFeature(namespace, featureName)).
 				WithContext(ctx).
 				WithTimeout(fixtures.Timeout).
 				WithPolling(fixtures.Interval).
@@ -79,10 +80,105 @@ var _ = Describe("feature cleanup", func() {
 
 	})
 
+	Context("cleaning up conditionally enabled features", Ordered, func() {
+
+		const (
+			featureName = "enabled-conditionally"
+			secretName  = "test-secret"
+		)
+
+		var (
+			dsci            *dsciv1.DSCInitialization
+			namespace       string
+			featuresHandler *feature.FeaturesHandler
+		)
+
+		BeforeAll(func() {
+			namespace = envtestutil.AppendRandomNameTo("test-conditional-cleanup")
+			dsci = fixtures.NewDSCInitialization(namespace)
+		})
+
+		It("should create feature, apply resource and create feature tracker", func(ctx context.Context) {
+			// given
+			err := fixtures.CreateOrUpdateNamespace(ctx, envTestClient, fixtures.NewNamespace("conditional-ns"))
+			Expect(err).To(Not(HaveOccurred()))
+
+			featuresHandler = feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error {
+				conditionalCreationErr := feature.CreateFeature(featureName).
+					For(handler).
+					UsingConfig(envTest.Config).
+					PreConditions(
+						feature.CreateNamespaceIfNotExists(namespace),
+					).
+					EnabledWhen(namespaceExists).
+					WithResources(fixtures.CreateSecret(secretName, namespace)).
+					Load()
+
+				Expect(conditionalCreationErr).ToNot(HaveOccurred())
+
+				return nil
+			})
+
+			// when
+			Expect(featuresHandler.Apply(ctx)).Should(Succeed())
+
+			// then
+			Eventually(createdSecretHasOwnerReferenceToOwningFeature(namespace, featureName)).
+				WithContext(ctx).
+				WithTimeout(fixtures.Timeout).
+				WithPolling(fixtures.Interval).
+				Should(Succeed())
+		})
+
+		It("should clean up resources when the condition is no longer met", func(ctx context.Context) {
+			// given
+			err := envTestClient.Delete(context.Background(), fixtures.NewNamespace("conditional-ns"))
+			Expect(err).To(Not(HaveOccurred()))
+
+			// Mimic reconcile by re-loading the feature handler
+			featuresHandler = feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error {
+				conditionalCreationErr := feature.CreateFeature(featureName).
+					For(handler).
+					UsingConfig(envTest.Config).
+					PreConditions(
+						feature.CreateNamespaceIfNotExists(namespace),
+					).
+					EnabledWhen(namespaceExists).
+					WithResources(fixtures.CreateSecret(secretName, namespace)).
+					Load()
+
+				Expect(conditionalCreationErr).ToNot(HaveOccurred())
+
+				return nil
+			})
+
+			Expect(featuresHandler.Apply(ctx)).Should(Succeed())
+
+			// then
+			Consistently(createdSecretHasOwnerReferenceToOwningFeature(namespace, featureName)).
+				WithContext(ctx).
+				WithTimeout(fixtures.Timeout).
+				WithPolling(fixtures.Interval).
+				Should(WithTransform(errors.IsNotFound, BeTrue()))
+
+			Consistently(func() error {
+				_, err := fixtures.GetFeatureTracker(ctx, envTestClient, namespace, featureName)
+				if errors.IsNotFound(err) {
+					return nil
+				}
+				return err
+			}).
+				WithContext(ctx).
+				WithTimeout(fixtures.Timeout).
+				WithPolling(fixtures.Interval).
+				Should(Succeed())
+		})
+	})
 })
 
-func createdSecretHasOwnerReferenceToOwningFeature(namespace, secretName string) func(context.Context) error {
+func createdSecretHasOwnerReferenceToOwningFeature(namespace, featureName string) func(context.Context) error {
 	return func(ctx context.Context) error {
+		secretName := "test-secret"
 		secret, err := envTestClientset.CoreV1().
 			Secrets(namespace).
 			Get(ctx, secretName, metav1.GetOptions{})
@@ -106,8 +202,28 @@ func createdSecretHasOwnerReferenceToOwningFeature(namespace, secretName string)
 		}
 
 		tracker := &featurev1.FeatureTracker{}
-		return envTestClient.Get(ctx, client.ObjectKey{
+		err = envTestClient.Get(ctx, client.ObjectKey{
 			Name: trackerName,
 		}, tracker)
+		if err != nil {
+			return err
+		}
+
+		expectedName := namespace + "-" + featureName
+		Expect(tracker.ObjectMeta.Name).To(Equal(expectedName))
+
+		return nil
+	}
+}
+
+func namespaceExists(ctx context.Context, f *feature.Feature) (bool, error) {
+	namespace, err := fixtures.GetNamespace(ctx, f.Client, "conditional-ns")
+	if errors.IsNotFound(err) {
+		return false, nil
+	}
+	// ensuring it fails if namespace is still deleting
+	if namespace.Status.Phase == corev1.NamespaceTerminating {
+		return false, nil
 	}
+	return true, nil
 }