From 4e95a12c5bc9c31f4f770c228c6eaadcce2d3786 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Tue, 1 Oct 2024 13:40:40 +0200 Subject: [PATCH 01/15] Add label removal finalizer --- internal/declarative/v2/reconciler.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index d85bd32b27..8d5781884c 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -42,6 +42,7 @@ const ( CustomResourceManagerFinalizer = "resource.kyma-project.io/finalizer" SyncedOCIRefAnnotation = "sync-oci-ref" defaultFinalizer = "declarative.kyma-project.io/finalizer" + labelRemovalFinalizer = "label-removal-finalizer" defaultFieldOwner client.FieldOwner = "declarative.kyma-project.io/applier" ) @@ -150,7 +151,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if manifest.GetDeletionTimestamp().IsZero() { partialMeta := r.partialObjectMetadata(manifest) - if controllerutil.AddFinalizer(partialMeta, defaultFinalizer) { + defaultFinalizerAdded := controllerutil.AddFinalizer(partialMeta, defaultFinalizer) + labelRemovalFinalizerAdded := controllerutil.AddFinalizer(partialMeta, labelRemovalFinalizer) + if defaultFinalizerAdded || labelRemovalFinalizerAdded { return r.ssaSpec(ctx, partialMeta, metrics.ManifestAddFinalizer) } } @@ -236,7 +239,7 @@ func (r *Reconciler) cleanupManifest(ctx context.Context, req ctrl.Request, mani ) (ctrl.Result, error) { r.ManifestMetrics.RemoveManifestDuration(req.Name) r.cleanUpMandatoryModuleMetrics(manifest) - finalizersToRemove := []string{defaultFinalizer} + finalizersToRemove := []string{defaultFinalizer, labelRemovalFinalizer} if errors.Is(originalErr, ErrAccessSecretNotFound) || manifest.IsUnmanaged() { finalizersToRemove = manifest.GetFinalizers() } From e2c99e039794b86de514c797e94d0036f1241c10 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Tue, 1 Oct 2024 16:05:22 +0200 Subject: [PATCH 02/15] Handle labels deletion from resources and default CR --- internal/declarative/v2/reconciler.go | 109 +++++++++++++++++++++----- internal/pkg/metrics/manifest.go | 1 + 2 files changed, 92 insertions(+), 18 deletions(-) diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index 8d5781884c..86091cbd49 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -139,10 +139,32 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu r.MandatoryModuleMetrics.RecordMandatoryModuleState(kymaName, moduleName, state) } + skrClient, err := r.getTargetClient(ctx, manifest) + if err != nil { + if !manifest.GetDeletionTimestamp().IsZero() && errors.Is(err, ErrAccessSecretNotFound) { + return r.cleanupManifest(ctx, req, manifest, manifestStatus, metrics.ManifestClientInit, + err) + } + + manifest.SetStatus(manifest.GetStatus().WithState(shared.StateError).WithErr(err)) + return r.finishReconcile(ctx, manifest, metrics.ManifestClientInit, manifestStatus, err) + } + if manifest.IsUnmanaged() { if !manifest.GetDeletionTimestamp().IsZero() { return r.cleanupManifest(ctx, req, manifest, manifestStatus, metrics.ManifestUnmanagedUpdate, nil) } + + if controllerutil.ContainsFinalizer(manifest, labelRemovalFinalizer) { + if err := r.handleLabelsRemovalFromResources(ctx, manifest, skrClient); err != nil { + return ctrl.Result{}, err + } + + if controllerutil.RemoveFinalizer(manifest, labelRemovalFinalizer) { + return r.updateManifest(ctx, manifest, metrics.ManifestResourcesLabelRemoval) + } + } + if err := r.Delete(ctx, manifest); err != nil { return ctrl.Result{}, fmt.Errorf("manifestController: %w", err) } @@ -172,17 +194,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return r.updateManifest(ctx, manifest, metrics.ManifestInitSyncedOCIRef) } - skrClient, err := r.getTargetClient(ctx, manifest) - if err != nil { - if !manifest.GetDeletionTimestamp().IsZero() && errors.Is(err, ErrAccessSecretNotFound) { - return r.cleanupManifest(ctx, req, manifest, manifestStatus, metrics.ManifestClientInit, - err) - } - - manifest.SetStatus(manifest.GetStatus().WithState(shared.StateError).WithErr(err)) - return r.finishReconcile(ctx, manifest, metrics.ManifestClientInit, manifestStatus, err) - } - target, current, err := r.renderResources(ctx, skrClient, manifest, spec) if err != nil { if util.IsConnectionRelatedError(err) { @@ -453,11 +464,11 @@ func (r *Reconciler) renderTargetResources(ctx context.Context, skrClient client converter ResourceToInfoConverter, manifest *v1beta2.Manifest, spec *Spec, ) ([]*resource.Info, error) { if !manifest.GetDeletionTimestamp().IsZero() { - deleted, err := checkCRDeletion(ctx, skrClient, manifest) + defaultCR, err := fetchCR(ctx, skrClient, manifest) if err != nil { return nil, err } - if deleted { + if defaultCR == nil { return ResourceList{}, nil } } @@ -486,9 +497,10 @@ func (r *Reconciler) renderTargetResources(ctx context.Context, skrClient client return target, nil } -func checkCRDeletion(ctx context.Context, skrClient client.Client, manifest *v1beta2.Manifest) (bool, error) { +func fetchCR(ctx context.Context, skrClient client.Client, manifest *v1beta2.Manifest) (*unstructured.Unstructured, + error) { if manifest.Spec.Resource == nil { - return true, nil + return nil, nil } name := manifest.Spec.Resource.GetName() @@ -505,11 +517,11 @@ func checkCRDeletion(ctx context.Context, skrClient client.Client, manifest *v1b if err := skrClient.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, resourceCR); err != nil { if util.IsNotFound(err) { - return true, nil + return nil, nil } - return false, fmt.Errorf("%w: failed to fetch default resource CR", err) + return nil, fmt.Errorf("%w: failed to fetch default resource CR", err) } - return false, nil + return resourceCR, nil } func (r *Reconciler) pruneDiff(ctx context.Context, clnt Client, manifest *v1beta2.Manifest, @@ -711,3 +723,64 @@ func (r *Reconciler) cleanUpMandatoryModuleMetrics(manifest *v1beta2.Manifest) { r.MandatoryModuleMetrics.CleanupMetrics(kymaName, moduleName) } } + +func (r *Reconciler) handleLabelsRemovalFromResources(ctx context.Context, manifest *v1beta2.Manifest, + skrClient Client) error { + for _, res := range manifest.Status.Synced { + objectKey := client.ObjectKey{ + Name: res.Name, + Namespace: res.Namespace, + } + gvk := schema.GroupVersionKind{ + Group: res.Group, + Version: res.Version, + Kind: res.Kind, + } + + obj := &unstructured.Unstructured{} + obj.SetGroupVersionKind(gvk) + if err := skrClient.Get(ctx, objectKey, obj); err != nil { + return fmt.Errorf("failed to get resource") + } + + if err := removeManagedByAndWatchedByLabels(ctx, obj, skrClient); err != nil { + return err + } + } + + defaultCR, err := fetchCR(ctx, skrClient, manifest) + if err != nil { + return fmt.Errorf("failed to fetch CR: %w", err) + } + + if defaultCR != nil { + if err := removeManagedByAndWatchedByLabels(ctx, defaultCR, skrClient); err != nil { + return err + } + } + + return nil +} + +func removeManagedByAndWatchedByLabels(ctx context.Context, resource *unstructured.Unstructured, + skrClient Client) error { + labels := resource.GetLabels() + _, managedByLabelExists := labels[shared.ManagedBy] + if managedByLabelExists { + delete(labels, shared.ManagedBy) + } + _, watchedByLabelExists := labels[shared.WatchedByLabel] + if watchedByLabelExists { + delete(labels, shared.WatchedByLabel) + } + + if managedByLabelExists || watchedByLabelExists { + resource.SetLabels(labels) + + if err := skrClient.Update(ctx, resource); err != nil { + return fmt.Errorf("failed to update object: %w", err) + } + } + + return nil +} diff --git a/internal/pkg/metrics/manifest.go b/internal/pkg/metrics/manifest.go index b358a75b45..149501bd69 100644 --- a/internal/pkg/metrics/manifest.go +++ b/internal/pkg/metrics/manifest.go @@ -32,6 +32,7 @@ const ( ManifestUnauthorized ManifestRequeueReason = "manifest_unauthorized" ManifestReconcileFinished ManifestRequeueReason = "manifest_reconcile_finished" ManifestUnmanagedUpdate ManifestRequeueReason = "manifest_unmanaged_update" + ManifestResourcesLabelRemoval ManifestRequeueReason = "manifest_labels_removal" ) type ManifestMetrics struct { From 61e6b4e46eff31076fa75764242533eb3c8e2203 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Wed, 2 Oct 2024 11:27:00 +0200 Subject: [PATCH 03/15] Add unit tests --- internal/declarative/v2/reconciler.go | 25 ++++++-------- internal/declarative/v2/reconciler_test.go | 40 ++++++++++++++++++++++ 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index 86091cbd49..f1558e0461 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -743,8 +743,10 @@ func (r *Reconciler) handleLabelsRemovalFromResources(ctx context.Context, manif return fmt.Errorf("failed to get resource") } - if err := removeManagedByAndWatchedByLabels(ctx, obj, skrClient); err != nil { - return err + if needsUpdateAfterLabelRemoval(obj) { + if err := skrClient.Update(ctx, obj); err != nil { + return fmt.Errorf("failed to update object: %w", err) + } } } @@ -754,16 +756,17 @@ func (r *Reconciler) handleLabelsRemovalFromResources(ctx context.Context, manif } if defaultCR != nil { - if err := removeManagedByAndWatchedByLabels(ctx, defaultCR, skrClient); err != nil { - return err + if needsUpdateAfterLabelRemoval(defaultCR) { + if err := skrClient.Update(ctx, defaultCR); err != nil { + return fmt.Errorf("failed to update object: %w", err) + } } } return nil } -func removeManagedByAndWatchedByLabels(ctx context.Context, resource *unstructured.Unstructured, - skrClient Client) error { +func needsUpdateAfterLabelRemoval(resource *unstructured.Unstructured) bool { labels := resource.GetLabels() _, managedByLabelExists := labels[shared.ManagedBy] if managedByLabelExists { @@ -774,13 +777,7 @@ func removeManagedByAndWatchedByLabels(ctx context.Context, resource *unstructur delete(labels, shared.WatchedByLabel) } - if managedByLabelExists || watchedByLabelExists { - resource.SetLabels(labels) + resource.SetLabels(labels) - if err := skrClient.Update(ctx, resource); err != nil { - return fmt.Errorf("failed to update object: %w", err) - } - } - - return nil + return watchedByLabelExists || managedByLabelExists } diff --git a/internal/declarative/v2/reconciler_test.go b/internal/declarative/v2/reconciler_test.go index 0d1e4ddee1..dd2010230a 100644 --- a/internal/declarative/v2/reconciler_test.go +++ b/internal/declarative/v2/reconciler_test.go @@ -14,6 +14,7 @@ import ( "k8s.io/cli-runtime/pkg/resource" "github.com/kyma-project/lifecycle-manager/api/shared" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) func TestPruneResource(t *testing.T) { @@ -230,3 +231,42 @@ func Test_hasStatusDiff(t *testing.T) { }) } } + +func Test_needsUpdateAfterLabelRemoval_WhenLabelsAreEmpty(t *testing.T) { + emptyLabels := map[string]string{} + res := &unstructured.Unstructured{} + res.SetLabels(emptyLabels) + actual := needsUpdateAfterLabelRemoval(res) + + require.Equal(t, false, actual) + require.Equal(t, emptyLabels, res.GetLabels()) +} + +func Test_needsUpdateAfterLabelRemoval_WhenWatchedByLabel(t *testing.T) { + labels := map[string]string{ + shared.WatchedByLabel: shared.WatchedByLabelValue, + "test": "value", + } + expectedLabels := map[string]string{ + "test": "value", + } + res := &unstructured.Unstructured{} + res.SetLabels(labels) + actual := needsUpdateAfterLabelRemoval(res) + + require.Equal(t, true, actual) + require.Equal(t, expectedLabels, res.GetLabels()) +} + +func Test_needsUpdateAfterLabelRemoval_WhenManagedByLabel(t *testing.T) { + labels := map[string]string{ + shared.ManagedBy: shared.ManagedByLabelValue, + } + expectedLabels := map[string]string{} + res := &unstructured.Unstructured{} + res.SetLabels(labels) + actual := needsUpdateAfterLabelRemoval(res) + + require.Equal(t, true, actual) + require.Equal(t, expectedLabels, res.GetLabels()) +} From 03cf3e2a4643c8b02de58eaacfebca50d611c714 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Wed, 2 Oct 2024 12:32:38 +0200 Subject: [PATCH 04/15] Fix linting issues --- internal/declarative/v2/reconciler.go | 70 ++++++++++++++-------- internal/declarative/v2/reconciler_test.go | 8 +-- 2 files changed, 50 insertions(+), 28 deletions(-) diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index f1558e0461..fcf1439a13 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -156,13 +156,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu } if controllerutil.ContainsFinalizer(manifest, labelRemovalFinalizer) { - if err := r.handleLabelsRemovalFromResources(ctx, manifest, skrClient); err != nil { - return ctrl.Result{}, err - } - - if controllerutil.RemoveFinalizer(manifest, labelRemovalFinalizer) { - return r.updateManifest(ctx, manifest, metrics.ManifestResourcesLabelRemoval) - } + return r.handleLabelsRemovalFinalizerForUnmanagedModule(ctx, manifest, skrClient) } if err := r.Delete(ctx, manifest); err != nil { @@ -245,6 +239,18 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return r.finishReconcile(ctx, manifest, metrics.ManifestReconcileFinished, manifestStatus, nil) } +func (r *Reconciler) handleLabelsRemovalFinalizerForUnmanagedModule(ctx context.Context, + manifest *v1beta2.Manifest, skrClient Client) (ctrl.Result, + error, +) { + if err := r.handleLabelsRemovalFromResources(ctx, manifest, skrClient); err != nil { + return ctrl.Result{}, err + } + + controllerutil.RemoveFinalizer(manifest, labelRemovalFinalizer) + return r.updateManifest(ctx, manifest, metrics.ManifestResourcesLabelRemoval) +} + func (r *Reconciler) cleanupManifest(ctx context.Context, req ctrl.Request, manifest *v1beta2.Manifest, manifestStatus shared.Status, requeueReason metrics.ManifestRequeueReason, originalErr error, ) (ctrl.Result, error) { @@ -464,11 +470,11 @@ func (r *Reconciler) renderTargetResources(ctx context.Context, skrClient client converter ResourceToInfoConverter, manifest *v1beta2.Manifest, spec *Spec, ) ([]*resource.Info, error) { if !manifest.GetDeletionTimestamp().IsZero() { - defaultCR, err := fetchCR(ctx, skrClient, manifest) + deleted, err := checkCRDeletion(ctx, skrClient, manifest) if err != nil { return nil, err } - if defaultCR == nil { + if deleted { return ResourceList{}, nil } } @@ -497,17 +503,32 @@ func (r *Reconciler) renderTargetResources(ctx context.Context, skrClient client return target, nil } -func fetchCR(ctx context.Context, skrClient client.Client, manifest *v1beta2.Manifest) (*unstructured.Unstructured, - error) { +func checkCRDeletion(ctx context.Context, skrClient client.Client, manifest *v1beta2.Manifest) (bool, + error, +) { if manifest.Spec.Resource == nil { - return nil, nil + return true, nil + } + + resourceCR, err := getCR(ctx, skrClient, manifest) + if err != nil { + if util.IsNotFound(err) { + return true, nil + } + return false, fmt.Errorf("%w: failed to fetch default resource CR", err) } + return resourceCR == nil, nil +} + +func getCR(ctx context.Context, skrClient client.Client, manifest *v1beta2.Manifest) (*unstructured.Unstructured, + error, +) { + resourceCR := &unstructured.Unstructured{} name := manifest.Spec.Resource.GetName() namespace := manifest.Spec.Resource.GetNamespace() gvk := manifest.Spec.Resource.GroupVersionKind() - resourceCR := &unstructured.Unstructured{} resourceCR.SetGroupVersionKind(schema.GroupVersionKind{ Group: gvk.Group, Version: gvk.Version, @@ -516,11 +537,9 @@ func fetchCR(ctx context.Context, skrClient client.Client, manifest *v1beta2.Man if err := skrClient.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, resourceCR); err != nil { - if util.IsNotFound(err) { - return nil, nil - } return nil, fmt.Errorf("%w: failed to fetch default resource CR", err) } + return resourceCR, nil } @@ -725,7 +744,8 @@ func (r *Reconciler) cleanUpMandatoryModuleMetrics(manifest *v1beta2.Manifest) { } func (r *Reconciler) handleLabelsRemovalFromResources(ctx context.Context, manifest *v1beta2.Manifest, - skrClient Client) error { + skrClient Client, +) error { for _, res := range manifest.Status.Synced { objectKey := client.ObjectKey{ Name: res.Name, @@ -740,7 +760,7 @@ func (r *Reconciler) handleLabelsRemovalFromResources(ctx context.Context, manif obj := &unstructured.Unstructured{} obj.SetGroupVersionKind(gvk) if err := skrClient.Get(ctx, objectKey, obj); err != nil { - return fmt.Errorf("failed to get resource") + return fmt.Errorf("failed to get resource, %w", err) } if needsUpdateAfterLabelRemoval(obj) { @@ -750,16 +770,18 @@ func (r *Reconciler) handleLabelsRemovalFromResources(ctx context.Context, manif } } - defaultCR, err := fetchCR(ctx, skrClient, manifest) + if manifest.Spec.Resource == nil { + return nil + } + + defaultCR, err := getCR(ctx, skrClient, manifest) if err != nil { return fmt.Errorf("failed to fetch CR: %w", err) } - if defaultCR != nil { - if needsUpdateAfterLabelRemoval(defaultCR) { - if err := skrClient.Update(ctx, defaultCR); err != nil { - return fmt.Errorf("failed to update object: %w", err) - } + if needsUpdateAfterLabelRemoval(defaultCR) { + if err := skrClient.Update(ctx, defaultCR); err != nil { + return fmt.Errorf("failed to update object: %w", err) } } diff --git a/internal/declarative/v2/reconciler_test.go b/internal/declarative/v2/reconciler_test.go index dd2010230a..8a80059026 100644 --- a/internal/declarative/v2/reconciler_test.go +++ b/internal/declarative/v2/reconciler_test.go @@ -11,10 +11,10 @@ import ( apicorev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/cli-runtime/pkg/resource" "github.com/kyma-project/lifecycle-manager/api/shared" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) func TestPruneResource(t *testing.T) { @@ -238,7 +238,7 @@ func Test_needsUpdateAfterLabelRemoval_WhenLabelsAreEmpty(t *testing.T) { res.SetLabels(emptyLabels) actual := needsUpdateAfterLabelRemoval(res) - require.Equal(t, false, actual) + require.False(t, actual) require.Equal(t, emptyLabels, res.GetLabels()) } @@ -254,7 +254,7 @@ func Test_needsUpdateAfterLabelRemoval_WhenWatchedByLabel(t *testing.T) { res.SetLabels(labels) actual := needsUpdateAfterLabelRemoval(res) - require.Equal(t, true, actual) + require.True(t, actual) require.Equal(t, expectedLabels, res.GetLabels()) } @@ -267,6 +267,6 @@ func Test_needsUpdateAfterLabelRemoval_WhenManagedByLabel(t *testing.T) { res.SetLabels(labels) actual := needsUpdateAfterLabelRemoval(res) - require.Equal(t, true, actual) + require.True(t, actual) require.Equal(t, expectedLabels, res.GetLabels()) } From a07fbdb6df3b689cade026e1523f7b102a7b5207 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Wed, 2 Oct 2024 12:55:39 +0200 Subject: [PATCH 05/15] Add E2E test --- pkg/testutils/unstructured.go | 28 +++++++++ tests/e2e/manifest_reconciliation_test.go | 4 +- tests/e2e/module_unmanage_test.go | 71 ++++++++++++++++++++++- tests/e2e/utils_test.go | 9 +-- 4 files changed, 104 insertions(+), 8 deletions(-) diff --git a/pkg/testutils/unstructured.go b/pkg/testutils/unstructured.go index 8a8d36a7b7..f2b6e2ff0f 100644 --- a/pkg/testutils/unstructured.go +++ b/pkg/testutils/unstructured.go @@ -10,10 +10,16 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" + "errors" "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/pkg/util" ) +var ( + ErrLabelNotFound = errors.New("label is not found") + ErrLabelValueNotCorrect = errors.New("label value is not as expected") +) + func DeleteCR(ctx context.Context, clnt client.Client, obj client.Object) error { err := clnt.Delete(ctx, obj) if err != nil && util.IsNotFound(err) { @@ -69,6 +75,28 @@ func IsResourceVersionSame(ctx context.Context, clnt client.Client, return false, nil } +func HasExpectedLabel(ctx context.Context, clnt client.Client, + objectKey client.ObjectKey, + gvk schema.GroupVersionKind, expectedLabelKey string, expectedLabelValue string, +) error { + obj, err := GetCR(ctx, clnt, objectKey, gvk) + if err != nil { + return err + } + + labels := obj.GetLabels() + value, exists := labels[expectedLabelKey] + if !exists { + return ErrLabelNotFound + } + + if value != expectedLabelValue { + return ErrLabelValueNotCorrect + } + + return nil +} + func CreateCR(ctx context.Context, clnt client.Client, obj client.Object) error { err := clnt.Create(ctx, obj) if !apierrors.IsAlreadyExists(err) { diff --git a/tests/e2e/manifest_reconciliation_test.go b/tests/e2e/manifest_reconciliation_test.go index d18ae4dfda..5b907eba9d 100644 --- a/tests/e2e/manifest_reconciliation_test.go +++ b/tests/e2e/manifest_reconciliation_test.go @@ -1,10 +1,9 @@ package e2e_test import ( - templatev1alpha1 "github.com/kyma-project/template-operator/api/v1alpha1" - "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" + templatev1alpha1 "github.com/kyma-project/template-operator/api/v1alpha1" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -43,6 +42,7 @@ var _ = Describe("Manifest Skip Reconciliation Label", Ordered, func() { WithArguments(TestModuleCRName, RemoteNamespace, skrClient, shared.ManagedBy, shared.ManagedByLabelValue). Should(Succeed()) + By("And the KCP Kyma CR is in a \"Ready\" State") Eventually(KymaIsInState). WithContext(ctx). diff --git a/tests/e2e/module_unmanage_test.go b/tests/e2e/module_unmanage_test.go index a55566d8bd..8631d27ab1 100644 --- a/tests/e2e/module_unmanage_test.go +++ b/tests/e2e/module_unmanage_test.go @@ -2,14 +2,15 @@ package e2e_test import ( templatev1alpha1 "github.com/kyma-project/template-operator/api/v1alpha1" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" + . "github.com/kyma-project/lifecycle-manager/pkg/testutils" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - - . "github.com/kyma-project/lifecycle-manager/pkg/testutils" ) var _ = Describe("Unmanaging Kyma Module", Ordered, func() { @@ -20,6 +21,7 @@ var _ = Describe("Unmanaging Kyma Module", Ordered, func() { module := NewTemplateOperator(v1beta2.DefaultChannel) Context("Given SKR Cluster", func() { + var manifestResources []shared.Resource It("When Kyma Module is enabled on SKR Kyma CR", func() { Eventually(EnableModule). WithContext(ctx). @@ -38,6 +40,38 @@ var _ = Describe("Unmanaging Kyma Module", Ordered, func() { WithContext(ctx). WithArguments(kyma.GetName(), kyma.GetNamespace(), kcpClient, shared.StateReady). Should(Succeed()) + By("And all manifest resources and default CR have managed-by and watched-by labels") + manifest, err := GetManifest(ctx, kcpClient, kyma.GetName(), kyma.GetNamespace(), + module.Name) + Expect(err).Should(Succeed()) + manifestResources = manifest.Status.Synced + for _, resource := range manifestResources { + objectKey := client.ObjectKey{Name: resource.Name, Namespace: resource.Namespace} + gvk := schema.GroupVersionKind{ + Group: resource.Group, + Version: resource.Version, + Kind: resource.Kind, + } + Eventually(HasExpectedLabel). + WithContext(ctx). + WithArguments(skrClient, objectKey, gvk, + shared.ManagedBy, shared.ManagedByLabelValue).Should(Succeed()) + Eventually(HasExpectedLabel). + WithContext(ctx). + WithArguments(skrClient, objectKey, gvk, + shared.WatchedByLabel, shared.WatchedByLabelValue).Should(Succeed()) + + } + Eventually(CheckSampleCRHasExpectedLabel). + WithContext(ctx). + WithArguments(TestModuleCRName, RemoteNamespace, skrClient, shared.ManagedBy, + shared.ManagedByLabelValue). + Should(Succeed()) + Eventually(CheckSampleCRHasExpectedLabel). + WithContext(ctx). + WithArguments(TestModuleCRName, RemoteNamespace, skrClient, shared.WatchedByLabel, + shared.WatchedByLabel). + Should(Succeed()) }) It("When Kyma Module is unmanaged", func() { @@ -66,6 +100,39 @@ var _ = Describe("Unmanaging Kyma Module", Ordered, func() { "apps", "v1", "Deployment", skrClient). Should(Succeed()) + By("And all manifest resources and default CR no longer have managed-by or watched-by labels") + manifest, err := GetManifest(ctx, kcpClient, kyma.GetName(), kyma.GetNamespace(), + module.Name) + Expect(err).Should(Succeed()) + manifestResources = manifest.Status.Synced + for _, resource := range manifestResources { + objectKey := client.ObjectKey{Name: resource.Name, Namespace: resource.Namespace} + gvk := schema.GroupVersionKind{ + Group: resource.Group, + Version: resource.Version, + Kind: resource.Kind, + } + Eventually(HasExpectedLabel). + WithContext(ctx). + WithArguments(skrClient, objectKey, gvk, + shared.ManagedBy, shared.ManagedByLabelValue).Should(Equal(ErrLabelNotFound)) + Eventually(HasExpectedLabel). + WithContext(ctx). + WithArguments(skrClient, objectKey, gvk, + shared.WatchedByLabel, shared.WatchedByLabelValue).Should(Equal(ErrLabelNotFound)) + } + + Eventually(CheckSampleCRHasExpectedLabel). + WithContext(ctx). + WithArguments(TestModuleCRName, RemoteNamespace, skrClient, shared.ManagedBy, + shared.ManagedByLabelValue). + Should(Equal(ErrLabelNotExistOnCR)) + Eventually(CheckSampleCRHasExpectedLabel). + WithContext(ctx). + WithArguments(TestModuleCRName, RemoteNamespace, skrClient, shared.WatchedByLabel, + shared.WatchedByLabel). + Should(Equal(ErrLabelNotExistOnCR)) + By("And KCP Kyma CR is in \"Ready\" State") Eventually(KymaIsInState). WithContext(ctx). diff --git a/tests/e2e/utils_test.go b/tests/e2e/utils_test.go index ee79bb0095..7e1c9c410f 100644 --- a/tests/e2e/utils_test.go +++ b/tests/e2e/utils_test.go @@ -28,7 +28,7 @@ var ( errKymaNotInExpectedState = errors.New("kyma CR not in expected state") errModuleNotExisting = errors.New("module does not exists in KymaCR") errLabelNotExistOnNamespace = errors.New("label does not exist on namespace") - errLabelNotExistOnSampleCR = errors.New("label does not exist on CustomResource") + ErrLabelNotExistOnCR = errors.New("label does not exist on CustomResource") ) const ( @@ -226,17 +226,18 @@ func CheckSampleCRHasExpectedLabel(ctx context.Context, name, namespace string, labelKey, labelValue string, ) error { customResource, err := GetCR(ctx, clnt, client.ObjectKey{Name: name, Namespace: namespace}, schema.GroupVersionKind{ - Group: "operator.kyma-project.io", - Version: "v1alpha1", + Group: templatev1alpha1.GroupVersion.Group, + Version: templatev1alpha1.GroupVersion.Version, Kind: string(templatev1alpha1.SampleKind), }) + if err != nil { return err } labels := customResource.GetLabels() if labels == nil || labels[labelKey] != labelValue { - return errLabelNotExistOnSampleCR + return ErrLabelNotExistOnCR } return nil From e7722ac7ffec730dacf3525c19fd6f913f1a182a Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Wed, 2 Oct 2024 13:01:48 +0200 Subject: [PATCH 06/15] Fix linting --- pkg/testutils/unstructured.go | 2 +- tests/e2e/utils_test.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/testutils/unstructured.go b/pkg/testutils/unstructured.go index f2b6e2ff0f..308345e85a 100644 --- a/pkg/testutils/unstructured.go +++ b/pkg/testutils/unstructured.go @@ -2,6 +2,7 @@ package testutils import ( "context" + "errors" "fmt" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -10,7 +11,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" - "errors" "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/pkg/util" ) diff --git a/tests/e2e/utils_test.go b/tests/e2e/utils_test.go index 7e1c9c410f..7eafb38a33 100644 --- a/tests/e2e/utils_test.go +++ b/tests/e2e/utils_test.go @@ -230,7 +230,6 @@ func CheckSampleCRHasExpectedLabel(ctx context.Context, name, namespace string, Version: templatev1alpha1.GroupVersion.Version, Kind: string(templatev1alpha1.SampleKind), }) - if err != nil { return err } From 0a085e44ca181823a6b992dba7bd496b1f7378d5 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Wed, 2 Oct 2024 14:22:22 +0200 Subject: [PATCH 07/15] Fix test --- tests/e2e/module_unmanage_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/tests/e2e/module_unmanage_test.go b/tests/e2e/module_unmanage_test.go index 8631d27ab1..64bc8ba64e 100644 --- a/tests/e2e/module_unmanage_test.go +++ b/tests/e2e/module_unmanage_test.go @@ -67,11 +67,6 @@ var _ = Describe("Unmanaging Kyma Module", Ordered, func() { WithArguments(TestModuleCRName, RemoteNamespace, skrClient, shared.ManagedBy, shared.ManagedByLabelValue). Should(Succeed()) - Eventually(CheckSampleCRHasExpectedLabel). - WithContext(ctx). - WithArguments(TestModuleCRName, RemoteNamespace, skrClient, shared.WatchedByLabel, - shared.WatchedByLabel). - Should(Succeed()) }) It("When Kyma Module is unmanaged", func() { @@ -127,11 +122,6 @@ var _ = Describe("Unmanaging Kyma Module", Ordered, func() { WithArguments(TestModuleCRName, RemoteNamespace, skrClient, shared.ManagedBy, shared.ManagedByLabelValue). Should(Equal(ErrLabelNotExistOnCR)) - Eventually(CheckSampleCRHasExpectedLabel). - WithContext(ctx). - WithArguments(TestModuleCRName, RemoteNamespace, skrClient, shared.WatchedByLabel, - shared.WatchedByLabel). - Should(Equal(ErrLabelNotExistOnCR)) By("And KCP Kyma CR is in \"Ready\" State") Eventually(KymaIsInState). From 254e31086210436d7ae9937acffdbf04cb9b8f5a Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Wed, 2 Oct 2024 14:23:14 +0200 Subject: [PATCH 08/15] Fix test --- tests/e2e/module_unmanage_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/e2e/module_unmanage_test.go b/tests/e2e/module_unmanage_test.go index 8631d27ab1..f2444d3a75 100644 --- a/tests/e2e/module_unmanage_test.go +++ b/tests/e2e/module_unmanage_test.go @@ -127,11 +127,6 @@ var _ = Describe("Unmanaging Kyma Module", Ordered, func() { WithArguments(TestModuleCRName, RemoteNamespace, skrClient, shared.ManagedBy, shared.ManagedByLabelValue). Should(Equal(ErrLabelNotExistOnCR)) - Eventually(CheckSampleCRHasExpectedLabel). - WithContext(ctx). - WithArguments(TestModuleCRName, RemoteNamespace, skrClient, shared.WatchedByLabel, - shared.WatchedByLabel). - Should(Equal(ErrLabelNotExistOnCR)) By("And KCP Kyma CR is in \"Ready\" State") Eventually(KymaIsInState). From d4abc1f0001f4e8cf5a037e9b0371ef713fcea2f Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Wed, 2 Oct 2024 14:25:42 +0200 Subject: [PATCH 09/15] Fix test --- tests/e2e/module_unmanage_test.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/e2e/module_unmanage_test.go b/tests/e2e/module_unmanage_test.go index f2444d3a75..64bc8ba64e 100644 --- a/tests/e2e/module_unmanage_test.go +++ b/tests/e2e/module_unmanage_test.go @@ -67,11 +67,6 @@ var _ = Describe("Unmanaging Kyma Module", Ordered, func() { WithArguments(TestModuleCRName, RemoteNamespace, skrClient, shared.ManagedBy, shared.ManagedByLabelValue). Should(Succeed()) - Eventually(CheckSampleCRHasExpectedLabel). - WithContext(ctx). - WithArguments(TestModuleCRName, RemoteNamespace, skrClient, shared.WatchedByLabel, - shared.WatchedByLabel). - Should(Succeed()) }) It("When Kyma Module is unmanaged", func() { From e490c02fc565b771e012b2109291fce87fa80a88 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Wed, 2 Oct 2024 14:40:38 +0200 Subject: [PATCH 10/15] Fix test --- tests/e2e/module_unmanage_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/e2e/module_unmanage_test.go b/tests/e2e/module_unmanage_test.go index 64bc8ba64e..714a4f804a 100644 --- a/tests/e2e/module_unmanage_test.go +++ b/tests/e2e/module_unmanage_test.go @@ -96,10 +96,6 @@ var _ = Describe("Unmanaging Kyma Module", Ordered, func() { Should(Succeed()) By("And all manifest resources and default CR no longer have managed-by or watched-by labels") - manifest, err := GetManifest(ctx, kcpClient, kyma.GetName(), kyma.GetNamespace(), - module.Name) - Expect(err).Should(Succeed()) - manifestResources = manifest.Status.Synced for _, resource := range manifestResources { objectKey := client.ObjectKey{Name: resource.Name, Namespace: resource.Namespace} gvk := schema.GroupVersionKind{ From 52871c6699af8a1b9ac1f02c8e66ae707b2ce03f Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Thu, 10 Oct 2024 13:44:39 +0200 Subject: [PATCH 11/15] Extract ManifestApiClient and label removal into new services --- api/v1beta2/moduletemplate_types.go | 2 +- cmd/main.go | 12 +- internal/controller/manifest/controller.go | 4 +- internal/controller/manifest/setup.go | 5 +- internal/declarative/v2/reconciler.go | 172 ++++--------- internal/declarative/v2/reconciler_test.go | 114 --------- .../manifest/labelsremoval/labels_removal.go | 85 +++++++ .../labelsremoval/labels_removal_test.go | 229 ++++++++++++++++++ .../manifestclient/manifest_client.go | 74 ++++++ .../manifestclient/manifest_client_test.go | 86 +++++++ .../custom_resource_check/suite_test.go | 6 + .../controller/manifest/suite_test.go | 6 + unit-test-coverage.yaml | 1 + 13 files changed, 551 insertions(+), 245 deletions(-) create mode 100644 internal/manifest/labelsremoval/labels_removal.go create mode 100644 internal/manifest/labelsremoval/labels_removal_test.go create mode 100644 internal/manifest/manifestclient/manifest_client.go create mode 100644 internal/manifest/manifestclient/manifest_client_test.go diff --git a/api/v1beta2/moduletemplate_types.go b/api/v1beta2/moduletemplate_types.go index 9601f4f274..e2e43037d8 100644 --- a/api/v1beta2/moduletemplate_types.go +++ b/api/v1beta2/moduletemplate_types.go @@ -136,7 +136,7 @@ type ModuleTemplateSpec struct { // Info contains metadata about the module. // +optional Info ModuleInfo `json:"info,omitempty"` - + // AssociatedResources is a list of module related resources that usually must be cleaned when uninstalling a module. Informational purpose only. // +optional AssociatedResources []apimetav1.GroupVersionKind `json:"associatedResources,omitempty"` diff --git a/cmd/main.go b/cmd/main.go index 76895134b7..86d3fab319 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -57,6 +57,7 @@ import ( "github.com/kyma-project/lifecycle-manager/internal/crd" "github.com/kyma-project/lifecycle-manager/internal/descriptor/provider" "github.com/kyma-project/lifecycle-manager/internal/event" + "github.com/kyma-project/lifecycle-manager/internal/manifest/manifestclient" "github.com/kyma-project/lifecycle-manager/internal/pkg/flags" "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/internal/remote" @@ -100,7 +101,8 @@ func main() { flagVar := flags.DefineFlagVar() flag.Parse() - ctrl.SetLogger(log.ConfigLogger(int8(flagVar.LogLevel), zapcore.Lock(os.Stdout))) //nolint:gosec // loglevel should always be between -128 to 127 + ctrl.SetLogger(log.ConfigLogger(int8(flagVar.LogLevel), //nolint:gosec // loglevel should always be between -128 to 127 + zapcore.Lock(os.Stdout))) setupLog.Info("starting Lifecycle-Manager version: " + buildVersion) if err := flagVar.Validate(); err != nil { setupLog.Error(err, "unable to start manager") @@ -183,7 +185,7 @@ func setupManager(flagVar *flags.FlagVar, cacheOptions cache.Options, scheme *ma setupKymaReconciler(mgr, descriptorProvider, skrContextProvider, eventRecorder, flagVar, options, skrWebhookManager, kymaMetrics, setupLog) - setupManifestReconciler(mgr, flagVar, options, sharedMetrics, mandatoryModulesMetrics, setupLog) + setupManifestReconciler(mgr, flagVar, options, sharedMetrics, mandatoryModulesMetrics, setupLog, eventRecorder) setupMandatoryModuleReconciler(mgr, descriptorProvider, flagVar, options, mandatoryModulesMetrics, setupLog) setupMandatoryModuleDeletionReconciler(mgr, descriptorProvider, eventRecorder, flagVar, options, setupLog) if flagVar.EnablePurgeFinalizer { @@ -266,7 +268,8 @@ func enableWebhooks(mgr manager.Manager, setupLog logr.Logger) { func controllerOptionsFromFlagVar(flagVar *flags.FlagVar) ctrlruntime.Options { return ctrlruntime.Options{ RateLimiter: workqueue.NewTypedMaxOfRateLimiter( - workqueue.NewTypedItemExponentialFailureRateLimiter[ctrl.Request](flagVar.FailureBaseDelay, flagVar.FailureMaxDelay), + workqueue.NewTypedItemExponentialFailureRateLimiter[ctrl.Request](flagVar.FailureBaseDelay, + flagVar.FailureMaxDelay), &workqueue.TypedBucketRateLimiter[ctrl.Request]{ Limiter: rate.NewLimiter(rate.Limit(flagVar.RateLimiterFrequency), flagVar.RateLimiterBurst), }, @@ -381,11 +384,13 @@ func setupPurgeReconciler(mgr ctrl.Manager, func setupManifestReconciler(mgr ctrl.Manager, flagVar *flags.FlagVar, options ctrlruntime.Options, sharedMetrics *metrics.SharedMetrics, mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, setupLog logr.Logger, + event event.Event, ) { options.MaxConcurrentReconciles = flagVar.MaxConcurrentManifestReconciles options.RateLimiter = internal.ManifestRateLimiter(flagVar.FailureBaseDelay, flagVar.FailureMaxDelay, flagVar.RateLimiterFrequency, flagVar.RateLimiterBurst) + manifestClient := manifestclient.NewManifestClient(event, mgr.GetClient()) if err := manifest.SetupWithManager( mgr, options, queue.RequeueIntervals{ Success: flagVar.ManifestRequeueSuccessInterval, @@ -398,6 +403,7 @@ func setupManifestReconciler(mgr ctrl.Manager, flagVar *flags.FlagVar, options c ListenerAddr: flagVar.ManifestListenerAddr, EnableDomainNameVerification: flagVar.EnableDomainNameVerification, }, metrics.NewManifestMetrics(sharedMetrics), mandatoryModulesMetrics, + manifestClient, ); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Manifest") os.Exit(bootstrapFailedExitCode) diff --git a/internal/controller/manifest/controller.go b/internal/controller/manifest/controller.go index 74b94f3505..bd841b576a 100644 --- a/internal/controller/manifest/controller.go +++ b/internal/controller/manifest/controller.go @@ -6,6 +6,7 @@ import ( declarativev2 "github.com/kyma-project/lifecycle-manager/internal/declarative/v2" "github.com/kyma-project/lifecycle-manager/internal/manifest" "github.com/kyma-project/lifecycle-manager/internal/manifest/img" + "github.com/kyma-project/lifecycle-manager/internal/manifest/manifestclient" "github.com/kyma-project/lifecycle-manager/internal/manifest/statecheck" "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/pkg/queue" @@ -19,6 +20,7 @@ func NewReconciler(mgr manager.Manager, requeueIntervals queue.RequeueIntervals, manifestMetrics *metrics.ManifestMetrics, mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, + manifestClient manifestclient.ManifestClient, ) *declarativev2.Reconciler { kcp := &declarativev2.ClusterInfo{ Client: mgr.GetClient(), @@ -30,7 +32,7 @@ func NewReconciler(mgr manager.Manager, statefulChecker := statecheck.NewStatefulSetStateCheck() deploymentChecker := statecheck.NewDeploymentStateCheck() return declarativev2.NewFromManager( - mgr, requeueIntervals, manifestMetrics, mandatoryModulesMetrics, + mgr, requeueIntervals, manifestMetrics, mandatoryModulesMetrics, manifestClient, manifest.NewSpecResolver(keyChainLookup, extractor), declarativev2.WithCustomStateCheck(statecheck.NewManagerStateCheck(statefulChecker, deploymentChecker)), declarativev2.WithRemoteTargetCluster(lookup.ConfigResolver), diff --git a/internal/controller/manifest/setup.go b/internal/controller/manifest/setup.go index a5b7a83b4f..10ca096e75 100644 --- a/internal/controller/manifest/setup.go +++ b/internal/controller/manifest/setup.go @@ -22,6 +22,7 @@ import ( "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/manifest/manifestclient" "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/pkg/queue" "github.com/kyma-project/lifecycle-manager/pkg/security" @@ -40,6 +41,7 @@ func SetupWithManager(mgr manager.Manager, settings SetupOptions, manifestMetrics *metrics.ManifestMetrics, mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, + manifestClient manifestclient.ManifestClient, ) error { var verifyFunc watcherevent.Verify if settings.EnableDomainNameVerification { @@ -84,7 +86,8 @@ func SetupWithManager(mgr manager.Manager, predicate.LabelChangedPredicate{}))). WatchesRawSource(skrEventChannel). WithOptions(opts). - Complete(NewReconciler(mgr, requeueIntervals, manifestMetrics, mandatoryModulesMetrics)); err != nil { + Complete(NewReconciler(mgr, requeueIntervals, manifestMetrics, mandatoryModulesMetrics, + manifestClient)); err != nil { return fmt.Errorf("failed to setup manager for manifest controller: %w", err) } diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index fcf1439a13..0acc79ef1c 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -20,6 +20,8 @@ import ( "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/internal" + "github.com/kyma-project/lifecycle-manager/internal/manifest/labelsremoval" + "github.com/kyma-project/lifecycle-manager/internal/manifest/manifestclient" "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/internal/pkg/resources" "github.com/kyma-project/lifecycle-manager/pkg/common" @@ -42,7 +44,6 @@ const ( CustomResourceManagerFinalizer = "resource.kyma-project.io/finalizer" SyncedOCIRefAnnotation = "sync-oci-ref" defaultFinalizer = "declarative.kyma-project.io/finalizer" - labelRemovalFinalizer = "label-removal-finalizer" defaultFieldOwner client.FieldOwner = "declarative.kyma-project.io/applier" ) @@ -50,6 +51,7 @@ func NewFromManager(mgr manager.Manager, requeueIntervals queue.RequeueIntervals, metrics *metrics.ManifestMetrics, mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, + manifestClient manifestclient.ManifestClient, specResolver SpecResolver, options ...Option, ) *Reconciler { @@ -58,6 +60,7 @@ func NewFromManager(mgr manager.Manager, reconciler.MandatoryModuleMetrics = mandatoryModulesMetrics reconciler.RequeueIntervals = requeueIntervals reconciler.specResolver = specResolver + reconciler.manifestClient = manifestClient reconciler.Options = DefaultOptions().Apply(WithManager(mgr)).Apply(options...) return reconciler } @@ -68,6 +71,7 @@ type Reconciler struct { ManifestMetrics *metrics.ManifestMetrics MandatoryModuleMetrics *metrics.MandatoryModulesMetrics specResolver SpecResolver + manifestClient manifestclient.ManifestClient } const waitingForResourcesMsg = "waiting for resources to become ready" @@ -155,8 +159,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return r.cleanupManifest(ctx, req, manifest, manifestStatus, metrics.ManifestUnmanagedUpdate, nil) } - if controllerutil.ContainsFinalizer(manifest, labelRemovalFinalizer) { - return r.handleLabelsRemovalFinalizerForUnmanagedModule(ctx, manifest, skrClient) + if controllerutil.ContainsFinalizer(manifest, labelsremoval.LabelRemovalFinalizer) { + return r.handleLabelsRemovalFinalizer(ctx, skrClient, manifest) } if err := r.Delete(ctx, manifest); err != nil { @@ -168,7 +172,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu if manifest.GetDeletionTimestamp().IsZero() { partialMeta := r.partialObjectMetadata(manifest) defaultFinalizerAdded := controllerutil.AddFinalizer(partialMeta, defaultFinalizer) - labelRemovalFinalizerAdded := controllerutil.AddFinalizer(partialMeta, labelRemovalFinalizer) + labelRemovalFinalizerAdded := controllerutil.AddFinalizer(partialMeta, labelsremoval.LabelRemovalFinalizer) if defaultFinalizerAdded || labelRemovalFinalizerAdded { return r.ssaSpec(ctx, partialMeta, metrics.ManifestAddFinalizer) } @@ -239,16 +243,20 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu return r.finishReconcile(ctx, manifest, metrics.ManifestReconcileFinished, manifestStatus, nil) } -func (r *Reconciler) handleLabelsRemovalFinalizerForUnmanagedModule(ctx context.Context, - manifest *v1beta2.Manifest, skrClient Client) (ctrl.Result, - error, -) { - if err := r.handleLabelsRemovalFromResources(ctx, manifest, skrClient); err != nil { +func (r *Reconciler) handleLabelsRemovalFinalizer(ctx context.Context, skrClient client.Client, + manifest *v1beta2.Manifest, +) (ctrl.Result, error) { + defaultCR, err := getCR(ctx, skrClient, manifest) + if err != nil { + return ctrl.Result{}, err + } + if err := labelsremoval.HandleLabelsRemovalFinalizerForUnmanagedModule(ctx, manifest, skrClient, + r.manifestClient, defaultCR); err != nil { return ctrl.Result{}, err } - controllerutil.RemoveFinalizer(manifest, labelRemovalFinalizer) - return r.updateManifest(ctx, manifest, metrics.ManifestResourcesLabelRemoval) + r.ManifestMetrics.RecordRequeueReason(metrics.ManifestResourcesLabelRemoval, queue.IntendedRequeue) + return ctrl.Result{Requeue: true}, nil } func (r *Reconciler) cleanupManifest(ctx context.Context, req ctrl.Request, manifest *v1beta2.Manifest, @@ -256,7 +264,7 @@ func (r *Reconciler) cleanupManifest(ctx context.Context, req ctrl.Request, mani ) (ctrl.Result, error) { r.ManifestMetrics.RemoveManifestDuration(req.Name) r.cleanUpMandatoryModuleMetrics(manifest) - finalizersToRemove := []string{defaultFinalizer, labelRemovalFinalizer} + finalizersToRemove := []string{defaultFinalizer, labelsremoval.LabelRemovalFinalizer} if errors.Is(originalErr, ErrAccessSecretNotFound) || manifest.IsUnmanaged() { finalizersToRemove = manifest.GetFinalizers() } @@ -503,14 +511,14 @@ func (r *Reconciler) renderTargetResources(ctx context.Context, skrClient client return target, nil } -func checkCRDeletion(ctx context.Context, skrClient client.Client, manifest *v1beta2.Manifest) (bool, +func checkCRDeletion(ctx context.Context, skrClient client.Client, manifestCR *v1beta2.Manifest) (bool, error, ) { - if manifest.Spec.Resource == nil { + if manifestCR.Spec.Resource == nil { return true, nil } - resourceCR, err := getCR(ctx, skrClient, manifest) + resourceCR, err := getCR(ctx, skrClient, manifestCR) if err != nil { if util.IsNotFound(err) { return true, nil @@ -521,28 +529,6 @@ func checkCRDeletion(ctx context.Context, skrClient client.Client, manifest *v1b return resourceCR == nil, nil } -func getCR(ctx context.Context, skrClient client.Client, manifest *v1beta2.Manifest) (*unstructured.Unstructured, - error, -) { - resourceCR := &unstructured.Unstructured{} - name := manifest.Spec.Resource.GetName() - namespace := manifest.Spec.Resource.GetNamespace() - gvk := manifest.Spec.Resource.GroupVersionKind() - - resourceCR.SetGroupVersionKind(schema.GroupVersionKind{ - Group: gvk.Group, - Version: gvk.Version, - Kind: gvk.Kind, - }) - - if err := skrClient.Get(ctx, - client.ObjectKey{Name: name, Namespace: namespace}, resourceCR); err != nil { - return nil, fmt.Errorf("%w: failed to fetch default resource CR", err) - } - - return resourceCR, nil -} - func (r *Reconciler) pruneDiff(ctx context.Context, clnt Client, manifest *v1beta2.Manifest, current, target []*resource.Info, spec *Spec, ) error { @@ -667,9 +653,8 @@ func (r *Reconciler) configClient(ctx context.Context, manifest *v1beta2.Manifes func (r *Reconciler) finishReconcile(ctx context.Context, manifest *v1beta2.Manifest, requeueReason metrics.ManifestRequeueReason, previousStatus shared.Status, originalErr error, ) (ctrl.Result, error) { - if err := r.patchStatusIfDiffExist(ctx, manifest, previousStatus); err != nil { - r.Event(manifest, "Warning", "PatchStatus", err.Error()) - return ctrl.Result{}, fmt.Errorf("failed to patch status: %w", err) + if err := r.manifestClient.PatchStatusIfDiffExist(ctx, manifest, previousStatus); err != nil { + return ctrl.Result{}, err } if originalErr != nil { r.ManifestMetrics.RecordRequeueReason(requeueReason, queue.UnexpectedRequeue) @@ -680,49 +665,25 @@ func (r *Reconciler) finishReconcile(ctx context.Context, manifest *v1beta2.Mani return ctrl.Result{RequeueAfter: requeueAfter}, nil } -func (r *Reconciler) patchStatusIfDiffExist(ctx context.Context, manifest *v1beta2.Manifest, - previousStatus shared.Status, -) error { - if hasStatusDiff(manifest.GetStatus(), previousStatus) { - resetNonPatchableField(manifest) - if err := r.Status().Patch(ctx, manifest, client.Apply, client.ForceOwnership, defaultFieldOwner); err != nil { - return fmt.Errorf("failed to patch status: %w", err) - } - } - - return nil -} - -func hasStatusDiff(first, second shared.Status) bool { - return first.State != second.State || first.LastOperation.Operation != second.LastOperation.Operation -} - func (r *Reconciler) ssaSpec(ctx context.Context, obj client.Object, requeueReason metrics.ManifestRequeueReason, ) (ctrl.Result, error) { - resetNonPatchableField(obj) r.ManifestMetrics.RecordRequeueReason(requeueReason, queue.IntendedRequeue) - if err := r.Patch(ctx, obj, client.Apply, client.ForceOwnership, defaultFieldOwner); err != nil { - r.Event(obj, "Warning", "PatchObject", err.Error()) - return ctrl.Result{}, fmt.Errorf("failed to patch object: %w", err) + if err := r.manifestClient.SsaSpec(ctx, obj); err != nil { + return ctrl.Result{}, err } return ctrl.Result{Requeue: true}, nil } -func resetNonPatchableField(obj client.Object) { - obj.SetUID("") - obj.SetManagedFields(nil) - obj.SetResourceVersion("") -} - func (r *Reconciler) updateManifest(ctx context.Context, manifest *v1beta2.Manifest, requeueReason metrics.ManifestRequeueReason, ) (ctrl.Result, error) { r.ManifestMetrics.RecordRequeueReason(requeueReason, queue.IntendedRequeue) - if err := r.Update(ctx, manifest); err != nil { - r.Event(manifest, "Warning", "UpdateObject", err.Error()) - return ctrl.Result{}, fmt.Errorf("failed to update object: %w", err) + + if err := r.manifestClient.UpdateManifest(ctx, manifest); err != nil { + return ctrl.Result{}, err } + return ctrl.Result{Requeue: true}, nil } @@ -743,63 +704,24 @@ func (r *Reconciler) cleanUpMandatoryModuleMetrics(manifest *v1beta2.Manifest) { } } -func (r *Reconciler) handleLabelsRemovalFromResources(ctx context.Context, manifest *v1beta2.Manifest, - skrClient Client, -) error { - for _, res := range manifest.Status.Synced { - objectKey := client.ObjectKey{ - Name: res.Name, - Namespace: res.Namespace, - } - gvk := schema.GroupVersionKind{ - Group: res.Group, - Version: res.Version, - Kind: res.Kind, - } - - obj := &unstructured.Unstructured{} - obj.SetGroupVersionKind(gvk) - if err := skrClient.Get(ctx, objectKey, obj); err != nil { - return fmt.Errorf("failed to get resource, %w", err) - } - - if needsUpdateAfterLabelRemoval(obj) { - if err := skrClient.Update(ctx, obj); err != nil { - return fmt.Errorf("failed to update object: %w", err) - } - } - } - - if manifest.Spec.Resource == nil { - return nil - } - - defaultCR, err := getCR(ctx, skrClient, manifest) - if err != nil { - return fmt.Errorf("failed to fetch CR: %w", err) - } - - if needsUpdateAfterLabelRemoval(defaultCR) { - if err := skrClient.Update(ctx, defaultCR); err != nil { - return fmt.Errorf("failed to update object: %w", err) - } - } +func getCR(ctx context.Context, skrClient client.Client, manifest *v1beta2.Manifest) (*unstructured.Unstructured, + error, +) { + resourceCR := &unstructured.Unstructured{} + name := manifest.Spec.Resource.GetName() + namespace := manifest.Spec.Resource.GetNamespace() + gvk := manifest.Spec.Resource.GroupVersionKind() - return nil -} + resourceCR.SetGroupVersionKind(schema.GroupVersionKind{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + }) -func needsUpdateAfterLabelRemoval(resource *unstructured.Unstructured) bool { - labels := resource.GetLabels() - _, managedByLabelExists := labels[shared.ManagedBy] - if managedByLabelExists { - delete(labels, shared.ManagedBy) - } - _, watchedByLabelExists := labels[shared.WatchedByLabel] - if watchedByLabelExists { - delete(labels, shared.WatchedByLabel) + if err := skrClient.Get(ctx, + client.ObjectKey{Name: name, Namespace: namespace}, resourceCR); err != nil { + return nil, fmt.Errorf("%w: failed to fetch default resource CR", err) } - resource.SetLabels(labels) - - return watchedByLabelExists || managedByLabelExists + return resourceCR, nil } diff --git a/internal/declarative/v2/reconciler_test.go b/internal/declarative/v2/reconciler_test.go index 8a80059026..6f706ddf94 100644 --- a/internal/declarative/v2/reconciler_test.go +++ b/internal/declarative/v2/reconciler_test.go @@ -3,7 +3,6 @@ package v2 import ( "testing" - "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -11,7 +10,6 @@ import ( apicorev1 "k8s.io/api/core/v1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/cli-runtime/pkg/resource" "github.com/kyma-project/lifecycle-manager/api/shared" @@ -158,115 +156,3 @@ func Test_hasDiff(t *testing.T) { }) } } - -func Test_hasStatusDiff(t *testing.T) { - type args struct { - first shared.Status - second shared.Status - } - tests := []struct { - name string - args args - want bool - }{ - { - name: "Different Status", - args: args{ - first: shared.Status{ - State: shared.StateReady, - LastOperation: shared.LastOperation{ - Operation: "resources are ready", - LastUpdateTime: apimetav1.Now(), - }, - }, - second: shared.Status{ - State: shared.StateProcessing, - LastOperation: shared.LastOperation{ - Operation: "installing resources", - LastUpdateTime: apimetav1.Now(), - }, - }, - }, - want: true, - }, - { - name: "Same Status", - args: args{ - first: shared.Status{ - State: shared.StateReady, - LastOperation: shared.LastOperation{ - Operation: "resources are ready", - LastUpdateTime: apimetav1.Now(), - }, - }, - second: shared.Status{ - State: shared.StateReady, - LastOperation: shared.LastOperation{ - Operation: "resources are ready", - LastUpdateTime: apimetav1.NewTime(time.Now().Add(time.Hour)), - }, - }, - }, - want: false, - }, - { - name: "Empty Status", - args: args{ - first: shared.Status{}, - second: shared.Status{ - State: shared.StateReady, - LastOperation: shared.LastOperation{ - Operation: "resources are ready", - LastUpdateTime: apimetav1.NewTime(time.Now().Add(time.Hour)), - }, - }, - }, - want: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - require.Equalf(t, tt.want, hasStatusDiff(tt.args.first, tt.args.second), "hasStatusDiff(%v, %v)", - tt.args.first, tt.args.second) - }) - } -} - -func Test_needsUpdateAfterLabelRemoval_WhenLabelsAreEmpty(t *testing.T) { - emptyLabels := map[string]string{} - res := &unstructured.Unstructured{} - res.SetLabels(emptyLabels) - actual := needsUpdateAfterLabelRemoval(res) - - require.False(t, actual) - require.Equal(t, emptyLabels, res.GetLabels()) -} - -func Test_needsUpdateAfterLabelRemoval_WhenWatchedByLabel(t *testing.T) { - labels := map[string]string{ - shared.WatchedByLabel: shared.WatchedByLabelValue, - "test": "value", - } - expectedLabels := map[string]string{ - "test": "value", - } - res := &unstructured.Unstructured{} - res.SetLabels(labels) - actual := needsUpdateAfterLabelRemoval(res) - - require.True(t, actual) - require.Equal(t, expectedLabels, res.GetLabels()) -} - -func Test_needsUpdateAfterLabelRemoval_WhenManagedByLabel(t *testing.T) { - labels := map[string]string{ - shared.ManagedBy: shared.ManagedByLabelValue, - } - expectedLabels := map[string]string{} - res := &unstructured.Unstructured{} - res.SetLabels(labels) - actual := needsUpdateAfterLabelRemoval(res) - - require.True(t, actual) - require.Equal(t, expectedLabels, res.GetLabels()) -} diff --git a/internal/manifest/labelsremoval/labels_removal.go b/internal/manifest/labelsremoval/labels_removal.go new file mode 100644 index 0000000000..01a90b9466 --- /dev/null +++ b/internal/manifest/labelsremoval/labels_removal.go @@ -0,0 +1,85 @@ +package labelsremoval + +import ( + "context" + "fmt" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + + "github.com/kyma-project/lifecycle-manager/api/shared" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/manifest/manifestclient" +) + +const LabelRemovalFinalizer = "label-removal-finalizer" + +func HandleLabelsRemovalFinalizerForUnmanagedModule(ctx context.Context, + manifest *v1beta2.Manifest, skrClient client.Client, manifestClnt manifestclient.ManifestClient, + defaultCR *unstructured.Unstructured, +) error { + if err := HandleLabelsRemovalFromResources(ctx, manifest, skrClient, defaultCR); err != nil { + return err + } + + controllerutil.RemoveFinalizer(manifest, LabelRemovalFinalizer) + return manifestClnt.UpdateManifest(ctx, manifest) +} + +func HandleLabelsRemovalFromResources(ctx context.Context, manifestCR *v1beta2.Manifest, + skrClient client.Client, defaultCR *unstructured.Unstructured, +) error { + for _, res := range manifestCR.Status.Synced { + objectKey := client.ObjectKey{ + Name: res.Name, + Namespace: res.Namespace, + } + gvk := schema.GroupVersionKind{ + Group: res.Group, + Version: res.Version, + Kind: res.Kind, + } + + obj := &unstructured.Unstructured{} + obj.SetGroupVersionKind(gvk) + if err := skrClient.Get(ctx, objectKey, obj); err != nil { + return fmt.Errorf("failed to get resource, %w", err) + } + + if NeedsUpdateAfterLabelRemoval(obj) { + if err := skrClient.Update(ctx, obj); err != nil { + return fmt.Errorf("failed to update object: %w", err) + } + } + } + + if defaultCR == nil { + return nil + } + + if NeedsUpdateAfterLabelRemoval(defaultCR) { + if err := skrClient.Update(ctx, defaultCR); err != nil { + return fmt.Errorf("failed to update object: %w", err) + } + } + + return nil +} + +func NeedsUpdateAfterLabelRemoval(resource *unstructured.Unstructured) bool { + labels := resource.GetLabels() + _, managedByLabelExists := labels[shared.ManagedBy] + if managedByLabelExists { + delete(labels, shared.ManagedBy) + } + _, watchedByLabelExists := labels[shared.WatchedByLabel] + if watchedByLabelExists { + delete(labels, shared.WatchedByLabel) + } + + resource.SetLabels(labels) + + return watchedByLabelExists || managedByLabelExists +} diff --git a/internal/manifest/labelsremoval/labels_removal_test.go b/internal/manifest/labelsremoval/labels_removal_test.go new file mode 100644 index 0000000000..b7d183be33 --- /dev/null +++ b/internal/manifest/labelsremoval/labels_removal_test.go @@ -0,0 +1,229 @@ +package labelsremoval_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + machineryruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/kyma-project/lifecycle-manager/api/shared" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/manifest/labelsremoval" +) + +func Test_needsUpdateAfterLabelRemoval_WhenLabelsAreEmpty(t *testing.T) { + emptyLabels := map[string]string{} + res := &unstructured.Unstructured{} + res.SetLabels(emptyLabels) + actual := labelsremoval.NeedsUpdateAfterLabelRemoval(res) + + require.False(t, actual) + require.Equal(t, emptyLabels, res.GetLabels()) +} + +func Test_needsUpdateAfterLabelRemoval_WhenWatchedByLabel(t *testing.T) { + labels := map[string]string{ + shared.WatchedByLabel: shared.WatchedByLabelValue, + "test": "value", + } + expectedLabels := map[string]string{ + "test": "value", + } + res := &unstructured.Unstructured{} + res.SetLabels(labels) + actual := labelsremoval.NeedsUpdateAfterLabelRemoval(res) + + require.True(t, actual) + require.Equal(t, expectedLabels, res.GetLabels()) +} + +func Test_needsUpdateAfterLabelRemoval_WhenManagedByLabel(t *testing.T) { + labels := map[string]string{ + shared.ManagedBy: shared.ManagedByLabelValue, + } + expectedLabels := map[string]string{} + res := &unstructured.Unstructured{} + res.SetLabels(labels) + actual := labelsremoval.NeedsUpdateAfterLabelRemoval(res) + + require.True(t, actual) + require.Equal(t, expectedLabels, res.GetLabels()) +} + +func Test_handleLabelsRemovalFromResources_WhenManifestResourcesHaveLabels(t *testing.T) { + gvk := schema.GroupVersionKind{ + Group: "test-group", + Version: "v1", + Kind: "TestKind", + } + + manifest := &v1beta2.Manifest{ + Status: shared.Status{ + Synced: []shared.Resource{ + { + Name: "test-resource-1", + Namespace: "test-1", + GroupVersionKind: apimetav1.GroupVersionKind{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + }, + }, + { + Name: "test-resource-2", + Namespace: "test-2", + GroupVersionKind: apimetav1.GroupVersionKind{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + }, + }, + }, + }, + } + + objs := []client.Object{ + &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": gvk.GroupVersion().String(), + "kind": gvk.Kind, + "version": gvk.Version, + }, + }, + &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": gvk.GroupVersion().String(), + "kind": gvk.Kind, + "version": gvk.Version, + }, + }, + } + objs[0].SetName("test-resource-1") + objs[0].SetNamespace("test-1") + objs[0].SetLabels(map[string]string{ + "operator.kyma-project.io/managed-by": "kyma", + }) + + objs[1].SetName("test-resource-2") + objs[1].SetNamespace("test-2") + objs[1].SetLabels(map[string]string{ + "operator.kyma-project.io/managed-by": "kyma", + }) + + scheme := machineryruntime.NewScheme() + err := v1beta2.AddToScheme(scheme) + require.NoError(t, err) + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() + + err = labelsremoval.HandleLabelsRemovalFromResources(context.TODO(), manifest, fakeClient, nil) + require.NoError(t, err) + + firstObj, secondObj := &unstructured.Unstructured{}, &unstructured.Unstructured{} + firstObj.SetGroupVersionKind(gvk) + err = fakeClient.Get(context.TODO(), client.ObjectKey{Name: "test-resource-1", Namespace: "test-1"}, + firstObj) + require.NoError(t, err) + require.Empty(t, firstObj.GetLabels()) + + secondObj.SetGroupVersionKind(gvk) + err = fakeClient.Get(context.TODO(), client.ObjectKey{Name: "test-resource-2", Namespace: "test-2"}, + secondObj) + require.NoError(t, err) + require.Empty(t, secondObj.GetLabels()) +} + +func Test_handleLabelsRemovalFromResources_WhenManifestResourcesAreNilAndNoDefaultCR(t *testing.T) { + manifest := &v1beta2.Manifest{ + Status: shared.Status{ + Synced: []shared.Resource{}, + }, + } + + scheme := machineryruntime.NewScheme() + err := v1beta2.AddToScheme(scheme) + require.NoError(t, err) + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() + + err = labelsremoval.HandleLabelsRemovalFromResources(context.TODO(), manifest, fakeClient, nil) + + require.NoError(t, err) +} + +func Test_handleLabelsRemovalFromResources_WhenManifestResourcesAndDefaultCRHaveLabels(t *testing.T) { + gvk := schema.GroupVersionKind{ + Group: "test-group", + Version: "v1", + Kind: "TestKind", + } + + manifest := &v1beta2.Manifest{ + Status: shared.Status{ + Synced: []shared.Resource{ + { + Name: "test-resource-1", + Namespace: "test-1", + GroupVersionKind: apimetav1.GroupVersionKind{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + }, + }, + }, + }, + } + + objs := []client.Object{ + &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": gvk.GroupVersion().String(), + "kind": gvk.Kind, + "version": gvk.Version, + }, + }, + } + objs[0].SetName("test-resource-1") + objs[0].SetNamespace("test-1") + objs[0].SetLabels(map[string]string{ + "operator.kyma-project.io/managed-by": "kyma", + }) + + defaultCR := &unstructured.Unstructured{} + defaultCR.SetName("default-cr") + defaultCR.SetNamespace("default-ns") + defaultCR.SetGroupVersionKind(gvk) + defaultCR.SetLabels(map[string]string{ + "operator.kyma-project.io/managed-by": "kyma", + }) + + objs = append(objs, defaultCR) + + scheme := machineryruntime.NewScheme() + err := v1beta2.AddToScheme(scheme) + require.NoError(t, err) + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).Build() + + err = labelsremoval.HandleLabelsRemovalFromResources(context.TODO(), manifest, fakeClient, + defaultCR) + + require.NoError(t, err) + + firstObj := &unstructured.Unstructured{} + firstObj.SetGroupVersionKind(gvk) + err = fakeClient.Get(context.TODO(), client.ObjectKey{Name: "test-resource-1", Namespace: "test-1"}, + firstObj) + require.NoError(t, err) + require.Empty(t, firstObj.GetLabels()) + + err = fakeClient.Get(context.TODO(), client.ObjectKey{Name: "default-cr", Namespace: "default-ns"}, + defaultCR) + require.NoError(t, err) + require.Empty(t, defaultCR.GetLabels()) +} diff --git a/internal/manifest/manifestclient/manifest_client.go b/internal/manifest/manifestclient/manifest_client.go new file mode 100644 index 0000000000..aa3cd9c86c --- /dev/null +++ b/internal/manifest/manifestclient/manifest_client.go @@ -0,0 +1,74 @@ +package manifestclient + +import ( + "context" + "fmt" + + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/kyma-project/lifecycle-manager/api/shared" + "github.com/kyma-project/lifecycle-manager/api/v1beta2" + "github.com/kyma-project/lifecycle-manager/internal/event" +) + +const defaultFieldOwner client.FieldOwner = "declarative.kyma-project.io/applier" + +type ManifestAPIClient interface { + UpdateManifest(ctx context.Context, manifest *v1beta2.Manifest) error + PatchStatusIfDiffExist(ctx context.Context, manifest *v1beta2.Manifest, + previousStatus shared.Status) error + SsaSpec(ctx context.Context, obj client.Object) error +} + +type ManifestClient struct { + client.Client + event.Event +} + +func NewManifestClient(event event.Event, kcpClient client.Client) ManifestClient { + return ManifestClient{ + Event: event, + Client: kcpClient, + } +} + +func (m *ManifestClient) UpdateManifest(ctx context.Context, manifest *v1beta2.Manifest) error { + if err := m.Update(ctx, manifest); err != nil { + m.Warning(manifest, "UpdateObject", err) + return fmt.Errorf("failed to update object: %w", err) + } + return nil +} + +func (m *ManifestClient) PatchStatusIfDiffExist(ctx context.Context, manifest *v1beta2.Manifest, + previousStatus shared.Status, +) error { + if HasStatusDiff(manifest.GetStatus(), previousStatus) { + resetNonPatchableField(manifest) + if err := m.Status().Patch(ctx, manifest, client.Apply, client.ForceOwnership, defaultFieldOwner); err != nil { + m.Warning(manifest, "PatchStatus", err) + return fmt.Errorf("failed to patch status: %w", err) + } + } + + return nil +} + +func (m *ManifestClient) SsaSpec(ctx context.Context, obj client.Object) error { + resetNonPatchableField(obj) + if err := m.Patch(ctx, obj, client.Apply, client.ForceOwnership, defaultFieldOwner); err != nil { + m.Warning(obj, "PatchObject", err) + return fmt.Errorf("failed to patch object: %w", err) + } + return nil +} + +func HasStatusDiff(first, second shared.Status) bool { + return first.State != second.State || first.LastOperation.Operation != second.LastOperation.Operation +} + +func resetNonPatchableField(obj client.Object) { + obj.SetUID("") + obj.SetManagedFields(nil) + obj.SetResourceVersion("") +} diff --git a/internal/manifest/manifestclient/manifest_client_test.go b/internal/manifest/manifestclient/manifest_client_test.go new file mode 100644 index 0000000000..45bf170561 --- /dev/null +++ b/internal/manifest/manifestclient/manifest_client_test.go @@ -0,0 +1,86 @@ +package manifestclient_test + +import ( + "testing" + "time" + + "github.com/stretchr/testify/require" + apimetav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/kyma-project/lifecycle-manager/api/shared" + "github.com/kyma-project/lifecycle-manager/internal/manifest/manifestclient" +) + +func Test_hasStatusDiff(t *testing.T) { + type args struct { + first shared.Status + second shared.Status + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "Different Status", + args: args{ + first: shared.Status{ + State: shared.StateReady, + LastOperation: shared.LastOperation{ + Operation: "resources are ready", + LastUpdateTime: apimetav1.Now(), + }, + }, + second: shared.Status{ + State: shared.StateProcessing, + LastOperation: shared.LastOperation{ + Operation: "installing resources", + LastUpdateTime: apimetav1.Now(), + }, + }, + }, + want: true, + }, + { + name: "Same Status", + args: args{ + first: shared.Status{ + State: shared.StateReady, + LastOperation: shared.LastOperation{ + Operation: "resources are ready", + LastUpdateTime: apimetav1.Now(), + }, + }, + second: shared.Status{ + State: shared.StateReady, + LastOperation: shared.LastOperation{ + Operation: "resources are ready", + LastUpdateTime: apimetav1.NewTime(time.Now().Add(time.Hour)), + }, + }, + }, + want: false, + }, + { + name: "Empty Status", + args: args{ + first: shared.Status{}, + second: shared.Status{ + State: shared.StateReady, + LastOperation: shared.LastOperation{ + Operation: "resources are ready", + LastUpdateTime: apimetav1.NewTime(time.Now().Add(time.Hour)), + }, + }, + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equalf(t, tt.want, manifestclient.HasStatusDiff(tt.args.first, tt.args.second), + "hasStatusDiff(%v, %v)", + tt.args.first, tt.args.second) + }) + } +} diff --git a/tests/integration/controller/manifest/custom_resource_check/suite_test.go b/tests/integration/controller/manifest/custom_resource_check/suite_test.go index b3e52e8511..93f2a1166f 100644 --- a/tests/integration/controller/manifest/custom_resource_check/suite_test.go +++ b/tests/integration/controller/manifest/custom_resource_check/suite_test.go @@ -37,11 +37,14 @@ import ( metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" "github.com/kyma-project/lifecycle-manager/api" + "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/internal" declarativev2 "github.com/kyma-project/lifecycle-manager/internal/declarative/v2" + "github.com/kyma-project/lifecycle-manager/internal/event" "github.com/kyma-project/lifecycle-manager/internal/manifest" "github.com/kyma-project/lifecycle-manager/internal/manifest/img" + "github.com/kyma-project/lifecycle-manager/internal/manifest/manifestclient" "github.com/kyma-project/lifecycle-manager/internal/manifest/statecheck" "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/pkg/log" @@ -141,6 +144,8 @@ var _ = BeforeSuite(func() { statefulChecker := statecheck.NewStatefulSetStateCheck() deploymentChecker := statecheck.NewDeploymentStateCheck() extractor := img.NewPathExtractor() + testEventRec := event.NewRecorderWrapper(mgr.GetEventRecorderFor(shared.OperatorName)) + manifestClient := manifestclient.NewManifestClient(testEventRec, kcpClient) reconciler = declarativev2.NewFromManager(mgr, queue.RequeueIntervals{ Success: 1 * time.Second, Busy: 1 * time.Second, @@ -148,6 +153,7 @@ var _ = BeforeSuite(func() { Warning: 1 * time.Second, }, metrics.NewManifestMetrics(metrics.NewSharedMetrics()), metrics.NewMandatoryModulesMetrics(), + manifestClient, manifest.NewSpecResolver(keyChainLookup, extractor), declarativev2.WithRemoteTargetCluster( func(_ context.Context, _ declarativev2.Object) (*declarativev2.ClusterInfo, error) { diff --git a/tests/integration/controller/manifest/suite_test.go b/tests/integration/controller/manifest/suite_test.go index 5df7ec83b7..3ee5e72dfc 100644 --- a/tests/integration/controller/manifest/suite_test.go +++ b/tests/integration/controller/manifest/suite_test.go @@ -37,11 +37,14 @@ import ( metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" "github.com/kyma-project/lifecycle-manager/api" + "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/internal" declarativev2 "github.com/kyma-project/lifecycle-manager/internal/declarative/v2" + "github.com/kyma-project/lifecycle-manager/internal/event" "github.com/kyma-project/lifecycle-manager/internal/manifest" "github.com/kyma-project/lifecycle-manager/internal/manifest/img" + "github.com/kyma-project/lifecycle-manager/internal/manifest/manifestclient" "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/pkg/log" "github.com/kyma-project/lifecycle-manager/pkg/queue" @@ -138,6 +141,8 @@ var _ = BeforeSuite(func() { kcp := &declarativev2.ClusterInfo{Config: cfg, Client: kcpClient} keyChainLookup := manifest.NewKeyChainProvider(kcp.Client) extractor := img.NewPathExtractor() + testEventRec := event.NewRecorderWrapper(mgr.GetEventRecorderFor(shared.OperatorName)) + manifestClient := manifestclient.NewManifestClient(testEventRec, kcpClient) reconciler = declarativev2.NewFromManager(mgr, queue.RequeueIntervals{ Success: 1 * time.Second, Busy: 1 * time.Second, @@ -145,6 +150,7 @@ var _ = BeforeSuite(func() { Warning: 1 * time.Second, }, metrics.NewManifestMetrics(metrics.NewSharedMetrics()), metrics.NewMandatoryModulesMetrics(), + manifestClient, manifest.NewSpecResolver(keyChainLookup, extractor), declarativev2.WithRemoteTargetCluster( func(_ context.Context, _ declarativev2.Object) (*declarativev2.ClusterInfo, error) { diff --git a/unit-test-coverage.yaml b/unit-test-coverage.yaml index 50122cafa0..64b5638fa4 100644 --- a/unit-test-coverage.yaml +++ b/unit-test-coverage.yaml @@ -6,6 +6,7 @@ packages: internal/manifest/statecheck: 71 internal/manifest/filemutex: 100 internal/manifest: 10 + internal/manifest/labelsremoval: 75 internal/istio: 63.3 internal/pkg/resources: 91.7 internal/remote: 6.4 From f9c62a001bfcf27819655af49797d398e5169734 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Thu, 10 Oct 2024 15:36:56 +0200 Subject: [PATCH 12/15] Remove watched by label since we no longer have it on the resources --- internal/manifest/labelsremoval/labels_removal.go | 6 +----- tests/e2e/module_unmanage_test.go | 8 -------- 2 files changed, 1 insertion(+), 13 deletions(-) diff --git a/internal/manifest/labelsremoval/labels_removal.go b/internal/manifest/labelsremoval/labels_removal.go index 01a90b9466..3a49d61337 100644 --- a/internal/manifest/labelsremoval/labels_removal.go +++ b/internal/manifest/labelsremoval/labels_removal.go @@ -74,12 +74,8 @@ func NeedsUpdateAfterLabelRemoval(resource *unstructured.Unstructured) bool { if managedByLabelExists { delete(labels, shared.ManagedBy) } - _, watchedByLabelExists := labels[shared.WatchedByLabel] - if watchedByLabelExists { - delete(labels, shared.WatchedByLabel) - } resource.SetLabels(labels) - return watchedByLabelExists || managedByLabelExists + return managedByLabelExists } diff --git a/tests/e2e/module_unmanage_test.go b/tests/e2e/module_unmanage_test.go index 714a4f804a..21afdaf428 100644 --- a/tests/e2e/module_unmanage_test.go +++ b/tests/e2e/module_unmanage_test.go @@ -56,10 +56,6 @@ var _ = Describe("Unmanaging Kyma Module", Ordered, func() { WithContext(ctx). WithArguments(skrClient, objectKey, gvk, shared.ManagedBy, shared.ManagedByLabelValue).Should(Succeed()) - Eventually(HasExpectedLabel). - WithContext(ctx). - WithArguments(skrClient, objectKey, gvk, - shared.WatchedByLabel, shared.WatchedByLabelValue).Should(Succeed()) } Eventually(CheckSampleCRHasExpectedLabel). @@ -107,10 +103,6 @@ var _ = Describe("Unmanaging Kyma Module", Ordered, func() { WithContext(ctx). WithArguments(skrClient, objectKey, gvk, shared.ManagedBy, shared.ManagedByLabelValue).Should(Equal(ErrLabelNotFound)) - Eventually(HasExpectedLabel). - WithContext(ctx). - WithArguments(skrClient, objectKey, gvk, - shared.WatchedByLabel, shared.WatchedByLabelValue).Should(Equal(ErrLabelNotFound)) } Eventually(CheckSampleCRHasExpectedLabel). From ae119ec149bf8d4817c2b9174ffba743a3bada2a Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Thu, 10 Oct 2024 16:06:16 +0200 Subject: [PATCH 13/15] fix unit test --- .../labelsremoval/labels_removal_test.go | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/internal/manifest/labelsremoval/labels_removal_test.go b/internal/manifest/labelsremoval/labels_removal_test.go index b7d183be33..ff4ffe0702 100644 --- a/internal/manifest/labelsremoval/labels_removal_test.go +++ b/internal/manifest/labelsremoval/labels_removal_test.go @@ -27,22 +27,6 @@ func Test_needsUpdateAfterLabelRemoval_WhenLabelsAreEmpty(t *testing.T) { require.Equal(t, emptyLabels, res.GetLabels()) } -func Test_needsUpdateAfterLabelRemoval_WhenWatchedByLabel(t *testing.T) { - labels := map[string]string{ - shared.WatchedByLabel: shared.WatchedByLabelValue, - "test": "value", - } - expectedLabels := map[string]string{ - "test": "value", - } - res := &unstructured.Unstructured{} - res.SetLabels(labels) - actual := labelsremoval.NeedsUpdateAfterLabelRemoval(res) - - require.True(t, actual) - require.Equal(t, expectedLabels, res.GetLabels()) -} - func Test_needsUpdateAfterLabelRemoval_WhenManagedByLabel(t *testing.T) { labels := map[string]string{ shared.ManagedBy: shared.ManagedByLabelValue, From 36d4bbe0539a2a39da2768c1ae244b610b5c187c Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Thu, 10 Oct 2024 16:09:32 +0200 Subject: [PATCH 14/15] review comments --- internal/declarative/v2/reconciler.go | 15 +- .../manifest/labelsremoval/labels_removal.go | 44 +++-- .../labelsremoval/labels_removal_test.go | 161 +++++++++++++----- pkg/testutils/builder/manifest.go | 6 + tests/e2e/module_unmanage_test.go | 4 +- unit-test-coverage.yaml | 2 +- 6 files changed, 170 insertions(+), 62 deletions(-) diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index 0acc79ef1c..97ea381db0 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -61,6 +61,7 @@ func NewFromManager(mgr manager.Manager, reconciler.RequeueIntervals = requeueIntervals reconciler.specResolver = specResolver reconciler.manifestClient = manifestClient + reconciler.managedLabelRemovalService = labelsremoval.NewManagedLabelRemovalService(manifestClient) reconciler.Options = DefaultOptions().Apply(WithManager(mgr)).Apply(options...) return reconciler } @@ -68,10 +69,11 @@ func NewFromManager(mgr manager.Manager, type Reconciler struct { queue.RequeueIntervals *Options - ManifestMetrics *metrics.ManifestMetrics - MandatoryModuleMetrics *metrics.MandatoryModulesMetrics - specResolver SpecResolver - manifestClient manifestclient.ManifestClient + ManifestMetrics *metrics.ManifestMetrics + MandatoryModuleMetrics *metrics.MandatoryModulesMetrics + specResolver SpecResolver + manifestClient manifestclient.ManifestClient + managedLabelRemovalService *labelsremoval.ManagedLabelRemovalService } const waitingForResourcesMsg = "waiting for resources to become ready" @@ -250,8 +252,9 @@ func (r *Reconciler) handleLabelsRemovalFinalizer(ctx context.Context, skrClient if err != nil { return ctrl.Result{}, err } - if err := labelsremoval.HandleLabelsRemovalFinalizerForUnmanagedModule(ctx, manifest, skrClient, - r.manifestClient, defaultCR); err != nil { + + if err := r.managedLabelRemovalService.HandleLabelsRemovalFinalizerForUnmanagedModule(ctx, manifest, skrClient, + defaultCR); err != nil { return ctrl.Result{}, err } diff --git a/internal/manifest/labelsremoval/labels_removal.go b/internal/manifest/labelsremoval/labels_removal.go index 3a49d61337..f4dab58983 100644 --- a/internal/manifest/labelsremoval/labels_removal.go +++ b/internal/manifest/labelsremoval/labels_removal.go @@ -16,16 +16,25 @@ import ( const LabelRemovalFinalizer = "label-removal-finalizer" -func HandleLabelsRemovalFinalizerForUnmanagedModule(ctx context.Context, - manifest *v1beta2.Manifest, skrClient client.Client, manifestClnt manifestclient.ManifestClient, - defaultCR *unstructured.Unstructured, +type ManagedLabelRemovalService struct { + manifestClient manifestclient.ManifestClient +} + +func NewManagedLabelRemovalService(manifestClient manifestclient.ManifestClient) *ManagedLabelRemovalService { + return &ManagedLabelRemovalService{ + manifestClient: manifestClient, + } +} + +func (l *ManagedLabelRemovalService) HandleLabelsRemovalFinalizerForUnmanagedModule(ctx context.Context, + manifest *v1beta2.Manifest, skrClient client.Client, defaultCR *unstructured.Unstructured, ) error { if err := HandleLabelsRemovalFromResources(ctx, manifest, skrClient, defaultCR); err != nil { return err } controllerutil.RemoveFinalizer(manifest, LabelRemovalFinalizer) - return manifestClnt.UpdateManifest(ctx, manifest) + return l.manifestClient.UpdateManifest(ctx, manifest) } func HandleLabelsRemovalFromResources(ctx context.Context, manifestCR *v1beta2.Manifest, @@ -36,19 +45,13 @@ func HandleLabelsRemovalFromResources(ctx context.Context, manifestCR *v1beta2.M Name: res.Name, Namespace: res.Namespace, } - gvk := schema.GroupVersionKind{ - Group: res.Group, - Version: res.Version, - Kind: res.Kind, - } - obj := &unstructured.Unstructured{} - obj.SetGroupVersionKind(gvk) + obj := constructResource(res) if err := skrClient.Get(ctx, objectKey, obj); err != nil { return fmt.Errorf("failed to get resource, %w", err) } - if NeedsUpdateAfterLabelRemoval(obj) { + if IsManagedLabelRemoved(obj) { if err := skrClient.Update(ctx, obj); err != nil { return fmt.Errorf("failed to update object: %w", err) } @@ -59,7 +62,7 @@ func HandleLabelsRemovalFromResources(ctx context.Context, manifestCR *v1beta2.M return nil } - if NeedsUpdateAfterLabelRemoval(defaultCR) { + if IsManagedLabelRemoved(defaultCR) { if err := skrClient.Update(ctx, defaultCR); err != nil { return fmt.Errorf("failed to update object: %w", err) } @@ -68,7 +71,20 @@ func HandleLabelsRemovalFromResources(ctx context.Context, manifestCR *v1beta2.M return nil } -func NeedsUpdateAfterLabelRemoval(resource *unstructured.Unstructured) bool { +func constructResource(resource shared.Resource) *unstructured.Unstructured { + gvk := schema.GroupVersionKind{ + Group: resource.Group, + Version: resource.Version, + Kind: resource.Kind, + } + + obj := &unstructured.Unstructured{} + obj.SetGroupVersionKind(gvk) + + return obj +} + +func IsManagedLabelRemoved(resource *unstructured.Unstructured) bool { labels := resource.GetLabels() _, managedByLabelExists := labels[shared.ManagedBy] if managedByLabelExists { diff --git a/internal/manifest/labelsremoval/labels_removal_test.go b/internal/manifest/labelsremoval/labels_removal_test.go index ff4ffe0702..6c04fe71e2 100644 --- a/internal/manifest/labelsremoval/labels_removal_test.go +++ b/internal/manifest/labelsremoval/labels_removal_test.go @@ -15,13 +15,15 @@ import ( "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/internal/manifest/labelsremoval" + "github.com/kyma-project/lifecycle-manager/internal/manifest/manifestclient" + "github.com/kyma-project/lifecycle-manager/pkg/testutils/builder" ) func Test_needsUpdateAfterLabelRemoval_WhenLabelsAreEmpty(t *testing.T) { emptyLabels := map[string]string{} res := &unstructured.Unstructured{} res.SetLabels(emptyLabels) - actual := labelsremoval.NeedsUpdateAfterLabelRemoval(res) + actual := labelsremoval.IsManagedLabelRemoved(res) require.False(t, actual) require.Equal(t, emptyLabels, res.GetLabels()) @@ -34,7 +36,7 @@ func Test_needsUpdateAfterLabelRemoval_WhenManagedByLabel(t *testing.T) { expectedLabels := map[string]string{} res := &unstructured.Unstructured{} res.SetLabels(labels) - actual := labelsremoval.NeedsUpdateAfterLabelRemoval(res) + actual := labelsremoval.IsManagedLabelRemoved(res) require.True(t, actual) require.Equal(t, expectedLabels, res.GetLabels()) @@ -47,30 +49,29 @@ func Test_handleLabelsRemovalFromResources_WhenManifestResourcesHaveLabels(t *te Kind: "TestKind", } - manifest := &v1beta2.Manifest{ - Status: shared.Status{ - Synced: []shared.Resource{ - { - Name: "test-resource-1", - Namespace: "test-1", - GroupVersionKind: apimetav1.GroupVersionKind{ - Group: gvk.Group, - Version: gvk.Version, - Kind: gvk.Kind, - }, + status := shared.Status{ + Synced: []shared.Resource{ + { + Name: "test-resource-1", + Namespace: "test-1", + GroupVersionKind: apimetav1.GroupVersionKind{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, }, - { - Name: "test-resource-2", - Namespace: "test-2", - GroupVersionKind: apimetav1.GroupVersionKind{ - Group: gvk.Group, - Version: gvk.Version, - Kind: gvk.Kind, - }, + }, + { + Name: "test-resource-2", + Namespace: "test-2", + GroupVersionKind: apimetav1.GroupVersionKind{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, }, }, }, } + manifest := builder.NewManifestBuilder().WithStatus(status).Build() objs := []client.Object{ &unstructured.Unstructured{ @@ -123,11 +124,7 @@ func Test_handleLabelsRemovalFromResources_WhenManifestResourcesHaveLabels(t *te } func Test_handleLabelsRemovalFromResources_WhenManifestResourcesAreNilAndNoDefaultCR(t *testing.T) { - manifest := &v1beta2.Manifest{ - Status: shared.Status{ - Synced: []shared.Resource{}, - }, - } + manifest := builder.NewManifestBuilder().Build() scheme := machineryruntime.NewScheme() err := v1beta2.AddToScheme(scheme) @@ -147,21 +144,20 @@ func Test_handleLabelsRemovalFromResources_WhenManifestResourcesAndDefaultCRHave Kind: "TestKind", } - manifest := &v1beta2.Manifest{ - Status: shared.Status{ - Synced: []shared.Resource{ - { - Name: "test-resource-1", - Namespace: "test-1", - GroupVersionKind: apimetav1.GroupVersionKind{ - Group: gvk.Group, - Version: gvk.Version, - Kind: gvk.Kind, - }, + status := shared.Status{ + Synced: []shared.Resource{ + { + Name: "test-resource-1", + Namespace: "test-1", + GroupVersionKind: apimetav1.GroupVersionKind{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, }, }, }, } + manifest := builder.NewManifestBuilder().WithStatus(status).Build() objs := []client.Object{ &unstructured.Unstructured{ @@ -206,8 +202,95 @@ func Test_handleLabelsRemovalFromResources_WhenManifestResourcesAndDefaultCRHave require.NoError(t, err) require.Empty(t, firstObj.GetLabels()) - err = fakeClient.Get(context.TODO(), client.ObjectKey{Name: "default-cr", Namespace: "default-ns"}, - defaultCR) require.NoError(t, err) require.Empty(t, defaultCR.GetLabels()) } + +func Test_HandleLabelsRemovalFinalizerForUnmanagedModule_WhenErrorIsReturned(t *testing.T) { + scheme := machineryruntime.NewScheme() + err := v1beta2.AddToScheme(scheme) + require.NoError(t, err) + + gvk := schema.GroupVersionKind{ + Group: "test-group", + Version: "v1", + Kind: "TestKind", + } + status := shared.Status{ + Synced: []shared.Resource{ + { + Name: "test-resource-1", + Namespace: "test-1", + GroupVersionKind: apimetav1.GroupVersionKind{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + }, + }, + }, + } + manifest := builder.NewManifestBuilder().WithStatus(status).Build() + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() + + manifestClnt := manifestclient.NewManifestClient(nil, fakeClient) + svc := labelsremoval.NewManagedLabelRemovalService(manifestClnt) + + err = svc.HandleLabelsRemovalFinalizerForUnmanagedModule(context.TODO(), manifest, fakeClient, nil) + require.ErrorContains(t, err, "failed to get resource") +} + +func Test_HandleLabelsRemovalFinalizerForUnmanagedModule_WhenFinalizerIsRemoved(t *testing.T) { + scheme := machineryruntime.NewScheme() + err := v1beta2.AddToScheme(scheme) + require.NoError(t, err) + + gvk := schema.GroupVersionKind{ + Group: "test-group", + Version: "v1", + Kind: "TestKind", + } + status := shared.Status{ + Synced: []shared.Resource{ + { + Name: "test-resource-1", + Namespace: "test-1", + GroupVersionKind: apimetav1.GroupVersionKind{ + Group: gvk.Group, + Version: gvk.Version, + Kind: gvk.Kind, + }, + }, + }, + } + finalizers := []string{"label-removal-finalizer"} + manifest := builder.NewManifestBuilder().WithFinalizers(finalizers).WithStatus(status).Build() + + objs := []client.Object{ + &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": gvk.GroupVersion().String(), + "kind": gvk.Kind, + "version": gvk.Version, + }, + }, + } + objs[0].SetName("test-resource-1") + objs[0].SetNamespace("test-1") + objs[0].SetLabels(map[string]string{ + "operator.kyma-project.io/managed-by": "kyma", + }) + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(manifest).WithObjects(objs...).Build() + + manifestClnt := manifestclient.NewManifestClient(nil, fakeClient) + svc := labelsremoval.NewManagedLabelRemovalService(manifestClnt) + + err = svc.HandleLabelsRemovalFinalizerForUnmanagedModule(context.TODO(), manifest, fakeClient, nil) + require.NoError(t, err) + + err = fakeClient.Get(context.TODO(), client.ObjectKey{Name: manifest.GetName(), Namespace: manifest.GetNamespace()}, + manifest) + require.NoError(t, err) + require.Empty(t, manifest.GetFinalizers()) +} diff --git a/pkg/testutils/builder/manifest.go b/pkg/testutils/builder/manifest.go index 25ca91f294..e7a4962689 100644 --- a/pkg/testutils/builder/manifest.go +++ b/pkg/testutils/builder/manifest.go @@ -85,6 +85,12 @@ func (mb ManifestBuilder) WithStatus(status shared.Status) ManifestBuilder { return mb } +// WithFinalizers sets v1beta2.Manifest.Finalizers. +func (mb ManifestBuilder) WithFinalizers(finalizers []string) ManifestBuilder { + mb.manifest.Finalizers = finalizers + return mb +} + // Build returns the built v1beta2.Manifest. func (mb ManifestBuilder) Build() *v1beta2.Manifest { return mb.manifest diff --git a/tests/e2e/module_unmanage_test.go b/tests/e2e/module_unmanage_test.go index 21afdaf428..406b59b010 100644 --- a/tests/e2e/module_unmanage_test.go +++ b/tests/e2e/module_unmanage_test.go @@ -40,7 +40,7 @@ var _ = Describe("Unmanaging Kyma Module", Ordered, func() { WithContext(ctx). WithArguments(kyma.GetName(), kyma.GetNamespace(), kcpClient, shared.StateReady). Should(Succeed()) - By("And all manifest resources and default CR have managed-by and watched-by labels") + By("And all manifest resources and default CR have managed-by label") manifest, err := GetManifest(ctx, kcpClient, kyma.GetName(), kyma.GetNamespace(), module.Name) Expect(err).Should(Succeed()) @@ -91,7 +91,7 @@ var _ = Describe("Unmanaging Kyma Module", Ordered, func() { "apps", "v1", "Deployment", skrClient). Should(Succeed()) - By("And all manifest resources and default CR no longer have managed-by or watched-by labels") + By("And all manifest resources and default CR no longer have managed-by labels") for _, resource := range manifestResources { objectKey := client.ObjectKey{Name: resource.Name, Namespace: resource.Namespace} gvk := schema.GroupVersionKind{ diff --git a/unit-test-coverage.yaml b/unit-test-coverage.yaml index 64b5638fa4..0f142680e3 100644 --- a/unit-test-coverage.yaml +++ b/unit-test-coverage.yaml @@ -6,7 +6,7 @@ packages: internal/manifest/statecheck: 71 internal/manifest/filemutex: 100 internal/manifest: 10 - internal/manifest/labelsremoval: 75 + internal/manifest/labelsremoval: 93 internal/istio: 63.3 internal/pkg/resources: 91.7 internal/remote: 6.4 From 4a27fa98f86f3e6da02d9b75c9db91c47af13cf0 Mon Sep 17 00:00:00 2001 From: "Badr, Nesma" Date: Fri, 11 Oct 2024 10:04:57 +0200 Subject: [PATCH 15/15] Review comments - 2 --- cmd/main.go | 2 +- internal/controller/manifest/controller.go | 3 +-- internal/controller/manifest/setup.go | 4 +-- internal/declarative/v2/reconciler.go | 26 ++++++++++++++----- .../manifest/labelsremoval/labels_removal.go | 11 +++++--- .../labelsremoval/labels_removal_test.go | 8 +++--- .../manifestclient/manifest_client.go | 11 ++------ 7 files changed, 36 insertions(+), 29 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index 86d3fab319..4c31955258 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -69,7 +69,6 @@ import ( _ "github.com/open-component-model/ocm/pkg/contexts/ocm" _ "k8s.io/client-go/plugin/pkg/client/auth" //nolint:gci // kubebuilder's scaffold imports must be appended here. - // +kubebuilder:scaffold:imports ) const ( @@ -391,6 +390,7 @@ func setupManifestReconciler(mgr ctrl.Manager, flagVar *flags.FlagVar, options c flagVar.FailureMaxDelay, flagVar.RateLimiterFrequency, flagVar.RateLimiterBurst) manifestClient := manifestclient.NewManifestClient(event, mgr.GetClient()) + if err := manifest.SetupWithManager( mgr, options, queue.RequeueIntervals{ Success: flagVar.ManifestRequeueSuccessInterval, diff --git a/internal/controller/manifest/controller.go b/internal/controller/manifest/controller.go index bd841b576a..ba5fca9e6f 100644 --- a/internal/controller/manifest/controller.go +++ b/internal/controller/manifest/controller.go @@ -6,7 +6,6 @@ import ( declarativev2 "github.com/kyma-project/lifecycle-manager/internal/declarative/v2" "github.com/kyma-project/lifecycle-manager/internal/manifest" "github.com/kyma-project/lifecycle-manager/internal/manifest/img" - "github.com/kyma-project/lifecycle-manager/internal/manifest/manifestclient" "github.com/kyma-project/lifecycle-manager/internal/manifest/statecheck" "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/pkg/queue" @@ -20,7 +19,7 @@ func NewReconciler(mgr manager.Manager, requeueIntervals queue.RequeueIntervals, manifestMetrics *metrics.ManifestMetrics, mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, - manifestClient manifestclient.ManifestClient, + manifestClient declarativev2.ManifestAPIClient, ) *declarativev2.Reconciler { kcp := &declarativev2.ClusterInfo{ Client: mgr.GetClient(), diff --git a/internal/controller/manifest/setup.go b/internal/controller/manifest/setup.go index 10ca096e75..617e9ca1cd 100644 --- a/internal/controller/manifest/setup.go +++ b/internal/controller/manifest/setup.go @@ -22,7 +22,7 @@ import ( "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" - "github.com/kyma-project/lifecycle-manager/internal/manifest/manifestclient" + v2 "github.com/kyma-project/lifecycle-manager/internal/declarative/v2" "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/pkg/queue" "github.com/kyma-project/lifecycle-manager/pkg/security" @@ -41,7 +41,7 @@ func SetupWithManager(mgr manager.Manager, settings SetupOptions, manifestMetrics *metrics.ManifestMetrics, mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, - manifestClient manifestclient.ManifestClient, + manifestClient v2.ManifestAPIClient, ) error { var verifyFunc watcherevent.Verify if settings.EnableDomainNameVerification { diff --git a/internal/declarative/v2/reconciler.go b/internal/declarative/v2/reconciler.go index 97ea381db0..6eae00eea8 100644 --- a/internal/declarative/v2/reconciler.go +++ b/internal/declarative/v2/reconciler.go @@ -21,7 +21,6 @@ import ( "github.com/kyma-project/lifecycle-manager/api/v1beta2" "github.com/kyma-project/lifecycle-manager/internal" "github.com/kyma-project/lifecycle-manager/internal/manifest/labelsremoval" - "github.com/kyma-project/lifecycle-manager/internal/manifest/manifestclient" "github.com/kyma-project/lifecycle-manager/internal/pkg/metrics" "github.com/kyma-project/lifecycle-manager/internal/pkg/resources" "github.com/kyma-project/lifecycle-manager/pkg/common" @@ -51,7 +50,7 @@ func NewFromManager(mgr manager.Manager, requeueIntervals queue.RequeueIntervals, metrics *metrics.ManifestMetrics, mandatoryModulesMetrics *metrics.MandatoryModulesMetrics, - manifestClient manifestclient.ManifestClient, + manifestAPIClient ManifestAPIClient, specResolver SpecResolver, options ...Option, ) *Reconciler { @@ -60,20 +59,33 @@ func NewFromManager(mgr manager.Manager, reconciler.MandatoryModuleMetrics = mandatoryModulesMetrics reconciler.RequeueIntervals = requeueIntervals reconciler.specResolver = specResolver - reconciler.manifestClient = manifestClient - reconciler.managedLabelRemovalService = labelsremoval.NewManagedLabelRemovalService(manifestClient) + reconciler.manifestClient = manifestAPIClient + reconciler.managedLabelRemovalService = labelsremoval.NewManagedLabelRemovalService(manifestAPIClient) reconciler.Options = DefaultOptions().Apply(WithManager(mgr)).Apply(options...) return reconciler } +type ManagedLabelRemoval interface { + RemoveManagedLabel(ctx context.Context, + manifest *v1beta2.Manifest, skrClient client.Client, defaultCR *unstructured.Unstructured, + ) error +} + +type ManifestAPIClient interface { + UpdateManifest(ctx context.Context, manifest *v1beta2.Manifest) error + PatchStatusIfDiffExist(ctx context.Context, manifest *v1beta2.Manifest, + previousStatus shared.Status) error + SsaSpec(ctx context.Context, obj client.Object) error +} + type Reconciler struct { queue.RequeueIntervals *Options ManifestMetrics *metrics.ManifestMetrics MandatoryModuleMetrics *metrics.MandatoryModulesMetrics specResolver SpecResolver - manifestClient manifestclient.ManifestClient - managedLabelRemovalService *labelsremoval.ManagedLabelRemovalService + manifestClient ManifestAPIClient + managedLabelRemovalService ManagedLabelRemoval } const waitingForResourcesMsg = "waiting for resources to become ready" @@ -253,7 +265,7 @@ func (r *Reconciler) handleLabelsRemovalFinalizer(ctx context.Context, skrClient return ctrl.Result{}, err } - if err := r.managedLabelRemovalService.HandleLabelsRemovalFinalizerForUnmanagedModule(ctx, manifest, skrClient, + if err := r.managedLabelRemovalService.RemoveManagedLabel(ctx, manifest, skrClient, defaultCR); err != nil { return ctrl.Result{}, err } diff --git a/internal/manifest/labelsremoval/labels_removal.go b/internal/manifest/labelsremoval/labels_removal.go index f4dab58983..5e397c1cb9 100644 --- a/internal/manifest/labelsremoval/labels_removal.go +++ b/internal/manifest/labelsremoval/labels_removal.go @@ -11,22 +11,25 @@ import ( "github.com/kyma-project/lifecycle-manager/api/shared" "github.com/kyma-project/lifecycle-manager/api/v1beta2" - "github.com/kyma-project/lifecycle-manager/internal/manifest/manifestclient" ) const LabelRemovalFinalizer = "label-removal-finalizer" +type ManifestAPIClient interface { + UpdateManifest(ctx context.Context, manifest *v1beta2.Manifest) error +} + type ManagedLabelRemovalService struct { - manifestClient manifestclient.ManifestClient + manifestClient ManifestAPIClient } -func NewManagedLabelRemovalService(manifestClient manifestclient.ManifestClient) *ManagedLabelRemovalService { +func NewManagedLabelRemovalService(manifestClient ManifestAPIClient) *ManagedLabelRemovalService { return &ManagedLabelRemovalService{ manifestClient: manifestClient, } } -func (l *ManagedLabelRemovalService) HandleLabelsRemovalFinalizerForUnmanagedModule(ctx context.Context, +func (l *ManagedLabelRemovalService) RemoveManagedLabel(ctx context.Context, manifest *v1beta2.Manifest, skrClient client.Client, defaultCR *unstructured.Unstructured, ) error { if err := HandleLabelsRemovalFromResources(ctx, manifest, skrClient, defaultCR); err != nil { diff --git a/internal/manifest/labelsremoval/labels_removal_test.go b/internal/manifest/labelsremoval/labels_removal_test.go index 6c04fe71e2..ec0f458818 100644 --- a/internal/manifest/labelsremoval/labels_removal_test.go +++ b/internal/manifest/labelsremoval/labels_removal_test.go @@ -206,7 +206,7 @@ func Test_handleLabelsRemovalFromResources_WhenManifestResourcesAndDefaultCRHave require.Empty(t, defaultCR.GetLabels()) } -func Test_HandleLabelsRemovalFinalizerForUnmanagedModule_WhenErrorIsReturned(t *testing.T) { +func Test_RemoveManagedLabel_WhenErrorIsReturned(t *testing.T) { scheme := machineryruntime.NewScheme() err := v1beta2.AddToScheme(scheme) require.NoError(t, err) @@ -236,11 +236,11 @@ func Test_HandleLabelsRemovalFinalizerForUnmanagedModule_WhenErrorIsReturned(t * manifestClnt := manifestclient.NewManifestClient(nil, fakeClient) svc := labelsremoval.NewManagedLabelRemovalService(manifestClnt) - err = svc.HandleLabelsRemovalFinalizerForUnmanagedModule(context.TODO(), manifest, fakeClient, nil) + err = svc.RemoveManagedLabel(context.TODO(), manifest, fakeClient, nil) require.ErrorContains(t, err, "failed to get resource") } -func Test_HandleLabelsRemovalFinalizerForUnmanagedModule_WhenFinalizerIsRemoved(t *testing.T) { +func Test_RemoveManagedLabel_WhenFinalizerIsRemoved(t *testing.T) { scheme := machineryruntime.NewScheme() err := v1beta2.AddToScheme(scheme) require.NoError(t, err) @@ -286,7 +286,7 @@ func Test_HandleLabelsRemovalFinalizerForUnmanagedModule_WhenFinalizerIsRemoved( manifestClnt := manifestclient.NewManifestClient(nil, fakeClient) svc := labelsremoval.NewManagedLabelRemovalService(manifestClnt) - err = svc.HandleLabelsRemovalFinalizerForUnmanagedModule(context.TODO(), manifest, fakeClient, nil) + err = svc.RemoveManagedLabel(context.TODO(), manifest, fakeClient, nil) require.NoError(t, err) err = fakeClient.Get(context.TODO(), client.ObjectKey{Name: manifest.GetName(), Namespace: manifest.GetNamespace()}, diff --git a/internal/manifest/manifestclient/manifest_client.go b/internal/manifest/manifestclient/manifest_client.go index aa3cd9c86c..4eddf5a03e 100644 --- a/internal/manifest/manifestclient/manifest_client.go +++ b/internal/manifest/manifestclient/manifest_client.go @@ -13,20 +13,13 @@ import ( const defaultFieldOwner client.FieldOwner = "declarative.kyma-project.io/applier" -type ManifestAPIClient interface { - UpdateManifest(ctx context.Context, manifest *v1beta2.Manifest) error - PatchStatusIfDiffExist(ctx context.Context, manifest *v1beta2.Manifest, - previousStatus shared.Status) error - SsaSpec(ctx context.Context, obj client.Object) error -} - type ManifestClient struct { client.Client event.Event } -func NewManifestClient(event event.Event, kcpClient client.Client) ManifestClient { - return ManifestClient{ +func NewManifestClient(event event.Event, kcpClient client.Client) *ManifestClient { + return &ManifestClient{ Event: event, Client: kcpClient, }