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: Fix assignments to resources #12076

Merged
merged 5 commits into from
Jun 8, 2021

Conversation

aristosvo
Copy link
Collaborator

@aristosvo aristosvo commented Jun 5, 2021

Fixes #12074
Fixes #12060
Fixes #12057
Fixes #12079
Fixes #12078
Fixes #12087

Related to/similar for go-azure-helpers: this PR

Done:

  • Reproduce the problem :)
❯ make acctests SERVICE='authorization' TESTARGS='-run=TestAccRoleAssignment_resourceScoped'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./azurerm/internal/services/authorization -run=TestAccRoleAssignment_resourceScoped -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/06/05 16:11:49 [DEBUG] not using binary driver name, it's no longer needed
2021/06/05 16:11:50 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccRoleAssignment_resourceScoped
=== PAUSE TestAccRoleAssignment_resourceScoped
=== CONT  TestAccRoleAssignment_resourceScoped
    testing.go:620: Step 1/2 error: Error running apply: exit status 1
        
        Error: Provider produced inconsistent result after apply
        
        When applying changes to azurerm_role_assignment.test, provider
        "provider[\"registry.terraform.io/hashicorp/azurerm\"]" produced an
        unexpected new value: Root resource was present, but now absent.
        
        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.
--- FAIL: TestAccRoleAssignment_resourceScoped (136.30s)
FAIL
FAIL    github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/authorization 139.640s
FAIL
make: *** [acctests] Error 1
  • Diagnose the problem:

    • Previously the ID was saved raw and without transformation/checks. Because the roleAssignmentID parser doesn't contain a resource/scope part, it wasn't saved and therefore not (able to be) regenerated correctly based on saved parameters: /subscriptions/<subscription>/resourceGroups/<resource_group>/providers/Microsoft.Storage/storageAccounts/<storage_account_name>/providers/Microsoft.Authorization/roleAssignments/<UUID> became /subscriptions/<subscription>/resourceGroups/<resource_group>/providers/Microsoft.Authorization/roleAssignments/<UUID>
  • Implement the fix

❯ make acctests SERVICE='authorization' TESTARGS='-run=TestAccRoleAssignment_resourceScoped'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./azurerm/internal/services/authorization -run=TestAccRoleAssignment_resourceScoped -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
2021/06/06 08:41:44 [DEBUG] not using binary driver name, it's no longer needed
2021/06/06 08:41:45 [DEBUG] not using binary driver name, it's no longer needed
=== RUN   TestAccRoleAssignment_resourceScoped
=== PAUSE TestAccRoleAssignment_resourceScoped
=== CONT  TestAccRoleAssignment_resourceScoped
--- PASS: TestAccRoleAssignment_resourceScoped (191.83s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm/internal/services/authorization       195.451s
  • Clean up
    • Fix the unit tests of other resources by providing a general id.SecondaryProvider, which solved the problems for the resources which have multiple provider entries in their IDs

@ghost ghost added size/S size/M and removed size/S labels Jun 5, 2021
@aristosvo aristosvo force-pushed the fix/azurerm_role_assignment branch 5 times, most recently from 705c7c7 to df14777 Compare June 6, 2021 06:46
@ghost ghost added size/L and removed size/M labels Jun 6, 2021
@aristosvo aristosvo force-pushed the fix/azurerm_role_assignment branch from df14777 to 00f0880 Compare June 6, 2021 06:53
@aristosvo aristosvo marked this pull request as ready for review June 6, 2021 07:41
@njuCZ
Copy link
Contributor

njuCZ commented Jun 7, 2021

@aristosvo thanks for this PR. I just provide another way to quick fix that might be simpler:
replace Line 247 with

id, err := parseRoleAssignmentId(d.Id())

then we could construct the correct resource ID by following, and use it in the read function:

fmt.Sprintf("%s/providers/Microsoft.Authorization/roleAssignments/%s", id.scope, id.name)

@aristosvo
Copy link
Collaborator Author

@njuCZ I don't see how your suggestion might fix the bug, but happy to help.

I believe this PR is complete and a forward compatible improvement, not just a fix, but correct me please if I'm wrong.

@njuCZ
Copy link
Contributor

njuCZ commented Jun 7, 2021

@aristosvo thanks for this PR, I just left some personal thought. Hopes to see it merged soon.

@sdebruyn
Copy link
Contributor

sdebruyn commented Jun 7, 2021

Can someone help with some kind of script to repair the mess that 2.62.0 created?
The PR will fix the issue by itself, but all role assignments are gone from the state files.

Importing them is a PITA, especially since the ID is generated by Azure and not something you can easily piece together. So you'll have to query with the Azure CLI to get the IDs to properly import in Terraform. Something that mimicks an "auto-import" feature would be a perfect solution.

I have to fix this in tens of projects and not looking forward to a manual process.

@aristosvo
Copy link
Collaborator Author

aristosvo commented Jun 7, 2021

@sdebruyn Dependent on your experience with the Azure provider forTerraform.., one dangerous and wild option is to remove the logic which is preventing recreating unimported resources for this specific resource, build your custom Azure TF provider and run it against your projects...?

It's probably better to have this discussion in one of the issues, as it is now cluttering this PR.

@katbyte katbyte added this to the v2.63.0 milestone Jun 7, 2021
@sdebruyn
Copy link
Contributor

sdebruyn commented Jun 8, 2021

I have a script ready and I'll share it when it succeeds. But I can't pin on .61 anymore since it won't decode azurerm_api_management_subscription from the state anymore with that version (another change in .62)

@aristosvo
Copy link
Collaborator Author

aristosvo commented Jun 8, 2021

@sdebruyn I can help you, but please post your question/solution under one of 8 open issues regarding this fix, not under the PR! I want this to be merged ASAP, this doesn't help with oversight.

Check DMs in Slack for direct communication:)

@etaham
Copy link

etaham commented Jun 8, 2021

Can this be patched? The bug causing state corruption for existing role assignments. The existing role assignments are being removed when state is applied. We needed to rollback to 2.61 and then import the assignments to get them back into state.

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 a ton @aristosvo - this LGTM 🚀

@katbyte katbyte merged commit f2ee194 into hashicorp:master Jun 8, 2021
@katbyte katbyte modified the milestones: v2.63.0, v2.62.1 Jun 8, 2021
katbyte pushed a commit that referenced this pull request Jun 8, 2021
@etaham
Copy link

etaham commented Jun 8, 2021

Thank you!!

@aristosvo aristosvo deleted the fix/azurerm_role_assignment branch June 8, 2021 16:55
katbyte added a commit that referenced this pull request Jun 8, 2021
@ghost
Copy link

ghost commented Jun 8, 2021

This has been released in version 2.62.1 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.1"
}
# ... other configuration ...

@Richard-CDS
Copy link

Richard-CDS commented Jun 16, 2021

I have a script ready and I'll share it when it succeeds. But I can't pin on .61 anymore since it won't decode azurerm_api_management_subscription from the state anymore with that version (another change in .62)

@sdebruyn - Just wondering if you were able to share your script to fix the state file?

@github-actions
Copy link

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 17, 2021
@aristosvo aristosvo restored the fix/azurerm_role_assignment branch April 14, 2023 07:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.