Skip to content
Snippets Groups Projects

client: add sidechannel support

Merged Jacob Vosmaer requested to merge jv-sidechannel-client into master
5 unresolved threads

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.

Edited by Jacob Vosmaer

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Jacob Vosmaer requested review from @qmnguyen0711

    requested review from @qmnguyen0711

  • Setting label groupscalability based on @jacobvosmaer-gitlab's group.

  • 🤖 GitLab Bot 🤖 added 1 deleted label

    added 1 deleted label

  • Quang-Minh Nguyen
  • Quang-Minh Nguyen
  • Quang-Minh Nguyen
  • Jacob Vosmaer added 24 commits

    added 24 commits

    Compare with previous version

  • Jacob Vosmaer added 1 commit

    added 1 commit

    • 854c114b - client: add sidechannel support

    Compare with previous version

  • Jacob Vosmaer marked this merge request as ready

    marked this merge request as ready

  • Jacob Vosmaer marked this merge request as draft

    marked this merge request as draft

  • Quang-Minh Nguyen
  • Jacob Vosmaer added 1 commit

    added 1 commit

    • 5e03706a - client: add sidechannel support

    Compare with previous version

  • Jacob Vosmaer added 42 commits

    added 42 commits

    Compare with previous version

  • Jacob Vosmaer added 1 commit

    added 1 commit

    • 26ad8572 - client: add sidechannel support

    Compare with previous version

  • Jacob Vosmaer marked this merge request as ready

    marked this merge request as ready

  • Jacob Vosmaer mentioned in merge request !3862 (merged)

    mentioned in merge request !3862 (merged)

  • Jacob Vosmaer added 1 commit

    added 1 commit

    • bed59deb - client: add sidechannel support

    Compare with previous version

  • Jacob Vosmaer
  • Jacob Vosmaer added 1 commit

    added 1 commit

    • 6c9c36eb - client: add sidechannel support

    Compare with previous version

  • Jacob Vosmaer changed the description

    changed the description

  • Jacob Vosmaer changed the description

    changed the description

  • Jacob Vosmaer requested review from @toon and @pks-t and removed review request for @qmnguyen0711

    requested review from @toon and @pks-t and removed review request for @qmnguyen0711

  • Toon Claes
  • Toon Claes approved this merge request

    approved this merge request

  • @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?

  • Toon Claes removed review request for @toon

    removed review request for @toon

  • Toon Claes changed milestone to %14.4

    changed milestone to %14.4

  • Jacob Vosmaer requested review from @jcaigitlab and removed review request for @pks-t

    requested review from @jcaigitlab and removed review request for @pks-t

  • Jacob Vosmaer added 1 commit

    added 1 commit

    • 1d72aee5 - client: add sidechannel support

    Compare with previous version

  • Jacob Vosmaer requested review from @toon

    requested review from @toon

  • Jacob Vosmaer removed review request for @toon

    removed review request for @toon

  • Jacob Vosmaer added 9 commits

    added 9 commits

    Compare with previous version

  • 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.

    • Please register or sign in to reply
  • 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 :thumbsup:

    • @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.

    • Please register or sign in to reply
  • mentioned in merge request gitlab!71047 (merged)

  • Jacob Vosmaer added 1 commit

    added 1 commit

    • 1c070927 - client: add sidechannel support

    Compare with previous version

  • 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.
    • This is here because the Workhorse test suite uses a mock Gitaly server to test that proxying works correctly. This will let Workhorse create a mock Gitaly server that supports sidechannels.

    • The reason TestSidechannelServer looks this way is that we on the one hand want to export some way for Workhorse to set up a test gRPC+sidechannel server, while on the other hand we want to minimize how much Go API we export from the Gitaly repo.

    • Please register or sign in to reply
  • 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() {
  • John Cai approved this merge request

    approved this merge request

  • @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

  • Jacob Vosmaer added 1 commit

    added 1 commit

    • e64a6c21 - client: add sidechannel support

    Compare with previous version

  • John Cai enabled an automatic merge when the pipeline for 9285629c succeeds

    enabled an automatic merge when the pipeline for 9285629c succeeds

  • merged

  • John Cai mentioned in commit 28f93e63

    mentioned in commit 28f93e63

  • added workflowcanary label and removed workflowstaging label

  • added workflowproduction label and removed workflowcanary 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

  • Please register or sign in to reply
    Loading