From 077827368be82e368df4481705d020caf9c53b79 Mon Sep 17 00:00:00 2001 From: Fuming Zhang Date: Tue, 6 Oct 2026 23:52:52 +0000 Subject: [PATCH 1/4] {AKS} Preserve resolved executable paths in aks-preview Use the selected executable consistently in diagnostics, credential conversion, and Bastion. Preserve argument boundaries and shell behavior, and add regression coverage for executable lookup and error handling. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/aks-preview/HISTORY.rst | 1 + src/aks-preview/README.rst | 9 + src/aks-preview/azext_aks_preview/_helpers.py | 30 ++- .../azext_aks_preview/aks_diagnostics.py | 38 +-- .../azext_aks_preview/bastion/bastion.py | 87 ++++--- src/aks-preview/azext_aks_preview/custom.py | 5 +- .../tests/latest/test_aks_bastion.py | 227 +++++++++++++++++- .../tests/latest/test_aks_diagnostics.py | 133 ++++++++++ .../tests/latest/test_custom.py | 59 +++++ .../tests/latest/test_helpers.py | 102 ++++++++ 10 files changed, 631 insertions(+), 60 deletions(-) diff --git a/src/aks-preview/HISTORY.rst b/src/aks-preview/HISTORY.rst index 38cdc910844..5a18cffa9ae 100644 --- a/src/aks-preview/HISTORY.rst +++ b/src/aks-preview/HISTORY.rst @@ -11,6 +11,7 @@ To release a new version, please select a new version number (usually plus 1 to Pending +++++++ +* `az aks kollect`, `az aks kanalyze`, `az aks get-credentials` and `az aks bastion tunnel`: Use resolved executable paths consistently. 22.0.0b10 +++++++++ diff --git a/src/aks-preview/README.rst b/src/aks-preview/README.rst index cf2b46be9af..9109b0a6230 100644 --- a/src/aks-preview/README.rst +++ b/src/aks-preview/README.rst @@ -14,6 +14,15 @@ Install this extension using the following CLI command `az extension add --name Remove this extension using the following CLI command `az extension remove --name aks-preview`. +External tools +============== + +The ``az aks kollect``, ``az aks kanalyze``, ``az aks get-credentials`` and +``az aks bastion tunnel`` commands resolve external executables before launching +them. Install these tools in absolute directories on ``PATH``; empty and relative +``PATH`` entries are not searched. Explicit executable paths are preserved, and +paths containing spaces are supported. + Dependency between aks-preview and azure-cli/acs (azure-cli-core) ================================================================= diff --git a/src/aks-preview/azext_aks_preview/_helpers.py b/src/aks-preview/azext_aks_preview/_helpers.py index f35536b78a8..d9c1dc5c7d7 100644 --- a/src/aks-preview/azext_aks_preview/_helpers.py +++ b/src/aks-preview/azext_aks_preview/_helpers.py @@ -136,17 +136,31 @@ def reset_agentpool_to_name_and_mode(agentpool, mode): def which(binary): - path_var = os.getenv('PATH') - if platform.system() == 'Windows': - binary = binary + '.exe' - parts = path_var.split(';') + """Resolve an explicit executable path or search absolute directories in PATH.""" + is_windows = platform.system() == 'Windows' + binaries = [binary] + if is_windows and not os.path.splitext(binary)[1]: + extensions = os.getenv('PATHEXT') or '.COM;.EXE;.BAT;.CMD' + binaries = [binary + extension for extension in extensions.split(';') if extension] + + explicit_path = bool(os.path.dirname(binary)) + if explicit_path: + parts = [''] else: - parts = path_var.split(':') + parts = os.getenv('PATH', '').split(';' if is_windows else ':') for part in parts: - bin_path = os.path.join(part, binary) - if os.path.exists(bin_path) and os.path.isfile(bin_path) and os.access(bin_path, os.X_OK): - return bin_path + if is_windows: + part = part.strip('"') + # Empty, relative and drive-relative entries depend on the working directory. + if not explicit_path and ( + not os.path.isabs(part) or (is_windows and not os.path.splitdrive(part)[0]) + ): + continue + for executable in binaries: + bin_path = os.path.abspath(os.path.join(part, executable)) + if os.path.isfile(bin_path) and os.access(bin_path, os.X_OK): + return bin_path return None diff --git a/src/aks-preview/azext_aks_preview/aks_diagnostics.py b/src/aks-preview/azext_aks_preview/aks_diagnostics.py index 5f1abc6529c..2c54e92b37c 100644 --- a/src/aks-preview/azext_aks_preview/aks_diagnostics.py +++ b/src/aks-preview/azext_aks_preview/aks_diagnostics.py @@ -72,7 +72,8 @@ def aks_kollect_cmd(cmd, # pylint: disable=too-many-statements,too-many-local mc = client.get(resource_group_name, name) - if not which('kubectl'): + kubectl_path = which('kubectl') + if not kubectl_path: raise CLIError('Can not find kubectl executable in PATH') storage_account_id = None @@ -166,32 +167,32 @@ def aks_kollect_cmd(cmd, # pylint: disable=too-many-statements,too-many-local print() print("Cleaning up aks-periscope resources if existing") - subprocess.call(["kubectl", "--kubeconfig", temp_kubeconfig_path, "delete", + subprocess.call([kubectl_path, "--kubeconfig", temp_kubeconfig_path, "delete", "serviceaccount,configmap,daemonset,secret", "--all", "-n", CONST_PERISCOPE_NAMESPACE, "--ignore-not-found"], stderr=subprocess.STDOUT) - subprocess.call(["kubectl", "--kubeconfig", temp_kubeconfig_path, "delete", + subprocess.call([kubectl_path, "--kubeconfig", temp_kubeconfig_path, "delete", "ClusterRoleBinding", "aks-periscope-role-binding", "--ignore-not-found"], stderr=subprocess.STDOUT) - subprocess.call(["kubectl", "--kubeconfig", temp_kubeconfig_path, "delete", + subprocess.call([kubectl_path, "--kubeconfig", temp_kubeconfig_path, "delete", "ClusterRoleBinding", "aks-periscope-role-binding-view", "--ignore-not-found"], stderr=subprocess.STDOUT) - subprocess.call(["kubectl", "--kubeconfig", temp_kubeconfig_path, "delete", + subprocess.call([kubectl_path, "--kubeconfig", temp_kubeconfig_path, "delete", "ClusterRole", "aks-periscope-role", "--ignore-not-found"], stderr=subprocess.STDOUT) - subprocess.call(["kubectl", "--kubeconfig", temp_kubeconfig_path, "delete", + subprocess.call([kubectl_path, "--kubeconfig", temp_kubeconfig_path, "delete", "--all", "apd", "-n", CONST_PERISCOPE_NAMESPACE, "--ignore-not-found"], stderr=subprocess.DEVNULL) - subprocess.call(["kubectl", "--kubeconfig", temp_kubeconfig_path, "delete", + subprocess.call([kubectl_path, "--kubeconfig", temp_kubeconfig_path, "delete", "CustomResourceDefinition", "diagnostics.aks-periscope.azure.github.com", "--ignore-not-found"], stderr=subprocess.STDOUT) @@ -199,7 +200,7 @@ def aks_kollect_cmd(cmd, # pylint: disable=too-many-statements,too-many-local print() print("Deploying aks-periscope") - subprocess.check_output(["kubectl", "--kubeconfig", temp_kubeconfig_path, "apply", "-k", + subprocess.check_output([kubectl_path, "--kubeconfig", temp_kubeconfig_path, "apply", "-k", kustomize_folder, "-n", CONST_PERISCOPE_NAMESPACE], stderr=subprocess.STDOUT) except subprocess.CalledProcessError as err: raise CLIError(err.output) from err @@ -249,7 +250,8 @@ def _get_temp_kubeconfig_path(cmd, client, resource_group_name: str, name: str, if has_aad_profile: # The current credentials require interactive login. We need to use kubelogin to update the kubeconfig credential. - if not which('kubelogin'): + kubelogin_path = which('kubelogin') + if not kubelogin_path: # No kubelogin found...but we can install it if the user wants. if not prompt_y_n('Can not find kubelogin executable in PATH. Install now?', default="y"): # The user doesn't want us to install kubelogin automatically, so we cannot continue. @@ -257,10 +259,13 @@ def _get_temp_kubeconfig_path(cmd, client, resource_group_name: str, name: str, # Install kubelogin kubelogin_install_location = _get_default_install_location('kubelogin') - k8s_install_kubelogin(cmd, 'latest', kubelogin_install_location) + if not kubelogin_install_location: + raise CLIError('Can not determine kubelogin install location. Use az aks install-cli to install.') + kubelogin_path = os.path.abspath(kubelogin_install_location) + k8s_install_kubelogin(cmd, 'latest', kubelogin_path) # kubelogin is installed. Run it to populate user credentials that don't require interactive login. - subprocess.check_output(["kubelogin", "convert-kubeconfig", "--kubeconfig", temp_kubeconfig_path, "--login", "azurecli"], stderr=subprocess.STDOUT) + subprocess.check_output([kubelogin_path, "convert-kubeconfig", "--kubeconfig", temp_kubeconfig_path, "--login", "azurecli"], stderr=subprocess.STDOUT) return temp_kubeconfig_path @@ -388,11 +393,12 @@ def _is_windows_hpc_supported(agent_pools): def _display_diagnostics_report(temp_kubeconfig_path): # pylint: disable=too-many-statements - if not which('kubectl'): + kubectl_path = which('kubectl') + if not kubectl_path: raise CLIError('Can not find kubectl executable in PATH') nodes = subprocess.check_output( - ["kubectl", "--kubeconfig", temp_kubeconfig_path, + [kubectl_path, "--kubeconfig", temp_kubeconfig_path, "get", "node", "--no-headers"], universal_newlines=True) logger.debug(nodes) @@ -422,7 +428,7 @@ def _display_diagnostics_report(temp_kubeconfig_path): # pylint: disable=too-m for retry in range(0, max_retry): if not apds_created: apd = subprocess.check_output( - ["kubectl", "--kubeconfig", temp_kubeconfig_path, "get", + [kubectl_path, "--kubeconfig", temp_kubeconfig_path, "get", "apd", "-n", CONST_PERISCOPE_NAMESPACE, "--no-headers"], universal_newlines=True ) @@ -443,14 +449,14 @@ def _display_diagnostics_report(temp_kubeconfig_path): # pylint: disable=too-m apdName = "aks-periscope-diagnostic-" + node_name try: network_config = subprocess.check_output( - ["kubectl", "--kubeconfig", temp_kubeconfig_path, + [kubectl_path, "--kubeconfig", temp_kubeconfig_path, "get", "apd", apdName, "-n", CONST_PERISCOPE_NAMESPACE, "-o=jsonpath={.spec.networkconfig}"], universal_newlines=True) logger.debug('Dns status for node %s is %s', node_name, network_config) network_status = subprocess.check_output( - ["kubectl", "--kubeconfig", temp_kubeconfig_path, + [kubectl_path, "--kubeconfig", temp_kubeconfig_path, "get", "apd", apdName, "-n", CONST_PERISCOPE_NAMESPACE, "-o=jsonpath={.spec.networkoutbound}"], universal_newlines=True) diff --git a/src/aks-preview/azext_aks_preview/bastion/bastion.py b/src/aks-preview/azext_aks_preview/bastion/bastion.py index 99002152517..44daf9baadb 100644 --- a/src/aks-preview/azext_aks_preview/bastion/bastion.py +++ b/src/aks-preview/azext_aks_preview/bastion/bastion.py @@ -5,7 +5,7 @@ import asyncio import os -import shutil +import shlex import signal import socket import subprocess @@ -15,6 +15,7 @@ import psutil import yaml +from azext_aks_preview._helpers import which from azure.cli.command_modules.acs._consts import DecoratorEarlyExitException from azure.cli.core.azclierror import (CLIInternalError, InvalidArgumentValueError, @@ -276,13 +277,14 @@ async def aks_bastion_runner( else: task2 = asyncio.create_task(_aks_bastion_launch_subshell(kubeconfig_path, port)) - _, pending = await asyncio.wait([task1, task2], return_when=asyncio.FIRST_COMPLETED) + done, pending = await asyncio.wait([task1, task2], return_when=asyncio.FIRST_COMPLETED) for task in pending: task.cancel() # Wait for the cancellations to finish await asyncio.gather(*pending, return_exceptions=True) + await asyncio.gather(*done) def aks_batsion_clean_up(): @@ -290,11 +292,12 @@ def aks_batsion_clean_up(): def _aks_bastion_get_az_cmd_name(): - """Get the name of the az command based on system platform.""" + """Resolve the az command for the current platform.""" - if sys.platform.startswith("win"): - return "az.cmd" - return "az" + executable = which("az.cmd" if sys.platform.startswith("win") else "az") + if not executable: + raise CLIInternalError("Cannot find Azure CLI executable in PATH.") + return executable def _aks_bastion_get_current_shell_cmd(): @@ -380,15 +383,15 @@ def _get_powershell_executable(grandparent): def _get_powershell_executable_from_path(): """Try to find PowerShell executable in PATH, preferring pwsh over powershell.""" - pwsh_path = shutil.which("pwsh") + pwsh_path = which("pwsh") if pwsh_path: logger.debug("Found pwsh at: %s", pwsh_path) - return "pwsh" + return pwsh_path - powershell_path = shutil.which("powershell") + powershell_path = which("powershell") if powershell_path: logger.debug("Found powershell at: %s", powershell_path) - return "powershell" + return powershell_path return None @@ -396,26 +399,32 @@ def _get_powershell_executable_from_path(): def _aks_bastion_prepare_shell_cmd(kubeconfig_path): """Prepare the shell command to launch a subshell with KUBECONFIG set.""" - shell_cmd = _aks_bastion_get_current_shell_cmd() - updated_shell_cmd = shell_cmd + detected_shell = _aks_bastion_get_current_shell_cmd() + shell_cmd = which(detected_shell) + if not shell_cmd: + raise CLIInternalError(f"Cannot find shell executable '{detected_shell}' in PATH.") + is_windows = sys.platform.startswith("win") + quoted_shell_cmd = f'"{shell_cmd}"' if is_windows else shlex.quote(shell_cmd) + updated_shell_cmd = quoted_shell_cmd # Handle different shell types if shell_cmd.endswith("bash") and os.path.exists(os.path.expanduser("~/.bashrc")): - updated_shell_cmd = ( - f"""{shell_cmd} -c '{shell_cmd} --rcfile <(cat ~/.bashrc; """ - f"""echo "export KUBECONFIG={kubeconfig_path}")'""" + export_command = f"export KUBECONFIG={shlex.quote(kubeconfig_path)}" + bash_command = ( + f"{quoted_shell_cmd} --rcfile <(cat ~/.bashrc; " + f"printf '%s\\n' {shlex.quote(export_command)})" ) + updated_shell_cmd = f"{quoted_shell_cmd} -c {shlex.quote(bash_command)}" elif shell_cmd in ["pwsh", "powershell"] or "pwsh" in shell_cmd.lower() or "powershell" in shell_cmd.lower(): # PowerShell: Set environment variable and start new session # Use proper PowerShell syntax for setting environment variables escaped_path = kubeconfig_path.replace("'", "''") # Escape single quotes for PowerShell - if shell_cmd == "pwsh" or "pwsh" in shell_cmd.lower(): - updated_shell_cmd = f'pwsh -NoExit -Command "$env:KUBECONFIG=\'{escaped_path}\'"' - else: - updated_shell_cmd = f'powershell -NoExit -Command "$env:KUBECONFIG=\'{escaped_path}\'"' + powershell_command = f"$env:KUBECONFIG='{escaped_path}'" + quoted_command = f'"{powershell_command}"' if is_windows else shlex.quote(powershell_command) + updated_shell_cmd = f"{quoted_shell_cmd} -NoExit -Command {quoted_command}" elif shell_cmd == "cmd" or "cmd" in shell_cmd.lower(): # CMD: Set environment variable and keep session open - updated_shell_cmd = f'cmd /k "set KUBECONFIG={kubeconfig_path}"' + updated_shell_cmd = f'{quoted_shell_cmd} /k set "KUBECONFIG={kubeconfig_path}"' return shell_cmd, updated_shell_cmd @@ -443,6 +452,11 @@ async def _aks_bastion_launch_subshell(kubeconfig_path, port): env = os.environ.copy() env.update({"KUBECONFIG": kubeconfig_path}) shell_cmd, updated_shell_cmd = _aks_bastion_prepare_shell_cmd(kubeconfig_path) + shell_executable = None + if sys.platform.startswith("win"): + shell_executable = which("cmd.exe") + if not shell_executable: + raise CLIInternalError("Cannot find command processor executable 'cmd.exe' in PATH.") logger.warning( "Launching subshell with command '%s'. Setting env var KUBECONFIG to '%s'.", updated_shell_cmd, @@ -458,6 +472,7 @@ async def _aks_bastion_launch_subshell(kubeconfig_path, port): stderr=None, shell=True, env=env, + executable=shell_executable, ) logger.info("Subshell launched with PID: %s", subshell_process.pid) @@ -493,22 +508,24 @@ async def _aks_bastion_launch_tunnel(bastion_resource, port, mc_id, subscription tunnel_proces = None try: az_cmd_name = _aks_bastion_get_az_cmd_name() - cmd = ( - f"{az_cmd_name} network bastion tunnel --resource-group {bastion_resource.resource_group} " - f"--name {bastion_resource.name} --port {port} --target-resource-id {mc_id} --resource-port 443" - ) + cmd = [ + az_cmd_name, "network", "bastion", "tunnel", + "--resource-group", bastion_resource.resource_group, + "--name", bastion_resource.name, "--port", str(port), + "--target-resource-id", mc_id, "--resource-port", "443", + ] # the bastion may live in a different subscription than the cluster; prefer the # subscription resolved from the bastion resource over the cluster subscription bastion_subscription_id = getattr(bastion_resource, "subscription", None) or subscription_id if bastion_subscription_id: - cmd += f" --subscription {bastion_subscription_id}" - logger.warning("Creating bastion tunnel with command: '%s'", cmd) + cmd.extend(["--subscription", bastion_subscription_id]) + logger.warning("Creating bastion tunnel with command: '%s'", subprocess.list2cmdline(cmd)) # Use start_new_session on Unix to create a new process group # This allows us to kill the entire process tree when cleaning up start_new_session = not sys.platform.startswith("win") tunnel_proces = await asyncio.create_subprocess_exec( - *(cmd.split()), + *cmd, stdin=asyncio.subprocess.DEVNULL, stdout=asyncio.subprocess.DEVNULL, stderr=asyncio.subprocess.DEVNULL, @@ -564,8 +581,13 @@ def _aks_bastion_kill_process_tree(process): if sys.platform.startswith("win"): # On Windows, use taskkill with /T flag to kill the process tree try: + taskkill_path = which("taskkill.exe") + if not taskkill_path: + logger.warning("Cannot find taskkill.exe in PATH. Terminating the tunnel process directly.") + process.terminate() + return subprocess.run( - ["taskkill", "/T", "/F", "/PID", str(pid)], + [taskkill_path, "/T", "/F", "/PID", str(pid)], capture_output=True, check=False, ) @@ -616,12 +638,15 @@ async def _aks_bastion_test_hook(kubeconfig_path, port, kubectl_path): """Test hook to validate the bastion tunnel and run a kubectl command.""" if not await _aks_bastion_validate_tunnel(port): raise CLIInternalError(f"Bastion tunnel failed to set up on port {port}.") - kubectl_process = await asyncio.create_subprocess_shell( - f"{kubectl_path} --kubeconfig {kubeconfig_path} get nodes", + executable = which(kubectl_path) + if not executable: + raise CLIInternalError(f"Cannot find kubectl executable '{kubectl_path}'.") + kubectl_process = await asyncio.create_subprocess_exec( + executable, "--kubeconfig", kubeconfig_path, "get", "nodes", stdin=asyncio.subprocess.DEVNULL, stdout=asyncio.subprocess.DEVNULL, stderr=asyncio.subprocess.DEVNULL, - shell=True, + shell=False, ) await asyncio.wait_for(kubectl_process.wait(), timeout=10) if kubectl_process.returncode != 0: diff --git a/src/aks-preview/azext_aks_preview/custom.py b/src/aks-preview/azext_aks_preview/custom.py index 53c4d0cce41..2bee70b31eb 100644 --- a/src/aks-preview/azext_aks_preview/custom.py +++ b/src/aks-preview/azext_aks_preview/custom.py @@ -1930,11 +1930,12 @@ def aks_get_credentials( path, kubeconfig, overwrite_existing, context_name) # Check if kubeconfig requires kubelogin with devicecode and convert it if uses_kubelogin_devicecode(kubeconfig): - if which("kubelogin"): + kubelogin_path = which("kubelogin") + if kubelogin_path: try: # Run kubelogin convert-kubeconfig -l azurecli subprocess.run( - ["kubelogin", "convert-kubeconfig", "-l", "azurecli"], + [kubelogin_path, "convert-kubeconfig", "-l", "azurecli"], cwd=os.path.dirname(path), check=True, ) diff --git a/src/aks-preview/azext_aks_preview/tests/latest/test_aks_bastion.py b/src/aks-preview/azext_aks_preview/tests/latest/test_aks_bastion.py index c42d4df02e7..dc6431f5939 100644 --- a/src/aks-preview/azext_aks_preview/tests/latest/test_aks_bastion.py +++ b/src/aks-preview/azext_aks_preview/tests/latest/test_aks_bastion.py @@ -3,16 +3,20 @@ # Licensed under the MIT License. See License.txt in the project root for license information. # -------------------------------------------------------------------------------------------- +import asyncio +import shlex import unittest from types import SimpleNamespace -from unittest.mock import AsyncMock, MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, call, patch +from azext_aks_preview.bastion import bastion as commands from azext_aks_preview.bastion.bastion import ( BastionResource, _aks_bastion_launch_tunnel, aks_bastion_parse_bastion_resource, ) from azext_aks_preview.custom import aks_bastion_tunnel +from azure.cli.core.azclierror import CLIInternalError class TestAksBastionParseResource(unittest.TestCase): @@ -64,7 +68,9 @@ async def test_tunnel_uses_bastion_subscription(self): with patch( "azext_aks_preview.bastion.bastion.asyncio.create_subprocess_exec", new=AsyncMock(return_value=mock_process), - ) as mock_exec: + ) as mock_exec, patch.object( + commands, "_aks_bastion_get_az_cmd_name", return_value="/installed tools/az" + ): await _aks_bastion_launch_tunnel( bastion_resource, port=12345, @@ -74,6 +80,7 @@ async def test_tunnel_uses_bastion_subscription(self): ) args = mock_exec.call_args.args + self.assertEqual(args[0], "/installed tools/az") self.assertIn("--subscription", args) sub_index = args.index("--subscription") self.assertEqual(args[sub_index + 1], "hub-sub-id") @@ -94,7 +101,9 @@ async def test_tunnel_falls_back_to_cluster_subscription(self): with patch( "azext_aks_preview.bastion.bastion.asyncio.create_subprocess_exec", new=AsyncMock(return_value=mock_process), - ) as mock_exec: + ) as mock_exec, patch.object( + commands, "_aks_bastion_get_az_cmd_name", return_value="/installed tools/az" + ): await _aks_bastion_launch_tunnel( bastion_resource, port=12345, @@ -104,11 +113,223 @@ async def test_tunnel_falls_back_to_cluster_subscription(self): ) args = mock_exec.call_args.args + self.assertEqual(args[0], "/installed tools/az") self.assertIn("--subscription", args) sub_index = args.index("--subscription") self.assertEqual(args[sub_index + 1], "aks-sub-id") +class TestBastionExecutablePaths(unittest.TestCase): + def test_powershell_returns_selected_path_and_preserves_preference(self): + pwsh = r"C:\Program Files\PowerShell\7\pwsh.exe" + powershell = r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe" + for results, expected, calls in ( + ([pwsh], pwsh, [call("pwsh")]), + ([None, powershell], powershell, [call("pwsh"), call("powershell")]), + ([None, None], None, [call("pwsh"), call("powershell")]), + ): + with self.subTest(results=results), patch.object(commands, "which", side_effect=results) as resolve: + self.assertEqual(commands._get_powershell_executable_from_path(), expected) + self.assertEqual(resolve.call_args_list, calls) + + def test_powershell_parent_executable_fallback_is_preserved(self): + parent = MagicMock() + parent.exe.return_value = r"C:\Custom Tools\pwsh.exe" + with patch.object(commands, "_get_powershell_executable_from_path", return_value=None): + self.assertEqual(commands._get_powershell_executable(parent), parent.exe.return_value) + self.assertEqual( + commands._handle_powershell_parent(parent.exe.return_value, "pwsh.exe"), + parent.exe.return_value, + ) + + def test_windows_shell_commands_keep_quoted_absolute_executables(self): + for executable, expected in ( + (r"C:\Program Files\PowerShell\7\pwsh.exe", '-NoExit -Command'), + (r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe", '-NoExit -Command'), + (r"C:\Windows\System32\cmd.exe", '/k'), + ): + with self.subTest(executable=executable), patch.object( + commands, "_aks_bastion_get_current_shell_cmd", return_value=executable + ), patch.object(commands, "which", return_value=executable), patch.object( + commands.sys, "platform", "win32" + ): + shell, command = commands._aks_bastion_prepare_shell_cmd(r"C:\test config\config") + self.assertEqual(shell, executable) + self.assertTrue(command.startswith(f'"{executable}" {expected}')) + self.assertIn(r"C:\test config\config", command) + + def test_posix_powershell_command_does_not_expand_environment_in_outer_shell(self): + executable = "/installed tools/pwsh" + kubeconfig = "/test config/user's config" + with patch.object( + commands, "_aks_bastion_get_current_shell_cmd", return_value="pwsh" + ), patch.object(commands, "which", return_value=executable), patch.object( + commands.sys, "platform", "linux" + ): + shell, command = commands._aks_bastion_prepare_shell_cmd(kubeconfig) + self.assertEqual(shell, executable) + self.assertEqual( + shlex.split(command), + [executable, "-NoExit", "-Command", "$env:KUBECONFIG='/test config/user''s config'"], + ) + + def test_bash_rcfile_preserves_absolute_executable_and_quoting(self): + executable = "/installed tools/bash" + with patch.object( + commands, "_aks_bastion_get_current_shell_cmd", return_value="bash" + ), patch.object(commands, "which", return_value=executable), patch.object( + commands.os.path, "exists", return_value=True + ), patch.object(commands.sys, "platform", "linux"): + shell, command = commands._aks_bastion_prepare_shell_cmd("/test config/user's config") + self.assertEqual(shell, executable) + arguments = shlex.split(command) + self.assertEqual(arguments[:2], [executable, "-c"]) + self.assertTrue(arguments[2].startswith(shlex.quote(executable) + " --rcfile")) + + def test_bash_without_rcfile_launches_selected_shell_directly(self): + executable = "/installed tools/bash" + with patch.object( + commands, "_aks_bastion_get_current_shell_cmd", return_value="bash" + ), patch.object(commands, "which", return_value=executable), patch.object( + commands.os.path, "exists", return_value=False + ), patch.object(commands.sys, "platform", "linux"): + shell, command = commands._aks_bastion_prepare_shell_cmd("test-kubeconfig") + self.assertEqual(shell, executable) + self.assertEqual(shlex.split(command), [executable]) + + def test_missing_shell_fails_instead_of_executing_bare_name(self): + with patch.object(commands, "_aks_bastion_get_current_shell_cmd", return_value="pwsh"), patch.object( + commands, "which", return_value=None + ): + with self.assertRaisesRegex(CLIInternalError, "shell executable"): + commands._aks_bastion_prepare_shell_cmd("test-kubeconfig") + + def test_azure_cli_is_resolved_for_each_platform(self): + for platform, name in (("win32", "az.cmd"), ("linux", "az")): + with self.subTest(platform=platform), patch.object(commands.sys, "platform", platform), patch.object( + commands, "which", return_value="/installed tools/" + name + ) as resolve: + self.assertEqual(commands._aks_bastion_get_az_cmd_name(), "/installed tools/" + name) + resolve.assert_called_once_with(name) + + def test_missing_azure_cli_fails_instead_of_executing_bare_name(self): + with patch.object(commands, "which", return_value=None): + with self.assertRaisesRegex(CLIInternalError, "Azure CLI executable"): + commands._aks_bastion_get_az_cmd_name() + + def test_windows_process_cleanup_uses_resolved_taskkill(self): + process = MagicMock(pid=12345) + executable = r"C:\Windows\System32\taskkill.exe" + with patch.object(commands.sys, "platform", "win32"), patch.object( + commands, "which", return_value=executable + ) as resolve, patch.object(commands.subprocess, "run") as run: + commands._aks_bastion_kill_process_tree(process) + resolve.assert_called_once_with("taskkill.exe") + run.assert_called_once_with( + [executable, "/T", "/F", "/PID", "12345"], capture_output=True, check=False + ) + process.terminate.assert_not_called() + + def test_missing_taskkill_warns_and_uses_process_handle(self): + process = MagicMock(pid=12345) + with patch.object(commands.sys, "platform", "win32"), patch.object( + commands, "which", return_value=None + ), patch.object(commands.subprocess, "run") as run, patch.object(commands.logger, "warning") as warning: + commands._aks_bastion_kill_process_tree(process) + run.assert_not_called() + process.terminate.assert_called_once() + warning.assert_called_once() + + +class TestBastionSubprocessPaths(unittest.IsolatedAsyncioTestCase): + async def test_windows_subshell_pins_outer_command_processor(self): + process = MagicMock() + process.wait = AsyncMock() + command_processor = r"C:\Windows\System32\cmd.exe" + with patch.object(commands, "_aks_bastion_validate_tunnel", return_value=True), patch.object( + commands, "_aks_bastion_prepare_shell_cmd", return_value=("pwsh.exe", '"selected pwsh.exe" -NoExit') + ), patch.object(commands.sys, "platform", "win32"), patch.object( + commands, "which", return_value=command_processor + ) as resolve, patch.object( + commands.asyncio.subprocess, "create_subprocess_shell", return_value=process + ) as run: + await commands._aks_bastion_launch_subshell("test-kubeconfig", 12345) + resolve.assert_called_once_with("cmd.exe") + self.assertEqual(run.call_args.kwargs["executable"], command_processor) + self.assertEqual(run.call_args.kwargs["env"]["KUBECONFIG"], "test-kubeconfig") + self.assertEqual(run.call_args.kwargs["cmd"], '"selected pwsh.exe" -NoExit') + self.assertTrue(run.call_args.kwargs["shell"]) + + async def test_missing_windows_command_processor_fails_before_launch(self): + with patch.object(commands, "_aks_bastion_validate_tunnel", return_value=True), patch.object( + commands, "_aks_bastion_prepare_shell_cmd", return_value=("pwsh.exe", '"selected pwsh.exe" -NoExit') + ), patch.object(commands.sys, "platform", "win32"), patch.object( + commands, "which", return_value=None + ), patch.object(commands.asyncio.subprocess, "create_subprocess_shell") as run: + with self.assertRaisesRegex(CLIInternalError, "command processor executable"): + await commands._aks_bastion_launch_subshell("test-kubeconfig", 12345) + run.assert_not_called() + + async def test_posix_subshell_preserves_inherited_streams_and_environment(self): + process = MagicMock() + process.wait = AsyncMock() + with patch.object(commands, "_aks_bastion_validate_tunnel", return_value=True), patch.object( + commands, "_aks_bastion_prepare_shell_cmd", return_value=("/bin/bash", "/bin/bash") + ), patch.object(commands.sys, "platform", "linux"), patch.object( + commands, "which" + ) as resolve, patch.object( + commands.asyncio.subprocess, "create_subprocess_shell", return_value=process + ) as run: + await commands._aks_bastion_launch_subshell("test-kubeconfig", 12345) + resolve.assert_not_called() + self.assertIsNone(run.call_args.kwargs["executable"]) + for stream in ("stdin", "stdout", "stderr"): + self.assertIsNone(run.call_args.kwargs[stream]) + self.assertEqual(run.call_args.kwargs["env"]["KUBECONFIG"], "test-kubeconfig") + + async def test_test_hook_uses_explicit_path_and_argument_list(self): + process = MagicMock(returncode=0) + process.wait = AsyncMock() + executable = "/installed tools/kubectl" + with patch.object(commands, "_aks_bastion_validate_tunnel", return_value=True), patch.object( + commands, "which", return_value=executable + ) as resolve, patch.object(commands.asyncio, "create_subprocess_exec", return_value=process) as run: + await commands._aks_bastion_test_hook("/test config/config", 12345, executable) + resolve.assert_called_once_with(executable) + self.assertEqual( + run.call_args.args, (executable, "--kubeconfig", "/test config/config", "get", "nodes") + ) + self.assertFalse(run.call_args.kwargs["shell"]) + + async def test_missing_test_hook_executable_fails_before_launch(self): + with patch.object(commands, "_aks_bastion_validate_tunnel", return_value=True), patch.object( + commands, "which", return_value=None + ), patch.object(commands.asyncio, "create_subprocess_exec") as run: + with self.assertRaisesRegex(CLIInternalError, "kubectl executable"): + await commands._aks_bastion_test_hook("test-kubeconfig", 12345, "/missing/kubectl") + run.assert_not_called() + + async def test_runner_propagates_failures_after_cancelling_other_task(self): + cancelled = asyncio.Event() + + async def wait_for_cancellation(*args): + try: + await asyncio.Event().wait() + finally: + cancelled.set() + + with patch.object( + commands, "_aks_bastion_launch_tunnel", side_effect=CLIInternalError("executable not found") + ), patch.object( + commands, "_aks_bastion_launch_subshell", side_effect=wait_for_cancellation + ): + with self.assertRaisesRegex(CLIInternalError, "executable not found"): + await commands.aks_bastion_runner( + BastionResource("bastion", "rg"), 12345, "test-cluster-id", "test-kubeconfig" + ) + self.assertTrue(cancelled.is_set()) + + class TestAksBastionTunnel(unittest.TestCase): def _run_tunnel(self, bastion_profile, bastion=None): cmd = SimpleNamespace(cli_ctx=MagicMock()) diff --git a/src/aks-preview/azext_aks_preview/tests/latest/test_aks_diagnostics.py b/src/aks-preview/azext_aks_preview/tests/latest/test_aks_diagnostics.py index 5bf1706da3e..60111f76ea7 100644 --- a/src/aks-preview/azext_aks_preview/tests/latest/test_aks_diagnostics.py +++ b/src/aks-preview/azext_aks_preview/tests/latest/test_aks_diagnostics.py @@ -3,11 +3,15 @@ # Licensed under the MIT License. See License.txt in the project root for license information. # -------------------------------------------------------------------------------------------- +import os +import subprocess +import tempfile import unittest from types import SimpleNamespace from unittest import mock import azext_aks_preview.aks_diagnostics as commands +from knack.util import CLIError class TestGenerateContainerName(unittest.TestCase): @@ -43,6 +47,78 @@ def test_attribute_sdk_model(self): class TestGetTempKubeconfigPath(unittest.TestCase): + def _get_aad_kubeconfig(self): + client = mock.Mock() + client.list_cluster_user_credentials.return_value = SimpleNamespace( + kubeconfigs=[SimpleNamespace(value=b"apiVersion: v1\nkind: Config\n")] + ) + with mock.patch.object(commands.tempfile, "mkstemp", return_value=(0, "test-kubeconfig")), mock.patch.object( + commands, "print_or_merge_credentials" + ): + return commands._get_temp_kubeconfig_path(None, client, "rg", "cluster", True) + + def test_executes_discovered_kubelogin(self): + executable = os.path.join(tempfile.gettempdir(), "installed tools", "kubelogin") + with mock.patch.object(commands, "which", return_value=executable) as resolve, mock.patch.object( + commands.subprocess, "check_output" + ) as run: + self.assertEqual(self._get_aad_kubeconfig(), "test-kubeconfig") + resolve.assert_called_once_with("kubelogin") + run.assert_called_once_with( + [executable, "convert-kubeconfig", "--kubeconfig", "test-kubeconfig", "--login", "azurecli"], + stderr=subprocess.STDOUT, + ) + + def test_executes_newly_installed_kubelogin_without_path_lookup(self): + executable = os.path.join(tempfile.gettempdir(), "installed tools", "kubelogin") + with mock.patch.object(commands, "which", return_value=None) as resolve, mock.patch.object( + commands, "prompt_y_n", return_value=True + ), mock.patch.object( + commands, "_get_default_install_location", return_value=executable + ), mock.patch.object(commands, "k8s_install_kubelogin") as install, mock.patch.object( + commands.subprocess, "check_output" + ) as run: + self._get_aad_kubeconfig() + resolve.assert_called_once_with("kubelogin") + install.assert_called_once_with(None, "latest", executable) + self.assertEqual(run.call_args.args[0][0], executable) + + def test_declining_installation_does_not_execute_kubelogin(self): + with mock.patch.object(commands, "which", return_value=None), mock.patch.object( + commands, "prompt_y_n", return_value=False + ), mock.patch.object(commands, "k8s_install_kubelogin") as install, mock.patch.object( + commands.subprocess, "check_output" + ) as run: + with self.assertRaisesRegex(CLIError, "kubelogin not found"): + self._get_aad_kubeconfig() + install.assert_not_called() + run.assert_not_called() + + def test_missing_install_location_fails_without_execution(self): + with mock.patch.object(commands, "which", return_value=None), mock.patch.object( + commands, "prompt_y_n", return_value=True + ), mock.patch.object( + commands, "_get_default_install_location", return_value=None + ), mock.patch.object(commands, "k8s_install_kubelogin") as install, mock.patch.object( + commands.subprocess, "check_output" + ) as run: + with self.assertRaisesRegex(CLIError, "install location"): + self._get_aad_kubeconfig() + install.assert_not_called() + run.assert_not_called() + + def test_installation_failure_does_not_execute_another_binary(self): + with mock.patch.object(commands, "which", return_value=None), mock.patch.object( + commands, "prompt_y_n", return_value=True + ), mock.patch.object( + commands, "_get_default_install_location", return_value="/installed/kubelogin" + ), mock.patch.object( + commands, "k8s_install_kubelogin", side_effect=CLIError("installation failed") + ), mock.patch.object(commands.subprocess, "check_output") as run: + with self.assertRaisesRegex(CLIError, "installation failed"): + self._get_aad_kubeconfig() + run.assert_not_called() + def test_calls_list_cluster_user_credentials_with_keyword_only_server_fqdn(self): """Regression test: the SDK signature made server_fqdn/format keyword-only. @@ -77,5 +153,62 @@ def fake_list_cluster_user_credentials(resource_group_name, name, *, server_fqdn ) +class TestDiagnosticsExecutablePaths(unittest.TestCase): + def test_kollect_pins_all_cleanup_and_apply_commands(self): + executable = os.path.join(tempfile.gettempdir(), "installed tools", "kubectl") + client = mock.Mock() + client.get.return_value = SimpleNamespace( + aad_profile=None, fqdn="cluster.example", private_fqdn=None + ) + storage_account = ( + "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg/" + "providers/Microsoft.Storage/storageAccounts/teststorage" + ) + with mock.patch.object(commands, "which", return_value=executable) as resolve, mock.patch.object( + commands, "prompt_y_n", side_effect=[True, False] + ), mock.patch.object( + commands, "_get_temp_kubeconfig_path", return_value="test-kubeconfig" + ), mock.patch.object( + commands, "_generate_container_name", return_value="test-container" + ), mock.patch.object( + commands, "_get_cluster_features", return_value=commands.ClusterFeatures.NONE + ), mock.patch.object( + commands, "_get_kustomize_yaml", return_value="resources: []" + ), mock.patch.object(commands.subprocess, "call") as cleanup, mock.patch.object( + commands.subprocess, "check_output" + ) as apply: + commands.aks_kollect_cmd( + mock.Mock(), client, "rg", "cluster", storage_account, "placeholder", + None, None, None, None, + ) + resolve.assert_called_once_with("kubectl") + self.assertEqual(cleanup.call_count, 6) + for call in cleanup.call_args_list: + self.assertEqual(call.args[0][:4], [executable, "--kubeconfig", "test-kubeconfig", "delete"]) + apply.assert_called_once() + self.assertEqual(apply.call_args.args[0][:5], [executable, "--kubeconfig", "test-kubeconfig", "apply", "-k"]) + + def test_analysis_pins_all_query_commands(self): + executable = os.path.join(tempfile.gettempdir(), "installed tools", "kubectl") + with mock.patch.object(commands, "which", return_value=executable) as resolve, mock.patch.object( + commands.subprocess, "check_output", + side_effect=["node1 Ready", "diagnostic1", '{"Name":"node1"}', '[{"Status":"OK"}]'], + ) as run: + commands._display_diagnostics_report("test-kubeconfig") + resolve.assert_called_once_with("kubectl") + self.assertEqual(run.call_count, 4) + for call in run.call_args_list: + self.assertEqual(call.args[0][:3], [executable, "--kubeconfig", "test-kubeconfig"]) + self.assertTrue(call.kwargs["universal_newlines"]) + + def test_missing_kubectl_fails_before_execution(self): + with mock.patch.object(commands, "which", return_value=None), mock.patch.object( + commands.subprocess, "check_output" + ) as run: + with self.assertRaisesRegex(CLIError, "kubectl executable in PATH"): + commands._display_diagnostics_report("test-kubeconfig") + run.assert_not_called() + + if __name__ == "__main__": unittest.main() diff --git a/src/aks-preview/azext_aks_preview/tests/latest/test_custom.py b/src/aks-preview/azext_aks_preview/tests/latest/test_custom.py index f8764fd2fc2..92b63c2008b 100644 --- a/src/aks-preview/azext_aks_preview/tests/latest/test_custom.py +++ b/src/aks-preview/azext_aks_preview/tests/latest/test_custom.py @@ -2,7 +2,11 @@ # Copyright (c) Microsoft Corporation. All rights reserved. # Licensed under the MIT License. See License.txt in the project root for license information. # -------------------------------------------------------------------------------------------- +import os +import subprocess +import tempfile import unittest +from types import SimpleNamespace from unittest.mock import Mock, patch from azext_aks_preview.__init__ import register_aks_preview_resource_type @@ -16,6 +20,7 @@ from azext_aks_preview.custom import ( aks_agentpool_get_bootstrap_data, aks_agentpool_upgrade, + aks_get_credentials, aks_machine_add, aks_machine_update, aks_stop, @@ -39,6 +44,60 @@ from knack.util import CLIError +class TestGetCredentialsExecutablePath(unittest.TestCase): + def setUp(self): + self.path = os.path.join(tempfile.gettempdir(), "test config", "config") + self.client = Mock() + self.client.list_cluster_user_credentials.return_value = SimpleNamespace( + kubeconfigs=[SimpleNamespace(value=( + b"users:\n- name: test-user\n user:\n exec:\n" + b" command: kubelogin\n args: [get-token, --login, devicecode]\n" + ))] + ) + + def test_executes_resolved_kubelogin_and_preserves_arguments(self): + executable = os.path.join(tempfile.gettempdir(), "installed tools", "kubelogin") + with patch("azext_aks_preview.custom.print_or_merge_credentials"), patch( + "azext_aks_preview.custom.which", return_value=executable + ) as resolve, patch("azext_aks_preview.custom.subprocess.run") as run: + aks_get_credentials(None, self.client, "rg", "cluster", path=self.path) + resolve.assert_called_once_with("kubelogin") + run.assert_called_once_with( + [executable, "convert-kubeconfig", "-l", "azurecli"], + cwd=os.path.dirname(self.path), + check=True, + ) + + def test_missing_kubelogin_warns_without_execution(self): + with patch("azext_aks_preview.custom.print_or_merge_credentials"), patch( + "azext_aks_preview.custom.which", return_value=None + ), patch("azext_aks_preview.custom.subprocess.run") as run, patch( + "azext_aks_preview.custom.logger.warning" + ) as warning: + aks_get_credentials(None, self.client, "rg", "cluster", path=self.path) + run.assert_not_called() + self.assertIn("Please install kubelogin", warning.call_args.args[0]) + + def test_conversion_failure_keeps_existing_warning_behavior(self): + with patch("azext_aks_preview.custom.print_or_merge_credentials"), patch( + "azext_aks_preview.custom.which", return_value="/installed/kubelogin" + ), patch( + "azext_aks_preview.custom.subprocess.run", + side_effect=subprocess.CalledProcessError(1, "kubelogin"), + ), patch("azext_aks_preview.custom.logger.warning") as warning: + aks_get_credentials(None, self.client, "rg", "cluster", path=self.path) + self.assertIn("Failed to convert kubeconfig", warning.call_args.args[0]) + + def test_other_authentication_does_not_resolve_or_execute_kubelogin(self): + self.client.list_cluster_user_credentials.return_value.kubeconfigs[0].value = b"users: []" + with patch("azext_aks_preview.custom.print_or_merge_credentials"), patch( + "azext_aks_preview.custom.which" + ) as resolve, patch("azext_aks_preview.custom.subprocess.run") as run: + aks_get_credentials(None, self.client, "rg", "cluster", path=self.path) + resolve.assert_not_called() + run.assert_not_called() + + class TestCustomCommand(unittest.TestCase): def setUp(self): # manually register CUSTOM_MGMT_AKS_PREVIEW diff --git a/src/aks-preview/azext_aks_preview/tests/latest/test_helpers.py b/src/aks-preview/azext_aks_preview/tests/latest/test_helpers.py index 28fa06c0edb..55915ad6c04 100644 --- a/src/aks-preview/azext_aks_preview/tests/latest/test_helpers.py +++ b/src/aks-preview/azext_aks_preview/tests/latest/test_helpers.py @@ -3,6 +3,9 @@ # Licensed under the MIT License. See License.txt in the project root for license information. # -------------------------------------------------------------------------------------------- +import ntpath +import os +import tempfile import unittest from types import SimpleNamespace from unittest.mock import Mock, patch @@ -19,6 +22,7 @@ filter_hard_taints, reset_agentpool_to_name_and_mode, validate_flexnodes_options, + which, ) from azext_aks_preview.__init__ import register_aks_preview_resource_type from azext_aks_preview._client_factory import CUSTOM_MGMT_AKS_PREVIEW @@ -38,6 +42,104 @@ from azext_aks_preview.tests.latest.mocks import MockCLI, MockCmd +class TestWhich(unittest.TestCase): + def setUp(self): + directory = tempfile.TemporaryDirectory() + self.addCleanup(directory.cleanup) + self.directory = directory.name + self.tools = os.path.join(self.directory, "installed tools") + self.working_directory = os.path.join(self.directory, "working") + os.mkdir(self.tools) + os.mkdir(self.working_directory) + self.binary = "test-tool.exe" if os.name == "nt" else "test-tool" + self.installed_binary = self._make_executable(self.tools) + self.local_binary = self._make_executable(self.working_directory) + old_cwd = os.getcwd() + os.chdir(self.working_directory) + self.addCleanup(os.chdir, old_cwd) + + def _make_executable(self, directory): + path = os.path.join(directory, self.binary) + with open(path, "w"): + pass + os.chmod(path, 0o700) + return path + + def test_selects_absolute_path_installation_not_working_directory(self): + with patch.dict(os.environ, {"PATH": self.tools}): + self.assertEqual(which(self.binary), self.installed_binary) + + def test_ignores_empty_and_relative_path_entries(self): + os.mkdir("relative") + self._make_executable("relative") + path = os.pathsep.join(("", ".", "relative", self.tools)) + with patch.dict(os.environ, {"PATH": path}): + self.assertEqual(which(self.binary), self.installed_binary) + + def test_missing_or_empty_path_does_not_search_working_directory(self): + for path in (None, "", ".", os.pathsep): + with self.subTest(path=path), patch.dict(os.environ): + if path is None: + os.environ.pop("PATH", None) + else: + os.environ["PATH"] = path + self.assertIsNone(which(self.binary)) + + def test_preserves_explicit_absolute_and_relative_paths(self): + with patch.dict(os.environ, {"PATH": self.tools}): + for path in (self.local_binary, os.path.join(".", self.binary)): + with self.subTest(path=path): + self.assertEqual(which(path), self.local_binary) + + def test_missing_explicit_path_does_not_fall_back_to_path(self): + with patch.dict(os.environ, {"PATH": self.tools}): + self.assertIsNone(which(os.path.join(self.directory, "missing", self.binary))) + + def test_missing_path_installation_does_not_fall_back_to_working_directory(self): + with patch.dict(os.environ, {"PATH": self.directory}): + self.assertIsNone(which(self.binary)) + + def test_ignores_directories_named_like_executables(self): + os.remove(self.installed_binary) + os.mkdir(self.installed_binary) + with patch.dict(os.environ, {"PATH": self.tools}): + self.assertIsNone(which(self.binary)) + + def test_preserves_path_order(self): + with patch.dict(os.environ, {"PATH": os.pathsep.join((self.tools, self.working_directory))}): + self.assertEqual(which(self.binary), self.installed_binary) + + @unittest.skipIf(os.name == "nt", "Windows does not use POSIX executable permission bits") + def test_ignores_non_executable_files(self): + os.chmod(self.installed_binary, 0o600) + with patch.dict(os.environ, {"PATH": self.tools}): + self.assertIsNone(which(self.binary)) + + def test_windows_path_and_extensions(self): + environment = { + "PATH": ';.;relative;C:relative;\\rooted;"C:\\Program Files\\Tools"', + "PATHEXT": ".EXE;.CMD", + } + for binary, expected in ( + ("kubectl", r"C:\Program Files\Tools\kubectl.EXE"), + ("kubectl.exe", r"C:\Program Files\Tools\kubectl.exe"), + ("az.cmd", r"C:\Program Files\Tools\az.cmd"), + ("pwsh", r"C:\Program Files\Tools\pwsh.CMD"), + ): + with self.subTest(binary=binary): + mock_os = Mock() + mock_os.getenv.side_effect = lambda name, default=None: environment.get(name, default) + mock_os.path = Mock(wraps=ntpath) + mock_os.path.isfile.side_effect = lambda path: path.lower() == expected.lower() + mock_os.access.return_value = True + with patch("azext_aks_preview._helpers.platform.system", return_value="Windows"), patch( + "azext_aks_preview._helpers.os", mock_os + ): + self.assertEqual(which(binary), expected) + for call in mock_os.path.isfile.call_args_list: + self.assertTrue(call.args[0].startswith("C:\\Program Files\\Tools\\")) + + class TestFuzzyMatch(unittest.TestCase): def setUp(self): self.expected = ['bord', 'birdy', 'fbird', 'bir', 'ird', 'birdwaj'] From 5523dd5cd920c29285b470986d34a1111bf1f374 Mon Sep 17 00:00:00 2001 From: Fuming Zhang Date: Wed, 7 Oct 2026 11:17:11 +0000 Subject: [PATCH 2/4] {AKS} Remove executable lookup note from README Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/aks-preview/README.rst | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/aks-preview/README.rst b/src/aks-preview/README.rst index 9109b0a6230..cf2b46be9af 100644 --- a/src/aks-preview/README.rst +++ b/src/aks-preview/README.rst @@ -14,15 +14,6 @@ Install this extension using the following CLI command `az extension add --name Remove this extension using the following CLI command `az extension remove --name aks-preview`. -External tools -============== - -The ``az aks kollect``, ``az aks kanalyze``, ``az aks get-credentials`` and -``az aks bastion tunnel`` commands resolve external executables before launching -them. Install these tools in absolute directories on ``PATH``; empty and relative -``PATH`` entries are not searched. Explicit executable paths are preserved, and -paths containing spaces are supported. - Dependency between aks-preview and azure-cli/acs (azure-cli-core) ================================================================= From b7180a8c35ecce960531fc85ae6eada96f67e4a8 Mon Sep 17 00:00:00 2001 From: Fuming Zhang Date: Thu, 8 Oct 2026 00:00:12 +0000 Subject: [PATCH 3/4] {AKS} Propagate unsuccessful Bastion tunnel exits Address review feedback by reporting nonzero tunnel process exits after cancelling the subshell, while preserving normal cancellation cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/aks-preview/HISTORY.rst | 1 + .../azext_aks_preview/bastion/bastion.py | 4 +- .../tests/latest/test_aks_bastion.py | 62 +++++++++++++++++++ 3 files changed, 66 insertions(+), 1 deletion(-) diff --git a/src/aks-preview/HISTORY.rst b/src/aks-preview/HISTORY.rst index 5a18cffa9ae..362db08b65f 100644 --- a/src/aks-preview/HISTORY.rst +++ b/src/aks-preview/HISTORY.rst @@ -12,6 +12,7 @@ To release a new version, please select a new version number (usually plus 1 to Pending +++++++ * `az aks kollect`, `az aks kanalyze`, `az aks get-credentials` and `az aks bastion tunnel`: Use resolved executable paths consistently. +* `az aks bastion tunnel`: Report unsuccessful tunnel process exits instead of returning success. 22.0.0b10 +++++++++ diff --git a/src/aks-preview/azext_aks_preview/bastion/bastion.py b/src/aks-preview/azext_aks_preview/bastion/bastion.py index 44daf9baadb..bd6e01a982c 100644 --- a/src/aks-preview/azext_aks_preview/bastion/bastion.py +++ b/src/aks-preview/azext_aks_preview/bastion/bastion.py @@ -536,7 +536,9 @@ async def _aks_bastion_launch_tunnel(bastion_resource, port, mc_id, subscription # tunnel process must not exit unless it encounters a failure or is deliberately shut down await tunnel_proces.wait() - logger.error("Bastion tunnel exited with code %s", tunnel_proces.returncode) + if tunnel_proces.returncode != 0: + raise CLIInternalError(f"Bastion tunnel exited with code {tunnel_proces.returncode}.") + logger.debug("Bastion tunnel exited with code %s", tunnel_proces.returncode) except asyncio.CancelledError: # attempt to terminate the tunnel process and all its children if tunnel_proces is not None: diff --git a/src/aks-preview/azext_aks_preview/tests/latest/test_aks_bastion.py b/src/aks-preview/azext_aks_preview/tests/latest/test_aks_bastion.py index dc6431f5939..a0a6437833b 100644 --- a/src/aks-preview/azext_aks_preview/tests/latest/test_aks_bastion.py +++ b/src/aks-preview/azext_aks_preview/tests/latest/test_aks_bastion.py @@ -329,6 +329,68 @@ async def wait_for_cancellation(*args): ) self.assertTrue(cancelled.is_set()) + async def test_runner_propagates_nonzero_tunnel_exit_after_cancelling_subshell(self): + for returncode in (1, 17, -15): + with self.subTest(returncode=returncode): + started = asyncio.Event() + cancelled = asyncio.Event() + process = MagicMock(returncode=returncode) + + async def wait_for_exit(): + await started.wait() + return returncode + + process.wait = AsyncMock(side_effect=wait_for_exit) + + async def wait_for_cancellation(*args): + try: + started.set() + await asyncio.Event().wait() + finally: + cancelled.set() + + with patch.object( + commands, "_aks_bastion_get_az_cmd_name", return_value="/installed tools/az" + ), patch.object( + commands.asyncio, "create_subprocess_exec", return_value=process + ), patch.object( + commands, "_aks_bastion_launch_subshell", side_effect=wait_for_cancellation + ): + with self.assertRaisesRegex(CLIInternalError, f"Bastion tunnel exited with code {returncode}"): + await commands.aks_bastion_runner( + BastionResource("bastion", "rg"), 12345, "test-cluster-id", "test-kubeconfig" + ) + process.wait.assert_awaited_once() + self.assertTrue(cancelled.is_set()) + + async def test_runner_cancels_tunnel_normally_when_subshell_exits(self): + started = asyncio.Event() + process = MagicMock(returncode=-15) + + async def wait_for_exit(): + if process.wait.await_count == 1: + started.set() + await asyncio.Event().wait() + return process.returncode + + process.wait = AsyncMock(side_effect=wait_for_exit) + + async def exit_subshell(*args): + await started.wait() + + with patch.object( + commands, "_aks_bastion_get_az_cmd_name", return_value="/installed tools/az" + ), patch.object( + commands.asyncio, "create_subprocess_exec", return_value=process + ), patch.object( + commands, "_aks_bastion_launch_subshell", side_effect=exit_subshell + ), patch.object(commands, "_aks_bastion_kill_process_tree") as kill_tree: + await commands.aks_bastion_runner( + BastionResource("bastion", "rg"), 12345, "test-cluster-id", "test-kubeconfig" + ) + kill_tree.assert_called_once_with(process) + self.assertEqual(process.wait.await_count, 2) + class TestAksBastionTunnel(unittest.TestCase): def _run_tunnel(self, bastion_profile, bastion=None): From 74f98100277c8c629c604beb8c39e4e565825d5d Mon Sep 17 00:00:00 2001 From: Fuming Zhang Date: Thu, 8 Oct 2026 00:20:09 +0000 Subject: [PATCH 4/4] {AKS} Detect Bastion shells by executable basename Address review feedback by normalizing the executable basename before selecting shell-specific arguments. Preserve quoted paths and cover directory-name collisions on POSIX and Windows. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/aks-preview/HISTORY.rst | 1 + .../azext_aks_preview/bastion/bastion.py | 13 ++-- .../tests/latest/test_aks_bastion.py | 68 +++++++++++++++++++ 3 files changed, 78 insertions(+), 4 deletions(-) diff --git a/src/aks-preview/HISTORY.rst b/src/aks-preview/HISTORY.rst index 362db08b65f..021f261683c 100644 --- a/src/aks-preview/HISTORY.rst +++ b/src/aks-preview/HISTORY.rst @@ -13,6 +13,7 @@ Pending +++++++ * `az aks kollect`, `az aks kanalyze`, `az aks get-credentials` and `az aks bastion tunnel`: Use resolved executable paths consistently. * `az aks bastion tunnel`: Report unsuccessful tunnel process exits instead of returning success. +* `az aks bastion tunnel`: Identify interactive shells by executable name rather than directory names. 22.0.0b10 +++++++++ diff --git a/src/aks-preview/azext_aks_preview/bastion/bastion.py b/src/aks-preview/azext_aks_preview/bastion/bastion.py index bd6e01a982c..8106e92a355 100644 --- a/src/aks-preview/azext_aks_preview/bastion/bastion.py +++ b/src/aks-preview/azext_aks_preview/bastion/bastion.py @@ -4,6 +4,7 @@ # -------------------------------------------------------------------------------------------- import asyncio +import ntpath import os import shlex import signal @@ -404,25 +405,29 @@ def _aks_bastion_prepare_shell_cmd(kubeconfig_path): if not shell_cmd: raise CLIInternalError(f"Cannot find shell executable '{detected_shell}' in PATH.") is_windows = sys.platform.startswith("win") + path_module = ntpath if is_windows else os.path + shell_name = path_module.basename(shell_cmd).lower() + if is_windows: + shell_name = path_module.splitext(shell_name)[0] quoted_shell_cmd = f'"{shell_cmd}"' if is_windows else shlex.quote(shell_cmd) updated_shell_cmd = quoted_shell_cmd # Handle different shell types - if shell_cmd.endswith("bash") and os.path.exists(os.path.expanduser("~/.bashrc")): + if not is_windows and shell_name == "bash" and os.path.exists(os.path.expanduser("~/.bashrc")): export_command = f"export KUBECONFIG={shlex.quote(kubeconfig_path)}" bash_command = ( f"{quoted_shell_cmd} --rcfile <(cat ~/.bashrc; " f"printf '%s\\n' {shlex.quote(export_command)})" ) updated_shell_cmd = f"{quoted_shell_cmd} -c {shlex.quote(bash_command)}" - elif shell_cmd in ["pwsh", "powershell"] or "pwsh" in shell_cmd.lower() or "powershell" in shell_cmd.lower(): + elif shell_name in ("pwsh", "powershell"): # PowerShell: Set environment variable and start new session # Use proper PowerShell syntax for setting environment variables escaped_path = kubeconfig_path.replace("'", "''") # Escape single quotes for PowerShell powershell_command = f"$env:KUBECONFIG='{escaped_path}'" quoted_command = f'"{powershell_command}"' if is_windows else shlex.quote(powershell_command) updated_shell_cmd = f"{quoted_shell_cmd} -NoExit -Command {quoted_command}" - elif shell_cmd == "cmd" or "cmd" in shell_cmd.lower(): + elif shell_name == "cmd": # CMD: Set environment variable and keep session open updated_shell_cmd = f'{quoted_shell_cmd} /k set "KUBECONFIG={kubeconfig_path}"' @@ -432,7 +437,7 @@ def _aks_bastion_prepare_shell_cmd(kubeconfig_path): def _aks_bastion_restore_shell(shell_cmd): """Restore the shell settings after the subshell exits.""" - if shell_cmd.endswith("bash"): + if not sys.platform.startswith("win") and os.path.basename(shell_cmd).lower() == "bash": subprocess.run(["stty", "sane"], stdin=sys.stdin) # PowerShell and CMD on Windows typically don't need special restoration # as they handle terminal state management internally diff --git a/src/aks-preview/azext_aks_preview/tests/latest/test_aks_bastion.py b/src/aks-preview/azext_aks_preview/tests/latest/test_aks_bastion.py index a0a6437833b..96b886a10c0 100644 --- a/src/aks-preview/azext_aks_preview/tests/latest/test_aks_bastion.py +++ b/src/aks-preview/azext_aks_preview/tests/latest/test_aks_bastion.py @@ -147,6 +147,10 @@ def test_windows_shell_commands_keep_quoted_absolute_executables(self): (r"C:\Program Files\PowerShell\7\pwsh.exe", '-NoExit -Command'), (r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe", '-NoExit -Command'), (r"C:\Windows\System32\cmd.exe", '/k'), + (r"C:\powershell tools\CMD.EXE", '/k'), + (r"C:\cmd tools\PWSH.EXE", '-NoExit -Command'), + (r"C:\installed tools\pwsh.cmd", '-NoExit -Command'), + (r"C:\installed tools\powershell.bat", '-NoExit -Command'), ): with self.subTest(executable=executable), patch.object( commands, "_aks_bastion_get_current_shell_cmd", return_value=executable @@ -158,6 +162,43 @@ def test_windows_shell_commands_keep_quoted_absolute_executables(self): self.assertTrue(command.startswith(f'"{executable}" {expected}')) self.assertIn(r"C:\test config\config", command) + def test_shell_selection_ignores_directory_names_and_partial_name_matches(self): + for executable in ( + "/home/cmduser/bin/zsh", + "/home/pwshuser/bin/fish", + "/installed powershell tools/dash", + "/installed bash tools/zsh", + "/installed tools/custombash", + "/installed tools/mypwsh", + "/installed tools/powershell-wrapper", + "/installed tools/customcmd", + ): + with self.subTest(executable=executable), patch.object( + commands, "_aks_bastion_get_current_shell_cmd", return_value=executable + ), patch.object(commands, "which", return_value=executable), patch.object( + commands.os.path, "exists", return_value=True + ), patch.object(commands.sys, "platform", "linux"): + shell, command = commands._aks_bastion_prepare_shell_cmd("test-kubeconfig") + self.assertEqual(shell, executable) + self.assertEqual(shlex.split(command), [executable]) + + def test_windows_shell_selection_ignores_directory_names_and_partial_name_matches(self): + for executable in ( + r"C:\powershell tools\zsh.exe", + r"C:\pwsh tools\fish.exe", + r"C:\cmd tools\other.exe", + r"C:\installed tools\mypwsh.exe", + r"C:\installed tools\customcmd.exe", + ): + with self.subTest(executable=executable), patch.object( + commands, "_aks_bastion_get_current_shell_cmd", return_value=executable + ), patch.object(commands, "which", return_value=executable), patch.object( + commands.sys, "platform", "win32" + ): + shell, command = commands._aks_bastion_prepare_shell_cmd("test-kubeconfig") + self.assertEqual(shell, executable) + self.assertEqual(command, f'"{executable}"') + def test_posix_powershell_command_does_not_expand_environment_in_outer_shell(self): executable = "/installed tools/pwsh" kubeconfig = "/test config/user's config" @@ -197,6 +238,33 @@ def test_bash_without_rcfile_launches_selected_shell_directly(self): self.assertEqual(shell, executable) self.assertEqual(shlex.split(command), [executable]) + def test_windows_bash_does_not_use_posix_rcfile_command(self): + executable = r"C:\Program Files\Git\bin\bash.exe" + with patch.object( + commands, "_aks_bastion_get_current_shell_cmd", return_value=executable + ), patch.object(commands, "which", return_value=executable), patch.object( + commands.os.path, "exists", return_value=True + ), patch.object(commands.sys, "platform", "win32"): + shell, command = commands._aks_bastion_prepare_shell_cmd("test-kubeconfig") + self.assertEqual(shell, executable) + self.assertEqual(command, f'"{executable}"') + + def test_shell_restoration_only_applies_to_bash(self): + for platform, executable, restore in ( + ("linux", "/installed tools/bash", True), + ("linux", "/home/bash/bin/zsh", False), + ("linux", "/installed tools/custombash", False), + ("win32", r"C:\Program Files\Git\bin\bash.exe", False), + ): + with self.subTest(platform=platform, executable=executable), patch.object( + commands.sys, "platform", platform + ), patch.object(commands.subprocess, "run") as run: + commands._aks_bastion_restore_shell(executable) + if restore: + run.assert_called_once_with(["stty", "sane"], stdin=commands.sys.stdin) + else: + run.assert_not_called() + def test_missing_shell_fails_instead_of_executing_bare_name(self): with patch.object(commands, "_aks_bastion_get_current_shell_cmd", return_value="pwsh"), patch.object( commands, "which", return_value=None