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

Merged
merged 1 commit into from
Jun 29, 2022
+4 −1
Merged

azurerm_static_site_custom_domain - make validation_type optional #15849

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion internal/services/web/static_site_custom_domain_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

ForceNew: true,
ValidateFunc: validation.StringInSlice([]string{
txtValidationType,
Expand Down Expand Up @@ -102,6 +102,9 @@ func resourceStaticSiteCustomDomainCreateOrUpdate(d *pluginsdk.ResourceData, met
}

validationMethod := d.Get("validation_type").(string)
if validationMethod == "" {
return fmt.Errorf("`validation_type` can't be empty string")
}

siteEnvelope := web.StaticSiteCustomDomainRequestPropertiesARMResource{
StaticSiteCustomDomainRequestPropertiesARMResourceProperties: &web.StaticSiteCustomDomainRequestPropertiesARMResourceProperties{
Expand Down