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_frontdoor - expose support for cache_duration and cache_query_parameters fields #12831

Merged
merged 39 commits into from
Aug 13, 2021
Merged

azurerm_frontdoor - expose support for cache_duration and cache_query_parameters fields #12831

merged 39 commits into from
Aug 13, 2021

Conversation

heoelri
Copy link
Contributor

@heoelri heoelri commented Aug 3, 2021

This PR extends the azurerm_frontdoor resource with some additional caching capabilities.

  • cache_query_parameters (new) is a comma-separated list of query parameters
  • cache_duration (new) contains the caching duration (iso 8601)
  • cache_query_parameter_strip_directive can now also be set to StripAllExcept

The Azure Front Door API version was updated from 2020-01-01 to 2020-05-01 to support the newly added capabilities.

github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/frontdoor/validate is now imported as azValidate (aligned with other resources like servicebus) to avoid conflicts with github.com/terraform-providers/terraform-provider-azurerm/azurerm/helpers/validate (needed for some ISO8601 validation).

The two new parameters have been added to the documentation and location was dropped from the example and the details do now contain the information that this parameter has been deprecated.

This is my first "code" PR and i've some issues with the tests. make acctests fails:

=== CONT  TestAccFrontDoor_EnableDisableCache
    testcase.go:88: Step 1/5 error: Error running apply: exit status 1

        Error: creating Front Door "acctest-FD-210803200548354949" (Resource Group "acctestRG-frontdoor-210803200548354949"): frontdoor.FrontDoorsClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="BadRequest" Message="Invalid cache duration format specified. Cache duration conforms to the following format: `PnYnMnDTnHnMnS` as specified by the ISO 8601 standard."

          with azurerm_frontdoor.test,
          on terraform_plugin_test.tf line 18, in resource "azurerm_frontdoor" "test":
          18: resource "azurerm_frontdoor" "test" {

--- FAIL: TestAccFrontDoor_EnableDisableCache (215.29s)

The module itself works like a charm:

~ forwarding_configuration {
    + cache_duration                        = "P6M"
    ~ cache_enabled                         = false -> true
    ~ cache_query_parameter_strip_directive = "StripAll" -> "StripAllExcept"
    + cache_query_parameters                = "width,height"
      # (3 unchanged attributes hidden)
  }

image

It would be great if someone can take a look and help me to get this through.

Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

@heoelri, thank you so much for this PR. I have left one minor comment requesting the addition of validation checks in the frontDoorCustomizeDiff, but other than that this LGTM! 🚀

azurerm/internal/services/frontdoor/frontdoor_resource.go Outdated Show resolved Hide resolved
@heoelri heoelri marked this pull request as ready for review August 4, 2021 13:12
@heoelri
Copy link
Contributor Author

heoelri commented Aug 4, 2021

I was able to solve the issues with the acctests mentioned in my first comment in this PR.

All FrontDoor_* tests (including the new one) are now successful.

==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./azurerm/internal/services/frontdoor -run=TestAccFrontDoor_* -timeout 60m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccFrontDoorCustomHttpsConfiguration_CustomHttps
=== PAUSE TestAccFrontDoorCustomHttpsConfiguration_CustomHttps
=== RUN   TestAccFrontDoorCustomHttpsConfiguration_DisabledWithConfigurationBlock
=== PAUSE TestAccFrontDoorCustomHttpsConfiguration_DisabledWithConfigurationBlock
=== RUN   TestAccFrontDoorCustomHttpsConfiguration_EnabledWithoutConfigurationBlock
=== PAUSE TestAccFrontDoorCustomHttpsConfiguration_EnabledWithoutConfigurationBlock
=== RUN   TestAccFrontDoorCustomHttpsConfiguration_EnabledFrontdoorExtraAttributes
=== PAUSE TestAccFrontDoorCustomHttpsConfiguration_EnabledFrontdoorExtraAttributes
=== RUN   TestAccFrontDoorCustomHttpsConfiguration_EnabledKeyVaultLatest
=== PAUSE TestAccFrontDoorCustomHttpsConfiguration_EnabledKeyVaultLatest
=== RUN   TestAccFrontDoorCustomHttpsConfiguration_EnabledKeyVaultLatestMissingAttributes
=== PAUSE TestAccFrontDoorCustomHttpsConfiguration_EnabledKeyVaultLatestMissingAttributes
=== RUN   TestAccFrontDoorCustomHttpsConfiguration_EnabledKeyVaultMissingAttributes
=== PAUSE TestAccFrontDoorCustomHttpsConfiguration_EnabledKeyVaultMissingAttributes
=== RUN   TestAccFrontDoorFirewallPolicy_basic
=== PAUSE TestAccFrontDoorFirewallPolicy_basic
=== RUN   TestAccFrontDoorFirewallPolicy_requiresImport
=== PAUSE TestAccFrontDoorFirewallPolicy_requiresImport
=== RUN   TestAccFrontDoorFirewallPolicy_update
=== PAUSE TestAccFrontDoorFirewallPolicy_update
=== RUN   TestAccFrontDoorFirewallPolicy_complete
=== PAUSE TestAccFrontDoorFirewallPolicy_complete
=== RUN   TestAccFrontDoor_basic
=== PAUSE TestAccFrontDoor_basic
=== RUN   TestAccFrontDoor_global
=== PAUSE TestAccFrontDoor_global
=== RUN   TestAccFrontDoor_requiresImport
=== PAUSE TestAccFrontDoor_requiresImport
=== RUN   TestAccFrontDoor_update
=== PAUSE TestAccFrontDoor_update
=== RUN   TestAccFrontDoor_multiplePools
=== PAUSE TestAccFrontDoor_multiplePools
=== RUN   TestAccFrontDoor_complete
=== PAUSE TestAccFrontDoor_complete
=== RUN   TestAccFrontDoor_waf
=== PAUSE TestAccFrontDoor_waf
=== RUN   TestAccFrontDoor_EnableDisableCache
=== PAUSE TestAccFrontDoor_EnableDisableCache
=== CONT  TestAccFrontDoorCustomHttpsConfiguration_CustomHttps
=== CONT  TestAccFrontDoor_basic
=== CONT  TestAccFrontDoor_waf
=== CONT  TestAccFrontDoor_complete
=== CONT  TestAccFrontDoor_EnableDisableCache
=== CONT  TestAccFrontDoorCustomHttpsConfiguration_EnabledKeyVaultMissingAttributes
=== CONT  TestAccFrontDoorFirewallPolicy_complete
=== CONT  TestAccFrontDoor_multiplePools
--- PASS: TestAccFrontDoorCustomHttpsConfiguration_EnabledKeyVaultMissingAttributes (288.13s)
=== CONT  TestAccFrontDoorFirewallPolicy_update
--- PASS: TestAccFrontDoorFirewallPolicy_complete (320.24s)
=== CONT  TestAccFrontDoorFirewallPolicy_requiresImport
--- PASS: TestAccFrontDoor_complete (390.92s)
=== CONT  TestAccFrontDoorFirewallPolicy_basic
--- PASS: TestAccFrontDoor_multiplePools (391.03s)
=== CONT  TestAccFrontDoor_update
--- PASS: TestAccFrontDoor_waf (455.60s)
=== CONT  TestAccFrontDoor_requiresImport
--- PASS: TestAccFrontDoor_basic (613.44s)
=== CONT  TestAccFrontDoorCustomHttpsConfiguration_EnabledFrontdoorExtraAttributes
--- PASS: TestAccFrontDoorFirewallPolicy_requiresImport (298.46s)
=== CONT  TestAccFrontDoorCustomHttpsConfiguration_EnabledKeyVaultLatestMissingAttributes
--- PASS: TestAccFrontDoorFirewallPolicy_basic (276.16s)
=== CONT  TestAccFrontDoor_global
--- PASS: TestAccFrontDoorFirewallPolicy_update (546.48s)
=== CONT  TestAccFrontDoorCustomHttpsConfiguration_EnabledKeyVaultLatest
--- PASS: TestAccFrontDoorCustomHttpsConfiguration_EnabledFrontdoorExtraAttributes (287.56s)
=== CONT  TestAccFrontDoorCustomHttpsConfiguration_EnabledWithoutConfigurationBlock
--- PASS: TestAccFrontDoor_requiresImport (445.71s)
=== CONT  TestAccFrontDoorCustomHttpsConfiguration_DisabledWithConfigurationBlock
--- PASS: TestAccFrontDoorCustomHttpsConfiguration_EnabledKeyVaultLatestMissingAttributes (289.03s)
--- PASS: TestAccFrontDoor_EnableDisableCache (997.34s)
--- PASS: TestAccFrontDoor_global (370.06s)
--- PASS: TestAccFrontDoorCustomHttpsConfiguration_EnabledKeyVaultLatest (256.37s)
--- PASS: TestAccFrontDoorCustomHttpsConfiguration_EnabledWithoutConfigurationBlock (238.84s)
--- PASS: TestAccFrontDoorCustomHttpsConfiguration_DisabledWithConfigurationBlock (238.73s)
--- PASS: TestAccFrontDoor_update (1358.36s)
--- PASS: TestAccFrontDoorCustomHttpsConfiguration_CustomHttps (1765.48s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/frontdoor   1765.522s

[pull] master from terraform-providers:master
@heoelri heoelri requested a review from WodansSon August 6, 2021 06:18
heoelri and others added 2 commits August 6, 2021 09:43
[pull] master from terraform-providers:master
# Conflicts:
#	azurerm/internal/services/frontdoor/client/client.go
#	azurerm/internal/services/frontdoor/customizediff.go
#	azurerm/internal/services/frontdoor/frontdoor_custom_https_configuration.go
#	azurerm/internal/services/frontdoor/frontdoor_custom_https_configuration_resource.go
#	azurerm/internal/services/frontdoor/frontdoor_firewall_policy_resource.go
#	azurerm/internal/services/frontdoor/frontdoor_resource.go
#	azurerm/internal/services/frontdoor/frontdoor_resource_test.go
#	azurerm/internal/services/frontdoor/helper.go
Copy link
Collaborator

@WodansSon WodansSon left a comment

Choose a reason for hiding this comment

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

@heoelri, thanks for pushing those changes. This LGTM! 🚀

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

aside from one comment LGTM

* `cache_query_parameter_strip_directive` - (Optional) Defines cache behaviour in relation to query string parameters. Valid options are `StripAll` or `StripNone`. Defaults to `StripAll`.
* `cache_query_parameter_strip_directive` - (Optional) Defines cache behaviour in relation to query string parameters. Valid options are `StripAll`, `StripAllExcept`, `StripOnly` or `StripNone`. Defaults to `StripAll`.

* `cache_query_parameters` - (Optional) Specify query parameters (comma separated). Works only in combination with `cache_query_parameter_strip_directive` set to `StripAllExcept` or `StripOnly`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be an array of values rather then a string?

Copy link
Contributor Author

@heoelri heoelri Aug 12, 2021

Choose a reason for hiding this comment

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

@katbyte thank you, that's a good idea. In Front Door's models.go has QueryParameters the type *string that's why i've started as a string. But i agree that it's a much better idea to use an array here. I've made some changes to make query_parameters an array now.

image

@katbyte katbyte added this to the v2.72.0 milestone Aug 12, 2021
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

(hit wrong button)

@katbyte katbyte modified the milestones: v2.72.0, v2.73.0 Aug 12, 2021
heoelri and others added 6 commits August 12, 2021 11:17
Making the `query_parameters` parameter an array
update feature/extend-frontdoor-caching branch from main
@WodansSon WodansSon dismissed katbyte’s stale review August 13, 2021 05:19

user made suggested changes

@WodansSon
Copy link
Collaborator

@heoelri, thanks for pushing those changes. We have one test failure, if you can get that fixed up we can get this merged! 🚀

=== RUN   TestAccFrontDoor_EnableDisableCache
=== PAUSE TestAccFrontDoor_EnableDisableCache
=== CONT  TestAccFrontDoor_EnableDisableCache
testcase.go:88: Step 4/5 error: Check failed: Check 5/6 error: azurerm_frontdoor.test: Attribute 'routing_rule.0.forwarding_configuration.0.cache_query_parameters' not found
--- FAIL: TestAccFrontDoor_EnableDisableCache (366.73s)
FAIL

@heoelri
Copy link
Contributor Author

heoelri commented Aug 13, 2021

@heoelri, thanks for pushing those changes. We have one test failure, if you can get that fixed up we can get this merged! 🚀

=== RUN   TestAccFrontDoor_EnableDisableCache
=== PAUSE TestAccFrontDoor_EnableDisableCache
=== CONT  TestAccFrontDoor_EnableDisableCache
testcase.go:88: Step 4/5 error: Check failed: Check 5/6 error: azurerm_frontdoor.test: Attribute 'routing_rule.0.forwarding_configuration.0.cache_query_parameters' not found
--- FAIL: TestAccFrontDoor_EnableDisableCache (366.73s)
FAIL

Thank you very much for your help @WodansSon. The test was still looking for a string in cache_query_parameters instead of an array. I've updated the frontdoor_resource_test.go file as suggested.

==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/frontdoor -run=TestAccFrontDoor_EnableDisableCache -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccFrontDoor_EnableDisableCache
=== PAUSE TestAccFrontDoor_EnableDisableCache
=== CONT  TestAccFrontDoor_EnableDisableCache
--- PASS: TestAccFrontDoor_EnableDisableCache (489.44s)

@WodansSon WodansSon changed the title Enhance Azure Front Door caching with cache_duration and cache_query_parameters azurerm_frontdoor - expose support for cache_duration and cache_query_parameters fields Aug 13, 2021
@WodansSon
Copy link
Collaborator

@heoelri, thanks for pushing those changes again! LGTM and all the tests pass now! 🚀
image

@WodansSon WodansSon merged commit fa4c788 into hashicorp:main Aug 13, 2021
WodansSon added a commit that referenced this pull request Aug 13, 2021
@github-actions
Copy link

This functionality has been released in v2.73.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

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 Sep 20, 2021
@heoelri heoelri deleted the feature/extend-frontdoor-caching branch October 26, 2021 07:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants