From ecbe2eda798cb047897ff4914ca494dc3bba2215 Mon Sep 17 00:00:00 2001 From: Soham Shirke <129677811+SShogun@users.noreply.github.com> Date: Mon, 21 Sep 2026 00:06:21 +0530 Subject: [PATCH 1/2] hardening: align lifecycle, CI, and architecture claims --- .github/workflows/go.yml | 26 ++++++-- ARCHITECTURE_STATUS.md | 66 +++++++++++++++++++++ README.md | 74 +++++++++++------------ cmd/web/main.go | 74 +++++++++++++++++++---- cmd/web/main_test.go | 124 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 309 insertions(+), 55 deletions(-) create mode 100644 ARCHITECTURE_STATUS.md create mode 100644 cmd/web/main_test.go diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 189c906..b16b590 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -1,10 +1,16 @@ name: CI -on: [push, pull_request] +on: + push: + pull_request: + +permissions: + contents: read jobs: test-and-build: runs-on: ubuntu-latest + timeout-minutes: 20 services: postgres: @@ -28,7 +34,11 @@ jobs: - name: Set up Go uses: actions/setup-go@v6 with: - go-version: 'stable' + go-version-file: go.mod + cache: true + + - name: Download modules + run: go mod download - name: Apply migrations env: @@ -38,13 +48,19 @@ jobs: psql -h localhost -U testuser -d controlplane_test -f "$f" done - - name: Run Go tests + - name: Check formatting + run: test -z "$(gofmt -l .)" + + - name: Vet + run: go vet ./... + + - name: Test with race detector env: TEST_DATABASE_URL: postgres://testuser:testpassword@localhost:5432/controlplane_test?sslmode=disable - run: go test -v -count=1 ./... + run: go test -race -count=1 ./... - name: Build application - run: go build -v ./... + run: go build ./... - name: Run golangci-lint uses: golangci/golangci-lint-action@v9 diff --git a/ARCHITECTURE_STATUS.md b/ARCHITECTURE_STATUS.md new file mode 100644 index 0000000..eed8a35 --- /dev/null +++ b/ARCHITECTURE_STATUS.md @@ -0,0 +1,66 @@ +# ControlPlane Architecture Status + +This document describes the repository as it exists today. It is intentionally narrower than the project name: ControlPlane is currently a **server-rendered Go application for internal knowledge and governance workflows**, not a Kubernetes-style controller or a separate distributed control-plane/data-plane system. + +## Current Shape + +The application is one Go process built around `net/http` and `chi`. + +Request flow: + +```text +HTTP request + -> chi middleware + -> session load/save + -> CSRF protection + -> authentication / role checks + -> handler + -> UserStore interface + -> PostgreSQL + -> server-rendered HTML response +``` + +PostgreSQL is the primary durable store. The same database also backs server-side sessions through `scs/pgxstore`. + +## Implemented + +- Email/password authentication with bcrypt verification. +- PostgreSQL-backed sessions and secure-cookie behavior in production mode. +- CSRF protection on the web application. +- Role checks for authenticated, reviewer, and admin routes. +- Notebook creation, revision history, tags, search, submission, and deletion flows. +- Reviewer approval/rejection workflows. +- Transactional approval/rejection persistence with approval records and audit events. +- Moderation flags and admin audit/team workflows. +- Graceful HTTP shutdown on `SIGINT`/`SIGTERM`, followed by PostgreSQL pool cleanup. +- Handler tests using fake stores and `httptest`. +- PostgreSQL integration tests using `TEST_DATABASE_URL`. +- GitHub Actions verification with migrations, formatting, vet, race-enabled tests, build, and golangci-lint. + +## Important Runtime Semantics + +Approval and rejection use explicit PostgreSQL transactions. Their revision state change, approval record, and audit event are committed together. + +Normal handler-side audit calls outside those transaction methods are synchronous database writes. They are not an external event bus and do not provide cross-service delivery guarantees. + +The application currently runs as a single process. Session state is centralized in PostgreSQL, but there is no replica coordination layer for rate limiting or background work. + +## Not Implemented Yet + +The following are future work and should not be inferred from the repository name or architecture diagrams: + +- Prometheus metrics endpoint. +- OpenTelemetry tracing. +- Production Docker image and Kubernetes manifests. +- `/healthz` and `/readyz` probe endpoints. +- Redis-backed distributed rate limiting. +- gRPC or external service-to-service APIs. +- A separate control-plane service and data-plane service. +- Kubernetes reconciliation/controller behavior. +- Distributed consensus, leader election, or multi-region coordination. + +## Verification Boundary + +CI proves that the checked-in migrations apply to PostgreSQL and that the Go code passes formatting, vet, race-enabled tests, build, and lint checks. + +The repository does **not** currently claim production deployment validation, load-test SLOs, multi-node failover testing, or Kubernetes operational readiness. diff --git a/README.md b/README.md index 4ada67c..56e8ca5 100644 --- a/README.md +++ b/README.md @@ -82,9 +82,9 @@ This is not a blog, not a todo app, and not a public wiki. It is an operational ## Current Status -> **Phase 5 — Production Hardening** (Complete) +> **Phase 5 — Repository Hardening** (CI-verified) -Phase 5 test files, integration tests, rate limiting, and CI pipelines are now complete. The backend is fully hardened for production use. +Phase 5 covers application-level hardening: tests, CI, lifecycle handling, and documentation/implementation parity. This is **not** a claim of production deployment readiness; metrics/tracing, container/Kubernetes deployment, health/readiness probes, and shared rate limiting remain future work. See [ARCHITECTURE_STATUS.md](./ARCHITECTURE_STATUS.md) for the as-built boundary. | Phase | Description | Status | |-------|-------------|--------| @@ -93,7 +93,7 @@ Phase 5 test files, integration tests, rate limiting, and CI pipelines are now c | Phase 2 | Forms, validation, sessions, auth, context | ✅ Complete | | Phase 3 | Notebooks, revisions, tags, search, DB-backed pages | ✅ Complete | | Phase 4 | Approvals, audit, moderation, governance | ✅ Complete | -| Phase 5 | Tests, CI, production hardening | ✅ Complete | +| Phase 5 | Tests, CI, graceful lifecycle, repository hardening | ✅ Complete | | Phase 6 | Advanced: Metrics, Tracing, Docker/K8s, Shared Rate Limiting | 🚧 Planned | --- @@ -103,7 +103,7 @@ Phase 5 test files, integration tests, rate limiting, and CI pipelines are now c The architecture is intentionally conservative and explicit: - `main.go` builds the long-lived dependencies once -- an `Application` struct carries shared dependencies (config, DB pool, session manager, template cache) +- an `Application` struct carries config, logger, `UserStore`, session manager, and template cache; `run()` owns the PostgreSQL pool lifecycle - `app.routes()` builds the router and middleware graph - handlers are methods on `app` - request-scoped data lives in `*http.Request` and `Context`, not on global variables @@ -147,7 +147,7 @@ Browser | Concept | Scope | Storage | Example | |---------|-------|---------|---------| -| Application | server lifetime | process memory | DB pool, session manager, template cache | +| Process dependencies | server lifetime | `run()` + `Application` | PostgreSQL pool, store, session manager, template cache | | Session | many requests | DB/cookie-backed | `userID`, flash message | | Context | one request | memory | current loaded user pointer | @@ -207,7 +207,8 @@ ControlPlane/ │ └── audit.sql.go ├── migrations/ │ ├── 0001_initial_schema.sql # Core tables, indexes, constraints -│ └── 0002_create_audit_logs.sql # Audit log table +│ ├── 0002_create_audit_logs.sql # Audit log table +│ └── 0003_create_sessions_table.sql # scs PostgreSQL session table ├── ui/templates/ # Server-rendered HTML templates ├── docs/ # Architecture and study documentation │ ├── Control Plane Notebook Architecture.md @@ -222,7 +223,7 @@ ControlPlane/ ├── sqlc.yaml # sqlc configuration ├── go.mod ├── go.sum -├── .env +├── ARCHITECTURE_STATUS.md └── .gitignore ``` @@ -237,7 +238,6 @@ The `Application` struct is dependency injection in plain Go. It avoids globals, ```go type Application struct { config Config - conn *pgxpool.Pool logger *slog.Logger store data.UserStore sessionManager *scs.SessionManager @@ -260,7 +260,8 @@ type UserStore interface { ListNotebookRevisions(ctx context.Context, notebookID int) ([]NotebookRevision, error) ListNotebookTags(ctx context.Context, notebookID int) ([]Tag, error) InsertAuditLog(ctx context.Context, params InsertAuditLogParams) error - ApproveRevisionTx(ctx context.Context, revisionID, notebookID, reviewerID int) error + ApproveRevisionTx(ctx context.Context, revisionID, notebookID, reviewerID int, note string) error + RejectRevisionTx(ctx context.Context, revisionID, reviewerID int, note string) error ListSubmittedRevisions(ctx context.Context) ([]NotebookRevision, error) UpdateRevisionStatus(ctx context.Context, params UpdateRevisionStatusParams) error // ... and more @@ -417,6 +418,7 @@ Without this split, drafts and approvals become fragile because you keep overwri | `GET` | `/notebooks/{id}/edit` | `notebookEditForm` | Edit draft form | | `POST` | `/notebooks/{id}/edit` | `notebookEditSubmit` | Submit draft edit | | `POST` | `/notebooks/{id}/submit` | `notebookSubmitForApproval` | Submit latest draft revision for review | +| `POST` | `/notebooks/{id}/delete` | `notebookDeleteSubmit` | Delete notebook | | `POST` | `/notebooks/{id}/flag` | `notebookFlagSubmit` | Create a moderation flag | | `GET` | `/notebooks/search` | `notebooksSearch` | Search notebooks | @@ -425,6 +427,7 @@ Without this split, drafts and approvals become fragile because you keep overwri | Method | Path | Handler | Description | |--------|------|---------|-------------| | `GET` | `/approvals` | `approvalQueue` | Submitted revisions queue | +| `GET` | `/approvals/{id}` | `approvalReviewView` | Review one submitted revision | | `POST` | `/approvals/approve` | `approveRevisionSubmit` | Approve and publish revision | | `POST` | `/approvals/reject` | `rejectRevisionSubmit` | Reject revision | | `GET` | `/moderation` | `moderationQueue` | Moderation flag queue | @@ -434,6 +437,9 @@ Without this split, drafts and approvals become fragile because you keep overwri | Method | Path | Handler | Description | |--------|------|---------|-------------| +| `GET` | `/admin/teams` | `adminTeams` | Team administration page | +| `POST` | `/admin/teams` | `adminTeamOnboardSubmit` | Onboard team/membership data | +| `POST` | `/admin/notebooks/{id}/delete` | `adminNotebookDeleteSubmit` | Administrative notebook deletion | | `GET` | `/admin/audit` | `adminAudit` | Audit event log | --- @@ -465,8 +471,12 @@ serveWithSession(app, handler, request) | `middleware_test.go` | Auth redirect, role enforcement | 3 | | `auth_handlers_test.go` | Login form, credentials, logout | 4 | | `notebook_handlers_test.go` | Create draft, view, validation | 4 | -| `approval_handlers_test.go` | Approve/reject transactions | 2 | -| `handlers_test.go` | Full handler integration tests | 10+ | +| `approval_handlers_test.go` | Approve/reject handler behavior | focused | +| `csrf_test.go` | CSRF enforcement and form behavior | focused | +| `ui_smoke_test.go` | Representative rendered routes | smoke | +| `main_test.go` | Graceful server lifecycle | 3 | +| `handlers_test.go` | Broader handler coverage | legacy/focused | +| `internal/data/store_test.go` | Real PostgreSQL data-path checks | integration | ### Running Tests @@ -478,31 +488,16 @@ go test -v ./... ## CI/CD Pipeline -GitHub Actions runs on every push to `main` and on pull requests: +GitHub Actions runs on all pushes and pull requests. The repository default branch is `master`, and the workflow is not restricted to a stale `main` branch filter: ```yaml -name: CI Pipeline - -on: - push: - branches: ["main"] - pull_request: - branches: ["main"] - -jobs: - test-and-build: - runs-on: ubuntu-latest - steps: - - name: Check out repository code - uses: actions/checkout@v4 - - name: Set up Go - uses: actions/setup-go@v5 - with: - go-version: '1.25.0' - - name: Run Go tests - run: go test -v ./... - - name: Build application - run: go build -v ./... +name: CI + +on: [push, pull_request] + +# PostgreSQL service + checked-in migrations +# Go version is read from go.mod +# Gates: gofmt, go vet, race-enabled tests, build, golangci-lint ``` --- @@ -598,7 +593,7 @@ gantt section Phase 5 Tests & CI :done, p5, after p4, 14d - Production hardening :done, p5b, after p5, 14d + Repository hardening :done, p5b, after p5, 14d section Phase 6 (Advanced) Metrics & Tracing : p6a, after p5b, 14d @@ -645,7 +640,7 @@ gantt - Moderation flags table - Admin audit page -### Phase 5 — Production Hardening ✅ +### Phase 5 — Repository Hardening ✅ - [x] Test infrastructure: `fakeStore`, `newTestApplication`, `serveWithSession` - [x] Context helper tests @@ -657,8 +652,8 @@ gantt - [x] GitHub Actions CI pipeline - [x] CI pipeline refinement and edge cases - [x] Structured error handling improvements -- [x] Request logging and observability -- [x] Graceful shutdown +- [x] Request logging via chi middleware +- [x] Graceful SIGINT/SIGTERM HTTP shutdown with PostgreSQL pool cleanup ### Phase 6 — Advanced Production Ops 🚧 (Future) @@ -691,7 +686,8 @@ Project documentation lives in [`docs/`](./docs): | Document | Description | |----------|-------------| -| [Architecture Guide](./docs/Control%20Plane%20Notebook%20Architecture.md) | System design and component relationships | +| [As-Built Architecture Status](./ARCHITECTURE_STATUS.md) | Current implementation boundary, runtime semantics, and explicit non-goals | +| [Architecture Guide](./docs/Control%20Plane%20Notebook%20Architecture.md) | Historical/system design and component relationships | | [Reading Map](./docs/Control%20Plane%20Notebook%20Reading%20Map.md) | Recommended study order | | [Notebook vs Ledger API](./docs/Control%20Plane%20Notebook%20vs%20Ledger%20API.md) | Design comparison | | [Phase 0-1 Wiring Recipe](./docs/Phase%200-1%20Wiring%20Recipe.md) | Foundation build guide | diff --git a/cmd/web/main.go b/cmd/web/main.go index 98fe135..a0b18cd 100644 --- a/cmd/web/main.go +++ b/cmd/web/main.go @@ -2,13 +2,17 @@ package main import ( "context" + "errors" + "fmt" "html/template" "log" "log/slog" "net/http" "os" + "os/signal" "path/filepath" "strconv" + "syscall" "time" "github.com/SShogun/ControlPlane/internal/data" @@ -40,6 +44,11 @@ type Application struct { templateCache map[string]*template.Template } +type gracefulServer interface { + ListenAndServe() error + Shutdown(context.Context) error +} + func newTemplateCache(dir string) (map[string]*template.Template, error) { cache := map[string]*template.Template{} pages, err := filepath.Glob(filepath.Join(dir, "*.page.tmpl")) @@ -58,16 +67,44 @@ func newTemplateCache(dir string) (map[string]*template.Template, error) { return cache, nil } -func main() { +func serve(ctx context.Context, server gracefulServer, shutdownTimeout time.Duration) error { + errCh := make(chan error, 1) + go func() { + errCh <- server.ListenAndServe() + }() + + select { + case err := <-errCh: + if err == nil || errors.Is(err, http.ErrServerClosed) { + return nil + } + return fmt.Errorf("serve: listen: %w", err) + case <-ctx.Done(): + shutdownCtx, cancel := context.WithTimeout(context.Background(), shutdownTimeout) + defer cancel() + + if err := server.Shutdown(shutdownCtx); err != nil { + return fmt.Errorf("serve: shutdown: %w", err) + } + + err := <-errCh + if err != nil && !errors.Is(err, http.ErrServerClosed) { + return fmt.Errorf("serve: listen after shutdown: %w", err) + } + return nil + } +} + +func run() error { logger := slog.New(slog.NewTextHandler(os.Stdout, nil)) csrfSecret := os.Getenv("CSRF_SECRET") if csrfSecret == "" { logger.Warn("CSRF_SECRET not set, using insecure development key") - csrfSecret = "dev-only-insecure-csrf-key!!!!!!" // exactly 32 bytes + csrfSecret = "dev-only-insecure-csrf-key!!!!!!" } if len(csrfSecret) < 32 { - log.Fatalf("CSRF_SECRET must be at least 32 bytes; got %d", len(csrfSecret)) + return fmt.Errorf("CSRF_SECRET must be at least 32 bytes; got %d", len(csrfSecret)) } stateStr := os.Getenv("ENV") @@ -96,12 +133,15 @@ func main() { ctx := context.Background() pool, err := pgxpool.New(ctx, cfg.Database) if err != nil { - panic(err) + return fmt.Errorf("open database pool: %w", err) } - pingCtx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - if err := pool.Ping(pingCtx); err != nil { - log.Fatalf("Database not reachable: %v", err) + defer pool.Close() + + pingCtx, cancelPing := context.WithTimeout(ctx, 5*time.Second) + err = pool.Ping(pingCtx) + cancelPing() + if err != nil { + return fmt.Errorf("database not reachable: %w", err) } sessionManager := scs.New() @@ -115,7 +155,7 @@ func main() { templateCache, err := newTemplateCache("./ui/templates") if err != nil { - log.Fatalf("failed to build template cache: %v", err) + return fmt.Errorf("build template cache: %w", err) } app := &Application{ @@ -133,7 +173,19 @@ func main() { WriteTimeout: 30 * time.Second, } - if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed { - log.Fatalf("server failed: %v", err) + serverCtx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + defer stop() + + logger.Info("server starting", "addr", server.Addr) + if err := serve(serverCtx, &server, 10*time.Second); err != nil { + return err + } + logger.Info("server stopped") + return nil +} + +func main() { + if err := run(); err != nil { + log.Fatal(err) } } diff --git a/cmd/web/main_test.go b/cmd/web/main_test.go new file mode 100644 index 0000000..f5d61f0 --- /dev/null +++ b/cmd/web/main_test.go @@ -0,0 +1,124 @@ +package main + +import ( + "context" + "errors" + "net/http" + "sync" + "testing" + "time" +) + +type fakeGracefulServer struct { + listenStarted chan struct{} + shutdownCalled chan struct{} + stop chan struct{} + listenErr error + shutdownErr error + shutdownOnce sync.Once +} + +func newFakeGracefulServer() *fakeGracefulServer { + return &fakeGracefulServer{ + listenStarted: make(chan struct{}), + shutdownCalled: make(chan struct{}), + stop: make(chan struct{}), + } +} + +func (s *fakeGracefulServer) ListenAndServe() error { + close(s.listenStarted) + if s.listenErr != nil { + return s.listenErr + } + <-s.stop + return http.ErrServerClosed +} + +func (s *fakeGracefulServer) Shutdown(context.Context) error { + s.shutdownOnce.Do(func() { + close(s.shutdownCalled) + close(s.stop) + }) + return s.shutdownErr +} + +func TestServeShutsDownWhenContextIsCancelled(t *testing.T) { + server := newFakeGracefulServer() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + done := make(chan error, 1) + go func() { + done <- serve(ctx, server, time.Second) + }() + + select { + case <-server.listenStarted: + case <-time.After(time.Second): + t.Fatal("server did not start") + } + + cancel() + + select { + case err := <-done: + if err != nil { + t.Fatalf("serve returned error: %v", err) + } + case <-time.After(time.Second): + t.Fatal("serve did not return after cancellation") + } + + select { + case <-server.shutdownCalled: + default: + t.Fatal("expected graceful shutdown to be called") + } +} + +func TestServeReturnsUnexpectedListenError(t *testing.T) { + wantErr := errors.New("listen failed") + server := newFakeGracefulServer() + server.listenErr = wantErr + + err := serve(context.Background(), server, time.Second) + if !errors.Is(err, wantErr) { + t.Fatalf("expected listen error %v, got %v", wantErr, err) + } + + select { + case <-server.shutdownCalled: + t.Fatal("shutdown should not run after an immediate listen failure") + default: + } +} + +func TestServeReturnsShutdownError(t *testing.T) { + wantErr := errors.New("shutdown failed") + server := newFakeGracefulServer() + server.shutdownErr = wantErr + ctx, cancel := context.WithCancel(context.Background()) + + done := make(chan error, 1) + go func() { + done <- serve(ctx, server, time.Second) + }() + + select { + case <-server.listenStarted: + case <-time.After(time.Second): + t.Fatal("server did not start") + } + + cancel() + + select { + case err := <-done: + if !errors.Is(err, wantErr) { + t.Fatalf("expected shutdown error %v, got %v", wantErr, err) + } + case <-time.After(time.Second): + t.Fatal("serve did not return after shutdown error") + } +} From 56740e28b1dc14965c70c843154e5e857f4341c3 Mon Sep 17 00:00:00 2001 From: Soham Shirke <129677811+SShogun@users.noreply.github.com> Date: Wed, 7 Oct 2026 18:46:02 +0530 Subject: [PATCH 2/2] docs: add Phase 5.5 correctness hardening roadmap --- README.md | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 56e8ca5..2be65bc 100644 --- a/README.md +++ b/README.md @@ -595,8 +595,12 @@ gantt Tests & CI :done, p5, after p4, 14d Repository hardening :done, p5b, after p5, 14d + section Phase 5.5 + Domain & correctness hardening : p55a, after p5b, 14d + Concurrency & invariant tests : p55b, after p55a, 14d + section Phase 6 (Advanced) - Metrics & Tracing : p6a, after p5b, 14d + Metrics & Tracing : p6a, after p55b, 14d K8s & Shared Limiting : p6b, after p6a, 14d ``` @@ -655,6 +659,23 @@ gantt - [x] Request logging via chi middleware - [x] Graceful SIGINT/SIGTERM HTTP shutdown with PostgreSQL pool cleanup +### Phase 5.5 — Domain & Correctness Hardening 🚧 (Next) + +The goal of this phase is to move correctness guarantees out of optimistic handler logic and into explicit authorization policy, transactional store operations, and database-enforced invariants. + +- [ ] Centralize authorization policy for notebook ownership, team membership, visibility, editing, submission, review, moderation, and deletion. +- [ ] Make notebook creation atomic: notebook row, initial revision, tags, and audit event succeed or fail together. +- [ ] Make draft editing transactional so content, revision history, tags, and audit state cannot diverge after partial failures. +- [ ] Make submission transactional so the state transition and audit record commit together. +- [ ] Guard lifecycle transitions in the data layer (for example, only `draft -> submitted` and `submitted -> approved/rejected`). +- [ ] Reject stale, repeated, or invalid transitions by checking affected rows and returning explicit domain errors. +- [ ] Prevent double approval/rejection under concurrent reviewer actions using row locking or optimistic concurrency. +- [ ] Add database constraints/indexes for workflow invariants, including preventing conflicting decisions for one revision. +- [ ] Add PostgreSQL integration tests for unauthorized access, invalid transitions, duplicate decisions, rollback behavior, and concurrent approval attempts. +- [ ] Add race/failure-path tests around the workflow boundary rather than testing only happy-path handlers. +- [ ] Align README/demo claims with the actual authorization and transactional guarantees. +- [ ] Merge the hardening branch once these invariants are verified so the default branch reflects the portfolio-ready state. + ### Phase 6 — Advanced Production Ops 🚧 (Future) - [ ] Prometheus metrics (`/metrics`) exposing request volume, latency, and domain events.