From 3476c72b827e156e694719194d254c21e02f046a Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Tue, 31 Mar 2020 14:18:03 +0800 Subject: [PATCH 1/4] Add data source for policy set definition --- .../data_source_policy_set_definition.go | 141 ++++++++++++++++++ .../services/policy/parse/definition.go | 6 +- .../services/policy/parse/set_definition.go | 43 ++++++ .../policy/parse/set_definition_test.go | 85 +++++++++++ .../internal/services/policy/registration.go | 3 +- .../policy/resource_arm_policy_assignment.go | 15 +- .../resource_arm_policy_set_definition.go | 10 +- .../data_source_policy_set_definition_test.go | 75 ++++++++++ .../resource_arm_policy_remediation_test.go | 2 +- ...resource_arm_policy_set_definition_test.go | 49 +++--- .../services/policy/validate/definition.go | 22 +-- .../policy/validate/set_definition.go | 22 +++ website/azurerm.erb | 4 + .../d/policy_set_definition.html.markdown | 57 +++++++ .../r/policy_set_definition.html.markdown | 8 +- 15 files changed, 479 insertions(+), 63 deletions(-) create mode 100644 azurerm/internal/services/policy/data_source_policy_set_definition.go create mode 100644 azurerm/internal/services/policy/parse/set_definition.go create mode 100644 azurerm/internal/services/policy/parse/set_definition_test.go create mode 100644 azurerm/internal/services/policy/tests/data_source_policy_set_definition_test.go create mode 100644 azurerm/internal/services/policy/validate/set_definition.go create mode 100644 website/docs/d/policy_set_definition.html.markdown diff --git a/azurerm/internal/services/policy/data_source_policy_set_definition.go b/azurerm/internal/services/policy/data_source_policy_set_definition.go new file mode 100644 index 000000000000..e41f9edb3cef --- /dev/null +++ b/azurerm/internal/services/policy/data_source_policy_set_definition.go @@ -0,0 +1,141 @@ +package policy + +import ( + "context" + "encoding/json" + "fmt" + "time" + + "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2018-05-01/policy" + "github.com/hashicorp/terraform-plugin-sdk/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/helper/validation" + "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/clients" + "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/timeouts" +) + +func dataSourceArmPolicySetDefinition() *schema.Resource { + return &schema.Resource{ + Read: dataSourceArmPolicySetDefinitionRead, + + Timeouts: &schema.ResourceTimeout{ + Read: schema.DefaultTimeout(5 * time.Minute), + }, + + Schema: map[string]*schema.Schema{ + "display_name": { + Type: schema.TypeString, + Optional: true, + Computed: true, + ValidateFunc: validation.StringIsNotEmpty, + ExactlyOneOf: []string{"name", "display_name"}, + }, + + "name": { + Type: schema.TypeString, + Optional: true, + Computed: true, + ValidateFunc: validation.StringIsNotEmpty, + ExactlyOneOf: []string{"name", "display_name"}, + }, + + "management_group_id": { + Type: schema.TypeString, + Optional: true, + }, + + "description": { + Type: schema.TypeString, + Computed: true, + }, + + "metadata": { + Type: schema.TypeString, + Computed: true, + }, + + "parameters": { + Type: schema.TypeString, + Computed: true, + }, + + "policy_definitions": { + Type: schema.TypeString, + Computed: true, + }, + + "policy_type": { + Type: schema.TypeString, + Computed: true, + }, + }, + } +} + +func dataSourceArmPolicySetDefinitionRead(d *schema.ResourceData, meta interface{}) error { + client := meta.(*clients.Client).Policy.SetDefinitionsClient + ctx, cancel := timeouts.ForRead(meta.(*clients.Client).StopContext, d) + defer cancel() + + name := d.Get("name").(string) + displayName := d.Get("display_name").(string) + managementGroupID := d.Get("management_group_id").(string) + + var setDefinition policy.SetDefinition + var err error + + if displayName != "" { + setDefinition, err = getPolicySetDefinitionByDisplayName(ctx, client, displayName, managementGroupID) + if err != nil { + return fmt.Errorf("failed to read Policy Set Definition (Display Name %q): %+v", displayName, err) + } + } + if name != "" { + setDefinition, err = getPolicySetDefinition(ctx, client, name, managementGroupID) + if err != nil { + return fmt.Errorf("failed to read Policy Set Definition %q: %+v", name, err) + } + } + + d.SetId(*setDefinition.ID) + d.Set("name", setDefinition.Name) + d.Set("display_name", setDefinition.DisplayName) + d.Set("description", setDefinition.Description) + d.Set("policy_type", setDefinition.PolicyType) + d.Set("metadata", flattenJSON(setDefinition.Metadata)) + d.Set("parameters", flattenJSON(setDefinition.Parameters)) + + definitionBytes, err := json.Marshal(setDefinition.PolicyDefinitions) + if err != nil { + return fmt.Errorf("unable to flatten JSON for `policy_defintions`: %+v", err) + } + d.Set("policy_definitions", string(definitionBytes)) + + return nil +} + +func getPolicySetDefinitionByDisplayName(ctx context.Context, client *policy.SetDefinitionsClient, displayName, managementGroupID string) (policy.SetDefinition, error) { + var setDefinitions policy.SetDefinitionListResultIterator + var err error + + if managementGroupID != "" { + setDefinitions, err = client.ListByManagementGroupComplete(ctx, managementGroupID) + } else { + setDefinitions, err = client.ListComplete(ctx) + } + if err != nil { + return policy.SetDefinition{}, fmt.Errorf("failed to load Policy Definition List: %+v", err) + } + + for setDefinitions.NotDone() { + def := setDefinitions.Value() + if def.DisplayName != nil && *def.DisplayName == displayName && def.ID != nil { + return def, nil + } + + if err := setDefinitions.NextWithContext(ctx); err != nil { + return policy.SetDefinition{}, fmt.Errorf("failed to load Policy Definition List: %s", err) + } + } + + return policy.SetDefinition{}, fmt.Errorf("failed to load Policy Definition List: could not find policy '%s'", displayName) +} diff --git a/azurerm/internal/services/policy/parse/definition.go b/azurerm/internal/services/policy/parse/definition.go index 16dc432f1eee..50b28992e694 100644 --- a/azurerm/internal/services/policy/parse/definition.go +++ b/azurerm/internal/services/policy/parse/definition.go @@ -10,10 +10,10 @@ type PolicyDefinitionId struct { PolicyScopeId } -// TODO: This paring function is currently suppressing every case difference due to github issue: https://github.com/Azure/azure-rest-api-specs/issues/8353 +// TODO: This parsing function is currently suppressing every case difference due to github issue: https://github.com/Azure/azure-rest-api-specs/issues/8353 func PolicyDefinitionID(input string) (*PolicyDefinitionId, error) { // in general, the id of a definition should be: - // {scope}/providers/Microsoft.PolicyInsights/remediations/{name} + // {scope}/providers/Microsoft.Authorization/policyDefinition/{name} regex := regexp.MustCompile(`/providers/[Mm]icrosoft\.[Aa]uthorization/policy[Dd]efinitions/`) if !regex.MatchString(input) { return nil, fmt.Errorf("unable to parse Policy Definition ID %q", input) @@ -28,7 +28,7 @@ func PolicyDefinitionID(input string) (*PolicyDefinitionId, error) { scope := segments[0] name := segments[1] if name == "" { - return nil, fmt.Errorf("unable to parse Policy Definition ID %q: assignment name is empty", input) + return nil, fmt.Errorf("unable to parse Policy Definition ID %q: definition name is empty", input) } scopeId, err := PolicyScopeID(scope) diff --git a/azurerm/internal/services/policy/parse/set_definition.go b/azurerm/internal/services/policy/parse/set_definition.go new file mode 100644 index 000000000000..5323a47f99bd --- /dev/null +++ b/azurerm/internal/services/policy/parse/set_definition.go @@ -0,0 +1,43 @@ +package parse + +import ( + "fmt" + "regexp" +) + +type PolicySetDefinitionId struct { + Name string + PolicyScopeId +} + +// TODO: This parsing function is currently suppressing case difference due to github issue: https://github.com/Azure/azure-rest-api-specs/issues/8353 +func PolicySetDefinitionID(input string) (*PolicySetDefinitionId, error) { + // in general, the id of a set definition should be: + // {scope}/providers/Microsoft.Authorization/policySetDefinitions/set1 + regex := regexp.MustCompile(`/providers/[Mm]icrosoft.[Aa]uthorization/policy[Ss]et[Dd]efinitions/`) + if !regex.MatchString(input) { + return nil, fmt.Errorf("unable to parse Policy Set Definition ID %q", input) + } + + segments := regex.Split(input, -1) + + if len(segments) != 2 { + return nil, fmt.Errorf("unable to parse Policy Set Definition ID %q: Expected 2 segments after split", input) + } + + scope := segments[0] + name := segments[1] + if name == "" { + return nil, fmt.Errorf("unable to parse Policy Set Definition ID %q: set definition name is empty", input) + } + + scopeId, err := PolicyScopeID(scope) + if err != nil { + return nil, fmt.Errorf("unable to parse Policy Set Definition ID %q: %+v", input, err) + } + + return &PolicySetDefinitionId{ + Name: name, + PolicyScopeId: scopeId, + }, nil +} diff --git a/azurerm/internal/services/policy/parse/set_definition_test.go b/azurerm/internal/services/policy/parse/set_definition_test.go new file mode 100644 index 000000000000..846db10584ff --- /dev/null +++ b/azurerm/internal/services/policy/parse/set_definition_test.go @@ -0,0 +1,85 @@ +package parse + +import ( + "reflect" + "testing" +) + +func TestPolicySetDefinitionID(t *testing.T) { + testData := []struct { + Name string + Input string + Error bool + Expected *PolicySetDefinitionId + }{ + { + Name: "empty", + Input: "", + Error: true, + }, + { + Name: "regular policy set definition", + Input: "/subscriptions/00000000-0000-0000-0000-000000000000/providers/Microsoft.Authorization/policySetDefinitions/set1", + Expected: &PolicySetDefinitionId{ + Name: "set1", + PolicyScopeId: ScopeAtSubscription{ + scopeId: "/subscriptions/00000000-0000-0000-0000-000000000000", + SubscriptionId: "00000000-0000-0000-0000-000000000000", + }, + }, + }, + { + Name: "regular policy set definition but no name", + Input: "/subscriptions/00000000-0000-0000-0000-000000000000/providers/Microsoft.Authorization/policySetDefinitions/", + Error: true, + }, + { + Name: "policy set definition in management group", + Input: "/providers/Microsoft.Management/managementGroups/00000000-0000-0000-0000-000000000000/providers/Microsoft.Authorization/policySetDefinitions/set1", + Expected: &PolicySetDefinitionId{ + Name: "set1", + PolicyScopeId: ScopeAtManagementGroup{ + scopeId: "/providers/Microsoft.Management/managementGroups/00000000-0000-0000-0000-000000000000", + ManagementGroupId: "00000000-0000-0000-0000-000000000000", + }, + }, + }, + { + Name: "policy set definition in management group with inconsistent casing", + Input: "/providers/Microsoft.Management/managementgroups/00000000-0000-0000-0000-000000000000/providers/Microsoft.Authorization/policySetDefinitions/set1", + Expected: &PolicySetDefinitionId{ + Name: "set1", + PolicyScopeId: ScopeAtManagementGroup{ + scopeId: "/providers/Microsoft.Management/managementgroups/00000000-0000-0000-0000-000000000000", + ManagementGroupId: "00000000-0000-0000-0000-000000000000", + }, + }, + }, + { + Name: "policy set definition in management group but no name", + Input: "/providers/Microsoft.Management/managementgroups/00000000-0000-0000-0000-000000000000/providers/Microsoft.Authorization/policySetDefinitions/", + Error: true, + }, + } + + for _, v := range testData { + t.Logf("[DEBUG] Testing %q", v.Name) + + actual, err := PolicySetDefinitionID(v.Input) + if err != nil { + if v.Error { + continue + } + + t.Fatalf("Expected a value but got an error: %+v", err) + } + + if actual.Name != v.Expected.Name { + t.Fatalf("Expected %q but got %q", v.Expected.Name, actual.Name) + } + + if !reflect.DeepEqual(v.Expected.PolicyScopeId, actual.PolicyScopeId) { + t.Fatalf("Expected %+v but got %+v", v.Expected.PolicyScopeId, actual.PolicyScopeId) + } + } +} diff --git a/azurerm/internal/services/policy/registration.go b/azurerm/internal/services/policy/registration.go index 8d5bd152e09e..bc904dfb8675 100644 --- a/azurerm/internal/services/policy/registration.go +++ b/azurerm/internal/services/policy/registration.go @@ -21,7 +21,8 @@ func (r Registration) WebsiteCategories() []string { // SupportedDataSources returns the supported Data Sources supported by this Service func (r Registration) SupportedDataSources() map[string]*schema.Resource { return map[string]*schema.Resource{ - "azurerm_policy_definition": dataSourceArmPolicyDefinition(), + "azurerm_policy_definition": dataSourceArmPolicyDefinition(), + "azurerm_policy_set_definition": dataSourceArmPolicySetDefinition(), } } diff --git a/azurerm/internal/services/policy/resource_arm_policy_assignment.go b/azurerm/internal/services/policy/resource_arm_policy_assignment.go index d845f8095b4b..776f55d88ecd 100644 --- a/azurerm/internal/services/policy/resource_arm_policy_assignment.go +++ b/azurerm/internal/services/policy/resource_arm_policy_assignment.go @@ -16,6 +16,9 @@ import ( "github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/tf" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/clients" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/features" + "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/policy/parse" + "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/policy/validate" + azSchema "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/tf/schema" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/timeouts" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils" ) @@ -26,9 +29,11 @@ func resourceArmPolicyAssignment() *schema.Resource { Update: resourceArmPolicyAssignmentCreateUpdate, Read: resourceArmPolicyAssignmentRead, Delete: resourceArmPolicyAssignmentDelete, - Importer: &schema.ResourceImporter{ - State: schema.ImportStatePassthrough, - }, + + Importer: azSchema.ValidateResourceIDPriorToImport(func(id string) error { + _, err := parse.PolicyAssignmentID(id) + return err + }), Timeouts: &schema.ResourceTimeout{ Create: schema.DefaultTimeout(30 * time.Minute), @@ -54,6 +59,10 @@ func resourceArmPolicyAssignment() *schema.Resource { Type: schema.TypeString, Required: true, ForceNew: true, + ValidateFunc: validation.Any( + validate.PolicyDefinitionID, + validate.PolicySetDefinitionID, + ), }, "description": { diff --git a/azurerm/internal/services/policy/resource_arm_policy_set_definition.go b/azurerm/internal/services/policy/resource_arm_policy_set_definition.go index a979f32d31fc..17c6b0dd5e4a 100644 --- a/azurerm/internal/services/policy/resource_arm_policy_set_definition.go +++ b/azurerm/internal/services/policy/resource_arm_policy_set_definition.go @@ -20,6 +20,8 @@ import ( "github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/tf" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/clients" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/features" + "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/policy/parse" + azSchema "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/tf/schema" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/timeouts" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils" ) @@ -30,9 +32,11 @@ func resourceArmPolicySetDefinition() *schema.Resource { Update: resourceArmPolicySetDefinitionCreateUpdate, Read: resourceArmPolicySetDefinitionRead, Delete: resourceArmPolicySetDefinitionDelete, - Importer: &schema.ResourceImporter{ - State: schema.ImportStatePassthrough, - }, + + Importer: azSchema.ValidateResourceIDPriorToImport(func(id string) error { + _, err := parse.PolicySetDefinitionID(id) + return err + }), Timeouts: &schema.ResourceTimeout{ Create: schema.DefaultTimeout(30 * time.Minute), diff --git a/azurerm/internal/services/policy/tests/data_source_policy_set_definition_test.go b/azurerm/internal/services/policy/tests/data_source_policy_set_definition_test.go new file mode 100644 index 000000000000..0c5270695086 --- /dev/null +++ b/azurerm/internal/services/policy/tests/data_source_policy_set_definition_test.go @@ -0,0 +1,75 @@ +package tests + +import ( + "fmt" + "testing" + + "github.com/hashicorp/terraform-plugin-sdk/helper/resource" + "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/acceptance" +) + +func TestAccDataSourceAzureRMPolicySetDefinition_customByName(t *testing.T) { + data := acceptance.BuildTestData(t, "data.azurerm_policy_set_definition", "test") + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acceptance.PreCheck(t) }, + Providers: acceptance.SupportedProviders, + CheckDestroy: testCheckAzureRMPolicySetDefinitionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccDataSourceAzureRMPolicySetDefinition_customByName(data), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(data.ResourceName, "name", fmt.Sprintf("acctestpolset-%d", data.RandomInteger)), + resource.TestCheckResourceAttr(data.ResourceName, "display_name", fmt.Sprintf("acctestpolset-display-%d", data.RandomInteger)), + resource.TestCheckResourceAttr(data.ResourceName, "policy_type", "Custom"), + resource.TestCheckResourceAttrSet(data.ResourceName, "parameters"), + resource.TestCheckResourceAttrSet(data.ResourceName, "policy_definitions"), + ), + }, + }, + }) +} + +func TestAccDataSourceAzureRMPolicySetDefinition_customByDisplayName(t *testing.T) { + data := acceptance.BuildTestData(t, "data.azurerm_policy_set_definition", "test") + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acceptance.PreCheck(t) }, + Providers: acceptance.SupportedProviders, + CheckDestroy: testCheckAzureRMPolicySetDefinitionDestroy, + Steps: []resource.TestStep{ + { + Config: testAccDataSourceAzureRMPolicySetDefinition_customByDisplayName(data), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr(data.ResourceName, "name", fmt.Sprintf("acctestpolset-%d", data.RandomInteger)), + resource.TestCheckResourceAttr(data.ResourceName, "display_name", fmt.Sprintf("acctestpolset-display-%d", data.RandomInteger)), + resource.TestCheckResourceAttr(data.ResourceName, "policy_type", "Custom"), + resource.TestCheckResourceAttrSet(data.ResourceName, "parameters"), + resource.TestCheckResourceAttrSet(data.ResourceName, "policy_definitions"), + ), + }, + }, + }) +} + +func testAccDataSourceAzureRMPolicySetDefinition_customByName(data acceptance.TestData) string { + template := testAzureRMPolicySetDefinition_custom(data) + return fmt.Sprintf(` +%s + +data "azurerm_policy_set_definition" "test" { + name = azurerm_policy_set_definition.test.name +} +`, template) +} + +func testAccDataSourceAzureRMPolicySetDefinition_customByDisplayName(data acceptance.TestData) string { + template := testAzureRMPolicySetDefinition_custom(data) + return fmt.Sprintf(` +%s + +data "azurerm_policy_set_definition" "test" { + display_name = azurerm_policy_set_definition.test.display_name +} +`, template) +} diff --git a/azurerm/internal/services/policy/tests/resource_arm_policy_remediation_test.go b/azurerm/internal/services/policy/tests/resource_arm_policy_remediation_test.go index 0508b0ce86ad..029dc959f83e 100644 --- a/azurerm/internal/services/policy/tests/resource_arm_policy_remediation_test.go +++ b/azurerm/internal/services/policy/tests/resource_arm_policy_remediation_test.go @@ -298,7 +298,7 @@ provider "azurerm" { data "azurerm_subscription" "current" {} resource "azurerm_policy_set_definition" "test" { - name = "testPolicySet" + name = "acctest-set-%[1]d" policy_type = "Custom" display_name = "Test Policy Set" diff --git a/azurerm/internal/services/policy/tests/resource_arm_policy_set_definition_test.go b/azurerm/internal/services/policy/tests/resource_arm_policy_set_definition_test.go index 3a233cd19069..629d761e0418 100644 --- a/azurerm/internal/services/policy/tests/resource_arm_policy_set_definition_test.go +++ b/azurerm/internal/services/policy/tests/resource_arm_policy_set_definition_test.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/terraform" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/acceptance" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/clients" + "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/policy/parse" "github.com/terraform-providers/terraform-provider-azurerm/azurerm/utils" ) @@ -108,7 +109,6 @@ resource "azurerm_policy_set_definition" "test" { } PARAMETERS - policy_definitions = <azurerm_policy_definition +
  • + azurerm_policy_set_definition +
    • +
  • azurerm_postgresql_server
    • diff --git a/website/docs/d/policy_set_definition.html.markdown b/website/docs/d/policy_set_definition.html.markdown new file mode 100644 index 000000000000..26eb5cde7f29 --- /dev/null +++ b/website/docs/d/policy_set_definition.html.markdown @@ -0,0 +1,57 @@ +--- +subcategory: "Policy" +layout: "azurerm" +page_title: "Azure Resource Manager: azurerm_policy_set_definition" +description: |- + Get information about a Policy Set Definition. +--- + +# Data Source: azurerm_policy_set_definition + +Use this data source to access information about an existing Policy Set Definition. + +## Example Usage + +```hcl +provider "azurerm" { + features {} +} + +data "azurerm_policy_set_definition" "example" { + display_name = "Policy Set Definition Example" +} + +output "id" { + value = data.azurerm_policy_set_definition.example.id +} +``` + +## Argument Reference + +* `name` - Specifies the name of the Policy Set Definition. Conflicts with `display_name`. + +* `display_name` - Specifies the display name of the Policy Set Definition. Conflicts with `name`. + +~> **NOTE** Since `display_name` does not have uniqueness on Azure, therefore when using `display_name` to retrieve existing policy set definitions, only one of the policy set definitions with same `display_name` will be retrieved. + +* `management_group_id` - (Optional) Only retrieve Policy Set Definitions from this Management Group. + +## Attributes Reference + +* `id` - The ID of the Policy Set Definition. + +* `description` - The Description of the Policy Set Definition. + +* `policy_type` - The Type of the Policy Set Definition. Possible values are `BuiltIn` and `Custom`. + +* `policy_definitions` - The policy definitions in the policy set definition. + +* `parameters` - Any Parameters defined in the Policy Set Definition. + +* `metadata` - Any Metadata defined in the Policy Set Definition. + +## Timeouts + +The `timeouts` block allows you to specify [timeouts](https://www.terraform.io/docs/configuration/resources.html#timeouts) for certain actions: + +* `read` - (Defaults to 5 minutes) Used when retrieving the Policy Set Definition. diff --git a/website/docs/r/policy_set_definition.html.markdown b/website/docs/r/policy_set_definition.html.markdown index 4a5ba8e9b800..9084f00405c4 100644 --- a/website/docs/r/policy_set_definition.html.markdown +++ b/website/docs/r/policy_set_definition.html.markdown @@ -15,6 +15,10 @@ Manages a policy set definition. ## Example Usage ```hcl +provider "azurerm" { + features {} +} + resource "azurerm_policy_set_definition" "example" { name = "testPolicySet" policy_type = "Custom" @@ -33,7 +37,6 @@ resource "azurerm_policy_set_definition" "example" { } PARAMETERS - policy_definitions = < Date: Wed, 8 Apr 2020 13:47:39 +0800 Subject: [PATCH 2/4] Resolve comments --- .../data_source_policy_set_definition.go | 34 ++---------- .../services/policy/parse/definition.go | 2 +- .../services/policy/parse/set_definition.go | 2 +- azurerm/internal/services/policy/policy.go | 55 +++++++++++++++++++ .../resource_arm_policy_set_definition.go | 18 ++---- .../data_source_policy_set_definition_test.go | 20 +++---- .../resource_arm_policy_remediation_test.go | 2 +- ...resource_arm_policy_set_definition_test.go | 4 +- .../d/policy_set_definition.html.markdown | 6 +- 9 files changed, 81 insertions(+), 62 deletions(-) create mode 100644 azurerm/internal/services/policy/policy.go diff --git a/azurerm/internal/services/policy/data_source_policy_set_definition.go b/azurerm/internal/services/policy/data_source_policy_set_definition.go index e41f9edb3cef..e8c18497a79c 100644 --- a/azurerm/internal/services/policy/data_source_policy_set_definition.go +++ b/azurerm/internal/services/policy/data_source_policy_set_definition.go @@ -38,7 +38,7 @@ func dataSourceArmPolicySetDefinition() *schema.Resource { ExactlyOneOf: []string{"name", "display_name"}, }, - "management_group_id": { + "management_group_name": { Type: schema.TypeString, Optional: true, }, @@ -78,11 +78,12 @@ func dataSourceArmPolicySetDefinitionRead(d *schema.ResourceData, meta interface name := d.Get("name").(string) displayName := d.Get("display_name").(string) - managementGroupID := d.Get("management_group_id").(string) + managementGroupID := d.Get("management_group_name").(string) var setDefinition policy.SetDefinition var err error + // we marked `display_name` and `name` as `ExactlyOneOf`, therefore there will only be one of display_name and name that have non-empty value here if displayName != "" { setDefinition, err = getPolicySetDefinitionByDisplayName(ctx, client, displayName, managementGroupID) if err != nil { @@ -90,7 +91,7 @@ func dataSourceArmPolicySetDefinitionRead(d *schema.ResourceData, meta interface } } if name != "" { - setDefinition, err = getPolicySetDefinition(ctx, client, name, managementGroupID) + setDefinition, err = getPolicySetDefinitionByName(ctx, client, name, managementGroupID) if err != nil { return fmt.Errorf("failed to read Policy Set Definition %q: %+v", name, err) } @@ -112,30 +113,3 @@ func dataSourceArmPolicySetDefinitionRead(d *schema.ResourceData, meta interface return nil } - -func getPolicySetDefinitionByDisplayName(ctx context.Context, client *policy.SetDefinitionsClient, displayName, managementGroupID string) (policy.SetDefinition, error) { - var setDefinitions policy.SetDefinitionListResultIterator - var err error - - if managementGroupID != "" { - setDefinitions, err = client.ListByManagementGroupComplete(ctx, managementGroupID) - } else { - setDefinitions, err = client.ListComplete(ctx) - } - if err != nil { - return policy.SetDefinition{}, fmt.Errorf("failed to load Policy Definition List: %+v", err) - } - - for setDefinitions.NotDone() { - def := setDefinitions.Value() - if def.DisplayName != nil && *def.DisplayName == displayName && def.ID != nil { - return def, nil - } - - if err := setDefinitions.NextWithContext(ctx); err != nil { - return policy.SetDefinition{}, fmt.Errorf("failed to load Policy Definition List: %s", err) - } - } - - return policy.SetDefinition{}, fmt.Errorf("failed to load Policy Definition List: could not find policy '%s'", displayName) -} diff --git a/azurerm/internal/services/policy/parse/definition.go b/azurerm/internal/services/policy/parse/definition.go index 50b28992e694..2ca10d050b1a 100644 --- a/azurerm/internal/services/policy/parse/definition.go +++ b/azurerm/internal/services/policy/parse/definition.go @@ -13,7 +13,7 @@ type PolicyDefinitionId struct { // TODO: This parsing function is currently suppressing every case difference due to github issue: https://github.com/Azure/azure-rest-api-specs/issues/8353 func PolicyDefinitionID(input string) (*PolicyDefinitionId, error) { // in general, the id of a definition should be: - // {scope}/providers/Microsoft.Authorization/policyDefinition/{name} + // {scope}/providers/Microsoft.Authorization/policyDefinitions/{name} regex := regexp.MustCompile(`/providers/[Mm]icrosoft\.[Aa]uthorization/policy[Dd]efinitions/`) if !regex.MatchString(input) { return nil, fmt.Errorf("unable to parse Policy Definition ID %q", input) diff --git a/azurerm/internal/services/policy/parse/set_definition.go b/azurerm/internal/services/policy/parse/set_definition.go index 5323a47f99bd..2a01d4946b54 100644 --- a/azurerm/internal/services/policy/parse/set_definition.go +++ b/azurerm/internal/services/policy/parse/set_definition.go @@ -14,7 +14,7 @@ type PolicySetDefinitionId struct { func PolicySetDefinitionID(input string) (*PolicySetDefinitionId, error) { // in general, the id of a set definition should be: // {scope}/providers/Microsoft.Authorization/policySetDefinitions/set1 - regex := regexp.MustCompile(`/providers/[Mm]icrosoft.[Aa]uthorization/policy[Ss]et[Dd]efinitions/`) + regex := regexp.MustCompile(`/providers/[Mm]icrosoft\.[Aa]uthorization/policy[Ss]et[Dd]efinitions/`) if !regex.MatchString(input) { return nil, fmt.Errorf("unable to parse Policy Set Definition ID %q", input) } diff --git a/azurerm/internal/services/policy/policy.go b/azurerm/internal/services/policy/policy.go new file mode 100644 index 000000000000..0bb172a8b2ad --- /dev/null +++ b/azurerm/internal/services/policy/policy.go @@ -0,0 +1,55 @@ +package policy + +import ("context" + "fmt" + + "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2018-05-01/policy" +) + +func getPolicySetDefinitionByName(ctx context.Context, client *policy.SetDefinitionsClient, name string, managementGroupID string) (res policy.SetDefinition, err error) { + if managementGroupID == "" { + res, err = client.Get(ctx, name) + } else { + res, err = client.GetAtManagementGroup(ctx, name, managementGroupID) + } + + return res, err +} + +func getPolicySetDefinitionByDisplayName(ctx context.Context, client *policy.SetDefinitionsClient, displayName, managementGroupID string) (policy.SetDefinition, error) { + var setDefinitions policy.SetDefinitionListResultIterator + var err error + + if managementGroupID != "" { + setDefinitions, err = client.ListByManagementGroupComplete(ctx, managementGroupID) + } else { + setDefinitions, err = client.ListComplete(ctx) + } + if err != nil { + return policy.SetDefinition{}, fmt.Errorf("failed to load Policy Set Definition List: %+v", err) + } + + var results []policy.SetDefinition + for setDefinitions.NotDone() { + def := setDefinitions.Value() + if def.DisplayName != nil && *def.DisplayName == displayName && def.ID != nil { + results = append(results, def) + } + + if err := setDefinitions.NextWithContext(ctx); err != nil { + return policy.SetDefinition{}, fmt.Errorf("failed to load Policy Set Definition List: %s", err) + } + } + + // throw error when we found none + if len(results) == 0 { + return policy.SetDefinition{}, fmt.Errorf("failed to load Policy Set Definition List: could not find policy '%s'", displayName) + } + + // throw error when we found more than one + if len(results) > 1 { + return policy.SetDefinition{}, fmt.Errorf("failed to load Policy Set Definition List: found more than one policy set definition '%s'", displayName) + } + + return results[0], nil +} diff --git a/azurerm/internal/services/policy/resource_arm_policy_set_definition.go b/azurerm/internal/services/policy/resource_arm_policy_set_definition.go index 17c6b0dd5e4a..552d16185b30 100644 --- a/azurerm/internal/services/policy/resource_arm_policy_set_definition.go +++ b/azurerm/internal/services/policy/resource_arm_policy_set_definition.go @@ -133,7 +133,7 @@ func resourceArmPolicySetDefinitionCreateUpdate(d *schema.ResourceData, meta int managementGroupID := d.Get("management_group_id").(string) if features.ShouldResourcesBeImported() && d.IsNewResource() { - existing, err := getPolicySetDefinition(ctx, client, name, managementGroupID) + existing, err := getPolicySetDefinitionByName(ctx, client, name, managementGroupID) if err != nil { if !utils.ResponseWasNotFound(existing.Response) { return fmt.Errorf("Error checking for presence of existing Policy Set Definition %q: %s", name, err) @@ -213,7 +213,7 @@ func resourceArmPolicySetDefinitionCreateUpdate(d *schema.ResourceData, meta int } var resp policy.SetDefinition - resp, err = getPolicySetDefinition(ctx, client, name, managementGroupID) + resp, err = getPolicySetDefinitionByName(ctx, client, name, managementGroupID) if err != nil { return fmt.Errorf("Error retrieving Policy Set Definition %q: %s", name, err) } @@ -235,7 +235,7 @@ func resourceArmPolicySetDefinitionRead(d *schema.ResourceData, meta interface{} managementGroupID := parseManagementGroupIdFromPolicySetId(d.Id()) - resp, err := getPolicySetDefinition(ctx, client, name, managementGroupID) + resp, err := getPolicySetDefinitionByName(ctx, client, name, managementGroupID) if err != nil { if utils.ResponseWasNotFound(resp.Response) { @@ -342,7 +342,7 @@ func parseManagementGroupIdFromPolicySetId(id string) string { func policySetDefinitionRefreshFunc(ctx context.Context, client *policy.SetDefinitionsClient, name string, managementGroupId string) resource.StateRefreshFunc { return func() (interface{}, string, error) { - res, err := getPolicySetDefinition(ctx, client, name, managementGroupId) + res, err := getPolicySetDefinitionByName(ctx, client, name, managementGroupId) if err != nil { return nil, strconv.Itoa(res.StatusCode), fmt.Errorf("Error issuing read request in policySetDefinitionRefreshFunc for Policy Set Definition %q: %s", name, err) } @@ -350,13 +350,3 @@ func policySetDefinitionRefreshFunc(ctx context.Context, client *policy.SetDefin return res, strconv.Itoa(res.StatusCode), nil } } - -func getPolicySetDefinition(ctx context.Context, client *policy.SetDefinitionsClient, name string, managementGroupID string) (res policy.SetDefinition, err error) { - if managementGroupID == "" { - res, err = client.Get(ctx, name) - } else { - res, err = client.GetAtManagementGroup(ctx, name, managementGroupID) - } - - return res, err -} diff --git a/azurerm/internal/services/policy/tests/data_source_policy_set_definition_test.go b/azurerm/internal/services/policy/tests/data_source_policy_set_definition_test.go index 0c5270695086..643b3d85c544 100644 --- a/azurerm/internal/services/policy/tests/data_source_policy_set_definition_test.go +++ b/azurerm/internal/services/policy/tests/data_source_policy_set_definition_test.go @@ -8,7 +8,7 @@ import ( "github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/acceptance" ) -func TestAccDataSourceAzureRMPolicySetDefinition_customByName(t *testing.T) { +func TestAccDataSourceAzureRMPolicySetDefinition_byName(t *testing.T) { data := acceptance.BuildTestData(t, "data.azurerm_policy_set_definition", "test") resource.ParallelTest(t, resource.TestCase{ @@ -17,10 +17,10 @@ func TestAccDataSourceAzureRMPolicySetDefinition_customByName(t *testing.T) { CheckDestroy: testCheckAzureRMPolicySetDefinitionDestroy, Steps: []resource.TestStep{ { - Config: testAccDataSourceAzureRMPolicySetDefinition_customByName(data), + Config: testAccDataSourceAzureRMPolicySetDefinition_byName(data), Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr(data.ResourceName, "name", fmt.Sprintf("acctestpolset-%d", data.RandomInteger)), - resource.TestCheckResourceAttr(data.ResourceName, "display_name", fmt.Sprintf("acctestpolset-display-%d", data.RandomInteger)), + resource.TestCheckResourceAttr(data.ResourceName, "name", fmt.Sprintf("acctestPolSet-%d", data.RandomInteger)), + resource.TestCheckResourceAttr(data.ResourceName, "display_name", fmt.Sprintf("acctestPolSet-display-%d", data.RandomInteger)), resource.TestCheckResourceAttr(data.ResourceName, "policy_type", "Custom"), resource.TestCheckResourceAttrSet(data.ResourceName, "parameters"), resource.TestCheckResourceAttrSet(data.ResourceName, "policy_definitions"), @@ -30,7 +30,7 @@ func TestAccDataSourceAzureRMPolicySetDefinition_customByName(t *testing.T) { }) } -func TestAccDataSourceAzureRMPolicySetDefinition_customByDisplayName(t *testing.T) { +func TestAccDataSourceAzureRMPolicySetDefinition_byDisplayName(t *testing.T) { data := acceptance.BuildTestData(t, "data.azurerm_policy_set_definition", "test") resource.ParallelTest(t, resource.TestCase{ @@ -39,10 +39,10 @@ func TestAccDataSourceAzureRMPolicySetDefinition_customByDisplayName(t *testing. CheckDestroy: testCheckAzureRMPolicySetDefinitionDestroy, Steps: []resource.TestStep{ { - Config: testAccDataSourceAzureRMPolicySetDefinition_customByDisplayName(data), + Config: testAccDataSourceAzureRMPolicySetDefinition_byDisplayName(data), Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr(data.ResourceName, "name", fmt.Sprintf("acctestpolset-%d", data.RandomInteger)), - resource.TestCheckResourceAttr(data.ResourceName, "display_name", fmt.Sprintf("acctestpolset-display-%d", data.RandomInteger)), + resource.TestCheckResourceAttr(data.ResourceName, "name", fmt.Sprintf("acctestPolSet-%d", data.RandomInteger)), + resource.TestCheckResourceAttr(data.ResourceName, "display_name", fmt.Sprintf("acctestPolSet-display-%d", data.RandomInteger)), resource.TestCheckResourceAttr(data.ResourceName, "policy_type", "Custom"), resource.TestCheckResourceAttrSet(data.ResourceName, "parameters"), resource.TestCheckResourceAttrSet(data.ResourceName, "policy_definitions"), @@ -52,7 +52,7 @@ func TestAccDataSourceAzureRMPolicySetDefinition_customByDisplayName(t *testing. }) } -func testAccDataSourceAzureRMPolicySetDefinition_customByName(data acceptance.TestData) string { +func testAccDataSourceAzureRMPolicySetDefinition_byName(data acceptance.TestData) string { template := testAzureRMPolicySetDefinition_custom(data) return fmt.Sprintf(` %s @@ -63,7 +63,7 @@ data "azurerm_policy_set_definition" "test" { `, template) } -func testAccDataSourceAzureRMPolicySetDefinition_customByDisplayName(data acceptance.TestData) string { +func testAccDataSourceAzureRMPolicySetDefinition_byDisplayName(data acceptance.TestData) string { template := testAzureRMPolicySetDefinition_custom(data) return fmt.Sprintf(` %s diff --git a/azurerm/internal/services/policy/tests/resource_arm_policy_remediation_test.go b/azurerm/internal/services/policy/tests/resource_arm_policy_remediation_test.go index 029dc959f83e..588618680fab 100644 --- a/azurerm/internal/services/policy/tests/resource_arm_policy_remediation_test.go +++ b/azurerm/internal/services/policy/tests/resource_arm_policy_remediation_test.go @@ -298,7 +298,7 @@ provider "azurerm" { data "azurerm_subscription" "current" {} resource "azurerm_policy_set_definition" "test" { - name = "acctest-set-%[1]d" + name = "acctest-policy-%[1]d" policy_type = "Custom" display_name = "Test Policy Set" diff --git a/azurerm/internal/services/policy/tests/resource_arm_policy_set_definition_test.go b/azurerm/internal/services/policy/tests/resource_arm_policy_set_definition_test.go index 629d761e0418..c04a38bdd140 100644 --- a/azurerm/internal/services/policy/tests/resource_arm_policy_set_definition_test.go +++ b/azurerm/internal/services/policy/tests/resource_arm_policy_set_definition_test.go @@ -180,9 +180,9 @@ PARAMETERS } resource "azurerm_policy_set_definition" "test" { - name = "acctestpolset-%d" + name = "acctestPolSet-%d" policy_type = "Custom" - display_name = "acctestpolset-display-%d" + display_name = "acctestPolSet-display-%d" parameters = < **NOTE** Since `display_name` does not have uniqueness on Azure, therefore when using `display_name` to retrieve existing policy set definitions, only one of the policy set definitions with same `display_name` will be retrieved. -* `management_group_id` - (Optional) Only retrieve Policy Set Definitions from this Management Group. +* `management_group_name` - (Optional) Only retrieve Policy Set Definitions from this Management Group. ## Attributes Reference @@ -42,9 +42,9 @@ output "id" { * `description` - The Description of the Policy Set Definition. -* `policy_type` - The Type of the Policy Set Definition. Possible values are `BuiltIn` and `Custom`. +* `policy_type` - The Type of the Policy Set Definition. -* `policy_definitions` - The policy definitions in the policy set definition. +* `policy_definitions` - The policy definitions contained within the policy set definition. * `parameters` - Any Parameters defined in the Policy Set Definition. From 065bdf99e31a3821dbd870d3af72c19b02516269 Mon Sep 17 00:00:00 2001 From: Dapeng Zhang Date: Wed, 8 Apr 2020 14:44:19 +0800 Subject: [PATCH 3/4] Fix CI --- .../services/policy/data_source_policy_set_definition.go | 1 - azurerm/internal/services/policy/policy.go | 5 +++-- website/docs/d/policy_set_definition.html.markdown | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/azurerm/internal/services/policy/data_source_policy_set_definition.go b/azurerm/internal/services/policy/data_source_policy_set_definition.go index e8c18497a79c..e551c57fb21d 100644 --- a/azurerm/internal/services/policy/data_source_policy_set_definition.go +++ b/azurerm/internal/services/policy/data_source_policy_set_definition.go @@ -1,7 +1,6 @@ package policy import ( - "context" "encoding/json" "fmt" "time" diff --git a/azurerm/internal/services/policy/policy.go b/azurerm/internal/services/policy/policy.go index 0bb172a8b2ad..8e21cb23679f 100644 --- a/azurerm/internal/services/policy/policy.go +++ b/azurerm/internal/services/policy/policy.go @@ -1,6 +1,7 @@ package policy -import ("context" +import ( + "context" "fmt" "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2018-05-01/policy" @@ -50,6 +51,6 @@ func getPolicySetDefinitionByDisplayName(ctx context.Context, client *policy.Set if len(results) > 1 { return policy.SetDefinition{}, fmt.Errorf("failed to load Policy Set Definition List: found more than one policy set definition '%s'", displayName) } - + return results[0], nil } diff --git a/website/docs/d/policy_set_definition.html.markdown b/website/docs/d/policy_set_definition.html.markdown index 0d616bb1d27b..d0860229dca0 100644 --- a/website/docs/d/policy_set_definition.html.markdown +++ b/website/docs/d/policy_set_definition.html.markdown @@ -32,7 +32,7 @@ output "id" { * `display_name` - Specifies the display name of the Policy Set Definition. Conflicts with `name`. -~> **NOTE** Since `display_name` does not have uniqueness on Azure, therefore when using `display_name` to retrieve existing policy set definitions, only one of the policy set definitions with same `display_name` will be retrieved. +**NOTE** Since `display_name` does not have uniqueness on Azure, therefore when using `display_name` to retrieve existing policy set definitions, errors may occur when there are multiple policy definitions with same display name. * `management_group_name` - (Optional) Only retrieve Policy Set Definitions from this Management Group. From e0c6dfc0c04e99b99168d97568635685aebdab7b Mon Sep 17 00:00:00 2001 From: kt Date: Thu, 9 Apr 2020 11:59:47 -0700 Subject: [PATCH 4/4] Update policy_set_definition.html.markdown --- website/docs/d/policy_set_definition.html.markdown | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/website/docs/d/policy_set_definition.html.markdown b/website/docs/d/policy_set_definition.html.markdown index d0860229dca0..178b1402447f 100644 --- a/website/docs/d/policy_set_definition.html.markdown +++ b/website/docs/d/policy_set_definition.html.markdown @@ -32,7 +32,7 @@ output "id" { * `display_name` - Specifies the display name of the Policy Set Definition. Conflicts with `name`. -**NOTE** Since `display_name` does not have uniqueness on Azure, therefore when using `display_name` to retrieve existing policy set definitions, errors may occur when there are multiple policy definitions with same display name. +**NOTE** As `display_name` is not unique errors may occur when there are multiple policy set definitions with same display name. * `management_group_name` - (Optional) Only retrieve Policy Set Definitions from this Management Group.