#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
|
@gehrkefc Currently this PR points to |
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| logrus.Infoln("Goroutine stopped.") |
There was a problem hiding this comment.
Unlikely but possible to have the context cancelled before we send anything in the r.readyCh. So Start() would never exit.
forward/forward.go
Outdated
| podClient v1.PodController | ||
| namespace string | ||
| labelSelector string | ||
| Ports []string |
There was a problem hiding this comment.
Any reason to have Ports public?
There was a problem hiding this comment.
No reason, making it private. Thanks!
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() ?
proxy/config.go
Outdated
| package proxy | ||
|
|
||
| import ( | ||
| "github.com/kelseyhightower/envconfig" |
There was a problem hiding this comment.
I'm not sure this is necessary. Simple os.Getenv should be enough. Also, looking at github.com/kelseyhightower/envconfig, I can see the last release was in 2019 and last commit was three years ago.
There was a problem hiding this comment.
Sure, this came from Max code. I'll change it.
proxy/config.go
Outdated
| ProxyPort int `required:"true" split_words:"true"` | ||
| PeerPort int `required:"true" split_words:"true"` | ||
| HTTPSPort int `required:"true" split_words:"true"` |
There was a problem hiding this comment.
Would be great to add comments to describe these, especially the ports because they can be pretty confusing.
There was a problem hiding this comment.
Just added some comments. Please see if it's enough.
proxy/server.go
Outdated
| core, err := core.NewFactoryFromConfigWithOptions(restConfig, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("build secret controller failed w/ err: %w", err) | ||
| } | ||
| secretController := core.Core().V1().Secret() |
There was a problem hiding this comment.
I don't think the controller / factory is ever started, is that correct? I'm not sure what the behavior of dynamiclistener is in that case, but I do see that it calls OnChange somewhere, so I think we'll want this to work.
There was a problem hiding this comment.
It currently stores the secret in the right place. Is there any other method I should call before using it ?
There was a problem hiding this comment.
Yeah you'll have to run the following code:
if err := core.Start(ctx, 1); err != nil {
// handle error
}This will sync the Secrets object with k8s into the in-memory cache and will start 1 worker to react on Secret changes.
proxyclient/client.go
Outdated
| logrus.Error("build secret controller failed: %w, defaulting to non TLS connection", err) | ||
| return nonTLSDialer, err |
There was a problem hiding this comment.
This makes me a bit nervous, is there a reason for falling back to insecure here?
There was a problem hiding this comment.
Not specifically. Do you prefer I just return the err and fail it ?
There was a problem hiding this comment.
Yes let's just return an error and fail. We don't need to support non-TLS connections (at least for now).
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. |
Issue: rancher/rancher#47037