New Resource - azurerm_role_assignment_marketplace
#22398
Conversation
…r-azurerm into role-assignment-marketplace2
…r-azurerm into role-assignment-marketplace2
github-actions
bot
added
dependencies
documentation
service/authorization
size/XL
labels
…r-azurerm into role-assignment-marketplace2
There was a problem hiding this comment.
Thanks for this PR @ms-zhenhua! I made a first pass and this looks mostly good. I'm unsure about the scoped ID parser that we're adding in this PR though and will need some time to discuss this internally.
Other than that just a few minor comments/suggestions/questions left in line.
| "delegated_managed_identity_resource_id": { | ||
| Type: pluginsdk.TypeString, | ||
| Optional: true, | ||
| ForceNew: true, | ||
| ValidateFunc: azure.ValidateResourceID, | ||
| }, |
There was a problem hiding this comment.
Could we use commonids.ValidateUserAssignedIdentityID here?
There was a problem hiding this comment.
Probably no. According to this issue: this property does not mean the managed identity resource id, but the resource which contains a managed identity.
| if v, ok := metadata.ResourceData.GetOk("role_definition_id"); ok { | ||
| roleDefinitionId = v.(string) | ||
| } else if v, ok := metadata.ResourceData.GetOk("role_definition_name"); ok { | ||
| roleName := v.(string) | ||
| roleDefinitions, err := roleDefinitionsClient.List(ctx, commonids.NewScopeID(scope), roledefinitions.ListOperationOptions{Filter: pointer.To(fmt.Sprintf("roleName eq '%s'", roleName))}) | ||
| if err != nil { | ||
| return fmt.Errorf("loading Role Definition List: %+v", err) | ||
| } | ||
|
|
||
| if roleDefinitions.Model != nil && len(*roleDefinitions.Model) == 1 && (*roleDefinitions.Model)[0].Id != nil { | ||
| roleDefinitionId = *(*roleDefinitions.Model)[0].Id | ||
| } else { | ||
| return fmt.Errorf("loading Role Definition List: failed to find role '%s'", roleName) | ||
| } | ||
| } else { | ||
| return fmt.Errorf("either 'role_definition_id' or 'role_definition_name' needs to be set") | ||
| } |
There was a problem hiding this comment.
This is personal preference but the multiple layers of if/else make this difficult to understand at first glance - could we refactor this to
| if v, ok := metadata.ResourceData.GetOk("role_definition_id"); ok { | |
| roleDefinitionId = v.(string) | |
| } else if v, ok := metadata.ResourceData.GetOk("role_definition_name"); ok { | |
| roleName := v.(string) | |
| roleDefinitions, err := roleDefinitionsClient.List(ctx, commonids.NewScopeID(scope), roledefinitions.ListOperationOptions{Filter: pointer.To(fmt.Sprintf("roleName eq '%s'", roleName))}) | |
| if err != nil { | |
| return fmt.Errorf("loading Role Definition List: %+v", err) | |
| } | |
| if roleDefinitions.Model != nil && len(*roleDefinitions.Model) == 1 && (*roleDefinitions.Model)[0].Id != nil { | |
| roleDefinitionId = *(*roleDefinitions.Model)[0].Id | |
| } else { | |
| return fmt.Errorf("loading Role Definition List: failed to find role '%s'", roleName) | |
| } | |
| } else { | |
| return fmt.Errorf("either 'role_definition_id' or 'role_definition_name' needs to be set") | |
| } | |
| if v, ok := metadata.ResourceData.GetOk("role_definition_id"); ok { | |
| roleDefinitionId = v.(string) | |
| } | |
| if v, ok := metadata.ResourceData.GetOk("role_definition_name"); ok { | |
| roleName := v.(string) | |
| roleDefinitions, err := roleDefinitionsClient.List(ctx, commonids.NewScopeID(scope), roledefinitions.ListOperationOptions{Filter: pointer.To(fmt.Sprintf("roleName eq '%s'", roleName))}) | |
| if err != nil { | |
| return fmt.Errorf("loading Role Definition List for %s: %+v", roleName, err) | |
| } | |
| if roleDefinitions.Model == nil || len(*roleDefinitions.Model) != 1 || (*roleDefinitions.Model)[0].Id == nil { | |
| return fmt.Errorf("loading Role Definition List: failed to find role '%s'", roleName) | |
| } | |
| roleDefinitionId = *(*roleDefinitions.Model)[0].Id | |
| } | |
| if roleDefinitionId == "" { | |
| return fmt.Errorf("either 'role_definition_id' or 'role_definition_name' needs to be set") | |
| } |
| if name == "" { | ||
| uuid, err := uuid.GenerateUUID() | ||
| if err != nil { | ||
| return fmt.Errorf("generating UUID for Role Assignment: %+v", err) | ||
| } | ||
|
|
||
| name = uuid | ||
| } | ||
|
|
||
| tenantId := "" | ||
| delegatedManagedIdentityResourceID := metadata.ResourceData.Get("delegated_managed_identity_resource_id").(string) | ||
| if len(delegatedManagedIdentityResourceID) > 0 { | ||
| var err error | ||
| tenantId, err = getTenantIdBySubscriptionId(ctx, subscriptionClient, subscriptionId) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
| if name == "" { | |
| uuid, err := uuid.GenerateUUID() | |
| if err != nil { | |
| return fmt.Errorf("generating UUID for Role Assignment: %+v", err) | |
| } | |
| name = uuid | |
| } | |
| tenantId := "" | |
| delegatedManagedIdentityResourceID := metadata.ResourceData.Get("delegated_managed_identity_resource_id").(string) | |
| if len(delegatedManagedIdentityResourceID) > 0 { | |
| var err error | |
| tenantId, err = getTenantIdBySubscriptionId(ctx, subscriptionClient, subscriptionId) | |
| if err != nil { | |
| return err | |
| } | |
| } | |
| var err error | |
| if name == "" { | |
| name, err = uuid.GenerateUUID() | |
| if err != nil { | |
| return fmt.Errorf("generating UUID for Role Assignment: %+v", err) | |
| } | |
| } | |
| tenantId := "" | |
| delegatedManagedIdentityResourceID := metadata.ResourceData.Get("delegated_managed_identity_resource_id").(string) | |
| if len(delegatedManagedIdentityResourceID) > 0 { | |
| tenantId, err = getTenantIdBySubscriptionId(ctx, subscriptionClient, subscriptionId) | |
| if err != nil { | |
| return err | |
| } | |
| } |
| } else if condition != "" || conditionVersion != "" { | ||
| return fmt.Errorf("`condition` and `conditionVersion` should be both set or unset") | ||
| } |
There was a problem hiding this comment.
Won't the RequiredWith in the schema already validate this/prevent this scenario from happening?
There was a problem hiding this comment.
yes, duplicated. Code updated.
| resp, err := client.GetById(ctx, commonids.NewScopeID(id.ScopedId.ID()), options) | ||
| if err != nil { | ||
| if response.WasNotFound(resp.HttpResponse) { | ||
| log.Printf("[DEBUG] Role Assignment ID %s was not found - removing from state", id) |
There was a problem hiding this comment.
| log.Printf("[DEBUG] Role Assignment ID %s was not found - removing from state", id) | |
| log.Printf("[DEBUG] %s was not found - removing from state", id) |
| if props.PrincipalType != nil { | ||
| metadata.ResourceData.Set("principal_type", string(*props.PrincipalType)) | ||
| } |
There was a problem hiding this comment.
| if props.PrincipalType != nil { | |
| metadata.ResourceData.Set("principal_type", string(*props.PrincipalType)) | |
| } | |
| metadata.ResourceData.Set("principal_type", pointer.From(props.PrincipalType)) |
| Assigns a given Principal (User or Group) to a given Role in Private Azure Marketplace. | ||
| --- | ||
|
|
||
| # azurerm_role_assignment_marketplace | ||
|
|
||
| Assigns a given Principal (User or Group) to a given Role in Private Azure Marketplace. |
There was a problem hiding this comment.
| Assigns a given Principal (User or Group) to a given Role in Private Azure Marketplace. | |
| --- | |
| # azurerm_role_assignment_marketplace | |
| Assigns a given Principal (User or Group) to a given Role in Private Azure Marketplace. | |
| Assigns a given Principal (User or Group) to a given Role in a Private Azure Marketplace. | |
| --- | |
| # azurerm_role_assignment_marketplace | |
| Assigns a given Principal (User or Group) to a given Role in a Private Azure Marketplace. |
|
|
||
| * `delegated_managed_identity_resource_id` - (Optional) The delegated Azure Resource ID which contains a Managed Identity. Changing this forces a new resource to be created. | ||
|
|
||
| ~> **NOTE:** this field is only used in cross tenant scenario. |
There was a problem hiding this comment.
| ~> **NOTE:** this field is only used in cross tenant scenario. | |
| ~> **NOTE:** This field is only used in cross tenant scenarios. |
|
|
||
| * `skip_service_principal_aad_check` - (Optional) If the `principal_id` is a newly provisioned `Service Principal` set this value to `true` to skip the `Azure Active Directory` check which may fail due to replication lag. This argument is only valid if the `principal_id` is a `Service Principal` identity. Defaults to `false`. Changing this forces a new resource to be created. | ||
|
|
||
| ~> **NOTE:** If it is not a `Service Principal` identity it will cause the role assignment to fail. |
There was a problem hiding this comment.
This note isn't clear on what we're expecting from the user - is this note saying that the principal_id cannot be set to any other type other than a Service Principal? Or should skip_service_principal_aad_check always be set to false if principal_id is something other than a Service Principal?
| ~> **NOTE:** If it is not a `Service Principal` identity it will cause the role assignment to fail. | |
| ~> **NOTE:** If it is not a `Service Principal` identity it will cause the role assignment to fail. |
There was a problem hiding this comment.
doc updated: This field takes effect only when principal_id is a Service Principal identity.
| terraform import azurerm_role_assignment_marketplace.example /providers/Microsoft.Marketplace/providers/Microsoft.Authorization/roleAssignments/00000000-0000-0000-0000-000000000000 | ||
| ``` | ||
|
|
||
| ~> **NOTE:** for cross tenant scenario, the format of `resource id` is composed of Azure resource ID and tenantId. for example: |
There was a problem hiding this comment.
| ~> **NOTE:** for cross tenant scenario, the format of `resource id` is composed of Azure resource ID and tenantId. for example: | |
| ~> **NOTE:** For cross tenant scenarios, the format of the `resource id` consists of the Azure resource ID and and the tenant ID, for example: |
|
Hi @stephybun, thank you for your review. I have resolved all comments. Kindly help take another review. Thanks. |
| func DestructRoleAssignmentId(id string) (string, string) { | ||
| parts := strings.Split(id, "|") | ||
| if len(parts) == 2 { | ||
| return parts[0], parts[1] | ||
| } | ||
| return id, "" | ||
| } |
There was a problem hiding this comment.
After internal discussion, this is fine for now. But we will want to create a composite ID type in the commonids package in the go-azure-helpers repo that can handle cases like these and will replace this in time. Just as an FYI.
| } | ||
|
|
||
| func (r RoleAssignmentMarketplaceResource) ResourceType() string { | ||
| return "azurerm_role_assignment_marketplace" |
There was a problem hiding this comment.
We have other role assignment resources where the scope comes before the resource name, can we do that here aswell please?
| return "azurerm_role_assignment_marketplace" | |
| return "azurerm_marketplace_role_assignment" |
There was a problem hiding this comment.
make sense. Code updated.
…role_assignment`
…r-azurerm into role-assignment-marketplace2
|
Hi @stephybun, thank you for your review. I have renamed the resource. Kindly take another review. Thanks. |
|
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. |
Related Links
#18387
#22178
#18439
Test Result
--- PASS: TestAccRoleAssignmentMarketplace_roleName (79.79s)
--- PASS: TestAccRoleAssignmentMarketplace_requiresImport (66.15s)
--- PASS: TestAccRoleAssignmentMarketplace_emptyName (71.63s)
--- PASS: TestAccRoleAssignmentMarketplace_ServicePrincipalWithType (71.94s)
--- PASS: TestAccRoleAssignmentMarketplace_builtin (72.01s)
--- PASS: TestAccRoleAssignmentMarketplace_ServicePrincipalGroup (78.87s)
--- PASS: TestAccRoleAssignmentMarketplace_ServicePrincipal (83.01s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/authorization 249.838s