From 6d5dc6783f6f5571b6d80568d97126e1311c02ed Mon Sep 17 00:00:00 2001 From: mikutas <23391543+mikutas@users.noreply.github.com> Date: Thu, 12 Oct 2023 00:25:34 +0900 Subject: [PATCH 1/4] fix(applicationset): prevent app deletion according to appset policy Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com> --- .../controllers/applicationset_controller.go | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go index be73c81ca0c3f..7a3e053cf2ab5 100644 --- a/applicationset/controllers/applicationset_controller.go +++ b/applicationset/controllers/applicationset_controller.go @@ -108,6 +108,18 @@ func (r *ApplicationSetReconciler) Reconcile(ctx context.Context, req ctrl.Reque // Do not attempt to further reconcile the ApplicationSet if it is being deleted. if applicationSetInfo.ObjectMeta.DeletionTimestamp != nil { + if controllerutil.ContainsFinalizer(&applicationSetInfo, argov1alpha1.ResourcesFinalizerName) { + deleteAllowed := utils.DefaultPolicy(applicationSetInfo.Spec.SyncPolicy, r.Policy, r.EnablePolicyOverride).AllowDelete() + if !deleteAllowed { + if err := r.removeOwnerReferencesOnDeleteAppSet(ctx, applicationSetInfo); err != nil { + return ctrl.Result{}, err + } + controllerutil.RemoveFinalizer(&applicationSetInfo, argov1alpha1.ResourcesFinalizerName) + if err := r.Update(ctx, &applicationSetInfo); err != nil { + return ctrl.Result{}, err + } + } + } return ctrl.Result{}, nil } @@ -875,6 +887,23 @@ func (r *ApplicationSetReconciler) removeFinalizerOnInvalidDestination(ctx conte return nil } +func (r *ApplicationSetReconciler) removeOwnerReferencesOnDeleteAppSet(ctx context.Context, applicationSet argov1alpha1.ApplicationSet) error { + applications, err := r.getCurrentApplications(ctx, applicationSet) + if err != nil { + return err + } + + for _, app := range applications { + app.SetOwnerReferences([]metav1.OwnerReference{}) + err := r.Client.Update(ctx, &app) + if err != nil { + return err + } + } + + return nil +} + func (r *ApplicationSetReconciler) performProgressiveSyncs(ctx context.Context, logCtx *log.Entry, appset argov1alpha1.ApplicationSet, applications []argov1alpha1.Application, desiredApplications []argov1alpha1.Application, appMap map[string]argov1alpha1.Application) (map[string]bool, error) { appDependencyList, appStepMap, err := r.buildAppDependencyList(logCtx, appset, desiredApplications) From 18657d4313d454e7bf1c7cca47f437a6ae88cf93 Mon Sep 17 00:00:00 2001 From: mikutas <23391543+mikutas@users.noreply.github.com> Date: Fri, 13 Oct 2023 15:34:39 +0900 Subject: [PATCH 2/4] test: add unit test Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com> --- .../applicationset_controller_test.go | 75 +++++++++++++++++++ 1 file changed, 75 insertions(+) diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go index add37780e7077..9e72019b4d64b 100644 --- a/applicationset/controllers/applicationset_controller_test.go +++ b/applicationset/controllers/applicationset_controller_test.go @@ -1593,6 +1593,81 @@ func TestRemoveFinalizerOnInvalidDestination_DestinationTypes(t *testing.T) { } } +func TestRemoveOwnerReferencesOnDeleteAppSet(t *testing.T) { + scheme := runtime.NewScheme() + err := v1alpha1.AddToScheme(scheme) + assert.Nil(t, err) + + err = v1alpha1.AddToScheme(scheme) + assert.Nil(t, err) + + for _, c := range []struct { + // name is human-readable test name + name string + }{ + { + name: "ownerReferences cleared", + }, + } { + t.Run(c.name, func(t *testing.T) { + appSet := v1alpha1.ApplicationSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "name", + Namespace: "namespace", + Finalizers: []string{v1alpha1.ResourcesFinalizerName}, + }, + Spec: v1alpha1.ApplicationSetSpec{ + Template: v1alpha1.ApplicationSetTemplate{ + Spec: v1alpha1.ApplicationSpec{ + Project: "project", + }, + }, + }, + } + + app := v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "app1", + Namespace: "namespace", + }, + Spec: v1alpha1.ApplicationSpec{ + Project: "project", + Source: &v1alpha1.ApplicationSource{Path: "path", TargetRevision: "revision", RepoURL: "repoURL"}, + Destination: v1alpha1.ApplicationDestination{ + Namespace: "namespace", + Server: "https://kubernetes.default.svc", + }, + }, + } + + err := controllerutil.SetControllerReference(&appSet, &app, scheme) + assert.NoError(t, err, "Unexpected error") + + initObjs := []crtclient.Object{&app, &appSet} + + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(initObjs...).Build() + + r := ApplicationSetReconciler{ + Client: client, + Scheme: scheme, + Recorder: record.NewFakeRecorder(10), + KubeClientset: nil, + Cache: &fakeCache{}, + } + + err = r.removeOwnerReferencesOnDeleteAppSet(context.Background(), appSet) + assert.NoError(t, err, "Unexpected error") + + retrievedApp := v1alpha1.Application{} + err = client.Get(context.Background(), crtclient.ObjectKeyFromObject(&app), &retrievedApp) + assert.NoError(t, err, "Unexpected error") + + ownerReferencesRemoved := len(retrievedApp.OwnerReferences) == 0 + assert.True(t, ownerReferencesRemoved) + }) + } +} + func TestCreateApplications(t *testing.T) { scheme := runtime.NewScheme() From ee15c4b312a01b06685666a41305cfbbdb9568f3 Mon Sep 17 00:00:00 2001 From: mikutas <23391543+mikutas@users.noreply.github.com> Date: Thu, 19 Oct 2023 08:57:48 +0900 Subject: [PATCH 3/4] fix: unit test Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com> --- applicationset/controllers/applicationset_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go index 9e72019b4d64b..65acce4b38edc 100644 --- a/applicationset/controllers/applicationset_controller_test.go +++ b/applicationset/controllers/applicationset_controller_test.go @@ -1645,7 +1645,7 @@ func TestRemoveOwnerReferencesOnDeleteAppSet(t *testing.T) { initObjs := []crtclient.Object{&app, &appSet} - client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(initObjs...).Build() + client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(initObjs...).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).Build() r := ApplicationSetReconciler{ Client: client, From 37ef33dd1dfae61446b85a4ce093838cd8b37c72 Mon Sep 17 00:00:00 2001 From: mikutas <23391543+mikutas@users.noreply.github.com> Date: Thu, 19 Oct 2023 10:25:03 +0900 Subject: [PATCH 4/4] fix: remove TODO Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com> --- applicationset/controllers/applicationset_controller.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go index 7a3e053cf2ab5..5f1205caf384a 100644 --- a/applicationset/controllers/applicationset_controller.go +++ b/applicationset/controllers/applicationset_controller.go @@ -743,10 +743,9 @@ func (r *ApplicationSetReconciler) createInCluster(ctx context.Context, logCtx * return r.createOrUpdateInCluster(ctx, logCtx, applicationSet, createApps) } -func (r *ApplicationSetReconciler) getCurrentApplications(_ context.Context, applicationSet argov1alpha1.ApplicationSet) ([]argov1alpha1.Application, error) { - // TODO: Should this use the context param? +func (r *ApplicationSetReconciler) getCurrentApplications(ctx context.Context, applicationSet argov1alpha1.ApplicationSet) ([]argov1alpha1.Application, error) { var current argov1alpha1.ApplicationList - err := r.Client.List(context.Background(), ¤t, client.MatchingFields{".metadata.controller": applicationSet.Name}, client.InNamespace(applicationSet.Namespace)) + err := r.Client.List(ctx, ¤t, client.MatchingFields{".metadata.controller": applicationSet.Name}, client.InNamespace(applicationSet.Namespace)) if err != nil { return nil, fmt.Errorf("error retrieving applications: %w", err)