refactor pulsar scaler auth #6969

wozniakjan · 2025-08-08T08:51:35Z

followup to #6848, no changelog update since the refactor has not been released yet and this essentially continues in that same entry.

Checklist

I have verified that my change is according to the deprecations & breaking changes policy
Tests have been added
Commits are signed with Developer Certificate of Origin (DCO - learn more)

Relates to #5797,

github-actions · 2025-08-08T08:51:45Z

Thank you for your contribution! 🙏

Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected.

While you are waiting, make sure to:

Add an entry in our changelog in alphabetical order and link related issue
Update the documentation, if needed
Add unit & e2e tests for your changes
GitHub checks are passing
Is the DCO check failing? Here is how you can fix DCO issues

Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient.

Learn more about our contribution guide.

wozniakjan · 2025-08-08T08:55:24Z

pkg/scalers/pulsar_scaler.go

+	if m.PulsarAuth.ClientSecret == "" {
+		// client_secret is not required for mtls OAuth(RFC8705)
+		// set secret to random string to work around the Go OAuth lib
+		m.PulsarAuth.ClientSecret = time.Now().String()
 	}
-
-	if auth != nil && auth.EnableOAuth {
-		if err := m.configureOAuth(auth); err != nil {
-			return err
+	var scopes []string
+	for _, scope := range m.PulsarAuth.Scopes {
+		if strings.TrimSpace(scope) != "" {
+			scopes = append(scopes, scope)
 		}
 	}
-
-	m.pulsarAuth = auth
-	return nil
-}
-
-// configureOAuth configures OAuth settings
-func (m *pulsarMetadata) configureOAuth(auth *authentication.AuthMeta) error {
-	if auth.OauthTokenURI == "" {
-		auth.OauthTokenURI = m.OauthTokenURI
-	}
-	if auth.Scopes == nil {
-		auth.Scopes = authentication.ParseScope(m.Scope)
-	}
-	if auth.ClientID == "" {
-		auth.ClientID = m.ClientID
+	m.PulsarAuth.Scopes = scopes
+	if len(m.PulsarAuth.Scopes) == 0 {
+		m.PulsarAuth.Scopes = nil


this post processing has reduced a bit thanks to using authentication.Config instead of authentication.AuthMeta but there are still some tweaks necessary because of how pulsar used to treat certain corner cases

wozniakjan · 2025-08-08T08:57:16Z

pkg/scalers/authentication/authentication_types.go

+	OauthTokenURI  string     `keda:"name=oauthTokenURI,  order=authParams;triggerMetadata"`
+	Scopes         []string   `keda:"name=scopes;scope,   order=authParams;triggerMetadata"`
+	ClientID       string     `keda:"name=clientID,       order=authParams;triggerMetadata"`
 	ClientSecret   string     `keda:"name=clientSecret,   order=authParams"`
-	EndpointParams url.Values `keda:"name=endpointParams, order=authParams"`
+	EndpointParams url.Values `keda:"name=endpointParams, order=authParams;triggerMetadata"`


pulsar is the first refactored scaler that allows setting these in trigger metadata, I guess apart from ClientSecret, it might be technically ok from a security perspective?

All those values are not sensitive, so it's fine in terms of security if we get them from metadata

wozniakjan · 2025-08-08T08:58:01Z

pkg/scalers/authentication/authentication_types.go

+	if c.EnabledOAuth() && (c.OauthTokenURI == "" || c.ClientID == "") {
+		return fmt.Errorf("oauthTokenURI=%v and clientID=%v are required when oauth is enabled", empty(c.OauthTokenURI), empty(c.ClientID))


thanks to pulsar, I learned that ClientSecret is actually not a compulsory parameter when setting oauth

There are confidential and public clients defined -> https://oauth.net/2/client-types/
Said this, public clients are to identify users and not to retrieve tokens on behalf of themselves (for example, the typical cli that needs to login a user). I'm wondering which is the use case for a public clients in this case

Checking the scaler code, it looks as it's because clients can be identified on pulsar also by mTLS (it's another client auth allowed by oauth). In this case, it makes sense to verify something like, clientSecrent OR mTLS

if c.EnabledOAuth() && (c.OauthTokenURI == "" || c.ClientID == "" || (!c.EnabledTLS() && c.ClientSecret == "")) {

got it, thanks for the explanation!

there are tests that configure only authModes: oauth with oauthTokenURI, clientID, no clientSecret and then only ca. That means technically TLS is not fully configured and enabled, only the ca is set. Should that be also valid?

details

keda/pkg/scalers/pulsar_scaler_test.go

Line 98 in 56a023d

{map[string]string{"adminURL": "http://172.20.0.151:80", "topic": "persistent://public/default/my-topic", "subscription": "sub1", "authModes": "oauth", "oauthTokenURI": "https2", "scope": "scope2", "clientID": "id2"}, map[string]string{"ca": "cadata", "oauthTokenURI": "", "scope": "", "clientID": "", "clientSecret": ""}, false, false, "", "", "cadata", "", "", "", false, "https2", "scope2", "id2", "", "", nil},

triggerMetadata: adminURL: "http://172.20.0.151:80" authModes: oauth clientID: id2 oauthTokenURI: https2 scope: scope2 subscription: sub1 topic: persistent://public/default/my-topic authParams: ca: "cadata" clientID: "" clientSecret: "" oauthTokenURI: "" scope: ""

keda/pkg/scalers/pulsar_scaler_test.go

Line 102 in 56a023d

{map[string]string{"adminURL": "http://172.20.0.151:80", "topic": "persistent://public/default/my-topic", "subscription": "sub1", "authModes": "oauth", "oauthTokenURI": "https4", "scope": " sc:scope2, \tsc:scope1 ", "clientID": "id4"}, map[string]string{"ca": "cadata", "oauthTokenURI": "", "scope": "", "clientID": "", "clientSecret": ""}, false, false, "", "", "cadata", "", "", "", false, "https4", "sc:scope1 sc:scope2", "id4", "", "", nil},

triggerMetadata: adminURL: "http://172.20.0.151:80" topic: "persistent://public/default/my-topic" subscription: "sub1" authModes: "oauth" oauthTokenURI: "https4" scope: " sc:scope2,tsc:scope1" clientID: "id4" authParams: ca: "cadata" oauthTokenURI: "" scope: "" clientID: "" clientSecret": ""

let me check the SDK under the hood, but my first thought is that this isn't safe at all xD

pfff..... the current implementation relies on golang.org/x/oauth2/clientcredentials and it uses client_credentials flow to retrieve a token on behalf of the client itself just with clientID and clientSecret (no other auth methods are used), so IMHO the secret must be mandatory in terms of security here.
ClientID without secret in this context means that whoever knows the ClientID (which is not a secret by definition) can call to pulsar as an authentication client.

Making the clientSecret mandatory is a breaking change on our side (nevertheless, I'd expect this requirement by pulsar server) so maybe we can raise a warning when it's not set, WDTY?

sure, so I will allow this in code to not introduce any breaking changes but log a warning. Thanks for checking!

wozniakjan · 2025-08-08T08:58:36Z

/run-e2e
Update: You can check the progress here

Copilot

Pull Request Overview

This PR refactors the Pulsar scaler authentication system to use a new structured authentication configuration approach. It replaces the previous pulsarAuth field with PulsarAuth and changes from direct field access to method-based access for authentication states.

Refactored authentication field from pulsarAuth to PulsarAuth with new type *authentication.Config
Updated authentication state checks to use method calls instead of direct field access
Enhanced error messages in authentication validation with more descriptive formatting

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
pkg/scalers/pulsar_scaler.go	Main refactor replacing authentication struct and implementation methods
pkg/scalers/pulsar_scaler_test.go	Updated test assertions to use new authentication API methods
pkg/scalers/authentication/authentication_types.go	Enhanced OAuth field tags and improved validation error messages

pkg/scalers/pulsar_scaler.go

pkg/scalers/authentication/authentication_types.go

rickbrouwer · 2025-08-08T09:22:09Z

First very minor feedback is that the commit/pr title contains a spelling error ;)

wozniakjan · 2025-08-08T10:29:11Z

First very minor feedback is that the commit/pr title contains a spelling error ;)

thanks! so I guess copilot doesn't check the PR titles...

Signed-off-by: Jan Wozniak <[email protected]>

wozniakjan · 2025-08-08T11:38:37Z

/run-e2e pulsar
Update: You can check the progress here

SpiritZhou · 2025-08-12T09:28:33Z

pkg/scalers/pulsar_scaler.go

 func (m *pulsarMetadata) handleBackwardsCompatibility(config *scalersconfig.ScalerConfig) {
 	// For backwards compatibility, we need to map "tls: enable" to auth modes
-	if m.TLS == enable && (config.AuthParams["cert"] != "" || config.AuthParams["key"] != "") {
+	if config.TriggerMetadata["tls"] == enable && (config.AuthParams["cert"] != "" || config.AuthParams["key"] != "") {


Could this part be moved to validate()?

That is a fair point, but I'm not sure putting it to Validate() would be the right thing.

This section is a bit odd, the scaler has supported setting TLS in a way that deviates from standard settings implemented with other scalers. I don't want to break that because I am sure some users expect this behavior, but putting this to Validate() would likely mean we don't deprecate this ever. Right now, I don't know enough about this scaler to bravely mark this for deprecation but I'm going to add a note for later inspection and hopefully we unite TLS and auth params configuration for Pulsar scaler to match other scalers.

rickbrouwer · 2025-09-12T13:55:27Z

Hi @wozniakjan

This PR is still in draft. Is this the correct status, or is it actually already ready for review?

wozniakjan · 2025-09-24T16:20:59Z

This PR is still in draft. Is this the correct status, or is it actually already ready for review?

still a draft, need to incorporate feedback from #6969 (comment)

wozniakjan force-pushed the pulsar_auth_refactor branch from ca74f79 to 439a0a7 Compare August 8, 2025 08:53

wozniakjan commented Aug 8, 2025

View reviewed changes

wozniakjan requested a review from Copilot August 8, 2025 08:59

wozniakjan marked this pull request as ready for review August 8, 2025 08:59

Copilot AI reviewed Aug 8, 2025

View reviewed changes

pkg/scalers/pulsar_scaler.go Show resolved Hide resolved

pkg/scalers/pulsar_scaler.go Show resolved Hide resolved

pkg/scalers/authentication/authentication_types.go Show resolved Hide resolved

wozniakjan requested review from a team and rickbrouwer August 8, 2025 09:04

wozniakjan mentioned this pull request Aug 8, 2025

Refactor and fix metrics-api scaler #6940

Merged

5 tasks

wozniakjan changed the title ~~refactor puslar scaler auth~~ refactor pulsar scaler auth Aug 8, 2025

wozniakjan marked this pull request as draft August 8, 2025 10:55

refactor pulsar scaler auth

73e8bd5

Signed-off-by: Jan Wozniak <[email protected]>

wozniakjan force-pushed the pulsar_auth_refactor branch from 439a0a7 to 73e8bd5 Compare August 8, 2025 11:37

SpiritZhou reviewed Aug 12, 2025

View reviewed changes

		if c.EnabledOAuth() && (c.OauthTokenURI == "" \|\| c.ClientID == "") {
		return fmt.Errorf("oauthTokenURI=%v and clientID=%v are required when oauth is enabled", empty(c.OauthTokenURI), empty(c.ClientID))

refactor pulsar scaler auth #6969

Are you sure you want to change the base?

refactor pulsar scaler auth #6969

Uh oh!

Conversation

wozniakjan commented Aug 8, 2025

Checklist

Uh oh!

github-actions bot commented Aug 8, 2025

Uh oh!

wozniakjan Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JorTurFer Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wozniakjan commented Aug 8, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rickbrouwer commented Aug 8, 2025

Uh oh!

wozniakjan commented Aug 8, 2025

Uh oh!

wozniakjan commented Aug 8, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickbrouwer commented Sep 12, 2025

Uh oh!

wozniakjan commented Sep 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wozniakjan Aug 8, 2025 •

edited

Loading

JorTurFer Aug 8, 2025 •

edited

Loading

wozniakjan commented Aug 8, 2025 •

edited by github-actions bot

Loading

wozniakjan commented Aug 8, 2025 •

edited by github-actions bot

Loading