Skip to content

Commit

Permalink
[CC-30768] don't allow node_count for serverless cluster
Browse files Browse the repository at this point in the history
Previously it was possible to pass in the node_count value for
serverless region because the type is shared with dedicated clusters.
This ended up causing a state inconsistency however becase node_count is
coerced to 0 by the api which conflicts with a value added in the
config. To solve this, we simply validate that node_count is not used
unless cluster plan is Advanced.
  • Loading branch information
fantapop committed Dec 9, 2024
1 parent 9f9fb49 commit a2c6d2f
Show file tree
Hide file tree
Showing 5 changed files with 63 additions and 4 deletions.
5 changes: 4 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [1.11.0] - 2024-12-05
### Fixed

- Validate that `node_count` is not passed in with serverless cluster regions.
Also, clear up in the documentation that this is not allowed.

### Added

Expand Down
2 changes: 1 addition & 1 deletion docs/resources/cluster.md
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ Required:

Optional:

- `node_count` (Number) Number of nodes in the region. Will always be 0 for serverless clusters.
- `node_count` (Number) Number of nodes in the region. Valid for Advanced clusters only.
- `primary` (Boolean) Set to true to mark this region as the primary for a serverless cluster. Exactly one region must be primary. Dedicated clusters expect to have no primary region.

Read-Only:
Expand Down
2 changes: 1 addition & 1 deletion docs/resources/cmek.md
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ Required:

Optional:

- `node_count` (Number) Number of nodes in the region. Will always be 0 for serverless clusters.
- `node_count` (Number) Number of nodes in the region. Valid for Advanced clusters only.
- `primary` (Boolean) Set to true to mark this region as the primary for a serverless cluster. Exactly one region must be primary. Dedicated clusters expect to have no primary region.

Read-Only:
Expand Down
34 changes: 33 additions & 1 deletion internal/provider/cluster_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ var regionSchema = schema.NestedAttributeObject{
PlanModifiers: []planmodifier.Int64{
int64planmodifier.UseStateForUnknown(),
},
Description: "Number of nodes in the region. Will always be 0 for serverless clusters.",
Description: "Number of nodes in the region. Valid for Advanced clusters only.",
},
"primary": schema.BoolAttribute{
Optional: true,
Expand Down Expand Up @@ -376,6 +376,38 @@ func (r *clusterResource) ConfigValidators(_ context.Context) []resource.ConfigV
}
}

func (r *clusterResource) ValidateConfig(ctx context.Context, req resource.ValidateConfigRequest, resp *resource.ValidateConfigResponse) {

// ValidateConfig seems to be called multiple times, once before any values
// are known. When all values are unknown we can't convert to our cluster
// model because we use some custom fields such as Regions. Wait until
// things are defined before converting the config to our model.
var regions types.List
resp.Diagnostics.Append(req.Config.GetAttribute(ctx, path.Root("regions"), &regions)...)
if regions.IsUnknown() {
return
}

var cluster CockroachCluster
resp.Diagnostics.Append(req.Config.Get(ctx, &cluster)...)
if resp.Diagnostics.HasError() {
return
}

plan, err := derivePlanType(&cluster)
if err != nil {
resp.Diagnostics.AddError("Invalid Configuration", err.Error())
return
}
if plan != client.PLANTYPE_ADVANCED {
for i, region := range cluster.Regions {
if IsKnown(region.NodeCount) {
resp.Diagnostics.AddAttributeError(path.Root("regions").AtListIndex(i), "Invalid Attribute", "node_count is supported for ADVANCED clusters only.")
}
}
}
}

func derivePlanType(cluster *CockroachCluster) (client.PlanType, error) {
var planType client.PlanType
if IsKnown(cluster.Plan) {
Expand Down
24 changes: 24 additions & 0 deletions internal/provider/cluster_resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -826,6 +826,30 @@ func TestIntegrationServerlessClusterResource(t *testing.T) {
finalCluster client.Cluster
ignoreImportPaths []string
}{
{
name: "STANDARD clusters must not have a node_count",
createStep: func() resource.TestStep {
return resource.TestStep{
Config: fmt.Sprintf(`
resource "cockroach_cluster" "test" {
name = "%s"
cloud_provider = "GCP"
serverless = {}
regions = [
{
name = "us-central1"
},
{
name = "us-central1"
node_count = 1
},
]
}
`, clusterName),
ExpectError: regexp.MustCompile("node_count is supported for ADVANCED clusters only"),
}
},
},
{
name: "single-region serverless BASIC cluster converted to unlimited resources",
createStep: func() resource.TestStep {
Expand Down

0 comments on commit a2c6d2f

Please sign in to comment.