more explicit secrets path error messages (#55015)

[FoxHelms]

---

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

[BasPH]

For reference: #55015

[BasPH]

Hi @FoxHelms, thanks for opening the PR! #55015 states that the error message isn't meaningful when the variable/connection path exists, but the required keys "value" or "conn_uri" inside the Vault secret aren't found.

Those checks happen here:

• airflow/providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py

  Line 228 in 7a930fd

  return response.get("value") if response else None

• airflow/providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py

  Line 247 in 7a930fd

  return response.get("value") if response else None

• airflow/providers/hashicorp/src/airflow/providers/hashicorp/secrets/vault.py

  Line 205 in 7a930fd

  uri = response.get("conn_uri")

Currently, Airflow simply returns None (for variable/config) or an empty Connection, and doesn't tell the user those keys weren't found. It ends up throwing this rather generic error.

I think Airfow should be more explicit here: could you log a more meaningful error in the Vault provider? I'm thinking something like this (untested)?

if not response:
    return None

try:
    return response["value"]
except KeyError:
    self.log.warning('Vault secret %s fetched but does not have required key "value"', key)
    return None

[FoxHelms]

Got it @BasPH, thanks for the feedback! I implemented your suggestion. I've tried to also test that the logger shows the correct warning, but I've been having trouble mocking it because its a BoundLoggerLazyProxy. Is this test sufficient?

[BasPH]

Tested this exact scenario on a live Vault, looking good:

Will check the unit test next

[BasPH]

@FoxHelms Can we replicate this check for fetching connections (which requires key "conn_uri") and config too?

[FoxHelms]

@BasPH I just updated get_config and get_connection. I'm not sure how to test get_connection because the function always returns a connection, and get_uri always returns a string. Maybe that unit test is unnecessary and displaying the warning message is sufficient?

[Lee-W]

Looks like there're some compatibility issues we need to resolve due to the log import

[FoxHelms]

Fixed!

[pierrejeambrun]

@BasPH The PR was updated, do you mind giving this another review to reconsider your request for change. :)

[BasPH]

Thanks for the persistence on this @FoxHelms!