Skip to content

graph: support idempotent - #9108

Merged
Yugang Wang (yugangw-msft) merged 7 commits into
Azure:devfrom
yugangw-msft:idempotent
Apr 16, 2019
Merged

Yugang Wang (yugangw-msft) merged 7 commits into
Azure:devfrom
yugangw-msft:idempotent

Conversation

@yugangw-msft

Copy link
Copy Markdown
Contributor

Fix #8750 Fix #8519 Fix #9032

  • The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).

  • I adhere to the Command Guidelines.

# workaround to ensure idempotent even AAD graph service doesn't support it
if matches:
if len(matches) > 1:
err = ('There are more than one groups with same display name and mail nick names: "{}". '

@marstr Martin Strobel (marstr) Apr 15, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[nit] Grammar is a little off here, I'd have this read: "There is more than one group with the same display and nick names: {}"

if len(matches) > 1:
err = ('There are more than one groups with same display name and mail nick names: "{}". '
'Please delete them first.')
raise CLIError(err.format(', '.join([x.object_id for x in matches])))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Given that the service doesn't create this restriction, and in the past we've allowed Groups with the same display/nick names, I think out-right preventing people from doing it is a step too far. I'd feel better if there was a flag to allow a "force" that disregards this check.

@marstr Martin Strobel (marstr) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@anschoewe

Copy link
Copy Markdown

Thanks!

Comment on lines +132 to +138
try:
return _create_role_assignment(cmd.cli_ctx, role, assignee or assignee_object_id, resource_group_name, scope,
resolve_assignee=(not assignee_object_id))
except Exception as ex: # pylint: disable=broad-except
if _error_caused_by_role_assignment_exists(ex): # for idempotent
return list_role_assignments(cmd, assignee, role, resource_group_name, scope)[0]
raise

@jiasli Jiashuo Li (jiasli) Feb 6, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This implementation of idempotence has an issue and leads to an internal ticket 365147494.

If --assignee-object-id is used and if the role assignment is newly created, the returned role assignment is correct:

> az role assignment create --assignee-object-id e323d08e-9b5f-450e-8500-342136c6bfd5 --assignee-principal-type ServicePrincipal --role Reader --scope /subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590
{
  "canDelegate": null,
  "condition": null,
  "conditionVersion": null,
  "description": null,
  "id": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/providers/Microsoft.Authorization/roleAssignments/41030363-55bb-4b7f-8613-23d043620db5",
  "name": "41030363-55bb-4b7f-8613-23d043620db5",
  "principalId": "e323d08e-9b5f-450e-8500-342136c6bfd5",
  "principalType": "ServicePrincipal",
  "roleDefinitionId": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-3385-48ef-bd42-f606fba81ae7",
  "scope": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590",
  "type": "Microsoft.Authorization/roleAssignments"
}

But if the role assignment already exists (results in a 409 HTTP response), the returned role assignment could be wrong:

> az role assignment create --assignee-object-id e323d08e-9b5f-450e-8500-342136c6bfd5 --assignee-principal-type ServicePrincipal --role Reader --scope /subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590
{
  "canDelegate": null,
  "condition": null,
  "conditionVersion": null,
  "description": null,
  "id": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/providers/Microsoft.Authorization/roleAssignments/332cf847-f85f-4e42-93c0-ac2d2269c9c6",
  "name": "332cf847-f85f-4e42-93c0-ac2d2269c9c6",
  "principalId": "5963f50c-7c43-405c-af7e-53294de76abd",
  "principalName": [admin2@AzureSDKTeam.onmicrosoft.com](mailto:admin2@AzureSDKTeam.onmicrosoft.com),
  "principalType": "User",
  "roleDefinitionId": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590/providers/Microsoft.Authorization/roleDefinitions/acdd72a7-3385-48ef-bd42-f606fba81ae7",
  "roleDefinitionName": "Reader",
  "scope": "/subscriptions/0b1f6471-1bf0-4dda-aec3-cb9272f09590",
  "type": "Microsoft.Authorization/roleAssignments"
}

The reason is because list_role_assignments doesn’t take assignee_object_id into consideration, so it actually returns the first role assignment object matching only specified role and scope.

Meanwhile, az role assignment list doesn’t support --assignee-object-id either, which is also inconsistent with az role assignment create.

The correct way to achieve idempotence is through --name argument (#24324), instead of the (role, assignee, scope) tuple.

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