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

Fix #26857: separate stdout from stderr in AzureCliCredential #26953

Merged
merged 7 commits into from
Oct 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sdk/identity/azure-identity/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

### Bugs Fixed

- `AzureCliCredential` now works even when `az` prints warnings to stderr. ([#26857](https://github.com/Azure/azure-sdk-for-python/issues/26857))
- Fixed issue where user-supplied `TokenCachePersistenceOptions` weren't propagated when using `SharedTokenCacheCredential` ([#26982](https://github.com/Azure/azure-sdk-for-python/issues/26982))

### Other Changes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ def _run_command(command):
working_directory = get_safe_working_dir()

kwargs = {
"stderr": subprocess.STDOUT,
"stderr": subprocess.PIPE,
"cwd": working_directory,
"universal_newlines": True,
"env": dict(os.environ, AZURE_CORE_NO_COLOR="true"),
Expand All @@ -154,14 +154,14 @@ def _run_command(command):
return subprocess.check_output(args, **kwargs)
except subprocess.CalledProcessError as ex:
# non-zero return from shell
if ex.returncode == 127 or ex.output.startswith("'az' is not recognized"):
if ex.returncode == 127 or ex.stderr.startswith("'az' is not recognized"):
raise CredentialUnavailableError(message=CLI_NOT_FOUND)
if "az login" in ex.output or "az account set" in ex.output:
if "az login" in ex.stderr or "az account set" in ex.stderr:
raise CredentialUnavailableError(message=NOT_LOGGED_IN)

# return code is from the CLI -> propagate its output
if ex.output:
message = sanitize_output(ex.output)
if ex.stderr:
message = sanitize_output(ex.stderr)
else:
message = "Failed to invoke Azure CLI"
raise ClientAuthenticationError(message=message)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,13 @@ async def _run_command(command: str) -> str:
proc = await asyncio.create_subprocess_exec(
*args,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.STDOUT,
stderr=asyncio.subprocess.PIPE,
cwd=working_directory,
env=dict(os.environ, AZURE_CORE_NO_COLOR="true")
)
stdout, _ = await asyncio.wait_for(proc.communicate(), 10)
output = stdout.decode()
stdout_b, stderr_b = await asyncio.wait_for(proc.communicate(), 10)
output = stdout_b.decode()
xiangyan99 marked this conversation as resolved.
Show resolved Hide resolved
stderr = stderr_b.decode()
except OSError as ex:
# failed to execute 'cmd' or '/bin/sh'; CLI may or may not be installed
error = CredentialUnavailableError(message="Failed to execute '{}'".format(args[0]))
Expand All @@ -117,11 +118,11 @@ async def _run_command(command: str) -> str:
if proc.returncode == 0:
return output

if proc.returncode == 127 or output.startswith("'az' is not recognized"):
if proc.returncode == 127 or stderr.startswith("'az' is not recognized"):
raise CredentialUnavailableError(CLI_NOT_FOUND)

if "az login" in output or "az account set" in output:
if "az login" in stderr or "az account set" in stderr:
raise CredentialUnavailableError(message=NOT_LOGGED_IN)

message = sanitize_output(output) if output else "Failed to invoke Azure CLI"
message = sanitize_output(stderr) if stderr else "Failed to invoke Azure CLI"
raise ClientAuthenticationError(message=message)
24 changes: 12 additions & 12 deletions sdk/identity/azure-identity/tests/test_cli_credential.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@
)


def raise_called_process_error(return_code, output, cmd="..."):
error = subprocess.CalledProcessError(return_code, cmd=cmd, output=output)
def raise_called_process_error(return_code, output="", cmd="...", stderr=""):
error = subprocess.CalledProcessError(return_code, cmd=cmd, output=output, stderr=stderr)
return mock.Mock(side_effect=error)


Expand Down Expand Up @@ -76,17 +76,17 @@ def test_get_token():
def test_cli_not_installed_linux():
"""The credential should raise CredentialUnavailableError when the CLI isn't installed"""

output = "/bin/sh: 1: az: not found"
with mock.patch(CHECK_OUTPUT, raise_called_process_error(127, output)):
stderr = "/bin/sh: 1: az: not found"
with mock.patch(CHECK_OUTPUT, raise_called_process_error(127, stderr=stderr)):
with pytest.raises(CredentialUnavailableError, match=CLI_NOT_FOUND):
AzureCliCredential().get_token("scope")


def test_cli_not_installed_windows():
"""The credential should raise CredentialUnavailableError when the CLI isn't installed"""

output = "'az' is not recognized as an internal or external command, operable program or batch file."
with mock.patch(CHECK_OUTPUT, raise_called_process_error(1, output)):
stderr = "'az' is not recognized as an internal or external command, operable program or batch file."
with mock.patch(CHECK_OUTPUT, raise_called_process_error(1, stderr=stderr)):
with pytest.raises(CredentialUnavailableError, match=CLI_NOT_FOUND):
AzureCliCredential().get_token("scope")

Expand All @@ -102,18 +102,18 @@ def test_cannot_execute_shell():
def test_not_logged_in():
"""When the CLI isn't logged in, the credential should raise CredentialUnavailableError"""

output = "ERROR: Please run 'az login' to setup account."
with mock.patch(CHECK_OUTPUT, raise_called_process_error(1, output)):
stderr = "ERROR: Please run 'az login' to setup account."
with mock.patch(CHECK_OUTPUT, raise_called_process_error(1, stderr=stderr)):
with pytest.raises(CredentialUnavailableError, match=NOT_LOGGED_IN):
AzureCliCredential().get_token("scope")


def test_unexpected_error():
"""When the CLI returns an unexpected error, the credential should raise an error containing the CLI's output"""

output = "something went wrong"
with mock.patch(CHECK_OUTPUT, raise_called_process_error(42, output)):
with pytest.raises(ClientAuthenticationError, match=output):
stderr = "something went wrong"
with mock.patch(CHECK_OUTPUT, raise_called_process_error(42, stderr=stderr)):
with pytest.raises(ClientAuthenticationError, match=stderr):
AzureCliCredential().get_token("scope")


Expand Down Expand Up @@ -181,7 +181,7 @@ def fake_check_output(command_line, **_):

token = AzureCliCredential(tenant_id=second_tenant).get_token("scope")
assert token.token == second_token

def test_multitenant_authentication():
default_tenant = "first-tenant"
first_token = "***"
Expand Down
18 changes: 9 additions & 9 deletions sdk/identity/azure-identity/tests/test_cli_credential_async.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ async def test_get_token():
async def test_cli_not_installed_linux():
"""The credential should raise CredentialUnavailableError when the CLI isn't installed"""

output = "/bin/sh: 1: az: not found"
with mock.patch(SUBPROCESS_EXEC, mock_exec(output, return_code=127)):
stderr = "/bin/sh: 1: az: not found"
with mock.patch(SUBPROCESS_EXEC, mock_exec("", stderr, return_code=127)):
with pytest.raises(CredentialUnavailableError, match=CLI_NOT_FOUND):
credential = AzureCliCredential()
await credential.get_token("scope")
Expand All @@ -111,8 +111,8 @@ async def test_cli_not_installed_linux():
async def test_cli_not_installed_windows():
"""The credential should raise CredentialUnavailableError when the CLI isn't installed"""

output = "'az' is not recognized as an internal or external command, operable program or batch file."
with mock.patch(SUBPROCESS_EXEC, mock_exec(output, return_code=1)):
stderr = "'az' is not recognized as an internal or external command, operable program or batch file."
with mock.patch(SUBPROCESS_EXEC, mock_exec("", stderr, return_code=1)):
with pytest.raises(CredentialUnavailableError, match=CLI_NOT_FOUND):
credential = AzureCliCredential()
await credential.get_token("scope")
Expand All @@ -130,8 +130,8 @@ async def test_cannot_execute_shell():
async def test_not_logged_in():
"""When the CLI isn't logged in, the credential should raise CredentialUnavailableError"""

output = "ERROR: Please run 'az login' to setup account."
with mock.patch(SUBPROCESS_EXEC, mock_exec(output, return_code=1)):
stderr = "ERROR: Please run 'az login' to setup account."
with mock.patch(SUBPROCESS_EXEC, mock_exec("", stderr, return_code=1)):
with pytest.raises(CredentialUnavailableError, match=NOT_LOGGED_IN):
credential = AzureCliCredential()
await credential.get_token("scope")
Expand All @@ -140,9 +140,9 @@ async def test_not_logged_in():
async def test_unexpected_error():
"""When the CLI returns an unexpected error, the credential should raise an error containing the CLI's output"""

output = "something went wrong"
with mock.patch(SUBPROCESS_EXEC, mock_exec(output, return_code=42)):
with pytest.raises(ClientAuthenticationError, match=output):
stderr = "something went wrong"
with mock.patch(SUBPROCESS_EXEC, mock_exec("", stderr, return_code=42)):
with pytest.raises(ClientAuthenticationError, match=stderr):
credential = AzureCliCredential()
await credential.get_token("scope")

Expand Down