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

Support setting principal and SASL extensions in oauth_cb, handle failures #1402

Conversation

Manicben
Copy link
Contributor

Fixes #1394

Extends the return values of the oauth_cb to include a principal string, and a dict of SASL extensions, which must be strings matching [A-Za-z]+. These return values are optional, meaning that existing callbacks still work.

When an exception is raised in the callback, rd_kafka_oauthbearer_set_token_failure is called to ensure another refresh is scheduled 10s later. This is for the case where the callback raising an exception is due to a token not being able to be fetched, which may succeed later. Without this, raising exceptions in the callback result in the client never re-fetching a token unless fully restarted.

Suggestions regarding the latter part are appreciated, an alternative would be to allow a return value of None to signal token fetch failure and the need to retry later. However raising an exception is a bit more Pythonic, so maybe raising a certain exception type should be the way to signal a failure.

@CLAassistant
Copy link

CLAassistant commented Jul 26, 2022

CLA assistant check
All committers have signed the CLA.

@Manicben Manicben force-pushed the oauth-cb-principal-sasl-extensions-set-token-failure branch from 4693796 to 5340258 Compare July 26, 2022 17:04
@Manicben Manicben marked this pull request as ready for review July 26, 2022 17:06
@Manicben Manicben requested a review from a team as a code owner July 26, 2022 17:06
@Manicben Manicben force-pushed the oauth-cb-principal-sasl-extensions-set-token-failure branch from 5340258 to 31d8b4a Compare July 29, 2022 10:25
@mhowlett
Copy link
Contributor

thanks for the PR @Manicben - we'll try to get to this soon.

@emasab
Copy link
Contributor

emasab commented Aug 2, 2022

Thanks @Manicben . I'm reviewing it today

@emasab emasab force-pushed the oauth-cb-principal-sasl-extensions-set-token-failure branch from 31d8b4a to 3604dc8 Compare August 2, 2022 19:13
@emasab emasab changed the base branch from master to v1.9.2rc August 2, 2022 21:02
Copy link
Contributor

@emasab emasab left a comment

Choose a reason for hiding this comment

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

LGTM, I've just added a commit to remove the global variables

@emasab emasab merged commit f8b6468 into confluentinc:v1.9.2rc Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No support for setting SASL extensions or Principal with OAuthBearer tokens
4 participants