argoproj / argo-cd

Declarative Continuous Deployment for Kubernetes
https://argo-cd.readthedocs.io
Apache License 2.0
17.76k stars 5.42k forks source link

Allow Application to read arbitrary values in Cluster secret #12533

Open CmdrSharp opened 1 year ago

CmdrSharp commented 1 year ago

Summary

I think that the Application resources should be allowed to read any arbitrary value specified within Cluster Secrets (under data), perhaps with the exception of config.

Motivation

This allows for scenarios where a central team might handle deployment of an application in Argo, but roll out the actual cluster via other config management tools and need the ability to alter values between each cluster, whilst keeping the root manifests the same.

In our example, we use Google Anthos (and its Config Management) to roll out cluster configuration - including actually installing Argo and creating the Cluster Secret. We roll out a number of deployments on each cluster - and want the ability to keep configuration that is likely to change between clusters (for example some helm values) in a Cluster Secret that we can then template in like other cluster values.

Proposal

Simply iterating over keys in the data of the secret should suffice. It seems this was attempted in PR #10935 but was closed immediately. If we want it more strict, perhaps it could read anything under a custom or values field, but that would require being able to extract sub-keys from those.

crenshaw-dev commented 1 year ago

The values used in ApplicationSets are destined for Application resources. Application resources are not designed to be safe stores for secret values. Since the data field of cluster secrets may contain truly-secret data, that field should not be made available for templating in an ApplicationSet.

If the data is not actually secret, it can be safely duplicated in the cluster secrets annotations field, which is available to ApplicationSets.

CmdrSharp commented 1 year ago

@crenshaw-dev In this case, it's perhaps not so much a secret as shared configuration that is local to that cluster only. The described use case is primarily for helm values that might change in between different clusters, for example.

Could these values be stored and effectively used within an annotation? Perhaps, I'll give it a go and report back.

crenshaw-dev commented 1 year ago

Yep, in a lot of cases nothing in the secret is actually secret. It’s just a convenient place to store configuration since, in many cases, some of the contents are secret.

To use configuration from the secret in an ApplicationSet, you’ll need to duplicate it: the ApplicationSet controller may only read from annotations, but the Application controller may only read from the data field.

CmdrSharp commented 1 year ago

@crenshaw-dev Hm, let me get this straight - are you saying the Application manifests can read arbitrary Secret keys (under data)? The docs list a specific few that it can read, but in my attempts to use a completely arbitrary value, it doesn't seem to work.

I've updated the issue since I realise I didn't actually mean for ApplicationSet to read these values, but for the rendered Application. Here's an example of what I want to be able to do:

---
apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: vector
  namespace: argocd
spec:
  generators:
  - clusters:
      selector:
        matchLabels:
          argocd.argoproj.io/secret-type: cluster
  template:
    metadata:
      name: 'vector'
    spec:
      project: "default"
      source:
        repoURL: https://helm.vector.dev
        chart: vector
        targetRevision: 0.17.0
        helm:
          version: v3
          passCredentials: false
          values: |-
            {{ vector-values }}
      destination:
        server: '{{ server }}'
        namespace: datadog
      syncPolicy:
        automated:
          prune: true
          selfHeal: true
          allowEmpty: false
        syncOptions:
        - PrunePropagationPolicy=foreground
        - PruneLast=true
        retry:
          limit: 5
          backoff:
            duration: 5s
            factor: 2
            maxDuration: 3m
      revisionHistoryLimit: 5

Note specifically this part:

        helm:
          version: v3
          passCredentials: false
          values: |-
            {{ vector-values }}

I've defined vector-values as a key on the cluster secret, and would like to be able to read that here.

crenshaw-dev commented 1 year ago

Hm, let me get this straight - are you saying the Application manifests can read arbitrary Secret keys (under data)?

The Application itself provides no interface to the data field. But the Application controller reads the data field to connect to the destination controller. In other words, the data field is the Application controller’s only source of truth. So if you want the data to also be available to the ApplicationSet controller, you’ll need to duplicate it onto an annotation.

The problem with the example above is that it would template the secrets into the Application manifests, where they would be stored in plaintext. In the ApplicationSet it’s just a reference. But the ApplicationSet just automates creating Applications. The Application would contain the actual value instead of just a reference to it.

There is a PR open from jannfis proposing a “dynamic parameters” feature which would allow an Application to reference secrets without actually storing the plaintext secrets in the Application resource itself. There’s also work being done in Helm which would open the possibility of Argo CD supporting the Helm lookup function, which would allow secret access.

CmdrSharp commented 1 year ago

I see.

The problem with the example above is that it would template the secrets into the Application manifests, where they would be stored in plaintext.

This is only scary if the data is actually secret. In the example mentioned, it's not - it's just that it happens to be the only place where data can be looked up right now. I see it as an interface where some data might not exist within or be managed by Argo - but I want Argo to use that data (which exists in the actual Kubernetes cluster) as input for, let's say, a Helm chart.

I'm still not sure if you are saying the example above should work, but it doesn't. Even if I do use the annotations field to store the yaml, it seems to fail templating it:

Error: failed to parse /tmp/de634c0b-da68-4d9f-a262-9b1488af468a: error converting YAML to JSON: yaml: line 36: could not find expected ':'

Inputting the exact same data (which is in the annotation) directly into the Application resource renders fine.

There is a PR open from jannfis proposing a “dynamic parameters” feature which would allow an Application to reference secrets without actually storing the plaintext secrets in the Application resource itself. There’s also work being done in Helm which would open the possibility of Argo CD supporting the Helm lookup function, which would allow secret access.

Both sound promising - but I take it to mean there's no current solution to this within Argo. Do you think this issue is worth keeping open to track this kind of "wanted behaviour", or should I close it?

crenshaw-dev commented 1 year ago

This is only scary if the data is actually secret. In the example mentioned, it's not

Yep, and the ApplicationSet controller has no way really of differentiating (right now).

it's just that it happens to be the only place where data can be looked up right now

I think annotations should work.

Error: failed to parse /tmp/de634c0b-da68-4d9f-a262-9b1488af468a: error converting YAML to JSON: yaml: line 36: could not find expected ':'

Hm, not quite sure what to make of that... I think something like this should work:

        helm:
          version: v3
          passCredentials: false
          values: |-
            {{ metadata.annotations.vector-values }}

Both sound promising - but I take it to mean there's no current solution to this within Argo. Do you think this issue is worth keeping open to track this kind of "wanted behaviour", or should I close it?

I think it's a good request! I just hope we can solve the problem without making the data field of the secret available to templates. Is there any reason duplicating the data in annotation's couldn't work?

CmdrSharp commented 1 year ago

Yep, and the ApplicationSet controller has no way really of differentiating (right now).

Since the Argo-native secrets are already protected (I saw code preventing the use of the config field for example) - does it matter, as long as the use case is documented? Do people store actual secrets (apart from the ones native to Argo, which it protects already) within the Argo cluster config file?

Though I guess this begs the question why secrets are used as the cluster definition to begin with. Perhaps it'd be better if Argo had a Cluster CRD which stores actual secrets (such as cluster credentials) using a secretRef. I think this is how most other tools usually handle it. That's another topic altogether I suppose!

I think annotations should work.

My experiments failed initially (not quite sure why, but my theory is that it didn't like my values starting with the YAML document splitter ---), but I gave it another go now, and indeed - that seems to work fine! That at least covers my immediate needs now, since I can handle cluster-specific configuration centrally. Many, many thanks!

Is there any reason duplicating the data in annotation's couldn't work?

I guess it's not even duplicating - just storing it there instead of somewhere else. No, I think that works! A more clean interface would definitely be allowing some kind of dynamic parameters like mentioned above, of course. That also covers a wide variety of other use cases. I'd love to be able to tell Argo "hey, go fetch this information in Vault" or something like that, should I need to.

blakepettersson commented 1 year ago

Perhaps it'd be better if Argo had a Cluster CRD which stores actual secrets (such as cluster credentials) using a secretRef. I think this is how most other tools usually handle it. That's another topic altogether I suppose.

Agreed, would love to see something like that myself (#5139)

hsalluri259 commented 1 day ago

There is a PR open from jannfis proposing a “dynamic parameters” feature which would allow an Application to reference secrets without actually storing the plaintext secrets in the Application resource itself. There’s also work being done in Helm which would open the possibility of Argo CD supporting the Helm lookup function, which would allow secret access.

This would be great if ArgoCD supports the Helm lookup function. In my case I am trying to install an aws-load-balancer-controller helm chart. When I install it with terraform the TLS kubernetes secret is not recreated everytime, but with ArgoCD it is created everytime I do sync. This app will be outOfSync after every refresh. The reason for it is, lookup function in _helpers.tpl returns nothing so instead of going to else if block and use existing values, it is always going into else block of the _helpers.tpl