-
Notifications
You must be signed in to change notification settings - Fork 668
Added support for SASL mechanism OAuthBearer #503
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
base: master
Are you sure you want to change the base?
Conversation
|
Anything more i can do, to get this merged? |
|
Hi dear Lux, get error: How to specify oauth scopes and other settings? |
|
Hi Marinafrank. The PR i have made does not support these parameters. It could be argued that "scope" parameter should be supported, since it is a normal OAuth parameter, supported by the go package out of the box. However the extensions, which seems to be some custom claims, are not supported out of the box, and are out of scope for this PR i believe. @danielqsj Would be awesome if you could weigh in here, and let me know if this is something we can get merged. I am happy to do any updates and alterations needed. |
|
I am hoping to use this feature when merged. |
|
I have sent @danielqsj an email, but have received no response. He is most likely too busy, so i think anyone needing this urgently should look for other solutions. In the meantime i hope this will still get approved once danielqsj has more time. |
kafka_exporter.go
Outdated
| accessToken = o.token | ||
| err = nil | ||
| } else { | ||
| token, _err := o.oauth2Config.Token(ctx) |
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.
currenttimeandctxonly be used once, so I suggest that no need to define them before- is it need to use
_errhere? Can we just useerr?
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.
Both points are correct. I am new to GO and had some false assumptions.
I have updated the code.
|
@LuxTheDude thanks, some comments left |
@danielqsj |
This PR introduces support for the SASL mechanism OAuthBearer.
It introduces a single new configuration variable "sasl.oauthbearer-token-url" which is required argument for using OAuthBearer.
It also updates the Helm Chart to include this variable in its values.
I have tested it on a Kafka instance that used Keycloak as the token provider, and it worked as intended.
I hope this small addition can be reviewed and merged rather quickly as I urgently need it.
It solves #422