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_role_assignment" supports property "delegated_managed_identity_resource_id" #11848

Merged
merged 4 commits into from
Jun 2, 2021

Conversation

njuCZ
Copy link
Contributor

@njuCZ njuCZ commented May 25, 2021

fix #9348

docs: https://docs.microsoft.com/en-us/azure/lighthouse/how-to/deploy-policy-remediation

this property does not mean the managed identity resource id, but the resource which contains a managed identity.
It's used for a cross tenant scenario. assume there are two tenants and two service principals in each tenant:

Tenant subscription User
tenantA subscriptionA userA
tenantB subscriptionB userB

By this new property, userA in tenantA could do role assignment in subscriptionB.

specific test step:

  1. userB should delegate subscriptionB to userA by using lighthouse RP. userB could do it in terraform in following script
provider "azurerm" {
  features {}
}

variable "managing_tenant_id" {
  type = string
  // default = tenantA
}

variable "managing_principal_id" {
  type = string
  // default = userA
}

data "azurerm_role_definition" "user_access_administrator" {
  role_definition_id = "18d7d88d-d35e-4fb5-a5c3-7773c20a72d9"
}

data "azurerm_role_definition" "contributor" {
  role_definition_id = "b24988ac-6180-42a0-ab88-20f7382dd24c"
}

data "azurerm_subscription" "test" {}

resource "azurerm_lighthouse_definition" "test" {
  name                     = "acctest-delegate"
  description              = "Acceptance Test Lighthouse Definition"
  managing_tenant_id       = var.managing_tenant_id
  scope                    = data.azurerm_subscription.test.id

  authorization {
    principal_id           = var.managing_principal_id
    role_definition_id     = data.azurerm_role_definition.user_access_administrator.role_definition_id
    delegated_role_definition_ids = [
      data.azurerm_role_definition.contributor.role_definition_id,
    ]
  }

  authorization {
    principal_id           = var.managing_principal_id
    role_definition_id     = data.azurerm_role_definition.contributor.role_definition_id
  }
}

resource "azurerm_lighthouse_assignment" "test" {
  scope                    = data.azurerm_subscription.test.id
  lighthouse_definition_id = azurerm_lighthouse_definition.test.id
}
  1. userA should switch to use subscriptionB
  2. userA execute the acctest

notes, there is an api limitation:
for cross tenant role assignment, we should pass the tenantId in the request, otherwise it could not read the resource.
But in normal cases, we could not pass the tenantId property, otherwise it will report error.

changes in this PR:
to fix the above limitation, in normal case, I don't change the id format. For cross tenant scenario, I append the tenantId into the id. This is not breaking change.

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 @njuCZ - LGTM 👍

@@ -197,19 +222,19 @@ func resourceArmRoleAssignmentCreate(d *pluginsdk.ResourceData, meta interface{}
properties.RoleAssignmentProperties.PrincipalType = authorization.ServicePrincipal
}

if err := pluginsdk.Retry(d.Timeout(pluginsdk.TimeoutCreate), retryRoleAssignmentsClient(d, scope, name, properties, meta)); err != nil {
if err := pluginsdk.Retry(d.Timeout(schema.TimeoutCreate), retryRoleAssignmentsClient(d, scope, name, properties, meta, tenantId)); err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should be pluginsdk.TimeoutCreate, I will submit a PR to fix it soon

Copy link
Collaborator

Choose a reason for hiding this comment

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

ty i was trying to resolve it via github - didn't go as well as i'd hoped 😓

@katbyte katbyte merged commit 359ce78 into hashicorp:master Jun 2, 2021
katbyte added a commit that referenced this pull request Jun 2, 2021
@ghost
Copy link

ghost commented Jun 4, 2021

This has been released in version 2.62.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.62.0"
}
# ... other configuration ...

@w0ut0
Copy link

w0ut0 commented Jun 4, 2021

Is it possible this causes #12060?

@HarleyB123
Copy link
Contributor

Hey, I think @w0ut0 is right, see my findings on that issue here.

@njuCZ I'm not sure how all the pieces fit together - hoping you might be able to see where it's going wrong 😄

@aristosvo
Copy link
Collaborator

The problem seems to be surfacing when the scope is on resource level instead of subscription, resource group etc.

Unfortunately this is not a scenario covered in the Acceptance Tests as far as I can see.

@njuCZ
Copy link
Contributor Author

njuCZ commented Jun 7, 2021

@w0ut0, @HarleyB123, @aristosvo sorry for causing the problem. I am glad to see that PR #12076 could fix it.

@etaham
Copy link

etaham commented Jun 8, 2021

Can this be patched? It is causing state corruption for existing role assignments.

@github-actions
Copy link

github-actions bot commented Jul 9, 2021

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 Jul 9, 2021
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.

Support for delegatedManagedIdentityResourceId to azurerm_role_definition resource
6 participants