Skip to content

admin: read-only DynamoDB tables endpoints (P1)#633

Closed
bootjp wants to merge 7 commits intomainfrom
feat/admin-dynamo-tables-readonly
Closed

admin: read-only DynamoDB tables endpoints (P1)#633
bootjp wants to merge 7 commits intomainfrom
feat/admin-dynamo-tables-readonly

Conversation

@bootjp
Copy link
Copy Markdown
Owner

@bootjp bootjp commented Apr 25, 2026

Summary

  • Wire GET /admin/api/v1/dynamo/tables and GET /admin/api/v1/dynamo/tables/{name} per design doc Section 4.1, both gated by the existing protect chain (BodyLimit → SessionAuth → Audit → CSRF). Audit is a no-op for GET so dashboard polling does not flood the audit log.
  • Adapter side: AdminListTables / AdminDescribeTable SigV4-bypass entrypoints (Section 3.2), exposing a stable AdminTableSummary DTO that keeps dynamoTableSchema private.
  • Pagination matches Section 4.3: limit (default 100, hard max 1000) plus an opaque next_token (base64url-wrapped last name). A vanished cursor fast-forwards rather than stalling the SPA.
  • ServerDeps.Tables is optional — leaving it nil keeps the dynamo paths off the wire (standard unknown_endpoint 404), so a build that ships only the cluster page works unchanged.
  • main_admin.go bridges *adapter.DynamoDBServer via dynamoTablesBridge. The two summary structs are intentionally isomorphic so any future field drift breaks the build at the bridge.
  • Internal errors are logged in full and surfaced to clients only as dynamo_list_failed / dynamo_describe_failed codes; raw err.Error() is never sent over the wire.

Stacked work this unblocks (separate PRs): POST /dynamo/tables (CreateTable), DELETE /dynamo/tables/{name}, AdminForward RPC for follower→leader forwarding (Section 3.3), and the matching S3 read-only endpoints.

Test plan

  • go build ./...
  • go vet ./...
  • golangci-lint run (admin, adapter, root: 0 issues)
  • go test -race ./internal/admin/...
  • go test ./adapter/ -run 'TestDynamoDB_Admin'
  • go test ./ -run 'TestStartAdminServer|TestParseCSV|TestResolveSigningKey|TestNewClusterInfoSource|TestBuildAdminConfig'
  • Manual smoke: hit /admin/api/v1/dynamo/tables?limit=2&next_token=... against a running node with a few tables created via the SigV4 client

Summary by CodeRabbit

  • New Features
    • Added admin API endpoints to list and describe DynamoDB tables with schema details (partition keys, sort keys, global secondary indexes).
    • Pagination support for table listing with configurable limits.
    • Endpoints require authentication; unauthenticated requests return 401.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 25, 2026

Warning

Rate limit exceeded

@bootjp has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 47 minutes and 35 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 47 minutes and 35 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 22bb07b6-99c8-49d0-91fc-17280525940d

📥 Commits

Reviewing files that changed from the base of the PR and between c7eb239 and b6bb577.

📒 Files selected for processing (10)
  • adapter/dynamodb_admin.go
  • adapter/dynamodb_admin_test.go
  • internal/admin/dynamo_handler.go
  • internal/admin/dynamo_handler_test.go
  • internal/admin/router.go
  • internal/admin/server.go
  • internal/admin/server_test.go
  • main.go
  • main_admin.go
  • main_admin_test.go
📝 Walkthrough

Walkthrough

This pull request introduces a DynamoDB admin API that exposes table metadata through HTTP endpoints. New adapter methods in the DynamoDB server (AdminListTables, AdminDescribeTable) retrieve table information. An HTTP handler routes requests to /admin/api/v1/dynamo/tables endpoints with pagination support, while the startup flow wires the DynamoDB server instance to the admin listener to enable this functionality.

Changes

Cohort / File(s) Summary
DynamoDB Adapter Admin Layer
adapter/dynamodb_admin.go, adapter/dynamodb_admin_test.go
Added AdminTableSummary and AdminGSISummary types, plus AdminListTables and AdminDescribeTable methods to expose table schemas and GSI metadata. Methods return deterministically sorted results for UI diffing/snapshot testing. Tests validate empty/non-empty table listings, missing table handling, and GSI metadata projection.
Admin HTTP Handler
internal/admin/dynamo_handler.go, internal/admin/dynamo_handler_test.go
Introduced DynamoHandler to serve table metadata over HTTP with two endpoints: paginated table listing at /admin/api/v1/dynamo/tables (supporting limit and next_token query params) and table details at /admin/api/v1/dynamo/tables/{name}. Handler validates inputs, generates base64url pagination cursors, and returns structured JSON errors. Tests cover pagination semantics, input validation, routing, and error handling.
Admin Server Integration
internal/admin/server.go, internal/admin/server_test.go
Extended ServerDeps with optional Tables field and wired DynamoHandler into admin mux with route prefix matching for Dynamo subpaths. Tests validate authentication enforcement, proper table endpoint access, and graceful 404 fallback when tables are not configured.
Application Startup
main.go, main_admin.go, main_admin_test.go
Modified startup flow to return *adapter.DynamoDBServer from startDynamoDBServer, store it in runner, and pass to admin bootstrap. Added bridge (dynamoTablesBridge) that translates adapter types to admin types and handles nil-safety. Updated test call sites to pass nil for new parameter.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Admin Handler
    participant Dynamo Adapter
    participant Storage

    Client->>Admin Handler: GET /admin/api/v1/dynamo/tables/{name}
    Note over Admin Handler: Validate name parameter
    Admin Handler->>Dynamo Adapter: AdminDescribeTable(ctx, name)
    Dynamo Adapter->>Storage: Load table schema
    Storage-->>Dynamo Adapter: Table metadata
    Dynamo Adapter->>Dynamo Adapter: Convert to AdminTableSummary<br/>(project fields, sort GSIs)
    Dynamo Adapter-->>Admin Handler: (*AdminTableSummary, exists, error)
    alt Table exists
        Admin Handler->>Admin Handler: Marshal to JSON
        Admin Handler-->>Client: 200 OK + JSON payload
    else Table not found
        Admin Handler-->>Client: 404 Not Found
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 Tables now exposed with admin delight,
Metadata flowing through endpoints so bright!
Pagination with cursors, deterministic and clean,
GSIs sorted lexicographically—pristine!
The DynamoDB secrets are served up just right. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding read-only DynamoDB table admin endpoints, which is the primary objective across all modified files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/admin-dynamo-tables-readonly

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@bootjp bootjp force-pushed the feat/admin-dynamo-tables-readonly branch from db8e210 to ec64f1c Compare April 25, 2026 13:39
@bootjp bootjp changed the title admin: add read-only DynamoDB tables endpoints (P1) admin: read-only DynamoDB tables endpoints (P1) Apr 25, 2026
@bootjp
Copy link
Copy Markdown
Owner Author

bootjp commented Apr 25, 2026

@claude review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a read-only administrative API for DynamoDB-compatible tables, allowing the admin dashboard to list and describe table schemas without SigV4 authentication. It adds internal methods to the DynamoDB adapter, implements a new HTTP handler with pagination support, and wires these components into the existing admin server. Review feedback highlights opportunities to prevent potential memory issues by moving pagination to the server-side and to improve response reliability by buffering JSON encoding before flushing the HTTP headers.

return
}

names, err := h.source.AdminListTables(r.Context())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current implementation of AdminListTables fetches the entire list of table names into memory before performing pagination in the handler. To prevent unbounded memory growth and potential OOM issues, apply a fixed bound or server-side pagination to collections that can grow from external requests. Consider updating the TablesSource interface to support server-side pagination (passing limit and startAfter to the source) to avoid loading the full dataset on every request.

References
  1. To prevent unbounded memory growth and potential OOM issues, apply a fixed bound to collections that can grow from external requests, such as pending configuration changes. Reject new requests when the bound is reached.

Comment thread internal/admin/dynamo_handler.go Outdated
w.Header().Set("Content-Type", "application/json; charset=utf-8")
w.Header().Set("Cache-Control", "no-store")
w.WriteHeader(http.StatusOK)
if err := json.NewEncoder(w).Encode(body); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using json.NewEncoder(w).Encode(body) streams the JSON response directly to the http.ResponseWriter. If an error occurs during encoding after the headers have been sent (e.g., due to a large payload or a broken connection), the client will receive a partial response with a 200 OK status. For admin endpoints where responses are typically small, it is safer to marshal the JSON into a buffer first to ensure the entire response is valid before committing the HTTP status code.

@bootjp
Copy link
Copy Markdown
Owner Author

bootjp commented Apr 25, 2026

@claude review (Gemini medium 2件対応)

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (4)
main.go (1)

985-991: Minor: clarify "exposed" in doc.

The field is unexported; the comment "is populated by start() and exposed so the admin listener can call..." can read as if it's exported. A small wording tweak (e.g., "made available to startAdminFromFlags in this package") avoids the ambiguity.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@main.go` around lines 985 - 991, The comment for the unexported field
`dynamoServer *adapter.DynamoDBServer` is ambiguous about "exposed"; rewrite it
to say that `dynamoServer` is populated by `start()` and made available to the
admin listener (for example, to `startAdminFromFlags` in this package) so that
admin code can call SigV4-bypass admin entrypoints without HTTP; update the
comment text accordingly to avoid implying the field is exported while keeping
the same intent.
internal/admin/dynamo_handler_test.go (2)

19-24: listOrder is declared but no test sets it.

The field exists with the comment "lets us simulate adapter ordering bugs", yet no test currently injects a custom order. If you do not plan to use it in the near term, drop it to avoid carrying dead test scaffolding; otherwise add a test exercising it so the configurability isn't speculative.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dynamo_handler_test.go` around lines 19 - 24, The struct
stubTablesSource currently contains an unused field listOrder on
stubTablesSource; either remove this dead field or add a unit test that sets
listOrder to a non-nil slice and asserts the adapter respects that ordering when
listing tables. To fix: either delete the listOrder field and its comment from
stubTablesSource, or add a test (e.g., in dynamo_handler_test.go) that
constructs a stubTablesSource{tables: ..., listOrder: []string{"B","A"}} then
calls the list method used by the handler and verifies the returned order
matches listOrder, ensuring the adapter ordering behavior is exercised.

55-63: Prefer sort.Strings over a hand-rolled insertion sort.

The sort package is part of stdlib and idiomatic; hand-rolling insertion sort here adds maintenance surface for no benefit.

♻️ Suggested replacement
-// stableSort is a tiny helper to avoid importing sort in test files
-// that already have their own sort dependency style.
-func stableSort(s []string) {
-	for i := 1; i < len(s); i++ {
-		for j := i; j > 0 && s[j-1] > s[j]; j-- {
-			s[j-1], s[j] = s[j], s[j-1]
-		}
-	}
-}
+// (use sort.Strings directly at call sites)

And in AdminListTables:

-	stableSort(out)
+	sort.Strings(out)

As per coding guidelines: "Go code should follow gofmt + project lint rules (golangci-lint)." A custom sort is harder to justify than the stdlib idiom.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/admin/dynamo_handler_test.go` around lines 55 - 63, Replace the
hand-rolled insertion sorter stableSort with the stdlib implementation: remove
the stableSort function and call sort.Strings(...) where sorting is needed
(e.g., in AdminListTables and any tests that currently call stableSort) so the
code uses the built-in sort package; add the import "sort" if not present and
ensure any tests or helpers now rely on sort.Strings for deterministic ordering.
adapter/dynamodb_admin_test.go (1)

173-183: Minor: hardcoded region may diverge from the server-side default.

us-west-2 is fine as long as the test DynamoDB server doesn't enforce a region match in SigV4 verification. If the server is configured (or later changes) to require a specific region, every admin test breaks at once. Worth a brief comment that this region is intentionally arbitrary, or sourcing it from the same place the server uses.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@adapter/dynamodb_admin_test.go` around lines 173 - 183, The test helper
newDynamoClient currently hardcodes config.WithRegion("us-west-2"); change this
so the region is not fixed: either document it with an explicit comment that the
region is arbitrary and kept to match test server behavior, or better, source
the region from a shared test constant/env used by the test server (e.g.,
replace "us-west-2" with a variable like testDynamoRegion or a getter such as
getTestRegion()) so newDynamoClient and the server use the same region for SigV4
checks; update the newDynamoClient function signature or test setup as needed to
read that shared value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@adapter/dynamodb_admin_test.go`:
- Around line 173-183: The test helper newDynamoClient currently hardcodes
config.WithRegion("us-west-2"); change this so the region is not fixed: either
document it with an explicit comment that the region is arbitrary and kept to
match test server behavior, or better, source the region from a shared test
constant/env used by the test server (e.g., replace "us-west-2" with a variable
like testDynamoRegion or a getter such as getTestRegion()) so newDynamoClient
and the server use the same region for SigV4 checks; update the newDynamoClient
function signature or test setup as needed to read that shared value.

In `@internal/admin/dynamo_handler_test.go`:
- Around line 19-24: The struct stubTablesSource currently contains an unused
field listOrder on stubTablesSource; either remove this dead field or add a unit
test that sets listOrder to a non-nil slice and asserts the adapter respects
that ordering when listing tables. To fix: either delete the listOrder field and
its comment from stubTablesSource, or add a test (e.g., in
dynamo_handler_test.go) that constructs a stubTablesSource{tables: ...,
listOrder: []string{"B","A"}} then calls the list method used by the handler and
verifies the returned order matches listOrder, ensuring the adapter ordering
behavior is exercised.
- Around line 55-63: Replace the hand-rolled insertion sorter stableSort with
the stdlib implementation: remove the stableSort function and call
sort.Strings(...) where sorting is needed (e.g., in AdminListTables and any
tests that currently call stableSort) so the code uses the built-in sort
package; add the import "sort" if not present and ensure any tests or helpers
now rely on sort.Strings for deterministic ordering.

In `@main.go`:
- Around line 985-991: The comment for the unexported field `dynamoServer
*adapter.DynamoDBServer` is ambiguous about "exposed"; rewrite it to say that
`dynamoServer` is populated by `start()` and made available to the admin
listener (for example, to `startAdminFromFlags` in this package) so that admin
code can call SigV4-bypass admin entrypoints without HTTP; update the comment
text accordingly to avoid implying the field is exported while keeping the same
intent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 223d06bb-ad6f-4fc8-8231-02487be51ac2

📥 Commits

Reviewing files that changed from the base of the PR and between dbe4725 and c7eb239.

📒 Files selected for processing (9)
  • adapter/dynamodb_admin.go
  • adapter/dynamodb_admin_test.go
  • internal/admin/dynamo_handler.go
  • internal/admin/dynamo_handler_test.go
  • internal/admin/server.go
  • internal/admin/server_test.go
  • main.go
  • main_admin.go
  • main_admin_test.go

@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@bootjp
Copy link
Copy Markdown
Owner Author

bootjp commented Apr 25, 2026

@claude review (CodeRabbit nitpick 4件対応)

@bootjp
Copy link
Copy Markdown
Owner Author

bootjp commented Apr 25, 2026

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a read-only admin dashboard for DynamoDB-compatible tables, providing endpoints to list tables with pagination and describe specific table schemas. The implementation includes a new DynamoHandler in the internal/admin package, a bridge to the adapter package, and comprehensive unit tests. Feedback was provided regarding potential memory pressure in the table listing handler due to full list materialization and the inclusion of a migration check in the read-only AdminDescribeTable method, which may cause unintended side effects.

// admin-cluster scale ever changes that calculus, the fix is to
// teach the adapter to stream, then plumb that through here —
// not to add a streaming layer on top of the materialised list.
names, err := h.source.AdminListTables(r.Context())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current implementation of handleList materializes the entire list of table names in memory by calling AdminListTables. For clusters with a very large number of tables, this could lead to significant memory pressure or OOM issues. To prevent unbounded memory growth, apply a fixed bound to this collection and reject new requests when the bound is reached, or implement server-side pagination to ensure system stability.

References
  1. To prevent unbounded memory growth and potential OOM issues, apply a fixed bound to collections that can grow from external requests, such as pending configuration changes. Reject new requests when the bound is reached.

Comment thread adapter/dynamodb_admin.go Outdated
Comment on lines +42 to +44
if err := d.ensureLegacyTableMigration(ctx, name); err != nil {
return nil, false, err
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The AdminDescribeTable method is intended for a read-only admin endpoint, but it triggers ensureLegacyTableMigration. If this migration involves state-changing operations (writes) to the underlying storage, it violates the read-only expectation of the admin dashboard and could lead to unexpected side effects during routine polling. Consider if this migration can be deferred or if the describe call can function without it.

bootjp added a commit that referenced this pull request Apr 25, 2026
Two findings from Gemini's third review pass on PR #633.

- adapter/dynamodb_admin.go: AdminDescribeTable used to invoke
  ensureLegacyTableMigration on every call, which writes to the
  cluster (Raft-coordinated key-encoding migration) as a side
  effect of a read-only dashboard endpoint. Drop the call. The
  admin describe is now strictly a snapshot read; legacy-format
  tables migrate lazily on the next SigV4 read or write of the
  same table, which is the existing behaviour for everything else
  that touches that path.

- internal/admin/dynamo_handler.go: strengthen the handleList
  doc comment to spell out the worst-case memory bound (255 B
  per name × 10k tables ≈ 2.5 MiB) and to call out that this
  matches the SigV4 listTables path which has shipped fine.
  Streaming the metadata scan is a separate adapter-level
  refactor — bolting one on top of the materialised slice would
  be cosmetic.
@bootjp
Copy link
Copy Markdown
Owner Author

bootjp commented Apr 25, 2026

@claude review (Gemini medium 対応: AdminDescribeTable から ensureLegacyTableMigration の write 副作用を削除、handleList の memory bound doc を強化)

bootjp added a commit that referenced this pull request Apr 25, 2026
Two findings from Gemini's third review pass on PR #633.

- adapter/dynamodb_admin.go: AdminDescribeTable used to invoke
  ensureLegacyTableMigration on every call, which writes to the
  cluster (Raft-coordinated key-encoding migration) as a side
  effect of a read-only dashboard endpoint. Drop the call. The
  admin describe is now strictly a snapshot read; legacy-format
  tables migrate lazily on the next SigV4 read or write of the
  same table, which is the existing behaviour for everything else
  that touches that path.

- internal/admin/dynamo_handler.go: strengthen the handleList
  doc comment to spell out the worst-case memory bound (255 B
  per name × 10k tables ≈ 2.5 MiB) and to call out that this
  matches the SigV4 listTables path which has shipped fine.
  Streaming the metadata scan is a separate adapter-level
  refactor — bolting one on top of the materialised slice would
  be cosmetic.
@bootjp bootjp force-pushed the feat/admin-dynamo-tables-readonly branch from 5991fc3 to 46cd2f4 Compare April 25, 2026 15:11
bootjp added a commit that referenced this pull request Apr 25, 2026
Two findings from Gemini's third review pass on PR #633.

- adapter/dynamodb_admin.go: AdminDescribeTable used to invoke
  ensureLegacyTableMigration on every call, which writes to the
  cluster (Raft-coordinated key-encoding migration) as a side
  effect of a read-only dashboard endpoint. Drop the call. The
  admin describe is now strictly a snapshot read; legacy-format
  tables migrate lazily on the next SigV4 read or write of the
  same table, which is the existing behaviour for everything else
  that touches that path.

- internal/admin/dynamo_handler.go: strengthen the handleList
  doc comment to spell out the worst-case memory bound (255 B
  per name × 10k tables ≈ 2.5 MiB) and to call out that this
  matches the SigV4 listTables path which has shipped fine.
  Streaming the metadata scan is a separate adapter-level
  refactor — bolting one on top of the materialised slice would
  be cosmetic.
@bootjp bootjp force-pushed the feat/admin-dynamo-tables-readonly branch from 46cd2f4 to e3b2c51 Compare April 25, 2026 15:44
@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

bootjp added 4 commits April 26, 2026 01:48
GET /admin/api/v1/dynamo/tables and GET /admin/api/v1/dynamo/tables/{name}
land the design doc Section 4.1 read-only paths. Both go through the
same protect chain (BodyLimit -> SessionAuth -> Audit -> CSRF) as
write endpoints; Audit is a no-op on GET so dashboard polling does not
flood the audit log.

Adapter side (SigV4 bypass per Section 3.2):
- adapter.AdminListTables / AdminDescribeTable internal entrypoints
- AdminTableSummary DTO keeps dynamoTableSchema private; admin gets a
  stable struct that does not drift with the wire format

Admin handler (internal/admin/dynamo_handler.go):
- TablesSource interface (production wired, tests stub)
- limit (default 100, hard max 1000) + opaque base64 next_token
  pagination matching design Section 4.3
- vanished-cursor fast-forward so a deleted-between-pages name does
  not stall the SPA
- internal errors hidden behind dynamo_list_failed / dynamo_describe_failed
  codes; raw err.Error() is never leaked to clients

Server wiring:
- ServerDeps.Tables is optional; nil leaves the dynamo paths off the
  wire entirely (returns the standard unknown_endpoint 404)
- main_admin.go bridges *adapter.DynamoDBServer via dynamoTablesBridge,
  translating adapter.AdminTableSummary <-> admin.DynamoTableSummary
  (isomorphic so any field drift breaks the build)
writeAdminJSON used to stream json.NewEncoder(w).Encode(...) directly
to the ResponseWriter. If marshalling failed midway through the body
(an unsupported type, a Marshaler returning an error), the 200 status
header was already on the wire and the client received a truncated /
malformed JSON object — unrecoverable on the SPA side.

Marshal to []byte first, *then* write the status + body. An encode
failure now upgrades to a clean 500 with the standard error envelope.
Write failures after the status is committed remain log-only.

Also documents the deliberate "materialise the full table list before
paginate-and-slice" choice in handleList so future readers know the
adapter's listTableNames already scans the entire metadata prefix in
one shot for the SigV4 path; streaming on top would not change the
adapter's memory profile.
Four nitpick-tier findings collapsed into one commit:

- main.go: clarify the dynamoServer field doc — drop "exposed",
  which read like the field were exported. State explicitly that it
  is package-private state passed to startAdminFromFlags.

- internal/admin/dynamo_handler_test.go: drop the unused listOrder
  field on stubTablesSource (no test ever set it; carried over from
  an aborted "ordering bug" simulation). Replace the hand-rolled
  insertion sort with sort.Strings — stdlib idiom, less surface to
  maintain.

- adapter/dynamodb_admin_test.go: document why us-west-2 is a fine
  hardcoded region in the SigV4 client (the adapter does not enforce
  region match). Pointer for the future: if that ever changes, the
  region should be sourced from the same constant on both sides
  rather than re-hardcoded everywhere.
Two findings from Gemini's third review pass on PR #633.

- adapter/dynamodb_admin.go: AdminDescribeTable used to invoke
  ensureLegacyTableMigration on every call, which writes to the
  cluster (Raft-coordinated key-encoding migration) as a side
  effect of a read-only dashboard endpoint. Drop the call. The
  admin describe is now strictly a snapshot read; legacy-format
  tables migrate lazily on the next SigV4 read or write of the
  same table, which is the existing behaviour for everything else
  that touches that path.

- internal/admin/dynamo_handler.go: strengthen the handleList
  doc comment to spell out the worst-case memory bound (255 B
  per name × 10k tables ≈ 2.5 MiB) and to call out that this
  matches the SigV4 listTables path which has shipped fine.
  Streaming the metadata scan is a separate adapter-level
  refactor — bolting one on top of the materialised slice would
  be cosmetic.
@bootjp bootjp force-pushed the feat/admin-dynamo-tables-readonly branch from e3b2c51 to 455ad39 Compare April 25, 2026 16:49
@bootjp
Copy link
Copy Markdown
Owner Author

bootjp commented Apr 25, 2026

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented Apr 25, 2026

Claude finished @bootjp's task in 3m 59s —— View job


Claude's Review

Tasks

  • Gather context and understand the request
  • Read changed files and git diff
  • Review adapter layer (dynamodb_admin.go)
  • Review HTTP handler (dynamo_handler.go)
  • Review server integration (server.go)
  • Review startup wiring (main.go, main_admin.go)
  • Review tests
  • Post comprehensive review feedback

Summary

This is a solid PR that adds read-only DynamoDB admin endpoints cleanly and safely. The previous review concerns from CodeRabbit and Gemini have been properly addressed. Below are my findings, organized by severity.


✅ Addressed from Prior Reviews

Prior concern Status
AdminDescribeTable calling ensureLegacyTableMigration (write side-effect on read path) ✅ Removed — AdminDescribeTable now calls only loadTableSchema
JSON streaming encoder committing status before detecting encode errors ✅ Fixed — writeAdminJSON marshals to buffer first, then writes status + body
Memory unbounded growth concern ✅ Documented — comment at dynamo_handler.go:134-150 explains the worst-case bound (10k tables ≈ 2.5 MiB), establishes parity with the existing SigV4 path, and notes the correct fix is at the adapter scan layer
GSI output instability from map iteration ✅ Fixed — sort.Strings applied on key names before building the slice
stableSort hand-rolled insertion sort in test file ✅ Fixed — stub now uses sort.Strings directly
listOrder dead field in stubTablesSource ✅ Removed from current code
Comment on "exposed" field in main.go ✅ Fixed — comment now says "made available to startAdminFromFlags in this package"

🔍 Observations & Minor Findings

1. writeJSONError still streams directly to ResponseWriter (pre-existing, not this PR)

router.go:282 uses json.NewEncoder(w).Encode(...) on error paths. The happy-path writeAdminJSON now correctly marshals to a buffer first, but writeJSONError (used by all error responses) still has the streaming-encode risk. Since errorResponse is a simple flat struct that will never fail to marshal in practice, this is low risk. It's also pre-existing and not introduced by this PR — but worth flagging for a follow-up for consistency.

2. writeAdminJSON parameter order (dynamo_handler.go:278)

func writeAdminJSON(w http.ResponseWriter, ctx context.Context, logger *slog.Logger, body any)

Go convention (and the context package docs) state that ctx should be the first parameter. Having ResponseWriter first before ctx is unconventional. This is a new function in this PR. Not a bug, but could confuse future readers. Fix this →

3. Nil-check on resp.Tables is dead code (dynamo_handler.go:168)

if resp.Tables == nil {
    resp.Tables = []string{}
}

paginateDynamoTableNames returns []string{} (non-nil) in all its branches, and listTableNames returns make([]string, 0, ...) (non-nil). This nil-check can never fire. Harmlessly defensive, but could mislead future readers into thinking the function can return nil. Safe to remove or leave as-is.

4. runtimeServerRunner receiver change is correct but implicit (main.go:1221)

start() was a value receiver; it's now correctly a pointer receiver to allow setting r.dynamoServer. This is a necessary correctness fix. Worth noting that any existing code holding a non-pointer copy of runtimeServerRunner would have silently broken before this change — the compiler now enforces pointer usage which is the right outcome.

5. TestServer_DynamoTables_ReadOnlyRoleAllowed doesn't send CSRF token

The test uses cookies from loginAndCookies but sends no X-CSRF-Token header on the protected GET request and still gets 200. This is correct — CSRFDoubleSubmit should only gate mutating methods. The test implicitly documents that CSRF is not enforced on GETs, which is the right security behavior. Worth adding a one-line comment to make this intent explicit so future maintainers don't "fix" it.

6. No X-Content-Type-Options: nosniff on admin responses

writeAdminJSON and writeJSONError both set Content-Type: application/json but don't set X-Content-Type-Options: nosniff. Standard security hardening for admin endpoints. Low risk since admin sessions are already cookie-gated, but easy to add. Fix this →


✅ Verified Correct

  • Pagination semantics: paginateDynamoTableNames correctly handles cursor fast-forward (deleted entry between pages resumes at next surviving name), last-page empty cursor, and clamped limit.
  • Bridge type isomorphism: convertAdminTableSummary in main_admin.go is a correct field-for-field translation; preserves GSI ordering since it iterates the already-sorted slice from summaryFromSchema.
  • Optional Tables dependency: nil Tables in ServerDeps correctly leaves dynamo paths unregistered (unknown_endpoint 404). TestServer_DynamoTables_NilSourceFallsThroughTo404 verifies this.
  • Error isolation: Raw err.Error() never reaches the wire — only dynamo_list_failed / dynamo_describe_failed codes.
  • Auth enforcement: Dynamo endpoints use the same protect chain as cluster. TestServer_DynamoTables_RequiresSession verifies 401 on missing session.
  • Empty array contract: []string{} (not null) returned when no tables exist, verified by TestDynamoHandler_ListTables_EmptyArrayNotNull.
  • Name validation in handleDescribe: Slash-bearing names and empty names are rejected with 404 before hitting the source. TestDynamoHandler_DescribeTable_RejectsSlashInName covers this.

Overall: Ready to merge with the minor items above as optional follow-ups. The two Gemini medium concerns are properly addressed, and the implementation is clean and well-tested.

Two minor items from Claude's review on PR #633:

- Add X-Content-Type-Options: nosniff to both writeJSONError and
  writeAdminJSON. The admin surface is JSON-only, so MIME sniffing
  is never useful and the header guards against XSS-via-sniffing on
  any payload that ever reaches the response writer through an
  unexpected path. Cookie-gated admin endpoints already constrain
  attack surface, but this is cheap defence in depth.

- Remove the dead `if resp.Tables == nil { resp.Tables = []string{} }`
  guard from handleList. paginateDynamoTableNames is total over its
  input — the "cursor past end" branch returns []string{} and every
  other branch returns a real sub-slice, both non-nil. Replace the
  guard with a comment that documents the producer-side invariant
  so future readers don't assume the function can return nil.
@bootjp
Copy link
Copy Markdown
Owner Author

bootjp commented Apr 25, 2026

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented Apr 25, 2026

Claude finished @bootjp's task in 2m 4s —— View job


Tasks

  • Gather context and understand the request
  • Read changed files
  • Review adapter layer (adapter/dynamodb_admin.go)
  • Review HTTP handler (internal/admin/dynamo_handler.go)
  • Review server integration (internal/admin/server.go, router.go)
  • Review startup wiring (main.go, main_admin.go)
  • Review tests
  • Post comprehensive review feedback

Review

This PR is in excellent shape. All prior concerns from Gemini and CodeRabbit reviews have been properly addressed. Here's a full accounting.


✅ Previously Flagged — All Addressed

Prior concern Status
AdminDescribeTable calling ensureLegacyTableMigration (write side-effect on read path) ✅ Removed — adapter/dynamodb_admin.go:53 now calls only loadTableSchema
JSON streaming encoder committing status before detecting encode errors ✅ Fixed — writeAdminJSON (dynamo_handler.go:279) marshals to buffer first, then writes
X-Content-Type-Options: nosniff missing on admin responses ✅ Added — present in both writeAdminJSON (dynamo_handler.go:298) and writeJSONError (router.go:284)
Cache-Control: no-store ✅ Set in both writeAdminJSON and writeJSONError
Memory bound for handleList not documented ✅ Documented at dynamo_handler.go:134–150 with worst-case bound and SigV4-path parity rationale
stableSort hand-rolled insertion sort in test stub ✅ Replaced with sort.Strings (dynamo_handler_test.go:34)
listOrder dead field in stubTablesSource ✅ Removed
Comment on "exposed" field in main.go ✅ Fixed — now says "made available to startAdminFromFlags in this package"
Hardcoded us-west-2 region with no rationale ✅ Documented with comment at adapter/dynamodb_admin_test.go:176–181

🔍 Observations

1. writeAdminJSON parameter order — dynamo_handler.go:279 (minor style)

func writeAdminJSON(w http.ResponseWriter, ctx context.Context, logger *slog.Logger, body any)

The context docs and broader Go convention say ctx context.Context should be the first parameter. ResponseWriter leads here, which is unusual for a function that also takes a context. No behaviour impact, but may surprise future readers. Fix this →

2. writeJSONError still streams directly — router.go:287 (pre-existing, low risk)

_ = json.NewEncoder(w).Encode(errorResponse{Error: code, Message: msg})

This is the pre-existing streaming form — not introduced by this PR. Since errorResponse is a flat struct with two string fields, it is essentially impossible for json.Marshal to fail on it, so the practical risk is zero. However it's now inconsistent with the buffer-first pattern in writeAdminJSON. Worth unifying in a follow-up for maintainability.

3. TestServer_DynamoTables_ReadOnlyRoleAllowed sends no CSRF token (correct, but implicit)

The test at server_test.go:245 uses cookies from loginAndCookies but intentionally sends no X-CSRF-Token on the GET request and still expects 200. This is correct — CSRFDoubleSubmit only gates mutating methods. But the intent is implicit; a future maintainer might "fix" this by adding a CSRF token (harmless) or might misread it as a test coverage gap. A one-line comment would eliminate the ambiguity.


✅ Verified Correct

  • Pagination: paginateDynamoTableNames handles cursor fast-forward (vanished entry resumes at next surviving name), last-page empty cursor, and limit > 0 validation correctly. The len(page) > 0 guard at dynamo_handler.go:265 is redundant (since limit > 0 ensures page is non-empty when end < len(names)), but harmless.
  • Error isolation: parseDynamoListLimit and decodeDynamoNextToken return hardcoded client-safe strings only. handleList and handleDescribe surface only dynamo_list_failed / dynamo_describe_failed codes; raw err.Error() never reaches the wire.
  • Bridge isomorphism: convertAdminTableSummary in main_admin.go:156–176 is a correct field-for-field copy; GSI ordering is preserved since it iterates the already-sorted slice from summaryFromSchema.
  • Optional Tables dep: nil Tables in ServerDeps correctly leaves dynamo paths unregistered. newDynamoTablesSource returning untyped-nil interface (when dynamoServer is nil) means deps.Tables != nil evaluates to false correctly — no typed-nil gotcha.
  • Auth chain: protect chain applies BodyLimit → SessionAuth → Audit → CSRF uniformly to dynamo GET paths. Audit is a no-op for GET so dashboard polling doesn't flood the audit log.
  • Empty array contract: paginateDynamoTableNames always returns []string{} (non-nil), not nil, so the JSON response is always "tables":[] not "tables":null. Confirmed by TestDynamoHandler_ListTables_EmptyArrayNotNull.
  • Name validation: Slash-bearing names and empty names are rejected at handleDescribe with 404 before reaching the source. TestDynamoHandler_DescribeTable_RejectsSlashInName and TestDynamoHandler_DescribeTable_TrailingSlashIsRejected cover both cases.

Overall: Ready to merge. The three observations above are minor optional polish; none block the PR.

bootjp added a commit that referenced this pull request Apr 25, 2026
)

Stacked on #633 (the read-only chunk). Writes are limited to the leader
node for now; follower-side `AdminForward` RPC (design Section 3.3
acceptance criteria 1-6) ships in a follow-up PR. Mergeable on its own —
followers respond `503 leader_unavailable` + `Retry-After: 1`.

## Summary

- `POST /admin/api/v1/dynamo/tables` and `DELETE
/admin/api/v1/dynamo/tables/{name}` both go through the existing protect
chain (BodyLimit → SessionAuth → Audit → CSRF). The handler also
enforces `RoleFull` so a read-only key cannot create or delete even with
a valid CSRF token.
- Adapter side: `AdminCreateTable` / `AdminDeleteTable` take an
`AdminPrincipal` and re-validate the role at the adapter layer even when
a higher tier already enforced it. Preserves the design's *adapter side
is the source of truth for authz* invariant (Section 3.2). Two sentinel
errors (`ErrAdminNotLeader`, `ErrAdminForbidden`) signal the structured
failure modes.
- Bridge in `main_admin.go` translates adapter errors to admin sentinels
(`ErrTablesNotLeader` to 503 + `Retry-After: 1`, `ErrTablesForbidden` to
403, `ResourceInUse` to 409, `ResourceNotFound` to 404,
`ValidationException` to 400). Raw adapter error text is never surfaced
to clients; everything else falls through to a generic 500 with the
original message logged at error level.
- Strict JSON decoding (`DisallowUnknownFields`); each validation
message is plain English so the SPA can render it directly.
- Two summary structs (`adapter.AdminCreateTableInput` /
`admin.CreateTableRequest`) stay independent so neither package imports
the other; the bridge keeps them in sync and any drift breaks the build
there.

## Test plan

- [x] `go build ./...`
- [x] `go vet ./...`
- [x] `golangci-lint run` (admin, adapter, root: 0 issues)
- [x] `go test ./internal/admin/ -count=1` (49 tests pass — 14 new
write-handler unit tests, 4 new server-level integration tests)
- [x] `go test ./adapter/ -count=1 -run 'TestDynamoDB_Admin'` (14 tests
pass — 9 new write-path tests including duplicate rejection, role
enforcement at adapter, validation errors, delete missing to
ResourceNotFound, etc.)
- [ ] Manual smoke against a running node:
- `curl -X POST .../dynamo/tables` with full-role cookies + CSRF header
to 201 + JSON summary
  - same against a follower to 503 + `Retry-After: 1`
  - `DELETE` on a non-existent table to 404 `not_found`

## Stacked roadmap

1. **#633** read-only `GET /tables` + `GET /tables/{name}` (in review)
2. **THIS PR** — `POST` + `DELETE` (leader-only)
3. AdminForward RPC + follower-leader forwarding (Section 3.3 acceptance
criteria 1-6)
4. S3 read-only endpoints
5. S3 write endpoints
6. SPA (React + Vite, embed.FS)
@bootjp
Copy link
Copy Markdown
Owner Author

bootjp commented Apr 25, 2026

Closed because the read-only commits on this branch were merged into main as part of #634's squash (commit 7d9a607). All four files added in #633 (adapter/dynamodb_admin.go, internal/admin/dynamo_handler.go, internal/admin/dynamo_handler_test.go, server-level integration tests) are present in 7d9a607 along with the write-side commits. Closing to avoid the stale-branch warning.

@bootjp bootjp closed this Apr 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant