samples(Storage): Add samples and tests for Bucket IP filter#22
samples(Storage): Add samples and tests for Bucket IP filter#22mahendra-google wants to merge 2 commits into
Conversation
![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 new C# code samples and corresponding tests for Google Cloud Storage bucket IP filter configurations, including functionality to create buckets with specific IP CIDR ranges and retrieve those settings. Feedback indicates that the ListBucketIpFilterTest needs to register created buckets for cleanup and include a sleep call to ensure consistency. Additionally, the XML documentation in ListBucketIpFilter.cs requires correction to properly nest the tag outside of the
| var listIpFilter = new ListBucketIpFiltersSample(); | ||
|
|
||
| var bucketName = _fixture.GenerateBucketName(); | ||
| createIpFilter.CreateBucketWithIpFilter(_fixture.ProjectId, bucketName); |
There was a problem hiding this comment.
The bucket created in this test is not registered for cleanup in _fixture.TempBucketNames, which will lead to orphaned buckets in the test project. Additionally, a call to SleepAfterBucketCreateUpdateDelete() is missing after bucket creation to ensure eventual consistency and avoid rate limiting issues, consistent with other tests in this suite.
var bucket = createIpFilter.CreateBucketWithIpFilter(_fixture.ProjectId, bucketName);
_fixture.SleepAfterBucketCreateUpdateDelete();
_fixture.TempBucketNames.Add(bucket.Name);| /// <summary> | ||
| /// Retrieves the IP Filter configuration for the bucket. | ||
| /// <param name="bucketName">The name of the bucket.</param> | ||
| /// </summary> |
There was a problem hiding this comment.
The <param> tag is incorrectly nested inside the <summary> tag. According to standard C# XML documentation practices, <param> should be a top-level tag within the documentation comment.
/// <summary>
/// Retrieves the IP Filter configuration for the bucket.
/// </summary>
/// <param name="bucketName">The name of the bucket.</param>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of C# code samples and unit tests for managing Google Cloud Storage Bucket IP filters, covering operations such as creation, retrieval, listing, updating, disabling, and bypassing filter rules. The review feedback highlights several areas for improvement: specifically, adding defensive null checks in the delete and update samples to prevent potential NullReferenceException errors, replacing hardcoded VPC network names with placeholders for better sample consistency, and refining console output to ensure VPC network details are printed correctly instead of their type names.
| var bucket = storage.GetBucket(bucketName); | ||
| bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Remove(ipAddress); | ||
| storage.UpdateBucket(bucket); | ||
| Console.WriteLine($"IP Address ({ipAddress}) from Allowed CIDR Ranges has been deleted from the bucket: {bucketName}"); |
There was a problem hiding this comment.
The code lacks defensive checks for IpFilter, PublicNetworkSource, and AllowedIpCidrRanges. If any of these are null, a NullReferenceException will occur. It is safer to verify these are not null before attempting to remove the IP address.
var bucket = storage.GetBucket(bucketName);
if (bucket.IpFilter?.PublicNetworkSource?.AllowedIpCidrRanges == null)
{
Console.WriteLine($"Bucket {bucketName} has no IP Filter CIDR ranges to delete.");
return;
}
bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Remove(ipAddress);
storage.UpdateBucket(bucket);
Console.WriteLine($"IP Address ({ipAddress}) from Allowed CIDR Ranges has been deleted from the bucket: {bucketName}");| if (bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges == null) | ||
| { | ||
| bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges = new List<string>(); | ||
| } |
There was a problem hiding this comment.
If bucket.IpFilter is not null but PublicNetworkSource is null, accessing bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges will throw a NullReferenceException. An additional check for PublicNetworkSource is recommended.
if (bucket.IpFilter.PublicNetworkSource == null)
{
bucket.IpFilter.PublicNetworkSource = new Bucket.IpFilterData.PublicNetworkSourceData();
}
if (bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges == null)
{
bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges = new List<string>();
}| { | ||
| new Bucket.IpFilterData.VpcNetworkSourcesData | ||
| { | ||
| Network = $"projects/{projectId}/global/networks/my-vpc-network", |
There was a problem hiding this comment.
The VPC network name is hardcoded as 'my-vpc-network'. For consistency with other parameters in this sample (like projectId and bucketName), it should use a placeholder or a variable.
Network = $"projects/{projectId}/global/networks/your-vpc-network",| foreach (var vpcNetwork in bucket.IpFilter.VpcNetworkSources) | ||
| { | ||
| Console.WriteLine($"- {vpcNetwork}"); | ||
| } |
There was a problem hiding this comment.
Printing the vpcNetwork object directly will likely output the type name instead of the network path. It is better to print the Network property and its associated CIDR ranges to provide useful output.
foreach (var vpcNetwork in bucket.IpFilter.VpcNetworkSources)
{
Console.WriteLine($"- Network: {vpcNetwork.Network}");
if (vpcNetwork.AllowedIpCidrRanges != null)
{
Console.WriteLine($" Allowed CIDR Ranges: {string.Join(", ", vpcNetwork.AllowedIpCidrRanges)}");
}
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several C# sample classes and integration tests for managing Google Cloud Storage bucket IP filtering, including creating, updating, retrieving, listing, disabling, and deleting IP filter configurations. The feedback suggests materializing the lazy enumerable in the bucket listing sample to avoid redundant API calls, and adding checks to prevent duplicate IP ranges from being added to the public and VPC CIDR lists during updates.
| public IEnumerable<Bucket> ListBucketWithIpFilter(string projectId = "your-project-id") | ||
| { | ||
| var storage = StorageClient.Create(); | ||
| IEnumerable<Bucket> buckets = storage.ListBuckets(projectId).Where(b => b.IpFilter != null); |
There was a problem hiding this comment.
The storage.ListBuckets(projectId) method returns a lazy enumerable that performs remote API calls. Because buckets is iterated once in the foreach loop and then returned to the caller (who iterates it again, e.g., in the integration test via FirstOrDefault), this causes multiple redundant API calls to Google Cloud Storage.\n\nMaterializing the query using .ToList() avoids this double-enumeration performance penalty.
var buckets = storage.ListBuckets(projectId).Where(b => b.IpFilter != null).ToList();| if (bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges == null) | ||
| { | ||
| bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges = new List<string>(); | ||
| } | ||
| bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Add(publicRange); |
There was a problem hiding this comment.
To prevent adding duplicate IP ranges to the allowed public CIDR ranges list, check if the range already exists before adding it.
if (bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges == null)\n {\n bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges = new List<string>();\n }\n if (!bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Contains(publicRange))\n {\n bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Add(publicRange);\n }| if (existingVpcNetwork != null) | ||
| { | ||
| if (existingVpcNetwork.AllowedIpCidrRanges == null) | ||
| { | ||
| existingVpcNetwork.AllowedIpCidrRanges = new List<string>(); | ||
| } | ||
| existingVpcNetwork.AllowedIpCidrRanges.Add(vpcRange); | ||
| } |
There was a problem hiding this comment.
To prevent adding duplicate IP ranges to the allowed VPC CIDR ranges list, check if the range already exists before adding it.
if (existingVpcNetwork != null)\n {\n if (existingVpcNetwork.AllowedIpCidrRanges == null)\n {\n existingVpcNetwork.AllowedIpCidrRanges = new List<string>();\n }\n if (!existingVpcNetwork.AllowedIpCidrRanges.Contains(vpcRange))\n {\n existingVpcNetwork.AllowedIpCidrRanges.Add(vpcRange);\n }\n }
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several new code samples and corresponding unit tests demonstrating Google Cloud Storage IP filtering capabilities, including creating, updating, getting, listing, disabling, deleting, and bypassing IP filters on buckets. The feedback highlights a duplicate region tag conflict in UpdateBucketIpFilter.cs and a logical/performance issue in ListBucketsWithIpFilter.cs where deferred execution of bucket listing leads to redundant API calls and incorrect filtering.
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| // [START storage_enable_ip_filtering] |
There was a problem hiding this comment.
The region tag storage_enable_ip_filtering is already used in CreateBucketWithIpFilter.cs. Reusing the same region tag in multiple files within the same language/directory will cause conflicts and build failures in the Google Cloud documentation publishing pipeline. Please use a unique region tag for this sample, such as storage_update_bucket_ip_filtering.
// [START storage_update_bucket_ip_filtering]| return updatedBucket; | ||
| } | ||
| } | ||
| // [END storage_enable_ip_filtering] |
There was a problem hiding this comment.
Please update the closing region tag to match the unique region tag storage_update_bucket_ip_filtering.
// [END storage_update_bucket_ip_filtering]| public IEnumerable<Bucket> ListBucketWithIpFilter(string projectId = "your-project-id") | ||
| { | ||
| var storage = StorageClient.Create(); | ||
| IEnumerable<Bucket> buckets = storage.ListBuckets(projectId).Where(b => b.IpFilter != null); |
There was a problem hiding this comment.
The method's XML documentation states it "Lists all buckets including their IP filtering status", and line 38 attempts to handle the case where IpFilter is null by falling back to "Not Configured". However, the .Where(b => b.IpFilter != null) filter on line 32 excludes all buckets without an IP filter, making the fallback unreachable and failing to list all buckets.\n\nAdditionally, storage.ListBuckets(projectId) returns a deferred PagedEnumerable. By not materializing it, iterating over buckets in the foreach loop and then returning it for the caller (or tests) to iterate again will trigger redundant API calls to GCS. Materializing it with .ToList() resolves both issues.
List<Bucket> buckets = storage.ListBuckets(projectId).ToList();
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces sample code and corresponding tests for managing IP filtering configurations on Google Cloud Storage buckets, including creating, updating, disabling, getting, listing, and deleting IP filter rules, as well as bypassing filters for service agents and cross-org VPCs. The review feedback highlights potential runtime issues where directly modifying collections returned by the client library (such as AllowedIpCidrRanges and VpcNetworkSources) could throw a NotSupportedException if they are read-only or arrays. Additionally, it is recommended to avoid eagerly loading all buckets into memory with .ToList() when listing buckets to prevent performance degradation in projects with a large number of buckets.
| bool removed = bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Remove(ipAddress); | ||
| if (!removed) | ||
| { | ||
| Console.WriteLine($"IP Address ({ipAddress}) was not found in the Allowed CIDR ranges."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Modifying the AllowedIpCidrRanges collection directly using .Remove() can throw a NotSupportedException if the underlying collection returned by the client library is read-only or an array. It is safer to copy the collection to a new List<string>, perform the removal, and then reassign it.
var ranges = new List<string>(bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges);
if (!ranges.Remove(ipAddress))
{
Console.WriteLine($"IP Address ({ipAddress}) was not found in the Allowed CIDR ranges.");
return;
}
bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges = ranges;| if (bucket.IpFilter == null) | ||
| { | ||
| bucket.IpFilter = new Bucket.IpFilterData | ||
| { | ||
| Mode = "Enabled", | ||
| PublicNetworkSource = new Bucket.IpFilterData.PublicNetworkSourceData | ||
| { | ||
| AllowedIpCidrRanges = new List<string>() | ||
| }, | ||
| VpcNetworkSources = new List<Bucket.IpFilterData.VpcNetworkSourcesData>() | ||
| }; | ||
| } | ||
|
|
||
| if (bucket.IpFilter.PublicNetworkSource == null) | ||
| { | ||
| bucket.IpFilter.PublicNetworkSource = new Bucket.IpFilterData.PublicNetworkSourceData(); | ||
| } | ||
| if (bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges == null) | ||
| { | ||
| bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges = new List<string>(); | ||
| } | ||
| if (!bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Contains(publicRange)) | ||
| { | ||
| bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Add(publicRange); | ||
| } | ||
| if (bucket.IpFilter.VpcNetworkSources == null) | ||
| { | ||
| bucket.IpFilter.VpcNetworkSources = new List<Bucket.IpFilterData.VpcNetworkSourcesData>(); | ||
| } | ||
|
|
||
| string networkName = $"projects/{projectId}/global/networks/{vpcNetwork}"; | ||
| var existingVpcNetwork = bucket.IpFilter.VpcNetworkSources.FirstOrDefault(v => v.Network == networkName); | ||
|
|
||
| if (existingVpcNetwork != null) | ||
| { | ||
| existingVpcNetwork.AllowedIpCidrRanges ??= new List<string>(); | ||
| if (!existingVpcNetwork.AllowedIpCidrRanges.Contains(vpcRange)) | ||
| { | ||
| existingVpcNetwork.AllowedIpCidrRanges.Add(vpcRange); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| bucket.IpFilter.VpcNetworkSources.Add(new Bucket.IpFilterData.VpcNetworkSourcesData | ||
| { | ||
| Network = networkName, | ||
| AllowedIpCidrRanges = new List<string> { vpcRange } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Directly modifying or adding items to the AllowedIpCidrRanges and VpcNetworkSources collections can throw a NotSupportedException if the underlying collections returned by the client library are read-only or arrays.
Additionally, the initialization and update logic can be simplified and made much more robust by using the null-coalescing assignment operator (??=) and copying the collections to new lists before modification.
bucket.IpFilter ??= new Bucket.IpFilterData { Mode = "Enabled" };
bucket.IpFilter.PublicNetworkSource ??= new Bucket.IpFilterData.PublicNetworkSourceData();
var publicRanges = bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges?.ToList() ?? new List<string>();
if (!publicRanges.Contains(publicRange))
{
publicRanges.Add(publicRange);
bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges = publicRanges;
}
var vpcSources = bucket.IpFilter.VpcNetworkSources?.ToList() ?? new List<Bucket.IpFilterData.VpcNetworkSourcesData>();
string networkName = $"projects/{projectId}/global/networks/{vpcNetwork}";
var existingVpcNetwork = vpcSources.FirstOrDefault(v => v.Network == networkName);
if (existingVpcNetwork != null)
{
var vpcRanges = existingVpcNetwork.AllowedIpCidrRanges?.ToList() ?? new List<string>();
if (!vpcRanges.Contains(vpcRange))
{
vpcRanges.Add(vpcRange);
existingVpcNetwork.AllowedIpCidrRanges = vpcRanges;
}
}
else
{
vpcSources.Add(new Bucket.IpFilterData.VpcNetworkSourcesData
{
Network = networkName,
AllowedIpCidrRanges = new List<string> { vpcRange }
});
}
bucket.IpFilter.VpcNetworkSources = vpcSources;| public IEnumerable<Bucket> ListBucketsWithIpFilterStatus(string projectId = "your-project-id") | ||
| { | ||
| var storage = StorageClient.Create(); | ||
| IEnumerable<Bucket> buckets = storage.ListBuckets(projectId).ToList(); |
There was a problem hiding this comment.
Calling .ToList() eagerly loads all buckets in the project into memory. If the project contains a large number of buckets, this can cause significant performance degradation and high memory usage. Since the method returns IEnumerable<Bucket> and only iterates over them, you can stream the buckets lazily by removing .ToList().
var buckets = storage.ListBuckets(projectId);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new sample classes and corresponding unit tests for managing IP filtering configurations on Google Cloud Storage buckets, including enabling, disabling, retrieving, listing, and deleting IP filter rules. The review feedback correctly identifies a critical bug in EnableBucketIpFilter where the bucket's IP filter mode is not updated to "Enabled" if an IP filter configuration already exists. Additionally, the feedback points out that the unit test EnableBucketIpFilterTest incorrectly asserts the mode to be "Disabled" to match this buggy behavior, and both should be updated to correctly handle and assert the "Enabled" state.
| if (bucket.IpFilter == null) | ||
| { | ||
| bucket.IpFilter = new Bucket.IpFilterData | ||
| { | ||
| Mode = "Enabled", | ||
| PublicNetworkSource = new Bucket.IpFilterData.PublicNetworkSourceData | ||
| { | ||
| AllowedIpCidrRanges = new List<string>() | ||
| }, | ||
| VpcNetworkSources = new List<Bucket.IpFilterData.VpcNetworkSourcesData>() | ||
| }; | ||
| } |
There was a problem hiding this comment.
In EnableBucketIpFilter, if bucket.IpFilter is already non-null (for example, if it was previously configured but disabled), the Mode is never updated to "Enabled". This means the method will fail to actually enable IP filtering for buckets that already have an existing IP filter configuration. Setting bucket.IpFilter.Mode = \"Enabled\" unconditionally (after ensuring bucket.IpFilter is initialized) fixes this issue.
if (bucket.IpFilter == null)
{
bucket.IpFilter = new Bucket.IpFilterData
{
PublicNetworkSource = new Bucket.IpFilterData.PublicNetworkSourceData
{
AllowedIpCidrRanges = new List<string>()
},
VpcNetworkSources = new List<Bucket.IpFilterData.VpcNetworkSourcesData>()
};
}
bucket.IpFilter.Mode = "Enabled";| var updatedBucket = updateSample.EnableBucketIpFilter(projectId, bucketName, newPublicRange, newVpcRange); | ||
| Assert.NotNull(updatedBucket.IpFilter); | ||
| Assert.Equal("Disabled", updatedBucket.IpFilter.Mode); |
There was a problem hiding this comment.
The test currently asserts that the bucket's IP filter mode is "Disabled" after calling EnableBucketIpFilter. This assertion was likely adjusted to match the buggy behavior of the sample implementation. Once the sample is fixed to correctly set the mode to "Enabled", this assertion should be updated to expect "Enabled".
var updatedBucket = updateSample.EnableBucketIpFilter(projectId, bucketName, newPublicRange, newVpcRange);
Assert.NotNull(updatedBucket.IpFilter);
Assert.Equal("Enabled", updatedBucket.IpFilter.Mode);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new C# samples and integration tests for managing IP filtering configurations on Google Cloud Storage buckets, including enabling, disabling, retrieving, listing, and deleting IP filter rules. The review feedback suggests two key improvements: removing .ToList() in ListBucketsWithIpFilterStatus to leverage lazy loading and avoid high memory consumption, and using conditional access defensively on updatedBucket.IpFilter in EnableBucketIpFilter to prevent potential NullReferenceExceptions.
| public IEnumerable<Bucket> ListBucketsWithIpFilterStatus(string projectId = "your-project-id") | ||
| { | ||
| var storage = StorageClient.Create(); | ||
| IEnumerable<Bucket> buckets = storage.ListBuckets(projectId).ToList(); |
There was a problem hiding this comment.
Calling .ToList() on the result of ListBuckets eagerly loads all buckets in the project into memory. For projects with a large number of buckets, this can lead to high memory consumption and performance degradation. Since the method returns IEnumerable<Bucket> and only iterates over them, you can safely remove .ToList() to leverage lazy loading.
var buckets = storage.ListBuckets(projectId);| var currentPublicRanges = updatedBucket.IpFilter.PublicNetworkSource?.AllowedIpCidrRanges != null | ||
| ? string.Join(", ", updatedBucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges) | ||
| : "None"; | ||
|
|
||
| var currentVpcRanges = updatedBucket.IpFilter.VpcNetworkSources != null && updatedBucket.IpFilter.VpcNetworkSources.Any() | ||
| ? string.Join(", ", updatedBucket.IpFilter.VpcNetworkSources.SelectMany(v => v.AllowedIpCidrRanges ?? new List<string>())) | ||
| : "None"; |
There was a problem hiding this comment.
To prevent a potential NullReferenceException, use conditional access (?.) on updatedBucket.IpFilter when retrieving the current public and VPC network ranges. Although IpFilter was just enabled, defensive programming is recommended for sample code to handle unexpected API responses gracefully.
var currentPublicRanges = updatedBucket.IpFilter?.PublicNetworkSource?.AllowedIpCidrRanges != null
? string.Join(", ", updatedBucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges)
: "None";
var currentVpcRanges = updatedBucket.IpFilter?.VpcNetworkSources != null && updatedBucket.IpFilter.VpcNetworkSources.Any()
? string.Join(", ", updatedBucket.IpFilter.VpcNetworkSources.SelectMany(v => v.AllowedIpCidrRanges ?? new List<string>()))
: "None";
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several new C# samples and tests for managing IP filtering configurations on Google Cloud Storage buckets, including enabling, disabling, getting, listing, and deleting IP filter rules. The StorageFixture test helper was also updated to support IP filter configurations. The review feedback suggests defensively using the null-conditional operator (?.) when accessing updatedBucket.IpFilter in both EnableBucketIpFilter.cs and DisableBucketIpFilter.cs to prevent potential NullReferenceExceptions if the server returns a bucket with a null IP filter configuration.
| var currentPublicRanges = updatedBucket.IpFilter.PublicNetworkSource?.AllowedIpCidrRanges != null | ||
| ? string.Join(", ", updatedBucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges) | ||
| : "None"; | ||
|
|
||
| var currentVpcRanges = updatedBucket.IpFilter.VpcNetworkSources != null && updatedBucket.IpFilter.VpcNetworkSources.Any() | ||
| ? string.Join(", ", updatedBucket.IpFilter.VpcNetworkSources.SelectMany(v => v.AllowedIpCidrRanges ?? new List<string>())) | ||
| : "None"; |
There was a problem hiding this comment.
Defensively use the null-conditional operator (?.) when accessing updatedBucket.IpFilter to prevent a potential NullReferenceException if the server returns a bucket with a null IpFilter configuration.
var currentPublicRanges = updatedBucket.IpFilter?.PublicNetworkSource?.AllowedIpCidrRanges != null
? string.Join(", ", updatedBucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges)
: "None";
var currentVpcRanges = updatedBucket.IpFilter?.VpcNetworkSources != null && updatedBucket.IpFilter.VpcNetworkSources.Any()
? string.Join(", ", updatedBucket.IpFilter.VpcNetworkSources.SelectMany(v => v.AllowedIpCidrRanges ?? new List<string>()))
: "None";| var updatedBucket = storage.UpdateBucket(bucket); | ||
| Console.WriteLine($"IP filtering has been {updatedBucket.IpFilter.Mode} for the Bucket: {bucketName}"); | ||
| return updatedBucket; |
There was a problem hiding this comment.
Defensively use the null-conditional operator (?.) when accessing updatedBucket.IpFilter to prevent a potential NullReferenceException if the server returns a bucket with a null IpFilter configuration.
var updatedBucket = storage.UpdateBucket(bucket);
Console.WriteLine($"IP filtering has been {updatedBucket.IpFilter?.Mode ?? "Disabled"} for the Bucket: {bucketName}");
return updatedBucket;
mahendra-google
force-pushed
the
bucket-ip-filter
branch
from
529edf2 to
d614041
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and tests for managing IP filtering configurations on Google Cloud Storage buckets, including enabling, disabling, retrieving, listing, and deleting IP filter rules. Feedback on the changes highlights a bug in EnableBucketIpFilter.cs where the VPC network source is not added during first-time initialization when bucket.IpFilter is null, alongside potential NullReferenceException risks. Additionally, the corresponding test in EnableBucketIpFilterTest.cs should be updated to create a bucket without pre-configured IP filters to properly test this initialization path.
| if (!bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Contains(publicRange)) | ||
| { | ||
| bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Add(publicRange); | ||
| } | ||
|
|
||
| string networkName = $"projects/{projectId}/global/networks/{vpcNetwork}"; | ||
| var existingVpcNetwork = bucket.IpFilter.VpcNetworkSources.FirstOrDefault(v => v.Network == networkName); | ||
|
|
||
| if (existingVpcNetwork != null) | ||
| { | ||
| existingVpcNetwork.AllowedIpCidrRanges ??= new List<string>(); | ||
| if (!existingVpcNetwork.AllowedIpCidrRanges.Contains(vpcRange)) | ||
| { | ||
| existingVpcNetwork.AllowedIpCidrRanges.Add(vpcRange); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a bug in the first-time initialization of the IP filtering configuration. If the bucket does not have an existing IP filter configuration (bucket.IpFilter is null), bucket.IpFilter is initialized with an empty VpcNetworkSources list. Consequently, existingVpcNetwork will be null, and because there is no else block to handle this, the requested VPC network and its CIDR range are never added to the bucket configuration.
Additionally, if bucket.IpFilter is not null but PublicNetworkSource or VpcNetworkSources are null, a NullReferenceException will be thrown.
We should safely initialize these properties and add an else block to handle adding a new VPC network source when it does not already exist.
bucket.IpFilter.PublicNetworkSource ??= new Bucket.IpFilterData.PublicNetworkSourceData();
bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges ??= new List<string>();
if (!bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Contains(publicRange))
{
bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Add(publicRange);
}
bucket.IpFilter.VpcNetworkSources ??= new List<Bucket.IpFilterData.VpcNetworkSourcesData>();
string networkName = $"projects/{projectId}/global/networks/{vpcNetwork}";
var existingVpcNetwork = bucket.IpFilter.VpcNetworkSources.FirstOrDefault(v => v.Network == networkName);
if (existingVpcNetwork != null)
{
existingVpcNetwork.AllowedIpCidrRanges ??= new List<string>();
if (!existingVpcNetwork.AllowedIpCidrRanges.Contains(vpcRange))
{
existingVpcNetwork.AllowedIpCidrRanges.Add(vpcRange);
}
}
else
{
bucket.IpFilter.VpcNetworkSources.Add(new Bucket.IpFilterData.VpcNetworkSourcesData
{
Network = networkName,
AllowedIpCidrRanges = new List<string> { vpcRange }
});
}| var projectId = _fixture.ProjectId; | ||
| string newPublicRange = "192.0.2.0/24"; | ||
| string newVpcRange = "10.0.0.0/24"; | ||
| _fixture.CreateBucket(bucketName, multiVersion: false, ipFilter: true, registerForDeletion: true); |
There was a problem hiding this comment.
The test currently creates the bucket with ipFilter: true, which pre-configures the IP filter and the default VPC network. This masks the bug where first-time initialization of IP filtering fails to add the VPC network source.
Changing ipFilter: true to ipFilter: false will ensure that the sample is tested against a bucket with no pre-existing IP filter configuration, which is the primary use case for enabling IP filtering.
_fixture.CreateBucket(bucketName, multiVersion: false, ipFilter: false, registerForDeletion: true);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new code samples and corresponding unit tests for managing Google Cloud Storage bucket IP filtering configurations, including enabling, disabling, retrieving, listing, and deleting IP filter rules. The review feedback highlights opportunities to improve null safety and robustness: specifically, adding null-conditional operators in EnableBucketIpFilter.cs to prevent potential NullReferenceExceptions when printing current ranges, and adding explicit Assert.NotNull checks in DeleteBucketIpFilterTest.cs to ensure clear test failure diagnostics rather than unexpected null reference errors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| var currentPublicRanges = updatedBucket.IpFilter.PublicNetworkSource?.AllowedIpCidrRanges != null | ||
| ? string.Join(", ", updatedBucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges) | ||
| : "None"; | ||
|
|
||
| var currentVpcRanges = updatedBucket.IpFilter.VpcNetworkSources != null && updatedBucket.IpFilter.VpcNetworkSources.Any() | ||
| ? string.Join(", ", updatedBucket.IpFilter.VpcNetworkSources.SelectMany(v => v.AllowedIpCidrRanges ?? new List<string>())) | ||
| : "None"; |
There was a problem hiding this comment.
If updatedBucket.IpFilter is null, accessing updatedBucket.IpFilter.PublicNetworkSource or updatedBucket.IpFilter.VpcNetworkSources will throw a NullReferenceException. Adding null-conditional operators ensures defensive programming and null safety.
var currentPublicRanges = updatedBucket.IpFilter?.PublicNetworkSource?.AllowedIpCidrRanges != null
? string.Join(", ", updatedBucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges)
: "None";
var currentVpcRanges = updatedBucket.IpFilter?.VpcNetworkSources != null && updatedBucket.IpFilter.VpcNetworkSources.Any()
? string.Join(", ", updatedBucket.IpFilter.VpcNetworkSources.SelectMany(v => v.AllowedIpCidrRanges ?? new List<string>()))
: "None";| var updatedBucket = storage.GetBucket(bucketName); | ||
| Assert.DoesNotContain(targetPublicIp, updatedBucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges); | ||
| Assert.False(updatedBucket.IpFilter?.VpcNetworkSources?.Any(v => v.Network == targetVpc) ?? false); |
There was a problem hiding this comment.
If updatedBucket.IpFilter or updatedBucket.IpFilter.PublicNetworkSource is null, the test will fail with a NullReferenceException instead of a clear assertion failure. Adding explicit Assert.NotNull checks improves test clarity and diagnostic messages.
var updatedBucket = storage.GetBucket(bucketName);
Assert.NotNull(updatedBucket.IpFilter);
Assert.NotNull(updatedBucket.IpFilter.PublicNetworkSource);
Assert.DoesNotContain(targetPublicIp, updatedBucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges);
Assert.False(updatedBucket.IpFilter.VpcNetworkSources?.Any(v => v.Network == targetVpc) ?? false);
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces C# code samples and corresponding tests for managing Google Cloud Storage bucket IP filtering, including enabling, disabling, retrieving, listing, and deleting IP filter rules. Feedback on the EnableBucketIpFilter sample highlights a security concern where existing custom settings for AllowAllServiceAgentAccess and AllowCrossOrgVpcs are unconditionally overwritten to true when updating a configuration, and suggests using null-coalescing assignment instead. Additionally, it is recommended to support both short network names and fully qualified resource paths for the VPC network parameter to make the sample more robust.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| else | ||
| { | ||
| bucket.IpFilter.Mode = "Enabled"; | ||
| bucket.IpFilter.AllowAllServiceAgentAccess = true; | ||
| bucket.IpFilter.AllowCrossOrgVpcs = true; | ||
| } |
There was a problem hiding this comment.
Unconditionally setting AllowAllServiceAgentAccess and AllowCrossOrgVpcs to true when updating an existing IpFilter configuration will overwrite any custom settings previously configured by the user (e.g., if they had set them to false for tighter security). To prevent silently making the bucket more permissive, use the null-coalescing assignment operator (??=) to only default them to true if they are not already set.
else
{
bucket.IpFilter.Mode = "Enabled";
bucket.IpFilter.AllowAllServiceAgentAccess ??= true;
bucket.IpFilter.AllowCrossOrgVpcs ??= true;
}| } | ||
|
|
||
| bucket.IpFilter.VpcNetworkSources ??= new List<Bucket.IpFilterData.VpcNetworkSourcesData>(); | ||
| string networkName = $"projects/{projectId}/global/networks/{vpcNetwork}"; |
There was a problem hiding this comment.
To make the sample more robust and user-friendly, check if vpcNetwork is already a fully qualified resource path (starting with projects/). This allows users to pass either the short network name (like "default") or the full resource path without causing a malformed network string.
string networkName = vpcNetwork.StartsWith("projects/") ? vpcNetwork : $"projects/{projectId}/global/networks/{vpcNetwork}";
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new C# code samples and corresponding integration tests for managing Google Cloud Storage bucket IP filtering configurations, including enabling, disabling, retrieving, listing, and deleting IP filter rules. The feedback suggests improving the robustness of the EnableBucketIpFilter sample by guarding against null or empty range inputs and handling fully qualified VPC network paths. Additionally, the DisableBucketIpFilterTest should be updated to first enable IP filtering on the bucket to properly test the transition to the disabled state.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| bucket.IpFilter.PublicNetworkSource ??= new Bucket.IpFilterData.PublicNetworkSourceData(); | ||
| bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges ??= new List<string>(); | ||
|
|
||
| if (!bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Contains(publicRange)) | ||
| { | ||
| bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Add(publicRange); | ||
| } | ||
|
|
||
| bucket.IpFilter.VpcNetworkSources ??= new List<Bucket.IpFilterData.VpcNetworkSourcesData>(); | ||
| string networkName = $"projects/{projectId}/global/networks/{vpcNetwork}"; | ||
| var existingVpcNetwork = bucket.IpFilter.VpcNetworkSources.FirstOrDefault(v => v.Network == networkName); | ||
|
|
||
| if (existingVpcNetwork != null) | ||
| { | ||
| existingVpcNetwork.AllowedIpCidrRanges ??= new List<string>(); | ||
| if (!existingVpcNetwork.AllowedIpCidrRanges.Contains(vpcRange)) | ||
| { | ||
| existingVpcNetwork.AllowedIpCidrRanges.Add(vpcRange); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| bucket.IpFilter.VpcNetworkSources.Add(new Bucket.IpFilterData.VpcNetworkSourcesData | ||
| { | ||
| Network = networkName, | ||
| AllowedIpCidrRanges = new List<string> { vpcRange } | ||
| }); | ||
| } |
There was a problem hiding this comment.
To make the sample more robust and reusable, we should guard against null or empty values for publicRange and vpcRange. This allows callers to selectively configure only public IP ranges or only VPC network sources without causing null-reference issues or sending invalid data to the API. Additionally, we should handle cases where the user passes a fully qualified VPC network path (starting with projects/) instead of just a short name, to prevent constructing an invalid nested path.
if (!string.IsNullOrEmpty(publicRange))
{
bucket.IpFilter.PublicNetworkSource ??= new Bucket.IpFilterData.PublicNetworkSourceData();
bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges ??= new List<string>();
if (!bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Contains(publicRange))
{
bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Add(publicRange);
}
}
if (!string.IsNullOrEmpty(vpcRange))
{
bucket.IpFilter.VpcNetworkSources ??= new List<Bucket.IpFilterData.VpcNetworkSourcesData>();
string networkName = vpcNetwork.StartsWith("projects/", StringComparison.OrdinalIgnoreCase)
? vpcNetwork
: $"projects/{projectId}/global/networks/{vpcNetwork}";
var existingVpcNetwork = bucket.IpFilter.VpcNetworkSources.FirstOrDefault(v => v.Network == networkName);
if (existingVpcNetwork != null)
{
existingVpcNetwork.AllowedIpCidrRanges ??= new List<string>();
if (!existingVpcNetwork.AllowedIpCidrRanges.Contains(vpcRange))
{
existingVpcNetwork.AllowedIpCidrRanges.Add(vpcRange);
}
}
else
{
bucket.IpFilter.VpcNetworkSources.Add(new Bucket.IpFilterData.VpcNetworkSourcesData
{
Network = networkName,
AllowedIpCidrRanges = new List<string> { vpcRange }
});
}
}| public void TestDisableBucketIpFilter() | ||
| { | ||
| var disableSample = new DisableBucketIpFilterSample(); | ||
| var bucketName = _fixture.GenerateBucketName(); | ||
| var ipFilteredBucket = _fixture.CreateBucket(bucketName, multiVersion: false, ipFilter: true, registerForDeletion: true); | ||
| var unfilteredBucket = disableSample.DisableBucketIpFilter(bucketName); | ||
| Assert.NotNull(unfilteredBucket.IpFilter); | ||
| Assert.Equal("Disabled", unfilteredBucket.IpFilter.Mode); | ||
| Assert.NotNull(unfilteredBucket.IpFilter.PublicNetworkSource?.AllowedIpCidrRanges); | ||
| Assert.Contains("203.0.113.0/24", unfilteredBucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges); | ||
| } |
There was a problem hiding this comment.
The current test creates a bucket with IP filtering already disabled (ipFilter: true initializes it with Mode = "Disabled" in the fixture). To properly test the functionality of disabling IP filtering, we should first enable it using EnableBucketIpFilterSample and then verify that calling DisableBucketIpFilter successfully transitions the mode to Disabled.
[Fact]
public void TestDisableBucketIpFilter()
{
var disableSample = new DisableBucketIpFilterSample();
var enableSample = new EnableBucketIpFilterSample();
var bucketName = _fixture.GenerateBucketName();
_fixture.CreateBucket(bucketName, multiVersion: false, ipFilter: false, registerForDeletion: true);
// Enable IP filtering first to test the transition to Disabled
enableSample.EnableBucketIpFilter(_fixture.ProjectId, bucketName);
var unfilteredBucket = disableSample.DisableBucketIpFilter(bucketName);
Assert.NotNull(unfilteredBucket.IpFilter);
Assert.Equal("Disabled", unfilteredBucket.IpFilter.Mode);
Assert.NotNull(unfilteredBucket.IpFilter.PublicNetworkSource?.AllowedIpCidrRanges);
Assert.Contains("192.0.2.0/24", unfilteredBucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges);
mahendra-google
requested review from
salilg-eng,
shubhangi-google and
thiyaguk09
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces new samples and tests for managing Google Cloud Storage bucket IP filtering, including enabling, disabling, retrieving, listing, and deleting IP filter rules. The feedback identifies two key issues: first, a logic error in DisableBucketIpFilterTest.cs where a timeout assertion always passes and silently succeeds instead of failing; second, a lack of defensive checks in EnableBucketIpFilter.cs for null or empty IP ranges, which could cause API update failures.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (isPropagationBlocked) | ||
| { | ||
| Assert.True(isPropagationBlocked, "Firewall propagation timeout encountered."); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The assertion Assert.True(isPropagationBlocked, ...) is executed inside the if (isPropagationBlocked) block, meaning isPropagationBlocked is guaranteed to be true. As a result, this assertion will always pass, and the subsequent return; will cause the test to succeed silently even if firewall propagation timed out. If the intention is to fail the test when propagation is blocked, you should use Assert.False(isPropagationBlocked, ...) instead.
Assert.False(isPropagationBlocked, "Firewall propagation timeout encountered.");| bucket.IpFilter.PublicNetworkSource ??= new Bucket.IpFilterData.PublicNetworkSourceData(); | ||
| bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges ??= new List<string>(); | ||
|
|
||
| if (!bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Contains(publicRange)) | ||
| { | ||
| bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Add(publicRange); | ||
| } | ||
|
|
||
| bucket.IpFilter.VpcNetworkSources ??= new List<Bucket.IpFilterData.VpcNetworkSourcesData>(); | ||
| string networkName = $"projects/{projectId}/global/networks/{vpcNetwork}"; | ||
| var existingVpcNetwork = bucket.IpFilter.VpcNetworkSources.FirstOrDefault(v => v.Network == networkName); | ||
|
|
||
| if (existingVpcNetwork != null) | ||
| { | ||
| existingVpcNetwork.AllowedIpCidrRanges ??= new List<string>(); | ||
| if (!existingVpcNetwork.AllowedIpCidrRanges.Contains(vpcRange)) | ||
| { | ||
| existingVpcNetwork.AllowedIpCidrRanges.Add(vpcRange); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| bucket.IpFilter.VpcNetworkSources.Add(new Bucket.IpFilterData.VpcNetworkSourcesData | ||
| { | ||
| Network = networkName, | ||
| AllowedIpCidrRanges = new List<string> { vpcRange } | ||
| }); | ||
| } |
There was a problem hiding this comment.
The method does not guard against null or empty values for publicRange and vpcRange. If a caller passes null or empty strings, it will add them to the allowed CIDR ranges lists, which will cause the subsequent UpdateBucket API call to fail. Adding defensive checks ensures that only valid, non-empty ranges are configured.
if (!string.IsNullOrEmpty(publicRange))
{
bucket.IpFilter.PublicNetworkSource ??= new Bucket.IpFilterData.PublicNetworkSourceData();
bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges ??= new List<string>();
if (!bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Contains(publicRange))
{
bucket.IpFilter.PublicNetworkSource.AllowedIpCidrRanges.Add(publicRange);
}
}
if (!string.IsNullOrEmpty(vpcRange) && !string.IsNullOrEmpty(vpcNetwork))
{
bucket.IpFilter.VpcNetworkSources ??= new List<Bucket.IpFilterData.VpcNetworkSourcesData>();
string networkName = $"projects/{projectId}/global/networks/{vpcNetwork}";
var existingVpcNetwork = bucket.IpFilter.VpcNetworkSources.FirstOrDefault(v => v.Network == networkName);
if (existingVpcNetwork != null)
{
existingVpcNetwork.AllowedIpCidrRanges ??= new List<string>();
if (!existingVpcNetwork.AllowedIpCidrRanges.Contains(vpcRange))
{
existingVpcNetwork.AllowedIpCidrRanges.Add(vpcRange);
}
}
else
{
bucket.IpFilter.VpcNetworkSources.Add(new Bucket.IpFilterData.VpcNetworkSourcesData
{
Network = networkName,
AllowedIpCidrRanges = new List<string> { vpcRange }
});
}
}
mahendra-google
force-pushed
the
bucket-ip-filter
branch
from
9f44694 to
3872154
Compare
2 participants
This PR introduces a set of code samples and corresponding tests for bucket IP filtering .
storage_enable_ip_filtering: Code to create or update IP filtering rules. It handles first-time initialization of the configuration block and appends unique public or VPC CIDR ranges safely without overwriting pre-existing rules.storage_delete_ip_filtering_rules: Code to selectively remove specific CIDR blocks fromPublicNetworkSourceor remove specifiedVpcNetworkSources, ensuring that IP filtering remains active for other public CIDR ranges.storage_disable_ip_filtering: Code to completely disable IP filtering on a targeted bucket.storage_get_ip_filtering: Code to retrieve and log the current active IP rules and access flags for an existing bucket.storage_list_buckets_ip_filtering: Code to iterate through all buckets , outputting a log of their names along with their respective IP filtering status.