Implement TapByResource in Tap Service #827

siggy · 2018-04-20T23:37:14Z

The TapByResource endpoint was previously a stub.

Implement end-to-end tapByResource functionality, with support for
specifying any kubernetes resource(s) as target and destination.

Fixes #803, #49

Signed-off-by: Andrew Seigner siggy@buoyant.io

Note that much of the Kubernetes Informer integration is copied from public-api. Once stabilized, this code will get factored out in a subsequent PR. This PR focuses on end-to-end functionality.

$ bin/conduit tapByResource deploy/web --to deploy/emoji
Error: deployment.apps "web" not found
...

$ bin/conduit tapByResource -n emojivoto deploy/web
req id=0:3087 src=10.1.7.138:55152 dst=10.1.7.137:80 :method=GET :authority=web-svc.emojivoto:80 :path=/api/vote
req id=0:3088 src=10.1.7.137:39482 dst=10.1.7.139:8080 :method=POST :authority=emoji-svc.emojivoto:8080 :path=/emojivoto.v1.EmojiService/FindByShortcode
rsp id=0:3088 src=10.1.7.137:39482 dst=10.1.7.139:8080 :status=200 latency=1424µs
end id=0:3088 src=10.1.7.137:39482 dst=10.1.7.139:8080 grpc-status=OK duration=22µs response-length=30B
req id=0:3089 src=10.1.7.137:57948 dst=10.1.7.136:8080 :method=POST :authority=voting-svc.emojivoto:8080 :path=/emojivoto.v1.VotingService/VoteSpeakNoEvil

$ bin/conduit tapByResource -n emojivoto deploy/web --to svc/voting-svc
req id=0:60254 src=10.1.7.153:43032 dst=10.1.7.152:8080 :method=POST :authority=voting-svc.emojivoto:8080 :path=/emojivoto.v1.VotingService/VoteBalloon
rsp id=0:60254 src=10.1.7.153:43032 dst=10.1.7.152:8080 :status=200 latency=2895µs
end id=0:60254 src=10.1.7.153:43032 dst=10.1.7.152:8080 grpc-status=OK duration=18µs response-length=5B

$ bin/conduit tapByResource ns/emojivoto --to deploy/emoji
req id=0:81 src=10.1.7.137:39482 dst=10.1.7.139:8080 :method=POST :authority=emoji-svc.emojivoto:8080 :path=/emojivoto.v1.EmojiService/ListAll
rsp id=0:81 src=10.1.7.137:39482 dst=10.1.7.139:8080 :status=200 latency=2465µs
end id=0:81 src=10.1.7.137:39482 dst=10.1.7.139:8080 grpc-status=OK duration=19µs response-length=2161B

$ bin/conduit tapByResource -n emojivoto deploy/web --to deploy/emoji
req id=0:226 src=10.1.7.137:39482 dst=10.1.7.139:8080 :method=POST :authority=emoji-svc.emojivoto:8080 :path=/emojivoto.v1.EmojiService/ListAll
rsp id=0:226 src=10.1.7.137:39482 dst=10.1.7.139:8080 :status=200 latency=2122µs
end id=0:226 src=10.1.7.137:39482 dst=10.1.7.139:8080 grpc-status=OK duration=18µs response-length=2161B

$ bin/conduit tapByResource -n emojivoto deploy/web --to deploy/voting
req id=0:254 src=10.1.7.137:57948 dst=10.1.7.136:8080 :method=POST :authority=voting-svc.emojivoto:8080 :path=/emojivoto.v1.VotingService/VotePoop
rsp id=0:254 src=10.1.7.137:57948 dst=10.1.7.136:8080 :status=200 latency=2152µs
end id=0:254 src=10.1.7.137:57948 dst=10.1.7.136:8080 grpc-status=Unknown duration=0µs response-length=0B

$ bin/conduit tapByResource -n emojivoto deploy/web --path "/emojivoto.v1.Emoji"
req id=0:386 src=10.1.7.137:39482 dst=10.1.7.139:8080 :method=POST :authority=emoji-svc.emojivoto:8080 :path=/emojivoto.v1.EmojiService/ListAll
rsp id=0:386 src=10.1.7.137:39482 dst=10.1.7.139:8080 :status=200 latency=1506µs
end id=0:386 src=10.1.7.137:39482 dst=10.1.7.139:8080 grpc-status=OK duration=24µs response-length=2161B

$ bin/conduit tapByResource -n emojivoto deploy/web --path "/emojivoto.v1.Voting"
req id=0:354 src=10.1.7.137:57948 dst=10.1.7.136:8080 :method=POST :authority=voting-svc.emojivoto:8080 :path=/emojivoto.v1.VotingService/VotePrincess
rsp id=0:354 src=10.1.7.137:57948 dst=10.1.7.136:8080 :status=200 latency=1884µs
end id=0:354 src=10.1.7.137:57948 dst=10.1.7.136:8080 grpc-status=OK duration=18µs response-length=5B

$ bin/conduit tapByResource -n emojivoto po/web-58c85b4c84-xq24j --to po/voting-699966dd6d-w522g
req id=0:579 src=10.1.7.137:57948 dst=10.1.7.136:8080 :method=POST :authority=voting-svc.emojivoto:8080 :path=/emojivoto.v1.VotingService/VoteStuckOutTongueWinkingEye
rsp id=0:579 src=10.1.7.137:57948 dst=10.1.7.136:8080 :status=200 latency=2100µs
end id=0:579 src=10.1.7.137:57948 dst=10.1.7.136:8080 grpc-status=OK duration=16µs response-length=5B

olix0r · 2018-04-23T17:38:08Z

controller/tap/server.go

+	// TODO: for now assume it's always a single, flat `All` match list
+	seq := match.GetAll()
+	if seq == nil {
+		return nil, errors.New("no match specified")


how are these errors manifested? We should be returning errors that gRPC can turn into semantic status codes. For instance, in this case I'd expect us to return Unimplemented.

Good point. I've modified all return sites in TapByResource to return gRPC errors. If functions lower in the stack return errors that need propagation, we now first check if it's a gRPC error. If it is, we propagate, if not, we wrap it in a gRPC error.

olix0r · 2018-04-23T17:47:49Z

controller/tap/server.go

+					},
+				}
+			default:
+				return nil, fmt.Errorf("unknown HTTP match type: %v", httpTyped)


error type?

olix0r · 2018-04-23T17:47:54Z

controller/tap/server.go

+			})
+
+		default:
+			return nil, fmt.Errorf("unknown match type: %v", typed)


error type?

olix0r · 2018-04-23T17:52:47Z

controller/cmd/tap/main.go

+		return []string{deployment}, nil
+	}
+
+	pods, err := k8s.NewPodIndex(clientSet, deploymentIndex)


to confirm, all of this was refactored out of NewServer and is not changed. and this is required to support the Tap call?

You are correct this code was moved out of server.go into main.go. The motivation was to enable testing via dependency injection in server_test.go. Now, NewServer accepts fully initialized clients, rather than a Kubernetes config file.

olix0r · 2018-04-23T17:53:36Z

controller/k8s/pods.go

@@ -26,7 +26,7 @@ type podIndex struct {
 	stopCh    chan struct{}
 }

-func NewPodIndex(clientset *kubernetes.Clientset, index cache.IndexFunc) (PodIndex, error) {
+func NewPodIndex(clientset kubernetes.Interface, index cache.IndexFunc) (PodIndex, error) {


is this necessary to support a refactor or just housekeeping?

This particular change was not necessary, but an analogous one was required in replicasets.go to enable passing in a fake k8s client, so err'd on the side of consistency.

olix0r · 2018-04-23T17:56:45Z

controller/tap/server.go

+	}
+
+	if len(pods) == 0 {
+		return fmt.Errorf("No pods found for ResourceSelection: %+v", *req.Target)


Similarly, how are these various errors turned into gRPC statuses?

olix0r · 2018-04-23T17:57:13Z

controller/tap/server.go

+
+	match, err := makeByResourceMatch(req.Match)
+	if err != nil {
+		return nil


is this correct? What is this condition?

Nice catch, this is a bug. I've added a test to catch this in case it regresses. Looks like we've had a similar bug in the original Tap, that is fixed as well, but no tests added as we'll be removing soon.

olix0r · 2018-04-23T17:57:29Z

controller/tap/server.go

+	for event := range events {
+		err := stream.Send(event)
+		if err != nil {
+			return err


error type?

olix0r · 2018-04-23T22:25:15Z

Gopkg.lock

@@ -746,6 +746,6 @@
 [solve-meta]
  analyzer-name = "dep"
  analyzer-version = 1
-  inputs-digest = "f5ff9662192d238efdb260226a886088d05da0924292833660466258fd80c8c0"


what changed in gopkg.lock that requires a go-deps change? afaict this bump looks unnecessary?

without this change the go vet ./... in ci failed:
https://travis-ci.org/runconduit/conduit/jobs/370317684

i suspect it may be changes to various imports, but no dependencies have been added/removed from the repo.

found it by diff'ing the output of bin/dep hash-inputs in master and this branch, new dependency:
k8s.io/apimachinery/pkg/api/errors

olix0r

thanks for addressing my concerns!

The TapByResource endpoint was previously a stub. Implement end-to-end tapByResource functionality, with support for specifying any kubernetes resource(s) as target and destination. Fixes #803, #49 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy added the area/controller label Apr 20, 2018

siggy added this to the 0.4.1 milestone Apr 20, 2018

siggy self-assigned this Apr 20, 2018

siggy requested a review from olix0r April 20, 2018 23:37

siggy added the review label Apr 20, 2018

siggy force-pushed the siggy/tap-service-to-proxy branch from 7fb98db to c28d1c7 Compare April 22, 2018 19:03

siggy mentioned this pull request Apr 22, 2018

CLI doesn't complain when given unknown deployments, pods, or paths #49

Closed

olix0r reviewed Apr 23, 2018

View reviewed changes

olix0r approved these changes Apr 23, 2018

View reviewed changes

Implement TapByResource in Tap Service

efed0be

The TapByResource endpoint was previously a stub. Implement end-to-end tapByResource functionality, with support for specifying any kubernetes resource(s) as target and destination. Fixes #803, #49 Signed-off-by: Andrew Seigner <siggy@buoyant.io>

siggy force-pushed the siggy/tap-service-to-proxy branch from c5ea33c to efed0be Compare April 23, 2018 22:52

siggy merged commit baf4ea1 into master Apr 23, 2018

siggy deleted the siggy/tap-service-to-proxy branch April 23, 2018 23:13

siggy removed the review label Apr 23, 2018

siggy mentioned this pull request Apr 23, 2018

service support in tap command #806

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement TapByResource in Tap Service #827

Implement TapByResource in Tap Service #827

siggy commented Apr 20, 2018 •

edited

Loading

olix0r Apr 23, 2018

siggy Apr 23, 2018

olix0r Apr 23, 2018

olix0r Apr 23, 2018

olix0r Apr 23, 2018

siggy Apr 23, 2018

olix0r Apr 23, 2018

siggy Apr 23, 2018 •

edited

Loading

olix0r Apr 23, 2018

olix0r Apr 23, 2018

siggy Apr 23, 2018

olix0r Apr 23, 2018

olix0r Apr 23, 2018

siggy Apr 23, 2018

siggy Apr 23, 2018

olix0r left a comment

Implement TapByResource in Tap Service #827

Implement TapByResource in Tap Service #827

Conversation

siggy commented Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siggy Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olix0r left a comment

Choose a reason for hiding this comment

siggy commented Apr 20, 2018 •

edited

Loading

siggy Apr 23, 2018 •

edited

Loading