diff --git a/.changelog/3105.txt b/.changelog/3105.txt new file mode 100644 index 0000000000..67a00812f4 --- /dev/null +++ b/.changelog/3105.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/mongodbatlas_project: Fixes import when `with_default_alerts_settings` is set +``` diff --git a/internal/common/customplanmodifier/create_only.go b/internal/common/customplanmodifier/create_only.go new file mode 100644 index 0000000000..65e664b306 --- /dev/null +++ b/internal/common/customplanmodifier/create_only.go @@ -0,0 +1,90 @@ +package customplanmodifier + +import ( + "context" + "fmt" + + "github.com/hashicorp/terraform-plugin-framework/attr" + "github.com/hashicorp/terraform-plugin-framework/diag" + "github.com/hashicorp/terraform-plugin-framework/path" + planmodifier "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" + "github.com/hashicorp/terraform-plugin-framework/tfsdk" + "github.com/hashicorp/terraform-plugin-framework/types" +) + +type Modifier interface { + planmodifier.String + planmodifier.Bool +} + +// CreateOnlyAttributePlanModifier returns a plan modifier that ensures that update operations fails when the attribute is changed. +// This is useful for attributes only supported in create and not in update. +// Never use a schema.Default for create only attributes, instead use WithXXXDefault, the default will lead to plan changes that are not expected after import. +// Implement CopyFromPlan if the attribute is not in the API Response. +func CreateOnlyAttributePlanModifier() Modifier { + return &createOnlyAttributePlanModifier{} +} + +func CreateOnlyAttributePlanModifierWithBoolDefault(b bool) Modifier { + return &createOnlyAttributePlanModifier{defaultBool: &b} +} + +type createOnlyAttributePlanModifier struct { + defaultBool *bool +} + +func (d *createOnlyAttributePlanModifier) Description(ctx context.Context) string { + return d.MarkdownDescription(ctx) +} + +func (d *createOnlyAttributePlanModifier) MarkdownDescription(ctx context.Context) string { + return "Ensures the update operation fails when updating an attribute. If the read after import don't equal the configuration value it will also raise an error." +} + +func isCreate(t *tfsdk.State) bool { + return t.Raw.IsNull() +} + +func (d *createOnlyAttributePlanModifier) UseDefault() bool { + return d.defaultBool != nil +} + +func (d *createOnlyAttributePlanModifier) PlanModifyBool(ctx context.Context, req planmodifier.BoolRequest, resp *planmodifier.BoolResponse) { + if isCreate(&req.State) { + if !IsKnown(req.PlanValue) && d.UseDefault() { + resp.PlanValue = types.BoolPointerValue(d.defaultBool) + } + return + } + if isUpdated(req.StateValue, req.PlanValue) { + d.addDiags(&resp.Diagnostics, req.Path, req.StateValue) + } + if !IsKnown(req.PlanValue) { + resp.PlanValue = req.StateValue + } +} + +func (d *createOnlyAttributePlanModifier) PlanModifyString(ctx context.Context, req planmodifier.StringRequest, resp *planmodifier.StringResponse) { + if isCreate(&req.State) { + return + } + if isUpdated(req.StateValue, req.PlanValue) { + d.addDiags(&resp.Diagnostics, req.Path, req.StateValue) + } + if !IsKnown(req.PlanValue) { + resp.PlanValue = req.StateValue + } +} + +func isUpdated(state, plan attr.Value) bool { + if !IsKnown(plan) { + return false + } + return !state.Equal(plan) +} + +func (d *createOnlyAttributePlanModifier) addDiags(diags *diag.Diagnostics, attrPath path.Path, stateValue attr.Value) { + message := fmt.Sprintf("%s cannot be updated or set after import, remove it from the configuration or use state value.", attrPath) + detail := fmt.Sprintf("The current state value is %s", stateValue) + diags.AddError(message, detail) +} diff --git a/internal/common/customplanmodifier/is_known.go b/internal/common/customplanmodifier/is_known.go new file mode 100644 index 0000000000..8eedd62889 --- /dev/null +++ b/internal/common/customplanmodifier/is_known.go @@ -0,0 +1,8 @@ +package customplanmodifier + +import "github.com/hashicorp/terraform-plugin-framework/attr" + +// IsKnown returns true if the attribute is known (not null or unknown). Note that !IsKnown is not the same as IsUnknown because null is !IsKnown but not IsUnknown. +func IsKnown(attribute attr.Value) bool { + return !attribute.IsNull() && !attribute.IsUnknown() +} diff --git a/internal/common/customplanmodifier/non_updatable.go b/internal/common/customplanmodifier/non_updatable.go deleted file mode 100644 index 7f47282bb1..0000000000 --- a/internal/common/customplanmodifier/non_updatable.go +++ /dev/null @@ -1,36 +0,0 @@ -package customplanmodifier - -import ( - "context" - "fmt" - - planmodifier "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" -) - -func NonUpdatableStringAttributePlanModifier() planmodifier.String { - return &nonUpdatableStringAttributePlanModifier{} -} - -type nonUpdatableStringAttributePlanModifier struct { -} - -func (d *nonUpdatableStringAttributePlanModifier) Description(ctx context.Context) string { - return d.MarkdownDescription(ctx) -} - -func (d *nonUpdatableStringAttributePlanModifier) MarkdownDescription(ctx context.Context) string { - return "Ensures that update operations fails when updating an attribute." -} - -func (d *nonUpdatableStringAttributePlanModifier) PlanModifyString(ctx context.Context, req planmodifier.StringRequest, resp *planmodifier.StringResponse) { - planAttributeValue := req.PlanValue - stateAttributeValue := req.StateValue - - if !stateAttributeValue.IsNull() && stateAttributeValue.ValueString() != planAttributeValue.ValueString() { - resp.Diagnostics.AddError( - fmt.Sprintf("%s cannot be updated", req.Path), - fmt.Sprintf("%s cannot be updated", req.Path), - ) - return - } -} diff --git a/internal/service/advancedclustertpf/plan_modifier.go b/internal/service/advancedclustertpf/plan_modifier.go index d430624d3f..f214d11dd4 100644 --- a/internal/service/advancedclustertpf/plan_modifier.go +++ b/internal/service/advancedclustertpf/plan_modifier.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/diag" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-framework/types/basetypes" + "github.com/mongodb/terraform-provider-mongodbatlas/internal/common/customplanmodifier" "github.com/mongodb/terraform-provider-mongodbatlas/internal/common/schemafunc" ) @@ -266,16 +267,11 @@ func TFModelObject[T any](ctx context.Context, input types.Object) *T { } func copyAttrIfDestNotKnown[T attr.Value](src, dest *T) { - if !isKnown(*dest) { + if !customplanmodifier.IsKnown(*dest) { *dest = *src } } -// isKnown returns true if the attribute is known (not null or unknown). Note that !isKnown is not the same as IsUnknown because null is !isKnown but not IsUnknown. -func isKnown(attribute attr.Value) bool { - return !attribute.IsNull() && !attribute.IsUnknown() -} - func minLen[T any](a, b []T) int { la, lb := len(a), len(b) if la < lb { diff --git a/internal/service/flexcluster/resource_schema.go b/internal/service/flexcluster/resource_schema.go index 08f5a9f471..fc3afe6430 100644 --- a/internal/service/flexcluster/resource_schema.go +++ b/internal/service/flexcluster/resource_schema.go @@ -21,14 +21,14 @@ func ResourceSchema(ctx context.Context) schema.Schema { "project_id": schema.StringAttribute{ Required: true, PlanModifiers: []planmodifier.String{ - customplanmodifier.NonUpdatableStringAttributePlanModifier(), + customplanmodifier.CreateOnlyAttributePlanModifier(), }, MarkdownDescription: "Unique 24-hexadecimal character string that identifies the project.", }, "name": schema.StringAttribute{ Required: true, PlanModifiers: []planmodifier.String{ - customplanmodifier.NonUpdatableStringAttributePlanModifier(), + customplanmodifier.CreateOnlyAttributePlanModifier(), }, MarkdownDescription: "Human-readable label that identifies the instance.", }, @@ -37,7 +37,7 @@ func ResourceSchema(ctx context.Context) schema.Schema { "backing_provider_name": schema.StringAttribute{ Required: true, PlanModifiers: []planmodifier.String{ - customplanmodifier.NonUpdatableStringAttributePlanModifier(), + customplanmodifier.CreateOnlyAttributePlanModifier(), }, MarkdownDescription: "Cloud service provider on which MongoDB Cloud provisioned the flex cluster.", }, @@ -58,7 +58,7 @@ func ResourceSchema(ctx context.Context) schema.Schema { "region_name": schema.StringAttribute{ Required: true, PlanModifiers: []planmodifier.String{ - customplanmodifier.NonUpdatableStringAttributePlanModifier(), + customplanmodifier.CreateOnlyAttributePlanModifier(), }, MarkdownDescription: "Human-readable label that identifies the geographic location of your MongoDB flex cluster. The region you choose can affect network latency for clients accessing your databases. For a complete list of region names, see [AWS](https://docs.atlas.mongodb.com/reference/amazon-aws/#std-label-amazon-aws), [GCP](https://docs.atlas.mongodb.com/reference/google-gcp/), and [Azure](https://docs.atlas.mongodb.com/reference/microsoft-azure/).", }, diff --git a/internal/service/project/resource_project_migration_test.go b/internal/service/project/resource_project_migration_test.go index 6e7d033f92..3d8100000b 100644 --- a/internal/service/project/resource_project_migration_test.go +++ b/internal/service/project/resource_project_migration_test.go @@ -84,7 +84,7 @@ func TestMigProject_withFalseDefaultSettings(t *testing.T) { orgID = os.Getenv("MONGODB_ATLAS_ORG_ID") projectOwnerID = os.Getenv("MONGODB_ATLAS_PROJECT_OWNER_ID") projectName = acc.RandomProjectName() - config = configWithFalseDefaultSettings(orgID, projectName, projectOwnerID) + config = configWithDefaultAlertSettings(orgID, projectName, projectOwnerID, false) ) resource.Test(t, resource.TestCase{ diff --git a/internal/service/project/resource_project_schema.go b/internal/service/project/resource_project_schema.go index 81c07081eb..bd17dc691c 100644 --- a/internal/service/project/resource_project_schema.go +++ b/internal/service/project/resource_project_schema.go @@ -6,7 +6,6 @@ import ( "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/resource/schema" - "github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault" "github.com/hashicorp/terraform-plugin-framework/resource/schema/boolplanmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/int64planmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/objectplanmodifier" @@ -15,6 +14,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" "github.com/hashicorp/terraform-plugin-framework/types" "github.com/mongodb/terraform-provider-mongodbatlas/internal/common/constant" + "github.com/mongodb/terraform-provider-mongodbatlas/internal/common/customplanmodifier" "go.mongodb.org/atlas-sdk/v20250219001/admin" ) @@ -53,10 +53,10 @@ func ResourceSchema(ctx context.Context) schema.Schema { }, "with_default_alerts_settings": schema.BoolAttribute{ // Default values also must be Computed otherwise Terraform throws error: - // Schema Using Attribute Default For Non-Computed Attribute - Optional: true, - Computed: true, - Default: booldefault.StaticBool(true), + // Provider produced invalid plan: planned an invalid value for a non-computed attribute. + Optional: true, + Computed: true, + PlanModifiers: []planmodifier.Bool{customplanmodifier.CreateOnlyAttributePlanModifierWithBoolDefault(true)}, }, "is_collect_database_specifics_statistics_enabled": schema.BoolAttribute{ Computed: true, diff --git a/internal/service/project/resource_project_test.go b/internal/service/project/resource_project_test.go index 45ba6109aa..d423f45355 100644 --- a/internal/service/project/resource_project_test.go +++ b/internal/service/project/resource_project_test.go @@ -22,6 +22,7 @@ import ( "github.com/mongodb/terraform-provider-mongodbatlas/internal/common/conversion" "github.com/mongodb/terraform-provider-mongodbatlas/internal/service/project" "github.com/mongodb/terraform-provider-mongodbatlas/internal/testutil/acc" + "github.com/mongodb/terraform-provider-mongodbatlas/internal/testutil/mig" ) var ( @@ -647,9 +648,20 @@ func TestAccGovProject_withProjectOwner(t *testing.T) { func TestAccProject_withFalseDefaultSettings(t *testing.T) { var ( - orgID = os.Getenv("MONGODB_ATLAS_ORG_ID") - projectOwnerID = os.Getenv("MONGODB_ATLAS_PROJECT_OWNER_ID") - projectName = acc.RandomProjectName() + orgID = os.Getenv("MONGODB_ATLAS_ORG_ID") + projectOwnerID = os.Getenv("MONGODB_ATLAS_PROJECT_OWNER_ID") + projectName = acc.RandomProjectName() + importResourceName = resourceName + "2" + alertSettingsFalse = configWithDefaultAlertSettings(orgID, projectName, projectOwnerID, false) + alertSettingsTrue = configWithDefaultAlertSettings(orgID, projectName, projectOwnerID, true) + // To test plan behavior after import it is necessary to use a different resource name, otherwise we get: + // Terraform is already managing a remote object for mongodbatlas_project.test. To import to this address you must first remove the existing object from the state. + // This happens because `ImportStatePersist` uses the previous WorkingDirectory where the state from previous steps are saved + // resource "mongodbatlas_project" "test" --> resource "mongodbatlas_project" "test2" + alertSettingsFalseImport = strings.Replace(alertSettingsFalse, "test", "test2", 1) + // Need BOTH mongodbatlas_project.test and mongodbatlas_project.test2, otherwise we get: + // expected empty plan, but mongodbatlas_project.test has planned action(s): [delete] + alertSettingsAbsent = alertSettingsFalse + strings.Replace(configBasic(orgID, projectName, "", false, nil, nil), "test", "test2", 1) ) resource.ParallelTest(t, resource.TestCase{ @@ -658,13 +670,32 @@ func TestAccProject_withFalseDefaultSettings(t *testing.T) { CheckDestroy: acc.CheckDestroyProject, Steps: []resource.TestStep{ { - Config: configWithFalseDefaultSettings(orgID, projectName, projectOwnerID), + Config: alertSettingsFalse, Check: resource.ComposeAggregateTestCheckFunc( checkExists(resourceName), resource.TestCheckResourceAttr(resourceName, "name", projectName), resource.TestCheckResourceAttr(resourceName, "org_id", orgID), ), }, + { + Config: alertSettingsTrue, + ExpectError: regexp.MustCompile("with_default_alerts_settings cannot be updated or set after import, remove it from the configuration or use state value"), + }, + { + Config: alertSettingsFalseImport, + ResourceName: importResourceName, + ImportStateIdFunc: acc.ImportStateProjectIDFunc(resourceName), + ImportState: true, + ImportStatePersist: true, // save the state to use it in the next plan + }, + { + Config: alertSettingsFalseImport, // when the value is set after import, the first plan will fail since the value cannot be read from API and the plan modifier will detect the change from null --> false + ExpectError: regexp.MustCompile("with_default_alerts_settings cannot be updated or set after import, remove it from the configuration or use state value"), + }, + { + Config: alertSettingsAbsent, // removing `with_default_alerts_settings` from the configuration should have no plan changes + ConfigPlanChecks: mig.TestStepConfigPlanChecksEmptyPlan(), + }, }, }) } @@ -688,7 +719,7 @@ func TestAccProject_withUpdatedSettings(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "name", projectName), resource.TestCheckResourceAttr(resourceName, "org_id", orgID), resource.TestCheckResourceAttr(resourceName, "project_owner_id", projectOwnerID), - resource.TestCheckResourceAttr(resourceName, "with_default_alerts_settings", "false"), + resource.TestCheckResourceAttr(resourceName, "with_default_alerts_settings", "true"), // uses default value resource.TestCheckResourceAttr(resourceName, "is_collect_database_specifics_statistics_enabled", "false"), resource.TestCheckResourceAttr(resourceName, "is_data_explorer_enabled", "false"), resource.TestCheckResourceAttr(resourceName, "is_extended_storage_sizes_enabled", "false"), @@ -701,7 +732,7 @@ func TestAccProject_withUpdatedSettings(t *testing.T) { Config: acc.ConfigProjectWithSettings(projectName, orgID, projectOwnerID, true), Check: resource.ComposeAggregateTestCheckFunc( checkExists(resourceName), - resource.TestCheckResourceAttr(resourceName, "with_default_alerts_settings", "true"), + resource.TestCheckResourceAttr(resourceName, "with_default_alerts_settings", "true"), // uses default value resource.TestCheckResourceAttr(resourceName, "is_collect_database_specifics_statistics_enabled", "true"), resource.TestCheckResourceAttr(resourceName, "is_data_explorer_enabled", "true"), resource.TestCheckResourceAttr(resourceName, "is_extended_storage_sizes_enabled", "true"), @@ -714,7 +745,7 @@ func TestAccProject_withUpdatedSettings(t *testing.T) { Config: acc.ConfigProjectWithSettings(projectName, orgID, projectOwnerID, false), Check: resource.ComposeAggregateTestCheckFunc( checkExists(resourceName), - resource.TestCheckResourceAttr(resourceName, "with_default_alerts_settings", "false"), + resource.TestCheckResourceAttr(resourceName, "with_default_alerts_settings", "true"), // uses default value resource.TestCheckResourceAttr(resourceName, "is_collect_database_specifics_statistics_enabled", "false"), resource.TestCheckResourceAttr(resourceName, "is_data_explorer_enabled", "false"), resource.TestCheckResourceAttr(resourceName, "is_extended_storage_sizes_enabled", "false"), @@ -1233,15 +1264,15 @@ func configGovWithOwner(orgID, projectName, projectOwnerID string) string { `, orgID, projectName, projectOwnerID) } -func configWithFalseDefaultSettings(orgID, projectName, projectOwnerID string) string { +func configWithDefaultAlertSettings(orgID, projectName, projectOwnerID string, withDefaultAlertsSettings bool) string { return fmt.Sprintf(` resource "mongodbatlas_project" "test" { org_id = %[1]q name = %[2]q project_owner_id = %[3]q - with_default_alerts_settings = false + with_default_alerts_settings = %[4]t } - `, orgID, projectName, projectOwnerID) + `, orgID, projectName, projectOwnerID, withDefaultAlertsSettings) } func configWithLimits(orgID, projectName string, limits []*admin.DataFederationLimit) string { diff --git a/internal/testutil/acc/project.go b/internal/testutil/acc/project.go index 8885fb7253..c1d902ed4c 100644 --- a/internal/testutil/acc/project.go +++ b/internal/testutil/acc/project.go @@ -36,7 +36,6 @@ func ConfigProjectWithSettings(projectName, orgID, projectOwnerID string, value name = %[1]q org_id = %[2]q project_owner_id = %[3]q - with_default_alerts_settings = %[4]t is_collect_database_specifics_statistics_enabled = %[4]t is_data_explorer_enabled = %[4]t is_extended_storage_sizes_enabled = %[4]t diff --git a/internal/testutil/mig/test_step.go b/internal/testutil/mig/test_step.go index 4de74c2f24..b116b5f1f6 100644 --- a/internal/testutil/mig/test_step.go +++ b/internal/testutil/mig/test_step.go @@ -10,11 +10,15 @@ func TestStepCheckEmptyPlan(config string) resource.TestStep { return resource.TestStep{ ProtoV6ProviderFactories: acc.TestAccProviderV6Factories, Config: config, - ConfigPlanChecks: resource.ConfigPlanChecks{ - PreApply: []plancheck.PlanCheck{ - acc.DebugPlan(), - plancheck.ExpectEmptyPlan(), - }, + ConfigPlanChecks: TestStepConfigPlanChecksEmptyPlan(), + } +} + +func TestStepConfigPlanChecksEmptyPlan() resource.ConfigPlanChecks { + return resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + acc.DebugPlan(), + plancheck.ExpectEmptyPlan(), }, } }