helm install fails when empty file or folder of same name as chart present in working dir

[alexsomesan]

Output of helm version:

version.BuildInfo{
Version:"v3.14.4", GitCommit:"81c902a123462fd4052bc5e9aa9c513c4c8fc142", 
GitTreeState:"clean", GoVersion:"go1.22.2" }

Output of kubectl version:

Client Version: v1.29.2
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: v1.29.2

Cloud Provider/Platform (AKS, GKE, Minikube etc.): kind v0.22.0 go1.21.7 darwin/arm64

Summary

Helm fails to install a chart when repository is specified on the command line with --repo and a file or directory of the same name as the chart is present in the local working directory.

Steps to reproduce

• Create a local file (or directory) of the same name as the chart: touch hello (or mkdir hello)
• Run helm install passing the repository via --repo so that chart name does include have the [repo-name]/ prefix
• Observe error. There will be different errors depending on whether a local file or a directory matches the chart name.

➜  helm-locate-chart touch hello
➜  helm-locate-chart ll
total 0
-rw-r--r--  1 alex  staff     0B May  6 22:42 hello

➜  helm-locate-chart helm install --repo https://cloudecho.github.io/charts/ hello hello
Error: INSTALLATION FAILED: file '/Users/alex/helm-locate-chart/hello' does not appear to be a gzipped archive; got 'application/octet-stream'

OR

➜  helm-locate-chart mkdir hello
➜  helm-locate-chart ll
total 0
drwxr-xr-x  2 alex  staff    64B May  7 13:33 hello
➜  helm-locate-chart helm install --repo https://cloudecho.github.io/charts/ hello hello
Error: INSTALLATION FAILED: Chart.yaml file is missing

Root cause analysis

The issue can be traced back to a check for the presence of a local filesystem entry of the same name as the chart (through os.Stat()) as a first step in locating the chart to be installed, in func (c *ChartPathOptions) LocateChart(...):

	if _, err := os.Stat(name); err == nil {
		abs, err := filepath.Abs(name)
		if err != nil {
			return abs, err
		}
		if c.Verify {
			if _, err := downloader.VerifyChart(abs, c.Keyring); err != nil {
				return "", err
			}
		}
		return abs, nil
	}

In the most common use-case, the user will invoke the install command with the chart name including the repository prefix, separated by a slash:

helm install hello cloudecho/hello

which just happens to not be a valid name for a local filesystem entry, and thus avoids causing a false positive in os.Stat(). However, no explicit validation exists to confirm the presence of a local chart.

When the CLI command includes the repository URL via the --repo argument, the chart name no longer includes the repo prefix and slash that was preventing it from matching a local FS entry. The conditions are thus created for the above mentioned installation error to take place.

Possible solutions

This code was originally intended to detect the presence of a local copy of the chart to be installed. However, the os.Stat() check is too generic and will yield false positive matches even when a simple empty file or directory of the given name is present, which is obviously not the intended behaviour.

This has been reported in various forms in the past which sparked conversations on wether a local chart should even be picked up when the command line explicitly requests one from a remote repository. It was pointed out that the current behaviour (of prioritising the local copy) needs to be preserved until the next major version of Helm.

However, here I am reporting an extreme manifestation of this issue which actually leads to install failure and should therefore be addressed as a bug within the confines of backward compatibility with the original intent.

A simple solution that would preserve the status quo while eliminating the error edge case would be to replace the os.Stat() with a more exhaustive check that determines with increased certainty if the local FS entry is indeed a valid chart, be it a directory or archive.

PR #13009 implements this proposal.

[gjenkins8]

Marking this as a feature, as the behavior was designed this way (not saying it is the best way).

replace the os.Stat() with a more exhaustive check that determines with increased certainty if the local FS entry is indeed a valid chart, be it a directory or archive.

I think perhaps the behavior can be improved (as a non-breaking change). But not to the extent that a FS entry must be "valid" for it to be considered. e.g. If I downloaded a corrupt archive, I would expect attempting to install of that corrupt archive from the FS to fail. Not e.g. ignore and proceed to lookup a repository because the chart (archive) is not valid. So any introduced checks would need to be very specific and intuitive (for all scenarios: otherwise the next user hits a different use case which is confusing for them).

And overall, while the behavior could be improved. It would need to be demonstrated it is worth it. To a certain extent, the "fail fast" behavior, and simple implementation of os.Stat(name) has benefits. But if there is a nice UX win out there, that would be nice.