diff --git a/AGENTS.md b/AGENTS.md index ce85dd2a..0980e3d4 100755 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,184 +1,91 @@ # AGENTS.md -This file provides guidance to AI coding agents working with the HyperFleet API repository. +## Project Identity -For Claude Code users: also see `CLAUDE.md` (auto-loaded) and `.claude/rules/` (loaded per file context). +HyperFleet API is a **stateless REST API** serving as the pure CRUD data layer for HyperFleet cluster lifecycle management. It persists clusters, node pools, and adapter statuses to PostgreSQL - no business logic, no events. Sentinel handles orchestration; adapters execute and report back. -## Commands +- **Language**: Go 1.26+ with FIPS crypto (`CGO_ENABLED=1 GOEXPERIMENT=boringcrypto`) +- **Database**: PostgreSQL 14.2 with GORM ORM +- **API Spec**: TypeSpec -> `hyperfleet-api-spec` Go module -> oapi-codegen -> Go models +- **Architecture**: Container-based dependency injection, config-driven route registration, transaction-per-request middleware -### Setup (fresh clone) +## Critical First Steps -``` -make generate-all # REQUIRED FIRST — generated code not in git -go mod download -make install-hooks # Install pre-commit hooks (secret scanning, linting, etc.) -make db/setup # Start local PostgreSQL container -make build # Build binary (CGO_ENABLED=1 GOEXPERIMENT=boringcrypto) -./bin/hyperfleet-api migrate -make run-no-auth # Start server without auth -``` - -### Build & Run - -``` -make build # Build hyperfleet-api binary to bin/ -make install # Build and install to GOPATH/bin -make run # Build, migrate, and run with auth (auto-generates dev JWT at /tmp/hf-dev-token.txt) -make run-no-auth # Build, migrate, and run without auth -``` - -### Code Generation +**Generated code is not checked into git.** Before building, testing, or even running `go mod download`: +```bash +make generate-all # Generates OpenAPI types + mock implementations ``` -make generate # Extract schema from hyperfleet-api-spec module, then run oapi-codegen -make generate-mocks # Regenerate mock implementations (go generate) -make generate-all # Both of the above -``` - -### Verification -``` -make verify # go vet + gofmt check -make lint # golangci-lint -make test # Unit tests (HYPERFLEET_ENV=unit_testing) -make test-integration # Integration tests with testcontainers (HYPERFLEET_ENV=integration_testing) -make test-helm # Helm chart lint + template validation -make verify-all # verify + lint + test — fast, no DB needed -make test-all # lint + test + test-integration + test-helm — full suite -``` +Setup sequence for a fresh clone: +1. `make generate-all` - generate OpenAPI models and mocks +2. `go mod download` - fetch dependencies +3. `make install-hooks` - install pre-commit hooks +4. `make db/setup` - start local PostgreSQL container +5. `make build` - build binary +6. `./bin/hyperfleet-api migrate` - apply database migrations +7. `make run-no-auth` - start server without authentication -### Database - -``` -make db/setup # Start PostgreSQL container -make db/login # Connect to local PostgreSQL -make db/teardown # Stop and remove container -``` +Tool versions are pinned in `tools/go.mod` and invoked via `go tool -modfile=tools/go.mod `. -Run `make help` for the complete target list. - -## Testing - -**Unit tests**: `make test` — sets `HYPERFLEET_ENV=unit_testing`, runs `./pkg/...` and `./cmd/...` - -**Integration tests**: `make test-integration` — sets `HYPERFLEET_ENV=integration_testing` and `TESTCONTAINERS_RYUK_DISABLED=true`. Testcontainers auto-creates isolated PostgreSQL instances. Located in `test/integration/`. - -**Helm tests**: `make test-helm` — lints and renders templates with multiple value combinations. - -**Mock generation**: `make generate-mocks` — uses `go generate` directives with `go.uber.org/mock/gomock`. Never write mocks manually. - -**Test factories**: `test/factories/` — create resources via the service layer, not directly in DB. Use `NewCluster()`, `NewClusterWithStatus()`, `NewClusterWithLabels()`. - -**Integration test setup**: `test.RegisterIntegration(t)` returns `(helper, client)`. Uses Gomega assertions and Resty HTTP client. - -**Environment variables for tests**: - -- `HYPERFLEET_ENV` — selects config: `unit_testing`, `integration_testing`, `development` -- `TESTCONTAINERS_RYUK_DISABLED=true` — required in CI -- Entity adapter requirements are configured per entity kind in `config.yaml` under `entities[].required_adapters` - -## Project Structure - -``` -cmd/hyperfleet-api/ # Entry point + subcommands (serve, migrate) - container/ # Lazily constructed dependencies (DAOs, services, validator, JWT) - servecmd/ # serve command; api_server.go is the composition root - server/ # HTTP servers, router, middleware, entity route registration - environments/ # Environment configs (development, unit_testing, etc.) -pkg/ - api/openapi/ # GENERATED — models + embedded spec (never edit) - handlers/ # HTTP handler pattern, validation and error handling - services/ # Service interfaces + sqlXxxService implementations - dao/ # DAO interfaces + sqlXxxDao implementations - db/ # SessionFactory, transaction middleware, migrations - errors/ # ServiceError type, RFC 9457 Problem Details - logger/ # Structured logging (slog-based) - config/ # Configuration management -openapi/ - README.md # Schema import, code generation, and validation details - openapi.yaml # Not in git — generated by make generate - oapi-codegen.yaml # Code generation config -test/ - integration/ # Integration tests (testcontainers) - factories/ # Test data factories -charts/ # Helm chart for Kubernetes deployment -``` +## Verification -**Generated code** (not in git — run `make generate-all`): - -- `pkg/api/openapi/` — Go models + embedded spec -- `*_mock.go` — Mock implementations - -## Code Style - -### Imports - -Order: stdlib → external → internal (`github.com/openshift-hyperfleet/hyperfleet-api/...`) - -### Errors - -Use constructor functions from `pkg/errors/errors.go`: `NotFound()`, `Validation()`, `GeneralError()`, `Conflict()`, `ValidationWithDetails()`. Error codes: `HYPERFLEET-CAT-NUM` format. All service methods return `*errors.ServiceError`. - -### Logging - -Use `pkg/logger/` — `logger.Info(ctx, "msg")`, `logger.With(ctx, "key", val).Error("msg")`. Never use `fmt.Println` or `log.Print`. - -### Services - -Interface + `sql*Service` struct. Constructor injection of DAOs. Return `*errors.ServiceError`. Add `//go:generate mockgen` directive for mocks. - -### DAOs - -Interface + `sql*Dao` struct. Get session via `sessionFactory.New(ctx)`. Call `db.MarkForRollback(ctx, err)` on write errors. Return stdlib `error`. - -### Entity Routes - -Entity types are config-driven — declared in `config.yaml` under `entities:` and auto-registered at startup. See `cmd/hyperfleet-api/server/routes_entities.go`. - -### Dependency Injection - -`cmd/hyperfleet-api/container` holds dependencies only (DAOs, services, schema validator, JWT handler), lazily constructed and cached. Composition — middleware chains, registrars, router, server — lives in `cmd/hyperfleet-api/servecmd/api_server.go` (`BuildAPIServer`), shared with `test/helper.go` so tests exercise production wiring. Do not add `*config.ApplicationConfig` to `cmd/hyperfleet-api/server`; it takes the narrow `cfg` interface instead. - -## Git Workflow - -### Commit Format - -``` -HYPERFLEET-### - type: description -``` - -Types: `feat`, `fix`, `refactor`, `test`, `docs`, `chore` - -Add co-author line for AI-assisted commits: - -``` -Co-Authored-By: Claude -``` +| Command | What it does | Requires DB? | +|---|---|---| +| `make verify` | go vet + gofmt check | No | +| `make lint` | golangci-lint | No | +| `make test` | Unit tests | No | +| `make test-integration` | Integration tests (testcontainers) | No (auto-creates) | +| `make test-helm` | Helm chart lint + template validation | No | +| `make verify-all` | verify + lint + test (single command) | No | +| `make test-all` | lint + test + test-integration + test-helm | Auto-creates | -### Pre-commit Hooks +Quick feedback: `make verify-all`. Full pre-push: `make test-all`. -Install: `make install-hooks` +## Source of Truth -Hooks: +| Topic | Where to look | +|---|---| +| OpenAPI spec & code generation | [openapi/README.md](openapi/README.md) | +| Handler pipeline & validation | `pkg/handlers/CLAUDE.md` | +| Service interface & status aggregation | `pkg/services/CLAUDE.md` | +| DAO patterns & session access | `pkg/dao/CLAUDE.md` | +| SessionFactory & transactions | `pkg/db/CLAUDE.md` | +| Error constructors & RFC 9457 | `pkg/errors/CLAUDE.md` | +| Test conventions & factories | `test/CLAUDE.md` | +| Helm chart testing | `charts/CLAUDE.md` | +| Development setup | [docs/development.md](docs/development.md) | +| Deployment | [docs/deployment.md](docs/deployment.md) | +| Authentication | [docs/authentication.md](docs/authentication.md) | +| Contributing | [CONTRIBUTING.md](CONTRIBUTING.md) | -- `leaktk.git.pre-commit` — secret scanning (open-source, no VPN required) -- `hyperfleet-commitlint` — validates commit message format (commit-msg stage) -- `hyperfleet-gofmt` — Go code formatting -- `hyperfleet-golangci-lint` — linting -- `hyperfleet-go-vet` — Go vet checks -- `trailing-whitespace` — removes trailing whitespace -- `end-of-file-fixer` — ensures files end with newline -- `check-added-large-files` — prevents large files from being committed +## Architecture Context -### Branching +**Request flow**: Router -> Middleware (logging, auth, transaction) -> Handler -> Service -> DAO -> GORM -> PostgreSQL -Create feature branches from `main`. PRs target `main`. +- **Startup wiring**: `servecmd.runServe` loads config -> `container.NewContainer(cfg)` -> `BuildAPIServer(...)` -> `server.NewRouterFromConfig` + `server.NewAPIServer`. Shutdown uses `pkg/closer` (LIFO order): readiness probe -> health drain -> metrics drain -> API drain -> JWT close -> OTel flush -> DB pool close +- **Entity routes are config-driven**: declared in `config.yaml` under `entities:`, registered at startup via `registry.LoadDescriptors()`, routes auto-generated by `RegisterEntityRoutes`. No per-entity Go code needed. +- **Transaction middleware** creates GORM transactions for write requests only (POST/PUT/PATCH/DELETE); reads skip for performance +- **Status aggregation**: Service layer synthesizes `Available`, `Reconciled`, and `LastKnownReconciled` conditions from adapter reports +- **Public routes** (`/openapi`, `/openapi.html`, metadata) bypass auth and schema validation; everything else is gated by both, auth outermost +- **Container** (`cmd/hyperfleet-api/container`) lazily constructs and caches dependencies. Holds DAOs, services, schema validator, JWT handler - but does NOT own their lifecycle. Shutdown ordering is handled by `pkg/closer` in the composition root. +- **Server package** (`cmd/hyperfleet-api/server`) deliberately does **not** import `pkg/config` - takes narrow `cfg` interfaces instead. Put anything needing `*config.ApplicationConfig` in the composition root. ## Boundaries -- **Never edit** `pkg/api/openapi/` or `*_mock.go` — regenerate with `make generate-all` -- **Never set** `status.phase` manually — calculated from adapter conditions -- **Never create** direct DB connections — use `SessionFactory.New(ctx)` for transaction participation +- **Never edit** files in `pkg/api/openapi/` or `*_mock.go` - regenerate with `make generate-all` +- **Never set** `status.phase` manually - calculated from adapter conditions +- **Never create** direct DB connections - use `SessionFactory.New(ctx)` for transaction participation - **FIPS required**: build with `CGO_ENABLED=1 GOEXPERIMENT=boringcrypto` -- **Spec source of truth**: `hyperfleet-api-spec` Go module; update `go.mod` to change spec versions — see [openapi/README.md](openapi/README.md) for full details on schema import, code generation, and validation -- **Tool versions** pinned in `tools/go.mod` — don't manually install oapi-codegen or golangci-lint +- **Spec source of truth**: `hyperfleet-api-spec` Go module; update `go.mod` to change spec versions - see [openapi/README.md](openapi/README.md) +- **Tool versions** pinned in `tools/go.mod` - don't manually install oapi-codegen or golangci-lint + +## Gotchas + +- **`make generate-all` is mandatory** - build and tests fail without it; generated code is gitignored +- **`pkg/api/openapi/` is read-only** - never hand-edit, always regenerate +- **Service methods return `*errors.ServiceError`, not stdlib `error`** - use constructor functions from `pkg/errors/errors.go`; see `pkg/errors/CLAUDE.md` for the full reference +- **`apiServer.Close()` severs in-flight requests** - always use `Shutdown(ctx)` with a drain budget; `Close()` is only the force-close fallback after `Shutdown` times out +- **Container has no `Close()`** - lifecycle (JWT handler, session factory, OTel) is managed by `pkg/closer` in the composition root, not on `Container` +- **Integration tests share a single testcontainer** - `test.NewHelper(t)` initializes once per process via `sync.Once`; the PostgreSQL container, API server, and JWK mock are shared across all tests in the suite +- **Schema validation requires the OpenAPI spec file** - if `HYPERFLEET_SERVER_OPENAPI_SCHEMA_PATH` is unset, validation is skipped; `TestMain` in integration tests auto-sets it to `test/validation-schema.yaml` diff --git a/CLAUDE.md b/CLAUDE.md index 6b642ea5..43c994c2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,148 +1 @@ -# CLAUDE.md - -## Project Identity - -HyperFleet API is a **stateless REST API** serving as the pure CRUD data layer for HyperFleet cluster lifecycle management. It persists clusters, node pools, and adapter statuses to PostgreSQL — no business logic, no events. Sentinel handles orchestration; adapters execute and report back. - -- **Language**: Go 1.26+ with FIPS crypto (`CGO_ENABLED=1 GOEXPERIMENT=boringcrypto`) -- **Database**: PostgreSQL 14.2 with GORM ORM -- **API Spec**: TypeSpec → `hyperfleet-api-spec` Go module → oapi-codegen → Go models -- **Architecture**: Container-based dependency injection, config-driven route registration, transaction-per-request middleware - -## Critical First Steps - -**Generated code is not checked into git.** Before building, testing, or even running `go mod download`: - -``` -make generate-all # Generates OpenAPI types + mock implementations -``` - -Setup sequence for a fresh clone: -1. `make generate-all` — generate OpenAPI models and mocks -2. `go mod download` — fetch dependencies -3. `make db/setup` — start local PostgreSQL container -4. `make build` — build binary (uses `CGO_ENABLED=1 GOEXPERIMENT=boringcrypto`) -5. `./bin/hyperfleet-api migrate` — apply database migrations -6. `make run-no-auth` — start server without authentication - -Tool versions are pinned in `tools/go.mod` and invoked via `go tool -modfile=tools/go.mod `. - -## Verification Commands - -| Command | What it does | Requires DB? | -|---|---|---| -| `make verify` | go vet + gofmt check | No | -| `make lint` | golangci-lint | No | -| `make test` | Unit tests (`HYPERFLEET_ENV=unit_testing`) | No | -| `make test-integration` | Integration tests (testcontainers) | No (auto-creates) | -| `make test-helm` | Helm chart lint + template validation | No | -| `make verify-all` | verify + lint + test (single command) | No | -| `make test-all` | lint + test + test-integration + test-helm | Auto-creates | - -Use `make verify-all` for fast feedback. Use `make test-all` for full validation. - -## Code Conventions - -### Commits -Format: `HYPERFLEET-### - type: description` (e.g., `HYPERFLEET-123 - fix: handle nil pointer in status aggregation`) -Co-author line: `Co-Authored-By: Claude ` - -### Import Ordering -1. Standard library -2. External packages -3. Internal packages (`github.com/openshift-hyperfleet/hyperfleet-api/...`) - -### Error Handling -All service methods return `*errors.ServiceError` (not stdlib error). Use constructor functions: -- Reference: `pkg/errors/errors.go` — `NotFound()`, `Validation()`, `GeneralError()`, `Conflict()`, `ValidationWithDetails()` -- Error codes follow `HYPERFLEET-CAT-NUM` format (e.g., `HYPERFLEET-NTF-001`) -- Errors convert to RFC 9457 Problem Details via `AsProblemDetails()` - -### Logging -Use the structured logging API — never `fmt.Println` or `log.Print`: -- Reference: `pkg/logger/logger.go` -- `logger.Info(ctx, "message")`, `logger.Error(ctx, "message")` -- Chainable: `logger.With(ctx, "key", value).WithError(err).Error("failed")` - -### Handler Pipeline -HTTP handlers (ResourceHandler, RootResourceHandler) are thin orchestration layers: -- Reference: `pkg/handlers/` -- Decode and validate requests, call services, marshal responses, handle errors -- No business logic — that lives in `pkg/services/` - -### Service Pattern -Interface + `sql*Service` implementation with constructor injection: -- Reference: `pkg/services/resource.go` — `ResourceService` interface, `sqlResourceService` struct -- Constructor: `NewResourceService(dao, adapterStatusDao, config) ResourceService` -- Generate mocks: `make generate-mocks` (uses `go generate` directives) - -### DAO Pattern -Interface + `sql*Dao` implementation using SessionFactory: -- Reference: `pkg/dao/resource.go` — `ResourceDao` interface, `sqlResourceDao` struct -- Get session: `db.New(ctx)` — extracts transaction from request context -- On write errors: call `db.MarkForRollback(ctx, err)` - -### Entity Route Registration -All entity types (Cluster, NodePool, Channel, Version, WifConfig) are config-driven — declared in `config.yaml` under `entities:`, -registered at startup via `registry.LoadDescriptors()`, routes auto-generated by -`cmd/hyperfleet-api/server/routes_entities.go` (`RegisterEntityRoutes`). No per-entity Go code needed. - -Registrars are passed to `server.NewRouterFromConfig` as `[]server.RouteRegistrar`, so adding a new route group means -constructing a registrar in the composition root — not adding an `init()` hook. - -### Dependency Injection -`cmd/hyperfleet-api/container` lazily constructs and caches DAOs, services, the schema validator, and the JWT -handler. It holds **dependencies only** — it does not assemble the server. -- Split by category: `container.go` (struct, constructor, `Close`), `daos.go`, `services.go`, `auth.go`, `validation.go` -- Constructed with explicit inputs: `NewContainer(cfg *config.ApplicationConfig, sessionFactory db.SessionFactory)` -- Not safe for concurrent initialization — startup is sequential -- `Close()` stops the JWT handler's JWKS refresh goroutine - -Composition (middleware chains, registrars, router, server) lives in `cmd/hyperfleet-api/servecmd/api_server.go` -(`BuildAPIServer`). It is exported because `test/helper.go` builds the API server through the same path, so -integration tests exercise production wiring. - -`cmd/hyperfleet-api/server` deliberately does **not** import `pkg/config` — `APIServer` takes the narrow `cfg` -interface in `api_server.go` instead. Keep it that way; put anything needing `*config.ApplicationConfig` in the -composition root. - -### Test Patterns -- Gomega assertions with `RegisterTestingT(t)` -- Test factories: `test/factories/` — create resources via service layer -- Integration tests: `test/integration/` — use `test.RegisterIntegration(t)` for setup -- Testcontainers for PostgreSQL — auto-creates isolated DB per test suite -- [Test placement strategy](https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/docs/e2e-testing/test-placement-strategy.md) — which layer a test belongs in (unit / integration / E2E) - -## Architecture Quick Reference - -**Request flow**: Router → Middleware (logging, auth, transaction) → Handler → Service → DAO → GORM → PostgreSQL - -- Transaction middleware creates GORM transactions for **write requests only** (POST/PUT/PATCH/DELETE): `pkg/db/transaction_middleware.go` -- Read requests (GET) skip transaction creation for performance -- OpenAPI spec and code generation: see [openapi/README.md](openapi/README.md) — run `make generate` before building; generated files in `pkg/api/openapi/` are **never edited** -- Status aggregation: Service layer synthesizes `Available`, `Reconciled`, and `LastKnownReconciled` conditions from adapter reports -- All entity types (Cluster, NodePool, Channel, etc.) are config-driven (`config.yaml` → `registry.LoadDescriptors` → auto-generated routes) -- **Startup wiring**: `servecmd.runServe` loads config → `container.NewContainer(cfg, sessionFactory)` → `BuildAPIServer(...)` → `server.NewRouterFromConfig` + `server.NewAPIServer` -- Public routes (`/openapi`, `/openapi.html`, metadata) bypass auth, schema validation, and transaction middleware; everything else is gated by both, auth outermost - -## Boundaries - -- **Never edit** files in `pkg/api/openapi/` — they are generated by `make generate` -- **Never edit** `*_mock.go` files — regenerate with `make generate-mocks` -- **Never set** `status.phase` manually — it is calculated from adapter conditions -- **Never create** direct DB connections — use `SessionFactory.New(ctx)` for transaction participation -- **FIPS required**: always build with `CGO_ENABLED=1 GOEXPERIMENT=boringcrypto` -- **OpenAPI spec**: not tracked in git — see [openapi/README.md](openapi/README.md) for spec versioning and generation details - -## Related CLAUDE.md Files - -Subdirectories contain context-specific guidance that loads when you work in those areas: - -- `pkg/handlers/CLAUDE.md` — Handler patterns, validation, and error handling -- `pkg/services/CLAUDE.md` — Service interface and status aggregation patterns -- `pkg/dao/CLAUDE.md` — DAO interface, session access, and rollback patterns -- `pkg/db/CLAUDE.md` — SessionFactory and transaction middleware -- `pkg/errors/CLAUDE.md` — Error constructors, codes, and RFC 9457 details -- `test/CLAUDE.md` — Test conventions, factories, and environment variables -- `charts/CLAUDE.md` — Helm chart testing and configuration -- `openapi/README.md` — OpenAPI schema import, code generation, schema validation, and oapi-codegen config +@AGENTS.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index aa0a149f..aac1f927 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -99,7 +99,7 @@ make test make ci-test-unit ``` -Unit tests run with `HYPERFLEET_ENV=unit_testing` and do not require a running database. +Unit tests do not require a running database. ### Integration Tests diff --git a/Makefile b/Makefile index 6d5183b7..269a6e16 100755 --- a/Makefile +++ b/Makefile @@ -70,11 +70,6 @@ db_image ?= docker.io/library/postgres:14.23 unit_test_json_output ?= ${PWD}/unit-test-results.json integration_test_json_output ?= ${PWD}/integration-test-results.json -### Environment-sourced variables with defaults -ifndef HYPERFLEET_ENV - HYPERFLEET_ENV := development -endif - ifndef TEST_SUMMARY_FORMAT TEST_SUMMARY_FORMAT = short-verbose endif @@ -184,8 +179,8 @@ dev-token: ## Generate a fresh JWT using the existing dev key (no server restart @echo 'Usage: curl -H "Authorization: Bearer $$(cat $(DEV_TOKEN_FILE))" http://localhost:8000/api/hyperfleet/v1/clusters' .PHONY: run-no-auth -run-no-auth: db/migrate ## Run the application without auth - ./bin/hyperfleet-api serve $(DB_FLAGS) --server-jwt-enabled=false +run-no-auth: db/migrate ## Run the application without auth or TLS (local dev) + HYPERFLEET_SERVER_JWT_ENABLED=false HYPERFLEET_SERVER_TLS_ENABLED=false ./bin/hyperfleet-api serve $(DB_FLAGS) .PHONY: run/docs run/docs: check-container-tool ## Run swagger and host the api spec @@ -218,24 +213,24 @@ clean: ## Delete temporary generated files .PHONY: test test: install ## Run unit tests - HYPERFLEET_ENV=unit_testing $(call gotool,gotestsum) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -v $(TESTFLAGS) \ + $(call gotool,gotestsum) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -v $(TESTFLAGS) \ ./pkg/... \ ./cmd/... .PHONY: ci-test-unit ci-test-unit: install ## Run unit tests with JSON output - HYPERFLEET_ENV=unit_testing $(call gotool,gotestsum) --jsonfile-timing-events=$(unit_test_json_output) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -v $(TESTFLAGS) \ + $(call gotool,gotestsum) --jsonfile-timing-events=$(unit_test_json_output) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -v $(TESTFLAGS) \ ./pkg/... \ ./cmd/... .PHONY: test-integration test-integration: install ## Run integration tests - TESTCONTAINERS_RYUK_DISABLED=true HYPERFLEET_ENV=integration_testing $(call gotool,gotestsum) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -ldflags -s -v -timeout 1h $(TESTFLAGS) \ + TESTCONTAINERS_RYUK_DISABLED=true $(call gotool,gotestsum) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -ldflags -s -v -timeout 1h $(TESTFLAGS) \ ./test/integration .PHONY: ci-test-integration ci-test-integration: install ## Run integration tests with JSON output - TESTCONTAINERS_RYUK_DISABLED=true HYPERFLEET_ENV=integration_testing $(call gotool,gotestsum) --jsonfile-timing-events=$(integration_test_json_output) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -ldflags -s -v -timeout 1h $(TESTFLAGS) \ + TESTCONTAINERS_RYUK_DISABLED=true $(call gotool,gotestsum) --jsonfile-timing-events=$(integration_test_json_output) --format $(TEST_SUMMARY_FORMAT) -- -p 1 -ldflags -s -v -timeout 1h $(TESTFLAGS) \ ./test/integration .PHONY: test-all diff --git a/charts/README.md b/charts/README.md index 4a1f2f56..4162f12b 100644 --- a/charts/README.md +++ b/charts/README.md @@ -44,7 +44,7 @@ helm install hyperfleet-api oci://REGISTRY/hyperfleet-api \ | ports.api | int | `8000` | API server port | | ports.health | int | `8080` | Health check endpoint port | | ports.metrics | int | `9090` | Prometheus metrics endpoint port | -| config | object | `{"database":{"debug":false,"dialect":"postgres","host":"","name":"hyperfleet","pool":{"conn_max_idle_time":"1m","conn_max_lifetime":"5m","conn_retry_attempts":10,"conn_retry_interval":"3s","max_connections":50,"max_idle_connections":10,"request_timeout":"30s"},"port":5432,"ssl":{"mode":"disable","root_cert_file":""}},"entities":[{"kind":"Cluster","name_max_len":53,"name_min_len":3,"plural":"clusters","require_spec_schema":true,"required_adapters":["validation","dns","pullsecret","hypershift"],"spec_schema_name":"ClusterSpec"},{"kind":"NodePool","name_max_len":15,"name_min_len":3,"on_parent_delete":"cascade","parent_kind":"Cluster","plural":"nodepools","require_spec_schema":true,"required_adapters":["validation","hypershift"],"spec_schema_name":"NodePoolSpec"},{"kind":"Channel","plural":"channels","spec_schema_name":"ChannelSpec"},{"kind":"Version","on_parent_delete":"restrict","parent_kind":"Channel","plural":"versions","spec_schema_name":"VersionSpec"},{"kind":"WifConfig","plural":"wifconfigs","spec_schema_name":"WifConfigSpec"}],"existingConfigMap":"","health":{"db_ping_timeout":"2s","host":"0.0.0.0","port":8080,"shutdown_timeout":"20s","tls":{"enabled":false}},"logging":{"format":"json","level":"info","masking":{"enabled":true,"fields":["password","secret","token","api_key","access_token","refresh_token","client_secret"],"headers":["Authorization","X-API-Key","Cookie","X-Auth-Token","X-Forwarded-Authorization","X-HyperFleet-Identity"]},"otel":{"enabled":false},"output":"stdout"},"metrics":{"host":"0.0.0.0","label_metrics_inclusion_duration":"168h","port":9090,"reconciliation_stuck_threshold":"10m","tls":{"enabled":false}},"server":{"host":"0.0.0.0","hostname":"","jwt":{"configs":[],"enabled":false},"port":8000,"timeouts":{"read":"5s","write":"30s"},"tls":{"cert_file":"","enabled":false,"key_file":""}}}` | Application configuration. All settings in this section generate the ConfigMap consumed by the API server. Set `config.existingConfigMap` to use a pre-existing ConfigMap instead. | +| config | object | `{"database":{"debug":false,"dialect":"postgres","host":"","name":"hyperfleet","pool":{"conn_max_idle_time":"1m","conn_max_lifetime":"5m","conn_retry_attempts":10,"conn_retry_interval":"3s","max_connections":50,"max_idle_connections":10,"request_timeout":"30s"},"port":5432,"ssl":{"mode":"disable","root_cert_file":""}},"entities":[{"kind":"Cluster","name_max_len":53,"name_min_len":3,"plural":"clusters","require_spec_schema":true,"required_adapters":["validation","dns","pullsecret","hypershift"],"spec_schema_name":"ClusterSpec"},{"kind":"NodePool","name_max_len":15,"name_min_len":3,"on_parent_delete":"cascade","parent_kind":"Cluster","plural":"nodepools","require_spec_schema":true,"required_adapters":["validation","hypershift"],"spec_schema_name":"NodePoolSpec"},{"kind":"Channel","plural":"channels","spec_schema_name":"ChannelSpec"},{"kind":"Version","on_parent_delete":"restrict","parent_kind":"Channel","plural":"versions","spec_schema_name":"VersionSpec"},{"kind":"WifConfig","plural":"wifconfigs","spec_schema_name":"WifConfigSpec"}],"existingConfigMap":"","health":{"db_ping_timeout":"2s","host":"0.0.0.0","port":8080,"shutdown_timeout":"20s","tls":{"enabled":false}},"logging":{"format":"json","level":"info","masking":{"enabled":true,"fields":["password","secret","token","api_key","access_token","refresh_token","client_secret"],"headers":["Authorization","X-API-Key","Cookie","X-Auth-Token","X-Forwarded-Authorization","X-HyperFleet-Identity"]},"output":"stdout"},"metrics":{"host":"0.0.0.0","label_metrics_inclusion_duration":"168h","port":9090,"reconciliation_stuck_threshold":"10m","tls":{"enabled":false}},"server":{"host":"0.0.0.0","hostname":"","jwt":{"configs":[],"enabled":false},"port":8000,"timeouts":{"read":"5s","write":"30s"},"tls":{"cert_file":"","enabled":false,"key_file":""}},"tracing":{"enabled":false,"service_name":"hyperfleet-api"}}` | Application configuration. All settings in this section generate the ConfigMap consumed by the API server. Set `config.existingConfigMap` to use a pre-existing ConfigMap instead. | | config.existingConfigMap | string | `""` | Use an existing ConfigMap instead of generating one. When set, all other `config.*` values are ignored. | | config.server | object | `{"host":"0.0.0.0","hostname":"","jwt":{"configs":[],"enabled":false},"port":8000,"timeouts":{"read":"5s","write":"30s"},"tls":{"cert_file":"","enabled":false,"key_file":""}}` | HTTP server settings | | config.server.hostname | string | `""` | Public hostname advertised by the API (leave empty for auto-detect) | @@ -77,16 +77,17 @@ helm install hyperfleet-api oci://REGISTRY/hyperfleet-api \ | config.database.pool.request_timeout | string | `"30s"` | Timeout for acquiring a connection from the pool | | config.database.pool.conn_retry_attempts | int | `10` | Number of connection retry attempts on startup | | config.database.pool.conn_retry_interval | string | `"3s"` | Interval between connection retry attempts | -| config.logging | object | `{"format":"json","level":"info","masking":{"enabled":true,"fields":["password","secret","token","api_key","access_token","refresh_token","client_secret"],"headers":["Authorization","X-API-Key","Cookie","X-Auth-Token","X-Forwarded-Authorization","X-HyperFleet-Identity"]},"otel":{"enabled":false},"output":"stdout"}` | Logging configuration | +| config.logging | object | `{"format":"json","level":"info","masking":{"enabled":true,"fields":["password","secret","token","api_key","access_token","refresh_token","client_secret"],"headers":["Authorization","X-API-Key","Cookie","X-Auth-Token","X-Forwarded-Authorization","X-HyperFleet-Identity"]},"output":"stdout"}` | Logging configuration | | config.logging.level | string | `"info"` | Log level (`debug`, `info`, `warn`, `error`) | | config.logging.format | string | `"json"` | Log format (`json` or `text`) | | config.logging.output | string | `"stdout"` | Log output destination | -| config.logging.otel | object | `{"enabled":false}` | OpenTelemetry tracing integration. See the [tracing standard](https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/standards/tracing.md#configuration). | -| config.logging.otel.enabled | bool | `false` | Enable OpenTelemetry log correlation | | config.logging.masking | object | `{"enabled":true,"fields":["password","secret","token","api_key","access_token","refresh_token","client_secret"],"headers":["Authorization","X-API-Key","Cookie","X-Auth-Token","X-Forwarded-Authorization","X-HyperFleet-Identity"]}` | Sensitive-data masking for logs | | config.logging.masking.enabled | bool | `true` | Enable log masking | | config.logging.masking.headers | list | `["Authorization","X-API-Key","Cookie","X-Auth-Token","X-Forwarded-Authorization","X-HyperFleet-Identity"]` | HTTP headers whose values are redacted in logs | | config.logging.masking.fields | list | `["password","secret","token","api_key","access_token","refresh_token","client_secret"]` | Field names whose values are redacted in logs | +| config.tracing | object | `{"enabled":false,"service_name":"hyperfleet-api"}` | OpenTelemetry tracing. See the [tracing standard](https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/standards/tracing.md#configuration). | +| config.tracing.enabled | bool | `false` | Enable OpenTelemetry tracing | +| config.tracing.service_name | string | `"hyperfleet-api"` | OTel service name (overridden by OTEL_SERVICE_NAME env var if set) | | config.metrics | object | `{"host":"0.0.0.0","label_metrics_inclusion_duration":"168h","port":9090,"reconciliation_stuck_threshold":"10m","tls":{"enabled":false}}` | Prometheus metrics endpoint settings | | config.metrics.host | string | `"0.0.0.0"` | Listen address (must be `0.0.0.0` for in-cluster access) | | config.metrics.port | int | `9090` | Listen port (must match `ports.metrics`) | @@ -99,7 +100,7 @@ helm install hyperfleet-api oci://REGISTRY/hyperfleet-api \ | config.health.port | int | `8080` | Listen port (must match `ports.health`) | | config.health.tls | object | `{"enabled":false}` | TLS configuration for the health endpoint | | config.health.tls.enabled | bool | `false` | Enable TLS on the health endpoint | -| config.health.shutdown_timeout | string | `"20s"` | Graceful shutdown timeout | +| config.health.shutdown_timeout | string | `"20s"` | How long the API server waits for in-flight requests to complete during graceful shutdown | | config.health.db_ping_timeout | string | `"2s"` | Timeout for the database liveness ping | | config.entities | list | `[{"kind":"Cluster","name_max_len":53,"name_min_len":3,"plural":"clusters","require_spec_schema":true,"required_adapters":["validation","dns","pullsecret","hypershift"],"spec_schema_name":"ClusterSpec"},{"kind":"NodePool","name_max_len":15,"name_min_len":3,"on_parent_delete":"cascade","parent_kind":"Cluster","plural":"nodepools","require_spec_schema":true,"required_adapters":["validation","hypershift"],"spec_schema_name":"NodePoolSpec"},{"kind":"Channel","plural":"channels","spec_schema_name":"ChannelSpec"},{"kind":"Version","on_parent_delete":"restrict","parent_kind":"Channel","plural":"versions","spec_schema_name":"VersionSpec"},{"kind":"WifConfig","plural":"wifconfigs","spec_schema_name":"WifConfigSpec"}]` | Entity descriptors registered at startup. Each entry auto-generates REST endpoints, spec validation, and delete policies. | | serviceAccount | object | `{"annotations":{},"create":true,"name":""}` | ServiceAccount configuration | @@ -120,7 +121,7 @@ helm install hyperfleet-api oci://REGISTRY/hyperfleet-api \ | resources | object | `{"limits":{"cpu":"500m","memory":"512Mi"},"requests":{"cpu":"100m","memory":"128Mi"}}` | CPU and memory resource requests and limits | | lifecycle | object | `{"preStop":{"exec":{"command":["/bin/sh","-c","sleep 5"]}}}` | Container lifecycle hooks. Use `preStop` to delay SIGTERM during rolling updates, giving the LoadBalancer time to drain the old pod. See HYPERFLEET-1306. | | strategy | object | `{"rollingUpdate":{"maxSurge":1,"maxUnavailable":0},"type":"RollingUpdate"}` | Deployment rollout strategy. `maxUnavailable: 0` ensures zero-downtime during rolling updates — the old pod stays until the new one is Ready. | -| terminationGracePeriodSeconds | int | `70` | Seconds Kubernetes waits after SIGTERM before SIGKILL. Shutdown sequence is sequential: preStop sleep (5s) + health server (20s max) + API server (10s max) + metrics server (10s max) + OTel (20s max) + DB close (no timeout). Typical shutdown is ~8s total; worst case exceeds 60s. Set to 70s to cover worst case with buffer. If the DB close hangs beyond this, Kubernetes sends SIGKILL — acceptable as a last resort. | +| terminationGracePeriodSeconds | int | `35` | Must be > preStop (5s) + shutdown_timeout (20s) + 9s overhead (2s metrics + 2s health + 5s OTel) | | nodeSelector | object | `{}` | Node selector constraints for pod scheduling | | tolerations | list | `[]` | Tolerations for pod scheduling | | affinity | object | `{}` | Affinity rules for pod scheduling | diff --git a/charts/templates/configmap.yaml b/charts/templates/configmap.yaml index 109bd98b..0f23e9ef 100644 --- a/charts/templates/configmap.yaml +++ b/charts/templates/configmap.yaml @@ -106,9 +106,6 @@ data: format: {{ .Values.config.logging.format }} output: {{ .Values.config.logging.output }} - otel: - enabled: {{ .Values.config.logging.otel.enabled }} - masking: enabled: {{ .Values.config.logging.masking.enabled }} headers: @@ -120,6 +117,10 @@ data: - {{ . }} {{- end }} + tracing: + enabled: {{ .Values.config.tracing.enabled }} + service_name: {{ .Values.config.tracing.service_name }} + metrics: host: {{ .Values.config.metrics.host | default "0.0.0.0" }} port: {{ .Values.config.metrics.port | default 9090 }} diff --git a/charts/values.yaml b/charts/values.yaml index db71b285..7f0103d2 100644 --- a/charts/values.yaml +++ b/charts/values.yaml @@ -134,12 +134,6 @@ config: # -- Log output destination output: stdout - # -- OpenTelemetry tracing integration. - # See the [tracing standard](https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/standards/tracing.md#configuration). - otel: - # -- Enable OpenTelemetry log correlation - enabled: false - # -- Sensitive-data masking for logs masking: # -- Enable log masking @@ -162,6 +156,14 @@ config: - refresh_token - client_secret + # -- OpenTelemetry tracing. + # See the [tracing standard](https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/standards/tracing.md#configuration). + tracing: + # -- Enable OpenTelemetry tracing + enabled: false + # -- OTel service name (overridden by OTEL_SERVICE_NAME env var if set) + service_name: hyperfleet-api + # -- Prometheus metrics endpoint settings metrics: # -- Listen address (must be `0.0.0.0` for in-cluster access) @@ -191,7 +193,7 @@ config: # -- Enable TLS on the health endpoint enabled: false - # -- Graceful shutdown timeout + # -- How long the API server waits for in-flight requests to complete during graceful shutdown shutdown_timeout: 20s # -- Timeout for the database liveness ping db_ping_timeout: 2s @@ -309,13 +311,8 @@ strategy: maxUnavailable: 0 type: RollingUpdate -# -- Seconds Kubernetes waits after SIGTERM before SIGKILL. -# Shutdown sequence is sequential: preStop sleep (5s) + health server (20s max) -# + API server (10s max) + metrics server (10s max) + OTel (20s max) + DB close -# (no timeout). Typical shutdown is ~8s total; worst case exceeds 60s. -# Set to 70s to cover worst case with buffer. If the DB close hangs beyond -# this, Kubernetes sends SIGKILL — acceptable as a last resort. -terminationGracePeriodSeconds: 70 +# -- Must be > preStop (5s) + shutdown_timeout (20s) + 9s overhead (2s metrics + 2s health + 5s OTel) +terminationGracePeriodSeconds: 35 # -- Node selector constraints for pod scheduling nodeSelector: {} diff --git a/cmd/hyperfleet-api/container/auth.go b/cmd/hyperfleet-api/container/auth.go index 3bc2b5af..484df940 100644 --- a/cmd/hyperfleet-api/container/auth.go +++ b/cmd/hyperfleet-api/container/auth.go @@ -7,7 +7,10 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/auth" ) -func (c *Container) JWTHandler() (*auth.JWTHandler, error) { +func (c *Container) JWTHandler() *auth.JWTHandler { + if !c.cfg.Server.JWT.Enabled { + return nil + } if c.jwtHandler == nil { jwtHandler, err := auth.NewJWTHandler( context.Background(), @@ -16,9 +19,10 @@ func (c *Container) JWTHandler() (*auth.JWTHandler, error) { }, ) if err != nil { - return nil, fmt.Errorf("unable to create JWT handler: %w", err) + panic(fmt.Sprintf("create JWT handler: %v", err)) } c.jwtHandler = jwtHandler + c.closer.Add(c.jwtHandler.Close) } - return c.jwtHandler, nil + return c.jwtHandler } diff --git a/cmd/hyperfleet-api/container/container.go b/cmd/hyperfleet-api/container/container.go index 51527a10..a0b0d7bc 100644 --- a/cmd/hyperfleet-api/container/container.go +++ b/cmd/hyperfleet-api/container/container.go @@ -2,6 +2,7 @@ package container import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/auth" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/closer" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" @@ -11,16 +12,9 @@ import ( // Container lazily constructs and caches application dependencies during // sequential startup. It is not safe for concurrent initialization. -// -// Container owns dependencies only. Assembling them into a running API server -// is the composition root's job - see BuildAPIServer in the servecmd package. -// -// TODO(HYPERFLEET-1371): Once the environments/ package is removed, -// Container should source SessionFactory directly (e.g. from config/Viper) -// rather than accepting it as a constructor parameter. Close() should also -// close the SessionFactory at that point. type Container struct { cfg *config.ApplicationConfig + closer *closer.Closer sessionFactory db.SessionFactory resourceDao dao.ResourceDao @@ -37,16 +31,6 @@ type Container struct { jwtHandler *auth.JWTHandler } -func NewContainer(cfg *config.ApplicationConfig, sessionFactory db.SessionFactory) *Container { - return &Container{cfg: cfg, sessionFactory: sessionFactory} -} - -func (c *Container) SessionFactory() db.SessionFactory { - return c.sessionFactory -} - -func (c *Container) Close() { - if c.jwtHandler != nil { - c.jwtHandler.Close() - } +func NewContainer(cfg *config.ApplicationConfig, c *closer.Closer) *Container { + return &Container{cfg: cfg, closer: c} } diff --git a/cmd/hyperfleet-api/container/container_test.go b/cmd/hyperfleet-api/container/container_test.go index 7b00d4bb..bb769e0b 100644 --- a/cmd/hyperfleet-api/container/container_test.go +++ b/cmd/hyperfleet-api/container/container_test.go @@ -5,8 +5,10 @@ import ( . "github.com/onsi/gomega" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/closer" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" dbmocks "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db/mocks" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" ) func newTestContainer(t *testing.T) *Container { @@ -15,7 +17,9 @@ func newTestContainer(t *testing.T) *Container { sessionFactory := dbmocks.NewMockSessionFactory() t.Cleanup(func() { _ = sessionFactory.Close() }) - return NewContainer(config.NewApplicationConfig(), sessionFactory) + c := NewContainer(config.NewApplicationConfig(), closer.New()) + c.sessionFactory = sessionFactory + return c } func TestContainerCachesDAOsAndServices(t *testing.T) { @@ -23,8 +27,6 @@ func TestContainerCachesDAOsAndServices(t *testing.T) { c := newTestContainer(t) - Expect(c.SessionFactory()).NotTo(BeNil()) - Expect(c.ResourceDao()).NotTo(BeNil()) Expect(c.ResourceDao()).To(BeIdenticalTo(c.ResourceDao())) Expect(c.ResourceLabelDao()).NotTo(BeNil()) @@ -60,3 +62,16 @@ func TestContainerConstructionIsLazy(t *testing.T) { Expect(c.schemaValidator).To(BeNil()) Expect(c.jwtHandler).To(BeNil()) } + +func TestContainerDoesNotInitializeGlobalRegistry(t *testing.T) { + RegisterTestingT(t) + + cfg := config.NewApplicationConfig() + cfg.Entities = []registry.EntityDescriptor{{Kind: "invalid-without-plural"}} + sessionFactory := dbmocks.NewMockSessionFactory() + t.Cleanup(func() { _ = sessionFactory.Close() }) + + Expect(func() { + NewContainer(cfg, closer.New()) + }).NotTo(Panic()) +} diff --git a/cmd/hyperfleet-api/container/daos.go b/cmd/hyperfleet-api/container/daos.go index a932936a..335f4cbb 100644 --- a/cmd/hyperfleet-api/container/daos.go +++ b/cmd/hyperfleet-api/container/daos.go @@ -6,35 +6,35 @@ import ( func (c *Container) ResourceDao() dao.ResourceDao { if c.resourceDao == nil { - c.resourceDao = dao.NewResourceDao(c.sessionFactory) + c.resourceDao = dao.NewResourceDao(c.SessionFactory()) } return c.resourceDao } func (c *Container) ResourceLabelDao() dao.ResourceLabelDao { if c.resourceLabelDao == nil { - c.resourceLabelDao = dao.NewResourceLabelDao(c.sessionFactory) + c.resourceLabelDao = dao.NewResourceLabelDao(c.SessionFactory()) } return c.resourceLabelDao } func (c *Container) AdapterStatusDao() dao.AdapterStatusDao { if c.adapterStatusDao == nil { - c.adapterStatusDao = dao.NewAdapterStatusDao(c.sessionFactory) + c.adapterStatusDao = dao.NewAdapterStatusDao(c.SessionFactory()) } return c.adapterStatusDao } func (c *Container) ResourceConditionDao() dao.ResourceConditionDao { if c.resourceConditionDao == nil { - c.resourceConditionDao = dao.NewResourceConditionDao(c.sessionFactory) + c.resourceConditionDao = dao.NewResourceConditionDao(c.SessionFactory()) } return c.resourceConditionDao } func (c *Container) GenericDao() dao.GenericDao { if c.genericDao == nil { - c.genericDao = dao.NewGenericDao(c.sessionFactory) + c.genericDao = dao.NewGenericDao(c.SessionFactory()) } return c.genericDao } diff --git a/cmd/hyperfleet-api/container/db.go b/cmd/hyperfleet-api/container/db.go new file mode 100644 index 00000000..60157a3a --- /dev/null +++ b/cmd/hyperfleet-api/container/db.go @@ -0,0 +1,14 @@ +package container + +import ( + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db/db_session" +) + +func (c *Container) SessionFactory() db.SessionFactory { + if c.sessionFactory == nil { + c.sessionFactory = db_session.NewProdFactory(c.cfg.Database) + c.closer.Add(c.sessionFactory.Close) + } + return c.sessionFactory +} diff --git a/cmd/hyperfleet-api/container/validation.go b/cmd/hyperfleet-api/container/validation.go index 7165306e..fb9b0dbb 100644 --- a/cmd/hyperfleet-api/container/validation.go +++ b/cmd/hyperfleet-api/container/validation.go @@ -8,15 +8,15 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/validators" ) -func (c *Container) SchemaValidator() (*validators.SchemaValidator, error) { +func (c *Container) SchemaValidator() *validators.SchemaValidator { if c.schemaValidator == nil { schemaPath := c.cfg.Server.OpenAPISchemaPath schemaValidator, err := validators.NewSchemaValidator(schemaPath) if err != nil { - return nil, fmt.Errorf("unable to create schema validator: %w", err) + panic(fmt.Sprintf("create schema validator: %v", err)) } c.schemaValidator = schemaValidator logger.With(context.Background(), logger.FieldSchemaPath, schemaPath).Info("Schema validation enabled") } - return c.schemaValidator, nil + return c.schemaValidator } diff --git a/cmd/hyperfleet-api/environments/e_development.go b/cmd/hyperfleet-api/environments/e_development.go deleted file mode 100755 index fabbe94c..00000000 --- a/cmd/hyperfleet-api/environments/e_development.go +++ /dev/null @@ -1,44 +0,0 @@ -package environments - -import ( - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db/db_session" -) - -// devEnvImpl environment is intended for local use while developing features -type devEnvImpl struct { - env *Env -} - -var _ EnvironmentImpl = &devEnvImpl{} - -func (e *devEnvImpl) OverrideDatabase(c *Database) error { - c.SessionFactory = db_session.NewProdFactory(e.env.Config.Database) - return nil -} - -func (e *devEnvImpl) OverrideConfig(c *config.ApplicationConfig) error { - c.Server.JWT.Enabled = false - c.Server.TLS.Enabled = false - - // Ensure SSL mode is set to disable for development (required for database connection) - if c.Database.SSL.Mode == "" { - c.Database.SSL.Mode = SSLModeDisable - } - - return nil -} - -func (e *devEnvImpl) OverrideServices(s *Services) error { - return nil -} - -func (e *devEnvImpl) OverrideHandlers(h *Handlers) error { - return nil -} - -func (e *devEnvImpl) EnvironmentDefaults() map[string]string { - // Return empty map - new config system has appropriate defaults - // and OverrideConfig() sets development-specific values programmatically - return map[string]string{} -} diff --git a/cmd/hyperfleet-api/environments/e_integration_testing.go b/cmd/hyperfleet-api/environments/e_integration_testing.go deleted file mode 100755 index 8046e29b..00000000 --- a/cmd/hyperfleet-api/environments/e_integration_testing.go +++ /dev/null @@ -1,78 +0,0 @@ -package environments - -import ( - "fmt" - "os" - - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db/db_session" -) - -var _ EnvironmentImpl = &integrationTestingEnvImpl{} - -// integrationTestingEnvImpl is configuration for integration tests using testcontainers -type integrationTestingEnvImpl struct { - env *Env -} - -func (e *integrationTestingEnvImpl) OverrideDatabase(c *Database) error { - c.SessionFactory = db_session.NewTestcontainerFactory(e.env.Config.Database) - return nil -} - -func (e *integrationTestingEnvImpl) OverrideConfig(c *config.ApplicationConfig) error { - // Support a one-off env to allow enabling db debug in testing - //nolint:goconst // "true" is not extracted to constant (standard env var idiom) - if os.Getenv("HYPERFLEET_DATABASE_DEBUG") == "true" { - c.Database.Debug = true - } - - // Integration tests use testcontainers — set defaults directly - c.Database.Name = "hyperfleet_test" - c.Database.Username = "test" - c.Database.Password = "test" - c.Database.Host = "localhost" - c.Database.Port = 5432 - - // Ensure SSL mode is set to disable for testing - if c.Database.SSL.Mode == "" { - c.Database.SSL.Mode = SSLModeDisable - } - - // Integration tests always use JWT. Config load may disable JWT so validation - // can pass before issuers exist; re-enable and bootstrap a complete issuer here. - // JWKCertURL is a placeholder; the test harness overwrites it once the JWK - // mock server is running. - c.Server.JWT.Enabled = true - if len(c.Server.JWT.Configs) == 0 { - c.Server.JWT.Configs = []config.JWTIssuerConfig{ - { - IssuerURL: "https://test-issuer.example.com", - JWKCertURL: "https://test-issuer.example.com/.well-known/jwks.json", - Header: "Authorization", - IdentityClaim: "email", - }, - } - } - - c.Server.JWT.ApplyDefaults() - if err := c.Server.JWT.Validate(); err != nil { - return fmt.Errorf("integration test JWT config validation failed: %w", err) - } - - return nil -} - -func (e *integrationTestingEnvImpl) OverrideServices(s *Services) error { - return nil -} - -func (e *integrationTestingEnvImpl) OverrideHandlers(h *Handlers) error { - return nil -} - -func (e *integrationTestingEnvImpl) EnvironmentDefaults() map[string]string { - // Return empty map - new config system has appropriate defaults - // and OverrideConfig() sets test-specific values programmatically - return map[string]string{} -} diff --git a/cmd/hyperfleet-api/environments/e_production.go b/cmd/hyperfleet-api/environments/e_production.go deleted file mode 100755 index 5f72b0f1..00000000 --- a/cmd/hyperfleet-api/environments/e_production.go +++ /dev/null @@ -1,36 +0,0 @@ -package environments - -import ( - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db/db_session" -) - -var _ EnvironmentImpl = &productionEnvImpl{} - -// productionEnvImpl is any deployed instance of the service through app-interface -type productionEnvImpl struct { - env *Env -} - -func (e *productionEnvImpl) OverrideDatabase(c *Database) error { - c.SessionFactory = db_session.NewProdFactory(e.env.Config.Database) - return nil -} - -func (e *productionEnvImpl) OverrideConfig(c *config.ApplicationConfig) error { - return nil -} - -func (e *productionEnvImpl) OverrideServices(s *Services) error { - return nil -} - -func (e *productionEnvImpl) OverrideHandlers(h *Handlers) error { - return nil -} - -func (e *productionEnvImpl) EnvironmentDefaults() map[string]string { - return map[string]string{ - "v": "1", - } -} diff --git a/cmd/hyperfleet-api/environments/e_unit_testing.go b/cmd/hyperfleet-api/environments/e_unit_testing.go deleted file mode 100755 index 5b83d430..00000000 --- a/cmd/hyperfleet-api/environments/e_unit_testing.go +++ /dev/null @@ -1,49 +0,0 @@ -package environments - -import ( - "os" - - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" - dbmocks "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db/mocks" -) - -var _ EnvironmentImpl = &unitTestingEnvImpl{} - -// unitTestingEnvImpl is configuration for unit tests using mocked database -type unitTestingEnvImpl struct { - env *Env -} - -func (e *unitTestingEnvImpl) OverrideDatabase(c *Database) error { - c.SessionFactory = dbmocks.NewMockSessionFactory() - return nil -} - -func (e *unitTestingEnvImpl) OverrideConfig(c *config.ApplicationConfig) error { - // Support a one-off env to allow enabling db debug in testing - //nolint:goconst // "true" is not extracted to constant (standard env var idiom) - if os.Getenv("HYPERFLEET_DATABASE_DEBUG") == "true" { - c.Database.Debug = true - } - - // Ensure SSL mode is set to disable for testing - if c.Database.SSL.Mode == "" { - c.Database.SSL.Mode = SSLModeDisable - } - // Unit tests use a mock DB and don't need real credentials - return nil -} - -func (e *unitTestingEnvImpl) OverrideServices(s *Services) error { - return nil -} - -func (e *unitTestingEnvImpl) OverrideHandlers(h *Handlers) error { - return nil -} - -func (e *unitTestingEnvImpl) EnvironmentDefaults() map[string]string { - // Return empty map - new config system has appropriate defaults - // and OverrideConfig() sets test-specific values programmatically - return map[string]string{} -} diff --git a/cmd/hyperfleet-api/environments/framework.go b/cmd/hyperfleet-api/environments/framework.go deleted file mode 100755 index ebf0c7ab..00000000 --- a/cmd/hyperfleet-api/environments/framework.go +++ /dev/null @@ -1,134 +0,0 @@ -package environments - -import ( - "context" - "os" - - "github.com/spf13/pflag" - - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments/registry" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" -) - -func init() { - once.Do(func() { - environment = &Env{} - - // Config must be set by caller using ConfigLoader before Initialize() - environment.Name = GetEnvironmentStrFromEnv() - - environments = map[string]EnvironmentImpl{ - DevelopmentEnv: &devEnvImpl{environment}, - UnitTestingEnv: &unitTestingEnvImpl{environment}, - IntegrationTestingEnv: &integrationTestingEnvImpl{environment}, - ProductionEnv: &productionEnvImpl{environment}, - } - }) -} - -// EnvironmentImpl defines a set of behaviors for a runtime environment. -// Each environment provides a set of flags for basic set/override of the environment -// and configuration functions for each component type. -type EnvironmentImpl interface { - EnvironmentDefaults() map[string]string - OverrideConfig(c *config.ApplicationConfig) error - OverrideServices(s *Services) error - OverrideDatabase(s *Database) error - OverrideHandlers(c *Handlers) error -} - -func GetEnvironmentStrFromEnv() string { - envStr, specified := os.LookupEnv(EnvironmentStringKey) - if !specified || envStr == "" { - envStr = EnvironmentDefault - } - return envStr -} - -func Environment() *Env { - return environment -} - -// SetEnvironmentDefaults sets environment-specific flag defaults -func (e *Env) SetEnvironmentDefaults(flags *pflag.FlagSet) error { - return setFlagDefaults(flags, environments[e.Name].EnvironmentDefaults()) -} - -// Initialize loads the environment's resources -// This should be called after the e.Config has been set appropriately though AddFlags and parsing, done elsewhere -// The environment does NOT handle flag parsing -func (e *Env) Initialize() error { - ctx := context.Background() - - // Re-read environment name from env var to support tests that set HYPERFLEET_ENV after init() - envName := GetEnvironmentStrFromEnv() - e.Name = envName - - logger.With(ctx, logger.FieldEnvironment, e.Name).Info("Initializing environment") - - envImpl, found := environments[e.Name] - if !found { - logger.With(ctx, logger.FieldEnvironment, e.Name).Error("Unknown runtime environment") - os.Exit(1) - } - - if err := envImpl.OverrideConfig(e.Config); err != nil { - logger.WithError(ctx, err).Error("Failed to configure ApplicationConfig") - os.Exit(1) - } - - // each env will set db explicitly because the DB impl has a `once` init section - if err := envImpl.OverrideDatabase(&e.Database); err != nil { - logger.WithError(ctx, err).Error("Failed to configure Database") - os.Exit(1) - } - - e.LoadServices() - if err := envImpl.OverrideServices(&e.Services); err != nil { - logger.WithError(ctx, err).Error("Failed to configure Services") - os.Exit(1) - } - - seedErr := e.Seed() - if seedErr != nil { - return seedErr - } - - if err := envImpl.OverrideHandlers(&e.Handlers); err != nil { - logger.WithError(ctx, err).Error("Failed to configure Handlers") - os.Exit(1) - } - - return nil -} - -func (e *Env) Seed() *errors.ServiceError { - return nil -} - -func (e *Env) LoadServices() { - e.Services.serviceRegistry = make(map[string]interface{}) - registry.LoadDiscoveredServices(&e.Services, e) -} - -func (e *Env) Teardown() { - ctx := context.Background() - if e.Database.SessionFactory != nil { - if err := e.Database.SessionFactory.Close(); err != nil { - logger.WithError(ctx, err).Error("Error closing database session factory") - } - } -} - -func setFlagDefaults(flags *pflag.FlagSet, defaults map[string]string) error { - ctx := context.Background() - for name, value := range defaults { - if err := flags.Set(name, value); err != nil { - logger.With(ctx, logger.FieldFlag, name).WithError(err).Error("Error setting flag") - return err - } - } - return nil -} diff --git a/cmd/hyperfleet-api/environments/framework_test.go b/cmd/hyperfleet-api/environments/framework_test.go deleted file mode 100755 index a7316ca4..00000000 --- a/cmd/hyperfleet-api/environments/framework_test.go +++ /dev/null @@ -1,60 +0,0 @@ -package environments - -import ( - "reflect" - "testing" - - . "github.com/onsi/gomega" - "github.com/spf13/pflag" - - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" -) - -func TestLoadServices(t *testing.T) { - // Set environment to unit_testing to use mocks - t.Setenv("HYPERFLEET_ENV", "unit_testing") - - // Create minimal configuration for unit test - cfg := config.NewApplicationConfig() - - env := Environment() - env.Config = cfg - - err := env.SetEnvironmentDefaults(pflag.CommandLine) - if err != nil { - t.Errorf("Unable to add flags for testing environment: %s", err.Error()) - return - } - pflag.Parse() - err = env.Initialize() - if err != nil { - t.Errorf("Unable to load testing environment: %s", err.Error()) - return - } - - s := reflect.ValueOf(&env.Services).Elem() - sType := s.Type() - - for i := 0; i < s.NumField(); i++ { - field := s.Field(i) - fieldType := sType.Field(i) - - // Skip unexported fields (lowercase first letter) - if !fieldType.IsExported() { - continue - } - - // Only check fields that are function types (service locators) - if field.Kind() == reflect.Func && field.IsNil() { - t.Errorf("Service locator %s is nil", fieldType.Name) - } - } -} - -func TestEnvironmentDefaultIsProduction(t *testing.T) { - RegisterTestingT(t) - Expect(EnvironmentDefault).To( - Equal(ProductionEnv), - "EnvironmentDefault must be ProductionEnv for secure-by-default behavior", - ) -} diff --git a/cmd/hyperfleet-api/environments/registry/registry.go b/cmd/hyperfleet-api/environments/registry/registry.go deleted file mode 100755 index 46a9dd64..00000000 --- a/cmd/hyperfleet-api/environments/registry/registry.go +++ /dev/null @@ -1,42 +0,0 @@ -package registry - -import ( - "sync" -) - -// ServiceLocatorFunc is a function that creates a service locator -type ServiceLocatorFunc func(env interface{}) interface{} - -// ServiceRegistry holds registered services -type ServiceRegistry struct { - services map[string]ServiceLocatorFunc - mu sync.RWMutex -} - -var globalRegistry = &ServiceRegistry{ - services: make(map[string]ServiceLocatorFunc), -} - -// RegisterService registers a service with the global registry -func RegisterService(name string, locatorFunc ServiceLocatorFunc) { - globalRegistry.mu.Lock() - defer globalRegistry.mu.Unlock() - globalRegistry.services[name] = locatorFunc -} - -// ServicesInterface defines the interface for the Services struct -type ServicesInterface interface { - SetService(name string, service interface{}) -} - -// LoadDiscoveredServices loads all registered services into the Services struct -func LoadDiscoveredServices(services ServicesInterface, env interface{}) { - globalRegistry.mu.RLock() - defer globalRegistry.mu.RUnlock() - - for name, locatorFunc := range globalRegistry.services { - // Call the locator function to create the service and store it in the registry - serviceLocator := locatorFunc(env) - services.SetService(name, serviceLocator) - } -} diff --git a/cmd/hyperfleet-api/environments/types.go b/cmd/hyperfleet-api/environments/types.go deleted file mode 100755 index 35e71396..00000000 --- a/cmd/hyperfleet-api/environments/types.go +++ /dev/null @@ -1,68 +0,0 @@ -package environments - -import ( - "sync" - - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" -) - -const ( - UnitTestingEnv string = "unit_testing" - IntegrationTestingEnv string = "integration_testing" - DevelopmentEnv string = "development" - ProductionEnv string = "production" - - EnvironmentStringKey string = "HYPERFLEET_ENV" - EnvironmentDefault = ProductionEnv - - // Database SSL modes - SSLModeDisable string = "disable" -) - -type Env struct { - Handlers Handlers - Database Database - Config *config.ApplicationConfig - Name string - Services Services -} - -type ApplicationConfig struct { - ApplicationConfig *config.ApplicationConfig -} - -type Database struct { - SessionFactory db.SessionFactory -} - -type Handlers struct{} - -type Services struct { - serviceRegistry map[string]interface{} - mutex sync.RWMutex -} - -func (s *Services) GetService(name string) interface{} { - s.mutex.RLock() - defer s.mutex.RUnlock() - if s.serviceRegistry == nil { - return nil - } - return s.serviceRegistry[name] -} - -func (s *Services) SetService(name string, service interface{}) { - s.mutex.Lock() - defer s.mutex.Unlock() - if s.serviceRegistry == nil { - s.serviceRegistry = make(map[string]interface{}) - } - s.serviceRegistry[name] = service -} - -var ( - environment *Env - once sync.Once - environments map[string]EnvironmentImpl -) diff --git a/cmd/hyperfleet-api/servecmd/api_server.go b/cmd/hyperfleet-api/servecmd/api_server.go index da22c6e2..8a5bcc28 100644 --- a/cmd/hyperfleet-api/servecmd/api_server.go +++ b/cmd/hyperfleet-api/servecmd/api_server.go @@ -21,9 +21,6 @@ import ( // Middleware slices that depend on runtime decisions (tracing on/off, auth // on/off) are built here rather than inside the server package, so that package // stays free of both pkg/config and those decisions. -// -// jwtHandler may be nil when cfg.Server.JWT.Enabled is false. When JWT is -// enabled, jwtHandler must be non-nil or BuildAPIServer returns an error. func BuildAPIServer( cfg *config.ApplicationConfig, resourceService services.ResourceService, @@ -31,22 +28,19 @@ func BuildAPIServer( schemaValidator *validators.SchemaValidator, jwtHandler *auth.JWTHandler, sessionFactory db.SessionFactory, - tracingEnabled bool, ) (*server.APIServer, error) { mainMiddleware := []server.Middleware{logger.RequestIDMiddleware} - if tracingEnabled { + if cfg.Tracing.Enabled { mainMiddleware = append(mainMiddleware, middleware.OTelMiddleware) } masker := middleware.NewMaskingMiddleware(cfg.Logging) mainMiddleware = append(mainMiddleware, requestlogging.RequestLoggingMiddleware(masker)) - // Applied to every API route (public and protected) - observability and compression. apiMiddleware := []server.Middleware{ server.MetricsMiddleware, server.CompressMiddleware, } - // Applied only behind auth - schema validation and DB transaction. protectedAPIMiddleware := []server.Middleware{ middleware.SchemaValidationMiddleware(schemaValidator), func(next http.Handler) http.Handler { diff --git a/cmd/hyperfleet-api/servecmd/cmd.go b/cmd/hyperfleet-api/servecmd/cmd.go index 6f710d55..01fd3ef8 100755 --- a/cmd/hyperfleet-api/servecmd/cmd.go +++ b/cmd/hyperfleet-api/servecmd/cmd.go @@ -2,21 +2,22 @@ package servecmd import ( "context" + "errors" + "fmt" "log/slog" "os" "os/signal" - "strconv" "syscall" + "time" "github.com/spf13/cobra" - "go.opentelemetry.io/otel/sdk/trace" "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/container" - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments" "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/server" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/auth" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/closer" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db/db_session" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/health" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" @@ -25,12 +26,23 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/telemetry" ) +const ( + // Fixed drain budgets for lightweight servers and OTel flush. + // terminationGracePeriodSeconds must be > preStop (5s) + shutdown_timeout + // + metricsDrainTimeout + healthDrainTimeout + otelFlushTimeout. + metricsDrainTimeout = 2 * time.Second + healthDrainTimeout = 2 * time.Second + otelFlushTimeout = 5 * time.Second +) + func NewServeCommand() *cobra.Command { cmd := &cobra.Command{ Use: "serve", Short: "Serve the hyperfleet", Long: "Serve the hyperfleet.", - Run: runServe, + RunE: runServe, + // runServe errors are runtime failures, not CLI misuse - don't dump usage on them. + SilenceUsage: true, } // Add configuration system flags @@ -39,226 +51,180 @@ func NewServeCommand() *cobra.Command { return cmd } -func runServe(cmd *cobra.Command, args []string) { - ctx := context.Background() +func runServe(cmd *cobra.Command, args []string) (runErr error) { + ctx := cmd.Context() + signals := make(chan os.Signal, 1) + signal.Notify(signals, os.Interrupt, syscall.SIGTERM) + defer signal.Stop(signals) - // ============================================================ - // CONFIGURATION LOADING - // ============================================================ - // Load configuration using Viper-based system loader := config.NewConfigLoader() cfg, err := loader.Load(ctx, cmd) if err != nil { - logger.WithError(ctx, err).Error("Failed to load configuration") - os.Exit(1) + return fmt.Errorf("load configuration: %w", err) } - // IMPORTANT: Set config BEFORE calling Initialize() - // Initialize() will apply environment-specific overrides (e.g., development disables JWT/TLS) - // and ensure SessionFactory, clients, services, handlers all use the correct config - environments.Environment().Config = cfg - - // Load entity descriptors from config before services and routes are built. - // Descriptors must be registered before Initialize() because services call - // registry.MustGet() at construction time. registry.LoadDescriptors(cfg.Entities) registry.Validate() - // Initialize environment (applies overrides, creates SessionFactory, loads clients, services, handlers) - err = environments.Environment().Initialize() - if err != nil { - logger.WithError(ctx, err).Error("Unable to initialize environment") - os.Exit(1) - } - - // Initialize logger with configured settings - initLogger() - - // Log effective configuration (with sensitive values redacted) - // This happens AFTER initLogger() so it uses the configured logger settings - logger.Info(ctx, "Starting HyperFleet API with configuration (sensitive values redacted):") - logger.Info(ctx, config.DumpConfig(environments.Environment().Config)) - - var tp *trace.TracerProvider + c := closer.New() + defer func() { + closeErr := c.Close() + runErr = errors.Join(runErr, closeErr) + if runErr == nil { + logger.Info(context.Background(), "Graceful shutdown completed") + } + }() - // Check for deprecated HYPERFLEET_LOGGING_OTEL_ENABLED variable - if deprecatedEnv := os.Getenv("HYPERFLEET_LOGGING_OTEL_ENABLED"); deprecatedEnv != "" { - logger.With(ctx, - "deprecated_variable", "HYPERFLEET_LOGGING_OTEL_ENABLED", - "replacement", "HYPERFLEET_TRACING_ENABLED", - ).Warn("HYPERFLEET_LOGGING_OTEL_ENABLED is deprecated and ignored. Please use HYPERFLEET_TRACING_ENABLED instead.") - } + ctr := container.NewContainer(cfg, c) - // Check for deprecated HYPERFLEET_LOGGING_OTEL_SAMPLING_RATE variable - if deprecatedEnv := os.Getenv("HYPERFLEET_LOGGING_OTEL_SAMPLING_RATE"); deprecatedEnv != "" { - logger.With(ctx, - "deprecated_variable", "HYPERFLEET_LOGGING_OTEL_SAMPLING_RATE", - "replacement", "OTEL_TRACES_SAMPLER_ARG", - ).Warn("HYPERFLEET_LOGGING_OTEL_SAMPLING_RATE is deprecated and ignored. Please use OTEL_TRACES_SAMPLER_ARG instead.") - } + initLogger(cfg) - // Determine if tracing is enabled using HYPERFLEET_TRACING_ENABLED (tracing standard) - var tracingEnabled bool - if tracingEnv := os.Getenv("HYPERFLEET_TRACING_ENABLED"); tracingEnv != "" { - if enabled, err := strconv.ParseBool(tracingEnv); err == nil { - tracingEnabled = enabled - } else { - logger.With(ctx, - logger.FieldHyperfleetTracingEnabled, tracingEnv, - "falling_back_to", environments.Environment().Config.Logging.OTel.Enabled). - WithError(err).Warn("Invalid HYPERFLEET_TRACING_ENABLED value, falling back to config") - tracingEnabled = environments.Environment().Config.Logging.OTel.Enabled - } - } else { - // Use config default if HYPERFLEET_TRACING_ENABLED not set - tracingEnabled = environments.Environment().Config.Logging.OTel.Enabled - } + sf := ctr.SessionFactory() + configureDBLogger(cfg, sf) - if tracingEnabled { - // OpenTelemetry configuration is driven entirely by standard environment variables: - serviceName := "hyperfleet-api" - if svcName := os.Getenv("OTEL_SERVICE_NAME"); svcName != "" { - serviceName = svcName - } + logger.Info(ctx, "Starting HyperFleet API with configuration (sensitive values redacted):") + logger.Info(ctx, config.DumpConfig(cfg)) - traceProvider, err := telemetry.InitTraceProvider(ctx, serviceName, api.Version) - if err != nil { - logger.WithError(ctx, err).Warn("Failed to initialize OpenTelemetry") + // OTel registered first so it flushes last - teardown spans are preserved. + if cfg.Tracing.Enabled { + traceProvider, traceErr := telemetry.InitTraceProvider(ctx, cfg.Tracing.ServiceName, api.Version) + if traceErr != nil { + logger.WithError(ctx, traceErr).Warn("Failed to initialize OpenTelemetry") } else { - tp = traceProvider - logger.With(ctx, logger.FieldServiceName, serviceName).Info("OpenTelemetry initialized") + logger.With(ctx, logger.FieldServiceName, cfg.Tracing.ServiceName).Info("OpenTelemetry initialized") + c.Add(func() error { + flushCtx, cancel := context.WithTimeout(context.Background(), otelFlushTimeout) + defer cancel() + return telemetry.Shutdown(flushCtx, traceProvider) + }) } } else { logger.With(ctx, logger.FieldOTelEnabled, false).Info("OpenTelemetry disabled") } logger.With(ctx, - "log_level", environments.Environment().Config.Logging.Level, - "log_format", environments.Environment().Config.Logging.Format, - "log_output", environments.Environment().Config.Logging.Output, - "masking_enabled", environments.Environment().Config.Logging.Masking.Enabled, + "log_level", cfg.Logging.Level, + "log_format", cfg.Logging.Format, + "log_output", cfg.Logging.Output, + "masking_enabled", cfg.Logging.Masking.Enabled, ).Info("Logger initialized") - if sf := environments.Environment().Database.SessionFactory; sf != nil { - if err := metrics.RegisterReconciliationCollector( - sf.DirectDB(), - environments.Environment().Config.Metrics.ReconciliationStuckThreshold, - ); err != nil { - logger.WithError(ctx, err).Error("Failed to register reconciliation collector") - } - } - - ctr := container.NewContainer(cfg, environments.Environment().Database.SessionFactory) - - // Only build the JWT handler when auth is on; it starts a JWKS refresh goroutine. - var jwtHandler *auth.JWTHandler - if cfg.Server.JWT.Enabled { - var jwtErr error - jwtHandler, jwtErr = ctr.JWTHandler() - if jwtErr != nil { - logger.WithError(ctx, jwtErr).Error("Unable to create JWT handler") - os.Exit(1) - } + if collectorErr := metrics.RegisterReconciliationCollector( + ctr.SessionFactory().DirectDB(), + cfg.Metrics.ReconciliationStuckThreshold, + ); collectorErr != nil { + logger.WithError(ctx, collectorErr).Error("Failed to register reconciliation collector") } - schemaValidator, schemaErr := ctr.SchemaValidator() - if schemaErr != nil { - logger.WithError(ctx, schemaErr).Error("Unable to create schema validator") - os.Exit(1) - } + jwtHandler := ctr.JWTHandler() - apiServer, buildErr := BuildAPIServer( + apiServer, err := BuildAPIServer( cfg, ctr.ResourceService(), ctr.AdapterStatusService(), - schemaValidator, + ctr.SchemaValidator(), jwtHandler, ctr.SessionFactory(), - tracingEnabled, ) - if buildErr != nil { - logger.WithError(ctx, buildErr).Error("Unable to build API server") - os.Exit(1) - } - go apiServer.Start() - - metricsServer := server.NewMetricsServer() - go metricsServer.Start() - - healthServer := server.NewHealthServer() - go healthServer.Start() - - // Wait for health server to be listening before marking as ready - if notifier, ok := healthServer.(server.ListenNotifier); ok { - <-notifier.NotifyListening() + if err != nil { + return fmt.Errorf("build API server: %w", err) + } + // Do NOT register srv.Close bare - it severs in-flight requests without + // draining. addDrain uses Shutdown with a budget, falling back to Close. + addDrain(c, apiServer, cfg.Health.ShutdownTimeout) + + metricsServer := server.NewMetricsServer(cfg.Metrics) + addDrain(c, metricsServer, metricsDrainTimeout) + + healthServer := server.NewHealthServer(cfg.Health, ctr.SessionFactory()) + addDrain(c, healthServer, healthDrainTimeout) + + // Readyz registered last so it runs first - immediately fails the probe. + c.Add(func() error { + health.GetReadinessState().SetShuttingDown() + logger.Info(context.Background(), "Marked as not ready, draining in-flight requests...") + return nil + }) + + serverResults := make(chan error, 3) + start := func(name string, srv server.Server) { + go func() { + if err := srv.Start(); err != nil { + serverResults <- fmt.Errorf("%s server failed: %w", name, err) + } + }() + } + start("API", apiServer) + start("metrics", metricsServer) + start("health", healthServer) + + allListening := make(chan struct{}) + go func() { + <-apiServer.NotifyListening() + <-metricsServer.NotifyListening() + <-healthServer.NotifyListening() + close(allListening) + }() + + var triggerErr error + shutdown := false + select { + case <-ctx.Done(): + shutdown = true + case <-signals: + shutdown = true + case triggerErr = <-serverResults: + case <-allListening: + } + if triggerErr == nil && !shutdown { + health.GetReadinessState().SetReady() + logger.Info(ctx, "Application ready to receive traffic") + select { + case <-ctx.Done(): + case <-signals: + case triggerErr = <-serverResults: + } } - // Mark application as ready to receive traffic - health.GetReadinessState().SetReady() - logger.Info(ctx, "Application ready to receive traffic") - - sigChan := make(chan os.Signal, 1) - signal.Notify(sigChan, os.Interrupt, syscall.SIGTERM) - <-sigChan - - logger.Info(ctx, "Shutdown signal received, starting graceful shutdown...") - - // Mark application as not ready (returns 503 on /readyz) - health.GetReadinessState().SetShuttingDown() - logger.Info(ctx, "Marked as not ready, draining in-flight requests...") - - if err := healthServer.Stop(); err != nil { - logger.WithError(ctx, err).Error("Failed to stop health server") - } - if err := apiServer.Stop(); err != nil { - logger.WithError(ctx, err).Error("Failed to stop API server") - } - if err := metricsServer.Stop(); err != nil { - logger.WithError(ctx, err).Error("Failed to stop metrics server") - } - ctr.Close() + logger.Info(context.Background(), "Shutdown requested, starting graceful shutdown...") + runErr = triggerErr + return runErr +} - if tp != nil { - shutdownCtx, cancel := context.WithTimeout( - context.Background(), environments.Environment().Config.Health.ShutdownTimeout, - ) +func addDrain(c *closer.Closer, srv server.Server, budget time.Duration) { + c.Add(func() error { + drainCtx, cancel := context.WithTimeout(context.Background(), budget) defer cancel() - if err := telemetry.Shutdown(shutdownCtx, tp); err != nil { - logger.WithError(ctx, err).Error("Failed to shutdown OpenTelemetry") + if err := srv.Shutdown(drainCtx); err != nil { + return errors.Join(err, srv.Close()) } - } - - // Close database connections - environments.Environment().Teardown() - - logger.Info(ctx, "Graceful shutdown completed") + return nil + }) } -// initLogger initializes the global slog logger from configuration -func initLogger() { +func initLogger(cfg *config.ApplicationConfig) { ctx := context.Background() - cfg := environments.Environment().Config.Logging + loggingCfg := cfg.Logging - level, err := logger.ParseLogLevel(cfg.Level) + level, err := logger.ParseLogLevel(loggingCfg.Level) if err != nil { - logger.With(ctx, logger.FieldLogLevel, cfg.Level).WithError(err).Warn("Invalid log level, using default") + logger.With(ctx, logger.FieldLogLevel, loggingCfg.Level).WithError(err).Warn("Invalid log level, using default") level = slog.LevelInfo } - format, err := logger.ParseLogFormat(cfg.Format) + format, err := logger.ParseLogFormat(loggingCfg.Format) if err != nil { - logger.With(ctx, logger.FieldLogFormat, cfg.Format).WithError(err).Warn("Invalid log format, using default") + logger.With(ctx, logger.FieldLogFormat, loggingCfg.Format).WithError(err).Warn("Invalid log format, using default") format = logger.FormatJSON } - output, err := logger.ParseLogOutput(cfg.Output) + output, err := logger.ParseLogOutput(loggingCfg.Output) if err != nil { - logger.With(ctx, logger.FieldLogOutput, cfg.Output).WithError(err).Warn("Invalid log output, using default") + logger.With(ctx, logger.FieldLogOutput, loggingCfg.Output).WithError(err).Warn("Invalid log output, using default") output = os.Stdout } - // Use configured hostname with fallback to os.Hostname() - hostname := environments.Environment().Config.Server.Hostname + hostname := cfg.Server.Hostname if hostname == "" { hostname, _ = os.Hostname() //nolint:errcheck // empty string is acceptable fallback } @@ -275,15 +241,11 @@ func initLogger() { // Use ReconfigureGlobalLogger instead of InitGlobalLogger because // InitGlobalLogger was already called in main() with default config logger.ReconfigureGlobalLogger(logConfig) +} - // Reconfigure database logger to follow global logging level - dbSessionFactory := environments.Environment().Database.SessionFactory - if dbSessionFactory != nil { - gormLevel := environments.Environment().Config.Database.SetLogLevel( - environments.Environment().Config.Logging.Level, - ) - if reconfigurable, ok := dbSessionFactory.(db_session.LoggerReconfigurable); ok { - reconfigurable.ReconfigureLogger(gormLevel) - } +func configureDBLogger(cfg *config.ApplicationConfig, sessionFactory db.SessionFactory) { + gormLevel := cfg.Database.SetLogLevel(cfg.Logging.Level) + if reconfigurable, ok := sessionFactory.(db_session.LoggerReconfigurable); ok { + reconfigurable.ReconfigureLogger(gormLevel) } } diff --git a/cmd/hyperfleet-api/server/api_server.go b/cmd/hyperfleet-api/server/api_server.go index d2bd8a04..47ea8b0d 100755 --- a/cmd/hyperfleet-api/server/api_server.go +++ b/cmd/hyperfleet-api/server/api_server.go @@ -1,93 +1,34 @@ package server import ( - "context" - "fmt" - "net" "net/http" - "os" "time" - - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" ) type cfg interface { + tlsCfg BindAddress() string ReadTimeout() time.Duration WriteTimeout() time.Duration - TLSEnabled() bool - TLSCertFile() string - TLSKeyFile() string } type APIServer struct { - cfg cfg - httpServer *http.Server + baseServer } func NewAPIServer(cfg cfg, handler http.Handler) *APIServer { return &APIServer{ - cfg: cfg, - httpServer: &http.Server{ - Addr: cfg.BindAddress(), - Handler: removeTrailingSlash(handler), - ReadTimeout: cfg.ReadTimeout(), - WriteTimeout: cfg.WriteTimeout(), - ReadHeaderTimeout: 10 * time.Second, // Hardcoded to prevent Slowloris attacks (not user-configurable) + baseServer: baseServer{ + name: "API server", + cfg: cfg, + listening: make(chan struct{}), + httpServer: &http.Server{ + Addr: cfg.BindAddress(), + Handler: removeTrailingSlash(handler), + ReadTimeout: cfg.ReadTimeout(), + WriteTimeout: cfg.WriteTimeout(), + ReadHeaderTimeout: 10 * time.Second, + }, }, } } - -// Serve start the blocking call to Serve. -// Useful for breaking up ListenAndServer (Start) when you require the server to be listening before continuing -func (s *APIServer) Serve(listener net.Listener) { - ctx := context.Background() - var err error - if s.cfg.TLSEnabled() { - if s.cfg.TLSCertFile() == "" || s.cfg.TLSKeyFile() == "" { - check( - fmt.Errorf( - "HTTPS certificate or key not configured; "+ - "set via server.tls.cert_file/key_file in config file, env vars, or flags", - ), - "Can't start https server", - ) - } - - logger.With(ctx, logger.FieldBindAddress, s.cfg.BindAddress()).Info("Serving with TLS") - err = s.httpServer.ServeTLS(listener, s.cfg.TLSCertFile(), s.cfg.TLSKeyFile()) - } else { - logger.With(ctx, logger.FieldBindAddress, s.cfg.BindAddress()).Info("Serving without TLS") - err = s.httpServer.Serve(listener) - } - - if err != nil && err != http.ErrServerClosed { - check(err, "Web server terminated with errors") - } else { - logger.Info(ctx, "Web server terminated") - } -} - -// Listen only start the listener, not the server. -// Useful for breaking up ListenAndServer (Start) when you require the server to be listening before continuing -func (s *APIServer) Listen() (listener net.Listener, err error) { - return net.Listen("tcp", s.cfg.BindAddress()) -} - -// Start listening on the configured port and start the server. -// This is a convenience wrapper for Listen() and Serve(listener Listener) -func (s *APIServer) Start() { - ctx := context.Background() - listener, err := s.Listen() - if err != nil { - logger.WithError(ctx, err).Error(fmt.Sprintf("Unable to start API server on %s", s.cfg.BindAddress())) - os.Exit(1) - } - s.Serve(listener) -} - -func (s *APIServer) Stop() error { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - return s.httpServer.Shutdown(ctx) -} diff --git a/cmd/hyperfleet-api/server/api_server_test.go b/cmd/hyperfleet-api/server/api_server_test.go index bad263ec..a5fd568f 100644 --- a/cmd/hyperfleet-api/server/api_server_test.go +++ b/cmd/hyperfleet-api/server/api_server_test.go @@ -1,6 +1,7 @@ package server import ( + "context" "crypto/rand" "crypto/rsa" "crypto/tls" @@ -79,7 +80,9 @@ func TestAPIServerServeWithoutTLS(t *testing.T) { t.Cleanup(func() { _ = resp.Body.Close() }) Expect(resp.StatusCode).To(Equal(http.StatusNoContent)) - Expect(s.Stop()).To(Succeed()) + shutdownCtx, cancel := context.WithTimeout(t.Context(), time.Second) + defer cancel() + Expect(s.Shutdown(shutdownCtx)).To(Succeed()) <-done } @@ -128,10 +131,88 @@ func TestAPIServerServeWithTLS(t *testing.T) { t.Cleanup(func() { _ = resp.Body.Close() }) Expect(resp.StatusCode).To(Equal(http.StatusAccepted)) - Expect(s.Stop()).To(Succeed()) + shutdownCtx, cancel := context.WithTimeout(t.Context(), time.Second) + defer cancel() + Expect(s.Shutdown(shutdownCtx)).To(Succeed()) <-done } +func TestAPIServerShutdownDrainsInFlightRequest(t *testing.T) { + RegisterTestingT(t) + + listener, err := net.Listen("tcp", "127.0.0.1:0") + Expect(err).NotTo(HaveOccurred()) + + requestStarted := make(chan struct{}) + releaseRequest := make(chan struct{}) + s := NewAPIServer( + testAPIServerConfig{bindAddress: listener.Addr().String()}, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + close(requestStarted) + <-releaseRequest + w.WriteHeader(http.StatusNoContent) + }), + ) + serveDone := make(chan error, 1) + go func() { + serveDone <- s.Serve(listener) + }() + + requestDone := make(chan error, 1) + go func() { + resp, requestErr := http.Get("http://" + listener.Addr().String()) + if requestErr == nil { + requestErr = resp.Body.Close() + } + requestDone <- requestErr + }() + <-requestStarted + + shutdownCtx, cancel := context.WithTimeout(t.Context(), time.Second) + defer cancel() + shutdownDone := make(chan error, 1) + go func() { + shutdownDone <- s.Shutdown(shutdownCtx) + }() + + Consistently(shutdownDone, "100ms").ShouldNot(Receive()) + close(releaseRequest) + Eventually(shutdownDone).Should(Receive(Succeed())) + Eventually(requestDone).Should(Receive(Succeed())) + Eventually(serveDone).Should(Receive(Succeed())) +} + +func TestAPIServerCloseCancelsRequestAfterDrainTimeout(t *testing.T) { + RegisterTestingT(t) + + listener, err := net.Listen("tcp", "127.0.0.1:0") + Expect(err).NotTo(HaveOccurred()) + + requestStarted := make(chan struct{}) + requestCanceled := make(chan struct{}) + s := NewAPIServer( + testAPIServerConfig{bindAddress: listener.Addr().String()}, + http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + close(requestStarted) + <-r.Context().Done() + close(requestCanceled) + }), + ) + go func() { + _ = s.Serve(listener) + }() + go func() { + _, _ = http.Get("http://" + listener.Addr().String()) + }() + <-requestStarted + + shutdownCtx, cancel := context.WithTimeout(t.Context(), 25*time.Millisecond) + defer cancel() + Expect(s.Shutdown(shutdownCtx)).To(MatchError(context.DeadlineExceeded)) + Expect(s.Close()).To(Succeed()) + Eventually(requestCanceled).Should(BeClosed()) +} + func writeSelfSignedCert(t *testing.T) (string, string) { t.Helper() diff --git a/cmd/hyperfleet-api/server/health_server.go b/cmd/hyperfleet-api/server/health_server.go index a912eaf5..d0a7df48 100644 --- a/cmd/hyperfleet-api/server/health_server.go +++ b/cmd/hyperfleet-api/server/health_server.go @@ -1,96 +1,46 @@ package server import ( - "context" - "fmt" - "net" "net/http" "time" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/health" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" ) -func NewHealthServer() Server { +type healthCfg interface { + tlsCfg + BindAddress() string + PingTimeout() time.Duration +} + +func NewHealthServer(cfg healthCfg, sessionFactory db.SessionFactory) *HealthServer { mainRouter := http.NewServeMux() - // health endpoints (HyperFleet standard) - healthHandler := health.NewHandler(env().Database.SessionFactory, env().Config.Health.DBPingTimeout) + healthHandler := health.NewHandler(sessionFactory, cfg.PingTimeout()) mainRouter.HandleFunc("GET /healthz", healthHandler.LivenessHandler) mainRouter.HandleFunc("GET /readyz", healthHandler.ReadinessHandler) mainHandler := WithNotFoundHandler(mainRouter) - s := &healthServer{ - shutdownTimeout: env().Config.Health.ShutdownTimeout, - listening: make(chan struct{}), - } - s.httpServer = &http.Server{ - Addr: env().Config.Health.BindAddress(), - Handler: mainHandler, - ReadHeaderTimeout: 10 * time.Second, + s := &HealthServer{ + baseServer: baseServer{ + name: "health server", + cfg: cfg, + listening: make(chan struct{}), + httpServer: &http.Server{ + Addr: cfg.BindAddress(), + Handler: mainHandler, + ReadTimeout: 5 * time.Second, + WriteTimeout: 10 * time.Second, + IdleTimeout: 60 * time.Second, + ReadHeaderTimeout: 10 * time.Second, + }, + }, } return s } -type healthServer struct { - httpServer *http.Server - listening chan struct{} - shutdownTimeout time.Duration -} - -var _ Server = &healthServer{} - -func (s *healthServer) Listen() (listener net.Listener, err error) { - return net.Listen("tcp", s.httpServer.Addr) -} - -func (s *healthServer) Serve(listener net.Listener) { - ctx := context.Background() - var err error - - if env().Config.Health.TLS.Enabled { - if env().Config.Server.TLS.CertFile == "" || env().Config.Server.TLS.KeyFile == "" { - check( - fmt.Errorf("unspecified required --https-cert-file, --https-key-file"), - "Can't start https server", - ) - } - - logger.With(ctx, logger.FieldBindAddress, env().Config.Health.BindAddress()).Info("Serving Health with TLS") - err = s.httpServer.ServeTLS(listener, env().Config.Server.TLS.CertFile, env().Config.Server.TLS.KeyFile) - } else { - logger.With(ctx, logger.FieldBindAddress, env().Config.Health.BindAddress()).Info("Serving Health without TLS") - err = s.httpServer.Serve(listener) - } - if err != nil && err != http.ErrServerClosed { - check(err, "Health server terminated with errors") - } else { - logger.Info(ctx, "Health server terminated") - } -} - -// Start is a convenience wrapper that calls Listen() and Serve() -func (s *healthServer) Start() { - listener, err := s.Listen() - if err != nil { - check(err, "Failed to create health server listener") - return - } - - // Signal that we're listening - close(s.listening) - - s.Serve(listener) -} - -// NotifyListening returns a channel that is closed when the server is listening -func (s *healthServer) NotifyListening() <-chan struct{} { - return s.listening -} - -func (s healthServer) Stop() error { - ctx, cancel := context.WithTimeout(context.Background(), s.shutdownTimeout) - defer cancel() - return s.httpServer.Shutdown(ctx) +type HealthServer struct { + baseServer } diff --git a/cmd/hyperfleet-api/server/metrics_server.go b/cmd/hyperfleet-api/server/metrics_server.go index 5bf51bdf..4b80442a 100755 --- a/cmd/hyperfleet-api/server/metrics_server.go +++ b/cmd/hyperfleet-api/server/metrics_server.go @@ -1,73 +1,42 @@ package server import ( - "context" - "fmt" - "net" "net/http" "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/handlers" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" ) -func NewMetricsServer() Server { +type metricsCfg interface { + tlsCfg + BindAddress() string +} + +func NewMetricsServer(cfg metricsCfg) *MetricsServer { mainRouter := http.NewServeMux() - // metrics endpoint only (health endpoints moved to health_server.go on port 8080) prometheusMetricsHandler := handlers.NewPrometheusMetricsHandler() - mainRouter.Handle("/metrics", prometheusMetricsHandler.Handler()) + mainRouter.Handle("GET /metrics", prometheusMetricsHandler.Handler()) mainHandler := WithNotFoundHandler(mainRouter) - s := &metricsServer{} - s.httpServer = &http.Server{ - Addr: env().Config.Metrics.BindAddress(), - Handler: mainHandler, - ReadHeaderTimeout: 10 * time.Second, - } - return s -} - -type metricsServer struct { - httpServer *http.Server -} - -var _ Server = &metricsServer{} - -func (s metricsServer) Listen() (listener net.Listener, err error) { - return nil, nil -} - -func (s metricsServer) Serve(listener net.Listener) { -} - -func (s metricsServer) Start() { - ctx := context.Background() - var err error - if env().Config.Metrics.TLS.Enabled { - if env().Config.Server.TLS.CertFile == "" || env().Config.Server.TLS.KeyFile == "" { - check( - fmt.Errorf("unspecified required --https-cert-file, --https-key-file"), - "Can't start https server", - ) - } - - logger.With(ctx, logger.FieldBindAddress, env().Config.Metrics.BindAddress()).Info("Serving Metrics with TLS") - err = s.httpServer.ListenAndServeTLS(env().Config.Server.TLS.CertFile, env().Config.Server.TLS.KeyFile) - } else { - logger.With(ctx, logger.FieldBindAddress, env().Config.Metrics.BindAddress()).Info("Serving Metrics without TLS") - err = s.httpServer.ListenAndServe() - } - if err != nil && err != http.ErrServerClosed { - check(err, "Metrics server terminated with errors") - } else { - logger.Info(ctx, "Metrics server terminated") + return &MetricsServer{ + baseServer: baseServer{ + name: "metrics server", + cfg: cfg, + listening: make(chan struct{}), + httpServer: &http.Server{ + Addr: cfg.BindAddress(), + Handler: mainHandler, + ReadTimeout: 5 * time.Second, + WriteTimeout: 10 * time.Second, + IdleTimeout: 60 * time.Second, + ReadHeaderTimeout: 10 * time.Second, + }, + }, } } -func (s metricsServer) Stop() error { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - defer cancel() - return s.httpServer.Shutdown(ctx) +type MetricsServer struct { + baseServer } diff --git a/cmd/hyperfleet-api/server/routes_entities.go b/cmd/hyperfleet-api/server/routes_entities.go index 693746c9..82b27478 100644 --- a/cmd/hyperfleet-api/server/routes_entities.go +++ b/cmd/hyperfleet-api/server/routes_entities.go @@ -1,8 +1,9 @@ package server import ( + "cmp" "fmt" - "sort" + "slices" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/handlers" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" @@ -18,8 +19,7 @@ func NewEntityRouteRegistrar( return RouteRegistrar{ Name: "entities", Register: func(router *Router) error { - RegisterEntityRoutes(router, resourceService, adapterStatusService, schemaValidator) - return nil + return RegisterEntityRoutes(router, resourceService, adapterStatusService, schemaValidator) }, } } @@ -39,27 +39,30 @@ func RegisterEntityRoutes( resourceService services.ResourceService, adapterStatusService services.AdapterStatusService, schemaValidator *validators.SchemaValidator, -) { - registerPerEntityRoutes(router, resourceService, adapterStatusService) +) error { + if err := registerPerEntityRoutes(router, resourceService, adapterStatusService); err != nil { + return fmt.Errorf("register entity routes: %w", err) + } registerRootResourceRoutes(router, resourceService, adapterStatusService, schemaValidator) + return nil } func registerPerEntityRoutes( router *Router, resourceService services.ResourceService, adapterStatusService services.AdapterStatusService, -) { +) error { descriptors := registry.All() - sort.Slice(descriptors, func(i, j int) bool { - return descriptors[i].Kind < descriptors[j].Kind + slices.SortFunc(descriptors, func(a, b registry.EntityDescriptor) int { + return cmp.Compare(a.Kind, b.Kind) }) for _, descriptor := range descriptors { if descriptor.Plural == "resources" { - panic(fmt.Sprintf( + return fmt.Errorf( "entity kind %q uses reserved plural %q which would shadow /resources root endpoint", descriptor.Kind, descriptor.Plural, - )) + ) } h := handlers.NewResourceHandler(descriptor, resourceService) sh := handlers.NewResourceStatusHandler(descriptor, resourceService, adapterStatusService) @@ -70,6 +73,7 @@ func registerPerEntityRoutes( } registerEntityResourceRoutes(router, "/"+descriptor.Plural, h, sh) } + return nil } func registerRootResourceRoutes( diff --git a/cmd/hyperfleet-api/server/server.go b/cmd/hyperfleet-api/server/server.go index d40dd2ec..27a22dcb 100755 --- a/cmd/hyperfleet-api/server/server.go +++ b/cmd/hyperfleet-api/server/server.go @@ -2,35 +2,98 @@ package server import ( "context" + "crypto/tls" + "errors" + "fmt" "net" "net/http" - "os" "strings" - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" ) type Server interface { - Start() - Stop() error - Listen() (net.Listener, error) - Serve(net.Listener) + Start() error + Shutdown(context.Context) error + Close() error + NotifyListening() <-chan struct{} } -// ListenNotifier is an optional interface that servers can implement -// to signal when they are ready to accept connections -type ListenNotifier interface { - NotifyListening() <-chan struct{} +type tlsCfg interface { + TLSEnabled() bool + TLSCertFile() string + TLSKeyFile() string +} + +type baseServer struct { + cfg tlsCfg + httpServer *http.Server + listening chan struct{} + name string +} + +func (s *baseServer) Listen() (net.Listener, error) { + return net.Listen("tcp", s.httpServer.Addr) +} + +func (s *baseServer) Serve(listener net.Listener) error { + ctx := context.Background() + var err error + if s.cfg.TLSEnabled() { + if s.cfg.TLSCertFile() == "" || s.cfg.TLSKeyFile() == "" { + configErr := fmt.Errorf( + "HTTPS certificate or key not configured; " + + "set via tls.cert_file/key_file in config file, env vars, or flags", + ) + return errors.Join(configErr, listener.Close()) + } + + logger.With(ctx, logger.FieldBindAddress, s.httpServer.Addr).Info("Serving " + s.name + " with TLS") + err = s.httpServer.ServeTLS(listener, s.cfg.TLSCertFile(), s.cfg.TLSKeyFile()) + } else { + logger.With(ctx, logger.FieldBindAddress, s.httpServer.Addr).Info("Serving " + s.name + " without TLS") + err = s.httpServer.Serve(listener) + } + if err != nil && !errors.Is(err, http.ErrServerClosed) { + return fmt.Errorf("%s terminated with errors: %w", s.name, err) + } + logger.Info(ctx, s.name+" terminated") + return nil +} + +func (s *baseServer) Start() error { + listener, err := s.Listen() + if err != nil { + return fmt.Errorf("unable to start %s on %s: %w", s.name, s.httpServer.Addr, err) + } + if s.cfg.TLSEnabled() { + if _, err := tls.LoadX509KeyPair(s.cfg.TLSCertFile(), s.cfg.TLSKeyFile()); err != nil { + return errors.Join( + fmt.Errorf("%s: invalid TLS certificate/key: %w", s.name, err), + listener.Close(), + ) + } + } + close(s.listening) + return s.Serve(listener) } -// TODO(HYPERFLEET-1371): env() is the last caller of the global environments -// singleton in this package (used by health_server.go and metrics_server.go). -// APIServer already takes its config via constructor injection (see cfg in -// api_server.go); HealthServer/MetricsServer should follow the same pattern -// once the environments/ package is removed. -func env() *environments.Env { - return environments.Environment() +func (s *baseServer) NotifyListening() <-chan struct{} { + return s.listening +} + +func (s *baseServer) Shutdown(ctx context.Context) error { + if err := s.httpServer.Shutdown(ctx); err != nil { + return fmt.Errorf("%s shutdown: %w", s.name, err) + } + return nil +} + +func (s *baseServer) Close() error { + if err := s.httpServer.Close(); err != nil { + return fmt.Errorf("%s close: %w", s.name, err) + } + return nil } func removeTrailingSlash(next http.Handler) http.Handler { @@ -39,12 +102,3 @@ func removeTrailingSlash(next http.Handler) http.Handler { next.ServeHTTP(w, r) }) } - -// Exit on error -func check(err error, msg string) { - ctx := context.Background() - if err != nil && err != http.ErrServerClosed { - logger.WithError(ctx, err).Error(msg) - os.Exit(1) - } -} diff --git a/docs/authentication.md b/docs/authentication.md index 0a09e647..a03badd4 100644 --- a/docs/authentication.md +++ b/docs/authentication.md @@ -30,7 +30,7 @@ For local development and testing, authentication can be disabled. ### Usage ```bash -# Start service without authentication +# Start service without authentication or TLS make run-no-auth # Access API without tokens diff --git a/docs/deployment.md b/docs/deployment.md index a7d6462b..57408f04 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -473,7 +473,7 @@ Before deploying to production, ensure: ## Production Best Practices -- **Environment**: Use default (ProductionEnv) for production deployments; never set `HYPERFLEET_ENV=development` +- **Configuration**: Use Helm values, config files, flags, or explicit `HYPERFLEET_*` configuration variables - **Database**: Use external managed database (Cloud SQL, RDS, Azure Database) with automated backups - **Secrets**: Store all sensitive data in Kubernetes Secrets, never in ConfigMap or values.yaml - **Authentication**: Enable JWT authentication with `config.server.jwt.enabled=true` diff --git a/docs/development.md b/docs/development.md index 33ff4a51..ced1f1e7 100644 --- a/docs/development.md +++ b/docs/development.md @@ -114,7 +114,7 @@ export HYPERFLEET_DATABASE_SSL_MODE=require # for remote databases make run-no-auth ``` -**Note**: The default runtime environment is `production`. For local development without authentication, use `make run-no-auth` or set `HYPERFLEET_ENV=development` (see [Development Environment Configuration](#development-environment-configuration) below). +**Note**: JWT and TLS are enabled by default. For local development without authentication, use `make run-no-auth` or disable JWT/TLS via flags or env vars (see [Runtime configuration](#runtime-configuration) below). The service starts on `localhost:8000` — see [Accessing the API](../README.md#accessing-the-api) for all available endpoints. @@ -233,7 +233,7 @@ make db/login # Connect to database shell | `make build` | Build hyperfleet-api executable to bin/ | | `make test` | Run unit tests | | `make test-integration` | Run integration tests | -| `make run-no-auth` | Start server without authentication | +| `make run-no-auth` | Start server without authentication or TLS | | `make run` | Start server with JWT authentication | | `make db/setup` | Create PostgreSQL container | | `make db/teardown` | Remove PostgreSQL container | @@ -462,67 +462,38 @@ make db/setup make test-integration ``` -## Development Environment Configuration +## Runtime configuration -### Development Environment Analysis - -**Background**: Prior to HYPERFLEET-1133, the API defaulted to `DevelopmentEnv` (insecure). To protect production deployments, HYPERFLEET-1133 changed the default to `ProductionEnv` (secure by default). - -**Analysis Question**: Is `e_development.go` still needed after this change? - -**Decision**: **KEEP `e_development.go`** with improved documentation. -**Why keep it**: - -- ✅ **One variable controls multiple settings** — `HYPERFLEET_ENV=development` forces JWT=false, TLS=false, SSL=disable -- ✅ **Convenient for scripts/CI** — One environment variable vs three separate flags -- ✅ **Semantic clarity** — "development mode" is clearer than remembering individual flags -- ✅ **Consistent with tests** — `unit_testing` and `integration_testing` use the same pattern -- ✅ **Production safe** — `EnvironmentDefault = ProductionEnv` prevents accidental use in production - -**How to use**: +Configure JWT, TLS, and DB SSL with flags, config file, or `HYPERFLEET_*` env vars. ```bash -# Full development mode (JWT/TLS/DB SSL all disabled) -HYPERFLEET_ENV=development ./bin/hyperfleet-api serve +# Local: disable auth, TLS, and DB SSL +HYPERFLEET_SERVER_JWT_ENABLED=false \ +HYPERFLEET_SERVER_TLS_ENABLED=false \ +HYPERFLEET_DATABASE_SSL_MODE=disable \ +./bin/hyperfleet-api serve -# JWT-only no-auth (TLS and DB SSL keep their defaults) +# Same as above for JWT/TLS (DB SSL follows make db defaults) make run-no-auth -# Production mode (JWT/TLS enabled, default) -./bin/hyperfleet-api serve # Uses EnvironmentDefault = ProductionEnv +# Defaults (JWT/TLS enabled) +./bin/hyperfleet-api serve ``` -**⚠️ IMPORTANT**: `HYPERFLEET_ENV=development` is for **local development ONLY**. Never use in production. The development environment forces insecure settings: - -- JWT authentication: **disabled** -- TLS encryption: **disabled** -- Database SSL: **disabled** - -**Production deployments**: Always use `EnvironmentDefault` (production) or explicitly enable security via Helm values: +Production example (Helm values): ```yaml config: server: jwt: - enabled: true # Production requires JWT + enabled: true tls: - enabled: true # Production requires TLS + enabled: true database: ssl: - mode: verify-full # Production requires SSL -``` - -**Alternative to `HYPERFLEET_ENV=development`**: If you prefer explicit flags over environment-based config, you can pass flags directly: - -```bash -./bin/hyperfleet-api serve \ - --server-jwt-enabled=false \ - --server-https-enabled=false \ - --db-ssl-mode=disable + mode: verify-full ``` -However, `HYPERFLEET_ENV=development` is recommended for local development as it's simpler and less error-prone. - --- ## Related Documentation diff --git a/docs/logging.md b/docs/logging.md index 2d7d35a8..d33daaed 100644 --- a/docs/logging.md +++ b/docs/logging.md @@ -417,16 +417,16 @@ Sensitive data is automatically masked when `MASKING_ENABLED=true`: **Default masked headers**: `Authorization`, `Cookie`, `X-API-Key`, `X-Auth-Token` **Default masked fields**: `password`, `token`, `secret`, `api_key`, `client_secret` -To add custom masking rules: +To add custom masking rules to a loaded `*config.ApplicationConfig` named `cfg`: ```go -env().Config.Logging.Masking.Headers = append( - env().Config.Logging.Masking.Headers, +cfg.Logging.Masking.Headers = append( + cfg.Logging.Masking.Headers, "X-Custom-Auth-Header", ) -env().Config.Logging.Masking.Fields = append( - env().Config.Logging.Masking.Fields, +cfg.Logging.Masking.Fields = append( + cfg.Logging.Masking.Fields, "credit_card", "ssn", ) diff --git a/docs/testcontainers.md b/docs/testcontainers.md index 40c89a14..9cd1985b 100755 --- a/docs/testcontainers.md +++ b/docs/testcontainers.md @@ -2,7 +2,7 @@ hyperfleet uses https://github.com/testcontainers/testcontainers-go/ for integration tests to spin up ephemeral containers for tests. -The containers used by the tests are initialized/destroyed in the `integration_testing` environment. +Integration tests explicitly construct a testcontainer-backed database session factory. The container is initialized when the integration test helper starts and destroyed during helper teardown. ## Compatibility with podman @@ -37,7 +37,7 @@ $ podman machine ssh Connecting to vm podman-machine-default. To close connection, use `~.` or `exit` Fedora CoreOS 40.20240808.2.0 -root@localhost:~# ls -al /var/run/podman/podman.sock +root@localhost:~# ls -al /var/run/podman/podman.sock srw-rw----. 1 root root 0 Dec 20 14:32 /var/run/podman/podman.sock exit @@ -52,6 +52,3 @@ export TESTCONTAINERS_RYUK_CONTAINER_PRIVILEGED=true $ sudo chmod a+xrw /var/run/podman $ sudo chmod a+xrw /var/run/podman/podman.sock ``` - - - diff --git a/pkg/auth/auth_middleware.go b/pkg/auth/auth_middleware.go index c4c192b9..56c99d1f 100755 --- a/pkg/auth/auth_middleware.go +++ b/pkg/auth/auth_middleware.go @@ -26,11 +26,6 @@ func NewCallerIdentityMiddleware() CallerIdentityMiddleware { // If an identity header is configured, it takes precedence over JWT claims. func (m *callerIdentityMiddleware) ResolveCallerIdentity(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if shouldSkipCallerIdentity(r.URL.Path) { - next.ServeHTTP(w, r) - return - } - ctx := r.Context() identity, err := CallerIdentityFromRequest(ctx, r) diff --git a/pkg/auth/identity.go b/pkg/auth/identity.go index e7aa7bbb..083ed993 100644 --- a/pkg/auth/identity.go +++ b/pkg/auth/identity.go @@ -84,11 +84,6 @@ func normalizeIdentity(raw string, source string) (string, error) { return value, nil } -func shouldSkipCallerIdentity(path string) bool { - return strings.HasPrefix(path, "/api/hyperfleet/v1/openapi") || - strings.HasPrefix(path, "/api/hyperfleet/v1/errors") -} - func isMutatingMethod(method string) bool { return method == http.MethodPost || method == http.MethodPatch || method == http.MethodDelete || method == http.MethodPut diff --git a/pkg/auth/jwt_handler.go b/pkg/auth/jwt_handler.go index a18a7667..39a21927 100644 --- a/pkg/auth/jwt_handler.go +++ b/pkg/auth/jwt_handler.go @@ -94,10 +94,11 @@ type JWTHandler struct { validators []issuerValidator } -func (h *JWTHandler) Close() { +func (h *JWTHandler) Close() error { if h.cancel != nil { h.cancel() } + return nil } // matchValidator tries each configured issuer validator against the request headers. diff --git a/pkg/closer/closer.go b/pkg/closer/closer.go new file mode 100644 index 00000000..c0d3ba47 --- /dev/null +++ b/pkg/closer/closer.go @@ -0,0 +1,60 @@ +package closer + +import ( + "context" + "errors" + "fmt" + "sync" + "time" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" +) + +type Closer struct { + result error + fns []func() error + once sync.Once + mu sync.Mutex + closed bool +} + +func New() *Closer { + return &Closer{} +} + +func (c *Closer) Add(fn func() error) { + c.mu.Lock() + defer c.mu.Unlock() + if c.closed { + panic("closer: Add called after Close started") + } + c.fns = append(c.fns, fn) +} + +func (c *Closer) Close() error { + c.mu.Lock() + if !c.closed { + c.closed = true + } + fns := c.fns + c.mu.Unlock() + + c.once.Do(func() { + ctx := context.Background() + var joined error + for i := len(fns) - 1; i >= 0; i-- { + start := time.Now() + err := fns[i]() + elapsed := time.Since(start) + if err != nil { + logger.With(ctx, "step", i, "duration", elapsed).WithError(err).Error("closer: step failed") + joined = errors.Join(joined, fmt.Errorf("step %d: %w", i, err)) + } else { + logger.With(ctx, "step", i, "duration", elapsed).Info("closer: step completed") + } + } + + c.result = joined + }) + return c.result +} diff --git a/pkg/closer/closer_test.go b/pkg/closer/closer_test.go new file mode 100644 index 00000000..00168585 --- /dev/null +++ b/pkg/closer/closer_test.go @@ -0,0 +1,194 @@ +package closer + +import ( + "errors" + "sync" + "testing" + + . "github.com/onsi/gomega" +) + +func TestReverseOrder(t *testing.T) { + RegisterTestingT(t) + + var order []int + c := New() + for i := range 5 { + c.Add(func() error { + order = append(order, i) + return nil + }) + } + + err := c.Close() + Expect(err).NotTo(HaveOccurred()) + Expect(order).To(Equal([]int{4, 3, 2, 1, 0})) +} + +func TestIdempotent(t *testing.T) { + RegisterTestingT(t) + + calls := 0 + sentinel := errors.New("boom") + c := New() + c.Add(func() error { + calls++ + return sentinel + }) + + err1 := c.Close() + err2 := c.Close() + + Expect(calls).To(Equal(1)) + Expect(err1).To(MatchError(sentinel)) + Expect(err2).To(Equal(err1)) +} + +func TestIdempotentNilError(t *testing.T) { + RegisterTestingT(t) + + calls := 0 + c := New() + c.Add(func() error { + calls++ + return nil + }) + + err1 := c.Close() + err2 := c.Close() + + Expect(calls).To(Equal(1)) + Expect(err1).NotTo(HaveOccurred()) + Expect(err2).NotTo(HaveOccurred()) +} + +func TestOneFailureDoesNotAbortUnwind(t *testing.T) { + RegisterTestingT(t) + + var order []int + c := New() + c.Add(func() error { order = append(order, 0); return nil }) + c.Add(func() error { order = append(order, 1); return errors.New("fail-1") }) + c.Add(func() error { order = append(order, 2); return nil }) + + err := c.Close() + + Expect(order).To(Equal([]int{2, 1, 0})) + Expect(err).To(HaveOccurred()) +} + +func TestEveryErrorRetrievableViaErrorsIs(t *testing.T) { + RegisterTestingT(t) + + errA := errors.New("a") + errB := errors.New("b") + errC := errors.New("c") + + c := New() + c.Add(func() error { return errA }) + c.Add(func() error { return errB }) + c.Add(func() error { return nil }) + c.Add(func() error { return errC }) + + err := c.Close() + + Expect(errors.Is(err, errA)).To(BeTrue()) + Expect(errors.Is(err, errB)).To(BeTrue()) + Expect(errors.Is(err, errC)).To(BeTrue()) +} + +func TestAllNilErrorsProduceNilResult(t *testing.T) { + RegisterTestingT(t) + + c := New() + c.Add(func() error { return nil }) + c.Add(func() error { return nil }) + + err := c.Close() + Expect(err).NotTo(HaveOccurred()) +} + +func TestEmptyCloserReturnsNil(t *testing.T) { + RegisterTestingT(t) + + c := New() + err := c.Close() + Expect(err).NotTo(HaveOccurred()) +} + +func TestConcurrentAdd(t *testing.T) { + RegisterTestingT(t) + + c := New() + var wg sync.WaitGroup + n := 100 + wg.Add(n) + for range n { + go func() { + defer wg.Done() + c.Add(func() error { return nil }) + }() + } + wg.Wait() + + Expect(c.Close()).NotTo(HaveOccurred()) + Expect(c.fns).To(HaveLen(n)) +} + +func TestTwoInstancesAreIndependent(t *testing.T) { + RegisterTestingT(t) + + var orderA, orderB []int + a := New() + b := New() + + a.Add(func() error { orderA = append(orderA, 1); return nil }) + a.Add(func() error { orderA = append(orderA, 2); return nil }) + b.Add(func() error { orderB = append(orderB, 3); return nil }) + + errA := a.Close() + Expect(errA).NotTo(HaveOccurred()) + Expect(orderA).To(Equal([]int{2, 1})) + Expect(orderB).To(BeEmpty()) + + errB := b.Close() + Expect(errB).NotTo(HaveOccurred()) + Expect(orderB).To(Equal([]int{3})) +} + +func TestAddDuringClosePanics(t *testing.T) { + RegisterTestingT(t) + + c := New() + c.Add(func() error { + Expect(func() { + c.Add(func() error { return nil }) + }).To(PanicWith("closer: Add called after Close started")) + return nil + }) + + Expect(c.Close()).NotTo(HaveOccurred()) +} + +func TestConcurrentClose(t *testing.T) { + RegisterTestingT(t) + + sentinel := errors.New("boom") + c := New() + c.Add(func() error { return sentinel }) + + var wg sync.WaitGroup + errs := make([]error, 10) + wg.Add(len(errs)) + for i := range errs { + go func(idx int) { + defer wg.Done() + errs[idx] = c.Close() + }(i) + } + wg.Wait() + + for i, err := range errs { + Expect(err).To(MatchError(sentinel), "goroutine %d", i) + } +} diff --git a/pkg/config/config.go b/pkg/config/config.go index e14d6f0a..8259eeb2 100755 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -10,6 +10,7 @@ type ApplicationConfig struct { Health *HealthConfig `mapstructure:"health" json:"health" validate:"required"` Database *DatabaseConfig `mapstructure:"database" json:"database" validate:"required"` Logging *LoggingConfig `mapstructure:"logging" json:"logging" validate:"required"` + Tracing *TracingConfig `mapstructure:"tracing" json:"tracing" validate:"required"` Entities []registry.EntityDescriptor `mapstructure:"entities" json:"entities"` } @@ -22,5 +23,6 @@ func NewApplicationConfig() *ApplicationConfig { Health: NewHealthConfig(), Database: NewDatabaseConfig(), Logging: NewLoggingConfig(), + Tracing: NewTracingConfig(), } } diff --git a/pkg/config/dump.go b/pkg/config/dump.go index d6b1d126..433b8c72 100644 --- a/pkg/config/dump.go +++ b/pkg/config/dump.go @@ -30,7 +30,9 @@ func DumpConfig(config *ApplicationConfig) string { Logging: Level: %s Format: %s - OTel.Enabled: %t + Tracing: + Enabled: %t + ServiceName: %s Metrics: BindAddress: %s Health: @@ -50,7 +52,8 @@ func DumpConfig(config *ApplicationConfig) string { config.Database.Debug, config.Logging.Level, config.Logging.Format, - config.Logging.OTel.Enabled, + config.Tracing.Enabled, + config.Tracing.ServiceName, config.Metrics.BindAddress(), config.Health.BindAddress(), len(config.Entities), diff --git a/pkg/config/health.go b/pkg/config/health.go index 4fd104c8..81f74c18 100644 --- a/pkg/config/health.go +++ b/pkg/config/health.go @@ -51,3 +51,19 @@ func NewHealthConfig() *HealthConfig { func (h *HealthConfig) BindAddress() string { return net.JoinHostPort(h.Host, strconv.Itoa(h.Port)) } + +func (h *HealthConfig) TLSEnabled() bool { + return h.TLS.Enabled +} + +func (h *HealthConfig) TLSCertFile() string { + return h.TLS.CertFile +} + +func (h *HealthConfig) TLSKeyFile() string { + return h.TLS.KeyFile +} + +func (h *HealthConfig) PingTimeout() time.Duration { + return h.DBPingTimeout +} diff --git a/pkg/config/loader.go b/pkg/config/loader.go index d7ef7d5d..6916a674 100644 --- a/pkg/config/loader.go +++ b/pkg/config/loader.go @@ -73,6 +73,9 @@ func (l *ConfigLoader) Load(ctx context.Context, cmd *cobra.Command) (*Applicati err) } + // Step 6.5: Migrate deprecated configuration before validation + l.migrateDeprecatedConfig(ctx, config) + // Step 7: Validate configuration if err := l.validateConfig(config); err != nil { return nil, err @@ -187,6 +190,9 @@ func (l *ConfigLoader) validateConfig(config *ApplicationConfig) error { if valErr := config.Metrics.Validate(); valErr != nil { return fmt.Errorf("metrics config validation failed: %w", valErr) } + if valErr := config.Tracing.Validate(); valErr != nil { + return fmt.Errorf("tracing config validation failed: %w", valErr) + } return nil } @@ -232,9 +238,33 @@ func (l *ConfigLoader) validateConfig(config *ApplicationConfig) error { return fmt.Errorf("%s", strings.Join(errMessages, "\n")) } +// migrateDeprecatedConfig applies backward-compat shims for renamed config. +// Keep until the Helm chart templates cert_file/key_file for health/metrics. +func (l *ConfigLoader) migrateDeprecatedConfig(ctx context.Context, config *ApplicationConfig) { + propagateTLS := func(name string, tls *TLSConfig) { + if tls.Enabled && tls.CertFile == "" && tls.KeyFile == "" { + tls.CertFile = config.Server.TLS.CertFile + tls.KeyFile = config.Server.TLS.KeyFile + logger.With(ctx, "server", name). + Warn("TLS enabled without cert/key - inheriting from server.tls (deprecated: set cert_file/key_file explicitly)") + } + } + propagateTLS("health", &config.Health.TLS) + propagateTLS("metrics", &config.Metrics.TLS) + + if l.viper.IsSet("logging.otel.enabled") { + if !l.viper.IsSet("tracing.enabled") { + config.Tracing.Enabled = config.Logging.OTel.Enabled + } + logger.Warn(ctx, "logging.otel.enabled is deprecated, use tracing.enabled instead") + } +} + // bindEnv wraps viper.BindEnv and tracks the key for validation func (l *ConfigLoader) bindEnv(key string) { - l.viper.BindEnv(key) //nolint:errcheck,gosec // BindEnv errors are rare and indicate programming errors + if err := l.viper.BindEnv(key); err != nil { + panic(fmt.Sprintf("bind env %q: %v", key, err)) + } l.explicitlyBoundKeys[key] = true } @@ -243,7 +273,9 @@ func (l *ConfigLoader) bindPFlag(key string, flag *pflag.Flag) { if flag == nil { return } - l.viper.BindPFlag(key, flag) //nolint:errcheck,gosec // BindPFlag errors are rare and indicate programming errors + if err := l.viper.BindPFlag(key, flag); err != nil { + panic(fmt.Sprintf("bind pflag %q: %v", key, err)) + } l.explicitlyBoundKeys[key] = true // Record the mapping from Viper key to flag name for validation error messages l.viperKeyToFlag[key] = flag.Name @@ -290,11 +322,14 @@ func (l *ConfigLoader) bindAllEnvVars() { l.bindEnv("logging.masking.enabled") l.bindEnv("logging.masking.headers") l.bindEnv("logging.masking.fields") + l.bindEnv("logging.otel.enabled") // deprecated: mapped to tracing.enabled // Metrics config l.bindEnv("metrics.host") l.bindEnv("metrics.port") l.bindEnv("metrics.tls.enabled") + l.bindEnv("metrics.tls.cert_file") + l.bindEnv("metrics.tls.key_file") l.bindEnv("metrics.label_metrics_inclusion_duration") l.bindEnv("metrics.reconciliation_stuck_threshold") @@ -302,9 +337,19 @@ func (l *ConfigLoader) bindAllEnvVars() { l.bindEnv("health.host") l.bindEnv("health.port") l.bindEnv("health.tls.enabled") + l.bindEnv("health.tls.cert_file") + l.bindEnv("health.tls.key_file") l.bindEnv("health.shutdown_timeout") l.bindEnv("health.db_ping_timeout") + // Tracing config + l.bindEnv("tracing.enabled") + l.bindEnv("tracing.service_name") + // OTEL_SERVICE_NAME is a standard OTel env var without the HYPERFLEET_ prefix. + if err := l.viper.BindEnv("tracing.service_name", "OTEL_SERVICE_NAME"); err != nil { + panic(fmt.Sprintf("bind env %q: %v", "tracing.service_name", err)) + } + // Entities: config-file-only (complex list-of-struct type). // No env var or CLI flag bindings — loaded exclusively via YAML config. } diff --git a/pkg/config/logging.go b/pkg/config/logging.go index dad1b188..dc14d852 100644 --- a/pkg/config/logging.go +++ b/pkg/config/logging.go @@ -11,13 +11,14 @@ type LoggingConfig struct { Format string `mapstructure:"format" json:"format" validate:"required,oneof=json text"` Output string `mapstructure:"output" json:"output" validate:"required,oneof=stdout stderr"` Masking MaskingConfig `mapstructure:"masking" json:"masking" validate:"required"` - OTel OTelConfig `mapstructure:"otel" json:"otel" validate:"required"` + // Deprecated: use TracingConfig.Enabled. Kept so UnmarshalExact accepts + // existing config files that still carry logging.otel.enabled. + OTel DeprecatedOTelConfig `mapstructure:"otel" json:"otel,omitempty"` } -// OTelConfig holds OpenTelemetry configuration -// Configuration is driven entirely by standard environment variables. -// See: https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/standards/tracing.md#configuration -type OTelConfig struct { +// DeprecatedOTelConfig exists solely to let viper unmarshal the old +// logging.otel key without rejecting it as unknown. +type DeprecatedOTelConfig struct { Enabled bool `mapstructure:"enabled" json:"enabled"` } @@ -35,9 +36,6 @@ func NewLoggingConfig() *LoggingConfig { Level: "info", Format: "json", Output: "stdout", - OTel: OTelConfig{ - Enabled: true, - }, Masking: MaskingConfig{ Enabled: true, Headers: []string{ diff --git a/pkg/config/logging_test.go b/pkg/config/logging_test.go index 65c6f27a..e9c1e9ee 100644 --- a/pkg/config/logging_test.go +++ b/pkg/config/logging_test.go @@ -18,7 +18,6 @@ func TestNewLoggingConfig_Defaults(t *testing.T) { Expect(cfg.Level).To(Equal("info")) Expect(cfg.Format).To(Equal("json")) Expect(cfg.Output).To(Equal("stdout")) - Expect(cfg.OTel.Enabled).To(BeTrue()) Expect(cfg.Masking.Enabled).To(BeTrue()) Expect(cfg.Masking.Headers).NotTo(BeEmpty()) Expect(cfg.Masking.Fields).NotTo(BeEmpty()) @@ -42,8 +41,69 @@ func TestConfigLoader_LoggingFromEnv(t *testing.T) { Expect(err).NotTo(HaveOccurred()) Expect(appConfig.Logging.Level).To(Equal("debug")) Expect(appConfig.Logging.Format).To(Equal("text")) - // OTel.Enabled defaults to true - Expect(appConfig.Logging.OTel.Enabled).To(BeTrue()) + Expect(appConfig.Tracing.Enabled).To(BeTrue()) + Expect(appConfig.Tracing.ServiceName).To(Equal("hyperfleet-api")) +} + +func TestConfigLoader_TracingFromEnv(t *testing.T) { + RegisterTestingT(t) + + tests := []struct { + envVars map[string]string + name string + expectedServiceName string + expectedEnabled bool + }{ + { + name: "defaults", + expectedEnabled: true, + expectedServiceName: "hyperfleet-api", + }, + { + name: "tracing disabled via env", + envVars: map[string]string{"HYPERFLEET_TRACING_ENABLED": "false"}, + expectedEnabled: false, + expectedServiceName: "hyperfleet-api", + }, + { + name: "service name via HYPERFLEET prefix", + envVars: map[string]string{"HYPERFLEET_TRACING_SERVICE_NAME": "custom-api"}, + expectedEnabled: true, + expectedServiceName: "custom-api", + }, + { + name: "OTEL_SERVICE_NAME overrides default", + envVars: map[string]string{"OTEL_SERVICE_NAME": "otel-api"}, + expectedEnabled: true, + expectedServiceName: "otel-api", + }, + { + name: "HYPERFLEET prefix wins over OTEL_SERVICE_NAME", + envVars: map[string]string{ + "HYPERFLEET_TRACING_SERVICE_NAME": "hyperfleet-name", + "OTEL_SERVICE_NAME": "otel-name", + }, + expectedEnabled: true, + expectedServiceName: "hyperfleet-name", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + RegisterTestingT(t) + SetMinimalTestEnv(t) + for k, v := range tt.envVars { + t.Setenv(k, v) + } + + loader := NewConfigLoader() + appConfig, err := loader.Load(context.Background(), &cobra.Command{}) + + Expect(err).NotTo(HaveOccurred()) + Expect(appConfig.Tracing.Enabled).To(Equal(tt.expectedEnabled)) + Expect(appConfig.Tracing.ServiceName).To(Equal(tt.expectedServiceName)) + }) + } } // TestLoggingConfig_GetSensitiveHeadersList tests the headers array accessor diff --git a/pkg/config/metrics.go b/pkg/config/metrics.go index 0b6ec28a..25719469 100755 --- a/pkg/config/metrics.go +++ b/pkg/config/metrics.go @@ -49,7 +49,14 @@ func (m *MetricsConfig) BindAddress() string { return net.JoinHostPort(m.Host, strconv.Itoa(m.Port)) } -// GetLabelMetricsInclusionDuration returns label metrics inclusion duration -func (m *MetricsConfig) GetLabelMetricsInclusionDuration() time.Duration { - return m.LabelMetricsInclusionDuration +func (m *MetricsConfig) TLSEnabled() bool { + return m.TLS.Enabled +} + +func (m *MetricsConfig) TLSCertFile() string { + return m.TLS.CertFile +} + +func (m *MetricsConfig) TLSKeyFile() string { + return m.TLS.KeyFile } diff --git a/pkg/config/tracing.go b/pkg/config/tracing.go new file mode 100644 index 00000000..8a20a9c7 --- /dev/null +++ b/pkg/config/tracing.go @@ -0,0 +1,22 @@ +package config + +import "fmt" + +type TracingConfig struct { + ServiceName string `mapstructure:"service_name" json:"service_name"` + Enabled bool `mapstructure:"enabled" json:"enabled"` +} + +func (c *TracingConfig) Validate() error { + if c.Enabled && c.ServiceName == "" { + return fmt.Errorf("tracing service_name is required when tracing is enabled") + } + return nil +} + +func NewTracingConfig() *TracingConfig { + return &TracingConfig{ + Enabled: true, + ServiceName: "hyperfleet-api", + } +} diff --git a/pkg/db/db_session/testcontainer.go b/pkg/db/db_session/testcontainer.go deleted file mode 100755 index 32d346ee..00000000 --- a/pkg/db/db_session/testcontainer.go +++ /dev/null @@ -1,241 +0,0 @@ -package db_session - -import ( - "context" - "database/sql" - "fmt" - "net/url" - "os" - "time" - - "github.com/testcontainers/testcontainers-go" - "github.com/testcontainers/testcontainers-go/modules/postgres" - "github.com/testcontainers/testcontainers-go/wait" - gormpostgres "gorm.io/driver/postgres" - "gorm.io/gorm" - gormlogger "gorm.io/gorm/logger" - - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db/db_context" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db/db_metrics" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" -) - -type Testcontainer struct { - config *config.DatabaseConfig - container *postgres.PostgresContainer - g2 *gorm.DB - sqlDB *sql.DB -} - -var _ db.SessionFactory = &Testcontainer{} - -// redactPassword redacts the password from a connection string for safe logging -func redactPassword(connStr string) string { - parsedURL, err := url.Parse(connStr) - if err != nil { - // If parsing fails, return a generic message to avoid leaking anything - return "" - } - if parsedURL.User != nil { - username := parsedURL.User.Username() - _, hasPassword := parsedURL.User.Password() - if hasPassword { - // Replace password with redacted value - parsedURL.User = url.UserPassword(username, "") - } - } - return parsedURL.String() -} - -// NewTestcontainerFactory creates a SessionFactory using testcontainers. -// This starts a real PostgreSQL container for integration testing. -func NewTestcontainerFactory(config *config.DatabaseConfig) *Testcontainer { - conn := &Testcontainer{ - config: config, - } - conn.Init(config) - return conn -} - -func (f *Testcontainer) Init(config *config.DatabaseConfig) { - ctx := context.Background() - - logger.Info(ctx, "Starting PostgreSQL testcontainer...") - - // Create PostgreSQL container - container, err := postgres.Run(ctx, - "postgres:14.23", - postgres.WithDatabase(config.Name), - postgres.WithUsername(config.Username), - postgres.WithPassword(config.Password), - testcontainers.WithWaitStrategy( - wait.ForListeningPort("5432/tcp"). - WithStartupTimeout(60*time.Second)), - ) - if err != nil { - logger.WithError(ctx, err).Error("Failed to start PostgreSQL testcontainer") - os.Exit(1) - } - - f.container = container - - // Get connection string from container - connStr, err := container.ConnectionString(ctx, "sslmode=disable") - if err != nil { - logger.WithError(ctx, err).Error("Failed to get connection string from testcontainer") - os.Exit(1) - } - - logger.With(ctx, logger.FieldConnectionString, redactPassword(connStr)).Info("PostgreSQL testcontainer started") - - // Open SQL connection - f.sqlDB, err = sql.Open("postgres", connStr) - if err != nil { - logger.WithError(ctx, err).Error("Failed to connect to testcontainer database") - os.Exit(1) - } - - // Configure connection pool - f.sqlDB.SetMaxOpenConns(config.Pool.MaxConnections) - - var gormLog gormlogger.Interface - if config.Debug { - gormLog = logger.NewGormLogger(gormlogger.Info, slowQueryThreshold) - } else { - gormLog = logger.NewGormLogger(gormlogger.Silent, slowQueryThreshold) - } - - conf := &gorm.Config{ - PrepareStmt: false, - FullSaveAssociations: false, - SkipDefaultTransaction: true, - Logger: gormLog, - } - - f.g2, err = gorm.Open(gormpostgres.New(gormpostgres.Config{ - Conn: f.sqlDB, - PreferSimpleProtocol: true, - }), conf) - if err != nil { - logger.WithError(ctx, err).Error("Failed to connect GORM to testcontainer database") - os.Exit(1) - } - - // Register database metrics GORM plugin - if err := db_metrics.RegisterPlugin(f.g2); err != nil { - logger.WithError(ctx, err).Warn("Failed to register database metrics plugin on testcontainer") - } - - // Register connection pool metrics collector - if err := db_metrics.RegisterPoolCollector(f.sqlDB); err != nil { - logger.WithError(ctx, err).Warn("Failed to register pool metrics collector on testcontainer") - } - - // Run migrations - logger.Info(ctx, "Running database migrations on testcontainer...") - if err := db.Migrate(f.g2); err != nil { - logger.WithError(ctx, err).Error("Failed to run migrations on testcontainer") - os.Exit(1) - } - - logger.Info(ctx, "Testcontainer database initialized successfully") -} - -func (f *Testcontainer) DirectDB() *sql.DB { - return f.sqlDB -} - -func (f *Testcontainer) New(ctx context.Context) *gorm.DB { - if tx, ok := db_context.Transaction(ctx); ok { - return tx.DB - } - - return f.g2.Session(&gorm.Session{ - Context: ctx, - }) -} - -func (f *Testcontainer) CheckConnection() error { - _, err := f.sqlDB.Exec("SELECT 1") - return err -} - -func (f *Testcontainer) Close() error { - // Use a timeout to prevent hanging indefinitely during teardown. - // Without this, a hung container.Terminate() would block the process from - // exiting, causing Prow CI jobs to stay in "pending" state (HYPERFLEET-625). - ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - defer cancel() - - // Close SQL connection - if f.sqlDB != nil { - if err := f.sqlDB.Close(); err != nil { - logger.WithError(ctx, err).Error("Error closing SQL connection") - } - } - - // Terminate container - if f.container != nil { - logger.Info(ctx, "Stopping PostgreSQL testcontainer...") - if err := f.container.Terminate(ctx); err != nil { - return fmt.Errorf("failed to terminate testcontainer: %s", err) - } - logger.Info(ctx, "PostgreSQL testcontainer stopped") - } - - return nil -} - -func (f *Testcontainer) ResetDB() { - // For testcontainers, we can just truncate all tables - ctx := context.Background() - g2 := f.New(ctx) - - // Truncate all business tables in the correct order (respecting FK constraints) - // Using CASCADE to handle foreign key constraints automatically - tables := []string{ - "adapter_statuses", // Polymorphic table, no FK constraints - "resource_conditions", // Has FK to resources - "resource_labels", // Has FK to resources - "resource_references", // Has FK to resources - "resources", // Main entity table - "events", // Independent table - } - for _, table := range tables { - if g2.Migrator().HasTable(table) { - if err := g2.Exec(fmt.Sprintf("TRUNCATE TABLE %s CASCADE", table)).Error; err != nil { - logger.With(ctx, logger.FieldTable, table).WithError(err).Error("Error truncating table") - } - } - } -} - -func (f *Testcontainer) NewListener(ctx context.Context, channel string, callback func(id string)) { - // Get the connection string for the listener - connStr, err := f.container.ConnectionString(ctx, "sslmode=disable") - if err != nil { - logger.WithError(ctx, err).Error("Failed to get connection string for listener") - return - } - - newListener(ctx, connStr, channel, callback) -} - -// ReconfigureLogger changes the GORM logger level at runtime -func (f *Testcontainer) ReconfigureLogger(level gormlogger.LogLevel) { - if f.g2 == nil { - return - } - newLogger := logger.NewGormLogger(level, slowQueryThreshold) - f.g2.Logger = newLogger -} - -func (f *Testcontainer) GetAdvisoryLockTimeout() int { - timeout := int(f.config.Pool.AdvisoryLockTimeout.Seconds()) - if timeout == 0 { - return 300 // Default: 5 minutes if not configured - } - return timeout -} diff --git a/test/CLAUDE.md b/test/CLAUDE.md index 62571dab..a15a3f56 100644 --- a/test/CLAUDE.md +++ b/test/CLAUDE.md @@ -2,14 +2,14 @@ ## Unit Tests -- Run: `make test` (sets `HYPERFLEET_ENV=unit_testing` automatically) +- Run: `make test` - Use Gomega assertions: `. "github.com/onsi/gomega"` with `RegisterTestingT(t)` - Mock generation: `make generate-mocks` — never write mocks manually - Mocks use `go.uber.org/mock/gomock` ## Integration Tests -- Run: `make test-integration` (sets `HYPERFLEET_ENV=integration_testing` and `TESTCONTAINERS_RYUK_DISABLED=true`) +- Run: `make test-integration` (sets `TESTCONTAINERS_RYUK_DISABLED=true`) - Located in `integration/` - Testcontainers auto-creates isolated PostgreSQL per test suite — no external DB needed - Setup: `test.RegisterIntegration(t)` returns `(helper, httpClient)` @@ -34,6 +34,5 @@ Reference: `factories/` ## Key Environment Variables -- `HYPERFLEET_ENV` — selects config environment: `unit_testing`, `integration_testing`, `development` - `TESTCONTAINERS_RYUK_DISABLED=true` — required for testcontainers in CI - `HYPERFLEET_SERVER_OPENAPI_SCHEMA_PATH` — path to OpenAPI schema for spec validation (auto-set by `TestMain` to `test/validation-schema.yaml`) diff --git a/test/helper.go b/test/helper.go index 13951ae5..7680d4f8 100755 --- a/test/helper.go +++ b/test/helper.go @@ -4,29 +4,29 @@ import ( "context" "crypto/rsa" "encoding/base64" - "encoding/json" "fmt" "log/slog" "net/http" "os" + "path/filepath" + "runtime" + "slices" "strings" "sync" - "testing" "time" "github.com/brianvoe/gofakeit/v7" "github.com/golang-jwt/jwt/v5" "github.com/google/uuid" "github.com/spf13/cobra" - "github.com/spf13/pflag" "gorm.io/gorm" "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/container" - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments" "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/servecmd" "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/server" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/auth" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/closer" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" @@ -36,11 +36,8 @@ import ( ) const ( - apiPort = ":8777" - jwtKeyFile = "test/support/jwt_private_key.pem" - jwtCAFile = "test/support/jwt_ca.pem" - jwkKID = "uhctestkey" - jwkAlg = "RS256" + jwkKID = "uhctestkey" + jwkAlg = "RS256" ) var ( @@ -50,316 +47,200 @@ var ( const defaultTestIdentityHeader = "X-HyperFleet-Identity" -// jwkURL stores the JWK mock server URL for testing -var jwkURL string +func integrationTestConfigPath() string { + _, thisFile, _, ok := runtime.Caller(0) + if !ok { + panic("test setup: runtime.Caller failed") + } + return filepath.Join(filepath.Dir(thisFile), "testdata", "integration-config.yaml") +} -// TimeFunc defines a way to get a new Time instance common to the entire test suite. -// Aria's environment has Virtual Time that may not be actual time. We compensate -// by synchronizing on a common time func attached to the test harness. -type TimeFunc func() time.Time +func defaultTestEntities() []registry.EntityDescriptor { + return []registry.EntityDescriptor{ + { + Kind: "Cluster", + Plural: "clusters", + SpecSchemaName: "ClusterSpec", + NameMinLen: 3, + NameMaxLen: 53, + RequireSpecSchema: true, + RequiredAdapters: []string{"validation", "dns", "pullsecret", "hypershift"}, + }, + { + Kind: "NodePool", + Plural: "nodepools", + ParentKind: "Cluster", + OnParentDelete: registry.OnParentDeleteCascade, + SpecSchemaName: "NodePoolSpec", + NameMinLen: 3, + NameMaxLen: 15, + RequireSpecSchema: true, + RequiredAdapters: []string{"validation", "hypershift"}, + }, + { + Kind: "Channel", + Plural: "channels", + SpecSchemaName: "ChannelSpec", + }, + { + Kind: "Version", + Plural: "versions", + ParentKind: "Channel", + OnParentDelete: registry.OnParentDeleteRestrict, + SpecSchemaName: "VersionSpec", + }, + { + Kind: "WifConfig", + Plural: "wifconfigs", + SpecSchemaName: "WifConfigSpec", + }, + } +} type Helper struct { - MetricsServer server.Server - Ctx context.Context - DBFactory db.SessionFactory Factories factories.Factories - HealthServer server.Server + DBFactory db.SessionFactory Container *container.Container APIServer *server.APIServer AppConfig *config.ApplicationConfig - TimeFunc TimeFunc JWTPrivateKey *rsa.PrivateKey JWTCA *rsa.PublicKey - T *testing.T - jwkTeardown func() error - teardowns []func() error + jwtHandler *auth.JWTHandler + closer *closer.Closer + tables []string } -func NewHelper(t *testing.T) *Helper { +func NewHelper() *Helper { once.Do(func() { - // Initialize logger first initTestLogger() ctx := context.Background() jwtKey, jwtCA, err := parseJWTKeys() if err != nil { - fmt.Println("Unable to read JWT keys - this may affect tests that make authenticated server requests") + panic(fmt.Sprintf("test setup: unable to load JWT keys: %v", err)) } - // Load configuration using ConfigLoader (same path as production). - // Integration tests bootstrap JWT issuers in OverrideConfig after Load. - // Config validation now requires issuers when JWT is enabled, so disable - // JWT for the Load step; OverrideConfig re-enables and configures issuers. - var restoreJWTEnv func() error - if environments.GetEnvironmentStrFromEnv() == environments.IntegrationTestingEnv { - prevJWTEnabled, hadJWTEnabled := os.LookupEnv("HYPERFLEET_SERVER_JWT_ENABLED") - if setenvErr := os.Setenv("HYPERFLEET_SERVER_JWT_ENABLED", "false"); setenvErr != nil { - logger.WithError(ctx, setenvErr).Error("Failed to disable JWT for integration config load") - os.Exit(1) - } - restoreJWTEnv = func() error { - if hadJWTEnabled { - return os.Setenv("HYPERFLEET_SERVER_JWT_ENABLED", prevJWTEnabled) - } - return os.Unsetenv("HYPERFLEET_SERVER_JWT_ENABLED") - } - } - emptyCmd := &cobra.Command{} + // Use an explicit --config flag, not HYPERFLEET_CONFIG, so we never hijack a developer's own env var. + cmd := &cobra.Command{} + cmd.Flags().String("config", "", "config file path") + cmd.Flags().Set("config", integrationTestConfigPath()) //nolint:errcheck // string flag, Set never errors + loader := config.NewConfigLoader() - cfg, err := loader.Load(ctx, emptyCmd) - if restoreJWTEnv != nil { - if restoreErr := restoreJWTEnv(); restoreErr != nil { - logger.WithError(ctx, restoreErr).Error("Failed to restore JWT env override after config load") - os.Exit(1) - } - } + cfg, err := loader.Load(ctx, cmd) if err != nil { - logger.WithError(ctx, err).Error("Failed to load test configuration") - os.Exit(1) + panic(fmt.Sprintf("test setup: load config: %v", err)) } - env := environments.Environment() - env.Config = cfg + if logLevel := os.Getenv("LOGLEVEL"); logLevel != "" { + logger.With(ctx, logger.FieldLogLevel, logLevel).Info("Using custom loglevel") + cfg.Logging.Level = logLevel + } - registry.LoadDescriptors(cfg.Entities) if len(cfg.Entities) == 0 { - loadDefaultTestEntities() + cfg.Entities = defaultTestEntities() } + registry.LoadDescriptors(cfg.Entities) registry.Validate() - err = env.SetEnvironmentDefaults(pflag.CommandLine) - if err != nil { - logger.WithError(ctx, err).Error("Unable to set environment defaults") - os.Exit(1) - } - if logLevel := os.Getenv("LOGLEVEL"); logLevel != "" { - logger.With(ctx, logger.FieldLogLevel, logLevel).Info("Using custom loglevel") - pflag.CommandLine.Set("-v", logLevel) //nolint:errcheck // best-effort log level override for tests + c := closer.New() + ctr := container.NewContainer(cfg, c) + + if err = db.Migrate(ctr.SessionFactory().New(ctx)); err != nil { + abortSetup(ctx, c, err, "migration failed") } - pflag.Parse() - err = env.Initialize() - if err != nil { - logger.WithError(ctx, err).Error("Unable to initialize testing environment") - // env.Initialize() starts the PostgreSQL testcontainer before it can - // fail later in the sequence (e.g. Seed()); tear it down here too. - env.Teardown() - os.Exit(1) + jwkURL, jwkTeardown := mocks.NewJWKCertServerMock(jwtCA, jwkKID, jwkAlg) + c.Add(jwkTeardown) + if len(cfg.Server.JWT.Configs) == 0 { + abortSetup(ctx, c, nil, "integration-config.yaml must define at least one JWT issuer") } + cfg.Server.JWT.Configs[0].JWKCertURL = jwkURL helper = &Helper{ - AppConfig: environments.Environment().Config, - DBFactory: environments.Environment().Database.SessionFactory, + Factories: factories.New(ctr.ResourceService()), + AppConfig: cfg, + DBFactory: ctr.SessionFactory(), + Container: ctr, JWTPrivateKey: jwtKey, JWTCA: jwtCA, - T: t, - } - // Teardown order: terminate the testcontainer FIRST so the - // container is removed before anything else. If server shutdown hangs - // and the force-exit goroutine kills the process, the container - // would remain alive and keep the Prow pod stuck (HYPERFLEET-625). - // CleanDB is omitted because the container is destroyed anyway. - // Every step is nil-safe so this same list can also run from - // failStartup before every resource in it has been created. - helper.teardowns = []func() error{ - helper.teardownEnv, - helper.stopAPIServer, - helper.closeContainer, - helper.stopMetricsServer, - helper.stopHealthServer, - helper.stopJWKMock, + closer: c, } - // Integration tests must run with JWT enabled; fail fast if that ever regresses. - if !cfg.Server.JWT.Enabled { - helper.failStartup(ctx, nil, - "Integration tests require JWT enabled, check OverrideConfig() in e_integration_testing.go") + tables, err := helper.getAllTables(ctr.SessionFactory().New(ctx)) + if err != nil { + abortSetup(ctx, c, err, "discover tables for truncation") } + helper.tables = tables - ctr := container.NewContainer(env.Config, env.Database.SessionFactory) - helper.Container = ctr - helper.Factories = factories.New(ctr.ResourceService()) - - // Start JWK certificate mock server for testing - helper.jwkTeardown = helper.StartJWKCertServerMock() helper.startAPIServer() - helper.startMetricsServer() - helper.startHealthServer() }) - helper.T = t return helper } -func (helper *Helper) Env() *environments.Env { - return environments.Environment() -} - -func (helper *Helper) teardownEnv() error { - helper.Env().Teardown() - return nil -} - func (helper *Helper) Teardown() { - for _, f := range helper.teardowns { - err := f() - if err != nil { - helper.T.Errorf("error running teardown func: %s", err) - } + if err := helper.closer.Close(); err != nil { + logger.WithError(context.Background(), err).Error("teardown errors") } } func (helper *Helper) requireJWTIssuers() { - if helper.Env().Config.Server.JWT.Enabled && len(helper.Env().Config.Server.JWT.Configs) == 0 { - helper.failStartup(context.Background(), nil, "JWT enabled but no issuer configs defined") + if helper.AppConfig.Server.JWT.Enabled && len(helper.AppConfig.Server.JWT.Configs) == 0 { + abortSetup(context.Background(), helper.closer, nil, "JWT enabled but no issuer configs defined") } } -// failStartup logs a fatal startup error, runs whatever teardowns have been -// registered so far (each is nil-safe for resources not yet created), and -// exits. Startup failures must go through this instead of a bare os.Exit so -// resources created earlier in NewHelper (e.g. the PostgreSQL testcontainer) -// don't leak and block the Prow pod (HYPERFLEET-625). -func (helper *Helper) failStartup(ctx context.Context, err error, msg string) { +// abortSetup logs msg, cleans up already-created resources via c, then panics - for unrecoverable NewHelper failures. +func abortSetup(ctx context.Context, c *closer.Closer, err error, msg string) { if err != nil { logger.WithError(ctx, err).Error(msg) } else { logger.Error(ctx, msg) } - for _, teardown := range helper.teardowns { - if tdErr := teardown(); tdErr != nil { - logger.WithError(ctx, tdErr).Error("error running teardown during startup failure") - } - } - os.Exit(1) + _ = c.Close() + panic(fmt.Sprintf("test setup: %s", msg)) } func (helper *Helper) startAPIServer() { ctx := context.Background() - // Configure JWK certificate URL for API server helper.requireJWTIssuers() - if len(helper.Env().Config.Server.JWT.Configs) > 0 { - cfg := &helper.Env().Config.Server.JWT.Configs[0] - cfg.JWKCertURL = jwkURL - if cfg.IdentityHeader == "" { - cfg.IdentityHeader = defaultTestIdentityHeader + if len(helper.AppConfig.Server.JWT.Configs) > 0 { + if helper.AppConfig.Server.JWT.Configs[0].IdentityHeader == "" { + helper.AppConfig.Server.JWT.Configs[0].IdentityHeader = defaultTestIdentityHeader } } - cfg := helper.Env().Config + cfg := helper.AppConfig - // Only build the JWT handler when auth is on; it starts a JWKS refresh goroutine. - var jwtHandler *auth.JWTHandler - if cfg.Server.JWT.Enabled { - var jwtErr error - jwtHandler, jwtErr = helper.Container.JWTHandler() - if jwtErr != nil { - helper.failStartup(ctx, jwtErr, "Unable to create JWT handler") - } - } - - schemaValidator, schemaErr := helper.Container.SchemaValidator() - if schemaErr != nil { - helper.failStartup(ctx, schemaErr, "Unable to create schema validator") - } + jwtHandler := helper.Container.JWTHandler() + helper.jwtHandler = jwtHandler - // Disable tracing for integration tests (no OTLP collector required) apiServer, err := servecmd.BuildAPIServer( cfg, helper.Container.ResourceService(), helper.Container.AdapterStatusService(), - schemaValidator, + helper.Container.SchemaValidator(), jwtHandler, - helper.Container.SessionFactory(), - false, + helper.DBFactory, ) if err != nil { - helper.failStartup(ctx, err, "Unable to build Test API server") + abortSetup(ctx, helper.closer, err, "Unable to build Test API server") } helper.APIServer = apiServer + // No graceful drain here: nothing depends on it, the test binary exits right after Teardown. + helper.closer.Add(helper.APIServer.Close) + listener, err := helper.APIServer.Listen() if err != nil { - helper.failStartup(ctx, err, "Unable to start Test API server") + abortSetup(ctx, helper.closer, err, "Unable to start Test API server") } go func() { logger.Debug(ctx, "Test API server started") - helper.APIServer.Serve(listener) + if err := helper.APIServer.Serve(listener); err != nil { + logger.WithError(ctx, err).Error("Test API server terminated with errors") + } logger.Debug(ctx, "Test API server stopped") }() } -func (helper *Helper) stopAPIServer() error { - if helper.APIServer == nil { - return nil - } - if err := helper.APIServer.Stop(); err != nil { - return fmt.Errorf("unable to stop api server: %w", err) - } - return nil -} - -func (helper *Helper) closeContainer() error { - if helper.Container == nil { - return nil - } - helper.Container.Close() - return nil -} - -func (helper *Helper) stopJWKMock() error { - if helper.jwkTeardown == nil { - return nil - } - return helper.jwkTeardown() -} - -func (helper *Helper) startMetricsServer() { - ctx := context.Background() - helper.MetricsServer = server.NewMetricsServer() - go func() { - logger.Debug(ctx, "Test Metrics server started") - helper.MetricsServer.Start() - logger.Debug(ctx, "Test Metrics server stopped") - }() -} - -func (helper *Helper) stopMetricsServer() error { - if helper.MetricsServer == nil { - return nil - } - if err := helper.MetricsServer.Stop(); err != nil { - return fmt.Errorf("unable to stop metrics server: %w", err) - } - return nil -} - -func (helper *Helper) stopHealthServer() error { - if helper.HealthServer == nil { - return nil - } - if err := helper.HealthServer.Stop(); err != nil { - return fmt.Errorf("unable to stop health server: %w", err) - } - return nil -} - -func (helper *Helper) startHealthServer() { - ctx := context.Background() - helper.HealthServer = server.NewHealthServer() - go func() { - logger.Debug(ctx, "Test health check server started") - helper.HealthServer.Start() - logger.Debug(ctx, "Test health check server stopped") - }() -} - -func (helper *Helper) RestartMetricsServer() { - ctx := context.Background() - if err := helper.stopMetricsServer(); err != nil { - logger.WithError(ctx, err).Warn("unable to stop metrics server on restart") - } - helper.startMetricsServer() - logger.Debug(ctx, "Test metrics server restarted") -} - // NewID creates a new unique ID used internally func (helper *Helper) NewID() string { id, err := uuid.NewV7() @@ -369,46 +250,22 @@ func (helper *Helper) NewID() string { return id.String() } -// NewUUID creates a new unique UUID, which has different formatting than ksuid -// UUID is used by telemeter and we validate the format. -func (helper *Helper) NewUUID() string { - return uuid.New().String() -} - -func (helper *Helper) RestURL(path string) string { - protocol := "http" //nolint:goconst // Protocol strings used across URL builders +func (helper *Helper) baseURL() string { + scheme := "http" if helper.AppConfig.Server.TLS.Enabled { - protocol = "https" //nolint:goconst // Protocol strings used across URL builders + scheme = "https" } - return fmt.Sprintf("%s://%s/api/hyperfleet/v1%s", protocol, helper.AppConfig.Server.BindAddress(), path) + return fmt.Sprintf("%s://%s", scheme, helper.AppConfig.Server.BindAddress()) } -func (helper *Helper) MetricsURL(path string) string { - protocol := "http" //nolint:goconst // Protocol strings used across URL builders - if helper.AppConfig.Metrics.TLS.Enabled { - protocol = "https" //nolint:goconst // Protocol strings used across URL builders - } - return fmt.Sprintf("%s://%s%s", protocol, helper.AppConfig.Metrics.BindAddress(), path) -} - -func (helper *Helper) HealthURL(path string) string { - protocol := "http" //nolint:goconst // Protocol strings used across URL builders - if helper.AppConfig.Health.TLS.Enabled { - protocol = "https" //nolint:goconst // Protocol strings used across URL builders - } - return fmt.Sprintf("%s://%s%s", protocol, helper.AppConfig.Health.BindAddress(), path) +func (helper *Helper) RestURL(path string) string { + return helper.baseURL() + "/api/hyperfleet/v1" + path } func (helper *Helper) NewAPIClient() *openapi.ClientWithResponses { - // Build the server URL - protocol := "http" //nolint:goconst // Protocol strings used across URL builders - if helper.AppConfig.Server.TLS.Enabled { - protocol = "https" //nolint:goconst // Protocol strings used across URL builders - } - serverURL := fmt.Sprintf("%s://%s", protocol, helper.AppConfig.Server.BindAddress()) - client, err := openapi.NewClientWithResponses(serverURL) + client, err := openapi.NewClientWithResponses(helper.baseURL()) if err != nil { - helper.T.Fatalf("Failed to create API client: %v", err) + panic(fmt.Sprintf("test setup: failed to create API client: %v", err)) } return client } @@ -442,7 +299,6 @@ func (helper *Helper) NewAccount(username, name, email string) *TestAccount { } } -// contextKeyAccessToken is a context key for storing the access token type contextKeyAccessToken struct{} // ContextAccessToken is the context key for access tokens (used by tests) @@ -483,107 +339,62 @@ func WithIdentityHeader(headerName, headerValue string) openapi.RequestEditorFn } // IdentityHeaderName returns the configured identity header name from the first JWT issuer config. -func IdentityHeaderName() string { - configs := environments.Environment().Config.Server.JWT.Configs +func (helper *Helper) IdentityHeaderName() string { + if helper == nil || helper.AppConfig == nil { + return "" + } + configs := helper.AppConfig.Server.JWT.Configs if len(configs) > 0 { return configs[0].IdentityHeader } return "" } -func (helper *Helper) StartJWKCertServerMock() (teardown func() error) { - helper.requireJWTIssuers() - jwkURL, teardown = mocks.NewJWKCertServerMock(helper.T, helper.JWTCA, jwkKID, jwkAlg) - if len(helper.Env().Config.Server.JWT.Configs) > 0 { - helper.Env().Config.Server.JWT.Configs[0].JWKCertURL = jwkURL - } - return teardown -} - -func (helper *Helper) DeleteAll(table interface{}) { - g2 := helper.DBFactory.New(context.Background()) - err := g2.Model(table).Delete(table).Error - if err != nil { - helper.T.Errorf("error deleting from table %v: %v", table, err) - } -} - -func (helper *Helper) Delete(obj interface{}) { - g2 := helper.DBFactory.New(context.Background()) - err := g2.Delete(obj).Error - if err != nil { - helper.T.Errorf("error deleting object %v: %v", obj, err) - } -} - -func (helper *Helper) SkipIfShort() { - if testing.Short() { - helper.T.Skip("Skipping execution of test in short mode") +func (helper *Helper) ResetDB() error { + if len(helper.tables) == 0 { + return nil } -} - -func (helper *Helper) Count(table string) int64 { g2 := helper.DBFactory.New(context.Background()) - var count int64 - err := g2.Table(table).Count(&count).Error - if err != nil { - helper.T.Errorf("error getting count for table %s: %v", table, err) + if err := g2.Exec(fmt.Sprintf("TRUNCATE TABLE %s CASCADE", strings.Join(helper.tables, ", "))).Error; err != nil { + return fmt.Errorf("truncate business tables: %w", err) } - return count + return nil } func (helper *Helper) MigrateDB() error { return db.Migrate(helper.DBFactory.New(context.Background())) } -func (helper *Helper) ClearAllTables() { - // Reserved for future use -} - func (helper *Helper) CleanDB() error { g2 := helper.DBFactory.New(context.Background()) tables, err := helper.getAllTables(g2) if err != nil { - helper.T.Errorf("error discovering tables: %v", err) - return err + return fmt.Errorf("error discovering tables: %w", err) } orderedTables, err := helper.orderTablesByDependencies(g2, tables) if err != nil { - helper.T.Errorf("error ordering tables by dependencies: %v", err) - return err + return fmt.Errorf("error ordering tables by dependencies: %w", err) } for _, table := range orderedTables { - if g2.Migrator().HasTable(table) { - if err := g2.Migrator().DropTable(table); err != nil { - helper.T.Errorf("error dropping table %s: %v", table, err) - return err - } + if err := g2.Migrator().DropTable(table); err != nil { + return fmt.Errorf("error dropping table %s: %w", table, err) } } - // Truncate migrations table so MigrateDB() will re-run all migrations - // This ensures tables are recreated after CleanDB() drops them if err := g2.Exec("TRUNCATE TABLE migrations").Error; err != nil { - helper.T.Errorf("error truncating migrations table: %v", err) - return err + return fmt.Errorf("error truncating migrations table: %w", err) } return nil } -// System tables should not be dropped var systemTables = []string{"migrations"} func isSystemTable(tableName string) bool { - for _, sysTable := range systemTables { - if tableName == sysTable { - return true - } - } - return false + return slices.Contains(systemTables, tableName) } func (helper *Helper) getAllTables(g2 *gorm.DB) ([]string, error) { @@ -602,23 +413,43 @@ func (helper *Helper) getAllTables(g2 *gorm.DB) ([]string, error) { return tables, nil } -// Child tables (with foreign keys) come before parent tables to ensure safe deletion +type fkEdge struct { + TableName string + ReferencedName string +} + func (helper *Helper) orderTablesByDependencies(g2 *gorm.DB, tables []string) ([]string, error) { - dependencies := make(map[string][]string) + var edges []fkEdge + query := ` + SELECT DISTINCT tc.table_name, ccu.table_name AS referenced_name + FROM information_schema.table_constraints AS tc + JOIN information_schema.key_column_usage AS kcu + ON tc.constraint_name = kcu.constraint_name + AND tc.table_schema = kcu.table_schema + JOIN information_schema.constraint_column_usage AS ccu + ON ccu.constraint_name = tc.constraint_name + AND ccu.table_schema = tc.table_schema + WHERE tc.constraint_type = 'FOREIGN KEY' + AND tc.table_schema = 'public' + ` + if err := g2.Raw(query).Scan(&edges).Error; err != nil { + return nil, fmt.Errorf("query foreign key edges: %w", err) + } + dependencies := make(map[string][]string, len(tables)) for _, table := range tables { - deps, err := helper.getTableDependencies(g2, table) - if err != nil { - return nil, err + dependencies[table] = nil + } + for _, e := range edges { + if e.TableName == e.ReferencedName { + continue } - - filteredDeps := []string{} - for _, dep := range deps { - if !isSystemTable(dep) { - filteredDeps = append(filteredDeps, dep) - } + if _, inScope := dependencies[e.ReferencedName]; !inScope { + continue + } + if !isSystemTable(e.ReferencedName) { + dependencies[e.TableName] = append(dependencies[e.TableName], e.ReferencedName) } - dependencies[table] = filteredDeps } ordered := []string{} @@ -631,9 +462,7 @@ func (helper *Helper) orderTablesByDependencies(g2 *gorm.DB, tables []string) ([ return nil } if visiting[table] { - err := fmt.Errorf("circular foreign key dependency detected involving table '%s'", table) - helper.T.Errorf("%v", err) - return err + return fmt.Errorf("circular foreign key dependency detected involving table '%s'", table) } visiting[table] = true @@ -654,53 +483,32 @@ func (helper *Helper) orderTablesByDependencies(g2 *gorm.DB, tables []string) ([ } } - for i, j := 0, len(ordered)-1; i < j; i, j = i+1, j-1 { - ordered[i], ordered[j] = ordered[j], ordered[i] - } + slices.Reverse(ordered) return ordered, nil } -func (helper *Helper) getTableDependencies(g2 *gorm.DB, tableName string) ([]string, error) { - var dependencies []string - query := ` - SELECT DISTINCT ccu.table_name - FROM information_schema.table_constraints AS tc - JOIN information_schema.key_column_usage AS kcu - ON tc.constraint_name = kcu.constraint_name - AND tc.table_schema = kcu.table_schema - JOIN information_schema.constraint_column_usage AS ccu - ON ccu.constraint_name = tc.constraint_name - AND ccu.table_schema = tc.table_schema - WHERE tc.constraint_type = 'FOREIGN KEY' - AND tc.table_schema = 'public' - AND tc.table_name = ? - ` - err := g2.Raw(query, tableName).Scan(&dependencies).Error - if err != nil { - return nil, err - } - return dependencies, nil -} - -func (helper *Helper) ResetDB() error { +func (helper *Helper) RebuildSchema() error { if err := helper.CleanDB(); err != nil { return err } - if err := helper.MigrateDB(); err != nil { return err } - + tables, err := helper.getAllTables(helper.DBFactory.New(context.Background())) + if err != nil { + return fmt.Errorf("discover tables for truncation: %w", err) + } + helper.tables = tables return nil } func (helper *Helper) CreateJWTString(account *TestAccount) string { helper.requireJWTIssuers() var issuerURL, audience string - if len(helper.Env().Config.Server.JWT.Configs) > 0 { - issuerURL = helper.Env().Config.Server.JWT.Configs[0].IssuerURL - audience = helper.Env().Config.Server.JWT.Configs[0].Audience + if len(helper.AppConfig.Server.JWT.Configs) > 0 { + issuerURL = helper.AppConfig.Server.JWT.Configs[0].IssuerURL + audience = helper.AppConfig.Server.JWT.Configs[0].Audience } claims := jwt.MapClaims{ "iss": issuerURL, @@ -723,59 +531,29 @@ func (helper *Helper) CreateJWTString(account *TestAccount) string { signedToken, err := token.SignedString(helper.JWTPrivateKey) if err != nil { - helper.T.Errorf("Unable to sign test jwt: %s", err) - return "" + panic(fmt.Sprintf("test setup: unable to sign test JWT: %s", err)) } return signedToken } -func (helper *Helper) CreateJWTToken(account *TestAccount) *jwt.Token { - tokenString := helper.CreateJWTString(account) - - token, err := jwt.Parse(tokenString, func(token *jwt.Token) (interface{}, error) { - return helper.JWTCA, nil - }) - if err != nil { - helper.T.Errorf("Unable to parse signed jwt: %s", err) - return nil - } - return token -} - -// OpenapiError Convert an error response body to an openapi error struct -func (helper *Helper) OpenapiError(body []byte) openapi.ProblemDetails { - var exErr openapi.ProblemDetails - jsonErr := json.Unmarshal(body, &exErr) - if jsonErr != nil { - helper.T.Errorf("Unable to convert error response to openapi error: %s", jsonErr) - } - return exErr -} - func parseJWTKeys() (*rsa.PrivateKey, *rsa.PublicKey, error) { privateBytes, err := privatebytes() if err != nil { - return nil, nil, fmt.Errorf("unable to read JWT key file %s: %w", jwtKeyFile, err) + return nil, nil, fmt.Errorf("unable to decode JWT private key: %w", err) } pubBytes, err := publicbytes() if err != nil { - return nil, nil, fmt.Errorf("unable to read JWT ca file %s: %w", jwtCAFile, err) + return nil, nil, fmt.Errorf("unable to decode JWT CA: %w", err) } - // Parse keys - // ParseRSAPrivateKeyFromPEMWithPassword is deprecated in the stdlib but there's - // no suitable alternative for our test fixture keys; explicitly silence the - // staticcheck warning here. //nolint:staticcheck privateKey, err := jwt.ParseRSAPrivateKeyFromPEMWithPassword(privateBytes, "passwd") if err != nil { - err = fmt.Errorf("unable to parse JWT private key: %s", err) - return nil, nil, err + return nil, nil, fmt.Errorf("unable to parse JWT private key: %w", err) } pubKey, err := jwt.ParseRSAPublicKeyFromPEM(pubBytes) if err != nil { - err = fmt.Errorf("unable to parse JWT ca: %s", err) - return nil, nil, err + return nil, nil, fmt.Errorf("unable to parse JWT ca: %w", err) } return privateKey, pubKey, nil @@ -841,11 +619,10 @@ RVJUSUZJQ0FURS0tLS0tCg==` return base64.StdEncoding.DecodeString(s) } -// initTestLogger initializes a default logger for tests func initTestLogger() { cfg := &logger.LogConfig{ Level: slog.LevelInfo, - Format: logger.FormatText, // Use text format for test readability + Format: logger.FormatText, Output: os.Stdout, Component: "hyperfleet-api-test", Version: "test", @@ -853,47 +630,3 @@ func initTestLogger() { } logger.InitGlobalLogger(cfg) } - -// loadDefaultTestEntities registers the standard entity descriptors that -// integration tests need when no config file provides them. -func loadDefaultTestEntities() { - registry.LoadDescriptors([]registry.EntityDescriptor{ - { - Kind: "Cluster", - Plural: "clusters", - SpecSchemaName: "ClusterSpec", - NameMinLen: 3, - NameMaxLen: 53, - RequireSpecSchema: true, - RequiredAdapters: []string{"validation", "dns", "pullsecret", "hypershift"}, - }, - { - Kind: "NodePool", - Plural: "nodepools", - ParentKind: "Cluster", - OnParentDelete: registry.OnParentDeleteCascade, - SpecSchemaName: "NodePoolSpec", - NameMinLen: 3, - NameMaxLen: 15, - RequireSpecSchema: true, - RequiredAdapters: []string{"validation", "hypershift"}, - }, - { - Kind: "Channel", - Plural: "channels", - SpecSchemaName: "ChannelSpec", - }, - { - Kind: "Version", - Plural: "versions", - ParentKind: "Channel", - OnParentDelete: registry.OnParentDeleteRestrict, - SpecSchemaName: "VersionSpec", - }, - { - Kind: "WifConfig", - Plural: "wifconfigs", - SpecSchemaName: "WifConfigSpec", - }, - }) -} diff --git a/test/integration/advisory_locks_test.go b/test/integration/advisory_locks_test.go index dda26594..8f16103a 100644 --- a/test/integration/advisory_locks_test.go +++ b/test/integration/advisory_locks_test.go @@ -236,7 +236,7 @@ func TestConcurrentMigrations(t *testing.T) { h, _ := test.RegisterIntegration(t) // First, reset the database to a clean state - err := h.ResetDB() + err := h.RebuildSchema() Expect(err).NotTo(HaveOccurred(), "Failed to reset database") total := 5 @@ -450,7 +450,7 @@ func TestMigrationFailureUnderLock(t *testing.T) { h, _ := test.RegisterIntegration(t) // Reset database to clean state - err := h.ResetDB() + err := h.RebuildSchema() Expect(err).NotTo(HaveOccurred(), "Failed to reset database") // Channels to coordinate goroutines diff --git a/test/integration/caller_identity_test.go b/test/integration/caller_identity_test.go index 4fbd2302..8b8f6138 100644 --- a/test/integration/caller_identity_test.go +++ b/test/integration/caller_identity_test.go @@ -75,7 +75,7 @@ func TestCallerIdentityCreate(t *testing.T) { opts := []openapi.RequestEditorFn{test.WithAuthToken(ctx)} if tc.setHeader { - opts = append(opts, test.WithIdentityHeader(test.IdentityHeaderName(), tc.headerActor)) + opts = append(opts, test.WithIdentityHeader(h.IdentityHeaderName(), tc.headerActor)) } resp, err := client.PostClusterWithResponse( @@ -151,7 +151,7 @@ func TestCallerIdentityPatch(t *testing.T) { opts := []openapi.RequestEditorFn{test.WithAuthToken(patchCtx)} if tc.setHeader { - opts = append(opts, test.WithIdentityHeader(test.IdentityHeaderName(), tc.headerActor)) + opts = append(opts, test.WithIdentityHeader(h.IdentityHeaderName(), tc.headerActor)) } patchResp, err := client.PatchClusterByIdWithResponse( @@ -218,7 +218,7 @@ func TestCallerIdentityMultiplePatches(t *testing.T) { ctxA, clusterID, openapi.PatchClusterByIdJSONRequestBody{Spec: &spec3}, test.WithAuthToken(ctxA), - test.WithIdentityHeader(test.IdentityHeaderName(), "user-c@gateway.com"), + test.WithIdentityHeader(h.IdentityHeaderName(), "user-c@gateway.com"), ) Expect(err).NotTo(HaveOccurred()) Expect(patch2.StatusCode()).To(Equal(http.StatusOK)) @@ -253,7 +253,7 @@ func TestCallerIdentityDelete(t *testing.T) { createResp, err := client.PostClusterWithResponse( ctx, openapi.PostClusterJSONRequestBody(clusterInput), test.WithAuthToken(ctx), - test.WithIdentityHeader(test.IdentityHeaderName(), "header-creator@corp.com"), + test.WithIdentityHeader(h.IdentityHeaderName(), "header-creator@corp.com"), ) Expect(err).NotTo(HaveOccurred()) Expect(createResp.StatusCode()).To(Equal(http.StatusCreated)) @@ -290,7 +290,7 @@ func TestCallerIdentityEmptyHeaderFallback(t *testing.T) { resp, err := client.PostClusterWithResponse( ctx, openapi.PostClusterJSONRequestBody(clusterInput), test.WithAuthToken(ctx), - test.WithIdentityHeader(test.IdentityHeaderName(), ""), + test.WithIdentityHeader(h.IdentityHeaderName(), ""), ) Expect(err).NotTo(HaveOccurred()) Expect(resp.StatusCode()).To(Equal(http.StatusCreated)) @@ -316,7 +316,7 @@ func TestCallerIdentityOversizedHeader(t *testing.T) { resp, err := client.PostClusterWithResponse( ctx, openapi.PostClusterJSONRequestBody(clusterInput), test.WithAuthToken(ctx), - test.WithIdentityHeader(test.IdentityHeaderName(), oversized), + test.WithIdentityHeader(h.IdentityHeaderName(), oversized), ) Expect(err).NotTo(HaveOccurred()) Expect(resp.StatusCode()).To(Equal(http.StatusUnauthorized)) diff --git a/test/integration/integration_test.go b/test/integration/integration_test.go index 696d5a57..0ce9c03e 100755 --- a/test/integration/integration_test.go +++ b/test/integration/integration_test.go @@ -9,6 +9,10 @@ import ( "testing" "time" + "github.com/testcontainers/testcontainers-go" + "github.com/testcontainers/testcontainers-go/modules/postgres" + "github.com/testcontainers/testcontainers-go/wait" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-api/test" ) @@ -22,23 +26,16 @@ func TestMain(m *testing.M) { // This enables schema validation middleware during tests // Uses HYPERFLEET_SERVER_OPENAPI_SCHEMA_PATH (config system standard) if os.Getenv("HYPERFLEET_SERVER_OPENAPI_SCHEMA_PATH") == "" { - // Get the repo root directory (2 levels up from test/integration) - // Use runtime.Caller to find this file's path _, filename, _, ok := runtime.Caller(0) if !ok { logger.Warn(ctx, "Failed to determine current file path via runtime.Caller, skipping schema path setup") } else { - // filename is like: /path/to/repo/test/integration/integration_test.go - // Navigate up: integration_test.go -> integration -> test -> repo - integrationDir := filepath.Dir(filename) // /path/to/repo/test/integration - testDir := filepath.Dir(integrationDir) // /path/to/repo/test - repoRoot := filepath.Dir(testDir) // /path/to/repo - - // Prefer the integration test validation schema, which includes every - // registered entity that declares SpecSchemaName. + integrationDir := filepath.Dir(filename) + testDir := filepath.Dir(integrationDir) + repoRoot := filepath.Dir(testDir) + schemaPath := filepath.Join(repoRoot, "test", "validation-schema.yaml") - // Verify the schema file exists before setting the env var if _, err := os.Stat(schemaPath); err != nil { logger.With(ctx, logger.FieldSchemaPath, schemaPath).WithError(err). Warn("Schema file not found, skipping schema path setup") @@ -50,7 +47,9 @@ func TestMain(m *testing.M) { } } - helper := test.NewHelper(&testing.T{}) + pgContainer := startTestcontainer(ctx) + + helper := test.NewHelper() exitCode := m.Run() // Force exit if teardown hangs (e.g., due to a panic leaving resources in a bad state). @@ -68,5 +67,61 @@ func TestMain(m *testing.M) { }() helper.Teardown() + + terminateContainer(ctx, pgContainer) os.Exit(exitCode) } + +func terminateContainer(ctx context.Context, pgContainer *postgres.PostgresContainer) { + termCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + if err := pgContainer.Terminate(termCtx); err != nil { + logger.WithError(ctx, err).Error("Failed to terminate testcontainer") + } +} + +func envOrDefault(key, fallback string) string { + if v := os.Getenv(key); v != "" { + return v + } + return fallback +} + +func startTestcontainer(ctx context.Context) *postgres.PostgresContainer { + dbName := envOrDefault("HYPERFLEET_DATABASE_NAME", "hyperfleet_test") + dbUser := envOrDefault("HYPERFLEET_DATABASE_USERNAME", "test") + dbPass := envOrDefault("HYPERFLEET_DATABASE_PASSWORD", "test") + + pgContainer, err := postgres.Run(ctx, + "postgres:14.23", + postgres.WithDatabase(dbName), + postgres.WithUsername(dbUser), + postgres.WithPassword(dbPass), + testcontainers.WithWaitStrategy( + wait.ForListeningPort("5432/tcp"). + WithStartupTimeout(60*time.Second)), + ) + if err != nil { + logger.WithError(ctx, err).Error("Failed to start PostgreSQL testcontainer") + os.Exit(1) + } + + host, err := pgContainer.Host(ctx) + if err != nil { + logger.WithError(ctx, err).Error("Failed to get testcontainer host") + terminateContainer(ctx, pgContainer) + os.Exit(1) + } + mappedPort, err := pgContainer.MappedPort(ctx, "5432/tcp") + if err != nil { + logger.WithError(ctx, err).Error("Failed to get testcontainer mapped port") + terminateContainer(ctx, pgContainer) + os.Exit(1) + } + + os.Setenv("HYPERFLEET_DATABASE_HOST", host) + os.Setenv("HYPERFLEET_DATABASE_PORT", mappedPort.Port()) + + logger.With(ctx, "host", host, "port", mappedPort.Port()).Info("PostgreSQL testcontainer started") + return pgContainer +} diff --git a/test/mocks/jwk_cert_server.go b/test/mocks/jwk_cert_server.go index 3cca2776..bf25e2b9 100755 --- a/test/mocks/jwk_cert_server.go +++ b/test/mocks/jwk_cert_server.go @@ -5,7 +5,6 @@ import ( "fmt" "net/http" "net/http/httptest" - "testing" "github.com/mendsley/gojwk" ) @@ -15,7 +14,6 @@ const ( ) func NewJWKCertServerMock( - t *testing.T, pubKey crypto.PublicKey, jwkKID string, jwkAlg string, @@ -25,19 +23,17 @@ func NewJWKCertServerMock( func(w http.ResponseWriter, r *http.Request) { pubjwk, err := gojwk.PublicKey(pubKey) if err != nil { - t.Errorf("Unable to generate public jwk: %s", err) + http.Error(w, fmt.Sprintf("generate public jwk: %v", err), http.StatusInternalServerError) return } pubjwk.Kid = jwkKID pubjwk.Alg = jwkAlg jwkBytes, err := gojwk.Marshal(pubjwk) if err != nil { - t.Errorf("Unable to marshal public jwk: %s", err) + http.Error(w, fmt.Sprintf("marshal public jwk: %v", err), http.StatusInternalServerError) return } - if _, err := fmt.Fprintf(w, `{"keys":[%s]}`, string(jwkBytes)); err != nil { - t.Errorf("error writing jwk response: %v", err) - } + fmt.Fprintf(w, `{"keys":[%s]}`, string(jwkBytes)) }, ) diff --git a/test/registration.go b/test/registration.go index 656df607..fdcbe458 100755 --- a/test/registration.go +++ b/test/registration.go @@ -11,14 +11,11 @@ import ( // RegisterIntegration Register a test // This should be run before every integration test func RegisterIntegration(t *testing.T) (*Helper, *openapi.ClientWithResponses) { - // Register the test with gomega gm.RegisterTestingT(t) - // Create a new helper - helper := NewHelper(t) - // Reset the database to a seeded blank state - helper.DBFactory.ResetDB() - // Create an api client + helper := NewHelper() + if err := helper.ResetDB(); err != nil { + t.Fatalf("failed to reset database: %v", err) + } client := helper.NewAPIClient() - return helper, client } diff --git a/test/support/certs.json b/test/support/certs.json deleted file mode 100755 index 71cb126f..00000000 --- a/test/support/certs.json +++ /dev/null @@ -1,12 +0,0 @@ -{ - "keys": [ - { - "kid": "HjYaVHwyM77lw0mv7ko-qC7tKri03jqSukNea0SWY7M", - "kty": "RSA", - "alg": "RS256", - "use": "sig", - "n": "q6DF0dZFJnnVVIUtyaVV9Hial9hsSRXtH8Z01kOoAdGwQLqFjKDzNeliOL9KL0i-D71Bo9vKp13Qo8r9UjjNPGV6HzxgXR95MIZP4nqWo9Qp_9SHOjxMSqg-ZFf45p0pSKRdgKTfzu0eJ1CpZt4BdYM9wM3iuOgon09hIMKcO0AU7xqX0KmCg-ToIgVDCaGtXqcC0qv3fr7acTUBoVd8sWNaIOKXiL90cR7oZX_wLoApF2cQyrgTozaMrdEe3RuvwU8hE_r3kYTUYsxTv0liJ8FRfuO5FJuEGVpYc7QDyIztt9YOqowQgHq_2IhqcWhULtzGIXh26voAgWfA2BGAFw", - "e": "AQAB" - } - ] -} diff --git a/test/support/jwt_ca.pem b/test/support/jwt_ca.pem deleted file mode 100755 index 458bcc5f..00000000 --- a/test/support/jwt_ca.pem +++ /dev/null @@ -1,19 +0,0 @@ ------BEGIN CERTIFICATE----- -MIIC/zCCAeegAwIBAgIBATANBgkqhkiG9w0BAQUFADAaMQswCQYDVQQGEwJVUzEL -MAkGA1UECgwCWjQwHhcNMTMwODI4MTgyODM0WhcNMjMwODI4MTgyODM0WjAaMQsw -CQYDVQQGEwJVUzELMAkGA1UECgwCWjQwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAw -ggEKAoIBAQDfdOqotHd55SYO0dLz2oXengw/tZ+q3ZmOPeVmMuOMIYO/Cv1wk2U0 -OK4pug4OBSJPhl09Zs6IwB8NwPOU7EDTgMOcQUYB/6QNCI1J7Zm2oLtuchzz4pIb -+o4ZAhVprLhRyvqi8OTKQ7kfGfs5Tuwmn1M/0fQkfzMxADpjOKNgf0uy6lN6utjd -TrPKKFUQNdc6/Ty8EeTnQEwUlsT2LAXCfEKxTn5RlRljDztS7Sfgs8VL0FPy1Qi8 -B+dFcgRYKFrcpsVaZ1lBmXKsXDRu5QR/Rg3f9DRq4GR1sNH8RLY9uApMl2SNz+sR -4zRPG85R/se5Q06Gu0BUQ3UPm67ETVZLAgMBAAGjUDBOMB0GA1UdDgQWBBQHZPTE -yQVu/0I/3QWhlTyW7WoTzTAfBgNVHSMEGDAWgBQHZPTEyQVu/0I/3QWhlTyW7WoT -zTAMBgNVHRMEBTADAQH/MA0GCSqGSIb3DQEBBQUAA4IBAQDHxqJ9y8alTH7agVMW -Zfic/RbrdvHwyq+IOrgDToqyo0w+IZ6BCn9vjv5iuhqu4ForOWDAFpQKZW0DLBJE -Qy/7/0+9pk2DPhK1XzdOovlSrkRt+GcEpGnUXnzACXDBbO0+Wrk+hcjEkQRRK1bW -2rknARIEJG9GS+pShP9Bq/0BmNsMepdNcBa0z3a5B0fzFyCQoUlX6RTqxRw1h1Qt -5F00pfsp7SjXVIvYcewHaNASbto1n5hrSz1VY9hLba11ivL1N4WoWbmzAL6BWabs -C2D/MenST2/X6hTKyGXpg3Eg2h3iLvUtwcNny0hRKstc73Jl9xR3qXfXKJH0ThTl -q0gq ------END CERTIFICATE----- diff --git a/test/testdata/integration-config.yaml b/test/testdata/integration-config.yaml new file mode 100644 index 00000000..aaa2dcc9 --- /dev/null +++ b/test/testdata/integration-config.yaml @@ -0,0 +1,16 @@ +database: + name: hyperfleet_test + username: test + password: test + ssl: + mode: disable + +server: + jwt: + enabled: true + configs: + - issuer_url: https://test-issuer.example.com + jwk_cert_url: https://jwks.invalid/.well-known/jwks.json + header: Authorization + identity_claim: email + identity_header: X-HyperFleet-Identity