Missing documentation on FilteringResourceEventHandler behavior with dynamic filters

[howardjohn]

https://github.com/kubernetes/kubernetes/blob/73bd83cfa74730ecab263a886698b7ea8a2e02ad/staging/src/k8s.io/client-go/tools/cache/controller.go#L291-L299 defines a convenient filtering handler.

However, it doesn't define what the behavior is with a FilterFunc that is not static. For example, we may have

AllowedNamespaces := sets.New("namespace1")
handler.FilterFunc = func(obj any) bool {
  return AllowedNamespaces.Has(obj.GetNamespace)
}

// Later...
AllowedNamespaces.Insert("new-namespace")

The current behavior is:

• New adds and deletes will not be handled
• there is races in OnUpdate, as we check the filter against newer and older seqentually. We could have the filter change in between these, leading to weird behavior like calling OnUpdate when OnAdd had never previously been called.

It would be helpful to document and test what is intended or not intended to be supported here.

We are also actually trying to use dynamic filters, so if there are any examples that already solve these problems it would be very helpful

/sig apimachinery

[k8s-ci-robot]

@howardjohn: The label(s) sig/apimachinery cannot be applied, because the repository doesn't have them.

In response to [this](https://github.com/kubernetes/kubernetes/issues/117706): >https://github.com/kubernetes/kubernetes/blob/73bd83cfa74730ecab263a886698b7ea8a2e02ad/staging/src/k8s.io/client-go/tools/cache/controller.go#L291-L299 defines a convenient filtering handler. > >However, it doesn't define what the behavior is with a FilterFunc that is not static. For example, we may have > >```go >AllowedNamespaces := sets.New("namespace1") >handler.FilterFunc = func(obj any) bool { > return AllowedNamespaces.Has(obj.GetNamespace) >} > >// Later... >AllowedNamespaces.Insert("new-namespace") >``` > >The current behavior is: >* New adds and deletes will not be handled >* there is races in OnUpdate, as we check the filter against `newer` and `older` seqentually. We could have the filter change in between these, leading to weird behavior like calling OnUpdate when OnAdd had never previously been called. > >It would be helpful to document and test what is intended or not intended to be supported here. > >We are also actually trying to use dynamic filters, so if there are any examples that already solve these problems it would be very helpful > >/sig apimachinery Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

[howardjohn]

/sig api-machinery

[cici37]

/cc @smarterclayton @lavalamp @MikeSpreitzer /help Seems like the code hasn't been changed for while. Would anyone have time to comment here? Thank you :)

[iyear]

/assign /remove-help

[iyear]

Some doubts for New adds and deletes will not be handled, do I misunderstand it?

filter := sets.New("foo")
handler := FilteringResourceEventHandler{
    FilterFunc: func(obj interface{}) bool {
        return filter.Has(obj.(string))
    },
    Handler: ResourceEventHandlerFuncs{AddFunc: func(obj interface{}) {
        t.Logf("add %v", obj)
    }},
}
handler.OnAdd("foo", true)
handler.OnAdd("bar", true) // should not be added
filter.Insert("bar")
handler.OnAdd("bar", true) 

This will output add bar:

add foo
add bar

which means filter is dynamic, is this the expected behavior?

[howardjohn]

Sorry to be unclear. This is what I mean:

filter := sets.New("foo")
handler := FilteringResourceEventHandler{
    FilterFunc: func(obj interface{}) bool {
        return filter.Has(obj.(string))
    },
    Handler: ResourceEventHandlerFuncs{AddFunc: func(obj interface{}) {
        t.Logf("add %v", obj)
    }},
}
handler.OnAdd("foo", true)
handler.OnAdd("bar", true) // should not be added
filter.Insert("bar")

Result:

add foo

So the user will need to do things on their end to get add bar to occur, should they want it.

[iyear]

Thanks for the reply, I now understand the "dynamic" you are talking about. It might be a bit hard to solve, I may add some comments to clarify it first.

[jpbetz]

/triage accepted

[MikeSpreitzer]

FilteringResourceEventHandler does not handle a dynamic filter function in what I would consider the proper way. I agree that the comment on FilteringResourceEventHandler should be updated to stipulate that the behavior of the filter function does not vary over time.

[wafuwafu13]

I would like to add comments and tests since this has not been worked on for a while.

/assign

[wafuwafu13]

/assign

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[Jefftree]

Hi @howardjohn, this issue hasn't been updated over a year, is there still work needed? /triage accepted

[howardjohn]

Nothing has changed since the issue was reported