Skip to content

Commit 2bae541

Browse files
authored
admin: S3 bucket write endpoints (P2 slice 2a) (#669)
P2 slice 2a of [docs/design/2026_04_24_proposed_admin_dashboard.md](https://github.com/bootjp/elastickv/blob/main/docs/design/2026_04_24_proposed_admin_dashboard.md). Ships the S3 admin write endpoints so the SPA's S3 modals stop receiving 405. Slice 2b will plumb AdminForward so a follower can hand these writes off to the leader transparently. ## Summary - **`*adapter.S3Server.AdminCreateBucket` / `AdminPutBucketAcl` / `AdminDeleteBucket`** — SigV4-bypass write methods with three in-method guards: principal must be `AdminRoleFull`, the local node must be the verified S3 leader, and bucket-name / ACL must pass the existing validators. `AdminCreateBucket` reuses the same atomic bucket-meta + ACL + generation-key txn the SigV4 path does — no new code path through the storage layer. `AdminDeleteBucket` rejects non-empty buckets (the dashboard cannot force recursive delete by design). - **`internal/admin` write surface** — `BucketsSource` gains the three write methods + `CreateBucketRequest` / `PutBucketACLRequest` types with the documented JSON shapes. `S3Handler.serveCollection` + `servePerBucket` route POST/PUT/DELETE through dedicated handlers with `principalForWrite` re-validating the role on every request against the live `MapRoleStore`. - **Strict body decoder** — `decodeAdminS3JSONBody` is generic over the request type, applies `DisallowUnknownFields`, rejects NUL bytes, rejects trailing tokens, and caps at 64 KiB (matches design 4.4). Used by both POST and PUT. - **`writeBucketsError`** translates the source-side sentinels into the design's HTTP statuses: 403 forbidden / 503 + Retry-After:1 leader_unavailable / 404 not_found / 409 already_exists / 409 bucket_not_empty / 400 invalid_request via `*ValidationError`. - **Bridge** — `bucketsBridge` gains write methods running through `translateAdminBucketsError`, mirroring `translateAdminTablesError` on the Dynamo side. Leader-churn sentinels from the kv coordinator route to `admin.ErrBucketsNotLeader` so the SPA's retry contract stays intact. ## What is NOT in this PR - AdminForward integration for S3 admin writes — slice 2b. - Rolling-upgrade compatibility flag (criterion 5) — still deferred behind a cluster-version bump. ## Test plan - [x] `go build ./...` - [x] `go vet ./...` - [x] `golangci-lint run` (admin + main + adapter packages: 0 issues) - [x] `go test ./internal/admin/ -count=1 -race` — 19 new handler tests pass - [x] `go test . -count=1 -race` — main package passes - [x] `go test -run "TestS3Server_Admin" ./adapter/ -count=1 -race` — 10 new adapter tests pass - [ ] Full `go test ./adapter/` times out at 120s due to a pre-existing flake (verified earlier on PRs #648 / #658 against `main` — unrelated to this branch) - [ ] End-to-end smoke against a 3-node cluster — slice 2b first, then a manual exercise ## Acceptance criteria coverage (Section 4.1) | Endpoint | This PR | |---|---| | `GET /admin/api/v1/s3/buckets` | ✓ (#658) | | `GET /admin/api/v1/s3/buckets/{name}` | ✓ (#658) | | `POST /admin/api/v1/s3/buckets` | ✅ | | `PUT /admin/api/v1/s3/buckets/{name}/acl` | ✅ | | `DELETE /admin/api/v1/s3/buckets/{name}` | ✅ | ## Self-review (5 lenses) 1. **Data loss**: `AdminCreateBucket` reuses `s.coordinator.Dispatch` with the same `OperationGroup` shape as the SigV4 path — bucket meta + generation key in one txn. No new FSM / Pebble / Raft path. `AdminDeleteBucket`'s "must be empty" guard is a SnapshotAt scan + size check identical to the SigV4 path. 2. **Concurrency**: Writes go through `retryS3Mutation` which already handles transient mid-dispatch leader churn. The leader check is `isVerifiedS3Leader` — same primitive the SigV4 path uses. Role gate is re-evaluated against the live `MapRoleStore` on every request, so a key downgrade picked up between login and write is enforced immediately. 3. **Performance**: One additional load-bucket-meta read on PutACL / Delete. No hot-path changes; admin writes are operator-rate, not data-plane-rate. 4. **Data consistency**: `AdminCreateBucket` writes (BucketMetaKey, BucketGenerationKey) atomically. `AdminPutBucketAcl` mutates only `meta.Acl` and re-encodes the entire BucketMeta — generation is preserved so existing object references stay valid. `AdminDeleteBucket` removes only BucketMetaKey (BucketGenerationKey is left behind, matching the SigV4 path's behaviour — a future re-create gets a fresh generation). 5. **Test coverage**: 29 new tests (19 admin-package + 10 adapter-level) covering happy paths, role gates, leader checks, validation rejections, all four sentinel error mappings, and the cross-method missing-principal 401. The existing `TestS3Handler_DescribeBucket_SubpathReturns404` was superseded by two more precise tests now that `/acl` is a real sub-resource. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * S3 bucket admin endpoints: create, update ACL, and delete with enforced write authorization, input validation, leader-forwarding, and clear HTTP error mappings. * **Documentation** * New admin docs covering dashboard config, TLS/role semantics, audit logging, and troubleshooting for admin operations. * **Tests** * Extensive end-to-end and unit tests for bucket lifecycle, forwarding, auth/validation, and error scenarios. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 parents b1b7a83 + 55c93c8 commit 2bae541

22 files changed

Lines changed: 3477 additions & 140 deletions

adapter/s3_admin.go

Lines changed: 264 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@ import (
44
"bytes"
55
"context"
66
"sort"
7+
"strings"
78

89
"github.com/bootjp/elastickv/internal/s3keys"
10+
"github.com/bootjp/elastickv/kv"
911
"github.com/bootjp/elastickv/store"
1012
"github.com/cockroachdb/errors"
1113
)
@@ -175,3 +177,265 @@ func summaryFromBucketMeta(name string, meta *s3BucketMeta) AdminBucketSummary {
175177
Owner: meta.Owner,
176178
}
177179
}
180+
181+
// Sentinel errors the admin write methods return so the bridge in
182+
// main_admin.go can translate them into admin-package vocabulary
183+
// without sniffing strings. Named separately from
184+
// ErrAdminTableAlreadyExists / ErrAdminTableNotFound on the Dynamo
185+
// side so a future per-resource role / status divergence does not
186+
// require renaming both packages' callers.
187+
var (
188+
// ErrAdminBucketAlreadyExists signals that AdminCreateBucket
189+
// targeted a name already in use. Maps to 409 Conflict.
190+
ErrAdminBucketAlreadyExists = errors.New("s3 admin: bucket already exists")
191+
// ErrAdminBucketNotFound signals that AdminDeleteBucket /
192+
// AdminPutBucketAcl targeted a missing bucket. Maps to 404.
193+
ErrAdminBucketNotFound = errors.New("s3 admin: bucket not found")
194+
// ErrAdminBucketNotEmpty signals that AdminDeleteBucket targeted
195+
// a bucket that still has objects. Maps to 409 Conflict to match
196+
// the SigV4 path's BucketNotEmpty response (the dashboard cannot
197+
// force a recursive delete; the operator must clean up first).
198+
ErrAdminBucketNotEmpty = errors.New("s3 admin: bucket is not empty")
199+
// ErrAdminInvalidBucketName signals that AdminCreateBucket got
200+
// a name that does not satisfy validateS3BucketName. Maps to 400.
201+
ErrAdminInvalidBucketName = errors.New("s3 admin: invalid bucket name")
202+
// ErrAdminInvalidACL signals that the ACL string did not pass
203+
// validateS3CannedAcl. Maps to 400 (the SigV4 path returns 501
204+
// NotImplemented for unsupported canned ACLs, but the admin API
205+
// is documented as private/public-read only and rejecting other
206+
// values as invalid input is a more useful contract for the
207+
// dashboard).
208+
ErrAdminInvalidACL = errors.New("s3 admin: invalid ACL")
209+
)
210+
211+
// AdminCreateBucket creates a bucket on behalf of the admin
212+
// dashboard. The principal MUST be re-validated by the caller (the
213+
// admin HTTP handler does this against the live RoleStore); this
214+
// method enforces the authorisation invariant a second time so a
215+
// follower-forwarded call cannot smuggle a read-only principal past
216+
// the check on the leader side (Section 3.2 "認可の真実は常に
217+
// adapter 側").
218+
//
219+
// The transaction is atomic: bucket meta + generation + ACL all land
220+
// in a single OperationGroup, mirroring the SigV4 createBucket path.
221+
// On success returns the freshly-stored summary; on conflict returns
222+
// ErrAdminBucketAlreadyExists; on a non-leader / non-full-role / bad
223+
// input returns the corresponding sentinel.
224+
func (s *S3Server) AdminCreateBucket(ctx context.Context, principal AdminPrincipal, name, acl string) (*AdminBucketSummary, error) {
225+
if !principal.Role.canWrite() {
226+
return nil, ErrAdminForbidden
227+
}
228+
if !s.isVerifiedS3Leader() {
229+
return nil, ErrAdminNotLeader
230+
}
231+
if err := validateS3BucketName(name); err != nil {
232+
return nil, errors.Wrapf(ErrAdminInvalidBucketName, "%s", err.Error())
233+
}
234+
acl = adminCanonicalACL(acl)
235+
if err := validateS3CannedAcl(acl); err != nil {
236+
return nil, errors.Wrapf(ErrAdminInvalidACL, "%s", err.Error())
237+
}
238+
239+
var summary *AdminBucketSummary
240+
err := s.retryS3Mutation(ctx, func() error {
241+
out, err := s.adminCreateBucketTxn(ctx, principal, name, acl)
242+
if err != nil {
243+
return err
244+
}
245+
summary = out
246+
return nil
247+
})
248+
if err != nil {
249+
return nil, err //nolint:wrapcheck // sentinel errors propagate as-is; structured errors are already wrapped above.
250+
}
251+
return summary, nil
252+
}
253+
254+
// adminCreateBucketTxn is the per-attempt body retryS3Mutation
255+
// invokes. Pulled out so AdminCreateBucket stays under the
256+
// cyclomatic ceiling without hiding the bucket-existence /
257+
// generation / commit-ts dance — every step has a meaningful
258+
// error path that the wrapping retry harness needs to see.
259+
func (s *S3Server) adminCreateBucketTxn(ctx context.Context, principal AdminPrincipal, name, acl string) (*AdminBucketSummary, error) {
260+
readTS := s.readTS()
261+
startTS := s.txnStartTS(readTS)
262+
readPin := s.pinReadTS(readTS)
263+
defer readPin.Release()
264+
265+
existing, exists, err := s.loadBucketMetaAt(ctx, name, readTS)
266+
if err != nil {
267+
return nil, errors.WithStack(err)
268+
}
269+
if exists && existing != nil {
270+
return nil, ErrAdminBucketAlreadyExists
271+
}
272+
nextGeneration, err := s.nextBucketGenerationAt(ctx, name, readTS)
273+
if err != nil {
274+
return nil, errors.WithStack(err)
275+
}
276+
commitTS, err := s.nextTxnCommitTS(startTS)
277+
if err != nil {
278+
return nil, errors.WithStack(err)
279+
}
280+
meta := &s3BucketMeta{
281+
BucketName: name,
282+
Generation: nextGeneration,
283+
CreatedAtHLC: commitTS,
284+
Region: s.effectiveRegion(),
285+
Owner: principal.AccessKey,
286+
Acl: acl,
287+
}
288+
body, err := encodeS3BucketMeta(meta)
289+
if err != nil {
290+
return nil, errors.WithStack(err)
291+
}
292+
_, err = s.coordinator.Dispatch(ctx, &kv.OperationGroup[kv.OP]{
293+
IsTxn: true,
294+
StartTS: startTS,
295+
CommitTS: commitTS,
296+
Elems: []*kv.Elem[kv.OP]{
297+
{Op: kv.Put, Key: s3keys.BucketMetaKey(name), Value: body},
298+
{Op: kv.Put, Key: s3keys.BucketGenerationKey(name), Value: encodeS3Generation(nextGeneration)},
299+
},
300+
})
301+
if err != nil {
302+
return nil, errors.WithStack(err)
303+
}
304+
out := summaryFromBucketMeta(name, meta)
305+
return &out, nil
306+
}
307+
308+
// AdminPutBucketAcl swaps the canned ACL on an existing bucket.
309+
// Same authorisation contract as AdminCreateBucket. Mutates only
310+
// the meta.Acl field; generation is preserved so existing object
311+
// references stay valid.
312+
func (s *S3Server) AdminPutBucketAcl(ctx context.Context, principal AdminPrincipal, name, acl string) error {
313+
if !principal.Role.canWrite() {
314+
return ErrAdminForbidden
315+
}
316+
if !s.isVerifiedS3Leader() {
317+
return ErrAdminNotLeader
318+
}
319+
acl = adminCanonicalACL(acl)
320+
if err := validateS3CannedAcl(acl); err != nil {
321+
return errors.Wrapf(ErrAdminInvalidACL, "%s", err.Error())
322+
}
323+
324+
err := s.retryS3Mutation(ctx, func() error {
325+
readTS := s.readTS()
326+
startTS := s.txnStartTS(readTS)
327+
readPin := s.pinReadTS(readTS)
328+
defer readPin.Release()
329+
330+
meta, exists, err := s.loadBucketMetaAt(ctx, name, readTS)
331+
if err != nil {
332+
return errors.WithStack(err)
333+
}
334+
if !exists || meta == nil {
335+
return ErrAdminBucketNotFound
336+
}
337+
meta.Acl = acl
338+
body, err := encodeS3BucketMeta(meta)
339+
if err != nil {
340+
return errors.WithStack(err)
341+
}
342+
_, err = s.coordinator.Dispatch(ctx, &kv.OperationGroup[kv.OP]{
343+
IsTxn: true,
344+
StartTS: startTS,
345+
Elems: []*kv.Elem[kv.OP]{
346+
{Op: kv.Put, Key: s3keys.BucketMetaKey(name), Value: body},
347+
},
348+
})
349+
return errors.WithStack(err)
350+
})
351+
if err != nil {
352+
return err //nolint:wrapcheck // sentinel errors propagate as-is.
353+
}
354+
return nil
355+
}
356+
357+
// AdminDeleteBucket removes a bucket if it is empty. Same
358+
// authorisation contract as the other admin write methods. The
359+
// bucket-must-be-empty rule mirrors the SigV4 deleteBucket path —
360+
// the dashboard cannot force a recursive delete, by design.
361+
//
362+
// Known orphan-race limitation (coderabbitai 🔴 / 🟠 on PR #669):
363+
// the empty-bucket probe (ScanAt with limit=1 on
364+
// ObjectManifestPrefixForBucket) reads at readTS but the
365+
// subsequent BucketMetaKey delete only carries that single point
366+
// key in its ReadKeys set. A concurrent PutObject that inserts a
367+
// manifest key in the scanned prefix between readTS and the
368+
// delete's commitTS will not conflict — the OCC validator only
369+
// inspects keys that appear in ReadKeys, and there is no
370+
// ReadRanges mechanism today. The object's manifest key survives
371+
// under a now-deleted bucket meta and becomes orphaned.
372+
//
373+
// This race exists pre-existing in the SigV4 path
374+
// (adapter/s3.go:deleteBucket — same shape, same limitation), so
375+
// AdminDeleteBucket inherits the contract; closing the gap
376+
// requires either (a) bumping BucketGenerationKey on every
377+
// PutObject so it can serve as an OCC token in this read set, or
378+
// (b) extending OperationGroup with ReadRanges and teaching the
379+
// FSM to validate range emptiness atomically with commit. Both
380+
// are larger changes outside this PR's scope; tracked in
381+
// docs/design/2026_04_24_partial_admin_dashboard.md under the
382+
// Outstanding open items section. Operators concerned about the
383+
// orphan window today should pause writes against the target
384+
// bucket before issuing the admin delete.
385+
func (s *S3Server) AdminDeleteBucket(ctx context.Context, principal AdminPrincipal, name string) error {
386+
if !principal.Role.canWrite() {
387+
return ErrAdminForbidden
388+
}
389+
if !s.isVerifiedS3Leader() {
390+
return ErrAdminNotLeader
391+
}
392+
393+
err := s.retryS3Mutation(ctx, func() error {
394+
readTS := s.readTS()
395+
startTS := s.txnStartTS(readTS)
396+
readPin := s.pinReadTS(readTS)
397+
defer readPin.Release()
398+
399+
meta, exists, err := s.loadBucketMetaAt(ctx, name, readTS)
400+
if err != nil {
401+
return errors.WithStack(err)
402+
}
403+
if !exists || meta == nil {
404+
return ErrAdminBucketNotFound
405+
}
406+
start := s3keys.ObjectManifestPrefixForBucket(name, meta.Generation)
407+
kvs, err := s.store.ScanAt(ctx, start, prefixScanEnd(start), 1, readTS)
408+
if err != nil {
409+
return errors.WithStack(err)
410+
}
411+
if len(kvs) > 0 {
412+
return ErrAdminBucketNotEmpty
413+
}
414+
_, err = s.coordinator.Dispatch(ctx, &kv.OperationGroup[kv.OP]{
415+
IsTxn: true,
416+
StartTS: startTS,
417+
Elems: []*kv.Elem[kv.OP]{
418+
{Op: kv.Del, Key: s3keys.BucketMetaKey(name)},
419+
},
420+
})
421+
return errors.WithStack(err)
422+
})
423+
if err != nil {
424+
return err //nolint:wrapcheck // sentinel errors propagate as-is.
425+
}
426+
return nil
427+
}
428+
429+
// adminCanonicalACL normalises an empty input to the canned
430+
// "private" default. The SigV4 createBucket / putBucketAcl paths
431+
// apply the same default after trimming the x-amz-acl header.
432+
// Pulled out so the admin write methods do not silently accept a
433+
// blank string and create / mutate with whatever validateS3CannedAcl
434+
// happens to allow on its empty branch.
435+
func adminCanonicalACL(acl string) string {
436+
trimmed := strings.TrimSpace(acl)
437+
if trimmed == "" {
438+
return s3AclPrivate
439+
}
440+
return trimmed
441+
}

0 commit comments

Comments
 (0)