Monitor kubectl logs when port forwarding and retry on error

[nkubala]

When kubectl port-forward encounters an error (most commonly from the container backing a resource being restarted), an error is logged to stderr, but the process does not actually return with an exit code, or notify the caller in any way than an error occurred. During port forwarding, skaffold verifies that a port is correctly forwarded by attempting to attach to the specified port and ensuring that a listening connection cannot be established, but since kubectl hangs onto the port and continues to run successfully when an error is actually encountered, this check succeeds erroneously.

To handle this, the only indication we have from kubectl that something has gone wrong is from its logs to stderr, so we attach to these logs and search the stream for "error forwarding port". A Monitor() call is kicked off in a separate goroutine for each forwarded port, and on seeing this error string in the logs for the associated kubectl port-forward process, it will issue a callback to the EntryManager to restart the port forwarding operation entirely.

Additionally, kubectl itself will not actually log this error until the first failed connection to the forwarded port, so in the Monitor() call we issue a ping to the port once per second, to tease out any errors that kubectl might encounter. This is very suboptimal, but the implementation (and shortcomings) of kubectl port-forward give us very little to work with here.

update: I removed the ping from the monitor, since this could be really spammy for services where connection logs actually matter (e.g. nginx). after a redeploy, the first attempted connection to a forwarded service will fail, but skaffold will see this and trigger a reconnection. again, not great UX, but this is ultimately a workaround for a bug in kubectl.

Fixes #2523

[nkubala]

thinking about this more, i think we might not want the ping in the monitor, it could get really spammy. i'm thinking either make it much less frequent, or remove it entirely. thoughts?

[balopat]

LGTM. I was worried about continuously pinging the app as that can create spammy connection logs.

[codecov]

Codecov Report

Merging #2543 into master will decrease coverage by 0.14%.
The diff coverage is 3.84%.

Impacted Files	Coverage Δ
...ffold/kubernetes/portforward/port_forward_entry.go	100% <ø> (ø)	⬆️
...affold/kubernetes/portforward/kubectl_forwarder.go	0% <0%> (ø)	⬆️
...g/skaffold/kubernetes/portforward/entry_manager.go	87.23% <14.28%> (-5.95%)	⬇️

[codecov]

Codecov Report

Merging #2543 into master will decrease coverage by 0.14%.
The diff coverage is 3.84%.

Impacted Files	Coverage Δ
...ffold/kubernetes/portforward/port_forward_entry.go	100% <ø> (ø)	⬆️
...affold/kubernetes/portforward/kubectl_forwarder.go	0% <0%> (ø)	⬆️
...g/skaffold/kubernetes/portforward/entry_manager.go	87.23% <14.28%> (-5.95%)	⬇️