Skip to content

Update linkerd top to use Tap APIService#3186

Merged
siggy merged 2 commits intomasterfrom
siggy/top-apiserver
Aug 2, 2019
Merged

Update linkerd top to use Tap APIService#3186
siggy merged 2 commits intomasterfrom
siggy/top-apiserver

Conversation

@siggy
Copy link
Member

@siggy siggy commented Aug 2, 2019

PR #3167 introduced a Tap APIService, and migrated linkerd tap to it.

This change migrates linkerd top to the new Tap APIService. It also
addresses a panic: close of closed channel issue, where two go
routines could both call close(done) on exit.

Fixes #3168

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

PR #3167 introduced a Tap APIService, and migrated `linkerd tap` to it.

This change migrates `linkerd top` to the new Tap APIService. It also
addresses a `panic: close of closed channel` issue, where two go
routines could both call `close(done)` on exit.

Fixes #3168

Signed-off-by: Andrew Seigner <[email protected]>
@siggy siggy added the area/tap label Aug 2, 2019
@siggy siggy requested review from adleong and kleimkuhler August 2, 2019 00:12
@siggy siggy self-assigned this Aug 2, 2019
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 2, 2019

siggy added a commit that referenced this pull request Aug 2, 2019
PR #3167 introduced a Tap APIService, and migrated linkerd tap to it.

This change migrates `linkerd profile --tap` to the new Tap APIService.

Depends on #3186
Fixes #3169

Signed-off-by: Andrew Seigner <[email protected]>
Copy link
Member

@adleong adleong left a comment

Choose a reason for hiding this comment

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

⭐ Very nice and clean ⭐ 🎩

@siggy siggy force-pushed the siggy/top-apiserver branch from b8ea6d7 to c7c3f87 Compare August 2, 2019 18:02
siggy added a commit that referenced this pull request Aug 2, 2019
PR #3167 introduced a Tap APIService, and migrated linkerd tap to it.

This change migrates `linkerd profile --tap` to the new Tap APIService.

Depends on #3186
Fixes #3169

Signed-off-by: Andrew Seigner <[email protected]>
Copy link
Contributor

@kleimkuhler kleimkuhler left a comment

Choose a reason for hiding this comment

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

Awesome--looks good!

@siggy siggy merged commit a185cae into master Aug 2, 2019
siggy added a commit that referenced this pull request Aug 2, 2019
PR #3167 introduced a Tap APIService, and migrated linkerd tap to it.

This change migrates `linkerd profile --tap` to the new Tap APIService.

Depends on #3186
Fixes #3169

Signed-off-by: Andrew Seigner <[email protected]>
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 2, 2019

siggy added a commit that referenced this pull request Aug 2, 2019
PR #3167 introduced a Tap APIService, and migrated linkerd tap to it.

This change migrates `linkerd profile --tap` to the new Tap APIService.

Depends on #3186
Fixes #3169

Signed-off-by: Andrew Seigner <[email protected]>
@siggy siggy deleted the siggy/top-apiserver branch August 2, 2019 19:45
cpretzer pushed a commit that referenced this pull request Aug 6, 2019
PR #3167 introduced a Tap APIService, and migrated `linkerd tap` to it.

This change migrates `linkerd top` to the new Tap APIService. It also
addresses a `panic: close of closed channel` issue, where two go
routines could both call `close(done)` on exit.

Fixes #3168

Signed-off-by: Andrew Seigner <[email protected]>
cpretzer pushed a commit that referenced this pull request Aug 6, 2019
PR #3167 introduced a Tap APIService, and migrated linkerd tap to it.

This change migrates `linkerd profile --tap` to the new Tap APIService.

Depends on #3186
Fixes #3169

Signed-off-by: Andrew Seigner <[email protected]>
kleimkuhler added a commit that referenced this pull request Aug 8, 2019
### Motivation

PR #3167 introduced the tap APIService and migrated `linkerd tap` to use it.
Subsequent PRs (#3186 and #3187) updated `linkerd top` and `linkerd profile
--tap` to use the tap APIService. This PR moves the web's Go server to now also
use the tap APIService instead of the public API. It also ensures an error
banner is shown to the user when unauthorized taps fail via `linkerd top`
command in *Overview* and *Top*, and `linkerd tap` command in *Tap*.

### Details

The majority of these changes are focused around piping through the HTTP error
that occurs and making sure the error banner generated displays the error
message explaining to view the tap RBAC docs.

`httpError` is now public (`HTTPError`) and the error message generated is short
enough to fit in a control frame (explained [here](https://github.com/linkerd/linkerd2/blob/kleimkuhler%2Fweb-tap-apiserver/web/srv/api_handlers.go#L173-L175)).

### Testing

The error we are testing for only occurs when the linkerd-web service account is
not authorzied to tap resources. Unforutnately that is not the case on Docker
For Mac (assuming that is what you use locally), so you'll need to test on a
different cluster. I chose a GKE cluster made through the GKE console--not made
through cluster-utils because it adds cluster-admin.

Checkout the branch locally and `bin/docker-build` or `ares-build` if you have
it setup. It should produce a linkerd with the version `git-04e61786`. I have
already pushed the dependent components, so you won't need to `bin/docker-push
git-04e61786`.

Install linkerd on this GKE cluster and try to run `tap` or `top` commands via
the web. You should see the following errors:

### Tap

![web-tap-unauthorized](https://user-images.githubusercontent.com/4572153/62661243-51464900-b925-11e9-907b-29d7ca3f815d.png)

### Top

![web-top-unauthorized](https://user-images.githubusercontent.com/4572153/62661308-894d8c00-b925-11e9-9498-6c9d38b371f6.png)

Signed-off-by: Kevin Leimkuhler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update linkerd top to use the new Tap APIService

4 participants