Skip to content

{Role} Fix: az role assignment list-changelogs fails with KeyError#14461

Merged
jiasli merged 7 commits intoAzure:devfrom
jiasli:list-changelogs
Sep 10, 2020
Merged

{Role} Fix: az role assignment list-changelogs fails with KeyError#14461
jiasli merged 7 commits intoAzure:devfrom
jiasli:list-changelogs

Conversation

@jiasli
Copy link
Member

@jiasli jiasli commented Jul 22, 2020

Symptom

az role assignment list-changelogs fails with KeyError:

> az role assignment list-changelogs --start-time 2020-07-21T12:29:00Z --end-time 2020-07-21T12:30:00Z
The command failed with an unexpected error. Here is the traceback:

'/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/aro-nw3napmz/providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c'
Traceback (most recent call last):
  File "C:\Users\VSSADM~1\AppData\Local\Temp\pip-install-_kvuguic\knack\knack\cli.py", line 215, in invoke
  File "D:\a\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\azure\cli\core\commands\__init__.py", line 654, in execute
  File "D:\a\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\azure\cli\core\commands\__init__.py", line 718, in _run_jobs_serially
  File "D:\a\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\azure\cli\core\commands\__init__.py", line 711, in _run_job
  File "C:\Users\VSSADM~1\AppData\Local\Temp\pip-install-_kvuguic\six\six.py", line 703, in reraise
  File "D:\a\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\azure\cli\core\commands\__init__.py", line 688, in _run_job
  File "D:\a\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\azure\cli\core\commands\__init__.py", line 325, in __call__
  File "D:\a\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\azure\cli\core\__init__.py", line 545, in default_command_handler
  File "D:\a\1\s\build_scripts\windows\artifacts\cli\Lib\site-packages\azure\cli\command_modules\role\custom.py", line 334, in list_role_assignment_change_logs
KeyError: '/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/aro-nw3napmz/providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c'

Cause

This is because when the role assignment is created, it uses a child role definition

/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/aro-nw3napmz/providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c

which inherits from the actual role definition

/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590                            /providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c

This can be verified by calling Role Definitions - Get:

> az rest -u "https://management.azure.com/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourceGroups/aro-nw3napmz/providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c?api-version=2015-07-01"
{
  "id": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/providers/Microsoft.Authorization/roleDefinitions/b24988ac-6180-42a0-ab88-20f7382dd24c",
  "name": "b24988ac-6180-42a0-ab88-20f7382dd24c",
  "properties": {
    ...
    "roleName": "Contributor",
    "type": "BuiltInRole",
    "updatedBy": null,
    "updatedOn": "2019-02-05T21:24:38.4580610Z"
  },
  "type": "Microsoft.Authorization/roleDefinitions"
}

but the result from Role Definitions - List only has the actual role definition, not the child one, so the key lookup fails to retrieve the child one:

entry['roleDefinitionId'] = role_defs[payload['roleDefinitionId']][1]

This PR changes the lookup logic to only use the resource name (last GUID part) as role definition key, so that the role definition can be resolved and shown correctly:

> az role assignment list-changelogs --start-time 2020-07-21T12:29:00Z --end-time 2020-07-21T12:30:00Z
[
  {
    "action": "Granted",
    "caller": "3e675290-31e1-49f6-a3cd-03df3d7d6e2e",
    "principalId": "a62417b1-489d-4e8c-b7b3-c5e32ca6774a",
    "principalName": "https://az.aro.azure.com/09a2399a-59c4-4953-b08b-e9d052e49f93",
    "roleDefinitionId": "b24988ac-6180-42a0-ab88-20f7382dd24c",
    "roleName": "Contributor",
    "scope": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/resourcegroups/aro-nw3napmz",
    "scopeName": "aro-nw3napmz",
    "scopeType": "Resource group",
    "timestamp": "2020-07-21T12:29:56.561242+00:00"
  },
  ...
]

Command Usage

Luckily, according to client telemetry, in the last 7 days, there are only 29 invocations of this command from 8 subscriptions.

Remarks

This issue is discovered when fixing the live tests for role command module (#14317).

@yonzhan yonzhan requested a review from arrownj July 22, 2020 12:03
@yonzhan yonzhan requested review from jsntcy and mmyyrroonn July 22, 2020 12:03
@yonzhan yonzhan added this to the S173 milestone Jul 22, 2020
@yonzhan
Copy link
Collaborator

yonzhan commented Jul 22, 2020

RBAC

@jiasli jiasli modified the milestones: S173, S174 Jul 29, 2020
@jiasli jiasli closed this Aug 21, 2020
@jiasli jiasli changed the title {RBAC} Fix: az role assignment list-changelogs fails with KeyError {Role} Fix: az role assignment list-changelogs fails with KeyError Aug 28, 2020
@jiasli jiasli reopened this Sep 9, 2020
@jiasli jiasli marked this pull request as ready for review September 9, 2020 03:11
Copy link
Contributor

@arrownj arrownj left a comment

Choose a reason for hiding this comment

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

👍

@jiasli jiasli modified the milestones: S174, S175 - For Ignite Sep 9, 2020
entry['roleName'] = role_defs[payload['roleDefinitionId']][0]
# Look up the resource `name`, like b24988ac-6180-42a0-ab88-20f7382dd24c
role_resource_name = payload['roleDefinitionId'].split('/')[-1]
entry['roleDefinitionId'] = role_resource_name
Copy link
Member

Choose a reason for hiding this comment

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

Who will use entry['roleDefinitionId'] later? Now it is like b24988ac-6180-42a0-ab88-20f7382dd24c, is it expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

This key has the same value as the original behavior:

d.id.split('/')[-1]

I didn't change the value.

@jiasli jiasli merged commit 962e8b3 into Azure:dev Sep 10, 2020
@jiasli jiasli deleted the list-changelogs branch September 10, 2020 02:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants