feat: Allow Helm to manage namespace where objects are created. #1132
Conversation
| @@ -1,3 +1,15 @@ | |||
| {{/* | |||
| If createNamespace is set to true, sets the namespace to "gatekeeper-system" | |||
There was a problem hiding this comment.
PR looks great! This is a little confusing, is it possible to use createNamespace to create any namespace?
helm install ... -> installs to gatekeeper-system
helm install --set createNamespace=false ... -> installs to gatekeeper-system (namespace should exist before)
helm install --namespace myNamespace ... -> installs to myNamespace (creating namespace)
helm install --namespace myNamespace --set createNamespace=false ... -> installs to myNamespace (namespace should exist before)
There was a problem hiding this comment.
Hi @sozercan. I agree that the createNamespace is confusing and was implemented before this change. It is considered best practice to not create namespaces in a Helm chart. That being said here is how this works.
By default in the values.yaml file createNamespace: true
So with that
helm install .... -> Creates the gatekeeper-system namespace and resources are installed there. Helm will manage the chart in the default namespace.
helm install --set createNamespace=false ... -> no namespace is created and one isn't specified in the helm command so it is installed in the default namespace
helm install -n myNamespace ... -> gatekeeper-system namespace is created and helm installs the resources there, however the chart is managed in myNamespace namespace. This is what happens today as well.
helm install -n myNamespace --set createNamespace=false ... helm installs the resources to myNamespace and manages the chart there.
When I say "manages the chart in this namespace", I am referring to if you where to run helm list -n <namespace> you would see charts "installed" in that namespace. The way it is now you are likely to install the resources in 1 namespace and have the chart managed in another.
Currently today how we install the chart is helm install -n gatekeeper-system --set createNamespace=false gatekeeper gatekeeper/gatekeeper --create-namespace This makes everything fairly clear what is going on and everything contained to 1 namespace, however we are stuck with using gatekeeper-system as our namespace.
There was a problem hiding this comment.
I didn't know there was a --create-namespace flag in Helm 3. I think we should deprecate the createNamespace in values when Helm 2 chart is removed.
There was a problem hiding this comment.
Do we need to update the readme/docs after we cut a new release to reflect these chart changes so we have better guidance for what is the recommended way to install this chart?
ie. install all the gk resources and the chart releases in the same namespace vs install all the gk resources in the gatekeeper-system ns while the chart releases are in another namespace?
|
@jdolce as mentioned in #1108 (comment), can you update e2es to verify this scenario is validated in our tests? |
Working on this now. |
jdolce
force-pushed
the
namespace
branch
8 times, most recently
from
332fc90
to
0572426
Compare
|
@sozercan I have added some tests to the that creates and tests the chart in a new namespace. The upgrade tests are failing but I have never had any luck getting those to pass, even before this change. Earlier today they where actually being skipped. Any ideas? |
.github/workflows/workflow.yaml
Outdated
|
|
||
| - name: Save logs | ||
| run: | | ||
| kubectl logs -n gatekeeper-system -l control-plane=controller-manager --tail=-1 > logs-helm-${{ matrix.HELM_VERSION }}-controller.json | ||
| kubectl logs -n gatekeeper-system -l control-plane=audit-controller --tail=-1 > logs-helm-${{ matrix.HELM_VERSION }}-audit.json | ||
| kubectl logs -n ${{ matrix.GATEKEEPER_NAMESPACE }} -l control-plane=controller-manager --tail=-1 > logs-helm-${{ matrix.HELM_VERSION }}-controller.json |
There was a problem hiding this comment.
this is going to overwrite the logs, can you add ${{ matrix.GATEKEEPER_NAMESPACE }} to the filename?
|
@jdolce re-ran the upgrade tests, passing now. pr looks great, added 1 last comment. |
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. Signed-off-by: Julian Dolce <jdolce@qnx.com>
|
@sozercan Good catch. Updated and tests are passing. Cheers! |
Codecov Report
@@ Coverage Diff @@
## master #1132 +/- ##
=======================================
Coverage 49.29% 49.29%
=======================================
Files 63 63
Lines 4390 4390
=======================================
Hits 2164 2164
Misses 1966 1966
Partials 260 260
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
Signed-off-by: Julian Dolce jdolce@qnx.com
What this PR does / why we need it:
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:
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.