override_host_set conflates different EDS health statuses

[markdroth]

This is something we discovered while in the process of implementing Envoy's stateful session affinity feature (see #16698) in gRPC.

From reading the code, it appears that there's a bug in Envoy's handling of the override_host_status field. The field allows setting individual health statuses for which the host-override behavior should be honored. However, internally, Envoy does not actually track all of the health statuses individually; in particular, it treats all of DRAINING, UNHEALTHY, and TIMEOUT as being exactly the same thing, and it treats UNKNOWN and HEALTHY as being the same thing (see code). So for example, if someone sets the override_host_status set to include DRAINING but not UNHEALTHY or TIMEOUT, Envoy will actually honor the host-override for all three of those states. I think this is a bug; it seems very clear from the API what is actually intended here.

I think the underlying problem here is that Envoy does not internally record the actual EDS health status in the Host object; instead, it does a lossy conversion to Host::Health. I suspect that the right fix is to have the Host object record the original EDS health status with full fidelity and then look at that when evaluating the override_host_status set.

CC @wbpcode @htuch @yanavlasov @sanjaypujare

[wbpcode]

I suspect that the right fix is to have the Host object record the original EDS health status with full fidelity and then look at that when evaluating the override_host_status set.

Sounds great. But how to handle the status set by active health checker or outlier detection? 🤔 Should we just keep two statuses flag or allow the health checker to overwrite the EDS status?

[markdroth]

I would think that an active health checker failure or an outlier detection ejection would map to the EDS UNHEALTHY state.

[sanjaypujare]

or may be maintain 2 status fields as @wbpcode suggested?

[markdroth]

I think having two status fields would make the API more complicated, because then we would have to expose a way of controlling whether a given host will be used as an override based on the value of both fields.

The only status that is currently exposed in the xDS API is the EDS health status enum, and that enum is capable of representing the health states that Envoy drives from places other than EDS as well, so it seems to me that that's the right thing to use here.

[wbpcode]

/assign

Any way, we should solve this problem.

[htuch]

FTR I think it makes sense to keep track of the original EDS status in Host, so +1 to that. At the very least this would make sense when reporting endpoint status in endpoint dumps, but there are other advantages as Mark points out.

[markdroth]

I don't think this is fully fixed yet.

[wbpcode]

Yeah, an additional minor patch is necessary to match the override status with the new health status.

I am doing it now.

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[wbpcode]

active