New resource: azurerm_security_center_storage_defender

[ziyeqf]

test

❯ tftest securitycenter TestAccSecurityCenterStorageDefender
=== RUN   TestAccSecurityCenterStorageDefender_basic
--- PASS: TestAccSecurityCenterStorageDefender_basic (181.97s)
=== RUN   TestAccSecurityCenterStorageDefender_update
--- PASS: TestAccSecurityCenterStorageDefender_update (237.33s)
=== RUN   TestAccSecurityCenterStorageDefender_complete
--- PASS: TestAccSecurityCenterStorageDefender_complete (213.27s)
=== RUN   TestAccSecurityCenterStorageDefender_reapply
--- PASS: TestAccSecurityCenterStorageDefender_reapply (262.71s)
=== RUN   TestAccSecurityCenterStorageDefender_requiresImport
=== PAUSE TestAccSecurityCenterStorageDefender_requiresImport
=== CONT  TestAccSecurityCenterStorageDefender_requiresImport
--- PASS: TestAccSecurityCenterStorageDefender_requiresImport (179.56s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/securitycenter        1074.899s

There are two design questions:

1. Shall we reset every value to default during delete?
   Since it could only be disabled but can not be deleted, these values will be kept if we dont do the reset.

2. Shall we use the scope id as the resource id in terraform?
   The ID defined in Swagger was /{resourceId}/providers/Microsoft.Security/defenderForStorageSettings/{settingName}, while the settingName could only be current for now. I'm bit concerned to use the storageAccountId/scopeId as the id of the defender. While the generated go-azure-sdk does not contains an id defenition for it but only scope id.

[stephybun]

Thanks for this @ziyeqf.

A couple of things need to be fixed up before we can merge this. Please take a look at the comments left in-line. Thanks!

[stephybun]

Alphabetical ordering

[stephybun]

For future reference these have to go into their own PR to avoid conflicts. So can you please revert this and also do a rebase since the go-azure-sdk has already been updated since the opening of this PR.

[stephybun]

We don't expose the enabled field as explained in the contributor docs.

This field should be controlled purely by the creation of it enabled = true and the deletion of it enabled = false.

[stephybun]

What does -1 mean in this case? Also what would 0 mean?

[ziyeqf]

the service does not accept 0, and -1 means no limit on that cap. Do we need to map user input 0 to payload -1?

[jackofallops]

I think this can be more clearly/simply achieved with:

Suggested change

	"malware_scanning_on_upload_cap_gb_per_month": {
	Type: pluginsdk.TypeInt,
	Optional: true,
	Default: -1,
	ValidateFunc: func(i interface{}, s string) (warnings []string, errors []error) {
	// it requires -1 or greater than 0
	v, ok := i.(int)
	if !ok {
	errors = append(errors, fmt.Errorf("expected type of %s to be integer", s))
	return warnings, errors
	}
	if v == -1 {
	return warnings, errors
	}
	return validation.IntAtLeast(-1)(i, s)
	},
	},
	"malware_scanning_on_upload_cap_gb_per_month": {
	Type: pluginsdk.TypeInt,
	Optional: true,
	Default: -1,
	ValidateFunc: validation.Any(
	validation.IntAtLeast(1),
	validation.IntInSlice([]int{-1}),
	),
	},

[stephybun]

Can we bump this up to 30, with such short timeouts we run the risk of being affected by rate limiting by the API.

[stephybun]

Here we should check if it's disabled, and if so then mark the resource as gone.

[stephybun]

Since this is a scoped ID, we need to parse the scope part of id as a storage account ID

Suggested change

	state := StorageDefenderModel{
	StorageAccountId: id.ID(),
	}
	storageAccountId, err := commonids.ParseStorageAccountID(id.Scope)
	if err != nil {
	return err
	}
	state := StorageDefenderModel{
	StorageAccountId: storageAccountId.ID(),
	}

[stephybun]

If the resource doesn't exist (or in this case disabled) when the delete is run we should raise an error instead of returning nil

Suggested change

	if err != nil {
	if !response.WasNotFound(resp.HttpResponse) {
	return fmt.Errorf("reading %+v", err)
	}
	}
	// if the resource has never been created, it returns 404.
	// once created, it could only be set to disable.
	if response.WasNotFound(resp.HttpResponse) {
	return nil
	}
	if err != nil {
	return fmt.Errorf("retrieving %s: %+v", id, err)
	}

[stephybun]

Suggested change

	return fmt.Errorf("deleting %+v", err)
	return fmt.Errorf("deleting %s: %+v", id, err)

[stephybun]

Can we extend this to parse the Scope part of the id as a storage account ID

[manicminer]

Thanks for making the changes @ziyeqf. Tests are passing and this LGTM.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.