Google-cloud-ruby: [Cloud Storage] bucket-level IAM change request

[frankyn]

Hi @blowmage and @quartzmo!

I recently worked on samples using the new bucket-level IAM API surface. (PR).

I want to request that the API surface for reloading bucket-level IAM policy settings be changed from bucket.policy force: true to bucket.policy.reload!. It's more idiomatic and self-descriptive.

I welcome your feedback, thanks!

[quartzmo]

@frankyn Thanks for this suggestion, and please keep them coming!

Since google-cloud-storage is now GA/1.x, I don't think we'll want to remove the force option. What do you think about adding bucket.policy.reload! even though we keep force?

[frankyn]

Is the rational for not removing force: because it would cause a breaking change?
Could this be an exception being that Bucket-level IAM is still in Alpha?

If not possible, I prefer not documenting the named parameter force: and only promoting the use of bucket.policy.reload!.

Thanks @quartzmo!

[quartzmo]

Could this be an exception being that Bucket-level IAM is still in Alpha?

Good question! I don't know if an exception can be made in this case. @blowmage what's your opinion on this?

[blowmage]

AFAIK the versioning policy is for the gem, not services the gem covers.

[blowmage]

Guidance should be to update the IAM policy with the block. If this is the only way the Policy values are changed I don't see a compelling need for force: true or Policy#reload! in the samples.

require "google/cloud/storage"

storage = Google::Cloud::Storage.new

bucket = storage.bucket "my-todo-app"

bucket.policy do |p|
  p.remove "roles/storage.admin", "user:[email protected]"
  p.add "roles/storage.admin", "user:[email protected]"
  p.roles["roles/storage.objectViewer"] = ["allUsers"]
end

[frankyn]

@blowmage that would make sense when updating the policy, but what about when testing changes similar to this?

[quartzmo]

Great point, @blowmage. The block usage example is already in place, I think only the force example needs to be removed.

@frankyn Is it acceptable to simply remove the force examples, since the primary use case for reloading state is updating, and that is now handled in an operation that reloads automatically?

[quartzmo]

Ah, I see @frankyn already has another use case...

[quartzmo]

@frankyn I'm going to work on a PR for this issue. I will not remove the force option (in order to maintain backwards compatibility), but I will remove the example. In a second commit I will add Policy#reload! with an example. We can debate whether to merge that commit in the PR.

[frankyn]

SGTM! Thanks @quartzmo.

[quartzmo]

@frankyn I opened a PR (#1433) removing the force example.

I am also working on an implementation of Policy#reload!. So far it is somewhat awkward,
since the resource must be retrieved using either Buckets: getIamPolicy or Objects: getIamPolicy, depending on which type owns the policy.

[frankyn]

Thanks @quartzmo!

Ah, I didn't think about Objects: getIamPolicy. How are you planning on implementing Policy#reload!?

[quartzmo]

@frankyn Which API method it uses will need to be conditional on whether the policy's resourceId matches the pattern buckets/bucket/objects/object or just buckets/bucket.

[frankyn]

That makes sense. I look forward to the PR!

[quartzmo]

One concern I have after implementing Policy#reload! in #1435 is that the memoized instance held in Bucket will not be updated when reload! is called, as shown in the example below:

bucket = storage.bucket "my-todo-app"

policy = bucket.policy # API call
policy_2 = bucket.policy.reload! # API call
policy_3 = bucket.policy # No API call. Stale reference, same instance as first line.

[frankyn]

That's a concern. Is it not simple to update the memoized instance held by Bucket? I'm guessing It's possible, but the solution isn't clean.

[quartzmo]

It's doable by passing in a reference to the bucket. But it's kind of out there as a side effect!

[blowmage]

@quartzmo @frankyn @remi This change raises a larger design issue around the IAM policies, which we want to be consistent across all Google Cloud libraries. What do you all think of simply removing the memoization on policy and making force a no-op? That would simplify the implementation, and avoid the problems raised by #1435. Also, one can argue that the behavior would be more inline with how most Google Cloud libraries behave.

[quartzmo]

@blowmage I am in favor with removing memoization of Policy from all libraries, because I agree that memoization seems to be the exception rather than the rule for retrieved resources. Was there any specific motivation for this design that you can remember?

Removing memoization of Policy everywhere merits a new issue, but in the meantime, here are the the usages of the force option to override memoization in the entire project. (There may be a few more cases of memoization, but these are the similar ones. All but one are for Policy.)

    google-cloud-pubsub/lib/google/cloud/pubsub  (2 usages found)
        subscription.rb  (1 usage found)
            (516: 11) # @param [Boolean] force Force the latest policy to be retrieved from
        topic.rb  (1 usage found)
            (300: 11) # @param [Boolean] force Force the latest policy to be retrieved from
    google-cloud-resource_manager/lib/google/cloud/resource_manager  (1 usage found)
        project.rb  (1 usage found)
            (333: 11) # @param [Boolean] force Force load the latest policy when `true`.
    google-cloud-spanner/lib/google/cloud/spanner  (3 usages found)
        database.rb  (2 usages found)
            (112: 11) # @param [Boolean] force Force the latest DDL statements to be retrieved
            (216: 11) # @param [Boolean] force Force the latest policy to be retrieved from
        instance.rb  (1 usage found)
            (314: 11) # @param [Boolean] force Force the latest policy to be retrieved from
    google-cloud-storage/lib/google/cloud/storage  (1 usage found)
        bucket.rb  (1 usage found)
            (901: 11) # @param [Boolean] force Force load the latest policy when `true`.

[blowmage]

I think the memoization made more sense before we introduced the block syntax to update a policy. But now it makes less sense. I would rather correct the underlying behavior than add more bandaids around it.

[blowmage]

@remi @frankyn Thoughts on removing the memoization?

[frankyn]

I'm okay with moving forward without memoization.

[quartzmo]

I have closed PR #1435, and will begin work on a new PR to remove memoization.

[frankyn]

Thanks @quartzmo and @blowmage for fixing this issue!

[quartzmo]

No prob! Planning to release the change today.