Improving port forwarding error handling

[ramiro-gamarra]

Reason for Change:

Port forwarding to a Kubernetes pod can be brittle but can be restarted if the appropriate signals are handled. This PR improves the API of the internal port forwarding wrapper PortForwarder to allow clients to maintain port forwarding sessions alive.

Example usage:

pfOpts := PortForwardingOpts{
    Namespace:     "default",
    LabelSelector: "type=goldpinger-pod",
    LocalPort:     9090,
    DestPort:      8080,
}

pf, _ := NewPortForwarder(restConfig, logger, pfOpts)

if err := pf.Forward(ctx); err != nil {
    // errors if initial connection fails
}

go pf.KeepAlive(ctx) // handles connection failures and re-attempts to port forward any pod matching the opts to the same local address

defer pf.Stop()

• uses conventional commit messages
• includes documentation
• adds unit tests

Notes:

[ramiro-gamarra]

Reconsidering if we even need to export this method? Seems like callers would just want to forward and stop, not have to also enable keep alive explicitly?

[rbtr]

agree

[timraymond]

What is the problem that motivated these changes? They're good, but I just don't understand the wider context.

[ramiro-gamarra]

What is the problem that motivated these changes? They're good, but I just don't understand the wider context.

@timraymond We leverage port forwarding in integration tests that deploy goldpinger to a kubernetes cluster, since the tests need to make assertions against responses from the API that goldpinger pods expose, and these endpoints are private to the cluster (tests are executed on a VM on a different network from the k8s cluster). Currently, if the port forwarding session dies, that error is not communicated to the ongoing test suite, and tests will eventually fail due to timeouts, leading to PR checks needing to be retried, possibly multiple times.

[rbtr]

I think this is worth taking as-is to immediately improve the CI reliability. Tweaks like maybe removing KeepAlive can come later 🙂