From 52cef271765a535f86b550a91d13d3a8c990a4ee Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Mon, 9 Nov 2020 13:56:14 -0600 Subject: [PATCH 1/2] client/fingerprint: detect unloaded dynamic bridge kernel module In Nomad v0.12.0, the client added additional fingerprinting around the presense of the bridge kernel module. The fingerprinter only checked in `/proc/modules` which is a list of loaded modules. In some cases, the bridge kernel module is builtin rather than dynamically loaded. The fix for that case is in #8721. However we were still missing the case where the bridge module is dynamically loaded, but not yet loaded during the startup of the Nomad agent. In this case the fingerprinter would believe the bridge module was unavailable when really it gets loaded on demand. This PR now has the fingerprinter scan the kernel module dependency file, which will contain an entry for the bridge module even if it is not yet loaded. In summary, the client now looks for the bridge kernel module in - /proc/modules - /lib/modules//modules.builtin - /lib/modules//modules.dep Closes #8423 --- client/fingerprint/bridge.go | 3 +- client/fingerprint/bridge_linux.go | 85 ++++++++++------ client/fingerprint/bridge_linux_test.go | 125 +++++++++++++++++++++++- 3 files changed, 178 insertions(+), 35 deletions(-) diff --git a/client/fingerprint/bridge.go b/client/fingerprint/bridge.go index 163e8cbea19..fd58386be0c 100644 --- a/client/fingerprint/bridge.go +++ b/client/fingerprint/bridge.go @@ -3,8 +3,9 @@ package fingerprint import log "github.com/hashicorp/go-hclog" type BridgeFingerprint struct { - logger log.Logger StaticFingerprinter + + logger log.Logger } func NewBridgeFingerprint(logger log.Logger) Fingerprint { diff --git a/client/fingerprint/bridge_linux.go b/client/fingerprint/bridge_linux.go index 39eeb4e1c6f..ff0b6c07a8c 100644 --- a/client/fingerprint/bridge_linux.go +++ b/client/fingerprint/bridge_linux.go @@ -6,71 +6,98 @@ import ( "os" "regexp" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/nomad/nomad/structs" "github.com/shirou/gopsutil/host" ) const bridgeKernelModuleName = "bridge" +const ( + dynamicModuleRe = `%s\s+.*$` + builtinModuleRe = `.+/%s.ko$` + dependsModuleRe = `.+/%s.ko:.*$` +) + func (f *BridgeFingerprint) Fingerprint(req *FingerprintRequest, resp *FingerprintResponse) error { - if err := f.checkKMod(bridgeKernelModuleName); err != nil { + if err := f.detect(bridgeKernelModuleName); err != nil { f.logger.Warn("failed to detect bridge kernel module, bridge network mode disabled", "error", err) return nil } resp.NodeResources = &structs.NodeResources{ - Networks: []*structs.NetworkResource{ - { - Mode: "bridge", - }, - }, - NodeNetworks: []*structs.NodeNetworkResource{ - { - Mode: "bridge", - Device: req.Config.BridgeNetworkName, - }, - }, + Networks: []*structs.NetworkResource{{ + Mode: "bridge", + }}, + NodeNetworks: []*structs.NodeNetworkResource{{ + Mode: "bridge", + Device: req.Config.BridgeNetworkName, + }}, } + resp.Detected = true return nil } -func (f *BridgeFingerprint) checkKMod(mod string) error { +func (f *BridgeFingerprint) regexp(pattern, module string) *regexp.Regexp { + return regexp.MustCompile(fmt.Sprintf(pattern, module)) +} + +func (f *BridgeFingerprint) detect(module string) error { + // accumulate errors from every place we might find the module + var errs error + + // check if the module has been dynamically loaded + dynamicPath := "/proc/modules" + if err := f.searchFile(module, dynamicPath, f.regexp(dynamicModuleRe, module)); err != nil { + errs = multierror.Append(errs, err) + } else { + return nil + } + + // will need kernel info to look for builtin and unloaded modules hostInfo, err := host.Info() if err != nil { return err } - dynErr := f.checkKModFile(mod, "/proc/modules", fmt.Sprintf("%s\\s+.*$", mod)) - if dynErr == nil { + // check if the module is builtin to the kernel + builtinPath := fmt.Sprintf("/lib/modules/%s/modules.builtin", hostInfo.KernelVersion) + if err := f.searchFile(module, builtinPath, f.regexp(builtinModuleRe, module)); err != nil { + errs = multierror.Append(errs, err) + } else { return nil } - builtinErr := f.checkKModFile(mod, - fmt.Sprintf("/lib/modules/%s/modules.builtin", hostInfo.KernelVersion), - fmt.Sprintf(".+\\/%s.ko$", mod)) - if builtinErr == nil { + // check if the module is dynamic but unloaded (will have a dep entry) + dependsPath := fmt.Sprintf("/lib/modules/%s/modules.dep", hostInfo.KernelVersion) + if err := f.searchFile(module, dependsPath, f.regexp(dependsModuleRe, module)); err != nil { + errs = multierror.Append(errs, err) + } else { return nil } - return fmt.Errorf("%v, %v", dynErr, builtinErr) + return errs } -func (f *BridgeFingerprint) checkKModFile(mod, fileName, pattern string) error { - file, err := os.Open(fileName) +func (f *BridgeFingerprint) searchFile(module, filename string, re *regexp.Regexp) error { + file, err := os.Open(filename) if err != nil { - return fmt.Errorf("could not read %s: %v", fileName, err) + return fmt.Errorf("failed to open %s: %v", filename, err) } - defer file.Close() + defer func() { + _ = file.Close() + }() scanner := bufio.NewScanner(file) for scanner.Scan() { - if matched, err := regexp.MatchString(pattern, scanner.Text()); matched { - return nil - } else if err != nil { - return fmt.Errorf("could not parse %s: %v", fileName, err) + if re.MatchString(scanner.Text()) { + return nil // found the module! } } + if err := scanner.Err(); err != nil { + return fmt.Errorf("failed to scan %s: %v", filename, err) + } - return fmt.Errorf("could not detect kernel module %s", mod) + return fmt.Errorf("module %s not in %s", module, filename) } diff --git a/client/fingerprint/bridge_linux_test.go b/client/fingerprint/bridge_linux_test.go index 4e55c92dcee..e8ac5c897f9 100644 --- a/client/fingerprint/bridge_linux_test.go +++ b/client/fingerprint/bridge_linux_test.go @@ -1,14 +1,129 @@ package fingerprint import ( + "fmt" + "io" + "io/ioutil" + "os" + "strings" "testing" + "github.com/hashicorp/nomad/helper/testlog" "github.com/stretchr/testify/require" ) -func TestBridgeFingerprint_checkKMod(t *testing.T) { - require := require.New(t) - f := &BridgeFingerprint{} - require.NoError(f.checkKMod("ip_tables")) - require.Error(f.checkKMod("nonexistentmodule")) +func TestBridgeFingerprint_detect(t *testing.T) { + f := &BridgeFingerprint{logger: testlog.HCLogger(t)} + require.NoError(t, f.detect("ip_tables")) + + err := f.detect("nonexistentmodule") + require.Error(t, err) + require.Contains(t, err.Error(), "3 errors occurred") +} + +func writeFile(t *testing.T, prefix, content string) string { + f, err := ioutil.TempFile("", "bridge-fp-") + require.NoError(t, err) + + _, err = io.Copy(f, strings.NewReader(content)) + require.NoError(t, err) + + err = f.Close() + require.NoError(t, err) + + return f.Name() +} + +func cleanupFile(t *testing.T, name string) { + err := os.Remove(name) + require.NoError(t, err) +} + +const ( + dynamicModuleContent = ` +ip_tables 32768 0 - Live 0xffffffffc03ee000 +x_tables 40960 1 ip_tables, Live 0xffffffffc03e3000 +autofs4 45056 2 - Live 0xffffffffc03d7000 +bpfilter 32768 0 - Live 0x0000000000000000 +br_netfilter 28672 0 - Live 0x0000000000000000 +bridge 176128 1 br_netfilter, Live 0x0000000000000000 +btrfs 1253376 0 - Live 0xffffffffc02a4000 +` + + builtinModuleContent = ` +kernel/drivers/mfd/max14577.ko +kernel/drivers/mfd/max77693.ko +kernel/drivers/mfd/sec-core.ko +kernel/drivers/mfd/sec-irq.ko +kernel/drivers/net/bridge.ko +kernel/drivers/net/tun.ko +kernel/drivers/net/xen-netfront.k +` + + dependsModuleContent = ` +kernel/net/bridge/netfilter/ebt_log.ko: kernel/net/netfilter/x_tables.ko +kernel/net/bridge/netfilter/ebt_nflog.ko: kernel/net/netfilter/x_tables.ko +kernel/net/bridge/bridge.ko: kernel/net/802/stp.ko kernel/net/llc/llc.ko +kernel/net/bridge/br_netfilter.ko: kernel/net/bridge/bridge.ko kernel/net/802/stp.ko kernel/net/llc/llc.ko +kernel/net/appletalk/appletalk.ko: kernel/net/802/psnap.ko kernel/net/llc/llc.ko +kernel/net/x25/x25.ko: +` +) + +func TestBridgeFingerprint_search(t *testing.T) { + f := &BridgeFingerprint{logger: testlog.HCLogger(t)} + + t.Run("dynamic loaded module", func(t *testing.T) { + t.Run("present", func(t *testing.T) { + file := writeFile(t, "bridge-fp-", dynamicModuleContent) + defer cleanupFile(t, file) + + err := f.searchFile("bridge", file, f.regexp(dynamicModuleRe, "bridge")) + require.NoError(t, err) + }) + + t.Run("absent", func(t *testing.T) { + file := writeFile(t, "bridge-fp-", dynamicModuleContent) + defer cleanupFile(t, file) + + err := f.searchFile("nonexistent", file, f.regexp(dynamicModuleRe, "nonexistent")) + require.EqualError(t, err, fmt.Sprintf("module nonexistent not in %s", file)) + }) + }) + + t.Run("builtin module", func(t *testing.T) { + t.Run("present", func(t *testing.T) { + file := writeFile(t, "bridge-fp-", builtinModuleContent) + defer cleanupFile(t, file) + + err := f.searchFile("bridge", file, f.regexp(builtinModuleRe, "bridge")) + require.NoError(t, err) + }) + + t.Run("absent", func(t *testing.T) { + file := writeFile(t, "bridge-fp-", builtinModuleContent) + defer cleanupFile(t, file) + + err := f.searchFile("nonexistent", file, f.regexp(builtinModuleRe, "nonexistent")) + require.EqualError(t, err, fmt.Sprintf("module nonexistent not in %s", file)) + }) + }) + + t.Run("dynamic unloaded module", func(t *testing.T) { + t.Run("present", func(t *testing.T) { + file := writeFile(t, "bridge-fp-", dependsModuleContent) + defer cleanupFile(t, file) + + err := f.searchFile("bridge", file, f.regexp(dependsModuleRe, "bridge")) + require.NoError(t, err) + }) + + t.Run("absent", func(t *testing.T) { + file := writeFile(t, "bridge-fp-", dependsModuleContent) + defer cleanupFile(t, file) + + err := f.searchFile("nonexistent", file, f.regexp(dependsModuleRe, "nonexistent")) + require.EqualError(t, err, fmt.Sprintf("module nonexistent not in %s", file)) + }) + }) } From 3ad75f0c04ed06967bce05d03e17ed83d272fc0b Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Mon, 9 Nov 2020 14:16:33 -0600 Subject: [PATCH 2/2] docs: add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5057e1ac914..691adfdfcc0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ BUG FIXES: * core: Fixed a bug where ACL handling prevented cross-namespace allocation listing [[GH-9278](https://github.com/hashicorp/nomad/issues/9278)] * config (Enterprise): Fixed default enterprise config merging. [[GH-9083](https://github.com/hashicorp/nomad/pull/9083)] * client: Fixed an issue with the Java fingerprinter on macOS causing pop-up notifications when no JVM installed. [[GH-9225](https://github.com/hashicorp/nomad/pull/9225)] + * client: Fixed an fingerprinter issue detecting bridge kernel module [[GH-9299](https://github.com/hashicorp/nomad/pull/9299)] * consul: Fixed a bug to correctly validate task when using script-checks in group-level services [[GH-8952](https://github.com/hashicorp/nomad/issues/8952)] * consul: Fixed a bug where canary_meta was not being interpolated with environment variables [[GH-9096](https://github.com/hashicorp/nomad/pull/9096)] * consul/connect: Fixed a bug to correctly trigger updates on jobspec changes [[GH-9029](https://github.com/hashicorp/nomad/pull/9029)]