Skip to content

fix failure to start container with networking enabled #162

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.

Sign up for GitHub

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

Open
ndeloof wants to merge 1 commit into
base: main
Choose a base branch
from
+16 −5
Open

fix failure to start container with networking enabled #162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/vm-configuration.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ You must build it from source following the instructions set in that repository.
```
(terminal 1) $ bin/gvproxy -debug -listen unix:///tmp/network.sock -listen-vfkit unixgram:///tmp/gvisor.sock
(terminal 2) $ ctr run -t --net-host --cap-add CAP_NET_ADMIN --rm --snapshotter erofs --runtime io.containerd.nerdbox.v1 \
--annotation io.containerd.nerdbox.network.0=socket=/tmp/gvisor.sock,mode=unixgram,mac=fa:43:25:5d:6f:b4,addr=192.168.127.2 \
--annotation io.containerd.nerdbox.network.0=socket=/tmp/gvisor.sock,mode=unixgram,mac=fa:43:25:5d:6f:b4,addr=192.168.127.2/16 \
docker.io/nicolaka/netshoot:latest test
```

Expand Down
5 changes: 4 additions & 1 deletion internal/shim/task/ctrnetworking.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,11 @@ func addResolvConf(ctx context.Context, b *bundle.Bundle, fallbackToHostRC bool)
_, _ = rcBuf.WriteRune('\n')
}
rcBytes = rcBuf.Bytes()
} else if fallbackToHostRC {
Copy link
Copy Markdown
Member

@akerouanton akerouanton Apr 14, 2026

Choose a reason for hiding this comment

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

IIRC all that code was added before nerdbox had proper support for bind-mounts.

I think the shim should be minimal and have less magic, i.e. we should remove this function and the io.containerd.nerdbox.ctr.dns annotation, and let higher-level runtimes inject their custom resolv.conf through standard bind-mounts.

} else {
// Try giving the VM a copy of the host's resolv.conf.
// This is always attempted (not only as a fallback) because on some
// platforms (e.g., macOS) the host path /etc/resolv.conf may contain
// symlinks that prevent it from being shared via virtiofs.
Comment on lines +190 to +194
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

fallbackToHostRC no longer gates whether host /etc/resolv.conf is used (behavior changed from else if fallbackToHostRC to unconditional else). This makes the parameter’s meaning inconsistent and may unintentionally opt-in to host-DNS copying even when callers pass false. Either restore the condition (while still addressing the macOS symlink problem via symlink resolution / file copy), or remove/rename the parameter and update call sites to reflect the new always-try behavior.

Suggested change
} else {
// Try giving the VM a copy of the host's resolv.conf.
// This is always attempted (not only as a fallback) because on some
// platforms (e.g., macOS) the host path /etc/resolv.conf may contain
// symlinks that prevent it from being shared via virtiofs.
} else if fallbackToHostRC {
// Try giving the VM a copy of the host's resolv.conf when enabled.
// Copying the file contents (rather than sharing the host path) avoids
// issues on platforms where /etc/resolv.conf may be a symlink that
// cannot be shared via virtiofs.

Copilot uses AI. Check for mistakes.
if c, err := os.ReadFile("/etc/resolv.conf"); err == nil {
rcBytes = c
}
Expand Down
14 changes: 11 additions & 3 deletions internal/shim/task/mount.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,16 +230,24 @@ func (bm *bindMounter) FromBundle(ctx context.Context, b *bundle.Bundle) error {
tag := fmt.Sprintf("bind-%x", hash[:8])
vmTarget := "/mnt/" + tag

// Resolve symlinks in the source path because libkrun opens shared
// directories with O_NOFOLLOW, which rejects symlinks
// (e.g., /etc -> /private/etc and /etc/resolv.conf -> ../var/run/resolv.conf on macOS).
resolvedSource, err := filepath.EvalSymlinks(m.Source)
Copy link
Copy Markdown
Member

@akerouanton akerouanton Apr 14, 2026

Choose a reason for hiding this comment

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

I'm always wary of blindly following symlinks due to the security risks they pose. I think nerdbox users should evaluate any symlinks if and when that makes sense instead.

if err != nil {
return fmt.Errorf("failed to resolve symlinks for bind mount source %s: %w", m.Source, err)
}
Comment on lines +233 to +239
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

The error message should quote the path to avoid ambiguity (especially if it contains spaces) and to match common Go error formatting. Prefer %q for the path, e.g. ... source %q: %w.

Copilot uses AI. Check for mistakes.

// For files, share the parent directory via virtiofs since virtiofs
// operates on directories. The spec source points to the file within
// the mounted directory.
hostSrc := m.Source
hostSrc := resolvedSource
specSrc := vmTarget
if !fi.IsDir() {
hostSrc = filepath.Dir(m.Source)
hostSrc = filepath.Dir(resolvedSource)
// Use path.Join (not filepath.Join) because this path is used
// inside the Linux VM where forward slashes are required.
specSrc = path.Join(vmTarget, filepath.Base(m.Source))
specSrc = path.Join(vmTarget, filepath.Base(resolvedSource))
}
Comment on lines +244 to 251
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

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

After introducing resolvedSource, the directory/file decision still depends on fi.IsDir() obtained earlier. To keep the logic consistent and self-contained, consider deriving fi from resolvedSource (or add an inline comment clarifying that fi is already based on os.Stat(m.Source) which follows symlinks). This prevents subtle mismatches if the earlier stat ever changes to Lstat or otherwise diverges from the resolved path.

Copilot uses AI. Check for mistakes.

transformed := bindMount{
Expand Down
Loading