From c16f3a4fed5e20c0ed8dac64ef1fa732feb171eb Mon Sep 17 00:00:00 2001 From: Eason09053360 <185830721+Eason09053360@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:40:54 +0800 Subject: [PATCH] Fix airflowctl tracebacks on an unusable credentials file Reading the credentials file guarded only against the file being absent, so anything else about it that stopped a command - wrong permissions, non-UTF-8 bytes, truncated JSON, the wrong shape - reached the operator as a Python stack trace. `$AIRFLOW_HOME/production.json` is a generic enough name in a shared directory that a foreign or half-written file is not far-fetched, and none of those errors were something the CLI's own handler recognised. The tolerance that lets `auth login` run without a credentials file now covers an unusable one too, so the recovery the message recommends is actually available. --- airflow-ctl/src/airflowctl/api/client.py | 115 +++++++++++------- airflow-ctl/src/airflowctl/ctl/cli_config.py | 15 +-- airflow-ctl/src/airflowctl/exceptions.py | 4 + .../tests/airflow_ctl/api/test_client.py | 37 ++++++ .../tests/airflow_ctl/ctl/test_cli_config.py | 10 +- 5 files changed, 124 insertions(+), 57 deletions(-) diff --git a/airflow-ctl/src/airflowctl/api/client.py b/airflow-ctl/src/airflowctl/api/client.py index e0c9c60f0eee6..3b1003d50fbe9 100644 --- a/airflow-ctl/src/airflowctl/api/client.py +++ b/airflow-ctl/src/airflowctl/api/client.py @@ -64,6 +64,7 @@ XComOperations, ) from airflowctl.exceptions import ( + AirflowCtlCredentialInvalidException, AirflowCtlCredentialNotFoundException, AirflowCtlException, AirflowCtlKeyringException, @@ -160,6 +161,26 @@ def _safe_path_under_airflow_home(airflow_home: str, filename: str) -> str: return str(target) +def _read_cli_config(path: str) -> dict: + """Read a credentials file, so an unusable one reaches the user as a message and not a traceback.""" + try: + with open(path, encoding="utf-8") as f: + config = json.load(f) + except FileNotFoundError: + # An absent file is a distinct case the callers already handle. + raise + except (OSError, ValueError) as e: + # OSError covers unreadable paths; ValueError covers both malformed JSON and non-UTF-8 bytes. + raise AirflowCtlCredentialInvalidException( + f"Credentials file {path} could not be read: {e}. Log in again to recreate it." + ) from e + if not isinstance(config, dict): + raise AirflowCtlCredentialInvalidException( + f"Credentials file {path} is not a JSON object. Log in again to recreate it." + ) + return config + + # Credentials for the API class Credentials: """Credentials for the API.""" @@ -253,56 +274,62 @@ def load(self) -> Credentials: default_config_dir = os.environ.get("AIRFLOW_HOME", os.path.expanduser("~/airflow")) config_path = _safe_path_under_airflow_home(default_config_dir, self.input_cli_config_file) try: - with open(config_path) as f: - credentials = json.load(f) - self.api_url = credentials["api_url"] - if self.client_kind == ClientKind.NO_AUTH: - return self - if self.api_token is not None: - return self - if os.getenv("AIRFLOW_CLI_DEBUG_MODE") == "true": - debug_creds_path = _safe_path_under_airflow_home( - default_config_dir, f"debug_creds_{self.input_cli_config_file}" + credentials = _read_cli_config(config_path) + if "api_url" not in credentials: + raise AirflowCtlCredentialInvalidException( + f"Credentials file {config_path} does not contain an api_url. " + "Log in again to recreate it." + ) + self.api_url = credentials["api_url"] + if self.client_kind == ClientKind.NO_AUTH: + return self + if self.api_token is not None: + return self + if os.getenv("AIRFLOW_CLI_DEBUG_MODE") == "true": + debug_creds_path = _safe_path_under_airflow_home( + default_config_dir, f"debug_creds_{self.input_cli_config_file}" + ) + try: + debug_credentials = _read_cli_config(debug_creds_path) + self.api_token = debug_credentials.get( + self.token_key_for_environment(self.api_environment) + ) + except FileNotFoundError as e: + if self.client_kind == ClientKind.CLI: + raise AirflowCtlCredentialNotFoundException( + f"Debug credentials file not found: {debug_creds_path}. " + "Set AIRFLOW_CLI_DEBUG_MODE=false or log in with debug mode enabled first." + ) from e + self.api_token = None + else: + try: + self.api_token = keyring.get_password( + "airflowctl", self.token_key_for_environment(self.api_environment) ) - try: - with open(debug_creds_path) as df: - debug_credentials = json.load(df) - self.api_token = debug_credentials.get( - self.token_key_for_environment(self.api_environment) - ) - except FileNotFoundError as e: - if self.client_kind == ClientKind.CLI: - raise AirflowCtlCredentialNotFoundException( - f"Debug credentials file not found: {debug_creds_path}. " - "Set AIRFLOW_CLI_DEBUG_MODE=false or log in with debug mode enabled first." - ) from e - self.api_token = None - else: - try: - self.api_token = keyring.get_password( - "airflowctl", self.token_key_for_environment(self.api_environment) - ) - except ValueError as e: - # Incorrect keyring password - log.warning( - "Could not access keyring for environment %s: %s", self.api_environment, e - ) - if self.client_kind == ClientKind.CLI: - raise AirflowCtlKeyringException( - f"Incorrect keyring password for environment {self.api_environment}" - ) from e - self.api_token = None - except NoKeyringError as e: - # No keyring backend available - log.error("No keyring backend available: %s", e) - if self.client_kind == ClientKind.CLI: - raise AirflowCtlKeyringException("Keyring backend is not available") from e - self.api_token = None + except ValueError as e: + # Incorrect keyring password + log.warning("Could not access keyring for environment %s: %s", self.api_environment, e) + if self.client_kind == ClientKind.CLI: + raise AirflowCtlKeyringException( + f"Incorrect keyring password for environment {self.api_environment}" + ) from e + self.api_token = None + except NoKeyringError as e: + # No keyring backend available + log.error("No keyring backend available: %s", e) + if self.client_kind == ClientKind.CLI: + raise AirflowCtlKeyringException("Keyring backend is not available") from e + self.api_token = None except FileNotFoundError: # This is expected during the auth login command. # Also allow token-only usage without local config (for commands like `version --remote`). if self.client_kind not in (ClientKind.AUTH, ClientKind.NO_AUTH) and self.api_token is None: raise AirflowCtlCredentialNotFoundException("No credentials file found. Please login first.") + except AirflowCtlCredentialInvalidException: + # Same tolerance as a missing file: `auth login` has to be able to replace an unusable one, + # and it is the recovery the message recommends. + if self.client_kind not in (ClientKind.AUTH, ClientKind.NO_AUTH): + raise return self diff --git a/airflow-ctl/src/airflowctl/ctl/cli_config.py b/airflow-ctl/src/airflowctl/ctl/cli_config.py index 4a385537eb105..e9cba8bdaae32 100755 --- a/airflow-ctl/src/airflowctl/ctl/cli_config.py +++ b/airflow-ctl/src/airflowctl/ctl/cli_config.py @@ -42,12 +42,7 @@ from airflowctl.api.operations import BaseOperations, ServerResponseError from airflowctl.ctl.console_formatting import AirflowConsole from airflowctl.ctl.utils.yaml import safe_load -from airflowctl.exceptions import ( - AirflowCtlConnectionException, - AirflowCtlCredentialNotFoundException, - AirflowCtlKeyringException, - AirflowCtlNotFoundException, -) +from airflowctl.exceptions import AirflowCtlException from airflowctl.utils.module_loading import import_string BUILD_DOCS = "BUILDING_AIRFLOW_DOCS" in os.environ @@ -76,12 +71,8 @@ def safe_call_command(function: Callable, args: Iterable[Arg]) -> None: try: function(args) - except ( - AirflowCtlCredentialNotFoundException, - AirflowCtlConnectionException, - AirflowCtlKeyringException, - AirflowCtlNotFoundException, - ) as e: + except AirflowCtlException as e: + # The base class, so a new AirflowCtl error never regresses to a raw traceback. rich.print(f"command failed due to {e}") sys.exit(1) except (httpx.RemoteProtocolError, httpx.ReadError) as e: diff --git a/airflow-ctl/src/airflowctl/exceptions.py b/airflow-ctl/src/airflowctl/exceptions.py index 0af8b9fa593cc..dc52d09ca5211 100644 --- a/airflow-ctl/src/airflowctl/exceptions.py +++ b/airflow-ctl/src/airflowctl/exceptions.py @@ -38,6 +38,10 @@ class AirflowCtlCredentialNotFoundException(AirflowCtlNotFoundException): """Raise when a credential couldn't be found while performing an operation.""" +class AirflowCtlCredentialInvalidException(AirflowCtlException): + """Raise when a credentials file exists but cannot be used.""" + + class AirflowCtlConnectionException(AirflowCtlException): """Raise when a connection error occurs while performing an operation.""" diff --git a/airflow-ctl/tests/airflow_ctl/api/test_client.py b/airflow-ctl/tests/airflow_ctl/api/test_client.py index b7e500738511c..df98b4897baf0 100644 --- a/airflow-ctl/tests/airflow_ctl/api/test_client.py +++ b/airflow-ctl/tests/airflow_ctl/api/test_client.py @@ -40,6 +40,7 @@ ) from airflowctl.api.operations import ServerResponseError from airflowctl.exceptions import ( + AirflowCtlCredentialInvalidException, AirflowCtlCredentialNotFoundException, AirflowCtlException, AirflowCtlKeyringException, @@ -343,6 +344,42 @@ def test_load_no_keyring_backend_token_provided(self, mock_keyring): credentials = Credentials(client_kind=cli_client, api_token="TEST_TOKEN").load() assert credentials.api_token == "TEST_TOKEN" + @pytest.mark.parametrize( + ("contents", "mode", "expected"), + [ + pytest.param("{not json", None, "could not be read", id="corrupt"), + pytest.param("", None, "could not be read", id="empty"), + pytest.param(b"\xff\xfe", None, "could not be read", id="not-utf8"), + pytest.param('{"api_url": "http://x"}', 0o000, "could not be read", id="unreadable"), + pytest.param(json.dumps(["a"]), None, "is not a JSON object", id="not-an-object"), + pytest.param(json.dumps({"other": 1}), None, "does not contain an api_url", id="no-api-url"), + ], + ) + def test_load_unusable_credentials_file_reports_an_error( + self, monkeypatch, tmp_path, contents, mode, expected + ): + """A file airflowctl cannot use must reach the user as a message, not a traceback.""" + monkeypatch.setenv("AIRFLOW_HOME", str(tmp_path)) + monkeypatch.setenv("AIRFLOW_CLI_ENVIRONMENT", "TEST_BAD_CONFIG") + config = tmp_path / "TEST_BAD_CONFIG.json" + config.write_bytes(contents) if isinstance(contents, bytes) else config.write_text( + contents, encoding="utf-8" + ) + if mode is not None: + config.chmod(mode) + + with pytest.raises(AirflowCtlCredentialInvalidException, match=expected): + Credentials(client_kind=ClientKind.CLI).load() + + @pytest.mark.parametrize("client_kind", [ClientKind.AUTH, ClientKind.NO_AUTH]) + def test_load_tolerates_an_unusable_file_for_auth_kinds(self, monkeypatch, tmp_path, client_kind): + """`auth login` is the recovery the error recommends, so it must survive an unusable file.""" + monkeypatch.setenv("AIRFLOW_HOME", str(tmp_path)) + monkeypatch.setenv("AIRFLOW_CLI_ENVIRONMENT", "TEST_BAD_CONFIG") + (tmp_path / "TEST_BAD_CONFIG.json").write_text("{not json", encoding="utf-8") + + assert Credentials(client_kind=client_kind).load().api_url is None + class TestBoundedGetNewPassword: @patch("airflowctl.api.client.getpass.getpass") diff --git a/airflow-ctl/tests/airflow_ctl/ctl/test_cli_config.py b/airflow-ctl/tests/airflow_ctl/ctl/test_cli_config.py index 8eed0b8108ff4..58206e3512f7a 100644 --- a/airflow-ctl/tests/airflow_ctl/ctl/test_cli_config.py +++ b/airflow-ctl/tests/airflow_ctl/ctl/test_cli_config.py @@ -41,6 +41,7 @@ ) from airflowctl.exceptions import ( AirflowCtlConnectionException, + AirflowCtlCredentialInvalidException, AirflowCtlCredentialNotFoundException, AirflowCtlKeyringException, AirflowCtlNotFoundException, @@ -470,11 +471,18 @@ class TestCliConfigMethods: "raised_exception", [ AirflowCtlCredentialNotFoundException("missing credentials"), + AirflowCtlCredentialInvalidException("unusable credentials"), AirflowCtlConnectionException("connection failed"), AirflowCtlKeyringException("keyring failure"), AirflowCtlNotFoundException("resource not found"), ], - ids=["credential-not-found", "connection-error", "keyring-error", "not-found"], + ids=[ + "credential-not-found", + "credential-invalid", + "connection-error", + "keyring-error", + "not-found", + ], ) def test_safe_call_command_exits_non_zero_for_airflowctl_exceptions(self, raised_exception): def raise_error(_args):