Fix issues with frontdoor custom https configuration #7498

luigibk · 2020-06-25T15:58:31Z

This PR aims to introduce the following changes:

A fix for the below issue which prevents custom https configurations in frontdoor from being updated properly

Separating the custom https configuration resource from the main frontdoor resource. This should allow for parallel update of these resources as currently they are processed serially and that always leads to a timeout given enough frontend endpoints or endless waiting given how long each custom https configuration update can take (e.g. certificate update).

(fixes #6057 )

luigibk · 2020-06-26T09:56:37Z

Hi @WodansSon
I currently have an issue at apply stage as after the custom https configurations get created (successfully) I get an error:

When applying changes to
azurerm_frontdoor_custom_https_configuration.example_custom_https_0, provider
"registry.terraform.io/-/azurerm" produced an unexpected new value for was
present, but now absent.
This is a bug in the provider, which should be reported in the provider's own
issue tracker.
Error: Provider produced inconsistent result after apply

and my tf state file is missing all the custom https config even if they have been created correctly.

# Conflicts: # azurerm/internal/services/frontdoor/frontdoor_resource.go

azurerm/internal/services/frontdoor/frontdoor_resource.go

timja · 2020-07-01T09:17:33Z

Missing website documentation for new resource and any required updates to the existing documentation

azurerm/internal/services/frontdoor/frontdoor_resource.go

Co-authored-by: Tim Jacomb <[email protected]>

… code to resource_arm_frontdoor_custom_https_configuration_resource file.

website/docs/r/frontdoor.html.markdown

website/docs/r/frontdoor_custom_https_configuration.html.markdown

luigibk · 2020-07-03T09:44:43Z

Hi @WodansSon

I have fixed the issue I asked about some time ago + a few others, did some clean up and have added docs. It would be great if you could have a look.

It's all working as far as I can see apart from one use case:

when I delete an endpoint + its custom https configuration, the 2 operations proceed in parallel and the custom https configuration disable returns an error as the endpoint has been (or is being) deleted. Not sure why that is so as a custom_https_configuration depends on its endpoint so my understanding is that the 2 operations should not run in parallel in the first place. Any ideas?

luigibk · 2020-07-08T22:42:05Z

@WodansSon I think there might be a misunderstanding.
I added a comment in the related issue:
#6057 (comment)

WodansSon · 2020-07-17T00:39:31Z

@luigibk you need to format the HCL in your test code to get rid of the lint error. I tried fixing it for you but I was denied permission to push to your fork.

WodansSon

@luigibk, I've left code suggestions to fix the lint errors.

azurerm/internal/services/frontdoor/tests/frontdoor_custom_https_configuration_resource_test.go

WodansSon

@luigibk, thanks for the PR, this is looking good... left a few comments, but overall it looks pretty good, this is really close to being able to be merged! 🚀

azurerm/internal/services/frontdoor/frontdoor_custom_https_configuration_resource.go

WodansSon · 2020-07-17T01:38:09Z

azurerm/internal/services/frontdoor/frontdoor_custom_https_configuration_resource.go

+		},
+
+		Schema: map[string]*schema.Schema{
+			"front_door_name": {


Can we change this to front_door_id instead of front_door_name? If you want to keep the front_door_name attribute can also you expose the front_door_id as a new attribute?

I think the best solution is to actually add a frontend_endpoint_id and remove both front_door_name and frontend_endpoint_name as they can be derived from the id. That way it should be easier to manage from a user perspective and the implementation should not be more complex after removing those 2 fields. How does such a solution sound to you?

That's a good idea, but because of the way Terraform works the core would not be able to build a proper dependency graph during the plan phase, if I am not mistaken, which is why we generally use the parent resources id attribute. You can give it a try and see if it builds a correct provisioning graph, but my gut is telling me it won't be able to connect the two together.

In this case the parent resource for the custom https configuration is the frontend endpoint. I've started work on this and it looks good so far. Hopefully won't be long before I commit it.

azurerm/internal/services/frontdoor/frontdoor_custom_https_configuration_resource.go

azurerm/internal/services/frontdoor/frontdoor_resource.go

website/docs/r/frontdoor.html.markdown

Co-authored-by: WS <[email protected]>

…urce

timja · 2020-07-17T10:21:02Z

I tried fixing it for you but I was denied permission to push to your fork.

https://github.community/t/how-can-we-enable-allow-edits-from-maintainers-by-default/2847/7
This was submitted from our org account, which apparently doesn't support the option to allow maintainers to push

We'll keep that in mind in future, for now if you have any follow up changes you want to do yourself feel free to send us a PR and we'll merge it in

luigibk · 2020-07-17T15:36:54Z

@WodansSon thanks for your review and comments. I have merged all of them but one and left a comment about the suggestion of adding a front_door_id.

WodansSon · 2020-07-18T02:38:41Z

I tried fixing it for you but I was denied permission to push to your fork.

https://github.community/t/how-can-we-enable-allow-edits-from-maintainers-by-default/2847/7
This was submitted from our org account, which apparently doesn't support the option to allow maintainers to push

We'll keep that in mind in future, for now if you have any follow up changes you want to do yourself feel free to send us a PR and we'll merge it in

Ahhh... that makes sense, thanks for the heads up. Yes, there are two things I think still need to be addressed:

The read timeout, I left a new comment on the revert. I think your fork might be behind origin/master cause I changed the read time out in azurerm_frontdoor - Fix timeout for read operation to match documentation #7408
I replied to you question about using front door ID vs. frontend endpoint ID in the PR proper.

Thanks again, this is so close, just a couple of tweaks and it will be ready to merge. Thanks!

timja · 2020-07-20T15:16:00Z

The read timeout, I left a new comment on the revert. I think your fork might be behind origin/master cause I changed the read time out in azurerm_frontdoor - Fix timeout for read operation to match documentation #7408

Hi @WodansSon
It seems that your timeout PR was reverted? #7485

luigibk · 2020-07-20T15:19:18Z

About the read timeout, it's currently set at 5minutes in frontdoor:

https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/internal/services/frontdoor/frontdoor_resource.go#L37

and changing it to 5 hours breaks this test:

https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/internal/provider/provider_test.go#L77

so I have left those timeouts at 5 minutes for now

luigibk · 2020-07-21T00:23:01Z

@WodansSon thanks for helping with this. I have just pushed the change related to replacing frontdoor_name and frontend_endpoint_name with the frontend_endpoint_id.

WodansSon · 2020-07-22T01:27:59Z

About the read timeout, it's currently set at 5minutes in frontdoor:

https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/internal/services/frontdoor/frontdoor_resource.go#L37

and changing it to 5 hours breaks this test:

https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/internal/provider/provider_test.go#L77

so I have left those timeouts at 5 minutes for now

It looks like Tom over wrote my change and reverted the value back to 5 minutes, so this is fine. Can we make sure the documentation also reflects this change?

WodansSon · 2020-07-22T02:03:43Z

WodansSon

@luigibk this LGTM now! 🚀

ghost · 2020-07-23T13:53:47Z

This has been released in version 2.20.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 = "~> 2.20.0"
}
# ... other configuration ...

ghost · 2020-08-21T14:50:06Z

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!

Fix issue with frontdoor custom https configuration not being updated

ab51e3c

ghost added the size/XL label Jun 25, 2020

katbyte requested a review from WodansSon June 25, 2020 16:21

katbyte assigned WodansSon Jun 25, 2020

Luigi Bitonti added 6 commits June 26, 2020 12:11

Clean up to fix lint warnings

b4fd147

Merge branch 'master' into frontdoor-custom-https

d192f0f

# Conflicts: # azurerm/internal/services/frontdoor/frontdoor_resource.go

Fix merge issues

8d22c00

Add synthetic resource id to custom_https_configuration

b79d273

Clean up custom_https_configuration + methods signaures

87dbe62

Fix validation. Fix read custom_https_configuration

da9c172

timja reviewed Jul 1, 2020

View reviewed changes

azurerm/internal/services/frontdoor/frontdoor_resource.go Outdated Show resolved Hide resolved

timja reviewed Jul 1, 2020

View reviewed changes

azurerm/internal/services/frontdoor/frontdoor_resource.go Outdated Show resolved Hide resolved

timja reviewed Jul 1, 2020

View reviewed changes

azurerm/internal/services/frontdoor/frontdoor_resource.go Outdated Show resolved Hide resolved

timja reviewed Jul 1, 2020

View reviewed changes

azurerm/internal/services/frontdoor/frontdoor_resource.go Outdated Show resolved Hide resolved

luigibk and others added 2 commits July 1, 2020 10:34

Apply suggestions from code review

abd13e1

Co-authored-by: Tim Jacomb <[email protected]>

Add docs. Move common code to helper. Move custom_https_configuration…

f6a871d

… code to resource_arm_frontdoor_custom_https_configuration_resource file.

ghost added the documentation label Jul 2, 2020

timja reviewed Jul 2, 2020

View reviewed changes

website/docs/r/frontdoor.html.markdown Outdated Show resolved Hide resolved

Move docs to separate file

125429c

timja reviewed Jul 3, 2020

View reviewed changes

website/docs/r/frontdoor_custom_https_configuration.html.markdown Outdated Show resolved Hide resolved

timja reviewed Jul 3, 2020

View reviewed changes

website/docs/r/frontdoor_custom_https_configuration.html.markdown Show resolved Hide resolved

Add clarifications + fixes to docs.

e090a70

luigibk marked this pull request as ready for review July 3, 2020 08:49

Fix file name

e8b3bdf

katbyte added service/frontdoor bug labels Jul 5, 2020

Fix endpoint delete conflict

cb75765

WodansSon requested changes Jul 17, 2020

View reviewed changes

WodansSon added the waiting-response label Jul 17, 2020

luigibk and others added 2 commits July 17, 2020 09:00

Apply suggestions from code review

4b84168

Co-authored-by: WS <[email protected]>

Fix merge issues + add frontend_id to custom_https_configuration reso…

43eaabd

…urce

ghost removed the waiting-response label Jul 17, 2020

Fix timeout (revert to: a08a2d8#diff-f40eb1b0d4646b0fed0eab1d3837c42d)

cc5707f

Merge branch 'master' into frontdoor-custom-https

0ebfdac

Replace front_door and frontend_endpoint names with frontend_endpoint_id

0fa855a

WodansSon added the new-resource label Jul 22, 2020

WodansSon added this to the v2.20.0 milestone Jul 22, 2020

WodansSon approved these changes Jul 22, 2020

View reviewed changes

WodansSon merged commit e6a276c into hashicorp:master Jul 22, 2020

WodansSon added a commit that referenced this pull request Jul 22, 2020

updating to include #7498

a27d281

luigibk deleted the frontdoor-custom-https branch July 22, 2020 23:28

This was referenced Aug 6, 2020

azurerm_frontdoor broken with recent Azure API update #8036

Closed

azurerm_frontdoor breaks on modification in 2.20 #8039

Closed

ghost locked and limited conversation to collaborators Aug 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with frontdoor custom https configuration #7498

Fix issues with frontdoor custom https configuration #7498

luigibk commented Jun 25, 2020 •

edited by WodansSon

Loading

luigibk commented Jun 26, 2020 •

edited

Loading

timja commented Jul 1, 2020

luigibk commented Jul 3, 2020

luigibk commented Jul 8, 2020

WodansSon commented Jul 17, 2020

WodansSon left a comment

WodansSon left a comment

WodansSon Jul 17, 2020

luigibk Jul 17, 2020

WodansSon Jul 18, 2020

luigibk Jul 20, 2020

timja commented Jul 17, 2020

luigibk commented Jul 17, 2020

WodansSon commented Jul 18, 2020

timja commented Jul 20, 2020

luigibk commented Jul 20, 2020

luigibk commented Jul 21, 2020

WodansSon commented Jul 22, 2020

WodansSon commented Jul 22, 2020

WodansSon left a comment

ghost commented Jul 23, 2020

ghost commented Aug 21, 2020

Fix issues with frontdoor custom https configuration #7498

Fix issues with frontdoor custom https configuration #7498

Conversation

luigibk commented Jun 25, 2020 • edited by WodansSon Loading

luigibk commented Jun 26, 2020 • edited Loading

timja commented Jul 1, 2020

luigibk commented Jul 3, 2020

luigibk commented Jul 8, 2020

WodansSon commented Jul 17, 2020

WodansSon left a comment

Choose a reason for hiding this comment

WodansSon left a comment

Choose a reason for hiding this comment

WodansSon Jul 17, 2020

Choose a reason for hiding this comment

luigibk Jul 17, 2020

Choose a reason for hiding this comment

WodansSon Jul 18, 2020

Choose a reason for hiding this comment

luigibk Jul 20, 2020

Choose a reason for hiding this comment

timja commented Jul 17, 2020

luigibk commented Jul 17, 2020

WodansSon commented Jul 18, 2020

timja commented Jul 20, 2020

luigibk commented Jul 20, 2020

luigibk commented Jul 21, 2020

WodansSon commented Jul 22, 2020

WodansSon commented Jul 22, 2020

WodansSon left a comment

Choose a reason for hiding this comment

ghost commented Jul 23, 2020

ghost commented Aug 21, 2020

luigibk commented Jun 25, 2020 •

edited by WodansSon

Loading

luigibk commented Jun 26, 2020 •

edited

Loading