Enable config file based configuration by MrEcco · Pull Request #484 · containers/gvisor-tap-vsock

MrEcco · 2025-03-09T19:23:59Z

This is a little rethink of how to configure. Since the previous implementation disallows to use the compiled binaries in releases, I have prepared a more flexible version. However, the legacy behavior (without configuration file) remains as is (proven by unit tests).

List of changes

Enable -config flag. If no such flag provided, then remain the original behavior
Partial refactoring of the initialization process
Fix small issue what prevented multiple hypervisors connecting to the controlling socket (e.g. now you can attach more than 1 qemu VM, and they can ping each other)

Most of all changes are tested, see unit tests.

Reasons

I'm trying to enable multi-VM local lab which should support different host OS, guest OS, guest arches. With this tool I have resolved the significant problem with the proper VM networking (at least for MacOS). However, I must fix a few issues, here's the PR.

Pealse accept it and release the new version of gvproxy binary. I would appreciate.

lstocchi

Looking at the code LGTM, need to do some test.

Adding @cfergeau and @evidolob as reviewers so they give it a look as well

cmd/gvproxy/main.go

cmd/gvproxy/config.go

MrEcco · 2025-03-13T17:52:50Z

Any news on it?

cfergeau · 2025-03-18T08:59:58Z

@MrEcco one thing which is missing from the PR is a signed-off-by, see https://github.com/containers/gvisor-tap-vsock/pull/484/checks?check_run_id=38666327398

MrEcco · 2025-03-21T21:57:26Z

Sorry to bother your GHCI minutes, I finally won this DCO requirement.

cfergeau · 2025-03-24T12:26:27Z

After the addition of the DCO, there is no longer a commit log to your change :-/ The initial description in the PR should be good as a commit log.
Can you also rebase this (and not merge) on top of main? There will a conflict related to 4bbb832 , I’m a bit worried of introducing a regression if I try to fix it myself :-/

There are also a number of issues reported by make lint

cmd/gvproxy/main.go

cfergeau · 2025-03-24T15:00:14Z

cmd/gvproxy/config.go

+
+	if args.config != "" {
+		if slices.Contains(os.Args, "-ssh-port") || slices.Contains(os.Args, "--ssh-port") {
+			log.Warningf("CLI argument \"-ssh-port\" is unavailable with config file. You need to add \"127.0.0.1:%d: 192.168.127.2:22\" entry into .stack.forwards in \"\" instead", args.sshPort)


Is it required to allow to specify a config file, but to allow to override some of its content with commandline options? or can we decide in the future that -config is mutually exclusive with any other cmdline flag? (with the exception of logging/debug)

I want to remain CLI flags as is as much as possible, because I'm not a maintainer here, you know. If you just say that I can reimplement the initialization from scratch, I would do it :) But it may disturb maintainers, no one likes when someone changes the code they already got used to.

If you propose me to reimplement configuration of it without default behavior with hardcoded configuration, I would do, with bells, tests, and whistles. Maybe with some portion of integration testing. Please answer in thread :)

BTW, CLI args and config file complement each other. You can find the proof of it in tests.

BTW, CLI args and config file complement each other. You can find the proof of it in tests.

I’ve seen that you can override some parts of the config with commandline args, but looking at the tests, I don’t really see tests for stuff like:

--config config.yaml --listen ... --forward... --ssh-port 1234

I think the code in the PR has kind support for matching both, but I was asking if you have a requirement for this? we could decide --config and other network-related args can’t be used together.

cfergeau · 2025-03-24T15:27:17Z

cmd/gvproxy/config.go

+const (
+	// 	gatewayIP   = "192.168.127.1"
+	sshHostPort = "192.168.127.2:22"
+	hostIP      = "192.168.127.254"


This could be derived from luaddr in GVProxyConfigure as is already partially done for config.Stack.NAT for example.

Yes, but please look at the original code. It seems like the maintainers used this code with these hardcoded variables and calls like go run main.go, without flags. I don't know if I can remove this feature.

I don’t expect many people to use go run main.go directly, at the very least you need to specify one of the listen args.
The main user of gvproxy that I’m aware of is podman-machine, mainly for use on macos and windows, but on linux it also starts gvproxy. Its commandline (on macos for example) is:

/opt/podman/bin/gvproxy -mtu 1500 -ssh-port 64450 -listen-vfkit unixgram:///var/folders/l0/rh4v8_j54k37h2w320__7d1h0000gn/T/podman/podman-machine-default-gvproxy.sock -forward-sock /var/folders/l0/rh4v8_j54k37h2w320__7d1h0000gn/T/podman/podman-machine-default-api.sock -forward-dest /run/user/501/podman/podman.sock -forward-user core -forward-identity /Users/teuf/.local/share/containers/podman/machine/machine -pid-file /var/folders/l0/rh4v8_j54k37h2w320__7d1h0000gn/T/podman/gvproxy.pid -log-file /var/folders/l0/rh4v8_j54k37h2w320__7d1h0000gn/T/podman/gvproxy.log

Imo the main reason for these hardcoded values is because it was easier this way rather than writing the code you added, where you specify the subnet you want, and everything is inferred from it.

cmd/gvproxy/config.go

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

containers#484 (comment) containers#484 (comment) containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

…issue) Fix small issue what prevented multiple hypervisors connecting to the controlling socket (e.g. now you can attach more than 1 qemu VM, and they can ping each other) containers#484 (comment) containers#484 (comment) containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

cfergeau · 2025-04-29T15:47:24Z

I’ve rebased this and squashed the commits which go together in https://github.com/cfergeau/gvisor-tap-vsock/tree/MrEcco
The modified version works fine with podman-machine and macadam on a mac. I need to take a closer look at the "multiaccept" changes though.

…issue) Fix small issue what prevented multiple hypervisors connecting to the controlling socket (e.g. now you can attach more than 1 qemu VM, and they can ping each other) containers#484 (comment) containers#484 (comment) containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

ggoodman · 2025-06-10T03:27:36Z

cmd/gvproxy/main.go

+			for {
+				select {
+				case <-ctx.Done():
+					return nil
+				default:
+					vfkitConn, err := transport.AcceptVfkit(conn)
+					if err != nil {
+						log.Errorf("vfkit accept error: %s", err)
+						continue
+					}
+					g.Go(func() error {
+						return vn.AcceptVfkit(ctx, vfkitConn)
+					})
+				}


I don't believe that this change does what it promises; here conn is a connectionless unix datagram socket. Handing the 'conn' off to another goroutine doesn't actually work.

Some form of affinity is needed. It's really not clear to me if the unixgram socket can even support multiplexing different vms.

here conn is a connectionless unix datagram socket. Handing the 'conn' off to another goroutine doesn't actually work.

That’s a good point, both transport.AcceptVfkit(conn) and vn.AcceptVfkit(ctx, vfkitConn) will be trying to read from conn, and if a second vfkit process tries to connect to gvproxy, then it’s supposed to be handled by transport.AcceptVfkit(conn) but the packet could be read by vn.AcceptVfkit(ctx, vfkitConn).

It's really not clear to me if the unixgram socket can even support multiplexing different vms.

The way the code is currently written, it’s not clear to me either if this kind of multiplexing is possible or not, I think it’s not. In general, multiplexing should be possible by looking at the net.UnixAddr of the vfkit-side of the connection, and then processing it in the right context.

…issue) Fix small issue what prevented multiple hypervisors connecting to the controlling socket (e.g. now you can attach more than 1 qemu VM, and they can ping each other) containers#484 (comment) containers#484 (comment) containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

cfergeau · 2025-09-12T10:12:50Z

So I’ve looked some more at this branch, rebased it and did a few more cleanups, mostly in the test code. Imo the config file commit is good to go, I’ll push it soon. Current code is in https://github.com/cfergeau/gvisor-tap-vsock/tree/MrEcco

However, the "multiple VMs" commit could use some work, for example I started gvproxy, connected 2 qemu VMs, killed one qemu with ctrl+c, and gvproxy exits. I don’t think it should be doing this. And thinking more about how this could impact existing users, it’s better to enable this using a command line/config file argument.

This is a little rethink of how to configure. Since the previous implementation disallows to use the compiled binaries in releases, I have prepared a more flexible version. However, the legacy behavior (without configuration file) remains as is (proven by unit tests). List of changes Enable -config flag. If no such flag provided, then remain the original behavior Partial refactoring of the initialization process Most of all changes are tested, see unit tests. I'm trying to enable multi-VM local lab which should support different host OS, guest OS, guest arches. With this tool I have resolved the significant problem with the proper VM networking (at least for MacOS). Signed-off-by: Andre Buryndin <themrecco@gmail.com> Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>

go 1.23 is no longer supported upstream, and gvisor-tap-vsock switched to go 1.24 Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>

cfergeau · 2025-09-12T12:52:59Z

/lgtm
/approve

openshift-ci · 2025-09-12T12:53:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau, MrEcco

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cfergeau]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

MrEcco · 2025-09-13T14:15:02Z

Sorry, I forgot about this PR. Months ago, I have reimplemented your idea with appropriate configuration (or just with my own understanding of "appropriate", for my case) and CLI interface, here: https://github.com/circumspectlabs/gvswitch. This little project is a well tested by my students, they use it with this Ansible code which can start multiple VMs with a shared private network: https://github.com/circumspectlabs/kubernetes-the-mindful-way/blob/main/code/molecule/default/create.yml#L255-L272.

You can take the CLI and config implementation from this repo. It works with 6 VMs (haven't succeeded to start more on my laptop). Haven't yet tested on Windows and Linux.

This was referenced Mar 9, 2025

Are multiple vms accessing single gvproxy supported? #383

Open

disable access to control routes via 192.168.127.1 #78

Open

Configuration file for gvproxy #35

Closed

lstocchi reviewed Mar 11, 2025

View reviewed changes

cmd/gvproxy/main.go Outdated Show resolved Hide resolved

cmd/gvproxy/config.go Show resolved Hide resolved

cmd/gvproxy/config.go Outdated Show resolved Hide resolved

cmd/gvproxy/config.go Show resolved Hide resolved

lstocchi requested review from cfergeau and evidolob March 11, 2025 13:24

MrEcco force-pushed the main branch from 5a216b0 to 1477bbd Compare March 21, 2025 21:39

openshift-merge-robot added needs-rebase and removed needs-rebase labels Mar 21, 2025

MrEcco force-pushed the main branch from 61681fa to 6a3d914 Compare March 21, 2025 21:53

openshift-merge-robot added needs-rebase and removed needs-rebase labels Mar 21, 2025

MrEcco requested a review from lstocchi March 22, 2025 11:51

cfergeau reviewed Mar 24, 2025

View reviewed changes

MrEcco force-pushed the main branch from dfca2c7 to 3d808d5 Compare March 24, 2025 18:08

MrEcco added a commit to MrEcco/gvisor-tap-vsock that referenced this pull request Mar 24, 2025

fix code smells

5281694

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

MrEcco added a commit to MrEcco/gvisor-tap-vsock that referenced this pull request Mar 24, 2025

fix code smells

e191cf5

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

MrEcco added a commit to MrEcco/gvisor-tap-vsock that referenced this pull request Mar 24, 2025

move log file and log level code to the earliest possible place

f8b4a1b

containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

MrEcco added a commit to MrEcco/gvisor-tap-vsock that referenced this pull request Mar 24, 2025

fix code smells

efc0d9a

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

MrEcco added a commit to MrEcco/gvisor-tap-vsock that referenced this pull request Mar 24, 2025

remove default listener for control socket

e1f1a47

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

MrEcco added a commit to MrEcco/gvisor-tap-vsock that referenced this pull request Mar 24, 2025

unglobal config var

66825a1

containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

MrEcco added a commit to MrEcco/gvisor-tap-vsock that referenced this pull request Mar 24, 2025

fix code smells

d63fa63

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

MrEcco added a commit to MrEcco/gvisor-tap-vsock that referenced this pull request Mar 24, 2025

fix code smells

e27856e

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

MrEcco added a commit to MrEcco/gvisor-tap-vsock that referenced this pull request Mar 24, 2025

move log file and log level code to the earliest possible place

51187ed

containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

MrEcco added a commit to MrEcco/gvisor-tap-vsock that referenced this pull request Mar 24, 2025

fix code smells

15c51e4

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

MrEcco added a commit to MrEcco/gvisor-tap-vsock that referenced this pull request Mar 24, 2025

remove default listener for control socket

958ac5e

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

cfergeau pushed a commit to cfergeau/gvisor-tap-vsock that referenced this pull request Apr 29, 2025

fix code smells

d395c7d

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

cfergeau pushed a commit to cfergeau/gvisor-tap-vsock that referenced this pull request Apr 29, 2025

fix code smells

d70c15f

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

cfergeau pushed a commit to cfergeau/gvisor-tap-vsock that referenced this pull request Apr 29, 2025

move log file and log level code to the earliest possible place

46ce19c

containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

cfergeau pushed a commit to cfergeau/gvisor-tap-vsock that referenced this pull request Apr 29, 2025

fix code smells

f067dae

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

cfergeau pushed a commit to cfergeau/gvisor-tap-vsock that referenced this pull request Apr 29, 2025

remove default listener for control socket

2b9cd5e

containers#484 (comment) containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

cfergeau pushed a commit to cfergeau/gvisor-tap-vsock that referenced this pull request Apr 29, 2025

unglobal config var

90f5073

containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

cfergeau pushed a commit to cfergeau/gvisor-tap-vsock that referenced this pull request Apr 29, 2025

fix issue with inproperly closed log file

ff3e097

containers#484 (comment) Signed-off-by: Andre Buryndin <themrecco@gmail.com>

ggoodman reviewed Jun 10, 2025

View reviewed changes

cfergeau mentioned this pull request Jun 10, 2025

Move most of gvproxy code to a reusable go package with an API #286

Open

cfergeau force-pushed the main branch from f5a0e2e to c58b936 Compare September 12, 2025 12:08

gha: Switch to go 1.24 and 1.25

2cd03e4

go 1.23 is no longer supported upstream, and gvisor-tap-vsock switched to go 1.24 Signed-off-by: Christophe Fergeau <cfergeau@redhat.com>

openshift-ci bot assigned cfergeau Sep 12, 2025

openshift-ci bot added the lgtm label Sep 12, 2025

openshift-ci bot added the approved label Sep 12, 2025

openshift-merge-bot bot merged commit dc117b2 into containers:main Sep 12, 2025
22 checks passed

cfergeau mentioned this pull request Sep 12, 2025

Add support for changing network address #543

Closed

Conversation

MrEcco commented Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

List of changes

Reasons

Uh oh!

lstocchi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MrEcco commented Mar 13, 2025

Uh oh!

cfergeau commented Mar 18, 2025

Uh oh!

MrEcco commented Mar 21, 2025

Uh oh!

cfergeau commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrEcco Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cfergeau commented Apr 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfergeau commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfergeau commented Sep 12, 2025

Uh oh!

openshift-ci bot commented Sep 12, 2025

Uh oh!

Uh oh!

MrEcco commented Sep 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MrEcco commented Mar 9, 2025 •

edited

Loading

cfergeau commented Mar 24, 2025 •

edited

Loading

MrEcco Mar 24, 2025 •

edited

Loading

cfergeau commented Sep 12, 2025 •

edited

Loading