RSDK-9760 - Tunneling over grpc #4745

cheukt · 2025-01-24T22:16:08Z

still working on a small test for the helpers, but wanted to get some feedback early

I was able to run a server on windows and use remote desktop with no issues.

Also able to share both reader/sender and receiver/writer loops in the client/server implementations.

corresponding API PR at viamrobotics/api#623

works better working checkpoint update go mod another checkpoint cleanup fix go mod fix parse refactor minor refactor move tunnel example, final set of changes move log location some fixes doc

cheukt · 2025-01-24T22:17:57Z

robot/client/client.go

+	cancel()
+	// We do close the connection to unblock the reader/sender loop, which is not clean
+	// but there isn't a cleaner way to exit from the reader/sender loop.
+	err = conn.Close()


I don't like that I have to close the connection here in order for reader.Read in ReaderSenderLoop to exit, but not sure if there are better options here.

Same question as above; can you leverage two error channels or an errgroup to communicate an error from one side to the other? Instead of leveraging context cancelation or closing the underlying connection?

using two channels, it feels kind of complicated but works fairly well. will think about how to improve it as a followup, but wanted to stop tinkering with the implementation for now

Handling in followup SGTM if what you have now works and doesn't obviously leak or deadlock.

cheukt · 2025-01-24T22:19:01Z

robot/client/client.go

@@ -1190,6 +1192,64 @@ func (rc *RobotClient) Version(ctx context.Context) (robot.VersionResponse, erro
 	return mVersion, nil
 }

+// Tunnel tunnels data to/from the read writer from/to the destination port on the server. This
+// function will close the connection passed in as part of cleanup.
+func (rc *RobotClient) Tunnel(ctx context.Context, conn net.Conn, dest int) error {


I may add this to the Robot interface, but since this is only useful in the client and doesn't touch the Robot in anyway, I'm inclined to keep it out for now

Keeping it off the robot interface is probably preferable to me.

benjirewis

Nice! Some initial questions/comments.

benjirewis · 2025-01-27T15:34:55Z

examples/tunnel/tunnel.go

@@ -0,0 +1,116 @@
+// main is an example of tunneling traffic over grpc


Any reason to have this example lying around once we implement the CLI version? Or, do we expect users to write their own programmatic Golang code creating their own tunnels?

good point, I'll remove it during the CLI task. In theory, users could write a tunneling program using a passed in connection/rwcloser, but the CLI code should suffice as an example

benjirewis · 2025-01-27T15:35:51Z

go.mod

@@ -75,7 +75,7 @@ require (
 	go.uber.org/atomic v1.11.0
 	go.uber.org/multierr v1.11.0
 	go.uber.org/zap v1.27.0
-	go.viam.com/api v0.1.383
+	go.viam.com/api v0.1.386-0.20250124195735-0d8303ff6186


Cool how did you do this? There's an autogenerated pseudoversion just from PRs? Or from your branch?

it was from a branch on the api repo, so might not work if on a fork. but otherwise, all I did was go get go.viam.com/api@{HASH}

Can update now I assume?

benjirewis · 2025-01-27T15:36:17Z

robot/client/client.go

@@ -1190,6 +1192,64 @@ func (rc *RobotClient) Version(ctx context.Context) (robot.VersionResponse, erro
 	return mVersion, nil
 }

+// Tunnel tunnels data to/from the read writer from/to the destination port on the server. This
+// function will close the connection passed in as part of cleanup.
+func (rc *RobotClient) Tunnel(ctx context.Context, conn net.Conn, dest int) error {


Keeping it off the robot interface is probably preferable to me.

benjirewis · 2025-01-27T15:40:15Z

robot/client/client.go

+	}); err != nil {
+		return err
+	}
+	rc.Logger().CDebugw(ctx, "creating tunnel to server", "port", dest)


[nit] Info level?

benjirewis · 2025-01-27T15:42:16Z

robot/client/client.go

+			// which is why we don't need an EOF field on the request message.
+			client.CloseSend()
+			// By cancelling this ctx, we will close the client, meaning client.Recv() in the RecvWriterLoop will exit
+			// and return an error.


Particularly because of the no stream for id; discarding error, I'm inclined to ask: are there any other synchronization mechanisms between the reader-sender and recv-writer goroutines besides a context cancelation? I buy that this works, but it's a little odd to force an error on the underlying bidi stream (cancel the context) rather than use a channel or something to communicate a required exit between the two goroutines.

Thinking if there's an alternative to mucking with our logs: would instead scheduling a time.AfterFunc(5*time.Second, cancel) help. Such that we only log when our closing takes > 5 seconds to "roundtrip".

IIUC we're properly propagating closing up and down the tunnel, but we just don't give it time to politely disconnect.

added an after func - with the current set of changes we do not get the no stream for id error at all, and usually everything cleans up before the 5 second mark

benjirewis · 2025-01-27T15:43:30Z

robot/client/client.go

+	cancel()
+	// We do close the connection to unblock the reader/sender loop, which is not clean
+	// but there isn't a cleaner way to exit from the reader/sender loop.
+	err = conn.Close()


Same question as above; can you leverage two error channels or an errgroup to communicate an error from one side to the other? Instead of leveraging context cancelation or closing the underlying connection?

benjirewis · 2025-01-27T15:44:11Z

robot/client/client.go

@@ -1190,6 +1192,64 @@ func (rc *RobotClient) Version(ctx context.Context) (robot.VersionResponse, erro
 	return mVersion, nil
 }

+// Tunnel tunnels data to/from the read writer from/to the destination port on the server. This
+// function will close the connection passed in as part of cleanup.
+func (rc *RobotClient) Tunnel(ctx context.Context, conn net.Conn, dest int) error {


Is there any restriction on multiple tunnels? Can I open multiple tunnels to the same destination port at once?

no restrictions - during testing, I find that browsers will open a lot of connections to the same port at the same time.

dgottlieb · 2025-01-27T21:06:52Z

tunnel/tunnel.go

+func ReaderSenderLoop(
+	ctx context.Context,
+	r io.Reader,
+	sendFunc func(buf []byte) error,


~~Did you try using an io.Writer here? The signature seems to largely match. And then I expect we could use io.Copy: https://pkg.go.dev/io#Copy~~

Cheuk and I talked offline. We feel that might just move the for-loop around instead of getting rid of it. I'm happy to not worry about it.

dgottlieb · 2025-01-27T21:07:33Z

tunnel/tunnel.go

+func RecvWriterLoop(
+	ctx context.Context,
+	w io.Writer,
+	recvFunc func() ([]byte, error),


~~Symmetrical question, it seems an io.Reader could be substituted allowing us to go to io.Copy~~

Talked offline

dgottlieb · 2025-01-27T21:23:09Z

robot/server/server.go

+		return fmt.Errorf("failed to receive first message from stream: %w", err)
+	}
+
+	dest := strconv.Itoa(int(req.DestinationPort))


Without finding the proto for this, I'm guessing DestinationPort is typed as an int32? Or is this a uint thing?

it is a uint32

dgottlieb · 2025-01-27T21:25:33Z

robot/server/server.go

+	)
+	wg.Add(1)
+	utils.PanicCapturingGo(func() {
+		defer func() {


The extra func() { }() doesn't seem necessary. In case this was an accidental leftover from a time where the defer may have had multiple statements to process?

add another statement, but yes this was accidental

dgottlieb · 2025-01-27T21:27:40Z

robot/server/server.go

+		defer func() {
+			wg.Done()
+		}()
+		sendFunc := func(data []byte) error { return srv.Send(&pb.TunnelResponse{Data: data}) }


Being our buffer is set to 32KB, this seems safe. But maybe worth calling out that assumption about input size here. As our gRPC limits can hypothetically get angry.

dgottlieb · 2025-01-27T21:31:43Z

robot/server/server.go

+		}
+		return req.Data, nil
+	}
+	recvWriterErr := tunnel.RecvWriterLoop(srv.Context(), conn, recvFunc, s.robot.Logger())


I might have swapped the arguments to be recvFunc, conn to match the naming of RecvWriterLoop. But given the types are different, I don't see a typo "incorrectly" compiling.

dgottlieb · 2025-01-27T21:48:30Z

robot/client/client.go

+			// which is why we don't need an EOF field on the request message.
+			client.CloseSend()
+			// By cancelling this ctx, we will close the client, meaning client.Recv() in the RecvWriterLoop will exit
+			// and return an error.


Thinking if there's an alternative to mucking with our logs: would instead scheduling a time.AfterFunc(5*time.Second, cancel) help. Such that we only log when our closing takes > 5 seconds to "roundtrip".

IIUC we're properly propagating closing up and down the tunnel, but we just don't give it time to politely disconnect.

cheukt · 2025-01-28T20:51:16Z

tunnel/tunnel.go

+		// For bidi streaming, Recv should be called on the client/server until it errors.
+		// See [grpc.NewStream] for related docs.
+		//
+		// If the reader/sender loop is done, stop copying bytes as that means the connection is no longer accepting bytes.
+		select {
+		case <-rsDone:
+			continue
+		default:
+		}


here, we continue until Recv errors. we're just straight discarding any bytes we get because the connection is likely no longer accepting bytes if reader/sender loop is done. In theory, maybe we only need to skip the Copy if the reader sender loop exits with a connection error of some kind, but in practice I start seeing closed connection/broken pipe errors if I try to make the condition more specific.

cheukt · 2025-01-28T20:58:00Z

made some hopefully final impl changes - main difference is that we no longer cancel until 5s after we exit the reader/sender loop. The receiver also continues to receive until it gets an error but will skip copying the data to the net.Conn if we expect it to be closed. working on some tests for ReaderSenderLoop and RecvWriterLoop. more comprehensive tests tracked at https://viam.atlassian.net/browse/RSDK-9852

benjirewis

LGTM!

benjirewis · 2025-01-28T21:56:53Z

go.mod

@@ -75,7 +75,7 @@ require (
 	go.uber.org/atomic v1.11.0
 	go.uber.org/multierr v1.11.0
 	go.uber.org/zap v1.27.0
-	go.viam.com/api v0.1.383
+	go.viam.com/api v0.1.386-0.20250124195735-0d8303ff6186


Can update now I assume?

benjirewis · 2025-01-28T21:57:43Z

robot/client/client.go

+			// to show up because the handler will have exited before we receive the last messages from the server.
+			// This is not an issue and is expected.
+			timerMu.Lock()
+			timer = time.AfterFunc(5*time.Second, cancel)


[q] How does this delay appear to the user? Just a five second delay in ending the tunnel?

this would be a failsafe, so at most the delay will be 5s

benjirewis · 2025-01-28T21:59:12Z

robot/client/client.go

+	cancel()
+	// We do close the connection to unblock the reader/sender loop, which is not clean
+	// but there isn't a cleaner way to exit from the reader/sender loop.
+	err = conn.Close()


Handling in followup SGTM if what you have now works and doesn't obviously leak or deadlock.

benjirewis · 2025-01-28T22:00:01Z

tunnel/tunnel.go

+	"go.viam.com/rdk/logging"
+)
+
+func filterError(ctx context.Context, err error, closeChan <-chan struct{}, logger logging.Logger) error {


Nice read-only on the channel 🫡 . This is a cool implementation.

cheukt · 2025-01-28T22:53:34Z

added a few tests for the helpers, will merge as soon as tests pass

working tunnel

7251ec6

works better working checkpoint update go mod another checkpoint cleanup fix go mod fix parse refactor minor refactor move tunnel example, final set of changes move log location some fixes doc

cheukt requested review from dgottlieb and benjirewis January 24, 2025 22:16

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jan 24, 2025

cheukt commented Jan 24, 2025

View reviewed changes

cheukt added 2 commits January 24, 2025 17:19

return error from closing conn

bb23f01

Merge branch 'main' into play

ed1ce6e

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 24, 2025

lint

aec741c

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 24, 2025

lint

192d407

benjirewis reviewed Jan 27, 2025

View reviewed changes

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 27, 2025

nits

cf97e89

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 27, 2025

cheukt added 2 commits January 27, 2025 14:35

minor

cdd26d4

Merge branch 'main' into play

75fa616

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 27, 2025

dgottlieb approved these changes Jan 27, 2025

View reviewed changes

cheukt added 2 commits January 28, 2025 15:02

pr feedback

94dbdbb

Merge branch 'main' into play

86e014b

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 28, 2025

add comment

66b982d

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 28, 2025

cheukt commented Jan 28, 2025

View reviewed changes

cheukt changed the title ~~RSDK-9780 - Tunneling over grpc~~ RSDK-9760 - Tunneling over grpc Jan 28, 2025

benjirewis approved these changes Jan 28, 2025

View reviewed changes

cheukt added 2 commits January 28, 2025 17:25

add tests

53ff83a

mod update

49e79cd

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 28, 2025

mod tidy

9a8c5f5

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 28, 2025

lint

7fb3bff

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 28, 2025

cheukt merged commit dccdac0 into main Jan 28, 2025
16 checks passed

cheukt deleted the grpc-tunnelling branch January 28, 2025 22:55

		@@ -0,0 +1,116 @@
		// main is an example of tunneling traffic over grpc

RSDK-9760 - Tunneling over grpc #4745

RSDK-9760 - Tunneling over grpc #4745

Conversation

cheukt commented Jan 24, 2025 • 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

benjirewis left a 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

cheukt Jan 27, 2025 • 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

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

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

cheukt commented Jan 28, 2025

benjirewis left a 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

cheukt commented Jan 28, 2025

cheukt commented Jan 24, 2025 •

edited

Loading

cheukt Jan 27, 2025 •

edited

Loading