cgivre commented on pull request #2401: URL: https://github.com/apache/drill/pull/2401#issuecomment-1017072684
@vvysotskyi To answer your questions, I actually first attempted this with a dedicated `OAuthCredentialProvider` class. The issue I ran into was that it needed the `SimpleHttp` class within the `HttpStoragePlugin` to actually do all the REST calls. For instance, if a user has a certificate store or web proxy configured, that would all be done in the config for the HTTP plugin and thus, you'd need that when making the token refreshes. I was thinking about trying to make this reusable for other plugins that may use OAuth. The issue there is that while the flow is the same, there are a lot of pieces which are different. For example, I've been poking at a storage plugin for Google Sheets, which uses OAuth. While both use OAuth, the configuration is completely different and it unfortunately isn't really possible to reuse much in that situation. The other thing to consider is that right now, the OAuth work only uses the `PlainCredentialProvider`, however users might want to store these tokens in Vault. I was thinking as a v2, to extend the functionality so that the tokens can be stored using the `VaultCredentialProvider`, but that will have to wait for another PR. Since some of these tokens are sensitive, it actually would make a lot of sense to store them more securely than in plain text in Zookeeper. ;-) Regarding concurrency, let's say that two users execute a query at virtually the same time. Let's say that `access_token_A` is expired. What should happen is that the first query to hit that server will get a 401 error. Drill would then send a new request with the `refresh_token` and update the access token to `access_token_B`. My understanding of OAuth is that the tokens time out, but during that time, if userB requested a new access token using the same refresh_token, userB would get the same updated access token (`access_token_b`). In this situation, I think the worst case scenario would be that a few extra refresh requests are sent. I think I may have mentioned this, but I'm also working on extending this a bit to allow for per-user tokens and credentials, so when that gets implemented, the chances of collisions happening is even less. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
