KIC 2.0 - Tighten Service/Endpoints Watch

[shaneutt]

Prior to the first alpha release of KIC 2.0 we overhauled all the relevant controllers. One thing left behind that needs to be sorted out before KIC 2.0 GA is that the watches for types like Service and Endpoints are very broad. The purpose of this issue is to build some new watchers for these controllers which only watch Services we are supporting via Ingress and other objects, and therefore only the relevant Endpoints to reduce load and noise in the controller prior to GA.

[mflendrich]

@shaneutt is this still relevant?

[shaneutt]

Yes, we can still fine-tune the watches for services/endpoints. I am in favor of doing this improvement asap as it's basically a "quick win" for performance.

[rainest]

Are there known ways to do this? I took a brief survey around the neighborhood and didn't see anything that looked like it was filtering out Services automatically:

• ingress-nginx filters Secrets that are obviously not relevant (because they're Helm config, but it doesn't appear it filters anything else.
• Emsissary (formerly Ambassador) gets a list of interesting types, builds essentially unfiltered queries for them (you can specify your own label or field selectors, but they are empty by default), and watches them.
• Tyk apparently sidesteps this by just using service hostnames always, so it doesn't need a Service or Endpoint controller. Elsewhere it filters on well-defined fields that we can't (we care about non-TLS Secrets too).

While limiting Service, Endpoint, and Secret watches (along with other resources, but those seem the most likely egregious offenders for "tons of resources we don't care about) would be desirable, I don't know of a good way we could do it with the tools I know are available.

Manual filtering (a la Emissary) on labels would be quite effective, but tedious to maintain.

Automatic filtering at the watch level seems like it'd require that we build a list of all Services we're interested in based on names listed in Ingresses and then edit them to add a label, requiring an expansion of permissions, additional API interactions to add them, and new questions about cleanup (figuring out when to remove the label with multiple controllers and no guarantees about uptime is hard).

[shaneutt]

Are there known ways to do this? I took a brief survey around the neighborhood and didn't see anything that looked like it was filtering out Services automatically:

* ingress-nginx [filters Secrets that are obviously not relevant (because they're Helm config](https://github.com/kubernetes/ingress-nginx/blob/402f21bcb7402942f91258c8971ff472d81f5322/internal/ingress/controller/store/store.go#L252-L277), but it doesn't appear it filters anything else.

* Emsissary (formerly Ambassador) [gets a list of interesting types](https://github.com/emissary-ingress/emissary/blob/v2.0.1-rc.2/cmd/entrypoint/watcher.go#L36-L37), builds [essentially unfiltered queries for them](https://github.com/emissary-ingress/emissary/blob/v2.0.1-rc.2/cmd/entrypoint/interesting_types.go#L31-L37) (you can specify your own label or field selectors, but they are empty by default), and [watches them](https://github.com/emissary-ingress/emissary/blob/v2.0.1-rc.2/cmd/entrypoint/watcher.go#L168).

* Tyk apparently sidesteps this by just [using service hostnames always](https://github.com/TykTechnologies/tyk-operator/blob/v0.7.1/controllers/ingress_controller.go#L157), so it doesn't need a Service or Endpoint controller. Elsewhere it filters on [well-defined fields](https://github.com/TykTechnologies/tyk-operator/blob/v0.7.1/controllers/secret_cert_controller.go#L99-L103) that we can't (we care about non-TLS Secrets too).

While limiting Service, Endpoint, and Secret watches (along with other resources, but those seem the most likely egregious offenders for "tons of resources we don't care about) would be desirable, I don't know of a good way we could do it with the tools I know are available.

Manual filtering (a la Emissary) on labels would be quite effective, but tedious to maintain.

Automatic filtering at the watch level seems like it'd require that we build a list of all Services we're interested in based on names listed in Ingresses and then edit them to add a label, requiring an expansion of permissions, additional API interactions to add them, and new questions about cleanup (figuring out when to remove the label with multiple controllers and no guarantees about uptime is hard).

My initial thoughts on the how was to index all the services referenced by other resources using their namespace/name, meaning that until we have some Ingress, KongIngress, TCPIngress, UDPIngress or otherwise that's actually referencing a Service we filter it out in the predicates.

Here's a mockup idea of how this index might look implemented as a threadsafe map:

type ServiceIndex struct {
    lock *sync.RWMutex
    index map[string]bool
}

func (s *ServiceIndex) Put(namespace, name string) {
    s.lock.Lock()
    defer s.lock.Unlock()
    s[fmt.Sprintf("%s/%s", namespace, name)] = true
    return
}

func (s *ServiceIndex) Delete(namespace, name string) {
    s.lock.Lock()
    defer s.lock.Unlock()
    delete(s[fmt.Sprintf("%s/%s", namespace, name)])
    return
}

func (s *ServiceIndex) Get(namespace, name string) (exists bool) {
    s.lock.RLock()
    defer s.lock.RUnlock()
    _, exists = s[fmt.Sprintf("%s/%s", namespace, name)]
    return
}

Then the Proxy methods UpdateObject and DeleteObject methods could be retrofitted to populate this index:

func (p *clientgoCachedProxyResolver) UpdateObject(obj client.Object) error {
    serviceRefs := getServiceRefsForObj(obj)
    for _, ref := range serviceRefs {
        if !index.Get(ref.namespace, ref.name) {
            index.Put(ref.namespace, ref.name)
        }
    }
    return p.cache.Add(obj)

Then, ultimately, the ServiceIndex object could be injected into the predicates for the Service and Entrypoint controllers, using index.Get to determine whether or not the object needs to be queued.

This is just for illustration purposes, and I can already see some small problems and tweaks that need to be done but rather than actually write the whole thing before I know if you like it, let me know your thoughts on that overall idea?

[rainest]

My question is more what we then do with that list to make our client only see those resources. It seems like that would be done through either https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.9.6/pkg/builder#WatchesOption or https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.9.6/pkg/builder#ForOption and, while documentation on those is quite scant, I didn't think providing an array of names was an option, and you were instead stuck with either single-name watches or label-filtered watches.

[shaneutt]

My question is more what we then do with that list to make our client only see those resources. It seems like that would be done through either https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.9.6/pkg/builder#WatchesOption or https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.9.6/pkg/builder#ForOption and, while documentation on those is quite scant, I didn't think providing an array of names was an option, and you were instead stuck with either single-name watches or label-filtered watches.

This is what I was getting at with using the index inside of predicate functions, we could wrap CRUD events with these predicates and drop services (and endpoints) which aren't present in the index.

We could take this further however and deeper into the controller runtime API. I think if you're OK with the general idea there's enough here to at least put together a POC.

[shaneutt]

I've done some experiments with this and have some notes, but the write-up for that is going to be fairly large, include multiple alternative solutions, technical details, and some need for a decision making protocol to get alignment among maintainers.

Additionally this is a performance optimization which we understand as maintainers but has no linked issues nor have we seen it be of consequence in our own performance testing and so while it's a "nice to have" it does not appear to be a blocker for 2.0 GA.

As such my intention is to take this issue in a different direction and start a KEP which will include my experiments and notes and some options for us to consider and work through this that way so that we can build alignment and prescribe a robust solution at a priority that fits well with our other work.