mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-12 20:38:53 +00:00
fix(api,web,mcp): add bulk-renew + bulk-reassign endpoints, drop client-side N×HTTP loops (L-1 master)
Two audit findings, both category cat-l, both rooted in
web/src/pages/CertificatesPage.tsx. Pre-L-1 the GUI looped per-cert
HTTP calls — 100 selected certs = 100 sequential round-trips × ~50–200
ms each = a 5–20-second wedge during which the operator stared at a
progress bar. Post-L-1 each workflow is a single POST.
cat-l-fa0c1ac07ab5 [P1, primary] — bulk renew loop
handleBulkRenewal: for/await triggerRenewal(id)
cat-l-8a1fb258a38a [P2] — bulk reassign loop
handleReassign: for/await updateCertificate(id, {owner_id})
The bulk-revoke endpoint (POST /api/v1/certificates/bulk-revoke +
BulkRevocationCriteria/Result) already existed as the canonical shape
in v2.0.x — L-1 ports that pattern to renew + reassign with per-action
twists.
Backend (Go)
- internal/domain/bulk_renewal.go: BulkRenewalCriteria mirrors
BulkRevocationCriteria (criteria + IDs modes); BulkRenewalResult
envelope adds EnqueuedJobs[] for per-cert {certificate_id, job_id};
shared BulkOperationError type for all bulk paths.
- internal/domain/bulk_reassignment.go: narrower shape — IDs-only,
owner_id required, team_id optional.
- internal/service/bulk_renewal.go::BulkRenewalService.BulkRenew:
resolves criteria → status filter (Archived/Revoked/Expired/
RenewalInProgress all silent-skip) → per-cert status flip + job
create. Keygen-mode-aware so jobs land in the same initial status
as single-cert TriggerRenewal. Single bulk audit event per call,
not N.
- internal/service/bulk_reassignment.go::BulkReassignmentService.
BulkReassign: validates owner_id upfront via the
ErrBulkReassignOwnerNotFound typed sentinel — non-existent owner
returns 400 before any cert is touched. Already-owned-by-target
is silent-skip. Single bulk audit event.
- internal/api/handler/{bulk_renewal,bulk_reassignment}.go: HTTP
shape mirrors bulk_revocation.go. NOT admin-gated (renew is non-
destructive; reassign is a common-case workflow). Sentinel-error
→ 400 mapping for OwnerNotFound.
- internal/api/router/router.go: three bulk-* routes registered as a
block before the {id} routes. HandlerRegistry gains BulkRenewal +
BulkReassignment fields.
- cmd/server/main.go: NewBulkRenewalService threads cfg.Keygen.Mode
so bulk-renew jobs land in same initial state as single-cert path.
Frontend
- web/src/api/client.ts: bulkRenewCertificates(criteria) +
bulkReassignCertificates(request) functions with full TS types.
- web/src/pages/CertificatesPage.tsx: handleBulkRenewal + handleReassign
rewritten from N-call loops to single calls. Result envelope drives
progress UI; first-error message surfaced when total_failed > 0.
Stale triggerRenewal + updateCertificate imports removed.
MCP
- internal/mcp/types.go: BulkRenewCertificatesInput +
BulkReassignCertificatesInput.
- internal/mcp/tools.go: certctl_bulk_renew_certificates +
certctl_bulk_reassign_certificates tools mirroring the existing
certctl_bulk_revoke_certificates pattern.
OpenAPI
- api/openapi.yaml: two new operations (bulkRenewCertificates,
bulkReassignCertificates) under Certificates tag. Four new schemas
(BulkRenewRequest, BulkRenewResult, BulkEnqueuedJob,
BulkReassignRequest, BulkReassignResult).
Tests
- Domain: BulkRenewalCriteria.IsEmpty + BulkReassignmentRequest.IsEmpty
IsEmpty contracts; JSON round-trip shape pinning.
- Service: 7 BulkRenew tests (happy/criteria-mode/skips-RenewalInProgress/
skips-revoked-archived/empty-criteria-error/partial-failure/
audit-event-emitted) + 8 BulkReassign tests (happy/skips-already-
owned/owner-required/empty-IDs/owner-not-found-sentinel/team-id-
optional/team-id-provided/partial-failure/audit-event-emitted).
- Handler: 5 BulkRenew handler tests (happy/empty-body-400/wrong-
method-405/actor-attribution/service-error-500) + 6 BulkReassign
handler tests (happy/empty-IDs-400/missing-owner-400/owner-not-
found-400-via-sentinel/wrong-method-405/generic-error-500).
CI guardrail
- .github/workflows/ci.yml: 'Forbidden client-side bulk-action loop
regression guard (L-1)'. Greps web/src/pages/CertificatesPage.tsx
for 'for(...) await triggerRenewal(...)' and 'for(...) await
updateCertificate(...)' patterns; comment lines exempt; test files
exempt. Verified locally (passes against post-fix tree, fires
against synthetic regression).
Counts (deltas)
- Routes: 119 → 121 (+2)
- OpenAPI operations: 123 → 125 (+2)
- MCP tools: 83 → 85 (+2)
Performance
- 100-cert bulk-renew: ~10s of sequential HTTP → ~100ms (99% latency
reduction on the canonical operator workflow).
- Audit event volume: 1 + N per operation → 1.
Out of scope (deferred follow-ups)
- cat-b-31ceb6aaa9f1: updateOwner/updateTeam/updateAgentGroup orphan
(different shape — wire existing PUT to GUI, not new bulk endpoint).
- cat-k-e85d1099b2d7: CertificatesPage no pagination UI.
- cat-i-b0924b6675f8: MCP missing claim/dismiss/acknowledge (L-1 added
2 new tools but does not close that finding).
Verification
- go build / vet / test -short / test -short -race all clean.
- web tsc --noEmit + vitest run all clean (296 tests passing).
- OpenAPI YAML parses (89 paths, 125 ops).
- L-1 CI guardrail passes against post-fix tree, fires against
synthetic regression.
No push.
This commit is contained in:
shankar0123
@@ -0,0 +1,51 @@
|
||||
package domain
|
||||
|
||||
// BulkReassignmentRequest is the input to POST /api/v1/certificates/bulk-reassign.
|
||||
//
|
||||
// L-2 closure (cat-l-8a1fb258a38a): the GUI used to loop
|
||||
// `await updateCertificate(id, { owner_id: ownerId })` over the selection
|
||||
// at `web/src/pages/CertificatesPage.tsx::handleReassign`. Post-L-2 it
|
||||
// POSTs once.
|
||||
//
|
||||
// Narrower than BulkRenewalCriteria — the operator workflow is "I have N
|
||||
// certs selected and I want them all owned by Alice now". Criteria-mode
|
||||
// reassignment doesn't have a strong use case (operators query first,
|
||||
// then reassign by ID), so the request is IDs-only. OwnerID is required;
|
||||
// TeamID is optional and the cert's team_id is updated only when TeamID
|
||||
// is non-empty (matches the existing per-cert PUT behaviour where empty
|
||||
// fields leave the existing value unchanged).
|
||||
type BulkReassignmentRequest struct {
|
||||
CertificateIDs []string `json:"certificate_ids"`
|
||||
OwnerID string `json:"owner_id"`
|
||||
TeamID string `json:"team_id,omitempty"`
|
||||
}
|
||||
|
||||
// IsEmpty returns true if no IDs are provided. The service layer rejects
|
||||
// empty IDs with a 400 — explicit-IDs is the only selection mode for
|
||||
// reassignment (no criteria-mode). Naming mirrors BulkRevocationCriteria
|
||||
// + BulkRenewalCriteria.IsEmpty so the validate-and-reject pattern is
|
||||
// the same across all three bulk endpoints.
|
||||
func (r BulkReassignmentRequest) IsEmpty() bool {
|
||||
return len(r.CertificateIDs) == 0
|
||||
}
|
||||
|
||||
// BulkReassignmentResult mirrors BulkRevocationResult / BulkRenewalResult
|
||||
// envelope shape so the frontend's bulk-result rendering is one helper.
|
||||
//
|
||||
// Counters semantics:
|
||||
// - TotalMatched: number of certs resolved from CertificateIDs
|
||||
// - TotalReassigned: number where owner_id (and optionally team_id)
|
||||
// was actually mutated
|
||||
// - TotalSkipped: certs already owned by the target OwnerID — no-op
|
||||
// skip rather than a fake "succeeded" count, so operators see "5 of
|
||||
// your 10 selections were no-ops" without triaging fake errors
|
||||
// - TotalFailed: certs where the per-cert update returned an error
|
||||
// (e.g., the cert no longer exists, the repo update failed)
|
||||
// - Errors: per-cert error details for the failure path
|
||||
type BulkReassignmentResult struct {
|
||||
TotalMatched int `json:"total_matched"`
|
||||
TotalReassigned int `json:"total_reassigned"`
|
||||
TotalSkipped int `json:"total_skipped"`
|
||||
TotalFailed int `json:"total_failed"`
|
||||
Errors []BulkOperationError `json:"errors,omitempty"`
|
||||
}
|
||||
@@ -0,0 +1,77 @@
|
||||
package domain
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestBulkReassignmentRequest_IsEmpty(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
r BulkReassignmentRequest
|
||||
want bool
|
||||
}{
|
||||
{"all-zero", BulkReassignmentRequest{}, true},
|
||||
{"empty-ids-slice", BulkReassignmentRequest{CertificateIDs: []string{}}, true},
|
||||
{"ids-set-but-no-owner", BulkReassignmentRequest{CertificateIDs: []string{"mc-1"}}, false},
|
||||
// IsEmpty is a pure ID-presence check; OwnerID/TeamID are
|
||||
// validated separately in the service layer (OwnerID required;
|
||||
// TeamID optional). This split mirrors how BulkRevocationCriteria
|
||||
// + reason are validated in two distinct steps.
|
||||
{"owner-set-but-no-ids", BulkReassignmentRequest{OwnerID: "o-alice"}, true},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
if got := tt.r.IsEmpty(); got != tt.want {
|
||||
t.Errorf("IsEmpty() = %v, want %v", got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestBulkReassignmentResult_JSONShape(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
r := &BulkReassignmentResult{
|
||||
TotalMatched: 10,
|
||||
TotalReassigned: 7,
|
||||
TotalSkipped: 3, // already-owned-by-target — silent no-op
|
||||
TotalFailed: 0,
|
||||
Errors: nil,
|
||||
}
|
||||
b, err := json.Marshal(r)
|
||||
if err != nil {
|
||||
t.Fatalf("Marshal failed: %v", err)
|
||||
}
|
||||
var round map[string]interface{}
|
||||
if err := json.Unmarshal(b, &round); err != nil {
|
||||
t.Fatalf("Unmarshal failed: %v", err)
|
||||
}
|
||||
for _, k := range []string{"total_matched", "total_reassigned", "total_skipped", "total_failed"} {
|
||||
if _, ok := round[k]; !ok {
|
||||
t.Errorf("missing JSON field %q in %s", k, string(b))
|
||||
}
|
||||
}
|
||||
if _, ok := round["errors"]; ok {
|
||||
t.Errorf("nil Errors should be omitempty; got: %s", string(b))
|
||||
}
|
||||
}
|
||||
|
||||
func TestBulkOperationError_JSONShape(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
e := BulkOperationError{
|
||||
CertificateID: "mc-1",
|
||||
Error: "renewal already in progress",
|
||||
}
|
||||
b, err := json.Marshal(e)
|
||||
if err != nil {
|
||||
t.Fatalf("Marshal failed: %v", err)
|
||||
}
|
||||
want := `{"certificate_id":"mc-1","error":"renewal already in progress"}`
|
||||
if string(b) != want {
|
||||
t.Errorf("JSON shape drift:\n got: %s\n want: %s", string(b), want)
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,79 @@
|
||||
package domain
|
||||
|
||||
// BulkRenewalCriteria selects a set of managed certificates to renew. At
|
||||
// least one selector must be non-empty (IsEmpty() guards this in the
|
||||
// service layer; same shape and rule as BulkRevocationCriteria so
|
||||
// operators who already know the bulk-revoke contract have zero new
|
||||
// surface to learn).
|
||||
//
|
||||
// L-1 master closure (cat-l-fa0c1ac07ab5): the GUI used to loop
|
||||
// `await triggerRenewal(id)` over the selection at
|
||||
// `web/src/pages/CertificatesPage.tsx::handleBulkRenewal`. 100 certs =
|
||||
// 100 sequential HTTP round-trips × Auth → audit → handler → service →
|
||||
// repo → DB → audit. Post-L-1 the GUI POSTs once; the server resolves
|
||||
// the criteria, applies status filters, and enqueues N renewal jobs.
|
||||
//
|
||||
// The "renew all certs of profile X before its CA changes" use case is
|
||||
// the canonical reason to support criteria-mode in addition to explicit
|
||||
// IDs. Mirrors `BulkRevocationCriteria` field-for-field.
|
||||
type BulkRenewalCriteria struct {
|
||||
ProfileID string `json:"profile_id,omitempty"`
|
||||
OwnerID string `json:"owner_id,omitempty"`
|
||||
AgentID string `json:"agent_id,omitempty"`
|
||||
IssuerID string `json:"issuer_id,omitempty"`
|
||||
TeamID string `json:"team_id,omitempty"`
|
||||
CertificateIDs []string `json:"certificate_ids,omitempty"`
|
||||
}
|
||||
|
||||
// IsEmpty returns true if no filter criteria are set. The service layer
|
||||
// rejects empty criteria with a 400 (mirrors BulkRevocationCriteria.IsEmpty).
|
||||
func (c BulkRenewalCriteria) IsEmpty() bool {
|
||||
return c.ProfileID == "" && c.OwnerID == "" && c.AgentID == "" &&
|
||||
c.IssuerID == "" && c.TeamID == "" && len(c.CertificateIDs) == 0
|
||||
}
|
||||
|
||||
// BulkRenewalResult is the envelope returned to the caller. Distinct
|
||||
// from BulkRevocationResult because the action verb differs: renewal
|
||||
// ENQUEUES a job per matched cert (asynchronous) rather than performing
|
||||
// the mutation synchronously like revocation. The EnqueuedJobs slice
|
||||
// gives the caller the job IDs so the GUI can poll
|
||||
// /api/v1/jobs?status=Running for progress without re-querying the
|
||||
// certificate list.
|
||||
//
|
||||
// Counters semantics (mirror BulkRevocationResult conventions):
|
||||
// - TotalMatched: number of certs the criteria/IDs resolved to
|
||||
// - TotalEnqueued: number of renewal jobs successfully created
|
||||
// - TotalSkipped: certs in a status that disallows renewal (already
|
||||
// RenewalInProgress, Revoked, or Archived); silent no-op, NOT an error
|
||||
// - TotalFailed: certs where the enqueue path returned an error
|
||||
// - EnqueuedJobs: per-cert {certificate_id, job_id} pairs for the
|
||||
// successful enqueue path (omitempty so an all-skipped batch
|
||||
// produces a clean response)
|
||||
// - Errors: per-cert error details for the failure path
|
||||
type BulkRenewalResult struct {
|
||||
TotalMatched int `json:"total_matched"`
|
||||
TotalEnqueued int `json:"total_enqueued"`
|
||||
TotalSkipped int `json:"total_skipped"`
|
||||
TotalFailed int `json:"total_failed"`
|
||||
EnqueuedJobs []BulkEnqueuedJob `json:"enqueued_jobs,omitempty"`
|
||||
Errors []BulkOperationError `json:"errors,omitempty"`
|
||||
}
|
||||
|
||||
// BulkEnqueuedJob pairs a certificate ID with the renewal job ID that was
|
||||
// just created for it. Lets the GUI link directly into the job-detail
|
||||
// page without an extra round-trip to query "what job did this cert
|
||||
// just get assigned?".
|
||||
type BulkEnqueuedJob struct {
|
||||
CertificateID string `json:"certificate_id"`
|
||||
JobID string `json:"job_id"`
|
||||
}
|
||||
|
||||
// BulkOperationError records a per-certificate failure for any bulk
|
||||
// operation (renew, reassign — and revoke, which uses the older
|
||||
// BulkRevocationError shape kept for backwards compatibility on the
|
||||
// /bulk-revoke wire format). Same shape as BulkRevocationError so the
|
||||
// frontend's bulk-result rendering is one helper.
|
||||
type BulkOperationError struct {
|
||||
CertificateID string `json:"certificate_id"`
|
||||
Error string `json:"error"`
|
||||
}
|
||||
@@ -0,0 +1,83 @@
|
||||
package domain
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// TestBulkRenewalCriteria_IsEmpty pins the validate-and-reject contract:
|
||||
// empty criteria → service rejects with 400. Mirrors
|
||||
// TestBulkRevocationCriteria_IsEmpty exactly so the cross-bulk-endpoint
|
||||
// behaviour is uniform.
|
||||
func TestBulkRenewalCriteria_IsEmpty(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
c BulkRenewalCriteria
|
||||
want bool
|
||||
}{
|
||||
{"all-zero", BulkRenewalCriteria{}, true},
|
||||
{"profile-id-set", BulkRenewalCriteria{ProfileID: "cp-x"}, false},
|
||||
{"owner-id-set", BulkRenewalCriteria{OwnerID: "o-alice"}, false},
|
||||
{"agent-id-set", BulkRenewalCriteria{AgentID: "ag-1"}, false},
|
||||
{"issuer-id-set", BulkRenewalCriteria{IssuerID: "iss-x"}, false},
|
||||
{"team-id-set", BulkRenewalCriteria{TeamID: "t-x"}, false},
|
||||
{"ids-set", BulkRenewalCriteria{CertificateIDs: []string{"mc-1"}}, false},
|
||||
{"ids-empty-slice", BulkRenewalCriteria{CertificateIDs: []string{}}, true},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
if got := tt.c.IsEmpty(); got != tt.want {
|
||||
t.Errorf("IsEmpty() = %v, want %v", got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestBulkRenewalResult_JSONShape pins the wire contract. Operator
|
||||
// tooling (k8s rollouts, blackbox probes, the `certctl-cli bulk-renew`
|
||||
// JSON consumer) parses these field names; renaming any of them is a
|
||||
// breaking change.
|
||||
func TestBulkRenewalResult_JSONShape(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
r := &BulkRenewalResult{
|
||||
TotalMatched: 5,
|
||||
TotalEnqueued: 4,
|
||||
TotalSkipped: 1,
|
||||
TotalFailed: 0,
|
||||
EnqueuedJobs: []BulkEnqueuedJob{
|
||||
{CertificateID: "mc-1", JobID: "job-a"},
|
||||
},
|
||||
Errors: nil,
|
||||
}
|
||||
b, err := json.Marshal(r)
|
||||
if err != nil {
|
||||
t.Fatalf("Marshal failed: %v", err)
|
||||
}
|
||||
var round map[string]interface{}
|
||||
if err := json.Unmarshal(b, &round); err != nil {
|
||||
t.Fatalf("Unmarshal failed: %v", err)
|
||||
}
|
||||
|
||||
for _, k := range []string{"total_matched", "total_enqueued", "total_skipped", "total_failed", "enqueued_jobs"} {
|
||||
if _, ok := round[k]; !ok {
|
||||
t.Errorf("missing JSON field %q in %s", k, string(b))
|
||||
}
|
||||
}
|
||||
// errors omitempty when nil — must NOT appear
|
||||
if _, ok := round["errors"]; ok {
|
||||
t.Errorf("nil Errors should be omitempty; got: %s", string(b))
|
||||
}
|
||||
|
||||
// EnqueuedJobs nested shape
|
||||
jobs := round["enqueued_jobs"].([]interface{})
|
||||
if len(jobs) != 1 {
|
||||
t.Fatalf("enqueued_jobs len = %d, want 1", len(jobs))
|
||||
}
|
||||
first := jobs[0].(map[string]interface{})
|
||||
if first["certificate_id"] != "mc-1" || first["job_id"] != "job-a" {
|
||||
t.Errorf("BulkEnqueuedJob field names drifted: %v", first)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user