Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override namespace for objects #220

Closed
gre9ory opened this issue Apr 25, 2023 · 8 comments · Fixed by #221, #222, #223, #224 or #225
Closed

Override namespace for objects #220

gre9ory opened this issue Apr 25, 2023 · 8 comments · Fixed by #221, #222, #223, #224 or #225
Assignees
Labels
enhancement New feature or request

Comments

@gre9ory
Copy link
Collaborator

gre9ory commented Apr 25, 2023

Based on @sczech's request:

I'm just wondering if it is possible to set the namespace for the objects inside of values.yaml? Ideally I would like to have a config.general.namespaceOverride key.

Currently the metadata.namespace fields of the objects are not touched or touchable by HULL. They are set by Helm on deployment where the -n or --namespace parameter sets the metadata.namespace for all created objects. This is I think standard Helm procedure.

In that sense having a global/general setting applied to all objects with a config.general.namespaceOverride option would amount to the same as setting a different namespace for helm install/upgrade? But maybe you have a different usecase in mind like one sketched below?

Overriding the namespace on a per object basis could be useful and implementation is rather easy. I can think of having as suggested a namespaceOverride property on the hull.object.base level where you can overwrite this for any given object.

Let's assume a case where you want to have one alternative backup namespace where some of the objects should go to and you want to have a central field to provide that namespace. For this you could then add the namespaceOverride to the objects in question and have them reference a shared field like hull.config.specific.backupNamespace to get the centrally configured namespace. I think that could be your use case?

@gre9ory gre9ory added the enhancement New feature or request label Apr 25, 2023
@gre9ory gre9ory mentioned this issue Apr 25, 2023
@sczech
Copy link

sczech commented Apr 25, 2023

Thank you for the quick response! My use case is that we don't install Helm Charts directly into the cluster. We have a deployment pipeline that does a helm template and commits the output into our infrastructure repo. Then the plain manifests are synced from this repo into our Cluster via FluxCD.
This is in line with GitOps best practices and allows us to have a full declarative state of our cluster in Git. But since Helm isn't communicating directly with our cluster we have to define metadata.namespace in advance and bake it into the template.

@gre9ory
Copy link
Collaborator Author

gre9ory commented Apr 25, 2023

Interesting, never looked at the metadata.namespace when helm templateing so far. Have used helm update/install all the time sofar in our workflows.

When I did a test just now it turns out -n and --namespace does not put a namespace in the templates (exactly as you said above). Quite surprising behavior but I see I am in good company here. One of the things where the donkey work is seemingly unnecessarily handed over to the chart maintainers (adding namespace property to all objects) or users (post-processing templates with whatever tool) 👎

Anyhow, now I totally understand what you are after with your request. Flux is also at the top of my list of things to do next since we want to use it at work too so sounds like a total win-win to allow this. It should also be not much work.

Some questions aligning on the desired implementation:

  • I see that (some) people just take the {{ .Release.Namespace }}, ergo the -n passed to helm template, to populate the metadata.namespace fields (like here).

  • Others have an extra field in mind like your config.general.namespaceOverride and fallback to the {{ .Release.Namespace }} when not provided as for example here:

    {{- if .Values.namespace -}}
    {{- .Values.namespace -}}
    {{- else -}}
    {{- .Release.Namespace -}}
    {{- end -}}
    

Small question I have whether there is any danger to always setting this metadata.namespace field for the helm upgrade/install use case? Though fairly sure Helm does not complain when encountering a metadata.namespace here in the templates (otherwise there wouldn't be all the examples above) but maybe you can confirm that there is no danger in explicitly populating this field always? If yes then I think I would go with the second approach where it always populates metadata.namespace either from config.general.namespaceOverride or {{ .Release.Namespace }} if config.general.namespaceOverride isn't present or empty.

@gre9ory
Copy link
Collaborator Author

gre9ory commented Apr 25, 2023

One other caveat I forgot about is the need to seperate non-namespaced objects from namespaced ones to set it only where it belongs: kubernetes-sigs/external-dns#2807

I think I have seen that Kubernetes is not complaining when a namespace is set on a non-namespaced object but better handle this properly from the get go.

@gre9ory
Copy link
Collaborator Author

gre9ory commented Apr 25, 2023

I think I would want to put it here:

hull:
  config:
    general:
      metadata:
        namespaceOverride: 

@sczech Do you think there is a usecase for individual namespaceOverride's on the instance level as mentioned before? So the logic on an per instance basis could populate the rendered namespace like so:

  1. hull.objects.<type>.<instance>.namespaceOverride if present and non-empty
  2. hull.config.metadata.namespaceOverride if present and non-empty
  3. .Release.Namespace otherwise

It would be not much extra effort to do so and it should cover all bases.

@sczech
Copy link

sczech commented Apr 25, 2023

The second solution is exactly how we have implemented it in our internal charts. We set .Release.Namespace as default and allow it to be overwritten by .Values.namespaceOverride. I haven't encountered any issues with this implementation (also tried helm install) and big charts like kube-prometheus-stack use it as well (https://github.com/prometheus-community/helm-charts/blob/6db99b756ff91586e26a1d009eceaf9cee8b1c40/charts/kube-prometheus-stack/templates/_helpers.tpl#L124) so I guess it should be fine to always populate metadata.namespace.

@sczech
Copy link

sczech commented Apr 25, 2023

I think I would want to put it here:

hull:
  config:
    general:
      metadata:
        namespaceOverride: 

@sczech Do you think there is a usecase for individual namespaceOverride's on the instance level as mentioned before? So the logic on an per instance basis could populate the rendered namespace like so:

  1. hull.objects.<type>.<instance>.namespaceOverride if present and non-empty
  2. hull.config.metadata.namespaceOverride if present and non-empty
  3. .Release.Namespace otherwise

It would be not much extra effort to do so and it should cover all bases.

I don't think there is a need for individual fields since a chart should really only be deployed to one namespace at a time. If you want to deploy to another namespace the preferred solution should be to use subcharts.

Concerning the namespaceOverride key location: I find it to be most intuitive to put it on the same level as nameOverride and fullnameOverride, at least that is where I would look for it first.

@gre9ory
Copy link
Collaborator Author

gre9ory commented Apr 27, 2023

Ok it is done, new releases with namespaceOverride:

Release 1.27,1
Release 1.26.3
Release 1.25.14

Followed your suggestion so you can use:

hull:
  config:
    general:
      metadata:
        namespaceOverride: abc

to change the namespace in the templates.

If not provided, the {{ .Release.Namespace }} is used.

Hope that works and if you have other problems. feature suggestions or find a bug let me know!

@gre9ory gre9ory self-assigned this Apr 27, 2023
@gre9ory gre9ory closed this as completed Apr 27, 2023
@sczech
Copy link

sczech commented Apr 27, 2023

Looking good! Thank you for the quick implementation 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
2 participants