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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add namespace support #3197

Merged
merged 1 commit into from Jan 9, 2019

Conversation

@FrenchBen
Copy link
Contributor

commented Aug 4, 2018

When deploying the K8s Dashboard, it's possible to scope it to a certain namespace. Yet even within a namespace, other than kube-system, the dashboard will still try to add secrets to the kube-system ns.

This attempts to open the discussion around how to fix it. In this case, I've added a simple flag to the binary (still needs some tests and url validation).

The following flag was added:
--namespace string When non-default namespace is used, create encryption key in the specified namespace. Default: 'kube-system'. (default "kube-system")

Another idea is for the dashboard to lookup what namespace it's running in, and simply use that. 馃敟 and 馃棧 are welcome :)

/assign @ianlewis

@codecov

This comment has been minimized.

Copy link

commented Aug 4, 2018

Codecov Report

Merging #3197 into master will increase coverage by 0.09%.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3197      +/-   ##
==========================================
+ Coverage   47.93%   48.03%   +0.09%     
==========================================
  Files         164      164              
  Lines        8008     8011       +3     
  Branches       43       43              
==========================================
+ Hits         3839     3848       +9     
+ Misses       3891     3885       -6     
  Partials      278      278
Impacted Files Coverage 螖
src/app/backend/auth/api/types.go 33.33% <酶> (酶) 猬嗭笍
src/app/backend/auth/api/common.go 100% <酶> (+35.71%) 猬嗭笍
src/app/backend/settings/handler.go 31.25% <0%> (酶) 猬嗭笍
src/app/backend/auth/jwe/keyholder.go 56.25% <100%> (酶) 猬嗭笍
src/app/backend/settings/manager.go 56.86% <75%> (+2.69%) 猬嗭笍
...p/backend/integration/metric/common/aggregation.go 90.9% <0%> (+1.81%) 猬嗭笍

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 7c38763...1b7fc98. Read the comment docs.

@FrenchBen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

Ping @kubernetes/dashboard-maintainers and @liggitt

@maciaszczykm maciaszczykm requested a review from jeefy Aug 9, 2018

@floreks

This comment has been minimized.

Copy link
Member

commented Aug 12, 2018

Unfortunately, this change is not enough to allow moving Dashboard to another namespace. It does not only use secret, but also config map that is stored in the kube-system namespace to synchronize configuration. Additionally, lookup for heapster (even though it is already deprecated) is also done only in kube-system. These are 3 things that would have to be configurable in order to fully allow moving it to another ns without any issues.

@FrenchBen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2018

@floreks The namespace would be specified in the YAML, for each object, which would actually move it to the proper namespace (verified) - It's true that heapster is an issue at this point, which needs to be removed.
This change to the backend is to allow secret to match those. The "ultimate" fix is for the backend to lookup what namespace it's deployed into, and create the secrets there (unsure how to currently do this)

How would you suggest I tackle this? Provide a YAML with the custom-namespace as an example?

@floreks

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

There are a couple of steps that should be taken here:

  1. Parametrize all external resources used by Dashboard, that includes:
    1. kubernetes-dashboard-key-holder secret that is by default stored in kube-system namespace
    2. kubernetes-dashboard-settings config map that is by default stored in kube-system namespace
    3. Heapster lookup that is by default done only in kube-system namespace
  2. Tighten the default Dashboard privileges and remove create permission for ConfigMap and Secret in the namespace that Dashboard is running. In case a required resource was not created during Dashboard deployment and does not exist, there should be an error saying that a required resource xxx is missing and should be created by the user in a namespace yyy.
  3. Update the documentation and deployment yamls.
@jeefy

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

@FrenchBen I agree with all of @floreks points. Apologies I haven't chimed in sooner. Was out of signal range for a bit.

Let me know if/when you address them and I'll start a review then. :) If you need to chat or want any help before then, feel free to ping me!

@FrenchBen

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2018

@floreks thanks for putting this together - Below are a couple of questions, to clarify some of those points.

  1. Parametrize all external resources used by Dashboard, that includes:
    i. kubernetes-dashboard-key-holder secret that is by default stored in kube-system namespace this was done in this PR
    ii. kubernetes-dashboard-settings config map that is by default stored in kube-system namespace this is done in the kubernetes-dashboard.yaml, and thus shouldn't be part of this PR - the user would change this
    iii. Heapster lookup that is by default done only in kube-system namespace can heapster run in non kube-system namespace? 馃
  2. Tighten the default Dashboard privileges and remove create permission for ConfigMap and Secret in the namespace that Dashboard is running. In case a required resource was not created during Dashboard deployment and does not exist, there should be an error saying that a required resource xxx is missing and should be created by the user in a namespace yyy. Not sure if I understand this, shouldn't the deployment be able to create configMaps/secrets in the same namespace?
  3. Update the documentation and deployment yamls. Docs can be updated to show the additional params, the YAML, imho, should remain as-is.

To clarify some of the above points, this PR aims to allow users to deploy the dashboard in a namespace that isn't the default kube-system.
The steps would be similar to what they would try today:

  1. Create a namespace
  2. Update the kubernetes-dashboard.yaml metadata to use the custom namespace they chose
  3. Add the additional flag of --enc-key-namespace to let the dashboard know where the secret should be created.
  4. ...
  5. Profit?

As a side conversation, is there a roadmap that moves the dashboard towards {metrics-server](https://github.com/kubernetes-incubator/metrics-server)

@jeefy

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

Gonna go backwards:

Metrics API - We're going to address this after the Angular migration. There's been some preliminary talk about how and certain trouble we see in the move away from Heapster

I agree on what the steps to implement a non-default ns deployment should be.

re: 3: Everything I've seen, Heapster is in kube-system only. If there's a way to deploy it otherwise, I haven't seen an example of it.

re: 2: The way I'm reading it is making sure the running dashboard cannot create CMs/Secrets. Have those CMs/Secrets be created upon deployment creation and allow read-only access to those resources by the dashboard.

@floreks

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

this is done in the kubernetes-dashboard.yaml, and thus shouldn't be part of this PR - the user would change this

It is not true. Even though config map is created in the yaml, changing its namespace will currently result in Dashboard error. As you can see below config map namespace and name are hardcoded. It should be configurable.

// SettingsConfigMapName contains a name of config map, that stores settings.
SettingsConfigMapName = "kubernetes-dashboard-settings"
// SettingsConfigMapNamespace contains a namespace of config map, that stores settings.
SettingsConfigMapNamespace = "kube-system"

can heapster run in non kube-system namespace?

I don't see a reason why it wouldn't be able to run in other namespaces. It is just the matter of giving it required permissions. Different namespace does not block heapster from reading metrics.

Not sure if I understand this, shouldn't the deployment be able to create configMaps/secrets in the same namespace?

Currently Dashboard has permissions to create secrets and config maps in the kube-system namespace. As in some scenarios this could cause security issues, I would remove them and adjust the backend. Required secret and config map should be created during Dashboard deployment and create permissions should be removed. In case someone deletes it or modifies yaml and they won't exist during Dashboard start, error should be shown as mentioned above.

As for the additional flag I would try to keep all Dashboard exclusive resources in the same namespace and add a single argument that will be used both by kubernetes-dashboard-key-holder secret and kubernetes-dashboard-settings config map. By default this argument should be set to kube-system. It could simply be named --namespace.

Regarding heapster it does not need to be in the same namespace as Dashboard. I think --heapster-host argument that we already have should be enough to configure it in case it is deployed in another namespace. Similar to how heapster configures sink --sink=influxdb:http://monitoring-influxdb.kube-system.svc:8086.

@todoesverso

This comment has been minimized.

Copy link

commented Aug 31, 2018

This is a really useful feature. I am planning to use dashboard developers to deploy jobs, but I want to make sure they only do it in a dedicated namespace. This instance of the dashboard would be only for them.

@FrenchBen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2018

@floreks thanks for the breakdown -
It's deceiving, but SettingsConfigMapNamespace is used in a few places, and will need a bit of redesign to make it fully dynamic - Admittedly, this PR is a semi-hack, as we look at the flags, and no longer the API types defined.

Happy to change the flag to be --namespace, wasn't sure if it was explicit enough?

I'll get started on making a proper namespace port, we can then resume some of this discussion.

@floreks

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

Happy to change the flag to be --namespace, wasn't sure if it was explicit enough?

IMHO it's fine. @maciaszczykm @jeefy do you think that it will be fine from the user perspective?

@jeefy

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

I think so. We need to clearly document what the flag actually does but otherwise it sounds good to me.

@FrenchBen FrenchBen force-pushed the FrenchBen:key-namespace branch from ee74cc1 to 7ac378f Sep 12, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Sep 12, 2018

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

馃摑 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@FrenchBen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2018

FYI, the following vendors seem to be missing from the core project - Should I go ahead and add them to this PR?
https://gist.github.com/FrenchBen/c45e460fb835972b3e20871595f31b64

@FrenchBen FrenchBen force-pushed the FrenchBen:key-namespace branch from 7ac378f to 90c76ed Sep 12, 2018

@FrenchBen FrenchBen force-pushed the FrenchBen:key-namespace branch from 90c76ed to 296ccc8 Sep 12, 2018

@FrenchBen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2018

@floreks @jeefy per the recommendations, I went ahead and update the flag to be a common --namespace. I've update the original comment to reflect that.

Should I go ahead and modify the deployment YAML as well?

@k8s-ci-robot k8s-ci-robot added the size/L label Jan 4, 2019

@jeefy

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

/approve cancel

@k8s-ci-robot k8s-ci-robot removed the approved label Jan 8, 2019

@monotek

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

Can somebody please give a short update on the status of this pr?

@jeefy

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

@monotek Waiting on one final change/discussion point from @FrenchBen before this moves along. Shouldn't be much longer now that a lot of us are back from break and not completely engrossed in larger projects. :)

@FrenchBen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

@jeefy it was my understanding that I had completed the latest changes? Is there something I missed?

Allow key namespace to be generated in a different namespace
Updated changes as requested
Added tests for URL rejection

Signed-off-by: FrenchBen <me+git@frenchben.com>

@FrenchBen FrenchBen force-pushed the FrenchBen:key-namespace branch from 6efb78f to 1b7fc98 Jan 8, 2019

@jeefy

This comment has been minimized.

Copy link
Member

commented Jan 8, 2019

/lgtm 馃槃

I'm going to let this hang for one more day (Sorry!) to let anyone else chime in, but I'll approve it in 24h. The only thing left (that comes to mind) is once this is merged, we will need to update documentation (Will create issue when relevant) in the wiki.

Thanks @FrenchBen !!

@jeefy

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 9, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Jan 9, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: FrenchBen, jeefy

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

The pull request process is described 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 k8s-ci-robot merged commit 6827715 into kubernetes:master Jan 9, 2019

5 checks passed

cla/linuxfoundation FrenchBen authorized
Details
codecov/patch 70% of diff hit (target 47.93%)
Details
codecov/project 48.03% (+0.09%) compared to 7c38763
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
tide In merge pool.
Details
@davidkarlsen

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

Awesome! Will there be a release soon?

@jeefy

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

@floreks

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

Unfortunately, this feature is not 100% ready. Certificates are still read from a kube-system namespace only and in case dashboard will be deployed in a different namespace with different privileges it will fail to start.

https://github.com/kubernetes/dashboard/blob/master/src/app/backend/auth/api/types.go#L29

@maciaszczykm

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

Should we create tracker issue for that?

@floreks

This comment has been minimized.

Copy link
Member

commented Jan 10, 2019

Definitely. It's quite important as it breaks dashboard in certain cases.

@FrenchBen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

@floreks 馃wondering how my deployment worked?
I've been using the early (and ugly) version of this PR, and have the kubernetes-dashboard-certs secrets properly created in my namespace. I know this to be 馃挴 correct, as I don't have access to the kube-system namespace.

@FrenchBen FrenchBen deleted the FrenchBen:key-namespace branch Jan 11, 2019

@floreks

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

@FrenchBen by default if you do not alter the yaml, certs are stored in-memory. Only if you choose to use custom certificates, you have to store them in a secret. In such case if Dashboard would be deployed in a different namespace than kube-system and it would try to access a secret in a kube-system namespace it might fail and crash.

@FrenchBen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

I did modify the YAML some to get it deployed - perhaps I got lucky?
@floreks Thanks for the follow up PR that cleans things up quite nicely 馃憦

@floreks

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

You would need to pass --tls-cert-file and --tls-key-file arguments with you custom certificates to the Dashboard. They are normally stored in a kubernetes-dashboard-certs secret, but in case in-memory certs are used, this secret is obsolete.

@monotek

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

It also worked if you deployed the dashboard to kube-system with cluster admin role first and after that to another namespace.

@floreks

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

Dashboard would need privileges to read secrets in a different namespace than it is deployed. That would not be secure.

@monotek

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

yes, just wanted to give an explanation why it may have worked for some people...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.