From 5b0ccce86793bf79c6ee8de04ec180984aa3b0c2 Mon Sep 17 00:00:00 2001 From: mattthew Date: Thu, 27 Aug 2026 15:46:47 -0400 Subject: [PATCH] fix(dgraphtest): give Zero the admin whitelist the Alpha already gets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Zero guards /moveTablet and /removeNode with adminAuthHandler(strict=true), so they are protected whether or not --security is configured: with neither a token nor a whitelist, only a loopback caller is admitted. dgraphtest passes --security to the Alpha and never to Zero, so a test process reaching Zero through a published container port cannot call those endpoints at all. TestUniqueMultipleGroups is the visible casualty. It fails on move tablet failed with status 401: ... Request is not from a whitelisted IP against an assertion about tablet placement, so the reported failure looks nothing like the cause. Two things hid it. The test only reaches MoveTablet when tablet placement happens to put email_group_1 and email_group_2 in the same group, which is not deterministic. And whether the guard fires at all depends on the host's Docker networking: on Linux a published port arrives from the bridge gateway and the request is refused, while on Docker Desktop it can arrive as loopback and be admitted. So the same commit passes CI on one runner and fails on another, and passes on a developer's Mac either way. The fix is the flag the Alpha has always had, with the token threaded through for the same reason: Zero authorizes an admin request on the token OR the whitelist, so a cluster where only the Alpha knows the token is incoherent rather than merely stricter. Pre-v21 is deliberately excluded. Those binaries predate both the --security superflag and the admin guard, so passing it there would fail an upgrade test with an unknown-flag error while fixing nothing. The Alpha's command makes the same split. Tested on the command rather than end to end, on purpose. An integration test that calls MoveTablet passes on Docker Desktop no matter what the harness passes, because loopback is admitted unconditionally — I wrote that test first and it passed without this fix, which makes it worse than no test. adminAuthHandler's own behaviour is already covered by the unit tests in dgraph/cmd/zero, including the loopback and remote cases; what was untested is the harness configuring Zero. Two of the three new tests fail without this change. Co-Authored-By: Claude Opus 5 (1M context) --- dgraphtest/dgraph.go | 15 +++++++++ dgraphtest/dgraph_test.go | 66 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+) create mode 100644 dgraphtest/dgraph_test.go diff --git a/dgraphtest/dgraph.go b/dgraphtest/dgraph.go index e49cf4582c3..ee2a704d9ca 100644 --- a/dgraphtest/dgraph.go +++ b/dgraphtest/dgraph.go @@ -142,6 +142,21 @@ func (z *zero) cmd(c *LocalCluster) []string { if c.lowerThanV21 { zcmd = append(zcmd, fmt.Sprintf(`--idx=%v`, z.id+1), "--telemetry=false") } else { + // Zero's destructive admin endpoints (/moveTablet, /removeNode) are guarded + // unconditionally: with neither a token nor a whitelist configured, only a + // loopback caller is admitted. A test reaching Zero through a published + // container port is not loopback, so without this the harness cannot exercise + // those endpoints at all — see adminAuthHandler in dgraph/cmd/zero/admin.go. + // + // Same value the Alpha already gets, for the same reason: the harness is not + // the place to enforce network policy, and a test cluster that refuses its own + // control plane fails in a way that looks nothing like its cause. + security := `--security=whitelist=0.0.0.0/0` + if c.conf.securityToken != "" { + security += fmt.Sprintf(`;token=%s`, c.conf.securityToken) + } + zcmd = append(zcmd, security) + zcmd = append(zcmd, fmt.Sprintf(`--raft=idx=%v`, z.id+1), "--telemetry=reports=false;", fmt.Sprintf(`--limit=refill-interval=%v;uid-lease=%v`, c.conf.refillInterval, c.conf.uidLease)) } diff --git a/dgraphtest/dgraph_test.go b/dgraphtest/dgraph_test.go new file mode 100644 index 00000000000..0bfdd64e39f --- /dev/null +++ b/dgraphtest/dgraph_test.go @@ -0,0 +1,66 @@ +/* + * SPDX-FileCopyrightText: © 2017-2026 Istari Digital, Inc. + * SPDX-License-Identifier: Apache-2.0 + */ + +package dgraphtest + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestZeroCmdCarriesSecurityWhitelist pins that the harness configures Zero's admin +// whitelist, not only the Alpha's. +// +// Zero guards /moveTablet and /removeNode with adminAuthHandler(strict=true), which +// applies whether or not --security is configured: with neither a token nor a +// whitelist, only a loopback caller is admitted. A test process reaching Zero through +// a published container port is not loopback, so a harness that omits this cannot +// exercise those endpoints at all — every call returns 401 ErrorUnauthorized. +// +// This is asserted on the COMMAND rather than end to end, deliberately. Whether the +// container observes a loopback peer depends on the host's Docker networking: on Linux +// a published port arrives from the bridge gateway and the guard fires, while on +// Docker Desktop it can arrive as loopback and the guard never fires. An integration +// test would therefore pass on some developer machines no matter what the harness +// passes, which is worse than no test. The guard's own behaviour is already covered by +// the adminAuthHandler unit tests in dgraph/cmd/zero. +func TestZeroCmdCarriesSecurityWhitelist(t *testing.T) { + c := &LocalCluster{conf: NewClusterConfig()} + z := &zero{id: 0, aliasName: "zero0"} + + cmd := strings.Join(z.cmd(c), " ") + require.Contains(t, cmd, "--security=whitelist=", + "Zero must get an admin whitelist, or the harness cannot reach /moveTablet and "+ + "/removeNode: they are guarded with strict=true, so without it only a loopback "+ + "caller is admitted") +} + +// TestZeroCmdCarriesSecurityToken: when a test configures a token, Zero has to know it +// too. Zero authorizes an admin request on the token OR the whitelist, so a cluster +// where only the Alpha knows the token is incoherent rather than merely stricter. +func TestZeroCmdCarriesSecurityToken(t *testing.T) { + const token = "shhh" + c := &LocalCluster{conf: NewClusterConfig().WithSecurityToken(token)} + z := &zero{id: 0, aliasName: "zero0"} + + cmd := strings.Join(z.cmd(c), " ") + require.Contains(t, cmd, "token="+token, + "Zero must be told the configured --security token, as the Alpha already is") +} + +// TestZeroCmdPreV21OmitsSecurity. Pre-v21 binaries predate both the --security +// superflag and the admin guard, so passing the flag there would fail an upgrade test +// with an unknown-flag error while fixing nothing. The Alpha's command makes the same +// split for the same reason. +func TestZeroCmdPreV21OmitsSecurity(t *testing.T) { + c := &LocalCluster{conf: NewClusterConfig(), lowerThanV21: true} + z := &zero{id: 0, aliasName: "zero0"} + + cmd := strings.Join(z.cmd(c), " ") + require.NotContains(t, cmd, "--security", + "a pre-v21 Zero does not understand --security and would fail to start") +}