Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/mongodb] Adding namespace override to values #20441

Closed
wants to merge 2 commits into from
Closed

[stable/mongodb] Adding namespace override to values #20441

wants to merge 2 commits into from

Conversation

benjamin-wright
Copy link

@benjamin-wright benjamin-wright commented Jan 30, 2020

Signed-off-by: Ben Wright benjamin.wright@nielsen.com

What this PR does / why we need it:

This PR allows parent charts to pass in a specific namespace for mongodb, facilitating an umbrella chart approach to declaratively define multi-namespace applications using helm dependencies. The only alternative is resorting to imperative scripts to partially apply segments of the chart, or break down applications into separate charts per namespace.

The default is still to deploy into the release namespace, so this change should not affect any existing uses of the chart.

Which issue this PR fixes

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the README.md
  • Title of the PR starts with chart name (e.g. [stable/mychartname])

@helm-bot helm-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 30, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: benjamin-wright
To complete the pull request process, please assign carrodher
You can assign the PR to them by writing /assign @carrodher in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot
Copy link
Contributor

Hi @benjamin-wright. Thanks for your PR.

I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 30, 2020
Ben Wright added 2 commits January 30, 2020 11:34
…20440)

Signed-off-by: Ben Wright (benjamin.p.wright@hotmail.com)
Signed-off-by: Ben Wright <benjamin.wright@nielsen.com>
Signed-off-by: Ben Wright <benjamin.p.wright@hotmail.com>
Signed-off-by: Ben Wright <benjamin.wright@nielsen.com>
@helm-bot helm-bot added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Jan 30, 2020
@benjamin-wright
Copy link
Author

/assign @carrodher

Copy link
Collaborator

@juan131 juan131 left a comment

Choose a reason for hiding this comment

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

Hi @benjamin-wright

Thanks so much for the PR!

I'd like to understand why it's necessary to specifically set the namespace in the manifests.

Helm (both 2 and 3 versions) is designed to adapt the manifests with the appropriate namespace (based on the --namespace flag), following the same behaviour as kubectl. So, what's the main advantage on adding this information to the manifests?

@juan131
Copy link
Collaborator

juan131 commented Jan 31, 2020

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 31, 2020
@benjamin-wright
Copy link
Author

Hi @juan131 ,

The main advantage is being able to deploy to more than one namespace from a single helm chart.

The template and dependency management features of helm make it absolutely ideal for representing an entire application made up of multiple moving parts in a declarative format. We've been using it as a way of reducing the amount of imperative scripting required for deployments, and the complexity management involved there, but always having to deploy to a single namespace really hobbles the utility value. You can deploy multiple times, switching parts of the chart on or off, or split the chart out into per-namespace components and install them individually, but then you water-down or lose the benefits of helm as a management tool for combining those components and add complexity to the tools upstream to perform a task that helm is perfectly capable of performing itself.

For charts that we're creating ourselves, being able to pass through namespaces to dependency charts works like a dream, we'd just like to make it possible to take that same approach here, while obviously not compromising its original behaviour.

@juan131
Copy link
Collaborator

juan131 commented Feb 3, 2020

Hi @benjamin-wright

The main advantage is being able to deploy to more than one namespace from a single helm chart.

Why wouldn't you able to deploy the current chart on multiple namespaces? Here you have a piece of code where I installed the same chart on different namespaces (using helm3):

$ kubectl create ns my-first-namespace
$ helm install mongodb stable/mongodb --namespace my-first-namespace
$ kubectl create ns my-second-namespace
$ helm install mongodb stable/mongodb --namespace my-second-namespace

As you can see, I could install the chart twice (on different namespaces) and everything is working:

$ helm status mongodb -n my-first-namespace
NAME: mongodb
LAST DEPLOYED: Mon Feb  3 13:12:35 2020
NAMESPACE: my-first-namespace
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
** Please be patient while the chart is being deployed **

MongoDB can be accessed via port 27017 on the following DNS name from within your cluster:

    mongodb.my-first-namespace.svc.cluster.local

To get the root password run:

    export MONGODB_ROOT_PASSWORD=$(kubectl get secret --namespace my-first-namespace mongodb -o jsonpath="{.data.mongodb-root-password}" | base64 --decode)

To connect to your database run the following command:

    kubectl run --namespace my-first-namespace mongodb-client --rm --tty -i --restart='Never' --image bitnami/mongodb --command -- mongo admin --host mongodb --authenticationDatabase admin -u root -p $MONGODB_ROOT_PASSWORD

To connect to your database from outside the cluster execute the following commands:

    kubectl port-forward --namespace my-first-namespace svc/mongodb 27017:27017 &
    mongo --host 127.0.0.1 --authenticationDatabase admin -p $MONGODB_ROOT_PASSWORD
$ kubectl get pod -n my-first-namespace
NAME                       READY   STATUS    RESTARTS   AGE
mongodb-5bbd7d5575-ldtlc   1/1     Running   0          80s
$ helm status mongodb -n my-second-namespace
NAME: mongodb
LAST DEPLOYED: Mon Feb  3 13:12:48 2020
NAMESPACE: my-second-namespace
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
** Please be patient while the chart is being deployed **

MongoDB can be accessed via port 27017 on the following DNS name from within your cluster:

    mongodb.my-second-namespace.svc.cluster.local

To get the root password run:

    export MONGODB_ROOT_PASSWORD=$(kubectl get secret --namespace my-second-namespace mongodb -o jsonpath="{.data.mongodb-root-password}" | base64 --decode)

To connect to your database run the following command:

    kubectl run --namespace my-second-namespace mongodb-client --rm --tty -i --restart='Never' --image bitnami/mongodb --command -- mongo admin --host mongodb --authenticationDatabase admin -u root -p $MONGODB_ROOT_PASSWORD

To connect to your database from outside the cluster execute the following commands:

    kubectl port-forward --namespace my-second-namespace svc/mongodb 27017:27017 &
    mongo --host 127.0.0.1 --authenticationDatabase admin -p $MONGODB_ROOT_PASSWORD
$ kubectl get pod -n my-second-namespace
NAME                       READY   STATUS    RESTARTS   AGE
mongodb-5bbd7d5575-vhqbc   1/1     Running   0          76s

@benjamin-wright
Copy link
Author

Hi @juan131,

Sorry, when I said:

more than one namespace from a single helm chart

I didn't mean that you couldn't use the same chart twice, targeting two different namespaces. I meant that you couldn't use a chart which deploys to two namespaces from a single call. The ambition I had here was to create a single chart which contains my entire application:

Umbrella Helm Chart:
dependencies:

  • Application 1
  • Application 2
  • Database 1
  • Queue 1
  • etc...

The benefit of this is that a single release can combine the outputs of my entire application, deployed with a single command, and a single values.yaml file will represent all of the pertinent application state. Application 1, Database 1, etc. can all be their own helm charts, and I can use k8s namespaces to separate the resources from each to prevent naming collisions, but my master helm chart can combine them all into a single deployable stack.

I get that this is slightly controversial, in that with kubectl it is generally considered a bad idea to hardcode namespaces into your manifests as this reduces the possibility for re-use, and helm models its behaviour on kubectl. And you absolutely could achieve the same effect with scripts like the one you described, and they are not even monstrously complicated. But it is possible to do the same thing within helm using only standard helm templating features. You then get the benefits of a single step, purely declarative deployment process, entirely within helm, without the need to hive off imperative logic into another language.

@benjamin-wright
Copy link
Author

@juan131 Bump

@mforutan
Copy link

mforutan commented Mar 2, 2020

hi @juan131, as @benjamin-wright mentioned above, the ability to override the namespace is not related to install a helm chart multiple times, it is about using this helm chart as a dependency in an umbrella chart but keep it in different namespace, there are a few other charts already updated with this:
#15202
#18986

can you please check this again?

@benjamin-wright
Copy link
Author

@juan131 Bump

@benjamin-wright
Copy link
Author

Or @prydonius or @carrodher, if I could get a nod in principal that you're happy with this being done, then I can sort out the merge conflicts (I think they're just version number bumps).

@mforutan
Copy link

if it help, I recently added the same change to [mangodb-replicaset] chart in this PR: #21203

@benjamin-wright what also I did was to move namespaceOverride value into global space so it can be overridden from multiple parents level: #21203 (comment)

@carrodher
Copy link
Collaborator

Hi,

Given the stable deprecation timeline, this Bitnami maintained Helm chart was deprecated and moved to bitnami/charts some weeks ago. Please visit the bitnami/charts GitHub repository to create Issues or PRs.

In this issue (#20969), we tried to explain more carefully the reasons and motivations behind this transition, please don't hesitate to add a comment in this issue if you have any question related to the migration itself.

@benjamin-wright
Copy link
Author

Hi @carrodher,

Ah, thanks, missed the deprecation notice going up, will transfer across :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. 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.

stable/mongodb: override namespace for mongo deployment
6 participants