[AIRFLOW-5303] Use project_id from GCP credentials #5907

mik-laj · 2019-08-24T11:16:31Z

Currently project_id is read correctly only if project_id is configured in extra.

However, the default project_id can also be configured in other place.:

Application Default Credentials - It not only provides credentials, but also tries to find a project id. More: https://cloud.google.com/docs/authentication/production
service account - It may contain information about the project id

It makes much easier to configure the connection during development
Currently, I need to setup connection using following command:

export AIRFLOW_CONN_google_cloud_default='google-cloud-platform://?extra__google_cloud_platform__key_path=%2Fkeys%2Fsa.json&extra__google_cloud_platform__project=polidea-airflow'

but I would like to be able to configure the connection using the following command:

gcloud auth application-default login

or when I use a service account:

export GOOGLE_APPLICATION_CREDENTIALS=/keys/sa.json

CC: @ryanyuan

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-5303
- In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
- In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
- In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

Passes flake8

mik-laj · 2019-08-25T10:25:42Z

airflow/contrib/hooks/gcp_api_base_hook.py

I wonder if this piece of code should not be transferred to the _get_credentials_and_project_id method. Then this method will never return an invalid value and it will be easier to use it.

kaxil

LGTM but would be good if we can add the test case mentioned in last comment

kaxil · 2019-08-29T13:28:35Z

airflow/contrib/hooks/gcp_api_base_hook.py

Can we add a test case for this?

(1) Mock project_id as None from credential object & being overriden by project_id from connection?

I added a 8 tests. I am testing a lot of different cases now.

kaxil · 2019-08-29T21:45:19Z

tests/contrib/hooks/test_gcp_api_base_hook.py

+
+    @mock.patch(  # type: ignore
+        MODULE_NAME + '.google.oauth2.service_account.Credentials.from_service_account_file',
+        **{'reutnr_value.project_id': "PROJECT_ID"}


Suggested change

**{'reutnr_value.project_id': "PROJECT_ID"}

**{'return_value.project_id': "PROJECT_ID"}

Fixed. I also added missing assertion.

…n methods

kaxil · 2019-08-29T22:10:36Z

tests/contrib/hooks/test_gcp_api_base_hook.py

+        }
+        result = self.instance._get_credentials_and_project_id()
+        mock_from_service_account_file.assert_called_once_with(service_account, scopes=self.instance.scopes)
+        self.assertEqual((mock_from_service_account_file.return_value, 'PROJECT_ID'), result)


(mock_from_service_account_file.return_value, 'PROJECT_ID') -> The first item is credential block?

Yes. This is a credentials object because the from_service_account_info method is the Credentials class factory method. It's allways return Credentials object. This PR does not change the behavior of it.

All tests pass locally, but let's wait for Travis.

mik-laj added the provider:google Google (including GCP) related issues label Aug 24, 2019

mik-laj force-pushed the AIRFLOW-5303 branch 2 times, most recently from 2466d16 to abddb0b Compare August 24, 2019 16:38

mik-laj changed the title ~~[AIRFLOW-5303] Return project_id for other authorization methods~~ [AIRFLOW-5303][WIP] Return project_id for other authorization methods Aug 24, 2019

mik-laj requested review from kaxil and potiuk August 24, 2019 20:15

mik-laj changed the title ~~[AIRFLOW-5303][WIP] Return project_id for other authorization methods~~ [AIRFLOW-5303] Return project_id for other authorization methods Aug 24, 2019

mik-laj commented Aug 25, 2019

View reviewed changes

mik-laj requested a review from jmcarp August 25, 2019 10:41

mik-laj force-pushed the AIRFLOW-5303 branch from 991c4af to 7e449f2 Compare August 25, 2019 18:34

mik-laj changed the title ~~[AIRFLOW-5303] Return project_id for other authorization methods~~ [AIRFLOW-5303] Use project_id from other authorization methods Aug 25, 2019

mik-laj changed the title ~~[AIRFLOW-5303] Use project_id from other authorization methods~~ [AIRFLOW-5303] Use project_id from gcp credentials Aug 25, 2019

mik-laj changed the title ~~[AIRFLOW-5303] Use project_id from gcp credentials~~ [AIRFLOW-5303] Use project_id from GCP credentials Aug 25, 2019

mik-laj mentioned this pull request Aug 27, 2019

[AIRFLOW-5303] Use project_id from GCP credentials PolideaInternal/airflow#253

Closed

mik-laj force-pushed the AIRFLOW-5303 branch from 7e449f2 to dec289f Compare August 28, 2019 18:38

kaxil approved these changes Aug 29, 2019

View reviewed changes

Kamil Breguła added 2 commits August 29, 2019 23:27

[AIRFLOW-5303] Return project_id for other authorization methods

1ff1adb

fixup! [AIRFLOW-5303] Return project_id for other authorization methods

c9420d9

mik-laj force-pushed the AIRFLOW-5303 branch from 45064b9 to c9420d9 Compare August 29, 2019 21:27

kaxil reviewed Aug 29, 2019

View reviewed changes

fixup! fixup! [AIRFLOW-5303] Return project_id for other authorizatio…

822cc6f

…n methods

kaxil reviewed Aug 29, 2019

View reviewed changes

mik-laj merged commit 7c0d9b4 into apache:master Aug 29, 2019

turbaszek mentioned this pull request Aug 30, 2019

Add system tests for DataFlow PolideaInternal/airflow#224

Closed

Jerryguo pushed a commit to Jerryguo/airflow that referenced this pull request Sep 2, 2019

[AIRFLOW-5303] Use project_id from GCP credentials (apache#5907)

d40f2e6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[AIRFLOW-5303] Use project_id from GCP credentials #5907

[AIRFLOW-5303] Use project_id from GCP credentials #5907

Uh oh!

mik-laj commented Aug 24, 2019 •

edited

Loading

Uh oh!

mik-laj Aug 25, 2019

Uh oh!

mik-laj Aug 25, 2019

Uh oh!

kaxil left a comment

Uh oh!

kaxil Aug 29, 2019

Uh oh!

mik-laj Aug 29, 2019

Uh oh!

kaxil Aug 29, 2019

Uh oh!

kaxil Aug 29, 2019

Uh oh!

mik-laj Aug 29, 2019

Uh oh!

kaxil Aug 29, 2019

Uh oh!

kaxil Aug 29, 2019

Uh oh!

mik-laj Aug 29, 2019 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	**{'reutnr_value.project_id': "PROJECT_ID"}
	**{'return_value.project_id': "PROJECT_ID"}

[AIRFLOW-5303] Use project_id from GCP credentials #5907

[AIRFLOW-5303] Use project_id from GCP credentials #5907

Uh oh!

Conversation

mik-laj commented Aug 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jira

Description

Tests

Commits

Documentation

Code Quality

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaxil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mik-laj Aug 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mik-laj commented Aug 24, 2019 •

edited

Loading

mik-laj Aug 29, 2019 •

edited

Loading