Skip to content

Conversation

bacongobbler
Copy link
Member

closes #6794

Signed-off-by: Matthew Fisher matt.fisher@microsoft.com

Signed-off-by: Matthew Fisher <matt.fisher@microsoft.com>
@helm-bot helm-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 20, 2020
@bacongobbler bacongobbler added this to the 3.2.0 milestone Feb 20, 2020
Copy link

@ichekrygin ichekrygin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work as intended.

@mattfarina mattfarina self-requested a review February 24, 2020 18:15
Copy link
Collaborator

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works for me.

@bacongobbler
Copy link
Member Author

@ichekrygin can you demonstrate how you tested this? You should just be able to run

$ helm install wordpress stable/wordpress --namespace wordpress --create-namespace

I manually tested this PR before submission. Happy to hear feedback on this.

@ichekrygin
Copy link

ichekrygin commented Feb 24, 2020

Helm version

⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ ./helm version
version.BuildInfo{Version:"v3.1+unreleased", GitCommit:"694cf61aacba6bc4ed59a65d23acd20f6c8b95bd", GitTreeState:"clean", GoVersion:"go1.12.12"}

Git Branch

⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ git log | head
commit 694cf61aacba6bc4ed59a65d23acd20f6c8b95bd
Author: Matthew Fisher <matt.fisher@microsoft.com>
Date:   Thu Feb 20 11:56:03 2020 -0800

    feat(install): introduce --create-namespace

    Signed-off-by: Matthew Fisher <matt.fisher@microsoft.com>

commit f7bcdf194b8782c10ec476a77929807199ad9baf
Merge: 99304c86 2a742

Create new chart foo

⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ ./helm create foo
Creating foo

⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ ls -l
total 41068
drwxr-xr-x 4 illya illya     4096 Feb 20 16:41 bar/
drwxr-xr-x 4 illya illya     4096 Feb 24 10:48 foo/
-rwxr-xr-x 1 illya illya 42038352 Feb 24 10:48 helm*

Test deploy foo into existing(default) namespace

⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ ./helm install foo ./foo
NAME: foo
LAST DEPLOYED: Mon Feb 24 10:48:49 2020
NAMESPACE: default
STATUS: deployed
REVISION: 1
NOTES:
1. Get the application URL by running these commands:
  export POD_NAME=$(kubectl get pods --namespace default -l "app.kubernetes.io/name=foo,app.kubernetes.io/instance=foo" -o jsonpath="{.items[0].metadata.name}")
  echo "Visit http://127.0.0.1:8080 to use your application"
  kubectl --namespace default port-forward $POD_NAME 8080:80

Show deployed chart artifacts

⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ kubectl get all
NAME                      READY   STATUS    RESTARTS   AGE
pod/foo-9458c558d-24nbw   1/1     Running   0          9s

NAME                 TYPE        CLUSTER-IP    EXTERNAL-IP   PORT(S)   AGE
service/foo          ClusterIP   10.99.16.16   <none>        80/TCP    9s
service/kubernetes   ClusterIP   10.96.0.1     <none>        443/TCP   24d

NAME                  READY   UP-TO-DATE   AVAILABLE   AGE
deployment.apps/foo   1/1     1            1           9s

NAME                            DESIRED   CURRENT   READY   AGE
replicaset.apps/foo-9458c558d   1         1         1       9s

Show installed helm chart

⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ ./helm list
NAME    NAMESPACE       REVISION        UPDATED                                 STATUS          CHART           APP VERSION
foo     default         1               2020-02-24 10:48:49.656651438 -0800 PST deployed        foo-0.1.0       1.16.0

Cleanup - delete helm chart

⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ helm delete foo
release "foo" uninstalled

# Verify that artifacts were removed
⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ kubectl get all
NAME                      READY   STATUS        RESTARTS   AGE
pod/foo-9458c558d-24nbw   0/1     Terminating   0          22s

NAME                 TYPE        CLUSTER-IP   EXTERNAL-IP   PORT(S)   AGE
service/kubernetes   ClusterIP   10.96.0.1    <none>        443/TCP   24d

# List all namespaces
⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ kubectl get ns
NAME              STATUS   AGE
bar               Active   3d17h
default           Active   24d
kube-node-lease   Active   24d
kube-public       Active   24d
kube-system       Active   24d

UPDATED (see next post) Test install helm chart into non-existing namespace

⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ helm install foo ./foo -n foo
Error: create: failed to create: namespaces "foo" not found

# Install helm chart into non-existing namespace with flag `--create-namespace`
⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ helm install foo ./foo -n foo --create-namespace
Error: unknown flag: --create-namespace

# Repeat with `--namespace` vs. `-n`
⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ helm install foo ./foo --namespace foo --create-namespace
Error: unknown flag: --create-namespace

@ichekrygin
Copy link

Yes, it does work - "user error" (embarrasing: I ran wrong helm version command for the last test)

⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ ./helm install foo ./foo --namespace foo --create-namespace
NAME: foo
LAST DEPLOYED: Mon Feb 24 11:03:07 2020
NAMESPACE: foo
STATUS: deployed
REVISION: 1
NOTES:
1. Get the application URL by running these commands:
  export POD_NAME=$(kubectl get pods --namespace foo -l "app.kubernetes.io/name=foo,app.kubernetes.io/instance=foo" -o jsonpath="{.items[0].metadata.name}")
  echo "Visit http://127.0.0.1:8080 to use your application"
  kubectl --namespace foo port-forward $POD_NAME 8080:80

⋊> ~/g/s/g/h/h/bin on fix-6794 ⨯ kubectl get ns
NAME              STATUS   AGE
bar               Active   3d17h
default           Active   24d
foo               Active   15s
kube-node-lease   Active   24d
kube-public       Active   24d
kube-system       Active   24d

@ichekrygin
Copy link

I am glad this feature is back in!

Just curious, what made you change your mind in light of #5753 (comment)

@bacongobbler
Copy link
Member Author

The user can choose to opt into this functionality or not depending on the presence of the flag. This was not an option before.

@bacongobbler bacongobbler merged commit ec2e77c into helm:master Feb 24, 2020
@bacongobbler bacongobbler deleted the fix-6794 branch February 24, 2020 19:20
@ichekrygin
Copy link

ichekrygin commented Feb 24, 2020

To clarify before as in Helm2?

What about:

namespaces are a global cluster resource and the user installing resources into that namespace may not have the proper administrative rights to install the namespace itself, as that implies you have full administrative rights to the cluster

And:

Additionally, there have been several asks in the community to allow modifying the namespace helm install creates (e.g. #3503), and the UX to support use cases like that would be incredibly painful to achieve. Offloading the namespace creation to a separate tool (perhaps a plugin?) provides users a way to solve these issues without imposing a restrictive user experience around these use cases.

I am correct, neither of those "justifications" is satisfied with this PR either.

Don't get me wrong, I am super happy about this PR change!
It would be nice to get some clarity though, to put this issue behind.

@bacongobbler
Copy link
Member Author

If the user assumes they have administrative rights to the cluster, they can opt into the functionality. Otherwise, they’ll be blocked.

@bacongobbler
Copy link
Member Author

The second point is a non-issue. Users who want to modify the namespace can create it with kubectl create out-of-band.

@ichekrygin
Copy link

ichekrygin commented Feb 24, 2020

If the user doesn't have admin functionality the "opt into" functionality is not applicable at all.
If we focus on user experience intalling chart into non-existent namespace:

User Type Command Helm2 Helm3 (opt in) Helm3 (not-opt-in)
Non-Admin helm install foo ./foo -n foo: install fails -n foo --create-namespace: install fails -n foo: install fails
Admin helm install foo ./foo -n foo: install succeeds -n foo --create-namespace: install succeeds -n foo: install fails

Under what circumstances users would choose to be "not-opt-in" or "opt-out"?

It is like: "I am an admin and I am installing the chart into the non-existing namespace and I want to be opt-out (or I don't want to be opt-in)", i.e. "I want it to fail"
I am having a hard time seeing this as a valid use-case.

@bacongobbler
Copy link
Member Author

bacongobbler commented Feb 24, 2020

You may wish to opt out if you know the namespace was created beforehand (like with kubectl create) and you do not want to create more namespaces. Or, you do not want to re-create the namespace if it was deleted out-of-band

A simpler example may be in the helm upgrade --install case: If you accidentally mistype foo as fooo, Helm will return an error reporting the namespace doesn't exist. If you added the --create-namespace flag, Helm will create the namespace, report success and install the chart in the fooo namespace, even if the intended behaviour was wrong. The operator only wanted to upgrade the existing chart, not create a new one in another namespace.

I think we've exhausted this conversation at this point. We're making a mountain out of a molehill here. The feature has been implemented and is in 3.2.

@ichekrygin
Copy link

I think we've exhausted this conversation at this point. We're making mountains out of molehills here. The feature has been implemented and is in 3.2.

👍

@ishankhare07
Copy link

When can we expect this to be released?

@bacongobbler
Copy link
Member Author

v3.2.0

@alexander-nolan
Copy link

@bacongobbler If I'm installing the parent chart with helm install, is there a way to pass the --create-namespace flag to a subchart?

@gofman8
Copy link

gofman8 commented Sep 22, 2020

We need to have ability to pass custom labels during namespace creation. Please reopen #3503

@shashankram
Copy link

We need to have ability to pass custom labels during namespace creation. Please reopen #3503

This will be super useful. Any plans on supporting this? A lot of resources filter on namespace selectors which are essentially based on labels on a namespace.

kaxing added a commit to kaxing/longhorn-website that referenced this pull request Jun 9, 2021
According to this commit helm/helm#7648
helm 3.2 is able to create namespace when it's not existed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Release Namespace Creation in Helm3
8 participants