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

azurerm_application_gateway - add trusted_root_certificate_names property #4821

Closed

Conversation

majimenez-stratio
Copy link

@majimenez-stratio majimenez-stratio commented Nov 6, 2019

Possible fix for issue #4502

@majimenez-stratio majimenez-stratio changed the title Possible fix for #4502 Bind trusted_root_certificate with http_settings for azurerm_application_gateway Nov 6, 2019
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @majimenez-stratio, this is a good start but we need to set this value in state and add a test that it works. I also suggested a different way to implement this for simplicity that I'm interested in discussing further.

Required: true,
},

"id": {
Copy link
Member

Choose a reason for hiding this comment

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

Do you see anyone using this id anywhere? If not, it might be worth removing this block entirely and just having an attribute called trusted_root_certificate_name

Copy link
Author

Choose a reason for hiding this comment

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

Hi @mbfrahry, you're right, I don't think id is needed, a list of strings should be enough. I pushed a new commit with your suggestion and managed the state. Let me know what you think. Unit test is still missing, I'll try to provide one asap.

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Thanks for swapping those out and these changes look good but we need a test to confirm that we can create and update an application_gateway with trusted root certificates

@katbyte katbyte changed the title Bind trusted_root_certificate with http_settings for azurerm_application_gateway azurerm_application_gateway - add trusted_root_certificate_names property Dec 18, 2019
@katbyte
Copy link
Collaborator

katbyte commented Dec 18, 2019

Hi @majimenez-stratio,

Thank you for opening this PR, given its been a while i've gone ahead and made the required changes. I hope you don't mind but as i cannot push to your branch i have opened #5204 to continue the work and get this meged.

As such i am going to close this pr in favour of it. Thanks again!

@katbyte katbyte closed this Dec 18, 2019
@katbyte katbyte added this to the v1.40.0 milestone Dec 18, 2019
katbyte added a commit that referenced this pull request Dec 18, 2019
@ghost
Copy link

ghost commented Jan 8, 2020

This has been released in version 1.40.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.40.0"
}
# ... other configuration ...

@ghost ghost removed the waiting-response label Jan 8, 2020
@ankurkapoor
Copy link

Sorry but i want to get some clarification on this fix. Is this going to allow me to use the authentication_certificate inside the backend_http_settings (as shown below)?

backend_http_settings {
name = "${local.http_setting_name}"
cookie_based_affinity = "Disabled"
port = 443
protocol = "Https"
request_timeout = 30
host_name = "XXX-qa.test.com"
authentication_certificate{
name = "${local.trusted_root_certificate_name}"
}
}

The reason i am trying to understand is this because even after upgrading to version 1.40 for terraform i am still getting same error. I did post this few days ago https://discuss.hashicorp.com/t/using-trusted-root-certificate-for-application-gateway-v2/4883 but can't really see any way forward.

@ghost
Copy link

ghost commented Jan 22, 2020

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 Jan 22, 2020
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.

5 participants