#47037 - Create Remote Dialer Proxy #90
Conversation
* Add a port-forward type * Add support for automatic reconnects when pods are terminated * List clients * Add a Dockerfile for the proxy * Add proxy * Update go.mod * Update package name * Add config parsing * Immediately close connection when there are no clients
gehrkefc
changed the title
|
|
||
| if failed { | ||
| return err | ||
| } |
There was a problem hiding this comment.
We can probably get rid of failed and use err in the same way
var readyErr error
...
if err := r.runForwarder(ctx, r.readyCh, r.stopCh, r.Ports); err != nil {
if errors.Is(err, portforward.ErrLostConnectionToPod) {
logrus.Errorf("Lost connection to pod (no automatic retry in this refactor): %v", err)
} else {
logrus.Errorf("Non-restartable error: %v", err)
readyErr = err
r.readyCh <- struct{}{}
return
}
}
...
if readyErr != nil {
return err
}
There was a problem hiding this comment.
Sure, just changed it. Thanks!
There was a problem hiding this comment.
I think this will trigger a race condition with the race detector because we have:
G1: write readyErr (init)
G2: write readyErr
G1: read readyErr
There was a problem hiding this comment.
Unresolving this due to my comment above^ Do you disagree @joshmeranda / @gehrkefc
|
@gehrkefc Currently this PR points to |
|
|
||
| if failed { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I think this will trigger a race condition with the race detector because we have:
G1: write readyErr (init)
G2: write readyErr
G1: read readyErr
forward/forward.go
Outdated
| logrus.Infoln("Goroutine stopped.") | ||
| return | ||
| default: | ||
| err := r.runForwarder(ctx, r.readyCh, r.stopCh, r.Ports) |
There was a problem hiding this comment.
Could we replace r.stopCh with ctx.Done() ?
There was a problem hiding this comment.
Sure. Replaced it! Thanks!
proxyclient/client.go
Outdated
| secret, err := secretController.Get(namespace, certSecretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| crtData, exists := secret.Data["tls.crt"] | ||
| if !exists { | ||
| return nil, fmt.Errorf("secret %s/%s missing tls.crt field", namespace, certSecretName) | ||
| } | ||
|
|
||
| rootCAs := x509.NewCertPool() | ||
| if ok := rootCAs.AppendCertsFromPEM(crtData); !ok { | ||
| return nil, fmt.Errorf("failed to parse tls.crt from secret into a CA pool") | ||
| } |
There was a problem hiding this comment.
The certs will be rotated by dynamiclistener before they expire (or at least that's the intention) so we should react to changes for this. We can add that functionality in the next PR, same as we're doing with @joshmeranda 's PR. We do need that though as part of the GH issue.
There was a problem hiding this comment.
I've implemented this. Please check it. I tested it modifying a local copy of dynamic listener to generate a new cert at each 20s. Please check the image below to see both working.
In the lower terminal you'll see the server regenerating the secret and updating it. Then in the upper terminal you'll see the client receiving the event with the new cert.
I've made another change in the server in the runProxyListener function because of a high cpu usage. |
Co-authored-by: Tom Lebreux <tom.lebreux@suse.com>
| Transport: roundTripper, | ||
| }, http.MethodPost, &serverURL) | ||
|
|
||
| stdout, stderr := new(bytes.Buffer), new(bytes.Buffer) |
There was a problem hiding this comment.
stdout and sterr aren't actually used. Meaning we're going to be adding to the buffer but those aren't being read from anywhere.
I'm not exactly sure what information goes into them, but I assume we'd want to print that information somewhere. We can figure this out in another iteration.
| if newSecret.Name == c.certSecretName && newSecret.Namespace == c.namespace { | ||
| rootCAs, err := buildCertFromSecret(c.namespace, c.certSecretName, newSecret) | ||
| if err != nil { | ||
| logrus.Errorf("build certificate failed: %s", err.Error()) |
There was a problem hiding this comment.
nit: We're already returning the error so wrangler will also print it, meaning we'll print the error twice.
I'd suggest just wrapping the error and returning it.
Issue: rancher/rancher#47037