fix(dgraphtest): give Zero the admin whitelist the Alpha already gets - #9818
fix(dgraphtest): give Zero the admin whitelist the Alpha already gets#9818matthewmcneely wants to merge 1 commit into
Conversation
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) <noreply@anthropic.com>
![Fix with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/fix-with-codesmith-light.png)
|
Closing this — the premise was wrong, and I want the record to say so rather than leave a Zero already gets its admin whitelist, via The 401 I was chasing was self-inflicted. #9813 had overwritten three Fixed in #9813 by restoring those files to For anyone who lands here from a search: |
1 participant
Description
Zero guards
/moveTabletand/removeNodewithadminAuthHandler(strict=true), so they areprotected whether or not
--securityis configured: with neither a token nor a whitelist, only aloopback caller is admitted.
dgraphtestpasses--securityto the Alpha and never to Zero, so atest process reaching Zero through a published container port cannot call those endpoints at all.
Pre-v21 is deliberately excluded: those binaries predate both the
--securitysuperflag and theadmin 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 for the same reason.
Testing
Asserted on the command rather than end to end, on purpose. An integration test that calls
MoveTabletpasses on Docker Desktop no matter what the harness passes, because loopback isadmitted unconditionally — I wrote that test first, it passed without this fix, and a test that
cannot fail is worse than no test.
adminAuthHandler's own behaviour is already covered by the unit tests indgraph/cmd/zero,including the loopback and remote cases. What was untested is the harness configuring Zero, so
that is what these three tests pin. Two of them fail without this change; the third (pre-v21)
passes either way by design, guarding against over-applying the flag.
Checklist