From 917e11e9ee721e1b967b94f9e2c16357bfeb4ad0 Mon Sep 17 00:00:00 2001 From: max-ts0gt Date: Fri, 22 Nov 2024 11:34:13 +0900 Subject: [PATCH 1/6] r/chatbot: Fix SNS topic ARNs ordering in Slack channel configuration The provider was being too strict about the order of SNS topic ARNs in the Slack channel configuration resource. This could cause unnecessary diffs in the Terraform plan when AWS returned the topics in a different order. This change implements sorting of SNS topic ARNs before comparison to ensure consistent behavior regardless of the order returned by the API. --- .../chatbot/slack_channel_configuration.go | 46 ++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/internal/service/chatbot/slack_channel_configuration.go b/internal/service/chatbot/slack_channel_configuration.go index 06e5c259f305..04ff94922520 100644 --- a/internal/service/chatbot/slack_channel_configuration.go +++ b/internal/service/chatbot/slack_channel_configuration.go @@ -5,6 +5,8 @@ package chatbot import ( "context" + "fmt" + "sort" "time" "github.com/aws/aws-sdk-go-v2/aws" @@ -435,7 +437,26 @@ func (loggingLevel) Values() []loggingLevel { } } -func slackChannelConfigurationHasChanges(_ context.Context, plan, state slackChannelConfigurationResourceModel) bool { +func slackChannelConfigurationHasChanges(ctx context.Context, plan, state slackChannelConfigurationResourceModel) bool { + // Sort SNS Topic ARNs before comparison + planSNSTopicARNs, err := sortSNSTopicARNs(ctx, plan.SNSTopicARNs) + if err != nil { + // Log error and fall back to direct comparison + tflog.Warn(ctx, "Failed to sort plan SNS Topic ARNs", map[string]interface{}{ + "error": err.Error(), + }) + planSNSTopicARNs = plan.SNSTopicARNs + } + + stateSNSTopicARNs, err := sortSNSTopicARNs(ctx, state.SNSTopicARNs) + if err != nil { + // Log error and fall back to direct comparison + tflog.Warn(ctx, "Failed to sort state SNS Topic ARNs", map[string]interface{}{ + "error": err.Error(), + }) + stateSNSTopicARNs = state.SNSTopicARNs + } + return !plan.ChatConfigurationARN.Equal(state.ChatConfigurationARN) || !plan.ConfigurationName.Equal(state.ConfigurationName) || !plan.GuardrailPolicyARNs.Equal(state.GuardrailPolicyARNs) || @@ -445,6 +466,27 @@ func slackChannelConfigurationHasChanges(_ context.Context, plan, state slackCha !plan.SlackChannelName.Equal(state.SlackChannelName) || !plan.SlackTeamID.Equal(state.SlackTeamID) || !plan.SlackTeamName.Equal(state.SlackTeamName) || - !plan.SNSTopicARNs.Equal(state.SNSTopicARNs) || + !planSNSTopicARNs.Equal(stateSNSTopicARNs) || !plan.UserAuthorizationRequired.Equal(state.UserAuthorizationRequired) } + +func sortSNSTopicARNs(ctx context.Context, arns types.List) (types.List, error) { + if arns.IsNull() || arns.IsUnknown() { + return arns, nil + } + + var arnStrings []string + diags := arns.ElementsAs(ctx, &arnStrings, false) + if diags.HasError() { + return arns, fmt.Errorf("error converting SNS Topic ARNs to strings: %v", diags) + } + + sort.Strings(arnStrings) + + sortedArns, diags := types.ListValueFrom(ctx, types.StringType, arnStrings) + if diags.HasError() { + return arns, fmt.Errorf("error converting sorted strings to List: %v", diags) + } + + return sortedArns, nil +} From d128c4e1979f8331730613a092bf87962729a8cd Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Fri, 22 Nov 2024 10:04:33 -0600 Subject: [PATCH 2/6] aws_chatbot_slack_channel_configuration: use flex HasChanges() --- .../chatbot/slack_channel_configuration.go | 72 ++++++++++--------- 1 file changed, 39 insertions(+), 33 deletions(-) diff --git a/internal/service/chatbot/slack_channel_configuration.go b/internal/service/chatbot/slack_channel_configuration.go index 04ff94922520..51fa4b7564c3 100644 --- a/internal/service/chatbot/slack_channel_configuration.go +++ b/internal/service/chatbot/slack_channel_configuration.go @@ -220,7 +220,13 @@ func (r *slackChannelConfigurationResource) Update(ctx context.Context, request conn := r.Meta().ChatbotClient(ctx) - if slackChannelConfigurationHasChanges(ctx, new, old) { + diff, d := fwflex.Calculate(ctx, new, old) + response.Diagnostics.Append(d...) + if response.Diagnostics.HasError() { + return + } + + if diff.HasChanges() { input := &chatbot.UpdateSlackChannelConfigurationInput{} response.Diagnostics.Append(fwflex.Expand(ctx, new, input)...) if response.Diagnostics.HasError() { @@ -437,38 +443,38 @@ func (loggingLevel) Values() []loggingLevel { } } -func slackChannelConfigurationHasChanges(ctx context.Context, plan, state slackChannelConfigurationResourceModel) bool { - // Sort SNS Topic ARNs before comparison - planSNSTopicARNs, err := sortSNSTopicARNs(ctx, plan.SNSTopicARNs) - if err != nil { - // Log error and fall back to direct comparison - tflog.Warn(ctx, "Failed to sort plan SNS Topic ARNs", map[string]interface{}{ - "error": err.Error(), - }) - planSNSTopicARNs = plan.SNSTopicARNs - } - - stateSNSTopicARNs, err := sortSNSTopicARNs(ctx, state.SNSTopicARNs) - if err != nil { - // Log error and fall back to direct comparison - tflog.Warn(ctx, "Failed to sort state SNS Topic ARNs", map[string]interface{}{ - "error": err.Error(), - }) - stateSNSTopicARNs = state.SNSTopicARNs - } - - return !plan.ChatConfigurationARN.Equal(state.ChatConfigurationARN) || - !plan.ConfigurationName.Equal(state.ConfigurationName) || - !plan.GuardrailPolicyARNs.Equal(state.GuardrailPolicyARNs) || - !plan.IAMRoleARN.Equal(state.IAMRoleARN) || - !plan.LoggingLevel.Equal(state.LoggingLevel) || - !plan.SlackChannelID.Equal(state.SlackChannelID) || - !plan.SlackChannelName.Equal(state.SlackChannelName) || - !plan.SlackTeamID.Equal(state.SlackTeamID) || - !plan.SlackTeamName.Equal(state.SlackTeamName) || - !planSNSTopicARNs.Equal(stateSNSTopicARNs) || - !plan.UserAuthorizationRequired.Equal(state.UserAuthorizationRequired) -} +//func slackChannelConfigurationHasChanges(ctx context.Context, plan, state slackChannelConfigurationResourceModel) bool { +// // Sort SNS Topic ARNs before comparison +// planSNSTopicARNs, err := sortSNSTopicARNs(ctx, plan.SNSTopicARNs) +// if err != nil { +// // Log error and fall back to direct comparison +// tflog.Warn(ctx, "Failed to sort plan SNS Topic ARNs", map[string]interface{}{ +// "error": err.Error(), +// }) +// planSNSTopicARNs = plan.SNSTopicARNs +// } +// +// stateSNSTopicARNs, err := sortSNSTopicARNs(ctx, state.SNSTopicARNs) +// if err != nil { +// // Log error and fall back to direct comparison +// tflog.Warn(ctx, "Failed to sort state SNS Topic ARNs", map[string]interface{}{ +// "error": err.Error(), +// }) +// stateSNSTopicARNs = state.SNSTopicARNs +// } +// +// return !plan.ChatConfigurationARN.Equal(state.ChatConfigurationARN) || +// !plan.ConfigurationName.Equal(state.ConfigurationName) || +// !plan.GuardrailPolicyARNs.Equal(state.GuardrailPolicyARNs) || +// !plan.IAMRoleARN.Equal(state.IAMRoleARN) || +// !plan.LoggingLevel.Equal(state.LoggingLevel) || +// !plan.SlackChannelID.Equal(state.SlackChannelID) || +// !plan.SlackChannelName.Equal(state.SlackChannelName) || +// !plan.SlackTeamID.Equal(state.SlackTeamID) || +// !plan.SlackTeamName.Equal(state.SlackTeamName) || +// !planSNSTopicARNs.Equal(stateSNSTopicARNs) || +// !plan.UserAuthorizationRequired.Equal(state.UserAuthorizationRequired) +//} func sortSNSTopicARNs(ctx context.Context, arns types.List) (types.List, error) { if arns.IsNull() || arns.IsUnknown() { From cac3107030e88c14da7db0e4ad23078fbf34c950 Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Fri, 22 Nov 2024 10:13:48 -0600 Subject: [PATCH 3/6] aws_chatbot_slack_channel_configuration: use flex custom types --- .../chatbot/slack_channel_configuration.go | 89 +++++++++---------- 1 file changed, 44 insertions(+), 45 deletions(-) diff --git a/internal/service/chatbot/slack_channel_configuration.go b/internal/service/chatbot/slack_channel_configuration.go index 51fa4b7564c3..dcbeb5a05ef9 100644 --- a/internal/service/chatbot/slack_channel_configuration.go +++ b/internal/service/chatbot/slack_channel_configuration.go @@ -5,8 +5,6 @@ package chatbot import ( "context" - "fmt" - "sort" "time" "github.com/aws/aws-sdk-go-v2/aws" @@ -21,6 +19,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/resource/schema/boolplanmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/listplanmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier" + "github.com/hashicorp/terraform-plugin-framework/resource/schema/setplanmodifier" "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringdefault" "github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier" "github.com/hashicorp/terraform-plugin-framework/types" @@ -66,9 +65,9 @@ func (r *slackChannelConfigurationResource) Schema(ctx context.Context, request Required: true, }, "guardrail_policy_arns": schema.ListAttribute{ - Optional: true, - Computed: true, - ElementType: types.StringType, + CustomType: fwtypes.ListOfStringType, + Optional: true, + Computed: true, PlanModifiers: []planmodifier.List{ listplanmodifier.UseStateForUnknown(), }, @@ -97,12 +96,12 @@ func (r *slackChannelConfigurationResource) Schema(ctx context.Context, request "slack_team_name": schema.StringAttribute{ Computed: true, }, - "sns_topic_arns": schema.ListAttribute{ - Optional: true, - Computed: true, - ElementType: types.StringType, - PlanModifiers: []planmodifier.List{ - listplanmodifier.UseStateForUnknown(), + "sns_topic_arns": schema.SetAttribute{ + CustomType: fwtypes.SetOfStringType, + Optional: true, + Computed: true, + PlanModifiers: []planmodifier.Set{ + setplanmodifier.UseStateForUnknown(), }, }, names.AttrTags: tftags.TagsAttribute(), @@ -406,20 +405,20 @@ func waitSlackChannelConfigurationDeleted(ctx context.Context, conn *chatbot.Cli } type slackChannelConfigurationResourceModel struct { - ChatConfigurationARN types.String `tfsdk:"chat_configuration_arn"` - ConfigurationName types.String `tfsdk:"configuration_name"` - GuardrailPolicyARNs types.List `tfsdk:"guardrail_policy_arns"` - IAMRoleARN types.String `tfsdk:"iam_role_arn"` - LoggingLevel fwtypes.StringEnum[loggingLevel] `tfsdk:"logging_level"` - SlackChannelID types.String `tfsdk:"slack_channel_id"` - SlackChannelName types.String `tfsdk:"slack_channel_name"` - SlackTeamID types.String `tfsdk:"slack_team_id"` - SlackTeamName types.String `tfsdk:"slack_team_name"` - SNSTopicARNs types.List `tfsdk:"sns_topic_arns"` - Tags tftags.Map `tfsdk:"tags"` - TagsAll tftags.Map `tfsdk:"tags_all"` - Timeouts timeouts.Value `tfsdk:"timeouts"` - UserAuthorizationRequired types.Bool `tfsdk:"user_authorization_required"` + ChatConfigurationARN types.String `tfsdk:"chat_configuration_arn"` + ConfigurationName types.String `tfsdk:"configuration_name"` + GuardrailPolicyARNs fwtypes.ListValueOf[types.String] `tfsdk:"guardrail_policy_arns"` + IAMRoleARN types.String `tfsdk:"iam_role_arn"` + LoggingLevel fwtypes.StringEnum[loggingLevel] `tfsdk:"logging_level"` + SlackChannelID types.String `tfsdk:"slack_channel_id"` + SlackChannelName types.String `tfsdk:"slack_channel_name"` + SlackTeamID types.String `tfsdk:"slack_team_id"` + SlackTeamName types.String `tfsdk:"slack_team_name"` + SNSTopicARNs fwtypes.SetValueOf[types.String] `tfsdk:"sns_topic_arns"` + Tags tftags.Map `tfsdk:"tags"` + TagsAll tftags.Map `tfsdk:"tags_all"` + Timeouts timeouts.Value `tfsdk:"timeouts"` + UserAuthorizationRequired types.Bool `tfsdk:"user_authorization_required"` } func (data *slackChannelConfigurationResourceModel) InitFromID() error { @@ -476,23 +475,23 @@ func (loggingLevel) Values() []loggingLevel { // !plan.UserAuthorizationRequired.Equal(state.UserAuthorizationRequired) //} -func sortSNSTopicARNs(ctx context.Context, arns types.List) (types.List, error) { - if arns.IsNull() || arns.IsUnknown() { - return arns, nil - } - - var arnStrings []string - diags := arns.ElementsAs(ctx, &arnStrings, false) - if diags.HasError() { - return arns, fmt.Errorf("error converting SNS Topic ARNs to strings: %v", diags) - } - - sort.Strings(arnStrings) - - sortedArns, diags := types.ListValueFrom(ctx, types.StringType, arnStrings) - if diags.HasError() { - return arns, fmt.Errorf("error converting sorted strings to List: %v", diags) - } - - return sortedArns, nil -} +//func sortSNSTopicARNs(ctx context.Context, arns types.List) (types.List, error) { +// if arns.IsNull() || arns.IsUnknown() { +// return arns, nil +// } +// +// var arnStrings []string +// diags := arns.ElementsAs(ctx, &arnStrings, false) +// if diags.HasError() { +// return arns, fmt.Errorf("error converting SNS Topic ARNs to strings: %v", diags) +// } +// +// sort.Strings(arnStrings) +// +// sortedArns, diags := types.ListValueFrom(ctx, types.StringType, arnStrings) +// if diags.HasError() { +// return arns, fmt.Errorf("error converting sorted strings to List: %v", diags) +// } +// +// return sortedArns, nil +//} From 205bb54d5f871bbfc0d0ae95f6d2b04e071709ce Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Fri, 22 Nov 2024 10:30:22 -0600 Subject: [PATCH 4/6] chore: cleanup unused functions --- .../chatbot/slack_channel_configuration.go | 54 ------------------- 1 file changed, 54 deletions(-) diff --git a/internal/service/chatbot/slack_channel_configuration.go b/internal/service/chatbot/slack_channel_configuration.go index dcbeb5a05ef9..472ea430539e 100644 --- a/internal/service/chatbot/slack_channel_configuration.go +++ b/internal/service/chatbot/slack_channel_configuration.go @@ -441,57 +441,3 @@ func (loggingLevel) Values() []loggingLevel { loggingLevelNone, } } - -//func slackChannelConfigurationHasChanges(ctx context.Context, plan, state slackChannelConfigurationResourceModel) bool { -// // Sort SNS Topic ARNs before comparison -// planSNSTopicARNs, err := sortSNSTopicARNs(ctx, plan.SNSTopicARNs) -// if err != nil { -// // Log error and fall back to direct comparison -// tflog.Warn(ctx, "Failed to sort plan SNS Topic ARNs", map[string]interface{}{ -// "error": err.Error(), -// }) -// planSNSTopicARNs = plan.SNSTopicARNs -// } -// -// stateSNSTopicARNs, err := sortSNSTopicARNs(ctx, state.SNSTopicARNs) -// if err != nil { -// // Log error and fall back to direct comparison -// tflog.Warn(ctx, "Failed to sort state SNS Topic ARNs", map[string]interface{}{ -// "error": err.Error(), -// }) -// stateSNSTopicARNs = state.SNSTopicARNs -// } -// -// return !plan.ChatConfigurationARN.Equal(state.ChatConfigurationARN) || -// !plan.ConfigurationName.Equal(state.ConfigurationName) || -// !plan.GuardrailPolicyARNs.Equal(state.GuardrailPolicyARNs) || -// !plan.IAMRoleARN.Equal(state.IAMRoleARN) || -// !plan.LoggingLevel.Equal(state.LoggingLevel) || -// !plan.SlackChannelID.Equal(state.SlackChannelID) || -// !plan.SlackChannelName.Equal(state.SlackChannelName) || -// !plan.SlackTeamID.Equal(state.SlackTeamID) || -// !plan.SlackTeamName.Equal(state.SlackTeamName) || -// !planSNSTopicARNs.Equal(stateSNSTopicARNs) || -// !plan.UserAuthorizationRequired.Equal(state.UserAuthorizationRequired) -//} - -//func sortSNSTopicARNs(ctx context.Context, arns types.List) (types.List, error) { -// if arns.IsNull() || arns.IsUnknown() { -// return arns, nil -// } -// -// var arnStrings []string -// diags := arns.ElementsAs(ctx, &arnStrings, false) -// if diags.HasError() { -// return arns, fmt.Errorf("error converting SNS Topic ARNs to strings: %v", diags) -// } -// -// sort.Strings(arnStrings) -// -// sortedArns, diags := types.ListValueFrom(ctx, types.StringType, arnStrings) -// if diags.HasError() { -// return arns, fmt.Errorf("error converting sorted strings to List: %v", diags) -// } -// -// return sortedArns, nil -//} From a2a768ba33f4b7ff6ddf2905b459a4d117d361ca Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Fri, 22 Nov 2024 10:49:51 -0600 Subject: [PATCH 5/6] add CHANGELOG entry --- .changelog/40253.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/40253.txt diff --git a/.changelog/40253.txt b/.changelog/40253.txt new file mode 100644 index 000000000000..2e0a205feb8a --- /dev/null +++ b/.changelog/40253.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_chatbot_slack_channel_configuration: Fix inconsistent provider result when order of `sns_topic_arns`changes +``` \ No newline at end of file From aa87459f6a29cad8c2c10d4da29419428b451487 Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Fri, 22 Nov 2024 10:54:24 -0600 Subject: [PATCH 6/6] chore: semgrep fix --- internal/service/rds/cluster_test.go | 2 +- internal/service/rds/instance.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/service/rds/cluster_test.go b/internal/service/rds/cluster_test.go index 5809dd64c20d..b7d937c83f72 100644 --- a/internal/service/rds/cluster_test.go +++ b/internal/service/rds/cluster_test.go @@ -581,7 +581,7 @@ func TestAccRDSCluster_storageTypeGeneralPurposeToProvisionedIOPS(t *testing.T) { Config: testAccClusterConfig_storageChange(rName, "gp3"), Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr(resourceName, "storage_type", "gp3"), + resource.TestCheckResourceAttr(resourceName, names.AttrStorageType, "gp3"), ), }, { diff --git a/internal/service/rds/instance.go b/internal/service/rds/instance.go index 9735d4b7846f..bfaf0c3a631f 100644 --- a/internal/service/rds/instance.go +++ b/internal/service/rds/instance.go @@ -2589,7 +2589,7 @@ func dbInstancePopulateModify(input *rds.ModifyDBInstanceInput, d *schema.Resour if slices.Contains([]string{storageTypeIO1, storageTypeIO2}, aws.ToString(input.StorageType)) { input.Iops = aws.Int32(int32(d.Get(names.AttrIOPS).(int))) - input.AllocatedStorage = aws.Int32(int32(d.Get("allocated_storage").(int))) + input.AllocatedStorage = aws.Int32(int32(d.Get(names.AttrAllocatedStorage).(int))) } }