-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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_app_service_managed_certificate
- bind certificate to the defined hostname
#9631
Conversation
Function App test passing:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jackofallops! This PR looks good but I'm worried about people upgrading their provider from the old version to this version and having to recreate the resource.
Type: schema.TypeString, | ||
Optional: true, | ||
ForceNew: true, | ||
Default: string(web.SslStateIPBasedEnabled), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, is this the default that's passed back from the API when we don't specify ssl_state
. I'd just like to confirm that this won't cause a recreation of this resource when people upgrade the provider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have to pick one, and it can only be applied here (the binding itself rejects a value here unless the thumbprint is also supplied at creation). Strictly speaking, this is Required
, but we try to avoid introducing new properties as such. So it's a call one way or the other... This will only have a value for customers that have manually bound the cert to the hostname, and we can add an upgrade note to the changelog to cover this change?
30bce2e
to
9317247
Compare
22b824b
to
fe47d69
Compare
Updated tests passing:
|
@jackofallops The binding operation is against a different resource than the certificate (a modification of a part of an existing hostname_binding), so not sure why we aren't creating a new binding resource which will supersede the binding part of the hostname_binding resource, and handle all certificate bindings (deprecating those fields in the existing resource). That was the start of what I proposed in #8069 There was an explicit example of another use case where this would be useful (not just for managed certificates), when using lets encrypt... #8069 (comment) I've also just submitted a PR #9701 which patches the existing managed_certificate resource, as ARM forces it (overrides what value you provide) for the resource group to be that of the service plan its hosted in. We need to be careful that the (real) certificate ID and the hostname_binding it patches could be in different RGs. This complexity is bypassed (as well as forming the basis for the initial point) if you use the resource proposal and interface I created for this in PR #9415 |
Apparently I'd taken too narrow a view of what we were looking to achieve, my apologies, I'd not considered the additional cases. I was out at short notice Friday, I'll pick this back up asap today. |
Closing this in light of additional use cases. Will work with @AdamCoulterOz to update #9415 to cover these. |
This has been released in version 2.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 = "~> 2.40.0"
}
# ... other configuration ... |
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! |
Upgrade Notes:
azurerm_app_service_managed_certificate
now requires thessl_state
property, which can be one ofSniEnabled
orIpBasedEnabled
to define the SSL type for the certificate on the hostname it is bound to.Cannot run in CI yet, but tests pass and resources are correctly configured:
![image](https://user-images.githubusercontent.com/11830746/100890596-50d5a800-34b0-11eb-85e6-d074d814768c.png)
TODO:
Replaces #9415
resolves #4824