samples(Storage): Add samples and tests for bucket encryption enforcement configuration#20
samples(Storage): Add samples and tests for bucket encryption enforcement configuration#20mahendra-google wants to merge 8 commits intomainfrom
Conversation
Summary of ChangesHello @mahendra-google, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Google Cloud Storage C# samples by adding new code examples and tests for managing bucket encryption enforcement configurations. It provides developers with the tools to programmatically define, retrieve, and clear encryption policies, supporting both Google-managed and customer-managed keys, and enabling strict enforcement of CMEK for heightened data security within their storage buckets. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds new samples and tests for managing bucket encryption enforcement configurations. The changes are generally good, but I've identified several issues, including incorrect method calls in tests, weak assertions, and a logic flaw in one of the samples. My review includes suggestions to fix these issues and improve the overall quality and correctness of the code.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and corresponding tests for managing bucket encryption enforcement configurations in Google Cloud Storage. The changes look good and cover setting, getting, and removing these configurations. I've provided a few suggestions to improve code clarity and conciseness in the new sample and test files. These include clarifying assertions in tests, simplifying string construction, and refactoring repetitive code for better readability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and corresponding tests for managing bucket encryption enforcement configurations in Google Cloud Storage. The changes are well-structured and the new samples for getting, setting, and removing encryption configurations are clear.
I've identified a couple of areas for improvement:
- In
BucketSetEncryptionEnforcementConfig.cs, I've suggested adding input validation to ensure that when CMEK enforcement is enabled, a KMS key name is also provided. This will provide clearer error messages to users of the sample. - In
BucketSetEncryptionEnforcementConfigTest.cs, I've proposed enhancing the test to cover both enabling and disabling CMEK enforcement, which will improve test coverage for the new functionality.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and tests for managing bucket encryption enforcement configurations. The implementation is solid, and the tests provide good coverage. I've identified a few minor issues in the sample code, including a typo in a console message, some redundant code that could be simplified, and a magic string that would be better as a constant. Addressing these points will improve the quality and maintainability of the samples.
|
|
||
| if (bucket?.Encryption == null) | ||
| { | ||
| Console.WriteLine("No Encryption Configuration is found (Default GMEK is active"); |
There was a problem hiding this comment.
There is a typo in the console output string; it's missing a closing parenthesis. This should be corrected to ensure the sample's output is well-formed.
Console.WriteLine("No Encryption Configuration is found (Default GMEK is active)");| var gmConfig = bucket?.Encryption?.GoogleManagedEncryptionEnforcementConfig; | ||
| if (gmConfig != null) | ||
| { | ||
| Console.WriteLine($"Google Managed (GMEK) Enforcement Restriction Mode: {gmConfig.RestrictionMode}"); | ||
| Console.WriteLine($"Google Managed (GMEK) Enforcement Effective Time: {gmConfig.EffectiveTimeRaw}"); | ||
| } | ||
| var cmConfig = bucket?.Encryption?.CustomerManagedEncryptionEnforcementConfig; | ||
| if (cmConfig != null) | ||
| { | ||
| Console.WriteLine($"Customer Managed (CMEK) Enforcement Restriction Mode: {cmConfig.RestrictionMode}"); | ||
| Console.WriteLine($"Customer Managed (CMEK) Enforcement Effective Time: {cmConfig.EffectiveTimeRaw}"); | ||
| } | ||
| var csConfig = bucket?.Encryption?.CustomerSuppliedEncryptionEnforcementConfig; | ||
| if (csConfig != null) | ||
| { | ||
| Console.WriteLine($"Customer Supplied (CSEK) Enforcement Restriction Mode: {csConfig.RestrictionMode}"); | ||
| Console.WriteLine($"Customer Supplied (CSEK) Enforcement Effective Time: {csConfig.EffectiveTimeRaw}"); | ||
| } |
There was a problem hiding this comment.
The null-conditional operators (?.) used here are redundant. The storage.GetBucket() call on line 30 will throw an exception if the bucket doesn't exist, so bucket will not be null. Furthermore, the check on line 33 ensures that bucket.Encryption is not null within this block. Removing the unnecessary ?. will make the code cleaner and easier to read.
var gmConfig = bucket.Encryption.GoogleManagedEncryptionEnforcementConfig;
if (gmConfig != null)
{
Console.WriteLine($"Google Managed (GMEK) Enforcement Restriction Mode: {gmConfig.RestrictionMode}");
Console.WriteLine($"Google Managed (GMEK) Enforcement Effective Time: {gmConfig.EffectiveTimeRaw}");
}
var cmConfig = bucket.Encryption.CustomerManagedEncryptionEnforcementConfig;
if (cmConfig != null)
{
Console.WriteLine($"Customer Managed (CMEK) Enforcement Restriction Mode: {cmConfig.RestrictionMode}");
Console.WriteLine($"Customer Managed (CMEK) Enforcement Effective Time: {cmConfig.EffectiveTimeRaw}");
}
var csConfig = bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig;
if (csConfig != null)
{
Console.WriteLine($"Customer Supplied (CSEK) Enforcement Restriction Mode: {csConfig.RestrictionMode}");
Console.WriteLine($"Customer Supplied (CSEK) Enforcement Effective Time: {csConfig.EffectiveTimeRaw}");
}| bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "FullyRestricted" |
There was a problem hiding this comment.
The string literal "FullyRestricted" is a magic string. To improve readability and maintainability, it's best to define this as a constant. This provides a single, authoritative source for the value and prevents potential typos if it's used in multiple places (such as in the corresponding test file).
You could define a constant within the class like so:
private const string FullyRestrictedMode = "FullyRestricted";And then use FullyRestrictedMode here.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and corresponding tests for managing bucket encryption enforcement configurations. The code is well-structured and the tests effectively validate the new functionality. I have provided a few suggestions aimed at improving code clarity and maintainability, such as using var for local variables, simplifying control flow, and replacing magic strings with constants.
| BucketSetEncryptionEnforcementConfigSample bucketSetEncConfigSample = new BucketSetEncryptionEnforcementConfigSample(); | ||
| BucketGetEncryptionEnforcementConfigSample bucketGetEncConfigSample = new BucketGetEncryptionEnforcementConfigSample(); |
There was a problem hiding this comment.
For consistency with modern C# practices and the rest of the file, var should be used for local variable declarations when the type is obvious from the right-hand side of the assignment. This improves readability by reducing redundancy.
var bucketSetEncConfigSample = new BucketSetEncryptionEnforcementConfigSample();
var bucketGetEncConfigSample = new BucketGetEncryptionEnforcementConfigSample();| BucketSetEncryptionEnforcementConfigSample bucketSetEncConfigSample = new BucketSetEncryptionEnforcementConfigSample(); | ||
| BucketRemoveAllEncryptionEnforcementConfigSample bucketRemoveEncConfigSample = new BucketRemoveAllEncryptionEnforcementConfigSample(); |
There was a problem hiding this comment.
For consistency with modern C# practices and the rest of the file, var should be used for local variable declarations when the type is obvious from the right-hand side of the assignment. This improves readability by reducing redundancy.
var bucketSetEncConfigSample = new BucketSetEncryptionEnforcementConfigSample();
var bucketRemoveEncConfigSample = new BucketRemoveAllEncryptionEnforcementConfigSample();| if (enforceCmek) | ||
| { | ||
| Assert.NotNull(bucket.Encryption.CustomerManagedEncryptionEnforcementConfig); | ||
| Assert.Equal("FullyRestricted", bucket.Encryption.CustomerManagedEncryptionEnforcementConfig.RestrictionMode); |
There was a problem hiding this comment.
The string literal "FullyRestricted" is a "magic string". To improve maintainability and prevent typos, it's better to use a constant. This constant could be defined in this test class or, ideally, use a public constant from the BucketSetEncryptionEnforcementConfigSample class if one is introduced there.
| bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "FullyRestricted" |
There was a problem hiding this comment.
The string literal "FullyRestricted" is a "magic string". To improve readability and maintainability, it should be defined as a public constant. This avoids potential typos and centralizes the value if it ever needs to be changed.
For example, you could add the following to your class:
public const string FullyRestrictedRestrictionMode = "FullyRestricted";And then use FullyRestrictedRestrictionMode here.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds new samples and corresponding tests for managing bucket encryption enforcement configurations. The implementation is generally good, but I've identified a couple of issues. There's a bug in one of the tests that would cause it to fail due to a location mismatch between the bucket and the KMS key. Additionally, the documentation for one of the new samples is misleading. My review includes suggestions to address these points.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and corresponding tests for managing bucket encryption enforcement configurations in Google Cloud Storage. The new samples demonstrate how to set, get, and remove these configurations. The code is well-structured and the tests provide good coverage for the new functionality. I have a few suggestions to improve the code by using strongly-typed properties for date/time values, which enhances type safety and readability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds new samples and corresponding tests for managing bucket encryption enforcement configurations. The new samples demonstrate how to get, set, and remove these configurations. The code is generally well-written and the new functionality is tested. I've found one critical issue in one of the new tests where the test logic is incorrect and will fail. My review includes a detailed explanation and a code suggestion to fix the test.
| var bucket = bucketSetEncConfigSample.BucketSetEncryptionEnforcementConfig( | ||
| projectId: _fixture.ProjectId, | ||
| bucketName: bucketName, | ||
| keyLocation: _fixture.KmsKeyLocation, | ||
| kmsKeyRing: _fixture.KmsKeyRing, | ||
| kmsKeyName: _fixture.KmsKeyName); | ||
| var bucketEncryption = bucketRemoveEncConfigSample.BucketRemoveAllEncryptionEnforcementConfig(bucket.Name); | ||
| Assert.Null(bucketEncryption.Encryption); |
There was a problem hiding this comment.
The test logic for BucketRemoveAllEncryptionEnforcementConfig is incorrect and will fail.
- The test setup does not enable encryption enforcement before attempting to remove it. The call to
BucketSetEncryptionEnforcementConfigshould includeenforceCmek: trueto properly test the removal logic. - The assertion
Assert.Null(bucketEncryption.Encryption)is incorrect. TheBucketRemoveAllEncryptionEnforcementConfigsample is designed to remove only the enforcement rules while preserving the default KMS key. Therefore, theEncryptionproperty on the bucket will not be null.
The test should verify that CustomerManagedEncryptionEnforcementConfig becomes null after the operation, while DefaultKmsKeyName remains set.
bucketSetEncConfigSample.BucketSetEncryptionEnforcementConfig(
projectId: _fixture.ProjectId,
bucketName: bucketName,
keyLocation: _fixture.KmsKeyLocation,
kmsKeyRing: _fixture.KmsKeyRing,
kmsKeyName: _fixture.KmsKeyName,
enforceCmek: true);
var updatedBucket = bucketRemoveEncConfigSample.BucketRemoveAllEncryptionEnforcementConfig(bucketName);
Assert.NotNull(updatedBucket.Encryption);
Assert.Null(updatedBucket.Encryption.CustomerManagedEncryptionEnforcementConfig);
Assert.NotNull(updatedBucket.Encryption.DefaultKmsKeyName);There was a problem hiding this comment.
The issues identified in the previous comment have been successfully addressed. The test now correctly enables CMEK enforcement and verifies that CustomerManagedEncryptionEnforcementConfig is null after removal, while DefaultKmsKeyName is preserved. The additional assertions for CustomerSuppliedEncryptionEnforcementConfig and GoogleManagedEncryptionEnforcementConfig are also good additions.
Great work!
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds new samples and tests for managing bucket encryption enforcement configurations. The new samples demonstrate how to get, set, and remove these configurations. While the overall contribution is valuable, I've found a critical issue in the logic of the SetBucketEncryptionEnforcement sample that could lead to incorrect bucket configurations. I've also identified a flaw in one of the new tests that causes it to pass vacuously without actually testing the intended functionality. Additionally, there's a minor opportunity for code cleanup in another test file. Please see my detailed comments for suggestions on how to address these issues.
| if (enforceCmek) | ||
| { | ||
| bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "NotRestricted" | ||
| }; | ||
| bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.CustomerSuppliedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "FullyRestricted" | ||
| }; | ||
| bucket.Encryption.GoogleManagedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.GoogleManagedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "FullyRestricted" | ||
| }; | ||
|
|
||
| Console.WriteLine($"CMEK Enforcement Enabled"); | ||
| } | ||
| else | ||
| { | ||
| bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = null; | ||
| } | ||
|
|
||
| if (enforceCsek) | ||
| { | ||
| bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.CustomerSuppliedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "NotRestricted" | ||
| }; | ||
| bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "FullyRestricted" | ||
| }; | ||
| bucket.Encryption.GoogleManagedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.GoogleManagedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "FullyRestricted" | ||
| }; | ||
|
|
||
| Console.WriteLine($"CSEK Enforcement Enabled"); | ||
| } | ||
| else | ||
| { | ||
| bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig = null; | ||
| } | ||
|
|
||
| if (enforceGmek) | ||
| { | ||
| bucket.Encryption.GoogleManagedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.GoogleManagedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "NotRestricted" | ||
| }; | ||
| bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.CustomerSuppliedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "FullyRestricted" | ||
| }; | ||
| bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "FullyRestricted" | ||
| }; | ||
|
|
||
| Console.WriteLine($"GMEK Enforcement Enabled"); | ||
| } | ||
|
|
||
| if (restrictGmek) | ||
| { | ||
| bucket.Encryption.GoogleManagedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.GoogleManagedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "FullyRestricted" | ||
| }; | ||
|
|
||
| Console.WriteLine($"GMEK Restriction Enabled"); | ||
| } | ||
|
|
||
| if (restrictCmek) | ||
| { | ||
| bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "FullyRestricted" | ||
| }; | ||
|
|
||
| Console.WriteLine($"CMEK Restriction Enabled"); | ||
| } | ||
|
|
||
| if (restrictCsek) | ||
| { | ||
| bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig = | ||
| new Bucket.EncryptionData.CustomerSuppliedEncryptionEnforcementConfigData | ||
| { | ||
| RestrictionMode = "FullyRestricted" | ||
| }; | ||
|
|
||
| Console.WriteLine($"CSEK Restriction Enabled"); | ||
| } |
There was a problem hiding this comment.
The logic for setting encryption enforcement and restrictions has several issues that can lead to unexpected behavior and incorrect configurations:
- Overwriting settings: The use of separate
ifstatements for eachenforce*andrestrict*flag means that settings can be unintentionally overwritten. For example, if bothenforceCmekandenforceCsekare true, the settings forCustomerManagedEncryptionEnforcementConfigapplied by theenforceCmekblock will be overwritten by theenforceCsekblock. - Conflicting parameters: The code doesn't prevent mutually exclusive parameters from being used together, such as
enforceCmekandrestrictCmek. This can lead to confusing outcomes where the lastifstatement wins. - Unclear logic: The
elseblocks that nullify configurations (e.g., lines 91-94) add to the confusion, as their effect depends on the order of execution relative to otherifblocks.
To make this sample robust and clear, I recommend refactoring this entire section. A better approach would be:
- Add validation at the start of the method to ensure that at most one
enforce*flag is true, and thatenforce*andrestrict*flags for the same key type are not used together. - Use an
if-else if-elsestructure to handle the mutually exclusive enforcement options. - Handle the restriction flags only in the final
elsecase, where no enforcement is being applied.
This will make the logic much clearer and prevent invalid configurations.
There was a problem hiding this comment.
Thank you for addressing the feedback! The refactored code now correctly implements the validation checks and uses an if-else if-else structure, which significantly improves the clarity and robustness of the encryption enforcement logic. The issues regarding overwriting settings, conflicting parameters, and unclear logic have been resolved. This is a great improvement!
| var bucket = bucketSetEncConfigSample.SetBucketEncryptionEnforcement( | ||
| bucketName: bucketName, | ||
| kmsKeyName: keyName); |
There was a problem hiding this comment.
The test setup doesn't seem to be testing the removal of encryption enforcement correctly. The call to SetBucketEncryptionEnforcement only sets the kmsKeyName but does not enable any enforcement policies because the enforce* parameters default to false. As a result, the enforcement configurations are already null before BucketRemoveAllEncryptionEnforcementConfig is called. To properly test the removal, you should first set an enforcement policy.
var bucket = bucketSetEncConfigSample.SetBucketEncryptionEnforcement(bucketName: bucketName, kmsKeyName: keyName, enforceCmek: true);There was a problem hiding this comment.
| var bucket = bucketSetEncConfigSample.SetBucketEncryptionEnforcement( | |
| bucketName: bucketName, | |
| kmsKeyName: keyName); | |
| var bucket = bucketSetEncConfigSample.SetBucketEncryptionEnforcement( | |
| bucketName: bucketName, | |
| kmsKeyName: keyName, | |
| enforceCmek: true); |
| var expectedKmsKeyName = $"projects/{_fixture.ProjectId}/locations/{_fixture.KmsKeyLocation}/keyRings/{_fixture.KmsKeyRing}/cryptoKeys/{_fixture.KmsKeyName}"; | ||
| Assert.Equal(expectedKmsKeyName, bucketEncryptionData.DefaultKmsKeyName); |
There was a problem hiding this comment.
The expectedKmsKeyName variable is a duplication of the keyName variable defined on line 35. You can remove this variable and use keyName directly in the assertion for better code clarity and to avoid redundancy.
Assert.Equal(keyName, bucketEncryptionData.DefaultKmsKeyName);There was a problem hiding this comment.
Thank you for addressing the previous comment. The suggested change has been implemented, improving code clarity by removing the redundant expectedKmsKeyName variable.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and tests for managing bucket encryption enforcement configurations. The new code is well-structured and the tests cover the new functionality. I've provided a couple of suggestions for improvement: one to enhance a test to make it more comprehensive, and another to fix a small typo in the code documentation. Overall, this is a solid contribution.
| var bucket = bucketSetEncConfigSample.SetBucketEncryptionEnforcement( | ||
| bucketName: bucketName, | ||
| kmsKeyName: keyName); | ||
| var bucketEncryptionData = bucketGetEncConfigSample.BucketGetEncryptionEnforcementConfig(bucket.Name); | ||
| Assert.NotNull(bucketEncryptionData); | ||
| Assert.Equal(keyName, bucketEncryptionData.DefaultKmsKeyName); |
There was a problem hiding this comment.
The test currently only verifies that the DefaultKmsKeyName is set, but it doesn't test the retrieval of an actual encryption enforcement configuration. To make the test more comprehensive and align with its name, it should set an enforcement configuration and then verify that it's retrieved correctly.
var bucket = bucketSetEncConfigSample.SetBucketEncryptionEnforcement(
bucketName: bucketName,
kmsKeyName: keyName,
enforceCmek: true);
var bucketEncryptionData = bucketGetEncConfigSample.BucketGetEncryptionEnforcementConfig(bucket.Name);
Assert.NotNull(bucketEncryptionData);
Assert.Equal(keyName, bucketEncryptionData.DefaultKmsKeyName);
Assert.Multiple(() =>
{
Assert.Equal("NotRestricted", bucketEncryptionData.CustomerManagedEncryptionEnforcementConfig?.RestrictionMode);
Assert.Equal("FullyRestricted", bucketEncryptionData.CustomerSuppliedEncryptionEnforcementConfig?.RestrictionMode);
Assert.Equal("FullyRestricted", bucketEncryptionData.GoogleManagedEncryptionEnforcementConfig?.RestrictionMode);
});
|
/gemini review |
There was a problem hiding this comment.
Code Review
The pull request introduces new samples and corresponding tests for managing bucket encryption enforcement configurations in Google Cloud Storage. The changes include functionalities to set, get, and remove encryption enforcement settings (CMEK, CSEK, GMEK). The code is well-structured, includes appropriate validation for input parameters, and the tests cover various scenarios to ensure correctness. The samples provide clear demonstrations of how to use these new features.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and corresponding tests for managing bucket encryption enforcement configurations. The new samples demonstrate how to set, get, and remove these configurations. The code is well-structured and includes good validation for input parameters in the SetBucketEncryptionEnforcement sample. The tests cover the main scenarios. I've provided one suggestion to improve the assertions in one of the test files to make it more robust and readable.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and corresponding tests for managing bucket encryption enforcement configurations in Google Cloud Storage. The added samples for setting, getting, and removing configurations are clear and effectively demonstrate the functionality. The tests are generally well-structured. I have provided a couple of suggestions to improve the readability and coverage of the tests, which should enhance the long-term maintainability of the code. Overall, this is a valuable addition.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and corresponding tests for managing bucket encryption enforcement configurations. The code is well-structured and includes good validation for input parameters. The tests effectively cover the new functionality using parameterized tests for various scenarios. I have one suggestion to improve the efficiency of the BucketRemoveAllEncryptionEnforcementConfig sample by avoiding an unnecessary API call when there is no configuration to remove.
There was a problem hiding this comment.
Code Review
The pull request introduces new samples and corresponding tests for managing bucket encryption enforcement configurations. The code is well-structured, and the tests cover various scenarios for setting, getting, and removing encryption enforcement. The SetBucketEncryptionEnforcement method includes robust validation for conflicting enforcement and restriction flags, which is excellent for preventing misconfigurations. Overall, the changes are clear, functional, and contribute positively to the codebase.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and corresponding tests for managing bucket encryption enforcement configurations, including setting, getting, and removing them. The code is well-structured and the tests cover the new functionality appropriately. I've suggested a minor refactoring in one of the samples to improve code clarity and reduce duplication. Overall, this is a great addition.
| if (enforceCmek) | ||
| { | ||
| bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData { RestrictionMode = "NotRestricted" }; | ||
| bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig = new Bucket.EncryptionData.CustomerSuppliedEncryptionEnforcementConfigData { RestrictionMode = "FullyRestricted" }; | ||
| bucket.Encryption.GoogleManagedEncryptionEnforcementConfig = new Bucket.EncryptionData.GoogleManagedEncryptionEnforcementConfigData { RestrictionMode = "FullyRestricted" }; | ||
| Console.WriteLine("Enforcement: CMEK Enforced (CSEK and GMEK Restricted)"); | ||
| } | ||
| else if (enforceCsek) | ||
| { | ||
| bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig = new Bucket.EncryptionData.CustomerSuppliedEncryptionEnforcementConfigData { RestrictionMode = "NotRestricted" }; | ||
| bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData { RestrictionMode = "FullyRestricted" }; | ||
| bucket.Encryption.GoogleManagedEncryptionEnforcementConfig = new Bucket.EncryptionData.GoogleManagedEncryptionEnforcementConfigData { RestrictionMode = "FullyRestricted" }; | ||
| Console.WriteLine("Enforcement: CSEK Enforced (CMEK and GMEK Restricted)"); | ||
| } | ||
| else if (enforceGmek) | ||
| { | ||
| bucket.Encryption.GoogleManagedEncryptionEnforcementConfig = new Bucket.EncryptionData.GoogleManagedEncryptionEnforcementConfigData { RestrictionMode = "NotRestricted" }; | ||
| bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig = new Bucket.EncryptionData.CustomerSuppliedEncryptionEnforcementConfigData { RestrictionMode = "FullyRestricted" }; | ||
| bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData { RestrictionMode = "FullyRestricted" }; | ||
| Console.WriteLine("Enforcement: GMEK Enforced (CMEK and CSEK Restricted)"); | ||
| } | ||
| else | ||
| { | ||
| bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = restrictCmek | ||
| ? new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData { RestrictionMode = "FullyRestricted" } | ||
| : null; | ||
|
|
||
| bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig = restrictCsek | ||
| ? new Bucket.EncryptionData.CustomerSuppliedEncryptionEnforcementConfigData { RestrictionMode = "FullyRestricted" } | ||
| : null; | ||
|
|
||
| bucket.Encryption.GoogleManagedEncryptionEnforcementConfig = restrictGmek | ||
| ? new Bucket.EncryptionData.GoogleManagedEncryptionEnforcementConfigData { RestrictionMode = "FullyRestricted" } | ||
| : null; | ||
|
|
||
| Console.WriteLine("Configuration: No single encryption key type enforced. Individual restrictions applied where requested."); | ||
| } |
There was a problem hiding this comment.
The logic for setting the encryption enforcement configuration can be simplified to reduce code duplication. The current if/else if structure for handling enforceCmek, enforceCsek, and enforceGmek contains repeated code for setting restriction modes.
You can consolidate this logic to make it more concise and maintainable.
if (enforceCmek || enforceCsek || enforceGmek)
{
bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData { RestrictionMode = enforceCmek ? "NotRestricted" : "FullyRestricted" };
bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig = new Bucket.EncryptionData.CustomerSuppliedEncryptionEnforcementConfigData { RestrictionMode = enforceCsek ? "NotRestricted" : "FullyRestricted" };
bucket.Encryption.GoogleManagedEncryptionEnforcementConfig = new Bucket.EncryptionData.GoogleManagedEncryptionEnforcementConfigData { RestrictionMode = enforceGmek ? "NotRestricted" : "FullyRestricted" };
string enforcedKeyType = enforceCmek ? "CMEK" : enforceCsek ? "CSEK" : "GMEK";
string restrictedKeyTypes = enforceCmek ? "CSEK and GMEK" : enforceCsek ? "CMEK and GMEK" : "CMEK and CSEK";
Console.WriteLine($"Enforcement: {enforcedKeyType} Enforced ({restrictedKeyTypes} Restricted)");
}
else
{
bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = restrictCmek
? new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData { RestrictionMode = "FullyRestricted" }
: null;
bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig = restrictCsek
? new Bucket.EncryptionData.CustomerSuppliedEncryptionEnforcementConfigData { RestrictionMode = "FullyRestricted" }
: null;
bucket.Encryption.GoogleManagedEncryptionEnforcementConfig = restrictGmek
? new Bucket.EncryptionData.GoogleManagedEncryptionEnforcementConfigData { RestrictionMode = "FullyRestricted" }
: null;
Console.WriteLine("Configuration: No single encryption key type enforced. Individual restrictions applied where requested.");
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and tests for managing bucket encryption enforcement configurations. The changes are generally well-implemented. However, I've identified a critical logical flaw in the SetBucketEncryptionEnforcement sample that prevents it from correctly removing an existing enforcement policy. Additionally, I've suggested adding input validation to this method to improve its robustness and prevent invalid usage. The tests are good, but they don't cover the failure case I've described.
| if (message != null) | ||
| { | ||
| bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData { RestrictionMode = cmek }; | ||
| bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig = new Bucket.EncryptionData.CustomerSuppliedEncryptionEnforcementConfigData { RestrictionMode = csek }; | ||
| bucket.Encryption.GoogleManagedEncryptionEnforcementConfig = new Bucket.EncryptionData.GoogleManagedEncryptionEnforcementConfigData { RestrictionMode = gmek }; | ||
| Console.WriteLine($"Bucket {bucketName} updated with {message}"); | ||
| } |
There was a problem hiding this comment.
The current logic only updates the encryption enforcement configuration if one of the boolean flags (enforceCmek, enforceGmek, restrictCsek) is true. If all are false, the if (message != null) condition is false, and no changes are applied to the bucket's encryption enforcement properties. This is a bug, as it prevents a user from removing a previously set enforcement policy by calling this method with all flags set to false (the expectation would be to reset the policy to NotRestricted for all encryption types).
The fix is to always update the bucket's encryption properties and only make the console message conditional on whether a specific policy was set.
bucket.Encryption.CustomerManagedEncryptionEnforcementConfig = new Bucket.EncryptionData.CustomerManagedEncryptionEnforcementConfigData { RestrictionMode = cmek };
bucket.Encryption.CustomerSuppliedEncryptionEnforcementConfig = new Bucket.EncryptionData.CustomerSuppliedEncryptionEnforcementConfigData { RestrictionMode = csek };
bucket.Encryption.GoogleManagedEncryptionEnforcementConfig = new Bucket.EncryptionData.GoogleManagedEncryptionEnforcementConfigData { RestrictionMode = gmek };
if (message != null)
{
Console.WriteLine($"Bucket {bucketName} updated with {message}");
}| bool enforceGmek = false, | ||
| bool restrictCsek = false) | ||
| { | ||
| var storage = StorageClient.Create(); |
There was a problem hiding this comment.
It's good practice to validate input arguments at the beginning of a method. This method should check for two invalid states:
enforceCmekandenforceGmekbeing simultaneously true, as they are mutually exclusive policies.enforceCmekbeing true without akmsKeyName, which is required in that case.
Adding these checks makes the sample more robust and easier to use correctly.
if (enforceCmek && enforceGmek)
{
throw new ArgumentException("Cannot enforce both Customer-Managed and Google-Managed keys simultaneously.");
}
if (enforceCmek && string.IsNullOrEmpty(kmsKeyName))
{
throw new ArgumentException("A KMS key name must be provided to enforce Customer-Managed keys.", nameof(kmsKeyName));
}
var storage = StorageClient.Create();
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and corresponding tests for managing bucket encryption enforcement configurations. The samples cover setting, getting, and removing these configurations, and the tests validate these operations. The code is well-structured, but I have a couple of suggestions for the BucketSetEncryptionEnforcementConfig.cs sample to enhance its robustness and readability.
| bool enforceGmek = false, | ||
| bool restrictCsek = false) | ||
| { | ||
| var storage = StorageClient.Create(); |
There was a problem hiding this comment.
The method summary correctly states that kmsKeyName is required when enforceCmek is true. It would be good practice to validate this condition at the beginning of the method to provide a clear error message to the user, rather than letting the underlying API call fail.
if (enforceCmek && string.IsNullOrEmpty(kmsKeyName))
{
throw new ArgumentException("kmsKeyName must be provided when enforceCmek is true.", nameof(kmsKeyName));
}
var storage = StorageClient.Create();| string cmek = "NotRestricted", csek = "NotRestricted", gmek = "NotRestricted"; | ||
| string message = null; | ||
|
|
||
| if (enforceCmek) | ||
| { | ||
| csek = gmek = "FullyRestricted"; | ||
| message = "CMEK-only enforcement policy"; | ||
| } | ||
| else if (enforceGmek) | ||
| { | ||
| cmek = csek = "FullyRestricted"; | ||
| message = "GMEK-only enforcement policy"; | ||
| } | ||
| else if (restrictCsek) | ||
| { | ||
| csek = "FullyRestricted"; | ||
| message = "policy to restrict CSEK"; | ||
| } |
There was a problem hiding this comment.
The logic for determining the restriction modes can be simplified and made more declarative, similar to how it's done in the corresponding test file. This change improves readability and makes the code's intent clearer.
string cmek = enforceGmek ? "FullyRestricted" : "NotRestricted";
string gmek = enforceCmek ? "FullyRestricted" : "NotRestricted";
string csek = (enforceCmek || enforceGmek || restrictCsek) ? "FullyRestricted" : "NotRestricted";
string message = null;
if (enforceCmek)
{
message = "CMEK-only enforcement policy";
}
else if (enforceGmek)
{
message = "GMEK-only enforcement policy";
}
else if (restrictCsek)
{
message = "policy to restrict CSEK";
}
mahendra-google
force-pushed
the
bucket-encryption-enforcement-config
branch
2 times, most recently
from
b9b9c6f to
b375f88
Compare
…ment configuration (http:/b/465329369)
mahendra-google
force-pushed
the
bucket-encryption-enforcement-config
branch
from
4d58b52 to
c3096a6
Compare
2 participants
This PR contains samples and tests to set encryption enforcement configuration, get encryption enforcement configuration and remove all encryption enforcement configurations. Please see b/465329369.