Add namespace to templates

[kenlasko]

Description of the change

Add metadata.namespace: {{ .Release.Namespace }} to all namespaced templates

Benefits

When using Kustomize to render the Helm chart, the namespace defined in the Helm chart doesn't get applied to all Helm-generated resources unless you explicitly tell Kustomize to force-add it to everything. This may not be desired in some situations where manifests might be included in the Kustomization bound to other namespaces.

Possible drawbacks

No known drawbacks.

Applicable issues

#791

• fixes Missing namespace on Helm manifests cause issues with Kustomize #791

Additional information

See here for an example of what I mean.

Checklist

• I have read the CONTRIBUTING.md doc.
• DCO has been signed off on the commit.
• Chart version bumped in Chart.yaml according to semver.
• (optional) Parameters are documented in the README.md

[wrenix]

Why not use the namespace option in kustomize?

https://github.com/kubernetes-sigs/kustomize/blob/master/examples/multibases/multi-namespace.md

[kenlasko]

@wrenix, I don't think that would solve the issue, which is that without the namespace being explicitly defined in each template, Kustomize won't apply the namespace UNLESS you force apply it to all objects. This is what I mean:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
# Don't want to apply namespace from base, but if I don't, then several resources don't get the namespace because they are missing in the template. This also overrides the namespace set in other manifests, which I don't want.
# Opened a ticket for this: https://github.com/nextcloud/helm/issues/791
namespace: nextcloud
helmCharts:
- name: nextcloud
  repo: https://nextcloud.github.io/helm/
  version: 8.4.1
  releaseName: nextcloud
  namespace: nextcloud # This should be sufficient to set the namespace for all Helm-managed resources, but only if the namespace is explicitly defined in all templates
  valuesFile: values.yaml
resources:
- backup.yaml
# - database.yaml
- external-secrets.yaml
- httproute.yaml
- namespace.yaml
- volume.yaml

This is properly handled by 95% of Helm charts that I use. Adding metadata.namespace: {{ .Release.Namespace }} to all resources causes no downsides that I can see.

[wrenix]

It is a unnessary because helm handle it well and it make the helmchart template less readible and easy to forgot that line of code.

I believe like in your line 3 described, you want a strange construction inside of kustomize and try it to solve in this helmchart. You should create multiple folder and layer of kustomize to solve your namespace problem.

Or like in your second code comment, you could open an issue on kustomize and ask, why is this not set there

[kenlasko]

My second code comment line works fine as long as the namespace is specified in the Helm templates.

This has been brought up before, but they decided the overall solution is to set namespace: {{ .Release.Namespace }}. If you look at any Helm template from other projects, you'll see the majority of them explictily add the namespace to the templates.

I don't see how adding a single line to each manifest makes things more difficult. It won't cause any issues with Helm deployments and Helm deployments via Kustomize will work fine. This isn't some major refactor. Its just something that increases compatibility with Kustomize.

[DrummyFloyd]

i think it's related to this issue
kubernetes-sigs/kustomize#5566

an there is no need to set {{ .Release.Namespace }}
you just have to wait for next Kustomize release

[volker-raschek]

Hi @wrenix and @DrummyFloyd,
I would be very happy to set the namespace especially.

It is a unnessary because helm handle it well and it make the helmchart template less readible and easy to forgot that line of code.

I hope that the helm unittest framework will be faster reviewed and approved, because helm unittest has a feature to test if the namespace is correctly defined. Alternatively your tests will fail. This is a standard test which should be implemented.

For example:

  chart:
    appVersion: 0.1.0
    version: 0.1.0
  suite: ConfigMap template
  release:
    name: nextcloud-unittest
+   namespace: testing
  templates:
  - templates/config.yaml
  tests:
  - it: Render configMap
    asserts:
    - containsDocument:
        apiVersion: v1
        kind: ConfigMap
        name: nextcloud-unittest-config
+       namespace: testing

To be honest, we should not take care of third party applications and their behavior. We should take care of our users and should protect them maybe for a breaking change in the tools which they use. For this reason, I don't see there any problems and appreciate the PR.

@kenlasko , please remove the patched version. Otherwise, the PR cannot be accepted at any time without having to adjust the version if necessary.

[kenlasko]

@DrummyFloyd, the linked Kustomize issue is not saying {{.release.namespace}} isn't necessary. In fact, it says the opposite (see kubernetes-sigs/kustomize#5566 (comment)).

Make sure to use the following in your Helm chart templates to ensure resources are deployed to the desired namespace:

metadata:
  namespace: {{ .Release.Namespace }}

The linked issue is for setting the namespace at the top level of a kustomization.yaml, which adds the namespace to all manifests outside of Helm. That issue has indeed been fixed this summer.