googleapis / java-spanner Public
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
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 |
Implements CMEK functionality for Spanner.