Skip to content

Commit

Permalink
review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@nesmabadr
nesmabadr committed Oct 10, 2024
1 parent ae119ec commit 36d4bbe
Show file tree
Hide file tree
Showing 6 changed files with 170 additions and 62 deletions.
15 changes: 9 additions & 6 deletions internal/declarative/v2/reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,19 @@ 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
}

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"
Expand Down Expand Up @@ -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
}

Expand Down
44 changes: 30 additions & 14 deletions internal/manifest/labelsremoval/labels_removal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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 {
Expand Down
161 changes: 122 additions & 39 deletions internal/manifest/labelsremoval/labels_removal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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())
Expand All @@ -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{
Expand Down Expand Up @@ -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)
Expand All @@ -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{
Expand Down Expand Up @@ -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())
}
6 changes: 6 additions & 0 deletions pkg/testutils/builder/manifest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/module_unmanage_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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{
Expand Down
2 changes: 1 addition & 1 deletion unit-test-coverage.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 36d4bbe

Please sign in to comment.