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 - Allow multiple custom_domain blocks within the ingress block #20842

Closed
wants to merge 5 commits into from

Conversation

srjobinar
Copy link
Contributor

@srjobinar srjobinar commented Mar 8, 2023

According to the azurerm_container_app documentation multiple custom domain blocks are allowed in the ingress block.
image

Currently terraform plan validation fails when multiple blocks are configured.

The following changes are made:

  1. Recreate the test certificate with CN acceptancetest.contoso.net and SAN acceptancetest.contoso.net
  2. Add tests which creates container Apps with multiple custom_domain block within ingress block
  3. Remove the maxItems schema validation for custom_domain schema

fixes #20784

Jobin Rufus and others added 5 commits March 6, 2023 17:25
…ck-docs

azurerm_container_app - Add documentation for custom domain block within the ingress block
* add tests for custom domain blocks and update test certificate with SAN

* Remove maxItems restriction for custom domain block

* Fix custom domain test steps

---------

Co-authored-by: Jobin Rufus <Jobin.Rufus@tesco.com>
@github-actions github-actions bot added the size/M label Mar 8, 2023
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @srjobinar, this change looks good though the test you've made is failing. Once that's fixed up, we can merge this in

=== CONT  TestAccContainerAppResource_basicWithCustomDomains
testcase.go:110: Step 1/2 error: Error running apply: exit status 1
Error: creating Container App (Subscription: "*******"
Resource Group Name: "acctestRG-CAEnv-230308223112188763"
Container App Name: "acctest-capp-230308223112188763"): performing CreateOrUpdate: containerapps.ContainerAppsClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="InvalidCustomHostNameValidation" Message="A TXT record pointing from asuid.acceptancetest.contoso.com to 633CA537530783A9D6C4F655280B4FACA5D10CD906F227C70F7B5C7DD73DE026 was not found."
with azurerm_container_app.test,
on terraform_plugin_test.tf line 48, in resource "azurerm_container_app" "test":
48: resource "azurerm_container_app" "test" {
creating Container App (Subscription: "*******"
Resource Group Name: "acctestRG-CAEnv-230308223112188763"
Container App Name: "acctest-capp-230308223112188763"): performing
CreateOrUpdate: containerapps.ContainerAppsClient#CreateOrUpdate: Failure
sending request: StatusCode=0 -- Original Error:
Code="InvalidCustomHostNameValidation" Message="A TXT record pointing from
asuid.acceptancetest.contoso.com to
633CA537530783A9D6C4F655280B4FACA5D10CD906F227C70F7B5C7DD73DE026 was not
found."
--- FAIL: TestAccContainerAppResource_basicWithCustomDomains (1918.67s)

@srjobinar
Copy link
Contributor Author

srjobinar commented Mar 9, 2023

Hey @mbfrahry, thanks for checking this out.
I am currently able to create container apps using azapi resource using a similar config.
The only difference being that the managed environment is within a VNet for my usecase.

Was wondering if this error occurs only for a managedEnv which is NOT within a VNet.

Additionally I can see that there is already an issue open with Microsoft for the same problem:
microsoft/azure-container-apps#525

@jackofallops jackofallops added upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR service/container-apps labels Mar 14, 2023
@jrnk
Copy link

jrnk commented May 5, 2023

Is this a bugfix or is this a newly added feature since the current documentation states: "(Optional) One or more custom_domain block as detailed below."

However, upon trying to apply this configuration we get an error:

│ No more than 1 "custom_domain" blocks are allowed```

@WolfspiritM

This comment was marked as off-topic.

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 @srjobinar
Thanks for this PR. It looks mostly good to me, but we'll need you to remove the changed binary file (testacc.pfx) from the PR please, and I'll get the tests run and a final review completed.

Thanks!

@srjobinar
Copy link
Contributor Author

Hi @srjobinar Thanks for this PR. It looks mostly good to me, but we'll need you to remove the changed binary file (testacc.pfx) from the PR please, and I'll get the tests run and a final review completed.

Thanks!

@jackofallops
The updated file was to add multiple domains in the certificate, if I remove that how do I test the change to allow multiple domains?

@@ -221,7 +221,6 @@ func ContainerAppIngressCustomDomainSchema() *pluginsdk.Schema {
return &pluginsdk.Schema{
Type: pluginsdk.TypeList,
Optional: true,
MaxItems: 1,

Choose a reason for hiding this comment

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

Based on the documentation, instead of removing the line, it looks like it might have been a typo. Should this be MinItems : 1, based on the documentation stating:

One or more custom_domain block as detailed below.

@jackofallops
Copy link
Member

Hey @srjobinar, this change looks good though the test you've made is failing. Once that's fixed up, we can merge this in

=== CONT  TestAccContainerAppResource_basicWithCustomDomains
testcase.go:110: Step 1/2 error: Error running apply: exit status 1
Error: creating Container App (Subscription: "*******"
Resource Group Name: "acctestRG-CAEnv-230308223112188763"
Container App Name: "acctest-capp-230308223112188763"): performing CreateOrUpdate: containerapps.ContainerAppsClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="InvalidCustomHostNameValidation" Message="A TXT record pointing from asuid.acceptancetest.contoso.com to 633CA537530783A9D6C4F655280B4FACA5D10CD906F227C70F7B5C7DD73DE026 was not found."
with azurerm_container_app.test,
on terraform_plugin_test.tf line 48, in resource "azurerm_container_app" "test":
48: resource "azurerm_container_app" "test" {
creating Container App (Subscription: "*******"
Resource Group Name: "acctestRG-CAEnv-230308223112188763"
Container App Name: "acctest-capp-230308223112188763"): performing
CreateOrUpdate: containerapps.ContainerAppsClient#CreateOrUpdate: Failure
sending request: StatusCode=0 -- Original Error:
Code="InvalidCustomHostNameValidation" Message="A TXT record pointing from
asuid.acceptancetest.contoso.com to
633CA537530783A9D6C4F655280B4FACA5D10CD906F227C70F7B5C7DD73DE026 was not
found."
--- FAIL: TestAccContainerAppResource_basicWithCustomDomains (1918.67s)

Hi @srjobinar - After re-review, this does indeed have a circular dependency as the value for custom_domain_verification_id isn't known until after the app is created, so I think it will need to be implemented in a different way. Specifically, I think this will need to be a new association / meta resource (something like azurerm_container_app_custom_domain) that updates the azurerm_container_app after creation, so that the CustomDomainVerificationId can be known and appropriate TXT record(s) created in the relevant DNS Zone(s), then the meta resource can be used to update the app appropriately. Is this something you feel you could do? If not, it's something we can try and schedule in for the team to take a look into?

If it helps, I think the new resource would look something like:

resource "azurerm_container_app_custom_domain" "example" {
  name                                     = "my-custom-host.mydomain.net"
  container_app_id                         = azurerm_container_app.example.id
  container_app_environment_certificate_id = azurerm_container_app_environment_certificate.example.id
  custom_domain_verification_record_id     = azurerm_dns_txt_record.example.id
}

There may need to be some additional consideration for cases where the DNS record isn't in Azure DNS?

@srjobinar
Copy link
Contributor Author

Is this something you feel you could do? If not, it's something we can try and schedule in for the team to take a look into?

Hey @jackofallops

I believe creating a new resource from scratch would be a bit hard for me considering my very limited experience in golang and other deliverables I have. Please do go ahead with the change :)

Just one point for you to consider maybe:
I was able to create multiple custom domains for container apps within a Vnet in Azure using terraform. Used azapi resource at the time for doing that. So there are two possibilities here:

  1. azapi resource somehow is not affected by this circular dependency. If so why?
  2. This circular dependency is not affecting container apps created within a Vnet, if so why?

Might be worth for you to check this as well. I'll try and find some time to redo the above step using azurerm resource, which should give me more clarity on which one of the above is true

@jackofallops
Copy link
Member

Is this something you feel you could do? If not, it's something we can try and schedule in for the team to take a look into?

Hey @jackofallops

I believe creating a new resource from scratch would be a bit hard for me considering my very limited experience in golang and other deliverables I have. Please do go ahead with the change :)

Just one point for you to consider maybe: I was able to create multiple custom domains for container apps within a Vnet in Azure using terraform. Used azapi resource at the time for doing that. So there are two possibilities here:

  1. azapi resource somehow is not affected by this circular dependency. If so why?
  2. This circular dependency is not affecting container apps created within a Vnet, if so why?

Might be worth for you to check this as well. I'll try and find some time to redo the above step using azurerm resource, which should give me more clarity on which one of the above is true

Hi @srjobinar - Within a Vnet, I suspect the DNS checks are skipped due to it being in local addressing? (I'd have to test this to be sure) Without a VNet, the domain must be considered public, and therefore checking DNS ownership via the validation TXT record is necessary. This limitation is not unique to AzureRM, az-api will have the same constraint / failure without a Vnet as the CustomDomainVerificationId is not known at creation time so the DNS validation will fail. Adding custom domain configuration as updates after the resource is created would be possible since the value would be known, and the appropriate TXT record could be added. However, unless Terraform/AzureRM is aware of these changes, due to the nature of the Azure API these can be removed even if configured using the AzApi provider. Since we must cater for an apply to complete all tasks in one execution, we need to be able to create the container app followed by the DNS record for CustomDomainVerificationId, followed in turn by adding the custom domains to the container app. The only way to do this in Terraform is to have a discrete resource for the custom domain so the original resource can be modified appropriately.

Whilst we could update the resource to allow for multiple custom domains in the existing resource, we'd only be catering for the "bring your own vnet" case, so we would need to still implement the additional association resource for users that do not. At that point, it makes more sense to have a single implementation which caters for both scenarios? WDYT?

@srjobinar
Copy link
Contributor Author

This limitation is not unique to AzureRM, az-api will have the same constraint / failure without a Vnet as the CustomDomainVerificationId is not known at creation time so the DNS validation will fail

Yeah that makes sense, have confirmed the same in my tests. Just for clarity it was working with azapi in my case as the resource was created without ingress first in which the DNS record was configured via terraform. Ingress was added in subsequent runs.

The only way to do this in Terraform is to have a discrete resource for the custom domain so the original resource can be modified appropriately.

Just wondering since custom domain is a configuration within the ingress block, should this be a separate resource for ingress resource itself or just the custom domain?

At that point, it makes more sense to have a single implementation which caters for both scenarios?

Yeah I agree, would not be ideal to have to do things differently for with/without the Vnet.

@WolfspiritM
Copy link

WolfspiritM commented Nov 6, 2023

What is the status of this? Why exactly is this waiting response?
We currently have to manually assign additional custom domains via the azure portal for months now.

I'm not exactly sure what the problem here is.
The customDomainVerificationId is also available from the environment but terraform doesn't expose that:
#20829
#20502

So IF you expose the customDomainVerificationId then domains can be provisioned with the correct records before provisioning the app. No need (yet) for an extra resource.

@teck-kcheema
Copy link

Would love to see this merged! Much needed

@jackofallops
Copy link
Member

Hi all - so by way of update on this long outstanding PR. It's waiting response as the resource has a circular reference since it requires that the custom_domain_verification_id be used for a TXT record. Since this needs to be created before the custom_domain can be used, this implementation will not work for create operations, and would need to be done in two sequential apply operations which is not how resources are supposed to work.

Based on this, I think this actually needs be considered for a virtual resource and the inline item should be removed (or at least become Computed only), as has been done in other situations like this. (e.g. azurerm_static_site_custom_domain).

Due to the above I'm going to close this PR out for the time being and I'll add a note directly to the related issue to the same effect.

Copy link

github-actions bot commented May 4, 2024

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/container-apps size/M upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR waiting-response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm_container_app - Does not allow multiple custom_domain blocks
8 participants