`azurerm_cdn_frontdoor_firewall_policy` - add support for `log_scrubbing` block by WodansSon · Pull Request #28834 · hashicorp/terraform-provider-azurerm

WodansSon · 2025-02-21T06:41:12Z

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

Adding support for the newly exposed log scrubbing feature exposed in the new Frontdoor WAF policies API.

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.

azurerm_cdn_frontdoor_firewall_policy - add support for the log_scrubbing code block [GH-00000]

This is a (please select all that apply):

Bug Fix
New Feature (ie adding a service, resource, or data source)
Enhancement
Breaking Change

Related Issue(s)

Fixes #28236

Note

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

jackofallops

Thanks @WodansSon - Just a few things to take a look at below if you could? Should be good to go when those are addressed (assuming tests pass, ofc!)

jackofallops · 2025-02-26T13:15:06Z

internal/services/cdn/cdn_frontdoor_firewall_policy_resource.go

+						"enabled": {
+							Type:     pluginsdk.TypeBool,
+							Optional: true,
+							Default:  true,
+						},
+
+						"rule": {
+							Type:     pluginsdk.TypeList,
+							MaxItems: 100,
+							Optional: true,
+							Elem: &pluginsdk.Resource{


Is there a default rule / behaviour here if log_scrubbing is enabled, but no rules are defined? Just wondering, if there's no default behaviour, if these should be Required / MinItems instead to ensure the block does something?

(The test cases suggest that your design is fine, but I wanted to be sure since it's a little confusing to have this without rules?)

Suggested change

"enabled": {

Type: pluginsdk.TypeBool,

Optional: true,

Default: true,

},

"rule": {

Type: pluginsdk.TypeList,

MaxItems: 100,

Optional: true,

Elem: &pluginsdk.Resource{

"enabled": {

Type: pluginsdk.TypeBool,

Required: true,

},

"rule": {

Type: pluginsdk.TypeList,

MaxItems: 100,

Minitems: 1,

Elem: &pluginsdk.Resource{

No, there isn't a default behavior, per the service team:

Correction on the behavior of the LogScrubbing state.

If LogScrubbing State is set to Enabled but no scrubbing rules are configured, it will result in a no op (no operation).

For log scrubbing to take effect, both the LogScrubbing State (Enabled) and at least one Scrubbing Rule with State Enabled must be configured.

jackofallops · 2025-02-26T13:25:46Z

internal/services/cdn/cdn_frontdoor_firewall_policy_resource.go

+			// NOTE: If the 'operator' is set to 'Equals' the 'selector' cannot be 'nil'...
+			if *item.Selector == "" {


This looks a little off? We're checking for empty, not nil here? To avoid a panic, should this be:

Suggested change

// NOTE: If the 'operator' is set to 'Equals' the 'selector' cannot be 'nil'...

if *item.Selector == "" {

// NOTE: If the 'operator' is set to 'Equals' the 'selector' cannot be 'nil'...

if pointer.From(item.Selector) == "" {

since we're only setting item.Selector if there's a v["selector"] value present?

jackofallops · 2025-02-26T13:27:19Z

internal/services/cdn/cdn_frontdoor_firewall_policy_resource.go

+			// NOTE: If the 'operator' is set to 'EqualsAny' the 'selector' must be 'nil'...
+			if *item.Selector != "" {


Again here, should this be?

Suggested change

// NOTE: If the 'operator' is set to 'EqualsAny' the 'selector' must be 'nil'...

if *item.Selector != "" {

// NOTE: If the 'operator' is set to 'EqualsAny' the 'selector' must be 'nil'...

if item.Selector == nil {

jackofallops · 2025-02-26T13:29:55Z

internal/services/cdn/cdn_frontdoor_firewall_policy_resource_test.go

 }

+func TestAccCdnFrontDoorFirewallPolicy_logScrubbingComplete(t *testing.T) {
+	// NOTE: Regression test case for issue #19088


copy paste error?

Suggested change

// NOTE: Regression test case for issue #19088

Yes sir... DOH! 🤣 Fixed.

jackofallops · 2025-02-26T13:35:14Z

internal/services/cdn/cdn_frontdoor_firewall_policy_resource_test.go

+	tmp := r.template(data)
+	return fmt.Sprintf(`
+%s
+
+resource "azurerm_cdn_frontdoor_firewall_policy" "test" {
+  name                = "accTestWAF%d"
+  resource_group_name = azurerm_resource_group.test.name
+  sku_name            = azurerm_cdn_frontdoor_profile.test.sku_name
+  mode                = "Prevention"
+
+  log_scrubbing {
+    enabled = true
+  }
+}
+`, tmp, data.RandomInteger)


We're pushing away from this pattern as it's unnecessary to assign the template to a var when it can be passed into the test directly. Would you mind updating these? (Just the new tests, don't worry about the others in the file, we'll be sorting this out more broadly in the future, just want to avoid more instances of it where possible. 🙈 )

Suggested change

tmp := r.template(data)

return fmt.Sprintf(`

%s

resource "azurerm_cdn_frontdoor_firewall_policy" "test" {

name = "accTestWAF%d"

resource_group_name = azurerm_resource_group.test.name

sku_name = azurerm_cdn_frontdoor_profile.test.sku_name

mode = "Prevention"

log_scrubbing {

enabled = true

}

}

`, tmp, data.RandomInteger)

return fmt.Sprintf(`

%s

resource "azurerm_cdn_frontdoor_firewall_policy" "test" {

name = "accTestWAF%d"

resource_group_name = azurerm_resource_group.test.name

sku_name = azurerm_cdn_frontdoor_profile.test.sku_name

mode = "Prevention"

log_scrubbing {

enabled = true

}

}

`, r.template(data), data.RandomInteger)

Since I will already be in the code I might as well just update all of them. Fixed.

website/docs/r/cdn_frontdoor_firewall_policy.html.markdown

jackofallops · 2025-02-26T13:37:45Z

website/docs/r/cdn_frontdoor_firewall_policy.html.markdown

+
+* `enabled` - (Optional) Is log scrubbing enabled? Possible values are `true` or `false`. Defaults to `true`.
+
+* `rule` - (Optional) One or more `scrubbing_rule` blocks as defined below.


Suggested change

* `rule` - (Optional) One or more `scrubbing_rule` blocks as defined below.

* `rule` - (Optional) One or more `rule` blocks as defined below.

Same as above, the managed_rule also uses rule so I gave it a different name in the documentation trying to not confuse the end user when they are looking up what the scrubbing_rule block contains.

jackofallops

Thanks @WodansSon - Just a few things to take a look at below if you could? Should be good to go when those are addressed (assuming tests pass, ofc!)

WodansSon · 2025-02-28T04:40:05Z

Comments have been addressed...

jackofallops

Thanks for the changes/responses @WodansSon - Given the external limitation, I don't think we will have to suck up a deviation from naming convention for now. I think once that's done, this should be good to go. Sorry for the back-and-forth.

website/docs/r/cdn_frontdoor_firewall_policy.html.markdown

jackofallops · 2025-03-04T12:25:20Z

internal/services/cdn/cdn_frontdoor_firewall_policy_resource.go

+							Default:  true,
+						},
+
+						"rule": {


As noted in the docs, if this doesn't match it will be difficult/frustrating for users to work out the correct block name. Since this is a limitation of the Registry docs, I don't think we have a choice here but to deviate from the convention due to the duplication of the block name in this resource.

Suggested change

"rule": {

"scrubbing_rule": {

WodansSon · 2025-03-06T08:11:41Z

jackofallops

Thanks @WodansSon for the changes, this LGTM now 👍

* Update CHANGELOG.md #28843 * Update CHANGELOG.md for #28748 * Update CHANGELOG.md for #28881 * Update CHANGELOG.md for #28909 * Update CHANGELOG.md fold in patch release and expand starred entry correctly * Update CHANGELOG.md remove duplicate entry for 28843 * Update CHANGELOG.md #28066 * Update CHANGELOG.md for #28902 and fix ordering * Update CHANGELOG.md for #28848 * Update CHANGELOG.md #27622 * Update CHANGELOG.md for #28271 * Update CHANGELOG.md update format * Update CHANGELOG.md #28934 * Update for #28619 #28598 * Update CHANGELOG.md #28940 * Update CHANGELOG.md for #28880 * Update CHANGELOG.md for #28617 * Update CHANGELOG.md #28721 * Update CHANGELOG.md #28949 * Update previous entry for 4.16.0 * Update CHANGELOG.md for #28834 * prep for release --------- Co-authored-by: Matthew Frahry <mbfrahry@gmail.com> Co-authored-by: jackofallops <11830746+jackofallops@users.noreply.github.com> Co-authored-by: catriona-m <86247157+catriona-m@users.noreply.github.com> Co-authored-by: stephybun <steph@hashicorp.com> Co-authored-by: Wodans Son <20408400+WodansSon@users.noreply.github.com> Co-authored-by: jackofallops <ste@hashicorp.com>

github-actions · 2025-04-06T02:20:38Z

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.

Initial Check-in...

fea5cfc

WodansSon added documentation service/cdn labels Feb 21, 2025

WodansSon requested a review from a team as a code owner February 21, 2025 06:41

github-actions bot added the size/M label Feb 21, 2025

WodansSon marked this pull request as draft February 21, 2025 06:42

WodansSon added 3 commits February 21, 2025 00:37

Add Log Rule examples table...

3645ceb

Remove tabs from Log rule table...

3415fc9

Removing more missed tabs... DOH!

4b4481d

github-actions bot added the enhancement label Feb 21, 2025

WodansSon added 3 commits February 21, 2025 01:24

Add comments in schema explaining the schema definition...

71729b5

Update notes to be more clear...

b362747

Check-in progress...

206cc12

github-actions bot added size/L waiting-response and removed size/M labels Feb 22, 2025

Rename 'log_rule' to 'scrubbing_rule'...

8818a3a

github-actions bot removed the waiting-response label Feb 22, 2025

Update docs to use 'scrubbing_rule' instead of 'log_rule'...

a5ad252

github-actions bot added the waiting-response label Feb 22, 2025

Fix text in error messages to reflect 'scrubbing_rule'...

352a865

github-actions bot added waiting-response and removed waiting-response labels Feb 22, 2025

Fix lint error...

2e431d3

github-actions bot added waiting-response and removed waiting-response labels Feb 22, 2025

gofumpt -w...

8f2b752

github-actions bot removed the waiting-response label Feb 22, 2025

WodansSon added 2 commits February 23, 2025 02:37

Tweak expand and flatten functions...

b58ba37

Add test cases...

e9e9767

WodansSon added this to the v4.21.0 milestone Feb 24, 2025

WodansSon assigned jackofallops Feb 24, 2025

WodansSon added 2 commits February 24, 2025 00:23

Remove some comments in the schema...

b765aa8

Update log scrubbing note in documentation...

9f82a30

johnbailey-transactcampus approved these changes Feb 26, 2025

View reviewed changes

jackofallops requested changes Feb 26, 2025

View reviewed changes

jackofallops previously requested changes Feb 26, 2025

View reviewed changes

github-actions bot added the waiting-response label Feb 26, 2025

Address PR comments...

dc90542

github-actions bot removed the waiting-response label Feb 27, 2025

WodansSon added 3 commits February 27, 2025 01:29

Update documentation...

da2892d

Mark logScrubbing rule as required...

5c78021

Fix SKU downgrade error test case...

a4b25de

Address test comment...

fea721b

WodansSon modified the milestones: v4.21.0, v4.22.0 Mar 4, 2025

jackofallops requested changes Mar 4, 2025

View reviewed changes

github-actions bot added the waiting-response label Mar 4, 2025

update rule to scrubbing_rule...

f4517c9

github-actions bot removed the waiting-response label Mar 6, 2025

jackofallops approved these changes Mar 6, 2025

View reviewed changes

WodansSon merged commit e66c290 into main Mar 6, 2025
35 checks passed

WodansSon deleted the e_frontdoor_log_scrubbing branch March 6, 2025 23:32

WodansSon added a commit that referenced this pull request Mar 6, 2025

Update CHANGELOG.md for #28834

0f455c0

github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2025

		// NOTE: If the 'operator' is set to 'Equals' the 'selector' cannot be 'nil'...
		if *item.Selector == "" {

		// NOTE: If the 'operator' is set to 'EqualsAny' the 'selector' must be 'nil'...
		if *item.Selector != "" {


		* `enabled` - (Optional) Is log scrubbing enabled? Possible values are `true` or `false`. Defaults to `true`.

		* `rule` - (Optional) One or more `scrubbing_rule` blocks as defined below.

	* `rule` - (Optional) One or more `scrubbing_rule` blocks as defined below.
	* `rule` - (Optional) One or more `rule` blocks as defined below.

Conversation

WodansSon commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Community Note

Description

PR Checklist

Changes to existing Resource / Data Source

Testing

Change Log

Related Issue(s)

Uh oh!

jackofallops left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WodansSon Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WodansSon Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jackofallops left a comment

Choose a reason for hiding this comment

Uh oh!

WodansSon commented Feb 28, 2025

Uh oh!

jackofallops left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WodansSon commented Mar 6, 2025

Uh oh!

jackofallops left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Apr 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

WodansSon commented Feb 21, 2025 •

edited

Loading

WodansSon Feb 27, 2025 •

edited

Loading

WodansSon Feb 27, 2025 •

edited

Loading