[Bug:] `azurerm_cdn_frontdoor_rule` - allow the `cdn_frontdoor_origin_group_id` field to be optional in the `route_configuration_override_action`, expose `Disabled` as a possible value of `cache_behavior` #18906

WodansSon · 2022-10-21T07:27:01Z

Optional `cdn_frontdoor_origin_group_id`:

actions {
  route_configuration_override_action {
    query_string_caching_behavior = "IncludeSpecifiedQueryStrings"
    query_string_parameters       = ["foo", "clientIp={client_ip}"]
    compression_enabled           = true
    cache_behavior                = "OverrideIfOriginMissing"
    cache_duration                = "365.23:59:59"
  }
}

NOTE:	If you do not define the `cdn_frontdoor_origin_group_id` you cannot define `forwarding_protocol` as this would result in an error from the API.

(fixes #18889)

Disable Cache Without the Origin Group ID:

  actions {
    route_configuration_override_action {
      cache_behavior = "Disabled"
    }
  }

Disable Cache With the Origin Group ID:

  actions {
    route_configuration_override_action {
      cdn_frontdoor_origin_group_id = azurerm_cdn_frontdoor_origin_group.example.id
      forwarding_protocol           = "HttpsOnly"
      cache_behavior                = "Disabled"
    }
  }

(fixes #19008)

stephybun

Thanks @WodansSon, unfortunately there's a test failure

------- Stdout: -------
=== RUN   TestAccCdnFrontDoorRule_basicRegression18889
=== PAUSE TestAccCdnFrontDoorRule_basicRegression18889
=== CONT  TestAccCdnFrontDoorRule_basicRegression18889
testcase.go:110: Step 1/2 error: Error running apply: exit status 1
Error: expanding 'actions': the 'route_configuration_override_action' block is not valid, 'forwarding_protocol' can not be defined if the 'cdn_frontdoor_origin_group_id' is not set, got "HttpsOnly"
with azurerm_cdn_frontdoor_rule.test,
on terraform_plugin_test.tf line 49, in resource "azurerm_cdn_frontdoor_rule" "test":
49: resource "azurerm_cdn_frontdoor_rule" "test" {
--- FAIL: TestAccCdnFrontDoorRule_basicRegression18889 (168.80s)
FAIL

WodansSon · 2022-10-23T04:33:18Z

@stephybun, sorry about that I thought I marked this as in progress, but thank you for the quick review! 🚀 I think this PR is actually ready for a review now. 🙂

WodansSon · 2022-10-24T08:22:51Z

My test finally ran after over 5 hours of waiting, it appears I need to add some extra validation around the input values during create... 😭

WodansSon · 2022-10-24T23:19:53Z

jackofallops · 2022-10-25T14:43:02Z

internal/services/cdn/cdn_frontdoor_rule_resource.go

 									"forwarding_protocol": {
 										Type:     pluginsdk.TypeString,
 										Optional: true,
-										Default:  string(cdn.ForwardingProtocolMatchRequest),


Does this now need to be Computed since this will come back as MatchRequest from the service?

This cannot be Computed because that would cause a perpetual diff since you would not actually be able to clear the value, which is required to make this to work correctly. I do still set the default value, but I had to move it to the expand function:

// set the default value for forwarding protocol to avoid a breaking change... if protocol == "" { protocol = string(cdn.ForwardingProtocolMatchRequest) }

I do realize that the existing resources will now get a diff if they do not update there config to actually define the forwarding_protocol field, but I am not sure how to work around that. If you have any suggestions on how to accomplish that I am all ears. 🙂

Due to another issue (mentioned in the comment thread of the original issue) I had to make every field for this action optional and fix it in the expand/flatten functions of the resource. This is all related to Portal magic, I have added the following to the documentation NOTE to the route_configuration_override_action block. I know this is not ideal, but I feel this is the best that we can do given the cards we have been delt.

NOTE: While cache_duration, cache_behavior and query_string_caching_behavior do have default values, if you do not define these properties in your configuration file you will receive a diff during plan.

WodansSon · 2022-10-26T07:53:49Z

@stephybun, I am sorry I had to cancel your build on the TC server because I resolved the documentation conflict with main. However, after KT merged the documentation PR and before I resolved the documentation conflict I did kick off a new TC build of this PR and I will attach the results to this comment thread once it completes. Feel free to kick off another one if you'd like for due diligence. 🙂

WodansSon · 2022-10-26T07:56:35Z

Issues have been addressed

tombuildsstuff · 2022-10-26T08:55:35Z

internal/services/cdn/frontdoorruleactions/cdn_frontdoor_rule_actions.go

-				originGroupId = *group.ID
+			// NOTE: Need to normalize this ID here because if you modified this in portal the resourceGroup comes back as resourcegroup.
+			// ignore the error here since it was set on the resource in Azure and we know it is valid.
+			if originGroup, err := parse.FrontDoorOriginGroupIDInsensitively(*override.OriginGroup.ID); err == nil {


in the Expand function we should be parsing these case-sensitively:

Suggested change

if originGroup, err := parse.FrontDoorOriginGroupIDInsensitively(*override.OriginGroup.ID); err == nil {

if originGroup, err := parse.FrontDoorOriginGroupID(*override.OriginGroup.ID); err == nil {

I added this code specifically because if I created the rule in the portal and attempted to import it it would fail because of the casing of the resource group segment(e.g. resourcegroup vs. resourceGroup). This code is in the FlattenCdnFrontDoorRouteConfigurationOverrideAction where it gets the info from the API and not the Configuration file.

apologies that's my bad for missing this was in the Flatten and not the Expand - we should raise this error if this fails to pass, but this is otherwise fine 👍

WodansSon · 2022-10-26T21:17:58Z

tombuildsstuff

Left a few comments inline, but this otherwise LGTM 👍

tombuildsstuff · 2022-10-27T07:39:42Z

internal/services/cdn/frontdoorruleactions/cdn_frontdoor_rule_actions.go

+		// set the default value for forwarding protocol to avoid a breaking change...
+		if protocol == "" {
+			protocol = string(cdn.ForwardingProtocolMatchRequest)
 		}


this'll want to be changed in v4.0, so could we feature-flag this?

I personally believe that we should keep the default values in v4.0 as well since the service will return a 400 bad request error if the fields are not in the create/update call. I can feature-flag them if you would like, but I think that would lead to a bad user experience in v4.0 if we remove them.

tombuildsstuff · 2022-10-27T07:41:44Z

internal/services/cdn/frontdoorruleactions/cdn_frontdoor_rule_actions.go

-				return nil, fmt.Errorf("the 'route_configuration_override_action' block is not valid, 'query_string_parameters' must not be set if the'query_string_caching_behavior' is set to 'UseQueryStrings' or 'IgnoreQueryStrings'")
+			// since 'cache_duration', 'query_string_caching_behavior' and 'cache_behavior' are optional create a default values
+			// for those values if not set.
+			cacheDuration := "1.12:00:00"


is there some context for this default value, presumably this is coming from the API's default value/the portal or something?

There is, Front Door will default the cache to somewhere between 1 to 3 days, I have no way of knowing what Front Door decided and since I had to offer a default value to match the portal experience and this field has to be optional now I picked a value in the middle of what the Front Door documentation says the cache range can be... it seemed to me to be the lesser of the two evils and also allows for parity in functionality between Terraform and the Portal.

tombuildsstuff · 2022-10-27T07:42:42Z

internal/services/cdn/frontdoorruleactions/cdn_frontdoor_rule_actions.go

-				originGroupId = *group.ID
+			// NOTE: Need to normalize this ID here because if you modified this in portal the resourceGroup comes back as resourcegroup.
+			// ignore the error here since it was set on the resource in Azure and we know it is valid.
+			if originGroup, err := parse.FrontDoorOriginGroupIDInsensitively(*override.OriginGroup.ID); err == nil {


apologies that's my bad for missing this was in the Flatten and not the Expand - we should raise this error if this fails to pass, but this is otherwise fine 👍

tombuildsstuff · 2022-10-27T07:45:13Z

website/docs/r/cdn_frontdoor_rule.html.markdown

@@ -197,7 +197,9 @@ An `url_redirect_action` block supports the following:

 A `route_configuration_override_action` block supports the following:

-* `cache_duration` - (Required) When Cache behavior is set to `Override` or `SetIfMissing`, this field specifies the cache duration to use. The maximum duration is 366 days specified in the `d.HH:MM:SS` format(e.g. `365.23:59:59`). If the desired maximum cache duration is less than 1 day then the maximum cache duration should be specified in the `HH:MM:SS` format(e.g. `23:59:59`).
+->**NOTE:** While `cache_duration`, `cache_behavior` and `query_string_caching_behavior` do have default values, if you do not define these properties in your configuration file you will receive a diff during plan.


this is the default behaviour for Terraform properties, perhaps what we should be saying is something along the lines of: "you can use Terraform's ignore_changes functionality to ignore these default values`?

katbyte

LGTM 🍄

WodansSon · 2022-10-27T22:03:05Z

github-actions · 2022-10-28T00:13:30Z

This functionality has been released in v3.29.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 · 2022-11-27T02:23:21Z

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...

8029d1b

WodansSon added bug documentation service/cdn labels Oct 21, 2022

remove unnecessary conversion

aef93ca

stephybun previously requested changes Oct 21, 2022

View reviewed changes

github-actions bot added the waiting-response label Oct 21, 2022

add test cases

740ae97

github-actions bot removed the waiting-response label Oct 23, 2022

WodansSon requested a review from stephybun October 23, 2022 04:35

WodansSon added this to the v3.29.0 milestone Oct 23, 2022

WodansSon changed the title ~~[Bug:] azurerm_cdn_frontdoor_rule - allow the route_configuration_override_action field to be optional~~ [WIP][Bug:] azurerm_cdn_frontdoor_rule - allow the route_configuration_override_action field to be optional Oct 24, 2022

jackofallops marked this pull request as draft October 24, 2022 10:20

jackofallops removed this from the v3.29.0 milestone Oct 24, 2022

remove CustomizeDiff

325e168

WodansSon changed the title ~~[WIP][Bug:] azurerm_cdn_frontdoor_rule - allow the route_configuration_override_action field to be optional~~ [Bug:] azurerm_cdn_frontdoor_rule - allow the route_configuration_override_action field to be optional Oct 24, 2022

WodansSon marked this pull request as ready for review October 24, 2022 23:20

WodansSon added this to the v3.29.0 milestone Oct 25, 2022

jackofallops reviewed Oct 25, 2022

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Oct 25, 2022

WodansSon marked this pull request as draft October 26, 2022 00:16

WodansSon added 3 commits October 25, 2022 21:25

add disable cache functionality

4c1e34e

update test names

872df6d

add disable cache update test

3065351

WodansSon marked this pull request as ready for review October 26, 2022 05:19

Add more test cases

b8db159

Merge branch 'main' into b_frontdoor_rule

3601c9b

WodansSon requested review from tombuildsstuff and jackofallops October 26, 2022 08:30

tombuildsstuff reviewed Oct 26, 2022

View reviewed changes

github-actions bot added waiting-response and removed waiting-response labels Oct 26, 2022

add test case for optional update

10fc3b5

WodansSon requested a review from tombuildsstuff October 27, 2022 06:20

tombuildsstuff approved these changes Oct 27, 2022

View reviewed changes

WodansSon added 5 commits October 27, 2022 14:54

address PR comments

6b80bb4

update note in documentation

fde30c0

update documentation

f6badbc

doc update

c2f82cf

update code comments and tests

3394a45

katbyte approved these changes Oct 27, 2022

View reviewed changes

WodansSon merged commit 409466d into main Oct 27, 2022

WodansSon deleted the b_frontdoor_rule branch October 27, 2022 22:06

WodansSon added a commit that referenced this pull request Oct 27, 2022

updated to include #18906

c7504be

github-actions bot locked as resolved and limited conversation to collaborators Nov 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug:] `azurerm_cdn_frontdoor_rule` - allow the `cdn_frontdoor_origin_group_id` field to be optional in the `route_configuration_override_action`, expose `Disabled` as a possible value of `cache_behavior` #18906

[Bug:] `azurerm_cdn_frontdoor_rule` - allow the `cdn_frontdoor_origin_group_id` field to be optional in the `route_configuration_override_action`, expose `Disabled` as a possible value of `cache_behavior` #18906

WodansSon commented Oct 21, 2022 •

edited

Loading

stephybun left a comment

WodansSon commented Oct 23, 2022

WodansSon commented Oct 24, 2022

WodansSon commented Oct 24, 2022

jackofallops Oct 25, 2022

WodansSon Oct 25, 2022 •

edited

Loading

WodansSon Oct 26, 2022 •

edited

Loading

WodansSon commented Oct 26, 2022 •

edited

Loading

WodansSon commented Oct 26, 2022

tombuildsstuff Oct 26, 2022

WodansSon Oct 26, 2022 •

edited

Loading

tombuildsstuff Oct 27, 2022

WodansSon commented Oct 26, 2022

tombuildsstuff left a comment

tombuildsstuff Oct 27, 2022

WodansSon Oct 27, 2022

tombuildsstuff Oct 27, 2022

WodansSon Oct 27, 2022 •

edited

Loading

tombuildsstuff Oct 27, 2022

tombuildsstuff Oct 27, 2022

WodansSon Oct 27, 2022

katbyte left a comment

WodansSon commented Oct 27, 2022

github-actions bot commented Oct 28, 2022

github-actions bot commented Nov 27, 2022

	if originGroup, err := parse.FrontDoorOriginGroupIDInsensitively(*override.OriginGroup.ID); err == nil {
	if originGroup, err := parse.FrontDoorOriginGroupID(*override.OriginGroup.ID); err == nil {

[Bug:] azurerm_cdn_frontdoor_rule - allow the cdn_frontdoor_origin_group_id field to be optional in the route_configuration_override_action, expose Disabled as a possible value of cache_behavior #18906

[Bug:] azurerm_cdn_frontdoor_rule - allow the cdn_frontdoor_origin_group_id field to be optional in the route_configuration_override_action, expose Disabled as a possible value of cache_behavior #18906

Conversation

WodansSon commented Oct 21, 2022 • edited Loading

Optional cdn_frontdoor_origin_group_id:

Disable Cache Without the Origin Group ID:

Disable Cache With the Origin Group ID:

stephybun left a comment

Choose a reason for hiding this comment

WodansSon commented Oct 23, 2022

WodansSon commented Oct 24, 2022

WodansSon commented Oct 24, 2022

Choose a reason for hiding this comment

WodansSon Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

WodansSon Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

WodansSon commented Oct 26, 2022 • edited Loading

WodansSon commented Oct 26, 2022

Choose a reason for hiding this comment

WodansSon Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WodansSon commented Oct 26, 2022

tombuildsstuff 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

WodansSon Oct 27, 2022 • 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

katbyte left a comment

Choose a reason for hiding this comment

WodansSon commented Oct 27, 2022

github-actions bot commented Oct 28, 2022

github-actions bot commented Nov 27, 2022

[Bug:] `azurerm_cdn_frontdoor_rule` - allow the `cdn_frontdoor_origin_group_id` field to be optional in the `route_configuration_override_action`, expose `Disabled` as a possible value of `cache_behavior` #18906

[Bug:] `azurerm_cdn_frontdoor_rule` - allow the `cdn_frontdoor_origin_group_id` field to be optional in the `route_configuration_override_action`, expose `Disabled` as a possible value of `cache_behavior` #18906

WodansSon commented Oct 21, 2022 •

edited

Loading

Optional `cdn_frontdoor_origin_group_id`:

WodansSon Oct 25, 2022 •

edited

Loading

WodansSon Oct 26, 2022 •

edited

Loading

WodansSon commented Oct 26, 2022 •

edited

Loading

WodansSon Oct 26, 2022 •

edited

Loading

WodansSon Oct 27, 2022 •

edited

Loading