Skip to content

Follow-up to Azure auth validation change#111252

Merged
DaveCTurner merged 1 commit intoelastic:mainfrom
DaveCTurner:2024/07/24/azure-auth-validation-followups
Jul 25, 2024
Merged

Follow-up to Azure auth validation change#111252
DaveCTurner merged 1 commit intoelastic:mainfrom
DaveCTurner:2024/07/24/azure-auth-validation-followups

Conversation

@DaveCTurner
Copy link
Copy Markdown
Member

Arising from a post-merge review of #111242

Arising from a post-merge review of elastic#111242
@DaveCTurner DaveCTurner added >test Issues or PRs that are addressing/adding tests :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.16.0 labels Jul 24, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Jul 24, 2024
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

private static class AzureBlobStoreHttpHandler extends AzureHttpHandler implements BlobStoreHttpHandler {
AzureBlobStoreHttpHandler(final String account, final String container) {
super(account, container, null /* no auth header validation */);
super(account, container, null /* no auth header validation - sometimes it's omitted in these tests (TODO why?) */);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As the (new) comment says, the Authorization header is sometimes missing in the calls the SDK makes, but apparently only in this specific test suite. I don't know why. I spent a little time investigating without success, but this is not on the critical path for what I'm actually working towards so I don't want to spend too long on it right now. Definitely worth digging deeper later on.

xcb.field("predicate", authHeaderPredicate.toString());
xcb.field("authorization", Objects.toString(getAuthHeader(exchange)));
xcb.startObject("headers");
try (exchange; var builder = XContentBuilder.builder(XContentType.JSON.xContent())) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Renamed 👍

Comment on lines +53 to +56
* @param account The name of the Azure Blob Storage account against which the request should be authorized..
* @return a predicate that matches the {@code Authorization} HTTP header that the Azure SDK sends when using shared key auth (i.e.
* using a key or SAS token).
* @see <a href="https://learn.microsoft.com/en-us/rest/api/storageservices/authorize-with-shared-key">Azure docs on shared key auth</a>
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hope this helps clarify - the header is coming from the Azure SDK, and I'm working towards a change that should cause it to emit a different kind of auth header, so I want to add these assertions now in order to better support the upcoming change.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very helpful 👍

Copy link
Copy Markdown
Contributor

@DiannaHohensee DiannaHohensee left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for circling back! LGTM 👍

Comment on lines +53 to +56
* @param account The name of the Azure Blob Storage account against which the request should be authorized..
* @return a predicate that matches the {@code Authorization} HTTP header that the Azure SDK sends when using shared key auth (i.e.
* using a key or SAS token).
* @see <a href="https://learn.microsoft.com/en-us/rest/api/storageservices/authorize-with-shared-key">Azure docs on shared key auth</a>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Very helpful 👍

@DaveCTurner DaveCTurner merged commit ea208d1 into elastic:main Jul 25, 2024
@DaveCTurner DaveCTurner deleted the 2024/07/24/azure-auth-validation-followups branch July 25, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed Meta label for distributed team. >test Issues or PRs that are addressing/adding tests v8.16.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants