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

azurerm_security_center_setting - fix a bug when name SENTINEL #24497

Merged
merged 8 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
Copy link
Member

Choose a reason for hiding this comment

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

Can we update the name of the state migration to be consistent with the file name of the resource
security_center_settings_v0_to_v1.go -> security_center_setting_v0_to_v1.go

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package migration

import (
"context"
"log"
"strings"

"github.com/hashicorp/terraform-provider-azurerm/internal/tf/pluginsdk"
)

var _ pluginsdk.StateUpgrade = SecurityCenterSettingsV0ToV1{}

type SecurityCenterSettingsV0ToV1 struct{}

func (SecurityCenterSettingsV0ToV1) Schema() map[string]*pluginsdk.Schema {
return map[string]*pluginsdk.Schema{
"enabled": {
Required: true,
Type: pluginsdk.TypeBool,
},
"setting_name": {
Required: true,
Type: pluginsdk.TypeString,
},
}
}

func (SecurityCenterSettingsV0ToV1) UpgradeFunc() pluginsdk.StateUpgraderFunc {
return func(ctx context.Context, rawState map[string]interface{}, meta interface{}) (map[string]interface{}, error) {
log.Println("[DEBUG] Migrating Security Center Settings from v0 to v1 format")
oldId := rawState["id"].(string)
// only find the last one
idx := strings.LastIndex(oldId, "/SENTINEL")
newId := oldId[:idx] + "/Sentinel"

log.Printf("[DEBUG] Updating ID from %q to %q", oldId, newId)

rawState["id"] = newId
Copy link
Member

Choose a reason for hiding this comment

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

This will crash on security settings with other setting names since strings.LastIndex will return -1 if it isn't SENTINEL. I think a safer way to do this would be like below

Suggested change
oldId := rawState["id"].(string)
// only find the last one
idx := strings.LastIndex(oldId, "/SENTINEL")
newId := oldId[:idx] + "/Sentinel"
log.Printf("[DEBUG] Updating ID from %q to %q", oldId, newId)
rawState["id"] = newId
oldId := strings.Split(rawState["id"].(string), "/")
// recase the last index if it's `SENTINEL`
if oldId[len(oldId)-1] == "SENTINEL" {
oldId[len(oldId)-1] = "Sentinel"
}
newId := strings.Join(oldId, "/")
log.Printf("[DEBUG] Updating ID from %q to %q", oldId, newId)
rawState["id"] = newId


return rawState, nil
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,14 @@ import (
"time"

"github.com/hashicorp/go-azure-sdk/resource-manager/security/2022-05-01/settings"
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
"github.com/hashicorp/terraform-provider-azurerm/helpers/tf"
"github.com/hashicorp/terraform-provider-azurerm/internal/clients"
"github.com/hashicorp/terraform-provider-azurerm/internal/features"
"github.com/hashicorp/terraform-provider-azurerm/internal/services/securitycenter/migration"
"github.com/hashicorp/terraform-provider-azurerm/internal/services/securitycenter/parse"
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/pluginsdk"
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/suppress"
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/validation"
"github.com/hashicorp/terraform-provider-azurerm/internal/timeouts"
)
Expand Down Expand Up @@ -46,11 +49,23 @@ func resourceSecurityCenterSetting() *pluginsdk.Resource {
Delete: pluginsdk.DefaultTimeout(10 * time.Minute),
},

SchemaVersion: 1,
StateUpgraders: pluginsdk.StateUpgrades(map[int]pluginsdk.StateUpgrade{
0: migration.SecurityCenterSettingsV0ToV1{},
}),

Schema: map[string]*pluginsdk.Schema{
"setting_name": {
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
Type: pluginsdk.TypeString,
Required: true,
ForceNew: true,
DiffSuppressFunc: func() func(string, string, string, *schema.ResourceData) bool {
// This is a workaround for `SENTINEL` value.
if !features.FourPointOhBeta() {
return suppress.CaseDifference
}
return nil
}(),
ValidateFunc: validation.StringInSlice(validSettingName, false),
},
"enabled": {
Expand All @@ -67,7 +82,12 @@ func resourceSecurityCenterSettingUpdate(d *pluginsdk.ResourceData, meta interfa
ctx, cancel := timeouts.ForUpdate(meta.(*clients.Client).StopContext, d)
defer cancel()

id := settings.NewSettingID(subscriptionId, settings.SettingName(d.Get("setting_name").(string)))
settingName := d.Get("setting_name").(string)
if settingName == "SENTINEL" {
settingName = "Sentinel"
}
Copy link
Member

@stephybun stephybun Jan 25, 2024

Choose a reason for hiding this comment

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

Can we put this behind the 4.0 flag as well


id := settings.NewSettingID(subscriptionId, settings.SettingName(settingName))

if d.IsNewResource() {
existing, err := client.Get(ctx, id)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/hashicorp/terraform-provider-azurerm/internal/acceptance"
"github.com/hashicorp/terraform-provider-azurerm/internal/acceptance/check"
"github.com/hashicorp/terraform-provider-azurerm/internal/clients"
"github.com/hashicorp/terraform-provider-azurerm/internal/features"
"github.com/hashicorp/terraform-provider-azurerm/internal/tf/pluginsdk"
"github.com/hashicorp/terraform-provider-azurerm/utils"
)
Expand All @@ -33,7 +34,7 @@ func testAccSecurityCenterSetting_update(t *testing.T) {
r := SecurityCenterSettingResource{}

// lintignore:AT001
data.ResourceSequentialTest(t, r, []acceptance.TestStep{
testcases := []acceptance.TestStep{
{
Config: r.cfg("MCAS", true),
Check: acceptance.ComposeTestCheckFunc(
Expand Down Expand Up @@ -78,7 +79,18 @@ func testAccSecurityCenterSetting_update(t *testing.T) {
Check: acceptance.ComposeTestCheckFunc(),
},
data.ImportStep(),
})
}

if !features.FourPointOhBeta() {
testcases = append(testcases, []acceptance.TestStep{{
Config: r.cfg("SENTINEL", true),
Check: acceptance.ComposeTestCheckFunc(),
}, {
Config: r.cfg("SENTINEL", false),
Check: acceptance.ComposeTestCheckFunc(),
}}...)
}
data.ResourceSequentialTest(t, r, testcases)
}

func testAccSecurityCenterSetting_requiresImport(t *testing.T) {
Expand Down
Loading