Repository: argoproj-labs/applicationset (go)

Issue:
ApplicationSet Controller Panics when Cluster Secret is malformed
Situation:  We found that the ApplicationSet controller v0.2.0 was in a crash loop back-off in my cluster.  The stack trace (will go looking for that in a moment) indicated an unexpected newline when unmarshaling the data.config, pointing to this line:

https://github.com/argoproj-labs/applicationset/blob/master/pkg/utils/clusterUtils.go#L107

The root cause was that the secret was created and the caBundle field was not base64 encoded.  Removing those secrets and restarting the ApplicationSet controller resolved the issue.

I'm wondering if a panic is an appropriate response, or just eliminate the affected cluster from the current compilation scope.

Pull request description:
ApplicationSet Controller Panics when Cluster Secret is malformed
Fixes #398

Patch:
diff --git a/pkg/utils/clusterUtils.go b/pkg/utils/clusterUtils.go
index df96d4d..ab10d3f 100644
--- a/pkg/utils/clusterUtils.go
+++ b/pkg/utils/clusterUtils.go
@@ -64,8 +64,13 @@ func ListClusters(ctx context.Context, clientset kubernetes.Interface, namespace
 	}
 	hasInClusterCredentials := false
 	for i, clusterSecret := range clusterSecrets {
-		cluster := *secretToCluster(&clusterSecret)
-		clusterList.Items[i] = cluster
+		// This line has changed from the original Argo CD code: now receives an error, and handles it
+		cluster, err := secretToCluster(&clusterSecret)
+		if err != nil || cluster == nil {
+			return nil, fmt.Errorf("unable to convert cluster secret to cluster object '%s': %v", clusterSecret.Name, err)
+		}
+
+		clusterList.Items[i] = *cluster
 		if cluster.Server == appv1.KubernetesInternalAPIServerAddr {
 			hasInClusterCredentials = true
 		}
@@ -99,12 +104,12 @@ func getLocalCluster(clientset kubernetes.Interface) *appv1.Cluster {
 }
 
 // secretToCluster converts a secret into a Cluster object
-func secretToCluster(s *corev1.Secret) *appv1.Cluster {
+func secretToCluster(s *corev1.Secret) (*appv1.Cluster, error) {
 	var config appv1.ClusterConfig
 	if len(s.Data["config"]) > 0 {
-		err := json.Unmarshal(s.Data["config"], &config)
-		if err != nil {
-			panic(err)
+		if err := json.Unmarshal(s.Data["config"], &config); err != nil {
+			// This line has changed from the original Argo CD: now returns an error rather than panicing.
+			return nil, err
 		}
 	}
 
@@ -140,7 +145,7 @@ func secretToCluster(s *corev1.Secret) *appv1.Cluster {
 		RefreshRequestedAt: refreshRequestedAt,
 		Shard:              shard,
 	}
-	return &cluster
+	return &cluster, nil
 }
 
 // ValidateDestination checks:


Test patch:
diff --git a/pkg/utils/clusterUtils_test.go b/pkg/utils/clusterUtils_test.go
index 6a971b8..4d003f1 100644
--- a/pkg/utils/clusterUtils_test.go
+++ b/pkg/utils/clusterUtils_test.go
@@ -32,7 +32,8 @@ func Test_secretToCluster(t *testing.T) {
 			"config": []byte("{\"username\":\"foo\"}"),
 		},
 	}
-	cluster := secretToCluster(secret)
+	cluster, err := secretToCluster(secret)
+	assert.Nil(t, err)
 	assert.Equal(t, *cluster, argoappv1.Cluster{
 		Name:   "test",
 		Server: "http://mycluster",
@@ -54,7 +55,8 @@ func Test_secretToCluster_NoConfig(t *testing.T) {
 			"server": []byte("http://mycluster"),
 		},
 	}
-	cluster := secretToCluster(secret)
+	cluster, err := secretToCluster(secret)
+	assert.Nil(t, err)
 	assert.Equal(t, *cluster, argoappv1.Cluster{
 		Name:   "test",
 		Server: "http://mycluster",