Skip to content

[EventGrid] Support user identity and mixed mode #21648

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.

Sign up for GitHub

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

Jump to bottom
Merged
evelyn-ys merged 13 commits into from
Apr 1, 2022
Merged

[EventGrid] Support user identity and mixed mode #21648

evelyn-ys merged 13 commits into from
Apr 1, 2022

Conversation

@rohkuma-microsoft
Copy link
Contributor

@rohkuma-microsoft rohkuma-microsoft commented Mar 15, 2022

Description

Testing Guide

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change
[Component Name 2] az command b: Add some customer-facing feature

This checklist is used to make sure that common guidelines for a pull request are followed.

@rohkuma-microsoft rohkuma-microsoft changed the title Adding User identity and mixed mode [Az EventGrid] Adding User identity and mixed mode Mar 15, 2022
@yonzhan
Copy link
Collaborator

yonzhan commented Mar 15, 2022

EventGrid

@yonzhan yonzhan requested a review from calvinhzy March 15, 2022 23:53
@yonzhan yonzhan added this to the Mar 2022 (2022-04-05) milestone Mar 15, 2022
@rohkuma-microsoft rohkuma-microsoft changed the title [Az EventGrid] Adding User identity and mixed mode [Az EventGrid] Add User identity and mixed mode Mar 24, 2022
@evelyn-ys
Copy link
Member

evelyn-ys commented Mar 24, 2022

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 24, 2022

Azure Pipelines successfully started running 2 pipeline(s).

@evelyn-ys evelyn-ys requested a review from zhoxing-ms March 24, 2022 09:44
@evelyn-ys
Copy link
Member

evelyn-ys commented Mar 24, 2022

The design doesn't seem to follow https://github.com/Azure/azure-cli/blob/dev/doc/managed_identity_command_guideline.md

Add @zhoxing-ms for review

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Mar 28, 2022 •
edited
Loading

For az <resource> create/update, please use --mi-user-assigned <AzureResourceId1> <AzureResourceId2> instead of --identity userassigned --user-assigned <AzureResourceId1> <AzureResourceId2> enable-managed-identity-during-resource-creation

Please note that the --user-assigned is used in the scenario of operate-managed-identity-on-existing-resource for az <resource> identity assign/remove commands

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Mar 29, 2022

Please note that the parameter --identity is no longer required in the design specifications.

  • --identity userassigned --mi-user-assigned <AzureResourceId1> <AzureResourceId2> should be --mi-user-assigned <AzureResourceId1> <AzureResourceId2>
  • --identity systemassigned should be --mi-system-assigned
  • --identity mixed --mi-user-assigned <AzureResourceId1> <AzureResourceId2> should be --mi-system-assigned --mi-user-assigned <AzureResourceId1> <AzureResourceId2>
    Therefore, you may need to deprecate the --identity parameter with the deprecate_info flag deprecating-commands-and-arguments

@evelyn-ys
Copy link
Member

evelyn-ys commented Mar 31, 2022

Please note that the parameter --identity is no longer required in the design specifications.

  • --identity userassigned --mi-user-assigned <AzureResourceId1> <AzureResourceId2> should be --mi-user-assigned <AzureResourceId1> <AzureResourceId2>
  • --identity systemassigned should be --mi-system-assigned
  • --identity mixed --mi-user-assigned <AzureResourceId1> <AzureResourceId2> should be --mi-system-assigned --mi-user-assigned <AzureResourceId1> <AzureResourceId2>
    Therefore, you may need to deprecate the --identity parameter with the deprecate_info flag deprecating-commands-and-arguments

Additionally, --identity now only accept noidentity and systemassigned and the definition is not changed in this PR

zhoxing-ms
zhoxing-ms reviewed Mar 31, 2022
View reviewed changes
src/azure-cli/azure/cli/command_modules/eventgrid/custom.py Outdated
elif identity_type_name is None and user_identity_properties is not None and mi_system_assigned is not None:
result = IDENTITY_MIXED_MODE
elif identity_type_name is not None and (user_identity_properties is not None or mi_system_assigned is not None):
raise CLIError('usage error: cannot use --identity together with --mi-system-assigned or --mi-user-assigned')
Copy link
Contributor

@zhoxing-ms zhoxing-ms Mar 31, 2022

Choose a reason for hiding this comment

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

Please use the specific error type MutuallyExclusiveArgumentError instead of CLIError

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Mar 31, 2022

Please deprecate the --identity parameter with the deprecate_info flag. doc link: deprecating-commands-and-arguments

Please note that adding deprecated_info flag will not directly cause breaking change to users, and users can temporarily keep their existing behavior. We add deprecated_info flag just to notify users migrate the current non-standard behavior to a compliant and unified way.

@zhoxing-ms
Copy link
Contributor

zhoxing-ms commented Mar 31, 2022

Please refer to doc submitting-pull-requests to write the current changes in detail in the PR title and PR description, these contents will be written into the history notes

@evelyn-ys evelyn-ys changed the title [Az EventGrid] Add User identity and mixed mode [EventGrid] Support user identity and mixed mode Apr 1, 2022
@evelyn-ys evelyn-ys merged commit 05c9098 into Azure:dev Apr 1, 2022
jiasli
jiasli reviewed Apr 1, 2022
View reviewed changes
src/azure-cli/azure/cli/command_modules/eventgrid/_params.py
c.argument('inbound_ip_rules', action=AddInboundIpRule, nargs='+')
c.argument('sku', arg_type=sku_type)
c.argument('identity', arg_type=identity_type)
c.argument('identity', arg_type=identity_type, deprecate_info=c.deprecate(expiration='2.46.0'))
Copy link
Member

@jiasli jiasli Apr 1, 2022

Choose a reason for hiding this comment

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

This may cause unexpected CI failure in the future. We recommend removing a deprecated argument manually.

Copy link
Member

@jiasli jiasli Apr 1, 2022

Choose a reason for hiding this comment

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

Well. Another deprecation indeed caused problem today after upgrading Azure CLI version to 2.35.0: #20682 (comment)

@evelyn-ys evelyn-ys mentioned this pull request Apr 6, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jiasli jiasli jiasli left review comments

@zhoxing-ms zhoxing-ms zhoxing-ms left review comments

@evelyn-ys evelyn-ys evelyn-ys approved these changes

@calvinhzy calvinhzy Awaiting requested review from calvinhzy calvinhzy is a code owner

Assignees

@evelyn-ys evelyn-ys

Labels

None yet

Projects

None yet

Milestone

Mar 2022 (2022-04-06)

Development

Successfully merging this pull request may close these issues.

5 participants

@rohkuma-microsoft @yonzhan @evelyn-ys @zhoxing-ms @jiasli