-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: add Spanner samples for MR CMEK #2772
base: main
Are you sure you want to change the base?
Conversation
Here is the summary of changes. You are about to add 4 region tags.
This comment is generated by snippet-bot.
|
Change KmsKeyName to KmsKeyNames
Change KmsKeyNamesAsCryptoKeyName to KmsKeyNamesAsCryptoKeyNames
Change KmsKeyNamesAsCryptoKeyName to be KmsKeyNamesAsCryptoKeyNames
Change KmsKeyNamesAsCryptoKeyName to be KmsKeyNamesAsCryptoKeyNames
Change KmsKeyNamesAsCryptoKeyName to be KmsKeyNamesAsCryptoKeyNames and use database encryption config to print
Use database encryption config to print
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.
Issues I've commented on appear on several places even though I flag them on a single place, please fix all ocurrences.
This code doesn't build, see the build log for errors.
When you have run the tests locally and they pass, let me know and I can run them on the PR with the backup tests enabled to make certain they run on CI before merging. At the moment these new tests will conflict with the existing backup samples and tests.
|
||
[CollectionDefinition(nameof(SpannerFixture))] | ||
public class SpannerFixture : IAsyncLifetime, ICollectionFixture<SpannerFixture> | ||
// Copyright 2020 Google Inc. |
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.
This seems to have made changes to line endings, which mean I cannot review the actual file changes.
I just fixed the line endings for this file on a separete commit in #2781 . If you wait for that to be merged and rebase this PR, then we should be fine.
@@ -0,0 +1,66 @@ | |||
// Copyright 2021 Google Inc. |
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.
Update copyright year for all new files.
using System; | ||
using System.Threading.Tasks; | ||
|
||
public class CreateBackupWithMRCMEKAsyncSample |
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.
Naming conventions: MrCmek
, acronyms are not all capital letters.
Skip.If(!_fixture.RunCmekBackupSampleTests, SpannerFixture.SkipCmekBackupSamplesMessage); | ||
// Create a backup with custom encryption keys. | ||
var sample = new CreateBackupWithMRCMEKAsyncSample(); | ||
var backup = await sample.CreateBackupWithMRCMEKAsync(_fixture.ProjectId, _fixture.InstanceId, _fixture.FixedEncryptedDatabaseId, _fixture.EncryptedBackupId, _fixture.KmsKeyNames); |
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.
You need to use a diferent backup ID, _fixture.EncryptedBackupId
is being used for the "plain" CMEK encruption samples.
Assert.Equal(_fixture.KmsKeyNames.Length, backup.EncryptionInfo.KmsKeyVersionsAsCryptoKeyVersionNames.Length); | ||
foreach (CryptoKeyVersionName KmsKeyVersion in backup.EncryptionInfo.KmsKeyVersionsAsCryptoKeyVersionNames) | ||
{ | ||
Assert.True(_fixture.KmsKeyNames.contains(KmsKeyVersion.CryptoKeyId)); | ||
} | ||
} |
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.
Probably cleaner to do something like:
Assert.Equal(_fixture.KmsKeyNames.Length, backup.EncryptionInfo.KmsKeyVersionsAsCryptoKeyVersionNames.Length); | |
foreach (CryptoKeyVersionName KmsKeyVersion in backup.EncryptionInfo.KmsKeyVersionsAsCryptoKeyVersionNames) | |
{ | |
Assert.True(_fixture.KmsKeyNames.contains(KmsKeyVersion.CryptoKeyId)); | |
} | |
} | |
Assert.All(backup.EncryptionInfo.KmsKeyVersionsAsCryptoKeyVersionNames, keyName => _fixture.KmsKeyNames.Contains(keyName.CriptoKeyId)); |
{ | ||
// Create a database with custom encryption keys. | ||
var sample = new CreateDatabaseWithMRCMEKAsyncSample(); | ||
var database = await sample.CreateDatabaseWithMRCMEKAsync(_fixture.ProjectId, _fixture.InstanceId, _fixture.EncryptedDatabaseId, _fixture.KmsKeyNames); |
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.
Similarly, use a different database ID here, this one is used for the plain encryption configuration.
|
||
var response = databaseAdminClient.CopyBackup(request); | ||
Console.WriteLine("Waiting for the operation to finish."); | ||
var completedResponse = response.PollUntilCompleted(new PollSettings(Expiration.FromTimeout(TimeSpan.FromMinutes(15)), TimeSpan.FromMinutes(2))); |
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'm guessing default poll settings are not enough here?
|
||
Console.WriteLine($"Backup created 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 {0} at {backup.CreateTime} from {backup.Database} and is in state {backup.State} and has version time {backup.VersionTime.ToDateTime()}", string.Join(", ", kmsKeyNames)); |
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.
Here and everywhere else where this is present:
Console.WriteLine($"Backup {backup.Name} of size {backup.SizeBytes} bytes was created with encryption keys {0} at {backup.CreateTime} from {backup.Database} and is in state {backup.State} and has version time {backup.VersionTime.ToDateTime()}", string.Join(", ", kmsKeyNames)); | |
Console.WriteLine($"Backup {backup.Name} of size {backup.SizeBytes} bytes was created with encryption keys {string.Join(", ", kmsKeyNames)} at {backup.CreateTime} from {backup.Database} and is in state {backup.State} and has version time {backup.VersionTime.ToDateTime()}" ); |
[Fact] | ||
public void CopyBackupWithMRCMEK() | ||
{ | ||
CopyBackupWithMRCMEKSample copyBackupWithMRCMEKSample = new CopyBackupWithMRCMEKSample(); |
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.
You cannot use the same backup IDs that are being used for the other backup tests. You need specific IDs for these tests, and make certain the backups are there when you need them, etc.
Add samples and tests for create database, create backup, copy backup, and restore backup.