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

Remove gRPC tap server listener from controller #3276

Merged
merged 9 commits into from
Aug 16, 2019

Conversation

kleimkuhler
Copy link
Contributor

Summary

As an initial attempt to secure the connection from clients to the gRPC tap
server on the tap Pod, the tap addr only listened on localhost.

As @adleong pointed out #3257, this was not actually secure because the inbound
proxy would establish a connection to localhost anyways.

This change removes the gRPC tap server listener and changes TapByResource
requests to interface with the server object directly.

From this, we know that all TapByResourceRequests have gone through the tap
APIServer and thus authorized by RBAC.

Details

NewAPIServer now takes a GRPCTapServer instead of a pb.TapClient so that
TapByResource requests can interact directly with the TapByResource method.

GRPCTapServer.TapByResource now makes a private grpcTapServer that satisfies
the tap.TapServer interface. Because this interface is satisfied, we can interact
with the tap server methods without spawning an additional listener.

Signed-off-by: Kevin Leimkuhler kleimkuhler@icloud.com

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
}
)
// GRPCTapServer describes the gRPC server implementing tap.Tap_TapByResourceServer
type GRPCTapServer struct {
Copy link
Contributor Author

@kleimkuhler kleimkuhler Aug 16, 2019

Choose a reason for hiding this comment

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

I decided to make this public because of lint warnings when the type is returned outside the package in main.go.

It does not need to be public, but if I should address the warning a different way I can make that change.

@@ -363,3 +343,38 @@ func renderJSONError(w http.ResponseWriter, err error, status int) {
w.WriteHeader(status)
w.Write(rsp)
}

type serverStream struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The implementation here reflects a previous revision of the tap server in stable-2.4.0

Copy link
Member

Choose a reason for hiding this comment

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

this part looks good. recommend putting a big comment above this line for context, and/or, moving everything from here below to another file.

// serverStream and tapByResourceServer provide functionality that satisfy the
// Tap_TapByResourceServer. This allows the Tap APIServer to call
// GRPCTapServer.TapByResource() directly, rather than make the request to an
// actual gRPC over the network.
// TODO: Share this code with streamServer and destinationServer in
// http_server.go.

@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 16, 2019

Integration test results for f6362df: success 🎉
Log output: https://gist.github.com/bea975a8b91188409ca8bc81ced7d2e4

go.mod Outdated Show resolved Hide resolved
controller/tap/server.go Outdated Show resolved Hide resolved
controller/tap/server.go Show resolved Hide resolved
controller/tap/apiserver.go Outdated Show resolved Hide resolved
Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

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

works well! a few housekeeping comments...

go.mod Outdated Show resolved Hide resolved
controller/tap/server.go Outdated Show resolved Hide resolved
controller/tap/handlers.go Show resolved Hide resolved
controller/cmd/tap/main.go Show resolved Hide resolved
@@ -363,3 +343,38 @@ func renderJSONError(w http.ResponseWriter, err error, status int) {
w.WriteHeader(status)
w.Write(rsp)
}

type serverStream struct {
Copy link
Member

Choose a reason for hiding this comment

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

this part looks good. recommend putting a big comment above this line for context, and/or, moving everything from here below to another file.

// serverStream and tapByResourceServer provide functionality that satisfy the
// Tap_TapByResourceServer. This allows the Tap APIServer to call
// GRPCTapServer.TapByResource() directly, rather than make the request to an
// actual gRPC over the network.
// TODO: Share this code with streamServer and destinationServer in
// http_server.go.

controller/tap/server.go Outdated Show resolved Hide resolved
@siggy siggy added the priority/P0 Release Blocker label Aug 16, 2019
Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 16, 2019

Integration test results for 923dbcb: fail 😕
Log output: https://gist.github.com/b19029134346d5e3d087e562dba3e6f1

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 16, 2019

Integration test results for 0bfebc0: success 🎉
Log output: https://gist.github.com/21daed6a3bbb5a6f55589d280d61fe72

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

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

lgtm! one comment fixup, shipit pending l5d-bot 👍 🚢

controller/tap/handlers.go Outdated Show resolved Hide resolved
Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
siggy added a commit that referenced this pull request Aug 16, 2019
Depends on #3276

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
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.

🔪 💁‍♂

@kleimkuhler kleimkuhler merged commit c9c41e2 into master Aug 16, 2019
@kleimkuhler kleimkuhler deleted the kleimkuhler/remove-tap-client branch August 16, 2019 20:39
siggy added a commit that referenced this pull request Aug 16, 2019
Depends on #3276

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
@l5d-bot
Copy link
Collaborator

l5d-bot commented Aug 16, 2019

Integration test results for 0a1ac19: success 🎉
Log output: https://gist.github.com/a3ea25d7879cfce836726e756285dedc

siggy added a commit that referenced this pull request Aug 16, 2019
Depends on #3276

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants