search

google / knative-gcp

GCP event implementations to use with Knative Eventing.
https://github.com/knative/eventing
Apache License 2.0
159 stars 75 forks source link

BrokerCell should ignore non-sensical Targets #2100

Closed Harwayne closed 3 years ago

Harwayne commented 3 years ago

Problem The BrokerCell will start up [Handlers]() for all Targets in the passed configuration that have target.State == config.State_READY.

https://github.com/google/knative-gcp/blob/5670000e3a2e6a8314b2f3bc992cef906e53513d/pkg/broker/handler/retry.go#L128-L130

But, it will not shut them down when the target becomes non-sensical. For example, when target.Address == "". Clearly no event can be successfully sent to the empty string URL. In such a case the BrokerCell should simply stop polling that Target.

An additional check should be added here to shut the handler down if it has any non-sensical values (e.g. target.Address == "").

Persona: Which persona is this feature for? System Integrator - There are no misleading error messages in the logs about failures to send events.

Additional context (optional) I believe we can get into this situation by:

  1. Create a Trigger pointing at a Service that doesn't exist.
  2. Send an event to the Broker.
  3. Observe in the Fanout (and retry?) logs "error": "Post \"\": unsupported protocol scheme \"\"",.

It might require that the Trigger in 1 was Ready=True at some point, but I don't think so.

One potential downside of this is that the Trigger event_count metrics will not be recording errors. The user would need a way to notice that the Trigger is Ready=False, which I'm not sure we have a metric around.

quentin-cha commented 3 years ago

/assign

github-actions[bot] commented 3 years ago

This issue is stale because it has been open for 90 days with no activity. It will automatically close after 30 more days of inactivity. Reopen the issue with /reopen. Mark the issue as fresh by adding the comment /remove-lifecycle stale.