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

kafkaexporter: Add support for AWS_MSK_IAM SASL Auth #5763

Conversation

MovieStoreGuy
Copy link
Contributor

Description:

This allows for developers to use this the existing kafka exporter and use the newly minted AWS_MSK_IAM SASL auth.

Link to tracking Issue: #5009
In a very loose definition of related ticket.

Testing:
I have some rather basic testing locally to see what I can get done with this.
I had followed https://github.com/aws/aws-msk-iam-auth#details as close as I could with this.

Documentation:
I haven't added any new documentation for this since I hadn't have the chance to actually validate this in a production like setting so I am hoping to leave it as a dark feature for the time being.

@MovieStoreGuy MovieStoreGuy requested a review from a team October 15, 2021 04:17
@MovieStoreGuy MovieStoreGuy force-pushed the smarciniak/add-initial-awsmskexporter branch from 1d86d7a to 99f868c Compare October 15, 2021 04:19
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

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

I'm glad the kafka exporter's auth is already pluggable enough to make this fairly straightforward

exporter/kafkaexporter/authentication.go Outdated Show resolved Hide resolved
exporter/kafkaexporter/internal/msk/iam_scram_client.go Outdated Show resolved Hide resolved
exporter/kafkaexporter/internal/msk/iam_scram_client.go Outdated Show resolved Hide resolved
exporter/kafkaexporter/internal/msk/doc.go Outdated Show resolved Hide resolved
exporter/kafkaexporter/internal/msk/iam_scram_client.go Outdated Show resolved Hide resolved
@MovieStoreGuy MovieStoreGuy force-pushed the smarciniak/add-initial-awsmskexporter branch from 88102f6 to ff25d82 Compare October 18, 2021 02:03
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Needs a rebase.

exporter/kafkaexporter/internal/awsmsk/iam_scram_client.go Outdated Show resolved Hide resolved
@MovieStoreGuy MovieStoreGuy force-pushed the smarciniak/add-initial-awsmskexporter branch 3 times, most recently from 2a748e2 to 55061c3 Compare October 19, 2021 22:32
@MovieStoreGuy
Copy link
Contributor Author

@bogdandrutu , @tigrannajaryan ,

Could I please get this merged in?

@tigrannajaryan
Copy link
Member

@MovieStoreGuy sorry, needs a rebase again. Once you do it ping me directly so that I can merge. If it stays open there is a chance go.sum will conflict again.

@MovieStoreGuy
Copy link
Contributor Author

No worries, let me resolve this and I shall get back to you.

Sean ZO Marciniak added 9 commits October 26, 2021 11:59
I have roughly worked through and figured out what needs to be done to
support this, however it is currently untested in production so should
be clear is a beta feature.
- Tidy up of over eager human optimisation
- Updated config
@MovieStoreGuy MovieStoreGuy force-pushed the smarciniak/add-initial-awsmskexporter branch from 55061c3 to c3ed66d Compare October 26, 2021 01:32
@MovieStoreGuy
Copy link
Contributor Author

@tigrannajaryan, would you mind merging?

@tigrannajaryan tigrannajaryan merged commit 67e165e into open-telemetry:main Oct 26, 2021
@MovieStoreGuy MovieStoreGuy deleted the smarciniak/add-initial-awsmskexporter branch October 26, 2021 21:57
datsabk added a commit to datsabk/opentelemetry-collector-contrib that referenced this pull request Mar 14, 2023
Updated documentation to reflect the changes made in (this PR)[open-telemetry#5763]
djaglowski pushed a commit that referenced this pull request Mar 17, 2023
Update README.md

Updated documentation to reflect the changes made in (this PR)[#5763]
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.

5 participants