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

…ahub-io#1103)

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.

`opendatahub.io/managed` is already exposed as an annotation and used by [other components](https://docs.redhat.com/en/documentation/red_hat_openshift_ai_self-managed/2.10/html/installing_and_uninstalling_openshift_ai_self-managed/preparing-openshift-ai-for-ibm-cpd_prepare-openshift-ai-ibm-cpd#editing-model-inferencing-configuration-ibm-cpd_prepare-openshift-ai-ibm-cpd).

Follow up to opendatahub-io#974

(cherry picked from commit e12d7d7)

pkg/deploy/deploy.go

@@ -46,6 +46,7 @@ import (
 	"sigs.k8s.io/kustomize/kyaml/filesys"
 
 	"github.com/opendatahub-io/opendatahub-operator/v2/components"
+	"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/pkg/plugins"
 )
@@ -222,7 +223,7 @@ func manageResource(ctx context.Context, cli client.Client, obj *unstructured.Un
 		if enabled {
 			// Exception to not update kserve with managed annotation
 			// do not reconcile kserve resource with annotation "opendatahub.io/managed: false"
-			if found.GetAnnotations()["opendatahub.io/managed"] == "false" && componentName == "kserve" {
+			if found.GetAnnotations()[annotations.ManagedByODHOperator] == "false" && componentName == "kserve" {
 				return nil
 			}
 			return updateResource(ctx, cli, obj, found, owner, componentName)

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 {
@@ -108,21 +106,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

@@ -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 {
+		objAnnotations := obj.GetAnnotations()
+		if objAnnotations == nil {
+			objAnnotations = make(map[string]string)
+		}
+
+		// If resource already has management mode defined, it should take precedence
+		if _, exists := objAnnotations[annotations.ManagedByODHOperator]; !exists {
+			objAnnotations[annotations.ManagedByODHOperator] = "true"
+			obj.SetAnnotations(objAnnotations)
+		}
+	}
+}
+
+func isUnmanaged(obj *unstructured.Unstructured) bool {
+	managed, isDefined := obj.GetAnnotations()[annotations.ManagedByODHOperator]
+	return isDefined && managed == "false"
+}
+
+func isManaged(obj *unstructured.Unstructured) bool {
+	managed, isDefined := obj.GetAnnotations()[annotations.ManagedByODHOperator]
+	return isDefined && managed == "true"
+}

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

@@ -2,7 +2,7 @@ apiVersion: v1
 kind: Service
 metadata:
   annotations:
-    test-annotation: "original-value"
+    test: "original-value"
   labels:
     experimental.istio.io/disable-gateway-port-translation: "true"
   name: knative-local-gateway

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

@@ -5,7 +5,7 @@ metadata:
   namespace: {{ .TargetNamespace }}
   annotations:
     opendatahub.io/managed: "true"
-    test-annotation: "original-value"
+    test: "original-value"
 spec:
   ports:
     - name: http2

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

@@ -5,7 +5,7 @@ metadata:
   namespace: {{ .TargetNamespace }}
   annotations:
     opendatahub.io/managed: "false"
-    test-annotation: "original-value"
+    test: "original-value"
 spec:
   ports:
     - name: http2

tests/integration/features/resources_int_test.go

@@ -2,6 +2,7 @@ package features_test
 
 import (
 	"context"
+	"fmt"
 	"path"
 
 	corev1 "k8s.io/api/core/v1"
@@ -26,9 +27,9 @@ var _ = Describe("Applying and updating resources", func() {
 	)
 
 	const (
-		testAnnotationKey = "test-annotation"
-		newValue          = "new-value"
-		originalValue     = "original-value"
+		testKey           = "test"
+		testNewValue      = "new-value"
+		testOriginalValue = "original-value"
 	)
 
 	BeforeEach(func(ctx context.Context) {
@@ -71,7 +72,7 @@ var _ = Describe("Applying and updating resources", func() {
 			)
 
 			// when
-			service.Annotations[testAnnotationKey] = newValue
+			service.Annotations[testKey] = testNewValue
 			Expect(envTestClient.Update(ctx, service)).To(Succeed())
 
 			// then
@@ -80,7 +81,7 @@ var _ = Describe("Applying and updating resources", func() {
 			updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway")
 			Expect(err).ToNot(HaveOccurred())
 			Expect(updatedService.Annotations).To(
-				HaveKeyWithValue(testAnnotationKey, originalValue),
+				HaveKeyWithValue(testKey, testOriginalValue),
 			)
 		})
 
@@ -101,7 +102,7 @@ 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.Annotations[testKey] = testNewValue
 			Expect(envTestClient.Update(ctx, service)).To(Succeed())
 
 			// then
@@ -110,7 +111,7 @@ var _ = Describe("Applying and updating resources", func() {
 			updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "unmanaged-svc")
 			Expect(err).ToNot(HaveOccurred())
 			Expect(updatedService.Annotations).To(
-				HaveKeyWithValue(testAnnotationKey, newValue),
+				HaveKeyWithValue(testKey, testNewValue),
 			)
 		})
 
@@ -133,7 +134,7 @@ 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.Annotations[testKey] = testNewValue
 			Expect(envTestClient.Update(ctx, service)).To(Succeed())
 
 			// then
@@ -142,7 +143,7 @@ var _ = Describe("Applying and updating resources", func() {
 			updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "knative-local-gateway")
 			Expect(err).ToNot(HaveOccurred())
 			Expect(updatedService.Annotations).To(
-				HaveKeyWithValue(testAnnotationKey, newValue),
+				HaveKeyWithValue(testKey, testNewValue),
 			)
 
 		})
@@ -164,7 +165,10 @@ 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.Annotations[testKey] = testNewValue
+			service.Spec.ClusterIP = ""
+			service.Spec.Type = corev1.ServiceTypeExternalName
+			service.Spec.ExternalName = "test-external-name"
 			Expect(envTestClient.Update(ctx, service)).To(Succeed())
 
 			// then
@@ -173,8 +177,10 @@ var _ = Describe("Applying and updating resources", func() {
 			updatedService, err := fixtures.GetService(ctx, envTestClient, testNamespace, "managed-svc")
 			Expect(err).ToNot(HaveOccurred())
 			Expect(updatedService.Annotations).To(
-				HaveKeyWithValue(testAnnotationKey, originalValue),
+				HaveKeyWithValue(testKey, testOriginalValue),
 			)
+			Expect(updatedService.Spec.Type).To(Equal(corev1.ServiceTypeClusterIP))
+			fmt.Println(updatedService.Spec.ExternalName)
 		})
 	})