feat: Combine files in a multisource repo (#12471 #12485)

[gczuczy]

Fixes #12471
Fixes #12476
Fixes #7189
Fixes #13220
Fixes #14521

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

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 included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• Optional. My organization is added to USERS.md.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).

[codecov]

Codecov Report

Attention: Patch coverage is 46.07330% with 206 lines in your changes are missing coverage. Please review.

Project coverage is 49.46%. Comparing base (2b6b9bf) to head (1b8cb9f).
Report is 6 commits behind head on master.

❗ Current head 1b8cb9f differs from pull request most recent head acf443f. Consider uploading reports for the commit acf443f to get more accurate results

Files	Patch %	Lines
reposerver/repository/repository.go	40.65%	156 Missing and 25 partials ⚠️
util/app/path/path.go	67.92%	13 Missing and 4 partials ⚠️
util/argo/argo.go	72.72%	4 Missing and 2 partials ⚠️
pkg/apis/application/v1alpha1/types.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12508      +/-   ##
==========================================
+ Coverage   49.45%   49.46%   +0.01%     
==========================================
  Files         273      271       -2     
  Lines       48662    48140     -522     
==========================================
- Hits        24066    23813     -253     
+ Misses      22235    21960     -275     
- Partials     2361     2367       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gczuczy]

There's a lot of code duplicated from repository.go:679 (valueFiles logic) to the new copyFrom logic. I think these should be refactored to call common functions. Also, they way that both parts are checking out repos and deferring to closer functions for cleanup individually is something I don't like the idea of. Probably repos should be checked out once, cached, and when it's time for the manifest generation released and cleaned up.

I think once the copyFrom logic is there and I both have a clearer view of how stuff works and I'm more comfortable with the logics here, I will see how can I clean this mess up.

And I'm also planning to address the LintGo failing test later.

[gczuczy]

I've got the following errors while running test-local:

# github.com/argoproj/argo-cd/v2/controller
controller/state.go:191:3: github.com/sirupsen/logrus.Debugf format %s has arg source of wrong type github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.ApplicationSource
(...)
# github.com/argoproj/argo-cd/v2/util/argo
util/argo/argo.go:417:14: fmt.Sprintf format %s has arg source of wrong type github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.ApplicationSource

And the String operator for ApplicationSources is defined at pkg/apis/application/v1alpha1/generated.pb.go:16086:

func (this *ApplicationSource) String() string {
  if this == nil {
    return "nil"
  }

IIRC I haven't fiddled with this part, and I'm not really sure what might have caused those test errors.

[gczuczy]

I think the PR looks feature-complete right now, unit tests are also passing. The place I couldn't find the tests for are the changes in the reposerver/repository/repository.go:runManifestGenAsync function, that's where the logic responsible for this change is called. Everything else should be covered by the unit tests.

Docs are still to be adjusted.

[gczuczy]

Docs are updated as well.

@ishitasequeira, could you please review it?

[ishitasequeira]

Docs are updated as well.

@ishitasequeira, could you please review it?

Will review it today.

[crenshaw-dev]

This is looking really cool!

We'll probably want some e2e tests to validate a few base use cases for this feature.

[ishitasequeira]

One thought that comes to my mind is, are we allowing a nested access of referenced sources for from? For example, something like below:

sources:
  - repoURL: http://my/repo.git
     targetRevision: master
     ref: values1
  - repoURL: http://my/repo-one.git
     targetRevision: 42.0.1
     ref: values2
     from:
       - ref: $values1
	 sourcePath: "dev/values.yaml"
          destinationPath: "env-values.yaml"
  - repoURL: http://my/repo-two.git
     targetRevision: 42.0.1
     from:
        - ref: $values2
	   sourcePath: "env-values.yaml"
           destinationPath: "dev/env-values.yaml"

I think this can lead to a lot of concerns. We should probably not allow ref and from in the same source. WDYT?

[gczuczy]

One thought that comes to my mind is, are we allowing a nested access of referenced sources for from? For example, something like below:

sources:
  - repoURL: http://my/repo.git
     targetRevision: master
     ref: values1
  - repoURL: http://my/repo-one.git
     targetRevision: 42.0.1
     ref: values2
     from:
       - ref: $values1
	 sourcePath: "dev/values.yaml"
          destinationPath: "env-values.yaml"
  - repoURL: http://my/repo-two.git
     targetRevision: 42.0.1
     from:
        - ref: $values2
	   sourcePath: "env-values.yaml"
           destinationPath: "dev/env-values.yaml"

I think this can lead to a lot of concerns. We should probably not allow ref and from in the same source. WDYT?

Currently that does make sense, however I'm considering another feature, where the output of a source can be referenced in another source building on this feature. Adding something like a saveOutput to the ApplicationSource, then being able to reference that both in the helm and from sections with something like $ref/$output. This with render the current mechanics into stage-like steps and interaction between them would be allowed. Moreover the builtin kustomize and helm processing facilities could coexists with standalone plugins. And right there disallowing having copy and ref at the same time would effectively block that feature.

I think, as of right now, putting a warning into the documentation is more effective.

Also, I think adding O_EXCL helps here with overwriting files:

O_EXCL Ensure  that this call creates the file: if this flag is specified in conjunction with O_CREAT, and pathname already exists, then open() fails with the error EEXIST```
This would solve accidental overwrites. Just tested this, and open indeed fails if the file already exists.

The question is, do we want to explicitly support transitive copies? I don't think that's something we should be factoring in.

[gczuczy]

I don't understand why the tests are broken, before i've synced the branch to master, everything was green.

[gczuczy]

I don't really understand why the e2e test TestNamespacedAppLogs is failing. I shouldn't have touched anything related to that test's codepath. Also, when I'm reproducing it locally it fails differently:

time="2023-03-08T12:51:45+01:00" level=error msg="`../../dist/argocd app create argocd-e2e-external/test-namespaced-app-logs --repo http://argocd-e2e-server:9081/argo-e2e/testdata.git --dest-server https://kubernetes.default.svc --path guestbook-logs --project default --dest-namespace argocd-e2e--test-namespaced-app-logs-hthfa --server 127.0.0.1:4443 --auth-token *** --insecure` failed exit status 20: time=\"2023-03-08T12:51:45+01:00\" level=fatal msg=\"rpc error: code = Unknown desc = namespace 'argocd-e2e-external' is not permitted\"" execID=ca1ee
    actions.go:195: failed expectation: error

And the most interesting part is, I've reverted to a commit prior to my changes, and it still fails the same way as shown above:

$ git status
HEAD detached at 9fb7847f
$ git log 9fb7847f | head -7
commit 9fb7847f4e054b05b222a6cfabe4cd821d90f6e5
Author: Noah Krause <--->
Date:   Tue Mar 7 16:03:50 2023 -0500

    fix: typo in doc link (#12744)

    Signed-off-by: Noah Krause <--->

[crenshaw-dev]

@gczuczy that test is flaky. I have an open PR to try to fix that.

[crenshaw-dev]

Partial review

[mircyb]

I'm eagerly waiting for the merge of this PR. Keep up the good work!

[gczuczy]

Here's a simplistic test example:

kubectl apply -f https://raw.githubusercontent.com/gczuczy/argocd-copyfrom-test/main/ArgoApp.yaml

This application has a kustomization in one repo, and one of the kustomization.yaml's inputs are in another repo, by environment. The involved repos are:
https://github.com/gczuczy/argocd-copyfrom-test
https://github.com/gczuczy/argocd-copyfrom-test-envs

There was another internal test, which is rather complicated, it deploys an ArgoCD ecosystem with a superchart, ArgoCD and ArgoWorkflows as a subchart, with a fair amount of secrets, for which we're using AVP to acquire. The gist of that secret are the following two snippets:

For the Application spec, here's the important part:

sources:
  - repoURL: 'https://valu.es/repo/sitory.git'
    targetRevision: master
    ref: valuesrepo
  - repoURL: 'https:/artifact.repo/testing'
    targetRevision: 0.42.24
    plugin:
      env:
        - name: VALUES
          value: env-values.yaml
    chart: our-argo-superchart
    from:
      - sourcePath: $valuesrepo/dev/values.yaml
        destinationPath: env-values.yaml

Here the chart has nil values for all the secrets, and the values repo's values.yaml has <placeholder>s all over the place. Even for the AVP credentials.

The environment-specific values.yaml is passed to the AVP plugin through the $ARGOCD_ENV_VALUES envvars, and the plugin definition has the following spec.generate.command:

      generate:
        command:
          - sh
          - "-c"
          - "/usr/local/bin/helm template $ARGOCD_APP_NAME -n $ARGOCD_APP_NAMESPACE --values $ARGOCD_ENV_VALUES . | /usr/local/bin/argocd-vault-plugin generate - -s namespace:avp-config-cm"

So, helm is using the environment-specific values from a separate repo and its output is fed to AVP, which is taking care of the <placeholder> replacements, and nicely adding the environment-specific secrets all over the place. This pattern deploys argocd itself in our internal testing environment.

[crenshaw-dev]

@3coma3

When we are able to compose those tools with multi-app features or patterns, pass values from Applications, extend lifecycle management, and so on, we are specifically choosing CD because it offers an advantage.

This is fair. I think there's a balance to be struck (e.g. pass things from Application to Helm in a way that takes advantage of an existing Helm feature instead of adding something entirely new, or implementing lifecycle hooks that map cleanly to Helm hooks). In general, I think it's best to cling tightly to other tools' designs and augment them only where there's a really strong value-add (like private values files with public Helm charts).

I agree that relying on AVP or deployment time injection for any and all secrets management is indeed abusing. But in my view, it is that abuse the anti-pattern, not the tool in itself.

100%, I think AVP is a great tool and definitely has solid use-cases. I agree that its overuse for problems which are better solved by other tools is the anti-pattern.

If you are using Helm charts directly from upstream and use the supported values-based customization, there might be or not a way to use secrets in a form that something like VSO/ESO might support, but that depends entirely on chart support. Again, this might lead to situations where the most straightforward solution was injection.

My hope is that, over time, common charts will rely more on secretRefs and less on values-injected secrets. At a higher level, I think operators instead of Helm charts to install many common tools might make sense.

A feature could be implemented to not cover for "any" case

Agreed. Some of the issue-specific threads discuss ways we could implement smaller features to solve very specific problems without opening the gates too far or making repo-server excessively complex (for the current maintainer resources).

@gczuczy

we're forced into a chicken-egg problem with setups where argo is managing remote clusters

The chicken-egg bootstrap problem is real. I agree that ideally a GitOps tool should be able to declaratively spin up/down your clusters with no user intervention. But someone has to hold the keys, and without native secrets support, I'm not sure Argo CD should be the tool holding the keys (yet).

the best would be if Argo team has sufficient core development capacity for extending ArgoCD's architecture to cover extended and more complex usecases

100%. In some parallel universe, there's an Argo CD team which can provide robust, secure, first-class secrets handling support. I just don't think we have that capacity right now.

@morremeyer

fileParameters

tbh, I think fileParameters is a great example of a one-off, small feature which may be worth the effort. It could probably piggy-back on a lot of the code that already exists for valueFiles.

Again, thanks all for the great discussion!

[morremeyer]

@crenshaw-dev Thank you! I'm going to take that input and might look into opening a PR in the future that might focus on fileParameters specifically to resolve #13220 without adding too much complexity.

[jkroepke]

Hi there,

thanks for the great discussion. As maintainer of helm-secrets, I would also not longer recommend todo secrets-injection outside of Kubernetes (doesn't matter, if you are using ArgoCD, Gitlab CI or GitHub Action).

However, FluxCD as well as ArgoCD side-kick project ArgoCD Vault Plugin are supporting this anti-pattern as well.

There is also an other problem with secret controllers: If the secret is changed, the application need a poke. This can be covered by https://github.com/stakater/Reloader. Then, you need 2 new tools the go out of the anti-pattern. I guess that the main reason, why people prefer this anti-pattern: The alternatives are complex, not easy to maintain.

Anyways, while the main purpose of helm-secrets is secret inject, It can be also used for alternative informations.

If we are looking are AWS IRSA or Azure Workload Identity, both solution are great to omit secrets at all.

Instead secrets, now we have the requirement to annotate the service account with the AWS Role ARN or Azure Client ID. In most setups, the role is created via Terraform, and some poor guy has the task to pass the information from terraform output to the helm values file. There is a information gab between Terraform Deployment and GitOps Deployment.

Unlike secrets, annotations are not a dedicated resource and there no known Kubernetes controller for this.

This includes external-secrets-operator which needs for example the client id of Azure Managed Identity.

Keep in mind. Secret Handling is only one part, Information Handling the other one.

[crenshaw-dev]

Keep in mind. Secret Handling is only one part, Information Handling the other one.

True, and that's the main reason I wanted to do a quick poll - non-secret CMP use cases don't implicate the secrets anti-pattern. But I think the number of those use cases is currently too small to justify the maintenance burden.

I also wonder how many of those use cases might be addressed by mutating webhooks? I haven't thought too much about it, but if the job is fundamentally to inject some static cluster-specific bit of information into resources, it seems like a mutating webhook might be a suitable tool.

[morremeyer]

For what it’s worth, we use the approach @jkroepke mentioned:

• external-secrets operator for secrets from Vault
• reloader to restart Pods when referenced resources change

While it’s true that this needs two tools, the maintenance overhead for both these is extremely low (at least for us), and, most importantly, reloader is completely decoupled from the deployment tooling.

This enables us to make debugging in our development clusters much easier since reloader will also restart workloads when a referenced ConfigMap or Secret changes because we manually edited it to later persist these changes, if they are correct.

I think it is important here that ArgoCD will almost never be the only tool, so we’ll have to take that into consideration when making decisions about the project.

„One tool, one job“ comes to mind, and for me, Argo is clearly the „deploy manifests, potentially templated with helm and/or kustomize“.

As such, supporting the usage of existing features of these tools is good, adding to them might not be the right direction.

I believe this is also what @crenshaw-dev is leaning towards - please correct me if I got that wrong.

[jkroepke]

I also wonder how many of those use cases might be addressed by mutating webhooks?

That would be a great and tool agnostic idea.

The gag is, both IRSA and Azure Workload Identity are mutating webhooks already. Of course, we can build an other mutating webhooks controller and chain them together.

[volk1234]

@crenshaw-dev
Thank you for detailed explanation!

Naming AVP approach "anti-pattern" you contradicted with this:

Argo CD is un-opinionated about how secrets are managed. There are many ways to do it, and there's no one-size-fits-all solution.

In my case we have dozen of clusters running external and have no access to Vault - therefore we cannot use operators. Approach with encrypting helm value files is looking ugly for me, not saying about probability of accidentally commit decrypted file to a public repository.

But I believe this feature is not about secrets at all.
Right now we can use this "anti-pattern" through the workaround with template secrets inside the AVP plugin helm template $ARGOCD_APP_NAME --include-crds -n $ARGOCD_APP_NAMESPACE $${ARGOCD_ENV_HELM_ARGS} . So its already works in ArgoCD.

I would vote to have this feature. As for complexity of "repo-server" I'd say it always the question of bаlance. From the other hand - why that change don't add complexity but this do?

[crenshaw-dev]

„One tool, one job“ comes to mind, and for me, Argo is clearly the „deploy manifests, potentially templated with helm and/or kustomize“.

Yep, that's a good summary. Not to say Argo CD can't be a good tool for doing things beyond "render manifests and deploy," just that it's always a question of whether the juice is worth the squeeze.

Naming AVP approach "anti-pattern" you contradicted with this:

Argo CD is un-opinionated about how secrets are managed.

Personally, I think those docs are due for an update. My opinion about secret management is that it belongs in either an encrypted-at-rest solution like SealedSecrets or an on-cluster secrets operator rather than in Argo CD. At least for now.

I would vote to have this feature. As for complexity of "repo-server" I'd say it always the question of bаlance. From the other hand - why that #14893 don't add complexity but this do?

iirc the templatePatch AppSet feature didn't actually touch the repo-server. But point taken, every feature comes at a maintenance cost, which must be weighed against the benefit. And again, I could be totally wrong about the cost/benefit analysis here. I'm super aware of the cost (having spent a lot of time in repo-server code lately). I'm less well-equipped to estimate that benefits, since I'm only familiar with a small subset of Kubernetes stacks (Intuit's and those of my past employers). So I'm relying on polls and thumbs-ups and conversations to try to get a rough idea how badly the feature is needed and for what use cases.

[grosenba]

What capabilities does our plugin have today:

• Setting variable values in all yaml and json files. The values come from labels located in the cluster-config in ArgoCD. This enables dynamic configuration.
• Loading our helm value files from other git repos
• load extra kustomize from other git repos
• combine helm and kustomize manifest
• Load secret from Vault

Issues:

• Plugins do not support multisource -> we have to clone everything that comes from gitlab and is outside the app definition for kustomize in the plugin - for helm, the values and additional kustomize descriptions must always be loaded from gitlab. -> To load it from gitlab, we need access to the secrets. These are currently in the plugin container. Which is a real pain. I can certainly be loaded via a k8s secret, but changes cannot be detected anyway.
• Since it's not multisourceable, we need to make sure we tell ArgoCD that something has changed in the application and ArgoCD needs to reload everything. Comparable to a hard refresh. In case of an update this has to be done by itself and if there is no significant change to the app there is no automatic update.

[alexgenon]

Hi all,
We're also facing the limitation reported in #13220. That is, being able to use in Helm the other source repo not only for valuesFiles param but also for other parameters such as fileParameters.
The overall solution would be more coherent (it seems counter-intuitive that the ref resolution is only applied to the  valuesFiles parameter) and it would avoid the need for umbrella charts just to do basic wiring.

I understand that the proposal from this PR provides more features than this simple ask and could lead to many anti-patterns, or prevent the argoCD community to migrate to cleaner approaches. But we should probably consider some of the use cases as there might be quick wins,

Thank you for the hard work! As argoCD users, we really appreciate the discussions and debates that are leading to a better solution.

[ntaylor1781]

I get the thought process on this being an anti-pattern. I think that makes perfect sense when you control the creation of the helm/kustomize. This gets more tricky when you are not in control. We currently only use ArgoCD to handle deploying supporting infrastructure, and not our apps. This means we are restricted by what is done in the upstream helm/kustomize. Not all projects follow best practices. I found 3 different projects, we use, that store secrets in ConfigMaps. Currently we are getting by without this PR (though it would be nice to have). If AVP is ruled an anti pattern, and not supported moving forward, it would create a lot of work for my team if we wanted to continue using ArgoCD.

I believe I would be able to open PRs to upstream projects to hopefully update their projects, but if those got denied it would mean maintaining our own copies of their projects and keeping up with their updates. That would introduce a lot of work, and may cause us to look for a solution other than ArgoCD.

If that was the case it may be that our use case is just not the intended use case for ArgoCD, and that would be fine. If the intended use case for ArgoCD is to handle upstream helm/kustomize, and not just custom, I think that is worth thinking about.

All you great folks do the work though, and know what you can/can't take on. I just wanted to share my view point, as I feel that the others didn't share it exactly. I am super grateful for all the work put into this project.

[crenshaw-dev]

being able to use in Helm the other source repo not only for valuesFiles param but also for other parameters such as fileParameters.

@alexgenon I agree, that feature would be a relatively intuitive, maintainable, and impactful feature. I'd definitely welcome that change!

This means we are restricted by what is done in the upstream helm/kustomize. Not all projects follow best practices.

100%. I wonder if the problem is more that these open source charts/kustomize bases can't support secret refs (not enough maintenance resources) or don't actually see it as a problem.

I found 3 different projects, we use, that store secrets in ConfigMaps.

I think that's just a general Kubernetes anti-pattern. Secrets should be stored in Secrets.

If AVP is ruled an anti pattern, and not supported moving forward, it would create a lot of work for my team if we wanted to continue using ArgoCD.... may cause us to look for a solution other than ArgoCD.

That's a fair analysis. In my opinion, "safe, robust secrets injection" lies outside what the Argo CD team can currently offer. To provide a strong core product, I think we should encourage folks to adopt the much more compatible solution of external secrets operators. Some won't be able to make that switch, but I don't think that's a problem the Argo team is currently well-positioned to solve.

[kingbj940429]

Hi guys

this was indeed a great discussion. As a argocd user, I'm curious about one thing: what happens next?

[newbenji]

Hi guys

this was indeed a great discussion. As a argocd user, I'm curious about one thing: what happens next?

consider if going for alternative og live with the decision

[av-sebastian]

ArgoCD 2.11: This comes with something that looks similar to the feature discussed here. - https://argo-cd.readthedocs.io/en/latest/user-guide/multiple_sources/
So there might come something good out of the wait time here :-)

[joebowbeer]

ArgoCD 2.11: This comes with something that looks similar to the feature discussed here. - https://argo-cd.readthedocs.io/en/latest/user-guide/multiple_sources/ So there might come something good out of the wait time here :-)

The multi-source docs haven't changed appreciably since v2.8

AFAICT, this feature is adding a from section

     from:
       - ref: $values1
         sourcePath: "dev/values.yaml"
         destinationPath: "env-values.yaml"

[gczuczy]

ArgoCD 2.11: This comes with something that looks similar to the feature discussed here. - https://argo-cd.readthedocs.io/en/latest/user-guide/multiple_sources/ So there might come something good out of the wait time here :-)

This PR would have expanded the multi-source support. The problem with multi-source is, it's not working with plugins, and many people need plugins and multisources, that's how this PR was born. Our usecase was AVP for secrets specifically.

[reachfivejenkins]

Hello, I ended up here after noticing multisource isn't compatible with helm-secrets. What's the point to have multisource, if no plugins can be used to decrypt values ? Who stores it sensible values unencrypted ?
So as of now, there is absolutly no way to do that ? That is a very common scenario, loading a versioned chart from repo, and encrypted values from another... I love argocd a lot but that restriction may oblige me to look for another solution

[morremeyer]

@reachfivejenkins There‘s other ways to deploy secrets, e.g. with external-secrets.

The point of multi-source as it is is to provide multi source applications to whoever can use them in the current state.

[Obirah]

Hello, I ended up here after noticing multisource isn't compatible with helm-secrets. What's the point to have multisource, if no plugins can be used to decrypt values ? Who stores it sensible values unencrypted ? So as of now, there is absolutly no way to do that ? That is a very common scenario, loading a versioned chart from repo, and encrypted values from another... I love argocd a lot but that restriction may oblige me to look for another solution

Been using this configuration for helm-secrets and multi-source for quite some time and it's working nicely for us.

[stephaneetje]

Hello, I ended up here after noticing multisource isn't compatible with helm-secrets. What's the point to have multisource, if no plugins can be used to decrypt values ? Who stores it sensible values unencrypted ? So as of now, there is absolutly no way to do that ? That is a very common scenario, loading a versioned chart from repo, and encrypted values from another... I love argocd a lot but that restriction may oblige me to look for another solution

Been using this configuration for helm-secrets and multi-source for quite some time and it's working nicely for us.

I looked at it, but we are using SOPS with googleKMS, i doesn't look compatible.
I have a helm repo + sops encrypted values github repo, we just moved from monorepo as we wanted to take advantage of multi source, but indeed multi source can be used here on very minor use cases as it can't load any plugins...

[morremeyer]

Could you please take this discussion to the appropriate forum/Q&A/discussion?

This is already a PR with lots of on-topic discussion that only gets harder to parse with more and more unrelated comments.

Thank you!