Update `azurerm_security_center_subscription_pricing` #8549

beandrad · 2020-09-21T16:04:09Z

Adds resource_type to azurerm_security_center_subscription_pricing to allow setting pricing tier (Free or Standard) per resource type. At the moment only VirtualMachines are affected by this resource.

The problem with the current code is that is using the v1 pricing client, which only allows specifying default as resource type, which seems to map to Virtual Machines resource type.

Fixes: #4846

Nepomuceno · 2020-09-23T18:38:08Z

If you are changing a resource schema you need to change the schema version and do a schema update script. Instructions on how to do that can be found here I would also change the way that the resource_type is defined to be a map this way there will be no need to define multiple of this to define each toggle.

Nepomuceno · 2020-09-29T08:35:40Z

LGTM

jackofallops

Hi @beandrad
Thanks for this PR, it's off to a good start.

We try to avoid using multiple API versions for a service wherever possible. I've had a quick scan over the service and I think this should be possible here without too much difficulty. Since the changes you are targeting in the resource are pretty much required for the API bump could you update to just using v3.0? I've put some other comments in below that will also need addressing. We can help you out as needed, and if you're not already joined the community slack, there are instructions for joining in the project readme.

azurerm/internal/services/securitycenter/resource_arm_security_center_subscription_pricing.go

jackofallops · 2020-09-29T14:08:32Z

azurerm/internal/services/securitycenter/resource_arm_security_center_subscription_pricing.go

@@ -44,17 +58,39 @@ func resourceArmSecurityCenterSubscriptionPricing() *schema.Resource {
 					string(security.Standard),
 				}, false),
 			},
+			"resource_type": {
+				Type:     schema.TypeString,
+				Required: true,


This will need to be Optional to avoid the breaking change. Setting a default value here (of default as before, or VirtualMachines if that replaces it?) would be appropriate.

default is not allowed in the new version of the API, VirtualMachines replaces it. I guess in this case, I can remove the StateUpgraders. The reasons I made it a required parameter are (1) in the rest API is required and it's nice to keep a consistent behaviour and (2) not having an explicit resource type can be misleading (when reviewing the code the reader may think that the pricing tier affects all the resources within the subscription).

@jackofallops do you still think that there should be a default value?

If the previous API used Default, and only actually referred to VirtualMachines, then I think perhaps this should be the default? It also means the state upgrader will need to update the resource ID for existing resources as these will all end in \default for the pricings segment.

azurerm/internal/services/securitycenter/resource_arm_security_center_subscription_pricing.go

jackofallops

Hi @beandrad
Thanks for the updates and changes. I've left a few more comments below, but looking really good so far.

jackofallops · 2020-10-01T12:45:12Z

azurerm/internal/services/securitycenter/resource_arm_security_center_subscription_pricing.go

@@ -44,17 +58,39 @@ func resourceArmSecurityCenterSubscriptionPricing() *schema.Resource {
 					string(security.Standard),
 				}, false),
 			},
+			"resource_type": {
+				Type:     schema.TypeString,
+				Required: true,


If the previous API used Default, and only actually referred to VirtualMachines, then I think perhaps this should be the default? It also means the state upgrader will need to update the resource ID for existing resources as these will all end in \default for the pricings segment.

...ernal/services/securitycenter/resource_arm_security_center_subscription_pricing_migration.go

azurerm/internal/services/securitycenter/resource_arm_security_center_workspace.go

examples/security/securitycenter-pricing/main.tf

website/docs/r/security_center_subscription_pricing.markdown

azurerm/internal/services/securitycenter/resource_arm_security_center_subscription_pricing.go

At the moment, only Virtual Machines are set with the security center standard or free pricing tiers when using `azurerm_security_center_subscription_pricing`. This change adds the field `resource_type`, which allows to specify the resource type for which we want to update the pricing tier. The v1 security center pricing client only allows to get the pricing tier of the default resource type (Virtual Machines) to check whether the subscription has standard or free security center pricing tier. However, a partial standard pricing tier (where one or more resource types have standar tier enabled) allows for a security center workspace to be created. This commit changes the client to v3 and checks if any resource type in the subscription has standard pricing tier enabled, and if so, it allows the creation of a security center workspace.

To avoid using multiple versions of the API.

Since these messages are already prefixed with `Error:`.

jackofallops

Hi @beandrad - Thanks for the continued great work here. The changes of API have broken a couple of the acceptnce tests, if you could fix those up also, I think we're good to merge.
For context, TestAccAzureRMSecurityCenter_pricingAndWorkspace and TestAccAzureRMSecurityCenter_contact are both still using default.
Thanks!

azurerm/internal/services/securitycenter/resource_arm_security_center_subscription_pricing.go

jackofallops · 2020-10-06T06:42:27Z

Tests passing (failures unrelated to change)

ghost · 2020-10-08T18:54:05Z

This has been released in version 2.31.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.31.0"
}
# ... other configuration ...

ghost · 2020-11-05T14:50:13Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

ghost added size/XXL dependencies labels Sep 21, 2020

beandrad changed the title ~~Update azurerm_security_center_subscription_pricing Issue #4846~~ Update azurerm_security_center_subscription_pricing Sep 21, 2020

beandrad force-pushed the issue-4846 branch 5 times, most recently from e3fd8b1 to f9e2dcd Compare September 23, 2020 15:14

beandrad force-pushed the issue-4846 branch from f9e2dcd to a207786 Compare September 24, 2020 15:07

jackofallops added breaking-change service/security-center labels Sep 29, 2020

jackofallops requested changes Sep 29, 2020

View reviewed changes

beandrad force-pushed the issue-4846 branch 7 times, most recently from c69d063 to c776112 Compare September 30, 2020 11:03

beandrad requested a review from jackofallops September 30, 2020 13:41

jackofallops requested changes Oct 1, 2020

View reviewed changes

beandrad added 3 commits October 1, 2020 16:44

Upgrade version of the security center client to v3

5eb820d

To avoid using multiple versions of the API.

Remove duplication of Error in fmt.Errorf messages

8eee6e8

Since these messages are already prefixed with `Error:`.

beandrad force-pushed the issue-4846 branch from c776112 to 8eee6e8 Compare October 1, 2020 16:56

beandrad requested a review from jackofallops October 1, 2020 18:17

jackofallops requested changes Oct 5, 2020

View reviewed changes

jackofallops added the waiting-response label Oct 5, 2020

tombuildsstuff reviewed Oct 6, 2020

View reviewed changes

azurerm/internal/services/securitycenter/resource_arm_security_center_subscription_pricing.go Show resolved Hide resolved

jackofallops approved these changes Oct 6, 2020

View reviewed changes

jackofallops added breaking-change and removed breaking-change labels Oct 6, 2020

jackofallops merged commit c813398 into hashicorp:master Oct 6, 2020

jackofallops added a commit that referenced this pull request Oct 6, 2020

Update CHANGELOG.md for #8549

f6568c9

katbyte added this to the v2.31.0 milestone Oct 6, 2020

ghost locked as resolved and limited conversation to collaborators Nov 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update `azurerm_security_center_subscription_pricing` #8549

Update `azurerm_security_center_subscription_pricing` #8549

beandrad commented Sep 21, 2020 •

edited

Loading

Nepomuceno commented Sep 23, 2020

Nepomuceno commented Sep 29, 2020

jackofallops left a comment

jackofallops Sep 29, 2020

beandrad Sep 29, 2020

beandrad Sep 29, 2020

jackofallops Oct 1, 2020

jackofallops left a comment

jackofallops Oct 1, 2020

jackofallops left a comment •

edited

Loading

jackofallops commented Oct 6, 2020

ghost commented Oct 8, 2020

ghost commented Nov 5, 2020

Update azurerm_security_center_subscription_pricing #8549

Update azurerm_security_center_subscription_pricing #8549

Conversation

beandrad commented Sep 21, 2020 • edited Loading

Nepomuceno commented Sep 23, 2020

Nepomuceno commented Sep 29, 2020

jackofallops left a comment

Choose a reason for hiding this comment

jackofallops Sep 29, 2020

Choose a reason for hiding this comment

beandrad Sep 29, 2020

Choose a reason for hiding this comment

beandrad Sep 29, 2020

Choose a reason for hiding this comment

jackofallops Oct 1, 2020

Choose a reason for hiding this comment

jackofallops left a comment

Choose a reason for hiding this comment

jackofallops Oct 1, 2020

Choose a reason for hiding this comment

jackofallops left a comment • edited Loading

Choose a reason for hiding this comment

jackofallops commented Oct 6, 2020

ghost commented Oct 8, 2020

ghost commented Nov 5, 2020

Update `azurerm_security_center_subscription_pricing` #8549

Update `azurerm_security_center_subscription_pricing` #8549

beandrad commented Sep 21, 2020 •

edited

Loading

jackofallops left a comment •

edited

Loading