Skip to content

Conversation

@zhoxing-ms
Copy link
Contributor

@zhoxing-ms zhoxing-ms commented Mar 1, 2022

Description

As mentioned in PR #20924 and #21326, we need to remove the default value Contributor of --role.

Screenshot 2022-03-01 172126

Testing Guide

Wait for PR #21369 to fix the problems of existing tests, then add tests and re-record related tests

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.

@ghost ghost requested a review from yonzhan March 1, 2022 09:33
@ghost ghost added the Auto-Assign Auto assign by bot label Mar 1, 2022
@ghost ghost requested a review from wangzelin007 March 1, 2022 09:33
@ghost ghost assigned zhoxing-ms Mar 1, 2022
@ghost ghost added this to the Mar 2022 (2022-04-05) milestone Mar 1, 2022
@ghost ghost added the Compute az vm/vmss/image/disk/snapshot label Mar 1, 2022
@ghost ghost requested a review from jiasli March 1, 2022 09:33
@ghost ghost assigned jiasli Mar 1, 2022
@ghost ghost added the RBAC az role label Mar 1, 2022
Copy link
Member

Choose a reason for hiding this comment

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

How about we put this in the command help? Otherwise, we have to have this text for both --scope and --role.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jiasli Since az vm create and az vmss create commands have too many other parameters and examples, I think it might be a good idea to add the description to both --scope and --role parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe we can not include this information in the help description, because we already have verification logic that will tell users that those two parameters need to be passed in at the same time

Copy link
Member

Choose a reason for hiding this comment

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

Duplication is bad. ad sp create-for-rbac puts this information in command help:

https://github.com/Azure/azure-cli/blob/b86a4ed33317e9b218c84d4c96498f75235df996/src/azure-cli/azure/cli/command_modules/role/_help.py#L392-L393

But I am open to this topic. 😉

Copy link
Contributor Author

@zhoxing-ms zhoxing-ms Mar 2, 2022

Choose a reason for hiding this comment

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

The situation of these two commands may be different, az vm create command has nearly 100 parameters and too much help information. 😂
Since the --scope and --role are only one of many usage scenarios, I'm not sure whether it's appropriate to put them in a global position in this case. And the help information printed by this command is too long, so users may not notice the description from the command level every time

In order to avoid duplicated prompts, I personally think maybe I can remove this information in the help description, because we already have the verification to tell users that --scope and --role parameters need to be passed in at the same time. What do you think of this idea?

@yonzhan
Copy link
Collaborator

yonzhan commented Mar 1, 2022

Compute

@zhoxing-ms zhoxing-ms force-pushed the remove_default_role_for_vm_vmss_creation branch from 24a817b to 4f62783 Compare March 11, 2022 10:12
@zhoxing-ms zhoxing-ms marked this pull request as ready for review March 11, 2022 10:12
@zhoxing-ms zhoxing-ms requested a review from jsntcy as a code owner March 11, 2022 10:12
@zhoxing-ms zhoxing-ms changed the title [Compute] BREAKING CHANGE: az vm/vmss create: Remove the default value Contributor of parameter --role [Compute] BREAKING CHANGE: az vm/vmss create: Remove the default value Contributor of parameter --role Mar 14, 2022
@zhoxing-ms zhoxing-ms force-pushed the remove_default_role_for_vm_vmss_creation branch 2 times, most recently from a0290a5 to ad46d3d Compare March 14, 2022 09:52
Comment on lines 1214 to 1215
Copy link
Member

Choose a reason for hiding this comment

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

Together with L1232, for simplicity, why don't we say:

Usage error: To create role assignments, specify both --role and --scopes.

just like #21323

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact, I wanted to be consistent with the error message that the --scope parameter was not passed in the original code https://github.com/Azure/azure-cli/pull/21474/files#r826612256

Copy link
Member

Choose a reason for hiding this comment

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

namespace.identity_role will never have is_default now, won't it?

Copy link
Contributor Author

@zhoxing-ms zhoxing-ms Mar 15, 2022

Choose a reason for hiding this comment

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

When from_set_command is True, it indicates that the command is az vm identity assign. At this time, the --role parameter still has the default value Contributor.
Therefore, when the user does not specify the --role parameter, this judgment getattr(namespace.identity_role, 'is_default', None) will be True, otherwise it will be None.

Copy link
Member

Choose a reason for hiding this comment

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

At this time, the --role parameter still has the default value Contributor.

This is definitely not secure. 🤔

Comment on lines 1232 to 1233
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the error message that --scope parameter is not passed in

Copy link
Member

Choose a reason for hiding this comment

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

I mean, I don't like this message either. It is hard to understand.

@zhoxing-ms zhoxing-ms force-pushed the remove_default_role_for_vm_vmss_creation branch from ad46d3d to b0ff664 Compare March 23, 2022 10:23
@wangzelin007
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@zhoxing-ms zhoxing-ms force-pushed the remove_default_role_for_vm_vmss_creation branch from b0ff664 to 6b3e25d Compare March 24, 2022 07:23
Comment on lines +1218 to +1219
if (namespace.identity_scope and not namespace.identity_role) or \
(not namespace.identity_scope and namespace.identity_role):
Copy link
Member

Choose a reason for hiding this comment

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

For operator precedence, not > and > or, so parentheses are not necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, actually I know this. I put parentheses here just for clearer readability~

@zhoxing-ms
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@zhoxing-ms zhoxing-ms merged commit 2e99589 into Azure:dev Mar 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Auto-Assign Auto assign by bot Compute az vm/vmss/image/disk/snapshot RBAC az role

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants