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/cmd/util/project.go b/cmd/util/project.go index b0a82883dc0bb..63dff2018c975 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().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)") } 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/cmd/util/project_test.go b/cmd/util/project_test.go index bde59d0ab5e88..8c61ee714f2c0 100644 --- a/cmd/util/project_test.go +++ b/cmd/util/project_test.go @@ -5,6 +5,8 @@ import ( "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) { @@ -22,3 +24,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(), + ) +} diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index 50fb08042719d..3556e7056c239 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 + additionalObjs []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.additionalObjs...) + 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..83a03d8a253d9 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -44,6 +44,10 @@ const ( // EnvVarSyncWaveDelay is an environment variable which controls the delay in seconds between // 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 = "!*[]{}\\/" ) func (m *appStateManager) getOpenAPISchema(server string) (openapi.Resources, error) { @@ -287,8 +291,13 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha } trackingMethod := argo.GetTrackingMethod(m.settingsMgr) - if m.settingsMgr.IsImpersonationEnabled() { - serviceAccountToImpersonate, err := deriveServiceAccountName(proj, app) + impersonationEnabled, err := m.settingsMgr.IsImpersonationEnabled() + if err != nil { + log.Errorf("could not get impersonation feature flag: %v", err) + return + } + if impersonationEnabled { + 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) @@ -557,9 +566,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 @@ -569,10 +578,18 @@ func deriveServiceAccountName(project *v1alpha1.AppProject, application *v1alpha // 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 "", 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: %w", err) + } if dstServerMatched && dstNamespaceMatched { - if strings.Contains(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 } else { diff --git a/controller/sync_test.go b/controller/sync_test.go index 1dbfa2ff9e1a5..a553fd3e37cf7 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,771 @@ 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 an application referring a project with no destination service accounts + 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 := deriveServiceAccountToImpersonate(f.project, f.application) + assert.Equal(t, expectedSA, sa) + + // 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 an application referring a project with exactly one destination service account that matches the application destination, + 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 := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there should be no error and should use the right service account for impersonation + require.NoError(t, err) + assert.Equal(t, expectedSA, sa) + }) + + t.Run("exact one match with multiple destination service accounts", func(t *testing.T) { + // given an application referring a project with multiple destination service accounts having one exact match for application destination + 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 := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there should be no error and should use the right service account for impersonation + require.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 an application referring a project with multiple destination service accounts having multiple match for application destination + 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 := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there should be no error and it should use the first matching service account for impersonation + require.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 an application referring a project with multiple destination service accounts with glob patterns matching the application destination + 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 := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there should not be any error and should use the first matching glob pattern service account for impersonation + require.NoError(t, err) + assert.Equal(t, expectedSA, sa) + }) + + t.Run("no match among a valid list", func(t *testing.T) { + // given an application referring a project with multiple destination service accounts with no matches for application destination + 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 := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there should be an error saying no match was found + require.EqualError(t, err, expectedErrMsg) + assert.Equal(t, expectedSA, sa) + }) + + t.Run("app destination namespace is empty", func(t *testing.T) { + // given an application referring a project with multiple destination service accounts with empty application destination namespace + 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 := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there should not be any error and the service account configured for with empty namespace should be used. + require.NoError(t, err) + assert.Equal(t, expectedSA, sa) + }) + + t.Run("match done via catch all glob pattern", func(t *testing.T) { + // given an application referring a project with multiple destination service accounts having a catch all glob pattern + 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 := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there should not be any error and the catch all service account should be returned + require.NoError(t, err) + 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() + 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 := 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. + require.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 an application referring a project with multiple destination service accounts and one exact match for application destination + 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 := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there should not be any error and the right service account must be returned. + require.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 an application referring a project with multiple destination service accounts and multiple matches for application destination + 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 := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there should not be any error and first matching service account should be used + require.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 an application referring a project with multiple destination service accounts with a matching glob pattern and exact match + 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 := 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. + require.NoError(t, err) + }) + + t.Run("no match among a valid list", func(t *testing.T) { + // given an application referring a project with multiple destination service accounts with no match + 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 := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there an error with appropriate message must be returned + require.EqualError(t, err, expectedErr) + assert.Equal(t, expectedSA, sa) + }) + + t.Run("match done via catch all glob pattern", func(t *testing.T) { + // given an application referring a project with multiple destination service accounts with matching catch all glob pattern + 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 := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there should not be any error and the service account of the glob pattern match must be returned. + require.NoError(t, err) + 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() + 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 := deriveServiceAccountToImpersonate(f.project, f.application) + + // then, there should not be any error and the service account with the given namespace prefix must be returned. + require.NoError(t, err) + assert.Equal(t, expectedSA, sa) + }) +} + +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), + }, + additionalObjs: 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 with an application referring a project no matching service account + 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 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 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 contains invalid chars ''" + + 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 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 app sync impersonation feature is enabled with an application referring a project matching service account + 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 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 app sync impersonation feature is disabled with an application referring a project matching service account + 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 application sync should pass using the control plane service account + 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/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/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/app_project_types.go b/pkg/apis/application/v1alpha1/app_project_types.go index b888cd37391b3..436e578548e3d 100644 --- a/pkg/apis/application/v1alpha1/app_project_types.go +++ b/pkg/apis/application/v1alpha1/app_project_types.go @@ -6,15 +6,22 @@ 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" "google.golang.org/grpc/status" 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 ( + // serviceAccountDisallowedCharSet contains the characters that are not allowed to be present + // in a DefaultServiceAccount configured for a DestinationServiceAccount + serviceAccountDisallowedCharSet = "!*[]{}\\/" ) type ErrApplicationNotAllowedToUseProject struct { @@ -267,12 +274,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.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 status.Errorf(codes.InvalidArgument, "server has an invalid format, '%s'", destServiceAcct.Server) + } + + _, err = globutil.Compile(destServiceAcct.Namespace) + if err != nil { + return status.Errorf(codes.InvalidArgument, "namespace has an invalid format, '%s'", destServiceAcct.Namespace) } key := fmt.Sprintf("%s/%s", destServiceAcct.Server, destServiceAcct.Namespace) @@ -461,7 +483,6 @@ func (proj AppProject) IsDestinationPermitted(dst ApplicationDestination, projec func (proj AppProject) isDestinationMatched(dst ApplicationDestination) bool { anyDestinationMatched := false - noDenyDestinationsMatched := true for _, item := range proj.Spec.Destinations { dstNameMatched := dst.Name != "" && globMatch(item.Name, dst.Name, true) @@ -471,12 +492,14 @@ func (proj AppProject) isDestinationMatched(dst ApplicationDestination) bool { matched := (dstServerMatched || dstNameMatched) && dstNamespaceMatched if matched { anyDestinationMatched = true - } else if ((!dstNameMatched && isDenyPattern(item.Name)) || (!dstServerMatched && isDenyPattern(item.Server))) || (!dstNamespaceMatched && isDenyPattern(item.Namespace)) { - noDenyDestinationsMatched = false + } else if (!dstNameMatched && isDenyPattern(item.Name)) || (!dstServerMatched && isDenyPattern(item.Server)) && dstNamespaceMatched { + return false + } else if !dstNamespaceMatched && isDenyPattern(item.Namespace) && dstServerMatched { + return false } } - return anyDestinationMatched && noDenyDestinationsMatched + return anyDestinationMatched } func isDenyPattern(pattern string) bool { 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; } 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/pkg/apis/application/v1alpha1/types_test.go b/pkg/apis/application/v1alpha1/types_test.go index 08b83c238a93d..ae1bf4bd05add 100644 --- a/pkg/apis/application/v1alpha1/types_test.go +++ b/pkg/apis/application/v1alpha1/types_test.go @@ -320,7 +320,7 @@ func TestAppProject_IsNegatedDestinationPermitted(t *testing.T) { Server: "*", Namespace: "!kube-system", }}, appDest: ApplicationDestination{Server: "https://kubernetes.default.svc", Namespace: "default"}, - isPermitted: false, + isPermitted: true, }, { projDest: []ApplicationDestination{{ Server: "*", Namespace: "*", @@ -339,6 +339,22 @@ func TestAppProject_IsNegatedDestinationPermitted(t *testing.T) { }}, appDest: ApplicationDestination{Name: "test", Namespace: "test"}, isPermitted: false, + }, { + projDest: []ApplicationDestination{{ + Server: "*", Namespace: "test", + }, { + Server: "!https://test-server", Namespace: "other", + }}, + appDest: ApplicationDestination{Server: "https://test-server", Namespace: "test"}, + isPermitted: true, + }, { + projDest: []ApplicationDestination{{ + Server: "*", Namespace: "*", + }, { + Server: "https://test-server", Namespace: "!other", + }}, + appDest: ApplicationDestination{Server: "https://other-test-server", Namespace: "other"}, + isPermitted: true, }} for _, data := range testData { @@ -350,7 +366,7 @@ func TestAppProject_IsNegatedDestinationPermitted(t *testing.T) { permitted, _ := proj.IsDestinationPermitted(data.appDest, func(project string) ([]*Cluster, error) { return []*Cluster{}, nil }) - assert.Equal(t, data.isPermitted, permitted) + assert.Equalf(t, data.isPermitted, permitted, "appDest mismatch for %+v with project destinations %+v", data.appDest, data.projDest) } } @@ -3959,3 +3975,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 an error with appropriate message. + 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/fixture/app/consequences.go b/test/e2e/fixture/app/consequences.go index 9ee99fec6ca6d..ff64dee0de4b8 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 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() + + 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/project_management_test.go b/test/e2e/project_management_test.go index f00c4e3eeba62..4a7bf5c035d0e 100644 --- a/test/e2e/project_management_test.go +++ b/test/e2e/project_management_test.go @@ -619,3 +619,225 @@ 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 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. + _, 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 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, '!abc'") + + // 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, '!*'") + + // 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) + 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) + 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) +} diff --git a/test/e2e/sync_with_impersonate_test.go b/test/e2e/sync_with_impersonate_test.go index 7bb57af1156d3..4c7cf166f4a09 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,32 +17,27 @@ import ( . "github.com/argoproj/argo-cd/v2/test/e2e/fixture/app" ) -func TestSyncWithImpersonateDisable(t *testing.T) { - Given(t). - Path("guestbook"). - When(). - SetParamInSettingConfigMap("application.sync.impersonation.enabled", "false"). - CreateFromFile(func(app *v1alpha1.Application) { - app.Spec.SyncPolicy = &v1alpha1.SyncPolicy{Automated: &v1alpha1.SyncPolicyAutomated{}} - }). - Then(). - Expect(SyncStatusIs(v1alpha1.SyncStatusCodeSynced)) -} +const ( + WaitDuration = time.Second + TimeoutDuration = time.Second * 3 +) -func TestSyncWithImpersonateDefaultNamespaceServiceAccountNoRBAC(t *testing.T) { +func TestSyncWithFeatureDisabled(t *testing.T) { Given(t). Path("guestbook"). When(). - SetParamInSettingConfigMap("application.sync.impersonation.enabled", "true"). + SetParamInSettingConfigMap("application.sync.impersonation.enabled", "false"). CreateFromFile(func(app *v1alpha1.Application) { app.Spec.SyncPolicy = &v1alpha1.SyncPolicy{Automated: &v1alpha1.SyncPolicyAutomated{}} }). Then(). - Expect(SyncStatusIs(v1alpha1.SyncStatusCodeOutOfSync)) + // 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. + ExpectConsistently(SyncStatusIs(v1alpha1.SyncStatusCodeSynced), WaitDuration, TimeoutDuration). + Expect(OperationMessageContains("successfully synced")) } -func TestSyncWithImpersonateDefaultNamespaceServiceAccountWithRBAC(t *testing.T) { - roleName := "default-sa-role" +func TestSyncWithNoDestinationServiceAccountsInProject(t *testing.T) { Given(t). Path("guestbook"). When(). @@ -49,25 +45,11 @@ func TestSyncWithImpersonateDefaultNamespaceServiceAccountWithRBAC(t *testing.T) 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 + ExpectConsistently(SyncStatusIs(v1alpha1.SyncStatusCodeOutOfSync), WaitDuration, TimeoutDuration). + 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 + ExpectConsistently(SyncStatusIs(v1alpha1.SyncStatusCodeSynced), WaitDuration, TimeoutDuration). + 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. + ExpectConsistently(SyncStatusIs(v1alpha1.SyncStatusCodeOutOfSync), WaitDuration, TimeoutDuration). + 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,10 @@ 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. + ExpectConsistently(SyncStatusIs(v1alpha1.SyncStatusCodeUnknown), WaitDuration, TimeoutDuration) } // createTestAppProject creates a test AppProject resource. diff --git a/ui-test/package.json b/ui-test/package.json index 6e2d5b9a70863..6343568c71bad 100644 --- a/ui-test/package.json +++ b/ui-test/package.json @@ -15,7 +15,7 @@ "@types/selenium-webdriver": "^4.1.26", "assert": "^2.1.0", "chromedriver": "^129.0.2", - "selenium-webdriver": "^4.24.1" + "selenium-webdriver": "^4.25.0" }, "devDependencies": { "@types/mocha": "^10.0.8", diff --git a/ui-test/yarn.lock b/ui-test/yarn.lock index 607d441b531e7..3b00628624d15 100644 --- a/ui-test/yarn.lock +++ b/ui-test/yarn.lock @@ -1272,10 +1272,10 @@ safe-buffer@^5.1.0, safe-buffer@~5.1.0, safe-buffer@~5.1.1: resolved "https://registry.npmjs.org/safe-buffer/-/safe-buffer-5.1.2.tgz" integrity sha512-Gd2UZBJDkXlY7GbJxfsE8/nvKkUEU1G38c1siN6QP6a9PT9MmHB8GnpscSmMJSoF8LOIrt8ud/wPtojys4G6+g== -selenium-webdriver@^4.24.1: - version "4.24.1" - resolved "https://registry.yarnpkg.com/selenium-webdriver/-/selenium-webdriver-4.24.1.tgz#4315214420cc26dddaa21ae26863dd452aec2829" - integrity sha512-fcK5BTI/54cSqIhiVtrd9li1YL6LW109yIwuVw6V+FlVE6y4riGiX2qdZxVzHq+sm2TJyps+D2sjzXrpDZe1Og== +selenium-webdriver@^4.25.0: + version "4.25.0" + resolved "https://registry.yarnpkg.com/selenium-webdriver/-/selenium-webdriver-4.25.0.tgz#3562b49668817974bb1d13d25a50e8bc0264fcf3" + integrity sha512-zl9IX93caOT8wbcCpZzAkEtYa+hNgJ4C5GUN8uhpzggqRLvsg1asfKi0p1uNZC8buYVvsBZbx8S+9MjVAjs4oA== dependencies: "@bazel/runfiles" "^5.8.1" jszip "^3.10.1" diff --git a/util/glob/glob.go b/util/glob/glob.go index 0ef7cda28635a..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 { @@ -13,3 +14,13 @@ 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 { + return false, err + } + return compiledGlob.Match(text), 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 7d192d9cb9461..81c0c2d658da4 100644 --- a/util/settings/settings.go +++ b/util/settings/settings.go @@ -535,6 +535,9 @@ const ( const ( // default max webhook payload size is 1GB defaultMaxWebhookPayloadSize = int64(1) * 1024 * 1024 * 1024 + + // application sync with impersonation feature is disabled by default. + defaultImpersonationEnabledFlag = false ) var sourceTypeToEnableGenerationKey = map[v1alpha1.ApplicationSourceType]string{ @@ -2336,11 +2339,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 defaultImpersonationEnabledFlag, fmt.Errorf("error checking %s property in configmap: %w", impersonationEnabledKey, err) } - return cm.Data[impersonationEnabledKey] == "true" + return cm.Data[impersonationEnabledKey] == "true", nil } 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") +}