[MS] provider/azurerm: New resource - Express Route Circuit #14265

whiskeyjay · 2017-05-06T06:40:34Z

#14122 Adding basic support for Express Route Circuit.

Setting up an Express Route Circuit involves two major steps:

Creating an Express Route Circuit, then provide the "service key" to the connectivity provider for provisioning on their side.
Once the connection is provisioned by the provider, you can then continue to setup peering.

This change enables the first step, once the resource is created, the service key is returned as an attribute (see documentation).

Peering setup support is still underway, will be in another PR.

Tests:

$ make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMExpressRouteCircuit_importBasic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/05 23:11:33 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMExpressRouteCircuit_importBasic -timeout 120m
=== RUN   TestAccAzureRMExpressRouteCircuit_importBasic
--- PASS: TestAccAzureRMExpressRouteCircuit_importBasic (97.99s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/azurerm	98.006s

$ make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMExpressRouteCircuit_basic'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/05 23:14:40 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMExpressRouteCircuit_basic -timeout 120m
=== RUN   TestAccAzureRMExpressRouteCircuit_basic
--- PASS: TestAccAzureRMExpressRouteCircuit_basic (125.20s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/azurerm	125.219s

tombuildsstuff

Hi @whiskeyjay

Thanks for this PR - I've reviewed and left some comments in-line, but it generally looks good :)

Out of interest, is the list of Service Providers Name/Locations static, or can it change from time-to-time? (I'm wondering if it's worth some validation around this)

Thanks!

tombuildsstuff · 2017-05-08T04:43:05Z

builtin/providers/azurerm/resource_arm_express_route_circuit.go

+		return err
+	}
+
+	if erc == nil {


We should be able to place this inside the error statement above - and explicitly check for a 404 here?

That (check for 404) is done in the retrieveErcByResourceId function.

tombuildsstuff · 2017-05-08T04:46:03Z

builtin/providers/azurerm/resource_arm_express_route_circuit.go

+				Required: true,
+			},
+
+			"sku_tier": {


seeing as we need multiple properties here, would it be worth making this it's own object to make it clearer? i.e.

sku { family = "Metered" tier = "Standard" }

tombuildsstuff · 2017-05-08T04:48:04Z

builtin/providers/azurerm/resource_arm_express_route_circuit.go

+			"sku_tier": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Default:  string(network.ExpressRouteCircuitSkuTierStandard),


we tend to make these explicit where possible, could we update this to remove the default and make this required?

Made "Required".

tombuildsstuff · 2017-05-08T04:48:06Z

builtin/providers/azurerm/resource_arm_express_route_circuit.go

+			"sku_family": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Default:  string(network.MeteredData),


(same here)

Made "Required".

tombuildsstuff · 2017-05-08T04:51:35Z

builtin/providers/azurerm/express_route_circuit.go

+	"github.com/hashicorp/errwrap"
+)
+
+func extractResourceGroupAndErcName(resourceId string) (resourceGroup string, name string, err error) {


Are these methods going to be used outside of the Express Route resource? Otherwise would it make more sense for them to live in the resource until they're needed elsewhere?

They will very likely be used by other types in the second part of Express Route work (peering). If not, I'll clean it up with peering support. Will that work? :)

tombuildsstuff · 2017-05-08T04:56:54Z

website/source/docs/providers/azurerm/r/express_route_circuit.html.markdown

+The following attributes are exported:
+
+* `id` - The Resource ID of the ExpressRoute circuit.
+* `service_provider_provisioning_state` - The ServiceProviderProvisioningState state of the resource. 


As far as I can tell, I believe the only two states exposed for this field are going to be NotProvisioned and Provisioned - for when the resource doesn't exist, and does exist respectively. As such, is there much to be gained from exposing this?

This is my bad - I copied the description from the REST API doc - this is actually the circuit provisioning state from the service provider, not the state of the resource representing the circuit. Only when the service provider has done their work, this attribute's value will be Provisioned and then further configuration of the ExpressRoute can be continued.

tombuildsstuff · 2017-05-08T04:59:34Z

builtin/providers/azurerm/resource_arm_express_route_circuit_test.go

+}
+
+func testAccAzureRMExpressRouteCircuit_basic(rInt int) string {
+	return strings.Replace(`


is it possible to switch this out for a fmt.Sprintf to match the other acceptance tests?

I'm trying to avoid future errors like adding/removing one or more %d in the config string but forgot to match the number of parameters supplied in Sprintf. That said, I also understand it would be nice to have a uniform coding style. Please let me know if this is indeed a style issue, I will be happy to change it to match existing ones.

@whiskeyjay so one option to avoid that issue when using a fmt.Sprintf is to use the substitute value of %[1]dto reference the first item rather than %d for this purpose (as you've seen we've started wrapping the test strings in a method, rather than formatting the string inside of each test to avoid the argument mismatch issue).

But yes, it'd be nice to make this consistent with the other acceptance tests if possible :)

@tombuildsstuff got it.

tombuildsstuff · 2017-05-08T05:00:34Z

builtin/providers/azurerm/resource_arm_express_route_circuit.go

+		return errwrap.Wrapf("Error Deleting ExpressRouteCircuit {{err}}", err)
+	}
+
+	d.SetId("")


This isn't needed in a Delete function - since the resource is removed from the state automatically :)

tombuildsstuff · 2017-05-08T05:01:23Z

builtin/providers/azurerm/resource_arm_express_route_circuit.go

+		return errwrap.Wrapf("Error Parsing Azure Resource ID {{err}}", err)
+	}
+
+	_, err = ercClient.Delete(resGroup, name, make(chan struct{}))


I think we can just return the result of this error, rather than checking the result?

whiskeyjay

Addressed a few comments and made changes accordingly. Still working on the rest.

whiskeyjay · 2017-05-09T02:15:48Z

builtin/providers/azurerm/express_route_circuit.go

+	"github.com/hashicorp/errwrap"
+)
+
+func extractResourceGroupAndErcName(resourceId string) (resourceGroup string, name string, err error) {


They will very likely be used by other types in the second part of Express Route work (peering). If not, I'll clean it up with peering support. Will that work? :)

whiskeyjay · 2017-05-09T02:16:09Z

builtin/providers/azurerm/resource_arm_express_route_circuit.go

+				Required: true,
+			},
+
+			"sku_tier": {


whiskeyjay · 2017-05-09T02:16:33Z

builtin/providers/azurerm/resource_arm_express_route_circuit.go

+			"sku_tier": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Default:  string(network.ExpressRouteCircuitSkuTierStandard),


Made "Required".

whiskeyjay · 2017-05-09T02:16:52Z

builtin/providers/azurerm/resource_arm_express_route_circuit.go

+			"sku_family": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Default:  string(network.MeteredData),


Made "Required".

whiskeyjay · 2017-05-09T06:37:06Z

website/source/docs/providers/azurerm/r/express_route_circuit.html.markdown

+The following attributes are exported:
+
+* `id` - The Resource ID of the ExpressRoute circuit.
+* `service_provider_provisioning_state` - The ServiceProviderProvisioningState state of the resource. 


This is my bad - I copied the description from the REST API doc - this is actually the circuit provisioning state from the service provider, not the state of the resource representing the circuit. Only when the service provider has done their work, this attribute's value will be Provisioned and then further configuration of the ExpressRoute can be continued.

whiskeyjay · 2017-05-12T00:18:11Z

@tombuildsstuff I made a few changes based on review comments; have one question regarding style in acc test file. Looking forward to your reply. Thanks!

tombuildsstuff

Hi @whiskeyjay

Thanks for the ongoing effort here, I've taken another look and left a couple of minor comments in-line, but this otherwise LGTM :)

Thanks!

tombuildsstuff · 2017-05-12T08:14:33Z

builtin/providers/azurerm/resource_arm_express_route_circuit.go

+	}
+}
+
+func resourceArmExpressRouteCircuitCreate(d *schema.ResourceData, meta interface{}) error {


minor: since this is both Creating and Updating, could we rename it resourceArmExpressRouteCircuitCreateOrUpdate?

tombuildsstuff · 2017-05-12T08:16:30Z

builtin/providers/azurerm/resource_arm_express_route_circuit.go

+	serviceProviderName := d.Get("service_provider_name").(string)
+	peeringLocation := d.Get("peering_location").(string)
+	bandwidthInMbps := int32(d.Get("bandwidth_in_mbps").(int))
+	sku := expandExpressRouteCircuitSku(d.Get("sku").(*schema.Set))


can we pass in d here and have the method take care of the casting, to be consistent with the other resources?

whiskeyjay

Made changes in test (Replace -> Sprintf), and changed Create to CreateOrUpdate.

whiskeyjay · 2017-05-12T21:26:18Z

builtin/providers/azurerm/resource_arm_express_route_circuit.go

+	}
+}
+
+func resourceArmExpressRouteCircuitCreate(d *schema.ResourceData, meta interface{}) error {


whiskeyjay · 2017-05-12T21:33:18Z

builtin/providers/azurerm/resource_arm_express_route_circuit.go

+	serviceProviderName := d.Get("service_provider_name").(string)
+	peeringLocation := d.Get("peering_location").(string)
+	bandwidthInMbps := int32(d.Get("bandwidth_in_mbps").(int))
+	sku := expandExpressRouteCircuitSku(d.Get("sku").(*schema.Set))


whiskeyjay · 2017-05-12T22:24:23Z

builtin/providers/azurerm/resource_arm_express_route_circuit_test.go

+}
+
+func testAccAzureRMExpressRouteCircuit_basic(rInt int) string {
+	return strings.Replace(`


@tombuildsstuff got it.

tombuildsstuff · 2017-05-14T18:28:50Z

Hi @whiskeyjay

Thanks for pushing the latest changes to this PR.

I've taken another look, pushed a minor tweak for consistency and run the tests and this LGTM :)

$ envchain azurerm make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMExpressRoute'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/05/14 13:26:45 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMExpressRoute -timeout 120m
=== RUN   TestAccAzureRMExpressRouteCircuit_importBasic
--- PASS: TestAccAzureRMExpressRouteCircuit_importBasic (124.33s)
=== RUN   TestAccAzureRMExpressRouteCircuit_basic
--- PASS: TestAccAzureRMExpressRouteCircuit_basic (143.59s)
PASS
ok  	github.com/hashicorp/terraform/builtin/providers/azurerm	267.938s

Thanks!

ghost · 2020-04-12T02:23:51Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

whiskeyjay added 7 commits April 25, 2017 18:37

Adds ExpressRoute circuit documentation

e0e0d42

Adds tests and doc improvements

81e1bef

Code for basic Express Route Circuit support

3edaa1f

Use the built-in validation helper

29a6b3c

Added ignoreCaseDiffSuppressFunc to a few fields

9d75711

Added more information to docs

722305d

Touchup

3123404

grubernaut added new-resource provider/azurerm labels May 7, 2017

tombuildsstuff reviewed May 8, 2017

View reviewed changes

whiskeyjay added 2 commits May 8, 2017 19:19

Moving SKU properties into a set.

e523756

Updates doc

6542073

whiskeyjay commented May 9, 2017

View reviewed changes

A bit more tweaks

0acf467

tombuildsstuff approved these changes May 12, 2017

View reviewed changes

tombuildsstuff added the waiting-response An issue/pull request is waiting for a response from the community label May 12, 2017

Switch to Sprintf for test string

2d74839

whiskeyjay commented May 12, 2017

View reviewed changes

tombuildsstuff removed the waiting-response An issue/pull request is waiting for a response from the community label May 14, 2017

Updating the acceptance test name for consistency

c0588aa

tombuildsstuff merged commit 6ca50dd into hashicorp:master May 14, 2017

tombuildsstuff added a commit that referenced this pull request May 14, 2017

Updating to include #14265

b17cfde

ghost locked and limited conversation to collaborators Apr 12, 2020

[MS] provider/azurerm: New resource - Express Route Circuit #14265

[MS] provider/azurerm: New resource - Express Route Circuit #14265

Conversation

whiskeyjay commented May 6, 2017

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

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

whiskeyjay 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

whiskeyjay commented May 12, 2017

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

whiskeyjay 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

tombuildsstuff commented May 14, 2017 • edited Loading

ghost commented Apr 12, 2020

tombuildsstuff commented May 14, 2017 •

edited

Loading