fix(plumbing): stop fallback factories overtaking specific ones

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)
+		// ResolveLinuxCompat accepts any Linux host, so it must never be able to
+		// answer ahead of ResolveLinux. See RegisterFallback.
+		provider.RegisterFallback(ResolveLinuxCompat)
 		return provider
 	})
 

os/defaultprovider_test.go

@@ -0,0 +1,103 @@
+package os
+
+import (
+	"testing"
+
+	ps "github.com/k0sproject/rig/v2/powershell"
+	"github.com/k0sproject/rig/v2/rigtest"
+)
+
+// linuxRunner returns a runner that answers every probe the Linux resolvers
+// make. Matchers are first-match-wins, so the specific ones are registered
+// before the general ones. 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("cat /etc/os-release || cat /usr/lib/os-release"), errCommandFailed)
+	} else {
+		mr.AddCommandOutput(rigtest.Equal("cat /etc/os-release || cat /usr/lib/os-release"), 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
+}
+
+const ubuntuOSRelease = "PRETTY_NAME=\"Ubuntu 22.04.5 LTS\"\nNAME=\"Ubuntu\"\nVERSION_ID=\"22.04\"\nID=ubuntu\nID_LIKE=debian\n"
+
+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 guards the interaction between Get's
+// promotion of a winning factory and ResolveLinuxCompat, which accepts any Linux
+// host.
+//
+// Resolving a Windows host promotes ResolveWindows to the front of the list. If
+// ResolveLinuxCompat were an ordinary factory, that swap would displace
+// ResolveLinux and leave the compat resolver ahead of it, so the next Linux host
+// would be reported as ID "linux" with no version. Registering the compat
+// resolver as a fallback keeps it behind every specific resolver.
+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")
+	}
+}
+
+// TestDefaultRegistryStillFallsBackToCompat confirms the fallback 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)
+	}
+}

os/linux_compat.go

@@ -82,5 +82,5 @@ func ResolveLinuxCompat(conn cmd.SimpleRunner) (*Release, bool) {
 // It should be registered after ResolveLinux so it only activates when the
 // standard os-release files are absent.
 func RegisterLinuxCompat(provider *Registry) {
-	provider.Register(ResolveLinuxCompat)
+	provider.RegisterFallback(ResolveLinuxCompat)
 }

plumbing/provider.go

@@ -9,6 +9,7 @@ type Factory[R any, T any] func(R) (T, bool)
 type Provider[R any, T any] struct {
 	mu        sync.RWMutex
 	factories []Factory[R, T]
+	fallbacks []Factory[R, T]
 	err       error
 }
 
@@ -19,10 +20,26 @@ func (p *Provider[R, T]) Register(f Factory[R, T]) {
 	p.factories = append(p.factories, f)
 }
 
+// RegisterFallback adds a factory that is only consulted once every factory
+// registered with Register has declined.
+//
+// Use this for factories that match a superset of another factory's hosts, such
+// as a last-resort resolver that accepts anything of a given family. Get moves
+// a winning factory to the front of the list, so a superset factory registered
+// with Register can overtake the more specific one it was meant to back up and
+// then answer in its place. Fallbacks are never reordered and never promoted,
+// so registration order between them is preserved.
+func (p *Provider[R, T]) RegisterFallback(f Factory[R, T]) {
+	p.mu.Lock()
+	defer p.mu.Unlock()
+	p.fallbacks = append(p.fallbacks, 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.
+// future lookups. Factories added with RegisterFallback are tried last, in
+// registration order, and are never reordered.
 func (p *Provider[R, T]) Get(r R) (T, error) {
 	p.mu.Lock()
 	defer p.mu.Unlock()
@@ -38,24 +55,36 @@ func (p *Provider[R, T]) Get(r R) (T, error) {
 			return t, nil
 		}
 	}
+	for _, f := range p.fallbacks {
+		if t, ok := f(r); ok {
+			return t, nil
+		}
+	}
+
 	return *new(T), p.err
 }
 
-// GetAll retrieves all values of type T from the Factories in the Provider.
+// GetAll retrieves all values of type T from the Factories in the Provider,
+// followed by any registered with RegisterFallback.
 // 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) {
 	p.mu.RLock()
 	defer p.mu.RUnlock()
 	var ts []T
 	for _, f := range p.factories {
-		t, ok := f(r)
-		if ok {
+		if t, ok := f(r); ok {
+			ts = append(ts, t)
+		}
+	}
+	for _, f := range p.fallbacks {
+		if t, ok := f(r); ok {
 			ts = append(ts, t)
 		}
 	}
 	if len(ts) == 0 {
 		return nil, p.err
 	}
+
 	return ts, nil
 }
 

plumbing/provider_test.go

@@ -64,3 +64,70 @@ func TestGetAllNoFactory(t *testing.T) {
 	require.Error(t, err)
 	assert.Nil(t, values)
 }
+
+// TestRegisterFallbackIsNeverPromoted guards the ordering guarantee that
+// RegisterFallback exists to provide.
+//
+// Get promotes a winning factory to the front of the list. A factory that
+// matches a superset of another's inputs must therefore not be registered with
+// Register, or it can overtake the specific factory it was meant to back up and
+// answer in its place from then on. Fallbacks are consulted only after every
+// Register'ed factory has declined, and are never reordered.
+func TestRegisterFallbackIsNeverPromoted(t *testing.T) {
+	p := plumbing.NewProvider[string, string](errors.New("no factory available"))
+
+	// Specific: only handles "linux".
+	p.Register(func(in string) (string, bool) {
+		if in == "linux" {
+			return "specific", true
+		}
+
+		return "", false
+	})
+	// Superset: accepts anything. Registered here, between the two specific
+	// factories, because that is the position that breaks if it participates in
+	// promotion: resolving "windows" swaps the windows factory into index 0 and
+	// pushes the linux factory behind this one.
+	p.RegisterFallback(func(_ string) (string, bool) {
+		return "fallback", true
+	})
+	// Specific: only handles "windows".
+	p.Register(func(in string) (string, bool) {
+		if in == "windows" {
+			return "windows", true
+		}
+
+		return "", false
+	})
+
+	got, err := p.Get("linux")
+	require.NoError(t, err)
+	assert.Equal(t, "specific", got, "fallback must not answer while a specific factory matches")
+
+	got, err = p.Get("windows")
+	require.NoError(t, err)
+	assert.Equal(t, "windows", got)
+
+	// The windows factory has now been promoted to index 0. The fallback must
+	// still not have moved ahead of the linux factory.
+	got, err = p.Get("linux")
+	require.NoError(t, err)
+	assert.Equal(t, "specific", got, "fallback overtook a specific factory after promotion reordered the list")
+
+	// And it is still reached when nothing specific matches.
+	got, err = p.Get("darwin")
+	require.NoError(t, err)
+	assert.Equal(t, "fallback", got)
+}
+
+// TestGetAllIncludesFallbacks confirms fallbacks participate in GetAll, after
+// the ordinary factories.
+func TestGetAllIncludesFallbacks(t *testing.T) {
+	p := plumbing.NewProvider[string, string](errors.New("no factory available"))
+	p.Register(func(_ string) (string, bool) { return "first", true })
+	p.RegisterFallback(func(_ string) (string, bool) { return "last", true })
+
+	got, err := p.GetAll("anything")
+	require.NoError(t, err)
+	assert.Equal(t, []string{"first", "last"}, got)
+}