fix: Handle malformed cluster secrets gracefully (#739)

Signed-off-by: jannfis <jann@mistrust.net>

Author: jannfis

internal/argocd/cluster/informer.go

@@ -44,6 +44,7 @@ func (m *Manager) onClusterAdded(res *v1.Secret) {
 	cluster, err := db.SecretToCluster(res)
 	if err != nil {
 		log().WithError(err).Error("Not a cluster secret or malformed data")
+		return
 	}
 
 	// TODO(jannfis): Do we want to validate cluster configuration here?
@@ -78,7 +79,7 @@ func (m *Manager) onClusterUpdated(old *v1.Secret, new *v1.Secret) {
 
 	c, err := db.SecretToCluster(new)
 	if err != nil {
-		log().WithError(err).Errorf("Could not update ")
+		log().WithError(err).Errorf("Not a cluster secret or malformed data")
 		return
 	}
 
@@ -90,9 +91,11 @@ func (m *Manager) onClusterUpdated(old *v1.Secret, new *v1.Secret) {
 		}
 	}
 
-	// Unmap cluster from old agent
+	// Unmap cluster from old agent. If the cluster isn't mapped yet (that
+	// can happen if onClusterAdded found a malformed secret), we treat the
+	// operation as upsert instead of unmapping the old cluster.
 	log().Tracef("Unmapping cluster %s from agent %s", c.Name, oldAgent)
-	if err = m.unmapCluster(oldAgent); err != nil {
+	if err = m.unmapCluster(oldAgent); err != nil && err != ErrNotMapped {
 		log().WithError(err).Errorf("Could not unmap cluster %s from agent %s", c.Name, oldAgent)
 		return
 	}

internal/argocd/cluster/informer_test.go

@@ -28,6 +28,23 @@ func Test_onClusterAdded(t *testing.T) {
 		m.onClusterAdded(s)
 		assert.Len(t, m.clusters, 1)
 	})
+	t.Run("Secret is malformed", func(t *testing.T) {
+		m, err := NewManager(context.TODO(), "argocd", "", "", "", kube.NewFakeKubeClient("argocd"))
+		require.NoError(t, err)
+		s := &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					LabelKeyClusterAgentMapping: "agent",
+					common.LabelKeySecretType:   common.LabelValueSecretTypeCluster,
+				},
+			},
+			Data: map[string][]byte{
+				"config": []byte("invalid json"),
+			},
+		}
+		m.onClusterAdded(s)
+		assert.Len(t, m.clusters, 0)
+	})
 	t.Run("Secret is missing one or more labels", func(t *testing.T) {
 		m, err := NewManager(context.TODO(), "argocd", "", "", "", kube.NewFakeKubeClient("argocd"))
 		require.NoError(t, err)
@@ -91,6 +108,50 @@ func Test_onClusterUpdated(t *testing.T) {
 		assert.Nil(t, m.mapping("agent1"))
 		assert.NotNil(t, m.mapping("agent2"))
 	})
+	t.Run("Secret is malformed", func(t *testing.T) {
+		old := &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					LabelKeyClusterAgentMapping: "agent1",
+					common.LabelKeySecretType:   common.LabelValueSecretTypeCluster,
+				},
+				Name:      "cluster1",
+				Namespace: "argocd",
+			},
+			Data: map[string][]byte{
+				"name": []byte("cluster1"),
+			},
+		}
+		new := &v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					LabelKeyClusterAgentMapping: "agent1",
+					common.LabelKeySecretType:   common.LabelValueSecretTypeCluster,
+				},
+				Name:      "cluster1",
+				Namespace: "argocd",
+			},
+			Data: map[string][]byte{
+				"name": []byte("cluster2"),
+			},
+		}
+		m, err := NewManager(context.TODO(), "argocd", "", "", "", kube.NewFakeKubeClient("argocd"))
+		require.NoError(t, err)
+		m.mapCluster("agent1", &v1alpha1.Cluster{Name: "cluster1"})
+		assert.NotNil(t, m.mapping("agent1"))
+		// First (successful) update will change the cluster name to "cluster2"
+		m.onClusterUpdated(old, new)
+		assert.NotNil(t, m.mapping("agent1"))
+		assert.Equal(t, "cluster2", m.mapping("agent1").Name)
+		old = new.DeepCopy()
+		// Inject some invalid data to the new secret to trigger an error
+		new.Data["name"] = []byte("cluster3")
+		new.Data["config"] = []byte("invalid json")
+		// Second (unsuccessful) update will not change the cluster name
+		m.onClusterUpdated(old, new)
+		assert.NotNil(t, m.mapping("agent1"))
+		assert.Equal(t, "cluster2", m.mapping("agent1").Name)
+	})
 
 	t.Run("Can't remap to an agent that has a mapping already", func(t *testing.T) {
 		old := &v1.Secret{