-
Notifications
You must be signed in to change notification settings - Fork 213
fix(resource/mongodbatlas_advanced_cluster): Emit warning when use_effective_fields and auto-scaling are enabled and spec fields change #4405
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
base: master
Are you sure you want to change the base?
Changes from all commits
71aba31
871784e
7fd265b
f279639
fe61ffd
d17b4d2
8eeeadb
80a7e52
933fb44
c43d6c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| ```release-note:bug | ||
| resource/mongodbatlas_advanced_cluster: Emits a warning during plan when `use_effective_fields` is true, auto-scaling is enabled, and `instance_size`, `disk_size_gb`, or `disk_iops` is modified, as Atlas silently ignores these changes in that combination | ||
| ``` |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,8 @@ package advancedcluster | |||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| import ( | ||||||||||||||||||||||||||||||||||
| "context" | ||||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||||
| "strings" | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| "github.com/hashicorp/terraform-plugin-framework/attr" | ||||||||||||||||||||||||||||||||||
| "github.com/hashicorp/terraform-plugin-framework/diag" | ||||||||||||||||||||||||||||||||||
|
|
@@ -12,6 +14,9 @@ import ( | |||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| var ( | ||||||||||||||||||||||||||||||||||
| // Spec fields that Atlas controls when auto-scaling is active. | ||||||||||||||||||||||||||||||||||
| autoScalingManagedSpecFields = []string{"instance_size", "disk_size_gb", "disk_iops"} | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // Change mappings uses `attribute_name`, it doesn't care about the nested level. | ||||||||||||||||||||||||||||||||||
| attributeRootChangeMapping = map[string][]string{ | ||||||||||||||||||||||||||||||||||
| "replication_specs": {}, | ||||||||||||||||||||||||||||||||||
|
|
@@ -58,6 +63,35 @@ func handleModifyPlan(ctx context.Context, diags *diag.Diagnostics, state, plan | |||||||||||||||||||||||||||||||||
| keepUnknown = append(keepUnknown, attributeChanges.KeepUnknown(attributeRootChangeMapping)...) | ||||||||||||||||||||||||||||||||||
| keepUnknown = append(keepUnknown, determineKeepUnknownsAutoScaling(ctx, diags, state, plan)...) | ||||||||||||||||||||||||||||||||||
| schemafunc.CopyUnknowns(ctx, state, plan, keepUnknown, nil) | ||||||||||||||||||||||||||||||||||
| WarnIgnoredSpecChange(ctx, diags, plan, attributeChanges) | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // WarnIgnoredSpecChange warns when use_effective_fields=true and auto-scaling is enabled but the user | ||||||||||||||||||||||||||||||||||
| // changed instance_size, disk_size_gb, or disk_iops. Atlas silently ignores these changes in that combination | ||||||||||||||||||||||||||||||||||
| func WarnIgnoredSpecChange(ctx context.Context, diags *diag.Diagnostics, plan *TFModel, attributeChanges schemafunc.AttributeChanges) { | ||||||||||||||||||||||||||||||||||
| if !plan.UseEffectiveFields.ValueBool() || !autoScalingUsed(ctx, diags, plan) { | ||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| if attributeChanges.ListLenChanges("replication_specs") || attributeChanges.ListLenChanges("region_configs") { | ||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| var changedFields []string | ||||||||||||||||||||||||||||||||||
| for _, field := range autoScalingManagedSpecFields { | ||||||||||||||||||||||||||||||||||
| if attributeChanges.AttributeChanged(field) { | ||||||||||||||||||||||||||||||||||
| changedFields = append(changedFields, field) | ||||||||||||||||||||||||||||||||||
|
marcabreracast marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| if len(changedFields) == 0 { | ||||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| diags.AddWarning( | ||||||||||||||||||||||||||||||||||
| "Spec change ignored when use_effective_fields is true and auto-scaling is enabled", | ||||||||||||||||||||||||||||||||||
| fmt.Sprintf("Your changes to %s will be stored in Terraform state but will not modify the actual cluster in Atlas. "+ | ||||||||||||||||||||||||||||||||||
| "When use_effective_fields is true and auto-scaling is enabled, Atlas controls instance_size, disk_size_gb, and disk_iops values. "+ | ||||||||||||||||||||||||||||||||||
| "To apply your changes, disable auto-scaling and apply, then re-enable auto-scaling in a separate apply. "+ | ||||||||||||||||||||||||||||||||||
| "See: https://registry.terraform.io/providers/mongodb/mongodbatlas/latest/docs/resources/advanced_cluster#manually-updating-specs-with-use_effective_fields", | ||||||||||||||||||||||||||||||||||
| strings.Join(changedFields, ", ")), | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+87
to
+94
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: style drifts a bit from the existing diagnostics in this file (e.g. the
Suggested change
separately: not sure about having the link as it might get outdated. also please check the message looks fine when multiple fields are changed simultaneously.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 80a7e52. Note that the link is added per the acceptance criteria of the ticket:
If we remove the link, we'd also have to adjust the acceptance criteria
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i see the risk of the link getting outdated but ok to leave |
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // adjustRegionConfigsChildren modifies the planned values of region configs based on the current state. | ||||||||||||||||||||||||||||||||||
|
|
@@ -164,28 +198,26 @@ func findDefinedElectableSpecInReplicationSpec(ctx context.Context, regionConfig | |||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| func determineKeepUnknownsAutoScaling(ctx context.Context, diags *diag.Diagnostics, state, plan *TFModel) []string { | ||||||||||||||||||||||||||||||||||
| if !autoScalingUsed(ctx, diags, state, plan) { | ||||||||||||||||||||||||||||||||||
| if !autoScalingUsed(ctx, diags, state) && !autoScalingUsed(ctx, diags, plan) { | ||||||||||||||||||||||||||||||||||
| return nil | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| // When either compute or disk auto-scaling is enabled, all three fields may be adjusted by Atlas | ||||||||||||||||||||||||||||||||||
| return []string{"instance_size", "disk_size_gb", "disk_iops"} | ||||||||||||||||||||||||||||||||||
| return autoScalingManagedSpecFields | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // autoScalingUsed checks if auto-scaling was enabled (state) or will be enabled (plan). | ||||||||||||||||||||||||||||||||||
| func autoScalingUsed(ctx context.Context, diags *diag.Diagnostics, state, plan *TFModel) bool { | ||||||||||||||||||||||||||||||||||
| for _, model := range []*TFModel{state, plan} { | ||||||||||||||||||||||||||||||||||
| repSpecsTF := TFModelList[TFReplicationSpecsModel](ctx, diags, model.ReplicationSpecs) | ||||||||||||||||||||||||||||||||||
| for i := range repSpecsTF { | ||||||||||||||||||||||||||||||||||
| regiongConfigsTF := TFModelList[TFRegionConfigsModel](ctx, diags, repSpecsTF[i].RegionConfigs) | ||||||||||||||||||||||||||||||||||
| for j := range regiongConfigsTF { | ||||||||||||||||||||||||||||||||||
| for _, autoScalingTF := range []types.Object{regiongConfigsTF[j].AutoScaling, regiongConfigsTF[j].AnalyticsAutoScaling} { | ||||||||||||||||||||||||||||||||||
| autoscaling := TFModelObject[TFAutoScalingModel](ctx, autoScalingTF) | ||||||||||||||||||||||||||||||||||
| if autoscaling == nil { | ||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| if autoscaling.ComputeEnabled.ValueBool() || autoscaling.DiskGBEnabled.ValueBool() { | ||||||||||||||||||||||||||||||||||
| return true | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| // autoScalingUsed checks if auto-scaling is enabled in the given cluster model. | ||||||||||||||||||||||||||||||||||
| func autoScalingUsed(ctx context.Context, diags *diag.Diagnostics, model *TFModel) bool { | ||||||||||||||||||||||||||||||||||
| repSpecsTF := TFModelList[TFReplicationSpecsModel](ctx, diags, model.ReplicationSpecs) | ||||||||||||||||||||||||||||||||||
| for i := range repSpecsTF { | ||||||||||||||||||||||||||||||||||
| regiongConfigsTF := TFModelList[TFRegionConfigsModel](ctx, diags, repSpecsTF[i].RegionConfigs) | ||||||||||||||||||||||||||||||||||
| for j := range regiongConfigsTF { | ||||||||||||||||||||||||||||||||||
| for _, autoScalingTF := range []types.Object{regiongConfigsTF[j].AutoScaling, regiongConfigsTF[j].AnalyticsAutoScaling} { | ||||||||||||||||||||||||||||||||||
| autoscaling := TFModelObject[TFAutoScalingModel](ctx, autoScalingTF) | ||||||||||||||||||||||||||||||||||
| if autoscaling == nil { | ||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| if autoscaling.ComputeEnabled.ValueBool() || autoscaling.DiskGBEnabled.ValueBool() { | ||||||||||||||||||||||||||||||||||
| return true | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,61 @@ | ||
| package advancedcluster_test | ||
|
|
||
| import ( | ||
| "context" | ||
| "testing" | ||
|
|
||
| "github.com/hashicorp/terraform-plugin-framework/attr" | ||
| "github.com/hashicorp/terraform-plugin-framework/diag" | ||
| "github.com/hashicorp/terraform-plugin-framework/types" | ||
| "github.com/hashicorp/terraform-plugin-testing/knownvalue" | ||
| "github.com/hashicorp/terraform-plugin-testing/plancheck" | ||
| "github.com/hashicorp/terraform-plugin-testing/tfjsonpath" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/mongodb/terraform-provider-mongodbatlas/internal/common/schemafunc" | ||
| "github.com/mongodb/terraform-provider-mongodbatlas/internal/service/advancedcluster" | ||
| "github.com/mongodb/terraform-provider-mongodbatlas/internal/testutil/unit" | ||
| ) | ||
|
|
||
| // Attr type maps mirrors the unexported vars in schema.go to avoid uppercasing them. | ||
| // They are redeclared here because this package is advancedcluster_test and cannot access package-private symbols. | ||
| // Done in order to be able to add unit tests for WarnIgnoredSpecChange function, as they allow to test the function without depending on acceptance tests | ||
| var ( | ||
| autoScalingAttrTypes = map[string]attr.Type{ | ||
| "compute_enabled": types.BoolType, | ||
| "compute_max_instance_size": types.StringType, | ||
| "compute_min_instance_size": types.StringType, | ||
| "compute_scale_down_enabled": types.BoolType, | ||
| "disk_gb_enabled": types.BoolType, | ||
| } | ||
| specsAttrTypes = map[string]attr.Type{ | ||
| "disk_iops": types.Int64Type, | ||
| "disk_size_gb": types.Float64Type, | ||
| "ebs_volume_type": types.StringType, | ||
| "instance_size": types.StringType, | ||
| "node_count": types.Int64Type, | ||
| } | ||
| regionConfigAttrTypes = map[string]attr.Type{ | ||
| "analytics_auto_scaling": types.ObjectType{AttrTypes: autoScalingAttrTypes}, | ||
| "analytics_specs": types.ObjectType{AttrTypes: specsAttrTypes}, | ||
| "auto_scaling": types.ObjectType{AttrTypes: autoScalingAttrTypes}, | ||
| "backing_provider_name": types.StringType, | ||
| "electable_specs": types.ObjectType{AttrTypes: specsAttrTypes}, | ||
| "priority": types.Int64Type, | ||
| "provider_name": types.StringType, | ||
| "read_only_specs": types.ObjectType{AttrTypes: specsAttrTypes}, | ||
| "region_name": types.StringType, | ||
| } | ||
| replicationSpecAttrTypes = map[string]attr.Type{ | ||
| "container_id": types.MapType{ElemType: types.StringType}, | ||
| "external_id": types.StringType, | ||
| "region_configs": types.ListType{ElemType: types.ObjectType{AttrTypes: regionConfigAttrTypes}}, | ||
| "zone_id": types.StringType, | ||
| "zone_name": types.StringType, | ||
| } | ||
| ) | ||
|
|
||
| var ( | ||
| repSpec0 = tfjsonpath.New("replication_specs").AtSliceIndex(0) | ||
| repSpec1 = tfjsonpath.New("replication_specs").AtSliceIndex(1) | ||
|
|
@@ -80,3 +126,134 @@ func TestPlanChecksClusterTwoRepSpecsWithAutoScalingAndSpecs(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| func buildPlanWithAutoScaling(t *testing.T, useEffectiveFields, computeEnabled, diskGBEnabled bool) *advancedcluster.TFModel { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. consider changing WarnIgnoredSpecChange signature to take useEffectiveFields and autoScalingEnabled bool instead of ctx and plan *TFModel, moving the autoScalingUsed call to the call site in handleModifyPlan. |
||
| t.Helper() | ||
| ctx := context.Background() | ||
|
|
||
| autoScaling, diags := types.ObjectValueFrom(ctx, autoScalingAttrTypes, advancedcluster.TFAutoScalingModel{ | ||
| ComputeEnabled: types.BoolValue(computeEnabled), | ||
| DiskGBEnabled: types.BoolValue(diskGBEnabled), | ||
| ComputeScaleDownEnabled: types.BoolValue(false), | ||
| ComputeMinInstanceSize: types.StringValue("M10"), | ||
| ComputeMaxInstanceSize: types.StringValue("M40"), | ||
| }) | ||
| require.Empty(t, diags) | ||
|
|
||
| regionConfig := advancedcluster.TFRegionConfigsModel{ | ||
| AutoScaling: autoScaling, | ||
| AnalyticsAutoScaling: types.ObjectNull(autoScalingAttrTypes), | ||
| AnalyticsSpecs: types.ObjectNull(specsAttrTypes), | ||
| ElectableSpecs: types.ObjectNull(specsAttrTypes), | ||
| ReadOnlySpecs: types.ObjectNull(specsAttrTypes), | ||
| ProviderName: types.StringValue("AWS"), | ||
| RegionName: types.StringValue("US_EAST_1"), | ||
| Priority: types.Int64Value(7), | ||
| BackingProviderName: types.StringNull(), | ||
| } | ||
| regionConfigs, diags := types.ListValueFrom(ctx, types.ObjectType{AttrTypes: regionConfigAttrTypes}, []advancedcluster.TFRegionConfigsModel{regionConfig}) | ||
| require.Empty(t, diags) | ||
|
|
||
| repSpec := advancedcluster.TFReplicationSpecsModel{ | ||
| RegionConfigs: regionConfigs, | ||
| ContainerId: types.MapNull(types.StringType), | ||
| ExternalId: types.StringNull(), | ||
| ZoneId: types.StringNull(), | ||
| ZoneName: types.StringNull(), | ||
| } | ||
| repSpecs, diags := types.ListValueFrom(ctx, types.ObjectType{AttrTypes: replicationSpecAttrTypes}, []advancedcluster.TFReplicationSpecsModel{repSpec}) | ||
| require.Empty(t, diags) | ||
|
|
||
| return &advancedcluster.TFModel{ | ||
| UseEffectiveFields: types.BoolValue(useEffectiveFields), | ||
| ReplicationSpecs: repSpecs, | ||
| Labels: types.MapNull(types.StringType), | ||
| Tags: types.MapNull(types.StringType), | ||
| } | ||
| } | ||
|
|
||
| func TestAdvancedCluster_WarnIgnoredSpecChange(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| changedFields schemafunc.AttributeChanges | ||
| useEffectiveFields bool | ||
| computeEnabled bool | ||
| diskGBEnabled bool | ||
| expectWarning bool | ||
| }{ | ||
| { | ||
| name: "warns when compute auto-scaling on and instance_size changed", | ||
| useEffectiveFields: true, | ||
| computeEnabled: true, | ||
| changedFields: schemafunc.AttributeChanges{"instance_size"}, | ||
| expectWarning: true, | ||
| }, | ||
| { | ||
| name: "warns when disk auto-scaling on and disk_size_gb changed", | ||
| useEffectiveFields: true, | ||
| diskGBEnabled: true, | ||
| changedFields: schemafunc.AttributeChanges{"disk_size_gb"}, | ||
| expectWarning: true, | ||
| }, | ||
| { | ||
| name: "warns when compute auto-scaling on and disk_iops changed", | ||
| useEffectiveFields: true, | ||
| computeEnabled: true, | ||
| changedFields: schemafunc.AttributeChanges{"disk_iops"}, | ||
| expectWarning: true, | ||
| }, | ||
| { | ||
| name: "no warning when use_effective_fields is false", | ||
| useEffectiveFields: false, | ||
| computeEnabled: true, | ||
| changedFields: schemafunc.AttributeChanges{"instance_size"}, | ||
| expectWarning: false, | ||
| }, | ||
| { | ||
| name: "no warning when auto-scaling is disabled", | ||
| useEffectiveFields: true, | ||
| computeEnabled: false, | ||
| diskGBEnabled: false, | ||
| changedFields: schemafunc.AttributeChanges{"instance_size"}, | ||
| expectWarning: false, | ||
| }, | ||
| { | ||
| name: "no warning when auto-scaling is on but no managed spec fields changed", | ||
| useEffectiveFields: true, | ||
| computeEnabled: true, | ||
| changedFields: schemafunc.AttributeChanges{"node_count"}, | ||
| expectWarning: false, | ||
| }, | ||
| { | ||
| name: "no warning when replication_specs list length changes (avoid false positive on new spec)", | ||
| useEffectiveFields: true, | ||
| computeEnabled: true, | ||
| changedFields: schemafunc.AttributeChanges{"replication_specs[+1]", "replication_specs[1].region_configs[0].instance_size", "instance_size"}, | ||
| expectWarning: false, | ||
| }, | ||
| { | ||
| name: "no warning when region_configs list length changes (avoid false positive on new region config)", | ||
| useEffectiveFields: true, | ||
| computeEnabled: true, | ||
| changedFields: schemafunc.AttributeChanges{"replication_specs[0].region_configs[+1]", "replication_specs[0].region_configs[1].instance_size", "instance_size"}, | ||
| expectWarning: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| ctx := context.Background() | ||
| plan := buildPlanWithAutoScaling(t, tc.useEffectiveFields, tc.computeEnabled, tc.diskGBEnabled) | ||
| var diags diag.Diagnostics | ||
|
|
||
| advancedcluster.WarnIgnoredSpecChange(ctx, &diags, plan, tc.changedFields) | ||
|
|
||
| assert.False(t, diags.HasError()) | ||
| if tc.expectWarning { | ||
| assert.Equal(t, 1, diags.WarningsCount()) | ||
| } else { | ||
| assert.Equal(t, 0, diags.WarningsCount()) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q:
autoScalingUsedORsauto_scalingandanalytics_auto_scaling, but per Atlas behaviorauto_scaling.compute_enabledonly governs electable/read_only specs andanalytics_auto_scaling.compute_enabledonly governs analytics specs (disk is governed by the regular block alone). soauto_scaling.compute_enabled = false+analytics_auto_scaling.compute_enabled = true+ change toelectable_specs.instance_size-> warning fires, but Atlas honors the changeis this v1 over-approximation acceptable (worth a follow-up ticket), or worth scoping the check per-block before merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with the approach of trying to scope this check before the merge so I can bring it to the tech sync and align with the team. Working on investigating the possibility of this