Skip to content

fix: reject unsupported hypervisor values in host resource validation - #337

Open
nagaboinaramgopal wants to merge 4 commits into
apache:mainfrom
nagaboinaramgopal:fix/host-hypervisor-validator
Open

fix: reject unsupported hypervisor values in host resource validation#337
nagaboinaramgopal wants to merge 4 commits into
apache:mainfrom
nagaboinaramgopal:fix/host-hypervisor-validator

Conversation

@nagaboinaramgopal

Copy link
Copy Markdown
Contributor

Description

The hypervisor ValidateFunc on the cloudstack_host resource used sort.SearchStrings as if it returned a "found" flag, but sort.SearchStrings returns the insertion index (0..len). The check >= len(validHypervisors) therefore only rejected a value that sorts after every supported entry, so unsupported values such as foo, docker, esxi, or kvm2 passed plan-time validation and failed later with an opaque server-side error at apply.

Fixed by treating a value as valid only when the element at the returned index actually equals it, which is the standard sort.SearchStrings idiom.

Testing

Added a unit test TestResourceCloudStackHostHypervisorValidation asserting the supported hypervisors are accepted and unsupported values are rejected. It needs no live CloudStack:

go test ./cloudstack/ -run TestResourceCloudStackHostHypervisorValidation

The test fails against the current code (bad values are accepted) and passes with the fix.

Comment thread cloudstack/resource_cloudstack_host.go Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The validation fix is correct and low-risk, and the added unit test directly covers the previously broken behavior.

Pull request overview

This PR fixes plan-time validation for the hypervisor argument on the cloudstack_host Terraform resource by correcting the sort.SearchStrings usage so only exact matches against the supported hypervisor list are accepted. This prevents unsupported values from slipping through planning and failing later during apply with less actionable CloudStack API errors.

Changes:

  • Corrected the hypervisor ValidateFunc to verify that the searched index is both in range and an exact string match.
  • Added a focused unit test to ensure supported hypervisors are accepted and unsupported values are rejected.
File summaries
File Description
cloudstack/resource_cloudstack_host.go Fixes the validation logic to only accept exact matches from the supported hypervisor list.
cloudstack/resource_cloudstack_host_unit_test.go Adds a unit test covering both accepted and rejected hypervisor values for plan-time validation.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

The hypervisor field accepted unsupported values at plan time because the
ValidateFunc used sort.SearchStrings as a found-check when it returns an
insertion index. Replace it with validation.StringInSlice, which rejects any
value not in the supported list. Keeps the unit test covering accepted and
rejected values.

Signed-off-by: Ramgopal Nagaboina <ramgopal.nagaboina.dev@gmail.com>
@nagaboinaramgopal
nagaboinaramgopal force-pushed the fix/host-hypervisor-validator branch from eef658a to 5dc14fb Compare September 3, 2026 14:07
@sudo87

sudo87 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Fixed the hypervisor allow-list to use CloudStack's actual enum casing (XenServer, KVM, VMware, Hyperv, BareMetal, Simulator, Ovm3) instead of all-lowercase — that mismatch is what was failing the acceptance tests (got Simulator). Also merged resource_cloudstack_host_unit_test.go into resource_cloudstack_host_test.go and updated it to match, plus added a case-sensitivity check.

@sureshanaparti sureshanaparti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants