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

[QUERY] Why is scope not automatically set for requests to storage endpoint? #39561

Closed
2 tasks done
freva opened this issue Apr 5, 2024 · 10 comments · Fixed by #40261
Closed
2 tasks done

[QUERY] Why is scope not automatically set for requests to storage endpoint? #39561

freva opened this issue Apr 5, 2024 · 10 comments · Fixed by #40261
Assignees
Labels
ARM customer-reported Issues that are reported by GitHub users external to the Azure organization. issue-addressed The Azure SDK team member assisting with this issue believes it to be addressed and ready to close. Mgmt This issue is related to a management-plane library. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that

Comments

@freva
Copy link
Contributor

freva commented Apr 5, 2024

Query/Question
We create a single HttpPipeline instance with all the policies we require, from this we create multiple clients, e.g. ComputeManagementClient, DnsManagementClient, BlobContainerClient. We are not setting any scope because this pipeline is used by multiple clients. Azure SDK already has handling for setting correct default scope if none is set, depending on which endpoint the request is for (via ResourceManagerUtils::getDefaultScopeFromUrl).
Question: Is there any reason for not handling storage endpoint in there as well? AzureEnvironment already has storageEndpointSuffix, though no corresponding AzureEnvironment.Endpoint is defined.

Why is this not a Bug or a feature Request?
Could be a bug/oversight, or maybe there's something I'm missing about the storage endpoint authentication (e.g. if there are some APIs that require a different scope?).

Setup (please complete the following information if applicable):

  • Library/Libraries: com.azure.resourcemanager:azure-resourcemanager-resources:2.37.0

Information Checklist
Kindly make sure that you have added all the following information above and checkoff the required fields otherwise we will treat the issuer as an incomplete report

  • Query Added
  • Setup information Added
@github-actions github-actions bot added ARM customer-reported Issues that are reported by GitHub users external to the Azure organization. Mgmt This issue is related to a management-plane library. needs-team-attention This issue needs attention from Azure service team or SDK team labels Apr 5, 2024
Copy link

github-actions bot commented Apr 5, 2024

Thank you for your feedback. Tagging and routing to the team member best able to assist.

@github-actions github-actions bot added the question The issue doesn't require a change to the product in order to be resolved. Most issues start as that label Apr 5, 2024
@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Apr 7, 2024

The reason top of my mind, is that SDK itself does not send any request to storage account endpoint. Hence there is no need to build the scope to storage account endpoint.

You can see the azure-storage-blob dependency is only in test
https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/resourcemanager/azure-resourcemanager-compute/pom.xml#L124-L128

@freva
Copy link
Contributor Author

freva commented Apr 7, 2024

Ok, but the same applies to, for example, keyvault (immediately above in your link). Keyvault is handled in ResourceManagerUtils::getDefaultScopeFromUrl: https://github.com/Azure/azure-sdk-for-java/blob/c32a69685e1e231dd24d987eca682ce3efbf6587/sdk/resourcemanager/azure-resourcemanager-resources/src/main/java/com/azure/resourcemanager/resources/fluentcore/utils/ResourceManagerUtils.java#L160:L163

Either way, AzureEnvironment already has the storage endpoint suffix, so it should be OK to in ResourceManagerUtils::getDefaultScopeFromUrl?

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Apr 9, 2024

KeyVault is because SDK do create the client (sharing the same pipeline) to access KeyVault endpoint
https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/resourcemanager/azure-resourcemanager-keyvault/src/main/java/com/azure/resourcemanager/keyvault/implementation/VaultImpl.java#L91-L102
(hence the pipeline need to apply the scope for KeyVault)

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Apr 9, 2024

If you need to share the pipeline with other applications (e.g. those need access StorageAcount), you may want to plugin a slightly different AuthenticationPolicy based on this impl.

The code in SDK that building the pipeline is here. You can duplicate the code and replace the AuthenticationPolicy line within.

@freva
Copy link
Contributor Author

freva commented Apr 9, 2024

Yes, that's what we've already done, but we hoped that was a temporary workaround and that this would be handled in the SDK by default instead.

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Apr 9, 2024

OK, we will take a look, but probably not at high priority as it not affect most customers.

One may not really need to share the HttpPipeline. The usual suggestion is to share a HttpClient (and the TokenCredential instance), as connection pool (that's most resource allocated for an app using http client) belongs to the HttpClient.

HttpPipeline is mostly just code over HttpClient.

@XiaofeiCao
Copy link
Contributor

@freva 2.39.0 will be released this week.

@XiaofeiCao XiaofeiCao reopened this May 21, 2024
@XiaofeiCao
Copy link
Contributor

Hi @freva , 2.39.0 released with storage auth scope supported.

@XiaofeiCao XiaofeiCao added the issue-addressed The Azure SDK team member assisting with this issue believes it to be addressed and ready to close. label May 24, 2024
Copy link

Hi @freva. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text "/unresolve" to remove the "issue-addressed" label and continue the conversation.

@github-actions github-actions bot removed the needs-team-attention This issue needs attention from Azure service team or SDK team label May 24, 2024
@freva freva closed this as completed May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ARM customer-reported Issues that are reported by GitHub users external to the Azure organization. issue-addressed The Azure SDK team member assisting with this issue believes it to be addressed and ready to close. Mgmt This issue is related to a management-plane library. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants