Skip to content

Commit

Permalink
refactor: remove app destination inferrence logic
Browse files Browse the repository at this point in the history 
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
  • Loading branch information
@crenshaw-dev
crenshaw-dev committed Dec 16, 2024
1 parent 030a7be commit 1a6c7b0
Show file tree
Hide file tree
Showing 26 changed files with 315 additions and 642 deletions.
10 changes: 4 additions & 6 deletions applicationset/controllers/applicationset_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,7 @@ func (r *ApplicationSetReconciler) validateGeneratedApplications(ctx context.Con
return nil, err
}

if err := utils.ValidateDestination(ctx, &app.Spec.Destination, r.KubeClientset, r.ArgoCDNamespace); err != nil {
if _, err := argoutil.GetDestinationCluster(ctx, app.Spec.Destination, r.ArgoDB); err != nil {
errorsByIndex[i] = fmt.Errorf("application destination spec is invalid: %s", err.Error())
continue
}
Expand Down Expand Up @@ -783,21 +783,19 @@ func (r *ApplicationSetReconciler) removeFinalizerOnInvalidDestination(ctx conte
var validDestination bool

// Detect if the destination is invalid (name doesn't correspond to a matching cluster)
if err := utils.ValidateDestination(ctx, &app.Spec.Destination, r.KubeClientset, r.ArgoCDNamespace); err != nil {
if destCluster, err := argoutil.GetDestinationCluster(ctx, app.Spec.Destination, r.ArgoDB); err != nil {
appLog.Warnf("The destination cluster for %s couldn't be found: %v", app.Name, err)
validDestination = false
} else {
// Detect if the destination's server field does not match an existing cluster

matchingCluster := false
for _, cluster := range clusterList.Items {
// Server fields must match. Note that ValidateDestination ensures that the server field is set, if applicable.
if app.Spec.Destination.Server != cluster.Server {
if destCluster.Server != cluster.Server {
continue
}

// The name must match, if it is not empty
if app.Spec.Destination.Name != "" && cluster.Name != app.Spec.Destination.Name {
if destCluster.Name != "" && cluster.Name != destCluster.Name {
continue
}

Expand Down
109 changes: 59 additions & 50 deletions applicationset/controllers/applicationset_controller_test.go
View file

Large diffs are not rendered by default.

60 changes: 0 additions & 60 deletions applicationset/utils/clusterUtils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,66 +47,6 @@ const (
ArgoCDSecretTypeCluster = "cluster"
)

// ValidateDestination checks:
// 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 := 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() && !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 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)
clusterList, err := ListClusters(ctx, clientset, argoCDNamespace)
if err != nil {
return "", err
}
var servers []string
for _, c := range clusterList.Items {
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", cluster)
}
return servers[0], nil
}

func ListClusters(ctx context.Context, clientset kubernetes.Interface, namespace string) (*appv1.ClusterList, error) {
clusterSecretsList, err := clientset.CoreV1().Secrets(namespace).List(ctx,
metav1.ListOptions{LabelSelector: common.LabelKeySecretType + "=" + common.LabelValueSecretTypeCluster})
Expand Down
119 changes: 0 additions & 119 deletions applicationset/utils/clusterUtils_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,12 @@
package utils

import (
"context"
"fmt"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes/fake"
kubetesting "k8s.io/client-go/testing"

argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
)
Expand Down Expand Up @@ -64,117 +59,3 @@ func Test_secretToCluster_NoConfig(t *testing.T) {
Server: "http://mycluster",
}, *cluster)
}

func createClusterSecret(secretName string, clusterName string, clusterServer string) *corev1.Secret {
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: secretName,
Namespace: fakeNamespace,
Labels: map[string]string{
ArgoCDSecretTypeLabel: ArgoCDSecretTypeCluster,
},
},
Data: map[string][]byte{
"name": []byte(clusterName),
"server": []byte(clusterServer),
"config": []byte("{\"username\":\"foo\",\"password\":\"foo\"}"),
},
}

return secret
}

// From util/argo/argo_test.go
// (ported to use kubeclientset)
func TestValidateDestination(t *testing.T) {
t.Run("Validate destination with server url", func(t *testing.T) {
dest := argoappv1.ApplicationDestination{
Server: "https://127.0.0.1:6443",
Namespace: "default",
}

secret := createClusterSecret("my-secret", "minikube", "https://127.0.0.1:6443")
objects := []runtime.Object{}
objects = append(objects, secret)
kubeclientset := fake.NewSimpleClientset(objects...)

appCond := ValidateDestination(context.Background(), &dest, kubeclientset, fakeNamespace)
require.NoError(t, appCond)
assert.False(t, dest.IsServerInferred())
})

t.Run("Validate destination with server name", func(t *testing.T) {
dest := argoappv1.ApplicationDestination{
Name: "minikube",
}

secret := createClusterSecret("my-secret", "minikube", "https://127.0.0.1:6443")
objects := []runtime.Object{}
objects = append(objects, secret)
kubeclientset := fake.NewSimpleClientset(objects...)

appCond := ValidateDestination(context.Background(), &dest, kubeclientset, fakeNamespace)
require.NoError(t, appCond)
assert.Equal(t, "https://127.0.0.1:6443", dest.Server)
assert.True(t, dest.IsServerInferred())
})

t.Run("Error when having both server url and name", func(t *testing.T) {
dest := argoappv1.ApplicationDestination{
Server: "https://127.0.0.1:6443",
Name: "minikube",
Namespace: "default",
}

err := ValidateDestination(context.Background(), &dest, nil, fakeNamespace)
assert.Equal(t, "application destination can't have both name and server defined: minikube https://127.0.0.1:6443", err.Error())
assert.False(t, dest.IsServerInferred())
})

t.Run("List clusters fails", func(t *testing.T) {
dest := argoappv1.ApplicationDestination{
Name: "minikube",
}
kubeclientset := fake.NewSimpleClientset()

kubeclientset.PrependReactor("list", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) {
return true, nil, fmt.Errorf("an error occurred")
})

err := ValidateDestination(context.Background(), &dest, kubeclientset, fakeNamespace)
assert.Equal(t, "unable to find destination server: an error occurred", err.Error())
assert.False(t, dest.IsServerInferred())
})

t.Run("Destination cluster does not exist", func(t *testing.T) {
dest := argoappv1.ApplicationDestination{
Name: "minikube",
}

secret := createClusterSecret("dind", "dind", "https://127.0.0.1:6443")
objects := []runtime.Object{}
objects = append(objects, secret)
kubeclientset := fake.NewSimpleClientset(objects...)

err := ValidateDestination(context.Background(), &dest, kubeclientset, fakeNamespace)
assert.Equal(t, "unable to find destination server: there are no clusters with this name: minikube", err.Error())
assert.False(t, dest.IsServerInferred())
})

t.Run("Validate too many clusters with the same name", func(t *testing.T) {
dest := argoappv1.ApplicationDestination{
Name: "dind",
}

secret := createClusterSecret("dind", "dind", "https://127.0.0.1:2443")
secret2 := createClusterSecret("dind2", "dind", "https://127.0.0.1:8443")

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

err := ValidateDestination(context.Background(), &dest, kubeclientset, fakeNamespace)
assert.Equal(t, "unable to find destination server: there are 2 clusters with the same name: [https://127.0.0.1:2443 https://127.0.0.1:8443]", err.Error())
assert.False(t, dest.IsServerInferred())
})
}
27 changes: 11 additions & 16 deletions cmd/argocd/commands/admin/cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,13 +123,6 @@ func loadClusters(ctx context.Context, kubeClient *kubernetes.Clientset, appClie
}

apps := appItems.Items
for i, app := range apps {
err := argo.ValidateDestination(ctx, &app.Spec.Destination, argoDB)
if err != nil {
return nil, err
}
apps[i] = app
}
clusters := make([]ClusterWithInfo, len(clustersList.Items))

batchSize := 10
Expand All @@ -154,7 +147,11 @@ func loadClusters(ctx context.Context, kubeClient *kubernetes.Clientset, appClie
}
nsSet := map[string]bool{}
for _, app := range apps {
if app.Spec.Destination.Server == cluster.Server {
destCluster, err := argo.GetDestinationCluster(ctx, app.Spec.Destination, argoDB)
if err != nil {
return fmt.Errorf("error validating application destination: %w", err)
}
if destCluster.Server == cluster.Server {
nsSet[app.Spec.Destination.Namespace] = true
}
}
Expand Down Expand Up @@ -281,18 +278,16 @@ func runClusterNamespacesCommand(ctx context.Context, clientConfig clientcmd.Cli
return fmt.Errorf("error listing application: %w", err)
}
apps := appItems.Items
for i, app := range apps {
if err := argo.ValidateDestination(ctx, &app.Spec.Destination, argoDB); err != nil {
return fmt.Errorf("error validating application destination: %w", err)
}
apps[i] = app
}

clusters := map[string][]string{}
for _, cluster := range clustersList.Items {
nsSet := map[string]bool{}
for _, app := range apps {
if app.Spec.Destination.Server != cluster.Server {
destCluster, err := argo.GetDestinationCluster(ctx, app.Spec.Destination, argoDB)
if err != nil {
return fmt.Errorf("error validating application destination: %w", err)
}

if destCluster.Server != cluster.Server {
continue
}
// Use namespaces of actually deployed resources, since some application use dummy target namespace
Expand Down
Loading

0 comments on commit 1a6c7b0

Please sign in to comment.