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_definition - swap to go-azure-sdk #24266

Merged
merged 8 commits into from
Feb 6, 2024

Conversation

ziyeqf
Copy link
Contributor

@ziyeqf ziyeqf commented Dec 18, 2023

swap from azure-sdk-for-go to go-azure-sdk, keep the same API version.
In next PR, it will be upgraded to 2022-05-01-preview, for feature request.

And one more question is about the id of azurerm_role_definition, I made on #23679 (but removed from it):

The customized resource Id was invovled in #6107, at that time, the resource id set to state file came from Azure, and in some case the scope was not included in the resource ID, e.g: role definition with scope of management group.

But for go-azure-sdk, the id is ScopedRoledefinitionId which always contain a scope, maybe we do not need to use the customized resource Id? If the answer is yes, I will be glad to open another PR to change the resource id.

Test

terraform-provider-azurerm tengzh/rbac/sdk_swap ≡3s 
❯ tftest authorization TestAccRoleDefinition     
=== RUN   TestAccRoleDefinitionDataSource_basic
=== PAUSE TestAccRoleDefinitionDataSource_basic
=== RUN   TestAccRoleDefinitionDataSource_basicByName
=== PAUSE TestAccRoleDefinitionDataSource_basicByName
=== RUN   TestAccRoleDefinitionDataSource_builtIn_contributor
=== PAUSE TestAccRoleDefinitionDataSource_builtIn_contributor
=== RUN   TestAccRoleDefinitionDataSource_builtIn_owner
=== PAUSE TestAccRoleDefinitionDataSource_builtIn_owner
=== RUN   TestAccRoleDefinitionDataSource_builtIn_reader
=== PAUSE TestAccRoleDefinitionDataSource_builtIn_reader
=== RUN   TestAccRoleDefinitionDataSource_builtIn_virtualMachineContributor
=== PAUSE TestAccRoleDefinitionDataSource_builtIn_virtualMachineContributor
=== RUN   TestAccRoleDefinition_basic
=== PAUSE TestAccRoleDefinition_basic
=== RUN   TestAccRoleDefinition_requiresImport
=== PAUSE TestAccRoleDefinition_requiresImport
=== RUN   TestAccRoleDefinition_complete
=== PAUSE TestAccRoleDefinition_complete
=== RUN   TestAccRoleDefinition_update
=== PAUSE TestAccRoleDefinition_update
=== RUN   TestAccRoleDefinition_updateEmptyId
=== PAUSE TestAccRoleDefinition_updateEmptyId
=== RUN   TestAccRoleDefinition_emptyName
=== PAUSE TestAccRoleDefinition_emptyName
=== RUN   TestAccRoleDefinition_emptyPermissions
=== PAUSE TestAccRoleDefinition_emptyPermissions
=== RUN   TestAccRoleDefinition_managementGroup
=== PAUSE TestAccRoleDefinition_managementGroup
=== RUN   TestAccRoleDefinition_assignToSmallerScope
=== PAUSE TestAccRoleDefinition_assignToSmallerScope
=== RUN   TestAccRoleDefinition_noAssignableScope
=== PAUSE TestAccRoleDefinition_noAssignableScope
=== CONT  TestAccRoleDefinitionDataSource_basic
=== CONT  TestAccRoleDefinition_complete
=== CONT  TestAccRoleDefinition_emptyPermissions
=== CONT  TestAccRoleDefinition_requiresImport
=== CONT  TestAccRoleDefinition_assignToSmallerScope
=== CONT  TestAccRoleDefinition_noAssignableScope
=== CONT  TestAccRoleDefinitionDataSource_builtIn_reader
=== CONT  TestAccRoleDefinitionDataSource_basicByName
--- PASS: TestAccRoleDefinitionDataSource_builtIn_reader (20.41s)
=== CONT  TestAccRoleDefinition_managementGroup
    testcase.go:113: Step 1/2 error: Error running apply: exit status 1
        
        Error: unable to create Management Group "e271adc4-1e3b-4e42-a376-3cfafc1a2eba": managementgroups.Client#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="BadRequest" Message="Permission to Microsoft.Management/managementGroups on resources of type 'Write' is required on the management group or its ancestors." Details=[{"raw":"Management Group ID: '/providers/Microsoft.Management/managementGroups/72f988bf-86f1-41af-91ab-2d7cd011db47'"}]
        
          with azurerm_management_group.test,
          on terraform_plugin_test.tf line 24, in resource "azurerm_management_group" "test":
          24: resource "azurerm_management_group" "test" {
        
--- FAIL: TestAccRoleDefinition_managementGroup (11.76s)
=== CONT  TestAccRoleDefinition_updateEmptyId
--- PASS: TestAccRoleDefinition_requiresImport (237.98s)
=== CONT  TestAccRoleDefinition_emptyName
--- PASS: TestAccRoleDefinition_emptyPermissions (323.85s)
=== CONT  TestAccRoleDefinition_basic
--- PASS: TestAccRoleDefinition_assignToSmallerScope (327.99s)
=== CONT  TestAccRoleDefinition_update
--- PASS: TestAccRoleDefinitionDataSource_basic (387.61s)
=== CONT  TestAccRoleDefinitionDataSource_builtIn_virtualMachineContributor
--- PASS: TestAccRoleDefinitionDataSource_builtIn_virtualMachineContributor (17.68s)
=== CONT  TestAccRoleDefinitionDataSource_builtIn_owner
--- PASS: TestAccRoleDefinitionDataSource_builtIn_owner (17.12s)
=== CONT  TestAccRoleDefinitionDataSource_builtIn_contributor
--- PASS: TestAccRoleDefinitionDataSource_builtIn_contributor (20.01s)
--- PASS: TestAccRoleDefinition_complete (454.16s)
--- PASS: TestAccRoleDefinition_emptyName (236.10s)
--- PASS: TestAccRoleDefinition_noAssignableScope (485.73s)
--- PASS: TestAccRoleDefinitionDataSource_basicByName (562.71s)
--- PASS: TestAccRoleDefinition_updateEmptyId (595.59s)
--- PASS: TestAccRoleDefinition_basic (321.54s)
--- PASS: TestAccRoleDefinition_update (673.80s)
FAIL
FAIL	github.com/hashicorp/terraform-provider-azurerm/internal/services/authorization	1001.871s
FAIL

rerun the failed one on another subscription


terraform-provider-azurerm tengzh/rbac/sdk_swap*​ ≡
❯ tftest authorization TestAccRoleDefinition_managementGroup
=== RUN   TestAccRoleDefinition_managementGroup
=== PAUSE TestAccRoleDefinition_managementGroup
=== CONT  TestAccRoleDefinition_managementGroup
--- PASS: TestAccRoleDefinition_managementGroup (703.00s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/authorization	703.059s

azurerm_role_assignment also affected
image

roleResp, err := roleDefinitionsClient.GetByID(ctx, *roleId)
if roleDefResourceId := props.RoleDefinitionID; roleDefResourceId != nil {
// The role definition id returned does not contain scope when the scope is some special case.
// Such as management group. So we might need to add scope here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we be more specific here that the modification is needed to workaround the issue: hashicorp/pandora#3257 for roledefinitions.ParseScopedRoleDefinitionID for the roles assigned at the tenant level (both for tenant scope and mgmt group scope)?

@ziyeqf ziyeqf marked this pull request as ready for review December 19, 2023 05:06
@ziyeqf
Copy link
Contributor Author

ziyeqf commented Dec 19, 2023

I believe the CI failure is not caused by this PR..

@magodo
Copy link
Collaborator

magodo commented Jan 25, 2024

LGTM!

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Can we remove the workaround, since this is no longer needed?

Description *string `json:"description,omitempty"`
Permissions *[]roledefinitions.Permission `json:"permissions,omitempty"`
RoleName *string `json:"roleName,omitempty"`
Type *string `json:"type,omitempty"`
// not exposed in the sdk
Copy link
Contributor

Choose a reason for hiding this comment

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

These fields are available in the newer API version which is already used by the Provider - as such I believe this workaround is no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I planed to remove them in another PR, and do the API version upgrade at that PR, how do you think?

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! thanks for this @ziyeqf

@mbfrahry mbfrahry added this to the v3.91.0 milestone Feb 6, 2024
@mbfrahry mbfrahry merged commit 442d722 into hashicorp:main Feb 6, 2024
30 checks passed
mbfrahry added a commit that referenced this pull request Feb 6, 2024
lemeurherve pushed a commit to jenkins-infra/azure that referenced this pull request Feb 9, 2024
<Actions>
<action
id="f410411e63aff4bb73a81c2aec1d373cf8a903e63b30dee2006b0030d8a94cc8">
        <h3>Bump Terraform `azurerm` provider version</h3>
<details
id="1d9343c012f5434ac9fe8a98135bae3667b399259be16d9b14302ea3bd424a24">
            <summary>Update Terraform lock file</summary>
<p>changes detected:&#xA;&#x9;&#34;hashicorp/azurerm&#34; updated from
&#34;3.90.0&#34; to &#34;3.91.0&#34; in file
&#34;.terraform.lock.hcl&#34;</p>
            <details>
                <summary>3.91.0</summary>
<pre>Changelog retrieved
from:&#xA;&#x9;https://github.com/hashicorp/terraform-provider-azurerm/releases/tag/v3.91.0&#xA;FEATURES:&#xA;&#xA;*
**New Data Source**: `azurerm_databricks_access_connector`
([#24769](hashicorp/terraform-provider-azurerm#24769
**New Resource**:
`azurerm_data_protection_backup_policy_kubernetes_cluster`
([#24718](hashicorp/terraform-provider-azurerm#24718
**New Resource**: `azurerm_chaos_studio_experiment`
([#24779](hashicorp/terraform-provider-azurerm#24779
**New Resource**: `azurerm_chaos_studio_capability`
([#24779](hashicorp/terraform-provider-azurerm#24779
**New Resource**: `azurerm_dev_center_gallery`
([#23760](hashicorp/terraform-provider-azurerm#23760
**New Resource:** `azurerm_kubernetes_fleet_member`
([#24792](hashicorp/terraform-provider-azurerm#24792
**New Resource:** `azurerm_iotcentral_organization`
([#23132](hashicorp/terraform-provider-azurerm#23132
**New Resource:**
`azurerm_spring_cloud_app_dynamics_application_performance_monitoring`
([#24750](https://github.com/hashicorp/terraform-provider-azurerm/issues/24750))&#xA;&#xA;ENHANCEMENTS:&#xA;&#xA;*
dependencies: updating to `v0.20240208.1095436` of
`github.com/hashicorp/go-azure-sdk/resource-manager`
([#24819](hashicorp/terraform-provider-azurerm#24819
dependencies: updating to `v0.20240208.1095436` of
`github.com/hashicorp/go-azure-sdk/sdk`
([#24819](hashicorp/terraform-provider-azurerm#24819
dependencies: refactor `azurerm_app_service_environment_v3` to use
`go-azure-sdk`
([#24760](hashicorp/terraform-provider-azurerm#24760
dependencies: refactor `azurerm_role_definition` to use `go-azure-sdk`
([#24266](hashicorp/terraform-provider-azurerm#24266
`managedhsm`: updating to use the transport layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest`
([#24761](hashicorp/terraform-provider-azurerm#24761
`hdinsight`: updating to API Version `2023-07-01`
([#24761](hashicorp/terraform-provider-azurerm#24761
`streamanalytics`: updating to use the transport layer from
`hashicorp/go-azure-sdk` rather than `Azure/go-autorest`
([#24819](hashicorp/terraform-provider-azurerm#24819
`azurerm_app_service_environment_v3` - support for the
`remote_debugging_enabled` property
([#24760](hashicorp/terraform-provider-azurerm#24760
`azurerm_storage_account` - support for the `local_user_enabled`
property
([#24800](hashicorp/terraform-provider-azurerm#24800
`azurerm_log_analytics_workspace_table` - support for the
`total_retention_in_days` property
([#24513](hashicorp/terraform-provider-azurerm#24513
`azurerm_maching_learning_workspace` - support for the `feature_store`
and `kind` properties
([#24716](hashicorp/terraform-provider-azurerm#24716
`azurerm_traffic_manager_azure_endpoint` - support for the
`always_serve_enabled` property
([#24573](hashicorp/terraform-provider-azurerm#24573
`azurerm_traffic_manager_external_endpoint` - support for the
`always_serve_enabled` property
([#24573](https://github.com/hashicorp/terraform-provider-azurerm/issues/24573))&#xA;&#xA;BUG
FIXES:&#xA;&#xA;* `azurerm_api_management` - the
`virtual_network_configuration` property now updates correctly outside
of `virtual_network_type`
([#24569](https://github.com/hashicorp/terraform-provider-azurerm/issues/24569))&#xA;&#xA;&#xA;</pre>
            </details>
        </details>
<a
href="https://infra.ci.jenkins.io/job/terraform-jobs/job/azure/job/main/1083/">Jenkins
pipeline link</a>
    </action>
</Actions>

---

<table>
  <tr>
    <td width="77">
<img src="https://www.updatecli.io/images/updatecli.png" alt="Updatecli
logo" width="50" height="50">
    </td>
    <td>
      <p>
Created automatically by <a
href="https://www.updatecli.io/">Updatecli</a>
      </p>
      <details><summary>Options:</summary>
        <br />
<p>Most of Updatecli configuration is done via <a
href="https://www.updatecli.io/docs/prologue/quick-start/">its
manifest(s)</a>.</p>
        <ul>
<li>If you close this pull request, Updatecli will automatically reopen
it, the next time it runs.</li>
<li>If you close this pull request and delete the base branch, Updatecli
will automatically recreate it, erasing all previous commits made.</li>
        </ul>
        <p>
Feel free to report any issues at <a
href="https://github.com/updatecli/updatecli/issues">github.com/updatecli/updatecli</a>.<br
/>
If you find this tool useful, do not hesitate to star <a
href="https://github.com/updatecli/updatecli/stargazers">our GitHub
repository</a> as a sign of appreciation, and/or to tell us directly on
our <a
href="https://matrix.to/#/#Updatecli_community:gitter.im">chat</a>!
        </p>
      </details>
    </td>
  </tr>
</table>

Co-authored-by: Jenkins Infra Bot (updatecli) <60776566+jenkins-infra-bot@users.noreply.github.com>
@ziyeqf ziyeqf deleted the tengzh/rbac/sdk_swap branch February 26, 2024 02:27
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 Apr 21, 2024
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