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

bump kubernetes dashboard version #2789

Merged
merged 1 commit into from
Jul 12, 2017

Conversation

gianrubio
Copy link
Contributor

@gianrubio gianrubio commented Jun 21, 2017

  • bump dashboard to v1.6.1
  • rbac rules for k8s dashboard

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 21, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @gianrubio. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot 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.

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

@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 Jun 21, 2017
@chrislovecnm
Copy link
Contributor

/assign @sethpollack

@k8s-ci-robot
Copy link
Contributor

@chrislovecnm: GitHub didn't allow me to assign the following users: sethpollack.

Note that only kubernetes members can be assigned.

In response to this:

/assign @sethpollack

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.

@chrislovecnm
Copy link
Contributor

@sethpollack can you review?

@chrislovecnm
Copy link
Contributor

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 22, 2017

---
apiVersion: rbac.authorization.k8s.io/v1beta1
kind: ClusterRoleBinding
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we want to just assume that the dashboard should have cluster-admin access.

@gianrubio
Copy link
Contributor Author

gianrubio commented Jun 22, 2017 via email

@sethpollack
Copy link
Contributor

Not sure, but that would give admin access to everyone with dashboard access. I don't think its a good idea to have that as the default. If anything the default should be no access.

@justinsb @liggitt any ideas?

@liggitt
Copy link
Member

liggitt commented Jun 22, 2017

Not sure, but that would give admin access to everyone with dashboard access. I don't think its a good idea to have that as the default. If anything the default should be no access.

@justinsb @liggitt any ideas?

Correct, don't give the dashboard any permissions you don't want available to anyone with network visibility to pod or service IPs.

I don't know what role I would expect as the default. Does kops have any indication whether the user wants a secured cluster they can open up permissions on as they wish, or a dev cluster that is wide open by default?

@sethpollack
Copy link
Contributor

Maybe don't bind to any role by default and let the user do that step manually.

@chrislovecnm
Copy link
Contributor

@liggitt

I don't know what role I would expect as the default. Does kops have any indication whether the user wants a secured cluster they can open up permissions on as they wish, or a dev cluster that is wide open by default?

We do not have any expectation, other that a kops cluster is building a production grade cluster. We would like to lock it down, and allow a user to open the perms as needed. Just my two cents.

@chrislovecnm
Copy link
Contributor

@liggitt / @sethpollack any further recommendations?

@sethpollack
Copy link
Contributor

sethpollack commented Jun 29, 2017

I would leave out the ClusterRoleBinding and add something to the readme about it.

@gianrubio gianrubio force-pushed the bump-dashboard branch 5 times, most recently from 4ddf99f to 1cfbec7 Compare July 12, 2017 12:36
@gianrubio
Copy link
Contributor Author

I removed the cluster role binding, I just bumping the dashboard version and creating a service account.

@sethpollack
Copy link
Contributor

lgtm, maybe just add something to the docs about the need to add your own role binding?

@gianrubio gianrubio force-pushed the bump-dashboard branch 5 times, most recently from 5d5fefe to 7f85391 Compare July 12, 2017 17:14
add serviceaccount for kubernetes-dashboard and wrote docs related to rbac
@gianrubio
Copy link
Contributor Author

@sethpollack done, please review again!

@sethpollack
Copy link
Contributor

👍

Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Thanks

@chrislovecnm
Copy link
Contributor

/assign
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 12, 2017
@chrislovecnm
Copy link
Contributor

merging per @sethpollack's review

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. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants