Skip to content

Conversation

toondaey
Copy link
Contributor

@toondaey toondaey commented Aug 7, 2023

Closes #

Describe your proposed changes here.

  • I've read the contributing section of the project CONTRIBUTING.md.
  • Signed CLA (if not already signed).

@toondaey toondaey changed the title Sasl impl feat: sasl auth impl Aug 7, 2023
@crepererum
Copy link
Collaborator

I see the commit msg checker fails here. You can probably squash everything into a single commit and mention the other two authors in the new "mega commit. See https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors on how to do this.

ramrengaswamy and others added 3 commits August 7, 2023 15:37
- PLAIN

[WIP]: Address review comments.

[WIP] - Rust format

WIP: Handle version 0 and 1 for SaslAuthenticateRequest

WIP - Expose SaslConfig struct from Client

WIP: Fix clippy errors

WIP: Rust fmt checks

WIP: Add client SASL test

WIP: Run Sasl integration test for Kafka

Skip Redpanda test, Updated README

feat: add sasl authentication

Adds PLAIN sasl authentication

[WIP]: Address review comments.

[WIP] - Rust format

WIP: Handle version 0 and 1 for SaslAuthenticateRequest

WIP - Expose SaslConfig struct from Client

WIP: Fix clippy errors

WIP: Rust fmt checks

WIP: Add client SASL test

WIP: Run Sasl integration test for Kafka
Co-authored-by: Ram Kumar Rengaswamy <[email protected]>
Co-authored-by: Seiji Kasahar <[email protected]>
Copy link
Collaborator

@crepererum crepererum left a comment

Choose a reason for hiding this comment

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

Minor nits, looks very good 👍

pub(crate) fn auth_bytes(&self) -> Vec<u8> {
match self {
Self::Plain { username, password } => {
let mut auth: Vec<u8> = vec![0];
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIRC I was asking for this in the original PR but it got lost in the meantime: could you add a link to the SASL spec that describes the PLAIN auth. I think this is https://datatracker.ietf.org/doc/html/rfc4616.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes so much sense.

@toondaey toondaey marked this pull request as ready for review August 7, 2023 16:53
@toondaey
Copy link
Contributor Author

toondaey commented Aug 7, 2023

All updated now.

@toondaey toondaey requested a review from crepererum August 7, 2023 17:06
@crepererum crepererum added the automerge Instruct kodiak to merge the PR label Aug 8, 2023
@kodiakhq kodiakhq bot merged commit 2f371bd into influxdata:main Aug 8, 2023
@crepererum
Copy link
Collaborator

thank you 🙂

@toondaey
Copy link
Contributor Author

toondaey commented Aug 8, 2023

thank you 🙂

You're welcome! Not to be selfish in anyway, but I wanted to ask if there's a release cycle for this package?

@crepererum
Copy link
Collaborator

You're welcome! Not to be selfish in anyway, but I wanted to ask if there's a release cycle for this package?

On demand, I try to get a release out in the next couple of days.

@toondaey
Copy link
Contributor Author

You're welcome! Not to be selfish in anyway, but I wanted to ask if there's a release cycle for this package?

On demand, I try to get a release out in the next couple of days.

Ok. Thanks! Looking forward to the next release.😀

@crepererum
Copy link
Collaborator

@toondaey 0.5.0 is released ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Instruct kodiak to merge the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants