storage: SignedURL should use Client/Bucket

[derekperkins]

Client

storage

Describe Your Environment

Docker on GKE

Expected Behavior

SignedURL is available on a Bucket object, using the client stored on the bucket

Actual Behavior

SignedURLs uses a completely separate auth and client path

Is there a good reason for these to be separate? I know that #1130 is waiting to be merged that makes it a little less onerous to created SignedURLs, but my expectation is that the api should be bucketHandle.SignedURL("shared-object", opts), and the access details would be retrieved from the client. Would you be open to a PR?

cc @josephbergevin @mordfustang

[jeanbza]

cc @frankyn

Speaking for myself, I don't have a strong preference either way. I believe SignedURL_ is sometimes used to hand out URLs for a variety of projects/buckets, so doing the NewClient/Bucket song and dance repeatedly could be annoying. But I could see this being convenient, too, and probably pretty low footprint.

FYI we don't accept PRs - see our CONTRIBUTING.md for details on the contributing process. (we use gerrit)

[frankyn]

Hi @derekperkins,

A subset of other storage language clients such as the PHP, Python and Ruby provide resource level signed URLs for both bucket and object. It can help reduce context overhead of providing the bucket name each time.

The best way to get this in is if you can send us a Gerrit contribution and we can help review and get it into the library.

Thanks for your feedback!

[derekperkins]

I think we'll go ahead and submit a Gerrit contribution. It's just very strange that almost everything in the package runs from the client except SignedURL. We generally make all our clients in main() and pass through into our code, but this breaks that assumption.

[frankyn]

You have a good point, it's very strange that almost everything in the package runs from the client except SignedURL. I honestly didn't see it until you pointed it out.

Looking at history in the past bucket and object operations were in a similar situation. It may have been missed or as Jean stated reduced overhead by not adding it into the client.

After #1130 is resolved then the method SignedUrl being added to at the Client and Bucket would help improve the experience when passing only the client in projects.
That said, I don't think this feature should be blocked by #1130 given it's an alias storage.SignedUrl. Please move forward if you are available.

Thanks @derekperkins, we look forward to your contribution!

[broady]

Kinda my fault.

When I moved the storage package to the Client/BucketHandle/ObjectHandle pattern, I didn't move SignedURL because it didn't perform any network operation. There's no other reason that it isn't under Client/BucketHandle/ObjectHandle. I didn't foresee the client options being useful for the SignedURL function.

Today, I think it makes sense to move under ObjectHandle.

[broady]

FWIW, I didn't read OP closely, which suggests BucketHandle. I do think this should be on ObjectHandle (or Client, if we really think we want to avoid allocs of the Handles).

[odeke-em]

Thank you Derek, Jean, Frank and Chris!

FWIW, I didn't read OP closely, which suggests BucketHandle. I do think this should be on ObjectHandle (or Client, if we really think we want to avoid allocs of the Handles).

I think let's add the method SignedURL to the BucketHandle because:
a) The bucket is a must
b) If an object isn't specified, then the user has requested for signed access to a bucket

Adding the method to ObjectHandle would mean (as you've mentioned) that we are creating a new ObjectHandle and yet as per point b) an object is optional but the bucket is a must.

[odeke-em]

To kick off the conversation, I've mailed https://code-review.googlesource.com/c/gocloud/+/42871

[derekperkins]

Thanks! That works exactly how I would expect it to. If someone feels strongly about it also being available on ObjectHandle, it's a pretty simple convenience wrapper to use this BucketHandle method.

[broady]

Ah, if object is optional then adding the function to BucketHandle seems fine.

…

On Thu, Jul 18, 2019, 1:49 AM Derek Perkins ***@***.***> wrote: That works exactly how I would expect it to. If someone feels strongly about it also being available on ObjectHandle, it's a pretty simple convenience wrapper to use this BucketHandle method. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1495?email_source=notifications&email_token=AAAGDFVY5AUWMK6FOTYFGB3QAAU73A5CNFSM4H6772G2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2HZFWY#issuecomment-512725723>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAGDFSKZ7RACTUHYGZYGCTQAAU73ANCNFSM4H6772GQ> .

[voodoo-dn]

What about this feature? Will be this feature reanimated?

[tritone]

A PR for this is up at #4604, taking the BucketHandle approach and also taking care of auth requirements automatically. Feel free to comment if you have thoughts.

[tritone]

This was resolved in #4604.