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

Improve Namespace handling in the chart: Set release Namespace as default and allow user overrides #81

Open
OuesFa opened this issue Mar 28, 2023 · 3 comments

Comments

@OuesFa
Copy link

OuesFa commented Mar 28, 2023

In the current situation, the creation of the namespace is managed by the 'create' configuration as we can see here: https://github.com/strimzi/drain-cleaner/blob/main/helm-charts/helm3/strimzi-drain-cleaner/templates/000-Namespace.yaml#L1.

On the other hand, the deployment is governed by the 'name' configuration at this location: https://github.com/strimzi/drain-cleaner/blob/main/helm-charts/helm3/strimzi-drain-cleaner/templates/060-Deployment.yaml#L7.

This leads to unusual behavior when the 'create' configuration is set to false, but the HelmRelease still attempts to deploy in the default 'strimzi-drain-cleaner' namespace.

Using the default namespace in this manner may not be the best practice, as typically, resources deployed by a chart share the same namespace as the release by default.

A suggested solution would be to adjust the conditions and set the release namespace as the default. This should only be overridden if the user specifically requests it.

What do you think?

@scholzj
Copy link
Member

scholzj commented Mar 28, 2023

The default namespace is not a place you want to deploy applications. It usually has special properties and might not even allow applications to run properly.

TBH, I think it works well from my point of view today:

  • If you disable the namespace creation, then you are ultimately saying that you are responsible for creating the namespace. so you should not complain if it fails because the namespace does not exist.
  • Changing the namespace used as default today to something else would mean backwards incompatible change. So I think we should stick with 'strimzi-drain-cleaner'.

@OuesFa
Copy link
Author

OuesFa commented Mar 28, 2023

Ok, pretty clear, thanks!

@diranged
Copy link

@scholzj So we just ran into a bug where the drain cleaner Certificate is invalid and not being creaetd properly - and we didn't notice. The issue is in https://github.com/strimzi/drain-cleaner/blob/0.4.2/helm-charts/helm3/strimzi-drain-cleaner/templates/041-Certificate.yaml#L13-L15. Why are you specifying .Values.namespace.name rather than using the fairly standard {{ .Release.Namespace }} or even {{ default .Release.Namespace .Values.namespace.name }}? I find it generally unusual to see helm charts where I have to hard-code in the namespace name... and that makes using tools like ct more difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants