-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
resourceArmRoleAssignment: Allow /providers/Micrososoft.Capacity
, /providers/BillingBenefits
and /
as scope value
#26663
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this @tiwood, this looks good but could we add a test to ensure this works as expected? Thanks!
Hi @catriona-m, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tiwood, I ran the tests and it is failing with this error:
------- Stdout: -------
=== RUN TestAccRoleAssignment_capacityProviderScoped
=== PAUSE TestAccRoleAssignment_capacityProviderScoped
=== CONT TestAccRoleAssignment_capacityProviderScoped
testcase.go:113: Step 1/2 error: Error running apply: exit status 1
Error: loading Role Definition List: unexpected status 404 (404 Not Found) with error: InvalidResourceNamespace: The resource namespace 'Micrososoft.Capacity' is invalid.
with azurerm_role_assignment.test,
on terraform_plugin_test.tf line 27, in resource "azurerm_role_assignment" "test":
27: resource "azurerm_role_assignment" "test" {
--- FAIL: TestAccRoleAssignment_capacityProviderScoped (20.46s)
FAIL
If we can get this fixed up we can take another look at this. Thanks!
Hi @catriona-m, thats because the If you ask me, it would be better to exclude this test as the impact/issues trying to get the provider registered outweighs the benefit to verify the regex works. WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tiwood I checked our subscription and the RP Microsoft.Capacity
is registered on it. It looks like the error is being caused by a typo.
@@ -67,6 +67,9 @@ func resourceArmRoleAssignment() *pluginsdk.Resource { | |||
// It seems only user account is allowed to be elevated access. | |||
validation.StringMatch(regexp.MustCompile("/providers/Microsoft.Subscription.*"), "Subscription scope is invalid"), | |||
|
|||
// This scope is used for the Reservations roles (Reservation Purchaser, Reservation Reader, etc.) | |||
validation.StringMatch(regexp.MustCompile("/providers/Micrososoft.Capacity"), "Capacity scope is invalid"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validation.StringMatch(regexp.MustCompile("/providers/Micrososoft.Capacity"), "Capacity scope is invalid"), | |
validation.StringMatch(regexp.MustCompile("/providers/Microsoft.Capacity"), "Capacity scope is invalid"), |
data "azurerm_client_config" "test" {} | ||
|
||
resource "azurerm_role_assignment" "test" { | ||
scope = "/providers/Micrososoft.Capacity" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scope = "/providers/Micrososoft.Capacity" | |
scope = "/providers/Microsoft.Capacity" |
@stephybun, you're right, I'm going to fix that. Nevertheless the provider needs to be registered on the global scope ( Example: az rest --method POST --url https://management.azure.com/providers/Microsoft.CostManagementExports/register\?api-version\=2024-03-01 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @tiwood. We usually try to make sure our tenant/subscription has all the necessary registrations and permissions outside of Terraform to enable thorough testing. Since we lack the permissions to register this on a tenant level the acceptance test probably doesn't make a lot of sense as you say. Could you remove it, this should be good to go then.
… available for all developers
/providers/Micrososoft.Capacity
as scope value/providers/Micrososoft.Capacity
, /providers/BillingBenefits
and /
as scope value
@stephybun , I've removed the tests and also added two additional items to the list of valid scopes (also updated the PR details):
|
Hi @stephybun, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Community Note
Description
To allow principals to manage
reservation orders
,savings plans
or global roles at/
we have to assign the appropriate roles to global provider scopes like/providers/Micrososoft.Capacity
.This PR adds this string als valid scope in the resource
azurerm_role_assignment
.Example:
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_role_assignment
- support for using/providers/Micrososoft.Capacity
in the scope property [GH-00000]This is a (please select all that apply):
Related Issue(s)
Fixes #0000
Note
If this PR changes meaningfully during the course of review please update the title and description as required.