Skip to content

Try to load kernel modules, without modprobe#49038

Merged
robmry merged 4 commits intomoby:masterfrom
robmry:modprobeless
Dec 6, 2024
Merged

Try to load kernel modules, without modprobe#49038
robmry merged 4 commits intomoby:masterfrom
robmry:modprobeless

Conversation

@robmry
Copy link
Contributor

@robmry robmry commented Dec 5, 2024

- 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, then modprobe if 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

- How to verify it

On a Debian 12.5 host, running dockerd 27.3.1, with systemd ...

  • With ip6_tables not loaded on the host (the host doesn't need it, because the daemon'sip6tables commands use a (deprecated) dbus interface to send raw firewall commands.
  • Started a dev container with the updated code, the ip6_tables module wasn't loaded.
  • Built the new dockerd and ran it, with ip6tables enabled by-default.
  • The module was loaded on the host, and inner-docker logs showed module was loaded using the ioctl method:
    • DEBU[2024-12-05T11:30:36.095500049Z] Modules loaded loadErrors="<nil>" loader=ioctl modules=ip6_tables
  • Restarted the inner docker, no load was attempted:
    • DEBU[2024-12-05T11:31:59.533424067Z] Modules already loaded modules=ip6_tables
  • Ditto (both times) for the conntrack modules:
    • DEBU[2024-12-05T11:31:59.590301221Z] Modules already loaded modules="nf_conntrack,nf_conntrack_netlink"

To check error logging, and fallback to modprobe:

if err := modprobe.LoadModules(context.TODO(), func() error {
        return fmt.Errorf("no fruit")
}, "bananas", "apples", "oranges"); err != nil {
        log.G(context.Background()).WithError(err).Debug("Loading breakfast")
}

Resulted in:

DEBU[2024-12-05T11:55:32.007075924Z] Modules not loaded                            checkResult="no fruit" loadErrors="<nil>" loader=ioctl modules="bananas,apples,oranges"
DEBU[2024-12-05T11:55:32.007147257Z] Modules not loaded                            checkResult="no fruit" loadErrors="modprobe bananas failed with message: \"\", error: exec: \"modprobe\": executable file not found in $PATH\nmodprobe apples failed with message: \"\", error: exec: \"modprobe\": executable file not found in $PATH\nmodprobe oranges failed with message: \"\", error: exec: \"modprobe\": executable file not found in $PATH" loader=modprobe modules="bananas,apples,oranges"
DEBU[2024-12-05T11:55:32.007172382Z] Loading breakfast                             error="no fruit"

- Description for the changelog

- Attempt to load kernel modules, including `ip6_tables` and `br_netfilter` when required, using a
  method that is likely to succeed inside a docker-in-docker container.

@robmry robmry added kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. area/networking Networking impact/changelog area/daemon Core Engine area/networking/ipv6 Networking labels Dec 5, 2024
@robmry robmry added this to the 28.0.0 milestone Dec 5, 2024
@robmry robmry self-assigned this Dec 5, 2024
@robmry robmry marked this pull request as ready for review December 5, 2024 14:30
@robmry robmry requested a review from tianon as a code owner December 5, 2024 14:30
@robmry
Copy link
Contributor Author

robmry commented Dec 5, 2024

Just to note ... I haven't added a modprobe for ip_set because it's not necessary.

Tested on a host with CONFIG_IP_SET=m, and the module not loaded on the host. In a moby dev container, started a dockerd that needs ipset - ip_set and ip_set_hash_net were loaded on the host (so the netlink calls take care of it).

Comment on lines +98 to +99
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))
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - thank you!

func LoadModules(ctx context.Context, isLoaded func() error, modNames ...string) error {
if isLoaded() == nil {
log.G(ctx).WithFields(log.Fields{
"modules": strings.Join(modNames, ","),
Copy link
Member

Choose a reason for hiding this comment

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

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"}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

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)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we'd ever need access to the original error, but if we would; perhaps %w instead of %v here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM (but left two minor comments / suggestions)

Should we have a link to that original tweet as well? 😂

@robmry
Copy link
Contributor Author

robmry commented Dec 6, 2024

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]>
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]>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

still LGTM (thx!)

@thaJeztah
Copy link
Member

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).

@robmry
Copy link
Contributor Author

robmry commented Dec 6, 2024

Unrelated Windows test failures ...

=== FAIL: github.com/docker/docker/integration/networking TestNatNetworkICC/Default_nat_network (6.27s)
    nat_windows_test.go:69: assertion failed: 1 (res.ExitCode int) != 0 (int)
    nat_windows_test.go:71: assertion failed: string "Ping request could not find host ctr1. Please check the name and try again.\r\n" does not contain "Sent = 1, Received = 1, Lost = 0"
=== FAIL: github.com/docker/docker/integration/networking TestNatNetworkICC/User_defined_nat_network (8.79s)
    nat_windows_test.go:62: assertion failed: error is not nil: Post "http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.48/containers/c8cd11475e4ca1688545ceeb7348d85baa650c6e4a3885dbaac777b6ff5fbb59/start": context deadline exceeded
    panic.go:629: assertion failed: error is not nil: Error response from daemon: error while removing network: network mynat id b5dc1e8ff33c9079c6d810ec101d735fd91e4c8b3b75c57b7adf31a60fcbf1f9 has active endpoints
=== FAIL: github.com/docker/docker/integration/container TestWaitConditions/removed (60.45s)
    wait_test.go:141: assertion failed: error is not nil: error during connect: Post "http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.48/containers/create": EOF
=== FAIL: github.com/docker/docker/integration/container TestWaitRestartedContainer/not-running (60.31s)
    wait_test.go:205: assertion failed: error is not nil: error during connect: Post "http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.48/containers/create": EOF

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/daemon Core Engine area/networking/ipv6 Networking area/networking Networking impact/changelog kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. process/cherry-picked status/2-code-review status/4-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants