New Resource: azurerm_app_service_managed_certificate

[AdamCoulterOz]

forked from @tiwood, updated with the latest master, and fixed the password issue you were having previously... it just needed to be set as an empty string, and it worked. This addresses part of #4824

	certificate := web.Certificate{
		CertificateProperties: &web.CertificateProperties{
			CanonicalName: utils.String(name),
			ServerFarmID:  utils.String(appServicePlanID),
			Password:      new(string),
		},
		Location: utils.String(location),
		Tags:     tags.Expand(t),
	}

I then needed to handle the 202 accepted "error" and wait 30 seconds for it to be created before trying to read it again.

I've tested this locally with my own Azure subscription.

This still needs to have tests added and documentation written. Its also probably worth creating the data resource at the same time.

fixes #7537

[AdamCoulterOz]

This is a problem though ... although I've worked around it for now.

Azure SDK for Go:
web.CertificatesClient.CreateOrUpdate always returns error due to not supporting 202 Accepted Rest Response

Azure/azure-sdk-for-go#13665

[jackofallops]

Thanks for getting this started @AdamCoulterOz - I've done some additional work on this over the weekend (I'll push it later today) and hit a possible 2nd issue with app service that I'm looking into (It may need us to bump the API to 2020-06-01). I'll update here as soon as I have progress.

[AdamCoulterOz]

@jackofallops:

Thanks for getting this started @AdamCoulterOz - I've done some additional work on this over the weekend (I'll push it later today) and hit a possible 2nd issue with app service that I'm looking into (It may need us to bump the API to 2020-06-01). I'll update here as soon as I have progress.

Thanks Steve...
Yep... for this to work, my other PR (#9372) which exposes the CustomDomainVerificationID needs to be included which also has a dependency on 2020-06-01, which I actually included already in that last week. I've noticed you've also now done it in a seperate PR too :-)

[jackofallops]

@AdamCoulterOz - Apologies, I missed that when I looked for an SDK bump in existing PR's. I think we should probably split it out of yours since we know there's at least one breaking change if that's OK, and include the additional property after to help keep things clean? I'll comment there also for completeness.

[AdamCoulterOz]

@jackofallops yep that's cool, I figured you guys would want it that way too, happy to wait, as long as it can be in the next week or so 😄 a bit stuck on a project atm.

[jackofallops]

@jackofallops yep that's cool, I figured you guys would want it that way too, happy to wait, as long as it can be in the next or so 😄 a bit stuck on a project atm.

It's my main focus this week, so 🤞

[AdamCoulterOz]

@jackofallops I've been working on this and I've realised there is no point creating this resource to be separate from azurerm_app_service_custom_hostname_binding and then creating a azurerm_app_service_managed_certificate when it can never be created separately from a hostname_binding, only to then create another azurerm_app_service_custom_hostname_certificate_binding to modify the existing hostname_binding.

We could do this much easier by adding a single attribute use_managed_certificate to the azurerm_app_service_custom_hostname_binding and create a managed_certificate inside that lifecycle. I can put together as an example, will be much less code to manage and much simpler to use.

[jackofallops]

Thanks @AdamCoulterOz - I think, given the current state of the service, it would be better to keep them as separate resources. I've got this all working locally, but hit something of a road-block on setting up something for the acceptance tests (something that will be a problem no matter the implementation). I'm going to get some good manual test coverage done, and discuss with the team about taking this forward on that basis (there's precedent for this approach in App Service, so shouldn't be a problem). I'll push after I revert my manual hacks to the acctests later today so another member of the team can review.

=== RUN   TestAccAzureRMAppServiceManagedCertificate_basicLinux
=== PAUSE TestAccAzureRMAppServiceManagedCertificate_basicLinux
=== CONT  TestAccAzureRMAppServiceManagedCertificate_basicLinux
--- PASS: TestAccAzureRMAppServiceManagedCertificate_basicLinux (274.42s)
PASS

[AdamCoulterOz]

@jackofallops if you're going to stick with azurerm_app_service_managed_certificate then I would change the inputs to be just this single attribute:

• app_service_custom_hostname_binding_id

This is because in order to create a managed certificate you must already have an azurerm_app_service_hostname_binding and can only use the other derived attributes as the allowed values.

For example, the current other input attributes:

• name - (name of the app_service) must be the same as the binding_id app service name
• canonical_name - must be identical to the hostname in the binding_id
• location - must be the location of the underlying app service plan
• resource_group_name - must be the same as the underlying app service plan
• app_service_plan_id - must be the id of the underlying app service plan

So there is no point allowing the user to specify any of them.

[jackofallops]

@jackofallops if you're going to stick with azurerm_app_service_managed_certificate then I would change the inputs to be just this single attribute:

• app_service_custom_hostname_binding_id

This is because in order to create a managed certificate you must already have an azurerm_app_service_hostname_binding and can only use the other derived attributes as the allowed values.

Very good shout 👍 - I'll refactor now that I have working tests.

[jackofallops]

CI not ready/able to run these tests yet, but passing locally:

➜  terraform-provider-azurerm git:(adamcoulteroz-app_service_cert) make testacc TESTARGS='-run TestAccAzureRMAppServiceManagedCertificate_' TEST=./azurerm/internal/services/web/tests/
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test ./azurerm/internal/services/web/tests/ -v -run TestAccAzureRMAppServiceManagedCertificate_ -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccAzureRMAppServiceManagedCertificate_basicLinux
=== PAUSE TestAccAzureRMAppServiceManagedCertificate_basicLinux
=== RUN   TestAccAzureRMAppServiceManagedCertificate_basicImport
=== PAUSE TestAccAzureRMAppServiceManagedCertificate_basicImport
=== RUN   TestAccAzureRMAppServiceManagedCertificate_basicWindows
=== PAUSE TestAccAzureRMAppServiceManagedCertificate_basicWindows
=== CONT  TestAccAzureRMAppServiceManagedCertificate_basicLinux
=== CONT  TestAccAzureRMAppServiceManagedCertificate_basicWindows
=== CONT  TestAccAzureRMAppServiceManagedCertificate_basicImport
--- PASS: TestAccAzureRMAppServiceManagedCertificate_basicWindows (239.06s)
--- PASS: TestAccAzureRMAppServiceManagedCertificate_basicLinux (246.19s)
--- PASS: TestAccAzureRMAppServiceManagedCertificate_basicImport (258.05s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/web/tests	259.985s

[tombuildsstuff]

hey @AdamCoulterOz / @jackofallops

Thanks for pushing those changes - I've taken a look through and whilst this is mostly looking good I've left a few comments inline, if we can fix those up then this should otherwise be good to merge 👍

Thanks!

[jackofallops]

Acceptance tests passing (failures unrelated) in addition to local tests above:

[ghost]

This has been released in version 2.38.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.38.0"
}
# ... other configuration ...

[imod]

@AdamCoulterOz sorry if i ask a dump question about the solution: does it handle the case where the cert has to be renewed? is it enough to just trigger terraform regularly? Is this documented?

[AdamCoulterOz]

@imod as per this documentation from Microsoft the certificate is automatically renewed every 5 months (and remains valid for 6 months).

https://azure.microsoft.com/en-au/updates/secure-your-custom-domains-at-no-cost-with-app-service-managed-certificates-preview/

My understanding is that once that cert is bound, you don't need to do anything to rotate it, and it just happens. We've tried to make it so the thumbprint (which changes with certificate renewal) isn't an identifying key in creating a managed certificate, so won't create a forced new resource if it does change later (i.e. is managed outside the scope of the terraformed certificate resource).

[tombuildsstuff]

@AdamCoulterOz given App Service will auto-rotate this for you behind the scenes, do you think it's worth adding an info box to the resource page to explain that behaviour?

[AdamCoulterOz]

@tombuildsstuff Sure, I'll put in a PR for it.

[tombuildsstuff]

👍 awesome, thanks @AdamCoulterOz :)

[ghost]

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!