Ignore "" namespaces in collectHelmReleasesNamespaces

[sarmad-abualkaz]

Fixes: #3311
Related: N/A
Merge before/after: N/A

Description

This fixes an issue specific to helm deployments , when the skaffold enduser is running with RBAC restriction scoped to a single namespace.

Behaviour on v1.12.1 - pod watcher logs this error starting logger: initializing pod watcher for "": unknown (get pods) and exits.

This also seems to, inadvertently 😄 , fix a new issue (potentially introduced as part of the fixes in this PR - #4460) whereby user with similar restrictions, single-ns scoped permissions, cannot list helm-released deployments anymore despite having SKAFFOLD_NAMESPACE env-variable or adding --namepsace/-n flag. This other issue logs the error below before exiting:

exiting dev mode because first deploy failed: could not fetch deployments: could not fetch deployments: deployments.apps is forbidden: User "<my-user>" cannot list resource "deployments" in API group "apps" at the cluster scope

The only work around for both these issue is to hardcode a namespace field in skaffold.yaml under deploy.helm.releases.

User facing changes (remove if N/A)

Before:
Running skaffold dev with a user permitted to only a single namespace would run into this error before exiting:
starting logger: initializing pod watcher for "": unknown (get pods).

The cause for this error appears related to the runner context that merges all namespaces into a single array (user RBAC, -n/--namespace SKAFFOLD_NAMESPACE and every helmRelease namespace specified). When the namespace field isn't included under deploy.helm.releases, this seems to add a blank (or "") namespace in the list of namespaces which may require cluster-scope permissions.

There's a work-around for this before this fix - adding the namespace under each helm-release will make this work:

    deploy:
      helm:
        releases:
        - name: <release-name>
          namespace: <release-namespace>

However we have users sharing the same repo/skaffold.yaml and attempt to deploy each to their own namespace, therefore hardcoding the namespace isn't ideal.

After:
Pod watcher and deployment status works without adding the namespace field under deploy.helm.releases. I've tested also using admin user (with cluster-wide rbac) and the behaviour works the same. Also tested multiple helmReleases with different namespaces, seems to work as well with this change.

Question to the contributors on this project: was there an intended reason to have this behaviour? (i.e. having an empty ns in namespaces list) The reason I ask is as you can see I had to change the test as well and remove "" from expected output.

Let me know.

Follow-up Work (remove if N/A)

[codecov]

Codecov Report

Merging #4568 into master will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4568      +/-   ##
==========================================
+ Coverage   72.61%   72.69%   +0.08%     
==========================================
  Files         335      335              
  Lines       12963    12972       +9     
==========================================
+ Hits         9413     9430      +17     
+ Misses       2957     2949       -8     
  Partials      593      593              

Impacted Files	Coverage Δ
pkg/skaffold/runner/util/util.go	92.59% <100.00%> (+0.28%)	⬆️
pkg/skaffold/docker/remote.go	53.44% <0.00%> (+23.44%)	⬆️

---

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 33b1d10...11b47a4. Read the comment docs.