fix: Better handling of K8s secrets for repository credentials & templates (#3016 #3028)

test/e2e/fixture/fixture.go

@@ -55,7 +55,6 @@ var (
  KubeClientset kubernetes.Interface
  AppClientset appclientset.Interface
  ArgoCDClientset argocdclient.Client
- settingsManager *settings.SettingsManager
  apiServerAddress string
  token string
  plainText bool
@@ -123,7 +122,6 @@ func init() {
  })
  CheckError(err)
 
- settingsManager = settings.NewSettingsManager(context.Background(), KubeClientset, "argocd-e2e")
  token = sessionResponse.Token
  plainText = !tlsTestResult.TLS
 

util/db/db_test.go

@@ -60,7 +60,7 @@ func TestCreateRepository(t *testing.T) {
  assert.Nil(t, err)
  assert.Equal(t, "https://github.com/argoproj/argocd-example-apps", repo.Repo)
 
- secret, err := clientset.CoreV1().Secrets(testNamespace).Get(repoURLToSecretName(repo.Repo), metav1.GetOptions{})
+ secret, err := clientset.CoreV1().Secrets(testNamespace).Get(repoURLToSecretName(repoSecretPrefix, repo.Repo), metav1.GetOptions{})
  assert.Nil(t, err)
 
  assert.Equal(t, common.AnnotationValueManagedByArgoCD, secret.Annotations[common.AnnotationKeyManagedBy])
@@ -81,7 +81,7 @@ func TestCreateRepoCredentials(t *testing.T) {
  assert.Nil(t, err)
  assert.Equal(t, "https://github.com/argoproj/", creds.URL)
 
- secret, err := clientset.CoreV1().Secrets(testNamespace).Get(repoURLToSecretName(creds.URL), metav1.GetOptions{})
+ secret, err := clientset.CoreV1().Secrets(testNamespace).Get(repoURLToSecretName(credSecretPrefix, creds.URL), metav1.GetOptions{})
  assert.Nil(t, err)
 
  assert.Equal(t, common.AnnotationValueManagedByArgoCD, secret.Annotations[common.AnnotationKeyManagedBy])

util/db/repository.go

@@ -21,6 +21,10 @@ import (
 )
 
 const (
+ // Prefix to use for naming repository secrets
+ repoSecretPrefix = "repo"
+ // Prefix to use for naming credential template secrets
+ credSecretPrefix = "creds"
  // The name of the key storing the username in the secret
  username = "username"
  // The name of the key storing the password in the secret
@@ -349,63 +353,66 @@ func (db *db) updateCredentialsSecret(credsInfo *settings.RepositoryCredentials,
  TLSClientCertData: c.TLSClientCertData,
  TLSClientCertKey: c.TLSClientCertKey,
  }
- var repoInfo settings.Repository
+ secretsData := make(map[string]map[string][]byte)
 
- err := db.updateRepositorySecrets(&repoInfo, r)
- if err != nil {
- return err
+ credsInfo.UsernameSecret = setSecretData(credSecretPrefix, r.Repo, secretsData, credsInfo.UsernameSecret, r.Username, username)
+ credsInfo.PasswordSecret = setSecretData(credSecretPrefix, r.Repo, secretsData, credsInfo.PasswordSecret, r.Password, password)
+ credsInfo.SSHPrivateKeySecret = setSecretData(credSecretPrefix, r.Repo, secretsData, credsInfo.SSHPrivateKeySecret, r.SSHPrivateKey, sshPrivateKey)
+ credsInfo.TLSClientCertDataSecret = setSecretData(credSecretPrefix, r.Repo, secretsData, credsInfo.TLSClientCertDataSecret, r.TLSClientCertData, tlsClientCertData)
+ credsInfo.TLSClientCertKeySecret = setSecretData(credSecretPrefix, r.Repo, secretsData, credsInfo.TLSClientCertKeySecret, r.TLSClientCertKey, tlsClientCertKey)
+ for k, v := range secretsData {
+ err := db.upsertSecret(k, v)
+ if err != nil {
+ return err
+ }
  }
-
- credsInfo.UsernameSecret = repoInfo.UsernameSecret
- credsInfo.PasswordSecret = repoInfo.PasswordSecret
- credsInfo.SSHPrivateKeySecret = repoInfo.SSHPrivateKeySecret
- credsInfo.TLSClientCertDataSecret = repoInfo.TLSClientCertDataSecret
- credsInfo.TLSClientCertKeySecret = repoInfo.TLSClientCertKeySecret
-
  return nil
 }
 
 func (db *db) updateRepositorySecrets(repoInfo *settings.Repository, r *appsv1.Repository) error {
  secretsData := make(map[string]map[string][]byte)
 
- setSecretData := func(secretKey *apiv1.SecretKeySelector, value string, defaultKeyName string) *apiv1.SecretKeySelector {
- if secretKey == nil && value != "" {
- secretKey = &apiv1.SecretKeySelector{
- LocalObjectReference: apiv1.LocalObjectReference{Name: repoURLToSecretName(r.Repo)},
- Key: defaultKeyName,
- }
+ repoInfo.UsernameSecret = setSecretData(repoSecretPrefix, r.Repo, secretsData, repoInfo.UsernameSecret, r.Username, username)
+ repoInfo.PasswordSecret = setSecretData(repoSecretPrefix, r.Repo, secretsData, repoInfo.PasswordSecret, r.Password, password)
+ repoInfo.SSHPrivateKeySecret = setSecretData(repoSecretPrefix, r.Repo, secretsData, repoInfo.SSHPrivateKeySecret, r.SSHPrivateKey, sshPrivateKey)
+ repoInfo.TLSClientCertDataSecret = setSecretData(repoSecretPrefix, r.Repo, secretsData, repoInfo.TLSClientCertDataSecret, r.TLSClientCertData, tlsClientCertData)
+ repoInfo.TLSClientCertKeySecret = setSecretData(repoSecretPrefix, r.Repo, secretsData, repoInfo.TLSClientCertKeySecret, r.TLSClientCertKey, tlsClientCertKey)
+ for k, v := range secretsData {
+ err := db.upsertSecret(k, v)
+ if err != nil {
+ return err
  }
+ }
+ return nil
+}
 
- if secretKey != nil {
- data, ok := secretsData[secretKey.Name]
- if !ok {
- data = map[string][]byte{}
- }
- if value != "" {
- data[secretKey.Key] = []byte(value)
- }
- secretsData[secretKey.Name] = data
+// Set data to be stored in a given secret used for repository credentials and templates.
+// The name of the secret is a combination of the prefix given, and a calculated value
+// from the repository or template URL.
+func setSecretData(prefix string, url string, secretsData map[string]map[string][]byte, secretKey *apiv1.SecretKeySelector, value string, defaultKeyName string) *apiv1.SecretKeySelector {
+ if secretKey == nil && value != "" {
+ secretKey = &apiv1.SecretKeySelector{
+ LocalObjectReference: apiv1.LocalObjectReference{Name: repoURLToSecretName(prefix, url)},
+ Key: defaultKeyName,
  }
+ }
 
- if value == "" {
- secretKey = nil
+ if secretKey != nil {
+ data, ok := secretsData[secretKey.Name]
+ if !ok {
+ data = map[string][]byte{}
  }
-
- return secretKey
+ if value != "" {
+ data[secretKey.Key] = []byte(value)
+ }
+ secretsData[secretKey.Name] = data
  }
 
- repoInfo.UsernameSecret = setSecretData(repoInfo.UsernameSecret, r.Username, username)
- repoInfo.PasswordSecret = setSecretData(repoInfo.PasswordSecret, r.Password, password)
- repoInfo.SSHPrivateKeySecret = setSecretData(repoInfo.SSHPrivateKeySecret, r.SSHPrivateKey, sshPrivateKey)
- repoInfo.TLSClientCertDataSecret = setSecretData(repoInfo.TLSClientCertDataSecret, r.TLSClientCertData, tlsClientCertData)
- repoInfo.TLSClientCertKeySecret = setSecretData(repoInfo.TLSClientCertKeySecret, r.TLSClientCertKey, tlsClientCertKey)
- for k, v := range secretsData {
- err := db.upsertSecret(k, v)
- if err != nil {
- return err
- }
+ if value == "" {
+ secretKey = nil
  }
- return nil
+
+ return secretKey
 }
 
 func (db *db) upsertSecret(name string, data map[string][]byte) error {
@@ -486,11 +493,8 @@ func getRepositoryCredentialIndex(repoCredentials []settings.RepositoryCredentia
 // repositories are _imperatively_ created and need its credentials to be stored in a secret.
 // NOTE: this formula should not be considered stable and may change in future releases.
 // Do NOT rely on this formula as a means of secret lookup, only secret creation.
-func repoURLToSecretName(repo string) string {
+func repoURLToSecretName(prefix string, repo string) string {
  h := fnv.New32a()
  _, _ = h.Write([]byte(repo))
- // Part of the original repo name is incorporated into the secret name for debugging purposes
- parts := strings.Split(strings.TrimSuffix(repo, ".git"), "/")
- shortName := strings.ToLower(strings.Replace(parts[len(parts)-1], "_", "-", -1))
- return fmt.Sprintf("repo-%s-%v", shortName, h.Sum32())
+ return fmt.Sprintf("%s-%v", prefix, h.Sum32())
 }

util/db/repository_test.go

@@ -8,16 +8,31 @@ import (
 
 func TestRepoURLToSecretName(t *testing.T) {
  tables := map[string]string{
- "git://git@github.com:argoproj/ARGO-cd.git": "repo-argo-cd-83273445",
- "https://github.com/argoproj/ARGO-cd": "repo-argo-cd-1890113693",
- "https://github.com/argoproj/argo-cd": "repo-argo-cd-42374749",
- "https://github.com/argoproj/argo-cd.git": "repo-argo-cd-821842295",
- "https://github.com/argoproj/argo_cd.git": "repo-argo-cd-1049844989",
- "ssh://git@github.com/argoproj/argo-cd.git": "repo-argo-cd-3569564120",
+ "git://git@github.com:argoproj/ARGO-cd.git": "repo-83273445",
+ "https://github.com/argoproj/ARGO-cd": "repo-1890113693",
+ "https://github.com/argoproj/argo-cd": "repo-42374749",
+ "https://github.com/argoproj/argo-cd.git": "repo-821842295",
+ "https://github.com/argoproj/argo_cd.git": "repo-1049844989",
+ "ssh://git@github.com/argoproj/argo-cd.git": "repo-3569564120",
  }
 
  for k, v := range tables {
- if sn := repoURLToSecretName(k); sn != v {
+ if sn := repoURLToSecretName(repoSecretPrefix, k); sn != v {
+ t.Errorf("Expected secret name %q for repo %q; instead, got %q", v, k, sn)
+ }
+ }
+}
+
+func Test_CredsURLToSecretName(t *testing.T) {
+ tables := map[string]string{
+ "git://git@github.com:argoproj": "creds-2483499391",
+ "git://git@github.com:argoproj/": "creds-1465032944",
+ "git@github.com:argoproj": "creds-2666065091",
+ "git@github.com:argoproj/": "creds-346879876",
+ }
+
+ for k, v := range tables {
+ if sn := repoURLToSecretName(credSecretPrefix, k); sn != v {
  t.Errorf("Expected secret name %q for repo %q; instead, got %q", v, k, sn)
  }
  }