New Application Gateway Resource / New Terraform Provider structure [MS] #120

isaacsgi · 2017-06-21T21:39:11Z

Incorporated changes from https://github.com/brandontosch/terraform/tree/brandontosch/GH-8670
Also made slight change of 'app' gateway to 'application' gateway throughout code

Incorporated changes from https://github.com/brandontosch/terraform/tree/brandontosch/GH-8670 Also made slight change of 'app' gateway to 'application' gateway throughout code

…tAccAzureRMApplicationGateway_basic and accept int as parameter

…be run alone

…er-azurerm

tombuildsstuff

Hey @isaacsgi

Apologies for the delay in reviewing this!

I've reviewed and left some comments in-line, but this is generally looking good. Besides the comments, would it be possible to add some Documentation (and a link in the sidebar) for this resource? In order for this to resource to be usable, it also needs to be added to the provider.go file

One observation - I was initially hoping that we could break this out into smaller sub-resources rather than referencing ID's all over the place (see: how Terraform implemented Load Balancers) - however it doesn't look like this is feasible based on the Required fields. It might be just me, but it feels like this is internal API logic which is being exposed in the SDK rather than within the API; as such do you think it's worth reaching out to the API team to see if splitting this out is on the roadmap? (e.g. create an Application Gateway, then add a SSL cert as a separate API call)

Thanks!

tombuildsstuff · 2017-07-14T00:33:38Z

azurerm/resource_arm_application_gateway.go

+			"backend_address_pool": {
+				Type:     schema.TypeList,
+				Required: true,
+				MinItems: 1,


minor this MinItems can be removed as it's required :)

tombuildsstuff · 2017-07-14T00:34:00Z

azurerm/resource_arm_application_gateway.go

+			"frontend_port": {
+				Type:     schema.TypeList,
+				Required: true,
+				MinItems: 1,


minor MinItems: 1 can be removed since it's required

tombuildsstuff · 2017-07-14T00:35:31Z

azurerm/resource_arm_application_gateway.go

+			"gateway_ip_configuration": {
+				Type:     schema.TypeList,
+				Required: true,
+				MinItems: 1,


minor MinItems: 1 can be removed (it's inferred) because Required is true :)

tombuildsstuff · 2017-07-14T00:41:50Z

azurerm/resource_arm_application_gateway.go

+	d.SetId(*read.ID)
+
+	log.Printf("[DEBUG] Waiting for ApplicationGateway (%s) to become available", name)
+	stateConf := &resource.StateChangeConf{


do we need to poll for this, or does the Azure SDK for Go handle this automatically?

It looks like the Terraform helper + Go SDK does this - so removed the extra code.

tombuildsstuff · 2017-07-14T00:42:08Z

azurerm/resource_arm_application_gateway.go

+		Timeout: 60 * time.Minute,
+	}
+	if _, err := stateConf.WaitForState(); err != nil {
+		return fmt.Errorf("Error waiting for ApplicationGateway (%s) to become available: %s", name, err)


the second one here needs to be %+v to display the error and not just a string

tombuildsstuff · 2017-07-18T12:19:07Z

azurerm/resource_arm_application_gateway_test.go

+}
+
+resource "azurerm_virtual_network" "test" {
+  name                = "vnet"


can we give this a unique name? (we run these tests in parallel, which can cause issues)

tombuildsstuff · 2017-07-18T12:19:15Z

azurerm/resource_arm_application_gateway_test.go

+}
+
+resource "azurerm_public_ip" "test" {
+  name                         = "public-ip"


can we give this a unique name?

tombuildsstuff · 2017-07-18T12:37:52Z

azurerm/resource_arm_application_gateway_test.go

+  ]
+
+  gateway_ip_configuration {
+    # id = computed


I think we can remove these comments?

tombuildsstuff · 2017-07-18T12:38:37Z

azurerm/resource_arm_application_gateway_test.go

+    name                = "probe-1"
+    protocol            = "Https"
+    path                = "/test"
+    host                = "azure.com"


out of interest does this need to match this list?

sorry - this link doesn't seem to take me to a list?

sorry, wrong link. I was referring to the fqdn_list on the backend_address_pool

tombuildsstuff · 2017-07-18T12:42:00Z

azurerm/resource_arm_application_gateway_test.go

+}
+
+resource "azurerm_virtual_network" "test" {
+  name                = "vnet"


(same as above) can we make this a unique network name? (we tend to prefix them with acctest - which will become more important in the near future)

isaacsgi · 2017-07-20T21:39:11Z

@tombuildsstuff thanks for the feedback! ... I was pulled into another initiative for a couple days - reviewing now ..

tombuildsstuff

Hey @isaacsgi

Thanks for pushing those updates - sorry for the delayed reply here. I've taken a look and left some more comments in-line but this is looking good.

Regarding the test certificate - as discussed in DM we can use static data for this for the moment, it'd be good to switch it over to a dynamically generated one in the future once the tls provider support this.

In addition regarding the comments around Set/List - that needs input from the developers of Terraform Core, so I've flagged them for input

Thanks!

tombuildsstuff · 2017-08-03T15:14:16Z

azurerm/resource_arm_application_gateway.go

+	"log"
+	"net/http"
+	"path"
+	//	"time"


minor can we remove this unused import?

tombuildsstuff · 2017-08-03T15:32:36Z

azurerm/resource_arm_application_gateway.go

+						},
+					},
+				},
+				Set: hashApplicationGatewaySku,


Hey @isaacsgi I have - however I feel this is better answered by of the Terraform Core developers (@apparentlymart / @jbardin) who are in a better position to answer this (I see Martin's already replied)

tombuildsstuff · 2017-08-03T15:32:53Z

azurerm/resource_arm_application_gateway.go

+							Type:             schema.TypeString,
+							Required:         true,
+							DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
+							ValidateFunc:     validation.StringInSlice([]string{"OWASP"}, true),


are there constants available in the SDK for this value?

strangely - not for this constant. It's a good action for the SDK folks that I'll chase down - thanks.

(fwiw this isn't a blocker by any means - so we can merge with this string for now and switch it out once it's added 😄)

tombuildsstuff · 2017-08-03T15:33:01Z

azurerm/resource_arm_application_gateway.go

+							DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
+							ValidateFunc:     validation.StringInSlice([]string{"2.2.9", "3.0"}, true),
+						},
+					},


are there constants available in the SDK for these values?

same as previous - doesn't seem to be, but I'll chase this down as well.

+1 here - this isn't a blocker by any means; so we can run with this string for now 😄

tombuildsstuff · 2017-08-03T15:36:15Z

azurerm/resource_arm_application_gateway.go

+
+	resp, err := client.Get(resGroup, name)
+	if err != nil {
+		if resp.StatusCode == http.StatusNotFound {


there's a new helper method for this which checks for nil values, can we switch this to using:

if responseWasNotFound(resp.Response) { ... }

tombuildsstuff · 2017-08-03T15:41:35Z

azurerm/resource_arm_application_gateway.go

+		}
+
+		if config.ApplicationGatewayURLPathMapPropertiesFormat.DefaultBackendAddressPool != nil {
+			pathMap["default_backend_address_pool_name"] = path.Base(*config.ApplicationGatewayURLPathMapPropertiesFormat.DefaultBackendAddressPool.ID)


we're assuming here the name will be at the end - can we switch this to using the parseAzureResourceID function to parse the values out of the URL to guarantee that this is the case?

tombuildsstuff · 2017-08-03T15:41:37Z

azurerm/resource_arm_application_gateway.go

+		}
+
+		if config.ApplicationGatewayURLPathMapPropertiesFormat.DefaultBackendHTTPSettings != nil {
+			pathMap["default_backend_http_settings_name"] = path.Base(*config.ApplicationGatewayURLPathMapPropertiesFormat.DefaultBackendHTTPSettings.ID)


we're assuming here the name will be at the end - can we switch this to using the parseAzureResourceID function to parse the values out of the URL to guarantee that this is the case?

tombuildsstuff · 2017-08-03T15:41:40Z

azurerm/resource_arm_application_gateway.go

+			}
+
+			if pathRuleConfig.ApplicationGatewayPathRulePropertiesFormat.BackendAddressPool != nil {
+				rule["backend_address_pool_name"] = path.Base(*pathRuleConfig.ApplicationGatewayPathRulePropertiesFormat.BackendAddressPool.ID)


we're assuming here the name will be at the end - can we switch this to using the parseAzureResourceID function to parse the values out of the URL to guarantee that this is the case?

tombuildsstuff · 2017-08-03T15:41:43Z

azurerm/resource_arm_application_gateway.go

+			}
+
+			if pathRuleConfig.ApplicationGatewayPathRulePropertiesFormat.BackendHTTPSettings != nil {
+				rule["backend_http_settings_name"] = path.Base(*pathRuleConfig.ApplicationGatewayPathRulePropertiesFormat.BackendHTTPSettings.ID)


we're assuming here the name will be at the end - can we switch this to using the parseAzureResourceID function to parse the values out of the URL to guarantee that this is the case?

tombuildsstuff · 2017-08-03T15:42:18Z

azurerm/resource_arm_application_gateway.go

+		certConfig := map[string]interface{}{
+			"id":               *config.ID,
+			"name":             *config.Name,
+			"public_cert_data": *config.ApplicationGatewaySslCertificatePropertiesFormat.PublicCertData,


do we need to add a check for if config.ApplicationGatewaySslCertificatePropertiesFormat != nil here?

@tombuildsstuff maybe I'm missing something, but I don't see how we would be in the for loop if there was nil data? Is the idea to error check the Azure API data population?

Hey @isaacsgi,
Looks to me like the for loop iterates over the *certs object, but doesn't hold a firm contract that every field in that struct will be populated.
Basically, if we add a simple conditional check if the ApplicationGatewaySslCertificatePropertiesFormat is not nil, we can potentially catch any nil pointer dereference panics that may occur if, for whatever reason, that field inside the struct we're iterating over isn't populated.

…form-provider-azurerm

…and id.Path

bingosummer · 2017-09-21T03:49:18Z

@isaacsgi @tombuildsstuff Is the PR ready? I'm also interested about the application gateway resource.

Fix referenced names in application gateway doc

tombuildsstuff · 2017-09-27T15:04:36Z

Closing this in favour of #357 (which has been rebased)

ghost · 2020-04-01T15:18:43Z

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!

For new Application Gateway Resource:

4accc25

Incorporated changes from https://github.com/brandontosch/terraform/tree/brandontosch/GH-8670 Also made slight change of 'app' gateway to 'application' gateway throughout code

This was referenced Jun 21, 2017

[WIP] provider/azurerm: create app_gateway resource hashicorp/terraform#14426

Closed

hashicorp#14426 I can create an application gateway azdevops/terraform#14

Open

Ubuntu added 2 commits June 22, 2017 19:12

Changed capital 'T' to lowercase in all functions that start with tes…

d0ae173

…tAccAzureRMApplicationGateway_basic and accept int as parameter

Missed one upper case

2a9bd94

tombuildsstuff mentioned this pull request Jun 27, 2017

Feature Request: Azure App Gateway #18

Closed

Gil ISAACS added 3 commits July 6, 2017 20:18

Added RuleSetType and RuleSetVersion for WAF

343cb50

validated application gateway tester - changed base method so it can …

ba0c53b

…be run alone

made sure files are added

f0c1c67

isaacsgi force-pushed the master branch from ba0c53b to f0c1c67 Compare July 17, 2017 18:13

Merge branch 'master' of https://github.com/isaacsgi/terraform-provid…

662e7fd

…er-azurerm

tombuildsstuff suggested changes Jul 18, 2017

View reviewed changes

tombuildsstuff added enhancement new-resource waiting-response labels Jul 18, 2017

Gil ISAACS added 5 commits July 31, 2017 22:12

changes based on reviewer feedback

02215cd

Merge branch 'master' into master

7d2e0c6

go fmt after resolving merge conflict using git online editor

a5a6e1b

removed unused gwID from application gateway tester

26ce9f9

Removed subscriptionID from application gateway tester

47ec63f

tombuildsstuff reviewed Aug 3, 2017

View reviewed changes

Gil ISAACS added 7 commits August 16, 2017 12:16

Merge branch 'master' of https://github.com/terraform-providers/terra…

db6b82a

…form-provider-azurerm

incorporated feedback from review - also rebased to master

bcf5282

switched flatten func's from using path.Base to parseAzureResourceID …

8569855

…and id.Path

Starting to make documentation changes for Application Gateway

052489e

testing markdown changes

7fb4e94

further documentation changes for application gateway

aed2ee6

minor doc change - todo's outstanding

bebdf4b

tombuildsstuff removed the waiting-response label Aug 24, 2017

niroyb and others added 4 commits September 22, 2017 13:32

Fix referenced names in application gateway doc

c58a667

Merge pull request #1 from niroyb/patch-1

65fd259

Fix referenced names in application gateway doc

finalizing documentation

e611aa5

Merge remote-tracking branch 'origin/master'

768ea13

tombuildsstuff closed this Sep 27, 2017

ghost locked and limited conversation to collaborators Apr 1, 2020

New Application Gateway Resource / New Terraform Provider structure [MS] #120

New Application Gateway Resource / New Terraform Provider structure [MS] #120

Conversation

isaacsgi commented Jun 21, 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

tombuildsstuff Jul 18, 2017 • edited Loading

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

isaacsgi commented Jul 20, 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

bingosummer commented Sep 21, 2017

tombuildsstuff commented Sep 27, 2017

ghost commented Apr 1, 2020

tombuildsstuff Jul 18, 2017 •

edited

Loading