Revamp GRPC Port forwarding tunnels to use existing proxy #2985

balajiv113 · 2024-12-06T12:35:24Z

Fixes #2968 and several GRPC port forwarding issues.

FYI - Running ddev testsuite to verify end-end (Done)
https://buildkite.com/test-305/ddev/builds/19#01939bef-af2a-4b7d-9ea8-fa1ca461f037

nirs · 2024-12-06T15:21:35Z

pkg/portfwdserver/server.go

 	return len(p), err
 }
+
+func (g GRPCServerRW) Read(p []byte) (n int, err error) {


Should we add a grpc module providing the server and client so we have the core of this code in one place? This will tiny dead code in the guest and host agent that may not be optimized out, but it will make it easier to understand and maintain when all the pieces are the same place.

I understand the reason behind. But i don't think we can create a common one. As server and client are 2 different one. Create a common will create cyclic dependency as well.

At max we can do is, have a Base conn with method like setDeadline, setWriteDeadline etc. But for now i will avoid it. If you need for sure i can do that as well

+1 to a base type.

nirs · 2024-12-06T15:22:39Z

pkg/portfwdserver/server.go

+}
+
+func (g GRPCServerRW) LocalAddr() net.Addr {
+	return &net.UnixAddr{Name: "grpc", Net: "unixpacket"}


This seems like the right way to support good logging, better than the NamedReadWriter added in #2944.

This is a pointless comment. The purpose of LocalAddr is to identify the connection (which this doesn't do) while the NamedReadWriter in #2944 is identifying the purpose of the connection.

This implementation here is a lie, it doesn't identify the connection and the address it returns cannot be used in any of the normal ways that net.Addr is used.

nirs · 2024-12-06T15:23:16Z

pkg/portfwdserver/server.go

+}
+
+func (g GRPCServerRW) Close() error {
+	return nil


Why this does nothing?

Server doesn't have any stream closing methods. So it doesn't do anything

So this is implemented only to satisfy the net.Conn interface? Should we log a debug message when about ignoring Close?

This code can be integrated in other code expecting that Close() does something. If doing nothing breaks something, the debug log will help to diagnose the issue.

debug logs are useful only in dev side, also grpc server doesn't provide close, its purely handled on client itself. I don't think adding debug is anywhere useful.

I don't want to spam it in our guestagent logs which doesn't give much value.

Returning nil on Close is fine if there is nothing to do. SetDeadline should return an error if it is not implemented.

pkg/portfwdserver/server.go

pkg/portfwd/client.go

nirs · 2024-12-06T15:40:09Z

pkg/portfwd/client.go

+}
+
+func (g GrpcClientRW) SetDeadline(t time.Time) error {
+	return nil


Why we don't implement it? should we log a warning to make the issue visible?

Too much logging will become a noise since these are connections. It will be called multiple times and its doesn't help much. SetDeadline is not even usecul in our case as ours is a grpc stream

Or just panic. If these functions don't do what they're supposed to (and are never called) a panic would be proof of that.

We cannot panic on all cases.

These are left as NO-OP wantedly. These methods may / will be called but its of no value to us as its a grpc tunnel wrapped conn

no-op on SetDeadline is not a reasonable thing to do. It should return an error.

@tamird Please understand the full flow before commenting. All left like that for a reason.

UDP Proxy internally will call SetReadDeadline so we cannot throw error. It will be no-op only. That's why all SetDeadline like methods are returning null.

no-op on SetDeadline is not a reasonable thing to do. It should return an error.

I think you are talking past each other. SetDeadline() must exist to implement the interface, but there is not actual implementation of that concept for this particular implementation (there is no deadline). Therefore the method does nothing. It doesn't mean it is an error; just a vestigial method that is not being used.

pkg/portfwd/client.go

nirs · 2024-12-06T17:21:31Z

Since this change convert the GRPC endpoint to net.Con interface, why not use it with bcopy? #2944

It has the same terminating issue as tcpproxy, but at least it logs errors. It will be easier to fix it to return an error and terminate the copies.

pkg/portfwd/client.go

balajiv113 · 2024-12-09T08:26:07Z

Ran DDEV Tests in buildkite. All tests passed

https://buildkite.com/test-305/ddev/builds/19#01939bef-af2a-4b7d-9ea8-fa1ca461f037

pkg/portfwdserver/server.go

tamird · 2024-12-10T15:15:42Z

pkg/portfwdserver/server.go

+}
+
+func (g GRPCServerRW) LocalAddr() net.Addr {
+	return &net.UnixAddr{Name: "grpc", Net: "unixpacket"}


This is a pointless comment. The purpose of LocalAddr is to identify the connection (which this doesn't do) while the NamedReadWriter in #2944 is identifying the purpose of the connection.

This implementation here is a lie, it doesn't identify the connection and the address it returns cannot be used in any of the normal ways that net.Addr is used.

pkg/portfwd/client.go

pkg/portfwdserver/server.go

tamird

I don't love this.

Delegating to a 3rd party library is a nice idea but as @nirs points out the error handling is subtle and in the case of this library, dubious. We also have to provide a largely fake interface implementation and instead of panicking (strict) we return nonsense (lax).

pkg/portfwd/client.go

balajiv113 · 2024-12-20T06:51:01Z

@nirs & @tamird - Updated logic to use bicopy.

Re-verified the ddev run as well
https://buildkite.com/test-305/ddev/builds/21

Signed-off-by: Balaji Vijayakumar <[email protected]>

AkihiroSuda

Thanks

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [lima-vm/lima](https://github.com/lima-vm/lima) | patch | `v1.0.2` -> `v1.0.3` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>lima-vm/lima (lima-vm/lima)</summary> ### [`v1.0.3`](https://github.com/lima-vm/lima/releases/tag/v1.0.3) [Compare Source](lima-vm/lima@v1.0.2...v1.0.3) #### Changes - QEMU: - Support Apple M4 ([#3032](lima-vm/lima#3032)) - gRPC port forwarder: - Improvements on stability ([#2971](lima-vm/lima#2971), [#2985](lima-vm/lima#2985), etc. thanks to [@nirs](https://github.com/nirs) [@balajiv113](https://github.com/balajiv113)) - Templates: - `archlinux`: allow 9p again ([#3048](lima-vm/lima#3048)) - `centos-stream-10`: New template ([#3023](lima-vm/lima#3023), [#3047](lima-vm/lima#3047), thanks to [@afbjorklund](https://github.com/afbjorklund)) - `opensuse`: disable virtiofs due to the lack of the kernel module in the default installation ([#3056](lima-vm/lima#3056)) - Updated to the latest revisions ([#3043](lima-vm/lima#3043)) Full changes: https://github.com/lima-vm/lima/milestone/52?closed=1 Thanks to [@PascalBourdier](https://github.com/PascalBourdier) [@afbjorklund](https://github.com/afbjorklund) [@alexandear](https://github.com/alexandear) [@balajiv113](https://github.com/balajiv113) [@cpick](https://github.com/cpick) [@jandubois](https://github.com/jandubois) [@nirs](https://github.com/nirs) [@olamilekan000](https://github.com/olamilekan000) #### Usage ```console [macOS]$ limactl create [macOS]$ limactl start ... INFO[0029] READY. Run `lima` to open the shell. [macOS]$ lima uname Linux ``` *** The binaries were built automatically on GitHub Actions. The build log is available for 90 days: https://github.com/lima-vm/lima/actions/runs/12517401436 The sha256sum of the SHA256SUMS file itself is `69423d9f9044fc9264925d24cd38c1d0efb4367cfb46c568313f53d6f0ed7ee2` . *** Release manager: [@AkihiroSuda](https://github.com/AkihiroSuda) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

balajiv113 force-pushed the grpc-pw branch from 76f929b to 7ec09c2 Compare December 6, 2024 12:36

nirs reviewed Dec 6, 2024

View reviewed changes

nirs mentioned this pull request Dec 6, 2024

portfwd: Fix handling the last UDP message #2971

Merged

nirs reviewed Dec 6, 2024

View reviewed changes

pkg/portfwd/client.go Show resolved Hide resolved

nirs reviewed Dec 6, 2024

View reviewed changes

pkg/portfwd/client.go Show resolved Hide resolved

balajiv113 force-pushed the grpc-pw branch from 7ec09c2 to 34ec468 Compare December 9, 2024 08:23

balajiv113 force-pushed the grpc-pw branch 2 times, most recently from 48b2d5b to 07d39e5 Compare December 9, 2024 08:48

balajiv113 requested a review from a team December 9, 2024 09:06

AkihiroSuda added this to the v1.0.3 milestone Dec 10, 2024

AkihiroSuda added the area/portfwd label Dec 10, 2024

nirs mentioned this pull request Dec 10, 2024

cleanup: refactor and use bicopy everywhere #2944

Open

tamird reviewed Dec 10, 2024

View reviewed changes

balajiv113 force-pushed the grpc-pw branch from 07d39e5 to e965c2c Compare December 11, 2024 07:04

AkihiroSuda requested review from tamird and nirs December 14, 2024 15:43

tamird reviewed Dec 14, 2024

View reviewed changes

pkg/portfwd/client.go Outdated Show resolved Hide resolved

pkg/portfwd/client.go Show resolved Hide resolved

pkg/portfwd/client.go Show resolved Hide resolved

pkg/portfwd/client.go Show resolved Hide resolved

balajiv113 force-pushed the grpc-pw branch 2 times, most recently from 9a28bf8 to 0be4bc1 Compare December 20, 2024 06:43

balajiv113 force-pushed the grpc-pw branch from 0be4bc1 to d9dff89 Compare December 20, 2024 06:51

Revamp GRPC Port forwarding tunnels to use existing proxy

17b2d58

Signed-off-by: Balaji Vijayakumar <[email protected]>

balajiv113 force-pushed the grpc-pw branch from d9dff89 to 17b2d58 Compare December 24, 2024 06:38

AkihiroSuda approved these changes Dec 24, 2024

View reviewed changes

AkihiroSuda merged commit 5fb9353 into lima-vm:master Dec 24, 2024
29 checks passed

balajiv113 deleted the grpc-pw branch December 24, 2024 07:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp GRPC Port forwarding tunnels to use existing proxy #2985

Revamp GRPC Port forwarding tunnels to use existing proxy #2985

balajiv113 commented Dec 6, 2024 •

edited

Loading

nirs Dec 6, 2024

balajiv113 Dec 9, 2024

tamird Dec 23, 2024

nirs Dec 6, 2024

tamird Dec 10, 2024

nirs Dec 6, 2024

balajiv113 Dec 7, 2024

nirs Dec 7, 2024

balajiv113 Dec 9, 2024

tamird Dec 23, 2024

nirs Dec 6, 2024

balajiv113 Dec 8, 2024

tamird Dec 20, 2024

balajiv113 Dec 20, 2024

tamird Dec 23, 2024

balajiv113 Dec 24, 2024

jandubois Dec 24, 2024

nirs commented Dec 6, 2024

balajiv113 commented Dec 9, 2024

tamird Dec 10, 2024

tamird left a comment

balajiv113 commented Dec 20, 2024

AkihiroSuda left a comment

Revamp GRPC Port forwarding tunnels to use existing proxy #2985

Revamp GRPC Port forwarding tunnels to use existing proxy #2985

Conversation

balajiv113 commented Dec 6, 2024 • 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

nirs commented Dec 6, 2024

balajiv113 commented Dec 9, 2024

Choose a reason for hiding this comment

tamird left a comment

Choose a reason for hiding this comment

balajiv113 commented Dec 20, 2024

AkihiroSuda left a comment

Choose a reason for hiding this comment

balajiv113 commented Dec 6, 2024 •

edited

Loading