feat: allow argocd-cm to reference K8S Secrets (#4188) #4342

nbendafi-yseop · 2020-09-15T20:49:40Z

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
The title of the PR states what changed and the related issues number (used for the release note).
I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
Optional. My organization is added to USERS.md.
I've signed the CLA and my build is green (troubleshooting builds).

codecov-commenter · 2020-09-15T20:59:04Z

Codecov Report

Merging #4342 (80e915f) into master (083390e) will increase coverage by 0.01%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master    #4342      +/-   ##
==========================================
+ Coverage   41.30%   41.31%   +0.01%     
==========================================
  Files         156      156              
  Lines       20702    20709       +7     
==========================================
+ Hits         8550     8556       +6     
+ Misses      10944    10942       -2     
- Partials     1208     1211       +3

Impacted Files	Coverage Δ
util/settings/settings.go	`47.36% <55.55%> (+0.36%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 083390e...80e915f. Read the comment docs.

nbendafi-yseop · 2020-09-15T21:21:18Z

This PR might also address #4052 and allow arbitrary Kubernetes Secret to be referenced and used in argocd-cm ConfigMap.
It allow definition of any entry such as

XXXSecretRef:
  name: <my secret>
  key: <key in my secret'data>

that we look for recursively in argocd-cm and output

XXX: <secret found in my secret>

So with following example (which is slightly modified form of proposal of #4188), assuming we have

data:
  url: https://argocd.example.com

  dex.config: |
    connectors:
      # GitHub example
      - type: github
        id: github
        name: GitHub
        config:
          clientID: aabbccddeeff00112233
          clientSecretSecretRef: # Note the SecretRef prefix
            name: whatever
            key: myGitHubClientSecret
          orgs:
          - name: your-github-org
  oidcConfig: |
      name: SSO
      clientID: argocd
      clientSecretSecretRef: # Note the SecretRef prefix
        name: whatever
        key: mySSOClientSecret

in argocd-cm
and a whatever Kubernetes Secret like

data:
  myGitHubClientSecret: a-value
  mySSOClientSecret: another-value

will produce

data:
  url: https://argocd.example.com

  dex.config: |
    connectors:
      # GitHub example
      - type: github
        id: github
        name: GitHub
        config:
          clientID: aabbccddeeff00112233
          clientSecret: a-value
          orgs:
          - name: your-github-org
  oidcConfig: |
    name: SSO
    clientID: argocd
    clientSecret: another-value

alexmt

Thanks for the PR @nbendafi-yseop ! Added two enhancement proposals.

alexmt · 2020-09-22T19:50:16Z

util/settings/settings.go

+			}
+
+			// Fetch K8S Secret
+			k8sSecret, err := mgr.clientset.CoreV1().Secrets(mgr.namespace).Get(context.Background(), secretRef.Name, metav1.GetOptions{})


Argo CD executes getConfigMap method very frequently. It is too expensive to make K8S API request. Please use mgr.GetSecretsLister instead:

lister, err := mgr.GetSecretsLister() if err != nil { return nil } lister.Secrets(mgr.namespace).Get(secretRef.Name)

As a side effect, each referenced secret must have "app.kubernetes.io/part-of": "argocd" label but I cannot see any other way to avoid performance degradation.

I think we can live with it. Even if secrets are not withing argocd-secret K8S secret, they are still part-of argocd. I'll update my code (and tests) accordingly.

After further testing, looks like "app.kubernetes.io/part-of": "argocd" only applies to ArgoCD ConfigMap, meaning lister.Secrets is able to look for K8S Secret even if they are not label with "app.kubernetes.io/part-of": "argocd" . But I might be wrong.

alexmt · 2020-09-22T19:56:05Z

util/settings/settings_test.go

+          name: GitHub
+          config:
+            clientID: aabbccddeeff00112233
+            clientSecretSecretRef:


Argo CD already supports referencing values from argocd-secret Secret. E.g. $dex.github.clientSecret. Can we keep using similar syntax for consistency. For example: $<secretName>:<key>. where <secretName> is optional secret name.

In this case secret sample config would looks like following:

config: clientID: aabbccddeeff00112233 clientSecret: $google:clientSecret

It is safe to use : as a delimiter since it cannot be used in secret name/secret data key.

I can understand the need for consistency. Here, I just wanted to be closer to SecretKeySelector type that exists in Kubernetes (cf. https://github.com/kubernetes/api/blob/master/core/v1/types.go#L1984) or in FluxCD's helm-operator (cf. https://github.com/fluxcd/helm-operator/blob/68de5288b733c42a316e03f9c01e7505cd6985b3/pkg/apis/helm.fluxcd.io/v1/types.go#L48).

I'm relatively new in this world, so I do not really have arguments against your proposal.

(And we do ArcgoCD here, not FluxCD 😄 )

ArgTang · 2021-01-11T14:57:11Z

@alexmt do you have time for another review? This would make it eaiser setup when integrating with oauth providers

sbose78 · 2021-01-19T19:09:57Z

USERS.md

@@ -80,6 +80,7 @@ Currently, the following organizations are **officially** using Argo CD:
 1. [Walkbase](https://www.walkbase.com/)
 1. [Whitehat Berlin](https://whitehat.berlin) by Guido Maria Serra +Fenaroli
 1. [Yieldlab](https://www.yieldlab.de/)
+1. [Yseop](https://www.yseop.com/)


Could you please add this change in a different PR ? :)

If that can make a difference, I've removed it from this PR.

sbose78

Could you please add PR description explaining the rationale of this change?
If this needs docs changes, could you add those to this PR too, please?

sbose78 · 2021-01-19T19:39:55Z

util/settings/settings_test.go

+          name: GitHub
+          config:
+            clientID: aabbccddeeff00112233
+            clientSecret: $google:clientSecret


Sorry, I'm having a hard time understanding the "why" of this PR :-)

Are we switching from $google.clientSecret to $google:clientSecret ?

Hello. The why is #4188 (as referred to the title of current PR).

To summarise, one would refer to secret that live outside of argocd-secret K8S Secret's data.

I'm not switching, rather adding another syntax.

Explanation by example (the one in settings_test is not obvious apparently), to clear the things out:

As defined (and limited by) in current documentation:

apiVersion: v1 kind: Secret metadata: name: argocd-secret namespace: argocd labels: app.kubernetes.io/name: argocd-secret app.kubernetes.io/part-of: argocd type: Opaque data: ... # Store client secret like below. # Ensure the secret is base64 encoded oidc.auth0.clientSecret: <client-secret-base64-encoded> ... ... apiVersion: v1 kind: ConfigMap metadata: name: argocd-cm namespace: argocd labels: app.kubernetes.io/name: argocd-cm app.kubernetes.io/part-of: argocd data: ... oidc.config: | name: Auth0 clientID: aabbccddeeff00112233 # Reference key in argocd-secret clientSecret: $oidc.auth0.clientSecret

so in clientSecret: $oidc.auth0.clientSecret, oidc... should be in argocd-secret's data, right ?

Now (after this PR, I mean), we can also have

apiVersion: v1 kind: Secret metadata: name: another-secret # that is not _handled_ by Argocd, and eventually synced by tools such as External-secret data: ... # Store client secret like below. # Ensure the secret is base64 encoded oidc.auth0.clientSecret: <client-secret-base64-encoded> ... apiVersion: v1 kind: ConfigMap metadata: name: argocd-cm data: ... oidc.config: | name: Auth0 clientID: aabbccddeeff00112233 # Reference key in argocd-secret clientSecret: $another-secret:oidc.auth0.clientSecret

where oidc.auth0.clientSecret is in data field of another K8S secret.

I was personally motivated to answer this issue because we have tool that sync secrets with external secret management tool (such as Vault or AWS secret manager) and periodically updates the whole K8S Secret which have the effect to be override the content of argocd-secret that ArgoCD updates as well (with its admin password, its certificates...)
So, yes, it indeed lakes of some documentation; that I'm gonna add.

This makes sense, thank you very much!

elocke · 2021-02-04T05:55:19Z

any updates for this PR? We just ran into this problem with kubernetes-external-secrets and SSO config.

nbendafi-yseop · 2021-05-03T15:15:54Z

@sbose78 Hi. Is there any chance to see this PR landing soon, to solve #4188, #4052, and now #5944 ? Or please let me know if any code update is needed.

Signed-off-by: Nabil BENDAFI <nbendafi@yseop.com>

mhaddon · 2021-06-04T00:42:21Z

It would be nice if this PR did not die, it would make ArgoCD integrate with the Keycloak Operator nicely.

MatteoJoliveau · 2021-06-18T16:32:43Z

@alexmt @sbose78 I understand the team has limited bandwidth available and there is a lot of work to be done. What can we do to help move this PR forward? It is a very useful contribution that will greatly increase ArgoCD's flexibility and integration with the rest of the kube ecosystem

alexmt · 2021-06-18T20:08:07Z

Sorry for letting this PR slip. Looking at it this week.

…ngsManager Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

alexmt · 2021-07-09T17:45:55Z

@nbendafi-yseop Sorry for the delay again. Better later than never.

I'm concerned that we start replacing secrets in all fields in Argo CD config map. It might cause unexpected side effects like white space removals etc.

I would like to propose a slightly different and safer implementation: change the existing GetSettings method to load secret values from multiple secrets, instead of only argocd-secret. So we don't have to touch replacing logic at all. I was not sure if it is possible so had to experiment and implemented changes in nbendafi-yseop#1

refactor: fetch values for all secrets in GetSettings method of SettingsManager

alexmt

Added one more comment about go linter failure. Also can you merge master to include documentation website build fix?

alexmt · 2021-07-12T16:15:31Z

util/settings/settings_test.go

+	}
+	kubeClient := fake.NewSimpleClientset(cm, secret, argocdSecret)
+	settingsManager := NewSettingsManager(context.Background(), kubeClient, "default")
+	cm, err := kubeClient.CoreV1().ConfigMaps("default").Get(context.Background(), common.ArgoCDConfigMapName, metav1.GetOptions{})


Looks like linter is not happy because cm variable is no longer needed in test . Please remove lines 925~926.

…rence-secret

Signed-off-by: Nabil BENDAFI <nbendafi@yseop.com>

alexmt

Thank you @nbendafi-yseop ! LGTM

jessesuen assigned alexmt Sep 15, 2020

nbendafi-yseop force-pushed the feature/4188_Reference-secret branch from d128611 to d837e89 Compare September 15, 2020 21:21

alexmt requested changes Sep 22, 2020

View reviewed changes

HumairAK mentioned this pull request Sep 23, 2020

Reduce manual steps for deploying ArgoCD operate-first/continuous-deployment#35

Closed

nbendafi-yseop force-pushed the feature/4188_Reference-secret branch from 211f5a2 to 746b96e Compare October 2, 2020 21:18

nbendafi-yseop requested a review from alexmt October 2, 2020 21:54

sbose78 suggested changes Jan 19, 2021

View reviewed changes

nbendafi-yseop force-pushed the feature/4188_Reference-secret branch 3 times, most recently from 34c5378 to be10a12 Compare January 21, 2021 15:28

nbendafi-yseop requested a review from sbose78 January 21, 2021 15:35

ErikLundJensen mentioned this pull request Mar 10, 2021

feat: Fallback to environment value when loading Argo CD configuration #5725

Closed

MatteoJoliveau mentioned this pull request Apr 1, 2021

GitLab Groups are not fetched from userinfo endpoint #5944

Open

3 tasks

nbendafi-yseop force-pushed the feature/4188_Reference-secret branch from be10a12 to 0331e36 Compare May 3, 2021 14:46

nbendafi-yseop added 6 commits May 4, 2021 12:07

feat: allow argocd-cm to reference K8S Secrets

a3ade54

Signed-off-by: Nabil BENDAFI <nbendafi@yseop.com>

fix: golangci-lint issues

8900dd8

Signed-off-by: Nabil BENDAFI <nbendafi@yseop.com>

fix: remove newline

f46696e

Signed-off-by: Nabil BENDAFI <nbendafi@yseop.com>

refactor: update secret reference lookup pattern

e67608d

Signed-off-by: Nabil BENDAFI <nbendafi@yseop.com>

docs: allow argocd-cm to reference K8S Secrets

feb24ce

Signed-off-by: Nabil BENDAFI <nbendafi@yseop.com>

fix: do not marshall empty yaml string

bea4b5f

Signed-off-by: Nabil BENDAFI <nbendafi@yseop.com>

nbendafi-yseop force-pushed the feature/4188_Reference-secret branch from 0331e36 to bea4b5f Compare May 4, 2021 10:10

refactor: fetch values for all secrets in GetSettings method of Setti…

91e18ee

…ngsManager Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

nbendafi-yseop mentioned this pull request Jul 12, 2021

refactor: fetch values for all secrets in GetSettings method of SettingsManager nbendafi-yseop/argo-cd#1

Merged

Merge pull request #1 from alexmt/feature/4188_Reference-secret

5b76056

refactor: fetch values for all secrets in GetSettings method of SettingsManager

alexmt requested changes Jul 12, 2021

View reviewed changes

nbendafi-yseop added 2 commits July 13, 2021 08:20

Merge remote-tracking branch 'upstream/master' into feature/4188_Refe…

c86a377

…rence-secret

fix(lint): ineffectual assignment to cm

80e915f

Signed-off-by: Nabil BENDAFI <nbendafi@yseop.com>

nbendafi-yseop force-pushed the feature/4188_Reference-secret branch from 3230193 to 80e915f Compare July 13, 2021 07:31

alexmt approved these changes Jul 16, 2021

View reviewed changes

alexmt merged commit 90bacef into argoproj:master Jul 16, 2021

alexmt added the needs-verification PR requires pre-release verification label Jul 16, 2021

alexmt added this to the v2.1 milestone Jul 16, 2021

alexmt mentioned this pull request Jul 22, 2021

OIDC config should reference dedicated secret instead of key in argocd-secret #4188

Closed

nbendafi-yseop deleted the feature/4188_Reference-secret branch September 28, 2021 10:05

benruland mentioned this pull request Jan 12, 2022

Allow reference of secrets for Git Webhook Secrets #8154

Open

jaideepr97 mentioned this pull request Feb 15, 2023

Audience validation breaks when reading oidc clientid from secret #12372

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow argocd-cm to reference K8S Secrets (#4188) #4342

feat: allow argocd-cm to reference K8S Secrets (#4188) #4342

nbendafi-yseop commented Sep 15, 2020 •

edited

codecov-commenter commented Sep 15, 2020 •

edited by codecov bot

nbendafi-yseop commented Sep 15, 2020

alexmt left a comment

alexmt Sep 22, 2020

nbendafi-yseop Sep 23, 2020

nbendafi-yseop Oct 2, 2020

alexmt Sep 22, 2020

nbendafi-yseop Sep 23, 2020

ArgTang commented Jan 11, 2021 •

edited

sbose78 Jan 19, 2021

nbendafi-yseop Jan 21, 2021

sbose78 left a comment •

edited

sbose78 Jan 19, 2021

nbendafi-yseop Jan 21, 2021

sbose78 Jan 21, 2021

elocke commented Feb 4, 2021

nbendafi-yseop commented May 3, 2021

mhaddon commented Jun 4, 2021

MatteoJoliveau commented Jun 18, 2021

alexmt commented Jun 18, 2021

alexmt commented Jul 9, 2021 •

edited

alexmt left a comment

alexmt Jul 12, 2021

nbendafi-yseop Jul 13, 2021

alexmt left a comment

feat: allow argocd-cm to reference K8S Secrets (#4188) #4342

feat: allow argocd-cm to reference K8S Secrets (#4188) #4342

Conversation

nbendafi-yseop commented Sep 15, 2020 • edited

codecov-commenter commented Sep 15, 2020 • edited by codecov bot

Codecov Report

nbendafi-yseop commented Sep 15, 2020

alexmt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArgTang commented Jan 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbose78 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elocke commented Feb 4, 2021

nbendafi-yseop commented May 3, 2021

mhaddon commented Jun 4, 2021

MatteoJoliveau commented Jun 18, 2021

alexmt commented Jun 18, 2021

alexmt commented Jul 9, 2021 • edited

alexmt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmt left a comment

Choose a reason for hiding this comment

nbendafi-yseop commented Sep 15, 2020 •

edited

codecov-commenter commented Sep 15, 2020 •

edited by codecov bot

ArgTang commented Jan 11, 2021 •

edited

sbose78 left a comment •

edited

alexmt commented Jul 9, 2021 •

edited