rgw/admin: fold User.Tenant into the uid on get/modify/remove - #1324
Draft
jhoblitt wants to merge 1 commit into
Draft
rgw/admin: fold User.Tenant into the uid on get/modify/remove#1324jhoblitt wants to merge 1 commit into
jhoblitt wants to merge 1 commit into
Conversation
The Tenant field is only in CreateUser's URL-parameter whitelist, so GetUser, ModifyUser and RemoveUser silently drop a populated Tenant and address the bare uid: the same-named user in the empty tenant. RGW resolves tenancy through the combined "tenant$uid" form on every operation, and the Admin Ops API documents the two spellings as equivalent. Fold a populated Tenant into the uid before encoding on those three calls: - a bare ID becomes "tenant$id" - an ID already carrying the same tenant is unchanged - an ID carrying a different tenant is an error - a Tenant with no ID (GetUser lookup by access key) is an error No signatures change, and the only affected calls are ones that today silently operate on the wrong user, which no correct program can rely on. Fixes: ceph#1323 Signed-off-by: Joshua Hoblitt <josh@hoblitt.com>
anoopcs9
reviewed
Aug 30, 2026
anoopcs9
left a comment
anoopcs9
left a comment
Collaborator
There was a problem hiding this comment.
lgtm, thanks.
Some nits:
- withTenantUID() only splits on the first
$, so it doesn't handle the three-parttenant$ns$idnamespace form - harmless here since struct admin.User has no namespace concept, but a comment notingIDis expected to be bare or "tenant$id" would make the scope explicit. - The
Tenantfield comment restates the fold rules that also live on the withTenantUID() doc. The in-struct comment itself is fine but you could trim the overlap - leaving the "what" on the field and the "how" on the method - to avoid documenting the behavior in two places.
Besides all these, is there any reason to not take this out of Draft status?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
admin.User.Tenantappears only inCreateUser's URL-parameter whitelist. OnGetUser,ModifyUserandRemoveUsera populatedTenantis silently dropped and the call addresses the bareuid— the same-named user in the empty tenant. RGW resolves tenancy through the combined uid on every operation, and the Admin Ops documentation declares the two spellings equivalent where the parameter exists: "Atenantmay either be specified as a part of uid or as an additional request param" (doc/radosgw/adminops.rst, Create User).Fixes #1323
Related: #1307 (empty values are never transmitted — a different gap in the same encoder; not addressed here)
Change
GetUser/ModifyUser/RemoveUsernow fold a populatedTenantinto the uid before encoding:IDwithout$→ sendsuid = Tenant + "$" + IDIDalreadytenant$uidwith the same tenant → unchangedIDalready tenanted with a different tenant → errorTenantset with emptyID(GetUser lookup by access key) → errorCreateUseris untouched. No signatures change and no new exported symbols are added; the only affected calls are ones that today silently operate on the wrong user — behavior no correct program can rely on.Testing
Local, no cluster (librados headers are not installed here;
-tags nautilusexcludes the one rgw/admin test file that importsrados):go build ./rgw/...— okgo vet -tags nautilus ./rgw/admin/— okgo test -tags nautilus ./rgw/admin/ -skip TestRadosGWTestSuite -v— PASS (all mock/unit tests, including the newTestUserWithTenantUID,TestGetUserTenantMockAPI,TestModifyUserTenantMockAPI,TestRemoveUserTenantMockAPI)revive -config .revive.toml ./rgw/admin/...(v1.7.0) — cleangofmt -l rgw/— cleanThe
RadosGWTestSuiteintegration tests need a live cluster and were not run locally.Server-side counterpart: https://tracker.ceph.com/issues/79816 proposes RGW itself accept the
tenantparameter on user info/modify/remove (implementation in ceph/ceph#71301). That change and this one are independent — the combined-uid fold here works against all existing RGW releases.AI assistance: this change was drafted by an AI agent under my direction and review.