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

Helm chart release namespace #1108

Closed
wants to merge 7 commits into from
Closed

Conversation

jdolce
Copy link
Contributor

@jdolce jdolce commented Feb 2, 2021

What this PR does / why we need it:

Namespaces in a Helm chart, in almost all instances, should be managed by the releases namespace, as well as not created within the chart. #981 made the Namespace creation optional, but this also forces users to install Gatekeeper in the gatekeeper-system namespace as it is hardcoded in the templates.

#1047 was incorrectly closed as it is not solved by #981. This change should allow users to install gatekeeper in any namespace they wish.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #1047

Special notes for your reviewer:
I tried to see if there was something special about the gatekeeper-system namespace but I couldn't find anything.
While I believe this change to be inline with Helm chart best practices, I can see a few issues with this change where users are expecting the Namespace to be there. For example, if you are using the output from helm template and manually applying it using kubectl you might be expecting the namespace creation to happen.

There is probably a few more things to clean up here and add some more documentation, but wanted to get thoughts from others before I went further.

@jdolce
Copy link
Contributor Author

jdolce commented Feb 2, 2021

To make this change clearer you might consider deprecating Helm 2 support - #1076

@sozercan
Copy link
Member

sozercan commented Feb 10, 2021

@jdolce thanks for the PR! We discussed this in the community call,

  • we should default to gatekeeper-system here and create the namespace by default (and keep the helm override to disable this)
  • make sure that this is deployed in our e2e with a different namespace and tested with that namespace
  • e2e will need to be updated to support a different namespace most likely
  • document that only a singleton instance of gatekeeper is supported

If Values.createNamespace=true, which it is by default, a 'gatekeeper-system' Namespace will be created,
and all objects created within it. If it is set to false, a Namespace is not created, and the Helm release
namespace is used.
@codecov-io
Copy link

codecov-io commented Feb 10, 2021

Codecov Report

Merging #1108 (d4be0eb) into master (001f4c3) will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1108      +/-   ##
==========================================
+ Coverage   48.51%   48.58%   +0.06%     
==========================================
  Files          63       63              
  Lines        4339     4339              
==========================================
+ Hits         2105     2108       +3     
+ Misses       1977     1975       -2     
+ Partials      257      256       -1     
Flag Coverage Δ
unittests 48.58% <ø> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...onstrainttemplate/constrainttemplate_controller.go 56.63% <0.00%> (+0.97%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 001f4c3...d4be0eb. Read the comment docs.

@jdolce
Copy link
Contributor Author

jdolce commented Feb 11, 2021

@sozercan Thanks for the feedback. Here is what I have working so far

helm template gatekeeper ./manifest_staging/charts/gatekeeper

This will create the gatekeeper-system namespace and install all of the objects there. Helm will manage the release from the default namespace.

helm -n custom-namespace template gatekeeper ./manifest_staging/charts/gatekeeper

This will create the gatekeeper-system namespace and install all of the objects there. Helm will manage the release from the custom-namespace namespace.

helm -n custom-namespace template gatekeeper ./manifest_staging/charts/gatekeeper --set createNamespace=false

This will NOT create the gatekeeper-system namespace. Helm will install all of the objects in the custom-namespace namespace and manage the release from there as well.

Happy to write some e2e tests for this as well. Currently the Helm 2 upgrade test is failing during the e2e test on the base 3.1.1 chart. Since this shouldn't be testing my code at all I am not sure what to make of it and was hoping you might have some ideas.

sozercan and others added 5 commits February 12, 2021 05:19
Co-authored-by: Rita Zhang <rita.z.zhang@gmail.com>
* Add pathtest functionality to assign mutators

Signed-off-by: Max Smythe <smythe@google.com>

* Add locking, deepcopy path tester memoization

Signed-off-by: Max Smythe <smythe@google.com>

* Add path parsing error context

Signed-off-by: Max Smythe <smythe@google.com>

* Tester should also have a DeepCopy method

Signed-off-by: Max Smythe <smythe@google.com>

* Eager initialize path tester, guard against nil

Signed-off-by: Max Smythe <smythe@google.com>
* Add locking, deepcopy path tester memoization

Signed-off-by: Max Smythe <smythe@google.com>

* Add path parsing error context

Signed-off-by: Max Smythe <smythe@google.com>

* Tester should also have a DeepCopy method

Signed-off-by: Max Smythe <smythe@google.com>

* Eager initialize path tester, guard against nil

Signed-off-by: Max Smythe <smythe@google.com>

* Add value test to assign mutator

Signed-off-by: Max Smythe <smythe@google.com>
@jdolce
Copy link
Contributor Author

jdolce commented Feb 12, 2021

I got my fork into a weird git state so closed this in favour of #1132.

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

Successfully merging this pull request may close these issues.

Allow change default "gatekeeper-system" namespace to a desired one when using charts
5 participants