[Dependency:] `azurerm_cdn_frontdoor_*` - update dependencies to use `2024-02-01` API #28233

WodansSon · 2024-12-10T07:56:29Z

Community Note

Please vote on this PR by adding a 👍 reaction to the original PR to help the community and maintainers prioritize for review
Please do not leave comments along the lines of "+1", "me too" or "any updates", they generate extra noise for PR followers and do not help prioritize for review

Description

This is the first step in implementing the new identity field and jschallenge field for CDN Front Door:

Data Source: azurerm_cdn_frontdoor_firewall_policy - update dependencies to use 2024-02-01 API
Data Source: azurerm_cdn_frontdoor_profile - update dependencies to use 2024-02-01 API

client - update dependencies to use 2024-02-01/profiles, 2024-02-01/securitypolicies and 2024-02-01/webapplicationfirewallpolicies API
azurerm_cdn_frontdoor_custom_domain - update dependencies to use 2024-02-01/profiles
azurerm_cdn_frontdoor_custom_domain - added a deprecated message for tls.minimum_tls_version property
azurerm_cdn_frontdoor_firewall_policy - update dependencies to use 2024-02-01 API
azurerm_cdn_frontdoor_origin - update dependencies to use 2024-02-01/profiles API
azurerm_cdn_frontdoor_profile - update dependencies to use 2024-02-01 API
azurerm_cdn_frontdoor_security_policy - update dependencies to use 2024-02-01, 2024-02-01/profiles and 2024-02-01/webapplicationfirewallpolicies API
cdn_frontdoor_shared_schema - update dependencies to use 2024-02-01/webapplicationfirewallpolicies and 2024-09-01/rulesAPI

Note: The API upgrades have been scoped specifically to target the resources to implement the new identity and mschallenge fields.

PR Checklist

I have followed the guidelines in our Contributing Documentation.
I have checked to ensure there aren't other open Pull Requests for the same update/change.
I have checked if my changes close any open issues. If so please include appropriate closing keywords below.
I have updated/added Documentation as required written in a helpful and kind way to assist users that may be unfamiliar with the resource / data source.
I have used a meaningful PR title to help maintainers and other users understand this change and help prevent duplicate work.
For example: “resource_name_here - description of change e.g. adding property new_property_name_here”

Changes to existing Resource / Data Source

I have added an explanation of what my changes do and why I'd like you to include them (This may be covered by linking to an issue above, but may benefit from additional explanation).
I have written new tests for my resource or datasource changes & updated any relevant documentation.
I have successfully run tests with my changes locally. If not, please provide details on testing challenges that prevented you running the tests.
(For changes that include a state migration only). I have manually tested the migration path between relevant versions of the provider.

Testing

My submission includes Test coverage as described in the Contribution Guide and the tests pass. (if this is not possible for any reason, please include details of why you did or could not add test coverage)

Change Log

Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.

Data Source: azurerm_cdn_frontdoor_firewall_policy - update dependencies to use 2024-02-01 API
Data Source: azurerm_cdn_frontdoor_profile - update dependencies to use 2024-02-01 API

./internal/services/cdn/client - update dependencies to use 2024-02-01/profiles API
./internal/services/cdn/client - update dependencies to use 2024-02-01/securitypolicies API
./internal/services/cdn/client - update dependencies to use 2024-02-01/webapplicationfirewallpolicies API
azurerm_cdn_frontdoor_custom_domain - update dependencies to use 2024-02-01/profiles
azurerm_cdn_frontdoor_custom_domain - added a deprecated message for tls.minimum_tls_version property
azurerm_cdn_frontdoor_firewall_policy - update dependencies to use 2024-02-01 API
azurerm_cdn_frontdoor_origin - update dependencies to use 2024-02-01/profiles API
azurerm_cdn_frontdoor_profile - update dependencies to use 2024-02-01 API
azurerm_cdn_frontdoor_security_policy - update dependencies to use 2024-02-01 API
azurerm_cdn_frontdoor_security_policy - update dependencies 2024-02-01/webapplicationfirewallpolicies API
azurerm_cdn_frontdoor_security_policy - update dependencies to use 2024-02-01/profiles API
cdn_frontdoor_shared_schema - update dependencies to use 2024-02-01/webapplicationfirewallpolicies API
cdn_frontdoor_shared_schema - update dependencies to use 2024-09-01/rules API

This is a (please select all that apply):

Note

If this PR changes meaningfully during the course of review please update the title and description as required.

jackofallops

Thanks @WodansSon - I wasn't sure if this was intended to cover all the resources migrating onto the go-azure-sdk, so I've assumed a partial scope for the review given the number of resources and complexity of change.

Looking good so far, I've called out a few pattern changes we've adopted as part of the migrations, particularly around Read functions because of the behaviour of the new SDKs. If you can take a look at the changes/comments below I'll loop back asap, also feel free to ping me offline if there are any questions.

Thanks!

jackofallops · 2024-12-10T10:06:35Z

internal/services/cdn/cdn_frontdoor_firewall_policy_data_source.go

+	if model.Sku == nil {
+		return fmt.Errorf("retrieving %s: 'model.Sku' was nil", id)
+	}


This is a behavioural change? lines 86-87 below just set this if it's not nil, rather than returning an error, we should probably keep this the same? something like:

Suggested change

if model.Sku == nil {

return fmt.Errorf("retrieving %s: 'model.Sku' was nil", id)

}

if sku := model.Sku; sku != nil {

d.Set("sku_name", pointer.From(sku.Name))

}

(maybe in the appropriate place below)

jackofallops · 2024-12-10T10:20:48Z

internal/services/cdn/cdn_frontdoor_firewall_policy_resource.go

 	if err != nil {
 		return err
 	}

-	existing, err := client.Get(ctx, id.ResourceGroup, id.FrontDoorWebApplicationFirewallPolicyName)
+	result, err := client.PoliciesGet(ctx, pointer.From(id))


While this isn't technically wrong, the ID Parse functions are nil-safe when error checked, so we're safe to use the dereference here and it's the standard pattern

Suggested change

result, err := client.PoliciesGet(ctx, pointer.From(id))

result, err := client.PoliciesGet(ctx, *id)

jackofallops · 2024-12-10T10:24:49Z

internal/services/cdn/cdn_frontdoor_firewall_policy_resource.go

+	if model.Sku == nil {
+		return fmt.Errorf("retrieving %s: 'model.Sku' was nil", *id)


Is it worth moving this check inside the d.HasChange("managed_rule") block below, it appears that's the only place it's used from the code here?

jackofallops · 2024-12-10T10:27:33Z

internal/services/cdn/cdn_frontdoor_firewall_policy_resource.go

+	if model.Sku == nil {
+		return fmt.Errorf("retrieving %s: 'model.Sku' was nil", *id)
+	}
+


As above, this looks like a behavioural change, so again I think we should maintain the previous behaviour by removing this and allowing the conditional set below to run?

Suggested change

if model.Sku == nil {

return fmt.Errorf("retrieving %s: 'model.Sku' was nil", *id)

}

jackofallops · 2024-12-10T10:29:23Z

internal/services/cdn/cdn_frontdoor_firewall_policy_resource.go

+	if sku := model.Sku; sku != nil {
+		skuName = string(pointer.From(sku.Name))
 	}
 	d.Set("sku_name", skuName)


This could be simplified to

Suggested change

if sku := model.Sku; sku != nil {

skuName = string(pointer.From(sku.Name))

}

d.Set("sku_name", skuName)

if sku := model.Sku; sku != nil {

d.Set("sku_name", string(pointer.From(sku.Name))

}

(and remove the skuName declaration above, GH won't let me "suggest" it here 🙄 )

jackofallops · 2024-12-10T12:22:01Z

internal/services/cdn/cdn_frontdoor_security_policy_resource.go

+	if profileModel.Sku.Name == nil {
+		return fmt.Errorf("profileModel.Sku.Name is 'nil'")
 	}


Do we need this since we're checking the value of this safely below in for the isStandardSku bool below?

Suggested change

if profileModel.Sku.Name == nil {

return fmt.Errorf("profileModel.Sku.Name is 'nil'")

}

I don't think we have to return the error here, but I do believe we should still check for nil , however I will just update the code to be like below to error on the side of caution if we cannot get the value for the sku:

isStandardSku := true if profileModel.Sku.Name != nil { isStandardSku = strings.HasPrefix(strings.ToLower(string(pointer.From(profileModel.Sku.Name))), "standard") }

I will also remove the check for the profile properties as they are not used in this function.

jackofallops · 2024-12-10T12:24:38Z

internal/services/cdn/cdn_frontdoor_security_policy_resource.go

+	if model == nil {
+		return fmt.Errorf("model is 'nil'")
+	}


Here too, for the if model := resp.Model; model != nil { d.Set() } pattern

jackofallops · 2024-12-10T12:31:16Z

internal/services/cdn/cdn_frontdoor_security_policy_resource.go

+
+		if id := v["cdn_frontdoor_domain_id"].(string); id != "" {
+			results = append(results, securitypolicies.ActivatedResourceReference{
+				Id: utils.String(id),


Suggested change

Id: utils.String(id),

Id: pointer.To(id),

jackofallops · 2024-12-10T12:33:57Z

internal/services/cdn/cdn_frontdoor_security_policy_resource.go

+		active := false
+		if item.IsActive != nil {
+			active = *item.IsActive
+		}
+
+		results = append(results, map[string]interface{}{
+			"active":                  active,
+			"cdn_frontdoor_domain_id": frontDoorDomainId,
+		})


I think we can simplify this to

Suggested change

active := false

if item.IsActive != nil {

active = *item.IsActive

}

results = append(results, map[string]interface{}{

"active": active,

"cdn_frontdoor_domain_id": frontDoorDomainId,

})

results = append(results, map[string]interface{}{

"active": pointer.From(item.IsActive),

"cdn_frontdoor_domain_id": frontDoorDomainId,

})

jackofallops · 2024-12-10T12:36:18Z

internal/services/cdn/cdn_frontdoor_shared_schema.go

+// NOTE: These const were taken from the 2021-06-01 API to remove the dependency on the legacy API.
+// SslProtocol enumerates the values for ssl protocol.


Feels like an oversight from this API, is it worth referring this to the service team to get these added in to this version?

I think they did include it in the new API, but I will need to import a different package to access it. In the old API everything was in the CDN package and now they have split them out. Let me see if I can find those values in the new API.

Additional:
I did find it, the type is now defined in the 2024-09-01/rules package.

jackofallops

Thanks @WodansSon for those changes. I spotted a handful of others this pass, but I think once they're covered we're good to go (maybe check the other Reads for the if model := resp.Model; model != nil {} In case I still missed any in my haste? 🙈 )

jackofallops · 2024-12-11T10:48:51Z

internal/services/cdn/cdn_frontdoor_firewall_policy_resource.go

-	future, err := client.CreateOrUpdate(ctx, id.ResourceGroup, id.FrontDoorWebApplicationFirewallPolicyName, existing)
+	model.Properties = pointer.To(props)
+
+	err = client.PoliciesCreateOrUpdateThenPoll(ctx, pointer.From(id), pointer.From(model))


These are nil safe at this point, so we should probably just use them directly as we do elsewhere in the provider.

Suggested change

err = client.PoliciesCreateOrUpdateThenPoll(ctx, pointer.From(id), pointer.From(model))

err = client.PoliciesCreateOrUpdateThenPoll(ctx, *id, *model)

jackofallops · 2024-12-11T10:49:45Z

internal/services/cdn/cdn_frontdoor_firewall_policy_resource.go

 	if err != nil {
 		return err
 	}

-	resp, err := client.Get(ctx, id.ResourceGroup, id.FrontDoorWebApplicationFirewallPolicyName)
+	result, err := client.PoliciesGet(ctx, pointer.From(id))


again, nil safe, so we usually use directly:

Suggested change

result, err := client.PoliciesGet(ctx, pointer.From(id))

result, err := client.PoliciesGet(ctx, *id)

internal/services/cdn/cdn_frontdoor_firewall_policy_data_source.go

jackofallops · 2024-12-12T08:11:26Z

internal/services/cdn/cdn_frontdoor_firewall_policy_resource.go

+	model := result.Model
+
+	if model == nil {
+		return fmt.Errorf("retrieving %s: 'model' was nil", *id)
+	}
+
+	if model.Properties == nil {
+		return fmt.Errorf("retrieving %s: 'model.Properties' was nil", *id)
+	}


Think I missed this on the last pass? Can we use the if model := result.Model; model != nil {} pattern here for the read? The d.Set() items from the ID can be performed outside this, ofc.

jackofallops · 2024-12-12T08:18:38Z

internal/services/cdn/cdn_frontdoor_origin_resource.go

+			if response.WasNotFound(profileResp.HttpResponse) {
+				d.SetId("")
+				return nil


This is unusual behaviour for an Update, if it's not found, Read on this resource (probably the parent actually) should have caught it and removed it from state? Is this a copy-paste error perhaps?

Suggested change

if response.WasNotFound(profileResp.HttpResponse) {

d.SetId("")

return nil

if response.WasNotFound(profileResp.HttpResponse) {

return fmt.Errorf("retrieving parent %s: not found", profileId)

May have been a CaP issue... Fixed.

jackofallops

Thanks @WodansSon - This LGTM now 👍
and the tests look good too:

Initial Check-in...

c1d5995

WodansSon requested a review from a team as a code owner December 10, 2024 07:56

github-actions bot added documentation dependencies service/cdn size/XL labels Dec 10, 2024

github-actions bot added the waiting-response label Dec 10, 2024

Fix lint errors...

9e457b0

github-actions bot removed the waiting-response label Dec 10, 2024

jackofallops requested changes Dec 10, 2024

View reviewed changes

github-actions bot added the waiting-response label Dec 10, 2024

Address PR comments...

3d5f220

github-actions bot removed the waiting-response label Dec 11, 2024

Include 2024-09-01/rules API for SslProtocol type definition...

bbff7bd

WodansSon marked this pull request as draft December 11, 2024 08:31

WodansSon added 2 commits December 11, 2024 17:44

Add webapplicationfirewallpolicies import to security policy resource...

39d4fd1

Remove resource id for deprecated resource...

2b75a3d

WodansSon marked this pull request as ready for review December 12, 2024 01:30

jackofallops requested changes Dec 12, 2024

View reviewed changes

github-actions bot added the waiting-response label Dec 12, 2024

Address latest PR comments...

10953b0

github-actions bot added waiting-response and removed waiting-response labels Dec 13, 2024

jackofallops approved these changes Dec 13, 2024

View reviewed changes

jackofallops merged commit e751874 into main Dec 13, 2024
37 checks passed

jackofallops deleted the d_frontdoor_2024-02-01 branch December 13, 2024 12:29

github-actions bot added this to the v4.15.0 milestone Dec 13, 2024

jackofallops added a commit that referenced this pull request Dec 13, 2024

Update CHANGELOG.md for #28233

d1623fd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Dependency:] `azurerm_cdn_frontdoor_*` - update dependencies to use `2024-02-01` API #28233

[Dependency:] `azurerm_cdn_frontdoor_*` - update dependencies to use `2024-02-01` API #28233

WodansSon commented Dec 10, 2024 •

edited

Loading

jackofallops left a comment

jackofallops Dec 10, 2024

WodansSon Dec 11, 2024

jackofallops Dec 10, 2024

WodansSon Dec 11, 2024

jackofallops Dec 10, 2024

WodansSon Dec 11, 2024

jackofallops Dec 10, 2024

WodansSon Dec 11, 2024

jackofallops Dec 10, 2024

WodansSon Dec 11, 2024

jackofallops Dec 10, 2024

WodansSon Dec 11, 2024 •

edited

Loading

jackofallops Dec 10, 2024

WodansSon Dec 11, 2024

jackofallops Dec 10, 2024

WodansSon Dec 11, 2024

jackofallops Dec 10, 2024

WodansSon Dec 11, 2024

jackofallops Dec 10, 2024

WodansSon Dec 11, 2024 •

edited

Loading

jackofallops left a comment

jackofallops Dec 11, 2024

WodansSon Dec 13, 2024

jackofallops Dec 11, 2024

WodansSon Dec 13, 2024

jackofallops Dec 12, 2024

WodansSon Dec 13, 2024

jackofallops Dec 12, 2024

WodansSon Dec 13, 2024 •

edited

Loading

jackofallops left a comment

	result, err := client.PoliciesGet(ctx, pointer.From(id))
	result, err := client.PoliciesGet(ctx, *id)

		if model.Sku == nil {
		return fmt.Errorf("retrieving %s: 'model.Sku' was nil", *id)

	if profileModel.Sku.Name == nil {
	return fmt.Errorf("profileModel.Sku.Name is 'nil'")
	}

		// NOTE: These const were taken from the 2021-06-01 API to remove the dependency on the legacy API.
		// SslProtocol enumerates the values for ssl protocol.

	err = client.PoliciesCreateOrUpdateThenPoll(ctx, pointer.From(id), pointer.From(model))
	err = client.PoliciesCreateOrUpdateThenPoll(ctx, id, model)

[Dependency:] azurerm_cdn_frontdoor_* - update dependencies to use 2024-02-01 API #28233

[Dependency:] azurerm_cdn_frontdoor_* - update dependencies to use 2024-02-01 API #28233

Conversation

WodansSon commented Dec 10, 2024 • edited Loading

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

jackofallops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WodansSon Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WodansSon Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

jackofallops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WodansSon Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

jackofallops left a comment

Choose a reason for hiding this comment

[Dependency:] `azurerm_cdn_frontdoor_*` - update dependencies to use `2024-02-01` API #28233

[Dependency:] `azurerm_cdn_frontdoor_*` - update dependencies to use `2024-02-01` API #28233

WodansSon commented Dec 10, 2024 •

edited

Loading

WodansSon Dec 11, 2024 •

edited

Loading

WodansSon Dec 11, 2024 •

edited

Loading

WodansSon Dec 13, 2024 •

edited

Loading