Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for pick-hostname behaviours #2658

Conversation

mdsills
Copy link
Contributor

@mdsills mdsills commented Jan 13, 2019

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.

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

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

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

"host": {
Type: schema.TypeString,
Required: true,
Optional: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also update the documentation with this change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
azurerm/resource_arm_application_gateway.go Show resolved Hide resolved
azurerm/resource_arm_application_gateway_test.go Outdated Show resolved Hide resolved
azurerm/resource_arm_application_gateway_test.go Outdated Show resolved Hide resolved
## Example Usage

```hcl
---
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@ghost ghost added size/L and removed size/XXL labels Jan 15, 2019
@@ -499,7 +504,7 @@ func resourceArmApplicationGateway() *schema.Resource {

"host": {
Type: schema.TypeString,
Required: true,
Optional: true,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 😅

Copy link

@JunyiYi JunyiYi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. good spot.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@katbyte
Copy link
Collaborator

katbyte commented Jan 18, 2019

Thanks for the udpates @mdsills 🙂 LGTM now

@katbyte katbyte merged commit 2f58e4d into hashicorp:master Jan 18, 2019
@katbyte katbyte added this to the 2.0.0 milestone Jan 18, 2019
@bfleming-ciena
Copy link

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

image

Thanks

@ghost
Copy link

ghost commented Mar 5, 2019

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!

@ghost ghost locked and limited conversation to collaborators Mar 5, 2019
@mdsills mdsills deleted the issue-1875-support-for-pick-hostname-from-backend-address branch March 11, 2019 10:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants