Skip to content

Conversation

@dheerajturaga
Copy link
Member

When using an encrypted keyring with airflowctl, entering an incorrect
keyring password caused an unhandled ValueError exception. This adds
proper exception handling in Credentials.load() to provide user-friendly
error messages with actionable steps for recovery. AUTH commands now
continue gracefully while CLI commands display helpful guidance.

Please enter password for encrypted keyring:
Please enter password for encrypted keyring:
Traceback (most recent call last):
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file.py", line 186, in _unlock
    ref_pw = self.get_password('keyring-setting', 'password reference')
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file_base.py", line 106, in get_password
    password = self.decrypt(password_encrypted, assoc).decode('utf-8')
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file.py", line 218, in decrypt
    assert plaintext.startswith(self.pw_prefix)
AssertionError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file_base.py", line 106, in get_password
    password = self.decrypt(password_encrypted, assoc).decode('utf-8')
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file.py", line 216, in decrypt
    cipher = self._create_cipher(self.keyring_key, data['salt'], data['IV'])
  File "/usr/python/lib/python3.10/site-packages/jaraco/classes/properties.py", line 76, in __get__
    return self.fget(obj)
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file.py", line 99, in keyring_key
    self._unlock()
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file.py", line 190, in _unlock
    raise ValueError("Incorrect Password")
ValueError: Incorrect Password

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file.py", line 186, in _unlock
    ref_pw = self.get_password('keyring-setting', 'password reference')
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file_base.py", line 106, in get_password
    password = self.decrypt(password_encrypted, assoc).decode('utf-8')
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file.py", line 218, in decrypt
    assert plaintext.startswith(self.pw_prefix)
AssertionError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/python/bin/airflowctl", line 10, in <module>
    sys.exit(main())
  File "/opt/airflow/airflow-ctl/src/airflowctl/__main__.py", line 34, in main
    safe_call_command(args.func, args=args)
  File "/opt/airflow/airflow-ctl/src/airflowctl/ctl/cli_config.py", line 76, in safe_call_command
    function(args)
  File "/opt/airflow/airflow-ctl/src/airflowctl/ctl/cli_config.py", line 59, in command
    return func(*args, **kwargs)
  File "/opt/airflow/airflow-ctl/src/airflowctl/api/client.py", line 355, in wrapper
    with get_client(kind=kind) as api_client:
  File "/usr/python/lib/python3.10/contextlib.py", line 135, in __enter__
    return next(self.gen)
  File "/opt/airflow/airflow-ctl/src/airflowctl/api/client.py", line 323, in get_client
    credentials = Credentials(client_kind=kind).load()
  File "/opt/airflow/airflow-ctl/src/airflowctl/api/client.py", line 172, in load
    self.api_token = keyring.get_password("airflowctl", f"api_token_{self.api_environment}")
  File "/usr/python/lib/python3.10/site-packages/keyring/core.py", line 63, in get_password
    return get_keyring().get_password(service_name, username)
  File "/usr/python/lib/python3.10/site-packages/keyring/backends/chainer.py", line 49, in get_password
    password = keyring.get_password(service, username)
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file_base.py", line 109, in get_password
    password = self.decrypt(password_encrypted).decode('utf-8')
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file.py", line 216, in decrypt
    cipher = self._create_cipher(self.keyring_key, data['salt'], data['IV'])
  File "/usr/python/lib/python3.10/site-packages/jaraco/classes/properties.py", line 76, in __get__
    return self.fget(obj)
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file.py", line 99, in keyring_key
    self._unlock()
  File "/usr/python/lib/python3.10/site-packages/keyrings/alt/file.py", line 190, in _unlock
    raise ValueError("Incorrect Password")
ValueError: Incorrect Password

  When using an encrypted keyring with airflowctl, entering an incorrect
  keyring password caused an unhandled ValueError exception. This adds
  proper exception handling in Credentials.load() to provide user-friendly
  error messages with actionable steps for recovery. AUTH commands now
  continue gracefully while CLI commands display helpful guidance.
f" 2. Or use AIRFLOW_CLI_DEBUG_MODE=true to bypass keyring\n"
f" 3. Or run 'airflowctl auth login' to re-authenticate"
)
raise AirflowCtlCredentialNotFoundException(error_msg) from e
Copy link
Member

Choose a reason for hiding this comment

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

Looks good but one nit.

Should we just print the erorr aand sys.exit(42) here ? cc: @bugraoz93

I see that we are already handling it elsewhere:

    try:
        function(args)
    except (
        AirflowCtlCredentialNotFoundException,
        AirflowCtlConnectionException,
        AirflowCtlNotFoundException,
    ) as e:
        rich.print(f"command failed due to {e}")
        sys.exit(1)

I think in case of a fatal error in CLIs, I find the default exception handling as verbose and unnecessary - so my usual approach for fatal CLI errors is to print as descriptive and actionable error message as possible, with possibly even more details in debug mode, and .... exit with error.

Otherwise - at least by default - your helpful error message will be overwhelmed by the stack trace printed by default - which usually in CLIs is very misleading, unhelpful for the users, confusing and intimidating.

So I would generally handle that in one of the two ways:

  1. print error message + sys.exit()
  2. raise a deliberate "ExitKindOfException" with appropriate error message, and handle it in one place with printing - but only printing THE MESSAGE - not the exception itself.

I found such sys.exit() a bit better because you can decide how to print he message, you can format and colour it apropriately ("actual error in red, warning - i.e. somtething to check - in yellow, instructions to follow in white"), this is pretty much approach we folow in breeze and it works nicely even for technical users of breeze, who could be - in principle - interested in stack trace and details, but in fact they really want to understand what error it is and how they can fix it.

Approach 2 is also good (you can have some implicit dependency on rich and format your message when you prepare it with rich directives) - also you can use some conventions to handle it differently in case of verbose output.

WDYT @bugraoz93 ?

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM with one comment about communicating errors to the users which requires @bugraoz93 input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants