Add file transfer to Connect by gzdunek · Pull Request #1225 · gravitational/webapps

gzdunek · 2022-09-30T12:03:42Z

Closes https://github.com/gravitational/webapps.e/issues/179
This PR refactors FileTransfer so it can be used in both WebUI and Connect.

The biggest change from the previous version is that FileTransfer no longer makes HTTP requests on its own. Instead, it allows you to provide your own implementations for upload/download functions.
There's also a refreshed UI and, as we discussed at the Design Meeting, there's no longer an Upload button. Uploading starts as soon as you select files.
Additionally, FileTransfer has been moved from teleport to the shared package.

How Web UI works

Upload/download in the WebUI works as before, they use HTTP (XHR) to transfer the files. When a user downloads a file, it is stored in the browser's memory, and when it is finished, the browser's download is triggered.

How Connect works

The upload/download for Connect is quite different. It doesn't physically move the files, but delegates this task to tshd (SFTP). We just specify the source/destination paths via gRPC server-side stream, and all the dirty work is done for us :) Server-side streaming is used to send the transfer progress back to the UI. To specify the destination path to download the file, we use the native OS save dialog.

Info for the reviewers

Review commit-by-commit.

At first I didn't plan to do a refactor, but then I realized that the changes would be significant anyway, so this is a great opportunity to refactor the whole module to modern React, add full TS support and simplify the final usage. I also added tests and better stories.

Although the PR added/updated/removed many files (sorry 😞), I tried to keep the logic in one place. The main logic of FileTransfer is in useFilesStore.tsx and FileTransfer.tsx.
Similarly DocumentSsh, DocumentTerminal, getHttpFileTransferHandlers (the XHR orchestration here is 1:1 to the previous Transfer service) are the places where most of the callers work is done.

ravicious

I'll take a look at the rest of the UI code and the backend code tomorrow.

ravicious

The keyboard navigation works fine now, thanks for fixing this.

ravicious · 2022-10-04T13:12:06Z

I didn't get around to testing if the Web UI file transfer still works correctly but I did test it for Connect.

Also, did you compare performance of file transfer through Connect vs Web UI and tsh? I won't be able to do that today but it'd be good to make sure we're not sluggish compared to them for some reason.

ravicious

As I mentioned in DMs, I somehow totally forgot to take a look at the non-UI code. I might have more followups soon, right now I'm looking at the problem with server ID on the backend side of this.

ravicious · 2022-10-05T09:58:05Z

+      abortSignal.addEventListener(() => call.cancel());
+
+      call.on('data', (data: api.FileTransferProgress) =>
+        listeners.onProgress(data.getPercentage())
+      );
+      call.on('end', () => {
+        listeners.onComplete();
+        call.removeAllListeners();
+      });
+      call.on('error', error => {
+        listeners.onError(error);
+        call.removeAllListeners();
+      });


I wonder if perhaps a better idea would be to make the tshd client return just the call object and then contain the rest of the logic within FileTransferClient.

Right now all FileTransferClient does is just pass some stuff further down – at this point one might wonder why we even bother creating this class at all.

See how I organized the stream stuff in my initial draft PR for tshd-initiated communication. I'm talking about the files in services/tshd. Though here you probably don't want to call FileTransferClient from within tshd client, but rather as I suggested, tsdh client would return the call object and then FileTransferClient could operate on it.

File transfer doesn't need to follow it 1:1. I think the files you created so far are alright it's just a matter of moving the logic around a bit.

Unfortunately, context bridge doesn't allow us to return call object, but I followed the structure from linked PR, so now we have a function createFileTransferStream that is returned from tshd client.

Also, as discussed in DM's, we decided not to pass FileTransferListeners as an argument, but instead we return it from the tshd/http calls. It makes the code easier to understand, especially getDownloader/getUploader parts (less arguments, one callback instead of a callback returning another callback).
I also added createFileTransferEventsEmitter utility function that makes working with the events/listeners more convenient.

…as an argument

ravicious

Getting close to ready here.

ravicious · 2022-10-06T10:18:10Z

+  constructor(private tshClient: TshClient) {}
+
+  transferFile(
+    options: FileTransferRequest.AsObject,


From what I've seen, we typically "reexport" those AsObject types:

webapps/packages/teleterm/src/services/tshd/types.ts

Lines 10 to 13 in 10e90a7

export type Application = apiApp.App.AsObject;

export type Kube = apiKube.Kube.AsObject;

export type Server = apiServer.Server.AsObject;

export type Gateway = apigateway.Gateway.AsObject;

Could you do the same for those few places where you use FileTransferRequest.AsObject?

ravicious · 2022-10-06T10:27:53Z

-): void {
+): FileTransferListeners {
  const server = appContext.clustersService.getServer(file.serverUri);
+  const eventsEmitter = createFileTransferEventsEmitter();


Why do we need that eventsEmitter here?

appContext.fileTransferClient.transferFile already returns an object that has all the properties that FileTransferListeners requires so unless I'm missing something, it seems that we can pass whatever we get from transferFile directly.

Once that event emitter is removed, we can remove createFileTransferEventsEmitter altogether by bringing back the inline usage of event emitter in httpFileTransferHandlers.

Yes, appContext.fileTransferClient.transferFile already returns an object that we can use, but I need the event emitter to create a layer between tshClient and FileTransfer. Events returned from the tshClient are used inside useTshFileTransferHandlers and FileTransfer uses these returned from useTshFileTransferHandlers.
It is needed, because the errors needs to go through retryWithRelogin which decides what to do with them first.
For example, when there is ssh: cert has expired error, it should show to the user only when he reject to log in.

I wanted to avoid this, but I don't see any other way not to create an event emitter.

ravicious · 2022-10-06T10:29:49Z

    };
-
-    return this.tshClient.transferFile(options, listeners, abortSignal);
+    return this.tshClient.transferFile(options, abortSignal);


It looks like FileTransferClient ended up with very few responsibilities after all. Still, I think it's better to encapsulate calls to tshClient (which has big surface area) within small services like that (which have very small surface area) rather than passing the big tshClient throughout the app.

Totally agree, we should avoid using tshClient directly.

ravicious · 2022-10-06T10:34:34Z

+import { TshClient } from 'teleterm/services/tshd/types';
+import { FileTransferRequest } from 'teleterm/services/tshd/v1/service_pb';
+
+export class FileTransferClient {


This could probably be named FileTransferService rather than Client, no? It'd fit better with the rest of the classes.

Yes, the name comes from the first iteration and doesn't have much sense now, changed.

(cherry picked from commit f614b7f)

gzdunek added 6 commits September 30, 2022 12:36

Refactor FileTransfer

ea2bfd7

Do not show hover on disabled ButtonIcon

672f18b

Use new FileTransfer in Web UI

af3d61a

Use new FileTransfer in Connect

4c60cae

Update protobuf files

09c9298

Remove old FileTransfer

ee465da

gzdunek requested review from avatus and ravicious September 30, 2022 12:03

gzdunek mentioned this pull request Sep 30, 2022

Add file transfer support to Connect gravitational/teleport#16880

Merged

JanKaczmarkiewicz reviewed Oct 3, 2022

View reviewed changes

Comment thread packages/shared/components/FileTransfer/FileTransferStateless/UploadForm/UploadForm.tsx Outdated

Comment thread packages/shared/components/FileTransfer/useFilesStore.ts Outdated

gzdunek added 3 commits October 3, 2022 16:21

Replace extractFilesFromFileList with Array.from

c3cde59

Fix files deps

c5b348b

Merge branch 'master' into gzdunek/refactor-file-transfer

aa8afe8

ravicious reviewed Oct 3, 2022

View reviewed changes

gzdunek added 4 commits October 4, 2022 13:12

Use div for Dropzone element

6b491cd

Simplify condition

daceb3d

Remove unnecessary deps, use unique for id

160881e

Change test names

e3b4d7c

JanKaczmarkiewicz reviewed Oct 4, 2022

View reviewed changes

Comment thread packages/shared/components/FileTransfer/FileTransfer.test.tsx Outdated

Use waitForElementToBeRemoved

f4cb53b

JanKaczmarkiewicz approved these changes Oct 4, 2022

View reviewed changes

ravicious approved these changes Oct 4, 2022

View reviewed changes

Comment thread packages/shared/components/FileTransfer/useFilesStore.ts Outdated

Comment thread packages/design/src/ButtonIcon/ButtonIcon.jsx Outdated

gzdunek added 5 commits October 5, 2022 11:45

Add comment to TransferHandlers, update tests names

d1a9ad4

Revert "use unique for id"

9d6a06c

Change ButtonIcon disabled state styles

3a358bd

Add retryWithRelogin download/upload

40b5beb

Merge branch 'master' into gzdunek/refactor-file-transfer

89e6e1f

ravicious reviewed Oct 5, 2022

View reviewed changes

Send hostname instead of serverUri

ee91444

gzdunek added 2 commits October 5, 2022 15:23

Update protobuf files

4c0fca9

Return FileTransferListeners from handlers instead of passing them …

c136931

…as an argument

ravicious self-requested a review October 6, 2022 10:35

ravicious reviewed Oct 6, 2022

View reviewed changes

gzdunek added 2 commits October 6, 2022 13:27

Get rid of FileTransferRequest.AsObject

66e58aa

Rename FileTransferClient to FileTransferService

c7449fd

gzdunek mentioned this pull request Oct 6, 2022

Teleport 11 Web UI and Connect Test Plan gravitational/teleport#16952

Closed

gzdunek added 3 commits October 10, 2022 17:27

Merge branch 'master' into gzdunek/refactor-file-transfer

106d5cc

Remove inputCss

a9cba6f

Update snapshots with fixed ButtonIcon hover and focus styles

884c4a6

gzdunek merged commit f614b7f into master Oct 10, 2022

gzdunek deleted the gzdunek/refactor-file-transfer branch October 10, 2022 16:38

gzdunek added a commit that referenced this pull request Oct 10, 2022

Add file transfer to Connect (#1225)

a1d30f6

(cherry picked from commit f614b7f)

gzdunek mentioned this pull request Oct 10, 2022

[v11] Add file transfer to Connect (#1225) #1244

Merged

gzdunek added a commit that referenced this pull request Oct 10, 2022

[v11] Add file transfer to Connect (#1225) (#1244)

6a2468e

	export type Application = apiApp.App.AsObject;
	export type Kube = apiKube.Kube.AsObject;
	export type Server = apiServer.Server.AsObject;
	export type Gateway = apigateway.Gateway.AsObject;

Conversation

gzdunek commented Sep 30, 2022

How Web UI works

How Connect works

Info for the reviewers

Uh oh!

Uh oh!

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ravicious commented Oct 4, 2022

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzdunek Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gzdunek Oct 6, 2022 •

edited

Loading