feat(storage): add support for bucket encryption enforcement config #15844
Conversation
product-auto-label
bot
added
the
api: storage
Summary of ChangesHello @bajajneha27, 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 significantly enhances the storage client library by adding robust support for bucket encryption enforcement configurations. It introduces new data structures to represent Google Managed, Customer Managed, and Customer Supplied encryption restrictions, and integrates these into the core 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. 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 support for bucket encryption enforcement configurations. The changes are well-structured and cover the necessary API, parsing, and testing aspects. However, I've identified several instances of code duplication that violate the repository's style guide, which prefers factoring out duplicated code that appears three or more times. I've also found a couple of issues in the tests that need to be addressed. My comments provide specific suggestions for refactoring the duplicated code and fixing the tests.
| "restriction_mode": "FULLY_RESTRICTED", | ||
| "effective_time": "2025-12-18T18:13:15Z" | ||
| }, | ||
| "customerManagedEncryptionEnforcementConfig": { | ||
| "restriction_mode": "NOT_RESTRICTED", | ||
| "effective_time": "2025-12-18T18:13:15Z" | ||
| }, | ||
| "customerSuppliedEncryptionEnforcementConfig": { | ||
| "restriction_mode": "NOT_RESTRICTED", | ||
| "effective_time": "2025-12-18T18:13:15Z" |
There was a problem hiding this comment.
The JSON keys for restriction_mode and effective_time should be in camelCase (restrictionMode, effectiveTime) to match the API specification and the parsing logic. The current snake_case keys will cause the parsing tests to fail or not test the intended functionality.
"restrictionMode": "FULLY_RESTRICTED",
"effectiveTime": "2025-12-18T18:13:15Z"
},
"customerManagedEncryptionEnforcementConfig": {
"restrictionMode": "NOT_RESTRICTED",
"effectiveTime": "2025-12-18T18:13:15Z"
},
"customerSuppliedEncryptionEnforcementConfig": {
"restrictionMode": "NOT_RESTRICTED",
"effectiveTime": "2025-12-18T18:13:15Z"| nlohmann::json expected_encryption_enforcement_config{ | ||
| {"googleManagedEncryptionEnforcementConfig", nlohmann::json{"restriction_mode", "FULLY_RESTRICTED"}}, | ||
| {"customerManagedEncryptionEnforcementConfig", nlohmann::json{"restriction_mode", "NOT_RESTRICTED"}}, | ||
| {"customerSuppliedEncryptionEnforcementConfig", nlohmann::json{"restriction_mode", "NOT_RESTRICTED"}}}; | ||
| EXPECT_EQ(expected_encryption_enforcement_config, actual["encryption"]); |
There was a problem hiding this comment.
This test assertion is incorrect for several reasons:
- The
expected_encryption_enforcement_configobject is constructed incorrectly.nlohmann::json{"key", "value"}creates a JSON array["key", "value"], not an object. - The expected JSON is missing the
defaultKmsKeyNameandeffectiveTimefields, which are present in theactual["encryption"]object. - The JSON key is
restriction_mode(snake_case), but it should berestrictionMode(camelCase).
Please check each field of actual["encryption"] individually instead of comparing against a partially constructed JSON object.
| struct GoogleManagedEncryptionEnforcementConfig { | ||
| std::string restriction_mode; | ||
| std::chrono::system_clock::time_point effective_time; | ||
| }; | ||
|
|
||
| ///@{ | ||
| /// @name Comparison operators For GoogleManagedEncryptionEnforcementConfig | ||
| inline bool operator==(GoogleManagedEncryptionEnforcementConfig const& lhs, | ||
| GoogleManagedEncryptionEnforcementConfig const& rhs) { | ||
| return std::tie(lhs.restriction_mode, lhs.effective_time) == | ||
| std::tie(rhs.restriction_mode, rhs.effective_time); | ||
| } | ||
|
|
||
| inline bool operator<(GoogleManagedEncryptionEnforcementConfig const& lhs, | ||
| GoogleManagedEncryptionEnforcementConfig const& rhs) { | ||
| return std::tie(lhs.restriction_mode, lhs.effective_time) < | ||
| std::tie(rhs.restriction_mode, rhs.effective_time); | ||
| } | ||
|
|
||
| inline bool operator!=(GoogleManagedEncryptionEnforcementConfig const& lhs, | ||
| GoogleManagedEncryptionEnforcementConfig const& rhs) { | ||
| return std::rel_ops::operator!=(lhs, rhs); | ||
| } | ||
|
|
||
| inline bool operator>(GoogleManagedEncryptionEnforcementConfig const& lhs, | ||
| GoogleManagedEncryptionEnforcementConfig const& rhs) { | ||
| return std::rel_ops::operator>(lhs, rhs); | ||
| } | ||
|
|
||
| inline bool operator<=(GoogleManagedEncryptionEnforcementConfig const& lhs, | ||
| GoogleManagedEncryptionEnforcementConfig const& rhs) { | ||
| return std::rel_ops::operator<=(lhs, rhs); | ||
| } | ||
|
|
||
| inline bool operator>=(GoogleManagedEncryptionEnforcementConfig const& lhs, | ||
| GoogleManagedEncryptionEnforcementConfig const& rhs) { | ||
| return std::rel_ops::operator>=(lhs, rhs); | ||
| } | ||
| ///@} | ||
|
|
||
| inline std::ostream& operator<<( | ||
| std::ostream& os, GoogleManagedEncryptionEnforcementConfig const& rhs) { | ||
| return os << "GoogleManagedEncryptionEnforcementConfig={restriction_mode=" | ||
| << rhs.restriction_mode << ", effective_time=" | ||
| << google::cloud::internal::FormatRfc3339(rhs.effective_time) | ||
| << "}"; | ||
| } |
There was a problem hiding this comment.
There is significant code duplication for the three ...EnforcementConfig structs. The struct definitions, comparison operators, and stream insertion operators are nearly identical for GoogleManagedEncryptionEnforcementConfig, CustomerManagedEncryptionEnforcementConfig, and CustomerSuppliedEncryptionEnforcementConfig. This could be refactored to reduce duplication and improve maintainability.
For example, you could use a template-based approach with tag dispatching to define the operators once for all three types, which would also help in maintaining type safety.
References
- The repository style guide prefers to factor out duplicated code if it appears 3 or more times in non-test files. The structs and their associated operators are duplicated three times here. (link)
| if (!v.google_managed_encryption_enforcement_config.restriction_mode | ||
| .empty()) { | ||
| builder.AddSubPatch( | ||
| "googleManagedEncryptionEnforcementConfig", | ||
| internal::PatchBuilder() | ||
| .SetStringField("restrictionMode", | ||
| v.google_managed_encryption_enforcement_config | ||
| .restriction_mode) | ||
| .SetStringField("effectiveTime", | ||
| google::cloud::internal::FormatRfc3339( | ||
| v.google_managed_encryption_enforcement_config | ||
| .effective_time))); | ||
| } | ||
| if (!v.customer_managed_encryption_enforcement_config.restriction_mode | ||
| .empty()) { | ||
| builder.AddSubPatch( | ||
| "customerManagedEncryptionEnforcementConfig", | ||
| internal::PatchBuilder() | ||
| .SetStringField("restrictionMode", | ||
| v.customer_managed_encryption_enforcement_config | ||
| .restriction_mode) | ||
| .SetStringField("effectiveTime", | ||
| google::cloud::internal::FormatRfc3339( | ||
| v.customer_managed_encryption_enforcement_config | ||
| .effective_time))); | ||
| } | ||
| if (!v.customer_supplied_encryption_enforcement_config.restriction_mode | ||
| .empty()) { | ||
| builder.AddSubPatch( | ||
| "customerSuppliedEncryptionEnforcementConfig", | ||
| internal::PatchBuilder() | ||
| .SetStringField("restrictionMode", | ||
| v.customer_supplied_encryption_enforcement_config | ||
| .restriction_mode) | ||
| .SetStringField("effectiveTime", | ||
| google::cloud::internal::FormatRfc3339( | ||
| v.customer_supplied_encryption_enforcement_config | ||
| .effective_time))); | ||
| } |
There was a problem hiding this comment.
The logic for adding sub-patches for each encryption enforcement config is duplicated three times. This can be refactored into a helper function to reduce code duplication and improve readability.
auto add_config_patch = [&](char const* name, auto const& config) {
if (config.restriction_mode.empty()) return;
builder.AddSubPatch(
name,
internal::PatchBuilder()
.SetStringField("restrictionMode", config.restriction_mode)
.SetStringField("effectiveTime",
google::cloud::internal::FormatRfc3339(
config.effective_time)));
};
add_config_patch("googleManagedEncryptionEnforcementConfig",
v.google_managed_encryption_enforcement_config);
add_config_patch("customerManagedEncryptionEnforcementConfig",
v.customer_managed_encryption_enforcement_config);
add_config_patch("customerSuppliedEncryptionEnforcementConfig",
v.customer_supplied_encryption_enforcement_config);References
- The repository style guide prefers to factor out duplicated code if it appears 3 or more times in non-test files. The logic to build the patch for each encryption config is duplicated three times. (link)
| StatusOr<GoogleManagedEncryptionEnforcementConfig> | ||
| ParseGoogleManagedEncryptionEnforcementConfig(nlohmann::json const& json) { | ||
| auto restriction_mode = json.value("restrictionMode", ""); | ||
| auto effective_time = internal::ParseTimestampField(json, "effectiveTime"); | ||
| if (!effective_time) return std::move(effective_time).status(); | ||
| return GoogleManagedEncryptionEnforcementConfig{std::move(restriction_mode), | ||
| *effective_time}; | ||
| } | ||
|
|
||
| StatusOr<CustomerManagedEncryptionEnforcementConfig> | ||
| ParseCustomerManagedEncryptionEnforcementConfig(nlohmann::json const& json) { | ||
| auto restriction_mode = json.value("restrictionMode", ""); | ||
| auto effective_time = internal::ParseTimestampField(json, "effectiveTime"); | ||
| if (!effective_time) return std::move(effective_time).status(); | ||
| return CustomerManagedEncryptionEnforcementConfig{std::move(restriction_mode), | ||
| *effective_time}; | ||
| } | ||
|
|
||
| StatusOr<CustomerSuppliedEncryptionEnforcementConfig> | ||
| ParseCustomerSuppliedEncryptionEnforcementConfig(nlohmann::json const& json) { | ||
| auto restriction_mode = json.value("restrictionMode", ""); | ||
| auto effective_time = internal::ParseTimestampField(json, "effectiveTime"); | ||
| if (!effective_time) return std::move(effective_time).status(); | ||
| return CustomerSuppliedEncryptionEnforcementConfig{ | ||
| std::move(restriction_mode), *effective_time}; | ||
| } |
There was a problem hiding this comment.
The three Parse...Config functions are identical except for their return type. This duplication can be avoided by using a single template function.
template <typename ConfigType>
StatusOr<ConfigType> ParseEncryptionEnforcementConfig(
nlohmann::json const& json) {
auto restriction_mode = json.value("restrictionMode", "");
auto effective_time = internal::ParseTimestampField(json, "effectiveTime");
if (!effective_time) return std::move(effective_time).status();
return ConfigType{std::move(restriction_mode), *effective_time};
}References
- The repository style guide prefers to factor out duplicated code if it appears 3 or more times in non-test files. The parsing logic for each encryption config is duplicated three times. (link)
| auto const& gmek = | ||
| meta.encryption().google_managed_encryption_enforcement_config; | ||
| if (!gmek.restriction_mode.empty()) { | ||
| nlohmann::json config; | ||
| config["restrictionMode"] = gmek.restriction_mode; | ||
| config["effectiveTime"] = | ||
| google::cloud::internal::FormatRfc3339(gmek.effective_time); | ||
| e["googleManagedEncryptionEnforcementConfig"] = std::move(config); | ||
| } | ||
| auto const& cmek = | ||
| meta.encryption().customer_managed_encryption_enforcement_config; | ||
| if (!cmek.restriction_mode.empty()) { | ||
| nlohmann::json config; | ||
| config["restrictionMode"] = cmek.restriction_mode; | ||
| config["effectiveTime"] = | ||
| google::cloud::internal::FormatRfc3339(cmek.effective_time); | ||
| e["customerManagedEncryptionEnforcementConfig"] = std::move(config); | ||
| } | ||
| auto const& csek = | ||
| meta.encryption().customer_supplied_encryption_enforcement_config; | ||
| if (!csek.restriction_mode.empty()) { | ||
| nlohmann::json config; | ||
| config["restrictionMode"] = csek.restriction_mode; | ||
| config["effectiveTime"] = | ||
| google::cloud::internal::FormatRfc3339(csek.effective_time); | ||
| e["customerSuppliedEncryptionEnforcementConfig"] = std::move(config); | ||
| } |
There was a problem hiding this comment.
The logic to serialize each of the three encryption enforcement configs to JSON is duplicated. This can be refactored into a helper function to reduce duplication and improve maintainability.
auto to_json_config = [&](char const* name, auto const& config_source) {
if (config_source.restriction_mode.empty()) return;
nlohmann::json config;
config["restrictionMode"] = config_source.restriction_mode;
config["effectiveTime"] =
google::cloud::internal::FormatRfc3339(config_source.effective_time);
e[name] = std::move(config);
};
to_json_config("googleManagedEncryptionEnforcementConfig",
meta.encryption().google_managed_encryption_enforcement_config);
to_json_config("customerManagedEncryptionEnforcementConfig",
meta.encryption().customer_managed_encryption_enforcement_config);
to_json_config("customerSuppliedEncryptionEnforcementConfig",
meta.encryption().customer_supplied_encryption_enforcement_config);References
- The repository style guide prefers to factor out duplicated code if it appears 3 or more times in non-test files. The serialization logic for each encryption config is duplicated three times. (link)
| auto const& gmek = | ||
| metadata.encryption().google_managed_encryption_enforcement_config; | ||
| if (!gmek.restriction_mode.empty()) { | ||
| auto& config = | ||
| *encryption.mutable_google_managed_encryption_enforcement_config(); | ||
| config.set_restriction_mode(gmek.restriction_mode); | ||
| *config.mutable_effective_time() = ToProtoTimestamp(gmek.effective_time); | ||
| } | ||
| auto const& cmek = | ||
| metadata.encryption().customer_managed_encryption_enforcement_config; | ||
| if (!cmek.restriction_mode.empty()) { | ||
| auto& config = | ||
| *encryption.mutable_customer_managed_encryption_enforcement_config(); | ||
| config.set_restriction_mode(cmek.restriction_mode); | ||
| *config.mutable_effective_time() = ToProtoTimestamp(cmek.effective_time); | ||
| } | ||
| auto const& csek = | ||
| metadata.encryption().customer_supplied_encryption_enforcement_config; | ||
| if (!csek.restriction_mode.empty()) { | ||
| auto& config = | ||
| *encryption.mutable_customer_supplied_encryption_enforcement_config(); | ||
| config.set_restriction_mode(csek.restriction_mode); | ||
| *config.mutable_effective_time() = ToProtoTimestamp(csek.effective_time); | ||
| } |
There was a problem hiding this comment.
The logic to update the proto for each of the three encryption enforcement configs is duplicated. This can be refactored into a helper lambda to reduce code duplication, similar to what was done in PatchEncryption.
auto update_config = [&](auto const& source, auto* dest) {
if (source.restriction_mode.empty()) return;
dest->set_restriction_mode(source.restriction_mode);
*dest->mutable_effective_time() = ToProtoTimestamp(source.effective_time);
};
update_config(metadata.encryption().google_managed_encryption_enforcement_config,
encryption.mutable_google_managed_encryption_enforcement_config());
update_config(metadata.encryption().customer_managed_encryption_enforcement_config,
encryption.mutable_customer_managed_encryption_enforcement_config());
update_config(metadata.encryption().customer_supplied_encryption_enforcement_config,
encryption.mutable_customer_supplied_encryption_enforcement_config());References
- The repository style guide prefers to factor out duplicated code if it appears 3 or more times in non-test files. The logic to update the proto for each encryption config is duplicated three times. (link)
bajajneha27
requested a deployment
to
external
GitHub Actions
Waiting
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15844 +/- ##
==========================================
+ Coverage 92.94% 92.96% +0.01%
==========================================
Files 2458 2458
Lines 227585 227742 +157
==========================================
+ Hits 211531 211717 +186
+ Misses 16054 16025 -29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
No description provided.