Skip to content

Commit d69639a

Browse files
gcp-cherry-pick-bot[bot]crenshaw-dev
gcp-cherry-pick-bot[bot]
and
crenshaw-dev
authored
fix(controller): impersonation with destination name (#23309) (cherry-pick #23504) (#23519)
Signed-off-by: Michael Crenshaw <[email protected]> Co-authored-by: Michael Crenshaw <[email protected]>
1 parent b75f532 commit d69639a

File tree

2 files changed

+113
-20
lines changed

2 files changed

+113
-20
lines changed

‎controller/sync.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,7 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
324324
return
325325
}
326326
if impersonationEnabled {
327-
serviceAccountToImpersonate, err := deriveServiceAccountToImpersonate(proj, app)
327+
serviceAccountToImpersonate, err := deriveServiceAccountToImpersonate(proj, app, destCluster)
328328
if err != nil {
329329
state.Phase = common.OperationError
330330
state.Message = fmt.Sprintf("failed to find a matching service account to impersonate: %v", err)
@@ -607,7 +607,7 @@ func syncWindowPreventsSync(app *v1alpha1.Application, proj *v1alpha1.AppProject
607607

608608
// deriveServiceAccountToImpersonate determines the service account to be used for impersonation for the sync operation.
609609
// The returned service account will be fully qualified including namespace and the service account name in the format system:serviceaccount:<namespace>:<service_account>
610-
func deriveServiceAccountToImpersonate(project *v1alpha1.AppProject, application *v1alpha1.Application) (string, error) {
610+
func deriveServiceAccountToImpersonate(project *v1alpha1.AppProject, application *v1alpha1.Application, destCluster *v1alpha1.Cluster) (string, error) {
611611
// spec.Destination.Namespace is optional. If not specified, use the Application's
612612
// namespace
613613
serviceAccountNamespace := application.Spec.Destination.Namespace
@@ -617,7 +617,7 @@ func deriveServiceAccountToImpersonate(project *v1alpha1.AppProject, application
617617
// Loop through the destinationServiceAccounts and see if there is any destination that is a candidate.
618618
// if so, return the service account specified for that destination.
619619
for _, item := range project.Spec.DestinationServiceAccounts {
620-
dstServerMatched, err := glob.MatchWithError(item.Server, application.Spec.Destination.Server)
620+
dstServerMatched, err := glob.MatchWithError(item.Server, destCluster.Server)
621621
if err != nil {
622622
return "", fmt.Errorf("invalid glob pattern for destination server: %w", err)
623623
}

‎controller/sync_test.go‎

Lines changed: 110 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -651,6 +651,7 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {
651651
type fixture struct {
652652
project *v1alpha1.AppProject
653653
application *v1alpha1.Application
654+
cluster *v1alpha1.Cluster
654655
}
655656

656657
setup := func(destinationServiceAccounts []v1alpha1.ApplicationDestinationServiceAccount, destinationNamespace, destinationServerURL, applicationNamespace string) *fixture {
@@ -676,9 +677,14 @@ func TestDeriveServiceAccountMatchingNamespaces(t *testing.T) {
676677
},
677678
},
678679
}
680+
cluster := &v1alpha1.Cluster{
681+
Server: "https://kubernetes.svc.local",
682+
Name: "test-cluster",
683+
}
679684
return &fixture{
680685
project: project,
681686
application: app,
687+
cluster: cluster,
682688
}
683689
}
684690

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

695701
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
696702
// when
697-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
703+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
698704
assert.Equal(t, expectedSA, sa)
699705

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

719725
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
720726
// when
721-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
727+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
722728

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

758764
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
759765
// when
760-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
766+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
761767

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

797803
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
798804
// when
799-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
805+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
800806

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

831837
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
832838
// when
833-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
839+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
834840

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

866872
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
867873
// when
868-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
874+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
869875

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

894900
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
895901
// when
896-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
902+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
897903

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

928934
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
929935
// when
930-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
936+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
931937

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

952958
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
953959
// when
954-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
960+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
955961

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

986992
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
987993
// when
988-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
994+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
995+
assert.Equal(t, expectedSA, sa)
996+
997+
// then, there should not be any error and the service account with its namespace should be returned.
998+
require.NoError(t, err)
999+
})
1000+
1001+
t.Run("app destination name instead of server URL", func(t *testing.T) {
1002+
t.Parallel()
1003+
destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{
1004+
{
1005+
Server: "https://kubernetes.svc.local",
1006+
Namespace: "*",
1007+
DefaultServiceAccount: "test-sa",
1008+
},
1009+
}
1010+
destinationNamespace := "testns"
1011+
destinationServerURL := "https://kubernetes.svc.local"
1012+
applicationNamespace := "argocd-ns"
1013+
expectedSA := "system:serviceaccount:testns:test-sa"
1014+
1015+
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
1016+
1017+
// Use destination name instead of server URL
1018+
f.application.Spec.Destination.Server = ""
1019+
f.application.Spec.Destination.Name = f.cluster.Name
1020+
1021+
// when
1022+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
9891023
assert.Equal(t, expectedSA, sa)
9901024

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

10041039
setup := func(destinationServiceAccounts []v1alpha1.ApplicationDestinationServiceAccount, destinationNamespace, destinationServerURL, applicationNamespace string) *fixture {
@@ -1024,9 +1059,14 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) {
10241059
},
10251060
},
10261061
}
1062+
cluster := &v1alpha1.Cluster{
1063+
Server: "https://kubernetes.svc.local",
1064+
Name: "test-cluster",
1065+
}
10271066
return &fixture{
10281067
project: project,
10291068
application: app,
1069+
cluster: cluster,
10301070
}
10311071
}
10321072

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

10631103
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
10641104
// when
1065-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
1105+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
10661106

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

11021142
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
11031143
// when
1104-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
1144+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
11051145

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

11361176
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
11371177
// when
1138-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
1178+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
11391179
assert.Equal(t, expectedSA, sa)
11401180

11411181
// then, there should not be any error and the service account of the glob pattern, being the first match should be returned.
@@ -1170,7 +1210,7 @@ func TestDeriveServiceAccountMatchingServers(t *testing.T) {
11701210

11711211
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
11721212
// when
1173-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
1213+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, &v1alpha1.Cluster{Server: destinationServerURL})
11741214

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

12051245
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
12061246
// when
1207-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
1247+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
12081248

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

12291269
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
12301270
// when
1231-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
1271+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
12321272

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

12631303
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
12641304
// when
1265-
sa, err := deriveServiceAccountToImpersonate(f.project, f.application)
1305+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, &v1alpha1.Cluster{Server: destinationServerURL})
12661306

12671307
// then, there should not be any error and the service account with the given namespace prefix must be returned.
12681308
require.NoError(t, err)
12691309
assert.Equal(t, expectedSA, sa)
12701310
})
1311+
1312+
t.Run("app destination name instead of server URL", func(t *testing.T) {
1313+
t.Parallel()
1314+
destinationServiceAccounts := []v1alpha1.ApplicationDestinationServiceAccount{
1315+
{
1316+
Server: "https://kubernetes.svc.local",
1317+
Namespace: "*",
1318+
DefaultServiceAccount: "test-sa",
1319+
},
1320+
}
1321+
destinationNamespace := "testns"
1322+
destinationServerURL := "https://kubernetes.svc.local"
1323+
applicationNamespace := "argocd-ns"
1324+
expectedSA := "system:serviceaccount:testns:test-sa"
1325+
1326+
f := setup(destinationServiceAccounts, destinationNamespace, destinationServerURL, applicationNamespace)
1327+
1328+
// Use destination name instead of server URL
1329+
f.application.Spec.Destination.Server = ""
1330+
f.application.Spec.Destination.Name = f.cluster.Name
1331+
1332+
// when
1333+
sa, err := deriveServiceAccountToImpersonate(f.project, f.application, f.cluster)
1334+
assert.Equal(t, expectedSA, sa)
1335+
1336+
// then, there should not be any error and the service account with its namespace should be returned.
1337+
require.NoError(t, err)
1338+
})
12711339
}
12721340

12731341
func TestSyncWithImpersonate(t *testing.T) {
@@ -1409,6 +1477,31 @@ func TestSyncWithImpersonate(t *testing.T) {
14091477
assert.Equal(t, common.OperationSucceeded, opState.Phase)
14101478
assert.Contains(t, opState.Message, opMessage)
14111479
})
1480+
1481+
t.Run("app destination name instead of server URL", func(t *testing.T) {
1482+
// given app sync impersonation feature is enabled with an application referring a project matching service account
1483+
f := setup(true, test.FakeDestNamespace, "test-sa")
1484+
opMessage := "successfully synced (no more tasks)"
1485+
1486+
opState := &v1alpha1.OperationState{
1487+
Operation: v1alpha1.Operation{
1488+
Sync: &v1alpha1.SyncOperation{
1489+
Source: &v1alpha1.ApplicationSource{},
1490+
},
1491+
},
1492+
Phase: common.OperationRunning,
1493+
}
1494+
1495+
f.application.Spec.Destination.Server = ""
1496+
f.application.Spec.Destination.Name = "minikube"
1497+
1498+
// when
1499+
f.controller.appStateManager.SyncAppState(f.application, opState)
1500+
1501+
// then app sync should not fail
1502+
assert.Equal(t, common.OperationSucceeded, opState.Phase)
1503+
assert.Contains(t, opState.Message, opMessage)
1504+
})
14121505
}
14131506

14141507
func TestClientSideApplyMigration(t *testing.T) {

0 commit comments

Comments
 (0)