fix(server): make a copy of secret objects when listing from the info…

…rmers #19913 (#20805) * fix(server): make a copy of secret objects when listing from the informers Signed-off-by: rumstead <[email protected]> * fix(server): make a copy of secret objects when listing from the informers Signed-off-by: rumstead <[email protected]> * fix(server): make a copy of secret objects when listing from the informers Signed-off-by: rumstead <[email protected]> * fix(server): make a copy of secret objects when listing from the informers Signed-off-by: rumstead <[email protected]> * fix(server): make a copy of secret objects when listing from the informers Signed-off-by: rumstead <[email protected]> * fix(server): make a copy of secret objects when listing from the informers Signed-off-by: rumstead <[email protected]> * e2e Signed-off-by: rumstead <[email protected]> --------- Signed-off-by: rumstead <[email protected]>
argoproj · Nov 21, 2024 · c8c22d3 · c8c22d3
1 parent 2c2e669
commit c8c22d3
Show file tree

Hide file tree

Showing 5 changed files with 127 additions and 0 deletions.
diff --git a/util/db/cluster_test.go b/util/db/cluster_test.go
@@ -421,3 +421,66 @@ func TestListClusters(t *testing.T) {
 		assert.Len(t, clusters.Items, 1)
 	})
 }
+
+// TestClusterRaceConditionClusterSecrets reproduces a race condition
+// on the cluster secrets. The test isn't asserting anything because
+// before the fix it would cause a panic from concurrent map iteration and map write
+func TestClusterRaceConditionClusterSecrets(t *testing.T) {
+	clusterSecret := &v1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "mycluster",
+			Namespace: "default",
+			Labels: map[string]string{
+				common.LabelKeySecretType: common.LabelValueSecretTypeCluster,
+			},
+		},
+		Data: map[string][]byte{
+			"server": []byte("http://mycluster"),
+			"config": []byte("{}"),
+		},
+	}
+	kubeClient := fake.NewSimpleClientset(
+		&v1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      common.ArgoCDConfigMapName,
+				Namespace: "default",
+				Labels: map[string]string{
+					"app.kubernetes.io/part-of": "argocd",
+				},
+			},
+			Data: map[string]string{},
+		},
+		&v1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      common.ArgoCDSecretName,
+				Namespace: "default",
+				Labels: map[string]string{
+					"app.kubernetes.io/part-of": "argocd",
+				},
+			},
+			Data: map[string][]byte{
+				"admin.password":   nil,
+				"server.secretkey": nil,
+			},
+		},
+		clusterSecret,
+	)
+	settingsManager := settings.NewSettingsManager(context.Background(), kubeClient, "default")
+	db := NewDB("default", settingsManager, kubeClient)
+	cluster, _ := SecretToCluster(clusterSecret)
+	go func() {
+		for {
+			// create a copy so we dont act on the same argo cluster
+			clusterCopy := cluster.DeepCopy()
+			_, _ = db.UpdateCluster(context.Background(), clusterCopy)
+		}
+	}()
+	// yes, we will take 15 seconds to run this test
+	// but it reliably triggered the race condition
+	for i := 0; i < 30; i++ {
+		// create a copy so we dont act on the same argo cluster
+		clusterCopy := cluster.DeepCopy()
+		_, _ = db.UpdateCluster(context.Background(), clusterCopy)
+		time.Sleep(time.Millisecond * 500)
+	}
+}
diff --git a/util/db/secrets.go b/util/db/secrets.go
@@ -20,6 +20,7 @@ import (
 	"k8s.io/client-go/tools/cache"
 
 	"github.com/argoproj/argo-cd/v2/common"
+	"github.com/argoproj/argo-cd/v2/util"
 )
 
 func (db *db) listSecretsByType(types ...string) ([]*apiv1.Secret, error) {
@@ -38,6 +39,10 @@ func (db *db) listSecretsByType(types ...string) ([]*apiv1.Secret, error) {
 	if err != nil {
 		return nil, err
 	}
+	// SecretNamespaceLister lists all Secrets in the indexer for a given namespace.
+	// Objects returned by the lister must be treated as read-only.
+	// To allow us to modify the secrets, make a copy
+	secrets = util.SecretCopy(secrets)
 	return secrets, nil
 }
 

diff --git a/util/settings/settings.go b/util/settings/settings.go
@@ -1339,6 +1339,10 @@ func (mgr *SettingsManager) GetSettings() (*ArgoCDSettings, error) {
 	if err != nil {
 		return nil, err
 	}
+	// SecretNamespaceLister lists all Secrets in the indexer for a given namespace.
+	// Objects returned by the lister must be treated as read-only.
+	// To allow us to modify the secrets, make a copy
+	secrets = util.SecretCopy(secrets)
 	var settings ArgoCDSettings
 	var errs []error
 	updateSettingsFromConfigMap(&settings, argoCDCM)

diff --git a/util/util.go b/util/util.go
@@ -3,6 +3,8 @@ package util
 import (
 	"crypto/rand"
 	"encoding/base64"
+
+	apiv1 "k8s.io/api/core/v1"
 )
 
 // MakeSignature generates a cryptographically-secure pseudo-random token, based on a given number of random bytes, for signing purposes.
@@ -16,3 +18,15 @@ func MakeSignature(size int) ([]byte, error) {
 	b = []byte(base64.StdEncoding.EncodeToString(b))
 	return b, err
 }
+
+// SecretCopy generates a deep copy of a slice containing secrets
+//
+// This function takes a slice of pointers to Secrets and returns a new slice
+// containing deep copies of the original secrets.
+func SecretCopy(secrets []*apiv1.Secret) []*apiv1.Secret {
+	secretsCopy := make([]*apiv1.Secret, len(secrets))
+	for i, secret := range secrets {
+		secretsCopy[i] = secret.DeepCopy()
+	}
+	return secretsCopy
+}
diff --git a/util/util_test.go b/util/util_test.go
@@ -4,6 +4,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	apiv1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"github.com/argoproj/argo-cd/v2/util"
 	"github.com/argoproj/argo-cd/v2/util/webhook"
@@ -39,3 +41,42 @@ func TestParseRevision(t *testing.T) {
 		})
 	}
 }
+
+func TestSecretCopy(t *testing.T) {
+	type args struct {
+		secrets []*apiv1.Secret
+	}
+	tests := []struct {
+		name string
+		args args
+		want []*apiv1.Secret
+	}{
+		{name: "nil", args: args{secrets: nil}, want: []*apiv1.Secret{}},
+		{
+			name: "Three", args: args{secrets: []*apiv1.Secret{
+				{ObjectMeta: metav1.ObjectMeta{Name: "one"}},
+				{ObjectMeta: metav1.ObjectMeta{Name: "two"}},
+				{ObjectMeta: metav1.ObjectMeta{Name: "three"}},
+			}},
+			want: []*apiv1.Secret{
+				{ObjectMeta: metav1.ObjectMeta{Name: "one"}},
+				{ObjectMeta: metav1.ObjectMeta{Name: "two"}},
+				{ObjectMeta: metav1.ObjectMeta{Name: "three"}},
+			},
+		},
+		{
+			name: "One", args: args{secrets: []*apiv1.Secret{{ObjectMeta: metav1.ObjectMeta{Name: "one"}}}},
+			want: []*apiv1.Secret{{ObjectMeta: metav1.ObjectMeta{Name: "one"}}},
+		},
+		{name: "Zero", args: args{secrets: []*apiv1.Secret{}}, want: []*apiv1.Secret{}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			secretsCopy := util.SecretCopy(tt.args.secrets)
+			assert.Equalf(t, tt.want, secretsCopy, "SecretCopy(%v)", tt.args.secrets)
+			for i := range tt.args.secrets {
+				assert.NotSame(t, secretsCopy[i], tt.args.secrets[i])
+			}
+		})
+	}
+}