Knative Serving with istio fails, net-istio controller is OOMkilled

[skonto]

What version of Knative?

1.3.0

Expected Behavior

Knative Serving with istio should install as expected.

Actual Behavior

Net-istio-controller pod is OOMkilled.

Steps to Reproduce the Problem

Similar issue has been previously reported at client-go lib here.

• Install Knative serving on minikube with istio following the steps here.

• Generate a dummy secret of 0.5M size

$head -c 500K </dev/urandom > secret.txt

Run the following script to create some dummy secrets:

#!/bin/bash
for i in {1..100}
  do
     echo "Creating namespace test-$i"
     oc create namespace test-$i
done

for i in {1..100}
  do
    for j in {1..50}
      do
      oc create secret generic test-secret-$j --from-file=./secret.txt -n test-$i
    done
done

• Check net-istio-controller pod:

$ kubectl get pods -n knative-serving
NAME                                    READY   STATUS      RESTARTS   AGE
activator-54b76b65dc-nq4z6              1/1     Running     0          8m37s
autoscaler-bbd99dfbb-jdrv8              1/1     Running     0          8m36s
controller-64769f58d6-447r4             1/1     Running     0          8m36s
domain-mapping-846667c656-7b84v         1/1     Running     0          8m36s
domainmapping-webhook-f44f4785b-kft95   1/1     Running     0          8m35s
net-istio-controller-8d456687b-hq95g    0/1     OOMKilled   1          33s
net-istio-webhook-6d98796d6-r46jk       1/1     Running     0          8m6s
webhook-56c48c5b66-m9bf6                1/1     Running     0          8m34s

• Controller pods gets stuck here:

{"severity":"INFO","timestamp":"2022-03-24T10:43:27.745116111Z","logger":"net-istio-controller","caller":"metrics/metrics_worker.go:76","message":"Flushing the existing exporter before setting up the new exporter.","commit":"c096fb6"}
{"severity":"INFO","timestamp":"2022-03-24T10:43:27.745358289Z","logger":"net-istio-controller","caller":"metrics/prometheus_exporter.go:51","message":"Created Prometheus exporter with config: &{knative.dev/net-istio net_istio_controller prometheus 5000000000 <nil>  false 9090 0.0.0.0}. Start the server for Prometheus exporter.","commit":"c096fb6"}
{"severity":"INFO","timestamp":"2022-03-24T10:43:27.745437056Z","logger":"net-istio-controller","caller":"metrics/metrics_worker.go:91","message":"Successfully updated the metrics exporter; old config: <nil>; new config &{knative.dev/net-istio net_istio_controller prometheus 5000000000 <nil>  false 9090 0.0.0.0}","commit":"c096fb6"}
{"level":"info","ts":1648118607.8376493,"logger":"fallback","caller":"injection/injection.go:61","msg":"Starting informers..."}
(edited)

$ kubectl get secrets -A | wc -l
1080

Check allocs:

Obviously secret lister consumes all the mem.

Note: This is a simple reproducer, but downstream a customer also faces this in a large cluster with many more secrets of various sizes. Also this is not restricted to secrets as we have other type of resources listed. In addition net-kourier is expected to suffer as it uses similar logic.

Potential solution:

Increasing the limits in the net-istio-controller pod does fix the problem but this is not satisfying enough as size can be unpredictable and may not be acceptable. At the customer side mem limit had to be set to 2Gi so that the pod could start normally.

A better option should be to only consider for caching the secrets we care about.
We get the secret lister here. This is derived from a secret sharedInformer here.
On that informer we call get. That informer is initialized from context here which uses this informer factory.
The factory initialized here only uses namespace filtering. If we use WithTweakListOptions (essentially K8s API ListOption) we should be able to filter based on same label what is fetched before it is stored in the cache.

[skonto]

Created a ft doc for a solution here.

[dprotaso]

Related upstream issue: istio/istio#14598

[dprotaso]

Some background for others:

When autoTLS is enabled certs will be generated and stored in K8s secrets in the Knative Service's namespace. These secrets are passed to the KIngress. Unfortunately we can't pass the secret name as the credentialName to Istio's Gateway API - Server TLS Settings. The reason being is that credentialName is actually the name of the secret in the Ingress Pod's namespace. There are no cross namespace references.

Here's a diagram from: https://docs.google.com/document/d/1gDhj058q2PKKI9SWpxE23ExEXdOkLiueT8JA_oEJ8Mo/edit#heading=h.e52u9pac28y5

In order for TLS to work net-istio copies the secret from the KIngress namespace into the ingress pod's namespace. We use informers to watch for changes and to update the copies. Thus the spike occurs when there are lots of secrets - especially if they're big.

Alternatives

Fixes in Istio

I asked around and it turns out expanding credentialName to support namespaces (ie. limiting it to the gateway's namespace) is a security risk because the gateway label selector is cross namespace. That would allow for a hacker to create a pod with the same selector (impersonating and ingress pod) and start getting updates from XDS. That would allow them to read the gateway credential (cross namespace).

Gateway API

The suggestion was that this is supported using https://gateway-api.sigs.k8s.io/v1alpha2/api-types/referencepolicy/

This obviously doesn't help net-istio but something to check for in our net-gateway-api shim. If the user is able to use the gateway-api that could be an alternative.

[dprotaso]

I do wonder if we could watch for secret changes but not cache them indefinitely. If we hash the contents of the source secret and compare them to a label value on the destination secret we would know when changes occur. We would need a k8s secret namespace informer pointing to the ingress pod's namespace for quick lookups.

Or creating a custom informer with that only indexes things we track - though it would require handling cache misses. This doesn't alleviate the bandwidth spent transferring objects we don't care about over the wire though.

[dprotaso]

/triage accepted
/milestone Icebox

[skonto]

I do wonder if we could watch for secret changes but not cache them indefinitely. If we hash the contents of the source secret and compare them to a label value on the destination secret we would know when changes occur. We would need a k8s secret namespace informer pointing to the ingress pod's namespace for quick lookups.

Regarding istio, if I understand correctly, in order to hash the contents most likely we will have to fetch the secret from the user ns and also setup a watch there too, we monitor the source and the copy. Afaik Informer's cache is updated to latest afaik so if a secret is deleted we dont keep it indefinitely.

[dprotaso]

Regarding istio, if I understand correctly, in order to hash the contents most likely we will have to fetch the secret from the user ns and also setup a watch there too, we monitor the source and the copy.

Yeah

Afaik Informer's cache is updated to latest afaik so if a secret is deleted we dont keep it indefinitely.

Yeah deletion events remove items from the cache. I guess my point here is we can choose what the keep in the cache vs storing all the secrets.