-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add AWS MSK IAM authentication to Kafka scaler #5692
Conversation
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.
Hi @adrien-f , thanks for working on this. On a high level note, it think its good to me. Just a few notes from me.
In my opinion, awsRegion
should not be a secret and hence should be in triggerMetadata. Wdyt?
Hey @dttung2905, thanks for the review! Indeed, I was not sure where to put |
3050dba
to
7dad57d
Compare
/run-e2e kafka |
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.
This is great! Thanks for the contribution. Could you please also open keda-docs PR to update documentation for this addition.
@JorTurFer @tomkerkhove imho would be great if we can run e2e test against AWS MSK instance
This will be more complicated that we'd expect because IIRC, MSK instance has to run mandatory using private networking. This means that we have 2 options:
IDK which is the best option, probably we should check the cost in both cases to not consume more than the budget. I don't have time to do this either atm, but if any contributor is willing to add a PR to testing infra adding the required changes, I can review them |
@JorTurFer, it is indeed tricky to test , I totally agree
I just take a peak at the MSK ofifical doc, they do support exposing the broker publicly but you can't make it public at the creation stage
I don't think exposing the bootstrap server endpoint publicly is a good point ( security wise ) and we may still end up with either options as Jorge mentioned above ☝️ |
OK, let's ignore this. Maybe AWS decide to sponsor this in the future :) |
I will get started on updating the documentation! Thanks a lot for the review 👍 |
@adrien-f we plan to do a new release today or tomorrow morning. We can include this change as well, if you are able to provide docs soon. |
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.
Please add docs and Changelog
Fixes !5540 Signed-off-by: Adrien Fillon <adrien.fillon@manomano.com>
Hello @zroubalik ! I've pushed kedacore/keda-docs#1381 and a changelog entry |
/run-e2e kafka |
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.
LGTM
Following the discussion in #5531 and #5540, this adds the support for SASL OAuthBearer with MSK token provider to the Kafka scaler using sarama library.
I've added two new fields,
saslTokenProvider
(authParams or TriggerMetadata) andawsRegion
(TriggerMetadata).It is currently functional, to make it work, here's an example:
I am setting it as a draft while I continue going through the contributing process and pending implementation feedback!
Checklist
Fixes #5540
Relates to #5531