From 82058d5a1917514cf6fd738f6f7225f96b88cfac Mon Sep 17 00:00:00 2001 From: anandf Date: Fri, 7 Feb 2025 06:31:13 +0530 Subject: [PATCH] Using impersonation in app deletion phase Signed-off-by: anandf --- controller/appcontroller.go | 20 +++++ controller/appcontroller_test.go | 139 +++++++++++++++++++++++++++++++ test/e2e/app_deletion_test.go | 83 ++++++++++++++++++ 3 files changed, 242 insertions(+) diff --git a/controller/appcontroller.go b/controller/appcontroller.go index 025566c15b32d..21561b4ca2e09 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -16,6 +16,8 @@ import ( "sync" "time" + "k8s.io/client-go/rest" + clustercache "github.com/argoproj/gitops-engine/pkg/cache" "github.com/argoproj/gitops-engine/pkg/diff" "github.com/argoproj/gitops-engine/pkg/health" @@ -1245,6 +1247,24 @@ func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Applic } logCtx.Infof("Deleting application's resources with %s propagation policy", propagationPolicy) + impersonationEnabled, err := ctrl.settingsMgr.IsImpersonationEnabled() + if err != nil { + logCtx.Errorf("could not get impersonation feature flag: %v", err) + return err + } + if impersonationEnabled { + serviceAccountToImpersonate, err := deriveServiceAccountToImpersonate(proj, app) + if err != nil { + logCtx.Errorf("could not get service account for impersonation: %v", err) + return err + } + logCtx = logCtx.WithFields(log.Fields{"impersonationEnabled": "true", "serviceAccount": serviceAccountToImpersonate}) + // set the impersonation headers. + config.Impersonate = rest.ImpersonationConfig{ + UserName: serviceAccountToImpersonate, + } + } + err = kube.RunAllAsync(len(filteredObjs), func(i int) error { obj := filteredObjs[i] return ctrl.kubectl.DeleteResource(context.Background(), config, obj.GroupVersionKind(), obj.GetName(), obj.GetNamespace(), metav1.DeleteOptions{PropagationPolicy: &propagationPolicy}) diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index c09e922a3cec2..ced186f8a96aa 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "fmt" + "strconv" "testing" "time" @@ -2651,3 +2652,141 @@ func TestSyncTimeout(t *testing.T) { }) } } + +func TestFinalizeApplicationDeletionWithImpersonate(t *testing.T) { + now := metav1.Now() + type fixture struct { + application *v1alpha1.Application + controller *ApplicationController + patched *bool + } + + setup := func(impersonationEnabled bool, destinationNamespace, serviceAccountName string) *fixture { + app := newFakeApp() + app.SetCascadedDeletion(v1alpha1.ResourcesFinalizerName) + app.DeletionTimestamp = &now + app.Spec.Destination.Namespace = test.FakeArgoCDNamespace + app.Status.OperationState = nil + app.Status.History = nil + + project := v1alpha1.AppProject{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: test.FakeArgoCDNamespace, + Name: "default", + }, + Spec: v1alpha1.AppProjectSpec{ + SourceRepos: []string{"*"}, + Destinations: []v1alpha1.ApplicationDestination{ + { + Server: "*", + Namespace: "*", + }, + }, + DestinationServiceAccounts: []v1alpha1. + ApplicationDestinationServiceAccount{ + { + Server: "https://localhost:6443", + Namespace: destinationNamespace, + DefaultServiceAccount: serviceAccountName, + }, + }, + }, + } + additionalObjs := make([]runtime.Object, 0) + if serviceAccountName != "" { + syncServiceAccount := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: serviceAccountName, + Namespace: test.FakeDestNamespace, + }, + } + additionalObjs = append(additionalObjs, syncServiceAccount) + } + data := fakeData{ + apps: []runtime.Object{app, &project}, + manifestResponse: &apiclient.ManifestResponse{ + Manifests: []string{}, + Namespace: test.FakeDestNamespace, + Server: "https://localhost:6443", + Revision: "abc123", + }, + managedLiveObjs: map[kube.ResourceKey]*unstructured.Unstructured{}, + configMapData: map[string]string{ + "application.sync.impersonation.enabled": strconv.FormatBool(impersonationEnabled), + }, + additionalObjs: additionalObjs, + } + ctrl := newFakeController(&data, nil) + patched := false + fakeAppCs := ctrl.applicationClientset.(*appclientset.Clientset) + defaultReactor := fakeAppCs.ReactionChain[0] + fakeAppCs.ReactionChain = nil + fakeAppCs.AddReactor("get", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) { + return defaultReactor.React(action) + }) + fakeAppCs.AddReactor("patch", "*", func(_ kubetesting.Action) (handled bool, ret runtime.Object, err error) { + patched = true + return true, &v1alpha1.Application{}, nil + }) + + return &fixture{ + application: app, + controller: ctrl, + patched: &patched, + } + } + + t.Run("sync with impersonation and no matching service account", func(t *testing.T) { + // given app sync impersonation feature is enabled with an application referring a project no matching service account + f := setup(true, test.FakeArgoCDNamespace, "") + // when + err := f.controller.finalizeApplicationDeletion(f.application, func(_ string) ([]*v1alpha1.Cluster, error) { + return []*v1alpha1.Cluster{}, nil + }) + + // then, app sync should fail with expected error message in operation state + require.ErrorContains(t, err, "default service account contains invalid chars ''") + require.False(t, *f.patched) + }) + + t.Run("sync with impersonation and empty service account match", func(t *testing.T) { + // given app sync impersonation feature is enabled with an application referring a project matching service account that is an empty string + f := setup(true, test.FakeArgoCDNamespace, "") + // when + err := f.controller.finalizeApplicationDeletion(f.application, func(_ string) ([]*v1alpha1.Cluster, error) { + return []*v1alpha1.Cluster{}, nil + }) + + // then, app sync should fail with expected error message in operation state + require.ErrorContains(t, err, "default service account contains invalid chars ''") + require.False(t, *f.patched) + }) + + t.Run("sync with impersonation and matching sa", func(t *testing.T) { + // given app sync impersonation feature is enabled with an application referring a project matching service account + f := setup(true, test.FakeArgoCDNamespace, "test-sa") + + // when + err := f.controller.finalizeApplicationDeletion(f.application, func(_ string) ([]*v1alpha1.Cluster, error) { + return []*v1alpha1.Cluster{}, nil + }) + + // then, app sync should fail with expected error message in operation state + require.NoError(t, err) + require.True(t, *f.patched) + }) + + t.Run("sync without impersonation", func(t *testing.T) { + // given app sync impersonation feature is disabled with an application referring a project matching service account + f := setup(false, test.FakeDestNamespace, "") + + // when + err := f.controller.finalizeApplicationDeletion(f.application, func(_ string) ([]*v1alpha1.Cluster, error) { + return []*v1alpha1.Cluster{}, nil + }) + + // then, app sync should fail with expected error message in operation state + require.NoError(t, err) + require.True(t, *f.patched) + }) +} diff --git a/test/e2e/app_deletion_test.go b/test/e2e/app_deletion_test.go index caea0da09f12d..d362fa55ff8ae 100644 --- a/test/e2e/app_deletion_test.go +++ b/test/e2e/app_deletion_test.go @@ -1,14 +1,20 @@ package e2e import ( + "fmt" + "strings" "testing" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + . "github.com/argoproj/gitops-engine/pkg/sync/common" "github.com/stretchr/testify/assert" . "github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1" . "github.com/argoproj/argo-cd/v3/test/e2e/fixture" . "github.com/argoproj/argo-cd/v3/test/e2e/fixture/app" + "github.com/argoproj/argo-cd/v3/test/e2e/fixture/project" "github.com/argoproj/argo-cd/v3/util/errors" ) @@ -83,3 +89,80 @@ func TestDeletingAppByLabelWait(t *testing.T) { // delete is successful Expect(DoesNotExistNow()) } + +func TestDeletingAppWithImpersonation(t *testing.T) { + projectCtx := project.Given(t) + appCtx := Given(t) + + projectCtx. + Name("impersonation-test"). + SourceNamespaces([]string{"*"}). + SourceRepositories([]string{"*"}). + Destination("*,*"). + DestinationServiceAccounts([]string{fmt.Sprintf("%s,%s,%s", KubernetesInternalAPIServerAddr, "*", "default-sa")}). + When(). + Create(). + Then(). + Expect() + + appCtx. + Project("impersonation-test"). + Path(guestbookPath). + When(). + CreateApp("--label=foo=bar"). + WithImpersonationEnabled("default-sa", []rbacv1.PolicyRule{ + { + APIGroups: []string{"*"}, + Resources: []string{"*"}, + Verbs: []string{"*"}, + }, + }). + Sync(). + Then(). + Expect(SyncStatusIs(SyncStatusCodeSynced)). + When(). + DeleteBySelectorWithWait("foo=bar"). + Then(). + Expect(DoesNotExist()) +} + +func TestDeletingAppWithImpersonationMissingDeletePermission(t *testing.T) { + projectName := "impersonation-missing-delete-permission" + serviceAccountName := "default-sa" + + projectCtx := project.Given(t) + appCtx := Given(t) + + projectCtx. + Name(projectName). + SourceNamespaces([]string{"*"}). + SourceRepositories([]string{"*"}). + Destination("*,*"). + DestinationServiceAccounts([]string{fmt.Sprintf("%s,%s,%s", KubernetesInternalAPIServerAddr, "*", serviceAccountName)}). + When(). + Create(). + Then(). + Expect() + + appCtx. + Project(projectName). + Path(guestbookPath). + When(). + CreateApp("--label=foo=bar"). + WithImpersonationEnabled(serviceAccountName, []rbacv1.PolicyRule{ + { + APIGroups: []string{"*"}, + Resources: []string{"*"}, + Verbs: []string{"get", "list", "watch", "create", "update", "patch"}, + }, + }). + Sync(). + Then(). + Expect(SyncStatusIs(SyncStatusCodeSynced)). + When(). + Delete(false). + Then(). + Expect(DoesNotExist()). + ExpectConsistently(DoesNotExist(), WaitDuration, TimeoutDuration). + ExpectConsistently(Pod(func(p corev1.Pod) bool { return strings.HasPrefix(p.Name, "guestbook-ui") }), WaitDuration, TimeoutDuration) +}