Skip to content
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

samples: changed PAP unspecified to inherited #14141

Merged
merged 5 commits into from Oct 28, 2021
Merged

samples: changed PAP unspecified to inherited #14141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
126f857
samples: changed PAP unspecified to inherited
bajajneha27 Sep 29, 2021
20e11a6
Adding the right path of inherited sample.
bajajneha27 Sep 29, 2021
d702551
Samples test case fix.
bajajneha27 Oct 25, 2021
b05f5e1
Deprecating method public_access_prevention_unspecified?
bajajneha27 Oct 25, 2021
4e64f93
Addressing review comments.
bajajneha27 Oct 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 10 additions & 10 deletions google-cloud-storage/acceptance/storage/bucket_uniform_bucket_level_access_test.rb
View file
Open in desktop
Expand Up @@ -33,7 +33,7 @@
# always reset the uniform_bucket_level_access and public_access_prevention
# always reset the bucket permissions
bucket.uniform_bucket_level_access = false if bucket.uniform_bucket_level_access?
bucket.public_access_prevention = :unspecified if bucket.public_access_prevention_enforced?
bucket.public_access_prevention = :inherited if bucket.public_access_prevention_enforced?
bucket.default_acl.private!
bucket.files.all { |f| f.delete rescue nil }
end
Expand Down Expand Up @@ -212,11 +212,11 @@
expect do
bucket_pap.acl.public!
end.must_raise Google::Cloud::FailedPreconditionError
# Verify the setting can be patched to unspecified.
bucket_pap.public_access_prevention = :unspecified
# Verify the setting can be patched to inherited.
bucket_pap.public_access_prevention = :inherited
refute bucket_pap.public_access_prevention_enforced?
assert bucket_pap.public_access_prevention_unspecified?
_(bucket_pap.public_access_prevention).must_equal "unspecified"
assert bucket_pap.public_access_prevention_inherited?
_(bucket_pap.public_access_prevention).must_equal "inherited"
bucket_pap.acl.public!
ensure
safe_gcs_execute { bucket_pap.delete } if bucket_pap
Expand All @@ -233,9 +233,9 @@
end

it "sets public_access_prevention to enforced" do
# Insert a new bucket with Public Access Prevention Unspecified.
# Insert a new bucket with Public Access Prevention Inherited.
refute bucket.public_access_prevention_enforced?
_(bucket.public_access_prevention).must_equal "unspecified"
_(bucket.public_access_prevention).must_equal "inherited"
# Insert and Patch requests using unexpected PAP enum values return 400 error.
expect do
bucket.public_access_prevention = "BAD VALUE"
Expand All @@ -259,10 +259,10 @@
assert bucket.public_access_prevention_enforced?
_(bucket.public_access_prevention).must_equal "enforced"
# Modifying PAP on UBLA bucket does not affect UBLA setting.
bucket.public_access_prevention = :unspecified
bucket.public_access_prevention = :inherited
assert bucket.uniform_bucket_level_access?
refute bucket.public_access_prevention_enforced?
assert bucket.public_access_prevention_unspecified?
_(bucket.public_access_prevention).must_equal "unspecified"
assert bucket.public_access_prevention_inherited?
_(bucket.public_access_prevention).must_equal "inherited"
end
end
26 changes: 14 additions & 12 deletions google-cloud-storage/lib/google/cloud/storage/bucket.rb
View file
Open in desktop
Expand Up @@ -932,11 +932,11 @@ def policy_only_locked_at
end

##
# The value for Public Access Prevention in the bucket's IAM configuration. Currently, `unspecified` and
# The value for Public Access Prevention in the bucket's IAM configuration. Currently, `inherited` and
# `enforced` are supported. When set to `enforced`, Public Access Prevention is enforced in the bucket's IAM
# configuration. This value can be modified by calling {#public_access_prevention=}.
#
# @return [String, nil] Currently, `unspecified` and `enforced` are supported. Returns `nil` if the bucket has
# @return [String, nil] Currently, `inherited` and `enforced` are supported. Returns `nil` if the bucket has
# no IAM configuration.
#
# @example
Expand All @@ -958,7 +958,7 @@ def public_access_prevention
# calling {#public_access_prevention}.
#
# @param [Symbol, String] new_public_access_prevention The bucket's new Public Access Prevention configuration.
# Currently, `unspecified` and `enforced` are supported. When set to `enforced`, Public Access
# Currently, `inherited` and `enforced` are supported. When set to `enforced`, Public Access
# Prevention is enforced in the bucket's IAM configuration.
#
# @example Set Public Access Prevention to enforced:
Expand All @@ -971,15 +971,15 @@ def public_access_prevention
# bucket.public_access_prevention = :enforced
# bucket.public_access_prevention #=> "enforced"
#
# @example Set Public Access Prevention to unspecified:
# @example Set Public Access Prevention to inherited:
# require "google/cloud/storage"
#
# storage = Google::Cloud::Storage.new
#
# bucket = storage.bucket "my-bucket"
#
# bucket.public_access_prevention = :unspecified
# bucket.public_access_prevention #=> "unspecified"
# bucket.public_access_prevention = :inherited
# bucket.public_access_prevention #=> "inherited"
#
def public_access_prevention= new_public_access_prevention
@gapi.iam_configuration ||= API::Bucket::IamConfiguration.new
Expand Down Expand Up @@ -1011,11 +1011,11 @@ def public_access_prevention_enforced?
end

##
# Whether the value for Public Access Prevention in the bucket's IAM configuration is `unspecified`. The default
# Whether the value for Public Access Prevention in the bucket's IAM configuration is `inherited`. The default
# is `false`. This value can be modified by calling {Bucket#public_access_prevention=}.
#
# @return [Boolean] Returns `false` if the bucket has no IAM configuration or if Public Access Prevention is
# not `unspecified` in the IAM configuration. Returns `true` if Public Access Prevention is `unspecified` in
# not `inherited` in the IAM configuration. Returns `true` if Public Access Prevention is `inherited` in
# the IAM configuration.
#
# @example
Expand All @@ -1025,14 +1025,16 @@ def public_access_prevention_enforced?
#
# bucket = storage.bucket "my-bucket"
#
# bucket.public_access_prevention = :unspecified
# bucket.public_access_prevention_unspecified? # true
# bucket.public_access_prevention = :inherited
# bucket.public_access_prevention_inherited? # true
#
def public_access_prevention_unspecified?
def public_access_prevention_inherited?
Copy link
Member

Choose a reason for hiding this comment

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

Instead of making this breaking change, I would recommend adding a new method #public_access_prevention_inherited?, and deprecating #public_access_prevention_unspecified?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made an alias of this method at #1035

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, I missed that. If inherited and unspecified have the same meaning, then the alias is OK.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, best to deprecate with a reference to the new method.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think point 1 stands correct here but @shaffeeullah can confirm.

Copy link
Member

Choose a reason for hiding this comment

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

@shaffeeullah @tritone Can either of you shed any light on this? Specifically:

  1. After this change launches in the backend, If an existing bucket already had PAP set to unspecified prior to the change, does the backend still return the value unspecified or does it map the value to inherited and return that?
  2. After this change launches in the backend, if a user explicitly sets the value to unspecified, will the backend automatically interpret it as inherited or are we depending on users to alter their usage in order to get the desired functionality?

Copy link

Choose a reason for hiding this comment

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

  1. Currently, yes, if it was set to unspecified, it will return unspecified. In the future, unspecified will be deprecated and everything that was unspecified will be returning inherited instead.
  2. The backend will accept both unspecified and inherited. Eventually, setting unspecified will return inherited. 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).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @shaffeeullah! Given this info, I think what we should do is:

  • #public_access_prevention_inherited? should return true for either value (inherited or unspecified).
  • #public_access_prevention_unspecified? should alias #public_access_prevention_inherited?, so it also returns true for either value.
  • The setter should remain as-is.

return false unless @gapi.iam_configuration&.public_access_prevention
@gapi.iam_configuration.public_access_prevention.to_s == "unspecified"
["inherited", "unspecified"].include? @gapi.iam_configuration.public_access_prevention.to_s
end

alias public_access_prevention_unspecified? public_access_prevention_inherited?

##
# Updates the bucket with changes made in the given block in a single
# PATCH request. The following attributes may be set: {#cors},
Expand Down
20 changes: 10 additions & 10 deletions google-cloud-storage/samples/acceptance/buckets_test.rb
View file
Open in desktop
Expand Up @@ -43,7 +43,7 @@
require_relative "../storage_remove_retention_policy"
require_relative "../storage_set_bucket_default_kms_key"
require_relative "../storage_set_public_access_prevention_enforced"
require_relative "../storage_set_public_access_prevention_unspecified"
require_relative "../storage_set_public_access_prevention_inherited"
require_relative "../storage_set_retention_policy"

describe "Buckets Snippets" do
Expand Down Expand Up @@ -89,7 +89,7 @@
list_buckets
end

assert_includes out, "ruby_storage_sample_"
assert_includes out, "ruby-storage-samples-"

# get_bucket_metadata
out, _err = capture_io do
Expand Down Expand Up @@ -403,10 +403,10 @@

describe "public_access_prevention" do
it "set_public_access_prevention_enforced, get_public_access_prevention, " \
"set_public_access_prevention_unspecified" do
bucket.public_access_prevention = :unspecified
"set_public_access_prevention_inherited" do
bucket.public_access_prevention = :inherited
bucket.refresh!
_(bucket.public_access_prevention).must_equal "unspecified"
_(bucket.public_access_prevention).must_equal "inherited"

# set_public_access_prevention_enforced
assert_output "Public access prevention is set to enforced for #{bucket.name}.\n" do
Expand All @@ -422,14 +422,14 @@
end
_(bucket.public_access_prevention).must_equal "enforced"

# set_public_access_prevention_unspecified
assert_output "Public access prevention is 'unspecified' for #{bucket.name}.\n" do
set_public_access_prevention_unspecified bucket_name: bucket.name
# set_public_access_prevention_inherited
assert_output "Public access prevention is 'inherited' for #{bucket.name}.\n" do
set_public_access_prevention_inherited bucket_name: bucket.name
end

bucket.refresh!
_(bucket.public_access_prevention).must_equal "unspecified"
bucket.public_access_prevention = :unspecified
_(bucket.public_access_prevention).must_equal "inherited"
bucket.public_access_prevention = :inherited
end
end
end
31 changes: 31 additions & 0 deletions google-cloud-storage/samples/storage_set_public_access_prevention_inherited.rb
View file
Open in desktop
@@ -0,0 +1,31 @@
# Copyright 2020 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

# [START storage_set_public_access_prevention_inherited]
def set_public_access_prevention_inherited bucket_name:
# The ID of your GCS bucket
# bucket_name = "your-unique-bucket-name"

require "google/cloud/storage"

storage = Google::Cloud::Storage.new
bucket = storage.bucket bucket_name

bucket.public_access_prevention = :inherited

puts "Public access prevention is 'inherited' for #{bucket_name}."
end
# [END storage_set_public_access_prevention_inherited]

set_public_access_prevention_inherited bucket_name: ARGV.shift if $PROGRAM_NAME == __FILE__
4 changes: 2 additions & 2 deletions google-cloud-storage/support/doctest_helper.rb
View file
Open in desktop
Expand Up @@ -415,10 +415,10 @@ def mock_pubsub
end
end

doctest.before "Google::Cloud::Storage::Bucket#public_access_prevention=@Set Public Access Prevention to unspecified:" do
doctest.before "Google::Cloud::Storage::Bucket#public_access_prevention=@Set Public Access Prevention to inherited:" do
mock_storage do |mock|
mock.expect :get_bucket, bucket_gapi, ["my-bucket", Hash]
mock.expect :patch_bucket, bucket_gapi("my-bucket", public_access_prevention: "unspecified"), ["my-bucket", Google::Apis::StorageV1::Bucket, Hash]
mock.expect :patch_bucket, bucket_gapi("my-bucket", public_access_prevention: "inherited"), ["my-bucket", Google::Apis::StorageV1::Bucket, Hash]
end
end

Expand Down
32 changes: 16 additions & 16 deletions google-cloud-storage/test/google/cloud/storage/bucket_public_access_prevention_test.rb
View file
Open in desktop
Expand Up @@ -29,61 +29,61 @@
_(bucket.public_access_prevention_enforced?).must_equal false
end

it "knows its public_access_prevention_unspecified? value" do
_(bucket.public_access_prevention_unspecified?).must_equal false
it "knows its public_access_prevention_inherited? value" do
_(bucket.public_access_prevention_inherited?).must_equal false
end

it "updates its public_access_prevention" do
mock = Minitest::Mock.new
mock.expect :patch_bucket, resp_bucket_gapi(bucket_hash, public_access_prevention: "unspecified"),
patch_bucket_args(bucket_name, patch_bucket_gapi(public_access_prevention: "unspecified"))
mock.expect :patch_bucket, resp_bucket_gapi(bucket_hash, public_access_prevention: "inherited"),
patch_bucket_args(bucket_name, patch_bucket_gapi(public_access_prevention: "inherited"))
mock.expect :patch_bucket, resp_bucket_gapi(bucket_hash, public_access_prevention: "enforced"),
patch_bucket_args(bucket_name, patch_bucket_gapi(public_access_prevention: "enforced"))
bucket.service.mocked_service = mock

_(bucket.public_access_prevention).must_be :nil?
_(bucket.public_access_prevention_enforced?).must_equal false
_(bucket.public_access_prevention_unspecified?).must_equal false
_(bucket.public_access_prevention_inherited?).must_equal false

bucket.public_access_prevention = :unspecified
bucket.public_access_prevention = :inherited

_(bucket.public_access_prevention).must_equal "unspecified"
_(bucket.public_access_prevention).must_equal "inherited"
_(bucket.public_access_prevention_enforced?).must_equal false
_(bucket.public_access_prevention_unspecified?).must_equal true
_(bucket.public_access_prevention_inherited?).must_equal true

bucket.public_access_prevention = :enforced

_(bucket.public_access_prevention).must_equal "enforced"
_(bucket.public_access_prevention_enforced?).must_equal true
_(bucket.public_access_prevention_unspecified?).must_equal false
_(bucket.public_access_prevention_inherited?).must_equal false

mock.verify
end

it "updates its public_access_prevention with user_project set to true" do
mock = Minitest::Mock.new
mock.expect :patch_bucket, resp_bucket_gapi(bucket_hash, public_access_prevention: "unspecified"),
patch_bucket_args(bucket_name, patch_bucket_gapi(public_access_prevention: "unspecified"), user_project: "test")
mock.expect :patch_bucket, resp_bucket_gapi(bucket_hash, public_access_prevention: "inherited"),
patch_bucket_args(bucket_name, patch_bucket_gapi(public_access_prevention: "inherited"), user_project: "test")
mock.expect :patch_bucket, resp_bucket_gapi(bucket_hash, public_access_prevention: "enforced"),
patch_bucket_args(bucket_name, patch_bucket_gapi(public_access_prevention: "enforced"), user_project: "test")

bucket_user_project.service.mocked_service = mock

_(bucket_user_project.public_access_prevention).must_be :nil?
_(bucket_user_project.public_access_prevention_enforced?).must_equal false
_(bucket_user_project.public_access_prevention_unspecified?).must_equal false
_(bucket_user_project.public_access_prevention_inherited?).must_equal false

bucket_user_project.public_access_prevention = :unspecified
bucket_user_project.public_access_prevention = :inherited

_(bucket_user_project.public_access_prevention).must_equal "unspecified"
_(bucket_user_project.public_access_prevention).must_equal "inherited"
_(bucket_user_project.public_access_prevention_enforced?).must_equal false
_(bucket_user_project.public_access_prevention_unspecified?).must_equal true
_(bucket_user_project.public_access_prevention_inherited?).must_equal true

bucket_user_project.public_access_prevention = :enforced

_(bucket_user_project.public_access_prevention).must_equal "enforced"
_(bucket_user_project.public_access_prevention_enforced?).must_equal true
_(bucket_user_project.public_access_prevention_unspecified?).must_equal false
_(bucket_user_project.public_access_prevention_inherited?).must_equal false

mock.verify
end
Expand Down