Skip to content
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.

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

{Role} ad sp create-for-rbac: Retry Service Principal creation for "must in the local tenant" error #17412

Merged
merged 3 commits into from
Apr 8, 2021

Conversation

jiasli
Copy link
Member

@jiasli jiasli commented Mar 23, 2021

Description

Fix #14767

The retry-able check logic is firstly added by #1499:

if 'The appId of the service principal does not reference a valid application object' in str(ex):

It was later changed by #2567 (not sure why the change):

if l < _RETRY_TIMES and (' does not reference ' in str(ex) or ' does not exist ' in str(ex)):

This PR simply adds another retry-able check for

When using this permission, the backing application of the service principal being created must in the local tenant

just like what microsoft/onefuzz#716 does.

Testing Guide

With new Knack (microsoft/knack#228):

> pytest src\azure-cli\azure\cli\command_modules\role\tests\latest\test_graph.py::CreateForRbacScenarioTest::test_create_for_rbac_retry --override-ini log_cli=true

--------------------------------------- live log call ---------------------------------------
WARNING  cli.azure.cli.command_modules.role.custom:custom.py:1466 Creating service principal failed with error 'The appId '00000000-0000-0000-0000-000000000000' of the service principal does not reference a valid application object.'. Retrying: 1/36
WARNING  cli.azure.cli.command_modules.role.custom:custom.py:1466 Creating service principal failed with error 'When using this permission, the backing application of the service principal being created must in the local tenant'. Retrying: 2/36
WARNING  cli.azure.cli.command_modules.role.custom:custom.py:1505 The output includes credentials that you must protect. Be sure that you do not include these credentials in your code or check the credentials into your source control. For more information, see https://aka.ms/azadsp-cli
PASSED                                                                                 [100%]

Additional Context

As #2567 (review) pointed out, this is indeed very ugly.

@yonzhan
Copy link
Collaborator

yonzhan commented Mar 23, 2021

Role

@yonzhan yonzhan added this to the S185 milestone Mar 23, 2021
@TonySh127-ms
Copy link

Hi @jiasli , thanks for your time to add the comment at #18610. We understand the new version CLI has added the retry logical. May I know the permanent fix whether is in the recent version update plan?

@jiasli
Copy link
Member Author

jiasli commented Jun 25, 2021

May I know the permanent fix whether is in the recent version update plan?

Sorry I didn't really get your question.

According to 5544a0f, this fix has been included since azure-cli-2.22.0.

This failure is due to AAD service propagation and I don't think AAD has a plan to refine/fix it, so the retry logic will be necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants