azurerm_cdn_frontdoor_route - make cdn_frontdoor_origin_ids field optional#29350
azurerm_cdn_frontdoor_route - make cdn_frontdoor_origin_ids field optional#29350sreallymatt merged 9 commits intomainfrom
azurerm_cdn_frontdoor_route - make cdn_frontdoor_origin_ids field optional#29350Conversation
…r-azurerm into d_frontdoor_route
There was a problem hiding this comment.
This may have already been considered, but I was thinking about how giving the route a property for origin IDs only reflects part of what's actually happening at the Azure API level. As it is probably known, routes don't depend on specific origins—they just need their associated origin group to have at least one enabled origin before creation of the route can succeed.
What if we handled the issue in the provider itself with a custom poller? It could catch the specific "origin group needs at least one enabled origin" error from Azure and retry route creation for a few minutes. The Portal does not allow for origin groups with zero enabled origins, but the REST API does, even though such groups are of no practical value. If an origin group exists, I believe it's safe to assume that soon it will have at least one enabled origin.
The poller approach would:
- be more semantically accurate
- remove the need for workarounds in user configurations
- handle the timing automatically
The downside is a potential added delay during apply if origins aren't ready immediately, but that seems like a reasonable trade-off for cleaner resource modeling. Curious what others think about this approach, or if there are better alternatives, or reasons why the custom poller one wouldn't work?
There was a problem hiding this comment.
Could you replace the utils.String() etc with pointer.To()?
|
|
||
| Manages a Front Door (standard/premium) Route. | ||
|
|
||
| !>**Note:** The `azurerm_cdn_frontdoor_route` resource must **explicitly** reference its associated `azurerm_cdn_frontdoor_origin` resource(s). This can be achieved either by using a `depends_on` meta-argument that points to the `azurerm_cdn_frontdoor_origin` resource(s), or by specifying the `azurerm_cdn_frontdoor_origin` IDs via the `cdn_frontdoor_origin_ids` field. |
There was a problem hiding this comment.
| !>**Note:** The `azurerm_cdn_frontdoor_route` resource must **explicitly** reference its associated `azurerm_cdn_frontdoor_origin` resource(s). This can be achieved either by using a `depends_on` meta-argument that points to the `azurerm_cdn_frontdoor_origin` resource(s), or by specifying the `azurerm_cdn_frontdoor_origin` IDs via the `cdn_frontdoor_origin_ids` field. | |
| !> **Note:** The `azurerm_cdn_frontdoor_route` resource must **explicitly** reference its associated `azurerm_cdn_frontdoor_origin` resource(s). This can be achieved either by using a `depends_on` meta-argument that points to the `azurerm_cdn_frontdoor_origin` resource(s), or by specifying the `azurerm_cdn_frontdoor_origin` IDs via the `cdn_frontdoor_origin_ids` field. |
| Optional: true, | ||
|
|
||
| Elem: &pluginsdk.Schema{ |
There was a problem hiding this comment.
I've seen both ways in the code, but it seems most of the time there's no new line before Elem:
| Optional: true, | |
| Elem: &pluginsdk.Schema{ | |
| Optional: true, | |
| Elem: &pluginsdk.Schema{ |
That would likely be a more modern solution to the API’s provisioning order issue for this resource, as it would let Terraform handle the behavior more gracefully rather than relying on faux fields. However, the custom poller wasn’t really an option when this resource/PR was first created. I agree the custom poller approach is worth considering going forward, but for the scope of this PR, can we set that aside? I’m happy to open a follow-up PR to introduce the custom poller. For now, can we move this PR forward to address the current customer needs without that added functionality? |
Yes, the delay would be regrettable. I'm sorry to ask more of you, but from the user's perspective, I believe the improved experience a custom poller would provide would be worth the additional wait. Are there time-sensitive situations that would make the delay too costly? |
|
|
||
| Manages a Front Door (standard/premium) Route. | ||
|
|
||
| !> **Note:** The `azurerm_cdn_frontdoor_route` resource must **explicitly** reference its associated `azurerm_cdn_frontdoor_origin` resource(s). This can be achieved either by using a `depends_on` meta-argument that points to the `azurerm_cdn_frontdoor_origin` resource(s), or by specifying the `azurerm_cdn_frontdoor_origin` IDs via the `cdn_frontdoor_origin_ids` field. |
There was a problem hiding this comment.
We'd like to save the "!>" notes for cases where costly or irreversible damage could occur. The "~>" is a good choice for advising users how to avoid minor errors.
| !> **Note:** The `azurerm_cdn_frontdoor_route` resource must **explicitly** reference its associated `azurerm_cdn_frontdoor_origin` resource(s). This can be achieved either by using a `depends_on` meta-argument that points to the `azurerm_cdn_frontdoor_origin` resource(s), or by specifying the `azurerm_cdn_frontdoor_origin` IDs via the `cdn_frontdoor_origin_ids` field. | |
| ~> **Note:** The `azurerm_cdn_frontdoor_route` resource must **explicitly** reference its associated `azurerm_cdn_frontdoor_origin` resource(s). This can be achieved either by using a `depends_on` meta-argument that points to the `azurerm_cdn_frontdoor_origin` resource(s), or by specifying the `azurerm_cdn_frontdoor_origin` IDs via the `cdn_frontdoor_origin_ids` field. |
| ~> **Note:** The `cdn_frontdoor_origin_ids` field is not sent to the Azure API, it exists solely to ensure Terraform can manage the correct `provisioning` and `destruction` order of related resources. When importing an existing `azurerm_cdn_frontdoor_route` resource, you will need to manually add the `cdn_frontdoor_origin_ids` field to your configuration after the resource has been successfully imported. | ||
|
|
||
| ~> **Note:** If the `cdn_frontdoor_origin_ids` field is not defined in the configuration, you **must** use a `depends_on` meta-argument that references the corresponding `azurerm_cdn_frontdoor_origin` resource(s) for the route. When importing an existing `azurerm_cdn_frontdoor_route` resource from Azure, you will need to manually add the `depends_on` meta-argument to your configuration after the resource has been successfully imported. |
There was a problem hiding this comment.
Could these two notes be condensed and combined?
…n_ids in azurerm_cdn_frontdoor_route documentation
catriona-m
left a comment
There was a problem hiding this comment.
Hi @WodansSon thanks for fixing this up. It looks like there are some conflicts needing resolved on this, but otherwise it looks like this is nearly ready to go. Thanks!
…r-azurerm into d_frontdoor_route
Thanks for the calling that out, @catriona-m, I have fixed the conflicts and am running the tests again. 🙂 --- PASS: TestAccCdnFrontDoorRoute_complete (1606.21s)
--- PASS: TestAccCdnFrontDoorRoute_requiresImport (1611.69s)
--- PASS: TestAccCdnFrontDoorRoute_basic (1622.74s)
--- PASS: TestAccCdnFrontDoorRoute_basicDependsOnAndField (1629.25s)
--- PASS: TestAccCdnFrontDoorRoute_basicDependsOn (1696.55s)
--- PASS: TestAccCdnFrontDoorRoute_update (1746.74s)
--- PASS: TestAccCdnFrontDoorRoute_originPath (1752.09s) |
|
Test after adding the |
|
A custom poller can mitigate the issue in creation, but there are two cases:
I agree we should make |
sreallymatt
left a comment
There was a problem hiding this comment.
Thanks @WodansSon - LGTM ✅
Community Note
Description
In issue #29063 a community member raised an issue that:
cdn_frontdoor_origin_ids, it set tonullon importWhile this is by design, some community members stated that they would prefer that the
cdn_frontdoor_origin_idsfield be deprecated in favor of adepends_onargument. This PR addresses both issues allowing for both, with the caveat that this may cause more issues moving forward due to community members personal preference. In the design of this resource exposing thecdn_frontdoor_origin_idsfield was intentional, showing the relationship between the resources and allowing the Terraform core runtime to provision and destroy the resources in the correct order to avoid errors. With this change, while it will work, it puts that duty on the end user to ensure the correct order/relationship of the resources based on their personal preference (e.g.,cdn_frontdoor_origin_idsor adepends_onargument).PR Checklist
For example: “
resource_name_here- description of change e.g. adding propertynew_property_name_here”Changes to existing Resource / Data Source
Testing
Change Log
Below please provide what should go into the changelog (if anything) conforming to the Changelog Format documented here.
azurerm_cdn_frontdoor_route- makecdn_frontdoor_origin_idsfieldoptional[GH-00000]This is a (please select all that apply):
Related Issue(s)
Fixes #29063
Note
If this PR changes meaningfully during the course of review please update the title and description as required.