samples: changed PAP unspecified to inherited #14141
Conversation
product-auto-label
bot
added
the
samples
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
google-cla
bot
added
the
cla: yes
|
@bajajneha27 Regarding the current acceptance test failures for this PR, I pulled your branch and ran the acceptance tests locally using the same project. I got 45 errors! Almost all of them are of the following three types: Cannot insert legacy ACL 1) Error:
Google::Cloud::Storage::File::storage#test_0027_should copy an existing file, with force_copy_metadata set to true:
Google::Cloud::InvalidArgumentError: invalid: Cannot insert legacy ACL for an object when uniform bucket-level access is enabled. Read more at https://cloud.google.com/storage/docs/uniform-bucket-level-access
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/service.rb:764:in `rescue in execute'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/service.rb:761:in `execute'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/service.rb:351:in `insert_file'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/bucket.rb:1516:in `create_file'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/acceptance/storage/file_test.rb:647:in `block (2 levels) in <top (required)>'FailedPreconditionError: conditionNotMet: Request violates constraint 'constraints/storage.uniformBucketLevelAccess' 5) Error:
Google::Cloud::Storage::Bucket::uniform_bucket_level_access::storage#test_0014_raises when creating new bucket with public_access_prevention set to unexpected value:
Google::Cloud::FailedPreconditionError: conditionNotMet: Request violates constraint 'constraints/storage.uniformBucketLevelAccess'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/service.rb:764:in `rescue in execute'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/service.rb:761:in `execute'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/service.rb:126:in `patch_bucket'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/bucket.rb:2828:in `patch_gapi!'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/bucket.rb:885:in `uniform_bucket_level_access='
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/acceptance/storage/bucket_uniform_bucket_level_access_test.rb:35:in `block (2 levels) in <top (required)>'Cannot use ACL API to update bucket policy when uniform bucket-level access is enabled. 16) Error:
Google::Cloud::Storage::Bucket::uniform_bucket_level_access::storage#test_0013_creates new bucket with public_access_prevention enforced then sets public_access_prevention to enforced:
Google::Cloud::InvalidArgumentError: invalid: Cannot use ACL API to update bucket policy when uniform bucket-level access is enabled. Read more at https://cloud.google.com/storage/docs/uniform-bucket-level-access
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/service.rb:764:in `rescue in execute'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/service.rb:761:in `execute'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/service.rb:126:in `patch_bucket'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/bucket/acl.rb:441:in `update_predefined_acl!'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/lib/google/cloud/storage/bucket/acl.rb:409:in `public!'
/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/acceptance/storage/bucket_uniform_bucket_level_access_test.rb:220:in `block (2 levels) in <top (required)>' |
|
I also tried creating a new bucket manually in this project to see if UBLA is enabled by default. It is: irb(main):002:0> bucket_name = "ublatest321423"
=> "ublatest321423"
irb(main):003:0> bucket = storage.create_bucket(bucket_name)
=>
#<Google::Cloud::Storage::Bucket:0x00007f90456402a8
...
irb(main):004:0> bucket.uniform_bucket_level_access?
=> true
irb(main):005:0> bucket.gapi.iam_configuration
=>
#<Google::Apis::StorageV1::Bucket::IamConfiguration:0x00007f904557a288
@bucket_policy_only=
#<Google::Apis::StorageV1::Bucket::IamConfiguration::BucketPolicyOnly:0x00007f9045578280 @enabled=true, @locked_time=#<DateTime: 2021-12-28T17:17:12+00:00 ((2459577j,62232s,145000000n),+0s,2299161j)>>,
@public_access_prevention="inherited",
@uniform_bucket_level_access=
#<Google::Apis::StorageV1::Bucket::IamConfiguration::UniformBucketLevelAccess:0x00007f904556b120 @enabled=true, @locked_time=#<DateTime: 2021-12-28T17:17:12+00:00 ((2459577j,62232s,145000000n),+0s,2299161j)>>> |
|
I then tried manually disabling UBLA for my newly-created bucket (this is what causes many of the tests to fail at The docs say:
Therefore I'm wondering why I couldn't manually disable UBLA on a new bucket? Is this a backend bug? |
|
I am guessing that the |
@quartzmo Do you know why that might be a problem now when it wasn't before? This change should be purely a naming change. No behavior should have changed at all. |
|
@shaffeeullah I haven't been involved with this library since some time, but it appears to me that newly-created buckets in the CI project now have Is this correct behavior now? |
|
There is a test setup call in bucket_test.rb that ensures that ACLs are reset to private before each test: This test group uses a shared bucket fixture (search for |
|
The TL;DR is that the long-time approach of sharing of a single bucket fixture among many tests is now problematic unless UBLA can be repeatedly enabled and disabled to support using the same bucket in both ACL and UBLA tests. |
|
I tried running the acceptance test on my local and I'm facing different errors altogether: Multiple failures with these error messages. |
|
@bajajneha27 I also see about 16 similar errors when I run acceptance tests using my personal dev project, both against your branch and against master. I think they are unrelated to this PR. However, when running using my personal dev project against your branch, I also see these errors. They are related to this PR: 17) Failure:
Google::Cloud::Storage::Bucket::uniform_bucket_level_access::storage#test_0013_creates new bucket with public_access_prevention enforced then sets public_access_prevention to enforced [/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/acceptance/storage/bucket_uniform_bucket_level_access_test.rb:218]:
Expected false to be truthy.
18) Failure:
Google::Cloud::Storage::Bucket::uniform_bucket_level_access::storage#test_0015_sets public_access_prevention to enforced [/Users/quartzmo/code/google/codez/google-cloud-ruby/google-cloud-storage/acceptance/storage/bucket_uniform_bucket_level_access_test.rb:238]:
Expected: "inherited"
Actual: "unspecified" |
|
@quartzmo Yes. I think those errors are popping up because the project is not whitelisted for this change. |
shaffeeullah
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
@bajajneha27 this project is already on the whitelist. I'm not sure what is causing these failures based on the PR changes. |
it appears to me that newly-created buckets in the CI project now have |
@quartzmo I believe this may be due to internal google restrictions on testing projects. I had to apply for an exemption on my own GCS testing project and make sure it was in the correct folder that has exemptions from these restrictions so that features such as UBLA were not enabled by default. @bajajneha27 can we follow up offline to fix the project permissions? Also, there may be additional issues with your personal project that we can help with. |
|
@tritone Thanks for the background info regarding the testing projects. In order to get this PR to a state in which it can be approved, our Ruby CI project will need to reflect exactly the same backend behavior that will be public when the PR is merged and released. Once we're certain of that in the CI environment, I can advise @bajajneha27 about any changes needed to the tests. (Because of the use of shared buckets in the system tests, they are fairly brittle.) |
|
Okay, good news: we got the policy change approved and enacted. It should now be possible to disable UBLA for helical-zone-771's buckets. |
bajajneha27
added
the
kokoro:force-run
yoshi-kokoro
removed
the
kokoro:force-run
bajajneha27
force-pushed
the
gcs/pap-inherited
branch
from
19dee9d
to
d702551
Compare
All tests have now passed. :) Can we merge the PR now? |
| # | ||
| def public_access_prevention_unspecified? | ||
| def public_access_prevention_inherited? |
There was a problem hiding this comment.
Instead of making this breaking change, I would recommend adding a new method #public_access_prevention_inherited?, and deprecating #public_access_prevention_unspecified?.
There was a problem hiding this comment.
I made an alias of this method at #1035
There was a problem hiding this comment.
Ah, sorry, I missed that. If inherited and unspecified have the same meaning, then the alias is OK.
There was a problem hiding this comment.
Using an alias still changes the behavior of #public_access_prevention_unspecified? which is a breaking change. I think we should keep the original method but deprecate it.
There was a problem hiding this comment.
I agree, best to deprecate with a reference to the new method.
There was a problem hiding this comment.
Actually, now that I think about it some more, the right thing to do kind of depends on the behavior of the backend.
(1) If the values are being changed transparently by the backend, i.e. anything that used to be returned as "unspecified" are now magically returned as "inherited" by the backend, then yes, we should keep the alias in order to preserve the semantics of the call. (But in this case we might also need to update the setter so that if the user passes in :unspecified, we turn it into :inherited before we send it, in case the backend doesn't map that for us.)
(2) If the values are not being changed transparently, i.e. we are expecting the user to shift their usage, then we should keep the existing method but deprecate it.
There was a problem hiding this comment.
I think point 1 stands correct here but @shaffeeullah can confirm.
There was a problem hiding this comment.
@shaffeeullah @tritone Can either of you shed any light on this? Specifically:
- After this change launches in the backend, If an existing bucket already had PAP set to
unspecifiedprior to the change, does the backend still return the valueunspecifiedor does it map the value toinheritedand return that? - After this change launches in the backend, if a user explicitly sets the value to
unspecified, will the backend automatically interpret it asinheritedor are we depending on users to alter their usage in order to get the desired functionality?
There was a problem hiding this comment.
- Currently, yes, if it was set to
unspecified, it will returnunspecified. In the future,unspecifiedwill be deprecated and everything that wasunspecifiedwill be returninginheritedinstead. - The backend will accept both
unspecifiedandinherited. Eventually, settingunspecifiedwill returninherited. This is not the case yet.
Another note, for allowlisted projects, the default value is currently inherited. This will roll out to be the behavior for all projects early November. This changes the default from the previous unspecified. For all projects (regardless of allowlist), setting inherited will work as intended (even if it isn't the default).
There was a problem hiding this comment.
Thanks @shaffeeullah! Given this info, I think what we should do is:
#public_access_prevention_inherited?should return true for either value (inheritedorunspecified).#public_access_prevention_unspecified?should alias#public_access_prevention_inherited?, so it also returns true for either value.- The setter should remain as-is.
|
@shaffeeullah @tritone Does this look good to you? Shall we go ahead and merge this PR ? |
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: #14083