From 61892d15c82e6bddc5183c374d4dcf255f47e39e Mon Sep 17 00:00:00 2001 From: yasun Date: Wed, 10 Dec 2025 13:27:32 +0800 Subject: [PATCH] [HYPERFLEET-321] feat: add user-friendly search syntax and lowercase Base32 ID encoding update update update update --- README.md | 32 +- openapi/openapi.yaml | 6 +- pkg/api/resource_id.go | 19 +- pkg/api/resource_id_test.go | 67 ++++ .../migrations/202511111044_add_clusters.go | 9 + .../migrations/202511111055_add_node_pools.go | 9 + pkg/db/sql_helpers.go | 48 ++- pkg/handlers/validation.go | 7 +- pkg/handlers/validation_test.go | 3 + pkg/services/generic.go | 26 +- pkg/services/generic_test.go | 32 +- test/factories/clusters.go | 91 ++++++ test/factories/factory.go | 16 +- test/factories/node_pools.go | 91 ++++++ test/integration/search_field_mapping_test.go | 285 ++++++++++++++++++ 15 files changed, 717 insertions(+), 24 deletions(-) create mode 100644 pkg/api/resource_id_test.go create mode 100644 test/integration/search_field_mapping_test.go diff --git a/README.md b/README.md index 3ad1d9c..1152c58 100755 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ HyperFleet API - Simple REST API for cluster lifecycle management. Provides CRUD ### Technology Stack -- **Language**: Go 1.24.9 +- **Language**: Go 1.24 or higher - **API Definition**: TypeSpec → OpenAPI 3.0.3 - **Code Generation**: openapi-generator-cli v7.16.0 - **Database**: PostgreSQL with GORM ORM @@ -30,7 +30,7 @@ HyperFleet API - Simple REST API for cluster lifecycle management. Provides CRUD ``` hyperfleet-api/ -├── cmd/hyperfleet/ # Application entry point +├── cmd/hyperfleet-api/ # Application entry point ├── pkg/ │ ├── api/ # API models and handlers │ │ ├── openapi/ # Generated Go models from OpenAPI @@ -181,8 +181,24 @@ All list endpoints return consistent pagination metadata: - `?page=N` - Page number (default: 1) - `?pageSize=N` - Items per page (default: 100) -**Search Parameters (clusters only):** -- `?search=name='cluster-name'` - Filter by name +**Search Parameters:** +- Uses TSL (Tree Search Language) query syntax +- Supported fields: `name`, `status.phase`, `labels.` +- Supported operators: `=`, `in`, `and`, `or` +- Examples: + ```bash + # Simple query + curl -G http://localhost:8000/api/hyperfleet/v1/clusters \ + --data-urlencode "search=name='my-cluster'" + + # AND query + curl -G http://localhost:8000/api/hyperfleet/v1/clusters \ + --data-urlencode "search=status.phase='Ready' and labels.env='production'" + + # OR query + curl -G http://localhost:8000/api/hyperfleet/v1/clusters \ + --data-urlencode "search=labels.env='dev' or labels.env='staging'" + ``` ## Development Workflow @@ -541,6 +557,14 @@ curl -X POST http://localhost:8000/api/hyperfleet/v1/clusters/$CLUSTER_ID/status # 4. Get cluster with aggregated status curl http://localhost:8000/api/hyperfleet/v1/clusters/$CLUSTER_ID | jq + +# 5. Search with AND condition +curl -G http://localhost:8000/api/hyperfleet/v1/clusters \ + --data-urlencode "search=status.phase='Ready' and labels.env='production'" | jq + +# 6. Search with OR condition +curl -G http://localhost:8000/api/hyperfleet/v1/clusters \ + --data-urlencode "search=labels.env='dev' or labels.env='staging'" | jq ``` ## License diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index b28a8ba..08665c8 100644 --- a/openapi/openapi.yaml +++ b/openapi/openapi.yaml @@ -407,6 +407,10 @@ components: schema: type: string explode: false + description: | + Filter results using TSL (Tree Search Language) query syntax. + + Examples: `status.phase='NotReady'`, `name in ('c1','c2')`, `labels.region='us-east'` schemas: APIResource: type: object @@ -711,7 +715,7 @@ components: type: string minLength: 3 maxLength: 63 - pattern: ^[a-z0-9-]+$ + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ description: Cluster name (unique) spec: allOf: diff --git a/pkg/api/resource_id.go b/pkg/api/resource_id.go index cdd71af..dd93738 100755 --- a/pkg/api/resource_id.go +++ b/pkg/api/resource_id.go @@ -1,7 +1,22 @@ package api -import "github.com/segmentio/ksuid" +import ( + "encoding/base32" + "github.com/segmentio/ksuid" +) + +// NewID generates a new unique identifier using KSUID with lowercase Base32 encoding. +// The resulting 32-character lowercase string is compatible with Kubernetes DNS-1123 +// subdomain naming requirements, making it suitable for use as Kubernetes resource names +// and labels. The KSUID provides time-based ordering (second precision) and global +// uniqueness without requiring a central server. func NewID() string { - return ksuid.New().String() + return uidEncoding.EncodeToString(ksuid.New().Bytes()) } + +// uidAlphabet is the lowercase alphabet used to encode unique identifiers. +const uidAlphabet = "0123456789abcdefghijklmnopqrstuv" + +// uidEncoding is the lowercase variant of Base32 used to encode unique identifiers. +var uidEncoding = base32.NewEncoding(uidAlphabet).WithPadding(base32.NoPadding) diff --git a/pkg/api/resource_id_test.go b/pkg/api/resource_id_test.go new file mode 100644 index 0000000..58a3575 --- /dev/null +++ b/pkg/api/resource_id_test.go @@ -0,0 +1,67 @@ +package api + +import ( + "regexp" + "testing" +) + +func TestNewID(t *testing.T) { + // Generate multiple IDs to test uniqueness, format, and length + ids := make(map[string]bool) + for i := 0; i < 100; i++ { + id := NewID() + + // Verify length is 32 characters + if len(id) != 32 { + t.Errorf("Expected ID length 32, got %d: %s", len(id), id) + } + + // Verify only lowercase letters (a-v) and digits (0-9) + if !regexp.MustCompile(`^[0-9a-v]{32}$`).MatchString(id) { + t.Errorf("ID contains invalid characters (should be lowercase 0-9a-v): %s", id) + } + + // Verify uniqueness + if ids[id] { + t.Errorf("Duplicate ID generated: %s", id) + } + ids[id] = true + } +} + +func TestNewID_TimeOrdering(t *testing.T) { + // KSUID uses second-level timestamps, so IDs generated within the same second + // will have the same timestamp prefix. Time ordering is only guaranteed for IDs + // generated in different seconds. + id1 := NewID() + + // For practical testing, we verify consistency and uniqueness within the same second + // rather than waiting for the next second (which would slow down tests significantly). + // In production, most ID generations will be more than 1 second apart. + id2 := NewID() + + if len(id1) != 32 || len(id2) != 32 { + t.Errorf("IDs should have consistent length of 32") + } + + // Verify ID uniqueness even within the same second + if id1 == id2 { + t.Errorf("IDs should be unique even within the same second: %s == %s", id1, id2) + } +} + +func TestNewID_K8sCompatible(t *testing.T) { + id := NewID() + + // Verify DNS-1123 subdomain compatibility: + // - Must contain only lowercase letters, digits, '-', and '.' + // - Must start and end with alphanumeric characters + // - Maximum length is 253 characters + // + // Our IDs contain only lowercase letters (a-v) and digits (0-9), with a fixed + // length of 32 characters, so they are fully compatible. + dns1123Pattern := regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`) + if !dns1123Pattern.MatchString(id) { + t.Errorf("ID is not DNS-1123 subdomain compatible: %s", id) + } +} diff --git a/pkg/db/migrations/202511111044_add_clusters.go b/pkg/db/migrations/202511111044_add_clusters.go index 3145534..4e2e90b 100644 --- a/pkg/db/migrations/202511111044_add_clusters.go +++ b/pkg/db/migrations/202511111044_add_clusters.go @@ -67,10 +67,19 @@ func addClusters() *gormigrate.Migration { return err } + // Create index on status_last_updated_time for search optimization + // Sentinel queries frequently filter by this field to find stale resources + if err := tx.Exec("CREATE INDEX IF NOT EXISTS idx_clusters_status_last_updated_time ON clusters(status_last_updated_time);").Error; err != nil { + return err + } + return nil }, Rollback: func(tx *gorm.DB) error { // Drop indexes first + if err := tx.Exec("DROP INDEX IF EXISTS idx_clusters_status_last_updated_time;").Error; err != nil { + return err + } if err := tx.Exec("DROP INDEX IF EXISTS idx_clusters_status_phase;").Error; err != nil { return err } diff --git a/pkg/db/migrations/202511111055_add_node_pools.go b/pkg/db/migrations/202511111055_add_node_pools.go index d40411e..e19ea37 100644 --- a/pkg/db/migrations/202511111055_add_node_pools.go +++ b/pkg/db/migrations/202511111055_add_node_pools.go @@ -70,6 +70,12 @@ func addNodePools() *gormigrate.Migration { return err } + // Create index on status_last_updated_time for search optimization + // Sentinel queries frequently filter by this field to find stale resources + if err := tx.Exec("CREATE INDEX IF NOT EXISTS idx_node_pools_status_last_updated_time ON node_pools(status_last_updated_time);").Error; err != nil { + return err + } + // Add foreign key constraint to clusters addFKSQL := ` ALTER TABLE node_pools @@ -90,6 +96,9 @@ func addNodePools() *gormigrate.Migration { } // Drop indexes + if err := tx.Exec("DROP INDEX IF EXISTS idx_node_pools_status_last_updated_time;").Error; err != nil { + return err + } if err := tx.Exec("DROP INDEX IF EXISTS idx_node_pools_status_phase;").Error; err != nil { return err } diff --git a/pkg/db/sql_helpers.go b/pkg/db/sql_helpers.go index 82afb3d..d3eb450 100755 --- a/pkg/db/sql_helpers.go +++ b/pkg/db/sql_helpers.go @@ -3,6 +3,7 @@ package db import ( "fmt" "reflect" + "regexp" "strings" "github.com/jinzhu/inflection" @@ -11,6 +12,23 @@ import ( "gorm.io/gorm" ) +// Label key validation pattern: only lowercase letters, digits, and underscores to prevent SQL injection +var labelKeyPattern = regexp.MustCompile(`^[a-z0-9_]+$`) + +// validateLabelKey validates a label key to prevent SQL injection +// through field name interpolation. Only allows lowercase letters, digits, and underscores. +func validateLabelKey(key string) *errors.ServiceError { + if key == "" { + return errors.BadRequest("label key cannot be empty") + } + + if !labelKeyPattern.MatchString(key) { + return errors.BadRequest("label key '%s' is invalid: must contain only lowercase letters, digits, and underscores", key) + } + + return nil +} + // Check if a field name starts with properties. func startsWithProperties(s string) bool { return strings.HasPrefix(s, "properties.") @@ -33,6 +51,15 @@ func hasProperty(n tsl.Node) bool { return true } +// Field mapping rules for user-friendly syntax to database columns +var statusFieldMappings = map[string]string{ + "status.last_updated_time": "status_last_updated_time", + "status.last_transition_time": "status_last_transition_time", + "status.phase": "status_phase", + "status.observed_generation": "status_observed_generation", + "status.conditions": "status_conditions", +} + // getField gets the sql field associated with a name. func getField(name string, disallowedFields map[string]string) (field string, err *errors.ServiceError) { // We want to accept names with trailing and leading spaces @@ -44,6 +71,25 @@ func getField(name string, disallowedFields map[string]string) (field string, er return } + // Map user-friendly labels.xxx syntax to JSONB query: labels->>'xxx' + if strings.HasPrefix(trimmedName, "labels.") { + key := strings.TrimPrefix(trimmedName, "labels.") + + // Validate label key to prevent SQL injection + if validationErr := validateLabelKey(key); validationErr != nil { + err = validationErr + return + } + + field = fmt.Sprintf("labels->>'%s'", key) + return + } + + // Map user-friendly status.xxx syntax to database columns + if mapped, ok := statusFieldMappings[trimmedName]; ok { + trimmedName = mapped + } + // Check for nested field, e.g., subscription_labels.key checkName := trimmedName fieldParts := strings.Split(trimmedName, ".") @@ -55,7 +101,7 @@ func getField(name string, disallowedFields map[string]string) (field string, er checkName = fieldParts[1] } - // Check for allowed fields + // Check for disallowed fields _, ok := disallowedFields[checkName] if ok { err = errors.BadRequest("%s is not a valid field name", name) diff --git a/pkg/handlers/validation.go b/pkg/handlers/validation.go index fd67cf0..097ea1e 100755 --- a/pkg/handlers/validation.go +++ b/pkg/handlers/validation.go @@ -7,8 +7,9 @@ import ( "github.com/openshift-hyperfleet/hyperfleet-api/pkg/errors" ) -// Cluster/NodePool name pattern: lowercase alphanumeric and hyphens -var namePattern = regexp.MustCompile(`^[a-z0-9-]+$`) +// Cluster/NodePool name pattern: compliant with Kubernetes DNS Subdomain Names (RFC 1123) +// Must start and end with alphanumeric, can contain hyphens in the middle +var namePattern = regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`) func validateNotEmpty(i interface{}, fieldName string, field string) validate { return func() *errors.ServiceError { @@ -72,7 +73,7 @@ func validateName(i interface{}, fieldName string, field string, minLen, 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 errors.Validation("%s must start and end with lowercase letter or number, and contain only lowercase letters, numbers, and hyphens", field) } return nil diff --git a/pkg/handlers/validation_test.go b/pkg/handlers/validation_test.go index 42b315b..57b3c9a 100644 --- a/pkg/handlers/validation_test.go +++ b/pkg/handlers/validation_test.go @@ -78,6 +78,9 @@ func TestValidateName_InvalidCharacters(t *testing.T) { "test@cluster", // special char "test/cluster", // slash "test\\cluster", // backslash + "-test", // starts with hyphen + "test-", // ends with hyphen + "-test-", // starts and ends with hyphen } for _, name := range invalidNames { diff --git a/pkg/services/generic.go b/pkg/services/generic.go index c0714c2..1192bd3 100755 --- a/pkg/services/generic.go +++ b/pkg/services/generic.go @@ -38,8 +38,15 @@ type sqlGenericService struct { } var ( - SearchDisallowedFields = map[string]map[string]string{} - allFieldsAllowed = map[string]string{} + SearchDisallowedFields = map[string]map[string]string{ + "Cluster": { + "spec": "spec", // Provider-specific field, not searchable + }, + "NodePool": { + "spec": "spec", // Provider-specific field, not searchable + }, + } + allFieldsAllowed = map[string]string{} ) // wrap all needed pieces for the LIST function @@ -161,8 +168,14 @@ func (s *sqlGenericService) buildSearchValues(listCtx *listContext, d *dao.Gener if err != nil { return "", nil, errors.BadRequest("Failed to parse search query: %s", listCtx.args.Search) } + // apply field name mapping first (status.xxx -> status_xxx, labels.xxx -> labels->>'xxx') + // this must happen before treeWalkForRelatedTables to prevent treating "status" and "labels" as related resources + tslTree, serviceErr := db.FieldNameWalk(tslTree, *listCtx.disallowedFields) + if serviceErr != nil { + return "", nil, serviceErr + } // find all related tables - tslTree, serviceErr := s.treeWalkForRelatedTables(listCtx, tslTree, d) + tslTree, serviceErr = s.treeWalkForRelatedTables(listCtx, tslTree, d) if serviceErr != nil { return "", nil, serviceErr } @@ -332,11 +345,8 @@ func (s *sqlGenericService) treeWalkForAddingTableName(listCtx *listContext, tsl } func (s *sqlGenericService) treeWalkForSqlizer(listCtx *listContext, tslTree tsl.Node) (squirrel.Sqlizer, *errors.ServiceError) { - // Check field names in tree - tslTree, serviceErr := db.FieldNameWalk(tslTree, *listCtx.disallowedFields) - if serviceErr != nil { - return nil, serviceErr - } + // Note: FieldNameWalk is now called earlier in buildSearchValues to ensure field mapping + // happens before related table detection. No need to call it again here. // Convert the search tree into SQL [Squirrel] filter sqlizer, err := sqlFilter.Walk(tslTree) diff --git a/pkg/services/generic_test.go b/pkg/services/generic_test.go index de787f0..f21c9df 100755 --- a/pkg/services/generic_test.go +++ b/pkg/services/generic_test.go @@ -32,8 +32,8 @@ func TestSQLTranslation(t *testing.T) { "error": "hyperfleet-21: Failed to parse search query: garbage", }, { - "search": "id in ('123')", - "error": "hyperfleet-21: clusters.id is not a valid field name", + "search": "spec = '{}'", + "error": "hyperfleet-21: spec is not a valid field name", }, } for _, test := range tests { @@ -43,7 +43,6 @@ func TestSQLTranslation(t *testing.T) { listCtx, model, serviceErr := genericService.newListContext(context.Background(), "", &ListArguments{Search: search}, &list) Expect(serviceErr).ToNot(HaveOccurred()) d := g.GetInstanceDao(context.Background(), model) - (*listCtx.disallowedFields)["id"] = "id" _, serviceErr = genericService.buildSearch(listCtx, &d) Expect(serviceErr).To(HaveOccurred()) Expect(serviceErr.Code).To(Equal(errors.ErrorBadRequest)) @@ -57,6 +56,29 @@ func TestSQLTranslation(t *testing.T) { "sql": "username IN (?)", "values": ConsistOf("ooo.openshift"), }, + // Test status.xxx field mapping + { + "search": "status.phase = 'NotReady'", + "sql": "status_phase = ?", + "values": ConsistOf("NotReady"), + }, + { + "search": "status.last_updated_time < '2025-01-01T00:00:00Z'", + "sql": "status_last_updated_time < ?", + "values": ConsistOf("2025-01-01T00:00:00Z"), + }, + // Test labels.xxx field mapping + { + "search": "labels.environment = 'production'", + "sql": "labels->>'environment' = ?", + "values": ConsistOf("production"), + }, + // Test ID query (should be allowed) + { + "search": "id = 'cls-123'", + "sql": "id = ?", + "values": ConsistOf("cls-123"), + }, } for _, test := range tests { var list []api.Cluster @@ -67,6 +89,10 @@ func TestSQLTranslation(t *testing.T) { Expect(serviceErr).ToNot(HaveOccurred()) tslTree, err := tsl.ParseTSL(search) Expect(err).ToNot(HaveOccurred()) + // Apply field name mapping (status.xxx -> status_xxx, labels.xxx -> labels->>'xxx') + // This must happen before converting to sqlizer + tslTree, serviceErr = db.FieldNameWalk(tslTree, *listCtx.disallowedFields) + Expect(serviceErr).ToNot(HaveOccurred()) sqlizer, serviceErr := genericService.treeWalkForSqlizer(listCtx, tslTree) Expect(serviceErr).ToNot(HaveOccurred()) sql, values, err := sqlizer.ToSql() diff --git a/test/factories/clusters.go b/test/factories/clusters.go index 9c483d1..c727e93 100644 --- a/test/factories/clusters.go +++ b/test/factories/clusters.go @@ -2,9 +2,14 @@ package factories import ( "context" + "encoding/json" + "time" + + "gorm.io/gorm" "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" "github.com/openshift-hyperfleet/hyperfleet-api/plugins/clusters" ) @@ -46,3 +51,89 @@ func (f *Factories) NewClusters(id string) (*api.Cluster, error) { func (f *Factories) NewClustersList(name string, count int) ([]*api.Cluster, error) { return f.NewClusterList(name, count) } + +// reloadCluster reloads a cluster from the database to ensure all fields are current +func reloadCluster(dbSession *gorm.DB, cluster *api.Cluster) error { + return dbSession.First(cluster, "id = ?", cluster.ID).Error +} + +// NewClusterWithStatus creates a cluster with specific status phase and last_updated_time +// dbFactory parameter is needed to update database fields +func NewClusterWithStatus(f *Factories, dbFactory db.SessionFactory, id string, phase string, lastUpdatedTime *time.Time) (*api.Cluster, error) { + cluster, err := f.NewCluster(id) + if err != nil { + return nil, err + } + + // Update database record with status fields + dbSession := dbFactory.New(context.Background()) + updates := map[string]interface{}{ + "status_phase": phase, + } + if lastUpdatedTime != nil { + updates["status_last_updated_time"] = lastUpdatedTime + } + err = dbSession.Model(cluster).Updates(updates).Error + if err != nil { + return nil, err + } + + // Reload to get updated values + if err := reloadCluster(dbSession, cluster); err != nil { + return nil, err + } + return cluster, nil +} + +// NewClusterWithLabels creates a cluster with specific labels +func NewClusterWithLabels(f *Factories, dbFactory db.SessionFactory, id string, labels map[string]string) (*api.Cluster, error) { + cluster, err := f.NewCluster(id) + if err != nil { + return nil, err + } + + // Convert labels to JSON and update + labelsJSON, err := json.Marshal(labels) + if err != nil { + return nil, err + } + + dbSession := dbFactory.New(context.Background()) + err = dbSession.Model(cluster).Update("labels", labelsJSON).Error + if err != nil { + return nil, err + } + + // Reload to get updated values + if err := reloadCluster(dbSession, cluster); err != nil { + return nil, err + } + return cluster, nil +} + +// NewClusterWithStatusAndLabels creates a cluster with both status and labels +func NewClusterWithStatusAndLabels(f *Factories, dbFactory db.SessionFactory, id string, phase string, lastUpdatedTime *time.Time, labels map[string]string) (*api.Cluster, error) { + cluster, err := NewClusterWithStatus(f, dbFactory, id, phase, lastUpdatedTime) + if err != nil { + return nil, err + } + + if labels != nil { + labelsJSON, err := json.Marshal(labels) + if err != nil { + return nil, err + } + + dbSession := dbFactory.New(context.Background()) + err = dbSession.Model(cluster).Update("labels", labelsJSON).Error + if err != nil { + return nil, err + } + + if err := reloadCluster(dbSession, cluster); err != nil { + return nil, err + } + } + + return cluster, nil +} diff --git a/test/factories/factory.go b/test/factories/factory.go index f5551e3..f160fa5 100755 --- a/test/factories/factory.go +++ b/test/factories/factory.go @@ -1,10 +1,22 @@ package factories -import "github.com/segmentio/ksuid" +import ( + "encoding/base32" + + "github.com/segmentio/ksuid" +) type Factories struct { } +// NewID generates a new unique identifier using KSUID with lowercase Base32 encoding. +// The resulting identifier is compatible with Kubernetes DNS-1123 subdomain naming requirements. func (f *Factories) NewID() string { - return ksuid.New().String() + return uidEncoding.EncodeToString(ksuid.New().Bytes()) } + +// uidAlphabet is the lowercase alphabet used to encode unique identifiers. +const uidAlphabet = "0123456789abcdefghijklmnopqrstuv" + +// uidEncoding is the lowercase variant of Base32 used to encode unique identifiers. +var uidEncoding = base32.NewEncoding(uidAlphabet).WithPadding(base32.NoPadding) diff --git a/test/factories/node_pools.go b/test/factories/node_pools.go index 4824ade..bf1b8b9 100644 --- a/test/factories/node_pools.go +++ b/test/factories/node_pools.go @@ -2,10 +2,15 @@ package factories import ( "context" + "encoding/json" "fmt" + "time" + + "gorm.io/gorm" "github.com/openshift-hyperfleet/hyperfleet-api/cmd/hyperfleet-api/environments" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api" + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/db" "github.com/openshift-hyperfleet/hyperfleet-api/plugins/nodePools" ) @@ -66,3 +71,89 @@ func (f *Factories) NewNodePools(id string) (*api.NodePool, error) { func (f *Factories) NewNodePoolsList(name string, count int) ([]*api.NodePool, error) { return f.NewNodePoolList(name, count) } + +// reloadNodePool reloads a node pool from the database to ensure all fields are current +func reloadNodePool(dbSession *gorm.DB, nodePool *api.NodePool) error { + return dbSession.First(nodePool, "id = ?", nodePool.ID).Error +} + +// NewNodePoolWithStatus creates a node pool with specific status phase and last_updated_time +// dbFactory parameter is needed to update database fields +func NewNodePoolWithStatus(f *Factories, dbFactory db.SessionFactory, id string, phase string, lastUpdatedTime *time.Time) (*api.NodePool, error) { + nodePool, err := f.NewNodePool(id) + if err != nil { + return nil, err + } + + // Update database record with status fields + dbSession := dbFactory.New(context.Background()) + updates := map[string]interface{}{ + "status_phase": phase, + } + if lastUpdatedTime != nil { + updates["status_last_updated_time"] = lastUpdatedTime + } + err = dbSession.Model(nodePool).Updates(updates).Error + if err != nil { + return nil, err + } + + // Reload to get updated values + if err := reloadNodePool(dbSession, nodePool); err != nil { + return nil, err + } + return nodePool, nil +} + +// NewNodePoolWithLabels creates a node pool with specific labels +func NewNodePoolWithLabels(f *Factories, dbFactory db.SessionFactory, id string, labels map[string]string) (*api.NodePool, error) { + nodePool, err := f.NewNodePool(id) + if err != nil { + return nil, err + } + + // Convert labels to JSON and update + labelsJSON, err := json.Marshal(labels) + if err != nil { + return nil, err + } + + dbSession := dbFactory.New(context.Background()) + err = dbSession.Model(nodePool).Update("labels", labelsJSON).Error + if err != nil { + return nil, err + } + + // Reload to get updated values + if err := reloadNodePool(dbSession, nodePool); err != nil { + return nil, err + } + return nodePool, nil +} + +// NewNodePoolWithStatusAndLabels creates a node pool with both status and labels +func NewNodePoolWithStatusAndLabels(f *Factories, dbFactory db.SessionFactory, id string, phase string, lastUpdatedTime *time.Time, labels map[string]string) (*api.NodePool, error) { + nodePool, err := NewNodePoolWithStatus(f, dbFactory, id, phase, lastUpdatedTime) + if err != nil { + return nil, err + } + + if labels != nil { + labelsJSON, err := json.Marshal(labels) + if err != nil { + return nil, err + } + + dbSession := dbFactory.New(context.Background()) + err = dbSession.Model(nodePool).Update("labels", labelsJSON).Error + if err != nil { + return nil, err + } + + if err := reloadNodePool(dbSession, nodePool); err != nil { + return nil, err + } + } + + return nodePool, nil +} diff --git a/test/integration/search_field_mapping_test.go b/test/integration/search_field_mapping_test.go new file mode 100644 index 0000000..2e4f044 --- /dev/null +++ b/test/integration/search_field_mapping_test.go @@ -0,0 +1,285 @@ +package integration + +import ( + "fmt" + "net/http" + "testing" + "time" + + . "github.com/onsi/gomega" + + "github.com/openshift-hyperfleet/hyperfleet-api/pkg/api/openapi" + "github.com/openshift-hyperfleet/hyperfleet-api/test" + "github.com/openshift-hyperfleet/hyperfleet-api/test/factories" +) + +// TestSearchStatusPhaseMapping verifies that status.phase user-friendly syntax +// correctly maps to status_phase database field +func TestSearchStatusPhaseMapping(t *testing.T) { + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Create NotReady cluster using new factory method + notReadyCluster, err := factories.NewClusterWithStatus(&h.Factories, h.DBFactory, h.NewID(), "NotReady", nil) + Expect(err).NotTo(HaveOccurred()) + + // Create Ready cluster + readyCluster, err := factories.NewClusterWithStatus(&h.Factories, h.DBFactory, h.NewID(), "Ready", nil) + Expect(err).NotTo(HaveOccurred()) + + // Query NotReady clusters using user-friendly syntax + search := "status.phase='NotReady'" + list, resp, err := client.DefaultAPI.GetClusters(ctx).Search(search).Execute() + + Expect(err).NotTo(HaveOccurred()) + Expect(resp.StatusCode).To(Equal(http.StatusOK)) + Expect(list.Total).To(BeNumerically(">=", 1)) + + // Verify all returned clusters are NotReady + foundNotReady := false + for _, item := range list.Items { + if *item.Id == notReadyCluster.ID { + foundNotReady = true + // Status field structure depends on openapi.yaml + // Assuming status.phase exists + Expect(item.Status.Phase).To(Equal(openapi.NOT_READY)) + } + // Should not contain readyCluster + Expect(*item.Id).NotTo(Equal(readyCluster.ID)) + } + Expect(foundNotReady).To(BeTrue(), "Expected to find the NotReady cluster") +} + +// TestSearchStatusLastUpdatedTimeMapping verifies that status.last_updated_time +// user-friendly syntax correctly maps to status_last_updated_time database field +// and time comparison works correctly +func TestSearchStatusLastUpdatedTimeMapping(t *testing.T) { + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + now := time.Now() + oldTime := now.Add(-2 * time.Hour) + recentTime := now.Add(-30 * time.Minute) + + // Create old cluster (2 hours ago) + oldCluster, err := factories.NewClusterWithStatus(&h.Factories, h.DBFactory, h.NewID(), "Ready", &oldTime) + Expect(err).NotTo(HaveOccurred()) + + // Create recent cluster (30 minutes ago) + recentCluster, err := factories.NewClusterWithStatus(&h.Factories, h.DBFactory, h.NewID(), "Ready", &recentTime) + Expect(err).NotTo(HaveOccurred()) + + // Query clusters updated before 1 hour ago + threshold := now.Add(-1 * time.Hour) + search := fmt.Sprintf("status.last_updated_time < '%s'", threshold.Format(time.RFC3339)) + list, resp, err := client.DefaultAPI.GetClusters(ctx).Search(search).Execute() + + Expect(err).NotTo(HaveOccurred()) + Expect(resp.StatusCode).To(Equal(http.StatusOK)) + + // Should return at least oldCluster + Expect(list.Total).To(BeNumerically(">=", 1)) + + // Verify oldCluster is in results but recentCluster is not + foundOld := false + for _, item := range list.Items { + if *item.Id == oldCluster.ID { + foundOld = true + } + // Should not contain recentCluster (updated 30 mins ago) + Expect(*item.Id).NotTo(Equal(recentCluster.ID)) + } + Expect(foundOld).To(BeTrue(), "Expected to find the old cluster") +} + +// TestSearchLabelsMapping verifies that labels.xxx user-friendly syntax +// correctly maps to JSONB query labels->>'xxx' +func TestSearchLabelsMapping(t *testing.T) { + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Create cluster with production labels + prodCluster, err := factories.NewClusterWithLabels(&h.Factories, h.DBFactory, h.NewID(), map[string]string{ + "environment": "production", + "region": "us-east", + }) + Expect(err).NotTo(HaveOccurred()) + + // Create cluster with staging labels + stagingCluster, err := factories.NewClusterWithLabels(&h.Factories, h.DBFactory, h.NewID(), map[string]string{ + "environment": "staging", + }) + Expect(err).NotTo(HaveOccurred()) + + // Query production environment clusters using user-friendly syntax + search := "labels.environment='production'" + list, resp, err := client.DefaultAPI.GetClusters(ctx).Search(search).Execute() + + Expect(err).NotTo(HaveOccurred()) + Expect(resp.StatusCode).To(Equal(http.StatusOK)) + Expect(list.Total).To(BeNumerically(">=", 1)) + + // Verify returned clusters have correct label + foundProd := false + for _, item := range list.Items { + if *item.Id == prodCluster.ID { + foundProd = true + // Verify labels field contains environment=production + if item.Labels != nil { + Expect(*item.Labels).To(HaveKeyWithValue("environment", "production")) + } + } + // Should not contain stagingCluster + Expect(*item.Id).NotTo(Equal(stagingCluster.ID)) + } + Expect(foundProd).To(BeTrue(), "Expected to find the production cluster") +} + +// TestSearchSpecFieldRejected verifies that querying the spec field +// is correctly rejected with 400 Bad Request error +func TestSearchSpecFieldRejected(t *testing.T) { + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Attempt to query spec field (should be rejected) + search := "spec = '{}'" + _, resp, err := client.DefaultAPI.GetClusters(ctx).Search(search).Execute() + + // Should return error + Expect(err).To(HaveOccurred()) + Expect(resp.StatusCode).To(Equal(http.StatusBadRequest)) +} + +// TestSearchCombinedQuery verifies that combined queries (AND/OR) +// work correctly with field mapping +func TestSearchCombinedQuery(t *testing.T) { + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Create cluster with NotReady status and us-east region + matchCluster, err := factories.NewClusterWithStatusAndLabels( + &h.Factories, + h.DBFactory, + h.NewID(), + "NotReady", + nil, + map[string]string{"region": "us-east"}, + ) + Expect(err).NotTo(HaveOccurred()) + + // Create cluster with NotReady status but different region + wrongRegionCluster, err := factories.NewClusterWithStatusAndLabels( + &h.Factories, + h.DBFactory, + h.NewID(), + "NotReady", + nil, + map[string]string{"region": "us-west"}, + ) + Expect(err).NotTo(HaveOccurred()) + + // Create cluster with Ready status and us-east region + wrongStatusCluster, err := factories.NewClusterWithStatusAndLabels( + &h.Factories, + h.DBFactory, + h.NewID(), + "Ready", + nil, + map[string]string{"region": "us-east"}, + ) + Expect(err).NotTo(HaveOccurred()) + + // Query using combined AND condition + search := "status.phase='NotReady' and labels.region='us-east'" + list, resp, err := client.DefaultAPI.GetClusters(ctx).Search(search).Execute() + + Expect(err).NotTo(HaveOccurred()) + Expect(resp.StatusCode).To(Equal(http.StatusOK)) + Expect(list.Total).To(BeNumerically(">=", 1)) + + // Should only return matchCluster + foundMatch := false + for _, item := range list.Items { + if *item.Id == matchCluster.ID { + foundMatch = true + Expect(item.Status.Phase).To(Equal(openapi.NOT_READY)) + } + // Should not contain wrongRegionCluster or wrongStatusCluster + Expect(*item.Id).NotTo(Equal(wrongRegionCluster.ID)) + Expect(*item.Id).NotTo(Equal(wrongStatusCluster.ID)) + } + Expect(foundMatch).To(BeTrue(), "Expected to find the matching cluster") +} + +// TestSearchNodePoolFieldMapping verifies that NodePool also supports +// the same field mapping as Cluster +func TestSearchNodePoolFieldMapping(t *testing.T) { + RegisterTestingT(t) + h, client := test.RegisterIntegration(t) + + account := h.NewRandAccount() + ctx := h.NewAuthenticatedContext(account) + + // Create NotReady NodePool + notReadyNP, err := factories.NewNodePoolWithStatus(&h.Factories, h.DBFactory, h.NewID(), "NotReady", nil) + Expect(err).NotTo(HaveOccurred()) + + // Create Ready NodePool + readyNP, err := factories.NewNodePoolWithStatus(&h.Factories, h.DBFactory, h.NewID(), "Ready", nil) + Expect(err).NotTo(HaveOccurred()) + + // Query NotReady NodePools using user-friendly syntax + search := "status.phase='NotReady'" + list, resp, err := client.DefaultAPI.GetNodePools(ctx).Search(search).Execute() + + Expect(err).NotTo(HaveOccurred()) + Expect(resp.StatusCode).To(Equal(http.StatusOK)) + Expect(list.Total).To(BeNumerically(">=", 1)) + + // Verify NotReady NodePool is in results + foundNotReady := false + for _, item := range list.Items { + if *item.Id == notReadyNP.ID { + foundNotReady = true + Expect(item.Status.Phase).To(Equal(openapi.NOT_READY)) + } + // Should not contain readyNP + Expect(*item.Id).NotTo(Equal(readyNP.ID)) + } + Expect(foundNotReady).To(BeTrue(), "Expected to find the NotReady node pool") + + // Also test labels mapping for NodePools + npWithLabels, err := factories.NewNodePoolWithLabels(&h.Factories, h.DBFactory, h.NewID(), map[string]string{ + "environment": "test", + }) + Expect(err).NotTo(HaveOccurred()) + + searchLabels := "labels.environment='test'" + labelsList, labelsResp, labelsErr := client.DefaultAPI.GetNodePools(ctx).Search(searchLabels).Execute() + + Expect(labelsErr).NotTo(HaveOccurred()) + Expect(labelsResp.StatusCode).To(Equal(http.StatusOK)) + + foundLabeled := false + for _, item := range labelsList.Items { + if *item.Id == npWithLabels.ID { + foundLabeled = true + } + } + Expect(foundLabeled).To(BeTrue(), "Expected to find the labeled node pool") +}