azurerm_container_registry: support for network_rule_set property

[fraserdarwent]

In response to #3134

(upgrades "github.com/Azure/azure-sdk-for-go/services/containerregistry/mgmt/2017-10-01/containerregistry" to 2018-09-01)

Example Terraform code

resource "azurerm_container_registry" "acr" {
  name = "registry"
  resource_group_name = "${azurerm_resource_group.main.name}"
  location = "${azurerm_resource_group.main.location}"
  sku = "Premium"
  admin_enabled = false
  
  network_rules_profile {
    default_action = "Deny"
    
    ip_rule {
      action = "Allow"
      ip_range = "8.8.8.8/32"
    }
    
    subnet_rule {
      action = "Allow"
      subnet_id = "${azurerm_subnet.main.id}"
    }
  }

Getting an early PR in for feedback.
Still need to update docs and tests.

Fixes #3134

[tombuildsstuff]

hey @fraserdarwent

Thanks for this PR - apologies for the delayed review here!

Taking a look through this mostly LGTM - I've left some (mostly minor) comments in-line, but the main thing remaining is the tests; if we can fix up the comments and add some tests this should otherwise be good to merge 👍

Thanks!

[tombuildsstuff]

we're making all new fields case-sensitive; as such can we make this:

Suggested change

	}, true),
	}, false),

[tombuildsstuff]

(as above) we're making all new fields case-sensitive - as such can we make this:

Suggested change

	}, true),
	}, false),

[tombuildsstuff]

since this is a Subnet/Resource ID can we validate this:

Suggested change

	Required: true,
	Required: true,
	ValidateFunc: validate.AzureResourceID,

[tombuildsstuff]

(as above) all new fields in the provider are case-sensitive - as such can we make this:

Suggested change

	}, true),
	}, false),

[tombuildsstuff]

for consistency with other resources can we make this:

Suggested change

	* `action` - (Required) The behaviour for requests matching this rule. Can only be `Allow`
	* `action` - (Required) The behaviour for requests matching this rule. At this time the only supported value is `Allow`

[tombuildsstuff]

for consistency with other resources can we make this:

Suggested change

	* `subnet_id` - (Required) Resource ID of the subnet from which requests will match the rule.
	* `subnet_id` - (Required) The ID of the Subnet from which requests will match the rule.

[tombuildsstuff]

we can remove this last item here:

Suggested change

	virtualNetworkRules := make([]containerregistry.VirtualNetworkRule, 0, len(virtualNetworkRuleConfigs))
	virtualNetworkRules := make([]containerregistry.VirtualNetworkRule, 0)

[tombuildsstuff]

since we're making this a List (above) we can make this:

Suggested change

	configs := d.Get("network_access_profile").(*schema.Set).List()
	configs := d.Get("network_access_profile").([]interface{})

[tombuildsstuff]

(as above) we can remove this final argument here:

Suggested change

	ipRules := make([]containerregistry.IPRule, 0, len(ipRuleConfigs))
	ipRules := make([]containerregistry.IPRule, 0)

[fraserdarwent]

@tombuildsstuff addressed comments

[fraserdarwent]

@katbyte made suggested changes to pass the tests as the value is computed

[katbyte]

Hi @fraserdarwent,
Still seeing mulitple tests failing with:

------- Stdout: -------
=== RUN   TestAccAzureRMContainerRegistry_geoReplication
=== PAUSE TestAccAzureRMContainerRegistry_geoReplication
=== CONT  TestAccAzureRMContainerRegistry_geoReplication
--- FAIL: TestAccAzureRMContainerRegistry_geoReplication (405.09s)
    testing.go:568: Step 4 error: errors during apply:
        
        Error: `network_rule_set` can only be specified for a Premium Sku.
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test829217438/main.tf line 7:
          (source code not available)
        
        
FAIL

[katbyte]

Hi @fraserdarwent,

I hope you don't mind but i've pushed some changes to this branch to fix the above error, rename the property to better match the API as well as adding some more tests so I can merge this. It appears VNET rules are being ignored by the API? and only ip_rules are actually being set. As such i have opened an issue on the SDK and removed that from this PR. I'll open a new PR shortly with that property so once the API is fixed we can get that merged in too 🙂

[katbyte]

Have pushed required changes

[ghost]

This has been released in version 1.32.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 = "~> 1.32.0"
}
# ... other configuration ...

[ghost]

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 👉 hashibot-feedback@hashicorp.com. Thanks!