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

[Enhancement:] azurerm_cdn_frontdoor_firewall_policy - js_challenge_cookie_expiration_in_minutes causing BadRequest error with Standard_AzureFrontDoor SKU #28726

Merged
merged 16 commits into from
Feb 13, 2025

Conversation

WodansSon
Copy link
Collaborator

@WodansSon WodansSon commented Feb 8, 2025

  • azurerm_cdn_frontdoor_firewall_policy - added sku_name downgrade validation
  • azurerm_cdn_frontdoor_profile - added sku_name downgrade validation

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

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)

image

Change Log

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

BUG FIXES:

  • azurerm_cdn_frontdoor_firewall_policy - fixed an issue where the js_challenge_cookie_expiration_in_minutes policies default value was causing Standard_AzureFrontDoor skus to receive a BadRequest error [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 PR(s)

#28717
#28750

Related Issue(s)

Fixes #28716
Fixes #28713

Note

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

@WodansSon WodansSon changed the title [Enhancement:] Address issue of new field js_challenge_cookie_expiration_in_minutes causing issues with Standard_AzureFrontDoor SKUs [Enhancement:] azurerm_cdn_frontdoor_firewall_policy - Address issue of new field js_challenge_cookie_expiration_in_minutes causing issues with Standard_AzureFrontDoor SKUs Feb 8, 2025
@WodansSon WodansSon changed the title [Enhancement:] azurerm_cdn_frontdoor_firewall_policy - Address issue of new field js_challenge_cookie_expiration_in_minutes causing issues with Standard_AzureFrontDoor SKUs [Enhancement:] azurerm_cdn_frontdoor_firewall_policy - js_challenge_cookie_expiration_in_minutes causing BadRequest error with Standard_AzureFrontDoor SKU Feb 9, 2025
@WodansSon WodansSon marked this pull request as ready for review February 10, 2025 06:34
@WodansSon WodansSon added this to the v4.19.0 milestone Feb 10, 2025
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Thanks @WodansSon - Just a few items to take a look at, feel free to ping me in the off-line thread on these as needed as I know it's time sensitive (and I get so many GH notifications I'd not want to miss a reply 🙈)

Comment on lines 794 to 796
if *model.Sku.Name == waf.SkuNamePremiumAzureFrontDoor && policy.JavascriptChallengeExpirationInMinutes != nil {
d.Set("js_challenge_cookie_expiration_in_minutes", int(pointer.From(policy.JavascriptChallengeExpirationInMinutes)))
}
Copy link
Member

Choose a reason for hiding this comment

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

Since this is now O+C, should we not always set it? (relating to the comment on the schema default above...)

Copy link
Collaborator Author

@WodansSon WodansSon Feb 12, 2025

Choose a reason for hiding this comment

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

I don't think so, because if the resource is a Standard sku the javascriptChallengeExpirationInMinutes field will be nil, it will only have a value if it is a Premium sku, so I think some sort of filtering logic needs to be applied here. I have updated the logic in the Read function to only write the value of the javascriptChallengeExpirationInMinutes field to the state file if the javascriptChallengeExpirationInMinutes field is not nil. That said, I could flip the logic to always write the value if the sku is not Standard or to always write the value if the sku is Premium, at the end of the day it's six of one half dozen of the other, it kind of all works out in the wash whichever way we go. I am open any of the above options...

Fixed.

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @WodansSon - This LGTM now 👍
Tests are all green too:
image

@WodansSon WodansSon merged commit e17bb26 into main Feb 13, 2025
37 checks passed
@WodansSon WodansSon deleted the f_frontdoor_jschallenge_policy_sku branch February 13, 2025 07:11
WodansSon added a commit that referenced this pull request Feb 13, 2025
jackofallops added a commit that referenced this pull request Feb 14, 2025
* CHANGELOG.md for v4.19.0

* Update CHANGELOG.md #28523

* Update CHANGELOG.md #28691

* Updated to include #28717

* Update for #26680

* Update CHANGELOG.md #28633

* Update CHANGELOG.md for #28703

* Update CHANGELOG.md for #28391

* Update CHANGELOG.md #28725

* Update #28733

* Update CHANGELOG.md #28659

* Update for #28741

* Update CHANGELOG.md #28712

* Update CHANGELOG.md #28441

* Update CHANGELOG.md #28441

* Update CHANGELOG.md #28441

* Update CHANGELOG.md for #28602

* Update for #27424

* Update CHANGELOG.md for #28524

* Update CHANGELOG.md #28726

* Update for #28767

* Update for #28195

* prep for release v4.19.0

---------

Co-authored-by: sreallymatt <[email protected]>
Co-authored-by: Wodans Son <[email protected]>
Co-authored-by: stephybun <[email protected]>
Co-authored-by: Wyatt Fry <[email protected]>
Co-authored-by: Matthew Frahry <[email protected]>
Co-authored-by: jackofallops <[email protected]>
hqhqhqhqhqhqhqhqhqhqhq pushed a commit to hqhqhqhqhqhqhqhqhqhqhq/terraform-provider-azurerm that referenced this pull request Feb 26, 2025
* Initial Check-In...

* Fix lint error...

* Update test case names...

* Mostly working now, need to fix one test...

* Fix lint error...

* Add test cases and CustomizeDiffShim...

* Add custom_rule definition to jsChallengePolicyStandardSku test case...

* Fix error return value...

* Slightly tweak documentation and expose a CustomizeDiffShim for JSChallenge policy validation...

* Add test case for invalid JSChallenge Policy...

* Update tests...

* Update documentation...

* Format documentation...

* Address PR comments...
hqhqhqhqhqhqhqhqhqhqhq pushed a commit to hqhqhqhqhqhqhqhqhqhqhq/terraform-provider-azurerm that referenced this pull request Feb 26, 2025
* CHANGELOG.md for v4.19.0

* Update CHANGELOG.md hashicorp#28523

* Update CHANGELOG.md hashicorp#28691

* Updated to include hashicorp#28717

* Update for hashicorp#26680

* Update CHANGELOG.md hashicorp#28633

* Update CHANGELOG.md for hashicorp#28703

* Update CHANGELOG.md for hashicorp#28391

* Update CHANGELOG.md hashicorp#28725

* Update hashicorp#28733

* Update CHANGELOG.md hashicorp#28659

* Update for hashicorp#28741

* Update CHANGELOG.md hashicorp#28712

* Update CHANGELOG.md hashicorp#28441

* Update CHANGELOG.md hashicorp#28441

* Update CHANGELOG.md hashicorp#28441

* Update CHANGELOG.md for hashicorp#28602

* Update for hashicorp#27424

* Update CHANGELOG.md for hashicorp#28524

* Update CHANGELOG.md hashicorp#28726

* Update for hashicorp#28767

* Update for hashicorp#28195

* prep for release v4.19.0

---------

Co-authored-by: sreallymatt <[email protected]>
Co-authored-by: Wodans Son <[email protected]>
Co-authored-by: stephybun <[email protected]>
Co-authored-by: Wyatt Fry <[email protected]>
Co-authored-by: Matthew Frahry <[email protected]>
Co-authored-by: jackofallops <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment