security_center_subscription_pricing_resource: refactor disabled extensions logic

[Ihab-Zhaika]

Change the order of the extension statuses

[catriona-m]

Hi @Ihab-Zhaika, I ran the acceptance tests for this and it looks like it is failing:

=== RUN   TestAccSecurityCenterSubscriptionPricing_cloudPostureExtension
    testcase.go:120: Step 1/5 error: After applying this test step and performing a `terraform refresh`, the plan was not empty.
        stdout
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        Terraform will perform the following actions:
          # azurerm_security_center_subscription_pricing.test will be updated in-place
          ~ resource "azurerm_security_center_subscription_pricing" "test" {
                id            = "/subscriptions/*******/providers/Microsoft.Security/pricings/CloudPosture"
                # (2 unchanged attributes hidden)
              - extension {
                  - additional_extension_properties = {} -> null
                  - name                            = "AgentlessDiscoveryForKubernetes" -> null
                }
              - extension {
                  - additional_extension_properties = {} -> null
                  - name                            = "ContainerRegistriesVulnerabilityAssessments" -> null
                }
                # (2 unchanged blocks hidden)
            }
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccSecurityCenterSubscriptionPricing_cloudPostureExtension (43.86s)
FAIL```

[Ihab-Zhaika]

~ update in-place

Weird, I will check

Wanted to ask, If we have a critical need to put the "enabled" flag again, can we add it ?

[Ihab-Zhaika]

Hi @Ihab-Zhaika, I ran the acceptance tests for this and it looks like it is failing:

=== RUN   TestAccSecurityCenterSubscriptionPricing_cloudPostureExtension
    testcase.go:120: Step 1/5 error: After applying this test step and performing a `terraform refresh`, the plan was not empty.
        stdout
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        Terraform will perform the following actions:
          # azurerm_security_center_subscription_pricing.test will be updated in-place
          ~ resource "azurerm_security_center_subscription_pricing" "test" {
                id            = "/subscriptions/*******/providers/Microsoft.Security/pricings/CloudPosture"
                # (2 unchanged attributes hidden)
              - extension {
                  - additional_extension_properties = {} -> null
                  - name                            = "AgentlessDiscoveryForKubernetes" -> null
                }
              - extension {
                  - additional_extension_properties = {} -> null
                  - name                            = "ContainerRegistriesVulnerabilityAssessments" -> null
                }
                # (2 unchanged blocks hidden)
            }
        Plan: 0 to add, 1 to change, 0 to destroy.
--- FAIL: TestAccSecurityCenterSubscriptionPricing_cloudPostureExtension (43.86s)
FAIL```

@catriona-m can you rerun the tests please

[Ihab-Zhaika]

@catriona-m
I need advise regards the following
How I can know if d.HasChange(KEY) returns false because the Key is empty from the first place or because the key has the ignore_changes attribute ?
I need this differentiation

[catriona-m]

@catriona-m I need advise regards the following How I can know if d.HasChange(KEY) returns false because the Key is empty from the first place or because the key has the ignore_changes attribute ? I need this differentiation

Hi @Ihab-Zhaika. If the user has set ignore_changes on the field in the config (see below) we don't need the logic inside has.Changes() because we are ignoring the changes.

  lifecycle {
    ignore_changes = [extension]
  }

Otherwise, if there are no changes, we shouldn't need to do anything. Can you explain a bit more why we would need to know the difference in this case so we can work out the best way to resolve this? Thank you!

[catriona-m]

I have ran the acceptance tests again and it looks like there is a failure in another test now related to extension

------- Stdout: -------
=== RUN   TestAccSecurityCenterSubscriptionPricing_storageAccountSubplan
    testcase.go:120: Step 1/2 error: Error running apply: exit status 1
        Error: setting Pricing (Subscription: "*******"
        Pricing Name: "StorageAccounts"): pricings.PricingsClient#Update: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="InvalidInputJson" Message="Extension with name 'OnUploadMalwareScanning' is not supported for 'StorageAccounts' plan and 'PerStorageAccount' SubPlan"
          with azurerm_security_center_subscription_pricing.test,
          on terraform_plugin_test.tf line 21, in resource "azurerm_security_center_subscription_pricing" "test":
          21: resource "azurerm_security_center_subscription_pricing" "test" {
--- FAIL: TestAccSecurityCenterSubscriptionPricing_storageAccountSubplan (18.99s)
FAIL

[Ihab-Zhaika]

@catriona-m I need advise regards the following How I can know if d.HasChange(KEY) returns false because the Key is empty from the first place or because the key has the ignore_changes attribute ? I need this differentiation

Hi @Ihab-Zhaika. If the user has set ignore_changes on the field in the config (see below) we don't need the logic inside has.Changes() because we are ignoring the changes.

  lifecycle {
    ignore_changes = [extension]
  }

Otherwise, if there are no changes, we shouldn't need to do anything. Can you explain a bit more why we would need to know the difference in this case so we can work out the best way to resolve this? Thank you!

Hey @catriona-m

I have the following condition that on it I change the extensions [Line 165 in the PR]
if d.HasChange("extension") || d.IsNewResource() || isCurrentlyInFree

So what I want to achieve is the following:
When I am moving from Free to standard: => isCurrentlyInFree is true in this case
If extension is in ignore => I want to turn on all the extensions since in the docs we have the following

~> NOTE: If an extension is not defined, it will not be enabled. Use ignore_changes on the extension field if you want to use the default extensions.
This is the default behavior

if the extensions are empty [no in ignore], I want to keep all of them off

In our backend the default behavior is to have all the extensions on if the customer did not specify any since this is additional data to an object and not resource by it self, when I changed the code to comply with the removal of "enabled" it became hard to achieve this easily, so in the above case if the extension is empty or if it is in ignore_changes In the code I have currently no way to distinguish between those case(d.HasChange("extension") is false and d.GetOk("extension") returns empty), I can of course add new bool argument for example called "turn_on_default_extensions" which will ignore the content of the extension and turn all of them.

So what do you think that we can solve this issue ?

[Ihab-Zhaika]

To achieve this we will need to make a couple of changes to expandSecurityCenterSubscriptionPricingExtensions and

so the users will not able to use default values anymore? I'm a bit concerned about that.. And it's different with the best practice document?

@ziyeqf where a resource provisions nested items under it, such as in this case, any nested items should be removed during creation - since in this case these extensions are being enabled by default, these should be removed at the end of the Terraform run. For an example see API Management, which provisions APIs and some other items at initial creation, as such Terraform clears those up before marking the resource as provisioned - such that the users Terraform configuration matches the state of the resource in Azure.

@catriona-m
Updated it to what asked,
Acc tests tests

[catriona-m]

Thanks for making the additional changes @Ihab-Zhaika - I have tested this and it looks like it's working well now.

During testing, I did notice one further change we should implement. In the case where we are changing from Standard -> Free we are currently silently removing the extensions. Instead, in cases where the tier is Free and extensions are enabled in the config, we should raise an error indicating that extensions can only be enabled when the tier is Standard.

Once that is done, this should be good to merge! Thanks!

[Ihab-Zhaika]

Thanks for making the additional changes @Ihab-Zhaika - I have tested this and it looks like it's working well now.

During testing, I did notice one further change we should implement. In the case where we are changing from Standard -> Free we are currently silently removing the extensions. Instead, in cases where the tier is Free and extensions are enabled in the config, we should raise an error indicating that extensions can only be enabled when the tier is Standard.

Once that is done, this should be good to merge! Thanks!

@catriona-m Done, tests pass also

[Ihab-Zhaika]

Review changes

[Ihab-Zhaika]

Review changes

@catriona-m applied you suggestions

[catriona-m]

Review changes

@catriona-m applied you suggestions

Hi @Ihab-Zhaika , I had another look at this, but I can't see the new commit here yet?

[Ihab-Zhaika]

Review changes

@catriona-m applied you suggestions

Hi @Ihab-Zhaika , I had another look at this, but I can't see the new commit here yet?

@catriona-m indeed.., now it is applied

[catriona-m]

Thanks for making the additional changes @Ihab-Zhaika - I had another look through and this LGTM! Thanks!

[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.