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.

Sign up for GitHub

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

Jump to bottom

feat: implement oauthbearer token refresh cb setter #546

Merged
merged 5 commits into from
Jan 5, 2024
Merged

feat: implement oauthbearer token refresh cb setter #546

merged 5 commits into from
Jan 5, 2024

Conversation

cb-freddysart
Copy link
Contributor

No description provided.

jcunhafonte
jcunhafonte approved these changes Jan 3, 2024
View reviewed changes
Copy link

@jcunhafonte jcunhafonte left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great PR! I have been waiting for this PR for a while now. ❤️

arnaud-lb
arnaud-lb reviewed Jan 4, 2024
View reviewed changes
Copy link
Owner

@arnaud-lb arnaud-lb left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM appart from a few comments

conf.c Outdated

Z_ADDREF_P(&fci.function_name);

if (conf->cbs.log) {
Copy link
Owner

Choose a reason for hiding this comment

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

Should be conf->cbs.oauthbearer_token_refresh

conf.c Outdated
Comment on lines 741 to 744
#ifndef HAS_RD_KAFKA_OAUTHBEARER_TOKEN_REFRESH_CB
zend_throw_exception_ex(NULL, 0, "This version of rdkafka does not support the OAUTHBEARER sasl mechanism");
return;
#endif
Copy link
Owner

Choose a reason for hiding this comment

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

Please change this so that the setOauthbearerTokenRefreshCb method does not exist if HAS_RD_KAFKA_OAUTHBEARER_TOKEN_REFRESH_CB is not defined.

(stub files also support #ifdef)

@cb-freddysart
Copy link
Contributor Author

Thank you for the speedy review @arnaud-lb ! I'm a bit new to PHP extensions but I believe I've addressed your current round of feedback.

Also, sorry for not providing a summary. You beat me to the punch.

My goal here is to add support for OAUTHBEARER in such a way that we will be able to connect to AWS MSK with this library and I believe this PR is only half of that story.

The other half is adding support for rd_kafka_oauthbearer_set_token and rd_kafka_oauthbearer_set_token_failure (see https://github.com/confluentinc/librdkafka/blob/master/src/rdkafka.h#L2207-L2209). Would you be open to adding these methods as well? I would either open a separate PR or add them here and reorganize the commits to make review easier. Whichever works for you.

@arnaud-lb
Copy link
Owner

Nice work, thank you!

I'm generally open to adding any method provided by librdkafka. Please do so in a separate PR, as this will simplify reviews (maybe just one PR for the two methods, as they are very related).

@arnaud-lb arnaud-lb merged commit bcd5004 into arnaud-lb:6.x Jan 5, 2024
53 checks passed
@arnaud-lb arnaud-lb mentioned this pull request Oct 25, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arnaud-lb arnaud-lb arnaud-lb left review comments

@jcunhafonte jcunhafonte jcunhafonte approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cb-freddysart @arnaud-lb @jcunhafonte