New Resource: `azurerm_policy_definition` #1010

jaymitre · 2018-03-20T21:07:32Z

The goal of this change is to allow CRUD operations to be performed on azure policies using terraform.

tombuildsstuff

Thanks for this PR - I've taken a look through and left some comments in-line but this is mostly looking good; if we can fix up those issues this should be good to merge :)

Thanks!

tombuildsstuff · 2018-03-22T09:16:08Z

azurerm/resource_arm_policy_definition.go

+
+	d.Set("id", resp.ID)
+	d.Set("name", resp.Name)
+	d.Set("policy_type", resp.PolicyType)


given the API response will always return these inside the Properties object, can we read these from there and add a nil check to handle bad Swagger/API responses (for older resources or future swagger changes) e.g.

if props := resp.DefinitionProperties; props != nil { d.Set(“policy_type”, props.PolicyType) # ... }

tombuildsstuff · 2018-03-22T09:17:23Z

azurerm/resource_arm_policy_definition_test.go

+func testAzureRMPolicyDefinition(ri int) string {
+	return fmt.Sprintf(`
+resource "azurerm_policy_definition" "test" {
+  name                = "acctestRG-%d"


can we make this “acctestpol-%d”

tombuildsstuff · 2018-03-22T09:20:11Z

vendor/vendor.json

+			"path": "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2016-12-01/policy",
+			"revision": "e67cd39e942c417ae5e9ae1165f778d9fe8996e0",
+			"revisionTime": "2018-03-16T20:43:19Z"
+		},


can we pin this to the tag “v14.5.0” which will allow us to merge this PR once #1006 has been merged

tombuildsstuff · 2018-03-22T09:29:07Z

azurerm/resource_arm_policy_definition_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+	"github.com/hashicorp/terraform/terraform"
+	"net/http"
+	"testing"


minor can we order these based on internal then external using “goimports”

tombuildsstuff · 2018-03-22T09:34:30Z

azurerm/resource_arm_policy_definition.go

+
+func parsePolicyDefinitionNameFromId(id string) (string, error) {
+	idURL, err := url.ParseRequestURI(id)
+	if err != nil {


Given the ID should be in the format:

/subscriptions/{subscriptionId}/providers/Microsoft.Authorization/policyDefinitions/{policyDefinitionName}

based on what I can see in the SDK - I think we should be able to use the built in functions here:

We can access this via:

id, err := parseAzureResourceID(d.Id()) if err != nil { return nil } return id.Path["policyDefinitions"]

When we use parseAzureResourceID we get an error that a resource group isn't in the id string.

in which case can we return just the last segment of this? given we can guarantee there's only 6 segments in the URI (once it's split based on /) we should be able to make this:

components := strings.Split(path, "/") if len(components) != 6 { return nil, fmt.Errorf("Azure Policy Definition Id should have 6 segments, got %d: '%s'", len(components), path) } return components[5]

tombuildsstuff · 2018-03-22T09:37:46Z

azurerm/resource_arm_policy_definition_test.go

+	"testing"
+)
+
+func TestPolicyDefinitionCreate(t *testing.T) {


can we update the name to match the other tests? e.g. TestAccAzureRMPolicyDefinition_basic

tombuildsstuff · 2018-03-22T09:40:49Z

azurerm/resource_arm_policy_definition.go

+	"github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils"
+)
+
+func resourceArmPolicyDefinition() *schema.Resource {


can we add some documentation for this resource in the website folder?

tombuildsstuff · 2018-03-22T09:41:28Z

azurerm/config.go

@@ -864,6 +869,12 @@ func (c *ArmClient) registerWebClients(endpoint, subscriptionId string, auth aut
 	c.appServicesClient = appsClient
 }

+func (c *ArmClient) registerPolicyDefinitionsClients(endpoint, subscriptionId string, auth autorest.Authorizer) {


can we name this “registerPolicyClients” since this’ll register other types of clients (such as role assignments) in the future too

tombuildsstuff · 2018-03-22T09:42:38Z

azurerm/resource_arm_policy_definition_test.go

+	}
+}
+
+func checkIfPolicyDestroyed(s *terraform.State) error {


Can we update the name of this to be consistent with the other resources? eg testCheckAzureRMPolicyDefinitionDestroy

jaymitre · 2018-03-22T21:46:27Z

Thank you for taking a look at the PR and for your helpful feedback. I made a commit to update the PR based on your feedback.

tombuildsstuff · 2018-03-22T22:09:59Z

website/azurerm.erb

@@ -594,6 +594,15 @@
              </ul>
            </li>

+            <li<%= sidebar_current("docs-azurerm-policy-definition") %>>
+              <a href="#">Policy Definition</a>


missed this earlier - can we make this Policy Resources?

tombuildsstuff · 2018-03-22T22:10:23Z

website/docs/r/policy_definition.html.markdown

@@ -0,0 +1,89 @@
+---
+layout: "azurerm"
+page_title: "Azure Resource Manager: azure_policy_definition"


can we update azure_policy_definition -> azurerm_policy_definition

jaymitre · 2018-03-22T22:41:41Z

Updated the naming. Thank you.

tombuildsstuff

hey @jaymitre

Thanks for pushing those updates - I've left a couple of minor comments but this now mostly LGTM.

Just to let you know that since this PR is making use of v14 of the Azure SDK for Go, we'll need to hold off merging it until after #1006 has been merged (which next week).

Thanks!

tombuildsstuff · 2018-03-23T00:00:04Z

azurerm/resource_arm_policy_definition.go

+		return fmt.Errorf("Error reading Policy Definition %+v", err)
+	}
+
+	d.Set("id", resp.ID)


we don't need to set id here - since it's already set in the Create method above

jaymitre · 2018-03-27T16:54:20Z

Hey @tombuildsstuff, made a few changes with to address your remaining comments.

tombuildsstuff

hey @jaymitre

Thanks for pushing those updates :)

I've left a couple of minor comments but this now otherwise LGTM; so that we can get this merged I'm going to push a couple of commits to fix those issues and pin this to v12.5 of the Azure SDK for Go (since we're currently blocked upgrading to v14/v15) - I hope you don't mind!

Thanks!

tombuildsstuff · 2018-03-27T19:37:16Z

website/docs/r/policy_definition.html.markdown

+    new resource to be created.
+
+* `policy_type` - (Required) The policy type.  The value can be BuiltIn, Custom 
+    or NotSpecified. Changing this forces a new resource to be created.


minor can we quote these values e.g. "BuiltIn, Custom and NotSpecified"

tombuildsstuff · 2018-03-27T19:40:29Z

website/docs/r/policy_definition.html.markdown

+
+* `mode` - (Required) The policy mode that allows you to specify which resource
+    types will be evaluated.  The value can be All, Indexed or
+    NotSpecified. Changing this resource forces a new resource to be


(same here) can we quote these values e.g. All, Indexed or NotSpecified

tombuildsstuff · 2018-03-27T19:40:44Z

website/docs/r/policy_definition.html.markdown

+
+The following attributes are exported:
+
+* `id` - The policy defenition id.


minor defenition -> definition

tombuildsstuff · 2018-03-27T19:41:03Z

website/docs/r/policy_definition.html.markdown

+Policy Definitions can be imported using the `policy name`, e.g.
+
+```shell
+terraform import azurerm_policy_definition.testPolict  /subscriptions/<SUBSCRIPTION_ID>/providers/Microsoft.Authorization/policyDefinitions/<POLICY_NAME>


minor could we update testPolict to be testPolicy? :)

- adding an import test - serializing the json responses for metadata, parameters and policy_rule ``` $ acctests azurerm TestAccAzureRMPolicyDefinition_ === RUN TestAccAzureRMPolicyDefinition_importBasic --- PASS: TestAccAzureRMPolicyDefinition_importBasic (29.37s) === RUN TestAccAzureRMPolicyDefinition_basic --- PASS: TestAccAzureRMPolicyDefinition_basic (23.87s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 53.277s ```

tombuildsstuff · 2018-03-27T21:51:46Z

hey @jaymitre

Thanks for pushing those updates - I've opened a PR on your fork (since I don't have permissions to push to your fork's master branch) which includes a couple of bug fixes I noticed when testing this PR - if we can merge that in then this PR should be good to merge :)

Thanks!

Fixes for `azurerm_policy_definition`

jaymitre · 2018-03-27T22:01:23Z

@tombuildsstuff Thank you for taking the time to review the PR and make those fixes. Your changes have been merged.

tombuildsstuff

LGTM - thanks for merging that in :)

Thanks!

tombuildsstuff · 2018-03-27T22:09:18Z

Tests pass:

$ acctests azurerm TestAccAzureRMPolicyDefinition_
=== RUN   TestAccAzureRMPolicyDefinition_importBasic
--- PASS: TestAccAzureRMPolicyDefinition_importBasic (26.40s)
=== RUN   TestAccAzureRMPolicyDefinition_basic
--- PASS: TestAccAzureRMPolicyDefinition_basic (15.08s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	41.519s

JasonNguyenTX · 2018-04-13T20:16:49Z

Hello, does it support for policy tier? Currently it seems that the default assignment tier is free. I would like to see if it can support standard tier

JasonNguyenTX · 2018-04-13T20:21:12Z

Also does it support retrieving existing builtin policy definition? ie: "Allowed locations" def

ghost · 2020-03-31T15:12:37Z

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!

New resource to manage azure policies

c287731

tombuildsstuff self-assigned this Mar 22, 2018

tombuildsstuff added service/policy new-resource labels Mar 22, 2018

tombuildsstuff suggested changes Mar 22, 2018

View reviewed changes

tombuildsstuff added this to the 1.3.1 milestone Mar 22, 2018

tombuildsstuff removed their assignment Mar 22, 2018

Add documentation for policy definition resource

591d6ee

jaymitre force-pushed the master branch from dbcba5a to 591d6ee Compare March 22, 2018 20:31

Clean code based on PR feedback

632dfbb

tombuildsstuff reviewed Mar 22, 2018

View reviewed changes

Update policy definition resource documentation

1c9c58b

tombuildsstuff reviewed Mar 23, 2018

View reviewed changes

tombuildsstuff added the upstream/terraform This issue is blocked on an upstream issue within Terraform (Terraform Core/CLI, The Plugin SDK etc) label Mar 23, 2018

tombuildsstuff changed the title ~~New resource to manage azure policies~~ New Resource: azurerm_policy_definition Mar 23, 2018

Refactor policy definition resource

ece0f49

tombuildsstuff approved these changes Mar 27, 2018

View reviewed changes

jaymitre and others added 4 commits March 27, 2018 13:42

Update policy definition resource documentation

163edd5

Downgrading to v12.5.0-beta of the Azure SDK for Go

3046109

Adding the missing metadata field to the documentation

74156c0

tombuildsstuff mentioned this pull request Mar 27, 2018

Fixes for azurerm_policy_definition jaymitre/terraform-provider-azurerm#1

Merged

tombuildsstuff added waiting-response and removed upstream/terraform This issue is blocked on an upstream issue within Terraform (Terraform Core/CLI, The Plugin SDK etc) labels Mar 27, 2018

Merge pull request #1 from terraform-providers/policy-definition

06a5842

Fixes for `azurerm_policy_definition`

tombuildsstuff removed the waiting-response label Mar 27, 2018

tombuildsstuff approved these changes Mar 27, 2018

View reviewed changes

tombuildsstuff merged commit dfbbe6d into hashicorp:master Mar 27, 2018

tombuildsstuff added a commit that referenced this pull request Mar 27, 2018

Updating to include #1010

ac52029

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_policy_definition` #1010

New Resource: `azurerm_policy_definition` #1010

jaymitre commented Mar 20, 2018

tombuildsstuff left a comment

tombuildsstuff Mar 22, 2018

tombuildsstuff Mar 22, 2018

tombuildsstuff Mar 22, 2018

tombuildsstuff Mar 22, 2018

tombuildsstuff Mar 22, 2018

jaymitre Mar 22, 2018

tombuildsstuff Mar 22, 2018

tombuildsstuff Mar 22, 2018

tombuildsstuff Mar 22, 2018

tombuildsstuff Mar 22, 2018

tombuildsstuff Mar 22, 2018

jaymitre commented Mar 22, 2018 •

edited

Loading

tombuildsstuff Mar 22, 2018

tombuildsstuff Mar 22, 2018

jaymitre commented Mar 22, 2018

tombuildsstuff left a comment

tombuildsstuff Mar 23, 2018

jaymitre commented Mar 27, 2018

tombuildsstuff left a comment

tombuildsstuff Mar 27, 2018

tombuildsstuff Mar 27, 2018

tombuildsstuff Mar 27, 2018

tombuildsstuff Mar 27, 2018

tombuildsstuff commented Mar 27, 2018

jaymitre commented Mar 27, 2018 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff commented Mar 27, 2018

JasonNguyenTX commented Apr 13, 2018

JasonNguyenTX commented Apr 13, 2018

ghost commented Mar 31, 2020


		The following attributes are exported:

		* `id` - The policy defenition id.

New Resource: azurerm_policy_definition #1010

New Resource: azurerm_policy_definition #1010

Conversation

jaymitre commented Mar 20, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymitre commented Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymitre commented Mar 22, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymitre commented Mar 27, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff commented Mar 27, 2018

jaymitre commented Mar 27, 2018 • edited Loading

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff commented Mar 27, 2018

JasonNguyenTX commented Apr 13, 2018

JasonNguyenTX commented Apr 13, 2018

ghost commented Mar 31, 2020

New Resource: `azurerm_policy_definition` #1010

New Resource: `azurerm_policy_definition` #1010

jaymitre commented Mar 22, 2018 •

edited

Loading

jaymitre commented Mar 27, 2018 •

edited

Loading