search

googleapis / google-cloud-php

Google Cloud Client Library for PHP
https://cloud.google.com/php/docs/reference
Apache License 2.0
1.1k stars 436 forks source link

Checking gs bucket readability, fails when bucket is not writeable. #1182

Open Dannyzen opened 6 years ago

Dannyzen commented 6 years ago

Using the php stream wrapper a call like this is_readable("gs://foo/bar") will actually attempt to write to the bucket, in our case it fails as it's read only, but its add a $ cost and latency too. This is because of https://github.com/GoogleCloudPlatform/google-cloud-php/blob/master/Storage/src/StreamWrapper.php#L592 as it's hooked into the stat() call in the stream wrapper http://php.net/manual/en/streamwrapper.url-stat.php

Is this working as designed? Could we perform this check differently?

dwsupplee commented 6 years ago

Thanks for the report @Dannyzen. There was actually a discussion regarding this back when the original implementation was introduced.

@chingor13 raised some good points and it seems like we made a fair trade off - but it could be worth at least taking another look now that some time has passed. @chingor13 WDYT?

chingor13 commented 6 years ago

It looks like storage now has a testIamPermissions API which we could potentially use.

dwsupplee commented 6 years ago

Good call. Based on the access management documentation it seems we may need to check both IAM and ACL permissions for specific objects - do you still have any reservations regarding the complexity of the ACL rules?

Dannyzen commented 6 years ago

@chingor13 any thoughts on @dwsupplee 's feedback above?

chingor13 commented 6 years ago

Sorry for the late response. The rules are still complex - you need to check object ACLs and possibly the bucket ACLs if the object doesn't exist (you'd also need to know that the object doesn't exist).

It's possible that we could add a stream wrapper option that would allow us to skip checking permissions at the expense of invalid stat values for permissions (which is probably a reasonable tradeoff).

chingor13 commented 6 years ago

Switching to manually cross-referencing ACLs would change pricing to up to 3 Class-B Operations (test IAM, object acl list, and bucket acl get) from a single Class-A Operation at the expense of additional round trip API calls plus any logic to resolve ACL permissions. Although PHP can technically do non-blocking HTTP requests, I don't think we've allowed for that possibility for these storage APIs.

dwsupplee commented 5 years ago

@chingor13 thinking of moving this over to a feature request, WDYT?