New resource `azurerm_policy_remediation` #5746

ArcturusZhang · 2020-02-14T07:28:43Z

This is a revised version of PR 5277.

A azurerm_policy_remediation is used along with a azurerm_policy_assignment to remediate a policy.
A policy can be assigned in 4 different scopes: Subscription, Resource Group, Resource and Management Group. This remediation resource supports all of them using a scope field similar to the azurerm_policy_assignment.

Since the id of a management group (like /providers/Microsoft.Management/managementGroups/00000000-0000-0000-0000-000000000000) is not a valid azure resource id, passing it to azure.ParseResourceId will throw errors, I wrote some additional ID parsing functions to deal with this kind of situations.

And additionally, since all of the ids in a remediation will be converted to lower cases, I am using case diff suppression everywhere.

katbyte

Thanks for the new resource @ArcturusZhang, overall this looks good but i do have some concerns about the multi-purpose ID struct you created. As mentioned in the comments i'm not sure that is ideal given not all properties are valid for all types. Additionally we can pull the switch for the create/read/delete calls out into the resource, they won't be used anywhere else?

Additional what would the use case for the data source be? will the ID be used elsewhere?

azurerm/internal/clients/client.go

azurerm/internal/services/policyinsights/client/client.go

azurerm/internal/services/policyinsights/data_source_policy_remediation.go

azurerm/internal/services/policyinsights/resource_arm_policy_remediation.go

azurerm/internal/services/policyinsights/parse/remediation.go

azurerm/internal/services/policyinsights/remediations.go

ArcturusZhang · 2020-03-19T04:01:15Z

I think this will conflict with the prs: #5981 #6107 ? 🤔

Well, in general this PR should not have conflicts with them, because I did not change anything else in the policy service (including the policy_definition resource) other than the addition of policy remediation. Therefore I suppose my PR should not have conflicts with them.

But I think the changes for management group IDs in this PR would benefit those two PR. They are trying to handle the management group in their own resources, which I think is not a very good idea to introduce too much copy and paste work.

ArcturusZhang · 2020-03-19T04:07:29Z

I just noticed that one of them has been approved, then maybe a refinement of how to deal the management group in the policy service should be introduced after that and this PR get merged

katbyte

Thanks for the changes @ArcturusZhang, just a couple more minor comments i've left inline and this should be good to merge 👍

katbyte · 2020-03-13T07:29:00Z

azurerm/internal/services/managementgroup/data_source_management_group.go

-				Required: true,
+				Type:         schema.TypeString,
+				Required:     true,
+				ValidateFunc: validate.ManagementGroupName,


the property is group_id but this is validating the name?

Here group_id means this one in its ID:

/providers/Microsoft.ManagementGroup/managementGroups/{group_id}

the thing in other resources' IDs at the same position would be called as a name, for instance

/subscriptions/{subsID}/resourceGroups/group1/providers/Microsoft.Compute/virtualMachines/{name}

And another reason is that if we call this group_id in the validation function, the validation function would looks like ManagementGroupGroupID or ManagementGroupID. This would be confusing with the validation function of the real management group ID. Therefore I took this name.

katbyte · 2020-03-13T07:29:44Z

azurerm/internal/services/managementgroup/resource_arm_management_group.go

+				Optional:     true,
+				Computed:     true,
+				ForceNew:     true,
+				ValidateFunc: validate.ManagementGroupName,


The is a group_id property but we are validating a name?

Same reason as the above comment

azurerm/internal/services/policy/parse/definition.go

azurerm/internal/services/policy/resource_arm_policy_remediation.go

azurerm/internal/services/policy/tests/resource_arm_policy_remediation_test.go

website/docs/r/policy_remediation.html.markdown

robinsk · 2020-03-24T07:06:07Z

Nice to see getting closer to merge. Really looking forward to clean up our subscriptions programmatically.

@ArcturusZhang Thank you for working on this, much appreciated!

ArcturusZhang · 2020-03-25T09:58:20Z

Hi @katbyte I have resolved the comments.

And I just spotted a typo in the doc of azurerm_policy_set_definition when I am copying the timeout section, and I changed those in this PR, hope you would not mind.

katbyte

Thanks for the changes @ArcturusZhang! aside from two minor comments this LGTM 👍

azurerm/internal/services/policy/resource_arm_policy_remediation.go

azurerm/internal/services/policy/validate/definition.go

ArcturusZhang · 2020-03-26T06:08:05Z

Hi @katbyte I resolved the final two minor comments

ArcturusZhang · 2020-03-27T03:15:59Z

I also included some improvement of policy_definition in this PR. Acc test result for the policy definition:

But still the test case related with mgmt group fails with authorization error:

Status=403 Code="AuthorizationFailed" Message="The client '***' with object id '***' does not have authorization to perform action 'Microsoft.Management/managementGroups/read' over scope '/providers/Microsoft.Management/managementGroups/***' or the scope is invalid. If access was recently granted, please refresh your credentials."

ghost · 2020-03-27T11:03:54Z

This has been released in version 2.3.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.3.0"
}
# ... other configuration ...

kailunshi · 2020-04-01T02:06:06Z

Hi @ArcturusZhang

First at all, thank you for the great work. I know this is merged and you might not be following it but I discovered that ResourceDiscoveryMode option is not included. https://github.com/Azure/azure-sdk-for-go/blob/master/services/policyinsights/mgmt/2019-10-01/policyinsights/models.go#L2056

Could you please shed some light? let me know if you want me to create a new ticket.

ArcturusZhang · 2020-04-01T02:27:53Z

Hi @ArcturusZhang

First at all, thank you for the great work. I know this is merged and you might not be following it but I discovered that ResourceDiscoveryMode option is not included. https://github.com/Azure/azure-sdk-for-go/blob/master/services/policyinsights/mgmt/2019-10-01/policyinsights/models.go#L2056

Could you please shed some light? let me know if you want me to create a new ticket.

Well, this field initially does not exist in the version of go SDK terraform is using when I first work on this resource (Dec. 2019). I will take some time to add this into this resource, thanks for remind me.

kailunshi · 2020-04-01T05:04:01Z

Hi @ArcturusZhang
First at all, thank you for the great work. I know this is merged and you might not be following it but I discovered that ResourceDiscoveryMode option is not included. https://github.com/Azure/azure-sdk-for-go/blob/master/services/policyinsights/mgmt/2019-10-01/policyinsights/models.go#L2056
Could you please shed some light? let me know if you want me to create a new ticket.

Well, this field initially does not exist in the version of go SDK terraform is using when I first work on this resource (Dec. 2019). I will take some time to add this into this resource, thanks for remind me.

Makes total sense. azure is fast evolving. :)

Thank you!

ghost · 2020-04-26T14:50:01Z

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 documentation labels Feb 14, 2020

ArcturusZhang requested review from tombuildsstuff, mbfrahry and katbyte February 14, 2020 07:28

ArcturusZhang force-pushed the PolicyInsightsRemediation branch from 1201796 to 4120edb Compare February 14, 2020 07:30

ArcturusZhang added 3 commits February 25, 2020 14:34

Add ID parsing and validation functions, add name validate function

0c6555f

Add validate function for data source

5582e41

Goimports

cb3a052

ArcturusZhang force-pushed the PolicyInsightsRemediation branch 2 times, most recently from 496c912 to 68a5414 Compare March 2, 2020 09:47

katbyte added the service/policy label Mar 5, 2020

katbyte requested changes Mar 5, 2020

View reviewed changes

katbyte added the waiting-response label Mar 5, 2020

ArcturusZhang added 9 commits March 6, 2020 11:30

Added new resource and data source azurerm_policy_remediation

83435db

Add some more test cases

3c6ba09

Some changes to conform the changes in 2.0

c53b8db

Resolve some comments

689bcdc

Add ID parsing and validation functions, add name validate function

b281d69

Add validate function for data source

5eb2e9b

Goimports

22dc3b7

Resolve comments

cf1d469

Use parsing functions from mgmt group

48dc397

ArcturusZhang force-pushed the PolicyInsightsRemediation branch from 68a5414 to 48dc397 Compare March 6, 2020 03:37

ArcturusZhang added 5 commits March 6, 2020 11:50

Resolve more comments

aae0f5d

Resolve more comments

be89874

Fix CI failures

8142ed5

Some refactor

e4cf05a

Removed a useless comment

d083570

Merge branch 'master' into PolicyInsightsRemediation

f31c25a

katbyte requested changes Mar 24, 2020

View reviewed changes

katbyte added the waiting-response label Mar 24, 2020

ghost removed the waiting-response label Mar 24, 2020

katbyte added this to the v2.3.0 milestone Mar 24, 2020

katbyte added the waiting-response label Mar 24, 2020

Resolve comments

02cac69

ghost removed the waiting-response label Mar 25, 2020

ArcturusZhang requested a review from katbyte March 25, 2020 09:58

katbyte approved these changes Mar 25, 2020

View reviewed changes

azurerm/internal/services/policy/resource_arm_policy_remediation.go Outdated Show resolved Hide resolved

azurerm/internal/services/policy/validate/definition.go Outdated Show resolved Hide resolved

Resolve comments

1e0db42

ArcturusZhang and others added 4 commits March 27, 2020 11:17

Include some changes on policy definition

97bd696

Fix name validation to forbid upper case letters

e53b998

make fmt

3407903

Goimports :(

88cb232

katbyte merged commit 3fc355a into hashicorp:master Mar 27, 2020

katbyte added a commit that referenced this pull request Mar 27, 2020

Update CHANGELOG.md to include #5746

3af2146

ArcturusZhang deleted the PolicyInsightsRemediation branch March 27, 2020 05:56

ghost locked and limited conversation to collaborators Apr 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New resource `azurerm_policy_remediation` #5746

New resource `azurerm_policy_remediation` #5746

ArcturusZhang commented Feb 14, 2020

katbyte left a comment

ArcturusZhang commented Mar 19, 2020

ArcturusZhang commented Mar 19, 2020

katbyte left a comment

katbyte Mar 13, 2020

ArcturusZhang Mar 25, 2020 •

edited

Loading

katbyte Mar 13, 2020

ArcturusZhang Mar 25, 2020

robinsk commented Mar 24, 2020

ArcturusZhang commented Mar 25, 2020

katbyte left a comment

ArcturusZhang commented Mar 26, 2020

ArcturusZhang commented Mar 27, 2020

ghost commented Mar 27, 2020

kailunshi commented Apr 1, 2020

ArcturusZhang commented Apr 1, 2020

kailunshi commented Apr 1, 2020

ghost commented Apr 26, 2020

New resource azurerm_policy_remediation #5746

New resource azurerm_policy_remediation #5746

Conversation

ArcturusZhang commented Feb 14, 2020

katbyte left a comment

Choose a reason for hiding this comment

ArcturusZhang commented Mar 19, 2020

ArcturusZhang commented Mar 19, 2020

katbyte left a comment

Choose a reason for hiding this comment

katbyte Mar 13, 2020

Choose a reason for hiding this comment

ArcturusZhang Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

katbyte Mar 13, 2020

Choose a reason for hiding this comment

ArcturusZhang Mar 25, 2020

Choose a reason for hiding this comment

robinsk commented Mar 24, 2020

ArcturusZhang commented Mar 25, 2020

katbyte left a comment

Choose a reason for hiding this comment

ArcturusZhang commented Mar 26, 2020

ArcturusZhang commented Mar 27, 2020

ghost commented Mar 27, 2020

kailunshi commented Apr 1, 2020

ArcturusZhang commented Apr 1, 2020

kailunshi commented Apr 1, 2020

ghost commented Apr 26, 2020

New resource `azurerm_policy_remediation` #5746

New resource `azurerm_policy_remediation` #5746

ArcturusZhang Mar 25, 2020 •

edited

Loading