JetStream: add accountID param for when an account's name and id differ by ohhfishal · Pull Request #6611 · kedacore/keda

ohhfishal · 2025-03-07T23:34:52Z

Running into the same issue mentioned in pull 6415. Handles the case where an account name and id differ in a way that maintains backwards-compatibility since it is a trigger.

Checklist

When introducing a new scaler, I agree with the scaling governance policy
I have verified that my change is according to the deprecations & breaking changes policy
Tests have been added
Changelog has been updated and is aligned with our changelog requirements
A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
A PR is opened to update the documentation on (repo) (if applicable)
Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #

Relates to #
PR#1551
PR#6415

rickbrouwer · 2025-03-08T12:14:24Z

What happens if you specify both, but both with a different value?

The JetStream scaler also needs some refactoring (#5797). Is that something you can and want to do? No problem if you can't or don't want to. One advantage of the refactoring is the support for multinames, which I think you want to achieve here. Here is an example of how multinames is applied.

ohhfishal · 2025-03-08T14:14:12Z

As implemented, it defers to only use the ID in the filtering, since internally NATs uses the ID (From the slack message screenshot in #6415.

We can also see this in NATS source where the name is sometimes the ID since Name can be overridden.

I am not entirely sold that the solution of #6415 is the right approach, since it only modifies how we filter the response from the /jsz endpoint, when the issue is fundamentally that the acc filters accounts
by ID not Name. Which my code currently does not change.

Which a component of this is as a result of NATs docs using their terms incorrect. In the source for /jsz they filter by Name, but they expose it to the user as ID (and sometimes Name).

I would be interested in refactoring, but would want to get this fixed first unless refactoring would help in that (since it sounds like the part being refactored is only how the config is initialized).

Still not entirely sure on what the fix for this should be.

rickbrouwer · 2025-03-09T13:38:45Z

What caught my eye was this piece of code:

meta.accountID, err = GetFromAuthOrMeta(config, "accountID")
	if err != nil {
		meta.accountID = meta.account
	}

Which means that if accountID is not specified the value of account will be used. I wonder if the user who might configure both would know which one is being used, regardless of whether the outcome is correct (I don't know how JetStream works).

The point about refactoring would mean that someone could specify account or accountId, but both values would then be parsed to e.g. account, basically as you have created now.

ohhfishal · 2025-03-09T17:43:05Z

That solution from the refactor sounds like it would be a cleaner fix than what I have now. I'll see what I can do. To clarify the refactor is to have the config parsing done using the declarative struct tags akin to d5d0b10? Then in doing that using a multiname for account/accountID similar to the example you provided?

rickbrouwer · 2025-03-10T11:39:02Z

Yeah, it is cleaner. But it might cost you a bit more work to implement your use case (because the refactoring is added).
That's right, d5d0b10 is an example. More scalers have already been refactored. These are in #5797, although the status is not fully maintained. But at the bottom you can find all mentions of PRs.
For the multiname you can use my example, or #5997. It was introduced in this PR.

Signed-off-by: ohhfishal <[email protected]>

JorTurFer · 2025-03-22T23:04:35Z

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

Signed-off-by: Jorge Turrado Ferrero <[email protected]>

JorTurFer · 2025-03-23T00:27:50Z

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

…er (kedacore#6611) * JetStream: add accountID param Signed-off-by: ohhfishal <[email protected]> * docs: initial changelog Signed-off-by: ohhfishal <[email protected]> * fix: initial refactor and add ID Signed-off-by: ohhfishal <[email protected]> * docs: add PR link Signed-off-by: ohhfishal <[email protected]> * chore: go fmt Signed-off-by: ohhfishal <[email protected]> * chore: tidy of pre-commit failure Signed-off-by: ohhfishal <[email protected]> * fix: update tests Signed-off-by: ohhfishal <[email protected]> * fix: make account and accountID individually optional, but one required Signed-off-by: ohhfishal <[email protected]> --------- Signed-off-by: ohhfishal <[email protected]> Signed-off-by: Jorge Turrado Ferrero <[email protected]> Co-authored-by: Jorge Turrado Ferrero <[email protected]>

…er (kedacore#6611) * JetStream: add accountID param Signed-off-by: ohhfishal <[email protected]> * docs: initial changelog Signed-off-by: ohhfishal <[email protected]> * fix: initial refactor and add ID Signed-off-by: ohhfishal <[email protected]> * docs: add PR link Signed-off-by: ohhfishal <[email protected]> * chore: go fmt Signed-off-by: ohhfishal <[email protected]> * chore: tidy of pre-commit failure Signed-off-by: ohhfishal <[email protected]> * fix: update tests Signed-off-by: ohhfishal <[email protected]> * fix: make account and accountID individually optional, but one required Signed-off-by: ohhfishal <[email protected]> --------- Signed-off-by: ohhfishal <[email protected]> Signed-off-by: Jorge Turrado Ferrero <[email protected]> Co-authored-by: Jorge Turrado Ferrero <[email protected]> Signed-off-by: mittalvaibhav1 <[email protected]>

…er (kedacore#6611) * JetStream: add accountID param Signed-off-by: ohhfishal <[email protected]> * docs: initial changelog Signed-off-by: ohhfishal <[email protected]> * fix: initial refactor and add ID Signed-off-by: ohhfishal <[email protected]> * docs: add PR link Signed-off-by: ohhfishal <[email protected]> * chore: go fmt Signed-off-by: ohhfishal <[email protected]> * chore: tidy of pre-commit failure Signed-off-by: ohhfishal <[email protected]> * fix: update tests Signed-off-by: ohhfishal <[email protected]> * fix: make account and accountID individually optional, but one required Signed-off-by: ohhfishal <[email protected]> --------- Signed-off-by: ohhfishal <[email protected]> Signed-off-by: Jorge Turrado Ferrero <[email protected]> Co-authored-by: Jorge Turrado Ferrero <[email protected]>

…er (kedacore#6611) * JetStream: add accountID param Signed-off-by: ohhfishal <[email protected]> * docs: initial changelog Signed-off-by: ohhfishal <[email protected]> * fix: initial refactor and add ID Signed-off-by: ohhfishal <[email protected]> * docs: add PR link Signed-off-by: ohhfishal <[email protected]> * chore: go fmt Signed-off-by: ohhfishal <[email protected]> * chore: tidy of pre-commit failure Signed-off-by: ohhfishal <[email protected]> * fix: update tests Signed-off-by: ohhfishal <[email protected]> * fix: make account and accountID individually optional, but one required Signed-off-by: ohhfishal <[email protected]> --------- Signed-off-by: ohhfishal <[email protected]> Signed-off-by: Jorge Turrado Ferrero <[email protected]> Co-authored-by: Jorge Turrado Ferrero <[email protected]> Signed-off-by: David Pochopsky <[email protected]>

JetStream: add accountID param

db3d639

Signed-off-by: ohhfishal <[email protected]>

ohhfishal force-pushed the main branch from 95b6075 to db3d639 Compare March 10, 2025 18:47

ohhfishal added 7 commits March 10, 2025 16:12

docs: initial changelog

2cb32e8

Signed-off-by: ohhfishal <[email protected]>

fix: initial refactor and add ID

da6a2fa

Signed-off-by: ohhfishal <[email protected]>

docs: add PR link

2f1a311

Signed-off-by: ohhfishal <[email protected]>

chore: go fmt

961430a

Signed-off-by: ohhfishal <[email protected]>

chore: tidy of pre-commit failure

f15bb54

Signed-off-by: ohhfishal <[email protected]>

Merge branch 'main' into main

c8e1b76

fix: update tests

70addf7

Signed-off-by: ohhfishal <[email protected]>

ohhfishal marked this pull request as ready for review March 16, 2025 23:15

ohhfishal requested a review from a team as a code owner March 16, 2025 23:15

JorTurFer reviewed Mar 17, 2025

View reviewed changes

Comment thread pkg/scalers/nats_jetstream_scaler.go Outdated

fix: make account and accountID individually optional, but one required

9be2b91

Signed-off-by: ohhfishal <[email protected]>

JorTurFer approved these changes Mar 22, 2025

View reviewed changes

Merge branch 'main' into main

fab4af6

Signed-off-by: Jorge Turrado Ferrero <[email protected]>

JorTurFer merged commit b4c5bab into kedacore:main Mar 23, 2025

dttung2905 mentioned this pull request Oct 16, 2025

Refactor Scalers with new declarative scaler config #5797

Closed

58 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

JetStream: add accountID param for when an account's name and id differ#6611

JetStream: add accountID param for when an account's name and id differ#6611
JorTurFer merged 10 commits into
kedacore:mainfrom
ohhfishal:main

ohhfishal commented Mar 7, 2025 •

edited

Loading

Uh oh!

rickbrouwer commented Mar 8, 2025

Uh oh!

ohhfishal commented Mar 8, 2025

Uh oh!

rickbrouwer commented Mar 9, 2025

Uh oh!

ohhfishal commented Mar 9, 2025

Uh oh!

rickbrouwer commented Mar 10, 2025

Uh oh!

Uh oh!

JorTurFer commented Mar 22, 2025 •

edited by github-actions Bot

Loading

Uh oh!

JorTurFer commented Mar 23, 2025 •

edited by github-actions Bot

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ohhfishal commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

rickbrouwer commented Mar 8, 2025

Uh oh!

ohhfishal commented Mar 8, 2025

Uh oh!

rickbrouwer commented Mar 9, 2025

Uh oh!

ohhfishal commented Mar 9, 2025

Uh oh!

rickbrouwer commented Mar 10, 2025

Uh oh!

Uh oh!

JorTurFer commented Mar 22, 2025 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JorTurFer commented Mar 23, 2025 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ohhfishal commented Mar 7, 2025 •

edited

Loading

JorTurFer commented Mar 22, 2025 •

edited by github-actions Bot

Loading

JorTurFer commented Mar 23, 2025 •

edited by github-actions Bot

Loading