Implementation of Namespace Filtering with Cache Optimization for KAgent Controller #299
Conversation
sbx0r
force-pushed
the
feat/298/namespace-filtering
branch
2 times, most recently
from
b9862f0 to
8a4f6bc
Compare
sbx0r
force-pushed
the
feat/298/namespace-filtering
branch
from
db1ba6f to
da42a30
Compare
| # -- Namespaces the controller should watch. | ||
| # If empty, the controller will watch all namespaces (subject to RBAC permissions). | ||
| # @default -- [] (watches all namespaces) | ||
| watchNamespaces: [] |
There was a problem hiding this comment.
At a high level why would these ever need to be different?
There was a problem hiding this comment.
for example:
caching on [ns1,ns2,ns3]
filtering on:
- agents to the [ns1, ns2, ns3]
- tools to the [ns1, ns2]
just for future improvements.
one for cache optimization (for bigger clusters)
other one is recommended for security layer and more specific filtering
https://sdk.operatorframework.io/docs/building-operators/golang/operator-scope/#watching-resources-in-a-set-of-namespaces
There was a problem hiding this comment.
After deeper research i've decided to delete predicates and all surrounding logic.
Only namespace watch will persist for now.
The additional layer of predicates may be helpful (but not necessarily) during the RBAC module implementation. For now it only makes a noise and complicates the codebase
sbx0r
force-pushed
the
feat/298/namespace-filtering
branch
10 times, most recently
from
f1f6683 to
a042b8c
Compare
sbx0r
force-pushed
the
feat/298/namespace-filtering
branch
from
a042b8c to
60de072
Compare
helm/kagent/templates/_helpers.tpl
Outdated
| {{- $_ := set $nsSet .Release.Namespace "" }} |
There was a problem hiding this comment.
So I think there's actually a bug here. In the controller, the only way to watch all namespaces is by making sure this list is empty, but this function ensures that there's always at least one value in the list, aka the release namespace.
There was a problem hiding this comment.
You're right!
I've forgot to commit change for it 😫
There was a problem hiding this comment.
I agree that the better solution overall is to ONLY watch the release namespace by default, but I also don't want to break existing users. Ideally in the future we could offer an option where the user can use namespace scoped RBAC instead of cluster scoped, but that's definitely out of scope for this PR.
There was a problem hiding this comment.
solved!
Thanks a lot for this finding!
…clude release namespace automatically
sbx0r
force-pushed
the
feat/298/namespace-filtering
branch
from
5aed6fd to
c38f4c4
Compare
|
@EItanya WAIT! |
|
Sorry for the alert. After merging this PR, I’ll start working on multi-namespace CR handling. It’ll require changes to the data models and updates across all layers of the system (app/controller/UI). |
|
Would you mind creating a quick design in the issue so we're all on the same page? |
| } | ||
|
|
||
| return validNamespaces | ||
| } |
There was a problem hiding this comment.
I'm going to approve, but in a follow-up please move these functions and tests into a different package.
/resolves #298