fix(helm): handle templated namespaces consistently

[aaron-prindle]

fixes #6749

Currently if there is a templated namespace with helm that does not currently exist in the cluster, skaffold will fail even though a regular deploy would succeed. As mentioned in #6749, this is because currently skaffold fetches the namespaces before we template which results in this failure. The fix here is to template the helm namespaces before fetching them.

[codecov]

Codecov Report

Merging #6767 (8aadfd0) into main (290280e) will decrease coverage by 0.93%.
The diff coverage is 60.86%.

❗ Current head 8aadfd0 differs from pull request most recent head c268445. Consider uploading reports for the commit c268445 to get more accurate results

@@            Coverage Diff             @@
##             main    #6767      +/-   ##
==========================================
- Coverage   70.48%   69.54%   -0.94%     
==========================================
  Files         515      536      +21     
  Lines       23150    24419    +1269     
==========================================
+ Hits        16317    16982     +665     
- Misses       5776     6308     +532     
- Partials     1057     1129      +72     

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/flags.go	89.00% <ø> (-1.82%)	⬇️
cmd/skaffold/skaffold.go	0.00% <ø> (ø)
cmd/skaffold/app/cmd/lint.go	52.94% <52.94%> (ø)
cmd/skaffold/app/cmd/cmd.go	70.49% <75.00%> (-0.57%)	⬇️
cmd/skaffold/app/cmd/debug.go	100.00% <100.00%> (ø)
cmd/skaffold/app/cmd/runner.go	64.17% <100.00%> (ø)
pkg/skaffold/initializer/build/builders.go	42.85% <0.00%> (-17.15%)	⬇️
pkg/skaffold/build/cluster/logs.go	0.00% <0.00%> (-16.67%)	⬇️
pkg/skaffold/event/v2/status_check.go	85.45% <0.00%> (-14.55%)	⬇️
pkg/skaffold/kubernetes/watcher.go	67.10% <0.00%> (-14.53%)	⬇️
... and 122 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93e9a23...c268445. Read the comment docs.

[briandealwis]

Is there a way to test this?

[tejal29]

Is there a way to test this?

Either some unit tests or manual testing notes could help.

[briandealwis]

This should use the same approach as in releaseNamespace(), and it should surface any errors back up.
https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/deploy/helm/util.go#L159-L164

[briandealwis]

Some unit tests of collectHelmReleasesNamespaces() looks easy enough?

[aaron-prindle]

I've added the suggested changes and unit test for collectHelmReleasesNamespaces. I'm a bit confused though as to the suggested code change made. The suggestion was to reference logic at https://github.com/GoogleContainerTools/skaffold/blob/main/pkg/skaffold/deploy/helm/util.go#L159-L164. I just wanted to verify that the logic currently:

			for _, release := range cfg.Deploy.HelmDeploy.Releases {
				if release.Namespace != "" {
					templatedNamespace, err := util.ExpandEnvTemplateOrFail(release.Namespace, nil)
					if err != nil {
						return []string{}, fmt.Errorf("cannot parse the release namespace template: %w", err)
					}
					namespaces = append(namespaces, templatedNamespace)
				}
			}

is acceptable as theree was a suggested edit made that was essentially:

					if err != nil {
						namespaces = append(namespaces, release.Namespace)
                                                // which is now vvvv
						// return []string{}, fmt.Errorf("cannot parse the release namespace template: %w", err)
					}

but I'm a bit confused as this is what I believe should be an error case given the context. Thanks.

[briandealwis]

I was wrong with my comment. It happens more often than I'd like :-)

The important thing is that we treat inputs uniformly. It would seem to make sense to have collectHelmReleasesNamespaces() and Deployer.releaseNamespace() using the same function?

[aaron-prindle]

I'm not sure I understand, I believe the logic for namespace gathering from pipelines done in collectcollectHelmReleasesNamespaces is fundamentally different than the logic for Deployer.releaseNamespace. Reverting to the deployer namespace is only done in Deployer.releaseNamespace (code here). IIUC, the only shared logic would be something like:

func expandNamespace(ns string) (string, error) {
	expandedNs, err := util.ExpandEnvTemplateOrFail(r.Namespace, nil)
	if err != nil {
		return "", fmt.Errorf("cannot parse the release namespace template: %w", err)
	}
	return expandedNs, nil
}

which is essentially util.ExpandEnvTemplateOrFail

[aaron-prindle]

Currently I believe the implementation and tests are sufficient, was there any concern around better re-use of logic for Deployer.releaseNamespace and collectHelmReleasesNamespaces? From my comment above, I don't believe there is a worthwhile amount of logic to reuse but I may have missed something.

[briandealwis]

We have util.Expand* calls in a few places in the code now and pulling it into a single place might reduce the likelihood of errors, like this issue, in the future. I mean, who would expect to find Helm-specific code outside of deploy/helm?