chore: Added unit tests and fix e2e tests for application sync decoup…

…ling feature (argoproj#19966) * fixed doc comments and added unit tests Signed-off-by: anandf <[email protected]> * Added comments for the newly added unit tests Signed-off-by: anandf <[email protected]> * Refactored method name to deriveServiceAccountToImpersonate Signed-off-by: anandf <[email protected]> * Using const name in return value Signed-off-by: anandf <[email protected]> * Added unit tests for argocd proj add-destination-service-accounts Signed-off-by: anandf <[email protected]> * Fixed failing e2e tests Signed-off-by: anandf <[email protected]> * Fix linting errors Signed-off-by: anandf <[email protected]> * Using require package instead of assert and fixed code generation Signed-off-by: anandf <[email protected]> * Removed parallel execution of tests for sync with impersonate Signed-off-by: anandf <[email protected]> * Added err checks for glob validations Signed-off-by: anandf <[email protected]> * Fixed e2e tests for sync impersonation Signed-off-by: anandf <[email protected]> * Using consistently based expects in E2E tests Signed-off-by: anandf <[email protected]> * Added more unit tests and fixed go generate Signed-off-by: anandf <[email protected]> * Fixed failed lint errors, unit and e2e test failures Signed-off-by: anandf <[email protected]> * Fixed goimports linter issue Signed-off-by: anandf <[email protected]> * Added code comments and added few missing unit tests Signed-off-by: anandf <[email protected]> * Added missing unit test for GetDestinationServiceAccounts method Signed-off-by: anandf <[email protected]> * Fixed goimports formatting with local for project_test.go Signed-off-by: anandf <[email protected]> * Corrected typo in a field name additionalObjs Signed-off-by: anandf <[email protected]> * Fixed failing unit tests Signed-off-by: anandf <[email protected]> --------- Signed-off-by: anandf <[email protected]> Signed-off-by: Moleus <[email protected]>
moleus · Oct 7, 2024 · a02246a · a02246a
1 parent 9e88790
commit a02246a
Show file tree

Hide file tree

Showing 27 changed files with 1,422 additions and 86 deletions.
diff --git a/assets/swagger.json b/assets/swagger.json
diff --git 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
@@ -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
@@ -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
@@ -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:<namespace>:<service_account>
-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 {