Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions controller/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
return
}
if impersonationEnabled {
serviceAccountToImpersonate, err := deriveServiceAccountToImpersonate(proj, app)
serviceAccountToImpersonate, err := deriveServiceAccountToImpersonate(proj, app, destCluster)
if err != nil {
state.Phase = common.OperationError
state.Message = fmt.Sprintf("failed to find a matching service account to impersonate: %v", err)
Expand Down Expand Up @@ -607,7 +607,7 @@ func syncWindowPreventsSync(app *v1alpha1.Application, proj *v1alpha1.AppProject

// 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 deriveServiceAccountToImpersonate(project *v1alpha1.AppProject, application *v1alpha1.Application) (string, error) {
func deriveServiceAccountToImpersonate(project *v1alpha1.AppProject, application *v1alpha1.Application, destCluster *v1alpha1.Cluster) (string, error) {
// spec.Destination.Namespace is optional. If not specified, use the Application's
// namespace
serviceAccountNamespace := application.Spec.Destination.Namespace
Expand All @@ -617,7 +617,7 @@ func deriveServiceAccountToImpersonate(project *v1alpha1.AppProject, application
// Loop through the destinationServiceAccounts and see if there is any destination that is a candidate.
// if so, return the service account specified for that destination.
for _, item := range project.Spec.DestinationServiceAccounts {
dstServerMatched, err := glob.MatchWithError(item.Server, application.Spec.Destination.Server)
dstServerMatched, err := glob.MatchWithError(item.Server, destCluster.Server)
if err != nil {
return "", fmt.Errorf("invalid glob pattern for destination server: %w", err)
}
Expand Down
127 changes: 110 additions & 17 deletions controller/sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {
type fixture struct {
project *v1alpha1.AppProject
application *v1alpha1.Application
cluster *v1alpha1.Cluster
}

setup := func(destinationServiceAccounts []v1alpha1.ApplicationDestinationServiceAccount, destinationNamespace, destinationServerURL, applicationNamespace string) *fixture {
Expand All @@ -676,9 +677,14 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {
},
},
}
cluster := &v1alpha1.Cluster{
Server: "https://kubernetes.svc.local",
Name: "test-cluster",
}
return &fixture{
project: project,
application: app,
cluster: cluster,
}
}

Expand All @@ -694,7 +700,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
assert.Equal(t, expectedSA, sa)

// then, there should be an error saying no valid match was found
Expand All @@ -718,7 +724,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)

// then, there should be no error and should use the right service account for impersonation
require.NoError(t, err)
Expand Down Expand Up @@ -757,7 +763,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)

// then, there should be no error and should use the right service account for impersonation
require.NoError(t, err)
Expand Down Expand Up @@ -796,7 +802,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)

// then, there should be no error and it should use the first matching service account for impersonation
require.NoError(t, err)
Expand Down Expand Up @@ -830,7 +836,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)

// then, there should not be any error and should use the first matching glob pattern service account for impersonation
require.NoError(t, err)
Expand Down Expand Up @@ -865,7 +871,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)

// then, there should be an error saying no match was found
require.EqualError(t, err, expectedErrMsg)
Expand Down Expand Up @@ -893,7 +899,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)

// then, there should not be any error and the service account configured for with empty namespace should be used.
require.NoError(t, err)
Expand Down Expand Up @@ -927,7 +933,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)

// then, there should not be any error and the catch all service account should be returned
require.NoError(t, err)
Expand All @@ -951,7 +957,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)

// then, there must be an error as the glob pattern is invalid.
require.ErrorContains(t, err, "invalid glob pattern for destination namespace")
Expand Down Expand Up @@ -985,7 +991,35 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
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)
})

t.Run("app destination name instead of server URL", func(t *testing.T) {
t.Parallel()
destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{
{
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)

// Use destination name instead of server URL
f.application.Spec.Destination.Server = ""
f.application.Spec.Destination.Name = f.cluster.Name

// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
assert.Equal(t, expectedSA, sa)

// then, there should not be any error and the service account with its namespace should be returned.
Expand All @@ -999,6 +1033,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) {
type fixture struct {
project *v1alpha1.AppProject
application *v1alpha1.Application
cluster *v1alpha1.Cluster
}

setup := func(destinationServiceAccounts []v1alpha1.ApplicationDestinationServiceAccount, destinationNamespace, destinationServerURL, applicationNamespace string) *fixture {
Expand All @@ -1024,9 +1059,14 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) {
},
},
}
cluster := &v1alpha1.Cluster{
Server: "https://kubernetes.svc.local",
Name: "test-cluster",
}
return &fixture{
project: project,
application: app,
cluster: cluster,
}
}

Expand Down Expand Up @@ -1062,7 +1102,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)

// then, there should not be any error and the right service account must be returned.
require.NoError(t, err)
Expand Down Expand Up @@ -1101,7 +1141,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)

// then, there should not be any error and first matching service account should be used
require.NoError(t, err)
Expand Down Expand Up @@ -1135,7 +1175,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
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.
Expand Down Expand Up @@ -1170,7 +1210,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, &v1alpha1.Cluster{Server: destinationServerURL})

// then, there an error with appropriate message must be returned
require.EqualError(t, err, expectedErr)
Expand Down Expand Up @@ -1204,7 +1244,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)

// then, there should not be any error and the service account of the glob pattern match must be returned.
require.NoError(t, err)
Expand All @@ -1228,7 +1268,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)

// then, there must be an error as the glob pattern is invalid.
require.ErrorContains(t, err, "invalid glob pattern for destination server")
Expand Down Expand Up @@ -1262,12 +1302,40 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) {

f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, &v1alpha1.Cluster{Server: destinationServerURL})

// 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)
})

t.Run("app destination name instead of server URL", func(t *testing.T) {
t.Parallel()
destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{
{
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)

// Use destination name instead of server URL
f.application.Spec.Destination.Server = ""
f.application.Spec.Destination.Name = f.cluster.Name

// when
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
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 TestSyncWithImpersonate(t *testing.T) {
Expand Down Expand Up @@ -1409,6 +1477,31 @@ func TestSyncWithImpersonate(t *testing.T) {
assert.Equal(t, common.OperationSucceeded, opState.Phase)
assert.Contains(t, opState.Message, opMessage)
})

t.Run("app destination name instead of server URL", 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,
}

f.application.Spec.Destination.Server = ""
f.application.Spec.Destination.Name = "minikube"

// 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)
})
}

func TestClientSideApplyMigration(t *testing.T) {
Expand Down
Loading