Add support for Azure LB Standard and Public IP Standard #665

justaugustus · 2018-01-05T08:00:01Z

justaugustus · 2018-01-05T08:02:53Z

@tombuildsstuff would you mind taking a peep at this?

tombuildsstuff

Thanks for this PR - I've taken a look through and left some comments in-line but this is off to a good start. From what I can see there's three things missing to be able to merge this:

We need to set the field sku in the Read function like here https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/resource_arm_loadbalancer.go#L181 - we can handle existing LB's without a SKU set via:

if sku := loadBalancer.Sku; sku != nil {
  d.Set("sku", string(sku.Name))
}

We need to add an acceptance test for a Standard SKU Load Balancer - here's how we test the Basic SKU - we should only need to set sku = "standard" since the other scenarios / combinations are tested via the Basic SKU (which is fine for now)
Document the new sku field to mention there's two possible values; we'd tend to make this something similar to:

* `sku` - (Optional) The Name of the SKU to use for this Load Balancer. Accepted values are `Basic` and `Standard`. Defaults to `Basic`

That said - this is off to a good start and I've left some other comments in-line as appropriate :)

Thanks!

tombuildsstuff · 2018-01-05T10:03:53Z

azurerm/resource_arm_loadbalancer.go

@@ -114,6 +119,7 @@ func resourceArmLoadBalancerCreate(d *schema.ResourceData, meta interface{}) err
 	name := d.Get("name").(string)
 	location := d.Get("location").(string)
 	resGroup := d.Get("resource_group_name").(string)
+	sku := convertSkuStringToLoadBalancerSku(d.Get("sku").(string))


rather than parsing this out I think we can make this network.LoadBalancerSkuName(d.Get("sku").(string))?

tombuildsstuff · 2018-01-05T10:07:43Z

azurerm/resource_arm_loadbalancer.go

@@ -100,6 +100,11 @@ func resourceArmLoadBalancer() *schema.Resource {
 				},
 			},

+			"sku": {
+				Type:     schema.TypeString,
+				Required: true,


from what I can tell existing Load Balancers are of the Basic SKU; so we can instead default this to Basic and make this Optional; like so:

Default: string(network.LoadBalancerSkuNameBasic), Optional: true,

^ this will mean that existing users don't need to specify the SKU (or update the configuration) for any existing Load Balancers. We can also extend this to add Validation and allow mixed-case strings (e.g. Basic and basic) via:

ValidateFunc: validation.StringInSlice([]string{ string(network.LoadBalancerSkuNameBasic), string(network.LoadBalancerSkuNameStandard), }, true), DiffSupressFunc: ignoreCaseDiffSuppressFunc,

justaugustus · 2018-01-05T13:22:08Z

@tombuildsstuff Thanks so much for the quick CR!

I've addressed your notes and have a few follow-up questions...

The tests for basic and standard have the same resource names for azurerm_lb (test). Will that cause issues during testing?
To support public LB Standard resources, we'll also need to make a similar addition of the sku parameter to the azurerm_public_ip resource. Should I include that change with this PR or send another one?

tombuildsstuff · 2018-01-05T17:26:08Z

@justaugustus

I've addressed your notes and have a few follow-up questions...

The tests for basic and standard have the same resource names for azurerm_lb (test). Will that cause issues during testing?

Each of the acceptance tests suffixes the resource names (in this case the LB Name) with a random integer/string to make the name unique across tests - so there shouldn't be any issues here.

To support public LB Standard resources, we'll also need to make a similar addition of the sku parameter to the azurerm_public_ip resource. Should I include that change with this PR or send another one?

Ideally it should be split out into another PR (which we can merge before this one) - but given they're interdependent I don't see any harm combining them into one if that's easier :)

Thanks!

justaugustus · 2018-01-05T19:03:35Z

I'll send another PR for the Public IP Standard SKU stuff.
Thanks again for the feedback, @tombuildsstuff!

JunyiYi · 2018-01-10T02:16:11Z

@justaugustus , thanks for your contribution.
@tombuildsstuff , CoreOS is trying to understand when this feature will be supported in Terraform. Maybe you can give the date after this pull request is merged.

Thanks.

justaugustus · 2018-01-11T23:12:09Z

Hey @tombuildsstuff!
Changed my mind and decided to bundle the two SKUs in this PR (seemed simpler that way).

I added acceptance tests, but I was thinking through a few additional cases and a little unsure about how to approach them:

LB

Test SKU:

basic -> PASS
standard -> FAIL
OTHER -> FAIL

Test frontend IP config:

private -> PASS
public:
- PIP SKU:
  - basic:
    - LB SKU:
      - basic -> PASS
      - standard -> FAIL
  - standard:
    - LB SKU:
      - basic -> FAIL
      - standard -> PASS

PIP

Test SKU:

basic -> PASS
standard:
- allocation:
  - static -> PASS
  - dynamic -> FAIL
  - OTHER -> FAIL

Could you take another sweep through this?

justaugustus · 2018-01-16T16:18:07Z

@tombuildsstuff did you have a chance to take another look at this?

paultyng

@justaugustus I found a couple cleanup things if you don't mind addressing those and then @tombuildsstuff can take another pass at this.

paultyng · 2018-01-18T22:23:03Z

azurerm/resource_arm_loadbalancer_test.go

+resource "azurerm_lb" "test" {
+    name = "arm-test-loadbalancer-%d"
+    location = "${azurerm_resource_group.test.location}"
+		resource_group_name = "${azurerm_resource_group.test.name}"


got some mixed tabs and spaces here

@paultyng I'm noticing this across multiple files in the provider... are tabs or spaces preferred?

paultyng · 2018-01-18T22:25:20Z

azurerm/resource_arm_public_ip.go

+				Optional:         true,
+				Default:          string(network.PublicIPAddressSkuNameBasic),
+				ForceNew:         true,
+				ValidateFunc:     validatePublicIpSku,


This could just be StringInSlice couldn't it?

@paultyng I was mimicking the other validation functions in that file and figured it could be reused in the acceptance tests, similar to:

https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/resource_arm_public_ip_test.go#L41

https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/resource_arm_public_ip_test.go#L73

Is there a preferred approach?

It depends what's being tested - but in general we're using the Constants/Enums in the SDK via the StringInSlice (at least for Strings) for newer resources; given these constants exist - could we switch this for a StringInSlice?

paultyng · 2018-01-18T22:26:14Z

azurerm/resource_arm_public_ip_test.go

+    name = "acctestpublicip-%d"
+    location = "${azurerm_resource_group.test.location}"
+    resource_group_name = "${azurerm_resource_group.test.name}"
+		public_ip_address_allocation = "static"


mixed tabs and spaces

@paultyng I'm noticing this across multiple files in the provider... are tabs or spaces preferred?

paultyng · 2018-01-18T22:26:58Z

website/docs/r/public_ip.html.markdown

-* `public_ip_address_allocation` - (Required) Defines whether the IP address is stable or dynamic. Options are Static or Dynamic.
+* `sku` - (Optional) The SKU of the Public IP. Accepted values are `Basic` and `Standard`. Defaults to `Basic`.
+
+~> **Note** Public IP Standard SKUs require `public_ip_address_allocation` to be set to `static`.


Should this be in a validation?

@paultyng Agreed that that should be in a validation as well!

I dropped a note (#665 (comment)) hoping for a nudge in the right direction re: the structuring the tests correctly and figured adding something in the docs was a good start while waiting for a response.

@justaugustus apologies for the delayed response here.

At the current time it's only possible to access the value of that specific field in the validation method on the Schema - as such I believe we'd need to do this in the resourceArmPublicIpCreate method?

I've added a commit to explain that this feature is still in Preview and needs to be opted-in

Ref: hashicorp#579

* Set as optional parameter * Set default `sku` value * Set `ForceNew: true` (SKUs are immutable) * Add validation for accepted SKU values ("Basic", "Standard") * Remove SKU conversion func

justaugustus · 2018-01-25T06:53:25Z

@tombuildsstuff / @paultyng --
thanks for the feedback! I've addressed both of your reviews, so this is ripe for another sweep when you have a chance. :)

tombuildsstuff · 2018-01-25T13:27:29Z

@justaugustus hey - just to let you know that now the SDK upgrade work is completed that I'm hoping to take a look into this later today (apologies for the delay here - we've been trying to minimise merge conflicts during the SDK migration) :)

tombuildsstuff

hey @justaugustus

Thanks for pushing those updates - apologies for the delayed re-review here.

I've taken a look through and have left a comment in-line which I'll push a commit to resolve - but this otherwise LGTM. I'll kick off the test suite now

Thanks!

tombuildsstuff · 2018-01-26T12:08:10Z

azurerm/resource_arm_public_ip.go

+	}
+
+	if resp.PublicIPAddressPropertiesFormat.DNSSettings != nil && resp.PublicIPAddressPropertiesFormat.DNSSettings.Fqdn != nil && *resp.PublicIPAddressPropertiesFormat.DNSSettings.Fqdn != "" {
+		d.Set("fqdn", resp.PublicIPAddressPropertiesFormat.DNSSettings.Fqdn)


we can remove this line, since it's set below: https://github.com/terraform-providers/terraform-provider-azurerm/pull/665/files#diff-ed59d26a8bebf9bf3b188b200b02dd27L193

changes have been pushed since

tombuildsstuff

Given this functionality is in Preview - I've added a couple of comments about documenting this (I'll push a commit to fix this shortly).

tombuildsstuff · 2018-01-26T15:50:04Z

website/docs/r/public_ip.html.markdown

-* `public_ip_address_allocation` - (Required) Defines whether the IP address is stable or dynamic. Options are Static or Dynamic.
+* `sku` - (Optional) The SKU of the Public IP. Accepted values are `Basic` and `Standard`. Defaults to `Basic`.
+
+~> **Note** Public IP Standard SKUs require `public_ip_address_allocation` to be set to `static`.


I've added a commit to explain that this feature is still in Preview and needs to be opted-in

tombuildsstuff · 2018-01-26T15:50:11Z

website/docs/r/loadbalancer.html.markdown

@@ -45,6 +45,7 @@ The following arguments are supported:
 * `resource_group_name` - (Required) The name of the resource group in which to create the LoadBalancer.
 * `location` - (Required) Specifies the supported Azure location where the resource exists.
 * `frontend_ip_configuration` - (Optional) A frontend ip configuration block as documented below.
+* `sku` - (Optional) The SKU of the Azure Load Balancer. Accepted values are `Basic` and `Standard`. Defaults to `Basic`.


I've added a commit to explain that this feature is still in Preview and needs to be opted-in

tombuildsstuff · 2018-01-26T16:03:33Z

Tests pass for both LB's and Public IP's after registering for the Preview:

tombuildsstuff

LGTM 👍

justaugustus · 2018-01-27T22:15:17Z

🎉

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

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!

tombuildsstuff added new-resource service/load-balancers enhancement and removed new-resource labels Jan 5, 2018

tombuildsstuff added this to the 1.0.1 milestone Jan 5, 2018

tombuildsstuff suggested changes Jan 5, 2018

View reviewed changes

tombuildsstuff added the waiting-response label Jan 5, 2018

justaugustus changed the title ~~[WIP] Add support for Azure LB Standard~~ Add support for Azure LB Standard Jan 5, 2018

justaugustus force-pushed the add-lb-standard branch from 93ee2fb to 209b76f Compare January 10, 2018 23:49

justaugustus changed the title ~~Add support for Azure LB Standard~~ Add support for Azure LB Standard and Public IP Standard Jan 11, 2018

tombuildsstuff removed the waiting-response label Jan 11, 2018

tombuildsstuff modified the milestones: 1.0.1, 1.0.2 Jan 11, 2018

paultyng previously requested changes Jan 18, 2018

View reviewed changes

justaugustus added 9 commits January 24, 2018 21:35

lb: Add sku parameter to support LB Standard

4e97aa2

Ref: hashicorp#579

lb: Fix sku parameter definition

7208d8f

* Set as optional parameter * Set default `sku` value * Set `ForceNew: true` (SKUs are immutable) * Add validation for accepted SKU values ("Basic", "Standard") * Remove SKU conversion func

lb: Add sku to resourecArmLoadBalancerRead func

a817dd5

docs: Update LB docs to include sku parameter definition

2880895

tests/lb: Add acceptance test for LB Standard SKU

be935e8

pip: Add sku parameter to support Public IP Standard

5a2452e

pip: Add validation function for PIP SKU

192b638

pip: Update Read func to read or set SKU

5001cfe

tests/pip: Add acceptance test for PIP Standard SKU

bbabae9

justaugustus added 5 commits January 24, 2018 21:51

docs: Update Public IP docs to include sku parameter definition

aa86a39

pip: Remove TODOs

8816c60

azure: tabs --> spaces in validation templates

6c92a01

pip: Use StringInSlice for PIP SKU validation

e26e310

pip: Add validation for PIP allocation method with SKU

69581d8

justaugustus force-pushed the add-lb-standard branch from 8f3236c to 69581d8 Compare January 25, 2018 06:48

tombuildsstuff previously approved these changes Jan 26, 2018

View reviewed changes

tombuildsstuff added 2 commits January 26, 2018 12:44

Removing a duplicate assignment

cd20751

Adding a disclaimer that the Standard SKU's are in Preview

d356e56

tombuildsstuff reviewed Jan 26, 2018

View reviewed changes

tombuildsstuff dismissed their stale review via d356e56 January 26, 2018 15:52

Switching to the standard acctest prefix

b82e64f

tombuildsstuff approved these changes Jan 26, 2018

View reviewed changes

tombuildsstuff merged commit b863434 into hashicorp:master Jan 26, 2018

tombuildsstuff added a commit that referenced this pull request Jan 26, 2018

Updating to include #665

3a921db

justaugustus mentioned this pull request Jan 27, 2018

Feature Request: Add support for new Preview features (LB Standard, Public IP Standard, Availability Zones, High Availability Ports) #579

Closed

4 tasks

ghost locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Azure LB Standard and Public IP Standard #665

Add support for Azure LB Standard and Public IP Standard #665

justaugustus commented Jan 5, 2018

justaugustus commented Jan 5, 2018

tombuildsstuff left a comment

tombuildsstuff Jan 5, 2018

tombuildsstuff Jan 5, 2018

justaugustus commented Jan 5, 2018

tombuildsstuff commented Jan 5, 2018

justaugustus commented Jan 5, 2018

JunyiYi commented Jan 10, 2018

justaugustus commented Jan 11, 2018

justaugustus commented Jan 16, 2018

paultyng left a comment

paultyng Jan 18, 2018

justaugustus Jan 22, 2018

paultyng Jan 18, 2018

justaugustus Jan 22, 2018

tombuildsstuff Jan 22, 2018

paultyng Jan 18, 2018

justaugustus Jan 22, 2018

paultyng Jan 18, 2018

justaugustus Jan 22, 2018

tombuildsstuff Jan 22, 2018 •

edited

Loading

tombuildsstuff Jan 26, 2018

justaugustus commented Jan 25, 2018

tombuildsstuff commented Jan 25, 2018

tombuildsstuff left a comment

tombuildsstuff Jan 26, 2018

tombuildsstuff left a comment

tombuildsstuff Jan 26, 2018

tombuildsstuff Jan 26, 2018

tombuildsstuff commented Jan 26, 2018

tombuildsstuff left a comment

justaugustus commented Jan 27, 2018

ghost commented Mar 31, 2020

Add support for Azure LB Standard and Public IP Standard #665

Add support for Azure LB Standard and Public IP Standard #665

Conversation

justaugustus commented Jan 5, 2018

justaugustus commented Jan 5, 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

justaugustus commented Jan 5, 2018

tombuildsstuff commented Jan 5, 2018

justaugustus commented Jan 5, 2018

JunyiYi commented Jan 10, 2018

justaugustus commented Jan 11, 2018

LB

Test SKU:

Test frontend IP config:

PIP

Test SKU:

justaugustus commented Jan 16, 2018

paultyng 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

tombuildsstuff Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justaugustus commented Jan 25, 2018

tombuildsstuff commented Jan 25, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

tombuildsstuff commented Jan 26, 2018

tombuildsstuff left a comment

Choose a reason for hiding this comment

justaugustus commented Jan 27, 2018

ghost commented Mar 31, 2020

tombuildsstuff Jan 22, 2018 •

edited

Loading