http file transfer #2055

alex-kovoy · 2018-07-05T13:27:56Z

This work is still in progress but I would like you guys to take a look to provide an early feedback.

klizhentas

I think your overall design looks solid, I like it.

There are several rough edges that needs to be polished on the backend:

It's not clear to me why do you need to create a new client instead of using the method of the session
I would like you to run a check for file leaks - make sure all the resources are closed
Apply/address my general comments

klizhentas · 2018-07-06T17:07:17Z

lib/client/api.go

@@ -905,6 +906,59 @@ func (tc *TeleportClient) Play(ctx context.Context, namespace, sessionId string)
 	return trace.Wrap(err)
 }

+// RunSCPCommand executes scp command


Let's call it ExecuteSCP and elaborate in comments that it executes scp.Command and is used in lower-level API integrations opposed to SCP function that is used to mimic SCP CLI command behavior

klizhentas · 2018-07-06T17:07:45Z

lib/client/api.go

+	}
+	defer proxyClient.Close()
+
+	siteInfo, err := proxyClient.currentCluster()


clusterInfo, we are slowly renaming site to cluster everywhere

klizhentas · 2018-07-06T17:08:36Z

lib/client/api.go

+	}
+
+	// gets called to convert SSH error code to tc.ExitStatus
+	onError := func(err error) error {


onError is used only once, so I don't see a reason to factor this into closure, just inline the code

klizhentas · 2018-07-06T17:09:41Z

lib/client/api.go

@@ -983,7 +1052,21 @@ func (tc *TeleportClient) SCP(ctx context.Context, args []string, port int, recu
 		}
 		// copy everything except the last arg (that's destination)
 		for _, dest := range args[1:] {
-			err = client.Download(src, dest, recursive, tc.Stderr, progressWriter)
+			scpParams := scp.Config{


should be scpConfig for consistency (in similar code above, it is called scpCfg)

klizhentas · 2018-07-06T17:09:50Z

lib/client/api.go

 		// copy everything except the last arg (that's destination)
 		for _, src := range args[:len(args)-1] {
-			err = client.Upload(src, dest, recursive, tc.Stderr, progressWriter)
+			scpCfg := scp.Config{


let's call it scpConfig for consistency

klizhentas · 2018-07-06T18:01:22Z

lib/web/apiserver.go

@@ -181,6 +181,10 @@ func NewHandler(cfg Config, opts ...HandlerOption) (*RewritingHandler, error) {
 	h.GET("/webapi/sites/:site/namespaces/:namespace/sessions/:sid/events", h.WithClusterAuth(h.siteSessionEventsGet)) // get recorded session's timing information (from events)
 	h.GET("/webapi/sites/:site/namespaces/:namespace/sessions/:sid/stream", h.siteSessionStreamGet)                    // get recorded session's bytes (from events)

+	// scp file transfer
+	h.GET("/webapi/sites/:site/namespaces/:namespace/nodes/:node/:login/scp/*filepath", h.WithClusterAuth(h.downloadFile))


it feels like login should be really a query parameter, not part of the path, will be more REST-y this way

also I would stick to sid notation as in the function above for consistency instead of node

klizhentas · 2018-07-06T18:01:28Z

lib/web/apiserver.go

@@ -181,6 +181,10 @@ func NewHandler(cfg Config, opts ...HandlerOption) (*RewritingHandler, error) {
 	h.GET("/webapi/sites/:site/namespaces/:namespace/sessions/:sid/events", h.WithClusterAuth(h.siteSessionEventsGet)) // get recorded session's timing information (from events)
 	h.GET("/webapi/sites/:site/namespaces/:namespace/sessions/:sid/stream", h.siteSessionStreamGet)                    // get recorded session's bytes (from events)

+	// scp file transfer
+	h.GET("/webapi/sites/:site/namespaces/:namespace/nodes/:node/:login/scp/*filepath", h.WithClusterAuth(h.downloadFile))
+	h.POST("/webapi/sites/:site/namespaces/:namespace/nodes/:node/:login/scp/*filepath", h.WithClusterAuth(h.uploadFile))


klizhentas · 2018-07-06T18:05:01Z

lib/web/files.go

+
+func (h *Handler) downloadFile(w http.ResponseWriter, r *http.Request, p httprouter.Params, ctx *SessionContext, site reversetunnel.RemoteSite) (interface{}, error) {
+	req := fileTransferRequest{
+		Login:          p.ByName("login"),


the structure is not exported, so it does make sense to use lower case for struct member fields as well

klizhentas · 2018-07-06T18:05:59Z

lib/web/files.go

+		Username:      f.ctx.user,
+		ProxyHostPort: f.proxyHostPort,
+		// TODO: replace os io streams with custom streams
+		Stdout:           os.Stdout,


this should be done before the release for sure

klizhentas · 2018-07-06T18:06:30Z

lib/web/files.go

+	return nil
+}
+
+func (f *fileTransfer) createClient(req fileTransferRequest) (*client.TeleportClient, error) {


wait, why not use the client from the context? there is a function there that does pretty much the same

This behavior is similar to our web handlers for handling ssh connections. This method returns TeleportClient which then we use to connect to a node to run SCP command.

k, can you then try to factor it into the common utility function to reduce boilerplate?

i looked at 2 places where we create this config and moved initialization of common parameters to a separate method makeTeleportClientConfig

russjones · 2018-07-11T23:03:24Z

lib/sshutils/scp/scp.go

+// to teleport can pretend it launches real scp behind the scenes
+type command struct {
+	Config
+	*log.Entry


Can you call this variable log? Embedded *log.Entry like this pollutes autocomplete for me.

russjones · 2018-07-11T23:23:47Z

A general comment, you have a lot of Create* commands.

CreateCommand
CreateDownloadCommand
CreateUploadCommand
CreateHTTPUpload
CreateHTTPDownload

What do you think about consolidating them? Maybe something like the following:

cmd, err := scp.New(&scp.Config{
   Type: scp.HTTP | scp.File,
   Transfer: scp.Upload | scp.Download,
   ...
})
if err != nil {
   return trace.Wrap(err)
}

That way all the logic for how to construct the SCP command can be pushed down to the scp package.

alex-kovoy · 2018-07-12T14:02:56Z

Since there are many flags and parameters, I figured it would be simpler for a caller if some of it can be encapsulated by specific commands so there is no need to become familiar with every aspect of SCP to create, for example, a HTTPDownload command. CreateHTTPUpload accepts only HTTP specific parameters and takes care of the rest.

All this factory methods are inside scp package.

klizhentas · 2018-07-13T00:15:51Z

@alex-kovoy please backport this to branch/2.7 as well

alex-kovoy requested review from klizhentas and russjones July 5, 2018 13:28

alex-kovoy force-pushed the alexey/files branch 2 times, most recently from e0c5544 to 3b504e3 Compare July 5, 2018 13:39

klizhentas requested changes Jul 6, 2018

View reviewed changes

alex-kovoy force-pushed the alexey/files branch 4 times, most recently from 2ba7a39 to fd43171 Compare July 11, 2018 21:47

russjones reviewed Jul 11, 2018

View reviewed changes

alex-kovoy force-pushed the alexey/files branch from fd43171 to 647d010 Compare July 12, 2018 14:07

http file transfer

57b2789

alex-kovoy force-pushed the alexey/files branch from 647d010 to 57b2789 Compare July 12, 2018 18:59

klizhentas approved these changes Jul 13, 2018

View reviewed changes

alex-kovoy merged commit 775edea into master Jul 13, 2018

alex-kovoy deleted the alexey/files branch July 17, 2018 22:36

russjones mentioned this pull request Mar 1, 2021

Fix scp regressions for 6.x #5729

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http file transfer #2055

http file transfer #2055

alex-kovoy commented Jul 5, 2018

klizhentas left a comment

klizhentas Jul 6, 2018

klizhentas Jul 6, 2018

klizhentas Jul 6, 2018

klizhentas Jul 6, 2018

klizhentas Jul 6, 2018

klizhentas Jul 6, 2018

klizhentas Jul 6, 2018

klizhentas Jul 6, 2018

klizhentas Jul 6, 2018

klizhentas Jul 6, 2018

klizhentas Jul 6, 2018

alex-kovoy Jul 10, 2018

klizhentas Jul 10, 2018

alex-kovoy Jul 11, 2018

russjones Jul 11, 2018

alex-kovoy Jul 12, 2018

russjones commented Jul 11, 2018 •

edited

Loading

alex-kovoy commented Jul 12, 2018

klizhentas commented Jul 13, 2018

http file transfer #2055

http file transfer #2055

Conversation

alex-kovoy commented Jul 5, 2018

klizhentas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

russjones commented Jul 11, 2018 • edited Loading

alex-kovoy commented Jul 12, 2018

klizhentas commented Jul 13, 2018

russjones commented Jul 11, 2018 •

edited

Loading