Add file transfer support to Connect#16880
Conversation
| clusterServers, err := proxyClient.FindNodesByFilters(server.Context(), proto.ListResourcesRequest{ | ||
| Namespace: defaults.Namespace, | ||
| }) | ||
|
|
||
| if err != nil { | ||
| return trace.Wrap(err) | ||
| } | ||
|
|
||
| var foundServer types.Server | ||
| for _, clusterServer := range clusterServers { | ||
| if clusterServer.GetName() == request.GetServerId() { | ||
| foundServer = clusterServer | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if foundServer == nil { | ||
| return trace.BadParameter("Requested server does not exist") | ||
| } | ||
|
|
||
| err = c.clusterClient.TransferFiles(server.Context(), request.GetLogin(), foundServer.GetHostname()+":0", config) |
There was a problem hiding this comment.
I'm generally unsure about this part.
Is there a better way to provide host address to clusterClient.TransferFiles? Perhaps instead of sending serverId from Connect I should send the hostname? By doing this I wouldn't have to connect to proxy (clusterClient.TransferFiles also runs this function so it takes a good few seconds all together).
Additionally, downloading a file from a leaf cluster fails with an error No proxy address specified, missed --proxy flag?, did I miss something obvious?
There was a problem hiding this comment.
I'll try to look into it tomorrow, I probably won't have time for that today unfortunately.
There was a problem hiding this comment.
I looked into this and I think you're right, passing the hostname seems to be the way to go. I checked it and that's how both the Web UI and tsh does it. Well, in tsh you can specify the address instead of the hostname as well.
As for the problem with leaf clusters: it looks like the server URI that is used for the TransferFile RPC is wrong. Mine said /clusters/teleport-local-leaf/servers/4bbde2c9-b805-4dd8-8032-a0bed162d85b, suggesting that teleport-local-leaf is a root cluster but it's a leaf cluster.
There was a problem hiding this comment.
Thanks for help with this, works perfectly!
The only thing that puzzles me is request.GetHostname()+":0".
Without a port, it fails with an error failed connecting to node teleport_root. invalid format for proxy request: "proxy:teleport_root@default@teleport_root", expected 'proxy:host:port@cluster'.
I assume I have to add :0 to the hostname, so the the node address is resolved automatically?
There was a problem hiding this comment.
I don't know the details but yeah, that seems to be the case. tsh ssh needs to support both commands:
tsh scp file.text root@mbp.teleport-local:/tmp/
tsh scp -P 3022 file.text root@127.0.0.1:/tmp/
So I imagine in case port is set to 0, tsh tries to resolve the hostname to one of the available SSH servers.
| // ClusterLogin logs out a user from cluster | ||
| rpc Logout(LogoutRequest) returns (EmptyResponse); | ||
| // TransferFile downloads/uploads a file | ||
| rpc TransferFile(FileTransferRequest) returns (stream FileTransferProgress); |
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
I'm short on time today, I reviewed as much as I could. I didn't have the time to answer your questions, I look into that and the rest of the code tomorrow.
| clusterServers, err := proxyClient.FindNodesByFilters(server.Context(), proto.ListResourcesRequest{ | ||
| Namespace: defaults.Namespace, | ||
| }) | ||
|
|
||
| if err != nil { | ||
| return trace.Wrap(err) | ||
| } | ||
|
|
||
| var foundServer types.Server | ||
| for _, clusterServer := range clusterServers { | ||
| if clusterServer.GetName() == request.GetServerId() { | ||
| foundServer = clusterServer | ||
| break | ||
| } | ||
| } | ||
|
|
||
| if foundServer == nil { | ||
| return trace.BadParameter("Requested server does not exist") | ||
| } | ||
|
|
||
| err = c.clusterClient.TransferFiles(server.Context(), request.GetLogin(), foundServer.GetHostname()+":0", config) |
There was a problem hiding this comment.
I'll try to look into it tomorrow, I probably won't have time for that today unfortunately.
| "time" | ||
| ) | ||
|
|
||
| func (c *Cluster) TransferFile(request *api.FileTransferRequest, server api.TerminalService_TransferFileServer) error { |
There was a problem hiding this comment.
See what Alan had to say about passing streams around. Though I suppose at this point it might be too late to introduce significant refactors here but I also haven't given it too much thought.
There was a problem hiding this comment.
I agree, there is no need to pass the entire server, I replaced it with sendProgress function. I was wondering if this function shouldn't be called send, but perhaps longer name gives more context?
There was a problem hiding this comment.
I was wondering if this function shouldn't be called
send, but perhaps longer name gives more context?
Yeah, sendProgress sounds much better imho.
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
I need to look more closely at GrpcFileTransferProgress tomorrow but that's about what's left for me to review in this PR.
ravicious
left a comment
ravicious
left a comment
There was a problem hiding this comment.
Confirmed that upload/download works correctly after refreshing certs.
I left some minor comments but the rest looks fine.
…o `shouldSendProgress`
| } | ||
|
|
||
| type fileTransferProgress struct { | ||
| sendProgress func(progress *api.FileTransferProgress) error |
There was a problem hiding this comment.
Nit: since it's used multiple times here, I think you should create a type for this function prototype. Maybe progressSender?
There was a problem hiding this comment.
Good idea, I called it FileTransferProgressSender, because it is also used in daemon.go
| } | ||
| } | ||
|
|
||
| message FileTransferRequest { |
There was a problem hiding this comment.
It feels a little confusing to have source, destination and direction, and it isn't immediately obvious how to use this without looking at the implementation. Is there any comments we could add to this message or the file transfer direction enums to clarify the behaviour here ?
| var configErr error | ||
|
|
||
| direction := request.GetDirection() | ||
| if direction == api.FileTransferDirection_FILE_TRANSFER_DIRECTION_UNSPECIFIED { |
There was a problem hiding this comment.
It's worth being more careful with gRPC enums. If for some reason, a newer client was connected to this service, and that client sent a new direction not yet implemented in the server, this would result in a nil config. Whilst this is unlikely here since the server and the client are pretty tightly bound here, I think it's a good habit to have. Switching to a case statement with a default is probably safer.
There was a problem hiding this comment.
Good idea, switched to switch/case statement and moved to a new function.
strideynet
left a comment
strideynet
left a comment
There was a problem hiding this comment.
Approved assuming select/case statement used for gRPC enum.
5 participants
webapps counterpart: gravitational/webapps#1225
This PR adds new
TransferFileserver-side stream totshdto run SFTP file transfer.The
TransferFilestream itself does not physically transfer files, but delegates this task tosftpmodule. In the request we only need to specify the params like: source, destination, login, hostname. We don't support recursive upload/download. Server-side streaming is used to report the transfer progress to the UI.To allow sending the progress via gRPC, I changed
config.ProgressWriterto be a function that is called with the currently transferred file in the argument. Thanks to this,tsh scpand Connect can specify their own progress writers.