Adds IP Filter feature support to google_storage_bucket resource. #12945
Conversation
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 118 Click here to see the affected service packages
Action takenFound 105 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 118 Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 118 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 118 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
…cle within bucket resource.
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Errors
|
Tests analyticsTotal tests: 0 Click here to see the affected service packages
🔴 Tests were added that are skipped in VCR:
View the build log |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 118 Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
@roaks3 can you please continue your review on this PR? |
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @trodge, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
| @@ -963,7 +1050,7 @@ func resourceStorageBucketRead(d *schema.ResourceData, meta interface{}) error { | |||
|
|
|||
| // There seems to be some eventual consistency errors in some cases, so we want to check a few times | |||
| // to make sure it exists before moving on | |||
| res, err := config.NewStorageClient(userAgent).Buckets.Get(bucket).Do() | |||
| res, err := config.NewStorageClient(userAgent).Buckets.Get(bucket).Projection("full").Do() | |||
There was a problem hiding this comment.
I can't remember if this came up on the previous PR, but to confirm, this won't change the behavior for existing bucket users?
There was a problem hiding this comment.
So as per this documentation: https://cloud.google.com/storage/docs/json_api/v1/buckets/get, projection is an optional query parameter which can have two values: full and noAcl. The default is noAcl which also excludes ipFilter property from the API response. Now to include IP filter in the response we need to use full projection which also adds owner, acl and defaultObjectAcl fields. These extra fields are not being parsed by flatteners available in google_storage_bucket resource but they are available in google_storage_bucket_acl. Using full projection should not break existing user script as we are not going to parse additional fields coming from the API response. I will still check with Feature team on this if they have plans to add IP filter in noAcl projection. I will update on this PR.
There was a problem hiding this comment.
Ok yea, seems worth checking if we can just leave it as noAcl, but what you described does sound safe. I would consider this non-blocking, but let me know when you hear back.
This PR enables users to specify IP Filtering configuration within
google_storage_bucketresource.Original PR: #12576
Authors: @translucens, @kautikdk