Fix Go SDK handling of SAS timestamps

[AtOMiCNebula]

The Go SDK currently rewrites SAS timestamps to always match a specific format ("2006-01-02T15:04:05Z"). This is a problem, because if the timestamp doesn't roundtrip cleanly (i.e. a timestamp not matching that exact pattern) will be unusable with the Go SDK, because the SAS will get rewritten without regenerating the signature.

This change updates SASQueryParameters and friends to retain timestamps as their original string values, instead of as a Time. This should eliminate roundtripping issues, since the timestamps will no longer be rewritten.

I will note that given my API surface changes, this is a breaking change. I'm not sure how the Go SDK wants to handle breaking changes; it's still in preview so maybe you're not worried about them!
I think it's a breaking change worth taking too, but in case you disagree I'm happy to try to rewrite it to be a little more defensive on the API surface changes, I just didn't want to start there.

Separately, this is my first real change written in Go. Please do not hesitate to call out any Go-isms and whatnot that I may have missed. 😄

Resolves #168

[AtOMiCNebula]

There are similar problems with SAS handling in azure-storage-file-go and azure-storage-queue-go, worth taking fixes over there too?

Also, I think CI tests might be broken; I see lots of failures when running against HEAD of dev locally, though that could be because there are some environment settings I don't have correctly in place. 😇

[adreed-msft]

There is also a similar issue in the azbfs package of azcopy-- our integrated adlsg2 shim.

Much like my unexpectedEOF fixes, they're kind of everywhere.

[AtOMiCNebula]

@adreed-msft I noticed the copies in azcopy too, I tried patching code in there first while trying to get azcopy to work, and found out that (at least for the SAS code) none of it was actually being executed, and that it was using the Go SDK's SAS code (hence this patchset). Happy to get azcopy's fork patched too, though I don't know how I could test those changes if the code isn't getting reached. Suggestions?

[adreed-msft]

The code in there is specific to Azure Data Lakes Gen 2. You can use an existing blob sas against a dfs endpoint to test it.

[AtOMiCNebula]

While copying some changes around, I found that the File SDK has some slight differences here, and has for over a year (Azure/azure-storage-file-go@85623f5). It looks reasonably close to what I like to think I'd have put together for this to be a non-breaking-change (i.e. it adds additional string members, instead of switching Time to string). It's not clear to me what value there is in storing the value as a Time at all, which is another reason I took the approach I did.

Not sure why this change was applied to the File SDK but not the other two. But, thoughts? For the File SDK, should I clean up this change to align the File SDK with this change completely, or should I just leave the File SDK alone (allowing it to keep a diverged set of routines)?

[adreed-msft]

🕐

[AtOMiCNebula]

Second iteration is a near-port of Azure/azure-storage-file-go@85623f5 and Azure/azure-storage-file-go@edcebb6 (plus the addition of the high-precision time format). I largely rewrote the test, since it didn't port cleanly (File SASs are different enough, and it was a lot of copy-pasta that was different in subtle ways that I've tried to clean up).

I still don't like this approach, because it requires specific timestamp patterns to be specified in code, so I would not be surprised if this list needs further iteration in the future as people use their own timestamp formats that the Azure service accepts (but the Go SDK mangles). There's a more thorough fix here that is a combination of my original attempt and this one, but I don't have time to assemble it. ☹️

[adreed-msft]

I still don't like this approach, because it requires specific timestamp patterns to be specified in code, so I would not be surprised if this list needs further iteration in the future as people use their own timestamp formats that the Azure service accepts (but the Go SDK mangles). There's a more thorough fix here that is a combination of my original attempt and this one, but I don't have time to assemble it. ☹️

It seems like a future item to tackle that we infer the actual timestamp format from the text.

ISO-8601 has a very specific ordering and format, so I wonder how difficult that'd be.

[AtOMiCNebula]

@adreed-msft / @JeffreyRichter I resolved some merge conflicts, please take a look at this revised version (and/or let me know who else to ping 😄)

[adreed-msft]

Ah, pardon the slow response here. I'll try and get to reviewing this today.

[adreed-msft]

Twenty-six days ago, sorry! Taking a deep look now-- thanks for your work.

[adreed-msft]

[adreed-msft]

[adreed-msft]

By the way, I'd ported your work to the blobfs SDK and updated the files SDK for it in secondary branches to create a custom build of azcopy for internal usage-- Works like a charm!

[AtOMiCNebula]

@adreed-msft Tests still seem to be failing in CI, lots of them. I don't think the failures are related to my change, and I can get panics locally when running some of the affected tests without my changes applied. Is the test suite flaky, or am I just missing something obvious? 😕

[AtOMiCNebula]

Based on https://travis-ci.org/github/Azure/azure-storage-blob-go/pull_requests, it looks like all PR validation builds are failing, though normal CI validation looks okay (https://travis-ci.org/github/Azure/azure-storage-blob-go/branches).

[zezha-msft]

Hi @AtOMiCNebula, no worries, the CI fails because the test credentials are only available if the PR originates from inside the repo. I'll merge and it should run just fine.