azurerm_key_vault_certificate - Support update of certificates based on certificate_policy

[dvob]

This PR adds support for updating certificates created using a certificate_policy.
For this I factored out the create and wait logic into a separate createCertificate function which is then used in the actual create and update functions of the certificate resource. If a certificate already exists the CreateCertificate function from the SDK creates a new version of that certificate.

Should fix:

• fix Add support for updating Key Vault Certificate content #17358
• fix Azure key vault certificate recreate error when purge protection is enabled #13012
• fix azurerm_automation_certificate keeps recreating #12456
• fix Updating azurerm_key_vault_certificate fails when soft delete is enabled for Key Vault #10658

Further this PR fixes the problem when a certificate gets recovered the actual requested settings were not applied which meant you ended up with the old certificate (the one you deleted)

There still remains the problem that values which contain or depend on the certificate version (id, version, secret_id, certificate_data, certificate_data_bas64 and thumbprint) are not recomputed in the same run as the certificates change. To make it clear, this was already a problem before this change. If you imported a new version of a certificate using the field certificate.contents some fields are never updated .
I tried to fix this using a CustomizeDiffFunc (see dvob@ef3cb69). The problem with that was that the *schema.ResourceDiff always reported changes for the certificate_policy and hence the values got recomputed every time.
Further it was not possible to mark id as recomputed. This is probably due to the special treatment of the id field.
Probably the id should not contain the version. I'm not sure if this could be changed, because this would be a breaking change. On the other hand at the moment it probably also doesn't really behave as a user would expect it.

[wlitke]

@mbfrahry Can you already estimate when the merge could be completed? I'm really looking forward for the changes.
(BTW: I don't know whether you're the right person to ask but since you've been assigned two weeks ago I thought it would be worth a try ;))

@stephybun FYI

[tibers]

I am also getting bit by this presently and I could really use it merged in. :)

[manoj7shekhawat]

We are also eagerly waiting for this fix to be merged in. Hope it soon gets rolled out :)

[mbfrahry]

Hey @dvob, apologies for the late review here. We've been discussing internally the id of this resource will be changing during updates which means Terraform won't be idempotent as resources linked to this one will update after the certificate updates.

With that said, we've determined that it's find as the functionality for this outweighs the negative of the id change.

I've left some notes for things we should do slightly differently but this looks good otherwise! Thanks for this and for your patience.

[dvob]

do you have plans on when you want to release 4.x? because i think it would make sense to fix this entire resource on the next major version when you can introduce breaking changes.

[dvob]

@mbfrahry Changed all points based on your input apart from the RequestID thing (see comment above).
Also rebased on main that I got the ParseKeyVaultID in the commonids package.
What about the question I asked above concerning 4.x?

[mbfrahry]

Hey @dvob, thanks for making those changes but I'm still looking for you to remove the operation code. We also don't have specific dates for when we aim to release 4.0 but these changes should suffice for now as they are not breaking certificates.

[dvob]

@mbfrahry

this will cause issues with Terraform because Terraform is getting the latest version of the certificate during the Read function which should be whatever the latest operation is waiting on and not a specific operation (even though as you say, it will most likely be this specific operation)

The keyVaultCertificateCreationRefreshFunc is not used in the read part. It's only used to wait on completion in the createCertificate function.
If this error occurs users can see from the error that something else did start a create certificate operation which will end up with a certificate with different parameters. As a user of this resource this is the behavior I would prefer over silently returning success even if we can't be sure if the certificate with our certain parameters ever got created.

I still think that this check would offer the better UX but I'm also fine without it. I removed this check with the latest commit. If you agree with my latest argument above we can still revert the latest commit. Otherwise you can merge the current state if you don't have further points to address.

[dvob]

@mbfrahry concerning 4.x do you have a backlog somewhere with all the required changes for that release. then we could add it there that it will not be forgotten. Because the current state of the certificate resource is quite a mess.

Thank you for your review and your effort!

[mbfrahry]

The keyVaultCertificateCreationRefreshFunc is not used in the read part. It's only used to wait on completion in the createCertificate function. If this error occurs users can see from the error that something else did start a create certificate operation which will end up with a certificate with different parameters. As a user of this resource this is the behavior I would prefer over silently returning success even if we can't be sure if the certificate with our certain parameters ever got created.

keyVaultCertificateCreationRefreshFunc is not used during read but we grab the latest certificate version during the read which would presumably be whatever is the result of last operation run.

If we kept it the way you originally wrote that check, we would error if a new operation came in and then the next time we run Terraform, Terraform should show discrepancies between what the api has and what is in the config file. Regardless of having that error, Terraform will show discrepancies if they occur and that a user will have to update the certificate if they want their config to be what the latest key vault certificate should be.

I can see the value you are trying to add but we don't want Terraform to error if the api and the config file are showing differences during a Terraform run but rather we want to show those differences on a subsequent Terraform run.

[mbfrahry]

LGTM! Thanks for this @dvob

[wlitke]

Thank you to everyone who was participating in this! This will make the certificate handling surly less painful ;)