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

Jump to bottom

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

Show file tree
Hide file tree
Changes from all 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
48 changes: 31 additions & 17 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 @@ -28,10 +28,11 @@ type ContainerAppCustomDomainResource struct{}
var _ sdk.Resource = ContainerAppCustomDomainResource{}

type ContainerAppCustomDomainResourceModel struct {
Name string `tfschema:"name"`
ContainerAppId string `tfschema:"container_app_id"`
CertificateId string `tfschema:"container_app_environment_certificate_id"`
BindingType string `tfschema:"certificate_binding_type"`
Name string `tfschema:"name"`
ContainerAppId string `tfschema:"container_app_id"`
CertificateId string `tfschema:"container_app_environment_certificate_id"`
BindingType string `tfschema:"certificate_binding_type"`
ManagedCertificateId string `tfschema:"container_app_environment_managed_certificate_id"`
}

func (a ContainerAppCustomDomainResource) Arguments() map[string]*pluginsdk.Schema {
Expand Down Expand Up @@ -70,7 +71,12 @@ func (a ContainerAppCustomDomainResource) Arguments() map[string]*pluginsdk.Sche
}

func (a ContainerAppCustomDomainResource) Attributes() map[string]*pluginsdk.Schema {
return map[string]*pluginsdk.Schema{}
return map[string]*pluginsdk.Schema{
"container_app_environment_managed_certificate_id": {
Type: pluginsdk.TypeString,
Computed: true,
},
}
}

func (a ContainerAppCustomDomainResource) ModelObject() interface{} {
Expand Down Expand Up @@ -211,11 +217,20 @@ 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.
certId, err1 := managedenvironments.ParseCertificateIDInsensitively(pointer.From(v.CertificateId))
if err1 != nil {
managedCertId, err2 := managedenvironments.ParseManagedCertificateID(pointer.From(v.CertificateId))
if err2 != nil {
return err1
}
state.ManagedCertificateId = managedCertId.ID()
} else {
state.CertificateId = certId.ID()
}
state.CertificateId = certId.ID()
}

state.BindingType = string(pointer.From(v.BindingType))
Expand Down Expand Up @@ -243,14 +258,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 +277,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
6 changes: 6 additions & 0 deletions website/docs/r/container_app_custom_domain.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,12 @@ The following arguments are supported:

!> **NOTE:** If using an Azure Managed Certificate `container_app_environment_certificate_id` and `certificate_binding_type` should be added to `ignore_changes` to prevent resource recreation due to these values being modified asynchronously outside of Terraform.

## Attributes Reference

In addition to the Arguments listed above - the following Attributes are exported:

* `container_app_environment_managed_certificate_id` - The ID of the Container App Environment Managed Certificate to use.

## Timeouts

The `timeouts` block allows you to specify [timeouts](https://www.terraform.io/docs/configuration/resources.html#timeouts) for certain actions:
Expand Down
Loading