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.

Sign up for GitHub

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

Jump to bottom

Kms key encryption for seeds #193

Closed

b0lle wants to merge 7 commits into dbt-labs:main from b0lle:kms-key-encryption-for-seeds

b0lle commented May 25, 2022

resolves #191

Description

Support kms_key_name feature for seed data.

The integration test adds a kms, which can not be deleted. The testing service account needs the role "Cloud KMS Admin" to create the key. The hard part of the PR is the integration test. Please let me know, if this fits your quality requirements.

Checklist

I have signed the CLA
I have run this code in development and it appears to resolve the stated issue
This PR includes tests, or tests are not required/relevant for this PR
I have updated the CHANGELOG.md and added information about my change to the "dbt-bigquery next" section.

b0lle force-pushed the kms-key-encryption-for-seeds branch 2 times, most recently from 32c321a to e4ba036 Compare

May 25, 2022 20:21

cla-bot bot added the cla:yes label

dbt-labs deleted a comment from cla-bot bot

dbt-labs deleted a comment from cla-bot bot

dbt-labs deleted a comment from cla-bot bot

dbt-labs deleted a comment from cla-bot bot

b0lle mentioned this pull request

support kms_key_name feature for snapshots and seeds dbt-labs/docs.getdbt.com#1482

Closed

1 task

cla-bot bot commented May 31, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Julian.Frenzel.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

cla-bot bot removed the cla:yes label

b0lle force-pushed the kms-key-encryption-for-seeds branch from d45b22f to 59aaa46 Compare

May 31, 2022 15:02

cla-bot bot commented May 31, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Julian.Frenzel.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

b0lle force-pushed the kms-key-encryption-for-seeds branch from 59aaa46 to 949b946 Compare

May 31, 2022 15:07

cla-bot bot commented May 31, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Julian.Frenzel.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

b0lle force-pushed the kms-key-encryption-for-seeds branch from 949b946 to 7ecc17c Compare

May 31, 2022 15:10

cla-bot bot commented May 31, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Julian.Frenzel.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

b0lle force-pushed the kms-key-encryption-for-seeds branch from 7ecc17c to e8c73d0 Compare

May 31, 2022 15:21

cla-bot bot commented May 31, 2022

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Julian.Frenzel.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

b0lle force-pushed the kms-key-encryption-for-seeds branch from e8c73d0 to 92d6a57 Compare

May 31, 2022 15:25

cla-bot bot added the cla:yes label

McKnight-42 requested review from McKnight-42 and jtcohen6

May 31, 2022 16:27

b0lle and others added 2 commits

June 8, 2022 11:00


          support kms_key_name feature for seeds

f41b220


          pre commit hooks (black formatting)

6c79aa5

b0lle force-pushed the kms-key-encryption-for-seeds branch from 92d6a57 to 6c79aa5 Compare

June 8, 2022 09:00

jtcohen6 added the ready_for_review label

McKnight-42 added 4 commits

July 22, 2022 10:23


          Merge branch 'main' into kms-key-encryption-for-seeds

2c2ddaa


          Merge branch 'main' into kms-key-encryption-for-seeds

7f63fc4


          Merge branch 'main' into kms-key-encryption-for-seeds

aae75f8


          Merge branch 'main' into kms-key-encryption-for-seeds

55ee3b6

McKnight-42 requested review from VersusFacit and removed request for McKnight-42

August 15, 2022 15:19


          Merge branch 'main' into kms-key-encryption-for-seeds

1dfff90

Fleid mentioned this pull request

[CT-1937] [PR Review] Kms key encryption for seeds #193 #489

Closed

Fleid added the pr_tracked label

mikealfare reviewed

View reviewed changes

dbt/include/bigquery/macros/materializations/seed.sql

@@ @@ -10,8 +10,9 @@ @@
               {% macro bigquery__load_csv_rows(model, agate_table) %}
                 {%- set column_override = model['config'].get('column_types', {}) -%}
+                {%- set kms_key_name = model['config'].get('kms_key_name', {}) -%}

Contributor

mikealfare Jun 24, 2023

I think the data type of kms_key_name is str. If so, does {} make sense as the default here? Is the expected arg datatype on EncryptionConfiguration str or dict?

mikealfare reviewed

View reviewed changes

tests/integration/simple_seed_test/test_seed_kms_key_name.py

+              from google.cloud import bigquery
+              from google.cloud import kms
+              from tests.integration.base import DBTIntegrationTest, use_profile

Contributor

mikealfare Jun 24, 2023

We've moved away from the tests in tests/integration towards tests/functional. They are testing the same functionality, but use a new framework. One of the changes is that we moved from unittest to pytest. Could you migrate these tests to the new framework? At a cursory glance, I think you'd need to make these changes:

GcpKmsAdapter
- looks like it would move over as-is
TestSimpleSeedKmsKeyName
- you could probably jetison schema and project_id in favor of the update_project_config() fixture
- setUp would need to be a pytest fixture with autouse=True
TestSimpleSeedKmsKeyNameBq
- you'd need to find a new source for biqguery_profile(); I'm not that familiar with it, but I'm sure that's done in other tests in tests/functional

matthewshaver added the user docs label

mikealfare removed the pr_tracked label

Contributor

martynydbt commented Mar 21, 2024

Closing due to lack of engagement.

martynydbt closed this

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

Labels

cla:yes ready_for_review user docs