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

altieresfreitas · 2021-02-17T05:36:29Z

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

Link to tracking Issue:
#2252

Testing:
Unit Tests

Documentation:
Added to Readme

jpkrohling · 2021-02-17T08:07:14Z

~~Before reviewing this, isn't this in conflict with your other PR, #2322?~~

Sorry, I should have read the newest comments on the linked PR... I'll take a look soon.

jpkrohling · 2021-02-17T08:12:56Z

The same test seems to have failed, but I doubt this test alone would be the culprit.

These are the results for the current main branch:

Test                                    |Result|Duration|CPU Avg%|CPU Max%|RAM Avg MiB|RAM Max MiB|Sent Items|Received Items|
----------------------------------------|------|-------:|-------:|-------:|----------:|----------:|---------:|-------------:|
TraceNoBackend10kSPS/NoMemoryLimit      |PASS  |     15s|    28.3|    32.3|        108|        149|    148190|             0|
TraceNoBackend10kSPS/MemoryLimit        |PASS  |     15s|    30.9|    32.3|         57|         69|    146890|             0|
Trace1kSPSWithAttrs/0*0bytes            |PASS  |     15s|     6.9|     8.0|         49|         60|     14990|         14990|
Trace1kSPSWithAttrs/100*50bytes         |PASS  |     15s|    35.6|    36.0|         55|         66|     14990|         14990|
Trace1kSPSWithAttrs/10*1000bytes        |PASS  |     15s|    14.7|    15.7|         51|         62|     14990|         14990|
Trace1kSPSWithAttrs/20*5000bytes        |PASS  |     15s|    46.4|    47.0|         60|         74|     14990|         14990|

And here are the results for this PR:

Test                                    |Result|Duration|CPU Avg%|CPU Max%|RAM Avg MiB|RAM Max MiB|Sent Items|Received Items|
----------------------------------------|------|-------:|-------:|-------:|----------:|----------:|---------:|-------------:|
TraceNoBackend10kSPS/NoMemoryLimit      |FAIL  |     12s|    35.0|    37.3|         98|        151|    119180|             0|RAM consumption is 151 MiB, max expected is 150 MiB
TraceNoBackend10kSPS/MemoryLimit        |PASS  |     15s|    33.8|    35.7|         58|         70|    145600|             0|
Trace1kSPSWithAttrs/0*0bytes            |PASS  |     15s|     8.1|     8.7|         52|         60|     14990|         14990|
Trace1kSPSWithAttrs/100*50bytes         |PASS  |     15s|    32.2|    32.7|         54|         66|     14990|         14990|
Trace1kSPSWithAttrs/10*1000bytes        |PASS  |     15s|    14.5|    15.3|         52|         63|     14990|         14990|
Trace1kSPSWithAttrs/20*5000bytes        |PASS  |     15s|    42.9|    43.3|         59|         73|     14990|         14990|

jpkrohling

LGTM. One improvement could be checking that both the username and password are set.

exporter/kafkaexporter/authentication.go

altieresfreitas · 2021-02-17T16:09:12Z

The same test seems to have failed, but I doubt this test alone would be the culprit.

These are the results for the current main branch:

Test                                    |Result|Duration|CPU Avg%|CPU Max%|RAM Avg MiB|RAM Max MiB|Sent Items|Received Items|
----------------------------------------|------|-------:|-------:|-------:|----------:|----------:|---------:|-------------:|
TraceNoBackend10kSPS/NoMemoryLimit      |PASS  |     15s|    28.3|    32.3|        108|        149|    148190|             0|
TraceNoBackend10kSPS/MemoryLimit        |PASS  |     15s|    30.9|    32.3|         57|         69|    146890|             0|
Trace1kSPSWithAttrs/0*0bytes            |PASS  |     15s|     6.9|     8.0|         49|         60|     14990|         14990|
Trace1kSPSWithAttrs/100*50bytes         |PASS  |     15s|    35.6|    36.0|         55|         66|     14990|         14990|
Trace1kSPSWithAttrs/10*1000bytes        |PASS  |     15s|    14.7|    15.7|         51|         62|     14990|         14990|
Trace1kSPSWithAttrs/20*5000bytes        |PASS  |     15s|    46.4|    47.0|         60|         74|     14990|         14990|

And here are the results for this PR:

Test                                    |Result|Duration|CPU Avg%|CPU Max%|RAM Avg MiB|RAM Max MiB|Sent Items|Received Items|
----------------------------------------|------|-------:|-------:|-------:|----------:|----------:|---------:|-------------:|
TraceNoBackend10kSPS/NoMemoryLimit      |FAIL  |     12s|    35.0|    37.3|         98|        151|    119180|             0|RAM consumption is 151 MiB, max expected is 150 MiB
TraceNoBackend10kSPS/MemoryLimit        |PASS  |     15s|    33.8|    35.7|         58|         70|    145600|             0|
Trace1kSPSWithAttrs/0*0bytes            |PASS  |     15s|     8.1|     8.7|         52|         60|     14990|         14990|
Trace1kSPSWithAttrs/100*50bytes         |PASS  |     15s|    32.2|    32.7|         54|         66|     14990|         14990|
Trace1kSPSWithAttrs/10*1000bytes        |PASS  |     15s|    14.5|    15.3|         52|         63|     14990|         14990|
Trace1kSPSWithAttrs/20*5000bytes        |PASS  |     15s|    42.9|    43.3|         59|         73|     14990|         14990|

Probably some transient failure here.

The result for the same PR.

Started: Wed, 17 Feb 2021 c16:00:45 +0000

Test	Result	Duration	CPU Avg%	CPU Max%	RAM Avg MiB	RAM Max MiB	Sent Items	Received Items
TraceNoBackend10kSPS/NoMemoryLimit	PASS	15s	27.3	31.0	109	150	148010	0
TraceNoBackend10kSPS/MemoryLimit	PASS	15s	31.0	32.0	57	69	145700	0
Trace1kSPSWithAttrs/0*0bytes	PASS	15s	6.7	8.3	52	63	14990	14990
Trace1kSPSWithAttrs/100*50bytes	PASS	15s	34.1	35.7	54	65	14990	14990
Trace1kSPSWithAttrs/10*1000bytes	PASS	15s	14.5	15.0	53	64	14990	14990
Trace1kSPSWithAttrs/20*5000bytes	PASS	15s	44.6	46.3	60	75	14990	14990

altieresfreitas · 2021-02-17T16:20:48Z

@bogdandrutu, can you take a look at this PR, please?
This is related to #2322

Thanks

codecov · 2021-02-18T01:02:05Z

Codecov Report

Merging #2503 (6b25ed1) into main (846b971) will decrease coverage by 0.04%.
The diff coverage is 68.96%.

@@            Coverage Diff             @@
##             main    #2503      +/-   ##
==========================================
- Coverage   91.76%   91.71%   -0.05%     
==========================================
  Files         266      267       +1     
  Lines       15111    15140      +29     
==========================================
+ Hits        13866    13886      +20     
- Misses        866      874       +8     
- Partials      379      380       +1

Impacted Files	Coverage Δ
exporter/kafkaexporter/scram_client.go	`0.00% <0.00%> (ø)`
exporter/kafkaexporter/authentication.go	`96.07% <90.90%> (-3.93%)`	⬇️
testutil/testutil.go	`81.60% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 846b971...6b25ed1. Read the comment docs.

jpkrohling

This can be merged. I have only one minor comment about an error message, but it's in no way blocking the merge.

exporter/kafkaexporter/authentication.go

tigrannajaryan · 2021-02-18T16:11:30Z

@bogdandrutu do you plan to review this or good to merge?

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> Fix Typo on test comment Co-authored-by: Juraci Paixão Kröhling <juraci.github@kroehling.de> Add username and password validation to prevent empty values Add specific error message on username and password validation

gramidt

LGTM

jpkrohling · 2021-02-23T08:04:41Z

@tigrannajaryan would you please merge this one?

Neustradamus · 2022-01-11T00:23:09Z

@altieresfreitas and all others: Thanks a lot for all about SCRAM.

Linked to:

State of Play scram-sasl/info#1

…en-telemetry#2503)

altieresfreitas requested a review from a team February 17, 2021 05:36

altieresfreitas mentioned this pull request Feb 17, 2021

Add SASL_SCRAM authentication mechanism on kafka exporter #2322

Closed

jpkrohling self-assigned this Feb 17, 2021

jpkrohling self-requested a review February 17, 2021 08:07

jpkrohling approved these changes Feb 17, 2021

View reviewed changes

exporter/kafkaexporter/authentication.go Show resolved Hide resolved

altieresfreitas force-pushed the kafka_scram branch 2 times, most recently from 3a72fb3 to 8906b09 Compare February 17, 2021 15:52

jpkrohling approved these changes Feb 18, 2021

View reviewed changes

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

altieresfreitas force-pushed the kafka_scram branch 8 times, most recently from 16f086e to e792973 Compare February 18, 2021 21:50

altieresfreitas force-pushed the kafka_scram branch from e792973 to 6b25ed1 Compare February 18, 2021 22:07

gramidt approved these changes Feb 19, 2021

View reviewed changes

bogdandrutu merged commit 49d0b61 into open-telemetry:main Feb 23, 2021

Neustradamus mentioned this pull request Jan 11, 2022

Add SASL/SCRAM authentication to Kafka receiver and exporter open-telemetry/opentelemetry-collector-contrib#5009

Closed

nurdann mentioned this pull request May 2, 2024

Does otel agent support AWS MSK authentication via provider chain? #10073

Open

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

[BUILD] Better handling of OPENTELEMETRY_STL_VERSION under Bazel. (op…

497eaf4

…en-telemetry#2503)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

altieresfreitas commented Feb 17, 2021

jpkrohling commented Feb 17, 2021 •

edited

Loading

jpkrohling commented Feb 17, 2021

jpkrohling left a comment

altieresfreitas commented Feb 17, 2021

altieresfreitas commented Feb 17, 2021

codecov bot commented Feb 18, 2021 •

edited

Loading

jpkrohling left a comment

tigrannajaryan commented Feb 18, 2021

gramidt left a comment

jpkrohling commented Feb 23, 2021

Neustradamus commented Jan 11, 2022

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

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

Conversation

altieresfreitas commented Feb 17, 2021

jpkrohling commented Feb 17, 2021 • edited Loading

jpkrohling commented Feb 17, 2021

jpkrohling left a comment

Choose a reason for hiding this comment

altieresfreitas commented Feb 17, 2021

altieresfreitas commented Feb 17, 2021

codecov bot commented Feb 18, 2021 • edited Loading

Codecov Report

jpkrohling left a comment

Choose a reason for hiding this comment

tigrannajaryan commented Feb 18, 2021

gramidt left a comment

Choose a reason for hiding this comment

jpkrohling commented Feb 23, 2021

Neustradamus commented Jan 11, 2022

jpkrohling commented Feb 17, 2021 •

edited

Loading

codecov bot commented Feb 18, 2021 •

edited

Loading