feat[Spanner]: Add Multi Region Encryption samples #2832

amanda-tarafa · 2024-10-29T21:54:08Z

No description provided.

snippet-bot · 2024-10-29T21:54:15Z

Here is the summary of changes.

You are about to add 4 region tags.

spanner/api/Spanner.Samples/CopyBackupWithMultiRegionEncryptionAsync.cs:15, tag spanner_copy_backup_with_MR_CMEK
spanner/api/Spanner.Samples/CreateBackupWithMultiRegionEncryptionAsync.cs:15, tag spanner_create_backup_with_MR_CMEK
spanner/api/Spanner.Samples/CreateDatabaseWithMultiRegionEncryptionAsync.cs:15, tag spanner_create_database_with_MR_CMEK
spanner/api/Spanner.Samples/RestoreDatabaseWithMultiRegionEncryptionAsync.cs:15, tag spanner_restore_backup_with_MR_CMEK

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

Refresh this comment

amanda-tarafa · 2024-10-29T21:54:47Z

@panerorenn9541 FYI

jskeet · 2024-10-30T07:22:20Z

spanner/api/Spanner.Samples.Tests/CopyBackupWithMultiRegionEncryptionAsyncTest.cs

+    public async Task TestCopyBackupWithMultiRegionEncryptionAsync()
+    {
+        CopyBackupWithMultiRegionEncryptionAsyncSample copyBackupSample = new CopyBackupWithMultiRegionEncryptionAsyncSample();
+        string source_Project_id = _fixture.ProjectId;


Any reason these are using underscores instead of sourceProjectId etc?

You could potentially introduce a record type to make this simpler:

var source = new BackupName(_fixture.ProjectId, _fixture.InstanceId, _fixture.BackupId); var target = new BackupName(_fixture.ProjectId, _fixture.InstanceId, SpannerFixture.GenerateId("test_", 16)); ... var backup = await copyBackupSample.CopyBackupWithMultiRegionEncryptionAsync( source.InstanceId, source.ProjectId, source.BackupId, target.InstanceId, target.ProjectId, target.BackupId, expireTime, kmsKeyNames); ... } // As a nested type public record BackupName(string ProjectId, string InstanceId, string BackupId)

This is assuming there isn't a good BackupName class already, of course - I'd really have hoped there would be...

It looks like we do have BackupName, because it's used later. Just use that?

I simpliefied this. I used named parameters when calling the method and pass the fixture values directly.

Note that we don't pass the BackupName to the sample as parameter because we want to show how to use BackupName.

I've also fixed the code from where this was copied.

jskeet · 2024-10-30T07:26:15Z

spanner/api/Spanner.Samples.Tests/CreateBackupWithMultiRegionEncryptionAsyncTest.cs

+    [SkippableFact]
+    public async Task TestCreateBackupWithMultiRegionEncryptionAsync()
+    {
+        Skip.If(!_fixture.RunCmekBackupSampleTests, SpannerFixture.SkipCmekBackupSamplesMessage);


Perhaps change RunCmekBackupSampleTests to SkipCmekBackupSampleTests for consistency between the two properties, and to avoid the negation?

Yes! And changed everywhere.

jskeet · 2024-10-30T07:33:05Z

spanner/api/Spanner.Samples/CopyBackupWithMultiRegionEncryptionAsync.cs

+
+        Console.WriteLine($"Backup copied successfully.");
+        Console.WriteLine($"Backup with Id {sourceBackupId} has been copied from {sourceProjectId}/{sourceInstanceId} to {targetProjectId}/{targetInstanceId} Backup {targetBackupId}");
+        Console.WriteLine($"Backup {backup.Name} of size {backup.SizeBytes} bytes was created with encryption keys {string.Join(", ", (object[]) kmsKeyNames)} at {backup.CreateTime} from {backup.Database} and is in state {backup.State} and has version time {backup.VersionTime.ToDateTime()}");


Why the cast to object[]? I wouldn't expect that to be necessary. (I assume I'm missing something, but I'd like to know what...)

I don't think we need to call ToDateTime() either - I believe we'll end up with an ISO-8601-formatted timestamp if you just use {backup.VersionTime}. (You're doing that later with CreateTime, so I'd assume it would work the same?)

Why the cast to object[]? I wouldn't expect that to be necessary.

This is an array of CryptoKeyNames so the call to string.Join is ambiguos between object[] and IEnumerable<T>. I'm changing this to be an IEnumerable<CryptoKeyNames> instead.

I don't think we need to call ToDateTime() either

Yes, removed the call to ToDateTime, here and in the cody from where this was copied.

jskeet · 2024-10-30T07:34:33Z

spanner/api/Spanner.Samples/CreateBackupWithMultiRegionEncryptionAsync.cs

+        }
+
+        var backup = completedResponse.Result;
+        Console.WriteLine($"Backup {backup.Name} of size {backup.SizeBytes} bytes was created with encryption keys {string.Join(", ", (object[]) kmsKeyNames)} at {backup.CreateTime}");


Ditto on the cast here.

(It's in later samples too.)

Turned the parameter to IEnumerable<CryptoKeyName> everywhere. And removed the cast.

jskeet · 2024-10-30T07:36:18Z

spanner/api/Spanner.Samples/CreateDatabaseWithMultiRegionEncryptionAsync.cs

+        DatabaseAdminClient databaseAdminClient = DatabaseAdminClient.Create();
+        // Define create table statement for table #1.
+        var createSingersTable =
+        @"CREATE TABLE Singers (


Nit: indent this line? (Ditto for createAlbumsTable)

Done here and in all the create database samples.

jskeet

Approving so that you can merge after addressing the comments, which are all nits really.

amanda-tarafa

@jskeet all comments addressed and fixed elsewhere. Changes in the second commit. PTAL.

amanda-tarafa · 2024-10-30T17:53:03Z

spanner/api/Spanner.Samples.Tests/CopyBackupWithMultiRegionEncryptionAsyncTest.cs

+    public async Task TestCopyBackupWithMultiRegionEncryptionAsync()
+    {
+        CopyBackupWithMultiRegionEncryptionAsyncSample copyBackupSample = new CopyBackupWithMultiRegionEncryptionAsyncSample();
+        string source_Project_id = _fixture.ProjectId;


I simpliefied this. I used named parameters when calling the method and pass the fixture values directly.

Note that we don't pass the BackupName to the sample as parameter because we want to show how to use BackupName.

amanda-tarafa · 2024-10-30T17:58:56Z

spanner/api/Spanner.Samples.Tests/CreateBackupWithMultiRegionEncryptionAsyncTest.cs

+    [SkippableFact]
+    public async Task TestCreateBackupWithMultiRegionEncryptionAsync()
+    {
+        Skip.If(!_fixture.RunCmekBackupSampleTests, SpannerFixture.SkipCmekBackupSamplesMessage);


Yes! And changed everywhere.

amanda-tarafa · 2024-10-30T18:02:52Z

spanner/api/Spanner.Samples.Tests/CopyBackupWithMultiRegionEncryptionAsyncTest.cs

+    public async Task TestCopyBackupWithMultiRegionEncryptionAsync()
+    {
+        CopyBackupWithMultiRegionEncryptionAsyncSample copyBackupSample = new CopyBackupWithMultiRegionEncryptionAsyncSample();
+        string source_Project_id = _fixture.ProjectId;


I've also fixed the code from where this was copied.

amanda-tarafa · 2024-10-30T18:07:36Z

spanner/api/Spanner.Samples/CopyBackupWithMultiRegionEncryptionAsync.cs

+
+        Console.WriteLine($"Backup copied successfully.");
+        Console.WriteLine($"Backup with Id {sourceBackupId} has been copied from {sourceProjectId}/{sourceInstanceId} to {targetProjectId}/{targetInstanceId} Backup {targetBackupId}");
+        Console.WriteLine($"Backup {backup.Name} of size {backup.SizeBytes} bytes was created with encryption keys {string.Join(", ", (object[]) kmsKeyNames)} at {backup.CreateTime} from {backup.Database} and is in state {backup.State} and has version time {backup.VersionTime.ToDateTime()}");


Why the cast to object[]? I wouldn't expect that to be necessary.

This is an array of CryptoKeyNames so the call to string.Join is ambiguos between object[] and IEnumerable<T>. I'm changing this to be an IEnumerable<CryptoKeyNames> instead.

amanda-tarafa · 2024-10-30T18:09:09Z

spanner/api/Spanner.Samples/CopyBackupWithMultiRegionEncryptionAsync.cs

+
+        Console.WriteLine($"Backup copied successfully.");
+        Console.WriteLine($"Backup with Id {sourceBackupId} has been copied from {sourceProjectId}/{sourceInstanceId} to {targetProjectId}/{targetInstanceId} Backup {targetBackupId}");
+        Console.WriteLine($"Backup {backup.Name} of size {backup.SizeBytes} bytes was created with encryption keys {string.Join(", ", (object[]) kmsKeyNames)} at {backup.CreateTime} from {backup.Database} and is in state {backup.State} and has version time {backup.VersionTime.ToDateTime()}");


I don't think we need to call ToDateTime() either

Yes, removed the call to ToDateTime, here and in the cody from where this was copied.

amanda-tarafa · 2024-10-30T18:19:50Z

spanner/api/Spanner.Samples/CreateBackupWithMultiRegionEncryptionAsync.cs

+        }
+
+        var backup = completedResponse.Result;
+        Console.WriteLine($"Backup {backup.Name} of size {backup.SizeBytes} bytes was created with encryption keys {string.Join(", ", (object[]) kmsKeyNames)} at {backup.CreateTime}");


Turned the parameter to IEnumerable<CryptoKeyName> everywhere. And removed the cast.

amanda-tarafa · 2024-10-30T18:23:08Z

spanner/api/Spanner.Samples/CreateDatabaseWithMultiRegionEncryptionAsync.cs

+        DatabaseAdminClient databaseAdminClient = DatabaseAdminClient.Create();
+        // Define create table statement for table #1.
+        var createSingersTable =
+        @"CREATE TABLE Singers (


Done here and in all the create database samples.

jskeet

One simple nit - rest looks good.

jskeet · 2024-10-31T06:29:11Z

spanner/api/Spanner.Samples.Tests/CopyBackupTest.cs

@@ -38,9 +38,11 @@ public void CopyBackup()
        string target_backupId = SpannerFixture.GenerateId("test_", 16);
        DateTimeOffset expireTime = DateTimeOffset.UtcNow.AddHours(12);

-        Backup backup = copyBackupSample.CopyBackup(source_Instance_id, source_Project_id, source_backupId, 
-            target_Instance_id, target_Project_id, target_backupId, expireTime);
+        Backup backup = copyBackupSample.CopyBackup(


The variables are still declared above, just not used any more.

Done, thanks!

amanda-tarafa requested a review from jskeet October 29, 2024 21:54

amanda-tarafa requested review from a team as code owners October 29, 2024 21:54

product-auto-label bot added the samples Issues that are directly related to samples. label Oct 29, 2024

blunderbuss-gcf bot assigned jskeet Oct 29, 2024

amanda-tarafa force-pushed the spanner-mr-cmek branch from d7d1609 to 9ba9295 Compare October 29, 2024 21:56

amanda-tarafa mentioned this pull request Oct 29, 2024

feat: add Spanner samples for MR CMEK #2772

Closed

amanda-tarafa force-pushed the spanner-mr-cmek branch from 9ba9295 to ab94b89 Compare October 30, 2024 06:15

jskeet reviewed Oct 30, 2024

View reviewed changes

jskeet previously approved these changes Oct 30, 2024

View reviewed changes

amanda-tarafa dismissed jskeet’s stale review via e10cf06 October 30, 2024 18:26

amanda-tarafa force-pushed the spanner-mr-cmek branch from ab94b89 to e10cf06 Compare October 30, 2024 18:26

amanda-tarafa commented Oct 30, 2024

View reviewed changes

amanda-tarafa force-pushed the spanner-mr-cmek branch from e10cf06 to 95a5008 Compare October 30, 2024 20:25

jskeet previously approved these changes Oct 31, 2024

View reviewed changes

feat[Spanner]: Add Multi Region Encryption samples

adb188b

amanda-tarafa dismissed jskeet’s stale review via adb188b October 31, 2024 06:34

amanda-tarafa force-pushed the spanner-mr-cmek branch from 95a5008 to adb188b Compare October 31, 2024 06:34

jskeet approved these changes Oct 31, 2024

View reviewed changes

amanda-tarafa merged commit c906f07 into GoogleCloudPlatform:main Oct 31, 2024
8 checks passed

amanda-tarafa deleted the spanner-mr-cmek branch October 31, 2024 16:07

This was referenced Dec 4, 2024

docs: add Multi Region Encryption samples googleapis/java-spanner#3524

Merged

feat(spanner): add samples for using multi-region encryption keys for backups and database GoogleCloudPlatform/golang-samples#4790

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat[Spanner]: Add Multi Region Encryption samples #2832

feat[Spanner]: Add Multi Region Encryption samples #2832

amanda-tarafa commented Oct 29, 2024

snippet-bot bot commented Oct 29, 2024 •

edited

Loading

amanda-tarafa commented Oct 29, 2024

jskeet Oct 30, 2024

jskeet Oct 30, 2024

jskeet Oct 30, 2024

amanda-tarafa Oct 30, 2024

amanda-tarafa Oct 30, 2024

jskeet Oct 30, 2024

amanda-tarafa Oct 30, 2024

jskeet Oct 30, 2024

jskeet Oct 30, 2024

amanda-tarafa Oct 30, 2024

amanda-tarafa Oct 30, 2024

jskeet Oct 30, 2024

jskeet Oct 30, 2024

amanda-tarafa Oct 30, 2024

jskeet Oct 30, 2024

amanda-tarafa Oct 30, 2024

jskeet left a comment

amanda-tarafa left a comment

amanda-tarafa Oct 30, 2024

amanda-tarafa Oct 30, 2024

amanda-tarafa Oct 30, 2024

amanda-tarafa Oct 30, 2024

amanda-tarafa Oct 30, 2024

amanda-tarafa Oct 30, 2024

amanda-tarafa Oct 30, 2024

jskeet left a comment

jskeet Oct 31, 2024

amanda-tarafa Oct 31, 2024

feat[Spanner]: Add Multi Region Encryption samples #2832

feat[Spanner]: Add Multi Region Encryption samples #2832

Conversation

amanda-tarafa commented Oct 29, 2024

snippet-bot bot commented Oct 29, 2024 • edited Loading

amanda-tarafa commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

amanda-tarafa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jskeet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snippet-bot bot commented Oct 29, 2024 •

edited

Loading