diff --git a/util/db/cluster_test.go b/util/db/cluster_test.go index 50cca2eff5d55..b32120674e1bc 100644 --- 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 index 021cf899c0e17..38136d8c43dba 100644 --- 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 index 516116f0ecd99..60b78f405d4eb 100644 --- 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 index f7a8d8e39a401..862d3848984ba 100644 --- 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 index ff7674419cab3..1edafb9350491 100644 --- 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]) + } + }) + } +}