Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

azurerm_policy_set_definition/azurerm_policy_definition - Reverse the order of policies lookup to favour builtin #18338

Merged
merged 1 commit into from
Oct 28, 2022

Conversation

Nepomuceno
Copy link
Contributor

@Nepomuceno Nepomuceno commented Sep 10, 2022

This one would solve #18337

It changed the plan time from over 2 minutes to less than 40 seconds fot the sample case

terraform {
}

provider "azurerm" {
  features {}
}

data "azurerm_policy_set_definition" "policy_set" {
  name = "179d1daa-458f-4e47-8086-2a68d0d6c38f"
}

data "azurerm_policy_definition" "for_each_pol" {
    count = length(data.azurerm_policy_set_definition.policy_set.policy_definition_reference)
    name = data.azurerm_policy_set_definition.policy_set.policy_definition_reference[0].reference_id
}

There could be a solution long term for you to sepecify the policy type when creating a data block but those would probablt be corner cases.

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Nepomuceno - however i don't think we can just change the order without impacting existing users (and the ability to lookup a custom policy with the same name as a built in policy)

instead how about we add 2 new properties? a name_builtin and name_user_defined which allows a user to specify the type? this will allow you to save the time while also not impacting existing users. WDYT?

@Nepomuceno
Copy link
Contributor Author

This would have no change pratically for who it is using the provider already. It would just change when those throtololing would happen. for the vast majority of people their tplan would just become a lot faster.

This is not a breaking change.

We could add just a flag built_in and keep just the name property if this flag was not set I would look at both if this flag was set I woudl look only on the built in ones or I would not look in the built in ones if it was set but false.

Burt even adding this property does not change the fact that whejn looking at both it is more efficient to look first in the built in ones.

@wuxu92
Copy link
Contributor

wuxu92 commented Sep 22, 2022

what about using a parallel query: one for built-in policy and another for custom policy and then joining both results? there will be no scheme update, and users will not aware of it.

@Nepomuceno
Copy link
Contributor Author

This would just increase the problem. The problem right now it is that if you have too many policies to lookup you go over the 120 queries per minute limit and needs to keep waiting the throttling. If you execute both queries always you will reach this even earlier.

@katbyte
Copy link
Collaborator

katbyte commented Sep 22, 2022

@Nepomuceno - on internal discussion the consensus is yes it could affect users in the case their custom policy is named the same as the builtin. I think the best way is to just add two new properties that users can switch to depending on what type of policy they are looking for & document this can save a lot of time for built-in policies (as well provide a way to limit the search)

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! After another internal discussion, we should be ok here as @katbyte was thinking about display_name versus name. Thanks for getting this written out @Nepomuceno

@mbfrahry mbfrahry changed the title Reverse the order of policies lookup to favour builtin azurerm_policy_set_definition/azurerm_policy_definition - Reverse the order of policies lookup to favour builtin Oct 28, 2022
@mbfrahry mbfrahry added this to the v3.30.0 milestone Oct 28, 2022
@mbfrahry mbfrahry merged commit c87c3bf into hashicorp:main Oct 28, 2022
mbfrahry added a commit that referenced this pull request Oct 28, 2022
@Nepomuceno
Copy link
Contributor Author

Yeah calling this property "name" it is quite misleading. It should have been called ID

@github-actions
Copy link

github-actions bot commented Nov 4, 2022

This functionality has been released in v3.30.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
Copy link

github-actions bot commented Dec 5, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants