setup bucket with ip filter #49
Conversation
Summary of ChangesHello @shubhangi-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 introduces the capability to configure IP filters for Google Cloud Storage buckets directly through the Ruby client library. It extends existing bucket management functionalities to allow specifying IP filter settings during bucket creation and provides methods to update these settings on existing buckets. The changes also include new sample code to demonstrate the usage of this feature, although some debugging code is still present, suggesting it is a work-in-progress. 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
|
shubhangi-google
changed the title
![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 introduces IP filter functionality for Google Cloud Storage buckets. The core library changes in google-cloud-storage/lib/google/cloud/storage/bucket.rb and google-cloud-storage/lib/google/cloud/storage/project.rb appear to be correctly implemented. However, the sample code and acceptance tests in google-cloud-storage/samples/acceptance/buckets_test.rb and google-cloud-storage/samples/storage_create_bucket_with_ip_filter.rb contain several issues, including debugging statements (binding.pry), commented-out code, and a significant mix-up of responsibilities in the update_bucket_with_ip_filter sample, which attempts to manage IAM roles instead of solely updating the IP filter. These issues need to be addressed to ensure the samples are correct, clean, and functional.
| require "google/cloud/resource_manager/v3" | ||
| storage = Google::Cloud::Storage.new | ||
|
|
||
| ip_filter = Google::Apis::StorageV1::Bucket::IpFilter.new( | ||
| mode: "Enabled", | ||
| public_network_source: Google::Apis::StorageV1::Bucket::IpFilter::PublicNetworkSource.new( | ||
| allowed_ip_cidr_ranges: [ | ||
| "0.0.0.0/0", "::/0" | ||
| ] | ||
| ), | ||
| allow_all_service_agent_access: false | ||
| ) | ||
| # require "google/cloud/iam" | ||
| # Initialize the IAM Admin Client (Not Credentials Client) | ||
| # iam_client = Google::Cloud::Iam::Service.new | ||
| client = Google::Cloud::ResourceManager::V3::Projects::Client.new | ||
|
|
||
|
|
||
| project_id = "storage-sdk-vendor" | ||
| role_id = "GcsIpExemptRole" | ||
|
|
||
| # Define the Custom Role | ||
| # role = Google::Cloud::Iam::V1::Role.new( | ||
| # title: "GCS IP Exemption Role", | ||
| # included_permissions: ["storage.buckets.exemptFromIpFilter"], | ||
| # stage: :GA | ||
| # ) | ||
| resource = "projects/#{project_id}" | ||
|
|
||
| # Call test_iam_permissions. | ||
| # In Ruby, you can pass a Hash that matches the request structure. | ||
| response = client.test_iam_permissions( | ||
| resource: resource, | ||
| permissions: ["storage.buckets.exemptFromIpFilter"] | ||
| ) | ||
|
|
||
| # The response object contains a 'permissions' array of granted permissions | ||
| puts "Granted permissions: #{response.permissions}" | ||
|
|
||
| response.permissions | ||
| binding.pry | ||
|
|
||
| # role = "projects/#{storage.project}/roles/GcsIpExemptRole" | ||
| member = "serviceAccount:insecure-cloudtop-shared-user@cloudtop-prod-asia-east.iam.gserviceaccount.com" | ||
|
|
||
| bucket.policy requested_policy_version: 3 do |policy| | ||
| policy.bindings.insert role: role, members: [member] | ||
| end | ||
| # bucket = storage.create_bucket bucket_name | ||
| # bucket.policy do |p| | ||
| # p.add "roles/storage.buckets.exemptFromIpFilter", "serviceAccount:insecure-cloudtop-shared-user@cloudtop-prod-asia-east.iam.gserviceaccount.com" | ||
| # end | ||
| # binding.pry | ||
|
|
There was a problem hiding this comment.
The update_bucket_with_ip_filter function contains significant logic related to IAM (requiring google/cloud/resource_manager/v3, test_iam_permissions, and modifying bucket.policy). This sample's purpose, as indicated by its name, should be to update the IP filter of an existing bucket, not to manage IAM roles or test permissions. Mixing these concerns makes the sample overly complex, confusing, and potentially misleading for users. Please refactor this function to focus solely on updating the bucket's IP filter. If IAM role management is needed, it should be in a separate, dedicated sample.
# The ID to give your GCS bucket
# bucket_name = "your-unique-bucket-name"
require "google/cloud/storage"
storage = Google::Cloud::Storage.new
bucket = storage.bucket bucket_name
ip_filter = Google::Apis::StorageV1::Bucket::IpFilter.new(
mode: "Enabled",
public_network_source: Google::Apis::StorageV1::Bucket::IpFilter::PublicNetworkSource.new(
allowed_ip_cidr_ranges: [
"0.0.0.0/0", "::/0"
]
),
allow_all_service_agent_access: false
)
bucket.update do |b|
b.ip_filter = ip_filter
end| require_relative "../storage_get_autoclass" | ||
| require_relative "../storage_set_autoclass" | ||
| require_relative "../storage_move_object" | ||
| require 'pry' |
There was a problem hiding this comment.
The require 'pry' statement is a debugging tool and should not be committed to the repository. Please remove it.
| # describe "bucket lifecycle" do | ||
| # it "create_bucket, create_bucket_class_location, list_buckets, get_bucket_metadata, delete_bucket" do | ||
| # # create_bucket | ||
| # bucket_name = random_bucket_name | ||
| # refute storage_client.bucket bucket_name | ||
|
|
||
| # retry_resource_exhaustion do | ||
| # assert_output "Created bucket: #{bucket_name}\n" do | ||
| # create_bucket bucket_name: bucket_name | ||
| # end | ||
| # end | ||
|
|
||
| # refute_nil storage_client.bucket bucket_name | ||
|
|
||
| # # create_bucket_class_location | ||
|
|
||
| # secondary_bucket_name = random_bucket_name | ||
| # location = "ASIA" | ||
| # storage_class = "COLDLINE" | ||
| # refute storage_client.bucket secondary_bucket_name | ||
|
|
||
| # retry_resource_exhaustion do | ||
| # assert_output "Created bucket #{secondary_bucket_name} in #{location} with #{storage_class} class\n" do | ||
| # create_bucket_class_location bucket_name: secondary_bucket_name | ||
| # end | ||
| # end | ||
|
|
||
| # secondary_bucket = storage_client.bucket secondary_bucket_name | ||
| # refute_nil secondary_bucket | ||
| # assert_equal location, secondary_bucket.location | ||
| # assert_equal storage_class, secondary_bucket.storage_class | ||
|
|
||
| # # list_buckets | ||
| # out, _err = capture_io do | ||
| # list_buckets | ||
| # end | ||
|
|
||
| # assert_includes out, "ruby-storage-samples-" | ||
|
|
||
| # # get_bucket_metadata | ||
| # out, _err = capture_io do | ||
| # get_bucket_metadata bucket_name: bucket_name | ||
| # end | ||
|
|
||
| # assert_includes out, bucket_name | ||
|
|
||
| # # delete_bucket | ||
| # assert_output "Deleted bucket: #{bucket_name}\n" do | ||
| # delete_bucket bucket_name: bucket_name | ||
| # end | ||
|
|
||
|
|
||
| # refute storage_client.bucket bucket_name | ||
|
|
||
| # delete_bucket_helper bucket_name | ||
| # delete_bucket_helper secondary_bucket_name | ||
| # end | ||
| # end |
There was a problem hiding this comment.
Large sections of existing tests (bucket lifecycle, storage_create_bucket_dual_region, storage_create_bucket_hierarchical_namespace) have been commented out. This is problematic as it disables existing test coverage. Please ensure that all existing tests remain active and functional. If these tests are temporarily disabled for development, they should be re-enabled before merging.
|
|
||
| delete_bucket_helper bucket_name | ||
| after :all do | ||
| binding.pry |
There was a problem hiding this comment.
The binding.pry statement is a debugging tool and should not be committed to the repository. Please remove it.
# delete_bucket_helper @bucket_name| delete_bucket_helper bucket_name | ||
| after :all do | ||
| binding.pry | ||
| # delete_bucket_helper @bucket_name |
There was a problem hiding this comment.
The delete_bucket_helper @bucket_name call is commented out in the after :all block. This means that buckets created during the test run will not be cleaned up, potentially leading to resource leaks and issues in subsequent test runs. Please uncomment this line to ensure proper resource cleanup.
delete_bucket_helper @bucket_name| # it "creates IP filter enabled bucket" do | ||
| # expected = "Created bucket #{@bucket_name} with IP filter enabled.\n" | ||
| # retry_resource_exhaustion do | ||
| # assert_output expected do | ||
| # create_bucket_with_ip_filter bucket_name: @bucket_name | ||
| # end | ||
| # end | ||
| # end | ||
|
|
||
| # it "removes IP filter of an existing bucket" do | ||
| # expected = "Updated IP filter for bucket #{@bucket_name}.\n" | ||
| # retry_resource_exhaustion do | ||
| # assert_output expected do | ||
| # removes_bucket_ip_filter bucket_name: @bucket_name | ||
| # end | ||
| # end | ||
| # end |
There was a problem hiding this comment.
The new tests for storage_create_bucket_ip_filter and removes_bucket_ip_filter are commented out. For this feature to be properly tested and verified, these tests need to be uncommented and made functional.
| # require "google/cloud/iam" | ||
| # Initialize the IAM Admin Client (Not Credentials Client) | ||
| # iam_client = Google::Cloud::Iam::Service.new | ||
| client = Google::Cloud::ResourceManager::V3::Projects::Client.new |
There was a problem hiding this comment.
The client = Google::Cloud::ResourceManager::V3::Projects::Client.new line is part of the IAM-related code that should be removed from this sample. It's an unnecessary dependency and complexity for a bucket IP filter update.
| puts "Granted permissions: #{response.permissions}" | ||
|
|
||
| response.permissions | ||
| binding.pry |
There was a problem hiding this comment.
The binding.pry statement is a debugging tool and should not be committed to the repository. Please remove it.
| allowed_ip_cidr_ranges: [ | ||
| "0.0.0.0/0", "::/0" | ||
| ] |
There was a problem hiding this comment.
The allowed_ip_cidr_ranges are set to "0.0.0.0/0" and "::/0". While this might be for demonstration purposes, it effectively allows all IP addresses, meaning no actual filtering is applied. For a sample demonstrating IP filtering, it would be more illustrative to use specific, non-global IP ranges (e.g., "192.168.1.0/24") to show how the filter works in practice. Please consider updating this to a more representative example of IP filtering.
allowed_ip_cidr_ranges: [
"192.168.1.0/24"
]|
|
||
|
|
||
| project_id = "storage-sdk-vendor" | ||
| role_id = "GcsIpExemptRole" |
There was a problem hiding this comment.
The role_id variable is defined but not used in this function. This indicates dead code and should be removed.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
bundle exec rake ciin the gem subdirectory.closes: #<issue_number_goes_here>