Added SASL/OAuthBearer #630
Conversation
Codecov Report
@@ Coverage Diff @@
## master #630 +/- ##
==========================================
- Coverage 98.33% 97.84% -0.50%
==========================================
Files 31 31
Lines 5118 5159 +41
==========================================
+ Hits 5033 5048 +15
- Misses 85 111 +26
Continue to review full report at Codecov.
|
|
This pull request introduces 1 alert when merging 4d177db into 10bdd29 - view on LGTM.com new alerts:
|
aiokafka/conn.py
Outdated
|
|
||
| def _build_oauth_client_request(self): | ||
| return "n,,\x01auth=Bearer {}{}\x01\x01".format( | ||
| self._sasl_oauth_token_provider.token(), self._token_extensions() |
There was a problem hiding this comment.
It would be great to be able to have token as an async def function. The library is asynchronous, so would assume many people would prefer it that way.
Something like:
def __init__(self):
self._token_sent = False
async def step(self, payload):
assert not self._token_sent, "Server should either close or send an empty response"
token = await self._sasl_oauth_token_provider.token()
token_extensions = self._token_extensions()
self._token_sent = True
return self._build_oauth_client_request(token, token_extensions)What do you think? If people need to provide synchronous callbacks they can do so by calling:
async def token(self):
return asyncio.get_running_loop().run_in_executor(None, self._token)
def _token(self):
# The actual token function implementation.There was a problem hiding this comment.
shouldn't we return None based on self._token_sent?
| sasl_plain_username (str): username for sasl PLAIN authentication. | ||
| Default: None | ||
| sasl_plain_password (str): password for sasl PLAIN authentication. | ||
| Default: None | ||
| sasl_oauth_token_provider (kafka.oauth.abstract.AbstractTokenProvider): |
There was a problem hiding this comment.
If we were to go with an asynchronous implementation we would need our own abstract class. You can put it in aiokafka.abc.
|
@oulydna Great thanks for working on this! I think the code is good to merge, but if you have time to invest into changing the implementation to an asynchronous callback I would be really grateful. |
|
Thanks @tvoinarovskyi. Really appreciate the quick reply. Yes, I tested it in our own environment. Good point on the async callback. I will look into this. |
|
@tvoinarovskyi can you please have a look at the new changes? |
|
@oulydna Looks good for a merge, few minor points if you have time and let's ship it 🚢 |
* hotfix/ memory leak in comsumer getmany * hotfix/ Added changes in change log * hotfix/ Fixed case where pending future set is empty * hotfix/ renamed pending future variable
|
Thanks again! |
|
Thanks you @tvoinarovskyi . Really glad to have this integrated. We like aiokafka, and will like it better. |
Changes
Fixes issue #618
Added
OAUTHBEARERas a newsasl_mechanism. The implementation is mainly based on this PR in kafka-python.Checklist
CHANGESfolder<issue_id>.<type>(e.g.588.bugfix)issue_idchange it to the pr id after creating the PR.feature: Signifying a new feature..bugfix: Signifying a bug fix..doc: Signifying a documentation improvement..removal: Signifying a deprecation or removal of public API..misc: A ticket has been closed, but it is not of interest to users.Fix issue with non-ascii contents in doctest text files.