feat: provide option to control reconciliation when creating resources (

opendatahub-io#974)

* add reconiliation option to feature+createResource

* fix typo in test

* extract markAsManaged

Co-authored-by: Bartosz Majsak <[email protected]>

* remove dead const

* re-add fixture, rework MarkAsManaged defn

* refactor/rename ApplyResources

* rework testing, unexport applyResources

* error out faster in applyResources

---------

Co-authored-by: Bartosz Majsak <[email protected]>

pkg/feature/builder.go

@@ -25,6 +25,7 @@ type featureBuilder struct {
 	featuresHandler *FeaturesHandler
 	fsys            fs.FS
 	targetNS        string
+	managed         bool
 }
 
 func CreateFeature(name string) *usingFeaturesHandler { //nolint:golint,revive //No need to export featureBuilder.
@@ -188,6 +189,7 @@ func (fb *featureBuilder) Load() error {
 
 	feature.Spec.TargetNamespace = fb.targetNS
 	feature.fsys = fb.fsys
+	feature.Managed = fb.managed
 
 	fb.featuresHandler.features = append(fb.featuresHandler.features, feature)
 
@@ -220,3 +222,9 @@ func (fb *featureBuilder) TargetNamespace(targetNs string) *featureBuilder {
 
 	return fb
 }
+
+func (fb *featureBuilder) Managed() *featureBuilder {
+	fb.managed = true
+
+	return fb
+}

pkg/feature/feature.go

@@ -22,6 +22,7 @@ type Feature struct {
 	Name    string
 	Spec    *Spec
 	Enabled bool
+	Managed bool
 	Tracker *featurev1.FeatureTracker
 
 	Client client.Client
@@ -104,6 +105,10 @@ func (f *Feature) applyFeature() error {
 			return &withConditionReasonError{reason: featurev1.ConditionReason.ApplyManifests, err: processErr}
 		}
 
+		if f.Managed {
+			manifest.MarkAsManaged(objs)
+		}
+
 		if err := apply(objs); err != nil {
 			return &withConditionReasonError{reason: featurev1.ConditionReason.ApplyManifests, err: err}
 		}
@@ -155,7 +160,7 @@ func (f *Feature) createApplier(m Manifest) applier {
 	}
 
 	return func(objects []*unstructured.Unstructured) error {
-		return createResources(f.Client, objects, OwnedBy(f))
+		return applyResources(f.Client, objects, OwnedBy(f))
 	}
 }
 
@@ -177,6 +182,10 @@ func (f *Feature) ApplyManifest(path string) error {
 			return errors.WithStack(err)
 		}
 
+		if f.Managed {
+			manifest.MarkAsManaged(objs)
+		}
+
 		if err = apply(objs); err != nil {
 			return errors.WithStack(err)
 		}

pkg/feature/manifest.go

@@ -12,11 +12,15 @@ 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 {
 	// Process allows any arbitrary struct to be passed and used while processing the content of the manifest.
 	Process(data any) ([]*unstructured.Unstructured, error)
+	// MarkAsManaged sets all non-patch objects to be managed/reconciled by setting the annotation.
+	MarkAsManaged(objects []*unstructured.Unstructured)
 }
 
 type rawManifest struct {
@@ -44,6 +48,12 @@ func (b *rawManifest) Process(_ any) ([]*unstructured.Unstructured, error) {
 	return convertToUnstructuredSlice(resources)
 }
 
+func (b *rawManifest) MarkAsManaged(objects []*unstructured.Unstructured) {
+	if !b.patch {
+		markAsManaged(objects)
+	}
+}
+
 var _ Manifest = (*templateManifest)(nil)
 
 type templateManifest struct {
@@ -83,6 +93,24 @@ func (t *templateManifest) Process(data any) ([]*unstructured.Unstructured, erro
 	return convertToUnstructuredSlice(resources)
 }
 
+func (t *templateManifest) MarkAsManaged(objects []*unstructured.Unstructured) {
+	if !t.patch {
+		markAsManaged(objects)
+	}
+}
+
+func markAsManaged(objs []*unstructured.Unstructured) {
+	for _, obj := range objs {
+		objAnnotations := obj.GetAnnotations()
+		if objAnnotations == nil {
+			objAnnotations = make(map[string]string)
+		}
+
+		objAnnotations[annotations.ManagedByODHOperator] = "true"
+		obj.SetAnnotations(objAnnotations)
+	}
+}
+
 func loadManifestsFrom(fsys fs.FS, path string) ([]Manifest, error) {
 	var manifests []Manifest
 

pkg/feature/raw_resources.go

@@ -17,45 +17,50 @@ import (
 	"context"
 	"fmt"
 
-	"github.com/pkg/errors"
-	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	k8stypes "k8s.io/apimachinery/pkg/types"
 	"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"
 )
 
 const (
 	YamlSeparator = "(?m)^---[ \t]*$"
 )
 
-func createResources(cli client.Client, objects []*unstructured.Unstructured, metaOptions ...cluster.MetaOptions) error {
+func applyResources(cli client.Client, objects []*unstructured.Unstructured, metaOptions ...cluster.MetaOptions) error {
 	for _, object := range objects {
 		for _, opt := range metaOptions {
 			if err := opt(object); err != nil {
-				return err // return immediately if any of the MetaOptions functions fail
+				return err
 			}
 		}
 
 		name := object.GetName()
 		namespace := object.GetNamespace()
 
 		err := cli.Get(context.TODO(), k8stypes.NamespacedName{Name: name, Namespace: namespace}, object.DeepCopy())
-		if err == nil {
-			// object already exists, skip reconcile allowing users to tweak it
-			continue
-		}
-		if !k8serrors.IsNotFound(err) {
-			return errors.WithStack(err)
+		if client.IgnoreNotFound(err) != nil {
+			return fmt.Errorf("failed to get object %s/%s: %w", namespace, name, err)
 		}
 
-		err = cli.Create(context.TODO(), object)
 		if err != nil {
-			return errors.WithStack(err)
+			// object does not exist and should be created
+			if createErr := cli.Create(context.TODO(), object); client.IgnoreAlreadyExists(createErr) != nil {
+				return fmt.Errorf("failed to create object %s/%s: %w", namespace, name, createErr)
+			}
 		}
+		// object exists, check if it is managed
+		isManaged, isAnnotated := object.GetAnnotations()[annotations.ManagedByODHOperator]
+		if isAnnotated && isManaged == "true" {
+			// update the object since we manage it
+			if updateErr := cli.Update(context.TODO(), object); updateErr != nil {
+				return fmt.Errorf("failed to update object %s/%s: %w", namespace, name, updateErr)
+			}
+		}
+		// object exists and is not manged, skip reconcile allowing users to tweak it
 	}
-
 	return nil
 }
 

pkg/metadata/annotations/annotations.go

@@ -1,6 +1,6 @@
 package annotations
 
-// skip reconcile.
+// ManagedByODHOperator is used to denote if a resource/component should be reconciled - when true, reconcile.
 const ManagedByODHOperator = "opendatahub.io/managed"
 
 // trust CA bundler.

tests/integration/features/fixtures/cluster_test_fixtures.go

@@ -23,13 +23,13 @@ func CreateSubscription(client client.Client, namespace, subscriptionYaml string
 	}
 
 	ns := NewNamespace(namespace)
-	if err := createOrUpdateNamespace(client, ns); err != nil {
+	if err := CreateOrUpdateNamespace(client, ns); err != nil {
 		return err
 	}
 	return createOrUpdateSubscription(client, subscription)
 }
 
-func createOrUpdateNamespace(client client.Client, ns *v1.Namespace) error {
+func CreateOrUpdateNamespace(client client.Client, ns *v1.Namespace) error {
 	_, err := controllerutil.CreateOrUpdate(context.Background(), client, ns, func() error {
 		return nil
 	})

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

@@ -0,0 +1,16 @@
+apiVersion: v1
+kind: Service
+metadata:
+  name: managed-svc
+  namespace: "test-namespace"
+  annotations:
+    opendatahub.io/managed: "true"
+spec:
+  ports:
+    - name: http2
+      port: 80
+      protocol: TCP
+      targetPort: 8081
+  selector:
+    knative: ingressgateway
+  type: ClusterIP

tests/integration/features/manifests_int_test.go

@@ -122,5 +122,4 @@ metadata:
 		Expect(err).ToNot(HaveOccurred())
 		Expect(realNs.Name).To(Equal("real-file-test-ns"))
 	})
-
 })

tests/integration/features/resources_int_test.go

@@ -0,0 +1,142 @@
+package features_test
+
+import (
+	"context"
+	"path"
+
+	v1 "k8s.io/api/core/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	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/tests/envtestutil"
+	"github.com/opendatahub-io/opendatahub-operator/v2/tests/integration/features/fixtures"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Applying and updating resources", func() {
+	var (
+		testNamespace   string
+		namespace       *v1.Namespace
+		objectCleaner   *envtestutil.Cleaner
+		dsci            *dsciv1.DSCInitialization
+		dummyAnnotation string
+	)
+
+	BeforeEach(func() {
+		objectCleaner = envtestutil.CreateCleaner(envTestClient, envTest.Config, fixtures.Timeout, fixtures.Interval)
+
+		testNamespace = "test-namespace"
+		dummyAnnotation = "fake-anno"
+
+		var err error
+		namespace, err = cluster.CreateNamespace(envTestClient, testNamespace)
+		Expect(err).ToNot(HaveOccurred())
+
+		dsci = fixtures.NewDSCInitialization(testNamespace)
+		dsci.Spec.ServiceMesh.ControlPlane.Namespace = namespace.Name
+	})
+
+	When("a feature is managed", func() {
+		It("should reconcile the object to its managed state", func() {
+			// given managed feature
+			featuresHandler := createAndApplyFeature(dsci, true, "create-local-gw-svc", "local-gateway-svc.tmpl.yaml")
+
+			// expect created svc to have managed annotation
+			service := getServiceAndExpectAnnotations(envTestClient, testNamespace, "knative-local-gateway", map[string]string{
+				"example-annotation":             "",
+				annotations.ManagedByODHOperator: "true",
+			})
+
+			// modify managed service
+			modifyAndExpectUpdate(envTestClient, service, "example-annotation", dummyAnnotation)
+
+			// expect that modification is reconciled away
+			Expect(featuresHandler.Apply()).To(Succeed())
+			verifyAnnotation(envTestClient, testNamespace, service.Name, "example-annotation", "")
+		})
+	})
+
+	When("a feature is unmanaged", func() {
+		It("should not reconcile the object", func() {
+			// given unmanaged feature
+			featuresHandler := createAndApplyFeature(dsci, false, "create-local-gw-svc", "local-gateway-svc.tmpl.yaml")
+
+			// modify unmanaged service object
+			service, err := fixtures.GetService(envTestClient, testNamespace, "knative-local-gateway")
+			Expect(err).ToNot(HaveOccurred())
+			modifyAndExpectUpdate(envTestClient, service, "example-annotation", dummyAnnotation)
+
+			// expect modification to remain after "reconcile"
+			Expect(featuresHandler.Apply()).To(Succeed())
+			verifyAnnotation(envTestClient, testNamespace, service.Name, "example-annotation", dummyAnnotation)
+		})
+	})
+
+	When("a feature is unmanaged but the object is marked as managed", func() {
+		It("should reconcile this object", func() {
+			// given unmanaged feature but object marked with managed annotation
+			featuresHandler := createAndApplyFeature(dsci, false, "create-managed-svc", "managed-svc.yaml")
+
+			// expect service to have managed annotation
+			service := getServiceAndExpectAnnotations(envTestClient, testNamespace, "managed-svc", map[string]string{
+				"example-annotation":             "",
+				annotations.ManagedByODHOperator: "true",
+			})
+
+			// modify managed service
+			modifyAndExpectUpdate(envTestClient, service, "example-annotation", dummyAnnotation)
+
+			// expect that modification is reconciled away
+			Expect(featuresHandler.Apply()).To(Succeed())
+			verifyAnnotation(envTestClient, testNamespace, service.Name, "example-annotation", "")
+		})
+	})
+
+	AfterEach(func() {
+		objectCleaner.DeleteAll(namespace)
+	})
+})
+
+func createAndApplyFeature(dsci *dsciv1.DSCInitialization, managed bool, featureName, yamlFile string) *feature.FeaturesHandler {
+	featuresHandler := feature.ClusterFeaturesHandler(dsci, func(handler *feature.FeaturesHandler) error {
+		creator := feature.CreateFeature(featureName).
+			For(handler).
+			UsingConfig(envTest.Config).
+			ManifestSource(fixtures.TestEmbeddedFiles).
+			Manifests(path.Join(fixtures.BaseDir, yamlFile))
+		if managed {
+			creator.Managed()
+		}
+		return creator.Load()
+	})
+	Expect(featuresHandler.Apply()).To(Succeed())
+	return featuresHandler
+}
+
+func getServiceAndExpectAnnotations(testClient client.Client, namespace, serviceName string, annotations map[string]string) *v1.Service {
+	service, err := fixtures.GetService(testClient, namespace, serviceName)
+	Expect(err).ToNot(HaveOccurred())
+	for key, val := range annotations {
+		Expect(service.Annotations[key]).To(Equal(val))
+	}
+	return service
+}
+
+func modifyAndExpectUpdate(client client.Client, service *v1.Service, annotationKey, newValue string) {
+	if service.Annotations == nil {
+		service.Annotations = make(map[string]string)
+	}
+	service.Annotations[annotationKey] = newValue
+	Expect(client.Update(context.Background(), service)).To(Succeed())
+}
+
+func verifyAnnotation(client client.Client, namespace, serviceName, annotationKey, expectedValue string) {
+	updatedService, err := fixtures.GetService(client, namespace, serviceName)
+	Expect(err).ToNot(HaveOccurred())
+	Expect(updatedService.Annotations[annotationKey]).To(Equal(expectedValue))
+}