Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add nsq scaler #6230

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

Ulminator
Copy link

@Ulminator Ulminator commented Oct 11, 2024

This change adds a new scaler for NSQ, enabling auto-scaling based on topic/channel depth. The main components of NSQ that the scaler interacts with are nsqlookupd and nsqd. First the scaler makes /lookup requests to the nsqlookupd instances in the cluster to determine which nsqd instances have messages for the given topic. Then for each nsqd instance the scaler makes /stats requests to and aggregates the results to determine a final depth value.

NSQ consumers create the channel they are reading if they do not exist. As such, if the channel does not exist on an nsqd host, the topic depth is used. The consumer pods that KEDA would be responsible in spawning would then create the channel.

Checklist

Fixes #3281

Relates to:

@Ulminator Ulminator requested a review from a team as a code owner October 11, 2024 00:06
Copy link

semgrep-app bot commented Oct 11, 2024

Semgrep found 7 no-direct-write-to-responsewriter findings:

Detected directly writing or similar in 'http.ResponseWriter.write()'. This bypasses HTML escaping that prevents cross-site scripting vulnerabilities. Instead, use the 'html/template' package and render data using 'template.Execute()'.

Ignore this finding from no-direct-write-to-responsewriter.

Semgrep found 20 sprintf-host-port findings:

use net.JoinHostPort instead of fmt.Sprintf(parsedURL.Port(), parsedURL.Hostname())

Ignore this finding from sprintf-host-port.

Copy link

semgrep-app bot commented Oct 11, 2024

Semgrep found 7 no-fprintf-to-responsewriter findings:

Detected 'Fprintf' or similar writing to 'http.ResponseWriter'. This bypasses HTML escaping that prevents cross-site scripting vulnerabilities. Instead, use the 'html/template' package to render data to users.

Ignore this finding from no-fprintf-to-responsewriter.

@Ulminator Ulminator force-pushed the nsq_autoscaler branch 3 times, most recently from 7d7ee13 to 32b9535 Compare October 14, 2024 19:09
Signed-off-by: Matt Ulmer <[email protected]>
@Ulminator
Copy link
Author

@JorTurFer thanks for merging in kedacore/test-tools#175! I updated the code here to utilize the image that can be created from that PR.

I also wanted to let you know that I have created the corresponding docs PR as well here: kedacore/keda-docs#1485

The last remaining TODO's seem to just be getting these CI checks to pass. Is it safe to ignore the remaining semgrep checks since they just relate to tests? I initially got flagged for no-direct-write-to-responsewriter and now I'm getting no-fprintf-to-responsewriter, yet I do see both of these patterns being used in other test files.

CHANGELOG.md Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

zroubalik commented Oct 15, 2024

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

@Ulminator
Copy link
Author

Ulminator commented Oct 15, 2024

I see the e2e tests have failed, but I'm wondering if the e2e test image (ghcr.io/kedacore/tests-nsq:latest) was created? 🤔 I don't see it here. I'm thinking the issue was an image pull error due to it not existing yet.

# Local run w output snippet from describing a consumer pod
  Warning  Failed     9s    kubelet            Failed to pull image "ghcr.io/kedacore/tests-nsq:latest": failed to pull and unpack image "ghcr.io/kedacore/tests-nsq:latest": failed to resolve reference "ghcr.io/kedacore/tests-nsq:latest": failed to authorize: failed to fetch anonymous token: unexpected status from GET request to https://ghcr.io/token?scope=repository%3Akedacore%2Ftests-nsq%3Apull&service=ghcr.io: 401 Unauthorized
  Warning  Failed     9s    kubelet            Error: ErrImagePull
  Normal   BackOff    8s    kubelet            Back-off pulling image "ghcr.io/kedacore/tests-nsq:latest"
  Warning  Failed     8s    kubelet            Error: ImagePullBackOff

I also noticed I had only updated one of the refs of my custom image to ghcr.io/kedacore/tests-nsq:latest so I went ahead and made that fix.

@JorTurFer
Copy link
Member

The image was created but GH creates them as private by default, now it's public

@JorTurFer
Copy link
Member

JorTurFer commented Oct 16, 2024

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

@JorTurFer
Copy link
Member

JorTurFer commented Oct 16, 2024

Do we need to update the policy to include any extra permission? https://github.com/kedacore/testing-infrastructure/blob/main/terraform/modules/aws/iam/main.tf#L88-L96

If yes, could you open a PR there?

@Ulminator
Copy link
Author

Do we need to update the policy to include any extra permission? https://github.com/kedacore/testing-infrastructure/blob/main/terraform/modules/aws/iam/main.tf#L88-L96

If yes, could you open a PR there?

No additional permissions are required 👍

Signed-off-by: Matt Ulmer <[email protected]>
@Ulminator
Copy link
Author

@JorTurFer just wanted to check in and see if you need anything else before merging this in?

@JorTurFer JorTurFer mentioned this pull request Nov 3, 2024
28 tasks
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Looking good. I've kept some comments inline.
I'm worried about the lifecycle of the project, I closed the issue with the feature request a year and a half ago because there wasn't activity, and it looked as no longer maintained.
@zroubalik @wozniakjan

pkg/scalers/nsq_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/nsq_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/nsq_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/nsq_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/nsq_scaler.go Outdated Show resolved Hide resolved
pkg/scalers/nsq_scaler.go Outdated Show resolved Hide resolved
tests/scalers/nsq/nsq_test.go Outdated Show resolved Hide resolved
tests/scalers/nsq/nsq_test.go Outdated Show resolved Hide resolved
Signed-off-by: Matt Ulmer <[email protected]>
@Ulminator
Copy link
Author

@JorTurFer thanks for taking a look! I believe I addressed all of your comments, but let me know if anything else needs to change of course.

I'm worried about the lifecycle of the project, I closed the issue with the feature request a year and a half ago because there wasn't activity, and it looked as no longer maintained.

I'm an engineer at Bitly, which is where NSQ was initially developed. We use it extensively throughout our stack, and while updates to the project are not as frequent as they were in the past, it is stable. There are plans for updates in the near future and it is under active development.

tests/scalers/nsq/nsq_test.go Outdated Show resolved Hide resolved
tests/scalers/nsq/nsq_test.go Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

JorTurFer commented Nov 5, 2024

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

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome job! 🙇

tests/scalers/nsq/nsq_test.go Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

@zroubalik @wozniakjan PTAL

Co-authored-by: Jorge Turrado Ferrero <[email protected]>
Signed-off-by: Matt Ulmer <[email protected]>
@Ulminator
Copy link
Author

@JorTurFer thanks for approving the PR and for your prompt feedback!

@JorTurFer
Copy link
Member

JorTurFer commented Nov 5, 2024

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

Copy link
Member

@wozniakjan wozniakjan left a comment

Choose a reason for hiding this comment

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

lgtm, thank you!

not sure what is wrong with Semgrep / Analyze Semgrep action, it doesn't allow me to see anything in there

@wozniakjan wozniakjan enabled auto-merge (squash) November 11, 2024 08:49
auto-merge was automatically disabled November 15, 2024 00:31

Head branch was pushed to by a user without write access

@Ulminator
Copy link
Author

Ulminator commented Nov 15, 2024

Thanks for the review @wozniakjan !

@JorTurFer, I have updated this PR along with kedacore/docs#1485 to account for the 2.16 release that happened last week, PTAL!

Signed-off-by: Matt Ulmer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add NSQ Scaler
4 participants