HYPERFLEET-649 - fix: Reject cluster creation with missing spec field

[Mischulee]

https://issues.redhat.com/browse/HYPERFLEET-649

Summary by CodeRabbit

• Bug Fixes
  • Cluster and NodePool creation now require a valid non-null Spec. Creating without Spec or with null Spec returns a 400 validation error (code HYPERFLEET-VAL-000) with messages like "spec is required" or "spec field must be an object".

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9e95000 and e8ff1ba.

📒 Files selected for processing (6)

• pkg/handlers/cluster.go
• pkg/handlers/cluster_nodepools.go
• pkg/handlers/validation.go
• pkg/handlers/validation_test.go
• test/integration/clusters_test.go
• test/integration/node_pools_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• test/integration/clusters_test.go
• pkg/handlers/validation_test.go

---

Walkthrough

The PR adds validation for the Spec field on cluster and nodepool creation requests. A new private validateSpec function (using reflection) was introduced to ensure the Spec field is present and non-nil; handlers for cluster and cluster nodepools now call this validator during Create. Unit tests were added for valid, empty, and nil Spec cases. Integration tests were added to ensure POST requests with a null or missing spec return 400 with error code HYPERFLEET-VAL-000 and appropriate error details.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• HYPERFLEET-364 - feat: Align cluster and nodepool name validation with CS rules #48: Modifies create handlers for clusters and nodepools to adjust request validation; related to added Spec validation here.

Suggested labels

lgtm, approved

Suggested reviewers

• crizzo71

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 69.23% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding validation to reject cluster creation when the spec field is missing. It aligns with the primary objective and changeset modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

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

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

---

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

[rafabene]

The validateSpec check was added to the cluster creation handler, but the nodepool creation
handler in cluster_nodepools.go has the same gap. The NodePoolCreateRequest schema also
defines spec as a required field, and the SchemaValidationMiddleware skips validation when the
spec key is entirely absent from the JSON body (it only catches spec: null). This means a
nodepool can still be created without a spec field.

Consider adding validateSpec(&req, "Spec", "spec") to the validators list in
clusterNodePoolsHandler.Create:

  []validate{
      validateEmpty(&req, "Id", "id"),
      validateName(&req, "Name", "name", 3, 15),
      validateKind(&req, "Kind", "kind", "NodePool"),
      validateSpec(&req, "Spec", "spec"),
  },

This may be intentionally out of scope for this ticket, but worth noting since it's the same
vulnerability

[Mischulee]

Great catch! I will include it in this PR since its same bug with same solution both violating OpenAPI specification.

[rafabene]

The validateSpec function doesn't follow the same defensive pattern used by all other
validators in this file. Every other validator checks value.Kind() == reflect.Ptr before
calling methods like IsNil() and dereferences the pointer when needed. Consider adding the
same guard for consistency and safety:

  func validateSpec(i interface{}, fieldName string, field 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()
                }

                if !value.IsValid() || value.IsNil() {
                        return errors.Validation("%s is required", field)
                }

                return nil
        }
  }

This makes the function safe for reuse if the Spec type ever changes (e.g., to a pointer to a
struct) and keeps the codebase consistent.

[Mischulee]

Done. validateSpec now follows the same defensive pattern.

[rh-amarin]

/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rh-amarin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [rh-amarin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment