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.

Sign up for GitHub

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_custom_domain - fix parsing the certificate ID error #25972

Merged
merged 3 commits into from
Sep 20, 2024
Merged

azurerm_container_app_custom_domain - fix parsing the certificate ID error #25972

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
83c5939
fix gh issue 25788
sinbai May 15, 2024
2f58385
fix run bash ./scripts/fun-gradually-deprecated.sh error
sinbai May 16, 2024
69a0591
fix comments
sinbai May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 22 additions & 12 deletions internal/services/containerapps/container_app_custom_domain_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,11 +211,22 @@ func (a ContainerAppCustomDomainResource) Read() sdk.ResourceFunc {
state.Name = id.CustomDomainName
state.ContainerAppId = containerAppId.ID()
if pointer.From(v.CertificateId) != "" {
certId, err := managedenvironments.ParseCertificateIDInsensitively(pointer.From(v.CertificateId))
if err != nil {
return err
// The `v.CertificateId` returned from API has two possible values. when using an Azure created Managed Certificate,
// its format is "/subscriptions/%s/resourceGroups/%s/providers/Microsoft.App/managedEnvironments/%s/managedCertificates/%s",
// another format is "/subscriptions/%s/resourceGroups/%s/providers/Microsoft.App/managedEnvironments/%s/certificates/%s",
Comment on lines +220 to +222
Copy link
Contributor

Choose a reason for hiding this comment

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

These are two different Resource Types within the API - Microsoft.App/managedEnvironments/certificates and Microsoft.App/managedEnvironments/managedCertificates - so should be exposed as two different properties on our side.

Presumably that'd mean introducing a new property container_app_environment_managed_certificate_id to go alongside the existing container_app_environment_certificate_id - what's the reasoning for shoe-horning these into a single field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback. The code has been updated. Could you please take another look?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since it's also possible to create/delete/read Managed Certificates I suspect we'll need an associated data source/resource to manage those, so that this is usable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume that this parsing issue is introduced by support the ability to use Azure Managed Certificates PR. Per the PR's description "This is required to support the automatic creation of Azure Managed Certificates. This PR is in Draft as we are considering design options and may significantly change how this is implemented before inclusion in the provider", I assume that there may be a special reason for not using Managed Certificates API , although I don't know what the reason is. Now that TF already supports Managed Certificates through azurerm_container_app_custom_domain, is it possible to fix the parsing error to unlock the user first?

Copy link

Choose a reason for hiding this comment

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

I agree with @sinbai that this parsing error should be fixed first to support the user. We are using azurerm_container_app_custom_domain to suppress change on the certificate-related fields, so we can manually click through Azure portal to add the managed certificate; until managed certificate is supported in TF.

// both cases are handled here to avoid parsing error.
certificateId := ""
certId, err1 := managedenvironments.ParseCertificateIDInsensitively(pointer.From(v.CertificateId))
if err1 != nil {
managedCertId, err2 := managedenvironments.ParseManagedCertificateID(pointer.From(v.CertificateId))
if err2 != nil {
return err1
}
certificateId = managedCertId.ID()
} else {
certificateId = certId.ID()
}
state.CertificateId = certId.ID()
state.CertificateId = certificateId
}

state.BindingType = string(pointer.From(v.BindingType))
Expand Down Expand Up @@ -243,14 +254,6 @@ func (a ContainerAppCustomDomainResource) Delete() sdk.ResourceFunc {
return err
}

// attempt to lock the cert if we have the ID
if certIdRaw := metadata.ResourceData.Get("container_app_environment_certificate_id").(string); certIdRaw != "" {
if certId, err := managedenvironments.ParseCertificateID(certIdRaw); err == nil {
locks.ByID(certId.ID())
defer locks.UnlockByID(certId.ID())
}
}

containerAppId := containerapps.NewContainerAppID(id.SubscriptionId, id.ResourceGroupName, id.ContainerAppName)

containerApp, err := client.Get(ctx, containerAppId)
Expand All @@ -270,6 +273,13 @@ func (a ContainerAppCustomDomainResource) Delete() sdk.ResourceFunc {
for _, v := range *customDomains {
if !strings.EqualFold(v.Name, id.CustomDomainName) {
updatedCustomDomains = append(updatedCustomDomains, v)
} else {
// attempt to lock the cert if we have the ID
certificateId := pointer.From(v.CertificateId)
if certificateId != "" {
locks.ByID(certificateId)
defer locks.UnlockByID(certificateId)
}
}
}
}
Expand Down
Loading