Skip to content

Import gvisor-tap-vsock/pkg/tcpproxy into pkg/tcpproxy #3686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda added this to the v1.2.0 milestone Jul 4, 2025
@AkihiroSuda AkihiroSuda requested a review from balajiv113 July 4, 2025 06:47
@AkihiroSuda AkihiroSuda force-pushed the fix-3685 branch 2 times, most recently from a3eb1bb to f946103 Compare July 4, 2025 06:54
balajiv113
balajiv113 previously approved these changes Jul 4, 2025
Copy link
Member

@balajiv113 balajiv113 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍 Thanks for the updates

@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented Jul 4, 2025

Failures on WSL2 🤔
https://github.com/lima-vm/lima/actions/runs/16067751826/job/45346747240?pr=3686

❌ Forwarding TCP from 127.0.0.2:4031 to 172.29.160.1:4031
   Guest received: ''
❌ Forwarding TCP from 0.0.0.0:4040 to 127.0.0.1:4040
   Guest received: ''
❌ Forwarding TCP from [::]:4041 to 127.0.0.1:4041
   Guest received: ''

proxy := tcpproxy.DialProxy{DialContext: func(_ context.Context, _, _ string) (net.Conn, error) {
return conn, nil
}}
proxy.HandleConn(rw)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unlike bicopy.Bicopy(), this ignores errors silently. I don't think this library should be used for anything in the current state.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should we do then ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Port forwarding seems to be lima core functionality, and using a library makes it hard to get good logging, so It think the best way is to copy the missing bits from tcpproxy into our own implementation.

This may be related:
inetaf/tcpproxy#46

tcpproxy seems to be about

Package tcpproxy lets users build TCP proxies, optionally making routing decisions based on HTTP/1 Host headers and the SNI hostname in TLS connections.

Our usage seems to be hacky way to reuse part of the library.

It also says:

This package makes no API stability promises. If you depend on it, vendor it.

Other options:

  • Log errors in the wrappers we pass to tcpproxy
  • Fix tcpproxy to log errors properly

If nobody can own this code in lima we can use tcpproxy as a temporary quick fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Port forwarding seems to be lima core functionality, and using a library makes it hard to get good logging, so It think the best way is to copy the missing bits from tcpproxy into our own implementation.

I have no idea how big tcpproxy is, but we have copied single file implementations from other projects before:

ag adapted
pkg/hostagent/dns/dns.go
4:// This file has been adapted from https://github.com/norouter/norouter/blob/v0.6.4/pkg/agent/dns/dns.go

pkg/reflectutil/reflectutil.go
4:// This file has been adapted from https://github.com/containerd/nerdctl/blob/v1.0.0/pkg/reflectutil/reflectutil.go

So assuming the implementation isn't too big, I think importing it gives us the maximum flexibility going forward.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Port forwarding seems to be lima core functionality, and using a library makes it hard to get good logging, so It think the best way is to copy the missing bits from tcpproxy into our own implementation.

I have no idea how big tcpproxy is

tcpproxy is pretty small, and we basically use one function:
https://github.com/inetaf/tcpproxy/blob/c4b9df066048ad2ab5c32235362fa94444a24ebe/tcpproxy.go#L376

func (dp *DialProxy) HandleConn(src net.Conn) {
	ctx := context.Background()
	var cancel context.CancelFunc
	if dp.DialTimeout >= 0 {
		ctx, cancel = context.WithTimeout(ctx, dp.dialTimeout())
	}
	dst, err := dp.dialContext()(ctx, "tcp", dp.Addr)
	if cancel != nil {
		cancel()
	}
	if err != nil {
		dp.onDialError()(src, err)
		return
	}
	defer dst.Close()

	if err = dp.sendProxyHeader(dst, src); err != nil {
		dp.onDialError()(src, err)
		return
	}
	defer src.Close()

	if ka := dp.keepAlivePeriod(); ka > 0 {
		for _, c := range []net.Conn{src, dst} {
			if c, ok := tcpConn(c); ok {
				c.SetKeepAlive(true)
				c.SetKeepAlivePeriod(ka)
			}
		}
	}

	errc := make(chan error, 2)
	go proxyCopy(errc, src, dst)
	go proxyCopy(errc, dst, src)
	<-errc
	<-errc
}

In this file we use it to proxy packets between GRPC endpoint and net.Con - both are connected, so we hack the proxy DialContext function to not dial anything.

We don't need the sendProxyHeader() thing since we don't to the optional http proxy feature.

I'm not sure if the keepalive is needed for our use case, but GRPC is not a net.TCPCon so it does nothing for it.

So we are left with:

	defer dst.Close()
	defer src.Close()

	if ka := dp.keepAlivePeriod(); ka > 0 {
		for _, c := range []net.Conn{src, dst} {
			if c, ok := tcpConn(c); ok {
				c.SetKeepAlive(true)
				c.SetKeepAlivePeriod(ka)
			}
		}
	}

	errc := make(chan error, 2)
	go proxyCopy(errc, src, dst)
	go proxyCopy(errc, dst, src)
	<-errc
	<-errc

Note how the errors are dropped silently to make the user life more interesting.

proxyCopy is:

func proxyCopy(errc chan<- error, dst, src net.Conn) {
	defer closeRead(src)
	defer closeWrite(dst)

	// Before we unwrap src and/or dst, copy any buffered data.
	if wc, ok := src.(*Conn); ok && len(wc.Peeked) > 0 {
		if _, err := dst.Write(wc.Peeked); err != nil {
			errc <- err
			return
		}
		wc.Peeked = nil
	}

	// Unwrap the src and dst from *Conn to *net.TCPConn so Go
	// 1.11's splice optimization kicks in.
	src = UnderlyingConn(src)
	dst = UnderlyingConn(dst)

	_, err := io.Copy(dst, src)
	errc <- err
}

Since we don't use tcpproxy.Con, and we don't do route matching we don't need to copy buffered data.

Since GRPC endpoint is not a net.TCPConn unwrapping the underlying connection will fail, and the splice optimization will not kick in.

So we are left with:

func proxyCopy(errc chan<- error, dst, src net.Conn) {
	defer closeRead(src)
	defer closeWrite(dst)
	_, err := io.Copy(dst, src)
	errc <- err
}

I'm not sure how this code supports TCP half-close and bicopy does not. They seem to do the same thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue might not be relevant to tcp half-close , as this does not seem to implement half-close methods

type GRPCServerRW struct {

Not sure why tcpproxy fixes the issue, except on WSL2 🤔

@AkihiroSuda AkihiroSuda marked this pull request as draft July 4, 2025 11:55
@AkihiroSuda AkihiroSuda changed the title portfwdserver: fix half-close Import gvisor-tap-vsock/pkg/tcpproxy into pkg/tcpproxy Jul 8, 2025
@AkihiroSuda AkihiroSuda force-pushed the fix-3685 branch 3 times, most recently from 8db0dbe to 6e67594 Compare July 8, 2025 09:28
Import https://github.com/containers/gvisor-tap-vsock/blob/v0.8.6/pkg/tcpproxy/tcpproxy.go
into `pkg/tcpproxy`.

Changes to the file will be added in follow-up commits.

Fix issue 3685

Signed-off-by: Akihiro Suda <[email protected]>
Signed-off-by: Akihiro Suda <[email protected]>
Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda
Copy link
Member Author

@AkihiroSuda AkihiroSuda closed this Jul 9, 2025
@AkihiroSuda AkihiroSuda removed this from the v1.2.0 milestone Jul 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[gRPC portfwd] client connection is not closed immediately when server closed the connection
4 participants