Enables SSL_SASL #4416

abohmeed · 2022-10-31T13:21:18Z

What this PR does / why we need it:
Enables SASL_SSL for Kafka cluster connections
Which issue(s) this PR fixes:
#4415

Fixes #

Special notes for your reviewer:

seldondev · 2022-10-31T13:21:26Z

Hi @abohmeed. Thanks for your PR.

I'm waiting for a SeldonIO or todo member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository.

abohmeed · 2022-10-31T13:22:01Z

/assign @RafalSkolasinski

abohmeed · 2022-10-31T13:42:59Z

@RafalSkolasinski I've reopened the PR since the last one was incorrect

RafalSkolasinski · 2022-10-31T13:46:41Z

@abohmeed thanks for linking the issue - it may be better to re-open the previous PR as @agrski already left some review comments there? There is no need to re-opening as new PR.

abohmeed · 2022-10-31T14:11:35Z

@RafalSkolasinski Thanks. The old PR had unnecessary files modified (like topics.go) on which @agrski placed comments. So, I had to fix my branch and reopen a new PR where only two files are changed: server.go and utils.go. Sorry for any inconvenience.

RafalSkolasinski · 2022-10-31T14:16:35Z

@abohmeed Once you fix your branch and push the PR should automatically update itself. If this is not the case it could be because you opened it from your master branch - you could then try to create feature branch in your fork and open a PR from it. Other than changing the source branch of the PR you should not need to open a new one. Unless I am missing something?

abohmeed · 2022-10-31T14:32:45Z

@RafalSkolasinski Can't we just use this PR instead? I'm afraid I cannot reopen the old one since I re-forked the repo to avoid any issues.

RafalSkolasinski · 2022-10-31T14:35:19Z

Okay, re-forking probably changes the situation. Let's continue here then 👍

abohmeed · 2022-10-31T14:49:48Z

@agrski I've addressed your comments from the previous PR (closed) except for this one #4414 (comment). Can we open a new conversation about it in this PR?

RafalSkolasinski · 2022-10-31T16:16:56Z

/test integration

RafalSkolasinski · 2022-10-31T16:29:14Z

We have executor tests failing with

vet: api/util/utils.go:195:3: unknown field Password in struct literal

RafalSkolasinski · 2022-10-31T16:31:13Z

@abohmeed you should be able to run these tests locally with make test inside executor directory.

abohmeed · 2022-10-31T17:23:23Z

@RafalSkolasinski Thanks for getting back to me.
I made the necessary fixes and ran make test locally and it passed.

RafalSkolasinski · 2022-10-31T17:27:00Z

/test integration

agrski

Thanks again @abohmeed for improving the Kafka support here. Thanks also for tidying up the PR to
These comments aren't blockers, but it'd be good to discuss/implement them to make this PR even more useful.

On the other hand, I think this PR should also update executor/logger/worker.go to add in equivalent config handling there. This is the component that writes inference logs to Kafka (as opposed to a REST endpoint).

agrski · 2022-11-01T11:57:27Z

executor/api/util/utils.go

@@ -157,6 +157,16 @@ func (o SslKakfa) String() string {
 	return "SslKakfa"
 }

+type SaslKafka struct {
+	UserName  string


Suggested change

UserName string

Username string

🙃 Minor point for consistency with Kafka, which treats this as one word rather than two

agrski · 2022-11-01T11:58:58Z

executor/api/kafka/server.go

 			sslKakfaServer := util.GetSslElements()
+			saslKafkaServer := util.GetSaslElements()


💭 The SASL config is only relevant when we know we're using SASL_SSL so belongs within that conditional, ideally. This isn't a major change, but would be a little cleaner.

executor/api/kafka/server.go

abohmeed · 2022-11-02T09:34:10Z

/test integration

seldondev · 2022-11-02T09:34:13Z

@abohmeed: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test integration

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the jenkins-x/lighthouse repository.

abohmeed · 2022-11-02T09:58:31Z

@agrski Thanks for your comments. I've addressed both points.

abohmeed · 2022-11-04T08:54:41Z

@RafalSkolasinski @agrski Any updates? 🙂

agrski · 2022-11-04T11:07:52Z

@RafalSkolasinski @agrski Any updates? slightly_smiling_face

Hi @abohmeed, just getting back to this now. Thanks for being so responsive and applying the recommendations.

agrski · 2022-11-04T11:10:58Z

executor/api/kafka/server.go

+				}
+			}
+		}
+		if util.GetKafkaSecurityProtocol() == "SASL_PLAIN" { //if we also have SASL enabled, then we need to provide the necessary (no SSL)


💭 To avoid the potential for duplication here, structuring this block of code like the below pseudo-code would help:

if protocol == "SSL" || protocol == "SASL_SSL" { getSslElements() // set up SSL } if protocol == "PLAIN" || protocol == "SASL_PLAIN" { getSaslElements() // set up SASL }

Does that makes sense? The idea is that instead of nesting logic, we do SSL setup if we need to, and independently SASL setup if we need to.

This pattern would then apply to all 3 locations where changes need to happen (currently plaintext is only handled in 2 of them!)

Yes, but if the protocol is PLAIN without SASL then why grap SASL username and password at all? afaik PLAIN mean no authentication mechanism.

OK after reading your second comment, I will apply the change (if the docs say so 😉)

agrski · 2022-11-04T11:27:04Z

executor/logger/worker.go

+				}
+			}
+		}
+		if util.GetKafkaSecurityProtocol() == "SASL_PLAIN" { //if we also have SASL enabled, then we need to provide the necessary (no SSL)


🔧 From checking Kafka docs, I'm pretty sure the security protocol should be SASL_PLAINTEXT (or PLAINTEXT) instead of SASL_PLAIN.
It's the SASL mechanism that might be PLAIN.

abohmeed · 2022-11-04T11:48:36Z

@agrski I've addressed your latest comments

abohmeed · 2022-11-07T08:24:57Z

@RafalSkolasinski @agrski Any updates yet? 🙂

agrski

Let's get this in!

seldondev · 2022-11-07T15:10:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: agrski

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [agrski]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

agrski · 2022-11-07T15:54:03Z

The docs test failing feels like flakiness given nothing in this PR affects it, so I'll merge in spite of that.

Enables SSL_SASL

42811af

seldondev added needs-ok-to-test size/M labels Oct 31, 2022

seldondev assigned RafalSkolasinski Oct 31, 2022

RafalSkolasinski requested a review from agrski October 31, 2022 14:34

Addresses reviewer comments

35deeab

Fixes errors when testing

349155e

agrski reviewed Nov 1, 2022

View reviewed changes

Adds SASL_PLAIN support, modifies logger/worker.go

8cc2ee3

agrski reviewed Nov 4, 2022

View reviewed changes

Addresses comment discussion_r1013886734

4bbef47

agrski approved these changes Nov 7, 2022

View reviewed changes

seldondev added the approved label Nov 7, 2022

agrski merged commit 7da77d7 into SeldonIO:master Nov 7, 2022

RafalSkolasinski mentioned this pull request Nov 9, 2022

Enable SASL_SSL for Kafka #4415

Closed

RafalSkolasinski mentioned this pull request May 4, 2023

Enable SASL_SSL kafka protocol with PLAIN mechanism #4828

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enables SSL_SASL #4416

Enables SSL_SASL #4416

abohmeed commented Oct 31, 2022

seldondev commented Oct 31, 2022

abohmeed commented Oct 31, 2022

abohmeed commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

abohmeed commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

abohmeed commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

abohmeed commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

abohmeed commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

agrski left a comment

agrski Nov 1, 2022

agrski Nov 1, 2022

abohmeed commented Nov 2, 2022

seldondev commented Nov 2, 2022

abohmeed commented Nov 2, 2022

abohmeed commented Nov 4, 2022

agrski commented Nov 4, 2022

agrski Nov 4, 2022

agrski Nov 4, 2022

abohmeed Nov 4, 2022

abohmeed Nov 4, 2022

abohmeed Nov 4, 2022

agrski Nov 4, 2022

abohmeed Nov 4, 2022

abohmeed commented Nov 4, 2022

abohmeed commented Nov 7, 2022

agrski left a comment

seldondev commented Nov 7, 2022

agrski commented Nov 7, 2022

		sslKakfaServer := util.GetSslElements()
		saslKafkaServer := util.GetSaslElements()

Enables SSL_SASL #4416

Enables SSL_SASL #4416

Conversation

abohmeed commented Oct 31, 2022

seldondev commented Oct 31, 2022

abohmeed commented Oct 31, 2022

abohmeed commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

abohmeed commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

abohmeed commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

abohmeed commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

abohmeed commented Oct 31, 2022

RafalSkolasinski commented Oct 31, 2022

agrski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abohmeed commented Nov 2, 2022

seldondev commented Nov 2, 2022

abohmeed commented Nov 2, 2022

abohmeed commented Nov 4, 2022

agrski commented Nov 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abohmeed commented Nov 4, 2022

abohmeed commented Nov 7, 2022

agrski left a comment

Choose a reason for hiding this comment

seldondev commented Nov 7, 2022

agrski commented Nov 7, 2022