Signed URL resumable upload support #640

evildour · 2016-12-11T00:56:46Z

(It might help to read this section of docs for background)

The main thing needing review is the SignedUrlUploader class (which I'll comment and move to its own file if we decide to stick with it) and the way in which the user must create a the signed URL for the initial POST to kick off the resumable upload. I had a few choices here:

Use the existing Sign methods and have a special http method named "RESUMABLE" that when specified, will set a special header needed for the upload and sign a URL for a POST method request instead. This is the route the gcloud SDK went with (see here). The main drawback here is its not easily discoverable without reading the remarks.
Use the existing Sign methods and let the user specify the POST method and we'll automatically add the special header. This might require breaking changes if we ever want to support other kinds of POST requests in the future (currently no other POST requests are possible with signed URLs though).
Use the existing Sign methods, let the user specify POST, and also require that they specify the special header manually. This seems like extra boilerplate we shouldn't require the users to perform.
Create new methods such as SignResumable perhaps. This would require 4 additional methods though which are mostly identical to the current methods.

So I opted for 1, but I'm open to changing this.

Note that this requires a refactor of ResumableUpload<TRequest>, which I've split into a base ResumableUpload class and derived class. So @chrisdunelm, I've added you in case you want to look at those changes. However, keep in mind the refactor is kind of hacked together for now, but if we agree on this, I'll make the changes to google-api-dotnet-client in a much cleaner way. Once that's merged and published, I'll send out the real PR for resumable upload support.

evildour · 2016-12-11T15:10:10Z

Forgot to mention another issue: right now I have signed URLs returned as strings. But if you use the SignedUrlUploader to obtain a session URI, that's returned as a Uri instance because that's what the base logic in ResumableUpload uses. It seemed a little strange to "upwrap" it to return a string and then wrap it in a Uri again when it is passed back to ResumableUpload. So I just want to get anyone's thoughts on whether it seems ok to leave it like this or if we should be consistent and use all strings or all Uris. I'm leaning to all Uris if we do want to be consistent.

evildour · 2016-12-12T02:25:21Z

(fixed the CI builds)

jskeet · 2016-12-12T13:39:18Z

Just a quick note - I'm not ignoring this; it's on my to-do list. I just have other more urgent things today. Will try to review tomorrow (or later today if it all works out).

evildour · 2016-12-12T15:06:41Z

No rush

jskeet · 2016-12-13T17:13:15Z

Just to check, is the TmpMediaDownloader needed at all? I haven't spotted why it would be, but I could be missing something.

I'm having trouble understanding the ResumableUpload differences at the moment, but I'll have another look at those when I'm more awake.

evildour · 2016-12-13T17:31:12Z

The TmpMediaDownloader was just added to quiet some compiler errors if I recall. As for ResumableUpload, like I said its kind of hacked for now but essentially its split into a base and derived class and there are two virtual members now (overridden in SignedUrlUploader). I don't think it needs a full review yet. That can wait until I implement the refactoring in the base library.

evildour · 2016-12-13T17:45:38Z

Rebased and removed TmpMediaDownloader. It wasn't needed after all.

jskeet · 2016-12-13T18:02:41Z

Great. Will concentrate on just the signing bit then - thanks.

jskeet

Okay, I've had a look now, mostly at the SignedUrlUploader part. I think I need a little more context. In particular, I can see two use cases here:

My app wants to create a signed upload URL that another app can use to upload content.
My app wants to use a signed upload URL provided by another app to upload content.

In each case, the "other app" may or may not be in .NET, using this client library.

With your choices you laid out in the first comment, it's not clear whether this is even feasible, as it sounds like there may not be any standardized way of constructing the signed URL.

The regular resumable uploader goes through a "get a session URI, then use it" process, doesn't it? Is there any reason we need to expose a different process here? I may well be missing something - it feels like we should be able to just introduce a virtual method for "get session URI" and override that (and only that) in SignedUrlUploader. (In an ideal world, the signed URL would contain everything required, and we wouldn't need any extra code for this, possibly after a small change to the ResumableUpload code to allow for extra information.)

It could be worth us having a VC, as I suspect I may be missing several subtleties.

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.cs

+        public static async Task<SignedUrlUploader> CreateFromSignedUrlAsync(
+            string signedUrl,
+            Stream contentStream,
+            ConfigurableHttpClient httpClient = null,


evildour · 2016-12-14T17:23:48Z

(VC might be best if this doesn't clear things up, and sorry for the 2nd wall of text)
Here are my thoughts on things (and it's possible I'm wrong about some or all of this). There are two ways an app with permissions (X) can grant an app without permissions (Y) temporary access to upload a large file:

X creates a signed URL and sends it to Y. Y must perform the initial POST request to get the session URI and then perform PUT requests with that session URI to upload the file (ResumableUpload takes care of the PUTs).
X creates a signed URL and performs the initial POST request to get the session URI, which it then sends to Y. Y then performs PUT requests with that session URI to upload the file. This is what I thought was described in the first paragraph here but as I was just rereading it now, I realized I missed the part about "avoiding...signing URLs" for this scenario. So this is all wrong.

So it looks like 1 is the only case I need to worry about here. However, due to my misunderstanding about 2, I have unintentionally created support for Y's side of things in the scenario described in the help there: X performs initial POST and sends session URI to Y. With this PR's code, Y can use SignedUrlUploader.CreateFromSessionUri. However, now it is clear than SignedUrlUploader is no longer an appropriate name for this class. Also, we may want to expose support for X's side of things in this scenario as well, but that's for a different PR.

As for your other points:

Yes, there is a standard way of forming the signed URLs. If a python app X creates a signed URL and sends it to a .NET app Y, it can use SignedUrlUploader.CreateFromSignedUrlAsync with it.
This PR does introduce a virtual to "get session URI" and override it: SignedUrlUploader.InitializeUpload. Although it should probably be renamed to GetSessionUriAsync to be much clearer.

evildour · 2016-12-20T00:13:48Z

@jskeet PTAL. After offline discussion with you and Nathan, it looks like these are the scenarios to be supported. They involve authenticated app X communicating with unauthenticated app Y (possibly via Xc, an unauthenticated client of X). S is the signed URL to which a POST request can be made to initiate a resumable upload and U is the session URI for the upload session.

Scenario 1
a. X creates S using UrlSigner.Sign and sends it to Y.
b. Y receives S and uses ResumableUpload.CreateFromSignedUrl to get a ResumableUpload instance. It then uses UploadAsync on that instance which does the following: POST to S and get U then perform PUT requests to U to upload the file.
Scenario 2
a. X initiates resumable upload by performing POST (not using signed URL) and sends the resulting U to Y. (This is the only part not handled by this PR)
b. Y receives U and uses ResumableUpload.CreateFromSessionUri to get a ResumableUpload instance. It then uses UploadAsync on that instance which performs PUT requests to U to upload the file.
Scenario 3
a. X creates S using UrlSigner.Sign (same as 1a) and sends it to Xc.
b. Xc receives S and uses ResumableUpload.GetSessionUriAsync to POST to S and get U, which it then sends to Y.
c. Y receives U and uses ResumableUpload.CreateFromSessionUri to get a ResumableUpload instance. It then uses UploadAsync on that instance which performs PUT requests to U to upload the file (same as 2b).

Also, I've moved everything pertinent to the review to the top of ResumableUpload. Open questions that still remain: should we use the RESUMABLE HTTP method when creating the signed URL and should we use all strings or all Uri instances?

jskeet · 2016-12-20T07:56:33Z

@evildour: I'm on vacation now until early January - I'll see what I can do anyway, but it may be in bits and pieces.

evildour · 2016-12-20T14:26:27Z

Not a problem. In that case, I may start adding comments to the new APIs at some point.

jskeet

Apologies if I'm being dense again - I'm hoping we can simplify this further.

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/RemoveMe/ResumableUpload.cs

+
+        private ResumableUploadSessionOptions Options { get; }
+
+        private string SignedUrl { get; set; }


jskeet · 2017-01-04T08:46:37Z

Oh, and your other questions:

It sounds like we need to use RESUMABLE when creating the signed URL, so that it'll be compatible with Python. Or at least, this way presumably we can create the same URL as in Python, which is probably a good thing.
I'm not overly worried about string vs Uri - it may well become more obvious when we've got the final shape of the API.

evildour · 2017-01-06T19:36:01Z

For the RESUMABLE method, that never goes into the signed URL. The URL is created as if you had used the POST method. "RESUMABLE" is just a sentinel value meaning 'please create a signed URL with the POST method and the x-goog-resumable header value of "start" (and all other info specified by the caller)'. I don't think there would be any conflict if we went with something else. The only issue would probably be potential user confusion.

And I agree about string vs. Uri - let's wait until everything else is clearer.

jskeet · 2017-01-06T21:05:29Z

I'm now away from work for a week - will see if I can find 15 minutes to try to get my head back into this. We'll get there :)

evildour · 2017-01-06T21:12:05Z

Not a problem. I'm a bit swamped with some other stuff anyway, so I probably won't have much time for this either until later next week or the week after.

jskeet · 2017-01-17T09:00:20Z

Okay, I think I now understand what's going on, and it's looking good. The tricky bit is understanding how much the existing ResumableUpload is changing due to this. Obviously we've gone back and forth via quite huge changes - is it now reasonably simple? I can see the addition of ResumableUploadOptions, and is the rest about how the session URI is managed? I suspect so.

I think this is now the form we want... I'm sufficiently convinced that it would worth creating a PR on google-api-dotnet-client to make that diff clear, if that's feasible. Happy to have a VC to iron out any lingering doubts though.

evildour · 2017-01-18T17:40:57Z

Yes, as far as what's changing in google-api-dotnet-client, its the addition of ResumableUploadOptions and the non-generic ResumableUpload and the rest is about initiating the session and managing the session URI.

I'll try to put together a PR for google-api-dotnet-client later today or tomorrow so we can get this in. I'll also leave this PR open for now just in case that process reveals something we haven't thought of.

Thanks!

jskeet · 2017-02-10T07:31:08Z

Now that there's been a new storage release, presumably this can be modified so it's releasable?

evildour · 2017-02-10T13:52:32Z

Yes. About to push an update. Although I got a little confused with the visibility of MediaApiErrorHandling, forgetting that it was internal to Google.Apis. So I had to hack things a bit in the static SignedUrlResumableUpload.InitiateSessionAsync by creating a temporary instance to initiate the session. I think it should be fine, but let me know if you feel different.

evildour · 2017-02-10T13:53:44Z

This is ready for submission now. PTAL.

jskeet

A few comments, but generally I think we're there. Hooray :)

apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1.IntegrationTests/UrlSignerTest.cs

+                        name,
+                        duration,
+                        UrlSigner.ResumableHttpMethod,
+                        requestHeaders: new Dictionary<string, IEnumerable<string>> {


apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/SignedUrlResumableUpload.cs

+            return uploader.InitiateSessionAsync(cancellationToken);
+        }
+
+        private static T GetResult<T>(Func<CancellationToken, Task<T>> operation)


apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/UrlSigner.cs

+            {
+                requestMethod = HttpMethod.Get;
+            }
+            else if (requestMethod.Equals(ResumableHttpMethod))


apis/Google.Cloud.Storage.V1/Google.Cloud.Storage.V1/SignedUrlResumableUpload.cs

+            ResumableUploadOptions options = null,
+            CancellationToken cancellationToken = default(CancellationToken))
+        {
+            var uploader = new SignedUrlResumableUpload(signedUrl, new MemoryStream(), options);


evildour · 2017-02-10T17:37:34Z

Thanks Jon! Will merge when green, unless you have additional comments.

jskeet · 2017-02-10T18:11:27Z

Continue with the operation. You may merge when ready.

evildour added the api: storage Issues related to the Cloud Storage API. label Dec 11, 2016

evildour requested review from jskeet and chrisdunelm December 11, 2016 00:56

evildour force-pushed the resumable-uploads branch from 85787f5 to 85e36dc Compare December 12, 2016 02:24

evildour force-pushed the resumable-uploads branch from 85e36dc to 94a4b78 Compare December 13, 2016 17:45

jskeet reviewed Dec 14, 2016

View reviewed changes

evildour force-pushed the resumable-uploads branch from 94a4b78 to cefabbb Compare December 20, 2016 00:13

jskeet reviewed Jan 4, 2017

View reviewed changes

evildour force-pushed the resumable-uploads branch from cefabbb to 6a2841e Compare January 13, 2017 23:39

evildour mentioned this pull request Jan 19, 2017

Split ResumableUpload class and introduced associated options googleapis/google-api-dotnet-client#902

Merged

Signed URL resumable upload support

f6fd64f

Derived uploader for signed URLs

30961c5

evildour force-pushed the resumable-uploads branch from 6a2841e to 30961c5 Compare February 10, 2017 13:52

evildour changed the title ~~NOT FOR SUBMISSION Signed URL resumable upload support~~ Signed URL resumable upload support Feb 10, 2017

jskeet approved these changes Feb 10, 2017

View reviewed changes

Address PR feedback

d8c4623

evildour merged commit bf6c299 into googleapis:master Feb 10, 2017

evildour deleted the resumable-uploads branch February 10, 2017 18:06

evildour mentioned this pull request Feb 13, 2017

Expose ResumableUpload.InitiateSessionAsync googleapis/google-api-dotnet-client#919

Merged

evildour mentioned this pull request Mar 16, 2017

Allow resumable upload session initiation #890

Merged


		private ResumableUploadSessionOptions Options { get; }

		private string SignedUrl { get; set; }

Signed URL resumable upload support #640

Signed URL resumable upload support #640

Uh oh!

Conversation

evildour commented Dec 11, 2016

Uh oh!

evildour commented Dec 11, 2016

Uh oh!

evildour commented Dec 12, 2016

Uh oh!

jskeet commented Dec 12, 2016

Uh oh!

evildour commented Dec 12, 2016

Uh oh!

jskeet commented Dec 13, 2016

Uh oh!

evildour commented Dec 13, 2016

Uh oh!

evildour commented Dec 13, 2016

Uh oh!

jskeet commented Dec 13, 2016

Uh oh!

jskeet left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

evildour commented Dec 14, 2016

Uh oh!

evildour commented Dec 20, 2016

Uh oh!

jskeet commented Dec 20, 2016

Uh oh!

evildour commented Dec 20, 2016

Uh oh!

jskeet left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

jskeet commented Jan 4, 2017

Uh oh!

evildour commented Jan 6, 2017

Uh oh!

jskeet commented Jan 6, 2017

Uh oh!

evildour commented Jan 6, 2017

Uh oh!

jskeet commented Jan 17, 2017

Uh oh!

evildour commented Jan 18, 2017

Uh oh!

jskeet commented Feb 10, 2017

Uh oh!

evildour commented Feb 10, 2017

Uh oh!

evildour commented Feb 10, 2017

Uh oh!

jskeet left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.