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

azurerm_container_app_environment - add support for the Custom DNS Suffix (via the new azurerm_container_app_environment_custom_domain resource) and expose the custom_domain_verification_id attribute #24346

Merged

Conversation

iambaim
Copy link
Contributor

@iambaim iambaim commented Dec 27, 2023

Community Note

  • Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
  • Please do not leave "+1" or "me too" comments, they generate extra noise for PR followers and do not help prioritize for review

Description

This PR:

  1. Enables the Custom environment DNS Suffix in Azure Container Apps (https://learn.microsoft.com/en-us/azure/container-apps/environment-custom-dns-suffix) by creating a new virtual resource azurerm_container_app_environment_custom_domain.
  2. Exposes the Custom Domain Validation ID (custom_domain_verification_id attribute) in azurerm_container_app_environment's resource and data source.

PR Checklist

  • I have followed the guidelines in our Contributing Documentation.
  • I have checked to ensure there aren't other open Pull Requests for the same update/change.
  • I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
  • I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
  • I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.

Changes to existing Resource / Data Source

  • I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
  • I have written new tests for my resource or datasource changes & updated any relevent documentation.
  • I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
  • (For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

  • My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)
==> 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 -v ./internal/services/containerapps -parallel=1 -run=TestAccContainerAppEnvironmentCustomDomainResource_ -timeout 180m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccContainerAppEnvironmentCustomDomainResource_basic
=== PAUSE TestAccContainerAppEnvironmentCustomDomainResource_basic
=== RUN   TestAccContainerAppEnvironmentCustomDomainResource_requiresImport
=== PAUSE TestAccContainerAppEnvironmentCustomDomainResource_requiresImport
=== RUN   TestAccContainerAppEnvironmentCustomDomainResource_update
=== PAUSE TestAccContainerAppEnvironmentCustomDomainResource_update
=== CONT  TestAccContainerAppEnvironmentCustomDomainResource_basic
--- PASS: TestAccContainerAppEnvironmentCustomDomainResource_basic (859.72s)
=== CONT  TestAccContainerAppEnvironmentCustomDomainResource_update
--- PASS: TestAccContainerAppEnvironmentCustomDomainResource_update (1025.33s)
=== CONT  TestAccContainerAppEnvironmentCustomDomainResource_requiresImport
--- PASS: TestAccContainerAppEnvironmentCustomDomainResource_requiresImport (898.41s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/containerapps	2783.480s

https://github.com/iambaim/terraform-provider-azurerm/actions/runs/8861417824/job/24333261609#step:7:1

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

  • New Resource: azurerm_container_app_environment_custom_domain #24346
  • azurerm_container_app_environment - expose the custom_domain_verification_id attribute #24346

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

Related Issue(s)

Fixes #24190, fixes #20829, fixes #20502

@iambaim
Copy link
Contributor Author

iambaim commented Jan 4, 2024

Hi @katbyte, thanks for approving the workflows earlier. I can see one of the checks fails but that seems to be unrelated to my changes.

==> Checking for use of gradually deprecated functions...
internal/services/iothub/iothub_resource.go:73:    Create: resourceIotHubCreateUpdate,
New Resources should no longer use combined CreateUpdate methods, please
split these into two separate Create and Update methods.

Existing resources can continue to use combined CreateUpdate methods
for the moment - but over time these will be split into separate Create and
Update methods.

I have now rebase my commits to the latest main. Hopefully all the checks are good now.

@iambaim
Copy link
Contributor Author

iambaim commented Jan 9, 2024

Can I have a review, @jackofallops ? Thanks!

@iambaim iambaim force-pushed the container_app_environment_custom_dns_suffix branch 2 times, most recently from 4f74e14 to 9f0d6b3 Compare January 11, 2024 16:49
@davidkarlsen
Copy link
Contributor

Or maybe @katbyte can review?

@lgmorand
Copy link

lgmorand commented Feb 22, 2024

Hi @jackofallops

anyone who could do the review please ? :) My Cx is looking for the same thing and it blocks the deployment of ACA. I did a code review and it looks fine to me. thanks :)

ps: @iambaim thanks for the work !

@goldmark
Copy link

Hi @jackofallops are there any plans to merge this request? any ETA?

@raddaoui
Copy link

any updates on this PR? my customer is blocked

@Chigurusetty
Copy link

Hi Team, when this could be available? any ETA please

@lgmorand
Copy link

@katbyte for visibility. thanks <3

@iambaim
Copy link
Contributor Author

iambaim commented Mar 22, 2024

Just synced the code with main. Ready for another round of testing, @katbyte and @jackofallops . Thanks!

@davidkarlsen
Copy link
Contributor

@tombuildsstuff can this be merged?

  • it's a small thing
  • many have voted for it
  • it passes
  • it is sitting for 4th month on a row waiting for merge
  • it's really useful for enterprise customers which have more requirements to networking

@iambaim
Copy link
Contributor Author

iambaim commented Apr 13, 2024

Users wanting to implement any form of custom domains in their Azure Container Apps are in a bit of a pickle now as this is still pending and the on-going issue with azurerm_container_app_custom_domain that clashes with secrets. (#25196) :(

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @iambaim - Thanks for this PR.

The successful completion of this configuration requires that the CustomDomainVerificationId already be present in resolvable DNS so, for this to be supported in Terraform, this will need to be a separate resource, as is necessary for azurerm_container_app_custom_domain, perhaps called azurerm_container_app_environment_custom_domain. This virtual resource can then update the Environment after the verification ID is known, and a DNS record can be created in the correct sequence to be present (and used as reference to ensure the dependency graph is in the correct order).

The CustomDomainVerificationId should also be added to the azurerm_container_app_environment Data Source when the resource is updated.

Can you take a look at splitting this into a new virtual resource, and adding the property to the Data Source, and we'll pick up review from there?

Thanks!

@iambaim
Copy link
Contributor Author

iambaim commented Apr 26, 2024

Hi @jackofallops , thanks for the suggestion. I agree, this is a better approach. WIll update the branch in a few days :)

@iambaim iambaim force-pushed the container_app_environment_custom_dns_suffix branch from dd25ca1 to 60b7220 Compare April 27, 2024 10:41
@github-actions github-actions bot added size/XL and removed size/M labels Apr 27, 2024
@iambaim iambaim force-pushed the container_app_environment_custom_dns_suffix branch from 60b7220 to 0e2142c Compare April 27, 2024 16:59
@iambaim iambaim force-pushed the container_app_environment_custom_dns_suffix branch from 0e2142c to d0ec8e1 Compare April 27, 2024 17:08
@iambaim iambaim changed the title azurerm_container_app_environment - add support for the Custom DNS Suffix and expose the custom_domain_verification_id attribute azurerm_container_app_environment - add support for the Custom DNS Suffix (via the new azurerm_container_app_environment_custom_domain resource) and expose the custom_domain_verification_id attribute Apr 27, 2024
@iambaim
Copy link
Contributor Author

iambaim commented Apr 29, 2024

Hello @jackofallops . The codes are now ready for review.

FYI, the addition of testdata/testacc_nopassword.pfx file is necessary to test the resource update feature, because updating using the same cert will trigger an Azure API error. I copied the file from internal/services/web/testdata/app_service_certificate_nopassword.pfx.

Thanks!

@iambaim
Copy link
Contributor Author

iambaim commented May 7, 2024

Thanks for the review, @katbyte. I have updated the code.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aside from a few minor comments this LGTM 🕯️

@katbyte katbyte merged commit ad22416 into hashicorp:main May 9, 2024
32 checks passed
@github-actions github-actions bot added this to the v3.103.0 milestone May 9, 2024
katbyte added a commit that referenced this pull request May 9, 2024
@lgmorand
Copy link

thanks @iambaim & @katbyte

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment