Skip to content

feat(storage): add support for bucket ip filter #15250

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.

Sign up for GitHub

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

Jump to bottom
Open
ddelgrosso1 wants to merge 12 commits into
base: main
Choose a base branch
from
Open

feat(storage): add support for bucket ip filter #15250

ddelgrosso1 wants to merge 12 commits into from

Conversation

@ddelgrosso1
Copy link
Contributor

@ddelgrosso1 ddelgrosso1 commented Jul 2, 2025 •
edited by scotthart
Loading

This change is Reviewable

@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Jul 2, 2025
@codecov
Copy link

codecov bot commented Jul 2, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 96.96970% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.95%. Comparing base (19851cb) to head (7284fb1).

Files with missing lines Patch % Lines
google/cloud/storage/bucket_ip_filter_test.cc 92.85% 4 Missing ⚠️
...gle/cloud/storage/internal/bucket_requests_test.cc 95.31% 3 Missing ⚠️
...ud/storage/internal/grpc/bucket_metadata_parser.cc 94.23% 3 Missing ⚠️
google/cloud/storage/bucket_ip_filter.cc 97.36% 1 Missing ⚠️
google/cloud/storage/bucket_metadata_test.cc 97.56% 1 Missing ⚠️
...torage/internal/grpc/bucket_request_parser_test.cc 96.00% 1 Missing ⚠️
...gle/cloud/storage/tests/bucket_integration_test.cc 95.65% 1 Missing ⚠️
Additional details and impacted files 
@@           Coverage Diff            @@
##             main   #15250    +/-   ##
========================================
  Coverage   92.95%   92.95%            
========================================
  Files        2448     2450     +2     
  Lines      226468   226930   +462     
========================================
+ Hits       210514   210949   +435     
- Misses      15954    15981    +27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ddelgrosso1 ddelgrosso1 marked this pull request as ready for review July 2, 2025 17:40
@ddelgrosso1 ddelgrosso1 requested review from a team as code owners July 2, 2025 17:40
rajeevpodar
rajeevpodar reviewed Jul 3, 2025
View reviewed changes
google/cloud/storage/internal/bucket_metadata_parser.cc
return Status{};
}

Status ParseIpFilter(BucketMetadata& meta, nlohmann::json const& json) {
Copy link

@rajeevpodar rajeevpodar Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code can be made more readable by using from_json semantics.

Copy link
Contributor Author

@ddelgrosso1 ddelgrosso1 Jul 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to leave this for the time being as it follows the coding patterns in the rest of the file. Longer term we may want to refactor to use to/from_json.

google/cloud/storage/internal/bucket_metadata_parser.cc
json["iamConfiguration"] = std::move(value);
}

void ToJsonIpFilter(nlohmann::json& json, BucketMetadata const& meta) {
Copy link

@rajeevpodar rajeevpodar Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, to_json semantics seems to be potential candidate here.

Copy link
Contributor Author

@ddelgrosso1 ddelgrosso1 Jul 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to leave this for the time being as it follows the coding patterns in the rest of the file. Longer term we may want to refactor to use to/from_json.

google/cloud/storage/internal/grpc/bucket_request_parser.cc
}

Status PatchIpFilter(Bucket& b, nlohmann::json const& p) {
if (p.is_null()) {
Copy link

@rajeevpodar rajeevpodar Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from_json seems more suitable

Copy link
Contributor Author

@ddelgrosso1 ddelgrosso1 Jul 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to leave this for the time being as it follows the coding patterns in the rest of the file. Longer term we may want to refactor to use to/from_json.

bajajneha27
bajajneha27 previously approved these changes Jul 3, 2025
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rajeevpodar rajeevpodar rajeevpodar left review comments

@bajajneha27 bajajneha27 bajajneha27 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

api: storage Issues related to the Cloud Storage API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ddelgrosso1 @rajeevpodar @bajajneha27