shahar1 commented on code in PR #76287:
URL: https://github.com/apache/airflow/pull/76287#discussion_r4457114569
##########
airflow-ctl/src/airflowctl/api/client.py:
##########
@@ -241,15 +241,29 @@ def save(self, skip_keyring: bool = False):
Skip saving the token to keyring if skip_keyring is True, in this case,
only the config file with the API URL is created.
+
+ The token already stored in keyring for the environment is removed
first,
+ so a failure later on cannot leave it paired with the new API URL.
"""
default_config_dir = os.environ.get("AIRFLOW_HOME",
os.path.expanduser("~/airflow"))
os.makedirs(default_config_dir, exist_ok=True)
config_path = _safe_path_under_airflow_home(default_config_dir,
self.input_cli_config_file)
+ debug_mode = os.getenv("AIRFLOW_CLI_DEBUG_MODE") == "true"
+ if not debug_mode:
Review Comment:
**minor** — the same stale pairing survives a debug-mode login.
`AIRFLOW_CLI_DEBUG_MODE=true airflowctl auth login --api-url http://new ...`
writes the new URL to `prod.json` and the token to the debug file, and leaves
the keyring untouched. The next command run without debug mode reads the URL
from the file and the token from the keyring, so an earlier login's token goes
to the new URL, which is the bug this PR fixes. The PR body's "Debug mode does
not use the keyring, so it does not change" holds only while debug mode stays
on.
Niche, so fine to leave for a follow-up, but then the docstring above should
say that debug-mode logins keep an earlier keyring token in place. If you want
to close it here: run the removal in debug mode too and tolerate every
`KeyringError` there, since debug mode is typically used where no keyring works.
##########
airflow-ctl/src/airflowctl/api/client.py:
##########
@@ -241,15 +241,29 @@ def save(self, skip_keyring: bool = False):
Skip saving the token to keyring if skip_keyring is True, in this case,
only the config file with the API URL is created.
+
+ The token already stored in keyring for the environment is removed
first,
+ so a failure later on cannot leave it paired with the new API URL.
"""
default_config_dir = os.environ.get("AIRFLOW_HOME",
os.path.expanduser("~/airflow"))
os.makedirs(default_config_dir, exist_ok=True)
config_path = _safe_path_under_airflow_home(default_config_dir,
self.input_cli_config_file)
+ debug_mode = os.getenv("AIRFLOW_CLI_DEBUG_MODE") == "true"
+ if not debug_mode:
+ try:
+ keyring.delete_password("airflowctl",
self.token_key_for_environment(self.api_environment))
+ except (PasswordDeleteError, NoKeyringError):
+ pass
+ except KeyringError as e:
Review Comment:
**minor** — `--skip-keyring` now needs a reachable keyring.
The docs describe the flag as the escape hatch when "there's no keyring
available in the system where airflowctl is running". Before this change the
flag never touched the keyring. Now a backend that is selected but unusable,
for example SecretService raising `KeyringLocked` on a headless session or
macOS Keychain over SSH, makes `auth login --skip-keyring` fail with "The API
URL was not changed", and
`test_save_skip_keyring_keeps_old_url_when_token_cannot_be_removed` pins that.
Only `NoKeyringError` (no backend at all) is tolerated.
In that state the old token cannot be read from the keyring either, so
refusing the login protects little while it blocks the documented workaround.
One option: under `skip_keyring=True`, log a warning for any other
`KeyringError` and still write the URL, since those users must pass
`AIRFLOW_CLI_TOKEN` or `--api-token` anyway and `load()` short-circuits on an
explicit token. If you prefer to keep the strict behaviour, the
`--skip-keyring` docs should say the keyring must still be reachable to remove
an earlier token.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
