New Data Source/Resource: `azurerm_cdn_frontdoor_origin_group` / New Resource: `azurerm_cdn_frontdoor_origin` #17089

tombuildsstuff · 2022-06-03T11:27:24Z

This PR adds a new Data Source and Resource for azurerm_cdn_frontdoor_origin_group and a new Resource for azurerm_cdn_frontdoor_origin - split out from #16671.

I've left a couple of TODO's in here related to CDN FrontDoor Origin for @WodansSon, namely:

Should the certificate_name_check_enabled field be defaulted to true as this is a security related setting? (That's validating the hostname matches on the SSL certificate from the origin)
The Acceptance Tests need a custom Check function to approve the Private Link (which should be doable via retrieving the associated private link, from the info we have, and updating it?)

Porting across from #16671 Co-authored-by: Jeffrey Cline <[email protected]>

…Origin Group

Splitting out from #16671 Co-authored-by: Jeffrey Cline <[email protected]>

Splitting this out from #16671 Co-authored-by: Jeffrey Cline <[email protected]>

WodansSon · 2022-06-04T04:36:24Z

internal/services/cdn/cdn_frontdoor_origin_group_resource.go

+						"request_type": {
+							Type:     pluginsdk.TypeString,
+							Required: true,
+							ValidateFunc: validation.StringInSlice([]string{
+								string(cdn.HealthProbeRequestTypeGET),
+								string(cdn.HealthProbeRequestTypeHEAD),
+							}, false),
+						},


Should this be an optional field with the default as HEAD like the service?

Tbh I'd argue this, Protocol and Path want to be Required since these values are always going to be specific to the application/host being targeted, so we may as well make the three required so users are aware which healthcheck's configured to alleviate any confusion?

WodansSon · 2022-06-04T04:45:31Z

internal/services/cdn/cdn_frontdoor_origin_resource.go

+			"certificate_name_check_enabled": {
+				Type:     pluginsdk.TypeBool,
+				Optional: true,
+				// TODO: should this default to `true`?
+			},


I believe from a security perspective that is the correct behavior, but iirc I made this false because on the legacy Frontdoor resource the end users were complaining about having to set the value to false as most of them ended up setting it to false anyways, so I made false the default in the new version as well. Totally up to you, I am fine either way, but if you do make the default true we may get more issues about it. Another option here would be if we just make it a required field and then the end users will have to make an explicit declaration of what it should be, so they will be fully aware of the security state of the origin group without assuming what the default value is? I think that is what I ended up doing in the legacy resource as well.

I would also opt to make it a required field. The docs should also contain a note that this needs to be set to true when using private link

Yeah making this required would be fine - I'm surprised that this isn't defaulted to true in the API tbh (since it's a security setting) - but 👍

I have adjusted the schema and made this a required field

WodansSon · 2022-06-04T04:57:25Z

internal/services/cdn/cdn_frontdoor_origin_resource_test.go

+				// TODO: approve the connection by looking up and updating the private link
+				// data.CheckWithClient(func(ctx context.Context, clients *clients.Client, state *terraform.InstanceState) error {
+				//	clients.Network.PrivateLinkServiceClient.UpdatePrivateEndpointConnection()
+				// }),


I am not sure, but it does make sense that it would be possible to do that. I had planned on investigating that after the release and just manually tested it in the interim as this resources was a large chunk of work to sort out, as you know. 🙂

If approving could be automated by TF that would be awesome. But agree that it's probably not a P1 for release

I guess there's potentially a resource here along the lines of azurerm_private_link_approval or something? Whilst this isn't a blocker if we can manually test it - we do need this as a part of the test to ensure we're capturing this correctly, given deletion works slightly differently when there's a private link involved (as we've seen from other resources)

sebader · 2022-06-07T07:42:48Z

website/docs/r/cdn_frontdoor_origin.html.markdown

+
+## Attributes Reference
+
+In addition to the Arguments listed above - the following Attributes are exported:


is it possible to export the private endpoint connection resource id, when using Private Link? that would make approval much easier instead of trying to pull out the connection afterwards.

sebader · 2022-06-20T07:24:33Z

@tombuildsstuff any updates on these? now that the first parts of AFDx have shipped, it would be great to get the missing pieces in, too

tombuildsstuff · 2022-06-25T07:02:09Z

@sebader due to other commitments I've had to hand this back to @WodansSon, but we'll give this a review when changes are pushed 👍

WodansSon · 2022-07-13T00:54:34Z

@sebader @tombuildsstuff I didn't realize when this was handed back to me that it also included this PR which was still in progress. I will pick this back up and try to get the rest of the resources shipped ASAP.

WodansSon · 2022-07-13T21:54:41Z

@tombuildsstuff

2. The Acceptance Tests need a custom Check function to approve the Private Link (which should be doable via retrieving the associated private link, from the info we have, and updating it?)

I am going to skip the private link stuff for now since the resources have been delayed for quite some time now. Once all of the resources are released I will revisit the private link issues, but for now I believe leaving it a manual step is acceptable.

WodansSon · 2022-07-14T04:41:55Z

tombuildsstuff

Left a few comments inline @WodansSon but this otherwise LGTM, thanks for fixing this up - I can't approve it since I opened the PR though 🙃

tombuildsstuff · 2022-07-21T15:55:57Z

website/docs/r/cdn_frontdoor_origin.html.markdown

+
+* `location` - (Required) Specifies the location where the Private Link resource should exist.
+
+~> **Note:** For performance reasons this value should match the location of the `private_link_target_id` resource.


might be worth spelling this out since cross-region transfers have a cost-component too?

tombuildsstuff · 2022-07-21T15:57:41Z

website/docs/d/cdn_frontdoor_origin_group.html.markdown

+
+The following arguments are supported:


Suggested change

The following arguments are supported:

tombuildsstuff · 2022-07-21T16:27:46Z

internal/services/cdn/cdn_frontdoor_origin_group_resource.go

+						"request_type": {
+							Type:     pluginsdk.TypeString,
+							Required: true,
+							ValidateFunc: validation.StringInSlice([]string{
+								string(cdn.HealthProbeRequestTypeGET),
+								string(cdn.HealthProbeRequestTypeHEAD),
+							}, false),
+						},


Tbh I'd argue this, Protocol and Path want to be Required since these values are always going to be specific to the application/host being targeted, so we may as well make the three required so users are aware which healthcheck's configured to alleviate any confusion?

tombuildsstuff · 2022-07-21T16:28:08Z

website/docs/r/cdn_frontdoor_origin_group.html.markdown

+  cdn_frontdoor_profile_id = azurerm_cdn_frontdoor_profile.example.id
+  session_affinity_enabled = true
+
+  restore_traffic_time_to_healed_or_new_endpoint_in_minutes = 10


this isn't documented below fwiw

tombuildsstuff · 2022-07-21T16:30:32Z

internal/services/cdn/cdn_frontdoor_origin_resource.go

+			"certificate_name_check_enabled": {
+				Type:     pluginsdk.TypeBool,
+				Required: true,
+				// TODO: should this default to `true`?


I believe we chatted about this elsewhere and some folks wanted a default to false, however we're defaulting this to Required so folks can intentionally opt-out of this security feature if they're not concerned with SNI checks - so I don't think this is required? @WodansSon

Suggested change

// TODO: should this default to `true`?

tombuildsstuff · 2022-07-21T16:30:54Z

internal/services/cdn/cdn_frontdoor_origin_resource.go

+			"health_probes_enabled": {
+				Type:     pluginsdk.TypeBool,
+				Optional: true,
+				Default:  false,


should this be defaulted to true?

WodansSon · 2022-07-22T00:18:58Z

@tombuildsstuff:

Tbh I'd argue this, Protocol and Path want to be Required since these values are always going to be specific to the application/host being targeted, so we may as well make the three required so users are aware which healthcheck's configured to alleviate any confusion?

I do not believe the path should be required, because depending on what request_type you use the path may not be needed since front door gets the URL from the backend. path is really only helpful in the GET scenario and most people do not use the GET request_type because every time the front door health check issues a GET call to ping the edge servers that edge server cost increases because it has to return a message-body in the response to the GET call. That said I do agree that the interval_in_seconds should be required as well, since that can affect the load on the edge servers, forcing the end users to pick this number I think is totally valid.

I would also tend to think that request_type should be optional with a default of HEAD as that matches the behavior in portal and the product documentation.

…r-azurerm into f/cdn-frontdoor-origin-and-groups

katbyte

Aside from one comment this LGTM 🎩

katbyte · 2022-07-22T01:46:06Z

CHANGELOG.md

@@ -1,20 +1,20 @@
-## 3.14.0 (Unreleased)


you will have to roll back the merge main

WodansSon · 2022-07-22T02:11:40Z

github-actions · 2022-07-25T06:50:42Z

This functionality has been released in v3.15.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-08-25T02:40:51Z

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.

tombuildsstuff and others added 5 commits June 3, 2022 10:37

New Resource: azurerm_cdn_frontdoor_endpoint

2c0f40f

Porting across from #16671 Co-authored-by: Jeffrey Cline <[email protected]>

cdn: generating the ID Parsers/Validators for CDN FrontDoor Origin & …

6d2edfb

…Origin Group

New Resource: azurerm_cdn_frontdoor_origin_group

6d5bbe3

Splitting out from #16671 Co-authored-by: Jeffrey Cline <[email protected]>

New Data Source: azurerm_cdn_frontdoor_origin_group

eefd37e

New Resource: azurerm_cdn_frontdoor_origin

1f186a0

Splitting this out from #16671 Co-authored-by: Jeffrey Cline <[email protected]>

tombuildsstuff added new-resource new-data-source service/frontdoor labels Jun 3, 2022

tombuildsstuff added this to the v3.10.0 milestone Jun 3, 2022

tombuildsstuff assigned WodansSon Jun 3, 2022

tombuildsstuff marked this pull request as draft June 3, 2022 11:27

github-actions bot added documentation service/cdn size/XL and removed service/frontdoor labels Jun 3, 2022

tombuildsstuff mentioned this pull request Jun 3, 2022

azurerm_cdn_frontdoor - Add CDN FrontDoor example configurations #16671

Merged

refactor: linting

c9163aa

WodansSon reviewed Jun 4, 2022

View reviewed changes

sebader reviewed Jun 7, 2022

View reviewed changes

tombuildsstuff modified the milestones: v3.10.0, v3.11.0 Jun 10, 2022

jackofallops modified the milestones: v3.11.0, v3.12.0 Jun 23, 2022

tombuildsstuff modified the milestones: v3.12.0, v3.13.0 Jul 1, 2022

manicminer modified the milestones: v3.13.0, v3.14.0 Jul 8, 2022

WodansSon added this to the v3.15.0 milestone Jul 13, 2022

WodansSon added 2 commits July 12, 2022 18:58

Merge branch 'main' into f/cdn-frontdoor-origin-and-groups

0d1d917

Make name check required

a102c0b

WodansSon marked this pull request as ready for review July 13, 2022 21:54

WodansSon added 3 commits July 13, 2022 16:20

fix rule set datasource test

3ff2fe1

fix test case part duex

2869a49

Fix test part wej

bc0a929

tombuildsstuff commented Jul 21, 2022

View reviewed changes

v3.14.0

a878d7c

WodansSon added 3 commits July 21, 2022 19:04

addressing PR comment

62f3f33

Merge branch 'main' of https://github.com/hashicorp/terraform-provide…

6c62b4d

…r-azurerm into f/cdn-frontdoor-origin-and-groups

go mod tidy

3f73092

github-actions bot added the dependencies label Jul 22, 2022

katbyte removed this from the v3.15.0 milestone Jul 22, 2022

WodansSon added this to the v3.16.0 milestone Jul 22, 2022

katbyte force-pushed the main branch from a878d7c to d5a19b0 Compare July 22, 2022 01:37

katbyte approved these changes Jul 22, 2022

View reviewed changes

Update changelog

7b573cf

WodansSon merged commit 3a01a8f into main Jul 22, 2022

WodansSon deleted the f/cdn-frontdoor-origin-and-groups branch July 22, 2022 03:03

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

Updating to include #17089

5c13669

WodansSon modified the milestones: v3.16.0, v3.15.0 Jul 22, 2022

github-actions bot locked as resolved and limited conversation to collaborators Aug 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Data Source/Resource: `azurerm_cdn_frontdoor_origin_group` / New Resource: `azurerm_cdn_frontdoor_origin` #17089

New Data Source/Resource: `azurerm_cdn_frontdoor_origin_group` / New Resource: `azurerm_cdn_frontdoor_origin` #17089

tombuildsstuff commented Jun 3, 2022

WodansSon Jun 4, 2022

tombuildsstuff Jul 21, 2022

WodansSon Jun 4, 2022

sebader Jun 6, 2022

tombuildsstuff Jun 7, 2022

WodansSon Jul 13, 2022

WodansSon Jun 4, 2022

sebader Jun 6, 2022

tombuildsstuff Jun 7, 2022

sebader Jun 7, 2022

sebader commented Jun 20, 2022

tombuildsstuff commented Jun 25, 2022

WodansSon commented Jul 13, 2022

WodansSon commented Jul 13, 2022 •

edited

Loading

WodansSon commented Jul 14, 2022

tombuildsstuff left a comment

tombuildsstuff Jul 21, 2022

WodansSon Jul 22, 2022

tombuildsstuff Jul 21, 2022

WodansSon Jul 21, 2022

tombuildsstuff Jul 21, 2022

tombuildsstuff Jul 21, 2022

WodansSon Jul 22, 2022

tombuildsstuff Jul 21, 2022

WodansSon Jul 22, 2022

tombuildsstuff Jul 21, 2022

WodansSon Jul 22, 2022

WodansSon commented Jul 22, 2022 •

edited

Loading

katbyte left a comment

katbyte Jul 22, 2022

WodansSon Jul 22, 2022

WodansSon commented Jul 22, 2022

github-actions bot commented Jul 25, 2022

github-actions bot commented Aug 25, 2022


		## Attributes Reference

		In addition to the Arguments listed above - the following Attributes are exported:


		* `location` - (Required) Specifies the location where the Private Link resource should exist.

		~> Note: For performance reasons this value should match the location of the `private_link_target_id` resource.

New Data Source/Resource: azurerm_cdn_frontdoor_origin_group / New Resource: azurerm_cdn_frontdoor_origin #17089

New Data Source/Resource: azurerm_cdn_frontdoor_origin_group / New Resource: azurerm_cdn_frontdoor_origin #17089

Conversation

tombuildsstuff commented Jun 3, 2022

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

Choose a reason for hiding this comment

sebader commented Jun 20, 2022

tombuildsstuff commented Jun 25, 2022

WodansSon commented Jul 13, 2022

WodansSon commented Jul 13, 2022 • edited Loading

WodansSon commented Jul 14, 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

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 Jul 22, 2022 • edited Loading

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

WodansSon commented Jul 22, 2022

github-actions bot commented Jul 25, 2022

github-actions bot commented Aug 25, 2022

New Data Source/Resource: `azurerm_cdn_frontdoor_origin_group` / New Resource: `azurerm_cdn_frontdoor_origin` #17089

New Data Source/Resource: `azurerm_cdn_frontdoor_origin_group` / New Resource: `azurerm_cdn_frontdoor_origin` #17089

WodansSon commented Jul 13, 2022 •

edited

Loading

WodansSon commented Jul 22, 2022 •

edited

Loading