Fix linux detection when cluster contains windows hosts

docs/EXTENDING.md

@@ -19,9 +19,10 @@ Every pluggable subsystem follows one shape:
 - A **`Factory`** is a detection closure: `func(runner) (Impl, bool)`. It inspects the
   host and returns `(impl, true)` if it can serve this host, or `(nil, false)` to pass.
 - A **`Registry`** holds an ordered list of factories. `Registry.Get(runner)` tries them
-  in registration order and returns the first match (the matched factory is then moved to
-  the front to speed up subsequent hosts in a multi-host run). If none match, it returns
-  the subsystem's "not found" error.
+  in registration order and returns the first match. If none match, it returns
+  the subsystem's "not found" error. A lookup never reorders the list, so what one host
+  resolved to cannot change what the next one resolves to. `Register` appends;
+  `RegisterFirst` puts a factory at the front, ahead of everything else.
 - A **`With*Provider`** client option injects a registry's `Get` method into a client.
 
 `Registry.Get` has exactly the signature the matching `With*Provider` option expects, so
@@ -36,7 +37,8 @@ they compose directly. The subsystems and their types:
 | Remote filesystem | `remotefs` | `cmd.Runner` | `remotefs.FS` | `WithRemoteFSProvider` |
 
 Each package also exposes a `DefaultRegistry()` (a memoized singleton holding rig's
-built-in factories) and `NewRegistry()` (an empty one). The default client uses
+built-in factories), `NewRegistry()` (an empty one) and `RegisterDefaults(reg)`
+(rig's built-ins, added to a registry of yours). The default client uses
 `packagemanager.DefaultRegistry().Get` and similar for the other providers.
 
 ## Example: a custom package manager
@@ -93,13 +95,12 @@ can't serve, so the registry can fall through to the next candidate.
 
 #### Per client
 
-Build a registry with your factory first, add whichever built-ins you still want as fallbacks, and inject it:
+Build a registry with your factory first, add the built-ins behind it, and inject it:
 
 ```go
 reg := packagemanager.NewRegistry()
-mypkg.RegisterFoo(reg)              // tried first → wins when foopkg is present
-packagemanager.RegisterApt(reg)     // fall back to the built-ins you care about
-packagemanager.RegisterApk(reg)
+mypkg.RegisterFoo(reg)               // tried first → wins when foopkg is present
+packagemanager.RegisterDefaults(reg) // then everything rig ships
 
 client, err := rig.NewClient(
 	rig.WithConnection(conn),
@@ -108,22 +109,56 @@ client, err := rig.NewClient(
 ```
 
 Order matters: factories are tried in registration order, so put a factory that should
-override a built-in ahead of it. (To keep every built-in as a fallback without
-listing them, register your factory and then re-register the defaults or use the
-global approach below if you only want to add support, not override it.)
+override a built-in ahead of it. `RegisterDefaults` appends, so calling it before
+`RegisterFoo` would leave your factory behind the built-ins. If you only want a few
+of them, the individual `RegisterApt`/`RegisterApk`/… functions are still there —
+at the cost of not picking up managers rig adds in later versions.
 
 #### Globally
 
-Append your factory to the shared default registry from an `init()`. It becomes available to
-every client built with default options. Because built-ins are registered first, yours is
-used only when none of them match:
+Add your factory to the shared default registry from an `init()`. It becomes available to
+every client built with default options. `Register` appends, so yours is used only when
+none of the built-ins match:
 
 ```go
 func init() {
 	mypkg.RegisterFoo(packagemanager.DefaultRegistry())
 }
 ```
 
+#### Overriding a built-in
+
+Appending is not enough when a built-in matches the same hosts your factory does —
+it is consulted first, so yours is never reached. `RegisterFirst` puts a factory at
+the very front, ahead of everything registered before or after it.
+
+A `RegisterFoo`-style helper picks `Register` on the caller's behalf, so export the
+factory itself if you want to leave them the choice:
+
+```go
+// In mypkg, alongside RegisterFoo.
+func FooFactory(c cmd.ContextRunner) (packagemanager.PackageManager, bool) {
+	// ...same detection as above...
+}
+
+// In the consumer.
+func init() {
+	packagemanager.DefaultRegistry().RegisterFirst(mypkg.FooFactory)
+}
+```
+
+This is the lever for the case rig's own factories solve by self-exclusion, where a
+factory matching a superset of another's hosts declines the ones the more specific
+factory handles — `os.ResolveLinuxCompat` stands down for any host `os-release` can
+name, `yum` for a host with `dnf`, SysVinit for a host with systemd. That is the
+better pattern where it is available, but it is only available to whoever owns the
+broad factory. A caller cannot make a built-in stand down for a factory that did not
+exist when it was written, and `RegisterFirst` is what they use instead.
+
+Two things to know about it: where it is called more than once the most recent call
+wins, and a resolved value is memoized per host, so register from an `init()` or
+before constructing clients rather than after.
+
 ## Init system and OS release
 
 The mechanism is identical; only the factory's input/output types differ.

initsystem/defaultprovider.go

@@ -59,13 +59,7 @@ var (
 	// DefaultRegistry is the default repository for init systems.
 	DefaultRegistry = sync.OnceValue(func() *Registry {
 		provider := NewRegistry()
-		RegisterSystemd(provider)
-		RegisterOpenRC(provider)
-		RegisterUpstart(provider)
-		RegisterSysVinit(provider)
-		RegisterWinSCM(provider)
-		RegisterRunit(provider)
-		RegisterLaunchd(provider)
+		RegisterDefaults(provider)
 		return provider
 	})
 
@@ -86,3 +80,19 @@ type Registry = plumbing.Provider[cmd.ContextRunner, ServiceManager]
 func NewRegistry() *Registry {
 	return plumbing.NewProvider[cmd.ContextRunner, ServiceManager](ErrNoInitSystem)
 }
+
+// RegisterDefaults registers the init systems rig ships with, which is what
+// DefaultRegistry holds. Use it to build a registry of your own without having to
+// list them, and without missing init systems added in later versions.
+//
+// The factories are appended, so one of your own that has to take precedence over
+// them must be registered before this call, or with Registry.RegisterFirst.
+func RegisterDefaults(provider *Registry) {
+	RegisterSystemd(provider)
+	RegisterOpenRC(provider)
+	RegisterUpstart(provider)
+	RegisterSysVinit(provider)
+	RegisterWinSCM(provider)
+	RegisterRunit(provider)
+	RegisterLaunchd(provider)
+}

os/defaultprovider.go

@@ -12,10 +12,7 @@ var (
 	// DefaultRegistry is the default OS release registry.
 	DefaultRegistry = sync.OnceValue(func() *Registry {
 		provider := NewRegistry()
-		provider.Register(ResolveLinux)
-		provider.Register(ResolveLinuxCompat)
-		provider.Register(ResolveWindows)
-		provider.Register(ResolveDarwin)
+		RegisterDefaults(provider)
 		return provider
 	})
 
@@ -36,3 +33,19 @@ type ReleaseProvider func(cmd.SimpleRunner) (*Release, error)
 func NewRegistry() *Registry {
 	return plumbing.NewProvider[cmd.SimpleRunner, *Release](ErrNotRecognized)
 }
+
+// RegisterDefaults registers the resolvers rig ships with, which is what
+// DefaultRegistry holds. Use it to build a registry of your own without having to
+// list them, and without missing resolvers added in later versions.
+//
+// The resolvers are appended, so a resolver of your own that has to take
+// precedence over one of them must be registered before this call, or with
+// Registry.RegisterFirst.
+func RegisterDefaults(provider *Registry) {
+	RegisterLinux(provider)
+	RegisterWindows(provider)
+	RegisterDarwin(provider)
+	// Registered last because it is the last resort, but it does not depend on
+	// that: it declines any host ResolveLinux can identify. See readOSRelease.
+	RegisterLinuxCompat(provider)
+}

os/defaultprovider_test.go

@@ -0,0 +1,215 @@
+package os
+
+import (
+	"testing"
+
+	"github.com/k0sproject/rig/v2/cmd"
+	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)
+	}
+}
+
+// TestRegisterDefaultsBuildsTheDefaultRegistry checks that a registry assembled
+// with RegisterDefaults answers like DefaultRegistry, since that is what lets a
+// caller build one of their own without listing the resolvers by hand.
+func TestRegisterDefaultsBuildsTheDefaultRegistry(t *testing.T) {
+	registry := NewRegistry()
+	RegisterDefaults(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("got ID %q version %q, want %q %q", rel.ID, rel.Version, "ubuntu", "22.04")
+	}
+
+	// The compat resolver came along with the rest and is still reached last.
+	rel, err = registry.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")
+	}
+}
+
+// myOSResolver resolves the hosts of a fleet that has no os-release file, which is
+// the case ResolveLinuxCompat also claims. It stands down for hosts os-release can
+// name so it does not shadow ResolveLinux in turn.
+func myOSResolver(conn cmd.SimpleRunner) (*Release, bool) {
+	if _, ok := readOSRelease(conn); ok {
+		return nil, false
+	}
+
+	return &Release{ID: "myos", Version: "1.0"}, true
+}
+
+// TestRegisterFirstOverridesTheCompatResolver covers what RegisterFirst is for.
+// ResolveLinuxCompat matches every Linux host os-release cannot name, so a
+// resolver a caller appends for such a host is never reached, and they cannot fix
+// that with self-exclusion because ResolveLinuxCompat is not theirs to change.
+func TestRegisterFirstOverridesTheCompatResolver(t *testing.T) {
+	appended := NewRegistry()
+	RegisterDefaults(appended)
+	appended.Register(myOSResolver)
+
+	rel, err := appended.Get(linuxRunner(""))
+	if err != nil {
+		t.Fatalf("resolving a Linux host with no os-release failed: %v", err)
+	}
+	if rel.ID != "linux" {
+		t.Errorf("appended resolver: got ID %q, want %q -- a resolver added with Register is expected to land behind the catch-all", rel.ID, "linux")
+	}
+
+	prepended := NewRegistry()
+	RegisterDefaults(prepended)
+	prepended.RegisterFirst(myOSResolver)
+
+	rel, err = prepended.Get(linuxRunner(""))
+	if err != nil {
+		t.Fatalf("resolving a Linux host with no os-release failed: %v", err)
+	}
+	if rel.ID != "myos" || rel.Version != "1.0" {
+		t.Errorf("got ID %q version %q, want %q %q -- the compat resolver answered ahead of one added with RegisterFirst",
+			rel.ID, rel.Version, "myos", "1.0")
+	}
+
+	// A host os-release can name is still resolved by ResolveLinux.
+	rel, err = prepended.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("got ID %q version %q, want %q %q", rel.ID, rel.Version, "ubuntu", "22.04")
+	}
+}