New resource: azurerm_api_management_custom_domain

[sirlatrom]

Closes #3058.

The approach used is to modify the API Management service (corresponding to an azurerm_api_management resource) by updating an existing instance. A note is added to the documentation in both that and the new resource telling that you should not configure the custom domains in both resources, as that would likely create conflicts.

Semantically, if you delete an azurerm_api_management_custom_domain, the hostname_configurations field will be set to be empty (null) when the API Management service instance is updated. There should not be more than one azurerm_api_management_custom_domain resource for an azurerm_api_management, but I have not found a way to enforce this using the Terraform Provider SDK.

---

Includes:

• Implementation
• Acceptance tests
• Website docs

[sirlatrom]

Hi,
Do you have a plan when this feature will be completed?

You can vote for the issue to be prioritized by giving a 👍 reaction to the top-level issue description that this PR should solve: #3058 (comment).

Whenever a maintainer has time to look through the proposed changes and additions, this PR can move forward.

[sirlatrom]

Rebased just in case.

[sirlatrom]

@manicminer Is there any chance of this getting reviewed before 2.30.0?

[manicminer]

Hi @sirlatrom, thanks for this PR! I have a few minor edits and suggestions inline, and I think some more test coverage is needed.

The testAccAzureRMApiManagementCustomDomain_basic test looks good, but there is no test that sets any of the custom domains. I'd suggest adding a testAccAzureRMApiManagementCustomDomain_complete test having at least two types configured, and then a testAccAzureRMApiManagementCustomDomain_update test which first creates a resource with no domains (basic), then adds some domains (complete) and lastly removes then (back to basic), with import steps in between.

If we can expand the tests and fix up the other edits, this looks great to merge!

[sirlatrom]

Hi @sirlatrom, thanks for this PR! I have a few minor edits and suggestions inline, and I think some more test coverage is needed.

The testAccAzureRMApiManagementCustomDomain_basic test looks good, but there is no test that sets any of the custom domains. I'd suggest adding a testAccAzureRMApiManagementCustomDomain_complete test having at least two types configured, and then a testAccAzureRMApiManagementCustomDomain_update test which first creates a resource with no domains (basic), then adds some domains (complete) and lastly removes then (back to basic), with import steps in between.

If we can expand the tests and fix up the other edits, this looks great to merge!

@manicminer I've addressed the code comments but have yet to write a testAccAzureRMApiManagementCustomDomain_update test. I'm not actually sure if it's actually required to have at least one domain configured - it might be. I've updated the basic test to set two domains of different types. I'm not too familiar with the update type of test.

[sirlatrom]

Wow, turns out the success I thought I'd had with my first attempt was a false positive, and it really did nothing. I've added several commits and have now verified that it actually does what it is supposed to now.

[manicminer]

@sirlatrom I haven't tested your recent edits, but it makes sense at a glance.

When testing it seemed like having at least one domain wasn't required, so I figure it makes sense to be able to have a resource with no domains configured - thus enabling removal of existing configured domains?

Given that, it's useful to have a test that configures no domains ("basic"), another that configures some ("complete"), and an "update" test that applies a series of configurations ("basic", then "complete", then "basic") to test that moving from no domains, to some domains, and back to no domains, works as expected.

[sirlatrom]

@sirlatrom I haven't tested your recent edits, but it makes sense at a glance.

When testing it seemed like having at least one domain wasn't required, so I figure it makes sense to be able to have a resource with no domains configured - thus enabling removal of existing configured domains?

Given that, it's useful to have a test that configures no domains ("basic"), another that configures some ("complete"), and an "update" test that applies a series of configurations ("basic", then "complete", then "basic") to test that moving from no domains, to some domains, and back to no domains, works as expected.

@manicminer Thanks for your feedback and patience with this PR.

The intention (and current implementation in this PR) with regard to being able to remove existing configured domains is that if you've once created an azurerm_api_management_custom_domain resource, when you destroy it the custom domains will be removed again. However, I agree it makes sense to validate adding or removing different types of domains in an update test. I'll try and get that done within the next couple of days.

[jackofallops]

Hi @sirlatrom
Thanks for this, I've noted a few changes / issues below.
Aside from those, was there a reason you chose this as a separate resource over extending the azrure_api_management resource with an additional Optional schema for this config? (The custom ID note below wouldn't be needed in that approach)

[sirlatrom]

Hi @sirlatrom
Thanks for this, I've noted a few changes / issues below.
Aside from those, was there a reason you chose this as a separate resource over extending the azrure_api_management resource with an additional Optional schema for this config? (The custom ID note below wouldn't be needed in that approach)

Yes, the sole purpose is to be able to create an azurerm_key_vault_access_policy in between the azurerm_api_management and this resource. Otherwise, the APIM's System Assigned Identity won't be able to get access to the certificates in the Key Vault.

[sirlatrom]

@manicminer I appreciate how frustrating APIM is to work with, but is there any chance you have made any progress on this PR since last week, or is there perhaps something I can do to move it forward? Thanks to GitHub cache I didn't see your commit until now - please disregard my comment.

[manicminer]

Test Results

TestAccDataSourceAzureRMApiManagement_

TestAccAzureRMApiManagementCustomDomain_

[jackofallops]

Thanks @manicminer & @sirlatrom - This LGTM now 👍

[manicminer]

Thanks @jackofallops 👍

Latest test results:

[ghost]

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

[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!