From 19583279e7e43a0a34110071498cc69b9cb5df82 Mon Sep 17 00:00:00 2001 From: anandf Date: Mon, 16 Sep 2024 18:59:03 +0530 Subject: [PATCH 01/20] fixed doc comments and added unit tests Signed-off-by: anandf --- controller/appcontroller_test.go | 5 +- controller/sync.go | 11 +- controller/sync_test.go | 729 ++++++++++++++++++ .../app-sync-using-impersonation.md | 4 +- docs/operator-manual/argocd-cm.yaml | 8 +- ...plication-sync-user-using-impersonation.md | 13 +- pkg/apis/application/v1alpha1/types.go | 6 +- util/settings/settings.go | 11 +- 8 files changed, 765 insertions(+), 22 deletions(-) diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index 50fb08042719d..cd4c84b788ecf 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -67,6 +67,7 @@ type fakeData struct { metricsCacheExpiration time.Duration applicationNamespaces []string updateRevisionForPathsResponse *apiclient.UpdateRevisionForPathsResponse + additionalsObjs []runtime.Object } type MockKubectl struct { @@ -136,7 +137,9 @@ func newFakeController(data *fakeData, repoErr error) *ApplicationController { }, Data: data.configMapData, } - kubeClient := fake.NewSimpleClientset(&clust, &cm, &secret) + runtimeObjs := []runtime.Object{&clust, &secret, &cm} + runtimeObjs = append(runtimeObjs, data.additionalsObjs...) + kubeClient := fake.NewSimpleClientset(runtimeObjs...) settingsMgr := settings.NewSettingsManager(context.Background(), kubeClient, test.FakeArgoCDNamespace) kubectl := &MockKubectl{Kubectl: &kubetest.MockKubectlCmd{}} ctrl, err := NewApplicationController( diff --git a/controller/sync.go b/controller/sync.go index 5f896a522f942..6603cff155148 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -287,7 +287,12 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha } trackingMethod := argo.GetTrackingMethod(m.settingsMgr) - if m.settingsMgr.IsImpersonationEnabled() { + impersonationEnabled, err := m.settingsMgr.IsImpersonationEnabled() + if err != nil { + log.Errorf("could not get impersonation feature flag: %v", err) + return + } + if impersonationEnabled { serviceAccountToImpersonate, err := deriveServiceAccountName(proj, app) if err != nil { state.Phase = common.OperationError @@ -572,7 +577,9 @@ func deriveServiceAccountName(project *v1alpha1.AppProject, application *v1alpha dstServerMatched := glob.Match(item.Server, application.Spec.Destination.Server) dstNamespaceMatched := glob.Match(item.Namespace, application.Spec.Destination.Namespace) if dstServerMatched && dstNamespaceMatched { - if strings.Contains(item.DefaultServiceAccount, ":") { + if item.DefaultServiceAccount == "" { + return "", fmt.Errorf("default service account cannot be an empty string") + } else if strings.Contains(item.DefaultServiceAccount, ":") { // service account is specified along with its namespace. return fmt.Sprintf("system:serviceaccount:%s", item.DefaultServiceAccount), nil } else { diff --git a/controller/sync_test.go b/controller/sync_test.go index 1dbfa2ff9e1a5..1ece00ac2353c 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -2,6 +2,7 @@ package controller import ( "context" + "strconv" "testing" "github.com/argoproj/gitops-engine/pkg/sync" @@ -9,6 +10,7 @@ import ( "github.com/argoproj/gitops-engine/pkg/utils/kube" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" @@ -644,6 +646,733 @@ func TestNormalizeTargetResourcesWithList(t *testing.T) { }) } +func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { + type fixture struct { + project *v1alpha1.AppProject + application *v1alpha1.Application + } + + setup := func(destinationServiceAccounts []v1alpha1.ApplicationDestinationServiceAccount, destinationNamespace, destinationServerURL, applicationNamespace string) *fixture { + project := + &v1alpha1.AppProject{ + ObjectMeta: v1.ObjectMeta{ + Namespace: "argocd-ns", + Name: "testProj", + }, + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: destinationServiceAccounts, + }, + } + app := + &v1alpha1.Application{ + ObjectMeta: v1.ObjectMeta{ + Namespace: applicationNamespace, + Name: "testApp", + }, + Spec: v1alpha1.ApplicationSpec{ + Project: "testProj", + Destination: v1alpha1.ApplicationDestination{ + Server: destinationServerURL, + Namespace: destinationNamespace, + }, + }, + } + return &fixture{ + project: project, + application: app, + } + } + + t.Run("empty destination service accounts", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{} + destinationNamespace := "testns" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "" + expectedErrMsg := "no matching service account found for destination server https://kubernetes.svc.local and namespace testns" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should fail + assert.EqualError(t, err, expectedErrMsg) + }) + + t.Run("exact match of destination namespace", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + Namespace: "testns", + DefaultServiceAccount: "test-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "system:serviceaccount:testns:test-sa" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should not fail + assert.NoError(t, err) + }) + + t.Run("exact one match with multiple destination service accounts", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + Namespace: "guestbook", + DefaultServiceAccount: "guestbook-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "guestbook-test", + DefaultServiceAccount: "guestbook-test-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "default", + DefaultServiceAccount: "default-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "testns", + DefaultServiceAccount: "test-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "system:serviceaccount:testns:test-sa" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should not fail + assert.NoError(t, err) + }) + + t.Run("first match to be used when multiple matches are available", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + Namespace: "testns", + DefaultServiceAccount: "test-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "testns", + DefaultServiceAccount: "test-sa-2", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "testns", + DefaultServiceAccount: "test-sa-3", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "guestbook", + DefaultServiceAccount: "guestbook-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "system:serviceaccount:testns:test-sa" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should fail + assert.NoError(t, err) + }) + + t.Run("first match to be used when glob pattern is used", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + Namespace: "test*", + DefaultServiceAccount: "test-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "testns", + DefaultServiceAccount: "test-sa-2", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "default", + DefaultServiceAccount: "default-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "system:serviceaccount:testns:test-sa" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should not fail + assert.NoError(t, err) + }) + + t.Run("no match among a valid list", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + Namespace: "test1", + DefaultServiceAccount: "test-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "test2", + DefaultServiceAccount: "test-sa-2", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "default", + DefaultServiceAccount: "default-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "" + expectedErrMsg := "no matching service account found for destination server https://kubernetes.svc.local and namespace testns" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should fail + assert.EqualError(t, err, expectedErrMsg) + }) + + t.Run("app destination namespace is empty", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + DefaultServiceAccount: "test-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "*", + DefaultServiceAccount: "test-sa-2", + }, + } + destinationNamespace := "" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "system:serviceaccount:argocd-ns:test-sa" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should fail + assert.NoError(t, err) + }) + + t.Run("match done via catch all glob pattern", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + Namespace: "testns1", + DefaultServiceAccount: "test-sa-2", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "default", + DefaultServiceAccount: "default-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "*", + DefaultServiceAccount: "test-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "system:serviceaccount:testns:test-sa" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should fail + assert.NoError(t, err) + }) + + t.Run("sa specified with a namespace", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + Namespace: "testns", + DefaultServiceAccount: "myns:test-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "default", + DefaultServiceAccount: "default-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "*", + DefaultServiceAccount: "test-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "system:serviceaccount:myns:test-sa" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should fail + assert.NoError(t, err) + }) + +} + +func TestDeriveServiceAccountMatchingServers(t *testing.T) { + type fixture struct { + project *v1alpha1.AppProject + application *v1alpha1.Application + } + + setup := func(destinationServiceAccounts []v1alpha1.ApplicationDestinationServiceAccount, destinationNamespace, destinationServerURL, applicationNamespace string) *fixture { + project := + &v1alpha1.AppProject{ + ObjectMeta: v1.ObjectMeta{ + Namespace: "argocd-ns", + Name: "testProj", + }, + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: destinationServiceAccounts, + }, + } + app := + &v1alpha1.Application{ + ObjectMeta: v1.ObjectMeta{ + Namespace: applicationNamespace, + Name: "testApp", + }, + Spec: v1alpha1.ApplicationSpec{ + Project: "testProj", + Destination: v1alpha1.ApplicationDestination{ + Server: destinationServerURL, + Namespace: destinationNamespace, + }, + }, + } + return &fixture{ + project: project, + application: app, + } + } + + t.Run("exact one match with multiple destination service accounts", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + Namespace: "guestbook", + DefaultServiceAccount: "guestbook-sa", + }, + { + Server: "https://abc.svc.local", + Namespace: "guestbook", + DefaultServiceAccount: "guestbook-test-sa", + }, + { + Server: "https://cde.svc.local", + Namespace: "guestbook", + DefaultServiceAccount: "default-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "testns", + DefaultServiceAccount: "test-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "system:serviceaccount:testns:test-sa" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should fail + assert.NoError(t, err) + }) + + t.Run("first match to be used when multiple matches are available", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + Namespace: "testns", + DefaultServiceAccount: "test-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "testns", + DefaultServiceAccount: "test-sa-2", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "default", + DefaultServiceAccount: "default-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "guestbook", + DefaultServiceAccount: "guestbook-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "system:serviceaccount:testns:test-sa" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should not fail + assert.NoError(t, err) + }) + + t.Run("first match to be used when glob pattern is used", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + Namespace: "test*", + DefaultServiceAccount: "test-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "testns", + DefaultServiceAccount: "test-sa-2", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "default", + DefaultServiceAccount: "default-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "system:serviceaccount:testns:test-sa" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should not fail + assert.NoError(t, err) + }) + + t.Run("no match among a valid list", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + Namespace: "testns", + DefaultServiceAccount: "test-sa", + }, + { + Server: "https://abc.svc.local", + Namespace: "testns", + DefaultServiceAccount: "test-sa-2", + }, + { + Server: "https://cde.svc.local", + Namespace: "default", + DefaultServiceAccount: "default-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://xyz.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "" + expectedErr := "no matching service account found for destination server https://xyz.svc.local and namespace testns" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should fail + assert.EqualError(t, err, expectedErr) + }) + + t.Run("match done via catch all glob pattern", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + Namespace: "testns1", + DefaultServiceAccount: "test-sa-2", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "default", + DefaultServiceAccount: "default-sa", + }, + { + Server: "*", + Namespace: "*", + DefaultServiceAccount: "test-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://localhost:6443" + applicationNamespace := "argocd-ns" + expectedSA := "system:serviceaccount:testns:test-sa" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should not fail + assert.NoError(t, err) + }) + + t.Run("sa specified with a namespace", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://abc.svc.local", + Namespace: "testns", + DefaultServiceAccount: "myns:test-sa", + }, + { + Server: "https://kubernetes.svc.local", + Namespace: "default", + DefaultServiceAccount: "default-sa", + }, + { + Server: "*", + Namespace: "*", + DefaultServiceAccount: "test-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://abc.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "system:serviceaccount:myns:test-sa" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountName(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // then, app sync should fail + assert.NoError(t, err) + }) + +} + +func TestSyncWithImpersonate(t *testing.T) { + type fixture struct { + project *v1alpha1.AppProject + application *v1alpha1.Application + controller *ApplicationController + } + + setup := func(impersonationEnabled bool, destinationNamespace, serviceAccountName string) *fixture { + app := newFakeApp() + app.Status.OperationState = nil + app.Status.History = nil + project := &v1alpha1.AppProject{ + ObjectMeta: v1.ObjectMeta{ + Namespace: test.FakeArgoCDNamespace, + Name: "default", + }, + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: []v1alpha1. + ApplicationDestinationServiceAccount{ + { + Server: "https://localhost:6443", + Namespace: destinationNamespace, + DefaultServiceAccount: serviceAccountName, + }, + }, + }, + } + additionalObjs := []runtime.Object{} + if serviceAccountName != "" { + syncServiceAccount := &corev1.ServiceAccount{ + ObjectMeta: v1.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), + }, + additionalsObjs: additionalObjs, + } + ctrl := newFakeController(&data, nil) + return &fixture{ + project: project, + application: app, + controller: ctrl, + } + } + + t.Run("sync with impersonation and no matching service account", func(t *testing.T) { + // given app sync impersonation feature is enabled and has a project no matching service account + t.Parallel() + f := setup(true, test.FakeArgoCDNamespace, "") + opMessage := "failed to find a matching service account to impersonate: no matching service account found for destination server https://localhost:6443 and namespace fake-dest-ns" + + opState := &v1alpha1.OperationState{ + Operation: v1alpha1.Operation{ + Sync: &v1alpha1.SyncOperation{ + Source: &v1alpha1.ApplicationSource{}, + }, + }, + Phase: common.OperationRunning, + } + // when + f.controller.appStateManager.SyncAppState(f.application, opState) + + // then, app sync should fail + assert.Equal(t, common.OperationError, opState.Phase) + assert.Contains(t, opState.Message, opMessage) + }) + + t.Run("sync with impersonation and empty service account match", func(t *testing.T) { + // given a project with an active deny sync window and an operation in progress + t.Parallel() + f := setup(true, test.FakeDestNamespace, "") + opMessage := "failed to find a matching service account to impersonate: default service account cannot be an empty string" + + opState := &v1alpha1.OperationState{ + Operation: v1alpha1.Operation{ + Sync: &v1alpha1.SyncOperation{ + Source: &v1alpha1.ApplicationSource{}, + }, + }, + Phase: common.OperationRunning, + } + // when + f.controller.appStateManager.SyncAppState(f.application, opState) + + // then + assert.Equal(t, common.OperationError, opState.Phase) + assert.Contains(t, opState.Message, opMessage) + }) + + t.Run("sync with impersonation and matching sa", func(t *testing.T) { + // given a project with an active deny sync window and an operation in progress + t.Parallel() + f := setup(true, test.FakeDestNamespace, "test-sa") + opMessage := "successfully synced (no more tasks)" + + opState := &v1alpha1.OperationState{ + Operation: v1alpha1.Operation{ + Sync: &v1alpha1.SyncOperation{ + Source: &v1alpha1.ApplicationSource{}, + }, + }, + Phase: common.OperationRunning, + } + // when + f.controller.appStateManager.SyncAppState(f.application, opState) + + // then + assert.Equal(t, common.OperationSucceeded, opState.Phase) + assert.Contains(t, opState.Message, opMessage) + }) + + t.Run("sync without impersonation", func(t *testing.T) { + // given a project with an active deny sync window and an operation in progress + t.Parallel() + f := setup(false, test.FakeDestNamespace, "") + opMessage := "successfully synced (no more tasks)" + + opState := &v1alpha1.OperationState{ + Operation: v1alpha1.Operation{ + Sync: &v1alpha1.SyncOperation{ + Source: &v1alpha1.ApplicationSource{}, + }, + }, + Phase: common.OperationRunning, + } + // when + f.controller.appStateManager.SyncAppState(f.application, opState) + + // then + assert.Equal(t, common.OperationSucceeded, opState.Phase) + assert.Contains(t, opState.Message, opMessage) + }) +} + func dig[T any](obj interface{}, path []interface{}) T { i := obj diff --git a/docs/operator-manual/app-sync-using-impersonation.md b/docs/operator-manual/app-sync-using-impersonation.md index 9314f0b376b8e..98174a82d0e9e 100644 --- a/docs/operator-manual/app-sync-using-impersonation.md +++ b/docs/operator-manual/app-sync-using-impersonation.md @@ -1,7 +1,7 @@ # Application Sync using impersonation !!! warning "Alpha Feature" - This is an experimental, alpha-quality feature that allows you to control the service account used for the sync operation. The configured service account, could have lesser privileges required for creating resources compared to the highly privileged access required for the control plane operations. + This is an experimental, alpha-quality feature that allows you to control the service account used for the sync operation. The configured service account could have lesser privileges required for creating resources compared to the highly privileged access required for the control plane operations. !!! warning Please read this documentation carefully before you enable this feature. Misconfiguration could lead to potential security issues. @@ -94,7 +94,7 @@ spec: sourceRepos: - '*' destinations: - - * + - '*' destinationServiceAccounts: - server: https://kubernetes.default.svc namespace: guestbook diff --git a/docs/operator-manual/argocd-cm.yaml b/docs/operator-manual/argocd-cm.yaml index a8d3be645bcfb..52b7eb1678026 100644 --- a/docs/operator-manual/argocd-cm.yaml +++ b/docs/operator-manual/argocd-cm.yaml @@ -329,14 +329,14 @@ data: # spread out the refreshes and give time to the repo-server to catch up. The jitter is the maximum duration that can be # added to the sync timeout. So, if the sync timeout is 3 minutes and the jitter is 1 minute, then the actual timeout will # be between 3 and 4 minutes. Disabled when the value is 0, defaults to 0. - timeout.reconciliation.jitter: 0 + timeout.reconciliation.jitter: "0" # cluster.inClusterEnabled indicates whether to allow in-cluster server address. This is enabled by default. cluster.inClusterEnabled: "true" # The maximum number of pod logs to render in UI. If the application has more than this number of pods, the logs will not be rendered. # This is to prevent the UI from becoming unresponsive when rendering a large number of logs. Default is 10. - server.maxPodLogsToRender: 10 + server.maxPodLogsToRender: "10" # Application pod logs RBAC enforcement enables control over who can and who can't view application pod logs. # When you enable the switch, pod logs will be visible only to admin role by default. Other roles/users will not be able to view them via cli and UI. @@ -425,7 +425,7 @@ data: name: some-cluster server: https://some-cluster # The maximum size of the payload that can be sent to the webhook server. - webhook.maxPayloadSizeMB: 1024 + webhook.maxPayloadSizeMB: "1024" - # application.sync.impersonation.enabled indicates whether the application sync can be decoupled from control plane service account using impersonation. + # application.sync.impersonation.enabled enables application sync to use a custom service account, via impersonation. This allows decoupling sync from control-plane service account. application.sync.impersonation.enabled: "false" diff --git a/docs/proposals/decouple-application-sync-user-using-impersonation.md b/docs/proposals/decouple-application-sync-user-using-impersonation.md index 050c8d6b0a635..2f4ebe776ed8f 100644 --- a/docs/proposals/decouple-application-sync-user-using-impersonation.md +++ b/docs/proposals/decouple-application-sync-user-using-impersonation.md @@ -68,9 +68,9 @@ This proposal would allow ArgoCD administrators to manage the cluster permission ### Goals - Applications may only impersonate ServiceAccounts that live in the same namespace as the destination namespace configured in the application.If the service account is created in a different namespace, then the user can provide the service account name in the format `:` . ServiceAccount to be used for syncing each application is determined by the target destination configured in the `AppProject` associated with the `Application`. -- If impersonation feature is enabled, and no service account name is provided in the associated `AppProject`, then the sync operation would fail with an appropriate error message. Users can configure a catch all service account matching all destinations to avoid such sync errors. +- If impersonation feature is enabled, and no service account name is provided in the associated `AppProject`, then the default service account of the destination namespace of the `Application` should be used. - Access restrictions implemented through properties in AppProject (if done) must have the existing behavior. From a security standpoint, any restrictions that were available before switching to a service account based approach should continue to exist even when the impersonation feature is enabled. -- The feature can be enabled/disabled only at the system level. Once enabled/disabled, it is applicable to all ArgoCD `Applications`. +- The feature can be enabled/disabled only at the system level. Once enabled/disabled, it is applicable to all Argo CD `Applications`. ### Non-Goals @@ -82,7 +82,7 @@ As part of this proposal, it would be possible for an ArgoCD Admin to specify a When applications gets synced, based on its destination (target cluster and namespace combination), the `defaultServiceAccount` configured in the `AppProject` will be selected and used for impersonation when executing the kubectl commands for the sync operation. -We would be introducing a new element `destinationServiceAccounts` in `AppProject.spec`. This element is used for the sole purpose of specifying the impersonation configuration. The `defaultServiceAccount` configured for the `AppProject` would be used for the sync operation for a particular destination cluster and namespace. If impersonation feature is enabled and no specific service account is provided in the `AppProject` CR, then the sync operation will fail with an error. Users can configure a catch all service account matching all destinations to avoid such sync errors. +We would be introducing a new element `destinationServiceAccounts` in `AppProject.spec`. This element is used for the sole purpose of specifying the impersonation configuration. The `defaultServiceAccount` configured for the `AppProject` would be used for the sync operation for a particular destination cluster and namespace. If impersonation feature is enabled and no specific service account is provided in the `AppProject` CR, then the `default` service account in the destination namespace would be used for impersonation. ```yaml apiVersion: argoproj.io/v1alpha1 @@ -109,7 +109,7 @@ spec: - server: https://kubernetes.default.svc namespace: guestbook-stage defaultServiceAccount: guestbook-stage-deployer - - server: '* + - server: '*' namespace: '*' defaultServiceAccount: default # catch all service account to be used when all other matches fail. ``` @@ -161,7 +161,10 @@ So that, I can use a generic convention of naming service accounts and avoid ass #### Component: ArgoCD Application Controller -- Provide a configuration in `argocd-cm` which can be modified to enable the Impersonation feature. Set `application.sync.impersonation.enabled: "true"` in the Argo CD ConfigMap. Default value of `application.sync.impersonation.enabled` would be `"false"` and user has to explicitly override it to use this feature. +- Provide a configuration in `argocd-cm` which can be modified to enable the Impersonation feature. Set `applicationcontroller.enable.impersonation: true` in the Argo CD ConfigMap. Default value of `applicationcontroller.enable.impersonation` would be `false` and user has to explicitly override it to use this feature. +- Provide an option to override the Impersonation feature using environment variables. +Set `ARGOCD_APPLICATION_CONTROLLER_ENABLE_IMPERSONATION=true` in the Application controller environment variables. Default value of the environment variable must be `false` and user has to explicitly set it to `true` to use this feature. +- Provide an option to enable this feature using a command line flag `--enable-impersonation`. This new argument option needs to be added to the Application controller args. - Fix Application Controller `sync.go` to set the Impersonate configuration from the AppProject CR to the `SyncContext` Object (rawConfig and restConfig field, need to understand which config is used for the actual sync and if both configs need to be impersonated.) #### Component: ArgoCD UI diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index 1337bd8c72772..8fa1f6e99c0b2 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -2767,11 +2767,11 @@ type KustomizeOptions struct { // ApplicationDestinationServiceAccount holds information about the service account to be impersonated for the application sync operation. type ApplicationDestinationServiceAccount struct { // Server specifies the URL of the target cluster's Kubernetes control plane API. - Server string `json:"server,omitempty" protobuf:"bytes,1,opt,name=server"` + Server string `json:"server" protobuf:"bytes,1,opt,name=server"` // Namespace specifies the target namespace for the application's resources. Namespace string `json:"namespace,omitempty" protobuf:"bytes,2,opt,name=namespace"` - // ServiceAccountName to be used for impersonation during the sync operation - DefaultServiceAccount string `json:"defaultServiceAccount,omitempty" protobuf:"bytes,3,opt,name=defaultServiceAccount"` + // DefaultServiceAccount to be used for impersonation during the sync operation + DefaultServiceAccount string `json:"defaultServiceAccount" protobuf:"bytes,3,opt,name=defaultServiceAccount"` } // CascadedDeletion indicates if the deletion finalizer is set and controller should delete the application and it's cascaded resources diff --git a/util/settings/settings.go b/util/settings/settings.go index 7d192d9cb9461..0112ab088dd39 100644 --- a/util/settings/settings.go +++ b/util/settings/settings.go @@ -534,7 +534,8 @@ const ( const ( // default max webhook payload size is 1GB - defaultMaxWebhookPayloadSize = int64(1) * 1024 * 1024 * 1024 + defaultMaxWebhookPayloadSize = int64(1) * 1024 * 1024 * 1024 + defaultImpersonationEnabledFlag = false ) var sourceTypeToEnableGenerationKey = map[v1alpha1.ApplicationSourceType]string{ @@ -2336,11 +2337,11 @@ func (mgr *SettingsManager) GetMaxWebhookPayloadSize() int64 { return maxPayloadSizeMB * 1024 * 1024 } -// GetIsImpersonationEnabled returns true if application sync with impersonation feature is enabled in argocd-cm configmap -func (mgr *SettingsManager) IsImpersonationEnabled() bool { +// IsImpersonationEnabled returns true if application sync with impersonation feature is enabled in argocd-cm configmap +func (mgr *SettingsManager) IsImpersonationEnabled() (bool, error) { cm, err := mgr.getConfigMap() if err != nil { - return false + return false, fmt.Errorf("error checking %s property in configmap: %w", impersonationEnabledKey, err) } - return cm.Data[impersonationEnabledKey] == "true" + return cm.Data[impersonationEnabledKey] == "true", nil } From c8161bf5af6ad6d368b1885983bfd76d3b2c57b2 Mon Sep 17 00:00:00 2001 From: anandf Date: Tue, 17 Sep 2024 18:16:27 +0530 Subject: [PATCH 02/20] Added comments for the newly added unit tests Signed-off-by: anandf --- controller/sync_test.go | 100 ++++++++++++++++++++-------------------- 1 file changed, 50 insertions(+), 50 deletions(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index 1ece00ac2353c..ebdca70b67f20 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -684,7 +684,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { } t.Run("empty destination service accounts", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with no destination service accounts t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{} destinationNamespace := "testns" @@ -698,12 +698,12 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { sa, err := deriveServiceAccountName(f.project, f.application) assert.Equal(t, expectedSA, sa) - // then, app sync should fail + // then, there should be an error saying no valid match was found assert.EqualError(t, err, expectedErrMsg) }) t.Run("exact match of destination namespace", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with exactly one destination service account that matches the application destination, t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -720,14 +720,14 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when sa, err := deriveServiceAccountName(f.project, f.application) - assert.Equal(t, expectedSA, sa) - // then, app sync should not fail + // then, there should be no error and should use the right service account for impersonation assert.NoError(t, err) + assert.Equal(t, expectedSA, sa) }) t.Run("exact one match with multiple destination service accounts", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with multiple destination service accounts having one exact match for application destination t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -759,14 +759,14 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when sa, err := deriveServiceAccountName(f.project, f.application) - assert.Equal(t, expectedSA, sa) - // then, app sync should not fail + // then, there should be no error and should use the right service account for impersonation assert.NoError(t, err) + assert.Equal(t, expectedSA, sa) }) t.Run("first match to be used when multiple matches are available", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with multiple destination service accounts having multiple match for application destination t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -798,14 +798,14 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when sa, err := deriveServiceAccountName(f.project, f.application) - assert.Equal(t, expectedSA, sa) - // then, app sync should fail + // then, there should be no error and it should use the first matching service account for impersonation assert.NoError(t, err) + assert.Equal(t, expectedSA, sa) }) t.Run("first match to be used when glob pattern is used", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with multiple destination service accounts with glob patterns matching the application destination t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -832,14 +832,14 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when sa, err := deriveServiceAccountName(f.project, f.application) - assert.Equal(t, expectedSA, sa) - // then, app sync should not fail + // then, there should not be any error and should use the first matching glob pattern service account for impersonation assert.NoError(t, err) + assert.Equal(t, expectedSA, sa) }) t.Run("no match among a valid list", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with multiple destination service accounts with no matches for application destination t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -867,14 +867,14 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when sa, err := deriveServiceAccountName(f.project, f.application) - assert.Equal(t, expectedSA, sa) - // then, app sync should fail + // then, there should be an error saying no match was found assert.EqualError(t, err, expectedErrMsg) + assert.Equal(t, expectedSA, sa) }) t.Run("app destination namespace is empty", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with multiple destination service accounts with empty application destination namespace t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -895,14 +895,14 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when sa, err := deriveServiceAccountName(f.project, f.application) - assert.Equal(t, expectedSA, sa) - // then, app sync should fail + // then, there should not be any error and the service account configured for with empty namespace should be used. assert.NoError(t, err) + assert.Equal(t, expectedSA, sa) }) t.Run("match done via catch all glob pattern", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with multiple destination service accounts having a catch all glob pattern t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -929,14 +929,14 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when sa, err := deriveServiceAccountName(f.project, f.application) - assert.Equal(t, expectedSA, sa) - // then, app sync should fail + // then, there should not be any error and the catch all service account should be returned assert.NoError(t, err) + assert.Equal(t, expectedSA, sa) }) t.Run("sa specified with a namespace", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with multiple destination service accounts having a matching service account specified with its namespace t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -965,7 +965,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { sa, err := deriveServiceAccountName(f.project, f.application) assert.Equal(t, expectedSA, sa) - // then, app sync should fail + // then, there should not be any error and the service account with its namespace should be returned. assert.NoError(t, err) }) @@ -1009,7 +1009,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { } t.Run("exact one match with multiple destination service accounts", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with multiple destination service accounts and one exact match for application destination t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -1041,14 +1041,14 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when sa, err := deriveServiceAccountName(f.project, f.application) - assert.Equal(t, expectedSA, sa) - // then, app sync should fail + // then, there should not be any error and the right service account must be returned. assert.NoError(t, err) + assert.Equal(t, expectedSA, sa) }) t.Run("first match to be used when multiple matches are available", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with multiple destination service accounts and multiple matches for application destination t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -1080,14 +1080,14 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when sa, err := deriveServiceAccountName(f.project, f.application) - assert.Equal(t, expectedSA, sa) - // then, app sync should not fail + // then, there should not be any error and first matching service account should be used assert.NoError(t, err) + assert.Equal(t, expectedSA, sa) }) t.Run("first match to be used when glob pattern is used", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with multiple destination service accounts with a matching glob pattern and exact match t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -1116,12 +1116,12 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { sa, err := deriveServiceAccountName(f.project, f.application) assert.Equal(t, expectedSA, sa) - // then, app sync should not fail + // then, there should not be any error and the service account of the glob pattern, being the first match should be returned. assert.NoError(t, err) }) t.Run("no match among a valid list", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with multiple destination service accounts with no match t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -1149,14 +1149,14 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when sa, err := deriveServiceAccountName(f.project, f.application) - assert.Equal(t, expectedSA, sa) - // then, app sync should fail + // then, there an error with appropriate message must be returned assert.EqualError(t, err, expectedErr) + assert.Equal(t, expectedSA, sa) }) t.Run("match done via catch all glob pattern", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given an application referring a project with multiple destination service accounts with matching catch all glob pattern t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -1183,14 +1183,14 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when sa, err := deriveServiceAccountName(f.project, f.application) - assert.Equal(t, expectedSA, sa) - // then, app sync should not fail + // then, there should not be any error and the service account of the glob pattern match must be returned. assert.NoError(t, err) + assert.Equal(t, expectedSA, sa) }) t.Run("sa specified with a namespace", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given app sync impersonation feature is enabled and matching service account is prefixed with a namespace t.Parallel() destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ { @@ -1217,10 +1217,10 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when sa, err := deriveServiceAccountName(f.project, f.application) - assert.Equal(t, expectedSA, sa) - // then, app sync should fail + // then, there should not be any error and the service account with the given namespace prefix must be returned. assert.NoError(t, err) + assert.Equal(t, expectedSA, sa) }) } @@ -1285,7 +1285,7 @@ func TestSyncWithImpersonate(t *testing.T) { } t.Run("sync with impersonation and no matching service account", func(t *testing.T) { - // given app sync impersonation feature is enabled and has a project no matching service account + // given app sync impersonation feature is enabled with an application referring a project no matching service account t.Parallel() f := setup(true, test.FakeArgoCDNamespace, "") opMessage := "failed to find a matching service account to impersonate: no matching service account found for destination server https://localhost:6443 and namespace fake-dest-ns" @@ -1301,13 +1301,13 @@ func TestSyncWithImpersonate(t *testing.T) { // when f.controller.appStateManager.SyncAppState(f.application, opState) - // then, app sync should fail + // then, app sync should fail with expected error message in operation state assert.Equal(t, common.OperationError, opState.Phase) assert.Contains(t, opState.Message, opMessage) }) t.Run("sync with impersonation and empty service account match", func(t *testing.T) { - // given a project with an active deny sync window and an operation in progress + // given app sync impersonation feature is enabled with an application referring a project matching service account that is an empty string t.Parallel() f := setup(true, test.FakeDestNamespace, "") opMessage := "failed to find a matching service account to impersonate: default service account cannot be an empty string" @@ -1323,13 +1323,13 @@ func TestSyncWithImpersonate(t *testing.T) { // when f.controller.appStateManager.SyncAppState(f.application, opState) - // then + // then app sync should fail with expected error message in operation state assert.Equal(t, common.OperationError, opState.Phase) assert.Contains(t, opState.Message, opMessage) }) t.Run("sync with impersonation and matching sa", func(t *testing.T) { - // given a project with an active deny sync window and an operation in progress + // given app sync impersonation feature is enabled with an application referring a project matching service account t.Parallel() f := setup(true, test.FakeDestNamespace, "test-sa") opMessage := "successfully synced (no more tasks)" @@ -1345,13 +1345,13 @@ func TestSyncWithImpersonate(t *testing.T) { // when f.controller.appStateManager.SyncAppState(f.application, opState) - // then + // then app sync should not fail assert.Equal(t, common.OperationSucceeded, opState.Phase) assert.Contains(t, opState.Message, opMessage) }) t.Run("sync without impersonation", func(t *testing.T) { - // given a project with an active deny sync window and an operation in progress + // given app sync impersonation feature is disabled with an application referring a project matching service account t.Parallel() f := setup(false, test.FakeDestNamespace, "") opMessage := "successfully synced (no more tasks)" @@ -1367,7 +1367,7 @@ func TestSyncWithImpersonate(t *testing.T) { // when f.controller.appStateManager.SyncAppState(f.application, opState) - // then + // then application sync should pass using the control plane service account assert.Equal(t, common.OperationSucceeded, opState.Phase) assert.Contains(t, opState.Message, opMessage) }) From 26c2585b3c964e7b85837ec90515c2473c1b5876 Mon Sep 17 00:00:00 2001 From: anandf Date: Tue, 17 Sep 2024 18:18:50 +0530 Subject: [PATCH 03/20] Refactored method name to deriveServiceAccountToImpersonate Signed-off-by: anandf --- controller/sync.go | 6 +++--- controller/sync_test.go | 30 +++++++++++++++--------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/controller/sync.go b/controller/sync.go index 6603cff155148..f3336a9ef99b7 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -293,7 +293,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha return } if impersonationEnabled { - serviceAccountToImpersonate, err := deriveServiceAccountName(proj, app) + serviceAccountToImpersonate, err := deriveServiceAccountToImpersonate(proj, app) if err != nil { state.Phase = common.OperationError state.Message = fmt.Sprintf("failed to find a matching service account to impersonate: %v", err) @@ -562,9 +562,9 @@ func syncWindowPreventsSync(app *v1alpha1.Application, proj *v1alpha1.AppProject return !window.CanSync(isManual) } -// deriveServiceAccountName determines the service account to be used for impersonation for the sync operation. +// deriveServiceAccountToImpersonate determines the service account to be used for impersonation for the sync operation. // The returned service account will be fully qualified including namespace and the service account name in the format system:serviceaccount:: -func deriveServiceAccountName(project *v1alpha1.AppProject, application *v1alpha1.Application) (string, error) { +func deriveServiceAccountToImpersonate(project *v1alpha1.AppProject, application *v1alpha1.Application) (string, error) { // spec.Destination.Namespace is optional. If not specified, use the Application's // namespace serviceAccountNamespace := application.Spec.Destination.Namespace diff --git a/controller/sync_test.go b/controller/sync_test.go index ebdca70b67f20..5c4790d508f11 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -695,7 +695,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) assert.Equal(t, expectedSA, sa) // then, there should be an error saying no valid match was found @@ -719,7 +719,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should be no error and should use the right service account for impersonation assert.NoError(t, err) @@ -758,7 +758,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should be no error and should use the right service account for impersonation assert.NoError(t, err) @@ -797,7 +797,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should be no error and it should use the first matching service account for impersonation assert.NoError(t, err) @@ -831,7 +831,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and should use the first matching glob pattern service account for impersonation assert.NoError(t, err) @@ -866,7 +866,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should be an error saying no match was found assert.EqualError(t, err, expectedErrMsg) @@ -894,7 +894,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and the service account configured for with empty namespace should be used. assert.NoError(t, err) @@ -928,7 +928,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and the catch all service account should be returned assert.NoError(t, err) @@ -962,7 +962,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) assert.Equal(t, expectedSA, sa) // then, there should not be any error and the service account with its namespace should be returned. @@ -1040,7 +1040,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and the right service account must be returned. assert.NoError(t, err) @@ -1079,7 +1079,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and first matching service account should be used assert.NoError(t, err) @@ -1113,7 +1113,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) assert.Equal(t, expectedSA, sa) // then, there should not be any error and the service account of the glob pattern, being the first match should be returned. @@ -1148,7 +1148,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there an error with appropriate message must be returned assert.EqualError(t, err, expectedErr) @@ -1182,7 +1182,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and the service account of the glob pattern match must be returned. assert.NoError(t, err) @@ -1216,7 +1216,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) // when - sa, err := deriveServiceAccountName(f.project, f.application) + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and the service account with the given namespace prefix must be returned. assert.NoError(t, err) From 535f2046a8a9a1f75ae12ba2dca392a6e5034ed8 Mon Sep 17 00:00:00 2001 From: anandf Date: Tue, 17 Sep 2024 23:10:41 +0530 Subject: [PATCH 04/20] Using const name in return value Signed-off-by: anandf --- util/settings/settings.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/settings/settings.go b/util/settings/settings.go index 0112ab088dd39..fb4a06906f99d 100644 --- a/util/settings/settings.go +++ b/util/settings/settings.go @@ -2341,7 +2341,7 @@ func (mgr *SettingsManager) GetMaxWebhookPayloadSize() int64 { func (mgr *SettingsManager) IsImpersonationEnabled() (bool, error) { cm, err := mgr.getConfigMap() if err != nil { - return false, fmt.Errorf("error checking %s property in configmap: %w", impersonationEnabledKey, err) + return defaultImpersonationEnabledFlag, fmt.Errorf("error checking %s property in configmap: %w", impersonationEnabledKey, err) } return cm.Data[impersonationEnabledKey] == "true", nil } From 8080c62c102e37457b79feef25c9af1f988014d4 Mon Sep 17 00:00:00 2001 From: anandf Date: Mon, 23 Sep 2024 16:23:38 +0530 Subject: [PATCH 05/20] Added unit tests for argocd proj add-destination-service-accounts Signed-off-by: anandf --- test/e2e/project_management_test.go | 102 ++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/test/e2e/project_management_test.go b/test/e2e/project_management_test.go index f00c4e3eeba62..c7c26cb5de7ec 100644 --- a/test/e2e/project_management_test.go +++ b/test/e2e/project_management_test.go @@ -619,3 +619,105 @@ func TestGetVirtualProjectMatch(t *testing.T) { _, err = fixture.RunCli("app", "sync", fixture.Name(), "--resource", ":Service:guestbook-ui", "--timeout", fmt.Sprintf("%v", 10)) assert.Contains(t, err.Error(), "blocked by sync window") } + +func TestAddProjectDestinationServiceAccount(t *testing.T) { + fixture.EnsureCleanState(t) + + projectName := "proj-" + strconv.FormatInt(time.Now().Unix(), 10) + _, err := fixture.AppClientset.ArgoprojV1alpha1().AppProjects(fixture.TestNamespace()).Create( + context.Background(), &v1alpha1.AppProject{ObjectMeta: metav1.ObjectMeta{Name: projectName}}, metav1.CreateOptions{}) + if err != nil { + t.Fatalf("Unable to create project %v", err) + } + + // Given, an existing project + // When, a default destination service account with all valid fields is added to it, + // Then, there is no error. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "test-ns", + "test-sa", + ) + if err != nil { + t.Fatalf("Unable to add project destination service account %v", err) + } + + // Given, an existing project + // When, a default destination service account with empty namespace is added to it, + // Then, there is no error. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "", + "test-sa", + ) + if err != nil { + t.Fatalf("Unable to add project destination service account %v", err) + } + + // Given, an existing project, + // When, a default destination service account is added with a custom service account namespace, + // Then, there is no error. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "test-ns1", + "test-sa", + "--service-account-namespace", + "default", + ) + if err != nil { + t.Fatalf("Unable to add project destination service account %v", err) + } + + // Given, an existing project, + // When, a duplicate default destination service account is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "test-ns", + "test-sa", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "already defined") + + // Given, an existing project, + // When, a default destination service account with negation glob pattern for server is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "!*", + "test-ns", + "test-sa", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "server has an invalid format, '!*'") + + // Given, an existing project, + // When, a default destination service account with negation glob pattern for namespace is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "!*", + "test-sa", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "namespace has an invalid format, '!*'") + + proj, err := fixture.AppClientset.ArgoprojV1alpha1().AppProjects(fixture.TestNamespace()).Get(context.Background(), projectName, metav1.GetOptions{}) + require.NoError(t, err) + assert.Equal(t, projectName, proj.Name) + assert.Len(t, proj.Spec.DestinationServiceAccounts, 2) + + assert.Equal(t, "https://192.168.99.100:8443", proj.Spec.DestinationServiceAccounts[0].Server) + assert.Equal(t, "test-ns", proj.Spec.DestinationServiceAccounts[0].Namespace) + assert.Equal(t, "test-sa", proj.Spec.DestinationServiceAccounts[0].DefaultServiceAccount) + + assert.Equal(t, "https://192.168.99.100:8443", proj.Spec.DestinationServiceAccounts[1].Server) + assert.Equal(t, "", proj.Spec.DestinationServiceAccounts[1].Namespace) + assert.Equal(t, "test-sa", proj.Spec.DestinationServiceAccounts[1].DefaultServiceAccount) + + assert.Equal(t, "https://192.168.99.100:8443", proj.Spec.DestinationServiceAccounts[2].Server) + assert.Equal(t, "test-ns1", proj.Spec.DestinationServiceAccounts[2].Namespace) + assert.Equal(t, "default:test-sa", proj.Spec.DestinationServiceAccounts[2].DefaultServiceAccount) + + assertProjHasEvent(t, proj, "update", argo.EventReasonResourceUpdated) + +} From 627897a27e424b63d72748883cdfb39b9eae48f8 Mon Sep 17 00:00:00 2001 From: anandf Date: Tue, 24 Sep 2024 23:11:50 +0530 Subject: [PATCH 06/20] Fixed failing e2e tests Signed-off-by: anandf --- test/e2e/project_management_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/project_management_test.go b/test/e2e/project_management_test.go index c7c26cb5de7ec..585c47e933f37 100644 --- a/test/e2e/project_management_test.go +++ b/test/e2e/project_management_test.go @@ -704,7 +704,7 @@ func TestAddProjectDestinationServiceAccount(t *testing.T) { proj, err := fixture.AppClientset.ArgoprojV1alpha1().AppProjects(fixture.TestNamespace()).Get(context.Background(), projectName, metav1.GetOptions{}) require.NoError(t, err) assert.Equal(t, projectName, proj.Name) - assert.Len(t, proj.Spec.DestinationServiceAccounts, 2) + assert.Len(t, proj.Spec.DestinationServiceAccounts, 3) assert.Equal(t, "https://192.168.99.100:8443", proj.Spec.DestinationServiceAccounts[0].Server) assert.Equal(t, "test-ns", proj.Spec.DestinationServiceAccounts[0].Namespace) From 142fcf8b2535ad04fb3f092cb7c24a61414d5aa0 Mon Sep 17 00:00:00 2001 From: anandf Date: Tue, 24 Sep 2024 23:41:55 +0530 Subject: [PATCH 07/20] Fix linting errors Signed-off-by: anandf --- controller/sync_test.go | 90 ++++++++++++++--------------- test/e2e/project_management_test.go | 1 - 2 files changed, 42 insertions(+), 49 deletions(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index 5c4790d508f11..62536bf0aca01 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -653,30 +653,28 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { } setup := func(destinationServiceAccounts []v1alpha1.ApplicationDestinationServiceAccount, destinationNamespace, destinationServerURL, applicationNamespace string) *fixture { - project := - &v1alpha1.AppProject{ - ObjectMeta: v1.ObjectMeta{ - Namespace: "argocd-ns", - Name: "testProj", - }, - Spec: v1alpha1.AppProjectSpec{ - DestinationServiceAccounts: destinationServiceAccounts, - }, - } - app := - &v1alpha1.Application{ - ObjectMeta: v1.ObjectMeta{ - Namespace: applicationNamespace, - Name: "testApp", - }, - Spec: v1alpha1.ApplicationSpec{ - Project: "testProj", - Destination: v1alpha1.ApplicationDestination{ - Server: destinationServerURL, - Namespace: destinationNamespace, - }, + project := &v1alpha1.AppProject{ + ObjectMeta: v1.ObjectMeta{ + Namespace: "argocd-ns", + Name: "testProj", + }, + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: destinationServiceAccounts, + }, + } + app := &v1alpha1.Application{ + ObjectMeta: v1.ObjectMeta{ + Namespace: applicationNamespace, + Name: "testApp", + }, + Spec: v1alpha1.ApplicationSpec{ + Project: "testProj", + Destination: v1alpha1.ApplicationDestination{ + Server: destinationServerURL, + Namespace: destinationNamespace, }, - } + }, + } return &fixture{ project: project, application: app, @@ -968,7 +966,6 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { // then, there should not be any error and the service account with its namespace should be returned. assert.NoError(t, err) }) - } func TestDeriveServiceAccountMatchingServers(t *testing.T) { @@ -978,30 +975,28 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { } setup := func(destinationServiceAccounts []v1alpha1.ApplicationDestinationServiceAccount, destinationNamespace, destinationServerURL, applicationNamespace string) *fixture { - project := - &v1alpha1.AppProject{ - ObjectMeta: v1.ObjectMeta{ - Namespace: "argocd-ns", - Name: "testProj", - }, - Spec: v1alpha1.AppProjectSpec{ - DestinationServiceAccounts: destinationServiceAccounts, - }, - } - app := - &v1alpha1.Application{ - ObjectMeta: v1.ObjectMeta{ - Namespace: applicationNamespace, - Name: "testApp", - }, - Spec: v1alpha1.ApplicationSpec{ - Project: "testProj", - Destination: v1alpha1.ApplicationDestination{ - Server: destinationServerURL, - Namespace: destinationNamespace, - }, + project := &v1alpha1.AppProject{ + ObjectMeta: v1.ObjectMeta{ + Namespace: "argocd-ns", + Name: "testProj", + }, + Spec: v1alpha1.AppProjectSpec{ + DestinationServiceAccounts: destinationServiceAccounts, + }, + } + app := &v1alpha1.Application{ + ObjectMeta: v1.ObjectMeta{ + Namespace: applicationNamespace, + Name: "testApp", + }, + Spec: v1alpha1.ApplicationSpec{ + Project: "testProj", + Destination: v1alpha1.ApplicationDestination{ + Server: destinationServerURL, + Namespace: destinationNamespace, }, - } + }, + } return &fixture{ project: project, application: app, @@ -1222,7 +1217,6 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { assert.NoError(t, err) assert.Equal(t, expectedSA, sa) }) - } func TestSyncWithImpersonate(t *testing.T) { diff --git a/test/e2e/project_management_test.go b/test/e2e/project_management_test.go index 585c47e933f37..6a3dbca2345f3 100644 --- a/test/e2e/project_management_test.go +++ b/test/e2e/project_management_test.go @@ -719,5 +719,4 @@ func TestAddProjectDestinationServiceAccount(t *testing.T) { assert.Equal(t, "default:test-sa", proj.Spec.DestinationServiceAccounts[2].DefaultServiceAccount) assertProjHasEvent(t, proj, "update", argo.EventReasonResourceUpdated) - } From 1cb20848dafb6d238e40159dba3e2712585a18f6 Mon Sep 17 00:00:00 2001 From: anandf Date: Wed, 25 Sep 2024 00:02:25 +0530 Subject: [PATCH 08/20] Using require package instead of assert and fixed code generation Signed-off-by: anandf --- assets/swagger.json | 2 +- controller/sync_test.go | 28 +++++++++---------- manifests/core-install.yaml | 5 +++- manifests/crds/appproject-crd.yaml | 5 +++- manifests/ha/install.yaml | 5 +++- manifests/install.yaml | 5 +++- pkg/apis/application/v1alpha1/generated.proto | 2 +- 7 files changed, 32 insertions(+), 20 deletions(-) diff --git a/assets/swagger.json b/assets/swagger.json index 1e8a981c51c66..68027dc60a45a 100644 --- a/assets/swagger.json +++ b/assets/swagger.json @@ -6084,7 +6084,7 @@ "properties": { "defaultServiceAccount": { "type": "string", - "title": "ServiceAccountName to be used for impersonation during the sync operation" + "title": "DefaultServiceAccount to be used for impersonation during the sync operation" }, "namespace": { "description": "Namespace specifies the target namespace for the application's resources.", diff --git a/controller/sync_test.go b/controller/sync_test.go index 62536bf0aca01..acb298eb22209 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -720,7 +720,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should be no error and should use the right service account for impersonation - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, expectedSA, sa) }) @@ -759,7 +759,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should be no error and should use the right service account for impersonation - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, expectedSA, sa) }) @@ -798,7 +798,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should be no error and it should use the first matching service account for impersonation - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, expectedSA, sa) }) @@ -832,7 +832,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and should use the first matching glob pattern service account for impersonation - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, expectedSA, sa) }) @@ -867,7 +867,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should be an error saying no match was found - assert.EqualError(t, err, expectedErrMsg) + require.EqualError(t, err, expectedErrMsg) assert.Equal(t, expectedSA, sa) }) @@ -895,7 +895,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and the service account configured for with empty namespace should be used. - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, expectedSA, sa) }) @@ -929,7 +929,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and the catch all service account should be returned - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, expectedSA, sa) }) @@ -964,7 +964,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { assert.Equal(t, expectedSA, sa) // then, there should not be any error and the service account with its namespace should be returned. - assert.NoError(t, err) + require.NoError(t, err) }) } @@ -1038,7 +1038,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and the right service account must be returned. - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, expectedSA, sa) }) @@ -1077,7 +1077,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and first matching service account should be used - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, expectedSA, sa) }) @@ -1112,7 +1112,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { assert.Equal(t, expectedSA, sa) // then, there should not be any error and the service account of the glob pattern, being the first match should be returned. - assert.NoError(t, err) + require.NoError(t, err) }) t.Run("no match among a valid list", func(t *testing.T) { @@ -1146,7 +1146,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there an error with appropriate message must be returned - assert.EqualError(t, err, expectedErr) + require.EqualError(t, err, expectedErr) assert.Equal(t, expectedSA, sa) }) @@ -1180,7 +1180,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and the service account of the glob pattern match must be returned. - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, expectedSA, sa) }) @@ -1214,7 +1214,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { sa, err := deriveServiceAccountToImpersonate(f.project, f.application) // then, there should not be any error and the service account with the given namespace prefix must be returned. - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, expectedSA, sa) }) } diff --git a/manifests/core-install.yaml b/manifests/core-install.yaml index 662230f5a3f80..4e4e874778f08 100644 --- a/manifests/core-install.yaml +++ b/manifests/core-install.yaml @@ -21735,7 +21735,7 @@ spec: sync operation. properties: defaultServiceAccount: - description: ServiceAccountName to be used for impersonation + description: DefaultServiceAccount to be used for impersonation during the sync operation type: string namespace: @@ -21746,6 +21746,9 @@ spec: description: Server specifies the URL of the target cluster's Kubernetes control plane API. type: string + required: + - defaultServiceAccount + - server type: object type: array destinations: diff --git a/manifests/crds/appproject-crd.yaml b/manifests/crds/appproject-crd.yaml index 07e98e13b5928..a72a8de146939 100644 --- a/manifests/crds/appproject-crd.yaml +++ b/manifests/crds/appproject-crd.yaml @@ -95,7 +95,7 @@ spec: sync operation. properties: defaultServiceAccount: - description: ServiceAccountName to be used for impersonation + description: DefaultServiceAccount to be used for impersonation during the sync operation type: string namespace: @@ -106,6 +106,9 @@ spec: description: Server specifies the URL of the target cluster's Kubernetes control plane API. type: string + required: + - defaultServiceAccount + - server type: object type: array destinations: diff --git a/manifests/ha/install.yaml b/manifests/ha/install.yaml index a7002d16fdafd..9b667cd45c13b 100644 --- a/manifests/ha/install.yaml +++ b/manifests/ha/install.yaml @@ -21735,7 +21735,7 @@ spec: sync operation. properties: defaultServiceAccount: - description: ServiceAccountName to be used for impersonation + description: DefaultServiceAccount to be used for impersonation during the sync operation type: string namespace: @@ -21746,6 +21746,9 @@ spec: description: Server specifies the URL of the target cluster's Kubernetes control plane API. type: string + required: + - defaultServiceAccount + - server type: object type: array destinations: diff --git a/manifests/install.yaml b/manifests/install.yaml index 3e312f184f1ed..99a7cb032e034 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -21735,7 +21735,7 @@ spec: sync operation. properties: defaultServiceAccount: - description: ServiceAccountName to be used for impersonation + description: DefaultServiceAccount to be used for impersonation during the sync operation type: string namespace: @@ -21746,6 +21746,9 @@ spec: description: Server specifies the URL of the target cluster's Kubernetes control plane API. type: string + required: + - defaultServiceAccount + - server type: object type: array destinations: diff --git a/pkg/apis/application/v1alpha1/generated.proto b/pkg/apis/application/v1alpha1/generated.proto index 0b79d47202cb9..0380e1a546d8d 100644 --- a/pkg/apis/application/v1alpha1/generated.proto +++ b/pkg/apis/application/v1alpha1/generated.proto @@ -156,7 +156,7 @@ message ApplicationDestinationServiceAccount { // Namespace specifies the target namespace for the application's resources. optional string namespace = 2; - // ServiceAccountName to be used for impersonation during the sync operation + // DefaultServiceAccount to be used for impersonation during the sync operation optional string defaultServiceAccount = 3; } From 583d1ad48336335fe3511a78cf15597b0b75ff14 Mon Sep 17 00:00:00 2001 From: anandf Date: Wed, 25 Sep 2024 00:22:51 +0530 Subject: [PATCH 09/20] Removed parallel execution of tests for sync with impersonate Signed-off-by: anandf --- controller/sync_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/controller/sync_test.go b/controller/sync_test.go index acb298eb22209..a5632aa22e897 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1280,7 +1280,6 @@ func TestSyncWithImpersonate(t *testing.T) { 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 - t.Parallel() f := setup(true, test.FakeArgoCDNamespace, "") opMessage := "failed to find a matching service account to impersonate: no matching service account found for destination server https://localhost:6443 and namespace fake-dest-ns" @@ -1302,7 +1301,6 @@ func TestSyncWithImpersonate(t *testing.T) { 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 - t.Parallel() f := setup(true, test.FakeDestNamespace, "") opMessage := "failed to find a matching service account to impersonate: default service account cannot be an empty string" @@ -1324,7 +1322,6 @@ func TestSyncWithImpersonate(t *testing.T) { 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 - t.Parallel() f := setup(true, test.FakeDestNamespace, "test-sa") opMessage := "successfully synced (no more tasks)" @@ -1346,7 +1343,6 @@ func TestSyncWithImpersonate(t *testing.T) { 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 - t.Parallel() f := setup(false, test.FakeDestNamespace, "") opMessage := "successfully synced (no more tasks)" From 256d4496a163363924c2fdf4d0909731f3521db0 Mon Sep 17 00:00:00 2001 From: anandf Date: Wed, 25 Sep 2024 01:40:28 +0530 Subject: [PATCH 10/20] Added err checks for glob validations Signed-off-by: anandf --- cmd/util/project.go | 6 ++++-- controller/sync.go | 10 ++++++++-- .../application/v1alpha1/app_project_types.go | 16 +++++++++++++++- util/glob/glob.go | 8 ++++++++ 4 files changed, 35 insertions(+), 5 deletions(-) diff --git a/cmd/util/project.go b/cmd/util/project.go index b0a82883dc0bb..3626f414aafd9 100644 --- a/cmd/util/project.go +++ b/cmd/util/project.go @@ -48,6 +48,8 @@ func AddProjFlags(command *cobra.Command, opts *ProjectOpts) { command.Flags().StringArrayVar(&opts.allowedNamespacedResources, "allow-namespaced-resource", []string{}, "List of allowed namespaced resources") command.Flags().StringArrayVar(&opts.deniedNamespacedResources, "deny-namespaced-resource", []string{}, "List of denied namespaced resources") command.Flags().StringSliceVar(&opts.SourceNamespaces, "source-namespaces", []string{}, "List of source namespaces for applications") + command.Flags().StringArrayVarP(&opts.destinationServiceAccounts, "dest-service-accounts", "", []string{}, + "Destination server, namespace and target service account (e.g. https://192.168.99.100:8443,default,default-sa)") } func getGroupKindList(values []string) []v1.GroupKind { @@ -98,8 +100,8 @@ func (opts *ProjectOpts) GetDestinationServiceAccounts() []v1alpha1.ApplicationD destinationServiceAccounts := make([]v1alpha1.ApplicationDestinationServiceAccount, 0) for _, destStr := range opts.destinationServiceAccounts { parts := strings.Split(destStr, ",") - if len(parts) != 2 { - log.Fatalf("Expected destination of the form: server,namespace. Received: %s", destStr) + if len(parts) != 3 { + log.Fatalf("Expected destination service account of the form: server,namespace, defaultServiceAccount. Received: %s", destStr) } else { destinationServiceAccounts = append(destinationServiceAccounts, v1alpha1.ApplicationDestinationServiceAccount{ Server: parts[0], diff --git a/controller/sync.go b/controller/sync.go index f3336a9ef99b7..3d39a42b01b01 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -574,8 +574,14 @@ func deriveServiceAccountToImpersonate(project *v1alpha1.AppProject, application // Loop through the destinationServiceAccounts and see if there is any destination that is a candidate. // if so, return the service account specified for that destination. for _, item := range project.Spec.DestinationServiceAccounts { - dstServerMatched := glob.Match(item.Server, application.Spec.Destination.Server) - dstNamespaceMatched := glob.Match(item.Namespace, application.Spec.Destination.Namespace) + dstServerMatched, err := glob.MatchWithError(item.Server, application.Spec.Destination.Server) + if err != nil { + return "", err + } + dstNamespaceMatched, err := glob.MatchWithError(item.Namespace, application.Spec.Destination.Namespace) + if err != nil { + return "", err + } if dstServerMatched && dstNamespaceMatched { if item.DefaultServiceAccount == "" { return "", fmt.Errorf("default service account cannot be an empty string") diff --git a/pkg/apis/application/v1alpha1/app_project_types.go b/pkg/apis/application/v1alpha1/app_project_types.go index b888cd37391b3..b2488ce6a6a52 100644 --- a/pkg/apis/application/v1alpha1/app_project_types.go +++ b/pkg/apis/application/v1alpha1/app_project_types.go @@ -8,7 +8,7 @@ import ( "github.com/argoproj/argo-cd/v2/util/git" "github.com/argoproj/argo-cd/v2/util/glob" - + globutil "github.com/gobwas/glob" "github.com/google/go-cmp/cmp" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -275,6 +275,20 @@ func (p *AppProject) ValidateProject() error { return status.Errorf(codes.InvalidArgument, "namespace has an invalid format, '!*'") } + if strings.Contains(destServiceAcct.DefaultServiceAccount, "*") { + return status.Errorf(codes.InvalidArgument, "defaultServiceAccount does not support glob patterns") + } + + _, err := globutil.Compile(destServiceAcct.Server) + if err != nil { + return err + } + + _, err = globutil.Compile(destServiceAcct.Namespace) + if err != nil { + return err + } + key := fmt.Sprintf("%s/%s", destServiceAcct.Server, destServiceAcct.Namespace) if _, ok := destServiceAccts[key]; ok { return status.Errorf(codes.InvalidArgument, "destinationServiceAccount '%s' already added", key) diff --git a/util/glob/glob.go b/util/glob/glob.go index 0ef7cda28635a..b104d9a184318 100644 --- a/util/glob/glob.go +++ b/util/glob/glob.go @@ -13,3 +13,11 @@ func Match(pattern, text string, separators ...rune) bool { } return compiledGlob.Match(text) } + +func MatchWithError(pattern, text string, separators ...rune) (bool, error) { + compiledGlob, err := glob.Compile(pattern, separators...) + if err != nil { + return false, err + } + return compiledGlob.Match(text), nil +} From 20506cf26c827e193c76ca09aa18e28cd2c7a4c8 Mon Sep 17 00:00:00 2001 From: anandf Date: Wed, 25 Sep 2024 19:33:01 +0530 Subject: [PATCH 11/20] Fixed e2e tests for sync impersonation Signed-off-by: anandf --- test/e2e/fixture/app/consequences.go | 25 +++++++++ test/e2e/sync_with_impersonate_test.go | 78 ++++++++++++-------------- 2 files changed, 61 insertions(+), 42 deletions(-) diff --git a/test/e2e/fixture/app/consequences.go b/test/e2e/fixture/app/consequences.go index 9ee99fec6ca6d..020d424ffbcda 100644 --- a/test/e2e/fixture/app/consequences.go +++ b/test/e2e/fixture/app/consequences.go @@ -42,6 +42,31 @@ func (c *Consequences) Expect(e Expectation) *Consequences { return c } +// ExpectConsistently will continously evaluate a condition, and it must be true each time it is evaluated, otherwise the test is failed. The condition will be repeatedly evaluated until 'expirationDuration' is met, waiting 'waitDuration' after each success. +func (c *Consequences) ExpectConsistently(e Expectation, waitDuration time.Duration, expirationDuration time.Duration) *Consequences { + // this invocation makes sure this func is not reported as the cause of the failure - we are a "test helper" + c.context.t.Helper() + + expiration := time.Now().Add(expirationDuration) + for time.Now().Before(expiration) { + state, message := e(c) + switch state { + case succeeded: + log.Infof("expectation succeeded: %s", message) + case failed: + c.context.t.Fatalf("failed expectation: %s", message) + return c + } + + // On condition success: wait, then retry + log.Infof("Expectation '%s' passes, repeating to ensure consistency", message) + time.Sleep(waitDuration) + } + + // If the condition never failed before expiring, it is a pass. + return c +} + func (c *Consequences) And(block func(app *Application)) *Consequences { c.context.t.Helper() block(c.app()) diff --git a/test/e2e/sync_with_impersonate_test.go b/test/e2e/sync_with_impersonate_test.go index 7bb57af1156d3..3e15da57215ab 100644 --- a/test/e2e/sync_with_impersonate_test.go +++ b/test/e2e/sync_with_impersonate_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "testing" + "time" "github.com/stretchr/testify/require" v1 "k8s.io/api/core/v1" @@ -16,7 +17,12 @@ import ( . "github.com/argoproj/argo-cd/v2/test/e2e/fixture/app" ) -func TestSyncWithImpersonateDisable(t *testing.T) { +const ( + WaitDuration = time.Second + TimeoutDuration = time.Second * 2 +) + +func TestSyncWithFeatureDisabled(t *testing.T) { Given(t). Path("guestbook"). When(). @@ -25,10 +31,13 @@ func TestSyncWithImpersonateDisable(t *testing.T) { app.Spec.SyncPolicy = &v1alpha1.SyncPolicy{Automated: &v1alpha1.SyncPolicyAutomated{}} }). Then(). - Expect(SyncStatusIs(v1alpha1.SyncStatusCodeSynced)) + // With the impersonation feature disabled, Application sync should continue to use + // the control plane service account for the sync operation and the sync should succeed. + Expect(SyncStatusIs(v1alpha1.SyncStatusCodeSynced)). + Expect(OperationMessageContains("successfully synced")) } -func TestSyncWithImpersonateDefaultNamespaceServiceAccountNoRBAC(t *testing.T) { +func TestSyncWithNoDestinationServiceAccountsInProject(t *testing.T) { Given(t). Path("guestbook"). When(). @@ -37,37 +46,10 @@ func TestSyncWithImpersonateDefaultNamespaceServiceAccountNoRBAC(t *testing.T) { app.Spec.SyncPolicy = &v1alpha1.SyncPolicy{Automated: &v1alpha1.SyncPolicyAutomated{}} }). Then(). - Expect(SyncStatusIs(v1alpha1.SyncStatusCodeOutOfSync)) -} - -func TestSyncWithImpersonateDefaultNamespaceServiceAccountWithRBAC(t *testing.T) { - roleName := "default-sa-role" - Given(t). - Path("guestbook"). - When(). - SetParamInSettingConfigMap("application.sync.impersonation.enabled", "true"). - CreateFromFile(func(app *v1alpha1.Application) { - app.Spec.SyncPolicy = &v1alpha1.SyncPolicy{Automated: &v1alpha1.SyncPolicyAutomated{}} - }). - And(func() { - err := createTestRole(roleName, fixture.DeploymentNamespace(), []rbac.PolicyRule{ - { - APIGroups: []string{"apps", ""}, - Resources: []string{"deployments"}, - Verbs: []string{"*"}, - }, - { - APIGroups: []string{""}, - Resources: []string{"services"}, - Verbs: []string{"*"}, - }, - }) - require.NoError(t, err) - err = createTestRoleBinding(roleName, "default", fixture.DeploymentNamespace()) - require.NoError(t, err) - }). - Then(). - Expect(SyncStatusIs(v1alpha1.SyncStatusCodeOutOfSync)) + // With the impersonation feature enabled, Application sync must fail + // when there are no destination service accounts configured in AppProject + Expect(SyncStatusIs(v1alpha1.SyncStatusCodeOutOfSync)). + Expect(OperationMessageContains("failed to find a matching service account to impersonate")) } func TestSyncWithImpersonateWithSyncServiceAccount(t *testing.T) { @@ -89,7 +71,7 @@ func TestSyncWithImpersonateWithSyncServiceAccount(t *testing.T) { { Server: "*", Namespace: fixture.DeploymentNamespace(), - DefaultServiceAccount: "false-serviceAccount", + DefaultServiceAccount: "missing-serviceAccount", }, } err := createTestServiceAccount(serviceAccountName, fixture.DeploymentNamespace()) @@ -118,10 +100,13 @@ func TestSyncWithImpersonateWithSyncServiceAccount(t *testing.T) { app.Spec.Project = projectName }). Then(). - Expect(SyncStatusIs(v1alpha1.SyncStatusCodeSynced)) + // With the impersonation feature enabled, Application sync should succeed + // as there is a valid match found in the available destination service accounts configured in AppProject + Expect(SyncStatusIs(v1alpha1.SyncStatusCodeSynced)). + Expect(OperationMessageContains("successfully synced")) } -func TestSyncWithImpersonateWithFalseServiceAccount(t *testing.T) { +func TestSyncWithMissingServiceAccount(t *testing.T) { projectName := "false-test-project" serviceAccountName := "test-account" roleName := "test-account-sa-role" @@ -135,7 +120,7 @@ func TestSyncWithImpersonateWithFalseServiceAccount(t *testing.T) { { Server: "*", Namespace: fixture.DeploymentNamespace(), - DefaultServiceAccount: "false-serviceAccount", + DefaultServiceAccount: "missing-serviceAccount", }, { Server: "*", @@ -169,11 +154,15 @@ func TestSyncWithImpersonateWithFalseServiceAccount(t *testing.T) { app.Spec.Project = projectName }). Then(). - Expect(SyncStatusIs(v1alpha1.SyncStatusCodeOutOfSync)) + // With the impersonation feature enabled, Application sync must fail + // when there is a valid match found in the available destination service accounts configured in AppProject, + // but the matching service account is missing. + Expect(SyncStatusIs(v1alpha1.SyncStatusCodeOutOfSync)). + Expect(OperationMessageContains("one or more objects failed to apply")) } -func TestSyncWithNegationApplicationDestinationNamespace(t *testing.T) { - projectName := "nagation-test-project" +func TestSyncWithValidSAButDisallowedDestination(t *testing.T) { + projectName := "negation-test-project" serviceAccountName := "test-account" roleName := "test-account-sa-role" Given(t). @@ -217,6 +206,7 @@ func TestSyncWithNegationApplicationDestinationNamespace(t *testing.T) { Expect(SyncStatusIs(v1alpha1.SyncStatusCodeSynced)). When(). And(func() { + // Patch destination to disallow target destination namespace patch := []byte(fmt.Sprintf(`{"spec": {"destinations": [{"namespace": "%s"}]}}`, "!"+fixture.DeploymentNamespace())) _, err := fixture.AppClientset.ArgoprojV1alpha1().AppProjects(fixture.TestNamespace()).Patch(context.Background(), projectName, types.MergePatchType, patch, metav1.PatchOptions{}) @@ -224,7 +214,11 @@ func TestSyncWithNegationApplicationDestinationNamespace(t *testing.T) { }). Refresh(v1alpha1.RefreshTypeNormal). Then(). - Expect(SyncStatusIs(v1alpha1.SyncStatusCodeUnknown)) + // With the impersonation feature enabled, Application sync must fail + // as there is a valid match found in the available destination service accounts configured in AppProject + // but the destination namespace is now disallowed. + Expect(SyncStatusIs(v1alpha1.SyncStatusCodeUnknown)). + Expect(OperationMessageContains("do not match any of the allowed destinations in project")) } // createTestAppProject creates a test AppProject resource. From e7f45a115e0e5bf52f47cb35e348c427043469aa Mon Sep 17 00:00:00 2001 From: anandf Date: Wed, 25 Sep 2024 21:34:44 +0530 Subject: [PATCH 12/20] Using consistently based expects in E2E tests Signed-off-by: anandf --- test/e2e/sync_with_impersonate_test.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/test/e2e/sync_with_impersonate_test.go b/test/e2e/sync_with_impersonate_test.go index 3e15da57215ab..4c7cf166f4a09 100644 --- a/test/e2e/sync_with_impersonate_test.go +++ b/test/e2e/sync_with_impersonate_test.go @@ -19,7 +19,7 @@ import ( const ( WaitDuration = time.Second - TimeoutDuration = time.Second * 2 + TimeoutDuration = time.Second * 3 ) func TestSyncWithFeatureDisabled(t *testing.T) { @@ -33,7 +33,7 @@ func TestSyncWithFeatureDisabled(t *testing.T) { Then(). // With the impersonation feature disabled, Application sync should continue to use // the control plane service account for the sync operation and the sync should succeed. - Expect(SyncStatusIs(v1alpha1.SyncStatusCodeSynced)). + ExpectConsistently(SyncStatusIs(v1alpha1.SyncStatusCodeSynced), WaitDuration, TimeoutDuration). Expect(OperationMessageContains("successfully synced")) } @@ -48,7 +48,7 @@ func TestSyncWithNoDestinationServiceAccountsInProject(t *testing.T) { Then(). // With the impersonation feature enabled, Application sync must fail // when there are no destination service accounts configured in AppProject - Expect(SyncStatusIs(v1alpha1.SyncStatusCodeOutOfSync)). + ExpectConsistently(SyncStatusIs(v1alpha1.SyncStatusCodeOutOfSync), WaitDuration, TimeoutDuration). Expect(OperationMessageContains("failed to find a matching service account to impersonate")) } @@ -102,7 +102,7 @@ func TestSyncWithImpersonateWithSyncServiceAccount(t *testing.T) { Then(). // With the impersonation feature enabled, Application sync should succeed // as there is a valid match found in the available destination service accounts configured in AppProject - Expect(SyncStatusIs(v1alpha1.SyncStatusCodeSynced)). + ExpectConsistently(SyncStatusIs(v1alpha1.SyncStatusCodeSynced), WaitDuration, TimeoutDuration). Expect(OperationMessageContains("successfully synced")) } @@ -157,7 +157,7 @@ func TestSyncWithMissingServiceAccount(t *testing.T) { // With the impersonation feature enabled, Application sync must fail // when there is a valid match found in the available destination service accounts configured in AppProject, // but the matching service account is missing. - Expect(SyncStatusIs(v1alpha1.SyncStatusCodeOutOfSync)). + ExpectConsistently(SyncStatusIs(v1alpha1.SyncStatusCodeOutOfSync), WaitDuration, TimeoutDuration). Expect(OperationMessageContains("one or more objects failed to apply")) } @@ -217,8 +217,7 @@ func TestSyncWithValidSAButDisallowedDestination(t *testing.T) { // With the impersonation feature enabled, Application sync must fail // as there is a valid match found in the available destination service accounts configured in AppProject // but the destination namespace is now disallowed. - Expect(SyncStatusIs(v1alpha1.SyncStatusCodeUnknown)). - Expect(OperationMessageContains("do not match any of the allowed destinations in project")) + ExpectConsistently(SyncStatusIs(v1alpha1.SyncStatusCodeUnknown), WaitDuration, TimeoutDuration) } // createTestAppProject creates a test AppProject resource. From ee743a2b7192002392c6c2c6ae280886ab43e23c Mon Sep 17 00:00:00 2001 From: anandf Date: Thu, 26 Sep 2024 13:29:08 +0530 Subject: [PATCH 13/20] Added more unit tests and fixed go generate Signed-off-by: anandf --- controller/sync.go | 10 +- controller/sync_test.go | 48 ++++++ .../argocd_admin_proj_generate-spec.md | 1 + .../user-guide/commands/argocd_proj_create.md | 1 + docs/user-guide/commands/argocd_proj_set.md | 1 + .../application/v1alpha1/app_project_types.go | 21 ++- pkg/apis/application/v1alpha1/types_test.go | 155 ++++++++++++++++++ test/e2e/project_management_test.go | 121 ++++++++++++++ 8 files changed, 346 insertions(+), 12 deletions(-) diff --git a/controller/sync.go b/controller/sync.go index 3d39a42b01b01..951374e052bea 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -44,6 +44,8 @@ const ( // EnvVarSyncWaveDelay is an environment variable which controls the delay in seconds between // each sync-wave EnvVarSyncWaveDelay = "ARGOCD_SYNC_WAVE_DELAY" + + serviceAccountDisallowedCharSet = "!*[]{}\\/" ) func (m *appStateManager) getOpenAPISchema(server string) (openapi.Resources, error) { @@ -576,15 +578,15 @@ func deriveServiceAccountToImpersonate(project *v1alpha1.AppProject, application for _, item := range project.Spec.DestinationServiceAccounts { dstServerMatched, err := glob.MatchWithError(item.Server, application.Spec.Destination.Server) if err != nil { - return "", err + return "", fmt.Errorf("invalid glob pattern for destination server: %v", err) } dstNamespaceMatched, err := glob.MatchWithError(item.Namespace, application.Spec.Destination.Namespace) if err != nil { - return "", err + return "", fmt.Errorf("invalid glob pattern for destination namespace: %v", err) } if dstServerMatched && dstNamespaceMatched { - if item.DefaultServiceAccount == "" { - return "", fmt.Errorf("default service account cannot be an empty string") + if strings.Trim(item.DefaultServiceAccount, " ") == "" || strings.ContainsAny(item.DefaultServiceAccount, "!*[]\\/") { + return "", fmt.Errorf("default service account contains invalid chars %s", item.DefaultServiceAccount) } else if strings.Contains(item.DefaultServiceAccount, ":") { // service account is specified along with its namespace. return fmt.Sprintf("system:serviceaccount:%s", item.DefaultServiceAccount), nil diff --git a/controller/sync_test.go b/controller/sync_test.go index a5632aa22e897..fcd3fb30fff3d 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -933,6 +933,30 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) { assert.Equal(t, expectedSA, sa) }) + t.Run("match done via invalid glob pattern", func(t *testing.T) { + // given an application referring a project with a destination service account having an invalid glob pattern for namespace + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://kubernetes.svc.local", + Namespace: "e[[a*", + DefaultServiceAccount: "test-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there must be an error as the glob pattern is invalid. + require.ErrorContains(t, err, "invalid glob pattern for destination namespace") + assert.Equal(t, expectedSA, sa) + }) + t.Run("sa specified with a namespace", func(t *testing.T) { // given an application referring a project with multiple destination service accounts having a matching service account specified with its namespace t.Parallel() @@ -1184,6 +1208,30 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) { assert.Equal(t, expectedSA, sa) }) + t.Run("match done via invalid glob pattern", func(t *testing.T) { + // given an application referring a project with a destination service account having an invalid glob pattern for server + t.Parallel() + destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "e[[a*", + Namespace: "test-ns", + DefaultServiceAccount: "test-sa", + }, + } + destinationNamespace := "testns" + destinationServerURL := "https://kubernetes.svc.local" + applicationNamespace := "argocd-ns" + expectedSA := "" + + f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace) + // when + sa, err := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there must be an error as the glob pattern is invalid. + require.ErrorContains(t, err, "invalid glob pattern for destination server") + assert.Equal(t, expectedSA, sa) + }) + t.Run("sa specified with a namespace", func(t *testing.T) { // given app sync impersonation feature is enabled and matching service account is prefixed with a namespace t.Parallel() diff --git a/docs/user-guide/commands/argocd_admin_proj_generate-spec.md b/docs/user-guide/commands/argocd_admin_proj_generate-spec.md index c94eba4365ef8..2be37408d8153 100644 --- a/docs/user-guide/commands/argocd_admin_proj_generate-spec.md +++ b/docs/user-guide/commands/argocd_admin_proj_generate-spec.md @@ -30,6 +30,7 @@ argocd admin proj generate-spec PROJECT [flags] --deny-namespaced-resource stringArray List of denied namespaced resources --description string Project description -d, --dest stringArray Permitted destination server and namespace (e.g. https://192.168.99.100:8443,default) + --dest-service-accounts stringArray Destination server, namespace and target service account (e.g. https://192.168.99.100:8443,default,default-sa) -f, --file string Filename or URL to Kubernetes manifests for the project -h, --help help for generate-spec -i, --inline If set then generated resource is written back to the file specified in --file flag diff --git a/docs/user-guide/commands/argocd_proj_create.md b/docs/user-guide/commands/argocd_proj_create.md index c8b27e35bb762..5d222c7a6eab0 100644 --- a/docs/user-guide/commands/argocd_proj_create.md +++ b/docs/user-guide/commands/argocd_proj_create.md @@ -27,6 +27,7 @@ argocd proj create PROJECT [flags] --deny-namespaced-resource stringArray List of denied namespaced resources --description string Project description -d, --dest stringArray Permitted destination server and namespace (e.g. https://192.168.99.100:8443,default) + --dest-service-accounts stringArray Destination server, namespace and target service account (e.g. https://192.168.99.100:8443,default,default-sa) -f, --file string Filename or URL to Kubernetes manifests for the project -h, --help help for create --orphaned-resources Enables orphaned resources monitoring diff --git a/docs/user-guide/commands/argocd_proj_set.md b/docs/user-guide/commands/argocd_proj_set.md index 7b4d79ff13588..daf633a7cb9ca 100644 --- a/docs/user-guide/commands/argocd_proj_set.md +++ b/docs/user-guide/commands/argocd_proj_set.md @@ -27,6 +27,7 @@ argocd proj set PROJECT [flags] --deny-namespaced-resource stringArray List of denied namespaced resources --description string Project description -d, --dest stringArray Permitted destination server and namespace (e.g. https://192.168.99.100:8443,default) + --dest-service-accounts stringArray Destination server, namespace and target service account (e.g. https://192.168.99.100:8443,default,default-sa) -h, --help help for set --orphaned-resources Enables orphaned resources monitoring --orphaned-resources-warn Specifies if applications should have a warning condition when orphaned resources detected diff --git a/pkg/apis/application/v1alpha1/app_project_types.go b/pkg/apis/application/v1alpha1/app_project_types.go index b2488ce6a6a52..12d6f10b22bf5 100644 --- a/pkg/apis/application/v1alpha1/app_project_types.go +++ b/pkg/apis/application/v1alpha1/app_project_types.go @@ -17,6 +17,10 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" ) +const ( + serviceAccountDisallowedCharSet = "!*[]{}\\/" +) + type ErrApplicationNotAllowedToUseProject struct { application string namespace string @@ -267,26 +271,27 @@ func (p *AppProject) ValidateProject() error { destServiceAccts := make(map[string]bool) for _, destServiceAcct := range p.Spec.DestinationServiceAccounts { - if destServiceAcct.Server == "!*" { - return status.Errorf(codes.InvalidArgument, "server has an invalid format, '!*'") + if strings.Contains(destServiceAcct.Server, "!") { + return status.Errorf(codes.InvalidArgument, "server has an invalid format, '%s'", destServiceAcct.Server) } - if destServiceAcct.Namespace == "!*" { - return status.Errorf(codes.InvalidArgument, "namespace has an invalid format, '!*'") + if strings.Contains(destServiceAcct.Namespace, "!") { + return status.Errorf(codes.InvalidArgument, "namespace has an invalid format, '%s'", destServiceAcct.Namespace) } - if strings.Contains(destServiceAcct.DefaultServiceAccount, "*") { - return status.Errorf(codes.InvalidArgument, "defaultServiceAccount does not support glob patterns") + if strings.Trim(destServiceAcct.DefaultServiceAccount, " ") == "" || + strings.ContainsAny(destServiceAcct.DefaultServiceAccount, serviceAccountDisallowedCharSet) { + return status.Errorf(codes.InvalidArgument, "defaultServiceAccount has an invalid format, '%s'", destServiceAcct.DefaultServiceAccount) } _, err := globutil.Compile(destServiceAcct.Server) if err != nil { - return err + return status.Errorf(codes.InvalidArgument, "server has an invalid format, '%s'", destServiceAcct.Server) } _, err = globutil.Compile(destServiceAcct.Namespace) if err != nil { - return err + return status.Errorf(codes.InvalidArgument, "namespace has an invalid format, '%s'", destServiceAcct.Namespace) } key := fmt.Sprintf("%s/%s", destServiceAcct.Server, destServiceAcct.Namespace) diff --git a/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go index 08b83c238a93d..4461a3418aa49 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/pkg/apis/application/v1alpha1/types_test.go @@ -3959,3 +3959,158 @@ func TestApplicationTree_Merge(t *testing.T) { }, }, tree) } + +func TestAppProject_ValidateDestinationServiceAccount(t *testing.T) { + testData := []struct { + server string + namespace string + defaultServiceAccount string + expectedErrMsg string + }{ + { + // Given, a project + // When, a default destination service account with all valid fields is added to it, + // Then, there is no error. + server: "https://192.168.99.100:8443", + namespace: "test-ns", + defaultServiceAccount: "test-sa", + expectedErrMsg: "", + }, + { + // Given, a project + // When, a default destination service account with negation glob pattern for server is added, + // Then, there is no error. + server: "!abc*", + namespace: "test-ns", + defaultServiceAccount: "test-sa", + expectedErrMsg: "server has an invalid format, '!abc*'", + }, + { + // Given, a project + // When, a default destination service account with empty namespace is added to it, + // Then, there is no error. + server: "https://192.168.99.100:8443", + namespace: "", + defaultServiceAccount: "test-sa", + expectedErrMsg: "", + }, + { + // Given, a project, + // When, a default destination service account with negation glob pattern for server is added, + // Then, there is an error with appropriate message. + server: "!*", + namespace: "test-ns", + defaultServiceAccount: "test-sa", + expectedErrMsg: "server has an invalid format, '!*'", + }, + { + // Given, a project, + // When, a default destination service account with negation glob pattern for namespace is added, + // Then, there is an error with appropriate message. + server: "https://192.168.99.100:8443", + namespace: "!*", + defaultServiceAccount: "test-sa", + expectedErrMsg: "namespace has an invalid format, '!*'", + }, + { + // Given, a project, + // When, a default destination service account with negation glob pattern for namespace is added, + // Then, there is an error with appropriate message. + server: "https://192.168.99.100:8443", + namespace: "!abc", + defaultServiceAccount: "test-sa", + expectedErrMsg: "namespace has an invalid format, '!abc'", + }, + { + // Given, a project, + // When, a default destination service account with empty service account is added, + // Then, there is an error with appropriate message. + server: "https://192.168.99.100:8443", + namespace: "test-ns", + defaultServiceAccount: "", + expectedErrMsg: "defaultServiceAccount has an invalid format, ''", + }, + { + // Given, a project, + // When, a default destination service account with service account having just white spaces is added, + // Then, there is an error with appropriate message. + server: "https://192.168.99.100:8443", + namespace: "test-ns", + defaultServiceAccount: " ", + expectedErrMsg: "defaultServiceAccount has an invalid format, ' '", + }, + { + // Given, a project, + // When, a default destination service account with service account having backwards slash char is added, + // Then, there is an error with appropriate message. + server: "https://192.168.99.100:8443", + namespace: "test-ns", + defaultServiceAccount: "test\\sa", + expectedErrMsg: "defaultServiceAccount has an invalid format, 'test\\sa'", + }, + { + // Given, a project, + // When, a default destination service account with service account having forward slash char is added, + // Then, there is an error with appropriate message. + server: "https://192.168.99.100:8443", + namespace: "test-ns", + defaultServiceAccount: "test/sa", + expectedErrMsg: "defaultServiceAccount has an invalid format, 'test/sa'", + }, + { + // Given, a project, + // When, a default destination service account with service account having square braces char is added, + // Then, there is an error with appropriate message. + server: "https://192.168.99.100:8443", + namespace: "test-ns", + defaultServiceAccount: "[test-sa]", + expectedErrMsg: "defaultServiceAccount has an invalid format, '[test-sa]'", + }, + { + // Given, a project, + // When, a default destination service account with service account having curly braces char is added, + // Then, there is an error with appropriate message. + server: "https://192.168.99.100:8443", + namespace: "test-ns", + defaultServiceAccount: "{test-sa}", + expectedErrMsg: "defaultServiceAccount has an invalid format, '{test-sa}'", + }, + { + // Given, a project, + // When, a default destination service account with service account having curly braces char is added, + // Then, there is an error with appropriate message. + server: "[[ech*", + namespace: "test-ns", + defaultServiceAccount: "test-sa", + expectedErrMsg: "server has an invalid format, '[[ech*'", + }, + { + // Given, a project, + // When, a default destination service account with service account having curly braces char is added, + // Then, there is an error with appropriate message. + server: "https://192.168.99.100:8443", + namespace: "[[ech*", + defaultServiceAccount: "test-sa", + expectedErrMsg: "namespace has an invalid format, '[[ech*'", + }, + } + for _, data := range testData { + proj := AppProject{ + Spec: AppProjectSpec{ + DestinationServiceAccounts: []ApplicationDestinationServiceAccount{ + { + Server: data.server, + Namespace: data.namespace, + DefaultServiceAccount: data.defaultServiceAccount, + }, + }, + }, + } + err := proj.ValidateProject() + if data.expectedErrMsg == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, data.expectedErrMsg) + } + } +} diff --git a/test/e2e/project_management_test.go b/test/e2e/project_management_test.go index 6a3dbca2345f3..09702cc77933e 100644 --- a/test/e2e/project_management_test.go +++ b/test/e2e/project_management_test.go @@ -679,6 +679,17 @@ func TestAddProjectDestinationServiceAccount(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "already defined") + // Given, an existing project, + // When, a duplicate default destination service account is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "test-ns", + "asdf", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "already added") + // Given, an existing project, // When, a default destination service account with negation glob pattern for server is added, // Then, there is an error with appropriate message. @@ -690,6 +701,17 @@ func TestAddProjectDestinationServiceAccount(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "server has an invalid format, '!*'") + // Given, an existing project, + // When, a default destination service account with negation glob pattern for server is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "!abc", + "test-ns", + "test-sa", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "server has an invalid format, '!*'") + // Given, an existing project, // When, a default destination service account with negation glob pattern for namespace is added, // Then, there is an error with appropriate message. @@ -701,6 +723,105 @@ func TestAddProjectDestinationServiceAccount(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "namespace has an invalid format, '!*'") + // Given, an existing project, + // When, a default destination service account with negation glob pattern for namespace is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "!abc", + "test-sa", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "namespace has an invalid format, '!abc'") + + // Given, an existing project, + // When, a default destination service account with empty service account is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "test-ns", + "", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "defaultServiceAccount has an invalid format, ''") + + // Given, an existing project, + // When, a default destination service account with service account having just white spaces is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "test-ns", + " ", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "defaultServiceAccount has an invalid format, ' '") + + // Given, an existing project, + // When, a default destination service account with service account having backwards slash char is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "test-ns", + "test\\sa", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "defaultServiceAccount has an invalid format, 'test\\sa'") + + // Given, an existing project, + // When, a default destination service account with service account having forward slash char is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "test-ns", + "test/sa", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "defaultServiceAccount has an invalid format, 'test/sa'") + + // Given, an existing project, + // When, a default destination service account with service account having square braces char is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "test-ns", + "[test-sa]", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "defaultServiceAccount has an invalid format, '[test-sa]'") + + // Given, an existing project, + // When, a default destination service account with service account having curly braces char is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "test-ns", + "{test-sa}", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "defaultServiceAccount has an invalid format, '{test-sa}'") + + // Given, an existing project, + // When, a default destination service account with service account having curly braces char is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "[[ech*", + "test-ns", + "test-sa", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "server has an invalid format, '[[ech*'") + + // Given, an existing project, + // When, a default destination service account with service account having curly braces char is added, + // Then, there is an error with appropriate message. + _, err = fixture.RunCli("proj", "add-destination-service-account", projectName, + "https://192.168.99.100:8443", + "[[ech*", + "test-sa", + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "namespace has an invalid format, '[[ech*'") + proj, err := fixture.AppClientset.ArgoprojV1alpha1().AppProjects(fixture.TestNamespace()).Get(context.Background(), projectName, metav1.GetOptions{}) require.NoError(t, err) assert.Equal(t, projectName, proj.Name) From 59557046bf05f93afe152bcf5c1d63b2c9969b95 Mon Sep 17 00:00:00 2001 From: anandf Date: Thu, 26 Sep 2024 15:15:11 +0530 Subject: [PATCH 14/20] Fixed failed lint errors, unit and e2e test failures Signed-off-by: anandf --- controller/sync.go | 8 ++++---- controller/sync_test.go | 2 +- test/e2e/fixture/app/consequences.go | 2 +- test/e2e/project_management_test.go | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/controller/sync.go b/controller/sync.go index 951374e052bea..ed5d985cd5b53 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -578,15 +578,15 @@ func deriveServiceAccountToImpersonate(project *v1alpha1.AppProject, application for _, item := range project.Spec.DestinationServiceAccounts { dstServerMatched, err := glob.MatchWithError(item.Server, application.Spec.Destination.Server) if err != nil { - return "", fmt.Errorf("invalid glob pattern for destination server: %v", err) + return "", fmt.Errorf("invalid glob pattern for destination server: %w", err) } dstNamespaceMatched, err := glob.MatchWithError(item.Namespace, application.Spec.Destination.Namespace) if err != nil { - return "", fmt.Errorf("invalid glob pattern for destination namespace: %v", err) + return "", fmt.Errorf("invalid glob pattern for destination namespace: %w", err) } if dstServerMatched && dstNamespaceMatched { - if strings.Trim(item.DefaultServiceAccount, " ") == "" || strings.ContainsAny(item.DefaultServiceAccount, "!*[]\\/") { - return "", fmt.Errorf("default service account contains invalid chars %s", item.DefaultServiceAccount) + if strings.Trim(item.DefaultServiceAccount, " ") == "" || strings.ContainsAny(item.DefaultServiceAccount, serviceAccountDisallowedCharSet) { + return "", fmt.Errorf("default service account contains invalid chars '%s'", item.DefaultServiceAccount) } else if strings.Contains(item.DefaultServiceAccount, ":") { // service account is specified along with its namespace. return fmt.Sprintf("system:serviceaccount:%s", item.DefaultServiceAccount), nil diff --git a/controller/sync_test.go b/controller/sync_test.go index fcd3fb30fff3d..0751434eab78c 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1350,7 +1350,7 @@ func TestSyncWithImpersonate(t *testing.T) { 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.FakeDestNamespace, "") - opMessage := "failed to find a matching service account to impersonate: default service account cannot be an empty string" + opMessage := "failed to find a matching service account to impersonate: default service account contains invalid chars ''" opState := &v1alpha1.OperationState{ Operation: v1alpha1.Operation{ diff --git a/test/e2e/fixture/app/consequences.go b/test/e2e/fixture/app/consequences.go index 020d424ffbcda..ff64dee0de4b8 100644 --- a/test/e2e/fixture/app/consequences.go +++ b/test/e2e/fixture/app/consequences.go @@ -42,7 +42,7 @@ func (c *Consequences) Expect(e Expectation) *Consequences { return c } -// ExpectConsistently will continously evaluate a condition, and it must be true each time it is evaluated, otherwise the test is failed. The condition will be repeatedly evaluated until 'expirationDuration' is met, waiting 'waitDuration' after each success. +// ExpectConsistently will continuously evaluate a condition, and it must be true each time it is evaluated, otherwise the test is failed. The condition will be repeatedly evaluated until 'expirationDuration' is met, waiting 'waitDuration' after each success. func (c *Consequences) ExpectConsistently(e Expectation, waitDuration time.Duration, expirationDuration time.Duration) *Consequences { // this invocation makes sure this func is not reported as the cause of the failure - we are a "test helper" c.context.t.Helper() diff --git a/test/e2e/project_management_test.go b/test/e2e/project_management_test.go index 09702cc77933e..4a7bf5c035d0e 100644 --- a/test/e2e/project_management_test.go +++ b/test/e2e/project_management_test.go @@ -710,7 +710,7 @@ func TestAddProjectDestinationServiceAccount(t *testing.T) { "test-sa", ) require.Error(t, err) - assert.Contains(t, err.Error(), "server has an invalid format, '!*'") + assert.Contains(t, err.Error(), "server has an invalid format, '!abc'") // Given, an existing project, // When, a default destination service account with negation glob pattern for namespace is added, @@ -765,7 +765,7 @@ func TestAddProjectDestinationServiceAccount(t *testing.T) { "test\\sa", ) require.Error(t, err) - assert.Contains(t, err.Error(), "defaultServiceAccount has an invalid format, 'test\\sa'") + assert.Contains(t, err.Error(), "defaultServiceAccount has an invalid format, 'test\\\\sa'") // Given, an existing project, // When, a default destination service account with service account having forward slash char is added, From e20d757d3b157e767a22122ad35525b6d3fa9aa8 Mon Sep 17 00:00:00 2001 From: anandf Date: Thu, 26 Sep 2024 18:48:36 +0530 Subject: [PATCH 15/20] Fixed goimports linter issue Signed-off-by: anandf --- pkg/apis/application/v1alpha1/app_project_types.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/apis/application/v1alpha1/app_project_types.go b/pkg/apis/application/v1alpha1/app_project_types.go index 12d6f10b22bf5..2ef133e6cc81b 100644 --- a/pkg/apis/application/v1alpha1/app_project_types.go +++ b/pkg/apis/application/v1alpha1/app_project_types.go @@ -6,8 +6,6 @@ import ( "strconv" "strings" - "github.com/argoproj/argo-cd/v2/util/git" - "github.com/argoproj/argo-cd/v2/util/glob" globutil "github.com/gobwas/glob" "github.com/google/go-cmp/cmp" "google.golang.org/grpc/codes" @@ -15,6 +13,9 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" + + "github.com/argoproj/argo-cd/v2/util/git" + "github.com/argoproj/argo-cd/v2/util/glob" ) const ( From a2f2b75f7b5f19f5874509079a7346ec1205888a Mon Sep 17 00:00:00 2001 From: anandf Date: Fri, 27 Sep 2024 18:04:50 +0530 Subject: [PATCH 16/20] Added code comments and added few missing unit tests Signed-off-by: anandf --- controller/sync.go | 2 + .../application/v1alpha1/app_project_types.go | 2 + pkg/apis/application/v1alpha1/types_test.go | 6 +-- util/glob/glob.go | 3 ++ util/glob/glob_test.go | 35 +++++++++++++-- util/settings/settings.go | 4 +- util/settings/settings_test.go | 43 +++++++++++++++++++ 7 files changed, 88 insertions(+), 7 deletions(-) diff --git a/controller/sync.go b/controller/sync.go index ed5d985cd5b53..83a03d8a253d9 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -45,6 +45,8 @@ const ( // each sync-wave EnvVarSyncWaveDelay = "ARGOCD_SYNC_WAVE_DELAY" + // serviceAccountDisallowedCharSet contains the characters that are not allowed to be present + // in a DefaultServiceAccount configured for a DestinationServiceAccount serviceAccountDisallowedCharSet = "!*[]{}\\/" ) diff --git a/pkg/apis/application/v1alpha1/app_project_types.go b/pkg/apis/application/v1alpha1/app_project_types.go index 2ef133e6cc81b..903d8aab29ddf 100644 --- a/pkg/apis/application/v1alpha1/app_project_types.go +++ b/pkg/apis/application/v1alpha1/app_project_types.go @@ -19,6 +19,8 @@ import ( ) const ( + // serviceAccountDisallowedCharSet contains the characters that are not allowed to be present + // in a DefaultServiceAccount configured for a DestinationServiceAccount serviceAccountDisallowedCharSet = "!*[]{}\\/" ) diff --git a/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go index 4461a3418aa49..e3ab6e3ef650c 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/pkg/apis/application/v1alpha1/types_test.go @@ -3979,11 +3979,11 @@ func TestAppProject_ValidateDestinationServiceAccount(t *testing.T) { { // Given, a project // When, a default destination service account with negation glob pattern for server is added, - // Then, there is no error. - server: "!abc*", + // Then, there is an error with appropriate message. + server: "!abc", namespace: "test-ns", defaultServiceAccount: "test-sa", - expectedErrMsg: "server has an invalid format, '!abc*'", + expectedErrMsg: "server has an invalid format, '!abc'", }, { // Given, a project diff --git a/util/glob/glob.go b/util/glob/glob.go index b104d9a184318..c96681b2aed93 100644 --- a/util/glob/glob.go +++ b/util/glob/glob.go @@ -5,6 +5,7 @@ import ( log "github.com/sirupsen/logrus" ) +// Match tries to match a text with a given glob pattern. func Match(pattern, text string, separators ...rune) bool { compiledGlob, err := glob.Compile(pattern, separators...) if err != nil { @@ -14,6 +15,8 @@ func Match(pattern, text string, separators ...rune) bool { return compiledGlob.Match(text) } +// MatchWithError tries to match a text with a given glob pattern. +// returns error if the glob pattern fails to compile. func MatchWithError(pattern, text string, separators ...rune) (bool, error) { compiledGlob, err := glob.Compile(pattern, separators...) if err != nil { diff --git a/util/glob/glob_test.go b/util/glob/glob_test.go index a0a3995382c92..201fac2acfaff 100644 --- a/util/glob/glob_test.go +++ b/util/glob/glob_test.go @@ -3,7 +3,7 @@ package glob import ( "testing" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func Test_Match(t *testing.T) { @@ -24,7 +24,7 @@ func Test_Match(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { res := Match(tt.pattern, tt.input) - assert.Equal(t, tt.result, res) + require.Equal(t, tt.result, res) }) } } @@ -53,7 +53,36 @@ func Test_MatchList(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { res := MatchStringInList(tt.list, tt.input, tt.patternMatch) - assert.Equal(t, tt.result, res) + require.Equal(t, tt.result, res) + }) + } +} + +func Test_MatchWithError(t *testing.T) { + tests := []struct { + name string + input string + pattern string + result bool + expectedErr string + }{ + {"Exact match", "hello", "hello", true, ""}, + {"Non-match exact", "hello", "hell", false, ""}, + {"Long glob match", "hello", "hell*", true, ""}, + {"Short glob match", "hello", "h*", true, ""}, + {"Glob non-match", "hello", "e*", false, ""}, + {"Invalid pattern", "e[[a*", "e[[a*", false, "unexpected end of input"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + res, err := MatchWithError(tt.pattern, tt.input) + require.Equal(t, tt.result, res) + if tt.expectedErr == "" { + require.NoError(t, err) + } else { + require.ErrorContains(t, err, tt.expectedErr) + } }) } } diff --git a/util/settings/settings.go b/util/settings/settings.go index fb4a06906f99d..81c0c2d658da4 100644 --- a/util/settings/settings.go +++ b/util/settings/settings.go @@ -534,7 +534,9 @@ const ( const ( // default max webhook payload size is 1GB - defaultMaxWebhookPayloadSize = int64(1) * 1024 * 1024 * 1024 + defaultMaxWebhookPayloadSize = int64(1) * 1024 * 1024 * 1024 + + // application sync with impersonation feature is disabled by default. defaultImpersonationEnabledFlag = false ) diff --git a/util/settings/settings_test.go b/util/settings/settings_test.go index 94d5eeccf8021..0a79f3f6e1a12 100644 --- a/util/settings/settings_test.go +++ b/util/settings/settings_test.go @@ -1725,3 +1725,46 @@ func TestRedirectAdditionalURLs(t *testing.T) { }) } } + +func TestIsImpersonationEnabled(t *testing.T) { + // When there is no argocd-cm itself, + // Then IsImpersonationEnabled() must return false (default value) and an error with appropriate error message. + kubeClient := fake.NewSimpleClientset() + settingsManager := NewSettingsManager(context.Background(), kubeClient, "default") + featureFlag, err := settingsManager.IsImpersonationEnabled() + require.False(t, featureFlag, + "with no argocd-cm config map, IsImpersonationEnabled() must return return false (default value)") + require.ErrorContains(t, err, "configmap \"argocd-cm\" not found", + "with no argocd-cm config map, IsImpersonationEnabled() must return an error") + + // When there is no impersonation feature flag present in the argocd-cm, + // Then IsImpersonationEnabled() must return false (default value) and nil error. + _, settingsManager = fixtures(map[string]string{}) + featureFlag, err = settingsManager.IsImpersonationEnabled() + require.False(t, featureFlag, + "with empty argocd-cm config map, IsImpersonationEnabled() must return false (default value)") + require.NoError(t, err, + "with empty argocd-cm config map, IsImpersonationEnabled() must not return any error") + + // When user disables the feature explicitly, + // Then IsImpersonationEnabled() must return false and nil error. + _, settingsManager = fixtures(map[string]string{ + "application.sync.impersonation.enabled": "false", + }) + featureFlag, err = settingsManager.IsImpersonationEnabled() + require.False(t, featureFlag, + "when user enables the flag in argocd-cm config map, IsImpersonationEnabled() must return user set value") + require.NoError(t, err, + "when user enables the flag in argocd-cm config map, IsImpersonationEnabled() must not return any error") + + // When user enables the feature explicitly, + // Then IsImpersonationEnabled() must return true and nil error. + _, settingsManager = fixtures(map[string]string{ + "application.sync.impersonation.enabled": "true", + }) + featureFlag, err = settingsManager.IsImpersonationEnabled() + require.True(t, featureFlag, + "when user enables the flag in argocd-cm config map, IsImpersonationEnabled() must return user set value") + require.NoError(t, err, + "when user enables the flag in argocd-cm config map, IsImpersonationEnabled() must not return any error") +} From 80714d0a516166344949d2c0f613dbe584c26cbf Mon Sep 17 00:00:00 2001 From: anandf Date: Fri, 27 Sep 2024 18:59:07 +0530 Subject: [PATCH 17/20] Added missing unit test for GetDestinationServiceAccounts method Signed-off-by: anandf --- cmd/util/project.go | 2 +- cmd/util/project_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/cmd/util/project.go b/cmd/util/project.go index 3626f414aafd9..63dff2018c975 100644 --- a/cmd/util/project.go +++ b/cmd/util/project.go @@ -48,7 +48,7 @@ func AddProjFlags(command *cobra.Command, opts *ProjectOpts) { command.Flags().StringArrayVar(&opts.allowedNamespacedResources, "allow-namespaced-resource", []string{}, "List of allowed namespaced resources") command.Flags().StringArrayVar(&opts.deniedNamespacedResources, "deny-namespaced-resource", []string{}, "List of denied namespaced resources") command.Flags().StringSliceVar(&opts.SourceNamespaces, "source-namespaces", []string{}, "List of source namespaces for applications") - command.Flags().StringArrayVarP(&opts.destinationServiceAccounts, "dest-service-accounts", "", []string{}, + command.Flags().StringArrayVar(&opts.destinationServiceAccounts, "dest-service-accounts", []string{}, "Destination server, namespace and target service account (e.g. https://192.168.99.100:8443,default,default-sa)") } diff --git a/cmd/util/project_test.go b/cmd/util/project_test.go index bde59d0ab5e88..fc5dc58b60399 100644 --- a/cmd/util/project_test.go +++ b/cmd/util/project_test.go @@ -3,6 +3,7 @@ package util import ( "testing" + "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/stretchr/testify/assert" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -22,3 +23,27 @@ func TestProjectOpts_ResourceLists(t *testing.T) { []v1.GroupKind{{Group: "rbac.authorization.k8s.io", Kind: "ClusterRole"}}, opts.GetDeniedClusterResources(), ) } + +func TestProjectOpts_GetDestinationServiceAccounts(t *testing.T) { + opts := ProjectOpts{ + destinationServiceAccounts: []string{ + "https://192.168.99.100:8443,test-ns,test-sa", + "https://kubernetes.default.svc.local:6443,guestbook,guestbook-sa", + }, + } + + assert.ElementsMatch(t, + []v1alpha1.ApplicationDestinationServiceAccount{ + { + Server: "https://192.168.99.100:8443", + Namespace: "test-ns", + DefaultServiceAccount: "test-sa", + }, + { + Server: "https://kubernetes.default.svc.local:6443", + Namespace: "guestbook", + DefaultServiceAccount: "guestbook-sa", + }, + }, opts.GetDestinationServiceAccounts(), + ) +} From 3a11ec98c29baf0b5243b546b8670c85e710c3b3 Mon Sep 17 00:00:00 2001 From: anandf Date: Fri, 27 Sep 2024 20:22:58 +0530 Subject: [PATCH 18/20] Fixed goimports formatting with local for project_test.go Signed-off-by: anandf --- cmd/util/project_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cmd/util/project_test.go b/cmd/util/project_test.go index fc5dc58b60399..8c61ee714f2c0 100644 --- a/cmd/util/project_test.go +++ b/cmd/util/project_test.go @@ -3,9 +3,10 @@ package util import ( "testing" - "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/stretchr/testify/assert" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" ) func TestProjectOpts_ResourceLists(t *testing.T) { From ecfd5487f02eeae471915b1da16a281fe1d058f3 Mon Sep 17 00:00:00 2001 From: anandf Date: Tue, 1 Oct 2024 17:17:30 +0530 Subject: [PATCH 19/20] Corrected typo in a field name additionalObjs Signed-off-by: anandf --- controller/appcontroller_test.go | 2 +- controller/sync_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index cd4c84b788ecf..1bb087788cd07 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -67,7 +67,7 @@ type fakeData struct { metricsCacheExpiration time.Duration applicationNamespaces []string updateRevisionForPathsResponse *apiclient.UpdateRevisionForPathsResponse - additionalsObjs []runtime.Object + additionalObjs []runtime.Object } type MockKubectl struct { diff --git a/controller/sync_test.go b/controller/sync_test.go index 0751434eab78c..a553fd3e37cf7 100644 --- a/controller/sync_test.go +++ b/controller/sync_test.go @@ -1316,7 +1316,7 @@ func TestSyncWithImpersonate(t *testing.T) { configMapData: map[string]string{ "application.sync.impersonation.enabled": strconv.FormatBool(impersonationEnabled), }, - additionalsObjs: additionalObjs, + additionalObjs: additionalObjs, } ctrl := newFakeController(&data, nil) return &fixture{ From 6d3f2d7964de2fc0b8d272e0781026fb4b0e2948 Mon Sep 17 00:00:00 2001 From: anandf Date: Tue, 1 Oct 2024 23:57:26 +0530 Subject: [PATCH 20/20] Fixed failing unit tests Signed-off-by: anandf --- controller/appcontroller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index 1bb087788cd07..3556e7056c239 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -138,7 +138,7 @@ func newFakeController(data *fakeData, repoErr error) *ApplicationController { Data: data.configMapData, } runtimeObjs := []runtime.Object{&clust, &secret, &cm} - runtimeObjs = append(runtimeObjs, data.additionalsObjs...) + runtimeObjs = append(runtimeObjs, data.additionalObjs...) kubeClient := fake.NewSimpleClientset(runtimeObjs...) settingsMgr := settings.NewSettingsManager(context.Background(), kubeClient, test.FakeArgoCDNamespace) kubectl := &MockKubectl{Kubectl: &kubetest.MockKubectlCmd{}}