docs(design): propose elastickv admin dashboard for DynamoDB/S3 CRUD#611
docs(design): propose elastickv admin dashboard for DynamoDB/S3 CRUD#611
Conversation
Proposes an embedded admin UI (Go + React SPA) that reuses the existing DynamoDB and S3 adapter handlers in-process, so no new write paths are introduced. Covers API shape, SigV4-backed auth with read-only/full roles, CSRF, phased rollout, testing strategy, and rejected alternatives.
|
Warning Rate limit exceeded
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 51 minutes and 21 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces a comprehensive design document for a proposed Admin Dashboard feature for elastickv, specifying architecture, API specifications, authentication mechanisms via JWT, security considerations including TLS enforcement, and a multi-phase implementation plan with testing strategies. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6f6ef16c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ``` | ||
|
|
||
| - 秘密鍵はサーバー側でのみ検証し、以降のリクエストに渡らない。 | ||
| - JWT シグナルキーはノードごとにランダム生成 (プロセスライフタイム)。ノード再起動で全セッションが失効するのは許容する (長期セッションは非ゴール)。 |
There was a problem hiding this comment.
The design says JWT signing keys are generated randomly per node/process lifetime, which means a cookie minted by node A cannot be verified by node B. In a typical multi-node deployment behind a load balancer, users will get intermittent auth failures/logouts whenever requests land on a different node, even with valid credentials. Use a shared cluster secret (or centralized session store) so tokens remain valid across nodes and restarts.
Useful? React with 👍 / 👎.
| - Admin listener をパブリック IP にさらさないことを README で強く推奨。`admin.enabled` のデフォルトは `false`。 | ||
| - ログインエンドポイントにレート制限 (per-IP、5 req/min)。 | ||
| - Secret key は JWT payload には入れず、サーバーでのみ照合する。 | ||
| - TLS 無効で non-loopback にバインドしようとした場合、起動時に警告ログを出す。 |
There was a problem hiding this comment.
The proposed login flow sends secret_key in the request body, but the deployment section only warns when non-loopback admin listeners run without TLS. In that configuration, credentials can be intercepted on the network and reused to mint privileged sessions. This should be enforced as a hard startup failure (or login disabled) unless TLS is enabled for non-local binds.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request proposes a design for an integrated Admin Dashboard for elastickv, providing a web-based UI for managing DynamoDB tables and S3 buckets. The design leverages existing adapters and embeds a React SPA into the Go binary. Feedback focuses on critical architectural issues: a contradiction in the authentication flow where internal calls might fail SigV4 verification, potential failures in leader forwarding due to auth mismatches, and session persistence issues in multi-node clusters caused by per-node JWT keys. Additionally, improvements are suggested regarding API pagination to reduce memory pressure and the enforcement of request body size limits to prevent DoS attacks.
| ### 3.1 Why re-use existing adapters instead of going direct to `store/` | ||
|
|
||
| - DynamoDB の `CreateTable` は `makeCreateTableRequest` (`adapter/dynamodb.go:926`) でテーブルスキーマを OperationGroup にエンコードする非自明なロジックを含む。S3 の `CreateBucket` も `s3BucketMeta` のメタキー書き込みを含む。これらを再実装すると整合性バグを招く。 | ||
| - 既存アダプタ経路を通すことで SigV4 検証・権限モデル・ACL チェックもそのまま再利用できる。 |
There was a problem hiding this comment.
There is a design contradiction between the goal of reusing SigV4 verification and the proposed authentication flow. Since the secret key is not stored on the server (Section 6.1), the Admin handler cannot generate SigV4 signatures for internal requests. If the existing adapter handlers rely on verifying a SigV4 signature in the http.Request, they will reject these internal calls. The design should clarify how the internal calls bypass or satisfy the adapter's authentication requirements.
| ## 11. Open Questions | ||
|
|
||
| 1. **管理権限キーのローテーション**: 現状、再起動が必要。Raft ベースの動的更新は将来検討。 | ||
| 2. **マルチノード同一ダッシュボード**: 任意のノードに繋げば同じ一覧が見える (すべて Raft 経由)。リーダーへのフォワードは既存 adapter が行うので追加実装不要のはず。ただしリーダーが不在の時の UX (503) の扱いを決める必要あり。 |
There was a problem hiding this comment.
The assumption that leader forwarding will work without additional implementation is likely incorrect. The existing proxyToLeader logic expects SigV4 authentication, while the Admin Dashboard uses JWT. This mismatch creates a significant operational risk for rolling upgrades where nodes may be running different versions. Per repository guidelines, the design must detail potential mitigation strategies, such as implementing a temporary 'bridge' or 'proxy' mode to ensure compatibility during the upgrade process.
References
- When a design document identifies a significant operational risk, such as the inability to perform rolling upgrades, it must also detail potential mitigation strategies, like implementing a temporary 'bridge' or 'proxy' mode.
|
|
||
| ### 4.3 Pagination | ||
|
|
||
| - `ListTables` / `ListBuckets` は内部で既にページネーションをサポートしているが、管理 UI 側は単一リスト表示で十分なため、admin handler 側で全ページを結合して返す (最大件数 10,000 件でハードキャップ)。 |
There was a problem hiding this comment.
Fetching and merging up to 10,000 items in memory to return a single JSON response could lead to significant latency and memory pressure on the node, especially in large clusters. It is recommended to implement pagination in the Admin API itself, mirroring the underlying DynamoDB/S3 pagination, to ensure the dashboard remains responsive and stable.
| ``` | ||
|
|
||
| - 秘密鍵はサーバー側でのみ検証し、以降のリクエストに渡らない。 | ||
| - JWT シグナルキーはノードごとにランダム生成 (プロセスライフタイム)。ノード再起動で全セッションが失効するのは許容する (長期セッションは非ゴール)。 |
There was a problem hiding this comment.
Generating a random JWT signing key per node will break session persistence in a multi-node cluster. If a user logs in on Node A and their next request is routed to Node B by a load balancer, the session will be invalid. Consider using a cluster-wide shared secret or synchronizing the key via Raft to ensure session consistency.
| ## 10. Security Considerations | ||
|
|
||
| - Admin listener をパブリック IP にさらさないことを README で強く推奨。`admin.enabled` のデフォルトは `false`。 | ||
| - ログインエンドポイントにレート制限 (per-IP、5 req/min)。 |
There was a problem hiding this comment.
In addition to rate limiting, the Admin API should enforce strict request body size limits using http.MaxBytesReader for all POST/PUT endpoints. This aligns with the project's general rules for preventing Denial of Service attacks from oversized payloads.
References
- When handling HTTP requests, always limit the size of the request body (e.g., using http.MaxBytesReader) to prevent Denial of Service attacks from oversized payloads.
Addresses inline review comments from gemini-code-assist and chatgpt-codex: - Clarify SigV4 bypass: expose adapter internal entrypoints that take an AuthPrincipal so Admin's JWT sessions don't fail SigV4 verification. - Replace "merge all pages up to 10k" with real pagination surfaced to clients via next_token + limit. - Make JWT signing key cluster-shared (admin.session_signing_key) with a two-key rotation scheme; fail startup if unset. - Mandate http.MaxBytesReader (64 KiB) on all POST/PUT endpoints. - Upgrade TLS rule from "log a warning" to "fail startup on non-loopback without TLS" unless an explicit allow_plaintext_non_loopback flag is set. - Spell out leader-forwarding compatibility: principal-aware proxy variant, 503 AdminUpgradeInProgress during rolling upgrades, compat matrix tests. - Expand testing plan to cover the bypass path, rotation, TLS enforcement, and leader-forwarding compatibility.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfbf165bbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var distFS embed.FS | ||
| ``` | ||
|
|
||
| Admin handler は `dist/` を `/admin/assets/*` で配信し、すべての `/admin/*` パスは `index.html` にフォールバックする (SPA ルーティング)。 |
There was a problem hiding this comment.
The design says all /admin/* paths should fall back to index.html, but the API and health endpoints are also under /admin (/admin/api/v1/*, /admin/healthz). If this is implemented as written, API/health requests can be served HTML instead of JSON/status responses, breaking dashboard CRUD calls and health checks. Limit the fallback to non-API frontend routes (or explicitly route API/health before the catch-all).
Useful? React with 👍 / 👎.
| } | ||
| ``` | ||
|
|
||
| 同様に `adapter/s3.go` の `handleCreateBucket` を呼び、続けて ACL 指定があれば `PutBucketAcl` 相当の処理を行う。 |
There was a problem hiding this comment.
POST /admin/api/v1/s3/buckets is defined to create the bucket first and then apply ACL, so any failure in the second step (validation, forwarding, transient errors) leaves a created bucket even though the request can return an error. This creates ambiguous outcomes and retry hazards (BucketAlreadyExists on retry) for operators. The API should either validate/apply ACL atomically with creation or document/return a partial-success contract so clients can recover safely.
Useful? React with 👍 / 👎.
Addresses second-round review from chatgpt-codex: - SPA fallback (P1): explicitly order routes so /admin/api/v1/* and /admin/healthz resolve before the index.html catch-all, and pin the behavior with unit tests. Without this, API/health requests would be served HTML and break dashboard calls. - S3 bucket create (P2): define POST /admin/api/v1/s3/buckets to accept the initial ACL and commit bucket metadata + ACL in a single Raft OperationGroup, eliminating the partial-success state where a retry would hit BucketAlreadyExists.
|
＠codex eview |
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f273209855
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| └─────────────┘ | ||
| ``` | ||
|
|
||
| - **Admin HTTP handler** (新規): 既存の `main.go` が起動する HTTP listener に `/admin/*` を追加する。 |
There was a problem hiding this comment.
Section 3 says /admin/* is added to the existing main.go HTTP listener, but Section 7 later defines a separate admin.listen endpoint with its own TLS enforcement and startup-fail rules. This inconsistency can lead to an implementation that serves admin APIs on public DynamoDB/S3 ports without the intended admin-specific guardrails, which weakens the security model and makes deployment behavior ambiguous. Please pick one listener topology and update the other sections to match it.
Useful? React with 👍 / 👎.
Addresses codex P2 review: Section 3 previously said /admin/* is added to the existing main.go HTTP listener, while Section 7 defined a separate admin.listen with admin-specific guardrails. Unify on a dedicated admin listener (default 127.0.0.1:8080), and spell out why it is kept separate from the DynamoDB/S3 data-plane listeners (avoids leaking admin-only guardrails to public data-plane ports and supports the common pattern of exposing data plane while keeping admin loopback-only).
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 272e45b1bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 2. **マルチノード同一ダッシュボード / リーダーフォワーディング互換性**: 既存 `proxyToLeader` は **follower で受けた SigV4 リクエストを gRPC の internal proxy でリーダーへ転送する**前提になっている。Admin の内部エントリポイント (Section 3.2) は SigV4 を経由しないため、follower で Admin リクエストを受けた場合にそのまま adapter 呼び出しをするとリーダー以外で書き込みが試みられる。 | ||
| 採用予定の緩和策: |
There was a problem hiding this comment.
In a multi-node deployment with a load balancer, admin write requests will regularly hit followers, and this section already notes that current proxyToLeader behavior assumes SigV4 while the admin path bypasses SigV4. Leaving this as an open question means the core correctness path for write endpoints is undefined and can lead to failed writes (or incorrect local write attempts) on follower nodes; this needs to be a required implementation item with explicit acceptance criteria before shipping write-capable admin APIs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/2026_04_24_proposed_admin_dashboard.md`:
- Around line 112-125: The endpoint table in Section 4.1 is missing the admin
login route; add a row for POST /admin/api/v1/auth/login (as defined in Section
6.1) with a brief description like "管理者ログイン — 認証トークン/セッションを発行" so the public
admin API contract includes the login endpoint; ensure the exact path POST
/admin/api/v1/auth/login and the concise description are added to the same table
format used for other routes.
- Around line 50-72: The markdown contains fenced code blocks without a language
tag (triggers MD040); update each fence that wraps the ASCII diagram (the block
starting with "┌─────────────────────────────────────────────────────────────┐")
and the block that starts with "1. POST /admin/api/v1/auth/login" to include a
language specifier (e.g., add ```text instead of ```), ensuring both opening
fences are changed so CI markdown lint no longer flags MD040.
- Around line 254-256: 設計で「ログインレスポンスで csrf_token をボディで返し、SPA が localStorage
に保管、全書き込みリクエストに付与する」記述は XSS 時にトークン盗難リスクが高いため、localStorage を使わない方針に改めてください。具体的には
csrf_token のクライアント保存を localStorage から削除し、代わりに double-submit cookie
パターンを採用する旨を明記し（X-Admin-CSRF ヘッダは維持）、ログイン時に SameSite=Strict/Lax + Secure 属性が付与された
CSRF cookie を発行し、SPA はその cookie を読み取り X-Admin-CSRF ヘッダに設定してサーバ側で cookie
とヘッダの一致を検証する（cookie を HttpOnly にする場合は別途サーバ側検証／CSRFトークン埋め込み戦略に切り替える旨も明記する）。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0b67c3e-8036-4b14-9040-9078f73f2afb
📒 Files selected for processing (1)
docs/design/2026_04_24_proposed_admin_dashboard.md
…ment Addresses codex P1 review: follower-write forwarding was left as an open question, which leaves the core correctness path for admin write APIs undefined. Promote it out of Open Questions into a dedicated Section 3.3 (Required, not optional) that specifies: - the principal-aware internal gRPC AdminForward RPC (separate from the existing SigV4-based proxyToLeader), - that the leader re-validates authorization rather than trusting the forwarded principal, - six explicit acceptance criteria (leader direct / follower transparent forward / leader-unavailable retry / re-auth on stale principal / rolling-upgrade compatibility matrix / audit trail) that must all be green before write-capable admin APIs can ship. Also mark P1 DoD in the implementation plan as blocked on those criteria and add the forwarding work to the P1 scope line.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 840f3c5913
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| read_only_access_keys = ["AKIA_READONLY_1"] | ||
| full_access_keys = ["AKIA_ADMIN_1"] | ||
| ``` |
There was a problem hiding this comment.
The role model does not define what should happen when the same access key appears in both read_only_access_keys and full_access_keys, so an operator misconfiguration can silently grant write permissions to a key intended to be read-only depending on implementation lookup order. Add an explicit startup validation rule (fail fast on overlap) to keep authorization deterministic and prevent accidental privilege escalation.
Useful? React with 👍 / 👎.
| server: signs a short-lived (1h) JWT with HS256 using a server-side | ||
| admin signing key. Returns JWT in an httpOnly cookie. | ||
|
|
There was a problem hiding this comment.
The login flow currently specifies only an httpOnly JWT cookie, which leaves cookie transport policy undefined; without explicitly requiring Secure (and a restrictive SameSite mode), session tokens can be exposed when plaintext admin access is enabled via opt-out or when browsers perform cross-site requests. The design should mandate these attributes so implementations don’t default to weaker cookie security.
Useful? React with 👍 / 👎.
Addresses round-5 review feedback: - CodeRabbit (MD040): add `text` language tag to ASCII-diagram and login-flow fenced code blocks so markdownlint passes. - CodeRabbit (minor): list POST /admin/api/v1/auth/login and /auth/logout in the Section 4.1 endpoint contract table so the admin API shape stays complete. - CodeRabbit (major) + codex P2: remove the localStorage CSRF scheme (XSS-exposed) and adopt double-submit cookie with Secure + SameSite=Strict; pair with a same-origin session cookie that is always HttpOnly + Secure + SameSite=Strict regardless of TLS mode. - codex P2: make overlap between admin.read_only_access_keys and admin.full_access_keys a hard startup failure rather than an implementation-order-dependent silent fallback.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Proposes an embedded admin UI (Go + React SPA) that reuses the existing DynamoDB and S3 adapter handlers in-process, so no new write paths are introduced. Covers API shape, SigV4-backed auth with read-only/full roles, CSRF, phased rollout, testing strategy, and rejected alternatives.
Summary by CodeRabbit