Add SASL_SCRAM authentication mechanism on kafka exporter #2322

altieresfreitas · 2020-12-29T02:41:07Z

Description:
This PR adds SASL/SCRAM authentication to kafka exporter and receiver.

Link to tracking Issue:
Resolves #2252

Documentation:
Added to readme

linux-foundation-easycla · 2020-12-29T02:41:09Z

The committers are authorized under a signed CLA.

✅ Altieres Freitas (097c438)

github-actions · 2021-01-05T03:46:30Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

tigrannajaryan · 2021-01-07T17:16:54Z

@jpkrohling can you review this please?

jpkrohling · 2021-01-11T15:50:26Z

Sorry, was on a long break. Will review this one soon.

jpkrohling

This is missing quite a few tests, but looks like it's a good start.

exporter/kafkaexporter/authentication.go

exporter/kafkaexporter/scram_client.go

altieresfreitas · 2021-01-12T04:13:21Z

This is missing quite a few tests, but looks like it's a good start.
Thanks for your comments @jpkrohling, I'll write the tests this week.

exporter/kafkaexporter/authentication.go

exporter/kafkaexporter/scram_client.go

jpkrohling

This is still missing a test and the build seems to be failing, but other than that, looks sane.

altieresfreitas · 2021-01-25T20:12:40Z

Hi @jpkrohling, sorry about the delay.

I've written some unit tests and changed the Authentication type from SCRAM to SASL.
In my point of view, to use sasl.mechanism = "SCRAM-SHA-256| SCRAM-SHA-512|PLAIN" is more clear and compliant with what is described by SASL Mechanisms and very similar with kafka and this PR merged on jaeger project.

Maybe PlainText could be deprecated in favor of sasl.mechanism = "PLAIN".

linux-foundation-easycla · 2021-01-25T20:53:54Z

The committers are authorized under a signed CLA.

✅ Altieres Freitas (0a253e8)

jpkrohling

Looks good then, it's only missing the PLAIN in one place.

exporter/kafkaexporter/README.md

exporter/kafkaexporter/authentication_test.go

jpkrohling

I haven't tried this PR myself as it's not that easy for me to get a properly configured Kafka instance, but once @4lt1 confirms that it was manually tested, this is ready to be merged.

altieresfreitas · 2021-01-26T15:45:13Z

I haven't tried this PR myself as it's not that easy for me to get a properly configured Kafka instance, but once @4lt1 confirms that it was manually tested, this is ready to be merged.

@jpkrohling, It was manually tested.

github-actions · 2021-02-05T03:26:46Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

jpkrohling · 2021-02-05T09:53:22Z

@open-telemetry/collector-maintainers, would one of you please review/merge this one?

github-actions · 2021-02-13T03:27:04Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

jpkrohling · 2021-02-15T11:00:02Z

@open-telemetry/collector-maintainers, would one of you please review/merge this one? I think it's only fair if you could also fix the go.mod conflict...

exporter/kafkaexporter/authentication.go

bogdandrutu · 2021-02-15T17:17:58Z

go.mod

@@ -48,6 +48,7 @@ require (
 	github.com/stretchr/testify v1.7.0
 	github.com/tinylib/msgp v1.1.5
 	github.com/uber/jaeger-lib v2.4.0+incompatible
+	github.com/xdg-go/scram v0.0.0-20180814205039-7eeb5667e42c


How ok are we to depend on a very old, not actively maintained package?

Does not even have go[mod/sum].

Honestly? It doesn't worry me that much, as the code is fairly simple and based on a couple of RFCs. Once the standard is implemented, there's not much else to do. I do have thought about the responsiveness of the author in that repository (see the issue opened by @yurishkuro) but I don't see a reason for implementing this ourselves at this moment.

linux-foundation-easycla · 2021-02-17T00:07:37Z

The committers are authorized under a signed CLA.

✅ Altieres Freitas (ffc1192)

Fix godoc comment to better explain struct proposal Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de> Fix return error message format Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de> Add Unit tests Change Authentication config to SASL Use SASL mechanisms as described in RFC https://tools.ietf.org/html/rfc4422#section-3.1 Add PLAIN mechanism on README documentation Fix Typo on test comment Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de> Add comments on SASLConfig describing expected values

bogdandrutu · 2021-02-17T00:58:59Z

@4lt1 do you mind creating a new PR (just clone the branch and open a new PR) to make circleCI trigger to run the tests? Thank you, and sorry for this

altieresfreitas · 2021-02-17T05:42:55Z

@4lt1 do you mind creating a new PR (just clone the branch and open a new PR) to make circleCI trigger to run the tests? Thank you, and sorry for this

No problem at all @bogdandrutu, the new PR is #2503

jpkrohling · 2021-02-17T08:13:36Z

I'm closing this in favor of #2503

Generated from a modified version of the v1.5.0 release of the specification using a modified version of the semconvgen tools. The specification contained capitalized IDs that caused errors from the semconvgen tool. These errors were manually resolved and the rest of the Go formatting tools were used to provide consistent code. Important to note, the Go semconvgen tooling includes new name capitalization rules for ReplicaSet, StatefulSet, and DaemonSet that mean code names are not backwards compatible. This is included in the changelog to help users perform the upgrade.

…y#2322) Bumps [go.etcd.io/etcd/client/v2](https://github.com/etcd-io/etcd) from 2.305.5 to 2.305.6. - [Release notes](https://github.com/etcd-io/etcd/releases) - [Changelog](https://github.com/etcd-io/etcd/blob/main/Dockerfile-release.amd64) - [Commits](etcd-io/etcd@client/v2.305.5...client/v2.305.6) --- updated-dependencies: - dependency-name: go.etcd.io/etcd/client/v2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

altieresfreitas requested a review from a team December 29, 2020 02:41

github-actions bot added the Stale label Jan 5, 2021

tigrannajaryan assigned jpkrohling Jan 7, 2021

jpkrohling reviewed Jan 11, 2021

View reviewed changes

altieresfreitas force-pushed the kafka_sasl_scram branch from 097c438 to b6211f3 Compare January 12, 2021 03:48

github-actions bot removed the Stale label Jan 12, 2021

jpkrohling reviewed Jan 12, 2021

View reviewed changes

exporter/kafkaexporter/authentication.go Outdated Show resolved Hide resolved

exporter/kafkaexporter/authentication.go Outdated Show resolved Hide resolved

exporter/kafkaexporter/scram_client.go Show resolved Hide resolved

jpkrohling mentioned this pull request Jan 12, 2021

WIP - Adding scram authentication for kafka jaegertracing/jaeger#2110

Closed

jpkrohling reviewed Jan 19, 2021

View reviewed changes

altieresfreitas force-pushed the kafka_sasl_scram branch from acee1a1 to 1ed9d6f Compare January 25, 2021 19:28

altieresfreitas force-pushed the kafka_sasl_scram branch from 1ed9d6f to 0a253e8 Compare January 25, 2021 20:21

altieresfreitas force-pushed the kafka_sasl_scram branch from 33354ac to 0a253e8 Compare January 25, 2021 21:18

jpkrohling reviewed Jan 26, 2021

View reviewed changes

exporter/kafkaexporter/README.md Outdated Show resolved Hide resolved

exporter/kafkaexporter/authentication_test.go Outdated Show resolved Hide resolved

altieresfreitas force-pushed the kafka_sasl_scram branch 2 times, most recently from 32bbbde to 4f8d110 Compare January 26, 2021 15:30

jpkrohling approved these changes Jan 26, 2021

View reviewed changes

altieresfreitas force-pushed the kafka_sasl_scram branch from 42d9d88 to 4f8d110 Compare January 26, 2021 16:10

Base automatically changed from master to main January 28, 2021 18:06

github-actions bot added the Stale label Feb 5, 2021

github-actions bot removed the Stale label Feb 6, 2021

github-actions bot added the Stale label Feb 13, 2021

bogdandrutu reviewed Feb 15, 2021

View reviewed changes

exporter/kafkaexporter/authentication.go Show resolved Hide resolved

bogdandrutu reviewed Feb 15, 2021

View reviewed changes

github-actions bot removed the Stale label Feb 16, 2021

altieresfreitas force-pushed the kafka_sasl_scram branch from 1c86efc to ffc1192 Compare February 17, 2021 00:22

jpkrohling mentioned this pull request Feb 17, 2021

Add SASL/SCRAM authentication mechanism on kafka receiver and exporter #2503

Merged

jpkrohling closed this Feb 17, 2021

Troels51 pushed a commit to Troels51/opentelemetry-collector that referenced this pull request Jul 5, 2024

Fix lifetime issues in test utils (open-telemetry#2322)

ca67af0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SASL_SCRAM authentication mechanism on kafka exporter #2322

Add SASL_SCRAM authentication mechanism on kafka exporter #2322

altieresfreitas commented Dec 29, 2020 •

edited

Loading

linux-foundation-easycla bot commented Dec 29, 2020 •

edited

Loading

github-actions bot commented Jan 5, 2021

tigrannajaryan commented Jan 7, 2021

jpkrohling commented Jan 11, 2021

jpkrohling left a comment

altieresfreitas commented Jan 12, 2021

jpkrohling left a comment

altieresfreitas commented Jan 25, 2021 •

edited

Loading

linux-foundation-easycla bot commented Jan 25, 2021 •

edited

Loading

jpkrohling left a comment

jpkrohling left a comment

altieresfreitas commented Jan 26, 2021

github-actions bot commented Feb 5, 2021

jpkrohling commented Feb 5, 2021

github-actions bot commented Feb 13, 2021

jpkrohling commented Feb 15, 2021 •

edited

Loading

bogdandrutu Feb 15, 2021

jpkrohling Feb 16, 2021

linux-foundation-easycla bot commented Feb 17, 2021 •

edited

Loading

bogdandrutu commented Feb 17, 2021

altieresfreitas commented Feb 17, 2021

jpkrohling commented Feb 17, 2021

Add SASL_SCRAM authentication mechanism on kafka exporter #2322

Add SASL_SCRAM authentication mechanism on kafka exporter #2322

Conversation

altieresfreitas commented Dec 29, 2020 • edited Loading

linux-foundation-easycla bot commented Dec 29, 2020 • edited Loading

github-actions bot commented Jan 5, 2021

tigrannajaryan commented Jan 7, 2021

jpkrohling commented Jan 11, 2021

jpkrohling left a comment

Choose a reason for hiding this comment

altieresfreitas commented Jan 12, 2021

jpkrohling left a comment

Choose a reason for hiding this comment

altieresfreitas commented Jan 25, 2021 • edited Loading

linux-foundation-easycla bot commented Jan 25, 2021 • edited Loading

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

altieresfreitas commented Jan 26, 2021

github-actions bot commented Feb 5, 2021

jpkrohling commented Feb 5, 2021

github-actions bot commented Feb 13, 2021

jpkrohling commented Feb 15, 2021 • edited Loading

bogdandrutu Feb 15, 2021

Choose a reason for hiding this comment

jpkrohling Feb 16, 2021

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Feb 17, 2021 • edited Loading

bogdandrutu commented Feb 17, 2021

altieresfreitas commented Feb 17, 2021

jpkrohling commented Feb 17, 2021

altieresfreitas commented Dec 29, 2020 •

edited

Loading

linux-foundation-easycla bot commented Dec 29, 2020 •

edited

Loading

altieresfreitas commented Jan 25, 2021 •

edited

Loading

linux-foundation-easycla bot commented Jan 25, 2021 •

edited

Loading

jpkrohling commented Feb 15, 2021 •

edited

Loading

linux-foundation-easycla bot commented Feb 17, 2021 •

edited

Loading