Skip to content

keyvault: make commands work in cloud shell or vms with identity#6319

Merged
yugangw-msft merged 2 commits into
Azure:devfrom
yugangw-msft:kv
May 10, 2018
Merged

keyvault: make commands work in cloud shell or vms with identity#6319
yugangw-msft merged 2 commits into
Azure:devfrom
yugangw-msft:kv

Conversation

@yugangw-msft
Copy link
Copy Markdown
Contributor

Fix #6313

  • 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.

@yugangw-msft yugangw-msft requested a review from tjprescott May 9, 2018 20:51
@promptws
Copy link
Copy Markdown

promptws commented May 9, 2018

View a preview at https://prompt.ws/r/Azure/azure-cli/6319
This is an experimental preview for @microsoft users.

'The vault may not exist or you may need to flush your DNS cache '
'and try again later.')
raise CLIError(ex)
else:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tjprescott, this is an optional change, if you feel it has side impact, I can revert/improve. So far this error handler swallows exceptions and hides the real issue.

from azure.cli.core._profile import Profile
try:
return Profile(cli_ctx=cli_ctx).get_login_credentials(resource)[0]._token_retriever() # pylint: disable=protected-access
return Profile(cli_ctx=cli_ctx).get_raw_token(resource)[0]
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't write tests in this PR as i feel the existing testbed should be sufficient to capture potential regressions

Copy link
Copy Markdown
Member

@tjprescott tjprescott left a comment

Choose a reason for hiding this comment

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

LGTM. Would like @schaabs to review.

@tjprescott tjprescott requested a review from schaabs May 9, 2018 21:31
@tjprescott tjprescott added bug This issue requires a change to an existing behavior in the product in order to be resolved. KeyVault az keyvault labels May 9, 2018
@tjprescott tjprescott added this to the Sprint 37 milestone May 9, 2018
'and try again later.')
raise CLIError(ex)
else:
raise ex
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.

What fails with this?

Copy link
Copy Markdown
Contributor Author

@yugangw-msft yugangw-msft May 10, 2018

Choose a reason for hiding this comment

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

See below. I can still throw non CLIError, but need confirmation/encourage from your guys to proceed

    @ResourceGroupPreparer(name_prefix='cli_test_kv_cert_issuer')
    def test_keyvault_certificate_issuers(self, resource_group):
       .....
        # test admin commands
        self.cmd('keyvault certificate issuer admin add --vault-name {kv} --issuer-name issuer1 --email test@test.com --first-name Test --last-name Admin --phone 123-456-7890', checks=[
            ...
        ])
        # ---> fail below which i guess caused by adding dupe issuers<-----
        self.cmd('keyvault certificate issuer admin add --vault-name {kv} --issuer-name issuer1 --email test@test.com')  
  File "d:\sdk\azure-cli\src\command_modules\azure-cli-keyvault\azure\cli\command_modules\keyvault\_command_type.py", line 118, in keyvault_command_handler
    return keyvault_exception_handler(ex)
  File "d:\sdk\azure-cli\src\command_modules\azure-cli-keyvault\azure\cli\command_modules\keyvault\_command_type.py", line 47, in keyvault_exception_handler
    raise ex
  File "d:\sdk\azure-cli\src\command_modules\azure-cli-keyvault\azure\cli\command_modules\keyvault\_command_type.py", line 98, in keyvault_command_handler
    result = op(**command_args)
  File "d:\sdk\azure-cli\src\command_modules\azure-cli-keyvault\azure\cli\command_modules\keyvault\custom.py", line 800, in add_certificate_issuer_admin
    raise CLIError("admin '{}' already exists".format(email))
knack.util.CLIError: admin 'test@test.com' already exists

Copy link
Copy Markdown

@schaabs schaabs left a comment

Choose a reason for hiding this comment

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

The changes look good to me

@yugangw-msft yugangw-msft merged commit 1c2a3ef into Azure:dev May 10, 2018
@yugangw-msft yugangw-msft deleted the kv branch May 10, 2018 02:44
@haroldrandom haroldrandom added bug This issue requires a change to an existing behavior in the product in order to be resolved. KeyVault az keyvault labels Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This issue requires a change to an existing behavior in the product in order to be resolved. KeyVault az keyvault

Projects

None yet

Development

Successfully merging this pull request may close these issues.

az keyvault secret fails to get secret information for managed service identity

5 participants