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

Support csv-format for WATCH_NAMESPACE env var #5631

Merged
merged 9 commits into from
Apr 10, 2024

Conversation

jkremser
Copy link
Contributor

@jkremser jkremser commented Mar 27, 2024

the current implementation gives a false impression that multiple namespaces are supported (method called GetWatchNamespaces + plular used here). However, it's not the case. No matter what's in the WATCH_NAMESPACE env var, it just passes its content to the cache options when calling NewManager in operator's main.go.

Let's change that :)

This couple of lines actually make it work so that user can specify WATCH_NAMESPACE=ns1,default,foo or just a singe ns and keda will watch for events in those whiteallowlisted namespaces. If left empty, it still watches for all.

I tested this locally by:

make run ARGS="--zap-log-level=debug --cert-dir=/Users/jkremser/workspace/keda" WATCH_NAMESPACE="ns1,ns2,default" KEDA_CLUSTER_OBJECT_NAMESPACE="keda"

# then creating ScaledObjects in those ^ namespaces worked, while those in other namespaces were ignored

Fix #5670

Signed-off-by: Jirka Kremser <jiri.kremser@gmail.com>
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Thanks, we should probably apply this also to webhook: https://github.com/kedacore/keda/blob/main/cmd/webhooks/main.go

@zroubalik
Copy link
Member

Thanks, we should probably apply this also to webhook: https://github.com/kedacore/keda/blob/main/cmd/webhooks/main.go

Not a good idea actually. Let's not do that at the moment.

@jkremser
Copy link
Contributor Author

👍 for the record:

having WATCH_NAMESPACE implemented also for the validating admission controller would lead to a situation where in certain namespace the malformed, say, ScaledObject could be created and in other it would be blocked. This feels too magic and isn't very standard.

If we changed the logic of the webhook is a way that it would be even blocking creation of those CRs in namespaces that are not watched (w/ some sensible message) that would complicate the possible scenario of having multiple KEDAs (with various configuration) installed in the same k8s cluster watching different namespaces.

Thanks, @zroubalik

@jkremser jkremser requested a review from zroubalik April 3, 2024 09:27
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

@jkremser please update changelog

jkremser and others added 2 commits April 9, 2024 11:53
Signed-off-by: Jirka Kremser <jiri.kremser@gmail.com>
Signed-off-by: Jirka Kremser <535866+jkremser@users.noreply.github.com>
@jkremser jkremser requested a review from zroubalik April 9, 2024 09:54
@jkremser
Copy link
Contributor Author

jkremser commented Apr 9, 2024

@zroubalik should be there now

…om namespaces that are not in WATCH_NAMESPACE

Signed-off-by: Jirka Kremser <jiri.kremser@gmail.com>
Signed-off-by: Jirka Kremser <jiri.kremser@gmail.com>
pkg/util/watch.go Show resolved Hide resolved
controllers/eventing/cloudeventsource_controller.go Outdated Show resolved Hide resolved
controllers/keda/scaledjob_controller.go Outdated Show resolved Hide resolved
controllers/keda/scaledobject_controller.go Outdated Show resolved Hide resolved
controllers/keda/triggerauthentication_controller.go Outdated Show resolved Hide resolved
Signed-off-by: Jirka Kremser <jiri.kremser@gmail.com>
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Could you please open a doc PR to document this setting? Probably a new section on this page:
https://keda.sh/docs/2.14/operate/cluster/

@jkremser
Copy link
Contributor Author

docs: kedacore/keda-docs#1357

@jkremser jkremser requested a review from zroubalik April 10, 2024 10:40
Signed-off-by: Jirka Kremser <jiri.kremser@gmail.com>
@jkremser jkremser force-pushed the support-multiple-namespaces branch from 49f9049 to 93e6055 Compare April 10, 2024 13:12
@zroubalik
Copy link
Member

zroubalik commented Apr 10, 2024

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

@zroubalik zroubalik merged commit 241c2e0 into kedacore:main Apr 10, 2024
20 checks passed
@jkremser jkremser deleted the support-multiple-namespaces branch April 10, 2024 17:49
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.

Multiple namespaces passed to WATCH_NAMESPACE
2 participants