Add vm-switch core logic #4
Conversation
go.mod
Outdated
| require ( | ||
| github.com/pkg/errors v0.9.1 // indirect | ||
| github.com/vishvananda/netns v0.0.0-20211101163701-50045581ed74 // indirect | ||
| ) | ||
|
|
There was a problem hiding this comment.
These shouldn't be in a separate section. Please merge all requires into a single section then re-run go mod tidy (so it sorts and splits all dependencies deterministically).
| func main() { | ||
| flag.BoolVar(&debug, "debug", true, "enable debug flag") | ||
| flag.StringVar(&tapIface, "tap-interface", defaultTapDevice, "tap interface name, eg. eth0, eth1") | ||
| flag.Parse() |
There was a problem hiding this comment.
Should we have options for (non-default) mtu, mac addr?
There was a problem hiding this comment.
The same mac address is also being used in the host switch so making this configurable is kind of pointless, also, the arp message is isolated to a network namespace so mac address conflict is not an option. As for the mtu, the default standard value for most networks is 1500 bytes (although we are setting it to 4000), we can always add an extra arg to configure this if there is a demand for it or if we ever decide to support jumbo frames but for now, it should be fine.
cmd/vm/switch_linux.go
Outdated
| // network namespace (rd1), the logic behind this approach was the | ||
| // lack of support for network namespaces in the AF_VSOCK libs. |
There was a problem hiding this comment.
The last sentence is backwards: this is required because AF_VSOCK is affected by network namespaces, therefore we need to open it before entering a new namespace (via unshare/nsenter).
Also, AF_VSOCK is not a library: it's a kernel feature.
There was a problem hiding this comment.
Here is the exact wording from the open issue on vsock kernel module thread:
it could be nice to support network namespace in vsock
There was a problem hiding this comment.
Well, the reason we need all thing fd-passing is because they ended up implementing network namespaces for AF_VSOCK (presumably in a commit later than that quoted bit); at least, that seems to be the observed behaviour (in that we don't get to use the same AF_VSOCK things inside the network namespace). That is, your comment here is exactly backwards — if AF_VSOCK didn't support namespaces, we wouldn't need to do this.
cmd/vm/switch_linux.go
Outdated
| // the FD is passed-in as an extra arg from exec.Command | ||
| // of the parent process. This is for the AF_VSOCK connection that | ||
| // is handed over from the default namespace to Rancher Desktop's | ||
| // network namespace (rd1), the logic behind this approach was the |
There was a problem hiding this comment.
As far as I know, we no longer need to name the namespace, right? So rd1 doesn't apply anymore. (Hint: they were never named; the "name" was just a name of an arbitrary mount point; both the netns package, and unshare was just bind-mounting /proc/<pid>/ns/net somewhere and calling that a name.)
There was a problem hiding this comment.
We create the rd1 namespace manually during the namespace setup, it is being used by the nsenter something like:
exec.Command(nsenter, fmt.Sprintf("-n/run/netns/%s", defaultNetworkNamespace), "-F", unshare, "--pid", "--mount-proc", "--fork", "--propagation", "slave", unshareArg)
There was a problem hiding this comment.
When we run nsenter, we'd have the pid of a process that's already in the namespace (and therefore don't need to do it by path), right?
There was a problem hiding this comment.
I think we should be able to do the pid.
cmd/vm/switch_linux.go
Outdated
| // is handed over from the default namespace to Rancher Desktop's | ||
| // network namespace (rd1), the logic behind this approach was the | ||
| // lack of support for network namespaces in the AF_VSOCK libs. | ||
| connFile := os.NewFile(uintptr(3), "RancherDesktop-AFVsock-Connection") |
There was a problem hiding this comment.
There's no reason not to use spaces in this name?
cmd/vm/switch_linux.go
Outdated
| return | ||
| } | ||
|
|
||
| if debug { |
There was a problem hiding this comment.
Would logrus.IsLevelEnabled() be a better check?
There was a problem hiding this comment.
I honestly don't see a huge benefit but more verbose code stuff...
cmd/vm/switch_linux.go
Outdated
| errCh <- fmt.Errorf("unexpected size %d", n) | ||
| return | ||
| } | ||
| size := int(binary.LittleEndian.Uint16(sizeBuf[0:2])) |
There was a problem hiding this comment.
Same as above — little endian?
cmd/vm/switch_linux.go
Outdated
| return | ||
| } | ||
|
|
||
| if _, err := tap.Write(buf[:size]); err != nil { |
There was a problem hiding this comment.
Shouldn't we check the returned size here?
cmd/vm/switch_linux.go
Outdated
| return | ||
| } | ||
|
|
||
| if debug { |
cmd/vm/switch_linux.go
Outdated
| } | ||
| } | ||
|
|
||
| func checkForExsitingIf(ifName string) error { |
There was a problem hiding this comment.
If sounds like the English conjunction. Can we be slightly more verbose here and use iface or interface?
Signed-off-by: Nino Kodabande <[email protected]>
cmd/vm/switch_linux.go
Outdated
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| logrus.Info("exiting tx goroutine") |
.gitignore
Outdated
| @@ -0,0 +1,4 @@ | |||
| ./bin/ | |||
There was a problem hiding this comment.
/bin/ would be better (to make it clearer that it's relative to the root of the repo), but this works too.
cmd/vm/switch_linux.go
Outdated
| logrus.Debugf("using a AF_VSOCK connection file from default namespace: %v", connFile) | ||
|
|
||
| // this should never happen | ||
| if err := checkForExsitingIface(tapIface); err != nil { |
There was a problem hiding this comment.
How did we miss this…
| if err := checkForExsitingIface(tapIface); err != nil { | |
| if err := checkForExistingIface(tapIface); err != nil { |
cmd/vm/switch_linux.go
Outdated
| go func() { | ||
| if err := dhcp(ctx, tapIface); err != nil { | ||
| errCh <- fmt.Errorf("dhcp error: %w", err) | ||
| ctx.Done() |
There was a problem hiding this comment.
I don't understand this line. ctx.Done() returns a channel that will emit when the context is done, but you're dropping the return value on the floor. (It doesn't cause the context to be cancelled; for that we need a different context.WithCancel().)
There was a problem hiding this comment.
I created the context.WithCancel() I called the wrong function mistakenly. I will pass in the cancel func.
Signed-off-by: Nino Kodabande <[email protected]>
| case s := <-sigChan: | ||
| logrus.Errorf("signal caught: %v", s) | ||
| cancel() | ||
| os.Exit(1) |
There was a problem hiding this comment.
FWIW, if you want to let deferred things run (given that this is main()), one way would be:
func main() {
exitCode := 0
defer os.Exit(exitCode)
…
// in this code here
exitCode = 1
return| } | ||
| } | ||
|
|
||
| func run(ctx context.Context, cancel context.CancelFunc, connFile io.ReadWriteCloser) error { |
There was a problem hiding this comment.
connFile is probably better named conn (because the fact that it's a file is opaque).
This PR adds VM Switch process that runs in an isolated network namespace and starts the tap device to talk to the Host Switch.
Fixes: rancher-sandbox/rancher-desktop#3955