fix failure to start container with networking enabled#162
fix failure to start container with networking enabled#162ndeloof wants to merge 1 commit intocontainerd:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Resolves a container startup failure when networking is enabled by ensuring bind-mount sources don’t include symlinks that libkrun rejects under O_NOFOLLOW (notably on macOS).
Changes:
- Resolve host bind-mount source paths via
filepath.EvalSymlinksbefore passing them to libkrun. - Return an explicit error if symlink resolution fails.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } 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. |
There was a problem hiding this comment.
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.
| } 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. |
| // 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) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to resolve symlinks for bind mount source %s: %w", m.Source, err) | ||
| } |
There was a problem hiding this comment.
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.
| 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)) | ||
| } |
There was a problem hiding this comment.
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.
akerouanton
left a comment
akerouanton
left a comment
There was a problem hiding this comment.
I think you're trying to invoke nerdbox through ctr. In that case, you should adapt your ctr invocation based on these findings.
| // 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) |
There was a problem hiding this comment.
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.
| _, _ = rcBuf.WriteRune('\n') | ||
| } | ||
| rcBytes = rcBuf.Bytes() | ||
| } else if fallbackToHostRC { |
There was a problem hiding this comment.
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.
Following README to run a container with networking (using gvproxy) failed on MacOS.
Claude-assisted diagnostic eventually went to this fix