From 6524faa0d5b5b03ef9668bf5e40b96a8d46654f8 Mon Sep 17 00:00:00 2001 From: yasun Date: Tue, 9 Dec 2025 21:30:55 +0800 Subject: [PATCH] Add cluster/nodepool name & kind validation and remove useless endpoint compatibility gofmt --- cmd/hyperfleet-api/server/routes.go | 4 - openapi/openapi.yaml | 111 ++++++++-------- pkg/api/cluster_types.go | 3 - pkg/api/cluster_types_test.go | 10 +- pkg/api/node_pool_types.go | 3 - pkg/api/node_pool_types_test.go | 10 +- pkg/api/openapi_embed.go | 1 - pkg/api/presenters/adapter_status_test.go | 4 +- pkg/auth/auth_middleware.go | 1 - pkg/dao/mocks/generic.go | 12 +- pkg/handlers/cluster.go | 2 + pkg/handlers/cluster_nodepools.go | 10 +- pkg/handlers/compatibility.go | 34 ----- pkg/handlers/node_pool.go | 8 +- pkg/handlers/validation.go | 68 ++++++++++ pkg/handlers/validation_test.go | 147 ++++++++++++++++++++++ pkg/logger/logger.go | 5 +- plugins/clusters/plugin.go | 1 - plugins/nodePools/plugin.go | 1 - test/helper.go | 7 ++ test/integration/api_contract_test.go | 40 ++++++ test/integration/clusters_test.go | 95 +++++++++++--- test/integration/compatibility_test.go | 34 ----- test/integration/integration_test.go | 6 +- test/integration/node_pools_test.go | 81 ++++++++++++ 25 files changed, 518 insertions(+), 180 deletions(-) delete mode 100644 pkg/handlers/compatibility.go create mode 100644 pkg/handlers/validation_test.go create mode 100644 test/integration/api_contract_test.go delete mode 100644 test/integration/compatibility_test.go diff --git a/cmd/hyperfleet-api/server/routes.go b/cmd/hyperfleet-api/server/routes.go index 2e077d5..3b867ba 100755 --- a/cmd/hyperfleet-api/server/routes.go +++ b/cmd/hyperfleet-api/server/routes.go @@ -84,10 +84,6 @@ func (s *apiServer) routes() *mux.Router { apiV1Router.HandleFunc("/openapi.html", openapiHandler.GetOpenAPIUI).Methods(http.MethodGet) apiV1Router.HandleFunc("/openapi", openapiHandler.GetOpenAPI).Methods(http.MethodGet) - // /api/hyperfleet/v1/compatibility - compatibilityHandler := handlers.NewCompatibilityHandler() - apiV1Router.HandleFunc("/compatibility", compatibilityHandler.Get).Methods(http.MethodGet) - registerApiMiddleware(apiV1Router) // Auto-discovered routes (no manual editing needed) diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index f7c17ba..b28a8ba 100644 --- a/openapi/openapi.yaml +++ b/openapi/openapi.yaml @@ -4,6 +4,9 @@ info: version: 1.0.0 contact: name: HyperFleet Team + license: + name: Apache 2.0 + url: https://www.apache.org/licenses/LICENSE-2.0 description: |- HyperFleet API provides simple CRUD operations for managing cluster resources and their status history. @@ -29,6 +32,8 @@ paths: application/json: schema: $ref: '#/components/schemas/ClusterList' + '400': + description: The server could not understand the request due to invalid syntax. default: description: An unexpected error response. content: @@ -52,6 +57,8 @@ paths: application/json: schema: $ref: '#/components/schemas/Cluster' + '400': + description: The server could not understand the request due to invalid syntax. default: description: An unexpected error response. content: @@ -82,6 +89,8 @@ paths: application/json: schema: $ref: '#/components/schemas/Cluster' + '400': + description: The server could not understand the request due to invalid syntax. default: description: An unexpected error response. content: @@ -112,6 +121,8 @@ paths: application/json: schema: $ref: '#/components/schemas/NodePoolList' + '400': + description: The server could not understand the request due to invalid syntax. default: description: An unexpected error response. content: @@ -136,6 +147,8 @@ paths: application/json: schema: $ref: '#/components/schemas/NodePoolCreateResponse' + '400': + description: The server could not understand the request due to invalid syntax. default: description: An unexpected error response. content: @@ -173,6 +186,8 @@ paths: application/json: schema: $ref: '#/components/schemas/NodePool' + '400': + description: The server could not understand the request due to invalid syntax. default: description: An unexpected error response. content: @@ -180,40 +195,6 @@ paths: schema: $ref: '#/components/schemas/Error' /api/hyperfleet/v1/clusters/{cluster_id}/nodepools/{nodepool_id}/statuses: - get: - operationId: getNodePoolsStatuses - summary: List all adapter statuses for nodepools - description: Returns adapter status reports for this nodepool - parameters: - - name: cluster_id - in: path - required: true - description: Cluster ID - schema: - type: string - - name: nodepool_id - in: path - required: true - schema: - type: string - - $ref: '#/components/parameters/QueryParams.page' - - $ref: '#/components/parameters/QueryParams.pageSize' - - $ref: '#/components/parameters/QueryParams.orderBy' - - $ref: '#/components/parameters/QueryParams.order' - - $ref: '#/components/parameters/SearchParams' - responses: - '200': - description: The request has succeeded. - content: - application/json: - schema: - $ref: '#/components/schemas/AdapterStatusList' - default: - description: An unexpected error response. - content: - application/json: - schema: - $ref: '#/components/schemas/Error' post: operationId: postNodePoolStatuses summary: Create or update adapter status @@ -254,11 +235,10 @@ paths: application/json: schema: $ref: '#/components/schemas/AdapterStatusCreateRequest' - /api/hyperfleet/v1/clusters/{cluster_id}/statuses: get: - operationId: getClusterStatuses - summary: List all adapter statuses for cluster - description: Returns adapter status reports for this cluster + operationId: getNodePoolsStatuses + summary: List all adapter statuses for nodepools + description: Returns adapter status reports for this nodepool parameters: - name: cluster_id in: path @@ -266,6 +246,11 @@ paths: description: Cluster ID schema: type: string + - name: nodepool_id + in: path + required: true + schema: + type: string - $ref: '#/components/parameters/QueryParams.page' - $ref: '#/components/parameters/QueryParams.pageSize' - $ref: '#/components/parameters/QueryParams.orderBy' @@ -278,8 +263,15 @@ paths: application/json: schema: $ref: '#/components/schemas/AdapterStatusList' - '404': - description: The server cannot find the requested resource. + '400': + description: The server could not understand the request due to invalid syntax. + default: + description: An unexpected error response. + content: + application/json: + schema: + $ref: '#/components/schemas/Error' + /api/hyperfleet/v1/clusters/{cluster_id}/statuses: post: operationId: postClusterStatuses summary: Create or update adapter status @@ -315,11 +307,17 @@ paths: application/json: schema: $ref: '#/components/schemas/AdapterStatusCreateRequest' - /api/hyperfleet/v1/compatibility: get: - operationId: getCompatibility - description: Returns the list of all nodepools + operationId: getClusterStatuses + summary: List all adapter statuses for cluster + description: Returns adapter status reports for this cluster parameters: + - name: cluster_id + in: path + required: true + description: Cluster ID + schema: + type: string - $ref: '#/components/parameters/QueryParams.page' - $ref: '#/components/parameters/QueryParams.pageSize' - $ref: '#/components/parameters/QueryParams.orderBy' @@ -329,11 +327,13 @@ paths: '200': description: The request has succeeded. content: - text/plain: + application/json: schema: - type: string - security: - - BearerAuth: [] + $ref: '#/components/schemas/AdapterStatusList' + '400': + description: The server could not understand the request due to invalid syntax. + '404': + description: The server cannot find the requested resource. /api/hyperfleet/v1/nodepools: get: operationId: getNodePools @@ -352,6 +352,8 @@ paths: application/json: schema: $ref: '#/components/schemas/NodePoolList' + '400': + description: The server could not understand the request due to invalid syntax. default: description: An unexpected error response. content: @@ -359,6 +361,11 @@ paths: schema: $ref: '#/components/schemas/Error' components: + securitySchemes: + BearerAuth: + type: http + scheme: bearer + parameters: QueryParams.order: name: order @@ -702,7 +709,7 @@ components: default: Cluster name: type: string - minLength: 1 + minLength: 3 maxLength: 63 pattern: ^[a-z0-9-]+$ description: Cluster name (unique) @@ -1194,11 +1201,7 @@ components: - Ready - Failed description: Phase of a resource (Cluster or NodePool) - securitySchemes: - BearerAuth: - type: http - scheme: bearer servers: - - url: http://localhost:8000 - description: Development + - url: https://api.hyperfleet.redhat.com + description: Production variables: {} diff --git a/pkg/api/cluster_types.go b/pkg/api/cluster_types.go index b05b4b6..72fb468 100644 --- a/pkg/api/cluster_types.go +++ b/pkg/api/cluster_types.go @@ -49,9 +49,6 @@ func (c *Cluster) BeforeCreate(tx *gorm.DB) error { c.ID = NewID() c.CreatedTime = now c.UpdatedTime = now - if c.Kind == "" { - c.Kind = "Cluster" - } if c.Generation == 0 { c.Generation = 1 } diff --git a/pkg/api/cluster_types_test.go b/pkg/api/cluster_types_test.go index c5a3b59..6c46458 100644 --- a/pkg/api/cluster_types_test.go +++ b/pkg/api/cluster_types_test.go @@ -67,13 +67,14 @@ func TestCluster_BeforeCreate_IDGeneration(t *testing.T) { Expect(len(cluster.ID)).To(BeNumerically(">", 0)) } -// TestCluster_BeforeCreate_KindDefault tests Kind default value -func TestCluster_BeforeCreate_KindDefault(t *testing.T) { +// TestCluster_BeforeCreate_KindPreservation tests Kind is preserved (not auto-set) +func TestCluster_BeforeCreate_KindPreservation(t *testing.T) { RegisterTestingT(t) - // Test default Kind + // Kind must be set before BeforeCreate (by handler validation) cluster := &Cluster{ Name: "test-cluster", + Kind: "Cluster", } err := cluster.BeforeCreate(nil) @@ -189,6 +190,7 @@ func TestCluster_BeforeCreate_Complete(t *testing.T) { cluster := &Cluster{ Name: "test-cluster", + Kind: "Cluster", // Kind must be set before BeforeCreate } err := cluster.BeforeCreate(nil) @@ -196,7 +198,7 @@ func TestCluster_BeforeCreate_Complete(t *testing.T) { // Verify all defaults Expect(cluster.ID).ToNot(BeEmpty()) - Expect(cluster.Kind).To(Equal("Cluster")) + Expect(cluster.Kind).To(Equal("Cluster")) // Kind is preserved, not auto-set Expect(cluster.Generation).To(Equal(int32(1))) Expect(cluster.StatusPhase).To(Equal("NotReady")) Expect(cluster.Href).To(Equal("/api/hyperfleet/v1/clusters/" + cluster.ID)) diff --git a/pkg/api/node_pool_types.go b/pkg/api/node_pool_types.go index aed130c..b966052 100644 --- a/pkg/api/node_pool_types.go +++ b/pkg/api/node_pool_types.go @@ -55,9 +55,6 @@ func (np *NodePool) BeforeCreate(tx *gorm.DB) error { np.ID = NewID() np.CreatedTime = now np.UpdatedTime = now - if np.Kind == "" { - np.Kind = "NodePool" - } if np.OwnerKind == "" { np.OwnerKind = "Cluster" } diff --git a/pkg/api/node_pool_types_test.go b/pkg/api/node_pool_types_test.go index c747594..72b5bbb 100644 --- a/pkg/api/node_pool_types_test.go +++ b/pkg/api/node_pool_types_test.go @@ -68,14 +68,15 @@ func TestNodePool_BeforeCreate_IDGeneration(t *testing.T) { Expect(len(nodepool.ID)).To(BeNumerically(">", 0)) } -// TestNodePool_BeforeCreate_KindDefault tests Kind default value -func TestNodePool_BeforeCreate_KindDefault(t *testing.T) { +// TestNodePool_BeforeCreate_KindPreservation tests Kind is preserved (not auto-set) +func TestNodePool_BeforeCreate_KindPreservation(t *testing.T) { RegisterTestingT(t) - // Test default Kind + // Kind must be set before BeforeCreate (by handler validation) nodepool := &NodePool{ Name: "test-nodepool", OwnerID: "cluster-123", + Kind: "NodePool", } err := nodepool.BeforeCreate(nil) @@ -230,6 +231,7 @@ func TestNodePool_BeforeCreate_Complete(t *testing.T) { nodepool := &NodePool{ Name: "test-nodepool", OwnerID: "cluster-complete", + Kind: "NodePool", // Kind must be set before BeforeCreate } err := nodepool.BeforeCreate(nil) @@ -237,7 +239,7 @@ func TestNodePool_BeforeCreate_Complete(t *testing.T) { // Verify all defaults Expect(nodepool.ID).ToNot(BeEmpty()) - Expect(nodepool.Kind).To(Equal("NodePool")) + Expect(nodepool.Kind).To(Equal("NodePool")) // Kind is preserved, not auto-set Expect(nodepool.OwnerKind).To(Equal("Cluster")) Expect(nodepool.StatusPhase).To(Equal("NotReady")) Expect(nodepool.Href).To(Equal("/api/hyperfleet/v1/clusters/cluster-complete/nodepools/" + nodepool.ID)) diff --git a/pkg/api/openapi_embed.go b/pkg/api/openapi_embed.go index e3f69f3..289168d 100755 --- a/pkg/api/openapi_embed.go +++ b/pkg/api/openapi_embed.go @@ -12,4 +12,3 @@ var openapiFS embed.FS func GetOpenAPISpec() ([]byte, error) { return fs.ReadFile(openapiFS, "openapi/api/openapi.yaml") } - diff --git a/pkg/api/presenters/adapter_status_test.go b/pkg/api/presenters/adapter_status_test.go index 2503517..c131637 100644 --- a/pkg/api/presenters/adapter_status_test.go +++ b/pkg/api/presenters/adapter_status_test.go @@ -194,7 +194,7 @@ func TestConvertAdapterStatus_ConditionStatusConversion(t *testing.T) { RegisterTestingT(t) testCases := []struct { - openapiStatus openapi.ConditionStatus + openapiStatus openapi.ConditionStatus expectedDomain api.ConditionStatus }{ {openapi.TRUE, api.ConditionTrue}, @@ -361,7 +361,7 @@ func TestPresentAdapterStatus_ConditionStatusConversion(t *testing.T) { RegisterTestingT(t) testCases := []struct { - domainStatus api.ConditionStatus + domainStatus api.ConditionStatus expectedOpenAPI openapi.ConditionStatus }{ {api.ConditionTrue, openapi.TRUE}, diff --git a/pkg/auth/auth_middleware.go b/pkg/auth/auth_middleware.go index 0117f63..59750d1 100755 --- a/pkg/auth/auth_middleware.go +++ b/pkg/auth/auth_middleware.go @@ -4,7 +4,6 @@ import ( "fmt" "net/http" - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" ) diff --git a/pkg/dao/mocks/generic.go b/pkg/dao/mocks/generic.go index a6ff716..c1a9d84 100755 --- a/pkg/dao/mocks/generic.go +++ b/pkg/dao/mocks/generic.go @@ -9,12 +9,12 @@ import ( var _ dao.GenericDao = &genericDaoMock{} type genericDaoMock struct { - preload string - orderBy string - joins string - group string - wheres []dao.Where - model interface{} + preload string + orderBy string + joins string + group string + wheres []dao.Where + model interface{} } func NewGenericDao() *genericDaoMock { diff --git a/pkg/handlers/cluster.go b/pkg/handlers/cluster.go index 4a4887a..3668488 100644 --- a/pkg/handlers/cluster.go +++ b/pkg/handlers/cluster.go @@ -33,6 +33,8 @@ func (h clusterHandler) Create(w http.ResponseWriter, r *http.Request) { &req, []validate{ validateEmpty(&req, "Id", "id"), + validateName(&req, "Name", "name", 3, 63), + validateKind(&req, "Kind", "kind", "Cluster"), }, func() (interface{}, *errors.ServiceError) { ctx := r.Context() diff --git a/pkg/handlers/cluster_nodepools.go b/pkg/handlers/cluster_nodepools.go index 3797946..14cf484 100644 --- a/pkg/handlers/cluster_nodepools.go +++ b/pkg/handlers/cluster_nodepools.go @@ -62,10 +62,10 @@ func (h clusterNodePoolsHandler) List(w http.ResponseWriter, r *http.Request) { } nodePoolList := struct { - Kind string `json:"kind"` - Page int32 `json:"page"` - Size int32 `json:"size"` - Total int32 `json:"total"` + Kind string `json:"kind"` + Page int32 `json:"page"` + Size int32 `json:"size"` + Total int32 `json:"total"` Items []openapi.NodePool `json:"items"` }{ Kind: "NodePoolList", @@ -128,6 +128,8 @@ func (h clusterNodePoolsHandler) Create(w http.ResponseWriter, r *http.Request) &req, []validate{ validateEmpty(&req, "Id", "id"), + validateName(&req, "Name", "name", 1, 255), + validateKind(&req, "Kind", "kind", "NodePool"), }, func() (interface{}, *errors.ServiceError) { ctx := r.Context() diff --git a/pkg/handlers/compatibility.go b/pkg/handlers/compatibility.go deleted file mode 100644 index 3626c86..0000000 --- a/pkg/handlers/compatibility.go +++ /dev/null @@ -1,34 +0,0 @@ -package handlers - -import ( - "net/http" - - "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" -) - -type compatibilityHandler struct{} - -func NewCompatibilityHandler() *compatibilityHandler { - return &compatibilityHandler{} -} - -// Get returns API compatibility information -func (h compatibilityHandler) Get(w http.ResponseWriter, r *http.Request) { - cfg := &handlerConfig{ - Action: func() (interface{}, *errors.ServiceError) { - response := map[string]interface{}{ - "api_version": "v1", - "compatible": true, - "features": []string{ - "clusters", - "nodepools", - "adapter_status", - "status_aggregation", - }, - } - return response, nil - }, - } - - handleGet(w, r, cfg) -} diff --git a/pkg/handlers/node_pool.go b/pkg/handlers/node_pool.go index b2291f8..8fe51ce 100644 --- a/pkg/handlers/node_pool.go +++ b/pkg/handlers/node_pool.go @@ -116,10 +116,10 @@ func (h nodePoolHandler) List(w http.ResponseWriter, r *http.Request) { } nodePoolList := struct { - Kind string `json:"kind"` - Page int32 `json:"page"` - Size int32 `json:"size"` - Total int32 `json:"total"` + Kind string `json:"kind"` + Page int32 `json:"page"` + Size int32 `json:"size"` + Total int32 `json:"total"` Items []openapi.NodePool `json:"items"` }{ Kind: "NodePoolList", diff --git a/pkg/handlers/validation.go b/pkg/handlers/validation.go index 43d579b..fd67cf0 100755 --- a/pkg/handlers/validation.go +++ b/pkg/handlers/validation.go @@ -2,10 +2,14 @@ package handlers import ( "reflect" + "regexp" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" ) +// Cluster/NodePool name pattern: lowercase alphanumeric and hyphens +var namePattern = regexp.MustCompile(`^[a-z0-9-]+$`) + func validateNotEmpty(i interface{}, fieldName string, field string) validate { return func() *errors.ServiceError { value := reflect.ValueOf(i).Elem().FieldByName(fieldName) @@ -37,3 +41,67 @@ func validateEmpty(i interface{}, fieldName string, field string) validate { return nil } } + +// validateName validates that a name field matches the pattern ^[a-z0-9-]+$ and length constraints +func validateName(i interface{}, fieldName string, field string, minLen, maxLen int) validate { + return func() *errors.ServiceError { + value := reflect.ValueOf(i).Elem().FieldByName(fieldName) + if value.Kind() == reflect.Ptr { + if value.IsNil() { + return errors.Validation("%s is required", field) + } + value = value.Elem() + } + + name := value.String() + + // Check if empty + if len(name) == 0 { + return errors.Validation("%s is required", field) + } + + // Check minimum length + if len(name) < minLen { + return errors.Validation("%s must be at least %d characters", field, minLen) + } + + // Check maximum length + if len(name) > maxLen { + return errors.Validation("%s must be at most %d characters", field, maxLen) + } + + // Check pattern: lowercase alphanumeric and hyphens only + if !namePattern.MatchString(name) { + return errors.Validation("%s must contain only lowercase letters, numbers, and hyphens", field) + } + + return nil + } +} + +// validateKind validates that the kind field matches the expected value +func validateKind(i interface{}, fieldName string, field string, expectedKind string) validate { + return func() *errors.ServiceError { + value := reflect.ValueOf(i).Elem().FieldByName(fieldName) + if value.Kind() == reflect.Ptr { + if value.IsNil() { + return errors.Validation("%s is required", field) + } + value = value.Elem() + } + + kind := value.String() + + // Check if empty + if len(kind) == 0 { + return errors.Validation("%s is required", field) + } + + // Check if matches expected kind + if kind != expectedKind { + return errors.Validation("%s must be '%s'", field, expectedKind) + } + + return nil + } +} diff --git a/pkg/handlers/validation_test.go b/pkg/handlers/validation_test.go new file mode 100644 index 0000000..42b315b --- /dev/null +++ b/pkg/handlers/validation_test.go @@ -0,0 +1,147 @@ +package handlers + +import ( + "testing" + + . "github.com/onsi/gomega" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" +) + +func TestValidateName_Valid(t *testing.T) { + RegisterTestingT(t) + + validNames := []string{ + "test", + "test-cluster", + "my-cluster-123", + "123", + "test-123-cluster", + "a1b2c3", + "abc", + } + + for _, name := range validNames { + req := openapi.ClusterCreateRequest{ + Name: name, + } + validator := validateName(&req, "Name", "name", 3, 63) + err := validator() + Expect(err).To(BeNil(), "Expected name '%s' to be valid", name) + } +} + +func TestValidateName_TooShort(t *testing.T) { + RegisterTestingT(t) + + shortNames := []string{ + "", // empty + "a", // 1 char + "ab", // 2 chars + } + + for _, name := range shortNames { + req := openapi.ClusterCreateRequest{ + Name: name, + } + validator := validateName(&req, "Name", "name", 3, 63) + err := validator() + Expect(err).ToNot(BeNil(), "Expected name '%s' to be invalid (too short)", name) + if name == "" { + Expect(err.Reason).To(ContainSubstring("required")) + } else { + Expect(err.Reason).To(ContainSubstring("at least 3 characters")) + } + } +} + +func TestValidateName_TooLong(t *testing.T) { + RegisterTestingT(t) + + req := openapi.ClusterCreateRequest{ + Name: "this-is-a-very-long-name-that-exceeds-the-maximum-allowed-length-for-cluster-names", + } + validator := validateName(&req, "Name", "name", 3, 63) + err := validator() + Expect(err).ToNot(BeNil()) + Expect(err.Reason).To(ContainSubstring("at most 63 characters")) +} + +func TestValidateName_InvalidCharacters(t *testing.T) { + RegisterTestingT(t) + + invalidNames := []string{ + "TEST", // uppercase + "Test", // mixed case + "test_cluster", // underscore + "test.cluster", // dot + "test cluster", // space + "test@cluster", // special char + "test/cluster", // slash + "test\\cluster", // backslash + } + + for _, name := range invalidNames { + req := openapi.ClusterCreateRequest{ + Name: name, + } + validator := validateName(&req, "Name", "name", 3, 63) + err := validator() + Expect(err).ToNot(BeNil(), "Expected name '%s' to be invalid", name) + Expect(err.Reason).To(ContainSubstring("lowercase letters, numbers, and hyphens")) + } +} + +func TestValidateKind_Valid(t *testing.T) { + RegisterTestingT(t) + + req := openapi.ClusterCreateRequest{ + Kind: "Cluster", + } + validator := validateKind(&req, "Kind", "kind", "Cluster") + err := validator() + Expect(err).To(BeNil()) +} + +func TestValidateKind_Invalid(t *testing.T) { + RegisterTestingT(t) + + invalidKinds := []string{ + "cluster", // lowercase + "CLUSTER", // uppercase + "NodePool", // wrong kind + "", // empty + } + + for _, kind := range invalidKinds { + req := openapi.ClusterCreateRequest{ + Kind: kind, + } + validator := validateKind(&req, "Kind", "kind", "Cluster") + err := validator() + Expect(err).ToNot(BeNil(), "Expected kind '%s' to be invalid", kind) + } +} + +func TestValidateKind_Empty(t *testing.T) { + RegisterTestingT(t) + + req := openapi.ClusterCreateRequest{ + Kind: "", + } + validator := validateKind(&req, "Kind", "kind", "Cluster") + err := validator() + Expect(err).ToNot(BeNil()) + Expect(err.Reason).To(ContainSubstring("required")) +} + +func TestValidateKind_WrongKind(t *testing.T) { + RegisterTestingT(t) + + req := openapi.ClusterCreateRequest{ + Kind: "WrongKind", + } + validator := validateKind(&req, "Kind", "kind", "Cluster") + err := validator() + Expect(err).ToNot(BeNil()) + Expect(err.Reason).To(ContainSubstring("must be 'Cluster'")) +} diff --git a/pkg/logger/logger.go b/pkg/logger/logger.go index cde256f..dafca42 100755 --- a/pkg/logger/logger.go +++ b/pkg/logger/logger.go @@ -28,8 +28,8 @@ type logger struct { level int32 accountID string // TODO username is unused, should we be logging it? Could be pii - username string - extra extra + username string + extra extra } // NewOCMLogger creates a new logger instance with a default verbosity of 1 @@ -125,4 +125,3 @@ func (l *logger) log(message string, glogFunc func(args ...interface{})) { prefixed := l.prepareLogPrefix(message, l.extra) glogFunc(prefixed) } - diff --git a/plugins/clusters/plugin.go b/plugins/clusters/plugin.go index cf1ea41..00cc2f1 100644 --- a/plugins/clusters/plugin.go +++ b/plugins/clusters/plugin.go @@ -42,7 +42,6 @@ func Service(s *environments.Services) services.ClusterService { return nil } - func init() { // Service registration registry.RegisterService("Clusters", func(env interface{}) interface{} { diff --git a/plugins/nodePools/plugin.go b/plugins/nodePools/plugin.go index e82aa00..4baee62 100644 --- a/plugins/nodePools/plugin.go +++ b/plugins/nodePools/plugin.go @@ -40,7 +40,6 @@ func Service(s *environments.Services) services.NodePoolService { return nil } - func init() { // Service registration registry.RegisterService("NodePools", func(env interface{}) interface{} { diff --git a/test/helper.go b/test/helper.go index 799ca63..f79b9fa 100755 --- a/test/helper.go +++ b/test/helper.go @@ -243,6 +243,13 @@ func (helper *Helper) HealthCheckURL(path string) string { func (helper *Helper) NewApiClient() *openapi.APIClient { config := openapi.NewConfiguration() + // Override the server URL to use the local test server + protocol := "http" + if helper.AppConfig.Server.EnableHTTPS { + protocol = "https" + } + config.Host = helper.AppConfig.Server.BindAddress + config.Scheme = protocol client := openapi.NewAPIClient(config) return client } diff --git a/test/integration/api_contract_test.go b/test/integration/api_contract_test.go new file mode 100644 index 0000000..8592e7e --- /dev/null +++ b/test/integration/api_contract_test.go @@ -0,0 +1,40 @@ +package integration + +import ( + "testing" + + . "github.com/onsi/gomega" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" +) + +// TestAPIContract_ResourcePhaseConstants verifies that domain and OpenAPI constants are in sync +// This test ensures that pkg/api.ResourcePhase constants match the generated openapi.ResourcePhase constants +func TestAPIContract_ResourcePhaseConstants(t *testing.T) { + RegisterTestingT(t) + + // Verify domain constants match OpenAPI generated constants + // If OpenAPI spec changes, this test will fail and alert us to update domain constants + Expect(string(api.PhaseNotReady)).To(Equal(string(openapi.NOT_READY)), + "api.PhaseNotReady must match openapi.NOT_READY") + Expect(string(api.PhaseReady)).To(Equal(string(openapi.READY)), + "api.PhaseReady must match openapi.READY") + Expect(string(api.PhaseFailed)).To(Equal(string(openapi.FAILED)), + "api.PhaseFailed must match openapi.FAILED") +} + +// TestAPIContract_ConditionStatusConstants verifies that domain and OpenAPI constants are in sync +// This test ensures that pkg/api.ConditionStatus constants match the generated openapi.ConditionStatus constants +func TestAPIContract_ConditionStatusConstants(t *testing.T) { + RegisterTestingT(t) + + // Verify domain constants match OpenAPI generated constants + // If OpenAPI spec changes, this test will fail and alert us to update domain constants + Expect(string(api.ConditionTrue)).To(Equal(string(openapi.TRUE)), + "api.ConditionTrue must match openapi.TRUE") + Expect(string(api.ConditionFalse)).To(Equal(string(openapi.FALSE)), + "api.ConditionFalse must match openapi.FALSE") + Expect(string(api.ConditionUnknown)).To(Equal(string(openapi.UNKNOWN)), + "api.ConditionUnknown must match openapi.UNKNOWN") +} diff --git a/test/integration/clusters_test.go b/test/integration/clusters_test.go index 700a57d..0a0a656 100644 --- a/test/integration/clusters_test.go +++ b/test/integration/clusters_test.go @@ -231,7 +231,7 @@ func TestClusterBoundaryValues(t *testing.T) { } _, resp, err = client.DefaultAPI.PostCluster(ctx).ClusterCreateRequest(tooLongInput).Execute() Expect(err).To(HaveOccurred(), "Should reject name exceeding 63 characters") - Expect(resp.StatusCode).To(Equal(http.StatusInternalServerError)) + Expect(resp.StatusCode).To(Equal(http.StatusBadRequest)) // Test 2: Empty name emptyNameInput := openapi.ClusterCreateRequest{ @@ -241,11 +241,9 @@ func TestClusterBoundaryValues(t *testing.T) { } _, resp, err = client.DefaultAPI.PostCluster(ctx).ClusterCreateRequest(emptyNameInput).Execute() - // Should either accept empty name or return 400 - if resp != nil { - t.Logf("Empty name test returned status: %d", resp.StatusCode) - } - Expect(err).ToNot(HaveOccurred()) + // Empty name should be rejected with 400 + Expect(err).To(HaveOccurred(), "Should reject empty name") + Expect(resp.StatusCode).To(Equal(http.StatusBadRequest)) // Test 3: Large spec JSON (test with ~10KB JSON) largeSpec := make(map[string]interface{}) @@ -268,17 +266,16 @@ func TestClusterBoundaryValues(t *testing.T) { Expect(err).NotTo(HaveOccurred()) Expect(len(retrieved.Spec)).To(Equal(100)) - // Test 4: Unicode in name + // Test 4: Unicode in name (should be rejected - pattern only allows [a-z0-9-]) unicodeNameInput := openapi.ClusterCreateRequest{ Kind: "Cluster", Name: "テスト-δοκιμή-🚀", Spec: map[string]interface{}{"test": "spec"}, } - unicodeNameCluster, resp, err := client.DefaultAPI.PostCluster(ctx).ClusterCreateRequest(unicodeNameInput).Execute() - Expect(err).NotTo(HaveOccurred(), "Should accept unicode in name") - Expect(resp.StatusCode).To(Equal(http.StatusCreated)) - Expect(unicodeNameCluster.Name).To(Equal("テスト-δοκιμή-🚀")) + _, resp, err = client.DefaultAPI.PostCluster(ctx).ClusterCreateRequest(unicodeNameInput).Execute() + Expect(err).To(HaveOccurred(), "Should reject unicode in name (pattern is ^[a-z0-9-]+$)") + Expect(resp.StatusCode).To(Equal(http.StatusBadRequest)) } // TestClusterSchemaValidation tests schema validation for cluster specs @@ -485,7 +482,7 @@ func TestClusterList_DefaultSorting(t *testing.T) { for i := 1; i <= 3; i++ { clusterInput := openapi.ClusterCreateRequest{ Kind: "Cluster", - Name: fmt.Sprintf("sort-test-%d-%s", i, h.NewID()), + Name: fmt.Sprintf("sort-test-%d-%s", i, strings.ToLower(h.NewID())), Spec: map[string]interface{}{"test": fmt.Sprintf("value-%d", i)}, } @@ -532,7 +529,7 @@ func TestClusterList_OrderByName(t *testing.T) { ctx := h.NewAuthenticatedContext(account) // Create clusters with names that will sort alphabetically - testPrefix := fmt.Sprintf("name-sort-%s", h.NewID()) + testPrefix := fmt.Sprintf("name-sort-%s", strings.ToLower(h.NewID())) names := []string{ fmt.Sprintf("%s-charlie", testPrefix), fmt.Sprintf("%s-alpha", testPrefix), @@ -581,7 +578,7 @@ func TestClusterList_OrderByNameDesc(t *testing.T) { ctx := h.NewAuthenticatedContext(account) // Create clusters with names that will sort alphabetically - testPrefix := fmt.Sprintf("desc-sort-%s", h.NewID()) + testPrefix := fmt.Sprintf("desc-sort-%s", strings.ToLower(h.NewID())) names := []string{ fmt.Sprintf("%s-alpha", testPrefix), fmt.Sprintf("%s-charlie", testPrefix), @@ -620,3 +617,73 @@ func TestClusterList_OrderByNameDesc(t *testing.T) { t.Logf("✓ Descending sorting works: clusters sorted by name desc") } + +// TestClusterPost_EmptyKind tests that empty kind field returns 400 +func TestClusterPost_EmptyKind(t *testing.T) { + h, _ := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + jwtToken := ctx.Value(openapi.ContextAccessToken) + + // Send request with empty kind + invalidInput := `{ + "kind": "", + "name": "test-cluster", + "spec": {} + }` + + restyResp, err := resty.R(). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). + SetBody(invalidInput). + Post(h.RestURL("/clusters")) + + Expect(err).ToNot(HaveOccurred()) + Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest)) + + // Parse error response + var errorResponse map[string]interface{} + err = json.Unmarshal(restyResp.Body(), &errorResponse) + Expect(err).ToNot(HaveOccurred()) + + // Verify error message contains "kind is required" + reason, ok := errorResponse["reason"].(string) + Expect(ok).To(BeTrue()) + Expect(reason).To(ContainSubstring("kind is required")) +} + +// TestClusterPost_WrongKind tests that wrong kind field returns 400 +func TestClusterPost_WrongKind(t *testing.T) { + h, _ := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + jwtToken := ctx.Value(openapi.ContextAccessToken) + + // Send request with wrong kind + invalidInput := `{ + "kind": "NodePool", + "name": "test-cluster", + "spec": {} + }` + + restyResp, err := resty.R(). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). + SetBody(invalidInput). + Post(h.RestURL("/clusters")) + + Expect(err).ToNot(HaveOccurred()) + Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest)) + + // Parse error response + var errorResponse map[string]interface{} + err = json.Unmarshal(restyResp.Body(), &errorResponse) + Expect(err).ToNot(HaveOccurred()) + + // Verify error message contains "kind must be 'Cluster'" + reason, ok := errorResponse["reason"].(string) + Expect(ok).To(BeTrue()) + Expect(reason).To(ContainSubstring("kind must be 'Cluster'")) +} diff --git a/test/integration/compatibility_test.go b/test/integration/compatibility_test.go deleted file mode 100644 index e8a7f6c..0000000 --- a/test/integration/compatibility_test.go +++ /dev/null @@ -1,34 +0,0 @@ -package integration - -import ( - "context" - "net/http" - "testing" - - . "github.com/onsi/gomega" - - "github.com/openshift-hyperfleet/hyperfleet-api/test" -) - -// TestCompatibilityGet tests the compatibility endpoint -func TestCompatibilityGet(t *testing.T) { - h, client := test.RegisterIntegration(t) - - account := h.NewRandAccount() - ctx := h.NewAuthenticatedContext(account) - - // GET /api/hyperfleet/v1/compatibility - result, resp, err := client.DefaultAPI.GetCompatibility(ctx).Execute() - Expect(err).NotTo(HaveOccurred(), "Error getting compatibility: %v", err) - Expect(resp.StatusCode).To(Equal(http.StatusOK)) - Expect(result).NotTo(BeEmpty()) -} - -// TestCompatibilityNoAuth tests that compatibility endpoint requires authentication -func TestCompatibilityNoAuth(t *testing.T) { - _, client := test.RegisterIntegration(t) - - // Try to get compatibility without authentication - _, _, err := client.DefaultAPI.GetCompatibility(context.Background()).Execute() - Expect(err).To(HaveOccurred(), "Expected authentication error") -} diff --git a/test/integration/integration_test.go b/test/integration/integration_test.go index b8fe42a..d0b68ff 100755 --- a/test/integration/integration_test.go +++ b/test/integration/integration_test.go @@ -27,9 +27,9 @@ func TestMain(m *testing.M) { } else { // filename is like: /path/to/repo/test/integration/integration_test.go // Navigate up: integration_test.go -> integration -> test -> repo - integrationDir := filepath.Dir(filename) // /path/to/repo/test/integration - testDir := filepath.Dir(integrationDir) // /path/to/repo/test - repoRoot := filepath.Dir(testDir) // /path/to/repo + integrationDir := filepath.Dir(filename) // /path/to/repo/test/integration + 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") diff --git a/test/integration/node_pools_test.go b/test/integration/node_pools_test.go index 5623104..62bbc9c 100644 --- a/test/integration/node_pools_test.go +++ b/test/integration/node_pools_test.go @@ -1,6 +1,7 @@ package integration import ( + "encoding/json" "fmt" "net/http" "testing" @@ -58,6 +59,7 @@ func TestNodePoolPost(t *testing.T) { // POST responses per openapi spec: 201, 409, 500 nodePoolInput := openapi.NodePoolCreateRequest{ + Kind: openapi.PtrString("NodePool"), Name: "test-name", Spec: map[string]interface{}{"test": "spec"}, } @@ -166,6 +168,7 @@ func TestGetNodePoolByClusterIdAndNodePoolId(t *testing.T) { // Create a nodepool for this cluster using the API nodePoolInput := openapi.NodePoolCreateRequest{ + Kind: openapi.PtrString("NodePool"), Name: "test-nodepool-get", Spec: map[string]interface{}{"instance_type": "m5.large", "replicas": 2}, } @@ -203,3 +206,81 @@ func TestGetNodePoolByClusterIdAndNodePoolId(t *testing.T) { Expect(err).To(HaveOccurred(), "Expected 404 when accessing nodepool from wrong cluster") Expect(resp.StatusCode).To(Equal(http.StatusNotFound)) } + +// TestNodePoolPost_EmptyKind tests that empty kind field returns 400 +func TestNodePoolPost_EmptyKind(t *testing.T) { + h, _ := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + jwtToken := ctx.Value(openapi.ContextAccessToken) + + // Create a cluster first + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + + // Send request with empty kind + invalidInput := `{ + "kind": "", + "name": "test-nodepool", + "spec": {} + }` + + restyResp, err := resty.R(). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). + SetBody(invalidInput). + Post(h.RestURL(fmt.Sprintf("/clusters/%s/nodepools", cluster.ID))) + + Expect(err).ToNot(HaveOccurred()) + Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest)) + + // Parse error response + var errorResponse map[string]interface{} + err = json.Unmarshal(restyResp.Body(), &errorResponse) + Expect(err).ToNot(HaveOccurred()) + + // Verify error message contains "kind is required" + reason, ok := errorResponse["reason"].(string) + Expect(ok).To(BeTrue()) + Expect(reason).To(ContainSubstring("kind is required")) +} + +// TestNodePoolPost_WrongKind tests that wrong kind field returns 400 +func TestNodePoolPost_WrongKind(t *testing.T) { + h, _ := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + jwtToken := ctx.Value(openapi.ContextAccessToken) + + // Create a cluster first + cluster, err := h.Factories.NewClusters(h.NewID()) + Expect(err).NotTo(HaveOccurred()) + + // Send request with wrong kind + invalidInput := `{ + "kind": "Cluster", + "name": "test-nodepool", + "spec": {} + }` + + restyResp, err := resty.R(). + SetHeader("Content-Type", "application/json"). + SetHeader("Authorization", fmt.Sprintf("Bearer %s", jwtToken)). + SetBody(invalidInput). + Post(h.RestURL(fmt.Sprintf("/clusters/%s/nodepools", cluster.ID))) + + Expect(err).ToNot(HaveOccurred()) + Expect(restyResp.StatusCode()).To(Equal(http.StatusBadRequest)) + + // Parse error response + var errorResponse map[string]interface{} + err = json.Unmarshal(restyResp.Body(), &errorResponse) + Expect(err).ToNot(HaveOccurred()) + + // Verify error message contains "kind must be 'NodePool'" + reason, ok := errorResponse["reason"].(string) + Expect(ok).To(BeTrue()) + Expect(reason).To(ContainSubstring("kind must be 'NodePool'")) +}