diff --git a/backend_darwin.go b/backend_darwin.go index f197ec8..abfd248 100644 --- a/backend_darwin.go +++ b/backend_darwin.go @@ -2,6 +2,8 @@ package dnsconfig import ( "context" + "errors" + "os" "github.com/DefinedNet/dnsconfig/internal/backend" "github.com/DefinedNet/dnsconfig/internal/backend/darwin" @@ -20,3 +22,17 @@ func newNativeBackend(ctx context.Context, kind BackendKind, opts Options) (back } return b, nil } + +// isRoot is replaced by tests so the declined path does not depend on the +// test runner's user. +var isRoot = func() bool { return os.Geteuid() == 0 } + +// nativeDeclined reports why this process cannot manage kind's DNS, or nil. +// The dynamic store, /etc/resolver, and the lock directory all require root, +// so a non-root darwin Manager could only fail and retry every attempt. +func nativeDeclined(kind BackendKind) error { + if kind == BackendDarwin && !isRoot() { + return errors.New("darwin: requires root") + } + return nil +} diff --git a/backend_other.go b/backend_other.go index 0f1ff66..3e1555c 100644 --- a/backend_other.go +++ b/backend_other.go @@ -11,3 +11,6 @@ import ( func newNativeBackend(_ context.Context, kind BackendKind, _ Options) (backend.Backend, error) { return nil, errBackendUnsupported(kind) } + +// nativeDeclined reports why this process cannot manage kind's DNS, or nil. +func nativeDeclined(BackendKind) error { return nil } diff --git a/backend_windows.go b/backend_windows.go index f60579c..012381f 100644 --- a/backend_windows.go +++ b/backend_windows.go @@ -22,3 +22,6 @@ func newNativeBackend(ctx context.Context, kind BackendKind, opts Options) (back } return b, nil } + +// nativeDeclined reports why this process cannot manage kind's DNS, or nil. +func nativeDeclined(BackendKind) error { return nil } diff --git a/declined_darwin_test.go b/declined_darwin_test.go new file mode 100644 index 0000000..5583635 --- /dev/null +++ b/declined_darwin_test.go @@ -0,0 +1,83 @@ +package dnsconfig + +import ( + "bytes" + "context" + "log/slog" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/DefinedNet/dnsconfig/internal/backend/noop" +) + +// withRoot answers the process root check for the duration of t. +func withRoot(t *testing.T, root bool) { + t.Helper() + prev := isRoot + isRoot = func() bool { return root } + t.Cleanup(func() { isRoot = prev }) +} + +// TestNew_DarwinWithoutRootDeclines checks that a non-root macOS process, +// which can write none of the store, /etc/resolver, or the lock directory, +// gets a declined Manager instead of one that retries failing attempts. +func TestNew_DarwinWithoutRootDeclines(t *testing.T) { + withRoot(t, false) + var logs bytes.Buffer + m, err := New(context.Background(), Options{ + Identity: "facade-declined", + ForceBackend: BackendDarwin, + Logger: slog.New(slog.NewTextHandler(&logs, &slog.HandlerOptions{Level: slog.LevelDebug})), + }) + require.NoError(t, err) + t.Cleanup(func() { _ = m.Close() }) + + s := m.Status() + require.ErrorIs(t, s.LastError, ErrConfigurationDeclined, "the refusal is visible from New on") + assert.ErrorContains(t, s.LastError, "requires root") + assert.False(t, s.LastConvergedCleared) + + assert.Equal(t, BackendDarwin, m.kind) + assert.Nil(t, m.locker, "a declined Manager takes no host locks") + assert.IsType(t, &noop.Backend{}, m.backend, "the darwin backend is never built") + + err = m.Set(sampleConfig(t)) + require.ErrorIs(t, err, ErrConfigurationDeclined) + assert.ErrorContains(t, err, "requires root") + var e *Error + require.ErrorAs(t, err, &e) + assert.Equal(t, BackendDarwin, e.Backend) + s = m.Status() + assert.Equal(t, GoalNone, s.Goal) + assert.Same(t, err, s.LastError) + + require.NoError(t, m.Clear()) + s = waitStatus(t, m, func(s Status) bool { return s.Converged }) + assert.Equal(t, GoalClear, s.Goal) + assert.True(t, s.LastChangedAt.IsZero(), "a declined Clear never changes the host") + assert.ErrorIs(t, s.LastError, ErrConfigurationDeclined, "a declined Clear verified nothing") + assert.False(t, s.LastConvergedCleared) + + require.NoError(t, m.Close()) + s = m.Status() + assert.ErrorIs(t, s.LastError, ErrConfigurationDeclined) + assert.ErrorContains(t, s.LastError, "requires root") + assert.False(t, s.LastConvergedCleared) + out := logs.String() + assert.Equal(t, 1, strings.Count(out, "level=INFO"), out) + assert.Contains(t, out, "DNS management declined") + assert.NotContains(t, out, "level=WARN") +} + +// TestNew_DarwinWithRootBuildsBackend pins that root keeps the live backend. +func TestNew_DarwinWithRootBuildsBackend(t *testing.T) { + withRoot(t, true) + m, err := New(context.Background(), Options{Identity: "facade-root", ForceBackend: BackendDarwin}) + require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, m.Close()) }) + assert.NotNil(t, m.locker) + assert.Nil(t, m.declined) +} diff --git a/doc.go b/doc.go index b868f9b..5b47b1f 100644 --- a/doc.go +++ b/doc.go @@ -10,9 +10,14 @@ // are expected to run as LocalSystem so Group Policy changes can be notified // directly; elevated Administrator operation falls back to a full policy // refresh; see "Convergence and repair". -// New logs a warning through Options.Logger when the macOS process is not root, -// or when the Windows process is not LocalSystem or its identity cannot be -// checked; construction still succeeds. +// On macOS, New declines DNS management for a process that is not root: it +// logs one Info line through Options.Logger, Set returns +// [ErrConfigurationDeclined], and Clear converges without touching the host. +// Status.LastError keeps that refusal for the Manager's life, so a converged +// Clear does not read as a verified clean host. +// On Windows, the first attempt that writes to the host logs a warning when +// the process is not LocalSystem or its identity cannot be checked; the +// attempt proceeds. // The OS tools the backends invoke are resolved by absolute path and never // through PATH. Unsupported platforms select [BackendNoop], which refuses // [Manager.Set] unless explicitly forced. diff --git a/errors.go b/errors.go index 000372f..75903a2 100644 --- a/errors.go +++ b/errors.go @@ -56,9 +56,15 @@ var ( // ErrConfigurationDeclined is returned by Set when the active backend // applies nothing and the caller did not ask for that. Detection answers - // BackendNoop on unsupported platforms. Set records no goal. Explicitly - // selecting BackendNoop through Options.ForceBackend permits a goal that - // converges without touching the host. + // BackendNoop on unsupported platforms, and New declines the macOS backend + // for a process that is not root, since every write it would attempt + // needs root. A declined macOS Manager names BackendDarwin in its errors, + // its refusal says why, and its Clear converges without touching the host or + // taking locks. Set records no goal. Status.LastError holds this error + // from New on and LastConvergedCleared stays false, since a converged + // Clear checked nothing. Explicitly selecting BackendNoop + // through Options.ForceBackend permits a goal that converges without + // touching the host. // // Retrying on the same Manager cannot change this condition. ErrConfigurationDeclined = errors.New("backend applies no DNS configuration") diff --git a/example_test.go b/example_test.go index ccf4f42..cf7de15 100644 --- a/example_test.go +++ b/example_test.go @@ -79,6 +79,9 @@ func ExampleManager_Clear_recovery() { for { s := m.Status() switch { + case errors.Is(s.LastError, dnsconfig.ErrConfigurationDeclined): + log.Printf("DNS management declined; nothing was checked: %v", s.LastError) + return case s.Converged && s.LastChangedAt.IsZero(): log.Print("no DNS configuration attributable to myapp was found") return diff --git a/internal/backend/darwin/conflict_test.go b/internal/backend/darwin/conflict_test.go index e30345b..2faea73 100644 --- a/internal/backend/darwin/conflict_test.go +++ b/internal/backend/darwin/conflict_test.go @@ -31,7 +31,7 @@ func (h *sharedHost) backend(identity string) *Backend { if b, ok := h.backends[identity]; ok { return b } - b, err := New(context.Background(), Options{Identity: identity, FS: h.fs, Runner: h.fake, Logger: discardLogger(), IsRoot: asRoot}) + b, err := New(context.Background(), Options{Identity: identity, FS: h.fs, Runner: h.fake, Logger: discardLogger()}) require.NoError(h.t, err) useFS(b, h.fs) h.t.Cleanup(func() { _ = b.Close() }) diff --git a/internal/backend/darwin/converge_test.go b/internal/backend/darwin/converge_test.go index f4d1efc..c812b7a 100644 --- a/internal/backend/darwin/converge_test.go +++ b/internal/backend/darwin/converge_test.go @@ -830,7 +830,6 @@ func TestConvergeLogsWhatChanged(t *testing.T) { FS: fs, Runner: fake, Logger: slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelInfo})), - IsRoot: asRoot, }) require.NoError(t, err) useFS(b, fs) diff --git a/internal/backend/darwin/darwin.go b/internal/backend/darwin/darwin.go index 72d15b1..27c7ddd 100644 --- a/internal/backend/darwin/darwin.go +++ b/internal/backend/darwin/darwin.go @@ -62,9 +62,6 @@ type Options struct { Runner runner.Runner // Logger receives non-fatal warnings. It is required. Logger *slog.Logger - // IsRoot reports whether the process runs as root, for the startup - // diagnostic. Nil checks the effective user ID. - IsRoot func() bool } type stateKeyConfig struct { @@ -121,13 +118,6 @@ func New(_ context.Context, opts Options) (*Backend, error) { if fs == nil { fs = backend.DiskFS{} } - isRoot := opts.IsRoot - if isRoot == nil { - isRoot = func() bool { return os.Geteuid() == 0 } - } - if !isRoot() { - opts.Logger.Warn("darwin: DNS configuration requires root for the dynamic store, /etc/resolver, and the lock directory; without it every Set and Clear attempt is refused before writing") - } r := opts.Runner if r == nil { r = runner.ExecRunner{} diff --git a/internal/backend/darwin/darwin_test.go b/internal/backend/darwin/darwin_test.go index 10b1f19..6c1d186 100644 --- a/internal/backend/darwin/darwin_test.go +++ b/internal/backend/darwin/darwin_test.go @@ -522,9 +522,6 @@ func discardLogger() *slog.Logger { return slog.New(slog.DiscardHandler) } -// asRoot keeps the startup diagnostic independent of the test runner's user. -func asRoot() bool { return true } - func newTestBackend(t *testing.T, fs dirFS, fake runner.Runner) *Backend { t.Helper() b, err := New(context.Background(), Options{ @@ -532,51 +529,12 @@ func newTestBackend(t *testing.T, fs dirFS, fake runner.Runner) *Backend { FS: fs, Runner: fake, Logger: discardLogger(), - IsRoot: asRoot, }) require.NoError(t, err) t.Cleanup(func() { _ = b.Close() }) return useFS(b, fs) } -func TestNewWarnsWithoutRoot(t *testing.T) { - for _, tc := range []struct { - name string - root bool - want string - }{ - {name: "root", root: true}, - {name: "not root", want: "requires root"}, - } { - t.Run(tc.name, func(t *testing.T) { - var logs bytes.Buffer - fs := newTestFS(t) - fake := newScutilFake() - calls := 0 - b, err := New(context.Background(), Options{ - Identity: testIdentity, - FS: fs, - Runner: fake, - Logger: slog.New(slog.NewTextHandler(&logs, nil)), - IsRoot: func() bool { - calls++ - return tc.root - }, - }) - require.NoError(t, err) - t.Cleanup(func() { _ = b.Close() }) - assert.Equal(t, 1, calls) - assertQuiet(t, observeHost(fs, fake), "construction must not change DNS") - if tc.want == "" { - assert.Empty(t, logs.String()) - return - } - assert.Equal(t, 1, strings.Count(logs.String(), "level=WARN")) - assert.Contains(t, logs.String(), tc.want) - }) - } -} - var ( newGoal = backend.Attempt{NewGoal: true} repeat = backend.Attempt{} @@ -990,7 +948,6 @@ func TestClearLeavesKeysItCannotAttribute(t *testing.T) { FS: fs, Runner: fr, Logger: discardLogger(), - IsRoot: asRoot, }) require.NoError(t, err) useFS(b, fs) diff --git a/internal/backend/windows/converge.go b/internal/backend/windows/converge.go index 4b06911..11aca43 100644 --- a/internal/backend/windows/converge.go +++ b/internal/backend/windows/converge.go @@ -34,6 +34,9 @@ type mutation struct { // notification for a write to the GP store. func (x *mutation) begin(base string) { + if !x.started { + x.b.warnIdentityOnce() + } x.started = true x.b.flushOwed = true if base == gpoNrptBase { diff --git a/internal/backend/windows/windows.go b/internal/backend/windows/windows.go index 58ad382..59f1ea2 100644 --- a/internal/backend/windows/windows.go +++ b/internal/backend/windows/windows.go @@ -225,7 +225,8 @@ type Options struct { // Logger receives structured diagnostics. It is required. Logger *slog.Logger - // IsLocalSystem checks the execution identity for the startup diagnostic. + // IsLocalSystem checks the execution identity for the diagnostic logged + // before the first registry write, including a reload trigger. // Nil uses the process token on Windows and skips the check elsewhere. IsLocalSystem func() (bool, error) @@ -278,6 +279,10 @@ type Backend struct { // The fields from here to mu are touched only by Set and Clear, which // the Manager never runs concurrently. + // checkIdentity is the LocalSystem check warnIdentityOnce runs, nil once + // it has run or where no check exists. + checkIdentity func() (bool, error) + // adapterTarget is the adapter whose NameServer and SearchList this // instance still owes cleanup: one it wrote to, or a recorded // predecessor's adapter whose clear failed. It survives external record @@ -319,15 +324,6 @@ func New(_ context.Context, opts Options) (*Backend, error) { if checkIdentity == nil { checkIdentity = newDefaultLocalSystemCheck() } - if checkIdentity != nil { - system, err := checkIdentity() - switch { - case err != nil: - logger.Warn("windows: could not verify execution identity; DNS configuration expects LocalSystem", "error", err) - case !system: - logger.Warn("windows: DNS configuration expects LocalSystem; running without it is best effort, and Group Policy DNS rules may remain effective after cleanup") - } - } r := opts.Runner if r == nil { r = runner.ExecRunner{} @@ -363,10 +359,29 @@ func New(_ context.Context, opts Options) (*Backend, error) { resolveInterface: resolve, notifier: notifier, domainJoined: domainJoined, + checkIdentity: checkIdentity, changes: make(chan struct{}, 1), }, nil } +// warnIdentityOnce logs the execution-identity diagnostic before the first +// registry write, from mutation.begin and reload. Every Windows process builds a backend, but only one that writes +// can leave Group Policy rules effective after cleanup. +func (b *Backend) warnIdentityOnce() { + check := b.checkIdentity + if check == nil { + return + } + b.checkIdentity = nil + system, err := check() + switch { + case err != nil: + b.logger.Warn("windows: could not verify execution identity; DNS configuration expects LocalSystem", "error", err) + case !system: + b.logger.Warn("windows: DNS configuration expects LocalSystem; running without it is best effort, and Group Policy DNS rules may remain effective after cleanup") + } +} + // Changes returns the coalesced change signal. It is closed by Close. func (b *Backend) Changes() <-chan struct{} { return b.changes } @@ -454,6 +469,9 @@ func (b *Backend) reload(ctx context.Context, mut *mutation) error { if err := ctx.Err(); err != nil { return err } + // A reload can be an attempt's only write, when a predecessor's artifacts + // already match. + b.warnIdentityOnce() if err := b.reg.SetString(nrptBase, reloadTriggerValue, "1"); err != nil { return fmt.Errorf("windows: trigger adapter reload: %w", err) } diff --git a/internal/backend/windows/windows_test.go b/internal/backend/windows/windows_test.go index 02293df..4d408a9 100644 --- a/internal/backend/windows/windows_test.go +++ b/internal/backend/windows/windows_test.go @@ -57,7 +57,10 @@ func discardLogger() *slog.Logger { return slog.New(slog.DiscardHandler) } -func TestExecutionIdentityWarningDoesNotAffectLifecycle(t *testing.T) { +// TestExecutionIdentityWarningAtFirstWrite checks that the LocalSystem +// diagnostic waits for the first write, so a process that only constructs a +// backend or clears an empty host logs nothing, and that it is logged once. +func TestExecutionIdentityWarningAtFirstWrite(t *testing.T) { for _, tc := range []struct { name string system bool @@ -83,30 +86,58 @@ func TestExecutionIdentityWarningDoesNotAffectLifecycle(t *testing.T) { }) require.NoError(t, err) t.Cleanup(func() { _ = b.Close() }) - assert.Empty(t, reg.writes, "construction must not change DNS") - if tc.want == "" { - assert.Empty(t, logs.String()) - } else { - assert.Contains(t, logs.String(), "level=WARN") - assert.Contains(t, logs.String(), tc.want) - if tc.err != nil { - assert.Contains(t, logs.String(), tc.err.Error()) - } - } - startup := logs.String() - logs.Reset() + require.NoError(t, b.clearGoal(ctx)) + assert.Empty(t, reg.writes, "an empty host gives Clear nothing to write") + assert.Zero(t, calls, "nothing written, nothing checked") + assert.NotContains(t, logs.String(), "LocalSystem") + require.NoError(t, b.setGoal(ctx, catchAll(t))) require.NoError(t, b.clearGoal(ctx)) + require.NoError(t, b.setGoal(ctx, catchAll(t))) require.NoError(t, b.Close()) assert.Equal(t, 1, calls) - assert.NotContains(t, logs.String(), "expects LocalSystem") - if tc.want != "" { - assert.Equal(t, 1, strings.Count(startup, "level=WARN")) + if tc.want == "" { + assert.NotContains(t, logs.String(), "level=WARN") + return + } + assert.Equal(t, 1, strings.Count(logs.String(), "level=WARN"), logs.String()) + assert.Contains(t, logs.String(), tc.want) + if tc.err != nil { + assert.Contains(t, logs.String(), tc.err.Error()) } }) } } +// TestExecutionIdentityWarningOnReloadOnlySet covers a restart whose +// predecessor's artifacts already match, so the reload trigger is the only +// write and must still be preceded by the diagnostic. +func TestExecutionIdentityWarningOnReloadOnlySet(t *testing.T) { + h := newHarness(t) + ctx := context.Background() + require.NoError(t, h.setGoal(ctx, catchAll(t))) + require.NoError(t, h.Close()) + + var logs bytes.Buffer + b, err := newTestBackend(ctx, Options{ + Registry: h.reg, + Logger: slog.New(slog.NewTextHandler(&logs, nil)), + IsLocalSystem: func() (bool, error) { return false, nil }, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = b.Close() }) + h.reg.writes = nil + res, err := b.Set(ctx, catchAll(t), backend.Attempt{NewGoal: true}) + require.NoError(t, err) + assert.Equal(t, backend.Result{}, res, "matching values need no write") + assert.Equal(t, []string{ + "SetString " + nrptBase + " " + reloadTriggerValue, + "DeleteValue " + nrptBase + " " + reloadTriggerValue, + }, h.reg.writes, "the only writes are one reload trigger") + assert.Equal(t, 1, strings.Count(logs.String(), "level=WARN"), logs.String()) + assert.Contains(t, logs.String(), "running without it is best effort") +} + func TestNewRequiresLogger(t *testing.T) { _, err := New(context.Background(), Options{Identity: testIdentity, Registry: newFakeRegistry()}) require.Error(t, err) diff --git a/manager.go b/manager.go index 81a2947..abb0bbc 100644 --- a/manager.go +++ b/manager.go @@ -74,11 +74,13 @@ type Manager struct { logger *slog.Logger kind BackendKind backend backend.Backend - // declined distinguishes unsupported hosts from an explicitly forced noop. - declined bool + // declined, when non-nil, is the ErrConfigurationDeclined refusal for a + // host this process cannot manage. It distinguishes those hosts from an + // explicitly forced noop. + declined error // locker issues the per-identity lifetime lock and the host lock. It is - // nil for noop, which never touches the host. + // nil for noop and a declined Manager, which never touch the host. locker hostlock.Locker // lifetime is held from the first attempt at a goal until a Clear // converges or Close finishes. Only the worker touches it while running; @@ -163,7 +165,9 @@ type attemptJob struct { // Invalid Options are returned as-is. Construction failures are wrapped in // *Error, with ErrInvalidOptions for a backend absent on this OS. No backend // touches the host at construction or needs Options.InterfaceName; Windows -// checks it at Set. +// checks it at Set. A host this process cannot manage, such as macOS without +// root, gets a declined Manager instead of an error; see +// [ErrConfigurationDeclined]. func New(ctx context.Context, opts Options) (*Manager, error) { if err := opts.Validate(); err != nil { return nil, err @@ -177,6 +181,18 @@ func New(ctx context.Context, opts Options) (*Manager, error) { kind = Detect() } + // A host this process cannot write gets the no-op backend and no locks, + // so Clear converges without attempts that could only fail and retry. + if reason := nativeDeclined(kind); reason != nil { + opts.Logger.Info("dnsconfig: DNS management declined", + slog.String("backend", string(kind)), slog.Any("reason", reason)) + b, _ := newBackend(ctx, BackendNoop, opts) + m := newWithBackend(opts, kind, b, nil) + m.declined = wrapSentinel(ErrConfigurationDeclined, reason) + m.start() + return m, nil + } + b, err := newBackend(ctx, kind, opts) if err != nil { return nil, &Error{Op: "New", Backend: kind, Err: err} @@ -199,12 +215,16 @@ func newWithBackend(opts Options, kind BackendKind, b backend.Backend, locker ho if opts.ShutdownTimeout == 0 { opts.ShutdownTimeout = DefaultShutdownTimeout } + var declined error + if kind == BackendNoop && opts.ForceBackend != BackendNoop { + declined = ErrConfigurationDeclined + } return &Manager{ opts: opts, logger: opts.Logger, kind: kind, backend: b, - declined: kind == BackendNoop && opts.ForceBackend != BackendNoop, + declined: declined, locker: locker, clock: realClock{}, hostLockTimeout: hostLockAcquireTimeout, @@ -237,7 +257,8 @@ func wrapSentinel(sentinel, err error) error { // alone, are returned directly // without an [Error] wrapper. The other refusals record no goal: // [ErrClosed] after Close, [ErrConfigurationDeclined] when -// detection selected the no-op backend, and [ErrInterfaceRequired] on Windows +// detection selected the no-op backend or the process lacks the privilege +// the backend needs, and [ErrInterfaceRequired] on Windows // without Options.InterfaceName. The last two are also recorded in // Status.LastError. // @@ -259,8 +280,8 @@ func (m *Manager) Set(cfg Config) error { if m.st.closed { return &Error{Op: "Set", Backend: m.kind, Err: ErrClosed} } - if m.declined { - return m.refuseSetLocked(ErrConfigurationDeclined) + if m.declined != nil { + return m.refuseSetLocked(m.declined) } // Recovery records identify the adapter for Clear, but a Set has no // adapter to write without the caller naming one. @@ -298,7 +319,9 @@ func (m *Manager) refuseSetLocked(cause error) error { // recovery does not need InterfaceName or a saved snapshot, and clears that // adapter's DNS values rather than restoring older ones. Discovering nothing // attributable is a successful Clear, so a setting whose ownership evidence -// was lost stays in place. +// was lost stays in place. A Manager that declined DNS management converges +// a Clear without touching the host; Status keeps [ErrConfigurationDeclined] +// in LastError and LastConvergedCleared false. // // Clear returns [ErrClosed] after Close and otherwise nil. Calling it while // Clear is already the goal is a no-op. @@ -812,6 +835,14 @@ func (m *Manager) Status() Status { LastChangedAt: m.st.lastChangedAt, LastConvergedCleared: m.st.lastConvergedCleared, } + // A declined Manager verifies nothing, so a converged Clear must not read + // as a clean host; the refusal stays visible for the Manager's life. + if m.declined != nil { + if s.LastError == nil { + s.LastError = &Error{Op: "New", Backend: m.kind, Err: m.declined} + } + s.LastConvergedCleared = false + } if m.st.goal == GoalSet { s.Desired = cloneConfig(m.st.desired) } diff --git a/manager_test.go b/manager_test.go index bf3e06c..983a705 100644 --- a/manager_test.go +++ b/manager_test.go @@ -70,6 +70,50 @@ func TestSet_ForcedNoopConverges(t *testing.T) { require.NoError(t, m.Close()) } +// TestStatus_DeclinedKeepsRefusal pins that a Manager whose backend applies +// nothing, unless the caller forced BackendNoop, never reports a verified +// clean host: its Clear converges, but LastError keeps the refusal and +// LastConvergedCleared stays false through Close. +func TestStatus_DeclinedKeepsRefusal(t *testing.T) { + for _, tc := range []struct { + name string + opts Options + declined bool + }{ + {name: "detected noop", declined: true}, + {name: "forced noop", opts: Options{ForceBackend: BackendNoop}}, + } { + t.Run(tc.name, func(t *testing.T) { + h := newHarness(t, BackendNoop, tc.opts) + check := func(when string) { + t.Helper() + s := h.m.Status() + if tc.declined { + assert.ErrorIs(t, s.LastError, ErrConfigurationDeclined, when) + assert.False(t, s.LastConvergedCleared, when) + } else { + assert.NoError(t, s.LastError, when) + } + } + check("after New") + var e *Error + if tc.declined { + require.ErrorAs(t, h.m.Status().LastError, &e) + assert.Equal(t, "New", e.Op) + } + + require.NoError(t, h.m.Clear()) + h.converged("clear") + assert.True(t, h.m.Status().Converged, "callers waiting for convergence still see it") + check("after Clear") + assert.Equal(t, !tc.declined, h.m.Status().LastConvergedCleared) + + h.closeAnswering() + check("after Close") + }) + } +} + func TestSet_ValidationReturnsDirectly(t *testing.T) { for _, tc := range []struct { name string diff --git a/native_test.go b/native_test.go index df96c54..cd552b0 100644 --- a/native_test.go +++ b/native_test.go @@ -16,7 +16,8 @@ func TestNewConstructsNativeBackend(t *testing.T) { var kind BackendKind switch runtime.GOOS { case "darwin": - kind = BackendDarwin + // A non-root test user would get a declined Manager here. + t.Skip("covered by TestNew_DarwinWithRootBuildsBackend, which forces the root check") case "windows": kind = BackendWindows default: diff --git a/options.go b/options.go index 3bebd0c..0bdbc2c 100644 --- a/options.go +++ b/options.go @@ -63,7 +63,10 @@ type Options struct { // ForceBackend selects a specific backend; the zero value requests // detection. New never falls back to a different backend: an undeclared // kind fails Validate, and a kind with no implementation on this OS fails - // New with ErrInvalidOptions. See [BackendNoop] for forcing the no-op one. + // New with ErrInvalidOptions. The exception is a declined host, such as + // BackendDarwin in a process that is not root, which gets a Manager that + // refuses Set with [ErrConfigurationDeclined]. See [BackendNoop] for + // forcing the no-op one. ForceBackend BackendKind // InterfaceName is the friendly name of a Windows adapter, such as a diff --git a/searchlimit_darwin_test.go b/searchlimit_darwin_test.go index f5516be..3ab9959 100644 --- a/searchlimit_darwin_test.go +++ b/searchlimit_darwin_test.go @@ -16,6 +16,7 @@ import ( // backend refuses a search list that does not fit one scutil line before a // goal is recorded, so nothing is applied and Close has nothing to revert. func TestSetRejectsSearchListOverScutilLine(t *testing.T) { + withRoot(t, true) m, err := New(context.Background(), Options{Identity: "facade-search-limit", ForceBackend: BackendDarwin}) require.NoError(t, err) t.Cleanup(func() { require.NoError(t, m.Close()) }) diff --git a/status.go b/status.go index 966d5a4..2d50677 100644 --- a/status.go +++ b/status.go @@ -31,7 +31,8 @@ type Status struct { // Converged reports that the most recent attempt at the current goal // passed its artifact checks and completed every owed side-effect - // request, and no newer goal has been recorded since. Later drift is + // request, and no newer goal has been recorded since. A declined + // Manager checks nothing; see LastError. Later drift is // found by a change signal or periodic check. Close's final cleanup, // when it runs, counts as an attempt: afterwards Converged is false for // a Set goal and reports whether that cleanup succeeded for a Clear goal. @@ -41,8 +42,11 @@ type Status struct { // Op "Converge", or of Close, or a Set refusal that Set also returned. // After a Close that left a live holder's configuration in place it // includes [ErrManagerAlreadyRunning] with Op "Close", which Close - // itself does not return. It is nil after a successful attempt; - // errors.Is and errors.As reach the cause. + // itself does not return. It is nil after a successful attempt, except + // on a Manager that declined DNS management: from New on it holds + // [ErrConfigurationDeclined], as Op "New" or a Set refusal, because + // nothing on the host was checked. errors.Is and errors.As reach the + // cause. // Before Close the worker retries a failed attempt with backoff; Set // refusals and Close's failure are not retried. LastError error @@ -55,6 +59,7 @@ type Status struct { // LastConvergedCleared reports that the most recent successful attempt, // including Close's final cleanup, was a Clear. It is false before any - // attempt succeeds. + // attempt succeeds and always false on a Manager that declined DNS + // management, which removes nothing. LastConvergedCleared bool }