diff --git a/os/defaultprovider.go b/os/defaultprovider.go index 45a8c37b..4ef2dd28 100644 --- a/os/defaultprovider.go +++ b/os/defaultprovider.go @@ -13,9 +13,11 @@ var ( DefaultRegistry = sync.OnceValue(func() *Registry { provider := NewRegistry() provider.Register(ResolveLinux) - provider.Register(ResolveLinuxCompat) provider.Register(ResolveWindows) provider.Register(ResolveDarwin) + // Registered last because it is the last resort, but it does not depend on + // that: it declines any host ResolveLinux can identify. See readOSRelease. + provider.Register(ResolveLinuxCompat) return provider }) diff --git a/os/defaultprovider_test.go b/os/defaultprovider_test.go new file mode 100644 index 00000000..ac5b9367 --- /dev/null +++ b/os/defaultprovider_test.go @@ -0,0 +1,138 @@ +package os + +import ( + "testing" + + ps "github.com/k0sproject/rig/v2/powershell" + "github.com/k0sproject/rig/v2/rigtest" +) + +const ubuntuOSRelease = `PRETTY_NAME="Ubuntu 22.04.5 LTS" +NAME="Ubuntu" +VERSION_ID="22.04" +ID=ubuntu +ID_LIKE=debian +` + +// linuxRunner returns a runner that answers every probe the Linux resolvers +// make. osRelease may be empty to simulate a host with no os-release file, +// which is what the compat resolver exists for. +func linuxRunner(osRelease string) *rigtest.MockRunner { + mr := rigtest.NewMockRunner() + mr.AddCommandFailure(rigtest.Equal("uname | grep -q Darwin"), errCommandFailed) + mr.AddCommandSuccess(rigtest.Equal("uname | grep -q Linux")) + mr.AddCommandOutput(rigtest.Equal("uname -m"), "x86_64") + + if osRelease == "" { + mr.AddCommandFailure(rigtest.Equal(osReleaseCommand), errCommandFailed) + } else { + mr.AddCommandOutput(rigtest.Equal(osReleaseCommand), osRelease) + } + + // apt-get present, everything else absent, so compat resolves to the debian family. + for _, entry := range packageManagerID { + probe := rigtest.Equal("command -v " + entry.bin + " > /dev/null 2>&1") + if entry.bin == "apt-get" { + mr.AddCommandSuccess(probe) + } else { + mr.AddCommandFailure(probe, errCommandFailed) + } + } + + return mr +} + +// windowsRunner returns a runner that answers the probes ResolveWindows makes. +func windowsRunner() *rigtest.MockRunner { + mr := rigtest.NewMockRunner() + mr.Windows = true + mr.AddCommandOutput(rigtest.Equal(ps.Cmd("Get-CimInstance -ClassName Win32_OperatingSystem | Select-Object Caption, Version | ConvertTo-Json")), + `{"Caption":"Microsoft Windows Server 2022","Version":"10.0.20348"}`) + mr.AddCommandOutput(rigtest.Equal(ps.Cmd("$env:PROCESSOR_ARCHITECTURE")), "AMD64") + + return mr +} + +// TestDefaultRegistryOrderingIsStable checks that resolving one host cannot +// change how the next one resolves, using the real DefaultRegistry rather than a +// purpose-built one. +// +// This is the sequence the bug was found on: Get used to move the factory that +// matched to the front of the list, so resolving a Windows host displaced +// ResolveLinux and left ResolveLinuxCompat -- which accepts any Linux host -- +// ahead of it. Every Linux host after that was reported as ID "linux" with no +// version. +func TestDefaultRegistryOrderingIsStable(t *testing.T) { + registry := DefaultRegistry() + + before, err := registry.Get(linuxRunner(ubuntuOSRelease)) + if err != nil { + t.Fatalf("resolving a Linux host failed: %v", err) + } + if before.ID != "ubuntu" || before.Version != "22.04" { + t.Fatalf("baseline: got ID %q version %q, want %q %q", before.ID, before.Version, "ubuntu", "22.04") + } + + win, err := registry.Get(windowsRunner()) + if err != nil { + t.Fatalf("resolving a Windows host failed: %v", err) + } + if win.ID != "windows" { + t.Fatalf("windows host: got ID %q, want %q", win.ID, "windows") + } + + after, err := registry.Get(linuxRunner(ubuntuOSRelease)) + if err != nil { + t.Fatalf("resolving a Linux host after a Windows host failed: %v", err) + } + if after.ID != "ubuntu" || after.Version != "22.04" { + t.Errorf("after resolving a Windows host: got ID %q version %q, want %q %q -- the compat fallback answered ahead of ResolveLinux", + after.ID, after.Version, "ubuntu", "22.04") + } +} + +// TestCompatResolverIsOrderIndependent is the property that replaces the ordering +// rule this bug came from. Because ResolveLinuxCompat declines any host os-release +// can identify, a registry that consults it first still resolves those hosts +// correctly -- so a caller can add resolvers to a registry without having to know +// where the last-resort one sits. +func TestCompatResolverIsOrderIndependent(t *testing.T) { + registry := NewRegistry() + // Deliberately the wrong way round: the catch-all resolver first. + RegisterLinuxCompat(registry) + RegisterLinux(registry) + + rel, err := registry.Get(linuxRunner(ubuntuOSRelease)) + if err != nil { + t.Fatalf("resolving a Linux host failed: %v", err) + } + if rel.ID != "ubuntu" || rel.Version != "22.04" { + t.Errorf("compat resolver answered ahead of ResolveLinux: got ID %q version %q, want %q %q", + rel.ID, rel.Version, "ubuntu", "22.04") + } + + // And it still answers for a host that has no os-release, from either position. + rel, err = registry.Get(linuxRunner("")) + if err != nil { + t.Fatalf("compat resolver was not reached: %v", err) + } + if rel.ID != "linux" { + t.Errorf("ID: got %q, want %q", rel.ID, "linux") + } +} + +// TestDefaultRegistryStillFallsBackToCompat confirms the compat resolver is still +// reached when no specific resolver matches, which is the case it exists for: a +// host with no os-release file at all. +func TestDefaultRegistryStillFallsBackToCompat(t *testing.T) { + rel, err := DefaultRegistry().Get(linuxRunner("")) + if err != nil { + t.Fatalf("compat fallback was not reached: %v", err) + } + if rel.ID != "linux" { + t.Errorf("ID: got %q, want %q", rel.ID, "linux") + } + if len(rel.IDLike) != 1 || rel.IDLike[0] != "debian" { + t.Errorf("IDLike: got %v, want [debian]", rel.IDLike) + } +} diff --git a/os/linux.go b/os/linux.go index 294f4aaf..e3184f51 100644 --- a/os/linux.go +++ b/os/linux.go @@ -9,6 +9,10 @@ import ( "github.com/k0sproject/rig/v2/log" ) +// osReleaseCommand reads the os-release file from either of the two standard +// locations. +const osReleaseCommand = "cat /etc/os-release || cat /usr/lib/os-release" + // ResolveLinux resolves the OS release information for a linux host. func ResolveLinux(conn cmd.SimpleRunner) (*Release, bool) { if conn.IsWindows() { @@ -20,20 +24,41 @@ func ResolveLinux(conn cmd.SimpleRunner) (*Release, bool) { return nil, false } - reader := conn.ExecReader("cat /etc/os-release || cat /usr/lib/os-release") - decoder := kv.NewDecoder(reader) - - version := &Release{} - if err := decoder.Decode(version); err != nil { - log.Trace(context.Background(), "linux os resolver: execreader returned an error", log.HostAttr(conn), log.ErrorAttr(err)) + release, ok := readOSRelease(conn) + if !ok { return nil, false } if arch, err := conn.ExecOutput("uname -m"); err == nil { - version.arch = strings.TrimSpace(arch) + release.arch = strings.TrimSpace(arch) + } + + return release, true +} + +// readOSRelease parses the os-release file of a host already known to be Linux. +// +// It reports false unless the file yields an ID, since a Release that does not +// name the distribution is of no use to a caller. A host whose os-release is +// missing, unreadable or silent about the ID is left to ResolveLinuxCompat, which +// can still identify it from its package manager. +// +// ResolveLinuxCompat calls this to decide whether ResolveLinux is going to handle +// a host, which keeps the two resolvers complementary without either of them +// depending on the order they were registered in. +func readOSRelease(conn cmd.SimpleRunner) (*Release, bool) { + release := &Release{} + if err := kv.NewDecoder(conn.ExecReader(osReleaseCommand)).Decode(release); err != nil { + log.Trace(context.Background(), "linux os resolver: failed to decode os-release", log.HostAttr(conn), log.ErrorAttr(err)) + return nil, false + } + + if release.ID == "" { + log.Trace(context.Background(), "linux os resolver: os-release did not yield an ID", log.HostAttr(conn)) + return nil, false } - return version, true + return release, true } // RegisterLinux registers the linux OS release resolver to a provider. diff --git a/os/linux_compat.go b/os/linux_compat.go index 2f144982..34d2fe17 100644 --- a/os/linux_compat.go +++ b/os/linux_compat.go @@ -43,6 +43,21 @@ func ResolveLinuxCompat(conn cmd.SimpleRunner) (*Release, bool) { return nil, false } + // ResolveLinux identifies any host whose os-release names the distribution, + // and this resolver matches every Linux host, so it has to stand down for + // those rather than rely on being consulted afterwards. Deciding it from the + // host keeps the two complementary however the registry happens to be + // ordered, including once a caller has added resolvers of their own. yum + // (defers to dnf) and SysVinit (defers to systemd) exclude themselves the + // same way. + if _, ok := readOSRelease(conn); ok { + log.Trace(context.Background(), "linux compat resolver: os-release identifies the host, deferring to the standard resolver", + log.HostAttr(conn), + ) + + return nil, false + } + release := &Release{ ID: "linux", Name: "Linux (compatibility mode)", @@ -79,8 +94,8 @@ func ResolveLinuxCompat(conn cmd.SimpleRunner) (*Release, bool) { } // RegisterLinuxCompat registers the Linux compatibility resolver to a provider. -// It should be registered after ResolveLinux so it only activates when the -// standard os-release files are absent. +// It excludes itself on any host ResolveLinux can identify, so it does not matter +// when it is registered relative to the other resolvers. func RegisterLinuxCompat(provider *Registry) { provider.Register(ResolveLinuxCompat) } diff --git a/os/linux_compat_test.go b/os/linux_compat_test.go index 3e23b637..d9ed4a4a 100644 --- a/os/linux_compat_test.go +++ b/os/linux_compat_test.go @@ -11,6 +11,9 @@ func setupCompatRunner(pm string) *rigtest.MockRunner { mr := rigtest.NewMockRunner() mr.AddCommand(rigtest.HasPrefix("uname"), func(_ *rigtest.A) error { return nil }) mr.AddCommandOutput(rigtest.Equal("uname -m"), "x86_64") + // No os-release: the case the compat resolver exists for. With one present it + // stands down for ResolveLinux instead. + mr.AddCommandFailure(rigtest.Equal(osReleaseCommand), errCommandFailed) for _, entry := range packageManagerID { if entry.bin == pm { mr.AddCommand(rigtest.Equal("command -v "+entry.bin+" > /dev/null 2>&1"), func(_ *rigtest.A) error { return nil }) @@ -91,6 +94,7 @@ func TestResolveLinuxCompatNoPackageManager(t *testing.T) { mr := rigtest.NewMockRunner() mr.AddCommand(rigtest.HasPrefix("uname"), func(_ *rigtest.A) error { return nil }) mr.AddCommandOutput(rigtest.Equal("uname -m"), "x86_64") + mr.AddCommandFailure(rigtest.Equal(osReleaseCommand), errCommandFailed) for _, entry := range packageManagerID { mr.AddCommandFailure(rigtest.Equal("command -v "+entry.bin+" > /dev/null 2>&1"), errCommandFailed) } @@ -106,6 +110,25 @@ func TestResolveLinuxCompatNoPackageManager(t *testing.T) { } } +// TestResolveLinuxCompatDefersToOSRelease is the self-exclusion that removes the +// need for any ordering rule between the two Linux resolvers: on a host whose +// os-release names the distribution, the compat resolver must decline so +// ResolveLinux answers, no matter which of them is consulted first. +func TestResolveLinuxCompatDefersToOSRelease(t *testing.T) { + mr := rigtest.NewMockRunner() + mr.AddCommand(rigtest.HasPrefix("uname"), func(_ *rigtest.A) error { return nil }) + mr.AddCommandOutput(rigtest.Equal("uname -m"), "x86_64") + mr.AddCommandOutput(rigtest.Equal(osReleaseCommand), ubuntuOSRelease) + + if _, ok := ResolveLinuxCompat(mr); ok { + t.Error("ResolveLinuxCompat answered for a host os-release can identify") + } + // It must decide that from os-release alone, without probing package managers. + if err := mr.NotReceived(rigtest.HasPrefix("command -v")); err != nil { + t.Errorf("compat resolver probed package managers before standing down: %v", err) + } +} + func TestResolveLinuxCompatNotLinux(t *testing.T) { mr := rigtest.NewMockRunner() mr.AddCommandFailure(rigtest.HasPrefix("uname"), errCommandFailed) diff --git a/os/linux_test.go b/os/linux_test.go index 8282c573..02f1d830 100644 --- a/os/linux_test.go +++ b/os/linux_test.go @@ -120,3 +120,28 @@ REDHAT_SUPPORT_PRODUCT_VERSION="8.9"` t.Errorf("Arch() returned wrong value: %q != 'amd64'", arch) } } + +// TestResolveLinuxRequiresAnID pins the condition ResolveLinuxCompat keys its +// self-exclusion off. An os-release that does not name the distribution is not a +// usable result, so ResolveLinux has to decline and leave the host to the compat +// resolver, which can still identify it from its package manager. +func TestResolveLinuxRequiresAnID(t *testing.T) { + for _, tc := range []struct { + name string + osRelease string + }{ + {"no ID field", "PRETTY_NAME=\"Something\"\nVERSION_ID=\"1.0\"\n"}, + {"empty file", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + mr := rigtest.NewMockRunner() + mr.AddCommandOutput(rigtest.Equal("uname -m"), "x86_64") + mr.AddCommand(rigtest.HasPrefix("uname"), func(_ *rigtest.A) error { return nil }) + mr.AddCommandOutput(rigtest.Equal(osReleaseCommand), tc.osRelease) + + if _, ok := ResolveLinux(mr); ok { + t.Error("ResolveLinux claimed a host whose os-release does not name the distribution") + } + }) + } +} diff --git a/plumbing/provider.go b/plumbing/provider.go index 797d0c1c..6d4e6e75 100644 --- a/plumbing/provider.go +++ b/plumbing/provider.go @@ -2,7 +2,11 @@ package plumbing import "sync" -// Factory is a function that takes a parameter of type R and returns a value of type T or an error. +// Factory is a function that takes a parameter of type R and returns a value of +// type T along with a boolean reporting whether it could handle R. +// +// A Factory may be called concurrently with itself, for the same input as well as +// for different ones, so it must not depend on being called one at a time. type Factory[R any, T any] func(R) (T, bool) // Provider is a generic provider of values of type T that can be initialized with a value of type R. @@ -13,50 +17,55 @@ type Provider[R any, T any] struct { } // Register adds a new factory to the provider. +// +// Factories are consulted in registration order and the first one to match wins. +// Prefer factories that decide for themselves whether they apply to an input over +// relying on that order: a factory matching a superset of another's inputs should +// exclude the cases the more specific one handles, the way os.ResolveLinuxCompat +// stands down for any host os-release can identify. A registry stays open for +// registration, so a broad factory registered early keeps winning over any more +// specific one a caller adds later. func (p *Provider[R, T]) Register(f Factory[R, T]) { p.mu.Lock() defer p.mu.Unlock() p.factories = append(p.factories, f) } -// Get retrieves the first value of type T from the Factories in the Provider. -// If none can be found, the error supplied at creation time is returned. -// The first factory that does not error is moved to the front of the list to optimize -// future lookups. -func (p *Provider[R, T]) Get(r R) (T, error) { - p.mu.Lock() - defer p.mu.Unlock() - for i, f := range p.factories { - t, ok := f(r) - if ok { - if i != 0 { - // Move the factory to the front of the list to optimize future lookups, since - // it's likely that most of the hosts during multi-host operations will be - // running the same kind of environment. - p.factories[0], p.factories[i] = p.factories[i], p.factories[0] - } +// Get returns the value from the first factory that reports a match, in +// registration order. If none of them match, the error supplied at creation time +// is returned. +// +// The order in which factories are consulted never changes, so the result for a +// given input does not depend on what was looked up before it. +func (p *Provider[R, T]) Get(input R) (T, error) { + p.mu.RLock() + defer p.mu.RUnlock() + for _, f := range p.factories { + if t, ok := f(input); ok { return t, nil } } + return *new(T), p.err } -// GetAll retrieves all values of type T from the Factories in the Provider. -// If none that does not error can be found, the error supplied at creation time is returned. -func (p *Provider[R, T]) GetAll(r R) ([]T, error) { +// GetAll returns the values from every factory that reports a match, in +// registration order. If none of them match, the error supplied at creation time +// is returned. +func (p *Provider[R, T]) GetAll(input R) ([]T, error) { p.mu.RLock() defer p.mu.RUnlock() - var ts []T + var values []T for _, f := range p.factories { - t, ok := f(r) - if ok { - ts = append(ts, t) + if t, ok := f(input); ok { + values = append(values, t) } } - if len(ts) == 0 { + if len(values) == 0 { return nil, p.err } - return ts, nil + + return values, nil } // NewProvider creates a new instance of Provider. diff --git a/plumbing/provider_test.go b/plumbing/provider_test.go index c1ea9b7c..302d0f85 100644 --- a/plumbing/provider_test.go +++ b/plumbing/provider_test.go @@ -2,6 +2,7 @@ package plumbing_test import ( "errors" + "sync" "testing" "github.com/k0sproject/rig/v2/plumbing" @@ -64,3 +65,144 @@ func TestGetAllNoFactory(t *testing.T) { require.Error(t, err) assert.Nil(t, values) } + +// TestGetPreservesRegistrationOrder covers the guarantee that makes registration +// order meaningful: where two factories both match an input, the one registered +// first wins, and looking up a different input beforehand cannot change that. +// +// Get used to move the factory that matched to the front of the list to save +// probes on later lookups, which broke exactly this. +func TestGetPreservesRegistrationOrder(t *testing.T) { + p := plumbing.NewProvider[string, string](errors.New("no factory available")) + + // Registered first, so it must win for "both". + p.Register(func(in string) (string, bool) { + if in == "both" { + return "first", true + } + + return "", false + }) + // Overlaps on "both", and is the only match for "other". + p.Register(func(in string) (string, bool) { + if in == "both" || in == "other" { + return "second", true + } + + return "", false + }) + + got, err := p.Get("both") + require.NoError(t, err) + assert.Equal(t, "first", got) + + // Resolving "other" is answered by the second factory... + got, err = p.Get("other") + require.NoError(t, err) + assert.Equal(t, "second", got) + + // ...which must not have moved it ahead of the first. + got, err = p.Get("both") + require.NoError(t, err) + assert.Equal(t, "first", got, "the second factory answered ahead of the one registered before it") +} + +// TestGetIsSafeForConcurrentUse asserts that parallel lookups all observe the +// same registration order. Get holds only a read lock, so factories run +// concurrently; this is the case that matters under -race. +func TestGetIsSafeForConcurrentUse(t *testing.T) { + p := plumbing.NewProvider[string, string](errors.New("no factory available")) + p.Register(func(in string) (string, bool) { + if in == "both" { + return "first", true + } + + return "", false + }) + p.Register(func(in string) (string, bool) { + if in == "both" || in == "other" { + return "second", true + } + + return "", false + }) + + const workers = 64 + + type result struct { + input string + got string + err error + } + results := make([]result, workers) + + var wg sync.WaitGroup + wg.Add(workers) + for i := range workers { + go func() { + defer wg.Done() + // Half look up the input only the second factory matches, which is what + // used to reorder the shared list out from under everyone else. + input := "both" + if i%2 == 0 { + input = "other" + } + got, err := p.Get(input) + results[i] = result{input: input, got: got, err: err} + }() + } + wg.Wait() + + want := map[string]string{"both": "first", "other": "second"} + for i, res := range results { + require.NoErrorf(t, res.err, "worker %d", i) + assert.Equalf(t, want[res.input], res.got, "worker %d looked up %q", i, res.input) + } +} + +// TestGetLetsASelfExcludingFactoryBeRegisteredFirst covers the property that +// makes a Provider safe to extend: a factory that matches a superset of another's +// inputs but excludes the cases that other one handles gives the same answer +// wherever it sits in the list. This is what os.ResolveLinuxCompat does, and it is +// what lets a caller add factories to an already-built registry. +func TestGetLetsASelfExcludingFactoryBeRegisteredFirst(t *testing.T) { + // Matches everything except "specific", which it leaves to the factory below. + selfExcluding := func(in string) (string, bool) { + if in == "specific" { + return "", false + } + + return "general", true + } + specific := func(in string) (string, bool) { + if in == "specific" { + return "specific", true + } + + return "", false + } + + // Registered in either order, the answers are the same. + for _, tc := range []struct { + name string + order []plumbing.Factory[string, string] + }{ + {"general first", []plumbing.Factory[string, string]{selfExcluding, specific}}, + {"specific first", []plumbing.Factory[string, string]{specific, selfExcluding}}, + } { + t.Run(tc.name, func(t *testing.T) { + p := plumbing.NewProvider[string, string](errors.New("no factory available")) + for _, f := range tc.order { + p.Register(f) + } + + got, err := p.Get("specific") + require.NoError(t, err) + assert.Equal(t, "specific", got, "the general factory answered for an input the specific one handles") + + got, err = p.Get("anything") + require.NoError(t, err) + assert.Equal(t, "general", got) + }) + } +} diff --git a/sudo/defaultprovider_test.go b/sudo/defaultprovider_test.go new file mode 100644 index 00000000..b5e8dd26 --- /dev/null +++ b/sudo/defaultprovider_test.go @@ -0,0 +1,62 @@ +package sudo_test + +import ( + "testing" + + "github.com/k0sproject/rig/v2/rigtest" + "github.com/k0sproject/rig/v2/sudo" + "github.com/stretchr/testify/require" +) + +// rootRunner is a host running as root that also has a working sudo. That +// combination is the point: both RegisterUID0Noop and RegisterSudo match it, so +// which one answers is decided purely by registration order. +func rootRunner() *rigtest.MockRunner { + mr := rigtest.NewMockRunner() + mr.ErrDefault = errProbe + mr.AddCommandSuccess(rigtest.Contains("id -u")) + mr.AddCommandSuccess(rigtest.Contains("sudo -n")) + mr.AddCommandSuccess(rigtest.Equal("whoami")) + + return mr +} + +// sudoerRunner is a host that is not root but can use sudo, so RegisterSudo is +// the only factory in DefaultRegistry that matches it. +func sudoerRunner() *rigtest.MockRunner { + mr := rigtest.NewMockRunner() + mr.ErrDefault = errProbe + mr.AddCommandSuccess(rigtest.Contains("sudo -n")) + + return mr +} + +// TestDefaultRegistryPrefersNoopForRoot pins the registration order in +// DefaultRegistry, where RegisterUID0Noop comes before RegisterSudo so a root +// host runs commands unmodified rather than wrapping them in sudo needlessly. +// +// Because a root host with sudo installed matches both factories, this only holds +// while lookups are answered in registration order. Get used to move the factory +// that matched to the front of the list, so resolving an ordinary sudo host first +// pushed RegisterSudo ahead of RegisterUID0Noop, and every root host resolved +// after that got its commands wrapped in sudo. +func TestDefaultRegistryPrefersNoopForRoot(t *testing.T) { + registry := sudo.DefaultRegistry() + + // Resolve a non-root sudo host first -- the lookup that used to reorder the + // shared registry. + sudoer, err := registry.Get(sudoerRunner()) + require.NoError(t, err) + require.NotNil(t, sudoer) + + // A root host must still be given the noop decorator. + mr := rootRunner() + runner, err := registry.Get(mr) + require.NoError(t, err) + require.NoError(t, runner.Exec("whoami")) + + require.NoError(t, mr.Received(rigtest.Equal("whoami")), + "root host did not run the command unmodified") + require.NoError(t, mr.NotReceived(rigtest.Contains("sudo -n")), + "root host was given the sudo decorator instead of noop") +}