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

Fix find blob by tag #1622

Open
wants to merge 6 commits into
base: legacy
Choose a base branch
from

Conversation

rafamerlin
Copy link

@rafamerlin rafamerlin commented Mar 11, 2024

There were 2 issues in here, one is that the API version got updated recently and that caused the response to be different than the one expected so querying blobs by tag wasn't working. I believe the version that the current main code has is the response for when we query with x-ms-version: 2019-12-12, however, we're now sending our requests with x-ms-version: 2022-11-02 which caused this to completely break.

Another item was when we add multiple tags to the where expression, that was failing because of the way the URL was being encoded and that doesn't seem to be accepted by Azure REST API.

I've been using a workaround branch that was still using the old API for quite a while without any issues with this URL fix, related to this issue I raised a while ago #1284

The error seems to have changed on the new API version as without the make_url_compatible_with_api if we query anything with more than 1 tag in the filter we get this error:

<?xml version="1.0" encoding="utf-8"?>
<Error>
    <Code>AuthenticationFailed</Code>
    <Message>Server failed to authenticate the request. Make sure the value of Authorization header
        is formed correctly including the
        signature.RequestId:602fdf47-a01e-0010-5d99-732f4c000000Time:2024-03-11T09:50:11.4905786Z</Message>
    <AuthenticationErrorDetail>
        The MAC signature found in the HTTP request
        &apos;{{redacted}}&apos; is not the same as any computed
        signature. Server used following string to sign: &apos;GETx-ms-date:Mon, 11 Mar 2024
        09:50:06
        GMTx-ms-version:2022-11-02/{{redacted}}/comp:blobswhere:@container=&apos;unit-tests-with-index&apos;+AND+tagis=&apos;a&apos;&apos;.</AuthenticationErrorDetail>
</Error>

One question I have regarding the extra structs I had to add to have the response matching the new format of the XML for the tag content, do we need that at all? I was wondering if we could just drop the tag content as it doesn't seem like it's being used anywhere. Happy to keep it though in case there's any intention of using it in the future or if we want it for information purposes.

There were 2 issues in here, one is that the API version got updated
recently and that caused the response to be different than the one
expected so querying blobs by tag wasn't working.

Another item was when we add multiple tags to the where expression, that
was failing because of the way the url was being encoded and that
doesn't seem to be accepted by Azure REST API
@heaths
Copy link
Member

heaths commented Mar 11, 2024

@seanmcc-msft is this a known breaking change to the format?

@rafamerlin
Copy link
Author

I took the chance to hook up the MaxResults into FindBlobsByTagsBuilder as it wasn't doing anything, removed the next_marker on the input and change it to Marker to match the ListBlobsBuilder and made the next_marker public on the response so we can actually use it.

@heaths
Copy link
Member

heaths commented Apr 23, 2024

@seanmcc-msft is there anyone on your team that can review this? It's mostly service-related code.

@seanmcc-msft
Copy link
Member

@jaschrep-msft @vincenttran-msft @jalauzon-msft please take a look.

Comment on lines +49 to +53
fn make_url_compatible_with_api(url: &mut Url) {
let compatible_query = url.query().map(|q| q.replace("+", "%20"));

url.set_query(compatible_query.as_deref());
}
Copy link
Member

Choose a reason for hiding this comment

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

General note, requiring %20 instead of + as the proper encoding for a space character is service-wide. Any URL encoding done by the library for storage requests needs to do this. Probably beyond the scope of this PR, but good future reference.

dependabot bot and others added 4 commits May 20, 2024 22:50
Updates the requirements on [base64](https://github.com/marshallpierce/rust-base64) to permit the latest version.
- [Changelog](https://github.com/marshallpierce/rust-base64/blob/master/RELEASE-NOTES.md)
- [Commits](marshallpierce/rust-base64@v0.21.0...v0.22.0)

---
updated-dependencies:
- dependency-name: base64
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@heaths
Copy link
Member

heaths commented Oct 16, 2024

main branch has been moved to legacy. Retargeting some old PRs.

@heaths heaths changed the base branch from main to legacy October 16, 2024 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Untriaged
Development

Successfully merging this pull request may close these issues.

4 participants