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_static_site_custom_domain - make validation_type optional #15849

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

magodo
Copy link
Collaborator

@magodo magodo commented Mar 16, 2022

This is to allow the imported azurerm_static_site_custom_domain not suffering force replacement issue.

Fixes #15837.

This is to allow the imported `azurerm_static_site_custom_domain` not suffering force replacement issue. Fixes #15837.
@@ -55,7 +55,7 @@ func resourceStaticSiteCustomDomain() *pluginsdk.Resource {

"validation_type": {
Type: pluginsdk.TypeString,
Required: true,
Optional: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

rather than doing this - we should instead update the Import function to set this field based on the value from the config

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tombuildsstuff During the import, you don't have access to the config (i.e. d.GetRawConfig returns a cty value whose internal value is nil). Also, this is the same case for terraform-plugin-framework, where the tfsdk.ImportResourceStateRequest only export the identifier which users have specified in the CLI. If we change the resource id into form like <azure resource id>:<validation type>, then it should work. However, this seems weired.

Copy link
Contributor

@tombuildsstuff tombuildsstuff Mar 17, 2022

Choose a reason for hiding this comment

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

@magodo we do this just fine in some other resources, it depends what's in the config at the time of import, as such it'll need to be defaulted but can override from the config as needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm assuming you mean this. But if I have following import code:

Importer: pluginsdk.ImporterValidatingResourceIdThen(
	func(id string) error {
		_, err := parse.StaticSiteCustomDomainID(id)
		return err
	},
	func(ctx context.Context, d *pluginsdk.ResourceData, meta interface{}) ([]*pluginsdk.ResourceData, error) {
		v := d.Get("validation_type")
		_ = v
		cfg := d.GetRawConfig()
		_ = cfg
		return []*pluginsdk.ResourceData{d}, nil
	},
),

And have following config in the workspace:

provider "azurerm" {
  features {}
}
resource "azurerm_resource_group" "test" {
  name     = "acctestRG-220316110941683707"
  location = "WestEurope"
}
resource "azurerm_static_site" "test" {
  name                = "acctestSS-220316110941683707"
  location            = azurerm_resource_group.test.location
  resource_group_name = azurerm_resource_group.test.name
}
resource "azurerm_static_site_custom_domain" "test" {
  static_site_id  = azurerm_static_site.test.id
  domain_name     = "ranran2.biz"
  validation_type = "dns-txt-token"
}

Then inspect at the last line of the Importer, I get:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Unsure what the status is here, reading the config while importing isn't possible then?

Copy link
Collaborator Author

@magodo magodo Apr 1, 2022

Choose a reason for hiding this comment

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

@fvdnabee Yes, for the terraform framwork SDK, it is clearly documented: https://www.terraform.io/plugin/framework/resources#read (read is similar to import)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for that link @magodo, reading the tf config during import appears to be documented as unsupported indeed.

Considering validation_type is not part of the state of the SWA customDomain Azure resource state [1], should it be tracked as part of the terraform state? Couldn't it be removed from the TF state altogether?

Once the domain is available, it seems irrelevant whether it was validated via a CNAME record or TXT record (and Azure appears to not be tracking the validation_type; see [1]). If the domain validation fails, then the custom domain resource fails to be created; so on the next tf apply it would be recreated and the provider can just read the validation_type from the config again. validation_type also does not seem to impact resource destruction. Maybe the validation type can just be read from the config when creating/updating the resource and it could be removed from the tf state? I'm unsure if this is feasible in the provider however.

[1] this is what azurerm returns for an active custom domain; note the validation type is not tracked:

❯ az rest -m get -u 'https://management.azure.com//subscriptions/REDACTED/resourceGroups/REDACTED/providers/Microsoft.Web/staticSites/stapp-d-10007318-0/customDomains/example.com?api-version=2021-02-01'
{
  "id": "/subscriptions/REDACTED/resourceGroups/REDACTED/providers/Microsoft.Web/staticSites/stapp-d-10007318-0/customDomains/example.com",
  "location": "West Europe",
  "name": "example.com",
  "properties": {
    "createdOn": "2022-03-14T15:49:02.5454741",
    "domainName": "example.com",
    "isDefault": false,
    "status": "Ready"
  },
  "type": "Microsoft.Web/staticSites/customDomains"
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fvdnabee Thanks for the clarification! Given the validation_type is not returned in GET, hence we didn't set it to state in read. In case the custom domain is created via Terraform, it is still set in the state during create as it is in the config. On the other hand, if it is imported, then the state will never have validation_type, which means you shouldn't specify it in the config as well, to avoid the diff.

That said, the validation_type is only useful during creation, and should be ignored in other cases. So one solution is to make the validation_type optional, but still mark it as required in the document to indicate that it is required during create, as this PR did. Another way to workaround this is to use the ignore_chanages to explicitly ignore it after you imported it.

Copy link
Contributor

@fvdnabee fvdnabee Apr 1, 2022

Choose a reason for hiding this comment

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

@magodo could you clarify the following:

In case the custom domain is created via Terraform, it is still set in the state during create as it is in the config.

This is a necessary requirement for terraform providers? Including the validation type in the resource creation request forces us to track it as part of the TF state? I understand it is impossible (or bad practice?) to include parameters in the resource creation request that are sourced from the TF config without them being tracked in the TF state?

I like the optional validation_type idea for importing (then it wouldn't be part of the TF state), but when creating a custom domain resourced it should indeed be clear that validation_type is a required argument. Is it possible to enforce this in the provider (i.e. validation_type must be specified when creating the resource; if validation_type is absent during creation the provider throws an error)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, provider will fail if validation_type is not set in creation.

For the reason why it is set in the state during creation, it is how the plugin SDKv2 works. The values in the config will be in the state if don't explicitly change them.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@tombuildsstuff tombuildsstuff added this to the v3.12.0 milestone Jun 29, 2022
@tombuildsstuff tombuildsstuff merged commit 52ce02f into main Jun 29, 2022
@tombuildsstuff tombuildsstuff deleted the static_site_custom_domain_validation_type branch June 29, 2022 16:47
tombuildsstuff added a commit that referenced this pull request Jun 29, 2022
@github-actions
Copy link

github-actions bot commented Jul 1, 2022

This functionality has been released in v3.12.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

github-actions bot commented Aug 1, 2022

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 Aug 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants