From f2a88835631873cad1aab321365b64887a9addde Mon Sep 17 00:00:00 2001 From: Matt Richardson Date: Mon, 19 Oct 2026 16:25:54 -0400 Subject: [PATCH] Drop the info API tests that tested the control socket's code twice The endpoint is one type under both servers, so the info API's copies of the rebind, failed-rebind and cancellation tests ran the control socket's code a second time, and in-place access changes were pinned twice more. - info API: the three endpoint tests, the mode test and the wire test's mode step go - the control socket's group hand-over test stands for both servers - registry: one Resolve test with both the typed and the worded assertions, in registry_test Co-Authored-By: Claude Fable 5.1 --- commands/registry_test.go | 19 +++++--- commands/request_test.go | 16 ------- localapi/access_unix_test.go | 29 +----------- localapi/info_test.go | 85 +----------------------------------- 4 files changed, 15 insertions(+), 134 deletions(-) diff --git a/commands/registry_test.go b/commands/registry_test.go index a0df728a..3692d9b9 100644 --- a/commands/registry_test.go +++ b/commands/registry_test.go @@ -87,19 +87,24 @@ func TestWithheldIsNotReportedAsUnknown(t *testing.T) { assert.Contains(t, out.String(), "did not understand: no-such-command") } -// TestResolveTypesTheWithheldRefusal: a surface with status codes tells a withheld -// command from an unknown one by the error's type, never by asking the unfiltered -// registry, which is the one thing a filtered surface must not consult. -func TestResolveTypesTheWithheldRefusal(t *testing.T) { +// TestResolveTellsUnknownFromWithheld: the two cases read differently to an operator, +// and a surface with status codes tells them apart by the error's type, never by asking +// the unfiltered registry, which is the one thing a filtered surface must not consult. +func TestResolveTellsUnknownFromWithheld(t *testing.T) { r := testRegistry().Filter(notWithheld, testReason) - _, err := r.Resolve("withheld") + c, err := r.Resolve("served") + require.NoError(t, err) + assert.Equal(t, Name("served"), c.Name) + + _, err = r.Resolve("withheld") var withheld *WithheldError require.ErrorAs(t, err, &withheld) - assert.Contains(t, err.Error(), testReason) + require.ErrorContains(t, err, "withheld is not available on this surface") + require.ErrorContains(t, err, testReason) _, err = r.Resolve("no-such-command") - require.Error(t, err) + require.ErrorContains(t, err, `unknown command "no-such-command"`) assert.NotErrorAs(t, err, &withheld, "an unknown name is not a withheld one") } diff --git a/commands/request_test.go b/commands/request_test.go index fd5becc6..9980a707 100644 --- a/commands/request_test.go +++ b/commands/request_test.go @@ -68,22 +68,6 @@ func TestParseLineCarriesWhatWasSetInParseableForm(t *testing.T) { assert.Equal(t, paramsSeen{Verbose: true, Wait: 2 * time.Second}, (*seen)[0]) } -// TestResolveTellsUnknownFromWithheld: the two cases read differently to an operator, -// and Resolve is the edge that decides which one a name from the wire is. -func TestResolveTellsUnknownFromWithheld(t *testing.T) { - r := testRegistry().Filter(notWithheld, testReason) - - c, err := r.Resolve("served") - require.NoError(t, err) - assert.Equal(t, Name("served"), c.Name) - - _, err = r.Resolve("withheld") - require.ErrorContains(t, err, "withheld is not available on this surface") - - _, err = r.Resolve("no-such-command") - require.ErrorContains(t, err, `unknown command "no-such-command"`) -} - // TestLineRunsTheFlagSetItParsed: the ssh path hands Run the FlagSet the line was parsed // into, with no trip through a Request. A flag.Func has no String worth re-parsing, so a // round trip would hand Run an empty value and no error; the line's value must arrive. diff --git a/localapi/access_unix_test.go b/localapi/access_unix_test.go index 98a8aa62..0467691c 100644 --- a/localapi/access_unix_test.go +++ b/localapi/access_unix_test.go @@ -4,7 +4,6 @@ package localapi import ( "context" - "net/http" "os" "os/user" "strconv" @@ -86,7 +85,7 @@ func TestControlAccessFromConfig(t *testing.T) { // TestControlSocketIsHandedToTheGroup: the socket is created with the group's gid and // 0660, whatever the umask, and a reload that takes the group away tightens the socket -// in place, without moving it. +// in place, without moving it. The endpoint is shared, so this covers the info API too. func TestControlSocketIsHandedToTheGroup(t *testing.T) { old := unix.Umask(0077) defer unix.Umask(old) @@ -177,32 +176,6 @@ func TestInfoAccessFromConfig(t *testing.T) { assert.Nil(t, cfg.Access.Group) } -// TestSocketIsHandedToTheGroup: the socket is created with the group's gid and the -// mode, and a changed group or mode on a reload is applied to the socket in place. -func TestSocketIsHandedToTheGroup(t *testing.T) { - name, gid := ownGroup(t) - group := &Group{Name: name, ID: gid} - want, err := strconv.ParseUint(gid, 10, 32) - require.NoError(t, err) - - s, path := serveInfo(t, infoRegistry(), 0600) - require.NoError(t, s.Rebind(context.Background(), path, Access{Mode: 0660, Group: group})) - fi, err := os.Lstat(path) - require.NoError(t, err) - assert.Equal(t, os.FileMode(0660), fi.Mode().Perm()) - assert.Equal(t, uint32(want), socketGroup(t, path)) - - status, _ := test.HTTPGet(t, path, "/v1/whoami") - assert.Equal(t, http.StatusOK, status, "the same listener is still serving") - - // The mode is the operator's to widen further. - require.NoError(t, s.Rebind(context.Background(), path, Access{Mode: 0664, Group: group})) - fi, err = os.Lstat(path) - require.NoError(t, err) - assert.Equal(t, os.FileMode(0664), fi.Mode().Perm()) - assert.Equal(t, path, s.Path()) -} - // TestRemovingTheGroupHandsTheSocketBack: a reload that drops the group key leaves the // mode the operator set but returns the socket to the daemon's own group, so the old // group's members are no longer admitted. The socket is first handed to a group this diff --git a/localapi/info_test.go b/localapi/info_test.go index 8e160b1b..a9f12981 100644 --- a/localapi/info_test.go +++ b/localapi/info_test.go @@ -23,7 +23,6 @@ import ( "github.com/slackhq/nebula/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "golang.org/x/sys/unix" ) type whoamiFlags struct { @@ -218,28 +217,6 @@ func TestInfoPresentationFlags(t *testing.T) { assert.Equal(t, compact, off) } -// TestInfoSocketMode: the mode is the operator's, pinned regardless of the umask, and a -// changed mode on a reload is applied in place rather than by rebinding. -func TestInfoSocketMode(t *testing.T) { - old := unix.Umask(0077) - defer unix.Umask(old) - - s, path := serveInfo(t, infoRegistry(), 0660) - - fi, err := os.Stat(path) - require.NoError(t, err) - assert.Equal(t, os.FileMode(0660), fi.Mode().Perm()) - - require.NoError(t, s.Rebind(context.Background(), path, Access{Mode: 0640})) - fi, err = os.Stat(path) - require.NoError(t, err) - assert.Equal(t, os.FileMode(0640), fi.Mode().Perm()) - assert.Equal(t, path, s.Path()) - - status, _ := test.HTTPGet(t, path, "/v1/whoami") - assert.Equal(t, http.StatusOK, status, "the same listener is still serving") -} - // TestInfoModeChangeRefusesASwappedSocket: whoever can write the directory can replace // the node with a link to a file of their choosing; the daemon must not set the // operator's mode on that file. The node is seen to be gone, the link is refused by the @@ -261,54 +238,6 @@ func TestInfoModeChangeRefusesASwappedSocket(t *testing.T) { assert.Empty(t, s.Path(), "nothing of ours answers at the path, so nothing is served") } -func TestInfoRebindMovesAndStopRemoves(t *testing.T) { - s, first := serveInfo(t, infoRegistry(), 0600) - - second := testSocket(t) - require.NoError(t, s.Rebind(context.Background(), second, Access{Mode: 0600})) - assert.Equal(t, second, s.Path()) - _, err := os.Stat(first) - assert.True(t, os.IsNotExist(err), "the old socket is removed") - - status, body := test.HTTPGet(t, second, "/v1/whoami?name=alice") - assert.Equal(t, http.StatusOK, status) - assert.JSONEq(t, `{"name":"alice"}`, body) - - s.Stop() - assert.Empty(t, s.Path()) - _, err = os.Stat(second) - assert.True(t, os.IsNotExist(err), "Stop removes the socket") -} - -func TestInfoRebindFailureKeepsTheOldSocket(t *testing.T) { - s, first := serveInfo(t, infoRegistry(), 0600) - - err := s.Rebind(context.Background(), filepath.Join(privateBase(t), "missing", "n", "s.sock"), Access{Mode: 0600}) - require.Error(t, err) - assert.Equal(t, first, s.Path()) - - status, _ := test.HTTPGet(t, first, "/v1/whoami") - assert.Equal(t, http.StatusOK, status) -} - -func TestInfoContextCancellationStops(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - path := testSocket(t) - s := NewInfoServer(test.NewLogger(), infoRegistry()) - require.NoError(t, s.Rebind(ctx, path, Access{Mode: 0600})) - t.Cleanup(s.Stop) - - cancel() - require.Eventually(t, func() bool { return s.Path() == "" }, time.Second, 10*time.Millisecond) - _, err := os.Stat(path) - assert.True(t, os.IsNotExist(err)) - - // And a reload that lands during shutdown must not bring it back. - err = s.Rebind(ctx, path, Access{Mode: 0600}) - require.Error(t, err) - assert.ErrorContains(t, err, "shutting down") -} - func TestInfoConfig(t *testing.T) { load := func(t *testing.T, yaml string) (infoConfig, error) { t.Helper() @@ -365,11 +294,8 @@ func TestWireInfoFollowsConfig(t *testing.T) { ctlPath := testSocket(t) infoPath := testSocket(t) - sectionWithMode := func(enabled bool, socket, mode string) string { - return fmt.Sprintf("local_api:\n control:\n socket: %s\n info:\n enabled: %t\n socket: %s\n socket_mode: \"%s\"\n", ctlPath, enabled, socket, mode) - } section := func(enabled bool, socket string) string { - return sectionWithMode(enabled, socket, "0600") + return fmt.Sprintf("local_api:\n control:\n socket: %s\n info:\n enabled: %t\n socket: %s\n", ctlPath, enabled, socket) } c := config.NewC(test.NewLogger()) @@ -391,18 +317,11 @@ func TestWireInfoFollowsConfig(t *testing.T) { require.NoError(t, c.ReloadConfigString(section(true, infoPath)).Err) assert.Equal(t, infoPath, eps.Info.Path()) - // A reload that changes only the mode applies it in place. - require.NoError(t, c.ReloadConfigString(sectionWithMode(true, infoPath, "0660")).Err) - assert.Equal(t, infoPath, eps.Info.Path()) - fi, err := os.Stat(infoPath) - require.NoError(t, err) - assert.Equal(t, os.FileMode(0660), fi.Mode().Perm()) - // A reload that moves the socket moves it, and removes the old one. moved := testSocket(t) require.NoError(t, c.ReloadConfigString(section(true, moved)).Err) assert.Equal(t, moved, eps.Info.Path()) - _, err = os.Stat(infoPath) + _, err := os.Stat(infoPath) assert.True(t, os.IsNotExist(err)) status, body = test.HTTPGet(t, moved, "/v1/whoami?name=bob") assert.Equal(t, http.StatusOK, status)