delay health checks until transport socket secrets are ready.

[mpuncel]

This fixes a sequencing issue that could result in undesirable Envoy
bootup times. When active health checks are configured on a cluster
using a transport socket that uses SDS to fetch secrets, it was possible
for the initial health check to run before secrets are loaded, which
results in a failure. The health check would then wait for the
no_traffic_interval (default of 60s) before trying again.

This commit makes health checks wait until secrets are loaded before
starting, implemented by adding support for registering a callback on
TransportSocketFactory. SslSocketFactory supports invoking the callback
when secrets are loaded, and all other socket types currently invoke the
callback immediately, preserving existing behavior.

Signed-off-by: Michael Puncel mpuncel@squareup.com

Commit Message:
delay health checks until transport socket secrets are ready.

This fixes a sequencing issue that could result in undesirable Envoy
bootup times. When active health checks are configured on a cluster
using a transport socket that uses SDS to fetch secrets, it was possible
for the initial health check to run before secrets are loaded, which
results in a failure. The health check would then wait for the
no_traffic_interval (default of 60s) before trying again.

This commit makes health checks wait until secrets are loaded before
starting, implemented by adding support for registering a callback on
TransportSocketFactory. SslSocketFactory supports invoking the callback
when secrets are loaded, and all other socket types currently invoke the
callback immediately, preserving existing behavior.

Additional Description:
Risk Level: Medium
Testing: Unit tests
Docs Changes: N/A
Release Notes: Updated
Fixes #12389

[PiotrSikora]

Drive-by comment, but wouldn't #13344 (keep clusters warming) also solve this?

[mpuncel]

@PiotrSikora interesting, I was unaware of that PR! From a quick look, I don't think that PR actually delays health checks, so it does not address the problem from #12389. If there was a delay fetching a secret, Envoy would still enter the LIVE state in the middle of a no_traffic_interval which could be a minute long, so there could be up to a minute where Envoy is "ready" but has not performed any "real" health checks (meaning TLS secrets were loaded for it).

I do think this PR might coincidentally fix #11120 for some cases, because the cluster is not considered warmed until health checks have run for one interval. There would still be some edge cases, however, for instance if health checks are performed over a raw_buffer socket that does not need to load secrets, they would still run immediately and the cluster might be marked as warmed before all secrets are loaded. So that PR still has value as well.

[mpuncel]

That PR could maybe be modified to delay loadCluster until secrets have loaded though, which I think would then obviate the need for this one. That might block more of the cluster initialization process than is necessary though, such as DNS resolution or EDS

[lizan]

@PiotrSikora interesting, I was unaware of that PR! From a quick look, I don't think that PR actually delays health checks, so it does not address the problem from #12389. If there was a delay fetching a secret, Envoy would still enter the LIVE state in the middle of a no_traffic_interval which could be a minute long, so there could be up to a minute where Envoy is "ready" but has not performed any "real" health checks (meaning TLS secrets were loaded for it).

Envoy will still enter LIVE state but the cluster won't be active, then no HC will be there, isn't that the case?

[mpuncel]

@lizan it looks like that PR doesn't change the flow until onClusterInit() is called, which is after the initial round of health checks has completed, so it doesn't solve my problem.

Even after that PR is merged, I believe this problematic sequence is still possible:

1. Health checks are performed (and fail because secrets aren't loaded)
2. (new functionality) cluster manager properly recognizes that the cluster isn't "warmed" and waiting on secrets load
3. secrets load, Envoy goes LIVE, all clusters marked active.
4. envoy goes LIVE, requests fail with "no healthy upstream". This period could be almost 60 seconds long because of the default for no_traffic_interval
5. Requests succeed after the next health check interval

That said, that PR could maybe be modified to block for secrets updating before calling cluster.initialize() or something like that, but that will also delay EDS/DNS waiting on secrets that aren't necessary. Alternatively change the cluster API so it can do like cluster.startHealthChecks() after secrets load

[lizan]

Can you merge main branch?

[mpuncel]

merged!

[rgs1]

x-posting in case someone else bumps into this issue, we had to revert this internally because it breaks (on a significant number of cases) hot restart for us (gets stuck in PRE_INIT).

[mattklein123]

@rgs1 @mpuncel can we get this change reverted while we debug?

[rgs1]

#13639

[mpuncel]

@mattklein123 fine with me! Where is the best place to discuss what might have gone wrong here and how to proceed? Should I open an issue or another PR after the revert is merged?

[mattklein123]

@mattklein123 fine with me! Where is the best place to discuss what might have gone wrong here and how to proceed? Should I open an issue or another PR after the revert is merged?

I would reopen a PR, and then try to work with @rgs1 to debug? I don't immediately see how hot restart would change the behavior of this PR so will need some poking.