✨ add namespace filtering #11370

marek-veber · 2024-11-04T12:25:54Z

What this PR does / why we need it:
This PR adds an option --excluded-namespace=<comma separated list of namespaces>

Which issue(s) this PR fixes:
Fixes #11193

/area/clusterresourceset

linux-foundation-easycla · 2024-11-04T12:26:00Z

The committers listed above are authorized under a signed CLA.

✅ login: marek-veber / name: Marek Veber (4c5ea6f)

k8s-ci-robot · 2024-11-04T12:26:01Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign fabriziopandini for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2024-11-04T12:26:02Z

This PR is currently missing an area label, which is used to identify the modified component when generating release notes.

Area labels can be added by org members by writing /area ${COMPONENT} in a comment

Please see the labels list for possible areas.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot · 2024-11-04T12:26:02Z

Welcome @marek-veber!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-11-04T12:26:03Z

Hi @marek-veber. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

sbueringer · 2024-11-04T13:03:30Z

I don't have time to contribute to this now / until code freeze / PTO / .. but let's please make sure we have a very clear understanding how whatever filtering we do works across:

the cache
the cached client (mgr.GetClient)
all our controllers (event filtering)
ClusterCache
every other part of Cluster API that is affected

I think something like the cache and the event filtering in our controller watches and especially all the calls we do on the cached client have to be carefully orchestrated with each other. Today we have a lot of options for performance improvements, these might go away if we surface the wrong configuration options to end users.

fabriziopandini · 2024-11-04T15:33:32Z

Let's also try to get a better understanding of the use case we are trying to address, because our current stance is that support for running multiple instances of Cluster API is should be considered best-effort + the issue linked above are not really well harmonised in term of requirements and proposed solution.

BTW, above statement, and the fact that as @sbueringer noted this is way trickier than it could appear at first sight, are the same reason why we have a few other long standing feature request in this area

JoelSpeed · 2024-11-05T11:12:47Z

bootstrap/kubeadm/main.go

@@ -122,6 +125,9 @@ func InitFlags(fs *pflag.FlagSet) {
 	fs.StringVar(&watchFilterValue, "watch-filter", "",
 		fmt.Sprintf("Label value that the controller watches to reconcile cluster-api objects. Label key is always %s. If unspecified, the controller watches for all cluster-api objects.", clusterv1.WatchLabel))

+	fs.StringVar(&watchFilterExcludedNamespaces, "excluded-namespaces", "",


Is there a StringSliceVar? We are using pflag so I think there is

JoelSpeed · 2024-11-05T11:19:47Z

util/predicates/generic_predicates.go

+func ResourceHasFilterExcludeNamespaces(scheme *runtime.Scheme, logger logr.Logger, excludedNamespaces []string) predicate.Funcs {
+	return predicate.Funcs{
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			return processIfNamespaceExcluded(scheme, logger.WithValues("predicate", "ResourceHasFilterExcludeNamespaces", "eventType", "update"), e.ObjectNew, excludedNamespaces)
+		},
+		CreateFunc: func(e event.CreateEvent) bool {
+			return processIfNamespaceExcluded(scheme, logger.WithValues("predicate", "ResourceHasFilterExcludeNamespaces", "eventType", "create"), e.Object, excludedNamespaces)
+		},
+		DeleteFunc: func(e event.DeleteEvent) bool {
+			return processIfNamespaceExcluded(scheme, logger.WithValues("predicate", "ResourceHasFilterExcludeNamespaces", "eventType", "delete"), e.Object, excludedNamespaces)
+		},
+		GenericFunc: func(e event.GenericEvent) bool {
+			return processIfNamespaceExcluded(scheme, logger.WithValues("predicate", "ResourceHasFilterExcludeNamespaces", "eventType", "generic"), e.Object, excludedNamespaces)
+		},
+	}
+}
+


I don't think this is the right approach to solving this issue. The suggestion in #11193 uses the FieldSelector concept of an API request to filter resources at the API server level, rather than the controller level.

Given the scale potential here, I think we want to follow that suggestion and move the filtering to the API server where possible.

I'm not sure if that approach also has RBAC advantages, perhaps we don't need permissions on the excluded namespace? But that could also be an advantage of the approach as suggested by Alberto

Thanks, @JoelSpeed! Please take a look at the new approach in this PR.

neolit123 · 2024-11-11T12:27:37Z

bootstrap/kubeadm/main.go

@@ -122,6 +124,9 @@ func InitFlags(fs *pflag.FlagSet) {
 	fs.StringVar(&watchFilterValue, "watch-filter", "",
 		fmt.Sprintf("Label value that the controller watches to reconcile cluster-api objects. Label key is always %s. If unspecified, the controller watches for all cluster-api objects.", clusterv1.WatchLabel))

+	fs.StringSliceVar(&watchFilterExcludedNamespaces, "excluded-namespaces", nil,


should it be excluded-namespace (singular, but still a comma separated list) to match the existing flag namespace, which cannot be changed to namespaces without a breaking change?

✨ update the --namespace <namespace> option to accept a comma-separated list for watching multiple selected namespaces #11397

changed --excluded-namespaces -> --excluded-namespace

sbueringer · 2024-11-11T12:29:04Z

/hold
xref: #11370 (comment) #11370 (comment)
(just to make sure these comments are not getting missed)

neolit123 · 2024-11-11T12:31:21Z

i recall recent discussions in core k8s on a KEP somewhere that this pattern is not preferred i.e. adding both an allow list and a deny list flag at the same time.

how about keeping this simple and doing the following:

--namespace not specified == watch all NS
--namespace specifies some NS as a comma separated list == only watch those NS

that means for a NS to be watched it must be in the --namespace list.
downside of that is that a controller restart is needed every time a new NS must be watched.
then again, same goes for anytime you need to exclude a certain NS.

marek-veber · 2024-11-11T13:00:59Z

We also need the following behavior (I’ve split it into two separate PRs):

If --namespace is not specified, it defaults to watching all namespaces.
Use --exclude-namespace=ns1,ns2,... to watch all namespaces except specified ones (e.g., ns1, ns2).

Additionally, there’s a more flexible option, see: #7775 (comment) that allows for more complex conditions. However, it doesn’t seem to support using a direct "positive" list of namespaces to watch.

fabriziopandini · 2024-11-13T16:09:52Z

+1 to what @sbueringer said
we should not add flags until we have a clear idea of what we want to achieve.
also, as written in a few comments, this is not as simple as adding a flag.

k8s-ci-robot added the do-not-merge/needs-area PR is missing an area label label Nov 4, 2024

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Nov 4, 2024

k8s-ci-robot requested review from elmiko and g-gaston November 4, 2024 12:26

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 4, 2024

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 4, 2024

JoelSpeed reviewed Nov 5, 2024

View reviewed changes

marek-veber changed the title ~~add namespace filtering ✨~~ (:sparkles:) add namespace filtering Nov 6, 2024

marek-veber changed the title ~~(:sparkles:) add namespace filtering~~ ✨ add namespace filtering Nov 6, 2024

marek-veber force-pushed the namespace-filtering branch from 84ef63f to beba100 Compare November 6, 2024 16:29

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 6, 2024

marek-veber force-pushed the namespace-filtering branch from beba100 to bbd7909 Compare November 11, 2024 08:39

neolit123 reviewed Nov 11, 2024

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 11, 2024

add namespace filtering

4c5ea6f

marek-veber force-pushed the namespace-filtering branch from bbd7909 to 4c5ea6f Compare November 11, 2024 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ add namespace filtering #11370

✨ add namespace filtering #11370

marek-veber commented Nov 4, 2024 •

edited

Loading

linux-foundation-easycla bot commented Nov 4, 2024 •

edited

Loading

k8s-ci-robot commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

sbueringer commented Nov 4, 2024

fabriziopandini commented Nov 4, 2024 •

edited

Loading

JoelSpeed Nov 5, 2024

marek-veber Nov 6, 2024

JoelSpeed Nov 5, 2024

marek-veber Nov 6, 2024

neolit123 Nov 11, 2024

marek-veber Nov 11, 2024

sbueringer commented Nov 11, 2024 •

edited

Loading

neolit123 commented Nov 11, 2024 •

edited

Loading

marek-veber commented Nov 11, 2024 •

edited

Loading

fabriziopandini commented Nov 13, 2024

✨ add namespace filtering #11370

Are you sure you want to change the base?

✨ add namespace filtering #11370

Conversation

marek-veber commented Nov 4, 2024 • edited Loading

linux-foundation-easycla bot commented Nov 4, 2024 • edited Loading

k8s-ci-robot commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

k8s-ci-robot commented Nov 4, 2024

sbueringer commented Nov 4, 2024

fabriziopandini commented Nov 4, 2024 • edited Loading

JoelSpeed Nov 5, 2024

Choose a reason for hiding this comment

marek-veber Nov 6, 2024

Choose a reason for hiding this comment

JoelSpeed Nov 5, 2024

Choose a reason for hiding this comment

marek-veber Nov 6, 2024

Choose a reason for hiding this comment

neolit123 Nov 11, 2024

Choose a reason for hiding this comment

marek-veber Nov 11, 2024

Choose a reason for hiding this comment

sbueringer commented Nov 11, 2024 • edited Loading

neolit123 commented Nov 11, 2024 • edited Loading

marek-veber commented Nov 11, 2024 • edited Loading

fabriziopandini commented Nov 13, 2024

marek-veber commented Nov 4, 2024 •

edited

Loading

linux-foundation-easycla bot commented Nov 4, 2024 •

edited

Loading

fabriziopandini commented Nov 4, 2024 •

edited

Loading

sbueringer commented Nov 11, 2024 •

edited

Loading

neolit123 commented Nov 11, 2024 •

edited

Loading

marek-veber commented Nov 11, 2024 •

edited

Loading