Bug: `azurerm_frontdoor` fix for caching issue #5358

WodansSon · 2020-01-09T23:09:07Z

Original PR #4618
Fixes #4461

tombuildsstuff

Thanks for rebasing this PR - I've taken a look through and left some comments inline but this is looking good; there's two main questions for me:

can we switch to using an ID parsing function and then conditionally check both known cases for the HealthProbeSettings string - rather than parsing the entire Resource ID case insensitively?
can we revert the breaking changes, instead adding TODO's where we can make these in 2.0

Thanks!

tombuildsstuff · 2020-01-10T10:42:47Z

azurerm/internal/services/frontdoor/helper.go

+// a known format so I can reliable parse the ID string.
+//
+// Link to issue: https://github.com/Azure/azure-sdk-for-go/issues/6762
+func ParseAzureResourceIDLowerPath(id string) (*azure.ResourceID, error) {


since there's two possible values for this - it'd be better to instead check both of the specific casings we need here e.g.

id, err := azure.ParseResourceID(...) .. name := id.Path["HealthProbeSettings"] if name == "" { name = id.Path["healthProbeSettings"] }

if we wrap this in an ID parsing function (e.g. as shown in #5356) we should be able to instead reuse the existing function?

tombuildsstuff · 2020-01-10T10:44:24Z

azurerm/internal/services/frontdoor/resource_arm_front_door.go

@@ -198,7 +200,7 @@ func resourceArmFrontDoor() *schema.Resource {
 											string(frontdoor.HTTPSOnly),
 											string(frontdoor.MatchRequest),
 										}, false),
-										Default: string(frontdoor.MatchRequest),
+										Default: string(frontdoor.HTTPSOnly),


this is a breaking change?

Yes, but the entire cache section it totally broken and this change enables the disabling of the cache. Currently there is no way to disable cache with the originally released resource.

tombuildsstuff · 2020-01-10T10:44:25Z

azurerm/internal/services/frontdoor/resource_arm_front_door.go

 									"cache_query_parameter_strip_directive": {
 										Type:     schema.TypeString,
 										Optional: true,
 										ValidateFunc: validation.StringInSlice([]string{
 											string(frontdoor.StripAll),
 											string(frontdoor.StripNone),
 										}, false),
-										Default: string(frontdoor.StripNone),


this is a breaking change?

website/docs/r/front_door.html.markdown

tombuildsstuff · 2020-01-10T10:53:23Z

website/docs/r/front_door.html.markdown


 * `custom_forwarding_path` - (Optional) Path to use when constructing the request to forward to the backend. This functions as a URL Rewrite. Default behavior preserves the URL path.

-* `forwarding_protocol` - (Optional) Protocol to use when redirecting. Valid options are `HttpOnly`, `HttpsOnly`, or `MatchRequest`. Defaults to `MatchRequest`.
+* `forwarding_protocol` - (Optional) Protocol to use when redirecting. Valid options are `HTTPOnly`, `HTTPSOnly`, or `MatchRequest`. Defaults to `HTTPSOnly`.


these aren't the values from the SDK:

// RedirectProtocol enumerates the values for redirect protocol. type RedirectProtocol string const ( // RedirectProtocolHTTPOnly ... RedirectProtocolHTTPOnly RedirectProtocol = "HttpOnly" // RedirectProtocolHTTPSOnly ... RedirectProtocolHTTPSOnly RedirectProtocol = "HttpsOnly" // RedirectProtocolMatchRequest ... RedirectProtocolMatchRequest RedirectProtocol = "MatchRequest" )

website/docs/r/front_door.html.markdown

Co-Authored-By: Tom Harvey <[email protected]>

WodansSon · 2020-01-24T21:56:50Z

@tombuildsstuff @katbyte This PR is 100% done and with no breaking changes and it would be great if this could make it into v1.42.0 as there are a lot of customers waiting for this fix.

WodansSon · 2020-01-24T22:00:15Z

katbyte

Aside from two minor comments this LGTM 👍

katbyte · 2020-01-25T00:16:50Z

website/docs/r/front_door.html.markdown


-* `cache_use_dynamic_compression` - (Optional) Whether to use dynamic compression when caching. Valid options are `true` or `false`. Defaults to `true`.
+* `cache_use_dynamic_compression` - (Optional) Whether to use dynamic compression when caching. Valid options are `true` or `false`. Defaults to `false`.


should this be true?

I believe this was a documentation error, as in the schema in master the default value is also false. So I think this is good.

katbyte · 2020-01-25T00:17:07Z

azurerm/internal/services/frontdoor/resource_arm_front_door.go

+						c["cache_enabled"] = false
+
+						//get `forwarding_configuration`
+						if o, ok := oldByName[*name]; ok {


Name could be nil so we'll need to nil check it

ghost · 2020-01-27T20:46:59Z

This has been released in version 1.42.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 1.42.0"
}
# ... other configuration ...

ghost · 2020-03-28T15:21:19Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

Port PR to new code base

465d09e

WodansSon added bug documentation size/L service/frontdoor labels Jan 9, 2020

WodansSon added this to the v1.41.0 milestone Jan 9, 2020

WodansSon requested review from tombuildsstuff and katbyte January 9, 2020 23:09

WodansSon self-assigned this Jan 9, 2020

tombuildsstuff suggested changes Jan 10, 2020

View reviewed changes

tombuildsstuff added the waiting-response label Jan 10, 2020

WodansSon and others added 17 commits January 13, 2020 15:24

Update website/docs/r/front_door.html.markdown

4904a20

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

f62f456

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

c85c9c1

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

cfe157d

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

ff57240

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

d9aecd8

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

17c05f6

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

e46221e

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

b88ef6b

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

5e21698

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

5b17a51

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

644ac71

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

8ef9e23

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

9cf59c2

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

2ca5437

Co-Authored-By: Tom Harvey <[email protected]>

Update website/docs/r/front_door.html.markdown

e8978c5

Co-Authored-By: Tom Harvey <[email protected]>

Progress

ba86c82

ghost added size/M and removed size/L labels Jan 14, 2020

WodansSon added 4 commits January 22, 2020 12:58

Update docs to document new behavior

dadf851

Fix tests and documentation

3eb71d9

Merge branch 'master' into rf_frontdoor_cache

fe605a3

Add test case for cache regression

9c358bd

ghost added size/XL and removed size/L labels Jan 23, 2020

WodansSon removed the waiting-response label Jan 23, 2020

WodansSon and others added 5 commits January 22, 2020 18:47

Fix test code terrafmt

6adb437

Fixed linting error in comment

8efc0ac

pull value through test

613bc0e

Update docs and remove commented code

3412cf1

Update test case

b2eda4f

tombuildsstuff modified the milestones: v1.42.0, v1.43.0 Jan 24, 2020

katbyte added 2 commits January 24, 2020 12:29

Merge remote-tracking branch 'origin/master' into rf_frontdoor_cache

4b6ecba

terrafmt

a2e78b0

WodansSon modified the milestones: v1.43.0, v1.42.0 Jan 24, 2020

Minor doc update

2c4f4f4

katbyte approved these changes Jan 25, 2020

View reviewed changes

Added nil check per PR review

efdfb58

WodansSon merged commit d42f493 into master Jan 25, 2020

WodansSon deleted the rf_frontdoor_cache branch January 25, 2020 01:44

WodansSon added a commit that referenced this pull request Jan 25, 2020

Updating to include #5358

4d5b72d

WodansSon mentioned this pull request Feb 27, 2020

'Ignore Changes' causes plan with Front Door to error out #4748

Closed

ghost locked and limited conversation to collaborators Mar 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug: `azurerm_frontdoor` fix for caching issue #5358

Bug: `azurerm_frontdoor` fix for caching issue #5358

WodansSon commented Jan 9, 2020 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff Jan 10, 2020

WodansSon Jan 14, 2020

tombuildsstuff Jan 10, 2020

WodansSon Jan 23, 2020

tombuildsstuff Jan 10, 2020

WodansSon Jan 23, 2020

tombuildsstuff Jan 10, 2020

WodansSon Jan 13, 2020

WodansSon commented Jan 24, 2020 •

edited

Loading

WodansSon commented Jan 24, 2020

katbyte left a comment

katbyte Jan 25, 2020

WodansSon Jan 25, 2020

katbyte Jan 25, 2020

WodansSon Jan 25, 2020

ghost commented Jan 27, 2020

ghost commented Mar 28, 2020


		* `cache_use_dynamic_compression` - (Optional) Whether to use dynamic compression when caching. Valid options are `true` or `false`. Defaults to `true`.
		* `cache_use_dynamic_compression` - (Optional) Whether to use dynamic compression when caching. Valid options are `true` or `false`. Defaults to `false`.

Bug: azurerm_frontdoor fix for caching issue #5358

Bug: azurerm_frontdoor fix for caching issue #5358

Conversation

WodansSon commented Jan 9, 2020 • edited Loading

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

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 commented Jan 24, 2020 • edited Loading

WodansSon commented Jan 24, 2020

katbyte 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

ghost commented Jan 27, 2020

ghost commented Mar 28, 2020

Bug: `azurerm_frontdoor` fix for caching issue #5358

Bug: `azurerm_frontdoor` fix for caching issue #5358

WodansSon commented Jan 9, 2020 •

edited

Loading

WodansSon commented Jan 24, 2020 •

edited

Loading