-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for secrets in Application parameters #1786
Comments
I would also suggest to add a
|
Contributing guide if you'd like to raise a PR: https://argoproj.github.io/argo-cd/CONTRIBUTING/ |
Argo CD has https://argoproj.github.io/argo-cd/user-guide/auto_sync/ feature which covers We would have to make application CRD changes and serval code changes in api server/app controller: CRD changes Argo CD supports string parameters for helm, jsonnet (plugins parameters are WIP ). In addition to value we need to support valueFrom:
I think it is ok to add support only for helm first and jsonnet, plugins later. Code changes: Manifest generation is performed by The https://github.com/argoproj/argo-cd/blob/master/controller/state.go#L113 To support |
Thanks for the detailed info! I'm not familiar with go and the codebase, but I'll try to look into it and raise a PR |
@alexmt I've finally got a bit of time and started to look into this, but I need a bit of help Here a bunch of changes, but I have already few doubts
Do you have an architectural diagram of how client/server/api and all the moving parts are connected? Unfortunately I'm also a golang newbie and I'm having a bit of difficulty to understand how everything works Thanks in advance for the help 😊 |
Hello @niqdev , thank you for working on it!
|
Hey @niqdev |
Hi @patst, |
Hi, Just a heads up, I'm starting to tackle this using the same strategy as the helm-operator from WeaveWorks. Both secrets and configmaps would be supported. My proposal is to introduce something like valuesFrom:
- secretKeyRef: ...
- configMapKeyRef: .... Since we already have support for passing inline values, the idea is to resolve any configmap/secrets that need to be fetched and merge them with the inline values before passing it off to the repo server. By doing that, it keeps the actual code that needs to change minimal. Secrets/ConfigMaps would need to exist in the same namespace as ArgoCD which does present a problem; you could potentially reference other people's secrets/configmaps since Argo runs with the same service account. Working around that seems like it'd require adding another resource type to ArgoCD which could then use argo's RBAC, but I'd like to hear someone's opinion that has more experience here. Are we OK with the secret/configmap issue for an MVP with an explicit carveout that secrets prefixes with "argocd-" would be disallowed to avoid leaking argo-cd related secrets? |
See #1364. |
First off, I do understand the motivation why some may feel this feature is necessary. Many Helm charts have a fundamental problem which allows secret values / sensitive information to be held in values.yaml files, essentially making values.yaml files sensitive objects. However, it's not Argo CD's responsibility to solutionize on what I consider a bad pattern or serious problems with the helm charts. A well written chart should allow existing secrets to be referenced in the chart, rather than containing the sensitive information directly in the values.yaml. Here is a good example of this done properly: Retrieving values from configmaps (#1903) and secrets in the argocd control plane namespace is straying into the territory of config management, which is something we intentionally eschew from Argo CD. This proposal (and #1903) is essentially making Argo CD a centralized configuration/secret store, of which all applications can then reference secrets from. At this point, we would additionally need to implement RBAC rules on what applications are allowed to reference secrets.
This is a perfect illustration of the type of problems we face if we go down this route. It is a slippery slope. Today, we already allow users to use arbitrary secrets in the argocd namespace to hold cluster credentials. These secrets do not necessarily start with the prefix A second issue with these proposals is that it is introducing yet another source of truth of the manifests, which is different than git. Today Argo CD provides two sources of truth which affect the manifests generation:
Taking aside from the additional complexity of introducing another lookup as part of manifest generation, being able to reference configmaps and secrets in the argocd namespace, adds yet another source of truth is needs to be managed independently, which is antithetical to the GitOps model. Lastly, these configmaps and secrets which are stored in the argocd control plane, are only under the control of an argocd operator, and outside the control over an application developer. To reference these secrets/configmaps would require coordination between developers and operators, violating the separation of concerns. For these reasons, I do not believe this issue or #1903 are ones that Argo CD should pursue. |
The reason why this works for Helm Operator, but doesn't for Argo CD, is because these secrets are co-located with the HelmRelease instance, and are protected with the same Kubernetes namespace level RBAC. This means it is multi-tenant by default. Whereas in the case of Argo CD, the secrets are stored in the control plane namespace, not to mention mixed alongside Argo CD specific secrets, with zero RBAC protection and nothing stopping a developer from exposing the control plane secrets. |
@jessesuen So beyond referencing secrets in a chart where I actually agree that a correctly designed chart will allow you to do something like For example, I do not necessarily know the ARN of an IAM Role ahead of time if I'm trying to re-use the same repo to manage multiple clusters and need to reference that role either via kiam, or the new IAM for Service Accounts on EKS. My solution today has been to create a ConfigMap as part of Terraform that specifies just the parts that Terraform has knowledge of; the rest continues to be defined in git with the application. WDYT? |
@jessesuen thanks for your feedback, I 100% agree with
but even if it's not Argo CD's responsibility, I personally think that like everything there must be a compromise. In work we are using Argo CD managing the secrets with aws-secret-operator and IRSA using our own charts - no issue so far. So I'll give you the use case for why I've opened this issue... I want to run on my DigitalOcean cluster (which doesn't have any out of the box solution to manage secrets like SSM) a bunch of applications/MVPs/POCs with very low security concerns and unfortunately I've encountered this issue. Given this use case and the fact that I wanna still be 100% fully declarative using Argo CD, I would be happy to accept/allow security risks - if that's the concern - and I'm sure that other people would have other reasons or use cases. WDYT and what would be your solution in situations like this one? |
I don't think the discussion is closed on this yet, I think we're monitoring it because there is interest. @niqdev we document solutions here, do you want to add https://github.com/mumoshu/aws-secret-operator? https://argoproj.github.io/argo-cd/operator-manual/secret-management/ |
@alexec I'll create a PR for that |
@jessesuen Putting some of the ideas that we were throwing around at KubeCon in here so they don't get lost: Right now, the problem with the current proposal is that for multi-tenant solutions, it would be horribly insecure since applications would have access to config/secrets that they shouldn't necessarily have access to. One potential way to solve this could be to enhance the custom tooling/plugins that you can provide to ArgoCD by providing the entirety of the application manifest to it via some file (manifest.json/yaml) which would then allow the custom plugin to combine the values in the manifest itself with values sourced elsewhere (configmaps, secrets, etc..). That allows you to accomplish the proposal here, but punts on the security aspect by requiring the user to implement this themselves. One potential avenue that comes to mind is signing an app manifest, then using a plugin that verifies the provenance of your application before allowing it access to the requested secrets. The actual functionality that would be needed for this seems to be minimal on the ArgoCD side of things; basically, just provide the entire app manifest to the plugin either via a file or environment variable that the user can parse and do whatever they want with. We probably can/should then discuss how we extract some of the core functionality of Helm/Kustomize/etc.. out into plugins that can be iterated on independently of ArgoCD itself. |
Looking forward to have this functionality! |
@jessesuen @alexec Just circling back to this - would something like what was proposed above be something that you'd be interested in / would accept into ArgoCD? Basically, make the entire application manifest as either a file or environment var (not sure which atm) available to custom tools that can then parse it and do whatever they want with those values. In addition, we would re-order argo-cd/reposerver/repository/repository.go Lines 353 to 361 in 57eeaa4
That should solve my personal use case since I could easily do something like have my own helm plugin that does something like |
@yolkov what information are you adding to the secret? |
for example dns zone ID, endpoint of kubernetes apiservers, etc and some parameters have to be duplicated for both terraform and argocd applications. |
Is there a reason you couldn't create that secret and then use a mutating webhook or controller to inject that configuration on the cluster instead of injecting it via Argo CD? |
This requires that this webhook be already installed in the cluster. But according to our flow, we’ll put ArgoCD on bare k8s and it will deliver everything else(including this webhook). There are several "kind: Application", using webhook will need to get certain parameters from the secret and mutate this “Applications”. I won’t immediately say that this is possible at all, but even if it is possible, it’s not trivial, it’s not very reliable + "Application" resource is managed by gitops, we need to add exceptions so as not to overwrite resources from the Git repository. |
I might be confused. The secret containing the config information you need, where is that located? In the Argo CD cluster, or in the destination cluster?
My suggestion isn't to use a mutating webhook to modify the Application resource (certainly not if there are secrets to inject) but rather to use a mutating webhook to inject the information into the actual resources being deployed. Ultimately, cluster-specific information has to be placed somewhere. Either it's placed somewhere for Argo CD to read and then inject, or it's placed elsewhere for some other tool to read and inject. Placing the information on the destination cluster relieves Argo CD of the responsibility of gathering data from places besides git to apply to the cluster. There's significant value in limiting Argo CD's responsibility to applying declarative configuration sourced from git. In Kubernetes, the role of applying non-declarative information at apply-time is traditionally taken by mutating webhooks, and the role of applying non-declarative information at runtime is traditionally taken by controllers. By expanding Argo CD's scope of responsibility from "applying declarative configuration from git" to "gathering external information and injecting it into manifests" we make it difficult for Argo CD to do either job well. This is especially true with secrets, which require an entirely different level of care to handle properly as compared to typical declarative config. imo it's better to have two tools to each do their job well: Argo CD to apply declarative config from git, and webhooks/controllers to apply external information on the cluster. |
While looking to migrate from flux, we have a slightly different view on valuesFrom. |
At the moment we use this pattern to do it. https://github.com/gitops-bridge-dev/gitops-bridge So it is possible just not "natively" |
any chance this could be really looked at ? this ticket was created 5 years ago. |
@jgournet https://argo-cd.readthedocs.io/en/stable/operator-manual/secret-management/ There are no good use-cases for implementing this on ArgoCD side. I don't understand why devs are not closing this issue. |
@amkartashov : ok, I'll admit I'm not sure anymore now:
ie: our use case is:
|
It was noted multiple times that it's helm chart responsibility to use sensitive data from existing secrets. Your usecase is covered by:
|
On my end, I see your point @amkartashov but I don't really agree.
For example, looking at the wordpress helm chart, we see that to configure the DB, we have to specify:
```yaml
mariadb.enabled=false externalDatabase.host=myexternalhost externalDatabase.user=myuser externalDatabase.password=mypassword
```
Specifying and external secrets file couples the helm underlying structure with the deployment, which is not very neat: the API is the values, not how they are used behind the scenes.
In this case, we could have something like:
```yaml
valuesFrom:
- name: externalDatabase.user
value:
SecretRef: ...
```
Cheers
…On 20 September 2024 07:50:18 CEST, Andrey Kartashov ***@***.***> wrote:
@jgournet
> we then tell argocd: "hey, use those secrets for this helm chart"
It was noted multiple times that it's helm chart responsibility to use sensitive data from existing secrets. Your usecase is covered by:
* you inject secrets into the cluster via terraform, as kubernetes secrets, into application namespace
* you then tell argocd: "hey, set values.existingSecret to that nam for this helm chart"
--
Reply to this email directly or view it on GitHub:
#1786 (comment)
You are receiving this because you are subscribed to this thread.
Message ID: ***@***.***>
|
@Frankkkkk the wordpress chart does support This is exactly what was noted by amkartashov above that it is the helm chart's responsibility to accept sensitive data from existing secret. And if the helm chart doesn't support this basic pattern, it shouldn't be considered mature enough to be used in production. |
Yes indeed, maybe it was a bad example, but an example nevertheless.
For example what if the name of the `externalCache.host` is inside a secret?
Sure, some values can be provided directly, but you can't expect *all* values to always be provided with existing secrets.
Bottom line is that is a feature that interests people because they need it. For example, flux supports that with their `.spec.valuesFrom`. if you search GitHub for this you'll see that it is used and needed
Cheers
…On 20 September 2024 10:10:15 CEST, Dhruvang Makadia ***@***.***> wrote:
@Frankkkkk the [wordpress chart](https://artifacthub.io/packages/helm/bitnami/wordpress) does support `externalDatabase.existingSecret` with the doc `The name of an existing secret with database credentials` which should at least support password if not user & url.
This is exactly what was noted by amkartashov above that it is the helm chart's responsibility to accept sensitive data from existing secret. And if the helm chart doesn't support this basic pattern, it shouldn't be considered mature enough to be used in production.
--
Reply to this email directly or view it on GitHub:
#1786 (comment)
You are receiving this because you were mentioned.
Message ID: ***@***.***>
|
@Frankkkkk the point is there are no good examples, good enough to justify adding this complex (because of security concerns) functionality to ArgoCD. With flux it's different (from security pov) because HelmRelease references secrets in its own namespace (same as application namespace by default). With argocd (or any other tool syncing manifests from git to k8s) - you have multiple ways to use sensitive data from existing secrets. |
I'm genuinely interested in knowing how you would do the following: Imagine you have a wordpress deployment on AKS (basically a kubernetes on Azure). The memcached server lives outside k8s. The host is stored on an azure keyvault. Using the akv2k8s controller, we can get the value from the keyvault and store it into a secret Then, how to do setup the helm chart value BTW, the values included with PS: when 130+ people are liking this issue, I find it a shame to see how rudely dismissive you are of the feature request |
As far as I can tell, no one's being rude or dismissive, and tone doesnt carry well over text. The conversation seems to be basically about the tech, and tbh it's a good coversation. You're both really getting to the pros/cons. Let's please keep it about the tech. |
Saving host into values file in gitops repo takes the same amount of effort. Another bad example. |
Picking up on some previous examples, I would like to share our case. From what I see in this discussion, the request is for ArgoCD to provide the community with something as flexible as helmrelease/valuesFrom where we can fetch variables to inject into the ApplicationSets (or other places). As far as I understand it @amkartashov points out that this is not trivial to do in ArgoCD, as evaluation can happen in multiple contexts/locations, which is probably correct, and would probably means that this capability in ArgoCD would have to be discussed, and decisions taken on the exact implementation and restrictions. However, this is not the same as dismissing what so many people are requesting as irrelevant or unnecessary. Having this feature would not force anyone to use it, and would enable new users (and even existing users) to decide on their own on whether they prefer to current and probably more pure-gitops approach, or this arguably more dynamic approach, depending on their preferences and use-cases. I would be happy to provide feedback and support the @niqdev and others in submitting an improvement proposal (https://argo-cd.readthedocs.io/en/latest/developer-guide/code-contributions/) to bring this into Argo-CD. |
@joaocc you're right, this is not trivial. But not only this. We can avoid possible security issues if we allow to use secrets/configmaps only from application destination namespace (as flux does with HelmRelease). As @jessesuen wrote this is a major concern. Anyway, I believe that nothing is preventing to use data from secrets/configmaps using pure manifests (f.e. you can inject it into pods as env vars or files). And pure manifests can be stored in git and ArgoCD can sync it from git to cluster. |
@amkartashov it would be great if that was the reality, but unfortunately it is not. |
Only noticed #12050 now... Looks like an amazing first steps which would probably address a lot of the needs being requested here. |
Sometimes you don't have a choice |
IIUC, the secrets would be referenced in manifests after generation and be exposed on live objects (correct me if I'm wrong). Though it's not the same as being exposed in the manifests code, which more people can have access to. Also, IICU, secrets can rotate and we want to avoid changing the application every time. One question I have is what should we do if the secret was rotated, since app manifest won't change and won't trigger update automatically? |
Not sure if I understood the full implication of the question. However, I can share that the observable in flux (in equivalent scenario), where a secret/config map is used to create an overlay of values.yaml, changes are indeed picked up by driftDetection. Not sure how it is done internally though (just that it does work and is very useful :D) |
It would be nice to have direct support for native secrets in the Application definition.
In the following use case, when referencing a public chart, it would be very handy to have the possibility to reference a secret directly.
Resources related to
external-dns
to have more detailsDeployment
envThe text was updated successfully, but these errors were encountered: