-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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
Enable Release Namespace Creation in Helm3 #6794
Comments
@ichekrygin Thanks for the detailed proposal and POC on this! Because this is a feature add (yes, it did exist in Helm 2, but was an explicit decision to remove), I have put this into the 3.1 milestone. We will come back around to this after we get through the Helm 3.0 release in a few weeks |
Good to know support for namespace creation during helm install might be back in v3.1. Is it possible to know the background about why it is considered "sub-optimal" in helm2? namespace creation with helm2 is working fine as far as I know. |
Sure thing @stevebail. So the tl;dr is that we wanted |
Focusing specifically on #5753 (comment)
I address 2. first for being the easiest. There could be multiple ways how we could have handled such requests, for example:
As for 1., I have the following concerns. Final point. I think to have P.S. at the end of #5753 (comment) @bacongobbler defers to @adamreese as to the authoritative source for information behind |
@ichekrygin I am definitively not an authorize source and was definitively not involved in any decision :)
My 2 cents. |
I'm not sure I understand the argument here. The goal behind any of these RBAC related decisions was explicitly to not be opinionated, meaning that we have no idea what kind of permissions a user may have. I don't think we can say the same for charts either, especially for charts in the stable repo. I think you do have a point around CRD support. Perhaps we should make CRD installation opt-in instead of opt-out and namespace creation would behave the same way, but it might be too late for that. |
My bad, I copied wrong GitHub user, now it is corrected. |
@thomastaylor312 perhaps it is my misinterpretation of #5753 (comment):
I think the chart author is the authoritative source on what RBAC rights installing user should have, with or without the creation of |
Ok, I missed one of the issues where this has sprawled across and now have a better grasp on it. Essentially there have been several design concerns around having the namespace auto created for you that have been documented and explained by @bacongobbler and others. What it boils down to is: The scope of the What it comes down to is that this is a design and shipping decision that we have agreed upon, but we do understand your desire and point of view. I know there are pain points here, but I think they are something that can be addressed with a plugin or feature after 3.0 comes out, so please let us know if you have ways to work around the aforementioned design concerns. We are not against the use case in any way, but we want a way to support this in a sustainable fashion. |
Well, I just gave in and whipped up a quick plugin for it: https://github.com/thomastaylor312/helm-namespace |
@thomastaylor312 |
@thomastaylor312 I think we are on the same page in terms of the timeline of the decision making. It is the consistency (or lack thereof) what I have the gripe with, i.e., why we chose to support CRD's one way, yet, deprecate support for |
In light of "one-stop-shop", I think the closest analogy would be if all the sudden Also, I made this point in other threads, with |
So I fully admit they are different, but that decision was made by several core maintainers working in tandem on the issue considering all the feedback we had on hand from users and came to the conclusion that it was the most simple and effective way to support CRDs. I was part of the process and we went back and forth for weeks on that decision. I'll admit that we didn't consider it together with the namespace, but based on all of the digging through the various edge cases around CRDs, we were ok with it. If I were to do it again, I might consider making it opt-in like I mentioned before And like I said, we aren't against adding support for namespaces back in, but it needs to be well thought out to handle all of the aforementioned design problems |
On a related note: @ichekrygin one option I could see here is making sure if there is a namespace manifest in a chart, that we install it first. Not sure if we could make that clean from a code perspective, but Helm does already know to install namespace objects first, but it doesn't get there because a namespace doesn't exist |
@thomastaylor312 I would appreciate any feedback on #6795 |
Yep, will get to that once we get past 3.0 release |
I was just tipped off that Kubernetes has an admission controller that handles this case. It might be worth looking further into before considering alternatives. https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#namespaceautoprovision For the lazy:
|
@bacongobbler IMHO, using a. b. From my very high-level understanding of |
Behaviour around namespaces changed a lot in Helm 3. Every helm command is now scoped to a namespace just like kubectl. We now need to specify the namespace in each helm command. Tiller is no longer required either, the `init` command was removed because of this. TODO: installing a chart to a namespace that doesn't exist currently fails. There's a proposal to bring this behaviour back: helm/helm#6794 Signed-off-by: Taylor Silva <tsilva@pivotal.io>
This could be accomplished using the new Because of that, I'm in favour of closing this, as the use case provided here can be accomplished using that pattern without introducing any further code. |
@bacongobbler how does |
I'm not sure how I can spell it out more clearly. The example provided in #7473 (comment) demonstrates how someone can use the |
@mattfarina couple of clarifications
I just want to emphasize, this issue and proposal are dedicated to the specific namespace: I understand there are many namespaces that chart can subsequently create, but those are out of the scope of this issue. The
First, the chart authors do not need to update anything, if they are satisfied with To address, why more elaborate "Namespace" is added to the chart was partially satisfy the following reason:
If we drop "modifying namespace" functionality, I totally see it would be possible to address this w/out any additional changes to existing charts. With that said, I did come across cases where some charts required additional modifications to the
|
Love the idea of moving this functionality back to a flag so that we as the user can opt into having helm auto-create the Release Namespace. If we allow this flag for [1] helm/charts#18719 and lots more like it. |
#7648 implements |
...Did you actually read my previous comments? The example I provided earlier in #7473 (comment) SPECIFICALLY creates the release namespace if it isn't present. I will post the example again for posterity. templates/namespace.yaml:
If In other words, it accomplishes the exact same task #6795 proposes to solve, without introducing any code to Helm. When #7648 is merged, we will have solved both cases:
Hope that clears things up. |
Yes, I did and replied on the same day - #7473 (comment) I don't mind doing this again. There must be something embarrassingly simple that I don't see.
|
@adamreese and I paired together and discussed the implementation of Here's the code where you're seeing that error occur: Lines 275 to 277 in d6fad6b
Put simply, before we go ahead and create the resources that the chart declared, we create a release ledger (a secret) in the release namespace to record its status. This was intentional to prevent the chart the ability to manage its own namespace. If we allow the chart to manage its own namespace, it brings up a huge design problem: should the chart ever decide to stop tracking the release namespace, the namespace and all its objects are removed... Including the release ledger, and any other charts installed within that namespace. Taking #6795 as an example. templates/namespace.yaml
Great. We now have the ability to manipulate the namespace from within the chart. But what if we remove it?
This scenario is relatively harmless, but it exposes a problem: It's too easy for the chart author to accidentally delete their own namespace (and everything within it, including the ledger). Or, in a more dangerous case, the chart author could delete OTHER user's resources, too. Taking that same example, let's install multiple charts in the same namespace.
Now, we remove the namespace from the chart and upgrade.
Going forward, it sounds like #7648 is the only safe option we can provide here, and we should close out #6795 as something we are not planning to support as per the above examples. #7648 would restore Helm 2's old functionality (as originally requested), without introducing any major design flaws. |
Sorry, @bacongobbler, is it possible to TL;DR; the comment above? Specifically, was I wrong in my previous comments stating that the |
I have mentioned earlier (most recently) that I am not "hard set" on having to provide
The only reason such functionality exists in this POC is to show one of the possible solution addressing:
This POC can be "enhanced" to not remove Moreover, I will be OK, if we add neither manifest nor flags. The bare minimum ask is to restore As for |
@bacongobbler - we can preserve
|
Consider this as an example of ^:
|
On the upside, there was quite a bit of activity here! I love it. Quickly to sum up today's activity:
|
@bacongobbler after a little digging... you can't use the The specific error is in the form:
|
Agreed. That was my mistake. I failed to remember a design decision we made in Helm 3. I tried to explain that in more depth with #6794 (comment) but I was not very clear with the messaging around that. Sorry about that. @ichekrygin after reading your follow-up comments, it's still unclear to me how #6795 is intended to restore Helm 2's functionality. As I understand, the goal of this ticket is to find a solution that allows a user to create the namespace on-the-fly as Helm 2 worked. Right? When we talk about roles in Helm, we often talk about two specific roles: the chart author, and the chart operator. The author is the one who writes the chart, and the operator is the one that "consumes" the chart. In Helm 2, the namespace was not something a chart author had to provide. The role of creating the namespace was a role for the chart operator as However, when we look at #6795, its functionality relies on the chart author to introduce a namespace as a template into the chart. The role of creating the namespace shifts to the chart author. This is a large behavioral shift in how charts are maintained and operated, including
This is why I approached this with #7648. By creating the namespace on-the-fly with a feature flag, the roles remain the same as in Helm 2: chart authors do not have to maintain the lifecycle of the namespace, and Helm can continue to assume the lifecycle management of the namespace, including design concerns such as the one described earlier when the author fails to introduce a resource policy. If we were to consider #6795, each and every chart would have to introduce the template to use this feature. That would mean that every chart is no longer compatible with older versions of Helm (i.e. we break backwards compatibility), and every single chart would have to be updated to introduce this functionality. If I'm understanding the proposal - to restore functionality without breaking compatibility - that would seem like a non-starter. Do you have any other thoughts you'd like to add here? |
I was able to verify #7648 |
Behaviour around namespaces changed in Helm 3. Every helm command is now scoped to a namespace just like kubectl. We now need to specify the namespace in each helm command. Namespaces are also not made automatically by helm if they don't exist. This results in "Namespace does not exist" errors from helm when trying to install a chart. This commit adds a function to create the namespace right before installing a chart using kubectl. There's a proposal to bring this behaviour back into helm: helm/helm#6794 Tiller is no longer required either, the `init` command was removed because of this. Signed-off-by: Taylor Silva <tsilva@pivotal.io>
What was the conclusion to this? I'm trying to follow the various merged/closed PRs, and I'm unclear how this was resolved. I'm trying something like the proposed change in the OP:
However, I get Omitting the namespace from the chart completely and passing |
Overview
Helm2
provided support for the Release namespace{{ .Release.Namespace }}
via--namespace
option if the release namespace did not exist. This functionality was considered rudimentary, and there were several requests from the community captured in #3503Helm3
took a more opinionated approach on how to handle--namespace
option w/non-existing namespaces by removing support for the release namespace creation. Irrespective of the--namespace
option, the{{ .Release.Namespace }}
is expected to exist before installing/upgrading anyhelm
chart that utilizes--namespace
option.By dropping support for the Release Namespace creation via
--namespace
option inHelm3,
we also dropped any possibility to install a Helm chart with non-existent namespace! As a result, the chart authors and chart users must rely on additional, external to Helm3 tools likekubectl
toprepare (create) Release Namespace before Helm chart installation.
It is understood that
--namespace
option to create namespace was sub-optimal inHelm2
, which prompted requests from the helm user community similar to #3503.Proposed Change
Rather than to remove support for the Release Namespace creation altogether, we can provide accommodations for helm chart authors to create the release namespace inline during the helm chart install by allow to define release namespace Inside as a
template
resource. Moreover, the helm chart authors will also be able to tailor the release namespace to the needs of the applications by including all necessary labels, annotations.Consider following template for the Release Namespace manifest.
In the example above, we chose to emulate current Helm3 chart functionality, i.e., do not create the release namespace by default.
The chart authors can decide whether or not allow multiple instances of their application to target the same namespace. For example, removing
{{- if ... end -}}
lines, effectively ensure that a given chart can only be installed into a newly created namespace and no two chart instances for this application can share the same namespace.Processing and Assumptions
While the Release Namespace resource resembles other Helm
template
resources very closely, the processing of this resource is different.Helm3 can perform a "special" pre-installation check to see if the Release Namespace is defined in the
templates
section. If it is - Helm3 will attempt to create the release namespace before recording release history, which utilizes the release namespace.The "special" handling of the Release Namespace hs following assumptions/implications:
templates
section, thus will be fully rendered just like any othertemplates
resource (unlike crds)templates
resources.Input Result Matrix (Helm3 Current)
Note: here and below,
Namespace
is synonymous toRelease Namespace.
Note: in this example for simplicity we assume that all Helm chart artifacts are installed into the Release Namespace
default
namespacedefault
namespaceProvided
release namespaceInput Result Matrix (Proposed)
Same use cases as above, but this time with proposed changed and with Helm chart that includes template definition for the Release Namespace.
default
namespacedefault
namespaceCreateNamespaceValue is set to
false`Provided
release namespaceSummary
The goal of this proposal is to entertain a possibility to provide Helm chart authors with functionality to define and provide support in terms of the properties when it comes down to the Release Namespace artifact.
With this proposal: Helm chart authors can allow users to:
while maintaining all the current Helm3 chart behaviors when it comes to storing Helm release information.
The text was updated successfully, but these errors were encountered: