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

Kafka Franz - add aws msk iam #1295

Merged
merged 6 commits into from
Jun 19, 2022

Conversation

ekeric13
Copy link
Contributor

@ekeric13 ekeric13 commented Jun 15, 2022

Still need to QA

Related:
#1292
#1293

Comment on lines +65 to +69
credentials:
profile: ""
id: ""
secret: ""
token: ""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

instead of explicitly defining these in the kafka yaml, a user can simply just pass in AWS_ACCESS_KEY and AWS_SECRET_KEY. And it will be caught due to the behavior of this:
https://github.com/ekeric13/benthos/blob/9595631f6c0d1514ab9bc5006e9d38b7421cabc3/internal/impl/aws/session.go#L52

correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, it's fine to leave the credentials fields empty or undefined.

@Jeffail
Copy link
Collaborator

Jeffail commented Jun 19, 2022

Thanks @ekeric13 looks good. I'm going to merge and then adapt these changes to make the AWS imports optional, similar to a recent change to elasticsearch: e2772b5, it'd be great if you could re-test after that change.

@Jeffail Jeffail merged commit 7050410 into redpanda-data:main Jun 19, 2022
@Jeffail
Copy link
Collaborator

Jeffail commented Jun 19, 2022

I've moved a bit of stuff around: 0ed2244, hope you don't mind but the credentials fields are now within the mechanism object, which would potentially allow you to try two different sets of AWS credentials if needed. It looks like this:

sasl:
  - mechanism: AWS_MSK_IAM
    aws:
      region: us-east-1
      credentials:
         profile: foo

@ekeric13
Copy link
Contributor Author

I don't mind moving stuff around at all. I do want to say that I wasn't able to fully test this one locally so I might have a follow up for it.

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.

None yet

2 participants