feat: implement support for clusters with CMEK encryption#855
feat: implement support for clusters with CMEK encryption#855kolea2 merged 2 commits intogoogleapis:masterfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## master #855 +/- ##
========================================
Coverage 99.22% 99.23%
========================================
Files 18 18
Lines 17416 17539 +123
Branches 1025 977 -48
========================================
+ Hits 17281 17404 +123
Misses 132 132
Partials 3 3
Continue to review full report at Codecov.
|
kolea2
left a comment
There was a problem hiding this comment.
Thanks for working on this! For this to be feature complete, I think we will need to add API to both backups and table as well, specifically to get encryption info on them - see the java impl for more details https://github.com/googleapis/java-bigtable/pull/656/files. Let me know if I'm missing anything :)
|
Thanks for the review @kolea2! Generally for anything that is accessible through the response from "getMetadata()", we won't create an additional method. However, we can of course make an exception if that's a requirement. How it works now: const [metadata] = await backup.getMetadata();
console.log(metadata.encryptionInfo); |
craiglabenz
left a comment
There was a problem hiding this comment.
Looks good from a Node perspective.
Thank you! Makes sense to me, thanks for the explanation. Will take a look at the rest. |
| const keyRingsBaseUrl = `https://cloudkms.googleapis.com/v1/projects/${projectId}/locations/us-central1/keyRings`; | ||
| kmsKeyName = `projects/${projectId}/locations/us-central1/keyRings/${keyRingId}/cryptoKeys/${cryptoKeyId}`; | ||
|
|
||
| await bigtable.auth.request({ |
There was a problem hiding this comment.
is this creating a key per test run? If so, I think these will need to be cleaned up as well?
There was a problem hiding this comment.
Good catch, it was. I added a commit that uses the destroy method to schedule the destroy of the key. Please let me know if there's a better or more thorough way to remove these crypto key resources!
🤖 I have created a release \*beep\* \*boop\* --- ## [3.5.0](https://www.github.com/googleapis/nodejs-bigtable/compare/v3.4.0...v3.5.0) (2021-05-05) ### Features * implement support for clusters with CMEK encryption ([#855](https://www.github.com/googleapis/nodejs-bigtable/issues/855)) ([0d5d8e6](https://www.github.com/googleapis/nodejs-bigtable/commit/0d5d8e66bb3ce7947903795a5ea0c74362327ebf)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
edit (kolea2): removing doc
This adds support for CMEK configuration of a Cluster when it is created.
Creating an instance
Creating a cluster
The
keyaliasInstead of providing the
encryptionoption, the user has a choice to use the aliaskey:await bigtable.createInstance('my-instance', { clusters: [ { id: 'my-cluster', - encryption: { - kmsKeyName: 'kms-key-name', - }, + key: 'kms-key-name', // ... other options }, ], }); await instance.createCluster('my-cluster', { - encryption: { - kmsKeyName: 'kms-key-name', - }, + key: 'kms-key-name', // ... other options });If both options are set, an error is thrown.