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

WIP: New Resource "azurerm_app_service_managed_certificate" #5092

Closed
wants to merge 1 commit into from

Conversation

tiwood
Copy link
Contributor

@tiwood tiwood commented Dec 6, 2019

WIP: New Resource "azurerm_app_service_managed_certificate"

Related issues

This closes #4824

TODO

@drdamour
Copy link
Contributor

drdamour commented Jan 3, 2020

@tiwood does the hostname have to already be setup as a custom domain binding for the app service? If so, then the https://www.terraform.io/docs/providers/azurerm/r/app_service_custom_hostname_binding.html would need to be split to having the cert for a binding be it's own resource as the chain would need to be

setup custom domain binding -> generate managed certificate -> bind managed certificate to custom domain binding

but if you can generate the managed cert with a hostname not already bound as a custom domain then probably fine.

I bring this up cause the GUI is certainly limited to picking from bound custom domains

image

@DaRosenberg
Copy link

@drdamour This would indicate that the custom hostname binding does not need to be split:
https://dotnetdevlife.wordpress.com/2019/11/11/app-service-managed-certificate/

Looks like the order is:

  1. App service plan
  2. App
  3. Certificate (referencing app service plan and domain name)
  4. Hostname binding (referencing thumbprint of certificate)

@tiwood Would certainly love to get this finished up - we are currently blocked in this...

@jackofallops
Copy link
Member

Hi @tiwood
Having reviewed this change it looks like the differentiating factor between this new resource and azurerm_app_service_certificate is the password attribute only? If so, once the blocking issue in the SDK is removed, it might be more appropriate to update the existing resource to cover this use case also? If you're happy with that, it would probably be better to close this PR and track the bug on issue #4824. WDYT?

@DaRosenberg
Copy link

@jackofallops Not sure if this is relevant to your point or not, just mentioning it just in case:

I managed to figure out a way to create these certificates using template deployment.

The necessary steps are:

  1. Create app service
  2. Create app
  3. Create hostname binding (without SSL enabled)
  4. Create managed certificate for the same hostname
  5. Update hostname binding with thumbprint of certificate, thus enabling SSL on it

Steps 4 and 5 are, in my case, done in an ARM template deployment resource.

So, I think for practical purposes, a new resource type for this should maybe include both steps 4 and 5, which makes me think maybe it's different enough from azurerm_app_service_certificate to require that it be its own resource type.

@jackofallops
Copy link
Member

@DaRosenberg
It sounds like you're describing a circular dependency there? If that's the case, it's likely something that Terraform cannot support.
If not, it would follow that the existing (modified, post-bug-fixed) resource could be used along with azurerm_app_service_custom_hostname_binding as with the normal flow?

@DaRosenberg
Copy link

@jackofallops Not sure what the technical definition of a circular dependency is in Terraform. I use the Terraform Azure Provider only through Pulumi. But from my point of view, it's not so much a circular dependency, as it is a need to refer to the same resource twice (once to create it, once more to update it).

@drdamour
Copy link
Contributor

The circular is what i was worried about in #5092 (comment) i think we need a new resource that binds a cert to a hostname binding

@tombuildsstuff
Copy link
Contributor

👋 hey @tiwood

Since this PR's still blocked on the upstream issue in the Azure SDK - rather than leaving this open until that's fixed, I'm going to temporarily close this PR for the moment. As this issue is already assigned to the "Blocked" milestone, once the upstream issue is fixed we'll re-open this and take another look - apologies for the delay here!

Thanks!

@ghost
Copy link

ghost commented Jul 26, 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 Jul 26, 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.

Add Support for App Service Managed Certificates
5 participants