From a679cd3d7cb3bbd271d93abde79b9c876b2e190b Mon Sep 17 00:00:00 2001 From: Cameron Garrison Date: Fri, 26 Apr 2024 11:23:02 -0400 Subject: [PATCH] feat: provide option to control reconciliation when creating resources (#974) * add reconiliation option to feature+createResource * fix typo in test * extract markAsManaged Co-authored-by: Bartosz Majsak * 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 --- pkg/feature/builder.go | 8 + pkg/feature/feature.go | 11 +- pkg/feature/manifest.go | 28 ++++ pkg/feature/raw_resources.go | 31 ++-- pkg/metadata/annotations/annotations.go | 2 +- .../fixtures/cluster_test_fixtures.go | 4 +- .../fixtures/templates/managed-svc.yaml | 16 ++ .../features/manifests_int_test.go | 1 - .../features/resources_int_test.go | 142 ++++++++++++++++++ 9 files changed, 225 insertions(+), 18 deletions(-) create mode 100644 tests/integration/features/fixtures/templates/managed-svc.yaml create mode 100644 tests/integration/features/resources_int_test.go diff --git a/pkg/feature/builder.go b/pkg/feature/builder.go index a8eaa9beebf..734945412a6 100644 --- a/pkg/feature/builder.go +++ b/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 +} diff --git a/pkg/feature/feature.go b/pkg/feature/feature.go index f667e9bdcb7..590c02e4f77 100644 --- a/pkg/feature/feature.go +++ b/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) } diff --git a/pkg/feature/manifest.go b/pkg/feature/manifest.go index c579bdec46d..390629b8a74 100644 --- a/pkg/feature/manifest.go +++ b/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 diff --git a/pkg/feature/raw_resources.go b/pkg/feature/raw_resources.go index 05898cb248b..3b88b42e82c 100644 --- a/pkg/feature/raw_resources.go +++ b/pkg/feature/raw_resources.go @@ -17,24 +17,23 @@ 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 } } @@ -42,20 +41,26 @@ func createResources(cli client.Client, objects []*unstructured.Unstructured, me 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 } diff --git a/pkg/metadata/annotations/annotations.go b/pkg/metadata/annotations/annotations.go index 535b55fe3ed..e2d4cfde922 100644 --- a/pkg/metadata/annotations/annotations.go +++ b/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. diff --git a/tests/integration/features/fixtures/cluster_test_fixtures.go b/tests/integration/features/fixtures/cluster_test_fixtures.go index 16762d98b45..77195fe33c4 100644 --- a/tests/integration/features/fixtures/cluster_test_fixtures.go +++ b/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 }) diff --git a/tests/integration/features/fixtures/templates/managed-svc.yaml b/tests/integration/features/fixtures/templates/managed-svc.yaml new file mode 100644 index 00000000000..09659e5a437 --- /dev/null +++ b/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 diff --git a/tests/integration/features/manifests_int_test.go b/tests/integration/features/manifests_int_test.go index e2882ccdb3d..803bc067d9d 100644 --- a/tests/integration/features/manifests_int_test.go +++ b/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")) }) - }) diff --git a/tests/integration/features/resources_int_test.go b/tests/integration/features/resources_int_test.go new file mode 100644 index 00000000000..078508b01a6 --- /dev/null +++ b/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)) +}