samples(Storage): Add samples and tests for bucket encryption enforcement configuration#3313
Conversation
|
Here is the summary of changes. You are about to add 3 region tags.
This comment is generated by snippet-bot.
|
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 introducing new functionalities for managing bucket encryption enforcement. It provides developers with clear examples and robust tests for setting, retrieving, and removing encryption policies, thereby improving data security and compliance options for their storage buckets. Highlights
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
|
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 new code is well-structured and the tests provide good coverage for the new samples. I have one suggestion to refactor a small part of the logic in one of the samples to improve its clarity and maintainability. Overall, this is a solid contribution.
storage/api/Storage.Samples/BucketSetEncryptionEnforcementConfig.cs
Outdated
Show resolved
Hide resolved
b9b9c6f to
b375f88
Compare
…ment configuration (http:/b/465329369)
4d58b52 to
c3096a6
Compare
|
|
||
| if (bucket.Encryption == null) | ||
| { | ||
| Console.WriteLine("No Encryption Configuration is found (Default GMEK is active)"); |
There was a problem hiding this comment.
Is Default GMEK if not specified?
| Console.WriteLine($"Google Managed (GMEK) Enforcement Restriction Mode: {gmConfig.RestrictionMode}"); | ||
| Console.WriteLine($"Google Managed (GMEK) Enforcement Effective Time: {gmConfig.EffectiveTimeRaw}"); |
There was a problem hiding this comment.
Can we print both the fields in the same line? for others as well
| /// <param name="enforceCmek">If true, enforces Customer-Managed Encryption Key.</param> | ||
| /// <param name="enforceGmek">If true, enforces Google-Managed Encryption Key.</param> | ||
| /// <param name="restrictCsek">If true, restricts Customer-Supplied Encryption Key.</param> | ||
| public Bucket SetBucketEncryptionEnforcementConfig( |
There was a problem hiding this comment.
Can we return encryption instead of the whole bucket?
| string gmek = enforceCmek ? "FullyRestricted" : "NotRestricted"; | ||
| string csek = (enforceCmek || enforceGmek || restrictCsek) ? "FullyRestricted" : "NotRestricted"; | ||
|
|
||
| string message = enforceCmek ? "CMEK-only enforcement policy" |
There was a problem hiding this comment.
Can we standardize the message across all conditions?
| /// Remove all encryption enforcement configurations from the bucket. | ||
| /// </summary> | ||
| /// <param name="bucketName">The name of the bucket.</param> | ||
| public Bucket BucketRemoveAllEncryptionEnforcementConfig(string bucketName = "your-unique-bucket-name") |
There was a problem hiding this comment.
Same here. Return Encryption Object instead of Bucket
|
|
||
| string cmek = enforceGmek ? "FullyRestricted" : "NotRestricted"; | ||
| string gmek = enforceCmek ? "FullyRestricted" : "NotRestricted"; | ||
| string csek = (enforceCmek || enforceGmek || restrictCsek) ? "FullyRestricted" : "NotRestricted"; |
There was a problem hiding this comment.
Why is this a special condition here? Can we just accept "enforceCsek" flag and have a similar condition as gmek and cmek?
| Assert.Equal(keyName, bucketEncryptionData.DefaultKmsKeyName); | ||
| Assert.Multiple(() => | ||
| { | ||
| Assert.Equal("NotRestricted", bucketEncryptionData.CustomerManagedEncryptionEnforcementConfig?.RestrictionMode); |
There was a problem hiding this comment.
If CMEK is being enforced, this should be FullyRestricted, right? And the others should be NotRestricted.
This PR includes samples and tests for the following bucket encryption enforcement operations:
Please see b/465329369.