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
feat!: customer-managed encryption keys for Spanner #666
Conversation
Warning: This pull request is touching the following templated files:
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
Warning: This pull request is touching the following templated files:
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
Codecov Report
@@ Coverage Diff @@
## master #666 +/- ##
============================================
- Coverage 85.22% 85.09% -0.13%
- Complexity 2650 2702 +52
============================================
Files 145 154 +9
Lines 14358 14555 +197
Branches 1391 1413 +22
============================================
+ Hits 12236 12386 +150
- Misses 1539 1569 +30
- Partials 583 600 +17
Continue to review full report at Codecov.
|
Warning: This pull request is touching the following templated files:
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
Warning: This pull request is touching the following templated files:
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITCmek.java
Outdated
Show resolved
Hide resolved
Warning: This pull request is touching the following templated files:
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small question on whether we should use the wrapper or proto class, otherwise LGTM.
google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java
Outdated
Show resolved
Hide resolved
Warning: This pull request is touching the following templated files:
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
google-cloud-spanner/src/main/java/com/google/cloud/spanner/BackupInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/DatabaseInfo.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/Restore.java
Outdated
Show resolved
Hide resolved
google-cloud-spanner/src/main/java/com/google/cloud/spanner/encryption/EncryptionConfig.java
Outdated
Show resolved
Hide resolved
final Backup.Builder backupBuilder = | ||
com.google.spanner.admin.database.v1.Backup.newBuilder() | ||
.setDatabase(databaseName) | ||
.setExpireTime(backupInfo.getExpireTime().toProto()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is guaranteed that backupInfo.getExpireTime()
will always be set, so this could cause a NullPointerException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be able to create a backup without an expire time:
java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/Backup.java
Lines 64 to 65 in 32fdd60
Preconditions.checkState( | |
getExpireTime() != null, "Cannot create a backup without an expire time"); |
I moved these checks into the database client instead (with illegal argument exception, instead of illegal state exceptions), so that we prevent the creation of a Backup without the necessary parameters.
* @param backupId the id of the backup to create. | ||
* @param backup the backup to create. The database and expireTime fields of the backup must be | ||
* filled. | ||
* @param backupInfo the backup to create. The instance, database and expireTime fields of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should check for these conditions in the builder for BackupInfo
to ensure that they are actually filled. So that would mean that the build()
method of the builder should fail if these fields have not been set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can not do this in the builder itself, because there are times where the backup can be built without some of the necessary information. For instance:
For this reason I moved the checks into the DatabaseAdminClientImpl.createBackup
instead.
...anner/src/test/java/com/google/cloud/spanner/encryption/EncryptionConfigProtoMapperTest.java
Outdated
Show resolved
Hide resolved
...anner/src/test/java/com/google/cloud/spanner/encryption/EncryptionConfigProtoMapperTest.java
Outdated
Show resolved
Hide resolved
...e-cloud-spanner/src/test/java/com/google/cloud/spanner/encryption/EncryptionConfigsTest.java
Outdated
Show resolved
Hide resolved
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
1 similar comment
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
3 similar comments
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
Co-authored-by: Knut Olav Løite <koloite@gmail.com>
Co-authored-by: Knut Olav Løite <koloite@gmail.com>
Co-authored-by: Knut Olav Løite <koloite@gmail.com>
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
This is an auto-generated regeneration of the .pb.go files by cloud.google.com/go/internal/gapicgen. Once this PR is submitted, genbot will update the corresponding PR to depend on the newer version of go-genproto, and assign reviewers. Whilst this or any regen PR is open in go-genproto, genbot will not create any more regeneration PRs. If all regen PRs are closed, gapicgen will create a new set of regeneration PRs once per night. If you have been assigned to review this PR, please: - Ensure that CI is passing. If it's failing, it requires your manual attention. - Approve and submit this PR if you believe it's ready to ship. That will prompt genbot to assign reviewers to the google-cloud-go PR. Corresponding google-cloud-go PR: googleapis/google-cloud-go#4415 Changes: chore: regenerate API index Source-Link: googleapis/googleapis@db8f3aa feat: Enables generation of client libraries for C#, php and ruby. PiperOrigin-RevId: 383957820 Source-Link: googleapis/googleapis@2d51c27 feat: add `minute_ranges` field to `RunRealtimeReportRequest` object which can be used to specify the time range (in minutes) for realtime report queries docs: document the increase from 8 to 9 of the number of allowed dimensions in a report query PiperOrigin-RevId: 383930639 Source-Link: googleapis/googleapis@41b50a3 chore: use gapic-generator-python 0.50.4 PiperOrigin-RevId: 383929493 Source-Link: googleapis/googleapis@dd1ef28 feat(datastore/admin): Publish message definitions for new Cloud Datastore migration logging steps. PiperOrigin-RevId: 383927106 Source-Link: googleapis/googleapis@e96a2e6 feat(secretmanager): Tune Secret Manager auto retry parameters UNKNOWN removed from retryable errors per AIP-194. Added RESOURCE_EXHAUSTED with adjusted parameters for better performance with spikes of AccessSecretVersion requests at or near quota limits. PiperOrigin-RevId: 383886699 Source-Link: googleapis/googleapis@1eade81 feat(container): allow updating security group on existing clusters PiperOrigin-RevId: 383869440 Source-Link: googleapis/googleapis@e2b7a98 Remove Artman from README (googleapis#666) Artman has been deprecated, artman's repo archived, mentioning artman in the readme is strictly harmful, as it runs monolith generator only, which this repository is not compatible with anymore. Source-Link: googleapis/googleapis@b2a7d27 chore(ruby): Switch Bazel jobs to use the Ruby gapic-generator 0.9.1 PiperOrigin-RevId: 383721241 Source-Link: googleapis/googleapis@54dc383 chore: regenerate API index Source-Link: googleapis/googleapis@449686b feat: Initial commit for the Transcoder V1 client libraries. PiperOrigin-RevId: 383681557 Source-Link: googleapis/googleapis@509489a
🤖 I have created a release \*beep\* \*boop\* --- ### Updating meta-information for bleeding-edge SNAPSHOT release. --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Implements CMEK functionality for Spanner.