add support for pick-hostname behaviours #2658

mdsills · 2019-01-13T23:19:05Z

Hi. Am unsure re policy/naming issues. Figured i'd submit PR and declare up-front that i'm happy to follow any helpful feedback and/or let someone else take it over the line. cheers.

…backend-address

katbyte

Hi @mdsills,

Thank you for these new properties. I've left some comments inline and noticed that the docs are showing a change for almost every line on github diff? Could we fix that so only the changes made show up ?

katbyte · 2019-01-14T21:26:18Z

azurerm/resource_arm_application_gateway.go

@@ -499,7 +504,7 @@ func resourceArmApplicationGateway() *schema.Resource {

 						"host": {
 							Type:     schema.TypeString,
-							Required: true,
+							Optional: true,


Now that this is optional we should probably add validation somewhere to make sure if this is empty, pick_host_name_from_backend_address is true.

We should also update the documentation with this change

@katbyte might you have any suggestions re "add validation somewhere"? - it seems like current validation mechanisms are per-property.

@mdsills, in the create function you can check both properties and if neither are set return an error.

Something like:

host := v["pick_host_name_from_backend_address"].(string) pick := v["pick_host_name_from_backend_address"].(bool) if !pick && host == "" { return fmt.Errorf("one of `host` or `pick_host_name_from_backend_http_settings` must be set") } if pick && host != "" { return fmt.Errorf("Only one of `host` or `pick_host_name_from_backend_http_settings` must be set") }

disclaimer that code might not compile 😅

thanks for the pointer. it's ended up looking rather different, but hopefully still acceptable, or at least a step towards acceptable...

azurerm/resource_arm_application_gateway.go

azurerm/resource_arm_application_gateway_test.go

metacpp · 2019-01-14T23:42:42Z

website/docs/r/application_gateway.html.markdown

-## Example Usage
-
-```hcl
+---


Did you reformat the whole markdown file? Looks like the commit introduces many unnecessary lines of changes.

@metacpp @katbyte seems the original file had a mix of windows and unix lineends. my editor defaults to unix lineends, hence the big diff. i'll sort it back to the actual changes, which i agree is a lot more useful.

katbyte · 2019-01-17T19:12:25Z

azurerm/resource_arm_application_gateway.go

@@ -499,7 +504,7 @@ func resourceArmApplicationGateway() *schema.Resource {

 						"host": {
 							Type:     schema.TypeString,
-							Required: true,
+							Optional: true,


@mdsills, in the create function you can check both properties and if neither are set return an error.

Something like:

host := v["pick_host_name_from_backend_address"].(string) pick := v["pick_host_name_from_backend_address"].(bool) if !pick && host == "" { return fmt.Errorf("one of `host` or `pick_host_name_from_backend_http_settings` must be set") } if pick && host != "" { return fmt.Errorf("Only one of `host` or `pick_host_name_from_backend_http_settings` must be set") }

disclaimer that code might not compile 😅

JunyiYi

Thanks @mdsills for the contributions. I have left several comments here and I think we are good to go as soon as they are resolved.

JunyiYi · 2019-01-17T19:13:58Z

website/docs/r/application_gateway.html.markdown

@@ -191,6 +191,8 @@ A `backend_http_settings` block supports the following:

 * `request_timeout` - (Required) The request timeout in seconds, which must be between 1 and 86400 seconds.

+* `pick_host_name_from_backend_address` - (Optional) Whether host header should be picked from the host name of the backend server.


Could we show the default value to optional attributes?

Suggested change

* `pick_host_name_from_backend_address` - (Optional) Whether host header should be picked from the host name of the backend server.

* `pick_host_name_from_backend_address` - (Optional) Whether host header should be picked from the host name of the backend server. Defaults to `false`.

JunyiYi · 2019-01-17T19:15:27Z

website/docs/r/application_gateway.html.markdown

@@ -281,6 +283,8 @@ A `probe` block support the following:

 * `unhealthy_threshold` - (Required) The Unhealthy Threshold for this Probe, which indicates the amount of retries which should be attempted before a node is deemed unhealthy. Possible values are from 1 - 20 seconds.

+* `pick_host_name_from_backend_http_settings` - (Optional) Whether the host header should be picked from the backend http settings.


Could we show the default value to optional attributes?

Suggested change

* `pick_host_name_from_backend_http_settings` - (Optional) Whether the host header should be picked from the backend http settings.

* `pick_host_name_from_backend_http_settings` - (Optional) Whether the host header should be picked from the backend http settings. Defaults to `false`.

JunyiYi · 2019-01-17T19:17:03Z

website/docs/r/application_gateway.html.markdown

@@ -267,7 +269,7 @@ A `path_rule` block supports the following:

 A `probe` block support the following:

-* `host` - (Required) The Hostname used for this Probe. If the Application Gateway is configured for a single site, by default the Host name should be specified as ‘127.0.0.1’, unless otherwise configured in custom probe.
+* `host` - (Required) The Hostname used for this Probe. If the Application Gateway is configured for a single site, by default the Host name should be specified as ‘127.0.0.1’, unless otherwise configured in custom probe. Cannot be set if `pick_host_name_from_backend_http_settings` is set to true.


Could we change the host to be optional? We also prefer true to be enquoted.

Suggested change

* `host` - (Required) The Hostname used for this Probe. If the Application Gateway is configured for a single site, by default the Host name should be specified as ‘127.0.0.1’, unless otherwise configured in custom probe. Cannot be set if `pick_host_name_from_backend_http_settings` is set to true.

* `host` - (Optional) The Hostname used for this Probe. If the Application Gateway is configured for a single site, by default the Host name should be specified as ‘127.0.0.1’, unless otherwise configured in custom probe. Cannot be set if `pick_host_name_from_backend_http_settings` is set to `true`.

done. good spot.

JunyiYi · 2019-01-17T19:18:45Z

azurerm/resource_arm_application_gateway_test.go

+				Config: testAccAzureRMApplicationGateway_settingsPickHostNameFromBackendAddress(ri, testLocation()),
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMApplicationGatewayExists(resourceName),
+					resource.TestCheckResourceAttrSet(resourceName, "backend_http_settings.0.pick_host_name_from_backend_address"),


Could we remove this line because TestCheckResourceAttr will check the existence?

Suggested change

resource.TestCheckResourceAttrSet(resourceName, "backend_http_settings.0.pick_host_name_from_backend_address"),

JunyiYi · 2019-01-17T19:19:40Z

azurerm/resource_arm_application_gateway_test.go

+				Config: testAccAzureRMApplicationGateway_probesPickHostNameFromBackendHTTPSettings(ri, testLocation()),
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMApplicationGatewayExists(resourceName),
+					resource.TestCheckResourceAttrSet(resourceName, "probe.0.pick_host_name_from_backend_http_settings"),


Could we remove this line because TestCheckResourceAttr will check the existence?

Suggested change

resource.TestCheckResourceAttrSet(resourceName, "probe.0.pick_host_name_from_backend_http_settings"),

katbyte · 2019-01-18T01:24:13Z

Thanks for the udpates @mdsills 🙂 LGTM now

bfleming-ciena · 2019-03-01T18:43:15Z

Need some help. I'm confused. Where is the support for providing the hostname for the header?

Thanks

ghost · 2019-03-05T13:50:45Z

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!

add support for pick-hostname behaviours

efbe649

ghost added size/XXL documentation labels Jan 13, 2019

katbyte added 2 commits January 14, 2019 00:05

Merge branch 'master' into issue-1875-support-for-pick-hostname-from-…

4bcebe9

…backend-address

cleanup after merge

631a4f2

katbyte force-pushed the issue-1875-support-for-pick-hostname-from-backend-address branch from b5e5e61 to 631a4f2 Compare January 14, 2019 08:33

make fmt; remove trailing whitespace

cb3902a

tombuildsstuff added enhancement service/application-gateway labels Jan 14, 2019

remove unnecessary type conversions

a9a78a8

katbyte requested changes Jan 14, 2019

View reviewed changes

katbyte added the waiting-response label Jan 14, 2019

metacpp reviewed Jan 14, 2019

View reviewed changes

Matthew and others added 3 commits January 15, 2019 05:38

add explicit defaults

2839008

check values in test

1653e40

reinstate windows lineends in documentation

fe639ae

ghost added size/L and removed size/XXL labels Jan 15, 2019

katbyte requested changes Jan 17, 2019

View reviewed changes

JunyiYi suggested changes Jan 17, 2019

View reviewed changes

Matthew added 6 commits January 17, 2019 22:43

app gateway: remove unnecessary two-stage test

42e72d1

app gateway: add defaults to documentation

28bd846

app gateway: mark host as optional

a6ad624

app gateway: cross checks on host/pick

811ecb9

merge terraform-providers master and fix conflict in app gateway docs

10c6b63

app gateway: add missed backticks on true

48eb905

katbyte approved these changes Jan 18, 2019

View reviewed changes

katbyte merged commit 2f58e4d into hashicorp:master Jan 18, 2019

katbyte added this to the 2.0.0 milestone Jan 18, 2019

katbyte added a commit that referenced this pull request Jan 18, 2019

Update CHANGELOG.md to include #2658

6e08c9f

mdsills mentioned this pull request Jan 18, 2019

Support for pickHostnameFromBackendAddress setting on Application Gateway #1875

Closed

tombuildsstuff modified the milestones: 2.0.0, 1.22.0 Jan 21, 2019

tombuildsstuff removed the waiting-response label Jan 22, 2019

ghost locked and limited conversation to collaborators Mar 5, 2019

mdsills deleted the issue-1875-support-for-pick-hostname-from-backend-address branch March 11, 2019 10:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for pick-hostname behaviours #2658

add support for pick-hostname behaviours #2658

mdsills commented Jan 13, 2019

katbyte left a comment

katbyte Jan 14, 2019

katbyte Jan 14, 2019

mdsills Jan 17, 2019

katbyte Jan 17, 2019

mdsills Jan 17, 2019

metacpp Jan 14, 2019

mdsills Jan 15, 2019

mdsills Jan 15, 2019

katbyte Jan 17, 2019

JunyiYi left a comment •

edited

Loading

JunyiYi Jan 17, 2019

mdsills Jan 17, 2019

JunyiYi Jan 17, 2019

mdsills Jan 17, 2019

JunyiYi Jan 17, 2019

mdsills Jan 17, 2019

JunyiYi Jan 17, 2019

mdsills Jan 17, 2019

JunyiYi Jan 17, 2019

mdsills Jan 17, 2019

katbyte commented Jan 18, 2019

bfleming-ciena commented Mar 1, 2019

ghost commented Mar 5, 2019

		@@ -191,6 +191,8 @@ A `backend_http_settings` block supports the following:

		* `request_timeout` - (Required) The request timeout in seconds, which must be between 1 and 86400 seconds.

		* `pick_host_name_from_backend_address` - (Optional) Whether host header should be picked from the host name of the backend server.

		@@ -281,6 +283,8 @@ A `probe` block support the following:

		* `unhealthy_threshold` - (Required) The Unhealthy Threshold for this Probe, which indicates the amount of retries which should be attempted before a node is deemed unhealthy. Possible values are from 1 - 20 seconds.

		* `pick_host_name_from_backend_http_settings` - (Optional) Whether the host header should be picked from the backend http settings.

	* `host` - (Required) The Hostname used for this Probe. If the Application Gateway is configured for a single site, by default the Host name should be specified as ‘127.0.0.1’, unless otherwise configured in custom probe. Cannot be set if `pick_host_name_from_backend_http_settings` is set to true.
	* `host` - (Optional) The Hostname used for this Probe. If the Application Gateway is configured for a single site, by default the Host name should be specified as ‘127.0.0.1’, unless otherwise configured in custom probe. Cannot be set if `pick_host_name_from_backend_http_settings` is set to `true`.

add support for pick-hostname behaviours #2658

add support for pick-hostname behaviours #2658

Conversation

mdsills commented Jan 13, 2019

katbyte 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

JunyiYi left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte commented Jan 18, 2019

bfleming-ciena commented Mar 1, 2019

ghost commented Mar 5, 2019

JunyiYi left a comment •

edited

Loading