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
HADOOP-18510. Support confidential client in Azure refresh token gran… #5082
HADOOP-18510. Support confidential client in Azure refresh token gran… #5082
Conversation
...ls/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/oauth2/AzureADAuthenticator.java
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @qcastel for your contribution. Can you please add UT if possible and also run Azure integrations tests and add as part of this PR?
Ref for Integration tests - https://hadoop.apache.org/docs/stable/hadoop-azure/testing_azure.html#Testing_the_hadoop-azure_Module
@ashutoshcipher I had a look at creating UT and it seems that there isn't any test yet covering the class impacted by this fix. Mocking is also made difficult by the HTTP request made behind. Perhaps you got a HTTP mocking framework already used in this repo that I could use? Hoverfly for example? Concerning the IT, looking at the XML to connect to the blob storage, they all uses account access keys. As the fix is for connecting with OAuth2, that won't be cover by the IT for sure. Kind of feel that currently, all your coverage is missing testing the OAuth2 layer of Azure. Would someone of your team could first write a test that cover the existing code? This way adding a test in this PR should be quite straight forward. |
💔 -1 overall
This message was automatically generated. |
going to invite @snvijaya to look at this. |
Any updates on your side @snvijaya ? |
Soon it will be the anniversary of this PR |
Description of PR
Enforcing public client is a bad idea. We should not weaker our software just so the hadoop connector is able to refresh the token.
Therefore the idea would be to support both public and confidential OAuth2 client.
The fix is pretty straight forward, it consist of transiting the client secret to the azure refresh token grant flow and inject it into the request if present.
How was this patch tested?
Not yet, will see how I can test it quickly on our side with a patch.
For code changes: