feat: Kafka SASL OAUTH token refreshing #2834

vlastahajek · 2024-09-18T11:39:19Z

Required checklist

Sample config files updated (both /etc folder and NewDemoConfig methods) (influxdb and plutonium)
openapi swagger.yml updated (if modified API) - link openapi PR
Signed CLA (if not already signed)

Description

Adding SASL OAUTH token refreshing for Kafka plugin. The same support as Telegraf has - custom, Auth0, AzureAD.

Kapacitor docs update PR: #5617

New Kafka config params:

sasl-oauth-service
sasl-oauth-client-id
sasl-oauth-client-secret
sasl-oauth-token-url
sasl-oauth-token-expiry-margin
[kafka.sasl-oauth-parameters]
sasl-oauth-tenant-id
sasl-oauth-scopes

srebhan

@vlastahajek nice work! I know you plan to clean this up a bit more but here are some early comments nonetheless. ;-)

.circleci/config.yml

srebhan · 2024-09-18T15:53:43Z

etc/kapacitor/kapacitor.conf

+  # sasl-oauth-service = ""
+  ## The client ID to use when authenticating with SASL/OAUTH.
+  # sasl-oauth-client-id = ""
+  ## The client secret to use when authenticating with SASL/OAUTH.
+  # sasl-oauth-client-secret = ""
+  ## The token URL to use when authenticating with SASL/OAUTH.
+  # sasl-oauth-token-url = ""


The sasl-oauth-service and sasl-oauth-token-url are somewhat redundant aren't they? Do we really want both or should we just use the URL and provide the URLs for common services in the config?

Generally, the core implementation and feature set are done according to Telegraf to provide a similar user experience
Choosing service makes sense to distinguish auth0, azure and other and provides space for future extensions

I know as I implemented this part in Telegraf. The big issue is that the two are redundant and you need to decide and document the behavior if both options are given and, in the worst case, contradict each other.

The only contradiction I see, for now, is we don't need the token-url for azure.

I still don't understand how it would work if there were only one. The token-url?

we need to check if the audience parameter is added for auth0

If a user would like to use azuread, the token-url would be azuread?

In case of azure we need to check tenant-id

I'm not saying you have to remove it but then please make the use and redundancy clear in the option description! As it is now, people might be inclined to specify both the token-url and the service.

Sure, the token-url param should be more contextually described

srebhan · 2024-09-18T15:55:08Z

services/kafka/sasl.go

+	"golang.org/x/oauth2"
+	"golang.org/x/oauth2/clientcredentials"
+
+	kafka "github.com/IBM/sarama"


Please remove the alias and just use

Suggested change

kafka "github.com/IBM/sarama"

"github.com/IBM/sarama"

as everything else is confusing...

This is very subjective. As the sarama is a Kafka client the alias makes it more clean. Also, the alias is already used in kafka/service.go

Well I don't like this but it might be OK if kapacitor guys agree. Let me give you my reasons:

You obfuscate the library used with an alias, making it harder for readers of the code to figure out what's going on.

Your chosen alias is the same as the package name, this will cause additional confusion.

You increase the PR size without any necessity.

But as I said, if that's how it is done in kapacitor I don't mind.

srebhan · 2024-09-18T15:56:27Z

services/kafka/sasl.go

+	if k.SASLMechanism == "" {
+		return nil
+	}
+	switch k.SASLMechanism {


Suggested change

if k.SASLMechanism == "" {

return nil

}

switch k.SASLMechanism {

switch k.SASLMechanism {

case "":

return nil

services/kafka/sasl.go

srebhan · 2024-09-18T15:59:22Z

services/kafka/sasl.go

+			if k.SASLOAUTHExpiryMargin == 0 {
+				k.SASLOAUTHExpiryMargin = 10 * time.Second
+			}


This was initialized before in the config, wasn't it? The current way the user cannot set the option to a valid zero...

srebhan · 2024-09-18T16:01:20Z

services/kafka/sasl.go

+func azureAD(tenant string) oauth2.Endpoint {
+	if tenant == "" {
+		tenant = "common"
+	}
+	return oauth2.Endpoint{
+		AuthURL:       "https://login.microsoftonline.com/" + tenant + "/oauth2/v2.0/authorize",
+		TokenURL:      "https://login.microsoftonline.com/" + tenant + "/oauth2/v2.0/token",
+		DeviceAuthURL: "https://login.microsoftonline.com/" + tenant + "/oauth2/v2.0/devicecode",
+	}
+}


Instead of this hand-crafted implementation, why not rely on https://pkg.go.dev/golang.org/x/oauth2@v0.23.0/endpoints?

Yes, this was mistakenly copied.

srebhan · 2024-09-18T16:03:56Z

services/kafka/token.go

+func NewRefreshingToken(source oauth2.TokenSource, cancel context.CancelFunc, extensions map[string]string) *RefreshingToken {
+	return &RefreshingToken{
+		source:     source,
+		cancel:     cancel,
+		extensions: extensions,
+	}
+}


The cancel function is never used in here and actually shouldn't be. The caller should cancel the context on exit...

Yes, handling closing is still TODO

…(custom, auth0, azure), added config validation

bednar

@vlastahajek, thanks for the nice improvement 👍. I've just added some minor requirements before acceptance.

bednar · 2024-09-25T10:46:44Z

etc/kapacitor/kapacitor.conf

+  # sasl-oauth-client-secret = ""
+  ## The token URL to use when sasl-oauth-service is custom or auth0. Leave empty otherwise.
+  # sasl-oauth-token-url = ""
+  ## The expiry margin for the token.


A small suggestion to clarify the meaning of this property:

Suggested change

## The expiry margin for the token.

## The margin for the token's expiration time.

Thanks, fixed

bednar · 2024-09-25T10:49:16Z

services/kafka/config.go

@@ -49,7 +49,7 @@ type Config struct {
 }

 func NewConfig() Config {
-	return Config{ID: DefaultID}
+	return Config{ID: DefaultID, SASLAuth: SASLAuth{SASLOAUTHExpiryMargin: 10 * time.Second}}


Please define the SASLOAUTHExpiryMargin value as a constant, similar to DefaultID.

Suggested change

return Config{ID: DefaultID, SASLAuth: SASLAuth{SASLOAUTHExpiryMargin: 10 * time.Second}}

return Config{ID: DefaultID, SASLAuth: SASLAuth{SASLOAUTHExpiryMargin: DefaultSASLOAUTHExpiryMargin}}

Thanks, fixed

bednar

@vlastahajek thanks for PR 👍

LGTM 🚀

vlastahajek marked this pull request as draft September 18, 2024 11:39

srebhan reviewed Sep 18, 2024

View reviewed changes

vlastahajek changed the title ~~Feat/kafka oauth~~ feat: Kafka SASL OAUTH token refreshing Sep 24, 2024

vlastahajek added 10 commits September 24, 2024 16:37

wip: Kafka SASL OAUTH2 MVP for Auth0

239356f

chore: code improvement

33bc879

wip: completing OAUTh token refresh for the same sources as Telegraf …

2a07733

…(custom, auth0, azure), added config validation

wip: added sample config for SASL OAUTH

8cfda9f

wip: formatting

fa08d68

wip: keeping cancel func

ef65302

wip: PR fixes

2b4f60d

wip: goimports

977fd79

docs: improving sasl-oauth-token-url description

7250fcc

feat: adding proper canceling of token source

65c000e

vlastahajek force-pushed the feat/kafka-oauth branch from 2977c54 to 65c000e Compare September 24, 2024 14:39

vlastahajek marked this pull request as ready for review September 24, 2024 14:39

vlastahajek added 2 commits September 24, 2024 17:26

fix: nil

d00a213

refactor: using single closer

f3ae1d1

vlastahajek mentioned this pull request Sep 25, 2024

Release Kapacitor v1.7.6, New Kafka Handler params influxdata/docs-v2#5617

Merged

bednar requested changes Sep 25, 2024

View reviewed changes

chore: review fixes

fc88c2f

vlastahajek requested a review from bednar September 25, 2024 14:32

bednar approved these changes Sep 26, 2024

View reviewed changes

bednar merged commit 71a5e1e into master Sep 26, 2024
3 checks passed

bednar deleted the feat/kafka-oauth branch September 26, 2024 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Kafka SASL OAUTH token refreshing #2834

feat: Kafka SASL OAUTH token refreshing #2834

vlastahajek commented Sep 18, 2024 •

edited by bednar

Loading

srebhan left a comment

srebhan Sep 18, 2024

vlastahajek Sep 19, 2024

srebhan Sep 19, 2024

vlastahajek Sep 19, 2024

srebhan Sep 19, 2024

vlastahajek Sep 19, 2024

vlastahajek Sep 24, 2024

srebhan Sep 18, 2024

vlastahajek Sep 19, 2024

srebhan Sep 19, 2024

srebhan Sep 18, 2024

vlastahajek Sep 19, 2024

srebhan Sep 18, 2024

vlastahajek Sep 19, 2024

srebhan Sep 18, 2024

vlastahajek Sep 19, 2024

srebhan Sep 18, 2024

vlastahajek Sep 19, 2024

vlastahajek Sep 24, 2024

bednar left a comment

bednar Sep 25, 2024

vlastahajek Sep 25, 2024

bednar Sep 25, 2024

vlastahajek Sep 25, 2024

bednar left a comment

	## The expiry margin for the token.
	## The margin for the token's expiration time.

	return Config{ID: DefaultID, SASLAuth: SASLAuth{SASLOAUTHExpiryMargin: 10 * time.Second}}
	return Config{ID: DefaultID, SASLAuth: SASLAuth{SASLOAUTHExpiryMargin: DefaultSASLOAUTHExpiryMargin}}

feat: Kafka SASL OAUTH token refreshing #2834

feat: Kafka SASL OAUTH token refreshing #2834

Conversation

vlastahajek commented Sep 18, 2024 • edited by bednar Loading

Required checklist

Description

srebhan left a 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

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

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

bednar left a 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

bednar left a comment

Choose a reason for hiding this comment

vlastahajek commented Sep 18, 2024 •

edited by bednar

Loading