From 7c492f12857732101a1715ff487acf5064362188 Mon Sep 17 00:00:00 2001 From: Takumi Sue <23391543+mikutas@users.noreply.github.com> Date: Fri, 10 Nov 2023 13:06:54 +0900 Subject: [PATCH] fix(appset): prevent app deletion according to appset policy (#12172) (#15903) * fix(applicationset): prevent app deletion according to appset policy Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com> * test: add unit test Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com> * fix: unit test Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com> * fix: remove TODO Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com> --------- Signed-off-by: mikutas <23391543+mikutas@users.noreply.github.com> Signed-off-by: jmilic1 <70441727+jmilic1@users.noreply.github.com> --- .../controllers/applicationset_controller.go | 34 ++++++++- .../applicationset_controller_test.go | 75 +++++++++++++++++++ 2 files changed, 106 insertions(+), 3 deletions(-) diff --git a/applicationset/controllers/applicationset_controller.go b/applicationset/controllers/applicationset_controller.go index be73c81ca0c3f..5f1205caf384a 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 } @@ -731,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) @@ -875,6 +886,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) diff --git a/applicationset/controllers/applicationset_controller_test.go b/applicationset/controllers/applicationset_controller_test.go index add37780e7077..65acce4b38edc 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...).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).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()