{Service Fabric} Migrate servicefabric command module to Microsoft Graph
#28105
Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
|
AD |
microsoft-github-policy-service
bot
added
Service Fabric
| Default permissions are created for the current user or service principal unless the `--no-self-perms` | ||
| or `--enable-rbac-authorization` flag is specified. |
There was a problem hiding this comment.
This changes the original cumbersome expression (#12074 (comment)).
| if not subscription: | ||
| return None | ||
|
|
||
| if subscription['user']: |
There was a problem hiding this comment.
It is not possible for a subscription to have no user field, as _USER_ENTITY(user) will always be set during creation:
azure-cli/src/azure-cli-core/azure/cli/core/_profile.py
Lines 408 to 415 in b00483c
| if subscription['user']: | ||
| if subscription['user']['type'] == 'user': | ||
| return _get_object_id_by_upn(graph_client, subscription['user']['name']) | ||
| if subscription['user']['type'] == 'servicePrincipal': | ||
| return _get_object_id_by_spn(graph_client, subscription['user']['name']) | ||
| logger.warning("Unknown user type '%s'", subscription['user']['type']) | ||
| else: | ||
| logger.warning('Current credentials are not from a user or service principal. ' | ||
| 'Azure Key Vault does not work with certificate credentials.') |
There was a problem hiding this comment.
I guess certificate credential is a remnant of the very old implementation for RDFE.
| return _get_object_id_by_upn(graph_client, subscription['user']['name']) | ||
| if subscription['user']['type'] == 'servicePrincipal': | ||
| return _get_object_id_by_spn(graph_client, subscription['user']['name']) | ||
| logger.warning("Unknown user type '%s'", subscription['user']['type']) |
There was a problem hiding this comment.
user and servicePrincipal are the only possible values for user.type:
Unless the user deliberately corrupt azureProfile.json, this warning can never be hit.
| if len(accounts) > 1: | ||
| logger.warning("Multiple service principals found with spn '%s'. " | ||
| "You can avoid this by specifying object id.", spn) | ||
| return None |
There was a problem hiding this comment.
It is not possible for multiple service principals to have the same spn, so this check will never be hit. azure.cli.command_modules.role.custom._resolve_object_id_and_type has such logic:
| accounts = list(graph_client.service_principal_list( | ||
| filter="servicePrincipalNames/any(c:c eq '{}')".format(spn))) | ||
| if not accounts: | ||
| logger.warning("Unable to find user with spn '%s'", spn) |
There was a problem hiding this comment.
spn stands for Service Principal Name. A user object never has spn property.
zhoxing-ms
modified the milestones:
February 2024 (2024-03-05),
March 2024 (2024-04-02)
|
If az-cli isn't being updated to remove EOL libraries, I guess it means that it is effectively unsupported. |
| from msrestazure.azure_exceptions import CloudError | ||
| try: | ||
| current_user = graph_client.signed_in_user.get() | ||
| if current_user and current_user.object_id: # pylint:disable=no-member |
There was a problem hiding this comment.
A user object can never have no object_id.
| except CloudError: | ||
| pass |
There was a problem hiding this comment.
I guess this line tries to silence the exception. However, L125 relies on the exception. The code is buggy.
| return None | ||
|
|
||
|
|
||
| def _get_object_id(graph_client, spn=None, upn=None): |
There was a problem hiding this comment.
There is no need for _get_object_id to support subscription argument. _get_object_id_from_subscription can be directly called by get_current_identity_object_id.
# Before
keyvault/servicefabric/lab -> _get_object_id -> [subscription] _get_object_id_from_subscription
-> [spn] _get_object_id_by_spn
-> [upn] _get_object_id_by_upn
# After
keyvault -> get_object_id -> _get_object_id -> [spn] _get_object_id_by_spn
-> [upn] _get_object_id_by_upn
keyvault/servicefabric/lab -> get_current_identity_object_id -> [subscription] _get_object_id_from_subscription
There was a problem hiding this comment.
There is no test named test_node_type.
There was a problem hiding this comment.
test_add_secondary_node_type_add_remove_node was changed to live only by #25735.
| except GraphError: | ||
| from azure.cli.core._profile import Profile | ||
| profile = Profile(cli_ctx) | ||
| subscription = profile.get_subscription(subscription=cli_ctx.data.get('subscription_id', None)) |
There was a problem hiding this comment.
Can we add a subscription parameter for get_current_identity_object_id to avoid calling profile.get_subscription() twice? Keyvault/Servicefabric has already got subscription before calling get_current_identity_object_id.
There was a problem hiding this comment.
The first attempt of get_current_identity_object_id() is to call _get_current_user_object_id(), it will use the current login context. Supporting a custom subscription parameter may cause inconsistency with _get_current_user_object_id().
Actually, I think even this line shouldn't pass subscription=cli_ctx.data.get('subscription_id', None) as this may cause inconsistency with _get_current_user_object_id(). This may result in using the current login context to look up the object ID of another identity.
We clearly documented this in the help message:
If you want to assign the default permission, you have to change the default subscription with
az account setfirst, instead of using--subscription.
There was a problem hiding this comment.
Not sure if this will break cross-tenant support (Not sure if it still works). I will do manual test this afternoon
| except GraphError: | ||
| from azure.cli.core._profile import Profile | ||
| profile = Profile(cli_ctx) | ||
| subscription = profile.get_subscription() |
There was a problem hiding this comment.
It surprised me that even we removed subscription=cmd.cli_ctx.data.get('subscription_id', None), it still used the value passed by --subscription
There was a problem hiding this comment.
Then I have no concerns. Cross-tenant is still supported
There was a problem hiding this comment.
Oh wait... It's created in --subscription because it's calling _get_current_user_object_id(), not _get_object_id_from_subscription().
It's difficult to prepare a cross-tenant sp account for test
There was a problem hiding this comment.
I finally understand that graph call is tenant level. There's no meaning to care about cross-sub level support in keyvault creation as it's always a mess.
No concerns, just go ahead
There was a problem hiding this comment.
I provided a comprehensive analysis on this issue at #29837
jiasli
requested review from
jsntcy,
yanzhudd,
calvinhzy and
bebound
as code owners
|
#29878 caused merge conflict on |
jiasli
force-pushed
the
module-graph
branch
from
bce9266
to
beaa4df
Compare
jiasli
changed the title
lab and servicefabric command modules to Microsoft Graphservicefabric command module to Microsoft Graph
jiasli
changed the title
servicefabric command module to Microsoft Graphservicefabric command module to Microsoft Graph
There was a problem hiding this comment.
Just a little question: could we just leave a blank line in the middle? Is there any reason for leaving two blank lines?
There was a problem hiding this comment.
Since > is used, 2 blank lines will be rendered as 1 blank line:
> az keyvault create -h
Command
az keyvault create : Create a Vault or HSM.
RBAC authorization is enabled by default. If `--enable-rbac-authorization` is manually
specified to `false` and `--no-self-perms` flag is not specified, default permissions are
created for the current user or service principal.
If you want to assign the default permission, you have to change the default subscription
with `az account set` first, instead of using `--subscription`.
If we only put 1 blank line here, these paragraphs will not be separated:
> az keyvault create -h
Command
az keyvault create : Create a Vault or HSM.
RBAC authorization is enabled by default. If `--enable-rbac-authorization` is manually
specified to `false` and `--no-self-perms` flag is not specified, default permissions are
created for the current user or service principal.
If you want to assign the default permission, you have to change the default subscription
with `az account set` first, instead of using `--subscription`.
Related command
az sfDescription
servicefabriccommand module directly useazure-graphrbacSDK to call AD Graph.This PR migrates
servicefabriccommand module to call Microsoft Graph to address #22174.