Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Populate destination name when destination server is specified #21063

Merged
merged 3 commits into from
Dec 14, 2024
Merged

fix: Populate destination name when destination server is specified #21063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
463e1f9
Populate destination name when destination server is specified
adriananeci Feb 14, 2024
b6cb6af
Lint nit
adriananeci Dec 10, 2024
94b9247
Trigger CI
adriananeci Dec 10, 2024
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
118 changes: 54 additions & 64 deletions applicationset/controllers/applicationset_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1894,12 +1894,6 @@ func TestValidateGeneratedApplications(t *testing.T) {
err := v1alpha1.AddToScheme(scheme)
require.NoError(t, err)

// Valid cluster
myCluster := v1alpha1.Cluster{
Server: "https://kubernetes.default.svc",
Name: "my-cluster",
}

// Valid project
myProject := &v1alpha1.AppProject{
ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "namespace"},
Expand Down Expand Up @@ -2066,18 +2060,12 @@ func TestValidateGeneratedApplications(t *testing.T) {
objects := append([]runtime.Object{}, secret)
kubeclientset := kubefake.NewSimpleClientset(objects...)

argoDBMock := dbmocks.ArgoDB{}
argoDBMock.On("GetCluster", mock.Anything, "https://kubernetes.default.svc").Return(&myCluster, nil)
argoDBMock.On("ListClusters", mock.Anything).Return(&v1alpha1.ClusterList{Items: []v1alpha1.Cluster{
myCluster,
}}, nil)

r := ApplicationSetReconciler{
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Generators: map[string]generators.Generator{},
ArgoDB: &argoDBMock,
ArgoDB: &dbmocks.ArgoDB{},
ArgoCDNamespace: "namespace",
KubeClientset: kubeclientset,
Metrics: metrics,
Expand Down Expand Up @@ -2159,17 +2147,9 @@ func TestReconcilerValidationProjectErrorBehaviour(t *testing.T) {
}

kubeclientset := kubefake.NewSimpleClientset()
argoDBMock := dbmocks.ArgoDB{}

client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet, &project).WithStatusSubresource(&appSet).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).Build()
metrics := appsetmetrics.NewFakeAppsetMetrics(client)
goodCluster := v1alpha1.Cluster{Server: "https://good-cluster", Name: "good-cluster"}
badCluster := v1alpha1.Cluster{Server: "https://bad-cluster", Name: "bad-cluster"}
argoDBMock.On("GetCluster", mock.Anything, "https://good-cluster").Return(&goodCluster, nil)
argoDBMock.On("GetCluster", mock.Anything, "https://bad-cluster").Return(&badCluster, nil)
argoDBMock.On("ListClusters", mock.Anything).Return(&v1alpha1.ClusterList{Items: []v1alpha1.Cluster{
goodCluster,
}}, nil)

r := ApplicationSetReconciler{
Client: client,
Expand All @@ -2179,7 +2159,7 @@ func TestReconcilerValidationProjectErrorBehaviour(t *testing.T) {
Generators: map[string]generators.Generator{
"List": generators.NewListGenerator(),
},
ArgoDB: &argoDBMock,
ArgoDB: &dbmocks.ArgoDB{},
KubeClientset: kubeclientset,
Policy: v1alpha1.ApplicationsSyncPolicySync,
ArgoCDNamespace: "argocd",
Expand Down Expand Up @@ -2363,7 +2343,6 @@ func TestSetApplicationSetStatusCondition(t *testing.T) {
}

kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
argoDBMock := dbmocks.ArgoDB{}

for _, testCase := range testCases {
client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&testCase.appset).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).WithStatusSubresource(&testCase.appset).Build()
Expand All @@ -2377,7 +2356,7 @@ func TestSetApplicationSetStatusCondition(t *testing.T) {
Generators: map[string]generators.Generator{
"List": generators.NewListGenerator(),
},
ArgoDB: &argoDBMock,
ArgoDB: &dbmocks.ArgoDB{},
KubeClientset: kubeclientset,
Metrics: metrics,
}
Expand Down Expand Up @@ -2433,16 +2412,28 @@ func applicationsUpdateSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp
},
}

kubeclientset := kubefake.NewSimpleClientset()
argoDBMock := dbmocks.ArgoDB{}
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "my-cluster",
Namespace: "argocd",
Labels: map[string]string{
argocommon.LabelKeySecretType: argocommon.LabelValueSecretTypeCluster,
},
},
Data: map[string][]byte{
// Since this test requires the cluster to be an invalid destination, we
// always return a cluster named 'my-cluster2' (different from app 'my-cluster', above)
"name": []byte("good-cluster"),
"server": []byte("https://good-cluster"),
"config": []byte("{\"username\":\"foo\",\"password\":\"foo\"}"),
},
}

objects := append([]runtime.Object{}, secret)
kubeclientset := kubefake.NewSimpleClientset(objects...)

client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet, &defaultProject).WithStatusSubresource(&appSet).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).Build()
metrics := appsetmetrics.NewFakeAppsetMetrics(client)
goodCluster := v1alpha1.Cluster{Server: "https://good-cluster", Name: "good-cluster"}
argoDBMock.On("GetCluster", mock.Anything, "https://good-cluster").Return(&goodCluster, nil)
argoDBMock.On("ListClusters", mock.Anything).Return(&v1alpha1.ClusterList{Items: []v1alpha1.Cluster{
goodCluster,
}}, nil)

r := ApplicationSetReconciler{
Client: client,
Expand All @@ -2452,7 +2443,7 @@ func applicationsUpdateSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp
Generators: map[string]generators.Generator{
"List": generators.NewListGenerator(),
},
ArgoDB: &argoDBMock,
ArgoDB: &dbmocks.ArgoDB{},
ArgoCDNamespace: "argocd",
KubeClientset: kubeclientset,
Policy: v1alpha1.ApplicationsSyncPolicySync,
Expand Down Expand Up @@ -2595,16 +2586,28 @@ func applicationsDeleteSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp
},
}

kubeclientset := kubefake.NewSimpleClientset()
argoDBMock := dbmocks.ArgoDB{}
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: "my-cluster",
Namespace: "argocd",
Labels: map[string]string{
argocommon.LabelKeySecretType: argocommon.LabelValueSecretTypeCluster,
},
},
Data: map[string][]byte{
// Since this test requires the cluster to be an invalid destination, we
// always return a cluster named 'my-cluster2' (different from app 'my-cluster', above)
"name": []byte("good-cluster"),
"server": []byte("https://good-cluster"),
"config": []byte("{\"username\":\"foo\",\"password\":\"foo\"}"),
},
}

objects := append([]runtime.Object{}, secret)
kubeclientset := kubefake.NewSimpleClientset(objects...)

client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&appSet, &defaultProject).WithStatusSubresource(&appSet).WithIndex(&v1alpha1.Application{}, ".metadata.controller", appControllerIndexer).Build()
metrics := appsetmetrics.NewFakeAppsetMetrics(client)
goodCluster := v1alpha1.Cluster{Server: "https://good-cluster", Name: "good-cluster"}
argoDBMock.On("GetCluster", mock.Anything, "https://good-cluster").Return(&goodCluster, nil)
argoDBMock.On("ListClusters", mock.Anything).Return(&v1alpha1.ClusterList{Items: []v1alpha1.Cluster{
goodCluster,
}}, nil)

r := ApplicationSetReconciler{
Client: client,
Expand All @@ -2614,7 +2617,7 @@ func applicationsDeleteSyncPolicyTest(t *testing.T, applicationsSyncPolicy v1alp
Generators: map[string]generators.Generator{
"List": generators.NewListGenerator(),
},
ArgoDB: &argoDBMock,
ArgoDB: &dbmocks.ArgoDB{},
ArgoCDNamespace: "argocd",
KubeClientset: kubeclientset,
Policy: v1alpha1.ApplicationsSyncPolicySync,
Expand Down Expand Up @@ -2689,23 +2692,23 @@ func TestDeletePerformedWithSyncPolicyCreateDelete(t *testing.T) {

apps := applicationsDeleteSyncPolicyTest(t, applicationsSyncPolicy, 3, true)

assert.Empty(t, apps.Items)
assert.NotNil(t, apps.Items[0].DeletionTimestamp)
}

func TestDeletePerformedWithSyncPolicySync(t *testing.T) {
applicationsSyncPolicy := v1alpha1.ApplicationsSyncPolicySync

apps := applicationsDeleteSyncPolicyTest(t, applicationsSyncPolicy, 3, true)

assert.Empty(t, apps.Items)
assert.NotNil(t, apps.Items[0].DeletionTimestamp)
}

func TestDeletePerformedWithSyncPolicyCreateOnlyAndAllowPolicyOverrideFalse(t *testing.T) {
applicationsSyncPolicy := v1alpha1.ApplicationsSyncPolicyCreateOnly

apps := applicationsDeleteSyncPolicyTest(t, applicationsSyncPolicy, 3, false)

assert.Empty(t, apps.Items)
assert.NotNil(t, apps.Items[0].DeletionTimestamp)
}

func TestPolicies(t *testing.T) {
Expand All @@ -2717,14 +2720,8 @@ func TestPolicies(t *testing.T) {
ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "argocd"},
Spec: v1alpha1.AppProjectSpec{SourceRepos: []string{"*"}, Destinations: []v1alpha1.ApplicationDestination{{Namespace: "*", Server: "https://kubernetes.default.svc"}}},
}
myCluster := v1alpha1.Cluster{
Server: "https://kubernetes.default.svc",
Name: "my-cluster",
}

kubeclientset := kubefake.NewSimpleClientset()
argoDBMock := dbmocks.ArgoDB{}
argoDBMock.On("GetCluster", mock.Anything, "https://kubernetes.default.svc").Return(&myCluster, nil)

for _, c := range []struct {
name string
Expand Down Expand Up @@ -2807,7 +2804,7 @@ func TestPolicies(t *testing.T) {
Generators: map[string]generators.Generator{
"List": generators.NewListGenerator(),
},
ArgoDB: &argoDBMock,
ArgoDB: &dbmocks.ArgoDB{},
ArgoCDNamespace: "argocd",
KubeClientset: kubeclientset,
Policy: policy,
Expand Down Expand Up @@ -2881,7 +2878,6 @@ func TestSetApplicationSetApplicationStatus(t *testing.T) {
require.NoError(t, err)

kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
argoDBMock := dbmocks.ArgoDB{}

for _, cc := range []struct {
name string
Expand Down Expand Up @@ -2965,7 +2961,7 @@ func TestSetApplicationSetApplicationStatus(t *testing.T) {
Generators: map[string]generators.Generator{
"List": generators.NewListGenerator(),
},
ArgoDB: &argoDBMock,
ArgoDB: &dbmocks.ArgoDB{},
KubeClientset: kubeclientset,
Metrics: metrics,
}
Expand Down Expand Up @@ -3714,14 +3710,13 @@ func TestBuildAppDependencyList(t *testing.T) {
} {
t.Run(cc.name, func(t *testing.T) {
kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
argoDBMock := dbmocks.ArgoDB{}

r := ApplicationSetReconciler{
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Generators: map[string]generators.Generator{},
ArgoDB: &argoDBMock,
ArgoDB: &dbmocks.ArgoDB{},
KubeClientset: kubeclientset,
Metrics: metrics,
}
Expand Down Expand Up @@ -4381,14 +4376,13 @@ func TestBuildAppSyncMap(t *testing.T) {
} {
t.Run(cc.name, func(t *testing.T) {
kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
argoDBMock := dbmocks.ArgoDB{}

r := ApplicationSetReconciler{
Client: client,
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Generators: map[string]generators.Generator{},
ArgoDB: &argoDBMock,
ArgoDB: &dbmocks.ArgoDB{},
KubeClientset: kubeclientset,
Metrics: metrics,
}
Expand Down Expand Up @@ -5326,7 +5320,6 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
} {
t.Run(cc.name, func(t *testing.T) {
kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
argoDBMock := dbmocks.ArgoDB{}

client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&cc.appSet).WithStatusSubresource(&cc.appSet).Build()
metrics := appsetmetrics.NewFakeAppsetMetrics(client)
Expand All @@ -5336,7 +5329,7 @@ func TestUpdateApplicationSetApplicationStatus(t *testing.T) {
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Generators: map[string]generators.Generator{},
ArgoDB: &argoDBMock,
ArgoDB: &dbmocks.ArgoDB{},
KubeClientset: kubeclientset,
Metrics: metrics,
}
Expand Down Expand Up @@ -6075,7 +6068,6 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) {
} {
t.Run(cc.name, func(t *testing.T) {
kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
argoDBMock := dbmocks.ArgoDB{}

client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(&cc.appSet).WithStatusSubresource(&cc.appSet).Build()
metrics := appsetmetrics.NewFakeAppsetMetrics(client)
Expand All @@ -6085,7 +6077,7 @@ func TestUpdateApplicationSetApplicationStatusProgress(t *testing.T) {
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Generators: map[string]generators.Generator{},
ArgoDB: &argoDBMock,
ArgoDB: &dbmocks.ArgoDB{},
KubeClientset: kubeclientset,
Metrics: metrics,
}
Expand Down Expand Up @@ -6286,7 +6278,6 @@ func TestUpdateResourceStatus(t *testing.T) {
} {
t.Run(cc.name, func(t *testing.T) {
kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
argoDBMock := dbmocks.ArgoDB{}

client := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(&cc.appSet).WithObjects(&cc.appSet).Build()
metrics := appsetmetrics.NewFakeAppsetMetrics(client)
Expand All @@ -6296,7 +6287,7 @@ func TestUpdateResourceStatus(t *testing.T) {
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Generators: map[string]generators.Generator{},
ArgoDB: &argoDBMock,
ArgoDB: &dbmocks.ArgoDB{},
KubeClientset: kubeclientset,
Metrics: metrics,
}
Expand Down Expand Up @@ -6376,7 +6367,6 @@ func TestResourceStatusAreOrdered(t *testing.T) {
} {
t.Run(cc.name, func(t *testing.T) {
kubeclientset := kubefake.NewSimpleClientset([]runtime.Object{}...)
argoDBMock := dbmocks.ArgoDB{}

client := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(&cc.appSet).WithObjects(&cc.appSet).Build()
metrics := appsetmetrics.NewFakeAppsetMetrics(client)
Expand All @@ -6386,7 +6376,7 @@ func TestResourceStatusAreOrdered(t *testing.T) {
Scheme: scheme,
Recorder: record.NewFakeRecorder(1),
Generators: map[string]generators.Generator{},
ArgoDB: &argoDBMock,
ArgoDB: &dbmocks.ArgoDB{},
KubeClientset: kubeclientset,
Metrics: metrics,
}
Expand Down
27 changes: 22 additions & 5 deletions applicationset/utils/clusterUtils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,24 +51,38 @@ const (
// if we used destination name we infer the server url
// if we used both name and server then we return an invalid spec error
func ValidateDestination(ctx context.Context, dest *appv1.ApplicationDestination, clientset kubernetes.Interface, argoCDNamespace string) error {
if dest.IsServerInferred() && dest.IsNameInferred() {
return fmt.Errorf("application destination can't have both name and server inferred: %s %s", dest.Name, dest.Server)
}
if dest.Name != "" {
if dest.Server == "" {
server, err := getDestinationServer(ctx, dest.Name, clientset, argoCDNamespace)
server, err := getDestinationBy(ctx, dest.Name, clientset, argoCDNamespace, true)
if err != nil {
return fmt.Errorf("unable to find destination server: %w", err)
}
if server == "" {
return fmt.Errorf("application references destination cluster %s which does not exist", dest.Name)
}
dest.SetInferredServer(server)
} else if !dest.IsServerInferred() {
} else if !dest.IsServerInferred() && !dest.IsNameInferred() {
return fmt.Errorf("application destination can't have both name and server defined: %s %s", dest.Name, dest.Server)
}
} else if dest.Server != "" {
if dest.Name == "" {
serverName, err := getDestinationBy(ctx, dest.Server, clientset, argoCDNamespace, false)
if err != nil {
return fmt.Errorf("unable to find destination server: %w", err)
}
if serverName == "" {
return fmt.Errorf("application references destination cluster %s which does not exist", dest.Server)
}
dest.SetInferredName(serverName)
}
}
return nil
}

func getDestinationServer(ctx context.Context, clusterName string, clientset kubernetes.Interface, argoCDNamespace string) (string, error) {
func getDestinationBy(ctx context.Context, cluster string, clientset kubernetes.Interface, argoCDNamespace string, byName bool) (string, error) {
// settingsMgr := settings.NewSettingsManager(context.TODO(), clientset, namespace)
// argoDB := db.NewDB(namespace, settingsMgr, clientset)
// clusterList, err := argoDB.ListClusters(ctx)
Expand All @@ -78,14 +92,17 @@ func getDestinationServer(ctx context.Context, clusterName string, clientset kub
}
var servers []string
for _, c := range clusterList.Items {
if c.Name == clusterName {
if byName && c.Name == cluster {
servers = append(servers, c.Server)
}
if !byName && c.Server == cluster {
servers = append(servers, c.Name)
}
}
if len(servers) > 1 {
return "", fmt.Errorf("there are %d clusters with the same name: %v", len(servers), servers)
} else if len(servers) == 0 {
return "", fmt.Errorf("there are no clusters with this name: %s", clusterName)
return "", fmt.Errorf("there are no clusters with this name: %s", cluster)
}
return servers[0], nil
}
Expand Down
Loading
Loading