fix(feature): only relies on desired state for reconcilation

This change ensures that management state is based exclusively on a
resource defined in desired state, i.e. the one provided by the
controller itself. This can be, but is not limited to, manifest files
provided through embedded file system.

Although the actual instance on the cluster (the target) might be
in a different state, we intentionally do not address this scenario due
to the lack of clear requirements on the extent to which users can
modify such resource.

This change allows to take control over previously undefined resources,
so those which are created by `Feature` API without explicit management
state defined. As long as original Feature does not have `Managed()`
called in the builder chain, nor used manifests have the label
explicitly defined the behaviour remain unchanged, that means the
resources being part of such a feature are not managed and thus not
reconciled after initial apply.

To be aligned with how `opendatahub.io/managed` is used by other
components, this change makes it a `label`, not an `annotation` as it
was previously defined.

Follow up to #974

pkg/feature/manifest.go

@@ -12,8 +12,6 @@ import (
 
 	"github.com/ghodss/yaml"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-
-	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations"
 )
 
 type Manifest interface {
@@ -99,21 +97,6 @@ func (t *templateManifest) MarkAsManaged(objects []*unstructured.Unstructured) {
 	}
 }
 
-func markAsManaged(objs []*unstructured.Unstructured) {
-	for _, obj := range objs {
-		objAnnotations := obj.GetAnnotations()
-		if objAnnotations == nil {
-			objAnnotations = make(map[string]string)
-		}
-
-		// If resource already has an annotation, leave it as defined
-		if _, exists := objAnnotations[annotations.ManagedByODHOperator]; !exists {
-			objAnnotations[annotations.ManagedByODHOperator] = "true"
-			obj.SetAnnotations(objAnnotations)
-		}
-	}
-}
-
 func loadManifestsFrom(fsys fs.FS, path string) ([]Manifest, error) {
 	var manifests []Manifest
 

pkg/feature/raw_resources.go

@@ -23,7 +23,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster"
-	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations"
+	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels"
 )
 
 const (
@@ -56,14 +56,11 @@ func applyResources(ctx context.Context, cli client.Client, objects []*unstructu
 			return nil
 		}
 
-		isManaged, isAnnotated := target.GetAnnotations()[annotations.ManagedByODHOperator]
-		if isAnnotated && isManaged == "true" {
-			// reconcile the target since we manage it
+		if shouldReconcile(source) {
 			if errUpdate := patchUsingApplyStrategy(ctx, cli, source, target); errUpdate != nil {
-				return fmt.Errorf("failed to update source %s/%s: %w", namespace, name, errUpdate)
+				return fmt.Errorf("failed to reconcile resource %s/%s: %w", namespace, name, errUpdate)
 			}
 		}
-		// source exists and is not manged, skip reconcile allowing users to tweak it
 	}
 
 	return nil
@@ -105,3 +102,50 @@ func patchUsingMergeStrategy(ctx context.Context, cli client.Client, patch *unst
 
 	return nil
 }
+
+// At this point we only look at what is defined for the resource in the desired state (source),
+// so the one provided by the operator (e.g. embedded manifest files).
+//
+// Although the actual instance on the cluster (the target) might be in a different state,
+// we intentionally do not address this scenario due to the lack of clear requirements
+// on the extent to which users can modify it. Additionally, addressing this would
+// necessitate extra reporting (e.g., via status.conditions on a FeatureTracker)
+// to highlight discrepancies between the actual and desired state when users opt out
+// of operator management/reconciliation.
+func shouldReconcile(source *unstructured.Unstructured) bool {
+	if isManaged(source) {
+		return true
+	}
+
+	if isUnmanaged(source) {
+		return false
+	}
+
+	// In all the other cases preserve original behaviour
+	return false
+}
+
+func markAsManaged(objs []*unstructured.Unstructured) {
+	for _, obj := range objs {
+		objLabels := obj.GetLabels()
+		if objLabels == nil {
+			objLabels = make(map[string]string)
+		}
+
+		// If resource already has the label defined it should take precedence
+		if _, exists := objLabels[labels.ManagedByODHOperator]; !exists {
+			objLabels[labels.ManagedByODHOperator] = "true"
+			obj.SetLabels(objLabels)
+		}
+	}
+}
+
+func isUnmanaged(obj *unstructured.Unstructured) bool {
+	managed, isLabeled := obj.GetLabels()[labels.ManagedByODHOperator]
+	return isLabeled && managed == "false"
+}
+
+func isManaged(obj *unstructured.Unstructured) bool {
+	managed, isLabeled := obj.GetLabels()[labels.ManagedByODHOperator]
+	return isLabeled && managed == "true"
+}

pkg/metadata/annotations/annotations.go

@@ -1,8 +1,5 @@
 package annotations
 
-// ManagedByODHOperator is used to denote if a resource/component should be reconciled - when true, reconcile.
-const ManagedByODHOperator = "opendatahub.io/managed"
-
 // trust CA bundler.
 const InjectionOfCABundleAnnotatoion = "security.opendatahub.io/inject-trusted-ca-bundle"
 

pkg/metadata/labels/types.go

@@ -1,10 +1,12 @@
 package labels
 
 const (
-	ODHAppPrefix      = "app.opendatahub.io"
-	InjectTrustCA     = "config.openshift.io/inject-trusted-cabundle"
-	SecurityEnforce   = "pod-security.kubernetes.io/enforce"
-	ClusterMonitoring = "openshift.io/cluster-monitoring"
+	// ManagedByODHOperator is used to denote if a resource/component should be reconciled - when true, reconcile.
+	ManagedByODHOperator = "opendatahub.io/managed"
+	ODHAppPrefix         = "app.opendatahub.io"
+	InjectTrustCA        = "config.openshift.io/inject-trusted-cabundle"
+	SecurityEnforce      = "pod-security.kubernetes.io/enforce"
+	ClusterMonitoring    = "openshift.io/cluster-monitoring"
 )
 
 // K8SCommon keeps common kubernetes labels [1]

tests/integration/features/fixtures/templates/local-gateway-svc.tmpl.yaml

@@ -1,9 +1,8 @@
 apiVersion: v1
 kind: Service
 metadata:
-  annotations:
-    test-annotation: "original-value"
   labels:
+    test-label: "original-value"
     experimental.istio.io/disable-gateway-port-translation: "true"
   name: knative-local-gateway
   namespace: {{ .ControlPlane.Namespace }}

tests/integration/features/fixtures/templates/managed-svc.tmpl.yaml

@@ -3,9 +3,9 @@ kind: Service
 metadata:
   name: managed-svc
   namespace: {{ .TargetNamespace }}
-  annotations:
+  labels:
     opendatahub.io/managed: "true"
-    test-annotation: "original-value"
+    test-label: "original-value"
 spec:
   ports:
     - name: http2

tests/integration/features/fixtures/templates/unmanaged-svc.tmpl.yaml

@@ -3,9 +3,9 @@ kind: Service
 metadata:
   name: unmanaged-svc
   namespace: {{ .TargetNamespace }}
-  annotations:
+  labels:
     opendatahub.io/managed: "false"
-    test-annotation: "original-value"
+    test-label: "original-value"
 spec:
   ports:
     - name: http2

tests/integration/features/resources_int_test.go

@@ -2,14 +2,15 @@ package features_test
 
 import (
 	"context"
+	"fmt"
 	"path"
 
 	corev1 "k8s.io/api/core/v1"
 
 	dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1"
 	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster"
 	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature"
-	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations"
+	"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels"
 	"github.com/opendatahub-io/opendatahub-operator/v2/tests/envtestutil"
 	"github.com/opendatahub-io/opendatahub-operator/v2/tests/integration/features/fixtures"
 
@@ -26,9 +27,9 @@ var _ = Describe("Applying and updating resources", func() {
 	)
 
 	const (
-		testAnnotationKey = "test-annotation"
-		newValue          = "new-value"
-		originalValue     = "original-value"
+		testLabelKey  = "test-label"
+		newValue      = "new-value"
+		originalValue = "original-value"
 	)
 
 	BeforeEach(func(ctx context.Context) {
@@ -66,21 +67,21 @@ var _ = Describe("Applying and updating resources", func() {
 			// expect created svc to have managed annotation
 			service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway")
 			Expect(err).ToNot(HaveOccurred())
-			Expect(service.Annotations).To(
-				HaveKeyWithValue(annotations.ManagedByODHOperator, "true"),
+			Expect(service.Labels).To(
+				HaveKeyWithValue(labels.ManagedByODHOperator, "true"),
 			)
 
 			// when
-			service.Annotations[testAnnotationKey] = newValue
+			service.Labels[testLabelKey] = newValue
 			Expect(envTestClient.Update(ctx, service)).To(Succeed())
 
 			// then
 			// expect that modification is reconciled away
 			Expect(featuresHandler.Apply(ctx)).To(Succeed())
 			updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway")
 			Expect(err).ToNot(HaveOccurred())
-			Expect(updatedService.Annotations).To(
-				HaveKeyWithValue(testAnnotationKey, originalValue),
+			Expect(updatedService.Labels).To(
+				HaveKeyWithValue(testLabelKey, originalValue),
 			)
 		})
 
@@ -101,16 +102,16 @@ var _ = Describe("Applying and updating resources", func() {
 			// when
 			service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "unmanaged-svc")
 			Expect(err).ToNot(HaveOccurred())
-			service.Annotations[testAnnotationKey] = newValue
+			service.Labels[testLabelKey] = newValue
 			Expect(envTestClient.Update(ctx, service)).To(Succeed())
 
 			// then
 			// expect that modification is reconciled away
 			Expect(featuresHandler.Apply(ctx)).To(Succeed())
 			updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "unmanaged-svc")
 			Expect(err).ToNot(HaveOccurred())
-			Expect(updatedService.Annotations).To(
-				HaveKeyWithValue(testAnnotationKey, newValue),
+			Expect(updatedService.Labels).To(
+				HaveKeyWithValue(testLabelKey, newValue),
 			)
 		})
 
@@ -133,16 +134,16 @@ var _ = Describe("Applying and updating resources", func() {
 			// when
 			service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway")
 			Expect(err).ToNot(HaveOccurred())
-			service.Annotations[testAnnotationKey] = newValue
+			service.Labels[testLabelKey] = newValue
 			Expect(envTestClient.Update(ctx, service)).To(Succeed())
 
 			// then
 			// expect that modification is reconciled away
 			Expect(featuresHandler.Apply(ctx)).To(Succeed())
 			updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway")
 			Expect(err).ToNot(HaveOccurred())
-			Expect(updatedService.Annotations).To(
-				HaveKeyWithValue(testAnnotationKey, newValue),
+			Expect(updatedService.Labels).To(
+				HaveKeyWithValue(testLabelKey, newValue),
 			)
 
 		})
@@ -164,17 +165,22 @@ var _ = Describe("Applying and updating resources", func() {
 			// when
 			service, err := fixtures.GetService(ctx, envTestClient, testNamespace, "managed-svc")
 			Expect(err).ToNot(HaveOccurred())
-			service.Annotations[testAnnotationKey] = newValue
+			service.Labels[testLabelKey] = newValue
+			service.Spec.ClusterIP = ""
+			service.Spec.Type = corev1.ServiceTypeExternalName
+			service.Spec.ExternalName = "test-external-name"
 			Expect(envTestClient.Update(ctx, service)).To(Succeed())
 
 			// then
 			// expect that modification is reconciled away
 			Expect(featuresHandler.Apply(ctx)).To(Succeed())
 			updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "managed-svc")
 			Expect(err).ToNot(HaveOccurred())
-			Expect(updatedService.Annotations).To(
-				HaveKeyWithValue(testAnnotationKey, originalValue),
+			Expect(updatedService.Labels).To(
+				HaveKeyWithValue(testLabelKey, originalValue),
 			)
+			Expect(updatedService.Spec.Type).To(Equal(corev1.ServiceTypeClusterIP))
+			fmt.Println(updatedService.Spec.ExternalName)
 		})
 	})