Helm lookup function support - enhancement proposal

[kchaber]

Summary

This is a proposal to support helm lookup function, which lack has already been reported and discussed in #5202.

Motivation

The helm lookup function is widely used by chart maintainers to manage passwords/certificates generation, in backward compatibility converters and auto component migrations. All of these actions requires helm to contact (aka look up) the destination cluster before rendering the final list of kuberenetes resources.

ArgoCD should allow using this feature as it is contained in the standard helm functions list. Based on the linked issue #5202, it looks like lack of helm lookup support affects lot of people & projects.

There are two known workarounds (1, 2) posted, however they will work in the specific ArgoCD setups and will NOT work in the multi-tenant/multi-cluster or non-k8s hosted instances, which drastically reduces number of use cases, where such workarounds can be applied.

Proposal

As per helm docs, the lookup functionality can be enabled by specifing the --dry-run=server option to the helm command.
This will be translated to the argocd CLI:

argocd app set <helm-app> --helm-server-dry-run

or using declarative syntax:

spec:
  source:
    helm:
      serverDryRun: true

The default value of --helm-server-dry-run will be false to be consistent and backward compatible with previous releases.

Setting --helm-server-dry-run will add --dry-run=server option to the helm template command running on the repo-server during manifest generation.

Another important point is that helm template --dry-run=server requires the connection to the k8s cluster and this will be provided with the --kubeconfig or with -kube-(apiserver|as-group|as-user|ca-file|context|insecure-skip-tls-verify|tls-server-name|token) options.
This configuration will be automatically generated and provided to helm based on the spec.destination cluster details.

This solution, in contrast to the mentioned workarounds, will support multi-tenant/multi-cluster setups. Everything will work "locally" within Application manifest and repo-server will gain the same privileges (and no more) as the application-server for sync to that specific destination.

[kchaber]

I could try with providing the appropriate PR once this proposal gets accepted.

[crenshaw-dev]

Over time I've become more opposed to lookup support. I think that manifest hydration should be deterministic and that runtime concerns should be managed by controllers at runtime.

However, I'm open to exploring the possible design here.

Here are my concerns:

1. How do we protect the kubeconfig and associated sensitive files? The repo-server has proven difficult to protect from directory traversal attacks. How do we prevent one hydration process from stealing credentials from a concurrent process?
2. How do we protect manifests that contain plaintext secrets? We currently cache unencrypted manifests in Redis, and we leave the repo-server service accessible without auth from within the cluster. Since no first-class Argo CD features currently populate Secrets in manifests, that's relatively okay. This proposal would make secrets population a first-class feature and would merit heightened protection.
3. How do we keep lookups from accessing things they shouldn't access? By default, application-controller is granted full access to destination clusters. Should we disable lookup unless you're using service account impersonation?

[kchaber]

I think that executing lookup function against same cluster over and over, which basically is what ArgoCD does during sync, will be quite deterministic once we suppose everything is done through ArgoCD. It's the lack of this support actually causes non-deterministic behaviour when chart is composed with lookup executions to prevent random data regeneration.

Regarding mentioned concerns:

1. We don't need to create any temporary files and can easily go with passing the cluster credentials directly as a helm command options (-kube-[apiserver|as-group|as-user|ca-file|context|insecure-skip-tls-verify|tls-server-name|token]), which should not cause any problems with the concurrent execution. I think the cluster credentials should be managed here similarly and analogically to RepoCreds.

2. I don't think that lookup support will change things in this matter. You can have plain text values filled directly inside helm values files or even set via argocd app set -p , so they will be present in the generated manifests in the same way as extracted after processing the lookup function.

3. We could use account impersonation or even introduce more granular control and allow specifying appropriate service account under helm.serverDryRun.serviceAccount config option. I would let the user decide whether more restricted SA for helm lookup is needed or can be executed with the default destination cluster config - everything depends on the specific use case/deployment/etc.

[ergoxi]

consider presenting a warning when a chart uses lookup