Skip to content

Conversation

@TinaHeiligers
Copy link
Contributor

fix #1144

Fix: Add client-side validation for Kibana SLO slo_id field

Summary

I've implemented client-side validation for the slo_id field in the Kibana SLO resource to prevent invalid values from reaching the API and causing confusing 400 errors.

Problem

Previously, the slo_id field lacked validation on the provider side, allowing invalid values like "supdawg" (only 7 characters) to pass through to the Kibana API. This resulted in unhelpful 400 Bad Request errors from the server instead of clear validation feedback at plan time.

As mentioned in the GitHub issue #1144, users (me!) currently experience:

  • Short IDs (< 8 characters) accepted by Terraform but rejected by the API
  • Unclear error messages when invalid characters are used
  • Poor user experience with late-stage failures during apply

Solution

This PR adds validation to the slo_id field that enforces the Kibana API requirements:

Changes

1. Schema Validation (internal/kibana/slo.go)

  • Added regexp import for character validation (follows existing codebase patterns, for example user.go)
  • Implemented dual validation using validation.All():
    • Length validation: validation.StringLenBetween(8, 48) ensures IDs are between 8-48 characters
    • Character validation: validation.StringMatch() with regex ^[a-zA-Z0-9_-]+$ ensures only letters, numbers, hyphens, and underscores
  • Updated field description to clearly document the requirements

2. Test Coverage (internal/kibana/slo_test.go)

  • Unit tests: Added TestSloIdValidation() to verify both valid and invalid cases
  • Acceptance tests: Added TestAccResourceSloValidation() with scenarios for:
    • Too short IDs (< 8 characters)
    • Too long IDs (> 48 characters)
    • Invalid characters (@, $, spaces, periods)
  • Helper function: Created getSLOConfigWithInvalidSloId() for test configuration generation

3. Documentation Updated

  • Automatically updated via make docs-generate to reflect the new validation requirements
  • Changed description from vague "8 and 36 characters" to precise "8 to 48 characters that contains only letters, numbers, hyphens, and underscores"

Validation Examples

The validation now catches these scenarios at plan time:

# Too short (< 8 characters)
slo_id = "short"     # ❌ Error: expected length of slo_id to be in the range (8 - 48)

# Too long (> 48 characters)  
slo_id = "this-id-is-way-too-long-and-exceeds-the-48-character-limit-for-slo-ids"  # ❌ Error: expected length of slo_id to be in the range (8 - 48)

# Invalid characters
slo_id = "invalid@id$"  # ❌ Error: must contain only letters, numbers, hyphens, and underscores

# Valid examples
slo_id = "valid_id"              # ✅ 8 chars with underscore
slo_id = "valid-id-123"          # ✅ Mixed case with numbers and hyphens  
slo_id = "my-very-long-slo-id"   # ✅ Within 48 character limit

Testing

All tests pass successfully:

  • Unit tests: Direct validation function testing
  • Acceptance tests: End-to-end validation scenarios
  • Existing tests: Confirmed no regressions in existing SLO functionality

The fix follows the existing codebase patterns (similar to validation in user.go) and uses the established SDK v2 validation framework, so no migration to plugin framework was needed.

Impact

This change improves the user experience by:

  • Providing immediate feedback at plan time instead of apply time
  • Giving clear, actionable error messages
  • Preventing wasted time with configurations that will never work
  • Maintaining backward compatibility (no breaking changes)

@wandergeek
Copy link
Contributor

Nice one! Welcome to the world of Terraform development! <3

LGTM, only thing I can think of is you need an entry in https://github.com/elastic/terraform-provider-elasticstack/blob/main/CHANGELOG.md

@TinaHeiligers TinaHeiligers enabled auto-merge (squash) July 30, 2025 00:23
@TinaHeiligers TinaHeiligers merged commit f106c32 into elastic:main Jul 30, 2025
23 checks passed
@TinaHeiligers TinaHeiligers deleted the tpe1144/kbn_validate_slo_id branch July 30, 2025 01:12
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.

[Bug] slo_id is not validated

3 participants