From ee9074d9938eeccb6e3e37b5a646bbcf22f6bf0c Mon Sep 17 00:00:00 2001 From: sherine-k Date: Tue, 2 Jun 2026 14:14:56 +0200 Subject: [PATCH 1/2] HYPERFLEET-1090 - feat: WIFConfig plugin registration with schema validation Register WIFConfig as a generic resource plugin using the new ResourceHandler pattern, with spec validation driven by the entity registry rather than hardcoded resource types. - Add WIFConfig plugin with CRUD routes and registry descriptor - Refactor SchemaValidationMiddleware to discover entities from registry.WithSpecSchema() - Refactor SchemaValidator to dynamically load schemas for all registered entities - Add rightmost-match path logic so nested routes (e.g. /clusters/{id}/nodepools) use the child schema - Add registry.WithSpecSchema() and registry.Reset() helpers - Add permissive test/validation-schema.yaml for integration tests - Add WIFConfig integration tests covering CRUD, schema validation, and error details - Add schema_validation_match_test.go for nested path disambiguation - Extend schema_validator_test.go with registry-driven validation coverage --- cmd/hyperfleet-api/main.go | 1 + pkg/middleware/schema_validation.go | 86 +++-- .../schema_validation_match_test.go | 113 ++++++ pkg/middleware/schema_validation_test.go | 152 ++++++++ pkg/registry/registry.go | 11 + pkg/registry/registry_test.go | 27 ++ pkg/validators/schema_validator.go | 120 +++++-- pkg/validators/schema_validator_test.go | 304 +++++++++++++++- plugins/wifconfigs/plugin.go | 44 +++ test/CLAUDE.md | 4 +- test/integration/clusters_test.go | 10 +- test/integration/integration_test.go | 5 +- test/integration/wifconfigs_test.go | 339 ++++++++++++++++++ test/validation-schema.yaml | 22 ++ 14 files changed, 1177 insertions(+), 61 deletions(-) create mode 100644 pkg/middleware/schema_validation_match_test.go create mode 100644 plugins/wifconfigs/plugin.go create mode 100644 test/integration/wifconfigs_test.go create mode 100644 test/validation-schema.yaml diff --git a/cmd/hyperfleet-api/main.go b/cmd/hyperfleet-api/main.go index 0ecd7f68..1b342d99 100755 --- a/cmd/hyperfleet-api/main.go +++ b/cmd/hyperfleet-api/main.go @@ -23,6 +23,7 @@ import ( _ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/nodePools" _ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/resources" _ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/versions" + _ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/wifconfigs" ) // nolint diff --git a/pkg/middleware/schema_validation.go b/pkg/middleware/schema_validation.go index 09d98d03..dca7a293 100644 --- a/pkg/middleware/schema_validation.go +++ b/pkg/middleware/schema_validation.go @@ -9,6 +9,7 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/validators" ) @@ -36,12 +37,30 @@ func handleValidationError(w http.ResponseWriter, r *http.Request, err *errors.S } } -// SchemaValidationMiddleware validates cluster and nodepool spec fields against OpenAPI schemas +// SchemaValidationMiddleware validates resource spec fields against OpenAPI schemas for every +// registered entity that declares SpecSchemaName. func SchemaValidationMiddleware(validator *validators.SchemaValidator) func(http.Handler) http.Handler { + + // TODO : HYPERFLEET-1159 - Remove this once Cluster and NodePool are registered + specEntities := []registry.EntityDescriptor{ + { + Kind: "Cluster", + Plural: "clusters", + SpecSchemaName: "ClusterSpec", + }, + { + Kind: "NodePool", + Plural: "nodepools", + ParentKind: "Cluster", + SpecSchemaName: "NodePoolSpec", + }, + } + + specEntities = append(specEntities, registry.WithSpecSchema()...) + return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Check if the request requires spec validation - shouldValidate, resourceType := shouldValidateRequest(r.Method, r.URL.Path) + shouldValidate, resourcePlural := shouldValidateRequest(r.Method, r.URL.Path, specEntities) if !shouldValidate { next.ServeHTTP(w, r) return @@ -85,9 +104,7 @@ func SchemaValidationMiddleware(validator *validators.SchemaValidator) func(http return } - // Validate spec using the resource type - // validator.Validate returns nil for unknown resource types - validationErr := validator.Validate(resourceType, specMap) + validationErr := validator.Validate(resourcePlural, specMap) // If validation failed, return 400 error if validationErr != nil { @@ -108,26 +125,55 @@ func SchemaValidationMiddleware(validator *validators.SchemaValidator) func(http } } -// shouldValidateRequest determines if the request requires spec validation -// Returns (shouldValidate bool, resourceType string) -func shouldValidateRequest(method, path string) (bool, string) { - // Only validate POST and PATCH requests +// shouldValidateRequest determines if the request requires spec validation. +// Returns (shouldValidate bool, resourcePlural string). +// +// When a path contains multiple registered plurals (e.g. /clusters/{id}/nodepools), +// the rightmost matching segment wins so nested resources use their own spec schema. +func shouldValidateRequest( + method, path string, specEntities []registry.EntityDescriptor, +) (bool, string) { if method != http.MethodPost && method != http.MethodPatch { return false, "" } - // Check nodepools first (more specific path) - // POST /api/hyperfleet/v1/clusters/{cluster_id}/nodepools - // PATCH /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id} - if strings.Contains(path, "/nodepools") { - return true, "nodepool" + // this path matching logic is used to determine the correct spec schema to use for validation + // based on the path of the request. + // For example, if the path is /clusters/abc-123/nodepools/np-456, the matched plural will be "nodepools". + var ( + matchedPlural string + matchedIndex = -1 + ) + + for _, d := range specEntities { + segment := "/" + d.Plural + if !pathMatchesSpecEntity(method, path, segment) { + continue + } + + idx := strings.LastIndex(path, segment) + if idx < matchedIndex { + continue + } + if idx > matchedIndex { + matchedIndex = idx + matchedPlural = d.Plural + } } - // POST /api/hyperfleet/v1/clusters - // PATCH /api/hyperfleet/v1/clusters/{cluster_id} - if strings.HasSuffix(path, "/clusters") || strings.Contains(path, "/clusters/") { - return true, "cluster" + if matchedPlural == "" { + return false, "" } + return true, matchedPlural +} - return false, "" +func pathMatchesSpecEntity(method, path, segment string) bool { + switch method { + case http.MethodPost: + return strings.HasSuffix(path, segment) + case http.MethodPatch: + return strings.Contains(path, segment+"/") + default: + return false + } } diff --git a/pkg/middleware/schema_validation_match_test.go b/pkg/middleware/schema_validation_match_test.go new file mode 100644 index 00000000..a27cf1c5 --- /dev/null +++ b/pkg/middleware/schema_validation_match_test.go @@ -0,0 +1,113 @@ +package middleware + +import ( + "net/http" + "testing" + + . "github.com/onsi/gomega" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" +) + +// clusterFirstEntities lists clusters before nodepools to ensure matching does not depend on slice order. +var clusterFirstEntities = []registry.EntityDescriptor{ + { + Kind: "Cluster", + Plural: "clusters", + SpecSchemaName: "ClusterSpec", + }, + { + Kind: "NodePool", + Plural: "nodepools", + ParentKind: "Cluster", + SpecSchemaName: "NodePoolSpec", + }, + { + Kind: "Channel", + Plural: "channels", + SpecSchemaName: "ChannelSpec", + }, + { + Kind: "Version", + Plural: "versions", + ParentKind: "Channel", + SpecSchemaName: "VersionSpec", + }, +} + +func TestShouldValidateRequest_PostNestedNodePoolPrefersNodePoolOverCluster(t *testing.T) { + RegisterTestingT(t) + + should, plural := shouldValidateRequest( + http.MethodPost, + "/api/hyperfleet/v1/clusters/abc-123/nodepools", + clusterFirstEntities, + ) + + Expect(should).To(BeTrue()) + Expect(plural).To(Equal("nodepools")) +} + +func TestShouldValidateRequest_PatchNestedNodePoolPrefersNodePoolOverCluster(t *testing.T) { + RegisterTestingT(t) + + should, plural := shouldValidateRequest( + http.MethodPatch, + "/api/hyperfleet/v1/clusters/abc-123/nodepools/np-456", + clusterFirstEntities, + ) + + Expect(should).To(BeTrue()) + Expect(plural).To(Equal("nodepools")) +} + +func TestShouldValidateRequest_PostClusterCollectionMatchesCluster(t *testing.T) { + RegisterTestingT(t) + + should, plural := shouldValidateRequest( + http.MethodPost, + "/api/hyperfleet/v1/clusters", + clusterFirstEntities, + ) + + Expect(should).To(BeTrue()) + Expect(plural).To(Equal("clusters")) +} + +func TestShouldValidateRequest_PatchClusterResourceMatchesCluster(t *testing.T) { + RegisterTestingT(t) + + should, plural := shouldValidateRequest( + http.MethodPatch, + "/api/hyperfleet/v1/clusters/abc-123", + clusterFirstEntities, + ) + + Expect(should).To(BeTrue()) + Expect(plural).To(Equal("clusters")) +} + +func TestShouldValidateRequest_PostNestedVersionPrefersVersionOverChannel(t *testing.T) { + RegisterTestingT(t) + + should, plural := shouldValidateRequest( + http.MethodPost, + "/api/hyperfleet/v1/channels/stable/versions", + clusterFirstEntities, + ) + + Expect(should).To(BeTrue()) + Expect(plural).To(Equal("versions")) +} + +func TestShouldValidateRequest_GetSkipsValidation(t *testing.T) { + RegisterTestingT(t) + + should, plural := shouldValidateRequest( + http.MethodGet, + "/api/hyperfleet/v1/clusters/abc-123/nodepools", + clusterFirstEntities, + ) + + Expect(should).To(BeFalse()) + Expect(plural).To(BeEmpty()) +} diff --git a/pkg/middleware/schema_validation_test.go b/pkg/middleware/schema_validation_test.go index 66a3958e..c31886bf 100644 --- a/pkg/middleware/schema_validation_test.go +++ b/pkg/middleware/schema_validation_test.go @@ -11,6 +11,7 @@ import ( . "github.com/onsi/gomega" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/validators" ) @@ -40,6 +41,15 @@ components: type: integer minimum: 1 maximum: 10 + + FooSpec: + type: object + required: + - bar + properties: + bar: + type: string + minLength: 1 ` func TestSchemaValidationMiddleware_PostRequestValidation(t *testing.T) { @@ -258,6 +268,68 @@ func TestSchemaValidationMiddleware_NodePoolValidation(t *testing.T) { Expect(rr.Code).To(Equal(http.StatusCreated)) } +func TestSchemaValidationMiddleware_NestedNodePoolPathUsesNodePoolSchema(t *testing.T) { + RegisterTestingT(t) + + validator := setupTestValidator(t) + middleware := SchemaValidationMiddleware(validator) + + // Valid for ClusterSpec (region) but invalid for NodePoolSpec (missing replicas). + clusterOnlySpec := map[string]interface{}{ + "name": "test-nodepool", + "spec": map[string]interface{}{ + "region": "us-central1", + }, + } + + body, _ := json.Marshal(clusterOnlySpec) + req := httptest.NewRequest(http.MethodPost, "/api/hyperfleet/v1/clusters/123/nodepools", bytes.NewBuffer(body)) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + + nextHandlerCalled := false + nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + nextHandlerCalled = true + }) + + middleware(nextHandler).ServeHTTP(rr, req) + + Expect(nextHandlerCalled).To(BeFalse(), "cluster-shaped spec must not pass on nested nodepool path") + Expect(rr.Code).To(Equal(http.StatusBadRequest)) +} + +func TestSchemaValidationMiddleware_NestedNodePoolPathRejectsClusterSpecOnPatch(t *testing.T) { + RegisterTestingT(t) + + validator := setupTestValidator(t) + middleware := SchemaValidationMiddleware(validator) + + clusterOnlySpec := map[string]interface{}{ + "spec": map[string]interface{}{ + "region": "us-east1", + }, + } + + body, _ := json.Marshal(clusterOnlySpec) + req := httptest.NewRequest( + http.MethodPatch, + "/api/hyperfleet/v1/clusters/123/nodepools/np-456", + bytes.NewBuffer(body), + ) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + + nextHandlerCalled := false + nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + nextHandlerCalled = true + }) + + middleware(nextHandler).ServeHTTP(rr, req) + + Expect(nextHandlerCalled).To(BeFalse()) + Expect(rr.Code).To(Equal(http.StatusBadRequest)) +} + func TestSchemaValidationMiddleware_NodePoolInvalidSpec(t *testing.T) { RegisterTestingT(t) @@ -353,6 +425,78 @@ func TestSchemaValidationMiddleware_InvalidSpecType(t *testing.T) { Expect(*errorResponse.Detail).To(ContainSubstring("spec field must be an object")) } +// TestSchemaValidationMiddleware_RegisteredEntityUsesValidator ensures entities from +// registry.WithSpecSchema() with a matching OpenAPI component are validated end-to-end. +func TestSchemaValidationMiddleware_RegisteredEntityUsesValidator(t *testing.T) { + RegisterTestingT(t) + + validator := setupTestValidator(t) + Expect(validator.HasSchema("foos")).To(BeTrue(), "FooSpec must be loaded for registered plural foos") + + middleware := SchemaValidationMiddleware(validator) + + t.Run("valid spec passes validation and reaches next handler", func(t *testing.T) { + validRequest := map[string]interface{}{ + "kind": "Foo", + "name": "test-foo", + "spec": map[string]interface{}{ + "bar": "ok", + }, + } + + body, err := json.Marshal(validRequest) + Expect(err).NotTo(HaveOccurred()) + + req := httptest.NewRequest(http.MethodPost, "/api/hyperfleet/v1/foos", bytes.NewBuffer(body)) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + + nextHandlerCalled := false + nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + nextHandlerCalled = true + w.WriteHeader(http.StatusCreated) + }) + + middleware(nextHandler).ServeHTTP(rr, req) + + Expect(nextHandlerCalled).To(BeTrue(), "validator should accept valid FooSpec") + Expect(rr.Code).To(Equal(http.StatusCreated)) + }) + + t.Run("invalid spec is rejected by validator before next handler", func(t *testing.T) { + invalidRequest := map[string]interface{}{ + "kind": "Foo", + "name": "test-foo-invalid", + "spec": map[string]interface{}{}, + } + + body, err := json.Marshal(invalidRequest) + Expect(err).NotTo(HaveOccurred()) + + req := httptest.NewRequest(http.MethodPost, "/api/hyperfleet/v1/foos", bytes.NewBuffer(body)) + req.Header.Set("Content-Type", "application/json") + rr := httptest.NewRecorder() + + nextHandlerCalled := false + nextHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + nextHandlerCalled = true + }) + + middleware(nextHandler).ServeHTTP(rr, req) + + Expect(nextHandlerCalled).To(BeFalse(), "validator should reject invalid FooSpec") + Expect(rr.Code).To(Equal(http.StatusBadRequest)) + + var errorResponse openapi.ProblemDetails + err = json.Unmarshal(rr.Body.Bytes(), &errorResponse) + Expect(err).NotTo(HaveOccurred()) + Expect(errorResponse.Code).ToNot(BeNil()) + Expect(*errorResponse.Code).To(Equal("HYPERFLEET-VAL-000")) + Expect(errorResponse.Detail).ToNot(BeNil()) + Expect(*errorResponse.Detail).To(ContainSubstring("Invalid FooSpec")) + }) +} + func TestSchemaValidationMiddleware_MalformedJSON(t *testing.T) { RegisterTestingT(t) @@ -379,6 +523,14 @@ func TestSchemaValidationMiddleware_MalformedJSON(t *testing.T) { // Helper function to setup test validator func setupTestValidator(t *testing.T) *validators.SchemaValidator { + t.Helper() + registry.Reset() + registry.Register(registry.EntityDescriptor{ + Kind: "Foo", + Plural: "foos", + SpecSchemaName: "FooSpec", + }) + tmpDir := t.TempDir() schemaPath := filepath.Join(tmpDir, "test-schema.yaml") err := os.WriteFile(schemaPath, []byte(testSchema), 0600) diff --git a/pkg/registry/registry.go b/pkg/registry/registry.go index 8c88e69f..95da534f 100644 --- a/pkg/registry/registry.go +++ b/pkg/registry/registry.go @@ -42,6 +42,17 @@ func All() []EntityDescriptor { return result } +// WithSpecSchema returns descriptors that declare an OpenAPI spec schema name. +func WithSpecSchema() []EntityDescriptor { + var result []EntityDescriptor + for _, d := range descriptors { + if d.SpecSchemaName != "" { + result = append(result, d) + } + } + return result +} + // ChildrenOf returns descriptors whose ParentKind matches the given kind. func ChildrenOf(parentKind string) []EntityDescriptor { var children []EntityDescriptor diff --git a/pkg/registry/registry_test.go b/pkg/registry/registry_test.go index 51eea76c..05a7050c 100644 --- a/pkg/registry/registry_test.go +++ b/pkg/registry/registry_test.go @@ -88,6 +88,33 @@ func TestAll_ReturnsSnapshot(t *testing.T) { Expect(types).To(HaveKey("Version")) } +func TestWithSpecSchema(t *testing.T) { + RegisterTestingT(t) + Reset() + + Expect(WithSpecSchema()).To(BeEmpty()) + + Register(EntityDescriptor{Kind: "Channel", Plural: "channels"}) + Register(EntityDescriptor{ + Kind: "Version", + Plural: "versions", + ParentKind: "Channel", + SpecSchemaName: "VersionSpec", + }) + Register(EntityDescriptor{Kind: "Cluster", Plural: "clusters", SpecSchemaName: "ClusterSpec"}) + Register(EntityDescriptor{Kind: "WifConfig", Plural: "wifconfigs", SpecSchemaName: "WifConfigSpec"}) + + result := WithSpecSchema() + Expect(result).To(HaveLen(3)) + + var kinds []string + for _, d := range result { + kinds = append(kinds, d.Kind) + Expect(d.SpecSchemaName).NotTo(BeEmpty()) + } + Expect(kinds).To(ConsistOf("Version", "Cluster", "WifConfig")) +} + func TestChildrenOf(t *testing.T) { RegisterTestingT(t) Reset() diff --git a/pkg/validators/schema_validator.go b/pkg/validators/schema_validator.go index 6e811e44..f6402f1e 100644 --- a/pkg/validators/schema_validator.go +++ b/pkg/validators/schema_validator.go @@ -7,8 +7,13 @@ import ( "github.com/getkin/kin-openapi/openapi3" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" ) +// TODO : HYPERFLEET-1159 - Uncomment this once Cluster and NodePool are registered +// var requiredSpecValidationKinds = []string{"Cluster", "NodePool"} + // ResourceSchema represents a validation schema for a specific resource type type ResourceSchema struct { Schema *openapi3.SchemaRef @@ -21,20 +26,64 @@ type SchemaValidator struct { schemas map[string]*ResourceSchema } -// NewSchemaValidator creates a new schema validator by loading an OpenAPI spec from the given path +// NewSchemaValidator creates a new schema validator by loading an OpenAPI spec from the given path. +// Cluster and NodePool must be registered with SpecSchemaName and have matching OpenAPI components. +// Other registered entities with SpecSchemaName are validated only when their component exists; +// missing components are skipped with a warning at startup. func NewSchemaValidator(schemaPath string) (*SchemaValidator, error) { - // Load OpenAPI spec loader := openapi3.NewLoader() doc, err := loader.LoadFromFile(schemaPath) if err != nil { return nil, fmt.Errorf("failed to load OpenAPI schema from %s: %w", schemaPath, err) } - // Validate the loaded document - if err := doc.Validate(context.Background()); err != nil { - return nil, fmt.Errorf("invalid OpenAPI schema: %w", err) + if validateErr := doc.Validate(context.Background()); validateErr != nil { + return nil, fmt.Errorf("invalid OpenAPI schema: %w", validateErr) + } + + schemas, err := buildSchemasMap(doc) + if err != nil { + return nil, err } + return &SchemaValidator{ + doc: doc, + schemas: schemas, + }, nil +} + +func buildSchemasMap(doc *openapi3.T) (map[string]*ResourceSchema, error) { + ctx := context.Background() + schemas := make(map[string]*ResourceSchema) + // registeredKinds := make(map[string]bool, len(requiredSpecValidationKinds)) + + for _, d := range registry.WithSpecSchema() { + schemaRef := doc.Components.Schemas[d.SpecSchemaName] + if schemaRef == nil { + // TODO : HYPERFLEET-1159 - Uncomment this once Cluster and NodePool are registered + // if isRequiredSpecValidationKind(d.Kind) { + // return nil, fmt.Errorf( + // "%s schema not found in OpenAPI spec (required for entity kind %q)", + // d.SpecSchemaName, d.Kind, + // ) + // } + + logger.With(ctx, + "schema_name", d.SpecSchemaName, + "kind", d.Kind, + "plural", d.Plural, + ).Warn("OpenAPI spec schema not found, skipping validation for entity") + continue + } + + schemas[d.Plural] = &ResourceSchema{ + TypeName: d.SpecSchemaName, + Schema: schemaRef, + } + // registeredKinds[d.Kind] = true + } + + // TODO : HYPERFLEET-1159 - Remove this once Cluster and NodePool are registered // Extract ClusterSpec schema clusterSpecSchema := doc.Components.Schemas["ClusterSpec"] if clusterSpecSchema == nil { @@ -48,29 +97,46 @@ func NewSchemaValidator(schemaPath string) (*SchemaValidator, error) { } // Build schemas map - schemas := map[string]*ResourceSchema{ - "cluster": { - TypeName: "ClusterSpec", - Schema: clusterSpecSchema, - }, - "nodepool": { - TypeName: "NodePoolSpec", - Schema: nodePoolSpecSchema, - }, + schemas["clusters"] = &ResourceSchema{ + TypeName: "ClusterSpec", + Schema: clusterSpecSchema, + } + schemas["nodepools"] = &ResourceSchema{ + TypeName: "NodePoolSpec", + Schema: nodePoolSpecSchema, } + // for _, kind := range requiredSpecValidationKinds { + // if !registeredKinds[kind] { + // return nil, fmt.Errorf( + // "entity kind %q with SpecSchemaName must be registered for schema validation", + // kind, + // ) + // } + // } + + return schemas, nil +} - return &SchemaValidator{ - doc: doc, - schemas: schemas, - }, nil +// TODO : HYPERFLEET-1159 - Uncomment this once Cluster and NodePool are registered +// func isRequiredSpecValidationKind(kind string) bool { +// for _, required := range requiredSpecValidationKinds { +// if kind == required { +// return true +// } +// } +// return false +// } + +// HasSchema reports whether a validation schema was loaded for the given resource plural. +func (v *SchemaValidator) HasSchema(resourcePlural string) bool { + return v.schemas[resourcePlural] != nil } -// Validate validates a spec for the given resource type -// Returns nil if resourceType is not found in schemas (allows graceful handling) -func (v *SchemaValidator) Validate(resourceType string, spec map[string]interface{}) error { - resourceSchema := v.schemas[resourceType] +// Validate validates a spec for the given resource plural (URL path segment). +// Returns nil when no schema is loaded for the plural (validation skipped). +func (v *SchemaValidator) Validate(resourcePlural string, spec map[string]interface{}) error { + resourceSchema := v.schemas[resourcePlural] if resourceSchema == nil { - // Unknown resource type, skip validation return nil } @@ -79,16 +145,16 @@ func (v *SchemaValidator) Validate(resourceType string, spec map[string]interfac // ValidateClusterSpec validates a cluster spec against the ClusterSpec schema // -// Deprecated: Use Validate("cluster", spec) instead +// Deprecated: Use Validate("clusters", spec) instead func (v *SchemaValidator) ValidateClusterSpec(spec map[string]interface{}) error { - return v.Validate("cluster", spec) + return v.Validate("clusters", spec) } // ValidateNodePoolSpec validates a nodepool spec against the NodePoolSpec schema // -// Deprecated: Use Validate("nodepool", spec) instead +// Deprecated: Use Validate("nodepools", spec) instead func (v *SchemaValidator) ValidateNodePoolSpec(spec map[string]interface{}) error { - return v.Validate("nodepool", spec) + return v.Validate("nodepools", spec) } // validateSpec performs the actual validation and converts errors to our error format diff --git a/pkg/validators/schema_validator_test.go b/pkg/validators/schema_validator_test.go index 9dd4d1f0..cd2708cc 100644 --- a/pkg/validators/schema_validator_test.go +++ b/pkg/validators/schema_validator_test.go @@ -1,12 +1,16 @@ package validators import ( + "bytes" + "log/slog" "os" "path/filepath" "testing" . "github.com/onsi/gomega" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" ) const testSchema = ` @@ -51,11 +55,34 @@ components: maximum: 100 autoscaling: type: boolean + + WifConfigSpec: + type: object + required: + - version + - project_id + properties: + version: + type: string + project_id: + type: string + minLength: 1 + + ChannelSpec: + type: object + required: + - display_name + properties: + display_name: + type: string + minLength: 1 ` func TestNewSchemaValidator(t *testing.T) { RegisterTestingT(t) + registerRequiredSpecValidationEntities() + // Create temporary schema file tmpDir := t.TempDir() schemaPath := filepath.Join(tmpDir, "test-schema.yaml") @@ -68,12 +95,13 @@ func TestNewSchemaValidator(t *testing.T) { Expect(validator).ToNot(BeNil()) Expect(validator.doc).ToNot(BeNil()) Expect(validator.schemas).ToNot(BeNil()) - Expect(validator.schemas["cluster"]).ToNot(BeNil()) - Expect(validator.schemas["cluster"].Schema).ToNot(BeNil()) - Expect(validator.schemas["cluster"].TypeName).To(Equal("ClusterSpec")) - Expect(validator.schemas["nodepool"]).ToNot(BeNil()) - Expect(validator.schemas["nodepool"].Schema).ToNot(BeNil()) - Expect(validator.schemas["nodepool"].TypeName).To(Equal("NodePoolSpec")) + Expect(validator.schemas["clusters"]).ToNot(BeNil()) + Expect(validator.schemas["clusters"].Schema).ToNot(BeNil()) + Expect(validator.schemas["clusters"].TypeName).To(Equal("ClusterSpec")) + Expect(validator.schemas["nodepools"]).ToNot(BeNil()) + Expect(validator.schemas["nodepools"].Schema).ToNot(BeNil()) + Expect(validator.schemas["nodepools"].TypeName).To(Equal("NodePoolSpec")) + Expect(validator.HasSchema("wifconfigs")).To(BeFalse()) } func TestNewSchemaValidator_InvalidPath(t *testing.T) { @@ -101,6 +129,8 @@ func TestNewSchemaValidator_MalformedContent(t *testing.T) { func TestNewSchemaValidator_MissingSchemas(t *testing.T) { RegisterTestingT(t) + registerRequiredSpecValidationEntities() + // Schema without required components invalidSchema := ` openapi: 3.0.0 @@ -125,6 +155,214 @@ components: Expect(err.Error()).To(ContainSubstring("ClusterSpec schema not found")) } +// TODO : HYPERFLEET-1159 - Uncomment this once Cluster and NodePool are registered +// func TestNewSchemaValidator_MissingRequiredEntityRegistration(t *testing.T) { +// RegisterTestingT(t) + +// registry.Reset() +// registry.Register(registry.EntityDescriptor{ +// Kind: "Cluster", +// Plural: "clusters", +// SpecSchemaName: "ClusterSpec", +// }) + +// tmpDir := t.TempDir() +// schemaPath := filepath.Join(tmpDir, "test-schema.yaml") +// err := os.WriteFile(schemaPath, []byte(testSchema), 0600) +// Expect(err).To(BeNil()) + +// _, err = NewSchemaValidator(schemaPath) +// Expect(err).ToNot(BeNil()) +// Expect(err.Error()).To(ContainSubstring(`entity kind "NodePool" with SpecSchemaName must be registered`)) +// } + +func TestNewSchemaValidator_OptionalEntityMissingOpenAPISchema_SkipsWithWarning(t *testing.T) { + RegisterTestingT(t) + + var logBuf bytes.Buffer + logger.ReconfigureGlobalLogger(&logger.LogConfig{ + Level: slog.LevelWarn, + Format: logger.FormatText, + Output: &logBuf, + Component: "validators-test", + }) + + registerRequiredSpecValidationEntities() + registry.Register(registry.EntityDescriptor{ + Kind: "WifConfig", + Plural: "wifconfigs", + SpecSchemaName: "WifConfigSpec", + }) + + schemaWithoutWifConfig := ` +openapi: 3.0.0 +info: + title: Cluster NodePool Only + version: 1.0.0 +paths: {} +components: + schemas: + ClusterSpec: + type: object + properties: + region: + type: string + NodePoolSpec: + type: object + properties: + replicas: + type: integer +` + + tmpDir := t.TempDir() + schemaPath := filepath.Join(tmpDir, "cluster-nodepool-only.yaml") + err := os.WriteFile(schemaPath, []byte(schemaWithoutWifConfig), 0600) + Expect(err).To(BeNil()) + + validator, err := NewSchemaValidator(schemaPath) + Expect(err).To(BeNil()) + Expect(validator.HasSchema("clusters")).To(BeTrue()) + Expect(validator.HasSchema("nodepools")).To(BeTrue()) + Expect(validator.HasSchema("wifconfigs")).To(BeFalse()) + + logOutput := logBuf.String() + Expect(logOutput).To(ContainSubstring("skipping validation for entity")) + Expect(logOutput).To(ContainSubstring("WifConfigSpec")) + Expect(logOutput).To(ContainSubstring("WifConfig")) +} + +func TestNewSchemaValidator_RequiredEntityMissingOpenAPISchema_Fails(t *testing.T) { + RegisterTestingT(t) + + registerRequiredSpecValidationEntities() + + schemaWithoutNodePool := ` +openapi: 3.0.0 +info: + title: Cluster Only + version: 1.0.0 +paths: {} +components: + schemas: + ClusterSpec: + type: object + properties: + region: + type: string +` + + tmpDir := t.TempDir() + schemaPath := filepath.Join(tmpDir, "cluster-only.yaml") + err := os.WriteFile(schemaPath, []byte(schemaWithoutNodePool), 0600) + Expect(err).To(BeNil()) + + _, err = NewSchemaValidator(schemaPath) + Expect(err).ToNot(BeNil()) + Expect(err.Error()).To(ContainSubstring("NodePoolSpec schema not found")) +} + +func TestValidate_SkipsWhenOptionalEntitySchemaNotLoaded(t *testing.T) { + RegisterTestingT(t) + + var logBuf bytes.Buffer + logger.ReconfigureGlobalLogger(&logger.LogConfig{ + Level: slog.LevelWarn, + Format: logger.FormatText, + Output: &logBuf, + Component: "validators-test", + }) + + registerRequiredSpecValidationEntities() + registry.Register(registry.EntityDescriptor{ + Kind: "WifConfig", + Plural: "wifconfigs", + SpecSchemaName: "WifConfigSpec", + }) + + schemaWithoutWifConfig := ` +openapi: 3.0.0 +info: + title: Cluster NodePool Only + version: 1.0.0 +paths: {} +components: + schemas: + ClusterSpec: + type: object + properties: + region: + type: string + NodePoolSpec: + type: object + properties: + replicas: + type: integer +` + + tmpDir := t.TempDir() + schemaPath := filepath.Join(tmpDir, "cluster-nodepool-only.yaml") + err := os.WriteFile(schemaPath, []byte(schemaWithoutWifConfig), 0600) + Expect(err).To(BeNil()) + + validator, err := NewSchemaValidator(schemaPath) + Expect(err).To(BeNil()) + + // Invalid spec would fail validation if WifConfigSpec were loaded. + err = validator.Validate("wifconfigs", map[string]interface{}{}) + Expect(err).To(BeNil()) +} + +func TestValidate_WifConfigSpec_Valid(t *testing.T) { + RegisterTestingT(t) + + validator := setupTestValidatorWithOptionalEntities(t) + + err := validator.Validate("wifconfigs", map[string]interface{}{ + "version": "4.17", + "project_id": "my-gcp-project", + }) + Expect(err).To(BeNil()) +} + +func TestValidate_WifConfigSpec_MissingRequiredField(t *testing.T) { + RegisterTestingT(t) + + validator := setupTestValidatorWithOptionalEntities(t) + + err := validator.Validate("wifconfigs", map[string]interface{}{ + "version": "4.17", + }) + Expect(err).ToNot(BeNil()) + + serviceErr := getServiceError(err) + Expect(serviceErr).ToNot(BeNil()) + Expect(serviceErr.Details).ToNot(BeEmpty()) +} + +func TestValidate_ChannelSpec_Valid(t *testing.T) { + RegisterTestingT(t) + + validator := setupTestValidatorWithOptionalEntities(t) + + err := validator.Validate("channels", map[string]interface{}{ + "display_name": "stable", + }) + Expect(err).To(BeNil()) +} + +func TestValidate_ChannelSpec_MissingRequiredField(t *testing.T) { + RegisterTestingT(t) + + validator := setupTestValidatorWithOptionalEntities(t) + + err := validator.Validate("channels", map[string]interface{}{}) + Expect(err).ToNot(BeNil()) + + serviceErr := getServiceError(err) + Expect(serviceErr).ToNot(BeNil()) + Expect(serviceErr.Details).ToNot(BeEmpty()) +} + func TestValidateClusterSpec_Valid(t *testing.T) { RegisterTestingT(t) @@ -340,6 +578,57 @@ func TestValidateNodePoolSpec_EmptyMachineType(t *testing.T) { // Helper functions func setupTestValidator(t *testing.T) *SchemaValidator { + t.Helper() + registerRequiredSpecValidationEntities() + + tmpDir := t.TempDir() + schemaPath := filepath.Join(tmpDir, "test-schema.yaml") + err := os.WriteFile(schemaPath, []byte(testSchema), 0600) + if err != nil { + t.Fatalf("Failed to create test schema: %v", err) + } + + validator, err := NewSchemaValidator(schemaPath) + if err != nil { + t.Fatalf("Failed to create validator: %v", err) + } + + return validator +} + +func registerRequiredSpecValidationEntities() { + registry.Reset() + registry.Register(registry.EntityDescriptor{ + Kind: "Cluster", + Plural: "clusters", + SpecSchemaName: "ClusterSpec", + }) + registry.Register(registry.EntityDescriptor{ + Kind: "NodePool", + Plural: "nodepools", + ParentKind: "Cluster", + SpecSchemaName: "NodePoolSpec", + }) +} + +func registerOptionalSpecValidationEntities() { + registry.Register(registry.EntityDescriptor{ + Kind: "WifConfig", + Plural: "wifconfigs", + SpecSchemaName: "WifConfigSpec", + }) + registry.Register(registry.EntityDescriptor{ + Kind: "Channel", + Plural: "channels", + SpecSchemaName: "ChannelSpec", + }) +} + +func setupTestValidatorWithOptionalEntities(t *testing.T) *SchemaValidator { + t.Helper() + registerRequiredSpecValidationEntities() + registerOptionalSpecValidationEntities() + tmpDir := t.TempDir() schemaPath := filepath.Join(tmpDir, "test-schema.yaml") err := os.WriteFile(schemaPath, []byte(testSchema), 0600) @@ -352,6 +641,9 @@ func setupTestValidator(t *testing.T) *SchemaValidator { t.Fatalf("Failed to create validator: %v", err) } + Expect(validator.HasSchema("wifconfigs")).To(BeTrue()) + Expect(validator.HasSchema("channels")).To(BeTrue()) + return validator } diff --git a/plugins/wifconfigs/plugin.go b/plugins/wifconfigs/plugin.go new file mode 100644 index 00000000..8a46d740 --- /dev/null +++ b/plugins/wifconfigs/plugin.go @@ -0,0 +1,44 @@ +package wifconfigs + +import ( + "net/http" + + "github.com/gorilla/mux" + + "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/plugins/resources" +) + +const ( + kindWifConfig = "WifConfig" + pluralWifConfigs = "wifconfigs" + specSchemaWifConfig = "WifConfigSpec" +) + +func init() { + registry.Register(registry.EntityDescriptor{ + Kind: kindWifConfig, + Plural: pluralWifConfigs, + SpecSchemaName: specSchemaWifConfig, + SearchDisallowedFields: []string{"spec"}, + }) + + server.RegisterRoutes(pluralWifConfigs, func(apiV1Router *mux.Router, svc server.ServicesInterface) { + envServices := svc.(*environments.Services) + descriptor := registry.MustGet(kindWifConfig) + h := handlers.NewResourceHandler( + descriptor, + resources.Service(envServices), + ) + + r := apiV1Router.PathPrefix("/" + descriptor.Plural).Subrouter() + r.HandleFunc("", h.List).Methods(http.MethodGet) + r.HandleFunc("", h.Create).Methods(http.MethodPost) + r.HandleFunc("/{id}", h.Get).Methods(http.MethodGet) + r.HandleFunc("/{id}", h.Patch).Methods(http.MethodPatch) + r.HandleFunc("/{id}", h.Delete).Methods(http.MethodDelete) + }) +} diff --git a/test/CLAUDE.md b/test/CLAUDE.md index c2b3acde..17497dd9 100644 --- a/test/CLAUDE.md +++ b/test/CLAUDE.md @@ -30,7 +30,7 @@ Reference: `factories/` `integration/integration_test.go` — `TestMain(m *testing.M)`: - Sets default adapter env vars (`HYPERFLEET_ADAPTERS_REQUIRED_CLUSTER`, `HYPERFLEET_ADAPTERS_REQUIRED_NODEPOOL`) -- Schema validation: `TestMain` auto-sets `HYPERFLEET_SERVER_OPENAPI_SCHEMA_PATH` to `openapi/openapi.yaml` in the repo root if not already set; override with a provider schema path to run provider-specific tests +- Schema validation: `TestMain` auto-sets `HYPERFLEET_SERVER_OPENAPI_SCHEMA_PATH` to `test/validation-schema.yaml` (permissive, all registered entities) if present, else `openapi/openapi.yaml`; override with a strict provider schema path to run `TestClusterSchemaValidationWithProviderSchema` - 45-second timeout safeguard for CI (prevents hung Prow jobs) ## Key Environment Variables @@ -38,4 +38,4 @@ Reference: `factories/` - `HYPERFLEET_ENV` — selects config environment: `unit_testing`, `integration_testing`, `development` - `TESTCONTAINERS_RYUK_DISABLED=true` — required for testcontainers in CI - `HYPERFLEET_ADAPTERS_REQUIRED_CLUSTER` / `HYPERFLEET_ADAPTERS_REQUIRED_NODEPOOL` — adapter lists for tests -- `HYPERFLEET_SERVER_OPENAPI_SCHEMA_PATH` — path to OpenAPI schema for spec validation (auto-set by `TestMain` to `openapi/openapi.yaml`; override with a provider schema to run `TestClusterSchemaValidationWithProviderSchema`) +- `HYPERFLEET_SERVER_OPENAPI_SCHEMA_PATH` — path to OpenAPI schema for spec validation (auto-set by `TestMain` to `test/validation-schema.yaml`; override with a strict provider schema to run `TestClusterSchemaValidationWithProviderSchema`) diff --git a/test/integration/clusters_test.go b/test/integration/clusters_test.go index 2a8bbb45..52fbb874 100644 --- a/test/integration/clusters_test.go +++ b/test/integration/clusters_test.go @@ -506,11 +506,13 @@ func TestClusterSchemaValidation(t *testing.T) { func TestClusterSchemaValidationWithProviderSchema(t *testing.T) { RegisterTestingT(t) - // Check if we're using a provider schema or base schema - // If base schema, skip detailed validation tests + // Run only when a strict provider-specific schema is configured. + // Default integration tests use test/validation-schema.yaml (permissive) or openapi/openapi.yaml. schemaPath := os.Getenv("HYPERFLEET_SERVER_OPENAPI_SCHEMA_PATH") - if schemaPath == "" || strings.HasSuffix(schemaPath, "openapi/openapi.yaml") { - t.Skip("Skipping provider schema validation test - using base schema") + if schemaPath == "" || + strings.HasSuffix(schemaPath, "openapi/openapi.yaml") || + strings.HasSuffix(schemaPath, "validation-schema.yaml") { + t.Skip("Skipping provider schema validation test - using permissive or base schema") return } diff --git a/test/integration/integration_test.go b/test/integration/integration_test.go index f392d089..7138a117 100755 --- a/test/integration/integration_test.go +++ b/test/integration/integration_test.go @@ -54,8 +54,9 @@ func TestMain(m *testing.M) { testDir := filepath.Dir(integrationDir) // /path/to/repo/test repoRoot := filepath.Dir(testDir) // /path/to/repo - // Build schema path using filepath.Join for cross-platform compatibility - schemaPath := filepath.Join(repoRoot, "openapi", "openapi.yaml") + // Prefer the integration test validation schema, which includes every + // registered entity that declares SpecSchemaName. + schemaPath := filepath.Join(repoRoot, "test", "validation-schema.yaml") // Verify the schema file exists before setting the env var if _, err := os.Stat(schemaPath); err != nil { diff --git a/test/integration/wifconfigs_test.go b/test/integration/wifconfigs_test.go new file mode 100644 index 00000000..89d8f716 --- /dev/null +++ b/test/integration/wifconfigs_test.go @@ -0,0 +1,339 @@ +package integration + +import ( + "encoding/json" + "fmt" + "net/http" + "os" + "strings" + "testing" + + . "github.com/onsi/gomega" + "gopkg.in/resty.v1" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" + "github.com/openshift-hyperfleet/hyperfleet-api/test" + + _ "github.com/openshift-hyperfleet/hyperfleet-api/plugins/wifconfigs" +) + +const wifConfigsPath = "/wifconfigs" + +// TestWifConfigSchemaValidation tests schema validation middleware for WifConfig resources. +// Default integration tests use test/validation-schema.yaml (permissive WifConfigSpec). +func TestWifConfigSchemaValidation(t *testing.T) { + h, _ := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + jwtToken := test.GetAccessTokenFromContext(ctx) + + // Test 1: Valid spec (permissive schema accepts any object) + validInput := openapi.ResourceCreateRequest{ + Kind: "WifConfig", + Name: "schema-valid-WifConfig", + Spec: map[string]interface{}{ + "projectId": "my-project", + "version": "4.17", + }, + } + validBody, err := json.Marshal(validInput) + Expect(err).NotTo(HaveOccurred()) + + resp, err := resty.R(). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). + SetBody(validBody). + Post(h.RestURL(wifConfigsPath)) + Expect(err).NotTo(HaveOccurred(), "Valid WifConfig spec should be accepted") + Expect(resp.StatusCode()).To(Equal(http.StatusCreated)) + + var created map[string]interface{} + err = json.Unmarshal(resp.Body(), &created) + Expect(err).NotTo(HaveOccurred()) + Expect(created["id"]).NotTo(BeEmpty()) + + // Test 2: Invalid spec type (spec must be object, not string) + invalidTypeJSON := `{ + "kind": "WifConfig", + "name": "schema-invalid-type", + "spec": "invalid-string-spec" + }` + + resp2, err := resty.R(). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). + SetBody(invalidTypeJSON). + Post(h.RestURL(wifConfigsPath)) + Expect(err).NotTo(HaveOccurred()) + Expect(resp2.StatusCode()).To(Equal(http.StatusBadRequest), + "Non-object spec must be rejected") + + var errorResponse openapi.ProblemDetails + Expect(json.Unmarshal(resp2.Body(), &errorResponse)).To(Succeed()) + Expect(errorResponse.Type).ToNot(BeEmpty()) + Expect(errorResponse.Code).ToNot(BeNil()) + Expect(errorResponse.Detail).ToNot(BeNil()) + + // Test 3: Empty spec (valid when WifConfigSpec has no required fields) + emptySpecInput := openapi.ResourceCreateRequest{ + Kind: "WifConfig", + Name: "schema-empty-spec", + Spec: map[string]interface{}{}, + } + emptyBody, err := json.Marshal(emptySpecInput) + Expect(err).NotTo(HaveOccurred()) + + resp3, err := resty.R(). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). + SetBody(emptyBody). + Post(h.RestURL(wifConfigsPath)) + Expect(err).NotTo(HaveOccurred(), "Empty spec should be accepted by permissive schema") + Expect(resp3.StatusCode()).To(Equal(http.StatusCreated)) +} + +// TestWifConfigSchemaValidationWithProviderSchema runs only when a strict provider-specific schema is configured. +func TestWifConfigSchemaValidationWithProviderSchema(t *testing.T) { + RegisterTestingT(t) + + schemaPath := os.Getenv("HYPERFLEET_SERVER_OPENAPI_SCHEMA_PATH") + if schemaPath == "" || + strings.HasSuffix(schemaPath, "openapi/openapi.yaml") || + strings.HasSuffix(schemaPath, "validation-schema.yaml") { + t.Skip("Skipping provider schema validation test - using permissive or base schema") + return + } + + h, _ := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + jwtToken := test.GetAccessTokenFromContext(ctx) + + // Missing required project_id (typical in strict WifConfigSpec overlays) + invalidInput := openapi.ResourceCreateRequest{ + Kind: "WifConfig", + Name: "provider-schema-invalid", + Spec: map[string]interface{}{ + "version": "4.17", + }, + } + body, err := json.Marshal(invalidInput) + Expect(err).NotTo(HaveOccurred()) + + resp, err := resty.R(). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). + SetBody(body). + Post(h.RestURL(wifConfigsPath)) + Expect(err).NotTo(HaveOccurred()) + Expect(resp.StatusCode()).To(Equal(http.StatusBadRequest), + "Should reject spec with missing required field") + + var errorResponse openapi.ProblemDetails + if err := json.Unmarshal(resp.Body(), &errorResponse); err != nil { + t.Fatalf("failed to unmarshal error response body: %v", err) + } + + Expect(errorResponse.Type).ToNot(BeEmpty()) + Expect(errorResponse.Code).ToNot(BeNil()) + Expect(*errorResponse.Code).To(Equal("HYPERFLEET-VAL-000")) + Expect(errorResponse.Errors).ToNot(BeEmpty(), "Should include field-level error details") + + foundProjectIDError := false + if errorResponse.Errors != nil { + for _, detail := range *errorResponse.Errors { + if strings.Contains(detail.Field, "project_id") { + foundProjectIDError = true + break + } + } + } + Expect(foundProjectIDError).To(BeTrue(), "Error details should mention missing 'project_id' field") +} + +// TestWifConfigSchemaValidationErrorDetails tests RFC 9457 error structure for invalid spec types. +func TestWifConfigSchemaValidationErrorDetails(t *testing.T) { + RegisterTestingT(t) + h, _ := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + jwtToken := test.GetAccessTokenFromContext(ctx) + + invalidTypeRequest := map[string]interface{}{ + "kind": "WifConfig", + "name": "error-details-test", + "spec": "not-an-object", + } + + body, err := json.Marshal(invalidTypeRequest) + Expect(err).NotTo(HaveOccurred()) + + resp, err := resty.R(). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). + SetBody(body). + Post(h.RestURL(wifConfigsPath)) + Expect(err).To(BeNil()) + + t.Logf("Response status: %d, body: %s", resp.StatusCode(), string(resp.Body())) + Expect(resp.StatusCode()).To(Equal(http.StatusBadRequest), "Should return 400 for invalid spec type") + + var errorResponse openapi.ProblemDetails + if err := json.Unmarshal(resp.Body(), &errorResponse); err != nil { + t.Fatalf("failed to unmarshal error response: %v, response body: %s", err, string(resp.Body())) + } + + Expect(errorResponse.Type).ToNot(BeEmpty()) + Expect(errorResponse.Title).ToNot(BeEmpty()) + Expect(errorResponse.Code).ToNot(BeNil()) + + validCodes := []string{"HYPERFLEET-VAL-000", "HYPERFLEET-VAL-006"} + Expect(validCodes).To(ContainElement(*errorResponse.Code)) + + Expect(errorResponse.Detail).ToNot(BeNil()) + Expect(*errorResponse.Detail).To(ContainSubstring("spec")) + Expect(errorResponse.Instance).ToNot(BeNil()) + Expect(errorResponse.TraceId).ToNot(BeNil()) +} + +// TestWifConfigPost_EmptyKind tests that empty kind field returns 400. +func TestWifConfigPost_EmptyKind(t *testing.T) { + RegisterTestingT(t) + h, _ := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + jwtToken := test.GetAccessTokenFromContext(ctx) + + invalidInput := `{ + "kind": "", + "name": "test-WifConfig", + "spec": {} + }` + + restyResp, err := resty.R(). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). + SetBody(invalidInput). + Post(h.RestURL(wifConfigsPath)) + + Expect(err).ToNot(HaveOccurred()) + Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest)) + + var errorResponse map[string]interface{} + err = json.Unmarshal(restyResp.Body(), &errorResponse) + Expect(err).ToNot(HaveOccurred()) + + detail, ok := errorResponse["detail"].(string) + Expect(ok).To(BeTrue()) + Expect(detail).To(ContainSubstring("kind is required")) +} + +// TestWifConfigPost_WrongKind tests that wrong kind field returns 400. +func TestWifConfigPost_WrongKind(t *testing.T) { + RegisterTestingT(t) + h, _ := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + jwtToken := test.GetAccessTokenFromContext(ctx) + + invalidInput := `{ + "kind": "Cluster", + "name": "test-WifConfig", + "spec": {} + }` + + restyResp, err := resty.R(). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). + SetBody(invalidInput). + Post(h.RestURL(wifConfigsPath)) + + Expect(err).ToNot(HaveOccurred()) + Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest)) + + var errorResponse map[string]interface{} + err = json.Unmarshal(restyResp.Body(), &errorResponse) + Expect(err).ToNot(HaveOccurred()) + + detail, ok := errorResponse["detail"].(string) + Expect(ok).To(BeTrue()) + Expect(detail).To(ContainSubstring("kind must be 'WifConfig'")) +} + +// TestWifConfigPost_NullSpec tests that null spec field returns 400. +func TestWifConfigPost_NullSpec(t *testing.T) { + RegisterTestingT(t) + h, _ := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + jwtToken := test.GetAccessTokenFromContext(ctx) + + invalidInput := `{ + "kind": "WifConfig", + "name": "test-WifConfig", + "spec": null + }` + + restyResp, err := resty.R(). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). + SetBody(invalidInput). + Post(h.RestURL(wifConfigsPath)) + + Expect(err).ToNot(HaveOccurred()) + Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest)) + + var errorResponse map[string]interface{} + err = json.Unmarshal(restyResp.Body(), &errorResponse) + Expect(err).ToNot(HaveOccurred()) + + code, ok := errorResponse["code"].(string) + Expect(ok).To(BeTrue()) + Expect(code).To(Equal("HYPERFLEET-VAL-000")) + + detail, ok := errorResponse["detail"].(string) + Expect(ok).To(BeTrue()) + Expect(detail).To(ContainSubstring("spec field must be an object")) +} + +// TestWifConfigPost_MissingSpec tests that missing spec field returns 400. +func TestWifConfigPost_MissingSpec(t *testing.T) { + RegisterTestingT(t) + h, _ := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + jwtToken := test.GetAccessTokenFromContext(ctx) + + invalidInput := `{ + "kind": "WifConfig", + "name": "test-WifConfig" + }` + + restyResp, err := resty.R(). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). + SetBody(invalidInput). + Post(h.RestURL(wifConfigsPath)) + + Expect(err).ToNot(HaveOccurred()) + Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest)) + + var errorResponse map[string]interface{} + err = json.Unmarshal(restyResp.Body(), &errorResponse) + Expect(err).ToNot(HaveOccurred()) + + code, ok := errorResponse["code"].(string) + Expect(ok).To(BeTrue()) + Expect(code).To(Equal("HYPERFLEET-VAL-000")) + + detail, ok := errorResponse["detail"].(string) + Expect(ok).To(BeTrue()) + Expect(detail).To(ContainSubstring("spec is required")) +} diff --git a/test/validation-schema.yaml b/test/validation-schema.yaml new file mode 100644 index 00000000..5783e125 --- /dev/null +++ b/test/validation-schema.yaml @@ -0,0 +1,22 @@ +openapi: 3.0.0 +info: + title: HyperFleet Integration Test Validation Schema + version: 1.0.0 +paths: {} +components: + schemas: + ClusterSpec: + type: object + description: Permissive cluster spec schema for integration tests. + NodePoolSpec: + type: object + description: Permissive nodepool spec schema for integration tests. + WifConfigSpec: + type: object + description: Permissive WifConfig spec schema for integration tests. + ChannelSpec: + type: object + description: Permissive channel spec schema for integration tests. + VersionSpec: + type: object + description: Permissive version spec schema for integration tests. From 4befe9c13e8c4a33031754a0566b1cf5ef67ddec Mon Sep 17 00:00:00 2001 From: sherine-k Date: Thu, 4 Jun 2026 17:23:58 +0200 Subject: [PATCH 2/2] HYPERFLEET-1090 - refactor: schema_validation: use regex path matching --- pkg/middleware/schema_validation.go | 90 ++++++++++--------- .../schema_validation_match_test.go | 41 ++++++--- pkg/middleware/schema_validation_test.go | 28 ++++-- 3 files changed, 95 insertions(+), 64 deletions(-) diff --git a/pkg/middleware/schema_validation.go b/pkg/middleware/schema_validation.go index dca7a293..8f0c9786 100644 --- a/pkg/middleware/schema_validation.go +++ b/pkg/middleware/schema_validation.go @@ -3,9 +3,10 @@ package middleware import ( "bytes" "encoding/json" + "fmt" "io" "net/http" - "strings" + "regexp" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/logger" @@ -37,6 +38,35 @@ func handleValidationError(w http.ResponseWriter, r *http.Request, err *errors.S } } +// specEntityMatcher pairs a resource plural with a pre-compiled regex for path matching. +type specEntityMatcher struct { + re *regexp.Regexp + plural string +} + +// buildEndsWithEntityRegex compiles a regex that matches a URL path ending in: +// - / +// - // +// - // +func buildEndsWithEntityRegex(plural string) *regexp.Regexp { + pattern := fmt.Sprintf( + `/%s(?:/?|/[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12})$`, + regexp.QuoteMeta(plural), + ) + return regexp.MustCompile(pattern) +} + +func buildMatchers(specEntities []registry.EntityDescriptor) []specEntityMatcher { + matchers := make([]specEntityMatcher, len(specEntities)) + for i, d := range specEntities { + matchers[i] = specEntityMatcher{ + plural: d.Plural, + re: buildEndsWithEntityRegex(d.Plural), + } + } + return matchers +} + // SchemaValidationMiddleware validates resource spec fields against OpenAPI schemas for every // registered entity that declares SpecSchemaName. func SchemaValidationMiddleware(validator *validators.SchemaValidator) func(http.Handler) http.Handler { @@ -57,10 +87,11 @@ func SchemaValidationMiddleware(validator *validators.SchemaValidator) func(http } specEntities = append(specEntities, registry.WithSpecSchema()...) + matchers := buildMatchers(specEntities) return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - shouldValidate, resourcePlural := shouldValidateRequest(r.Method, r.URL.Path, specEntities) + shouldValidate, resourcePlural := shouldValidateRequest(r.Method, r.URL.Path, matchers) if !shouldValidate { next.ServeHTTP(w, r) return @@ -97,7 +128,7 @@ func SchemaValidationMiddleware(validator *validators.SchemaValidator) func(http } // Convert spec to map[string]interface{} - specMap, ok := spec.(map[string]interface{}) + specMap, ok := spec.(map[string]any) if !ok { serviceErr := errors.Validation("spec field must be an object") handleValidationError(w, r, serviceErr) @@ -126,54 +157,25 @@ func SchemaValidationMiddleware(validator *validators.SchemaValidator) func(http } // shouldValidateRequest determines if the request requires spec validation. -// Returns (shouldValidate bool, resourcePlural string). // -// When a path contains multiple registered plurals (e.g. /clusters/{id}/nodepools), -// the rightmost matching segment wins so nested resources use their own spec schema. +// Each matcher's regex anchors at the end of the path, so when a path contains +// multiple registered plurals (e.g. /clusters/{id}/nodepools), only the +// rightmost (deepest) segment can match, and that match wins automatically. func shouldValidateRequest( - method, path string, specEntities []registry.EntityDescriptor, -) (bool, string) { + method, path string, matchers []specEntityMatcher, +) (shouldValidate bool, resourcePlural string) { if method != http.MethodPost && method != http.MethodPatch { return false, "" } - // this path matching logic is used to determine the correct spec schema to use for validation - // based on the path of the request. - // For example, if the path is /clusters/abc-123/nodepools/np-456, the matched plural will be "nodepools". - var ( - matchedPlural string - matchedIndex = -1 - ) - - for _, d := range specEntities { - segment := "/" + d.Plural - if !pathMatchesSpecEntity(method, path, segment) { - continue - } - - idx := strings.LastIndex(path, segment) - if idx < matchedIndex { - continue - } - if idx > matchedIndex { - matchedIndex = idx - matchedPlural = d.Plural + shouldValidate = false + for _, m := range matchers { + shouldValidate = m.re.MatchString(path) + if shouldValidate { + resourcePlural = m.plural + break } } - if matchedPlural == "" { - return false, "" - } - return true, matchedPlural -} - -func pathMatchesSpecEntity(method, path, segment string) bool { - switch method { - case http.MethodPost: - return strings.HasSuffix(path, segment) - case http.MethodPatch: - return strings.Contains(path, segment+"/") - default: - return false - } + return shouldValidate, resourcePlural } diff --git a/pkg/middleware/schema_validation_match_test.go b/pkg/middleware/schema_validation_match_test.go index a27cf1c5..8639a0aa 100644 --- a/pkg/middleware/schema_validation_match_test.go +++ b/pkg/middleware/schema_validation_match_test.go @@ -8,8 +8,8 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/registry" ) -// clusterFirstEntities lists clusters before nodepools to ensure matching does not depend on slice order. -var clusterFirstEntities = []registry.EntityDescriptor{ +// matchers lists clusters before nodepools to ensure matching does not depend on slice order. +var matchers = buildMatchers([]registry.EntityDescriptor{ { Kind: "Cluster", Plural: "clusters", @@ -32,15 +32,15 @@ var clusterFirstEntities = []registry.EntityDescriptor{ ParentKind: "Channel", SpecSchemaName: "VersionSpec", }, -} +}) func TestShouldValidateRequest_PostNestedNodePoolPrefersNodePoolOverCluster(t *testing.T) { RegisterTestingT(t) should, plural := shouldValidateRequest( http.MethodPost, - "/api/hyperfleet/v1/clusters/abc-123/nodepools", - clusterFirstEntities, + "/api/hyperfleet/v1/clusters/550e8400-e29b-41d4-a716-446655440000/nodepools", + matchers, ) Expect(should).To(BeTrue()) @@ -52,8 +52,8 @@ func TestShouldValidateRequest_PatchNestedNodePoolPrefersNodePoolOverCluster(t * should, plural := shouldValidateRequest( http.MethodPatch, - "/api/hyperfleet/v1/clusters/abc-123/nodepools/np-456", - clusterFirstEntities, + "/api/hyperfleet/v1/clusters/550e8400-e29b-41d4-a716-446655440000/nodepools/660e8400-e29b-41d4-a716-446655440001", + matchers, ) Expect(should).To(BeTrue()) @@ -66,7 +66,7 @@ func TestShouldValidateRequest_PostClusterCollectionMatchesCluster(t *testing.T) should, plural := shouldValidateRequest( http.MethodPost, "/api/hyperfleet/v1/clusters", - clusterFirstEntities, + matchers, ) Expect(should).To(BeTrue()) @@ -78,21 +78,34 @@ func TestShouldValidateRequest_PatchClusterResourceMatchesCluster(t *testing.T) should, plural := shouldValidateRequest( http.MethodPatch, - "/api/hyperfleet/v1/clusters/abc-123", - clusterFirstEntities, + "/api/hyperfleet/v1/clusters/550e8400-e29b-41d4-a716-446655440000", + matchers, ) Expect(should).To(BeTrue()) Expect(plural).To(Equal("clusters")) } +func TestShouldValidateRequest_PatchVersionResourceMatchesVersion(t *testing.T) { + RegisterTestingT(t) + + should, plural := shouldValidateRequest( + http.MethodPatch, + "/api/hyperfleet/v1/channels/cc0e8400-e29b-41d4-a716-446655440000/versions/550e8400-e29b-41d4-a716-446655440000", + matchers, + ) + + Expect(should).To(BeTrue()) + Expect(plural).To(Equal("versions")) +} + func TestShouldValidateRequest_PostNestedVersionPrefersVersionOverChannel(t *testing.T) { RegisterTestingT(t) should, plural := shouldValidateRequest( http.MethodPost, - "/api/hyperfleet/v1/channels/stable/versions", - clusterFirstEntities, + "/api/hyperfleet/v1/channels/cc0e8400-e29b-41d4-a716-446655440000/versions", + matchers, ) Expect(should).To(BeTrue()) @@ -104,8 +117,8 @@ func TestShouldValidateRequest_GetSkipsValidation(t *testing.T) { should, plural := shouldValidateRequest( http.MethodGet, - "/api/hyperfleet/v1/clusters/abc-123/nodepools", - clusterFirstEntities, + "/api/hyperfleet/v1/clusters/550e8400-e29b-41d4-a716-446655440000/nodepools", + matchers, ) Expect(should).To(BeFalse()) diff --git a/pkg/middleware/schema_validation_test.go b/pkg/middleware/schema_validation_test.go index c31886bf..e37bf60f 100644 --- a/pkg/middleware/schema_validation_test.go +++ b/pkg/middleware/schema_validation_test.go @@ -137,7 +137,11 @@ func TestSchemaValidationMiddleware_PatchRequestValidation(t *testing.T) { } body, _ := json.Marshal(validRequest) - req := httptest.NewRequest(http.MethodPatch, "/api/hyperfleet/v1/clusters/123", bytes.NewBuffer(body)) + req := httptest.NewRequest( + http.MethodPatch, + "/api/hyperfleet/v1/clusters/550e8400-e29b-41d4-a716-446655440000", + bytes.NewBuffer(body), + ) req.Header.Set("Content-Type", "application/json") rr := httptest.NewRecorder() @@ -184,7 +188,7 @@ func TestSchemaValidationMiddleware_DeleteRequestSkipped(t *testing.T) { middleware := SchemaValidationMiddleware(validator) // DELETE request should skip validation - req := httptest.NewRequest(http.MethodDelete, "/api/hyperfleet/v1/clusters/123", nil) + req := httptest.NewRequest(http.MethodDelete, "/api/hyperfleet/v1/clusters/550e8400-e29b-41d4-a716-446655440000", nil) rr := httptest.NewRecorder() nextHandlerCalled := false @@ -245,7 +249,11 @@ func TestSchemaValidationMiddleware_NodePoolValidation(t *testing.T) { } body, _ := json.Marshal(validRequest) - req := httptest.NewRequest(http.MethodPost, "/api/hyperfleet/v1/clusters/123/nodepools", bytes.NewBuffer(body)) + req := httptest.NewRequest( + http.MethodPost, + "/api/hyperfleet/v1/clusters/550e8400-e29b-41d4-a716-446655440000/nodepools", + bytes.NewBuffer(body), + ) req.Header.Set("Content-Type", "application/json") rr := httptest.NewRecorder() @@ -283,7 +291,11 @@ func TestSchemaValidationMiddleware_NestedNodePoolPathUsesNodePoolSchema(t *test } body, _ := json.Marshal(clusterOnlySpec) - req := httptest.NewRequest(http.MethodPost, "/api/hyperfleet/v1/clusters/123/nodepools", bytes.NewBuffer(body)) + req := httptest.NewRequest( + http.MethodPost, + "/api/hyperfleet/v1/clusters/550e8400-e29b-41d4-a716-446655440000/nodepools", + bytes.NewBuffer(body), + ) req.Header.Set("Content-Type", "application/json") rr := httptest.NewRecorder() @@ -313,7 +325,7 @@ func TestSchemaValidationMiddleware_NestedNodePoolPathRejectsClusterSpecOnPatch( body, _ := json.Marshal(clusterOnlySpec) req := httptest.NewRequest( http.MethodPatch, - "/api/hyperfleet/v1/clusters/123/nodepools/np-456", + "/api/hyperfleet/v1/clusters/550e8400-e29b-41d4-a716-446655440000/nodepools/660e8400-e29b-41d4-a716-446655440001", bytes.NewBuffer(body), ) req.Header.Set("Content-Type", "application/json") @@ -345,7 +357,11 @@ func TestSchemaValidationMiddleware_NodePoolInvalidSpec(t *testing.T) { } body, _ := json.Marshal(invalidRequest) - req := httptest.NewRequest(http.MethodPost, "/api/hyperfleet/v1/clusters/123/nodepools", bytes.NewBuffer(body)) + req := httptest.NewRequest( + http.MethodPost, + "/api/hyperfleet/v1/clusters/550e8400-e29b-41d4-a716-446655440000/nodepools", + bytes.NewBuffer(body), + ) req.Header.Set("Content-Type", "application/json") rr := httptest.NewRecorder()