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

Add kafka SASL plain authentication support #9584

Merged
merged 7 commits into from
Oct 11, 2024
Merged

Conversation

pracucci
Copy link
Collaborator

What this PR does

Add SASL plain authentication support to Kafka client both to Mimir and kafkatool. I also manually tested these changes.

Which issue(s) this PR fixes or relates to

N/A

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

@pracucci pracucci requested review from stevesg, grafanabot, a team and tacole02 as code owners October 10, 2024 15:53
@pracucci pracucci changed the title Add kafka sasl support Add kafka SASL plain authentication support Oct 10, 2024
Copy link
Contributor

@tacole02 tacole02 left a comment

Choose a reason for hiding this comment

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

Thank you!

Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
@pracucci pracucci force-pushed the add-kafka-sasl-support branch from 0cc5f95 to 1b51166 Compare October 11, 2024 07:19
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

sgtm

Copy link
Contributor

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM, great job - simple and clean!

@@ -115,6 +120,9 @@ func (cfg *KafkaConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet)
f.DurationVar(&cfg.WriteTimeout, prefix+".write-timeout", 10*time.Second, "How long to wait for an incoming write request to be successfully committed to the Kafka backend.")
f.IntVar(&cfg.WriteClients, prefix+".write-clients", 1, "The number of Kafka clients used by producers. When the configured number of clients is greater than 1, partitions are sharded among Kafka clients. A higher number of clients may provide higher write throughput at the cost of additional Metadata requests pressure to Kafka.")

f.StringVar(&cfg.SASLUsername, prefix+".sasl-username", "", "The username used to authenticate to Kafka using the SASL plain mechanism. To enable SASL, configure both the username and password.")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'm not sure which patterns we follow and if its common that when requiring multiple config to be set it basically no-ops unless you set them all. The flag documentation is very clear that you need both hence why this is a nit, but...

I can see from the library that:

	if auth.User == "" || auth.Pass == "" {
		return nil, nil, errors.New("PLAIN user and pass must be non-empty")
	}

hence the reasoning behind not setting it unless you get both, but I wonder if it's convenient to validate that here or let the error bubble up.

I'll leave the decision in your hands and this is considered a very nitpicky nit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we can add a validation in our config. Let me do it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in 8cef267

Signed-off-by: Marco Pracucci <[email protected]>
Copy link
Contributor

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Marco Pracucci <[email protected]>
@pracucci pracucci enabled auto-merge (squash) October 11, 2024 10:09
@pracucci pracucci merged commit 42284e7 into main Oct 11, 2024
29 checks passed
@pracucci pracucci deleted the add-kafka-sasl-support branch October 11, 2024 10:22
@grafanabot
Copy link
Contributor

The backport to r310 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-9584-to-r310 origin/r310
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 42284e756e4f6c10d9111444fe4cb53c138ce3e6
# Push it to GitHub
git push --set-upstream origin backport-9584-to-r310
git switch main
# Remove the local backport branch
git branch -D backport-9584-to-r310

Then, create a pull request where the base branch is r310 and the compare/head branch is backport-9584-to-r310.

@grafanabot
Copy link
Contributor

The backport to r311 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-9584-to-r311 origin/r311
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 42284e756e4f6c10d9111444fe4cb53c138ce3e6
# Push it to GitHub
git push --set-upstream origin backport-9584-to-r311
git switch main
# Remove the local backport branch
git branch -D backport-9584-to-r311

Then, create a pull request where the base branch is r311 and the compare/head branch is backport-9584-to-r311.

pracucci added a commit that referenced this pull request Oct 24, 2024
* Add SASL plain support to kafkatool

Signed-off-by: Marco Pracucci <[email protected]>

* Add SASL plain authentication support to Kafka client

Signed-off-by: Marco Pracucci <[email protected]>

* Update CHANGELOG

Signed-off-by: Marco Pracucci <[email protected]>

* Fix test compilation issue

Signed-off-by: Marco Pracucci <[email protected]>

* Address review feedback

Signed-off-by: Marco Pracucci <[email protected]>

* Add config validation

Signed-off-by: Marco Pracucci <[email protected]>

* Fix linter issue

Signed-off-by: Marco Pracucci <[email protected]>

---------

Signed-off-by: Marco Pracucci <[email protected]>
pracucci added a commit that referenced this pull request Oct 24, 2024
* Add SASL plain support to kafkatool

Signed-off-by: Marco Pracucci <[email protected]>

* Add SASL plain authentication support to Kafka client

Signed-off-by: Marco Pracucci <[email protected]>

* Update CHANGELOG

Signed-off-by: Marco Pracucci <[email protected]>

* Fix test compilation issue

Signed-off-by: Marco Pracucci <[email protected]>

* Address review feedback

Signed-off-by: Marco Pracucci <[email protected]>

* Add config validation

Signed-off-by: Marco Pracucci <[email protected]>

* Fix linter issue

Signed-off-by: Marco Pracucci <[email protected]>

---------

Signed-off-by: Marco Pracucci <[email protected]>
pracucci added a commit that referenced this pull request Oct 24, 2024
* Add SASL plain support to kafkatool

Signed-off-by: Marco Pracucci <[email protected]>

* Add SASL plain authentication support to Kafka client

Signed-off-by: Marco Pracucci <[email protected]>

* Update CHANGELOG

Signed-off-by: Marco Pracucci <[email protected]>

* Fix test compilation issue

Signed-off-by: Marco Pracucci <[email protected]>

* Address review feedback

Signed-off-by: Marco Pracucci <[email protected]>

* Add config validation

Signed-off-by: Marco Pracucci <[email protected]>

* Fix linter issue

Signed-off-by: Marco Pracucci <[email protected]>

---------

Signed-off-by: Marco Pracucci <[email protected]>
pracucci added a commit that referenced this pull request Oct 24, 2024
* Add SASL plain support to kafkatool

Signed-off-by: Marco Pracucci <[email protected]>

* Add SASL plain authentication support to Kafka client

Signed-off-by: Marco Pracucci <[email protected]>

* Update CHANGELOG

Signed-off-by: Marco Pracucci <[email protected]>

* Fix test compilation issue

Signed-off-by: Marco Pracucci <[email protected]>

* Address review feedback

Signed-off-by: Marco Pracucci <[email protected]>

* Add config validation

Signed-off-by: Marco Pracucci <[email protected]>

* Fix linter issue

Signed-off-by: Marco Pracucci <[email protected]>

---------

Signed-off-by: Marco Pracucci <[email protected]>
pracucci added a commit that referenced this pull request Oct 24, 2024
* Add SASL plain support to kafkatool

Signed-off-by: Marco Pracucci <[email protected]>

* Add SASL plain authentication support to Kafka client

Signed-off-by: Marco Pracucci <[email protected]>

* Update CHANGELOG

Signed-off-by: Marco Pracucci <[email protected]>

* Fix test compilation issue

Signed-off-by: Marco Pracucci <[email protected]>

* Address review feedback

Signed-off-by: Marco Pracucci <[email protected]>

* Add config validation

Signed-off-by: Marco Pracucci <[email protected]>

* Fix linter issue

Signed-off-by: Marco Pracucci <[email protected]>

---------

Signed-off-by: Marco Pracucci <[email protected]>
pracucci added a commit that referenced this pull request Oct 24, 2024
* Add SASL plain support to kafkatool

Signed-off-by: Marco Pracucci <[email protected]>

* Add SASL plain authentication support to Kafka client

Signed-off-by: Marco Pracucci <[email protected]>

* Update CHANGELOG

Signed-off-by: Marco Pracucci <[email protected]>

* Fix test compilation issue

Signed-off-by: Marco Pracucci <[email protected]>

* Address review feedback

Signed-off-by: Marco Pracucci <[email protected]>

* Add config validation

Signed-off-by: Marco Pracucci <[email protected]>

* Fix linter issue

Signed-off-by: Marco Pracucci <[email protected]>

---------

Signed-off-by: Marco Pracucci <[email protected]>
pracucci added a commit that referenced this pull request Oct 24, 2024
* Add SASL plain support to kafkatool

Signed-off-by: Marco Pracucci <[email protected]>

* Add SASL plain authentication support to Kafka client

Signed-off-by: Marco Pracucci <[email protected]>

* Update CHANGELOG

Signed-off-by: Marco Pracucci <[email protected]>

* Fix test compilation issue

Signed-off-by: Marco Pracucci <[email protected]>

* Address review feedback

Signed-off-by: Marco Pracucci <[email protected]>

* Add config validation

Signed-off-by: Marco Pracucci <[email protected]>

* Fix linter issue

Signed-off-by: Marco Pracucci <[email protected]>

---------

Signed-off-by: Marco Pracucci <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants