Skip to content

Introduce tap-admin ClusterRole, web privs flag#3203

Merged
siggy merged 2 commits intomasterfrom
siggy/tap-admin-role
Aug 8, 2019
Merged

Introduce tap-admin ClusterRole, web privs flag#3203
siggy merged 2 commits intomasterfrom
siggy/tap-admin-role

Conversation

@siggy
Copy link
Member

@siggy siggy commented Aug 6, 2019

The web dashboard will be migrating to the new Tap APIService, which
requires RBAC privileges to access.

Introduce a new ClusterRole, linkerd-linkerd-tap-admin, which gives
cluster-wide tap privileges. Also introduce a new ClusterRoleBinding,
linkerd-linkerd-web-admin which binds the linkerd-web service
account to the new tap ClusterRole. This ClusterRoleBinding is enabled
by default, but may be disabled via a new linkerd install flag
--restrict-dashboard-privileges.

Fixes #3177

Signed-off-by: Andrew Seigner [email protected]

@siggy siggy requested review from alpeb and ihcsim August 6, 2019 23:35
@siggy siggy self-assigned this Aug 6, 2019
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 7, 2019

Integration test results for afcf1ac: success 🎉
Log output: https://gist.github.com/19369d3f0ae7192b6cab0a32eef0b6bd

grampelberg
grampelberg reviewed Aug 7, 2019
View reviewed changes
Copy link
Contributor

@grampelberg grampelberg left a comment

Choose a reason for hiding this comment

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

I like it!

alpeb
alpeb reviewed Aug 7, 2019
View reviewed changes
cli/cmd/install.go Outdated
)
flags.BoolVar(
&options.restrictDashboardPrivileges, "restrict-dashboard-privileges", options.restrictDashboardPrivileges,
"Restrict the Linkerd Dashboard's default privileges to disallow Tap.",
Copy link
Member

@alpeb alpeb Aug 7, 2019

Choose a reason for hiding this comment

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

Minuscule nit: no other help description finishes with a dot 😜

alpeb
alpeb approved these changes Aug 7, 2019
View reviewed changes
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Besides my nit above, it looks good to me 👍
I assume the dashboard's tap still works with --restrict-dashboard-privileges because it's hitting the old tap API?

@alpeb
Copy link
Member

alpeb commented Aug 7, 2019

Oh I did found an issue afterwards: the new flag is exposed in linkerd install control-plane and not in linkerd install config but I think it should be the other way around...

@ihcsim
Copy link
Contributor

ihcsim commented Aug 7, 2019

Installation works great with both linkerd and helm, besides the CLI option brought up in #3203 (comment).

Some questions:

  1. Why not use RoleBinding for linkerd-linkerd-web-admin?
  2. Will the linkerd-controller needs this permission too?
  3. Why is --restrict-dashboard-privileges necessary? Can we just default to installing the linkerd-linkerd-web-admin the ClusterRoleBinding?

@siggy
Copy link
Member Author

siggy commented Aug 7, 2019

@alpeb Nice catch re: this flag not being on linkerd install config. I've pushed an update that puts the flag on both stages, since it affects the RBAC resource in linkerd install config, but also affects the flags stored in linkerd-config as part of linkerd install control-plane. It's not ideal that users must be careful to set this flags on both stages, but we were already in that situation with the linkerd-cni-enabled flag, and I've updated the code to couple the behavior of these two flags and make it explicit what's going on.

@siggy
Copy link
Member Author

siggy commented Aug 7, 2019

@ihcsim Good questions re: the rbac stuff.

  1. Why not use RoleBinding for linkerd-linkerd-web-admin?

My understanding is that since the tap.linkerd.io group contains non-namespaced resources., we need ClusterRoleBinding:
https://kubernetes.io/docs/reference/access-authn-authz/rbac/#rolebinding-and-clusterrolebinding

  1. Will the linkerd-controller needs this permission too?

If the linkerd-controller's Public API was calling the new Tap APIService, we would, but with the Tap APIService, our clients (CLI, web) no longer go through the Public API for Tap. Instead it's client -> K8s API -> TAP ApiServer (in the tap pod).

  1. Why is --restrict-dashboard-privileges necessary? Can we just default to installing the linkerd-linkerd-web-admin the ClusterRoleBinding?

We do default to installing linkerd-linkerd-web-admin, but thought it prudent to offer users a more locked-down approach option.

@siggy siggy force-pushed the siggy/tap-admin-role branch from 79adcdd to 859fb03 Compare August 7, 2019 22:33
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 7, 2019

Integration test results for 859fb03: fail 😕
Log output: https://gist.github.com/5cf94358f74d6318e7c2db911e71c8a8

@siggy siggy force-pushed the siggy/tap-admin-role branch from 859fb03 to b40e06c Compare August 7, 2019 22:59
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 7, 2019

Integration test results for b40e06c: fail 😕
Log output: https://gist.github.com/55b803ca59be5448d68040ae8f23ee64

@siggy siggy force-pushed the siggy/tap-admin-role branch 2 times, most recently from d9acd3b to a7be9e0 Compare August 7, 2019 23:37
siggy added 2 commits August 7, 2019 17:03
@siggy
Introduce tap-admin ClusterRole, web privs flag
facaafe 
The web dashboard will be migrating to the new Tap APIService, which
requires RBAC privileges to access.

Introduce a new ClusterRole, `linkerd-linkerd-tap-admin`, which gives
cluster-wide tap privileges. Also introduce a new ClusterRoleBinding,
`linkerd-linkerd-web-admin` which binds the `linkerd-web` service
account to the new tap ClusterRole. This ClusterRoleBinding is enabled
by default, but may be disabled via a new `linkerd install` flag
`--restrict-dashboard-privileges`.

Fixes #3177

Signed-off-by: Andrew Seigner <[email protected]>
@siggy
add restrict-dashboard-privileges to config cmd
773e9fc 
also misc cleanup of flag code

Signed-off-by: Andrew Seigner <[email protected]>
@siggy siggy force-pushed the siggy/tap-admin-role branch from a7be9e0 to 773e9fc Compare August 8, 2019 00:03
@siggy siggy mentioned this pull request Aug 8, 2019
Merged
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 8, 2019

Integration test results for 773e9fc: success 🎉
Log output: https://gist.github.com/76eb95bd4bbbb1d22e2150e3ae31b13b

@siggy siggy merged commit 0ff39dd into master Aug 8, 2019
@siggy siggy deleted the siggy/tap-admin-role branch August 8, 2019 17:28
@kleimkuhler kleimkuhler mentioned this pull request Aug 8, 2019
Closed
This was referenced Aug 8, 2019
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alpeb alpeb alpeb approved these changes

+2 more reviewers

@grampelberg grampelberg grampelberg left review comments

@ihcsim ihcsim ihcsim approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@siggy siggy

Labels

area/install area/tap

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Consider ClusterRole for tap.linkerd.io

5 participants

@siggy @l5d-bot @alpeb @ihcsim @grampelberg