Add cluster/nodepool name & kind validation and remove useless endpoint compatibility #15

yasun1 · 2025-12-09T14:10:09Z

Close HYPERFLEET-22

Summary by CodeRabbit

Breaking Changes
- Removed the /api/hyperfleet/v1/compatibility endpoint
- Cluster names now require a minimum of 3 characters
- Kind field is now required for clusters and node pools
Improvements
- Enhanced input validation for cluster and node pool names (alphanumeric and hyphens only)
- Improved error responses with clearer validation failure messages
- Updated API documentation to production endpoint

_{✏️ Tip: You can customize this high-level summary in your review settings.}

I still leave securitySchemes here; otherwise, we need to make a significant change to remove auth, and we actually want authorization in the future, and currently, setting EnableJWT=false is met the MVP requirement.

…nt compatibility gofmt

coderabbitai · 2025-12-09T14:10:34Z

Walkthrough

This pull request removes the compatibility API endpoint and modifies Kind field handling from auto-defaulting to explicit preservation. Input validation is enhanced for cluster and node pool names (enforcing 3-63 character length, lowercase alphanumeric with hyphens) and Kind fields (requiring "Cluster" or "NodePool" respectively). The OpenAPI specification is updated with Apache 2.0 license information, HTTP bearer authentication scheme, production server URL, and tightened cluster name constraints (minLength 1 to 3). Integration tests are updated to expect 400 Bad Request responses for validation failures, and new contract tests verify synchronization between domain and OpenAPI constants. Test client configuration now targets the local test server environment.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

Areas requiring attention:

Kind field handling shift: Verify that removal of auto-defaulting in pkg/api/cluster_types.go and pkg/api/node_pool_types.go aligns with the new validation requirements in handlers and doesn't introduce regressions with existing data or workflows.
Validation logic robustness: Review the regex pattern in pkg/handlers/validation.go for correctness and confirm validateName and validateKind helpers handle edge cases (null pointers, reflection operations) safely.
Compatibility endpoint removal: Ensure complete removal across routes.go, handler file deletion, and integration test cleanup with no dangling references.
OpenAPI specification changes: Validate production server URL, minLength constraint change for cluster names (1→3), and bearer auth scheme integration.
Test expectations: Confirm that updated integration tests in clusters_test.go and node_pools_test.go properly reflect new 400 error behaviors and error message assertions.

Suggested labels

lgtm

Suggested reviewers

rh-amarin
yingzhanredhat

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 61.76% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main changes: adding cluster/nodepool name and kind validation, and removing the unused compatibility endpoint.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (7)

pkg/handlers/validation.go (1)
10-11: Consider stricter pattern to disallow leading/trailing hyphens.

The current pattern ^[a-z0-9-]+$ allows names that start or end with hyphens (e.g., -cluster, cluster-, ---), which typically violates Kubernetes DNS-1123 naming conventions. If this API follows Kubernetes naming standards, consider using a pattern like ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$ to require alphanumeric start/end.
-// Cluster/NodePool name pattern: lowercase alphanumeric and hyphens
-var namePattern = regexp.MustCompile(`^[a-z0-9-]+$`)
+// Cluster/NodePool name pattern: lowercase alphanumeric and hyphens
+// Must start and end with alphanumeric character (Kubernetes DNS-1123 label convention)
+var namePattern = regexp.MustCompile(`^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`)
openapi/openapi.yaml (1)
35-36: 400 responses are missing error schema references.

The 400 Bad Request responses added throughout the spec don't include a response body schema. For consistency with the default error responses and to help clients handle validation errors, consider adding the Error schema reference.
         '400':
           description: The server could not understand the request due to invalid syntax.
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/Error'
pkg/handlers/cluster_nodepools.go (1)

124-161: NodePool Create validation is sensible; consider centralizing name length constraints

Using validateName(&req, "Name", "name", 1, 255) and validateKind(&req, "Kind", "kind", "NodePool") brings NodePool creation in line with the new validation approach and happens before any conversion or DB operations, which is good.

If cluster/nodepool name limits are meant to stay in sync with OpenAPI/DB constraints long‑term, consider extracting these min/max values into shared constants (e.g., ClusterNameMinLen, ClusterNameMaxLen, NodePoolNameMaxLen) to avoid silent drift between handlers, schema, and tests.

test/integration/node_pools_test.go (2)

210-247: Empty kind integration test correctly validates 400 behavior

TestNodePoolPost_EmptyKind cleanly exercises the handler by:

Creating a real cluster,

Sending raw JSON with \"kind\": \"\" under an authenticated context, and

Asserting a 400 plus an error reason containing “kind is required”.

This is a good end‑to‑end coverage of the new Kind validation path.

You could factor the repeated “POST raw JSON and parse error.reason” logic (here and in WrongKind) into a small helper to keep these tests DRY, but it’s not required.

249-286: Wrong kind integration test robustly checks specific validation messaging

TestNodePoolPost_WrongKind mirrors the empty‑kind test for the “wrong kind value” case, confirming 400 and a reason containing “kind must be 'NodePool'”. This gives good regression protection for both status code and user‑facing error text.

test/integration/clusters_test.go (2)

621-654: Empty kind cluster test accurately validates 400 and message content

TestClusterPost_EmptyKind:

Authenticates via JWT,

Sends raw JSON with \"kind\": \"\" to bypass client struct constraints, and

Verifies 400 plus an error reason containing “kind is required”.

This is a solid end‑to‑end check of the new Kind validation for clusters.

As with the nodepool tests, you might consider a small helper for “POST invalid payload and assert error.reason substring” to reduce repetition, but current code is clear enough.

656-689: Wrong kind cluster test mirrors nodepool coverage and looks good

TestClusterPost_WrongKind complements the empty‑kind case by asserting a 400 and a reason containing “kind must be 'Cluster'” when an incorrect kind is supplied. Together, these tests meaningfully lock in the new Kind API contract.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bc5c73e and 6524faa.

📒 Files selected for processing (25)

cmd/hyperfleet-api/server/routes.go (0 hunks)
openapi/openapi.yaml (14 hunks)
pkg/api/cluster_types.go (0 hunks)
pkg/api/cluster_types_test.go (2 hunks)
pkg/api/node_pool_types.go (0 hunks)
pkg/api/node_pool_types_test.go (2 hunks)
pkg/api/openapi_embed.go (0 hunks)
pkg/api/presenters/adapter_status_test.go (2 hunks)
pkg/auth/auth_middleware.go (0 hunks)
pkg/dao/mocks/generic.go (1 hunks)
pkg/handlers/cluster.go (1 hunks)
pkg/handlers/cluster_nodepools.go (2 hunks)
pkg/handlers/compatibility.go (0 hunks)
pkg/handlers/node_pool.go (1 hunks)
pkg/handlers/validation.go (2 hunks)
pkg/handlers/validation_test.go (1 hunks)
pkg/logger/logger.go (1 hunks)
plugins/clusters/plugin.go (0 hunks)
plugins/nodePools/plugin.go (0 hunks)
test/helper.go (1 hunks)
test/integration/api_contract_test.go (1 hunks)
test/integration/clusters_test.go (7 hunks)
test/integration/compatibility_test.go (0 hunks)
test/integration/integration_test.go (1 hunks)
test/integration/node_pools_test.go (4 hunks)

💤 Files with no reviewable changes (9)

cmd/hyperfleet-api/server/routes.go
plugins/clusters/plugin.go
pkg/handlers/compatibility.go
pkg/auth/auth_middleware.go
pkg/api/openapi_embed.go
plugins/nodePools/plugin.go
pkg/api/node_pool_types.go
pkg/api/cluster_types.go
test/integration/compatibility_test.go

🧰 Additional context used

📓 Path-based instructions (9)

**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

Use Go 1.24.9 with FIPS-compliant crypto enabled (CGO_ENABLED=1, GOEXPERIMENT=boringcrypto)

Files:

pkg/handlers/cluster.go
pkg/handlers/node_pool.go
test/integration/api_contract_test.go
test/integration/node_pools_test.go
pkg/api/presenters/adapter_status_test.go
test/helper.go
pkg/handlers/validation.go
pkg/api/cluster_types_test.go
pkg/dao/mocks/generic.go
test/integration/clusters_test.go
pkg/handlers/validation_test.go
pkg/api/node_pool_types_test.go
test/integration/integration_test.go
pkg/handlers/cluster_nodepools.go
pkg/logger/logger.go

pkg/{handlers,services}/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

Always retrieve database session from context via db.NewContext() instead of creating new connections

Files:

pkg/handlers/cluster.go
pkg/handlers/node_pool.go
pkg/handlers/validation.go
pkg/handlers/validation_test.go
pkg/handlers/cluster_nodepools.go

pkg/handlers/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

Implement all HTTP handlers following the pattern: parse request → call service/DAO → handle errors → send response

Files:

pkg/handlers/cluster.go
pkg/handlers/node_pool.go
pkg/handlers/validation.go
pkg/handlers/validation_test.go
pkg/handlers/cluster_nodepools.go

pkg/{handlers,services,dao}/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

pkg/{handlers,services,dao}/**/*.go: Use structured logging via pkg/logger/logger.go with context propagation including opid, accountID, and tx_id
Return errors using the structured ServiceError type from pkg/errors/ with HttpCode, Code, and Reason fields

Files:

pkg/handlers/cluster.go
pkg/handlers/node_pool.go
pkg/handlers/validation.go
pkg/dao/mocks/generic.go
pkg/handlers/validation_test.go
pkg/handlers/cluster_nodepools.go

pkg/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

Implement JWT authentication middleware that validates tokens and extracts account ID and username from claims

Files:

pkg/handlers/cluster.go
pkg/handlers/node_pool.go
pkg/api/presenters/adapter_status_test.go
pkg/handlers/validation.go
pkg/api/cluster_types_test.go
pkg/dao/mocks/generic.go
pkg/handlers/validation_test.go
pkg/api/node_pool_types_test.go
pkg/handlers/cluster_nodepools.go
pkg/logger/logger.go

test/integration/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

test/integration/**/*.go: Create integration tests using Testcontainers for isolated PostgreSQL instances per test
All API endpoints must be integrated tested in test/integration/ covering CRUD operations, pagination, search, and error cases

Files:

test/integration/api_contract_test.go
test/integration/node_pools_test.go
test/integration/clusters_test.go
test/integration/integration_test.go

pkg/api/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

Use //go:embed directive to embed the OpenAPI specification at compile time from pkg/api/openapi/api/openapi.yaml

Files:

pkg/api/presenters/adapter_status_test.go
pkg/api/cluster_types_test.go
pkg/api/node_pool_types_test.go

pkg/dao/**/*.go

📄 CodeRabbit inference engine (CLAUDE.md)

pkg/dao/**/*.go: Use context.Context as the first parameter in all DAO methods for transaction propagation
Increment generation field on each spec update to enable optimistic concurrency control
All DAO methods must support context-based session access for transaction participation

Files:

pkg/dao/mocks/generic.go

openapi/openapi.yaml

📄 CodeRabbit inference engine (CLAUDE.md)

TypeSpec definitions are maintained separately in hyperfleet-api-spec repository; only edit openapi/openapi.yaml in this repository for spec changes

Files:

openapi/openapi.yaml

🧠 Learnings (12)