Skip to content

Add 'name' label to created namespaces#4231

Merged
bacongobbler merged 1 commit intohelm:masterfrom
munnerz:patch-2
Jul 5, 2018
Merged

Add 'name' label to created namespaces#4231
bacongobbler merged 1 commit intohelm:masterfrom
munnerz:patch-2

Conversation

@munnerz
Copy link
Copy Markdown
Contributor

@munnerz munnerz commented Jun 16, 2018

This pull request changes Helm to add a 'name' label to namespaces it creates.

This is useful because a number of Kubernetes extension points allow configuring whether webhook/policy applies to a namespace based only on labels, and not namespace name (e.g. NetworkPolicy & Validating/MutatingWebhookConfiguration).

This PR will allow me to selectively disable resource validation on the namespace that my controller/application is deployed into.

This has been requested in #3503 (although that issue requests more metadata). Custom labels have been requested in #4178.

Given the security considerations with custom labels, and the complication in complexity with adding more metadata, for now, I have opted to add a simple 'name' label.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 16, 2018
@munnerz
Copy link
Copy Markdown
Contributor Author

munnerz commented Jun 16, 2018

I have also not implemented ensuring the label exists on the namespace, for the cases where a user may have pre-provisioned a namespace. This also means the label presumably won't be added on chart upgrades too.

@munnerz
Copy link
Copy Markdown
Contributor Author

munnerz commented Jun 29, 2018

Any chance this PR can get some eyes on it? Not sure what the release cycle/process is for Helm, but I'm really keen to get this in some form into Helm! Right now it is not possible for me to set up validating webhooks that exclude a particular namespace, as I am unable to set a label on that namespace 😬

/cc @bacongobbler

@prydonius
Copy link
Copy Markdown
Member

+1 seems like a reasonable add to me. I'll defer to @bacongobbler or someone else who works more on core to LGTM

Copy link
Copy Markdown
Member

@bacongobbler bacongobbler left a comment

Choose a reason for hiding this comment

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

LGTM

@bacongobbler bacongobbler merged commit cc826d3 into helm:master Jul 5, 2018
@Stono
Copy link
Copy Markdown

Stono commented Jul 11, 2018

#pleasereleasethis :D

@bacongobbler
Copy link
Copy Markdown
Member

@Stono feel free to either test this PR using either v2.10.0-rc.2 or the canaries for the time being. We're working on an official release; just fixing up a few snags we've found during testing. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants