Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GCP Secret Manager Error Handling for Missings Credentials #17264

Merged
merged 1 commit into from
Jul 29, 2021

Conversation

fhoda
Copy link
Contributor

@fhoda fhoda commented Jul 27, 2021

related: #16404

Split of the above referenced PR (secrets backend failover). See #16404 (comment)

This one just has the GCP Secret Manager error handling.

@fhoda fhoda requested a review from turbaszek as a code owner July 27, 2021 16:07
@boring-cyborg boring-cyborg bot added area:providers area:secrets provider:google Google (including GCP) related issues labels Jul 27, 2021
Comment on lines +115 to +117
'Unable to load credentials for GCP Secret Manager. '
'Make sure that the keyfile path, dictionary, or GOOGLE_APPLICATION_CREDENTIALS '
'environment variable is correct and properly configured.'
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it’d be a good idea to list the paths and check if GOOGLE_APPLICATION_CREDENTIALS is set (probably not a good idea to show its value). That’d make debugging much easier.

Copy link
Member

Choose a reason for hiding this comment

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

The log.exception will probably show that error as that is raised from/by get_credentials_and_project_id I think

def get_credentials_and_project(self) -> Tuple[google.auth.credentials.Credentials, str]:

self._log_info(
'Getting connection using `google.auth.default()` since no key file is defined for hook.'

if not self.key_path.endswith('.json'):
raise AirflowException('Unrecognised extension for key file.')

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but I was thinking it’d use useful to know what the value of self.key_path is in the above context, so I don’t need to hunt the value in my configs. But maybe google-auth already includes that in the exception, no idea.

@kaxil kaxil closed this Jul 28, 2021
@kaxil kaxil reopened this Jul 28, 2021
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Jul 28, 2021
@github-actions
Copy link

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@kaxil kaxil closed this Jul 29, 2021
@kaxil kaxil reopened this Jul 29, 2021
@kaxil kaxil merged commit c384f9b into apache:main Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers area:secrets okay to merge It's ok to merge this PR as it does not require more tests provider:google Google (including GCP) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants