Try to load kernel modules, without modprobe#49038
Conversation
|
Just to note ... I haven't added a modprobe for Tested on a host with |
internal/modprobe/modprobe_linux.go
Outdated
| preq := unsafe.Pointer(&req) // #nosec G103 -- Ignore "G103: Use of unsafe calls should be audited" | ||
| _, _, _ = syscall.Syscall(syscall.SYS_IOCTL, uintptr(s), syscall.SIOCGIFINDEX, uintptr(preq)) |
There was a problem hiding this comment.
internal/modprobe/modprobe_linux.go
Outdated
| func LoadModules(ctx context.Context, isLoaded func() error, modNames ...string) error { | ||
| if isLoaded() == nil { | ||
| log.G(ctx).WithFields(log.Fields{ | ||
| "modules": strings.Join(modNames, ","), |
There was a problem hiding this comment.
I was curious if we should let modNames as-is, or join them; log.Fields should be able to handle either, but when using "text-format" logs would print them using the go string formatter ([module-a module-b module-c] which could indeed be slightly confusing (no commas). OTOH, when using --log-format=json it would preserve them as structured info; https://go.dev/play/p/TvOYyx1ze6R
package main
import (
"context"
"strings"
"github.com/containerd/log"
)
func main() {
modNames := []string{"module-a", "module-b", "module-c"}
_ = log.SetFormat(log.TextFormat)
log.G(context.TODO()).WithFields(log.Fields{"modules": modNames}).Info("log as slice")
log.G(context.TODO()).WithFields(log.Fields{"modules": strings.Join(modNames, ",")}).Info("log as string")
_ = log.SetFormat(log.JSONFormat)
log.G(context.TODO()).WithFields(log.Fields{"modules": modNames}).Info("log as slice")
log.G(context.TODO()).WithFields(log.Fields{"modules": strings.Join(modNames, ",")}).Info("log as string")
}time="2009-11-10T23:00:00.000000000Z" level=info msg="log as slice" modules="[module-a module-b module-c]"
time="2009-11-10T23:00:00.000000000Z" level=info msg="log as string" modules="module-a,module-b,module-c"{"level":"info","modules":["module-a","module-b","module-c"],"msg":"log as slice","time":"2009-11-10T23:00:00.000000000Z"}
{"level":"info","modules":"module-a,module-b,module-c","msg":"log as string","time":"2009-11-10T23:00:00.000000000Z"}There was a problem hiding this comment.
Oh, not sure - I joined the strings to make the log entry easier to read, but hadn't considered JSON output ... if it's better not to join them, I can do that?
There was a problem hiding this comment.
It's definitely not a hard blocker; mostly in light of "simplify if we don't really need it" (looking at the above output, I think it's still ~ readable), and wondering if having it as actually structured info (in JSON) would be beneficial.
internal/modprobe/modprobe_linux.go
Outdated
| func (ml modprobeLoader) load(modName string) error { | ||
| out, err := exec.Command("modprobe", "-va", modName).CombinedOutput() | ||
| if err != nil { | ||
| return fmt.Errorf("modprobe %s failed with message: %q, error: %v", modName, strings.TrimSpace(string(out)), err) |
There was a problem hiding this comment.
Not sure if we'd ever need access to the original error, but if we would; perhaps %w instead of %v here
There was a problem hiding this comment.
Sure, will do - it's only logged by this package at the moment, not returned.
I dithered about what errors to return ... there might not be a useful error from the ioctl method, it might just not do anything, there might be errors from both methods, and for more than one module. In the end, I decided to log the detail and just return the error from the caller's check function to try to keep it readable/meaningful. If we need to debug a problem, we'll need debug logs - but we probably would anyway.
There was a problem hiding this comment.
Yeah, not sure how useful in practice; I guess we could get "permission" or "not found" errors, but we wouldn't be able to act on any of that, so not really important indeed. It mostly caught my eye that we were masking the underlying error.
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM (but left two minor comments / suggestions)
Should we have a link to that original tweet as well? 😂
Added to the PR description! |
An ioctl() call to get the "interface index" for a kernel module triggers the kernel to try to load the module, if the process is running with CAP_SYS_MODULE. This tends to be more reliable than "modprobe" for docker-in-docker. If the ioctl() method fails, fall back to trying "modprobe". Signed-off-by: Rob Murray <[email protected]>
Signed-off-by: Rob Murray <[email protected]>
dockerd will now do this itself, if ip6tables is enabled. Signed-off-by: Rob Murray <[email protected]>
These modprobes were added as a workaround in commit cce5dfe, but dockerd should now be able to load the modules it needs. Signed-off-by: Rob Murray <[email protected]>
|
I added a cherry-pick label, but we can decide whether that should go in 27.4.0 or a patch-release after (if we consider it important enough to include in a patch release). |
|
Unrelated Windows test failures ... |
- What I did
When running in a container (docker-in-docker), the host may not have required kernel modules loaded.
Try to trigger the module load using an
ioctl()call, thenmodprobeif that doesn't work.This should make IPv6 networks work in GitHub Codespaces / devcontainers:
This is based on the DinD official image's method of loading modules using
ip link show, which was inspired by https://twitter.com/lucabruno/status/902934379835662336- How I did it
ip6_tables.- How to verify it
On a Debian 12.5 host, running dockerd
27.3.1, with systemd ...ip6_tablesnot loaded on the host (the host doesn't need it, because the daemon'sip6tablescommands use a (deprecated) dbus interface to send raw firewall commands.ip6_tablesmodule wasn't loaded.ip6tablesenabled by-default.ioctlmethod:DEBU[2024-12-05T11:30:36.095500049Z] Modules loaded loadErrors="<nil>" loader=ioctl modules=ip6_tablesDEBU[2024-12-05T11:31:59.533424067Z] Modules already loaded modules=ip6_tablesDEBU[2024-12-05T11:31:59.590301221Z] Modules already loaded modules="nf_conntrack,nf_conntrack_netlink"To check error logging, and fallback to modprobe:
Resulted in:
- Description for the changelog