Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support_physical_cluster_replication field to cluster creation #261

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/data-sources/cluster.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ Read-Only:
- `num_virtual_cpus` (Number) Number of virtual CPUs per node in the cluster.
- `private_network_visibility` (Boolean) Indicates whether private IP addresses are assigned to nodes. Required for CMEK and other advanced networking features.
- `storage_gib` (Number) Storage amount per node in GiB.
- `support_physical_cluster_replication` (Boolean) Indicates whether to create a cluster using an architecture that supports physical cluster replication.


<a id="nestedatt--regions"></a>
Expand Down
1 change: 1 addition & 0 deletions docs/resources/cluster.md
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ Optional:
- `num_virtual_cpus` (Number) Number of virtual CPUs per node in the cluster.
- `private_network_visibility` (Boolean) Set to true to assign private IP addresses to nodes. Required for CMEK and other advanced networking features. Clusters created with this flag will have advanced security features enabled. This cannot be changed after cluster creation and incurs additional charges. See [Create an Advanced Cluster](https://www.cockroachlabs.com/docs/cockroachcloud/create-an-advanced-cluster.html#step-6-configure-advanced-security-features) and [Pricing](https://www.cockroachlabs.com/pricing/) for more information.
- `storage_gib` (Number) Storage amount per node in GiB.
- `support_physical_cluster_replication` (Boolean) Specifies whether a cluster should be started using an architecture that supports physical cluster replication.

Read-Only:

Expand Down
8 changes: 7 additions & 1 deletion examples/workflows/cockroach_advanced_cluster/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ variable "cidr_prefix_length" {
default = 0
}

variable "support_physical_cluster_replication" {
type = bool
nullable = true
}

terraform {
required_providers {
cockroach = {
Expand All @@ -98,7 +103,8 @@ resource "cockroach_cluster" "example" {
dedicated = {
storage_gib = var.storage_gib
num_virtual_cpus = var.num_virtual_cpus
cidr_range = "172.28.0.0/14"
# cidr_range = "172.28.0.0/14"
support_physical_cluster_replication = var.support_physical_cluster_replication
}
regions = [
for r in var.cloud_provider_regions : {
Expand Down
9 changes: 6 additions & 3 deletions internal/provider/cluster_data_source.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,10 @@ func (d *clusterDataSource) Schema(
Computed: true,
Description: "The IPv4 range in CIDR format that is in use by the cluster. It is only set on GCP clusters and is otherwise empty.",
},
"support_physical_cluster_replication": schema.BoolAttribute{
Computed: true,
Description: "Indicates whether to create a cluster using an architecture that supports physical cluster replication.",
},
},
},
"regions": schema.ListNestedAttribute{
Expand Down Expand Up @@ -191,11 +195,11 @@ func (d *clusterDataSource) Schema(
Description: "Indicates whether backups are enabled.",
},
"retention_days": schema.Int64Attribute{
Computed: true,
Computed: true,
MarkdownDescription: "The number of days to retain backups for.",
},
"frequency_minutes": schema.Int64Attribute{
Computed: true,
Computed: true,
Description: "The frequency of backups in minutes.",
},
},
Expand Down Expand Up @@ -281,7 +285,6 @@ func (d *clusterDataSource) Read(
return
}


// The concept of a plan doesn't apply to data sources.
// Using a nil plan means we won't try to re-sort the region list.
var newState CockroachCluster
Expand Down
68 changes: 44 additions & 24 deletions internal/provider/cluster_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,9 @@ func (r *clusterResource) Schema(
Description: "Storage amount per node in GiB.",
},
"disk_iops": schema.Int64Attribute{
Optional: true,
Computed: true,
Validators: []validator.Int64{
Optional: true,
Computed: true,
Validators: []validator.Int64{
// If supplied this value must be non-zero. 0 is a
// valid api value indicating the default being
// returned but it causes a provider inconsistency.
Expand All @@ -232,8 +232,8 @@ func (r *clusterResource) Schema(
Description: "Memory per node in GiB.",
},
"machine_type": schema.StringAttribute{
Optional: true,
Computed: true,
Optional: true,
Computed: true,
MarkdownDescription: "Machine type identifier within the given cloud provider, e.g., m6.xlarge, n2-standard-4. This attribute requires a feature flag to be enabled. It is recommended to leave this empty and use `num_virtual_cpus` to control the machine type.",
},
"num_virtual_cpus": schema.Int64Attribute{
Expand All @@ -257,6 +257,14 @@ func (r *clusterResource) Schema(
stringplanmodifier.UseStateForUnknown(),
},
},
"support_physical_cluster_replication": schema.BoolAttribute{
Optional: true,
Computed: true,
Comment on lines +261 to +262
Copy link
Collaborator

Choose a reason for hiding this comment

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

Normally when we have optional values, we'll let the server choose the default. computed: true, would normally allow the value in the state to be set by what the server chose. That way from provider version to version we could change the default on the server and it would remain working with our serverside default. However, I suggested we not add the field to the cluster response in that recent PR. I think that is a bit problematic with the current implementation here. I see 2 options:

  1. we have a hard default of false that's built into the provider for this version. That won't feel very ideal when we change the serverside default to true because people will probably forget to pass this optional parameter and end up with false. We could change the default in a later terraform version but it would be a breaking change to the provider.
  2. we follow through with adding a value to the cluster response that indicates which value was chosen.

Description: "Specifies whether a cluster should be started using an architecture that supports physical cluster replication.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have any documentation to suggest why a customer wouldn't want this set to true? Otherwise why wouldn't everyone set it to true? @kathancox this is probably more of a docs concern.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we should also say it's defaulting to false.

PlanModifiers: []planmodifier.Bool{
boolplanmodifier.UseStateForUnknown(),
},
},
},
},
"regions": schema.ListNestedAttribute{
Expand All @@ -267,7 +275,7 @@ func (r *clusterResource) Schema(
NestedObject: regionSchema,
},
"state": schema.StringAttribute{
Computed: true,
Computed: true,
PlanModifiers: []planmodifier.String{
stringplanmodifier.UseStateForUnknown(),
},
Expand Down Expand Up @@ -302,32 +310,32 @@ func (r *clusterResource) Schema(
Description: "Set to true to enable delete protection on the cluster. If unset, the server chooses the value on cluster creation, and preserves the value on cluster update.",
},
"backup_config": schema.SingleNestedAttribute{
Computed: true,
Optional: true,
Computed: true,
Optional: true,
MarkdownDescription: "The backup settings for a cluster.\n Each cluster has backup settings that determine if backups are enabled, how frequently they are taken, and how long they are retained for. Use this attribute to manage those settings.",
PlanModifiers: []planmodifier.Object{
objectplanmodifier.UseStateForUnknown(),
},
Attributes: map[string]schema.Attribute{
"enabled": schema.BoolAttribute{
Optional: true,
Computed: true,
Optional: true,
Computed: true,
PlanModifiers: []planmodifier.Bool{
boolplanmodifier.UseStateForUnknown(),
},
Description: "Indicates whether backups are enabled. If set to false, no backups will be created.",
},
"retention_days": schema.Int64Attribute{
Optional: true,
Computed: true,
Optional: true,
Computed: true,
PlanModifiers: []planmodifier.Int64{
int64planmodifier.UseStateForUnknown(),
},
MarkdownDescription: "The number of days to retain backups for. Valid values are [2, 7, 30, 90, 365]. Can only be set once, further changes require opening a support ticket. See [Updating backup retention](../guides/updating-backup-retention) for more information.",
},
"frequency_minutes": schema.Int64Attribute{
Optional: true,
Computed: true,
Optional: true,
Computed: true,
PlanModifiers: []planmodifier.Int64{
int64planmodifier.UseStateForUnknown(),
},
Expand Down Expand Up @@ -556,6 +564,9 @@ func (r *clusterResource) Create(
if cfg.CidrRange.ValueString() != "" {
dedicated.CidrRange = ptr(cfg.CidrRange.ValueString())
}
if cfg.SupportPhysicalClusterReplication.ValueBool() {
dedicated.SupportPhysicalClusterReplication = ptr(cfg.SupportPhysicalClusterReplication.ValueBool())
}
Comment on lines +567 to +569
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like if someone were to actually set the value to false, SupportPhysicalClusterReplication would be omitted from the request. I think we want it to to pass an explicit false in that case. Otherwise, once we start defaulting to true on the serverside the false pass via terraform will be ignored.

}
clusterSpec.SetDedicated(dedicated)
}
Expand Down Expand Up @@ -632,7 +643,7 @@ func (r *clusterResource) Create(
backupUpdateRequest.FrequencyMinutes = ptr(int32(planBackupConfig.FrequencyMinutes.ValueInt64()))
}

if backupUpdateRequest != (client.UpdateBackupConfigurationSpec{}) {
if backupUpdateRequest != (client.UpdateBackupConfigurationSpec{}) {
traceAPICall("UpdateBackupConfiguration")
remoteBackupConfig, _, err = r.provider.service.UpdateBackupConfiguration(ctx, clusterObj.Id, &backupUpdateRequest)
if err != nil {
Expand Down Expand Up @@ -790,6 +801,12 @@ func (r *clusterResource) ModifyPlan(
"isn't allowed. Please explicitly destroy this cluster before changing "+
"cidr range.")
}
if dedicated := plan.DedicatedConfig; dedicated != nil && dedicated.SupportPhysicalClusterReplication != state.DedicatedConfig.SupportPhysicalClusterReplication {
resp.Diagnostics.AddError("Cannot update support_physical_cluster_replication",
"To prevent accidental deletion of data, changing a cluster's "+
"support_physical_cluster_replication field isn't allowed. "+
"Please explicitly destroy this cluster before changing this field.")
}
}

if req.Plan.Raw.IsNull() {
Expand Down Expand Up @@ -1087,7 +1104,7 @@ func (r *clusterResource) Update(
backupUpdateRequest.FrequencyMinutes = ptr(int32(planBackupConfig.FrequencyMinutes.ValueInt64()))
}

if backupUpdateRequest != (client.UpdateBackupConfigurationSpec{}) {
if backupUpdateRequest != (client.UpdateBackupConfigurationSpec{}) {
traceAPICall("UpdateBackupConfiguration")
remoteBackupConfig, _, err = r.provider.service.UpdateBackupConfiguration(ctx, clusterObj.Id, &backupUpdateRequest)
if err != nil {
Expand Down Expand Up @@ -1309,6 +1326,10 @@ func loadClusterToTerraformState(
PrivateNetworkVisibility: types.BoolValue(clusterObj.GetNetworkVisibility() == client.NETWORKVISIBILITYTYPE_PRIVATE),
CidrRange: types.StringValue(clusterObj.CidrRange),
}

if plan != nil && plan.DedicatedConfig != nil && IsKnown(plan.DedicatedConfig.SupportPhysicalClusterReplication) {
state.DedicatedConfig.SupportPhysicalClusterReplication = plan.DedicatedConfig.SupportPhysicalClusterReplication
}
}

var diags diag.Diagnostics
Expand All @@ -1332,12 +1353,11 @@ var backupConfigElementTypes = map[string]attr.Type{
"retention_days": types.Int64Type,
}

func unknownBackupConfig(
) (basetypes.ObjectValue, diag.Diagnostics) {
func unknownBackupConfig() (basetypes.ObjectValue, diag.Diagnostics) {
elements := map[string]attr.Value{
"enabled": types.BoolUnknown(),
"enabled": types.BoolUnknown(),
"frequency_minutes": types.Int64Unknown(),
"retention_days": types.Int64Unknown(),
"retention_days": types.Int64Unknown(),
}
objectValue, diags := types.ObjectValue(backupConfigElementTypes, elements)
return objectValue, diags
Expand All @@ -1347,9 +1367,9 @@ func clientBackupConfigToProviderBackupConfig(
apiBackupConfig *client.BackupConfiguration,
) (basetypes.ObjectValue, diag.Diagnostics) {
elements := map[string]attr.Value{
"enabled": types.BoolValue(apiBackupConfig.GetEnabled()),
"enabled": types.BoolValue(apiBackupConfig.GetEnabled()),
"frequency_minutes": types.Int64Value(int64(apiBackupConfig.GetFrequencyMinutes())),
"retention_days": types.Int64Value(int64(apiBackupConfig.GetRetentionDays())),
"retention_days": types.Int64Value(int64(apiBackupConfig.GetRetentionDays())),
}
objectValue, diags := types.ObjectValue(backupConfigElementTypes, elements)
return objectValue, diags
Expand All @@ -1363,8 +1383,8 @@ func providerBackupConfigToClientBackupConfig(ctx context.Context, providerBacku
}

backupUpdateRequest := &client.BackupConfiguration{
Enabled: planBackupConfig.Enabled.ValueBool(),
RetentionDays: int32(planBackupConfig.RetentionDays.ValueInt64()),
Enabled: planBackupConfig.Enabled.ValueBool(),
RetentionDays: int32(planBackupConfig.RetentionDays.ValueInt64()),
FrequencyMinutes: int32(planBackupConfig.FrequencyMinutes.ValueInt64()),
}
return backupUpdateRequest, diags
Expand Down
15 changes: 8 additions & 7 deletions internal/provider/models.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,13 +41,14 @@ type Region struct {
}

type DedicatedClusterConfig struct {
MachineType types.String `tfsdk:"machine_type"`
NumVirtualCpus types.Int64 `tfsdk:"num_virtual_cpus"`
StorageGib types.Int64 `tfsdk:"storage_gib"`
MemoryGib types.Float64 `tfsdk:"memory_gib"`
DiskIops types.Int64 `tfsdk:"disk_iops"`
PrivateNetworkVisibility types.Bool `tfsdk:"private_network_visibility"`
CidrRange types.String `tfsdk:"cidr_range"`
MachineType types.String `tfsdk:"machine_type"`
NumVirtualCpus types.Int64 `tfsdk:"num_virtual_cpus"`
StorageGib types.Int64 `tfsdk:"storage_gib"`
MemoryGib types.Float64 `tfsdk:"memory_gib"`
DiskIops types.Int64 `tfsdk:"disk_iops"`
PrivateNetworkVisibility types.Bool `tfsdk:"private_network_visibility"`
CidrRange types.String `tfsdk:"cidr_range"`
SupportPhysicalClusterReplication types.Bool `tfsdk:"support_physical_cluster_replication"`
}

type ServerlessClusterConfig struct {
Expand Down
Loading