When the data plane can't reach the apiserver, it silently fails to forward

[evankanderson]

Describe the bug
In a non-default namespace, the default-broker-filter deployment fails to actually load any Trigger data, and instead reports:

sigs.k8s.io/controller-runtime/pkg/cache/internal/informers_map.go:126: Failed to list *v1alpha1.Trigger: Get https://10.15.240.1:443/apis/eventing.knative.dev/v1alpha1/namespaces/demo/triggers?limit=500&resourceVersion=0: net/http: TLS handshake timeout

and

jsonPayload: {
  caller:  "filter/main.go:52"   
  error:  "Get https://10.15.240.1:443/api?timeout=32s: dial tcp 10.15.240.1:443: connect: connection refused"   
  level:  "fatal"   
  logger:  "provisioner"   
  msg:  "Error starting up."   
  stacktrace:  "main.main
	/go/src/github.com/knative/eventing/cmd/broker/filter/main.go:52
runtime.main
	/usr/local/go/src/runtime/proc.go:200"   
  ts:  "2019-04-18T22:36:24.188Z"   
 }

Expected behavior
Broker and Trigger work in a namespace other than the default namespace.

To Reproduce
Deploy Broker/Trigger to a non-default namespace and then attempt to deliver via Trigger.

Knative release version
Release 0.5

[evankanderson]

/priority awaiting-more-evidence

We should repro with an e2e test or by hand?

[mchmarny]

I can replicate using this source/trigger/service combo in demo namespace. Will try to validate in default

[n3wscott]

/cc @nachocano
Nacho is currently rewriting the broker controller.

[Harwayne]

I don't think this is caused by using a Broker in a non-default namespace. The Broker e2e tests have been running in a non-default namespace since their creation.

TestDefaultBrokerWithManyTriggers runs in a non-default namespace (its name is generated here).

[evankanderson]

I think mark indicated that deleting and re-creating the pods got him unstuck.

However, the Broker and Triggers' status was Ready during this entire incident, despite the fact that the actual filter was unable to talk to the apiserver. I wonder if there is a way that we could detect this and report a failed status when the broker-filter pod(s) are unable to talk to the apiserver.

[evankanderson]

Renaming this bug; it appears to have been an intermittent network issue,

The larger issue is that all the objects reported Ready, but no Triggers were actually being routed. Traffic went as follows:

[source] --> [broker-filter] --> [broker-internal-channel] --> [subscription] --> [broker-filter] 🗑

[grantr]

@Harwayne has been working on making sure Ready status is only reported when the data plane is truly ready: #1064 #1071. Those PRs were merged before this bug was reported, but maybe the report was made with an older nightly? 🤞

[Harwayne]

Ready status is only reported when the data plane is truly ready: #1064 #1071.

That's not quite accurate. Those PRs don't mark the Broker/Trigger ready until all their pieces are ready, but that is all control plane activity. In particular, if the Pod is healthy and available, even when it can't talk to the API server, then we would still have this problem. An 'easy' fix would be to crash loop the container until it can talk to the API server. A better fix would be to add a readiness probe to the Deployments.

[grantr]

A better fix would be to add a readiness probe to the Deployments.

IIUC this used to be impossible or at least very difficult with mTLS enabled in Istio. According to istio/istio#4429 that's fixed, but unclear whether the fix is released.

@tcnghia can we start using liveness and readiness probes in the data path yet?

[tcnghia]

@grantr In serving we have been using readiness probe in data path for a while now. @ZhiminXiang is busy with AutoTLS work and hasn't gotten to confirming mTLS (probe rewrite) yet. May be we will put more investigation in 0.7 milestone after AutoTLS is done.

[evankanderson]

I believe we've also removed the Istio requirement at this point.

[matzew]

related: #1166 ?

[vaikas]

@Harwayne did this get fixed somewhere along the way?

[Harwayne]

@Harwayne did this get fixed somewhere along the way?

I don't think so. The removal of Istio has likely made it far less likely to occur, but I think it would still present the same problems. I think the readiness probe described in #1085 (comment) is still the proper, long term fix.

[vaikas]

Should we then have an issue to add a readiness probe to explicitly spell out what the problem is. Title seems to state a different issue?

[Harwayne]

/close

Closing in favor of #1656, which describes the desired solution.

[knative-prow-robot]

@Harwayne: Closing this issue.

In response to this:

/close

Closing in favor of #1656, which describes the desired solution.

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/test-infra repository.