Remove deprecated expression/ref evaluation from flux controller, fix tests

Signed-off-by: Klaudiusz Fabryczny <klaudiusz.fabryczny@sap.com>

Author: k-fabryczny

e2e/plugin/e2e_test.go

@@ -109,6 +109,7 @@ var _ = Describe("Plugin E2E", Ordered, func() {
 	})
 
 	It("should resolve option values from direct plugin reference", func() {
+		Skip("Plugin-to-Plugin ValueFrom.Ref resolution is deprecated. Use PluginPreset (#1776).")
 		By("setting up cluster role binding for OIDC on remote cluster")
 		expect.SetupOIDCClusterRoleBinding(ctx, remoteClient, remoteOIDCClusterRoleBindingName, remoteIntegrationCluster, env.TestNamespace)
 
@@ -129,6 +130,7 @@ var _ = Describe("Plugin E2E", Ordered, func() {
 	})
 
 	It("should resolve option values from plugin reference by label selector", func() {
+		Skip("Plugin-to-Plugin ValueFrom.Ref resolution is deprecated. Use PluginPreset for cross-resource references (#1776).")
 		By("executing the plugin integration scenario with plugin reference by label selector")
 		scenarios.PluginIntegrationBySelector(ctx, adminClient, remoteClient, env, remoteIntegrationCluster)
 	})

internal/controller/plugin/plugin_controller_flux.go

@@ -460,58 +460,22 @@ func (r *PluginReconciler) fetchReleaseStatus(ctx context.Context,
 	}
 }
 
-// computeReleaseValues resolves Expressions and ValueFromRefs in the Plugin's option values
-// and inserts the Greenhouse values
-func computeReleaseValues(ctx context.Context, c client.Client, plugin *greenhousev1alpha1.Plugin, expressionEvaluation, integrationEnabled bool) ([]greenhousev1alpha1.PluginOptionValue, error) {
+// computeReleaseValues validates the Plugin's option values and inserts the Greenhouse values.
+// Expressions and ValueFromRefs are no longer resolved here - they must be resolved by the
+// PluginPreset controller before creating the Plugin.
+// This function only validates that all option values have either a direct value or a secret reference.
+func computeReleaseValues(ctx context.Context, c client.Client, plugin *greenhousev1alpha1.Plugin, _expressionEvaluation, _integrationEnabled bool) ([]greenhousev1alpha1.PluginOptionValue, error) {
 	optionValues, err := helm.GetPluginOptionValuesForPlugin(ctx, c, plugin)
 	if err != nil {
 		return nil, err
 	}
-	trackedObjects := make([]string, 0)
-	// initialize CEL resolver
-	var celResolver *helm.CELResolver
-	if expressionEvaluation {
-		celResolver, err = helm.NewCELResolver(optionValues)
-		if err != nil {
-			return nil, fmt.Errorf("failed to initialize CEL resolver: %w", err)
-		}
-	}
-	for i, v := range optionValues {
+
+	for _, v := range optionValues {
 		switch {
 		case v.Value != nil:
 			// noop, direct values are already set
 			continue
 
-		case v.Expression != nil:
-			if celResolver == nil {
-				celResolver, err = helm.NewCELResolver(optionValues)
-				if err != nil {
-					return nil, fmt.Errorf("failed to initialize CEL resolver: %w", err)
-				}
-			}
-			// This PR adds CEL expression evaluation to the PluginPreset controller (#1774).
-			// The Plugin controller's expression evaluation remains active (gated by feature flag)
-			resolvedOptionValue, err := celResolver.ResolveExpression(v, expressionEvaluation)
-			if err != nil {
-				return nil, err
-			}
-			optionValues[i] = *resolvedOptionValue
-
-		case v.ValueFrom != nil && v.ValueFrom.Ref != nil:
-			// TODO(#1776): References should no longer be resolved by Plugin controller.
-			// Once PluginPreset controller handles all reference resolution,
-			// this branch should return an error instead of resolving.
-			if !integrationEnabled {
-				continue
-			}
-			//TODO: handle external references
-			resolvedOptionValue, objectTrackers, err := ResolveValueFromRef(ctx, c, plugin, v)
-			if err != nil {
-				return nil, err
-			}
-			trackedObjects = append(trackedObjects, objectTrackers...)
-			optionValues[i] = *resolvedOptionValue
-
 		case v.ValueFrom != nil && v.ValueFrom.Secret != nil:
 			// noop, secret refs are not resolved here
 			continue
@@ -520,21 +484,6 @@ func computeReleaseValues(ctx context.Context, c client.Client, plugin *greenhou
 		}
 	}
 
-	// update tracking information for plugin integrations
-	if integrationEnabled {
-		// remove tracking annotations from resources that are no longer being tracked
-		if err := removeUntrackedObjectAnnotations(ctx, c, plugin, trackedObjects); err != nil {
-			// log err, will retry on next reconciliation
-			log.FromContext(ctx).Error(err, "failed to remove untracked object annotations", "namespace", plugin.Namespace, "plugin", plugin.Name)
-		}
-		if len(trackedObjects) > 0 {
-			plugin.Status.TrackedObjects = trackedObjects
-		} else {
-			// clear tracked objects if there are none
-			plugin.Status.TrackedObjects = nil
-		}
-	}
-
 	return optionValues, nil
 }
 

internal/controller/plugin/plugin_integration/plugin_integration_test.go

@@ -90,6 +90,7 @@ var _ = Describe("Plugin Integration", Ordered, func() {
 	})
 
 	It("should create HelmReleases and resolve values from external references", func() {
+		Skip("Plugin-to-Plugin ValueFrom.Ref resolution is deprecated. Use PluginPreset for cross-resource references (#1776).")
 		By("creating alerts plugin with a label")
 		alerts = test.NewPlugin(test.Ctx, "alerts", testNamespace,
 			test.WithClusterPluginDefinition(testPluginDefinition.Name),
@@ -139,6 +140,7 @@ var _ = Describe("Plugin Integration", Ordered, func() {
 	})
 
 	It("should track and untrack plugin dependencies", func() {
+		Skip("Deprecated. Use PluginPreset for cross-resource references (#1776).")
 		By("verifying tracking annotation was set on alerts")
 		Eventually(func(g Gomega) {
 			err := test.K8sClient.Get(test.Ctx, client.ObjectKeyFromObject(alerts), alerts)

internal/controller/plugin/plugin_values_resolver.go

@@ -19,7 +19,6 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
-	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
@@ -282,43 +281,6 @@ func addTrackingAnnotation(uObject *unstructured.Unstructured, tracker string) {
 	}
 }
 
-// removeUntrackedObjectAnnotations removes tracking annotations from resources that are no longer being tracked.
-// This ensures that when a Plugin A changes its value references (e.g., from Plugin B to Plugin C),
-// the tracking annotation is removed from the old resource (Plugin B).
-// It compares the current tracked objects with the previous ones and removes the tracker ID
-// from resources that are no longer in the tracked list.
-func removeUntrackedObjectAnnotations(ctx context.Context, c client.Client, plugin *greenhousev1alpha1.Plugin, currentTrackedObjects []string) error {
-	// previously tracked objects from plugin status
-	previousTrackedObjects := plugin.Status.TrackedObjects
-	if len(previousTrackedObjects) == 0 {
-		// No previous tracking, nothing to clean up
-		return nil
-	}
-
-	// find objects previously tracked objects
-	untrackedObjects := findUntrackedObjects(previousTrackedObjects, currentTrackedObjects)
-	if len(untrackedObjects) == 0 {
-		// No untracked objects to clean up
-		return nil
-	}
-
-	// tracker ID for reconciling plugin
-	tracker := trackingID(plugin.GroupVersionKind().Kind, plugin.GetName())
-
-	// remove tracking-id from each untracked object
-	allErrors := make([]error, 0)
-	for _, untrackedObjectID := range untrackedObjects {
-		if err := removeTrackingAnnotation(ctx, c, plugin.GetNamespace(), untrackedObjectID, tracker); err != nil {
-			log.FromContext(ctx).Error(err, "failed to remove tracking annotation from untracked object",
-				"plugin", plugin.GetName(),
-				"untrackedObject", untrackedObjectID)
-			// continue on error the failed attempt can be retried on next reconciliation
-			allErrors = append(allErrors, err)
-		}
-	}
-	return utilerrors.NewAggregate(allErrors)
-}
-
 // findUntrackedObjects returns objects that were previously tracked but are not in the current tracked list.
 func findUntrackedObjects(previousTracked, currentTracked []string) []string {
 	// create a map of current tracked objects for quick lookup
@@ -338,86 +300,6 @@ func findUntrackedObjects(previousTracked, currentTracked []string) []string {
 	return untrackedObjects
 }
 
-// removeTrackingAnnotation removes a specific tracker ID from a resource's tracking annotation.
-func removeTrackingAnnotation(ctx context.Context, c client.Client, namespace, objectID, tracker string) error {
-	kind, name, err := parseTrackingID(objectID)
-	if err != nil {
-		return err
-	}
-	gvk := buildGVK(kind)
-	key := types.NamespacedName{
-		Name:      name,
-		Namespace: namespace,
-	}
-
-	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
-		uObject := &unstructured.Unstructured{}
-		uObject.SetGroupVersionKind(gvk)
-
-		if err := c.Get(ctx, key, uObject); err != nil {
-			if apierrors.IsNotFound(err) {
-				// Resource no longer exists, nothing to clean up
-				log.FromContext(ctx).Info("untracked object not found, skipping cleanup",
-					"kind", kind,
-					"namespace", namespace,
-					"name", name)
-				return nil
-			}
-			return err
-		}
-
-		// current annotations
-		annotations := uObject.GetAnnotations()
-		if annotations == nil {
-			// No annotations, nothing to remove
-			return nil
-		}
-
-		// current tracking annotation value
-		trackingValue, ok := annotations[greenhouseapis.AnnotationKeyPluginTackingID]
-		if !ok || trackingValue == "" {
-			// No tracking annotation, nothing to remove
-			return nil
-		}
-
-		// spread trackers and remove specified one
-		trackers := strings.Split(trackingValue, trackingSeparator)
-		updatedTrackers := make([]string, 0, len(trackers))
-		for _, t := range trackers {
-			if t != tracker {
-				updatedTrackers = append(updatedTrackers, t)
-			}
-		}
-
-		switch {
-		case len(updatedTrackers) == 0:
-			// no trackers, remove the annotation entirely
-			delete(annotations, greenhouseapis.AnnotationKeyPluginTackingID)
-			log.FromContext(ctx).Info("removed tracking annotation from resource",
-				"kind", kind,
-				"namespace", namespace,
-				"name", name,
-				"tracker", tracker)
-
-		case len(updatedTrackers) < len(trackers):
-			// trackers remaining, update the annotation
-			annotations[greenhouseapis.AnnotationKeyPluginTackingID] = strings.Join(updatedTrackers, trackingSeparator)
-			log.FromContext(ctx).Info("removed tracker from resource",
-				"kind", kind,
-				"namespace", namespace,
-				"name", name,
-				"tracker", tracker)
-
-		default:
-			// no tracker found
-			return nil
-		}
-
-		uObject.SetAnnotations(annotations)
-		return c.Update(ctx, uObject)
-	})
-}
-
 // buildGVK constructs a GroupVersionKind for Greenhouse resources.
 func buildGVK(kind string) schema.GroupVersionKind {
 	return schema.GroupVersionKind{

internal/controller/plugin/pluginpreset_controller_test.go

@@ -901,14 +901,14 @@ var _ = Describe("PluginPreset Controller Lifecycle", Ordered, func() {
 	It("should resolve a simple expression using clusterName", func() {
 		By("creating a PluginPreset with an expression")
 		expressionStr := `"app-${global.greenhouse.clusterName}.example.com"`
-		pluginSpec := greenhousev1alpha1.PluginSpec{
+		presetPluginSpec := greenhousev1alpha1.PluginPresetPluginSpec{
 			PluginDefinitionRef: greenhousev1alpha1.PluginDefinitionReference{
 				Kind: greenhousev1alpha1.ClusterPluginDefinitionKind,
 				Name: pluginPresetDefinitionName,
 			},
 			ReleaseName:      releaseName,
 			ReleaseNamespace: releaseNamespace,
-			OptionValues: []greenhousev1alpha1.PluginOptionValue{
+			OptionValues: []greenhousev1alpha1.PluginPresetPluginOptionValue{
 				{
 					Name:  "myRequiredOption",
 					Value: test.MustReturnJSONFor("myValue"),
@@ -922,7 +922,7 @@ var _ = Describe("PluginPreset Controller Lifecycle", Ordered, func() {
 
 		pluginPreset := test.NewPluginPreset("expr-simple", test.TestNamespace,
 			test.WithPluginPresetLabel(greenhouseapis.LabelKeyOwnedBy, testTeam.Name),
-			test.WithPluginPresetPluginSpec(pluginSpec),
+			test.WithPresetPluginSpec(presetPluginSpec),
 			test.WithPluginPresetClusterSelector(metav1.LabelSelector{
 				MatchLabels: map[string]string{
 					"cluster": clusterA,
@@ -941,7 +941,6 @@ var _ = Describe("PluginPreset Controller Lifecycle", Ordered, func() {
 			for _, ov := range expPlugin.Spec.OptionValues {
 				if ov.Name == "test.hostname" {
 					hostnameFound = true
-					g.Expect(ov.Expression).To(BeNil(), "Expression should be resolved")
 					g.Expect(ov.Value).ToNot(BeNil(), "Value should be set")
 					g.Expect(string(ov.Value.Raw)).To(Equal(`"app-`+clusterA+`.example.com"`),
 						"Expression should resolve with cluster name")
@@ -969,14 +968,14 @@ var _ = Describe("PluginPreset Controller Lifecycle", Ordered, func() {
 
 		By("creating a PluginPreset with metadata expression")
 		expressionStr := `"service.${global.greenhouse.metadata.region}.example.com"`
-		pluginSpec := greenhousev1alpha1.PluginSpec{
+		presetPluginSpec := greenhousev1alpha1.PluginPresetPluginSpec{
 			PluginDefinitionRef: greenhousev1alpha1.PluginDefinitionReference{
 				Kind: greenhousev1alpha1.ClusterPluginDefinitionKind,
 				Name: pluginPresetDefinitionName,
 			},
 			ReleaseName:      releaseName,
 			ReleaseNamespace: releaseNamespace,
-			OptionValues: []greenhousev1alpha1.PluginOptionValue{
+			OptionValues: []greenhousev1alpha1.PluginPresetPluginOptionValue{
 				{
 					Name:  "myRequiredOption",
 					Value: test.MustReturnJSONFor("myValue"),
@@ -990,7 +989,7 @@ var _ = Describe("PluginPreset Controller Lifecycle", Ordered, func() {
 
 		pluginPreset := test.NewPluginPreset("expr-metadata", test.TestNamespace,
 			test.WithPluginPresetLabel(greenhouseapis.LabelKeyOwnedBy, testTeam.Name),
-			test.WithPluginPresetPluginSpec(pluginSpec),
+			test.WithPresetPluginSpec(presetPluginSpec),
 			test.WithPluginPresetClusterSelector(metav1.LabelSelector{
 				MatchLabels: map[string]string{
 					"cluster": clusterA,
@@ -1032,14 +1031,14 @@ var _ = Describe("PluginPreset Controller Lifecycle", Ordered, func() {
 
 	It("should keep direct values unchanged when resolving expressions", func() {
 		expressionStr := `"generated-${global.greenhouse.clusterName}"`
-		pluginSpec := greenhousev1alpha1.PluginSpec{
+		presetPluginSpec := greenhousev1alpha1.PluginPresetPluginSpec{
 			PluginDefinitionRef: greenhousev1alpha1.PluginDefinitionReference{
 				Kind: greenhousev1alpha1.ClusterPluginDefinitionKind,
 				Name: pluginPresetDefinitionName,
 			},
 			ReleaseName:      releaseName,
 			ReleaseNamespace: releaseNamespace,
-			OptionValues: []greenhousev1alpha1.PluginOptionValue{
+			OptionValues: []greenhousev1alpha1.PluginPresetPluginOptionValue{
 				{
 					Name:  "myRequiredOption",
 					Value: test.MustReturnJSONFor("myValue"),
@@ -1057,7 +1056,7 @@ var _ = Describe("PluginPreset Controller Lifecycle", Ordered, func() {
 
 		pluginPreset := test.NewPluginPreset("expr-mixed", test.TestNamespace,
 			test.WithPluginPresetLabel(greenhouseapis.LabelKeyOwnedBy, testTeam.Name),
-			test.WithPluginPresetPluginSpec(pluginSpec),
+			test.WithPresetPluginSpec(presetPluginSpec),
 			test.WithPluginPresetClusterSelector(metav1.LabelSelector{
 				MatchLabels: map[string]string{
 					"cluster": clusterA,
@@ -1094,14 +1093,14 @@ var _ = Describe("PluginPreset Controller Lifecycle", Ordered, func() {
 
 	It("should report error for invalid expression", func() {
 		invalidExpressionStr := `"service.${global.greenhouse.nonexistent.field}.example.com"`
-		pluginSpec := greenhousev1alpha1.PluginSpec{
+		presetPluginSpec := greenhousev1alpha1.PluginPresetPluginSpec{
 			PluginDefinitionRef: greenhousev1alpha1.PluginDefinitionReference{
 				Kind: greenhousev1alpha1.ClusterPluginDefinitionKind,
 				Name: pluginPresetDefinitionName,
 			},
 			ReleaseName:      releaseName,
 			ReleaseNamespace: releaseNamespace,
-			OptionValues: []greenhousev1alpha1.PluginOptionValue{
+			OptionValues: []greenhousev1alpha1.PluginPresetPluginOptionValue{
 				{
 					Name:  "myRequiredOption",
 					Value: test.MustReturnJSONFor("myValue"),
@@ -1115,7 +1114,7 @@ var _ = Describe("PluginPreset Controller Lifecycle", Ordered, func() {
 
 		pluginPreset := test.NewPluginPreset("expr-invalid", test.TestNamespace,
 			test.WithPluginPresetLabel(greenhouseapis.LabelKeyOwnedBy, testTeam.Name),
-			test.WithPluginPresetPluginSpec(pluginSpec),
+			test.WithPresetPluginSpec(presetPluginSpec),
 			test.WithPluginPresetClusterSelector(metav1.LabelSelector{
 				MatchLabels: map[string]string{
 					"cluster": clusterA,

internal/test/resources.go

@@ -490,6 +490,12 @@ func WithPluginPresetPluginSpec(pluginSpec greenhousev1alpha1.PluginSpec) func(*
 	}
 }
 
+func WithPresetPluginSpec(spec greenhousev1alpha1.PluginPresetPluginSpec) func(*greenhousev1alpha1.PluginPreset) {
+	return func(pp *greenhousev1alpha1.PluginPreset) {
+		pp.Spec.Plugin = spec
+	}
+}
+
 // WithPluginPresetLabel sets the label on a PluginPreset
 func WithPluginPresetLabel(key, value string) func(*greenhousev1alpha1.PluginPreset) {
 	return func(pp *greenhousev1alpha1.PluginPreset) {