client: add sidechannel support
Implements gitlab-com/gl-infra/scalability#1303.
This change adds publicly exported code that allows Gitaly clients to accept sidechannel connections coming back from a Gitaly server they have connected to. This is done via a new dial function DialSidechannel.
We need this code to be public so that Workhorse can import it. Without this code, Workhorse won't be able to use sidechannel RPC's.
Merge request reports
Activity
assigned to @jacobvosmaer-gitlab
added groupscalability label
added devopscreate label
1 Error e64a6c21: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines. 1 Warning Please capitalize the merge request title Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, mention them as you normally would! Danger does not automatically notify them for you.
Category Reviewer Maintainer ~ Toon Claes ( @toon
) (UTC+2, same timezone as@jacobvosmaer-gitlab
)James Fargher ( @proglottis
) (UTC+13, 11 hours ahead of@jacobvosmaer-gitlab
)If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerEdited by GitalyBot- Resolved by Jacob Vosmaer
- Resolved by Jacob Vosmaer
@qmnguyen0711 could you review this?
requested review from @qmnguyen0711
added sectiondev label
Setting label groupscalability based on
@jacobvosmaer-gitlab
's group.Setting label(s) Category:Scalability based on groupscalability.
Setting label(s) Category:Scalability based on groupscalability.
- Automatically resolved by Jacob Vosmaer
- Automatically resolved by Jacob Vosmaer
- Resolved by Jacob Vosmaer
- Resolved by Jacob Vosmaer
- Resolved by Jacob Vosmaer
- Resolved by Quang-Minh Nguyen
added 24 commits
-
bb16fd11...dc297bbc - 23 commits from branch
master
- d6a4a1ba - client: add sidechannel support
-
bb16fd11...dc297bbc - 23 commits from branch
- Resolved by Quang-Minh Nguyen
Setting label(s) Category:Scalability based on groupscalability.
added Category:Scalability label
- Resolved by Jacob Vosmaer
added 42 commits
-
5e03706a...1ec2f768 - 39 commits from branch
master
- 098ee384 - sidechannel: remove waiter.Wait()
- a12b72cf - sidechannel: add proxy middleware
- 0ae58d09 - client: add sidechannel support
Toggle commit list-
5e03706a...1ec2f768 - 39 commits from branch
mentioned in merge request !3862 (merged)
- Automatically resolved by Jacob Vosmaer
requested review from @toon and @pks-t and removed review request for @qmnguyen0711
- Resolved by Jacob Vosmaer
@jacobvosmaer-gitlab AFAICT this MR looks good, I've just added one non-blocking issue. Patrick is OOO till Wednesday tho, so you might want to reassign another second reviewer. @jcaigitlab Would mind reviewing this? Although I'm not sure you're already up-to-date on the intentions of the sidechannel?
removed review request for @toon
changed milestone to %14.4
requested review from @jcaigitlab and removed review request for @pks-t
requested review from @toon
removed review request for @toon
added 9 commits
-
1d72aee5...743b3226 - 8 commits from branch
master
- 8dc11259 - client: add sidechannel support
-
1d72aee5...743b3226 - 8 commits from branch
215 return err 216 } 217 buf, err := io.ReadAll(conn) 218 if err != nil { 219 return err 220 } 221 if string(buf) != "pong" { 222 return fmt.Errorf("expected pong, got %q", buf) 223 } 224 225 return nil 226 }) 227 defer scw.Close() 228 229 req := &healthpb.HealthCheckRequest{Service: "test sidechannel"} 230 _, err = healthpb.NewHealthClient(conn).Check(ctx, req) @jcaigitlab for context: what happens here is that on the client side, we register a sidechannel callback into the outbound request context. Then we make a regular gRPC call. On the server side, we retrieve the other end of the sidechannel, and then we can exchange bytes via Read and Write. After the gRPC call, the client calls
scw.Close()
to learn if the callback ran successfully.
- client/sidechannel.go 0 → 100644
47 // value of the callback. If the callback has not been called yet, Close 48 // returns an error immediately. 49 func (w *SidechannelWaiter) Close() error { return w.waiter.Close() } 50 51 // SidechannelConn allows a client to read and write bytes with less 52 // overhead than doing so via gRPC messages. 53 type SidechannelConn interface { 54 io.ReadWriter 55 56 // CloseWrite tells the server we won't write any more data. We can still 57 // read data from the server after CloseWrite(). A typical use case is in 58 // an RPC where the byte stream has a request/response pattern: the 59 // client then uses CloseWrite() to signal the end of the request body. 60 // When the client calls CloseWrite(), the server receives EOF. 61 CloseWrite() error 62 } @jacobvosmaer-gitlab I detect some missing pieces while writing integration tests for workhorse client implementation. Sidechannel server setup is complicated. It consists of listenmux, backchannel, and sidechannel. Writing a mock server for it is not easy. I managed to create a working mock Gitaly server sidechannel implementation at gitlab!71047 (diffs). However, I have to replicate a significant amount of codes from Gitaly repo. So, I would like to seek your opinion. Should we export more things for testing purpose, or code replication is acceptable in this case?
If we don't want to replicate the codes, we will have to export:
- (Optional) Listenmux. It's not mandatory to export listenmux, as it's a thin layer to reach Backchannel server handhshaker.
- Backchannel ServerHandshaker and some utility methods to access Yamux session inside RPC handler.
- Sidechannel ServerConn.
@qmnguyen0711 what if we export this from the
gitaly/client
package:func TestSidechannelServer( logger *logrus.Entry, creds credentials.TransportCredentials, handler func(interface{}, grpc.ServerStream, io.ReadWriteCloser) error, ) []grpc.ServerOption { lm := listenmux.New(creds) lm.Register(backchannel.NewServerHandshaker(logger, backchannel.NewRegistry(), nil)) return []grpc.ServerOption{ grpc.Creds(lm), grpc.UnknownServiceHandler(func(srv interface{}, stream grpc.ServerStream) error { conn, err := sidechannel.OpenSidechannel(stream.Context()) if err != nil { return err } defer conn.Close() return handler(srv, stream, conn) }), } }
It's a little clunky but you can mock-implement any RPC you want to test in Workhorse with an UnknownServiceHandler.
Edited by Jacob Vosmaer@jacobvosmaer-gitlab I like this idea. In theory, this approach is much more slim than the recent approach. Let me try this out with my workhorse pending MR to see whether it works well or not
@jacobvosmaer-gitlab Thanks for adding the mock server. The integration into workhorse works very well: gitlab!71047 (diffs). I don't need to export any internal data structures nor exporting internal methods anymore.
mentioned in merge request gitlab!71047 (merged)
- client/sidechannel.go 0 → 100644
54 55 // SidechannelConn allows a client to read and write bytes with less 56 // overhead than doing so via gRPC messages. 57 type SidechannelConn interface { 58 io.ReadWriter 59 60 // CloseWrite tells the server we won't write any more data. We can still 61 // read data from the server after CloseWrite(). A typical use case is in 62 // an RPC where the byte stream has a request/response pattern: the 63 // client then uses CloseWrite() to signal the end of the request body. 64 // When the client calls CloseWrite(), the server receives EOF. 65 CloseWrite() error 66 } 67 68 // TestSidechannelServer allows downstream consumers of this package to 69 // create mock sidechannel gRPC servers. 147 153 } 148 154 } 149 155 156 func TestDialSidechannel(t *testing.T) { 157 if emitProxyWarning() { 158 t.Log("WARNING. Proxy configuration detected from environment settings. This test failure may be related to proxy configuration. Please process with caution") 159 } 160 161 stop, connectionMap := startListeners(t, func(creds credentials.TransportCredentials) *grpc.Server { 162 return grpc.NewServer(TestSidechannelServer(newLogger(t), creds, func( 163 _ interface{}, 164 stream grpc.ServerStream, 165 sidechannelConn io.ReadWriteCloser, 166 ) error { 167 if method, ok := grpc.Method(stream.Context()); !ok || method != "/grpc.health.v1.Health/Check" { 147 153 } 148 154 } 149 155 156 func TestDialSidechannel(t *testing.T) { 157 if emitProxyWarning() { @jcaigitlab thanks, I don't know how I ended up duplicating that. I've removed one of the warnings.
@jacobvosmaer-gitlab just a non-blocking nit. Looks good to me!
just ping me on the response to my question and I can merge this
enabled an automatic merge when the pipeline for 9285629c succeeds
mentioned in commit 28f93e63
added workflowstaging-canary label
added workflowstaging label and removed workflowstaging-canary label
added workflowcanary label and removed workflowstaging label
added workflowproduction label and removed workflowcanary label
added releasedcandidate label
added releasedpublished label and removed releasedcandidate label
added workflowstaging-ref label and removed workflowproduction label
Due to an issue the workflowstaging-ref label was incorrectly applied to this merge request. Re-setting back to workflowproduction using
https://gitlab.com/gitlab-org/release-tools/-/merge_requests/1620
added workflowproduction label and removed workflowstaging-ref label