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.

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

[storage] Adds support for Append Blobs #278

Merged
merged 1 commit into from
Feb 10, 2016
Merged

Conversation

BrianBland
Copy link
Contributor

Adds PutAppendBlob and AppendBlock methods

Addresses #275

@msftclas
Copy link

msftclas commented Feb 9, 2016

Hi @BrianBland, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@msftclas
Copy link

msftclas commented Feb 9, 2016

@BrianBland, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

@ahmetb
Copy link
Contributor

ahmetb commented Feb 9, 2016

@BrianBland thanks for the pull request. LGTM. extraHeaders will let users make use of x-ms-blob-condition-maxsize and x-ms-blob-condition-appendpos, which might be frequently used in the use cases of AppendBlob.

It looks like there's something funny going on about our storage tests. @colemickens have you changed something lately?

@BrianBland
Copy link
Contributor Author

@ahmetalpbalkan it looks like I need to change some things up to support the new signing code as of version 2015-02-21 (see https://msdn.microsoft.com/en-us/library/azure/mt584141.aspx)

@ahmetb
Copy link
Contributor

ahmetb commented Feb 9, 2016

@BrianBland oh that explains the problem. We were actually planning to switch on SharedKeyLite signing (which is way simpler, doesn't have Content-Length either). If you don't mind making the change to work with 2015-02-21, please go ahead. We can do SharedKeyLite in a separate PR.

@BrianBland
Copy link
Contributor Author

Hopefully you don't mind the repeated push trial and error with the CI system. I'm temporarily locked out of my Azure account 😞

@BrianBland
Copy link
Contributor Author

@ahmetalpbalkan if you have any idea why TestBlobSASURICorrectness is failing with the new API version, I'm ready to fix it. I can't find any documentation suggesting why this is suddenly invalid.

@ahmetb
Copy link
Contributor

ahmetb commented Feb 9, 2016

@BrianBland oh that's just comparing a precomputed signature with the algorithm's output. You just need to change the hard-coded computed value with the result of then modified algorithm.

@BrianBland
Copy link
Contributor Author

@ahmetalpbalkan I think you're referring to TestGetBlobSASURI. The failing test is getting a 403 status code from the server

@BrianBland
Copy link
Contributor Author

Apparently the canonicalizedResource for the presigned blob urls must begin with "/blob", even though the documentation suggests that it should begin with "blob"

@ahmetb
Copy link
Contributor

ahmetb commented Feb 10, 2016

@BrianBland I'll follow up on that. Might be a docs issue indeed.

uri := b.client.getEndpoint(blobServiceName, path, url.Values{})
headers := b.client.getStandardHeaders()
headers["x-ms-blob-type"] = string(BlobTypeAppend)
headers["Content-Length"] = "0"
Copy link
Contributor

Choose a reason for hiding this comment

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

@BrianBland Hmm, I don't think you will still need this header now that you updated buildCanonicalizedString with

+   contentLength := headers["Content-Length"]
+   if contentLength == "0" {
+       contentLength = ""
+   }

In fact, there should be more occurrences of headers["Content-Length"] = "0" in this file, I think we can remove all those if you don't mind doing this in PR as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, removed all of those

Adds PutAppendBlob and AppendBlock methods
Changes DefaultAPIVersion to 2015-02-21
Updates storage client signing code to support newer version

Signed-off-by: Brian Bland <brian.bland@docker.com>
@ahmetb
Copy link
Contributor

ahmetb commented Feb 10, 2016

@BrianBland thanks LGTM!

ahmetb added a commit that referenced this pull request Feb 10, 2016
[storage] Add support for Append Blobs, storage version changed to 2015-02-21
@ahmetb ahmetb merged commit 95361a2 into Azure:master Feb 10, 2016
@BrianBland BrianBland deleted the appendBlock branch February 10, 2016 18:46
@ahmetb
Copy link
Contributor

ahmetb commented Feb 25, 2016

@BrianBland hi there, just circling back on the docs issue https://msdn.microsoft.com/en-us/library/azure/dn140255.aspx I see that it says canonicalizedresource = "/blob/..., did you see a different value back then?

image

@BrianBland
Copy link
Contributor Author

I thought so, but at least it looks correct now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants