From f412da426a71b1da98c1b0e8d59d70aa05f158d0 Mon Sep 17 00:00:00 2001 From: Dmitrii Andreev Date: Thu, 23 Jul 2026 09:55:52 -0500 Subject: [PATCH] HYPERFLEET-1370 - refactor: introduce typed container for API server dependencies Replace the plugin-based route registration (plugins/entities, adapterStatus, generic, resources) with a typed Container (cmd/hyperfleet-api/container) that lazily constructs and caches DAOs, services, and handlers via constructor injection instead of a global environments singleton. - Add Container with lazy-cached DAO/service/handler accessors and an APIServer(tracingEnabled) builder - Move entity route registration into cmd/hyperfleet-api/server (routes_entities.go, renamed from plugins/entities/plugin.go) - Refactor APIServer to take injected cfg and handler via its constructor rather than reaching into environments.Environment() - Split router middleware into public (metadata/openapi) vs protected (auth + transaction) subrouters so JWT, schema-validation, and transaction middleware never wrap the public docs endpoints - Simplify JWTHandler to a multi-issuer, mux.MiddlewareFunc-based design, dropping the old PublicPaths/Next self-contained handler - Add unit coverage: container_test.go, api_server_test.go, and routes_test.go (asserts public routes bypass middleware while protected routes are gated by it) --- AGENTS.md | 15 +- CLAUDE.md | 30 ++- CONTRIBUTING.md | 8 +- README.md | 2 +- cmd/hyperfleet-api/container/auth.go | 24 +++ cmd/hyperfleet-api/container/container.go | 52 ++++++ .../container/container_test.go | 62 +++++++ cmd/hyperfleet-api/container/daos.go | 40 ++++ cmd/hyperfleet-api/container/services.go | 32 ++++ cmd/hyperfleet-api/container/validation.go | 21 +++ cmd/hyperfleet-api/main.go | 6 - cmd/hyperfleet-api/servecmd/api_server.go | 72 +++++++ cmd/hyperfleet-api/servecmd/cmd.go | 32 +++- cmd/hyperfleet-api/server/api_server.go | 103 ++++------- cmd/hyperfleet-api/server/api_server_test.go | 175 ++++++++++++++++++ cmd/hyperfleet-api/server/compress.go | 13 +- cmd/hyperfleet-api/server/routes.go | 139 +++++--------- .../hyperfleet-api/server/routes_entities.go | 56 +++--- .../server/routes_entities_test.go | 13 +- cmd/hyperfleet-api/server/routes_test.go | 90 +++++++++ cmd/hyperfleet-api/server/server.go | 10 + pkg/auth/jwt_handler.go | 104 +++++------ pkg/auth/jwt_handler_test.go | 157 ++++++++-------- pkg/config/server.go | 20 ++ pkg/db/sql_helpers.go | 2 +- pkg/handlers/resource_handler.go | 2 +- plugins/CLAUDE.md | 33 ---- plugins/adapterStatus/plugin.go | 38 ---- plugins/generic/plugin.go | 36 ---- plugins/resources/plugin.go | 42 ----- test/factories/clusters.go | 11 +- test/factories/factory.go | 6 + test/factories/node_pools.go | 4 +- test/helper.go | 88 +++++---- test/integration/resource_helpers.go | 4 +- 35 files changed, 960 insertions(+), 582 deletions(-) create mode 100644 cmd/hyperfleet-api/container/auth.go create mode 100644 cmd/hyperfleet-api/container/container.go create mode 100644 cmd/hyperfleet-api/container/container_test.go create mode 100644 cmd/hyperfleet-api/container/daos.go create mode 100644 cmd/hyperfleet-api/container/services.go create mode 100644 cmd/hyperfleet-api/container/validation.go create mode 100644 cmd/hyperfleet-api/servecmd/api_server.go create mode 100644 cmd/hyperfleet-api/server/api_server_test.go rename plugins/entities/plugin.go => cmd/hyperfleet-api/server/routes_entities.go (68%) rename plugins/entities/plugin_test.go => cmd/hyperfleet-api/server/routes_entities_test.go (91%) create mode 100644 cmd/hyperfleet-api/server/routes_test.go delete mode 100644 plugins/CLAUDE.md delete mode 100644 plugins/adapterStatus/plugin.go delete mode 100755 plugins/generic/plugin.go delete mode 100644 plugins/resources/plugin.go diff --git a/AGENTS.md b/AGENTS.md index cd40fb5f..f0878c4d 100755 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,6 +81,9 @@ Run `make help` for the complete target list. ``` 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) @@ -92,10 +95,6 @@ pkg/ errors/ # ServiceError type, RFC 9457 Problem Details logger/ # Structured logging (slog-based) config/ # Configuration management -plugins/ # Plugin registration (init-based) - entities/plugin.go # Config-driven entity route registration - resources/plugin.go # Resource routes - generic/plugin.go openapi/ README.md # Schema import, code generation, and validation details openapi.yaml # Not in git — generated by make generate @@ -141,9 +140,13 @@ Interface + `sql*Service` struct. Constructor injection of DAOs. Return `*errors Interface + `sql*Dao` struct. Get session via `sessionFactory.New(ctx)`. Call `db.MarkForRollback(ctx, err)` on write errors. Return stdlib `error`. -### Plugins +### Entity Routes -Entity types are config-driven — declared in `config.yaml` under `entities:` and auto-registered at startup. See `plugins/entities/plugin.go`. +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 diff --git a/CLAUDE.md b/CLAUDE.md index bb4ff7c6..08d19d96 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -7,7 +7,7 @@ HyperFleet API is a **stateless REST API** serving as the pure CRUD data layer f - **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**: Plugin-based route registration, transaction-per-request middleware +- **Architecture**: Container-based dependency injection, config-driven route registration, transaction-per-request middleware ## Critical First Steps @@ -82,10 +82,29 @@ Interface + `sql*Dao` implementation using SessionFactory: - Get session: `db.New(ctx)` — extracts transaction from request context - On write errors: call `db.MarkForRollback(ctx, err)` -### Plugin Registration +### 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 `plugins/entities/plugin.go`. -No per-entity Go code needed. See `plugins/CLAUDE.md` for details. +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.NewRouter` 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)` @@ -103,6 +122,8 @@ No per-entity Go code needed. See `plugins/CLAUDE.md` for details. - 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.NewRouter` + `server.NewAPIServer` +- Public routes (`/openapi`, `/openapi.html`, metadata) bypass auth, schema validation, and transaction middleware; everything else is gated by both, auth outermost ## Boundaries @@ -122,7 +143,6 @@ Subdirectories contain context-specific guidance that loads when you work in tho - `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 -- `plugins/CLAUDE.md` — Plugin registration (config-driven entities) - `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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 477d48c0..c7a8404a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -55,7 +55,10 @@ Overview of key directories and their purpose. ``` hyperfleet-api/ ├── cmd/hyperfleet-api/ # Application entry point and CLI commands -│ └── main.go # Server start, migrate, version commands +│ ├── main.go # Server start, migrate, version commands +│ ├── container/ # Dependency container (DAOs, services, validator, JWT handler) +│ ├── servecmd/ # serve command; api_server.go composes the API server +│ └── server/ # HTTP servers, router, middleware, entity routes ├── pkg/ │ ├── api/ # Generated OpenAPI types (DO NOT EDIT) │ │ └── openapi/ # Generated from openapi/openapi.yaml @@ -66,9 +69,6 @@ hyperfleet-api/ │ ├── logger/ # Structured logging (slog-based) │ ├── presenters/ # Response presenters (DAO models → API responses) │ └── services/ # Business logic layer (status aggregation, validation) -├── plugins/ # Plugin-based route registration (config-driven entities) -│ ├── entities/ # Entity route auto-generation from config -│ └── resources/ # Resource route registration ├── openapi/ # API specification source │ ├── openapi.yaml # Source spec (TypeSpec output, has $ref) │ └── oapi-codegen.yaml # Code generation configuration diff --git a/README.md b/README.md index 3e96a344..f05565c7 100755 --- a/README.md +++ b/README.md @@ -62,7 +62,7 @@ Groups of compute nodes within clusters. ### Generic Resources -The API also supports generic resource types registered via the plugin system. Currently available: +The API also supports generic resource types declared in `config.yaml` under `entities:`. Currently available: - **WifConfigs** — `GET/POST /api/hyperfleet/v1/wifconfigs`, `GET/PATCH/DELETE .../wifconfigs/{id}` - **Channels** — `GET/POST /api/hyperfleet/v1/channels`, `GET/PATCH/DELETE .../channels/{id}` diff --git a/cmd/hyperfleet-api/container/auth.go b/cmd/hyperfleet-api/container/auth.go new file mode 100644 index 00000000..3bc2b5af --- /dev/null +++ b/cmd/hyperfleet-api/container/auth.go @@ -0,0 +1,24 @@ +package container + +import ( + "context" + "fmt" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/auth" +) + +func (c *Container) JWTHandler() (*auth.JWTHandler, error) { + if c.jwtHandler == nil { + jwtHandler, err := auth.NewJWTHandler( + context.Background(), + auth.JWTHandlerConfig{ + Issuers: c.cfg.Server.JWT.Configs, + }, + ) + if err != nil { + return nil, fmt.Errorf("unable to create JWT handler: %w", err) + } + c.jwtHandler = jwtHandler + } + return c.jwtHandler, nil +} diff --git a/cmd/hyperfleet-api/container/container.go b/cmd/hyperfleet-api/container/container.go new file mode 100644 index 00000000..c44c1764 --- /dev/null +++ b/cmd/hyperfleet-api/container/container.go @@ -0,0 +1,52 @@ +package container + +import ( + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/auth" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/validators" +) + +// 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 + sessionFactory db.SessionFactory + + resourceDao dao.ResourceDao + resourceLabelDao dao.ResourceLabelDao + adapterStatusDao dao.AdapterStatusDao + resourceConditionDao dao.ResourceConditionDao + genericDao dao.GenericDao + + resourceService services.ResourceService + adapterStatusService services.AdapterStatusService + genericService services.GenericService + + schemaValidator *validators.SchemaValidator + 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() + } +} diff --git a/cmd/hyperfleet-api/container/container_test.go b/cmd/hyperfleet-api/container/container_test.go new file mode 100644 index 00000000..7b00d4bb --- /dev/null +++ b/cmd/hyperfleet-api/container/container_test.go @@ -0,0 +1,62 @@ +package container + +import ( + "testing" + + . "github.com/onsi/gomega" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" + dbmocks "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db/mocks" +) + +func newTestContainer(t *testing.T) *Container { + t.Helper() + + sessionFactory := dbmocks.NewMockSessionFactory() + t.Cleanup(func() { _ = sessionFactory.Close() }) + + return NewContainer(config.NewApplicationConfig(), sessionFactory) +} + +func TestContainerCachesDAOsAndServices(t *testing.T) { + RegisterTestingT(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()) + Expect(c.ResourceLabelDao()).To(BeIdenticalTo(c.ResourceLabelDao())) + Expect(c.AdapterStatusDao()).NotTo(BeNil()) + Expect(c.AdapterStatusDao()).To(BeIdenticalTo(c.AdapterStatusDao())) + Expect(c.ResourceConditionDao()).NotTo(BeNil()) + Expect(c.ResourceConditionDao()).To(BeIdenticalTo(c.ResourceConditionDao())) + Expect(c.GenericDao()).NotTo(BeNil()) + Expect(c.GenericDao()).To(BeIdenticalTo(c.GenericDao())) + + Expect(c.GenericService()).NotTo(BeNil()) + Expect(c.GenericService()).To(BeIdenticalTo(c.GenericService())) + Expect(c.AdapterStatusService()).NotTo(BeNil()) + Expect(c.AdapterStatusService()).To(BeIdenticalTo(c.AdapterStatusService())) + Expect(c.ResourceService()).NotTo(BeNil()) + Expect(c.ResourceService()).To(BeIdenticalTo(c.ResourceService())) +} + +func TestContainerConstructionIsLazy(t *testing.T) { + RegisterTestingT(t) + + c := newTestContainer(t) + + Expect(c.resourceDao).To(BeNil()) + Expect(c.resourceLabelDao).To(BeNil()) + Expect(c.adapterStatusDao).To(BeNil()) + Expect(c.resourceConditionDao).To(BeNil()) + Expect(c.genericDao).To(BeNil()) + Expect(c.resourceService).To(BeNil()) + Expect(c.adapterStatusService).To(BeNil()) + Expect(c.genericService).To(BeNil()) + Expect(c.schemaValidator).To(BeNil()) + Expect(c.jwtHandler).To(BeNil()) +} diff --git a/cmd/hyperfleet-api/container/daos.go b/cmd/hyperfleet-api/container/daos.go new file mode 100644 index 00000000..a932936a --- /dev/null +++ b/cmd/hyperfleet-api/container/daos.go @@ -0,0 +1,40 @@ +package container + +import ( + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" +) + +func (c *Container) ResourceDao() dao.ResourceDao { + if c.resourceDao == nil { + 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) + } + return c.resourceLabelDao +} + +func (c *Container) AdapterStatusDao() dao.AdapterStatusDao { + if c.adapterStatusDao == nil { + 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) + } + return c.resourceConditionDao +} + +func (c *Container) GenericDao() dao.GenericDao { + if c.genericDao == nil { + c.genericDao = dao.NewGenericDao(c.sessionFactory) + } + return c.genericDao +} diff --git a/cmd/hyperfleet-api/container/services.go b/cmd/hyperfleet-api/container/services.go new file mode 100644 index 00000000..fd652433 --- /dev/null +++ b/cmd/hyperfleet-api/container/services.go @@ -0,0 +1,32 @@ +package container + +import ( + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" +) + +func (c *Container) ResourceService() services.ResourceService { + if c.resourceService == nil { + c.resourceService = services.NewResourceService( + c.ResourceDao(), + c.ResourceLabelDao(), + c.AdapterStatusDao(), + c.ResourceConditionDao(), + c.GenericService(), + ) + } + return c.resourceService +} + +func (c *Container) AdapterStatusService() services.AdapterStatusService { + if c.adapterStatusService == nil { + c.adapterStatusService = services.NewAdapterStatusService(c.AdapterStatusDao()) + } + return c.adapterStatusService +} + +func (c *Container) GenericService() services.GenericService { + if c.genericService == nil { + c.genericService = services.NewGenericService(c.GenericDao()) + } + return c.genericService +} diff --git a/cmd/hyperfleet-api/container/validation.go b/cmd/hyperfleet-api/container/validation.go new file mode 100644 index 00000000..3bde4daa --- /dev/null +++ b/cmd/hyperfleet-api/container/validation.go @@ -0,0 +1,21 @@ +package container + +import ( + "context" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/validators" +) + +func (c *Container) SchemaValidator() *validators.SchemaValidator { + if c.schemaValidator == nil { + schemaPath := c.cfg.Server.OpenAPISchemaPath + schemaValidator, err := validators.NewSchemaValidator(schemaPath) + if err != nil { + panic("unable to create schema validator: " + err.Error()) + } + c.schemaValidator = schemaValidator + logger.With(context.Background(), logger.FieldSchemaPath, schemaPath).Info("Schema validation enabled") + } + return c.schemaValidator +} diff --git a/cmd/hyperfleet-api/main.go b/cmd/hyperfleet-api/main.go index aad589c7..88d73edf 100755 --- a/cmd/hyperfleet-api/main.go +++ b/cmd/hyperfleet-api/main.go @@ -13,12 +13,6 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/servecmd" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" - - // Import plugins to trigger their init() functions - _ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/adapterStatus" - _ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/entities" - _ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/generic" - _ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/resources" ) // nolint diff --git a/cmd/hyperfleet-api/servecmd/api_server.go b/cmd/hyperfleet-api/servecmd/api_server.go new file mode 100644 index 00000000..5248d6a1 --- /dev/null +++ b/cmd/hyperfleet-api/servecmd/api_server.go @@ -0,0 +1,72 @@ +package servecmd + +import ( + "net/http" + + "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/server" + requestlogging "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/server/logging" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/auth" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/config" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/middleware" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/validators" +) + +// BuildAPIServer assembles the middleware chains and route registrars, then +// wires them into a router and HTTP server. +// +// 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; callers construct +// it only when auth is on, so the JWKS refresh goroutine is never started for a +// server that will not authenticate. +func BuildAPIServer( + cfg *config.ApplicationConfig, + resourceService services.ResourceService, + adapterStatusService services.AdapterStatusService, + schemaValidator *validators.SchemaValidator, + jwtHandler *auth.JWTHandler, + sessionFactory db.SessionFactory, + tracingEnabled bool, +) (*server.APIServer, error) { + mainMiddleware := []server.Middleware{logger.RequestIDMiddleware} + if tracingEnabled { + mainMiddleware = append(mainMiddleware, middleware.OTelMiddleware) + } + masker := middleware.NewMaskingMiddleware(cfg.Logging) + mainMiddleware = append(mainMiddleware, requestlogging.RequestLoggingMiddleware(masker)) + + apiMiddleware := []server.Middleware{ + server.MetricsMiddleware, + middleware.SchemaValidationMiddleware(schemaValidator), + func(next http.Handler) http.Handler { + return db.TransactionMiddleware(next, sessionFactory, cfg.Database.Pool.RequestTimeout) + }, + server.CompressMiddleware, + } + + var protectedMiddleware []server.Middleware + if cfg.Server.JWT.Enabled { + callerIdentityMiddleware := auth.NewCallerIdentityMiddleware() + protectedMiddleware = append( + protectedMiddleware, + jwtHandler.Middleware, + callerIdentityMiddleware.ResolveCallerIdentity, + ) + } + + registrars := []server.RouteRegistrar{ + server.NewEntityRouteRegistrar(resourceService, adapterStatusService, schemaValidator), + } + + router, err := server.NewRouterFromConfig(mainMiddleware, apiMiddleware, protectedMiddleware, registrars) + if err != nil { + return nil, err + } + + return server.NewAPIServer(cfg.Server, router), nil +} diff --git a/cmd/hyperfleet-api/servecmd/cmd.go b/cmd/hyperfleet-api/servecmd/cmd.go index 2913da25..b95ab5b2 100755 --- a/cmd/hyperfleet-api/servecmd/cmd.go +++ b/cmd/hyperfleet-api/servecmd/cmd.go @@ -11,9 +11,11 @@ import ( "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/config" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db/db_session" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/health" @@ -146,7 +148,34 @@ func runServe(cmd *cobra.Command, args []string) { } } - apiServer := server.NewAPIServer(tracingEnabled) + 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) + } + } + + // Deliberately not reusing err: it is last written above, and reusing it here + // would make the intermediate inner err declarations shadow a live variable. + apiServer, buildErr := BuildAPIServer( + cfg, + ctr.ResourceService(), + ctr.AdapterStatusService(), + 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() @@ -183,6 +212,7 @@ func runServe(cmd *cobra.Command, args []string) { if err := metricsServer.Stop(); err != nil { logger.WithError(ctx, err).Error("Failed to stop metrics server") } + ctr.Close() if tp != nil { shutdownCtx, cancel := context.WithTimeout( diff --git a/cmd/hyperfleet-api/server/api_server.go b/cmd/hyperfleet-api/server/api_server.go index 91690682..d2bd8a04 100755 --- a/cmd/hyperfleet-api/server/api_server.go +++ b/cmd/hyperfleet-api/server/api_server.go @@ -8,68 +8,43 @@ import ( "os" "time" - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/auth" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" ) -type apiServer struct { - httpServer *http.Server - jwtHandler *auth.JWTHandler +type cfg interface { + BindAddress() string + ReadTimeout() time.Duration + WriteTimeout() time.Duration + TLSEnabled() bool + TLSCertFile() string + TLSKeyFile() string } -var _ Server = &apiServer{} - -func env() *environments.Env { - return environments.Environment() +type APIServer struct { + cfg cfg + httpServer *http.Server } -func NewAPIServer(tracingEnabled bool) Server { - s := &apiServer{} - - mainRouter := s.routes(tracingEnabled) - - // referring to the router as type http.Handler allows us to add middleware via more handlers - var mainHandler http.Handler = mainRouter - - if env().Config.Server.JWT.Enabled { - jwtHandler, err := auth.NewJWTHandler(context.Background(), auth.JWTHandlerConfig{ - Issuers: env().Config.Server.JWT.Configs, - PublicPaths: []string{ - "^/api/hyperfleet/?$", - "^/api/hyperfleet/v1/?$", - "^/api/hyperfleet/v1/openapi/?$", - "^/api/hyperfleet/v1/openapi.html/?$", - "^/api/hyperfleet/v1/errors(/.*)?$", - }, - Next: mainHandler, - }) - check(err, "Unable to create JWT authentication handler") - s.jwtHandler = jwtHandler - mainHandler = jwtHandler +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) + }, } - - mainHandler = removeTrailingSlash(mainHandler) - - s.httpServer = &http.Server{ - Addr: env().Config.Server.BindAddress(), - Handler: mainHandler, - ReadTimeout: env().Config.Server.Timeouts.Read, - WriteTimeout: env().Config.Server.Timeouts.Write, - ReadHeaderTimeout: 10 * time.Second, // Hardcoded to prevent Slowloris attacks (not user-configurable) - } - - return s } // 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) { +func (s *APIServer) Serve(listener net.Listener) { ctx := context.Background() var err error - if env().Config.Server.TLS.Enabled { - // Check https cert and key path - if env().Config.Server.TLS.CertFile == "" || env().Config.Server.TLS.KeyFile == "" { + if s.cfg.TLSEnabled() { + if s.cfg.TLSCertFile() == "" || s.cfg.TLSKeyFile() == "" { check( fmt.Errorf( "HTTPS certificate or key not configured; "+ @@ -79,15 +54,13 @@ func (s apiServer) Serve(listener net.Listener) { ) } - // Serve with TLS - logger.With(ctx, logger.FieldBindAddress, env().Config.Server.BindAddress()).Info("Serving with TLS") - err = s.httpServer.ServeTLS(listener, env().Config.Server.TLS.CertFile, env().Config.Server.TLS.KeyFile) + 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, env().Config.Server.BindAddress()).Info("Serving without TLS") + logger.With(ctx, logger.FieldBindAddress, s.cfg.BindAddress()).Info("Serving without TLS") err = s.httpServer.Serve(listener) } - // Web server terminated. if err != nil && err != http.ErrServerClosed { check(err, "Web server terminated with errors") } else { @@ -97,36 +70,24 @@ func (s apiServer) Serve(listener net.Listener) { // 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", env().Config.Server.BindAddress()) +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() { +func (s *APIServer) Start() { ctx := context.Background() listener, err := s.Listen() if err != nil { - logger.WithError(ctx, err).Error("Unable to start API server") + logger.WithError(ctx, err).Error(fmt.Sprintf("Unable to start API server on %s", s.cfg.BindAddress())) os.Exit(1) } s.Serve(listener) - - // after the server exits but before the application terminates - // we need to explicitly close Go's sql connection pool. - // this needs to be called *exactly* once during an app's lifetime. - if err := env().Database.SessionFactory.Close(); err != nil { - logger.WithError(ctx, err).Error("Error closing database connection") - } } -func (s apiServer) Stop() error { +func (s *APIServer) Stop() error { ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - err := s.httpServer.Shutdown(ctx) - // Close JWT handler after HTTP drain so in-flight requests can still verify tokens. - if s.jwtHandler != nil { - s.jwtHandler.Close() - } - return err + 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 new file mode 100644 index 00000000..bad263ec --- /dev/null +++ b/cmd/hyperfleet-api/server/api_server_test.go @@ -0,0 +1,175 @@ +package server + +import ( + "crypto/rand" + "crypto/rsa" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "net" + "net/http" + "os" + "path/filepath" + "testing" + "time" + + . "github.com/onsi/gomega" +) + +type testAPIServerConfig struct { + bindAddress string + tlsCertFile string + tlsKeyFile string + readTimeout time.Duration + writeTimeout time.Duration + tlsEnabled bool +} + +func (c testAPIServerConfig) BindAddress() string { + return c.bindAddress +} + +func (c testAPIServerConfig) ReadTimeout() time.Duration { + return c.readTimeout +} + +func (c testAPIServerConfig) WriteTimeout() time.Duration { + return c.writeTimeout +} + +func (c testAPIServerConfig) TLSEnabled() bool { + return c.tlsEnabled +} + +func (c testAPIServerConfig) TLSCertFile() string { + return c.tlsCertFile +} + +func (c testAPIServerConfig) TLSKeyFile() string { + return c.tlsKeyFile +} + +func TestAPIServerServeWithoutTLS(t *testing.T) { + RegisterTestingT(t) + + listener, err := net.Listen("tcp", "127.0.0.1:0") + Expect(err).NotTo(HaveOccurred()) + + s := NewAPIServer( + testAPIServerConfig{bindAddress: listener.Addr().String()}, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNoContent) + }), + ) + + done := make(chan struct{}) + go func() { + defer close(done) + s.Serve(listener) + }() + + var resp *http.Response + Eventually(func() error { + var err error + resp, err = http.Get("http://" + listener.Addr().String()) + return err + }, "2s", "25ms").Should(Succeed()) + t.Cleanup(func() { _ = resp.Body.Close() }) + Expect(resp.StatusCode).To(Equal(http.StatusNoContent)) + + Expect(s.Stop()).To(Succeed()) + <-done +} + +func TestAPIServerServeWithTLS(t *testing.T) { + RegisterTestingT(t) + + certFile, keyFile := writeSelfSignedCert(t) + listener, err := net.Listen("tcp", "127.0.0.1:0") + Expect(err).NotTo(HaveOccurred()) + + s := NewAPIServer( + testAPIServerConfig{ + bindAddress: listener.Addr().String(), + tlsEnabled: true, + tlsCertFile: certFile, + tlsKeyFile: keyFile, + }, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusAccepted) + }), + ) + + done := make(chan struct{}) + go func() { + defer close(done) + s.Serve(listener) + }() + + certPEM, err := os.ReadFile(certFile) + Expect(err).NotTo(HaveOccurred()) + certPool := x509.NewCertPool() + Expect(certPool.AppendCertsFromPEM(certPEM)).To(BeTrue()) + + client := &http.Client{ + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{RootCAs: certPool, MinVersion: tls.VersionTLS13}, + }, + } + + var resp *http.Response + Eventually(func() error { + var err error + resp, err = client.Get("https://" + listener.Addr().String()) + return err + }, "2s", "25ms").Should(Succeed()) + t.Cleanup(func() { _ = resp.Body.Close() }) + Expect(resp.StatusCode).To(Equal(http.StatusAccepted)) + + Expect(s.Stop()).To(Succeed()) + <-done +} + +func writeSelfSignedCert(t *testing.T) (string, string) { + t.Helper() + + privateKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("generate private key: %v", err) + } + + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "127.0.0.1"}, + DNSNames: []string{"localhost"}, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + BasicConstraintsValid: true, + } + + certDER, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &privateKey.PublicKey, privateKey) + if err != nil { + t.Fatalf("create certificate: %v", err) + } + + dir := t.TempDir() + certFile := filepath.Join(dir, "server.crt") + keyFile := filepath.Join(dir, "server.key") + + certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certDER}) + keyPEM := pem.EncodeToMemory(&pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(privateKey)}) + + if err := os.WriteFile(certFile, certPEM, 0o600); err != nil { + t.Fatalf("write cert: %v", err) + } + if err := os.WriteFile(keyFile, keyPEM, 0o600); err != nil { + t.Fatalf("write key: %v", err) + } + + return certFile, keyFile +} diff --git a/cmd/hyperfleet-api/server/compress.go b/cmd/hyperfleet-api/server/compress.go index 201a132b..de8147e8 100644 --- a/cmd/hyperfleet-api/server/compress.go +++ b/cmd/hyperfleet-api/server/compress.go @@ -19,23 +19,22 @@ type gzipResponseWriter struct { wroteHeader bool } -func (w *gzipResponseWriter) WriteHeader(statusCode int) { +func (w *gzipResponseWriter) ensureHeaders() { if !w.wroteHeader { w.wroteHeader = true w.Header().Set("Content-Encoding", "gzip") w.Header().Add("Vary", "Accept-Encoding") w.Header().Del("Content-Length") } +} + +func (w *gzipResponseWriter) WriteHeader(statusCode int) { + w.ensureHeaders() w.ResponseWriter.WriteHeader(statusCode) } func (w *gzipResponseWriter) Write(b []byte) (int, error) { - if !w.wroteHeader { - w.wroteHeader = true - w.Header().Set("Content-Encoding", "gzip") - w.Header().Add("Vary", "Accept-Encoding") - w.Header().Del("Content-Length") - } + w.ensureHeaders() return w.writer.Write(b) } diff --git a/cmd/hyperfleet-api/server/routes.go b/cmd/hyperfleet-api/server/routes.go index dbfeea5b..21df2c0d 100755 --- a/cmd/hyperfleet-api/server/routes.go +++ b/cmd/hyperfleet-api/server/routes.go @@ -1,127 +1,78 @@ package server import ( - "context" "fmt" - "net/http" - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/server/logging" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/auth" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/handlers" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/middleware" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/validators" ) -// APIBasePath is the root path of the HyperFleet API, below the host. -const APIBasePath = "/api/hyperfleet" +const apiBasePath = "/api/hyperfleet" -// APIV1BasePath is the root path of the v1 HyperFleet API. -const APIV1BasePath = APIBasePath + "/v1" +const apiV1BasePath = apiBasePath + "/v1" -type ServicesInterface interface { - GetService(name string) any +type RouteRegistrar struct { + Register func(*Router) error + Name string } -type RouteRegistrationFunc func( - apiV1Router *Router, - services ServicesInterface, -) - -var routeRegistry = make(map[string]RouteRegistrationFunc) - -func RegisterRoutes(name string, registrationFunc RouteRegistrationFunc) { - routeRegistry[name] = registrationFunc -} - -// LoadDiscoveredRoutes invokes all registered route registration functions. -// -// Note: All routes must use a method-prefixed pattern (e.g. "GET /path") to restrict HTTP methods. -func LoadDiscoveredRoutes( - apiV1Router *Router, - services ServicesInterface, -) { - for name, registrationFunc := range routeRegistry { - registrationFunc(apiV1Router, services) - _ = name // prevent unused variable warning - } -} - -func (s *apiServer) routes(tracingEnabled bool) *Router { - services := &env().Services - +func NewRouterFromConfig( + mainMiddleware []Middleware, + apiMiddleware []Middleware, + protectedMiddleware []Middleware, + registrars []RouteRegistrar, +) (*Router, error) { metadataHandler := handlers.NewMetadataHandler() // mainRouter is top level "/" mainRouter := NewRouter() - - // Request ID middleware sets a unique request ID in the context of each request for tracing - mainRouter.Use(logger.RequestIDMiddleware) - - // OpenTelemetry middleware (conditionally enabled) - // Extracts trace_id/span_id from traceparent header and adds to logger context - if tracingEnabled { - mainRouter.Use(middleware.OTelMiddleware) + for _, mw := range mainMiddleware { + mainRouter.Use(mw) } - // Initialize masking middleware once (reused across all requests) - masker := middleware.NewMaskingMiddleware(env().Config.Logging) - - // Request logging middleware logs pertinent information about the request and response - mainRouter.Use(logging.RequestLoggingMiddleware(masker)) - // /api/hyperfleet apiRouter := mainRouter.Group() - apiRouter.HandleFunc("GET "+APIBasePath, metadataHandler.Get) + apiRouter.HandleFunc("GET "+apiBasePath, metadataHandler.Get) // /api/hyperfleet/v1 apiV1Router := apiRouter.Group() - err := registerAPIMiddleware(apiV1Router) - check(err, "Failed to initialize API middleware") - - if env().Config.Server.JWT.Enabled { - callerIdentityMW := auth.NewCallerIdentityMiddleware() - apiV1Router.Use(callerIdentityMW.ResolveCallerIdentity) - } - // /api/hyperfleet/v1/openapi openapiHandler, err := handlers.NewOpenAPIHandler() - check(err, "Unable to create OpenAPI handler") - apiV1Router.HandleFunc("GET "+APIV1BasePath+"/openapi.html", openapiHandler.GetOpenAPIUI) - apiV1Router.HandleFunc("GET "+APIV1BasePath+"/openapi", openapiHandler.GetOpenAPI) - - // Auto-discovered routes (no manual editing needed) - LoadDiscoveredRoutes(apiV1Router, services) - - return mainRouter -} - -func registerAPIMiddleware(router *Router) error { - router.Use(MetricsMiddleware) - - registry.Validate() - - schemaPath := env().Config.Server.OpenAPISchemaPath - ctx := context.Background() - - schemaValidator, err := validators.NewSchemaValidator(schemaPath) if err != nil { - return fmt.Errorf("schema validation required but failed to load from %s: %w", schemaPath, err) + return nil, fmt.Errorf("unable to create OpenAPI handler: %w", err) + } + apiV1Router.HandleFunc("GET "+apiV1BasePath+"/openapi.html", openapiHandler.GetOpenAPIUI) + apiV1Router.HandleFunc("GET "+apiV1BasePath+"/openapi", openapiHandler.GetOpenAPI) + + // protectedMiddleware (auth) must be outermost so unauthenticated requests are + // rejected before apiMiddleware opens a DB transaction or runs schema validation. + protectedRouter := apiV1Router.Group() + for _, mw := range protectedMiddleware { + protectedRouter.Use(mw) + } + for _, mw := range apiMiddleware { + protectedRouter.Use(mw) } - logger.With(ctx, logger.FieldSchemaPath, schemaPath).Info("Schema validation enabled") - router.Use(middleware.SchemaValidationMiddleware(schemaValidator)) - - router.Use( - func(next http.Handler) http.Handler { - return db.TransactionMiddleware(next, env().Database.SessionFactory, env().Config.Database.Pool.RequestTimeout) - }, - ) + if err := registerRoutes(protectedRouter, registrars); err != nil { + return nil, err + } - router.Use(CompressMiddleware) + return mainRouter, nil +} +// SendNotFound is re-exported so tests can reference it without importing pkg/api. +var SendNotFound = api.SendNotFound + +func registerRoutes(router *Router, registrars []RouteRegistrar) error { + for _, registrar := range registrars { + if registrar.Register == nil { + return fmt.Errorf("register %s routes: registrar is nil", registrar.Name) + } + if err := registrar.Register(router); err != nil { + return fmt.Errorf("register %s routes: %w", registrar.Name, err) + } + } return nil } diff --git a/plugins/entities/plugin.go b/cmd/hyperfleet-api/server/routes_entities.go similarity index 68% rename from plugins/entities/plugin.go rename to cmd/hyperfleet-api/server/routes_entities.go index 85951d67..5d6a917a 100644 --- a/plugins/entities/plugin.go +++ b/cmd/hyperfleet-api/server/routes_entities.go @@ -1,37 +1,27 @@ -package entities +package server import ( "fmt" "sort" - "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/handlers" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/validators" - "github.com/openshift-hyperfleet/hyperfleet-api/plugins/adapterStatus" - "github.com/openshift-hyperfleet/hyperfleet-api/plugins/resources" ) -func init() { - server.RegisterRoutes("entities", func(apiV1Router *server.Router, svc server.ServicesInterface) { - envServices := svc.(*environments.Services) - resourceService := resources.Service(envServices) - adapterStatusService := adapterStatus.Service(envServices) - - schemaPath := environments.Environment().Config.Server.OpenAPISchemaPath - var schemaValidator *validators.SchemaValidator - if schemaPath != "" { - var err error - schemaValidator, err = validators.NewSchemaValidator(schemaPath) - if err != nil { - panic(fmt.Sprintf("failed to load schema validator from %s: %v", schemaPath, err)) - } - } - - RegisterEntityRoutes(apiV1Router, resourceService, adapterStatusService, schemaValidator) - }) +func NewEntityRouteRegistrar( + resourceService services.ResourceService, + adapterStatusService services.AdapterStatusService, + schemaValidator *validators.SchemaValidator, +) RouteRegistrar { + return RouteRegistrar{ + Name: "entities", + Register: func(apiV1Router *Router) error { + RegisterEntityRoutes(apiV1Router, resourceService, adapterStatusService, schemaValidator) + return nil + }, + } } // RegisterEntityRoutes creates handlers and registers routes for every entity @@ -40,12 +30,12 @@ func init() { // // Top-level entities get routes at /{plural}. Child entities (ParentKind != "") // get nested routes under /{parent_plural}/{parent_id}/{plural} plus flat -// read/update/delete access at /{plural} (POST rejected — needs parent context). +// read/update/delete access at /{plural} (POST rejected - needs parent context). // All entities get /{id}/statuses sub-routes for adapter status reporting. // // The kind-agnostic /resources root endpoint is registered separately. func RegisterEntityRoutes( - apiV1Router *server.Router, + apiV1Router *Router, resourceService services.ResourceService, adapterStatusService services.AdapterStatusService, schemaValidator *validators.SchemaValidator, @@ -55,7 +45,7 @@ func RegisterEntityRoutes( } func registerPerEntityRoutes( - apiV1Router *server.Router, + apiV1Router *Router, resourceService services.ResourceService, adapterStatusService services.AdapterStatusService, ) { @@ -76,20 +66,20 @@ func registerPerEntityRoutes( if descriptor.ParentKind != "" { parent := registry.MustGet(descriptor.ParentKind) - registerResourceRoutes(apiV1Router, "/"+parent.Plural+"/{parent_id}/"+descriptor.Plural, h, sh) + registerEntityResourceRoutes(apiV1Router, "/"+parent.Plural+"/{parent_id}/"+descriptor.Plural, h, sh) } - registerResourceRoutes(apiV1Router, "/"+descriptor.Plural, h, sh) + registerEntityResourceRoutes(apiV1Router, "/"+descriptor.Plural, h, sh) } } func registerRootResourceRoutes( - apiV1Router *server.Router, + apiV1Router *Router, resourceService services.ResourceService, adapterStatusService services.AdapterStatusService, schemaValidator *validators.SchemaValidator, ) { rootHandler := handlers.NewRootResourceHandler(resourceService, adapterStatusService, schemaValidator) - prefix := server.APIV1BasePath + "/resources" + prefix := apiV1BasePath + "/resources" apiV1Router.HandleFunc("GET "+prefix, rootHandler.List) apiV1Router.HandleFunc("POST "+prefix, rootHandler.Create) apiV1Router.HandleFunc("GET "+prefix+"/{id}", rootHandler.Get) @@ -100,11 +90,11 @@ func registerRootResourceRoutes( apiV1Router.HandleFunc("PUT "+prefix+"/{id}/statuses", rootHandler.CreateStatus) } -func registerResourceRoutes( - apiV1Router *server.Router, pathSuffix string, +func registerEntityResourceRoutes( + apiV1Router *Router, pathSuffix string, h *handlers.ResourceHandler, sh *handlers.ResourceStatusHandler, ) { - prefix := server.APIV1BasePath + pathSuffix + prefix := apiV1BasePath + pathSuffix apiV1Router.HandleFunc("GET "+prefix, h.List) apiV1Router.HandleFunc("POST "+prefix, h.Create) apiV1Router.HandleFunc("GET "+prefix+"/{id}", h.Get) diff --git a/plugins/entities/plugin_test.go b/cmd/hyperfleet-api/server/routes_entities_test.go similarity index 91% rename from plugins/entities/plugin_test.go rename to cmd/hyperfleet-api/server/routes_entities_test.go index a60fe6bb..914a6226 100644 --- a/plugins/entities/plugin_test.go +++ b/cmd/hyperfleet-api/server/routes_entities_test.go @@ -1,4 +1,4 @@ -package entities +package server import ( "net/http/httptest" @@ -7,7 +7,6 @@ import ( "github.com/google/uuid" . "github.com/onsi/gomega" - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/server" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" ) @@ -20,7 +19,7 @@ func TestRegisterEntityRoutes_TopLevelEntity(t *testing.T) { SpecSchemaName: "ChannelSpec", }) - apiV1 := server.NewRouter() + apiV1 := NewRouter() RegisterEntityRoutes(apiV1, nil, nil, nil) id := uuid.NewString() @@ -47,7 +46,7 @@ func TestRegisterEntityRoutes_ChildEntity(t *testing.T) { ParentKind: "Channel", }) - apiV1 := server.NewRouter() + apiV1 := NewRouter() RegisterEntityRoutes(apiV1, nil, nil, nil) parentID := uuid.NewString() @@ -81,7 +80,7 @@ func TestRegisterEntityRoutes_UnresolvableParentKind_Panics(t *testing.T) { ParentKind: "NonExistent", }) - apiV1 := server.NewRouter() + apiV1 := NewRouter() Expect(func() { RegisterEntityRoutes(apiV1, nil, nil, nil) @@ -92,14 +91,14 @@ func TestRegisterEntityRoutes_EmptyRegistry(t *testing.T) { RegisterTestingT(t) registry.Reset() - apiV1 := server.NewRouter() + apiV1 := NewRouter() Expect(func() { RegisterEntityRoutes(apiV1, nil, nil, nil) }).ToNot(Panic()) } -func assertRouteMatches(t *testing.T, router *server.Router, method, path string) { +func assertRouteMatches(t *testing.T, router *Router, method, path string) { t.Helper() req := httptest.NewRequest(method, path, nil) _, pattern := router.Handler(req) diff --git a/cmd/hyperfleet-api/server/routes_test.go b/cmd/hyperfleet-api/server/routes_test.go new file mode 100644 index 00000000..62d7a512 --- /dev/null +++ b/cmd/hyperfleet-api/server/routes_test.go @@ -0,0 +1,90 @@ +package server + +import ( + "net/http" + "net/http/httptest" + "testing" + + . "github.com/onsi/gomega" +) + +// TestNewRouterFromConfig_PublicVsProtectedMiddleware guards the auth boundary set up in NewRouterFromConfig: +// public routes (metadata, openapi, openapi.html) must never see apiMiddleware or +// protectedMiddleware; protected routes must see both, with protectedMiddleware +// (auth) gating the request before apiMiddleware (schema validation, DB +// transaction) ever runs. +func TestNewRouterFromConfig_PublicVsProtectedMiddleware(t *testing.T) { + RegisterTestingT(t) + + var apiMiddlewareCalls, protectedMiddlewareCalls int + var authorized bool + countingAPIMiddleware := func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + apiMiddlewareCalls++ + next.ServeHTTP(w, r) + }) + } + gatingProtectedMiddleware := func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + protectedMiddlewareCalls++ + if !authorized { + w.WriteHeader(http.StatusUnauthorized) + return + } + next.ServeHTTP(w, r) + }) + } + + registrar := RouteRegistrar{ + Name: "widgets", + Register: func(r *Router) error { + r.HandleFunc("GET "+apiV1BasePath+"/widgets", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + return nil + }, + } + + router, err := NewRouterFromConfig( + nil, + []Middleware{countingAPIMiddleware}, + []Middleware{gatingProtectedMiddleware}, + []RouteRegistrar{registrar}, + ) + Expect(err).NotTo(HaveOccurred()) + + publicPaths := []string{ + "/api/hyperfleet", + "/api/hyperfleet/v1/openapi", + "/api/hyperfleet/v1/openapi.html", + } + for _, path := range publicPaths { + apiMiddlewareCalls, protectedMiddlewareCalls = 0, 0 + + rr := httptest.NewRecorder() + router.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, path, nil)) + + Expect(rr.Code).To(Equal(http.StatusOK), "public path %s should be reachable without auth", path) + Expect(apiMiddlewareCalls).To(Equal(0), "apiMiddleware must not run for public path %s", path) + Expect(protectedMiddlewareCalls).To(Equal(0), "protectedMiddleware must not run for public path %s", path) + } + + authorized = false + apiMiddlewareCalls, protectedMiddlewareCalls = 0, 0 + rr := httptest.NewRecorder() + router.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, "/api/hyperfleet/v1/widgets", nil)) + + Expect(rr.Code).To(Equal(http.StatusUnauthorized), "protected route must be gated by protectedMiddleware") + Expect(protectedMiddlewareCalls).To(Equal(1), "protectedMiddleware must run for protected routes") + Expect(apiMiddlewareCalls).To(Equal(0), + "apiMiddleware (schema validation, DB transaction) must not run when auth rejects the request") + + authorized = true + apiMiddlewareCalls, protectedMiddlewareCalls = 0, 0 + rr = httptest.NewRecorder() + router.ServeHTTP(rr, httptest.NewRequest(http.MethodGet, "/api/hyperfleet/v1/widgets", nil)) + + Expect(rr.Code).To(Equal(http.StatusOK), "protected route must be reachable once auth passes") + Expect(protectedMiddlewareCalls).To(Equal(1), "protectedMiddleware must run for protected routes") + Expect(apiMiddlewareCalls).To(Equal(1), "apiMiddleware must run once auth allows the request through") +} diff --git a/cmd/hyperfleet-api/server/server.go b/cmd/hyperfleet-api/server/server.go index e3928c77..d40dd2ec 100755 --- a/cmd/hyperfleet-api/server/server.go +++ b/cmd/hyperfleet-api/server/server.go @@ -7,6 +7,7 @@ import ( "os" "strings" + "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" ) @@ -23,6 +24,15 @@ type ListenNotifier interface { NotifyListening() <-chan struct{} } +// 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 removeTrailingSlash(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { r.URL.Path = strings.TrimSuffix(r.URL.Path, "/") diff --git a/pkg/auth/jwt_handler.go b/pkg/auth/jwt_handler.go index 8550cd1a..a18a7667 100644 --- a/pkg/auth/jwt_handler.go +++ b/pkg/auth/jwt_handler.go @@ -9,7 +9,6 @@ import ( "fmt" "net/http" "os" - "regexp" "strings" "time" @@ -30,9 +29,7 @@ const ( ) type JWTHandlerConfig struct { - Next http.Handler - Issuers []config.JWTIssuerConfig - PublicPaths []string + Issuers []config.JWTIssuerConfig } // issuerValidator holds the pre-built keyfunc and parser for a single JWT issuer. @@ -84,31 +81,17 @@ func NewJWTHandler(ctx context.Context, cfg JWTHandlerConfig) (*JWTHandler, erro }) } - publicPatterns := make([]*regexp.Regexp, 0, len(cfg.PublicPaths)) - for _, p := range cfg.PublicPaths { - re, err := regexp.Compile(p) - if err != nil { - cancel() - return nil, fmt.Errorf("invalid public path pattern %q: %w", p, err) - } - publicPatterns = append(publicPatterns, re) - } - return &JWTHandler{ - validators: validators, - publicPatterns: publicPatterns, - next: cfg.Next, - cancel: cancel, + validators: validators, + cancel: cancel, }, nil } // JWTHandler validates JWT tokens on incoming requests. Call Close() during // shutdown to stop the background JWKS refresh goroutine. type JWTHandler struct { - validators []issuerValidator - next http.Handler - cancel context.CancelFunc - publicPatterns []*regexp.Regexp + cancel context.CancelFunc + validators []issuerValidator } func (h *JWTHandler) Close() { @@ -117,15 +100,12 @@ func (h *JWTHandler) Close() { } } -func (h *JWTHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { - for _, re := range h.publicPatterns { - if re.MatchString(r.URL.Path) { - h.next.ServeHTTP(w, r) - return - } - } - - // Try each issuer's validator: check its header, extract Bearer token, validate +// matchValidator tries each configured issuer validator against the request headers. +// It returns the parsed token and matching issuer config on success. On failure it +// returns a nil token, whether a non-Bearer scheme was seen, and the most relevant +// error (expired tokens take precedence over other parse errors). A nil error with +// a nil token means no configured issuer header was present at all. +func (h *JWTHandler) matchValidator(r *http.Request) (*jwt.Token, config.JWTIssuerConfig, bool, error) { var lastErr error sawNonBearer := false for _, v := range h.validators { @@ -148,45 +128,59 @@ func (h *JWTHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { continue } - ctx := SetJWTTokenContext(r.Context(), token) - ctx = SetJWTIssuerConfigContext(ctx, v.issuerCfg) - h.next.ServeHTTP(w, r.WithContext(ctx)) - return + return token, v.issuerCfg, false, nil } + return nil, config.JWTIssuerConfig{}, sawNonBearer, lastErr +} + +// Middleware returns a standard HTTP middleware that validates JWT tokens. +// Requests with a valid token proceed to next with claims in context; +// requests without valid credentials receive a 401 response. +func (h *JWTHandler) Middleware(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + token, issuerCfg, sawNonBearer, lastErr := h.matchValidator(r) + if token != nil { + ctx := SetJWTTokenContext(r.Context(), token) + ctx = SetJWTIssuerConfigContext(ctx, issuerCfg) + next.ServeHTTP(w, r.WithContext(ctx)) + return + } - // No validator matched — return the most appropriate error - if lastErr != nil { - logger.WithError(r.Context(), lastErr).Warn("JWT validation failed") - if errors.Is(lastErr, jwt.ErrTokenExpired) { - handleError(r.Context(), w, r, hferrors.CodeAuthExpiredToken, "JWT token has expired") - } else { - handleError(r.Context(), w, r, hferrors.CodeAuthInvalidCredentials, "invalid JWT token") + // No validator matched - return the most appropriate error + if lastErr != nil { + logger.WithError(r.Context(), lastErr).Warn("JWT validation failed") + if errors.Is(lastErr, jwt.ErrTokenExpired) { + handleError(r.Context(), w, r, hferrors.CodeAuthExpiredToken, "JWT token has expired") + } else { + handleError(r.Context(), w, r, hferrors.CodeAuthInvalidCredentials, "invalid JWT token") + } + return } - return - } - if sawNonBearer { - handleError(r.Context(), w, r, hferrors.CodeAuthInvalidCredentials, "authorization header does not use Bearer scheme") - return - } + if sawNonBearer { + handleError(r.Context(), w, r, + hferrors.CodeAuthInvalidCredentials, "authorization header does not use Bearer scheme") + return + } - handleError(r.Context(), w, r, hferrors.CodeAuthNoCredentials, "missing authorization header") + handleError(r.Context(), w, r, hferrors.CodeAuthNoCredentials, "missing authorization header") + }) } func buildKeyfunc(ctx context.Context, issuer config.JWTIssuerConfig) (keyfunc.Keyfunc, error) { hasFile := issuer.JWKCertFile != "" hasURL := issuer.JWKCertURL != "" - switch { - case !hasFile && !hasURL: + if !hasFile && !hasURL { return nil, fmt.Errorf("at least one of jwk_cert_file or jwk_cert_url must be provided") - case hasFile && !hasURL: + } + if hasFile && !hasURL { return buildFileOnlyKeyfunc(issuer) - case !hasFile && hasURL: + } + if !hasFile { return buildURLOnlyKeyfunc(ctx, issuer) - default: - return buildCombinedKeyfunc(ctx, issuer) } + return buildCombinedKeyfunc(ctx, issuer) } func buildFileOnlyKeyfunc(issuer config.JWTIssuerConfig) (keyfunc.Keyfunc, error) { diff --git a/pkg/auth/jwt_handler_test.go b/pkg/auth/jwt_handler_test.go index 7eb2efc1..1a8ef576 100644 --- a/pkg/auth/jwt_handler_test.go +++ b/pkg/auth/jwt_handler_test.go @@ -41,10 +41,9 @@ func TestJWTHandler(t *testing.T) { IssuerURL: "https://test-issuer.example.com", JWKCertURL: jwksServer.URL, }}, - PublicPaths: []string{"^/healthz$", "^/openapi$"}, - Next: nextHandler, }) Expect(err).NotTo(HaveOccurred()) + mw := handler.Middleware(nextHandler) t.Run("valid token passes through", func(t *testing.T) { RegisterTestingT(t) @@ -53,7 +52,7 @@ func TestJWTHandler(t *testing.T) { "exp": time.Now().Add(time.Hour).Unix(), "iat": time.Now().Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusOK)) Expect(rr.Body.String()).To(Equal("ok")) }) @@ -73,7 +72,6 @@ func TestJWTHandler(t *testing.T) { IssuerURL: "https://test-issuer.example.com", JWKCertURL: jwksServer.URL, }}, - Next: claimsHandler, }) Expect(err).NotTo(HaveOccurred()) @@ -83,7 +81,7 @@ func TestJWTHandler(t *testing.T) { "iat": time.Now().Unix(), "username": "testuser", }) - rr := serve(h, "/protected", "Bearer "+token) + rr := serve(h.Middleware(claimsHandler), "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusOK)) }) @@ -94,7 +92,7 @@ func TestJWTHandler(t *testing.T) { "exp": time.Now().Add(-time.Hour).Unix(), "iat": time.Now().Add(-2 * time.Hour).Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) }) @@ -107,7 +105,7 @@ func TestJWTHandler(t *testing.T) { "exp": time.Now().Add(time.Hour).Unix(), "iat": time.Now().Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) }) @@ -118,13 +116,13 @@ func TestJWTHandler(t *testing.T) { "exp": time.Now().Add(time.Hour).Unix(), "iat": time.Now().Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) }) t.Run("missing Authorization header returns 401", func(t *testing.T) { RegisterTestingT(t) - rr := serve(handler, "/protected", "") + rr := serve(mw, "/protected", "") Expect(rr.Code).To(Equal(http.StatusUnauthorized)) }) @@ -135,36 +133,22 @@ func TestJWTHandler(t *testing.T) { "exp": time.Now().Add(time.Hour).Unix(), "iat": time.Now().Unix(), }) - rr := serve(handler, "/protected", "bearer "+token) + rr := serve(mw, "/protected", "bearer "+token) Expect(rr.Code).To(Equal(http.StatusOK)) }) t.Run("malformed Authorization header returns 401", func(t *testing.T) { RegisterTestingT(t) - rr := serve(handler, "/protected", "Basic dXNlcjpwYXNz") + rr := serve(mw, "/protected", "Basic dXNlcjpwYXNz") Expect(rr.Code).To(Equal(http.StatusUnauthorized)) }) t.Run("garbage token returns 401", func(t *testing.T) { RegisterTestingT(t) - rr := serve(handler, "/protected", "Bearer not.a.jwt") + rr := serve(mw, "/protected", "Bearer not.a.jwt") Expect(rr.Code).To(Equal(http.StatusUnauthorized)) }) - t.Run("public endpoint without token passes through", func(t *testing.T) { - RegisterTestingT(t) - rr := serve(handler, "/healthz", "") - Expect(rr.Code).To(Equal(http.StatusOK)) - Expect(rr.Body.String()).To(Equal("ok")) - }) - - t.Run("public endpoint with invalid token still passes through", func(t *testing.T) { - RegisterTestingT(t) - rr := serve(handler, "/healthz", "Bearer garbage") - Expect(rr.Code).To(Equal(http.StatusOK)) - Expect(rr.Body.String()).To(Equal("ok")) - }) - t.Run("HS256 signed token rejected", func(t *testing.T) { RegisterTestingT(t) claims := jwt.MapClaims{ @@ -175,7 +159,7 @@ func TestJWTHandler(t *testing.T) { tok := jwt.NewWithClaims(jwt.SigningMethodHS256, claims) tokenString, err := tok.SignedString([]byte("secret-key-for-hmac")) Expect(err).NotTo(HaveOccurred()) - rr := serve(handler, "/protected", "Bearer "+tokenString) + rr := serve(mw, "/protected", "Bearer "+tokenString) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) }) } @@ -196,26 +180,25 @@ func TestJWTHandler_FailClosed_NoValidKeys(t *testing.T) { IssuerURL: "https://test-issuer.example.com", JWKCertURL: badServer.URL, }}, - Next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }), }) Expect(err).NotTo(HaveOccurred()) + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + token := signToken(t, privateKey, jwt.MapClaims{ "iss": "https://test-issuer.example.com", "exp": time.Now().Add(time.Hour).Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(handler.Middleware(next), "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) } func TestJWTHandler_RequiresIssuersConfig(t *testing.T) { RegisterTestingT(t) - _, err := NewJWTHandler(t.Context(), JWTHandlerConfig{ - Next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}), - }) + _, err := NewJWTHandler(t.Context(), JWTHandlerConfig{}) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("at least one issuer")) } @@ -235,12 +218,14 @@ func TestJWTHandler_WithAudience(t *testing.T) { JWKCertURL: jwksServer.URL, Audience: "my-api", }}, - Next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }), }) Expect(err).NotTo(HaveOccurred()) + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + mw := handler.Middleware(next) + t.Run("correct audience passes", func(t *testing.T) { RegisterTestingT(t) token := signToken(t, privateKey, jwt.MapClaims{ @@ -248,7 +233,7 @@ func TestJWTHandler_WithAudience(t *testing.T) { "aud": "my-api", "exp": time.Now().Add(time.Hour).Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusOK)) }) @@ -259,7 +244,7 @@ func TestJWTHandler_WithAudience(t *testing.T) { "aud": "wrong-api", "exp": time.Now().Add(time.Hour).Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) }) } @@ -278,18 +263,19 @@ func TestJWTHandler_WithoutAudience_AcceptsAny(t *testing.T) { IssuerURL: "https://test-issuer.example.com", JWKCertURL: jwksServer.URL, }}, - Next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }), }) Expect(err).NotTo(HaveOccurred()) + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + token := signToken(t, privateKey, jwt.MapClaims{ "iss": "https://test-issuer.example.com", "aud": "any-audience", "exp": time.Now().Add(time.Hour).Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(handler.Middleware(next), "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusOK)) } @@ -306,13 +292,15 @@ func TestJWTHandler_FileOnlyKeyfunc(t *testing.T) { IssuerURL: "https://test-issuer.example.com", JWKCertFile: jwksFile, }}, - Next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - fmt.Fprint(w, "ok") - }), }) Expect(err).NotTo(HaveOccurred()) + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + fmt.Fprint(w, "ok") + }) + mw := handler.Middleware(next) + t.Run("valid token accepted via file keys", func(t *testing.T) { RegisterTestingT(t) token := signToken(t, privateKey, jwt.MapClaims{ @@ -320,7 +308,7 @@ func TestJWTHandler_FileOnlyKeyfunc(t *testing.T) { "exp": time.Now().Add(time.Hour).Unix(), "iat": time.Now().Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusOK)) Expect(rr.Body.String()).To(Equal("ok")) }) @@ -334,7 +322,7 @@ func TestJWTHandler_FileOnlyKeyfunc(t *testing.T) { "exp": time.Now().Add(time.Hour).Unix(), "iat": time.Now().Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) }) } @@ -355,12 +343,14 @@ func TestJWTHandler_CombinedKeyfunc(t *testing.T) { JWKCertFile: jwksFile, JWKCertURL: jwksServer.URL, }}, - Next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }), }) Expect(err).NotTo(HaveOccurred()) + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + mw := handler.Middleware(next) + t.Run("constructor succeeds with both file and URL", func(t *testing.T) { RegisterTestingT(t) Expect(handler).NotTo(BeNil()) @@ -372,7 +362,7 @@ func TestJWTHandler_CombinedKeyfunc(t *testing.T) { "iss": "https://test-issuer.example.com", "exp": time.Now().Add(time.Hour).Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusOK)) }) @@ -384,7 +374,7 @@ func TestJWTHandler_CombinedKeyfunc(t *testing.T) { "iss": "https://test-issuer.example.com", "exp": time.Now().Add(time.Hour).Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) }) } @@ -409,13 +399,15 @@ func TestJWTHandler_TLSWithCAFile(t *testing.T) { JWKCertURL: tlsServer.URL, JWKCertCAFile: caFile, }}, - Next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }), }) Expect(err).NotTo(HaveOccurred()) defer handler.Close() + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + mw := handler.Middleware(next) + cases := []struct { signingKey *rsa.PrivateKey name string @@ -432,7 +424,7 @@ func TestJWTHandler_TLSWithCAFile(t *testing.T) { "exp": time.Now().Add(time.Hour).Unix(), "iat": time.Now().Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(tc.wantStatus)) }) } @@ -453,18 +445,20 @@ func TestJWTHandler_CombinedKeyfuncWithCA(t *testing.T) { JWKCertFile: jwksFile, JWKCertCAFile: caFile, }}, - Next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }), }) Expect(err).NotTo(HaveOccurred()) defer handler.Close() + + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + token := signToken(t, privateKey, jwt.MapClaims{ "iss": "https://test-issuer.example.com", "exp": time.Now().Add(time.Hour).Unix(), "iat": time.Now().Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(handler.Middleware(next), "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusOK)) } @@ -482,18 +476,19 @@ func TestJWTHandler_TLSWithoutCAFile_Fails(t *testing.T) { IssuerURL: "https://test-issuer.example.com", JWKCertURL: tlsServer.URL, }}, - Next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusOK) - }), }) Expect(err).NotTo(HaveOccurred()) defer handler.Close() + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + token := signToken(t, privateKey, jwt.MapClaims{ "iss": "https://test-issuer.example.com", "exp": time.Now().Add(time.Hour).Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(handler.Middleware(next), "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) } @@ -529,7 +524,6 @@ func TestJWTHandler_CAFileErrors(t *testing.T) { JWKCertURL: "https://keys.example.com", JWKCertCAFile: tc.caFile(t), }}, - Next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}), }) Expect(err).To(HaveOccurred()) Expect(err.Error()).To(ContainSubstring(tc.wantErrSubstr)) @@ -551,7 +545,6 @@ func TestJWTHandler_Close(t *testing.T) { IssuerURL: "https://test-issuer.example.com", JWKCertURL: jwksServer.URL, }}, - Next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}), }) Expect(err).NotTo(HaveOccurred()) @@ -573,13 +566,17 @@ func TestJWTHandler_ResponseBody(t *testing.T) { IssuerURL: "https://test-issuer.example.com", JWKCertURL: jwksServer.URL, }}, - Next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }), }) Expect(err).NotTo(HaveOccurred()) + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + mw := handler.Middleware(next) + t.Run("missing header returns problem+json with no-credentials code", func(t *testing.T) { RegisterTestingT(t) - rr := serve(handler, "/protected", "") + rr := serve(mw, "/protected", "") Expect(rr.Code).To(Equal(http.StatusUnauthorized)) Expect(rr.Header().Get("Content-Type")).To(ContainSubstring("application/problem+json")) @@ -596,7 +593,7 @@ func TestJWTHandler_ResponseBody(t *testing.T) { "exp": time.Now().Add(-time.Hour).Unix(), "iat": time.Now().Add(-2 * time.Hour).Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) Expect(rr.Header().Get("Content-Type")).To(ContainSubstring("application/problem+json")) @@ -608,7 +605,7 @@ func TestJWTHandler_ResponseBody(t *testing.T) { t.Run("invalid token returns problem+json with invalid-credentials code", func(t *testing.T) { RegisterTestingT(t) - rr := serve(handler, "/protected", "Bearer not.a.jwt") + rr := serve(mw, "/protected", "Bearer not.a.jwt") Expect(rr.Code).To(Equal(http.StatusUnauthorized)) Expect(rr.Header().Get("Content-Type")).To(ContainSubstring("application/problem+json")) @@ -620,7 +617,7 @@ func TestJWTHandler_ResponseBody(t *testing.T) { t.Run("non-Bearer scheme returns problem+json with invalid-credentials code", func(t *testing.T) { RegisterTestingT(t) - rr := serve(handler, "/protected", "Basic dXNlcjpwYXNz") + rr := serve(mw, "/protected", "Basic dXNlcjpwYXNz") Expect(rr.Code).To(Equal(http.StatusUnauthorized)) Expect(rr.Header().Get("Content-Type")).To(ContainSubstring("application/problem+json")) @@ -657,9 +654,9 @@ func TestJWTHandler_MultiIssuer(t *testing.T) { {IssuerURL: "https://issuer-1.example.com", JWKCertURL: jwksServer1.URL}, {IssuerURL: "https://issuer-2.example.com", JWKCertURL: jwksServer2.URL}, }, - Next: nextHandler, }) Expect(err).NotTo(HaveOccurred()) + mw := handler.Middleware(nextHandler) validIssuerCases := []struct { name string @@ -676,7 +673,7 @@ func TestJWTHandler_MultiIssuer(t *testing.T) { "iss": tc.issuerURL, "exp": time.Now().Add(time.Hour).Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusOK)) Expect(rr.Header().Get("X-Matched-Issuer")).To(Equal(tc.issuerURL)) }) @@ -688,7 +685,7 @@ func TestJWTHandler_MultiIssuer(t *testing.T) { "iss": "https://unknown-issuer.example.com", "exp": time.Now().Add(time.Hour).Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) }) @@ -700,7 +697,7 @@ func TestJWTHandler_MultiIssuer(t *testing.T) { "iss": "https://issuer-1.example.com", "exp": time.Now().Add(time.Hour).Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) }) } @@ -728,9 +725,9 @@ func TestJWTHandler_CustomHeader(t *testing.T) { JWKCertURL: jwksServer.URL, Header: "X-Custom-Auth", }}, - Next: nextHandler, }) Expect(err).NotTo(HaveOccurred()) + mw := handler.Middleware(nextHandler) t.Run("valid token on custom header passes", func(t *testing.T) { RegisterTestingT(t) @@ -739,7 +736,7 @@ func TestJWTHandler_CustomHeader(t *testing.T) { "exp": time.Now().Add(time.Hour).Unix(), "iat": time.Now().Unix(), }) - rr := serveWithHeader(handler, "/protected", "X-Custom-Auth", "Bearer "+token) + rr := serveWithHeader(mw, "/protected", "X-Custom-Auth", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusOK)) Expect(rr.Header().Get("X-Matched-Issuer")).To(Equal("https://test-issuer.example.com")) }) @@ -751,7 +748,7 @@ func TestJWTHandler_CustomHeader(t *testing.T) { "exp": time.Now().Add(time.Hour).Unix(), "iat": time.Now().Unix(), }) - rr := serve(handler, "/protected", "Bearer "+token) + rr := serve(mw, "/protected", "Bearer "+token) Expect(rr.Code).To(Equal(http.StatusUnauthorized)) }) } diff --git a/pkg/config/server.go b/pkg/config/server.go index 805fd33b..8ce8c046 100755 --- a/pkg/config/server.go +++ b/pkg/config/server.go @@ -205,3 +205,23 @@ func NewServerConfig() *ServerConfig { func (s *ServerConfig) BindAddress() string { return net.JoinHostPort(s.Host, strconv.Itoa(s.Port)) } + +func (s *ServerConfig) ReadTimeout() time.Duration { + return s.Timeouts.Read +} + +func (s *ServerConfig) WriteTimeout() time.Duration { + return s.Timeouts.Write +} + +func (s *ServerConfig) TLSEnabled() bool { + return s.TLS.Enabled +} + +func (s *ServerConfig) TLSCertFile() string { + return s.TLS.CertFile +} + +func (s *ServerConfig) TLSKeyFile() string { + return s.TLS.KeyFile +} diff --git a/pkg/db/sql_helpers.go b/pkg/db/sql_helpers.go index 09578d3b..9d7d644a 100755 --- a/pkg/db/sql_helpers.go +++ b/pkg/db/sql_helpers.go @@ -35,7 +35,7 @@ func validateJSONBKey(key, fieldType string) *errors.ServiceError { // getField gets the sql field associated with a name. func getField(name string) (field string, err *errors.ServiceError) { - trimmedName := strings.Trim(name, " ") + trimmedName := strings.TrimSpace(name) // Map user-friendly spec.xxx (and nested spec.xxx.yyy...) syntax to JSONB query. // v6 gives us the full dotted path directly: diff --git a/pkg/handlers/resource_handler.go b/pkg/handlers/resource_handler.go index ae0bf81b..09c673c7 100644 --- a/pkg/handlers/resource_handler.go +++ b/pkg/handlers/resource_handler.go @@ -14,7 +14,7 @@ import ( // ResourceHandler serves both flat and owner-nested routes for a single entity // kind. Every method branches on whether "parent_id" is present via // r.PathValue("parent_id") rather than dispatching statically per route. This is only correct because -// plugins/entities/plugin.go guarantees the invariant: a nested (ParentKind != "") +// cmd/hyperfleet-api/server/routes_entities.go guarantees the invariant: a nested (ParentKind != "") // descriptor is registered exclusively under a {parent_id} subrouter, and a flat // descriptor never is. If that registration is ever bypassed — e.g. a nested kind // wired to a flat route — these branches take the wrong path silently (Create diff --git a/plugins/CLAUDE.md b/plugins/CLAUDE.md deleted file mode 100644 index b2ef85e1..00000000 --- a/plugins/CLAUDE.md +++ /dev/null @@ -1,33 +0,0 @@ -# Claude Code Guidelines for Plugins - -## Config-Driven Entity Registration - -All entity types (Cluster, NodePool, Channel, Version, WifConfig) are declared in `config.yaml` -under `entities:` and registered at startup via `registry.LoadDescriptors()`. Routes are -auto-generated by `plugins/entities/plugin.go`. No per-entity plugin, DAO, service, or handler -code is needed. - -To add a new entity type: -1. Add an entry to the `entities:` section in `config.yaml` (and `charts/values.yaml` for Helm) -2. Add the spec schema to the provider OpenAPI spec (`hyperfleet-api-spec`) -3. Redeploy — routes, spec validation, and delete policies are generated automatically - -Reference: `plugins/entities/plugin.go`, `pkg/registry/load.go` - -## Entity Descriptor Fields - -Each entity entry in `config.yaml` supports: -- `kind` — discriminator value (e.g. "Cluster", "NodePool", "Channel") -- `plural` — URL path segment (e.g. "clusters", "nodepools") -- `parent_kind` — parent entity kind for nested resources (e.g. NodePool's parent is Cluster) -- `on_parent_delete` — `restrict` or `cascade` -- `spec_schema_name` — OpenAPI component name for spec validation -- `required_adapters` — adapters that must finalize before hard-delete -- `name_min_len` / `name_max_len` — name length constraints -- `require_spec_schema` — fail startup if spec schema is missing - -## Related CLAUDE.md Files - -- `pkg/handlers/CLAUDE.md` — Handler pipeline -- `pkg/services/CLAUDE.md` — Service patterns -- `pkg/dao/CLAUDE.md` — DAO patterns diff --git a/plugins/adapterStatus/plugin.go b/plugins/adapterStatus/plugin.go deleted file mode 100644 index dd700d92..00000000 --- a/plugins/adapterStatus/plugin.go +++ /dev/null @@ -1,38 +0,0 @@ -package adapterStatus - -import ( - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments" - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments/registry" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" -) - -// ServiceLocator Service Locator -type ServiceLocator func() services.AdapterStatusService - -func NewServiceLocator(env *environments.Env) ServiceLocator { - return func() services.AdapterStatusService { - return services.NewAdapterStatusService( - dao.NewAdapterStatusDao(env.Database.SessionFactory), - ) - } -} - -// Service helper function to get the adapter status service from the registry -func Service(s *environments.Services) services.AdapterStatusService { - if s == nil { - return nil - } - if obj := s.GetService("AdapterStatus"); obj != nil { - locator := obj.(ServiceLocator) - return locator() - } - return nil -} - -func init() { - // Service registration - registry.RegisterService("AdapterStatus", func(env interface{}) interface{} { - return NewServiceLocator(env.(*environments.Env)) - }) -} diff --git a/plugins/generic/plugin.go b/plugins/generic/plugin.go deleted file mode 100755 index d7c7ae6f..00000000 --- a/plugins/generic/plugin.go +++ /dev/null @@ -1,36 +0,0 @@ -package generic - -import ( - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments" - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments/registry" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" -) - -// ServiceLocator Service Locator -type ServiceLocator func() services.GenericService - -func NewServiceLocator(env *environments.Env) ServiceLocator { - return func() services.GenericService { - return services.NewGenericService(dao.NewGenericDao(env.Database.SessionFactory)) - } -} - -// Service helper function to get the generic service from the registry -func Service(s *environments.Services) services.GenericService { - if s == nil { - return nil - } - if obj := s.GetService("Generic"); obj != nil { - locator := obj.(ServiceLocator) - return locator() - } - return nil -} - -func init() { - // Service registration - registry.RegisterService("Generic", func(env interface{}) interface{} { - return NewServiceLocator(env.(*environments.Env)) - }) -} diff --git a/plugins/resources/plugin.go b/plugins/resources/plugin.go deleted file mode 100644 index 1e159883..00000000 --- a/plugins/resources/plugin.go +++ /dev/null @@ -1,42 +0,0 @@ -package resources - -import ( - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments" - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments/registry" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" - "github.com/openshift-hyperfleet/hyperfleet-api/plugins/generic" -) - -type ServiceLocator func() services.ResourceService - -// NewServiceLocator creates a ServiceLocator that builds a ResourceService from the environment. -func NewServiceLocator(env *environments.Env) ServiceLocator { - return func() services.ResourceService { - return services.NewResourceService( - dao.NewResourceDao(env.Database.SessionFactory), - dao.NewResourceLabelDao(env.Database.SessionFactory), - dao.NewAdapterStatusDao(env.Database.SessionFactory), - dao.NewResourceConditionDao(env.Database.SessionFactory), - generic.Service(&env.Services), - ) - } -} - -// Service retrieves the ResourceService from the service registry. -func Service(s *environments.Services) services.ResourceService { - if s == nil { - return nil - } - if obj := s.GetService("Resources"); obj != nil { - locator := obj.(ServiceLocator) - return locator() - } - return nil -} - -func init() { - registry.RegisterService("Resources", func(env interface{}) interface{} { - return NewServiceLocator(env.(*environments.Env)) - }) -} diff --git a/test/factories/clusters.go b/test/factories/clusters.go index 76a30b08..34e66af5 100644 --- a/test/factories/clusters.go +++ b/test/factories/clusters.go @@ -8,19 +8,11 @@ import ( "gorm.io/gorm" - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/dao" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" - "github.com/openshift-hyperfleet/hyperfleet-api/plugins/resources" ) -// resourceService retrieves the ResourceService for creating resources. -func resourceService() services.ResourceService { - return resources.Service(&environments.Environment().Services) -} - // reloadResource reloads a resource from the database to ensure all fields are current. func reloadResource(dbSession *gorm.DB, resource *api.Resource) error { return dbSession.Preload("Conditions").Preload("Labels").First(resource, "id = ?", resource.ID).Error @@ -70,7 +62,6 @@ func buildConditionsWithGeneration( } func (f *Factories) NewCluster(id string) (*api.Resource, error) { - svc := resourceService() ctx := context.Background() cluster := &api.Resource{ @@ -81,7 +72,7 @@ func (f *Factories) NewCluster(id string) (*api.Resource, error) { UpdatedBy: "test@example.com", } - result, svcErr := svc.Create(ctx, "Cluster", cluster, nil) + result, svcErr := f.resourceService.Create(ctx, "Cluster", cluster, nil) if svcErr != nil { return nil, fmt.Errorf("create cluster: %w", svcErr) } diff --git a/test/factories/factory.go b/test/factories/factory.go index fbb338d4..2b645fcf 100755 --- a/test/factories/factory.go +++ b/test/factories/factory.go @@ -4,9 +4,15 @@ import ( "fmt" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" ) type Factories struct { + resourceService services.ResourceService +} + +func New(resourceService services.ResourceService) Factories { + return Factories{resourceService: resourceService} } // NewID generates a new unique identifier using RFC4122 UUID v7. diff --git a/test/factories/node_pools.go b/test/factories/node_pools.go index c91aaf3e..2633eea1 100644 --- a/test/factories/node_pools.go +++ b/test/factories/node_pools.go @@ -11,7 +11,7 @@ import ( ) func (f *Factories) NewNodePool(id string) (*api.Resource, error) { - svc := resourceService() + svc := f.resourceService ctx := context.Background() // Create parent cluster @@ -46,7 +46,7 @@ func (f *Factories) NewNodePool(id string) (*api.Resource, error) { } func (f *Factories) NewNodePoolList(name string, count int) ([]*api.Resource, error) { - svc := resourceService() + svc := f.resourceService ctx := context.Background() // Create shared parent cluster diff --git a/test/helper.go b/test/helper.go index 9de6a6a5..b762153e 100755 --- a/test/helper.go +++ b/test/helper.go @@ -21,17 +21,18 @@ import ( "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/config" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" "github.com/openshift-hyperfleet/hyperfleet-api/test/factories" "github.com/openshift-hyperfleet/hyperfleet-api/test/mocks" - - _ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/entities" ) const ( @@ -61,7 +62,8 @@ type Helper struct { Factories factories.Factories Ctx context.Context DBFactory db.SessionFactory - APIServer server.Server + Container *container.Container + APIServer *server.APIServer MetricsServer server.Server HealthServer server.Server AppConfig *config.ApplicationConfig @@ -147,9 +149,13 @@ func NewHelper(t *testing.T) *Helper { os.Exit(1) } + ctr := container.NewContainer(env.Config, env.Database.SessionFactory) + helper = &Helper{ + Factories: factories.New(ctr.ResourceService()), AppConfig: environments.Environment().Config, DBFactory: environments.Environment().Database.SessionFactory, + Container: ctr, JWTPrivateKey: jwtKey, JWTCA: jwtCA, } @@ -164,6 +170,7 @@ func NewHelper(t *testing.T) *Helper { helper.teardowns = []func() error{ helper.teardownEnv, helper.stopAPIServer, + helper.closeContainer, helper.stopMetricsServer, helper.stopHealthServer, jwkMockTeardown, @@ -211,8 +218,33 @@ func (helper *Helper) startAPIServer() { cfg.IdentityHeader = defaultTestIdentityHeader } } + cfg := helper.Env().Config + + // 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.T.Fatalf("unable to create JWT handler: %s", jwtErr) + } + } + // Disable tracing for integration tests (no OTLP collector required) - helper.APIServer = server.NewAPIServer(false) + apiServer, err := servecmd.BuildAPIServer( + cfg, + helper.Container.ResourceService(), + helper.Container.AdapterStatusService(), + helper.Container.SchemaValidator(), + jwtHandler, + helper.Container.SessionFactory(), + false, + ) + if err != nil { + logger.WithError(ctx, err).Error("Unable to build Test API server") + os.Exit(1) + } + helper.APIServer = apiServer listener, err := helper.APIServer.Listen() if err != nil { logger.WithError(ctx, err).Error("Unable to start Test API server") @@ -231,6 +263,10 @@ func (helper *Helper) stopAPIServer() error { } return nil } +func (helper *Helper) closeContainer() error { + helper.Container.Close() + return nil +} func (helper *Helper) startMetricsServer() { ctx := context.Background() @@ -266,15 +302,6 @@ func (helper *Helper) startHealthServer() { }() } -func (helper *Helper) RestartServer() { - ctx := context.Background() - if err := helper.stopAPIServer(); err != nil { - logger.WithError(ctx, err).Warn("unable to stop api server on restart") - } - helper.startAPIServer() - logger.Debug(ctx, "Test API server restarted") -} - func (helper *Helper) RestartMetricsServer() { ctx := context.Background() if err := helper.stopMetricsServer(); err != nil { @@ -284,32 +311,6 @@ func (helper *Helper) RestartMetricsServer() { logger.Debug(ctx, "Test metrics server restarted") } -func (helper *Helper) Reset() { - ctx := context.Background() - logger.Info(ctx, "Resetting testing environment") - env := environments.Environment() - // Reset the configuration - env.Config = config.NewApplicationConfig() - - // Re-read command-line configuration into a NEW flagset - // This new flag set ensures we don't hit conflicts defining the same flag twice - // Also on reset, we don't care to be re-defining 'v' and other glog flags - flagset := pflag.NewFlagSet(helper.NewID(), pflag.ContinueOnError) - if err := env.SetEnvironmentDefaults(flagset); err != nil { - logger.WithError(ctx, err).Error("Unable to set environment defaults on Reset") - os.Exit(1) - } - pflag.Parse() - - err := env.Initialize() - if err != nil { - logger.WithError(ctx, err).Error("Unable to reset testing environment") - os.Exit(1) - } - helper.AppConfig = env.Config - helper.RestartServer() -} - // NewID creates a new unique ID used internally func (helper *Helper) NewID() string { id, err := uuid.NewV7() @@ -703,18 +704,13 @@ func (helper *Helper) OpenapiError(body []byte) openapi.ProblemDetails { } func parseJWTKeys() (*rsa.PrivateKey, *rsa.PublicKey, error) { - // projectRootDir := getProjectRootDir() - // privateBytes, err := os.ReadFile(filepath.Join(projectRootDir, jwtKeyFile)) privateBytes, err := privatebytes() if err != nil { - err = fmt.Errorf("unable to read JWT key file %s: %s", jwtKeyFile, err) - return nil, nil, err + return nil, nil, fmt.Errorf("unable to read JWT key file %s: %w", jwtKeyFile, err) } - // pubBytes, err := ioutil.ReadFile(filepath.Join(projectRootDir, jwtCAFile)) pubBytes, err := publicbytes() if err != nil { - err = fmt.Errorf("unable to read JWT ca file %s: %s", jwtKeyFile, err) - return nil, nil, err + return nil, nil, fmt.Errorf("unable to read JWT ca file %s: %w", jwtCAFile, err) } // Parse keys diff --git a/test/integration/resource_helpers.go b/test/integration/resource_helpers.go index abb239ef..36bb22b1 100644 --- a/test/integration/resource_helpers.go +++ b/test/integration/resource_helpers.go @@ -5,16 +5,14 @@ import ( "fmt" "testing" - "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/services" - "github.com/openshift-hyperfleet/hyperfleet-api/plugins/resources" "github.com/openshift-hyperfleet/hyperfleet-api/test" ) func setupResourceTest(t *testing.T) (services.ResourceService, *test.Helper) { h, _ := test.RegisterIntegration(t) - svc := resources.Service(&environments.Environment().Services) + svc := h.Container.ResourceService() return svc, h }