feat(helm): add ability for --dry-run to do lookup functions

[tapaskapadia]

When a helm command is run with the --dry-run flag, it will try to connect to the cluster
to be able to render lookup functions.
Closes #8137

Signed-off-by: Tapas Kapadia tapaskapadia10@gmail.com

What this PR does / why we need it: It is hard to debug the lookup function and currently there is not a good way to test it with any flags. #8137 Stated that the --dry-run was fair game to try to implement this logic as long as the helm template logic stays the same.

Special notes for your reviewer: This is my first PR for the Helm; please let me know if I need to add or change anything.

If applicable:

• this PR contains documentation
• this PR contains unit tests
• this PR has been tested for backwards compatibility

[tapaskapadia]

FYI, I believe this test is not testing what it states in the current master branch. I think it will pass regardless of the DryRun field being sent to "true" or "false". https://github.com/helm/helm/blob/master/pkg/action/install_test.go#L245

I have attached a document of a sample of the basic tests I ran. I did not see any good way to positively test a successful lookup without using a live cluster.

basicTests.txt

[DanielHabenicht]

Any updates on this? I would support if something is missing.

[iTaybb]

I'm also looking forward for this, and wish to help if any help is needed.

[rickardp]

@tapaskapadia Can you fix the merge conflicts? Or if you let me, I'm happy to do so. I can help out with testing if needed.

[rickardp]

This change is much more in line with user expectations, IMHO. Specifically, the sentence saying "when the user says to dry run, respect the user's wishes and do not connect to the cluster" is not in any way implied by naming convention or any best practices. Typically, dry run an operations means "do not mutate" not "do not read". Are there any documentation to update as well?

[tapaskapadia]

Good call on updating the documentation: https://helm.sh/docs/chart_template_guide/functions_and_pipelines/#using-the-lookup-function
This page does reference that helm install --dry-run is not supposed to contact the cluster.
If this is approved this page will also need to be changed: https://github.com/helm/helm-www/blob/2875cecd9b1b8b6290e9fee595e288807f033eb5/content/ko/docs/chart_template_guide/functions_and_pipelines.md (for all the languages)

[tapaskapadia]

@tapaskapadia Can you fix the merge conflicts? Or if you let me, I'm happy to do so. I can help out with testing if needed.

I'll rebase it to fix the merge conflicts. I may need a little help getting started if unit test cases are required as when I initially took at a stab at this it was not clear to me how to simulate having a real connection.

[mumoshu]

FWIW, helm-diff needs this, and I prefer this over changing helm template to have cluster access.

databus23/helm-diff#263

[joejulian]

I would prefer we not do this in this fashion, but instead do what kubectl does with --dry-run=<server|client>. I think an interim change to just default to --dry-run=client if --dry-run is specified without qualification could be sufficient to keep it from being considered a breaking change, then go ahead and open up the flood gates on server connections with --dry-run=server.

Unfortunately, dry-run already broke the client-only contract. I'd still be against changing the comments to make it seem ok to do that further, especially if we're going to want to walk that back in helm 4.

[beekhof]

I found myself nodding furiously to this comment from @rickardp

Specifically, the sentence saying "when the user says to dry run, respect the user's wishes and do not connect to the cluster" is not in any way implied by naming convention or any best practices. Typically, dry run an operations means "do not mutate" not "do not read".

That this topic is raised over and over again is surely a sign that dry-run must imply a client-only contract is not what everyone wants, because it makes Helm less useful for some of us, and our lives harder.

@joejulian 's --dry-run=<server|client> suggestion seems pretty reasonable compromise.

• Are there key folks that remain to be convinced before such an approach could be merged?
• Is someone already working on it, or are we waiting for volunteers? (I'd be happy to do so).

[ksa-real]

+1 on that one. I had to re-implement deployment scripts to do necessary lookups outside of helm upgrade --dry-run and then supply retrieved values via a temp file. My use of dry-run is to do a preflight check and helm diff with the already deployed release. At the moment not having lookup in -dry-mode generates too much noise and makes this mode too hard to use (without my enhancements).

[BrandonArp]

What is needed to get this merged? It looks like it has support from the community and, personally, I would really like to see it.

[v-sag]

Please support this.

[QuentinN42]

Any updates about the needed work to merge it ?
I have some time to pass some commits :)

[alexzhc]

ugently needed 👍🏻

[jgreat]

Can I ask a question here? Would this feature be more likely to be approved and merged if it wasn't tied to the --dry-run flag?

I can't speak for everyone, but what I'm looking for is the ability to do a full test run with connections to the remote cluster so lookups work. These cluster connections would run under the user's approved/provisioned security context, so there doesn't seem to be a security problem or potential to expose something that the user doesn't already have access to via kubectl.

[QuentinN42]

Looking at the k8s docs of the dry-run :

The request is still processed as typical request: the fields are defaulted, the object is validated, it goes through the validation admission chain, and through the mutating admission chain, and then the final object is returned to the user as it normally would, without being persisted.

So kubectl notion of dry-run is "I do all the stuff as normal, but at the last moment I do not apply changes".

I think helm upgrade with a --dry-run must do the same, lookups are executed, connection to the cluster is tested, and objects are validated through the k8s API.

If you want to NOT connect to the cluster, why not using helm template ?

[joejulian]

Looking at kubectl help:

    --dry-run='none':
	Must be "none", "server", or "client". If client strategy, only print the object that
	would be sent, without sending it. If server strategy, submit server-side request without
	persisting the resource.

If you don't specify, it suggests "client" which has always been the default:

W0919 10:06:16.181516 1786127 helpers.go:639] --dry-run is deprecated and can be replaced with --dry-run=client.

I'm suggesting helm would do the same.

[codablock]

+1 for this as I'm using the template functionality of Helm to integrate Helm into kluctl. I'm however not calling Helm binaries but instead use Helm as a go dependency and configure an install action so that it uses DryRun=true.

I'd like to ask that whatever the final solution will be to allow lookup+dryRun, that this is also possible when Helm is used as a library and not only when it is used as a binary.

[mikeshng]

+1 on this PR or any other PRs that can enhance the dry-run capability to perform lookup.

[QuentinN42]

@rickardp what is needed to merge this ?
@tapaskapadia are you still working on this ?

If needed I can help by creating a new MR with all this changes on the last version of helm:main.

[gjenkins8]

I second @joejulian said about --dry-run='none'|'client'|'server'. The PR here (as it stands) is a change in behavior.

Currently helm install --dry-run=true does not contact the cluster, and to the point of the security advisory/model (GHSA-q8q8-93cv-v6h8), I think would be a breaking change for some users (ie. they could suddenly get secrets printed and logged in a CI system; see e.g. #7275)

I put a proposal on the main issue (rather than this implementation PR here

[sandipchitale]

What about Capabilities.APIVersions and Capabilities.KubeVersion? Will this also work once lookup will work when correct flag is passed?

[crenshaw-dev]

@mattfarina is your review stale / should be dismissed?

[mattfarina]

I think this is unreachable code. With NoOptDefVal being set you shouldn't run into a value of "" here.

[dudicoco]

@tapaskapadia thank you for this important contribution!

[pwillis-eiq]

Thank you @tapaskapadia, @rickardp, @joejulian, @QuentinN42, @mattfarina, and everyone else who helped push this through!

This feature change took 2 years, 4 months and 20 days to merge! But it was finally completed - by a bunch of strangers on the internet, all working together, with no monetary incentive, contractual agreement, or under any hierarchical organization chart. Because of all this, a bunch of other strangers will now benefit - whether it's supporting a business, helping customers, just getting their day jobs done, or working on the tools they care about. Open Source lifts all boats ❤️ ⛵ ❤️