Remove tap from public API and associated test infrastructure #3240

kleimkuhler · 2019-08-13T17:49:45Z

Summary

After the addition of the tap APIServer, all the logic related to tap in the public API no longer needs to be there. The servers and clients that are created but not used, as well as all the old testing infrastrucure related to tap can be removed.

This deprecates TapByResource and therefore required an update to the protobuf files with bin/protoc-go.sh. While the change to deprecate this method was extremely small, a lot of protobuf fils were updated in the process. These changes to the code and protobuf files should probably remain coupled since TapByResource is officially deprecated in the public API, but a majority of the additions/deletions are related to those files.

This draft passes go test as well as a local run of the integration tests.

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

kleimkuhler · 2019-08-13T18:18:34Z

The diff size in this PR will go down significantly since it is rebased off of #3241

l5d-bot · 2019-08-13T19:56:52Z

Integration test results for 99f8242: success 🎉
Log output: https://gist.github.com/e0955b97ade99bd4d9fb940e002b024e

siggy

lgtm. shipit pending merge of #3241 🚢 👍

### Summary Changes from `bin/protoc-go.sh` An existing [draft PR](#3240) has a majority of its changes related to protobuf file updates. In order to separate these changes out into more related components, this PR updates the generated protobuf files so that #3240 can be rebased off this and have a more manageable diff. Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

l5d-bot · 2019-08-13T22:01:14Z

Integration test results for 9c8795d: success 🎉
Log output: https://gist.github.com/b0eaa420e54be657664555e240580e27

adleong · 2019-08-13T22:10:41Z

proto/controller/tap.proto

@@ -8,5 +8,5 @@ option go_package = "github.com/linkerd/linkerd2/controller/gen/controller/tap";

 service Tap {
  rpc Tap(public.TapRequest) returns (stream public.TapEvent) { option deprecated = true; }
-  rpc TapByResource(public.TapByResourceRequest) returns (stream public.TapEvent) {}


Is this actually deprecated? Isn't this the interface that the tap pod serves? (albeit as proto-over-http instead of gRPC)

As of this PR, it's true that the TapByResource endpoint in Tap's gRPC server is still accepting requests. In a followup PR, the plan is to remove that gRPC server.

Current request flow:
CLI -> K8s API Server -> Tap API Server -> Tap gRPC Server -> Proxy

The plan:
CLI -> K8s API Server -> Tap API Server -> Proxy

Maybe this is splitting hairs but I would say this grpc service definition here defines the contract of the Tap API server. i.e. it takes a TapByResourceRequest as input (encoded as proto-over-http) and returns a stream of TapEvents (encoded as proto-over-http).

Even though none of the grpc bindings generated from this IDL are used, I still wouldn't call it deprecated because it describes the server's interface.

It's true that there still exists a server endpoint that takes a proto-over-http TapByResourceRequest and returns TapEvent's, but I don't think it accurately describes the server's interface, since it's not a gRPC server, and there is not going to be an endpoint called TapByResource.

I agree about documenting our API contracts. In that case, we would document the HTTP interface for the TAP APIService. This could be a starting point:

linkerd2/controller/tap/handlers.go

Lines 72 to 104 in 4d01e37

func initRouter(h *handler) *httprouter.Router {

router := &httprouter.Router{}

router.GET("/", handleRoot)

router.GET("/apis", handleAPIs)

router.GET("/apis/"+gvk.Group, handleAPIGroup)

router.GET("/apis/"+gvk.GroupVersion().String(), handleAPIResourceList)

router.GET("/healthz", handleHealthz)

router.GET("/healthz/log", handleHealthz)

router.GET("/healthz/ping", handleHealthz)

router.GET("/metrics", handleMetrics)

router.GET("/openapi/v2", handleOpenAPI)

router.GET("/version", handleVersion)

router.NotFound = handleNotFound

for _, res := range resources {

route := ""

if !res.namespaced {

route = fmt.Sprintf("/apis/%s/watch/%s/:namespace", gvk.GroupVersion().String(), res.name)

} else {

route = fmt.Sprintf("/apis/%s/watch/namespaces/:namespace/%s/:name", gvk.GroupVersion().String(), res.name)

}

router.GET(route, handleRoot)

router.POST(route+"/tap", h.handleTap)

}

return router

}

// POST /apis/tap.linkerd.io/v1alpha1/watch/namespaces/:namespace/tap

// POST /apis/tap.linkerd.io/v1alpha1/watch/namespaces/:namespace/:resource/:name/tap

func (h *handler) handleTap(w http.ResponseWriter, req *http.Request, p httprouter.Params) {

@adleong Does this make sense or did you have any more questions about it?

Yeah. I guess this is weird either way. At the least, we should add comments to TapByResourceRequest and TapEvent to indicate that these structs are used by the tap API, since these structs are otherwise not used in any non-deprecated gRPC service.

@adleong Good point--I added some comments that clarify they are only used by the tap APIServer right now. In the future when we are able to re-expose this to authorized users through the gRPC interface, the comments can be updated.

siggy · 2019-08-13T22:10:49Z

you'll need to rebase on master again, and then update

linkerd2/charts/linkerd2/templates/controller.yaml

Line 83 in 4d01e37

- -tap-addr=linkerd-tap.{{.Namespace}}.svc.{{.ClusterDomain}}:8088

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

l5d-bot · 2019-08-14T20:36:00Z

Integration test results for e37ec3a: fail 😕
Log output: https://gist.github.com/19f3fc0cd03b7c375e0ee4daa3a4514c

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

l5d-bot · 2019-08-14T21:06:58Z

Integration test results for ef90517: fail 😕
Log output: https://gist.github.com/7c4c21791d1111d08b1038fe73d4954d

@adleong

Closes #7881 This makes the rest of the necessary fixes to satisfy the `staticcheck` lint. The only class of lints that are being skipped are those related to deprecated tap code. There was some discussion on the original change started by @adleong about if this _actually_ deprecated [here](#3240 (comment)); it doesn't look like we every came back around to fully removing it but I don't think it should be a blocker for enabling the lint right now. Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

kleimkuhler requested a review from siggy August 13, 2019 17:49

kleimkuhler self-assigned this Aug 13, 2019

kleimkuhler mentioned this pull request Aug 13, 2019

Update protobuf files #3241

Merged

kleimkuhler force-pushed the kleimkuhler/remove-public-tap branch from fe79ed8 to 99f8242 Compare August 13, 2019 18:12

siggy approved these changes Aug 13, 2019

View reviewed changes

kleimkuhler force-pushed the kleimkuhler/remove-public-tap branch from 99f8242 to 9c8795d Compare August 13, 2019 21:02

kleimkuhler requested a review from adleong August 13, 2019 21:03

kleimkuhler marked this pull request as ready for review August 13, 2019 21:03

adleong reviewed Aug 13, 2019

View reviewed changes

Remove tap from public API and associated tests

e37ec3a

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

kleimkuhler force-pushed the kleimkuhler/remove-public-tap branch from 9c8795d to e37ec3a Compare August 14, 2019 19:48

Add clarifying comments on gRPC tap structs

ef90517

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

adleong approved these changes Aug 14, 2019

View reviewed changes

kleimkuhler merged commit cc3c53f into master Aug 14, 2019

kleimkuhler deleted the kleimkuhler/remove-public-tap branch August 14, 2019 21:27

kleimkuhler mentioned this pull request Mar 10, 2022

enable staticcheck #8037

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove tap from public API and associated test infrastructure #3240

Remove tap from public API and associated test infrastructure #3240

kleimkuhler commented Aug 13, 2019

kleimkuhler commented Aug 13, 2019 •

edited

Loading

l5d-bot commented Aug 13, 2019

siggy left a comment

l5d-bot commented Aug 13, 2019

adleong Aug 13, 2019

siggy Aug 13, 2019

adleong Aug 13, 2019

siggy Aug 13, 2019

kleimkuhler Aug 14, 2019

adleong Aug 14, 2019

kleimkuhler Aug 14, 2019

siggy commented Aug 13, 2019

l5d-bot commented Aug 14, 2019

l5d-bot commented Aug 14, 2019

	func initRouter(h handler) httprouter.Router {
	router := &httprouter.Router{}

	router.GET("/", handleRoot)
	router.GET("/apis", handleAPIs)
	router.GET("/apis/"+gvk.Group, handleAPIGroup)
	router.GET("/apis/"+gvk.GroupVersion().String(), handleAPIResourceList)
	router.GET("/healthz", handleHealthz)
	router.GET("/healthz/log", handleHealthz)
	router.GET("/healthz/ping", handleHealthz)
	router.GET("/metrics", handleMetrics)
	router.GET("/openapi/v2", handleOpenAPI)
	router.GET("/version", handleVersion)
	router.NotFound = handleNotFound

	for _, res := range resources {
	route := ""
	if !res.namespaced {
	route = fmt.Sprintf("/apis/%s/watch/%s/:namespace", gvk.GroupVersion().String(), res.name)
	} else {
	route = fmt.Sprintf("/apis/%s/watch/namespaces/:namespace/%s/:name", gvk.GroupVersion().String(), res.name)
	}

	router.GET(route, handleRoot)
	router.POST(route+"/tap", h.handleTap)
	}

	return router
	}

	// POST /apis/tap.linkerd.io/v1alpha1/watch/namespaces/:namespace/tap
	// POST /apis/tap.linkerd.io/v1alpha1/watch/namespaces/:namespace/:resource/:name/tap
	func (h handler) handleTap(w http.ResponseWriter, req http.Request, p httprouter.Params) {

Remove tap from public API and associated test infrastructure #3240

Remove tap from public API and associated test infrastructure #3240

Conversation

kleimkuhler commented Aug 13, 2019

Summary

kleimkuhler commented Aug 13, 2019 • edited Loading

l5d-bot commented Aug 13, 2019

siggy left a comment

Choose a reason for hiding this comment

l5d-bot commented Aug 13, 2019

adleong Aug 13, 2019

Choose a reason for hiding this comment

siggy Aug 13, 2019

Choose a reason for hiding this comment

adleong Aug 13, 2019

Choose a reason for hiding this comment

siggy Aug 13, 2019

Choose a reason for hiding this comment

kleimkuhler Aug 14, 2019

Choose a reason for hiding this comment

adleong Aug 14, 2019

Choose a reason for hiding this comment

kleimkuhler Aug 14, 2019

Choose a reason for hiding this comment

siggy commented Aug 13, 2019

l5d-bot commented Aug 14, 2019

l5d-bot commented Aug 14, 2019

kleimkuhler commented Aug 13, 2019 •

edited

Loading