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

[AIRFLOW-2826] Add GoogleCloudKMSHook #3677

Merged
merged 1 commit into from Aug 8, 2018

Conversation

Projects
None yet
4 participants
@jakahn
Contributor

jakahn commented Aug 1, 2018

Jira

Description

  • Here are some details about my PR (no UI changes):
    • Adds a hook enabling encryption and decryption through Google Cloud KMS. Both operations also support the use of the "additionalAuthenticatedData" field for the requests.
    • This hook is also added in anticipation of integration with future work on AIRFLOW-2062.

Tests

  • My PR adds the following unit tests:
    • tests/contrib/hooks/test_gcp_kms_hook.py

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.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Aug 1, 2018

Codecov Report

Merging #3677 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3677   +/-   ##
=======================================
  Coverage   77.57%   77.57%           
=======================================
  Files         205      205           
  Lines       15771    15771           
=======================================
  Hits        12234    12234           
  Misses       3537     3537

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97bc70b...a4b7753. Read the comment docs.

codecov-io commented Aug 1, 2018

Codecov Report

Merging #3677 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3677   +/-   ##
=======================================
  Coverage   77.57%   77.57%           
=======================================
  Files         205      205           
  Lines       15771    15771           
=======================================
  Hits        12234    12234           
  Misses       3537     3537

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97bc70b...a4b7753. Read the comment docs.

Show outdated Hide outdated airflow/contrib/hooks/gcp_kms_hook.py
return build(
'cloudkms', 'v1', http=http_authorized, cache_discovery=False)
def encrypt(self, key_name, plaintext, authenticated_data=None):

This comment has been minimized.

@wwlian

wwlian Aug 2, 2018

Contributor

This method doesn't accept arbitrary byte strings, which seems like an important use case for KMS. I think it would be better if the hook were byte/bytearray-oriented. We can offer convenience functions that use the byte-oriented encrypt/decrypt functions that do string decoding/encoding.

Please also add tests that the hook can encrypt/decrypt non-ASCII byte strings.

@wwlian

wwlian Aug 2, 2018

Contributor

This method doesn't accept arbitrary byte strings, which seems like an important use case for KMS. I think it would be better if the hook were byte/bytearray-oriented. We can offer convenience functions that use the byte-oriented encrypt/decrypt functions that do string decoding/encoding.

Please also add tests that the hook can encrypt/decrypt non-ASCII byte strings.

This comment has been minimized.

@jakahn

jakahn Aug 2, 2018

Contributor

Done

@jakahn

jakahn Aug 2, 2018

Contributor

Done

@wwlian

wwlian approved these changes Aug 3, 2018

Show outdated Hide outdated airflow/contrib/hooks/gcp_kms_hook.py
request = keys.decrypt(name=key_name, body=body)
response = request.execute()
plaintext = _b64decode(response['plaintext'])

This comment has been minimized.

@wwlian

wwlian Aug 3, 2018

Contributor

Please run a quick manual integration test in python3 to verify that response['plaintext'] is indeed a str rather than a bytes object which won't have an encode method.

@wwlian

wwlian Aug 3, 2018

Contributor

Please run a quick manual integration test in python3 to verify that response['plaintext'] is indeed a str rather than a bytes object which won't have an encode method.

This comment has been minimized.

@jakahn

jakahn Aug 3, 2018

Contributor

I have, all calls to the API result in base64 encoded strings

@jakahn

jakahn Aug 3, 2018

Contributor

I have, all calls to the API result in base64 encoded strings

[AIRFLOW-2826] Add GoogleCloudKMSHook
Adds a hook enabling encryption and decryption through Google Cloud KMS.
In the future, this should also contribute to AIRFLOW-2062.
@jakahn

This comment has been minimized.

Show comment
Hide comment
@jakahn

jakahn Aug 7, 2018

Contributor

@Fokko PTAL when you can

Contributor

jakahn commented Aug 7, 2018

@Fokko PTAL when you can

@Fokko

Fokko approved these changes Aug 8, 2018

@jakahn Looks good, thanks!

@Fokko Fokko merged commit acca61c into apache:master Aug 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

lxneng added a commit to lxneng/incubator-airflow that referenced this pull request Aug 10, 2018

[AIRFLOW-2826] Add GoogleCloudKMSHook (#3677)
Adds a hook enabling encryption and decryption through Google Cloud KMS.
This should also contribute to AIRFLOW-2062.

dlebech added a commit to trustpilot/incubator-airflow that referenced this pull request Sep 11, 2018

[AIRFLOW-2826] Add GoogleCloudKMSHook (#3677)
Adds a hook enabling encryption and decryption through Google Cloud KMS.
This should also contribute to AIRFLOW-2062.

dlebech added a commit to trustpilot/incubator-airflow that referenced this pull request Sep 11, 2018

dalupus added a commit to modmed/incubator-airflow that referenced this pull request Sep 19, 2018

[AIRFLOW-2826] Add GoogleCloudKMSHook (#3677)
Adds a hook enabling encryption and decryption through Google Cloud KMS.
This should also contribute to AIRFLOW-2062.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment