Created image scp feature #10828

cdoern · 2021-06-30T17:22:09Z

Created image scp feature for secure image copy

added functionality for image secure copying from local to remote.

Added a new cmd/podman scp.go file which utilizes Save() and Load()
made necessary tests
edited man pages and documentation

docs/source/markdown/links/podman-image-scp.1

docs/source/markdown/podman-image.1.md

rhatdan · 2021-06-30T21:19:23Z

Podman push can already do almost all of the functionality you have. What podman image scp Is supposed to do is allow me to copy images between a to ssh connected hosts, as is my understanding.

podman image scp alpine REMOTESERVER:
podman image scp 10.20.3.15:alpine dir:/tmp/alpine
podman image scp fedora.registry.com:fedora server.dev.redhat.com:

cdoern · 2021-06-30T21:27:33Z

Okay @rhatdan I wasn't sure of the proper functionality. @jwhonce mentioned how Save() and Load() cannot be used in conjunction with ssh so I am unsure how we would get around that.

Edit: I think I see what you mean and am implementing an abi save() and then an ssh call to load the saved image

rhatdan · 2021-07-01T09:05:11Z

Yes, you would do the equivalent of

podman save image /tmp/TMPFILE
scp cp /tmp/TMPFILE HOST:/tmp
podman remote --connect HOST: load /tmp/TMPFILE

vrothberg · 2021-07-01T09:20:25Z

Yes, you would do the equivalent of

podman save image /tmp/TMPFILE
scp cp /tmp/TMPFILE HOST:/tmp
podman remote --connect HOST: load /tmp/TMPFILE

That's not entirely correct. podman-remote does not load any path on the remote machine. With podman-remote load -i fedora.tar "fedora.tar" is at the client but will be uploaded to the server.

So I think that image scp boils down to:
podman save -o /tmp/archive.tar $image
podman-remote load -i /tmp/archive.tar

rhatdan · 2021-07-01T09:45:41Z

Correct. podman -load would be doing the ssh stuff under the covers.
I guess it is best to only handle loading into container/storage and then force the user to do podman --remote if they want to get the image into other storage transports on the remote machine.

rhatdan · 2021-07-01T09:47:55Z

Actually the code will end up doing the equivalent of

podman save $image | podman-remote load

vrothberg · 2021-07-01T09:48:22Z

I guess it is best to only handle loading into container/storage and then force the user to do podman --remote if they want to get the image into other storage transports on the remote machine.

👍 limiting to storage SGTM

cmd/podman/images/scp.go

cdoern · 2021-07-01T12:29:59Z

@vrothberg @rhatdan so is the load I am doing in conjunction with --remote not the proper functionality?

I also had an idea if we really wanted to do a proper ssh I could mimic some of the connection behavior in add.go validating that the connection the user gave is one that exists in connection list and then execute an ssh session.

rhatdan · 2021-07-01T17:11:54Z

I have not looked at the underlying code, I am just trying to come to an understanding of the functionality. If you have podman-remote load code in here, then it SGTM.

cdoern · 2021-07-01T17:20:45Z

@rhatdan figured out a pretty interesting way to provide both --remote support and native ssh.

edsantiago · 2021-07-29T21:12:17Z

Just to weigh in with the experience of an old, old programmer: bugs deferred "for later" are never fixed. You move on to other things; review comments are misplaced and forgotten. It is painful to get everything just-right at the beginning, but it is exponentially more painful to find and fix them afterward.

@cdoern your work on this and your responsiveness have been outstanding. @mtrmac you know I always admire your review skills, in this case it's amazing: you're catching subtle but serious issues, and doing so despite constant churn. I don't know how you do it. If I may humbly offer some advice to both of you, it would be: take your time. Wait for each other to finish writing/resolving comments, then wait a little longer, even a day maybe, and then reiterate. That will give time for each issue to get digested. There is no hurry on this. Thanks to both of you for your hard work.

cdoern · 2021-07-29T21:35:17Z

Ok I agree with what both of you are saying @edsantiago @mtrmac lets hold off on merging until we get it right!!! I am confident we are getting close. I just implemented the shared execRemoteCommand function and eliminated all argument parsing errors. Thank you both for your input, this has really taught me ALOT about podman and development in general

cmd/podman/images/scp.go

cmd/podman/system/connection/add.go

cmd/podman/system/connection/shared.go

cmd/podman/images/scp.go

mtrmac

GitHub doesn’t like long PR discussions…

I have copied outstanding comments from the previous partial reviews here, so that my reviews before #10828 (review) can be ignored. Hopefully that’s helpful.

Podman maintainers, two outstanding functional issues:

The use of ParseNormalizedNamed probably interferes with short-name lookup
Copy from a remote server leaves around a large temporary file

The rest is style/corner cases.

cmd/podman/images/scp.go

mtrmac · 2021-07-30T16:39:21Z

cmd/podman/images/scp.go

+					cliConnections = append(cliConnections, args[0])
+					cliConnections = append(cliConnections, args[1])
+				} else if len(strings.Split(args[0], "::")[1]) > 1 && len(strings.Split(args[1], "::")[0]) > 1 {
+					return nil, errors.Wrapf(define.ErrInvalidArg, "cannot specify an image rename")
+				} else { // else its a load save (order of args)
+					cliConnections = append(cliConnections, args[1])
+					cliConnections = append(cliConnections, args[0])


I don’t see how the switching of order of arguments works — later the createConnection function assumes that the index in .Connections depends only on direction.

this is so fed::alpine fed:: and fed:: fed::alpine both mean the same thing. cliConnections/scpOpts.Connections always has the first entry be the from location and the second be the to location

In that case of copying from/to the same host, sure, that works, but for src::alpine dest:: and src:: dest::alpine this breaks, AFAICS, switching src and dest, exactly because “the first entry is the from location”.

I don't see how the example using different hosts makes a difference. the ordering paradigm of first entry in array = from location and second entry in array = to location still holds

I might have misunderstood the design, see elsewhere. Anyway, assuming my previous interpretation:

image scp a:: b::alpine:

My expectation of what this means: load from connection a, save to connection b.

I think the code does: The // is save->load w/ one image name or save-load w/ two image names code path, sets cliConnections to ["a::alpine", "b::"], i.e. scpOpts.URI is set to [aConnection, bConnection], i.e. from=a, to=b

i.e. works as I thought it is designed to

image scp a::alpine b:::

My expectation of what this means: load from connection a, save to connection b.

I think the code does: The // else its a load save (order of args) code path, sets cliConnections to ["b::" ,"a::alpine"], i.e. scpOpts.URI is set to [bConnection, aConnection], i.e. from=b, to=a

i.e. works contrary to my expectations

cmd/podman/images/scp.go

mtrmac · 2021-07-30T16:44:47Z

cmd/podman/images/scp.go

+		if strings.Contains(args[0], "::") {
+			if !(strings.Contains(args[1], "::")) && len(strings.Split(args[0], "::")[1]) <= 1 { // if an image is specified, this mean we are loading from our client
+				cliConnections = append(cliConnections, args[0])
+				scpOpts.ToRemote = true


Isn’t this image scp foo:: bar? Why is that ToRemote? What sets SourceImageName?

similar to this this ensures that both image scp foo:: bar is the same as image scp bar foo:: order of arguments shouldn't make a difference if they are valid

added a setting for SourceImageName I had never tested that path.

similar to this this ensures that both image scp foo:: bar is the same as image scp bar foo:: order of arguments shouldn't make a difference if they are valid

I don’t understand. How can the order of arguments not make a difference? One of the two means “copy remote file to local”, and the other to “copy local to remote”, doesn’t it? At least that I would expect based on the more generic case image scp host1::image1 host2::image2.

Or is this designed so that image scp foo::bar is remote→local, and a two-argument version is local→remote? If so, what will be the future syntax for remote→local while saving under a different name?

(The man page does not explain the meaning of the syntax either; it just shows the syntax.)

added a setting for SourceImageName I had never tested that path.

(That’s ultimately what unit tests are for, but that might truly be a later PR.)

mtrmac · 2021-07-30T16:47:11Z

cmd/podman/images/scp.go

+				scpOpts.ToRemote = true
+				if len(strings.Split(args[0], "::")[1]) <= 1 { // is save->load w/ one image name or save-load w/ two image names
+					cliConnections = append(cliConnections, args[0])
+					cliConnections = append(cliConnections, args[1])


Does anything guarantee at least one image name on this path? We only know that args[0] is …::, nothing about args[1].

validated below in the cliConnections loop, this part just gets the connections in the right place

The loop only validates image name if it is not empty; it does nothing in the image scp host1:: host2:: case AFAICS.

pkg/domain/entities/images.go

mtrmac · 2021-07-30T17:05:49Z

Podman maintainers, two outstanding functional issues:

Both resolved now. There might be some argument parsing issues but those would quickly show up in practical usage.

cdoern · 2021-07-30T18:03:55Z

Ok @mtrmac thank you again for all of the semantic and structural help. I really needed some practice with code elegance.

cmd/podman/images/scp.go

mtrmac · 2021-07-30T20:14:19Z

cmd/podman/images/scp.go

+		if strings.Contains(args[0], "::") {
+			if !(strings.Contains(args[1], "::")) && len(strings.Split(args[0], "::")[1]) <= 1 { // if an image is specified, this mean we are loading from our client
+				cliConnections = append(cliConnections, args[0])
+				scpOpts.ToRemote = true


similar to this this ensures that both image scp foo:: bar is the same as image scp bar foo:: order of arguments shouldn't make a difference if they are valid

I don’t understand. How can the order of arguments not make a difference? One of the two means “copy remote file to local”, and the other to “copy local to remote”, doesn’t it? At least that I would expect based on the more generic case image scp host1::image1 host2::image2.

Or is this designed so that image scp foo::bar is remote→local, and a two-argument version is local→remote? If so, what will be the future syntax for remote→local while saving under a different name?

(The man page does not explain the meaning of the syntax either; it just shows the syntax.)

mtrmac · 2021-07-30T20:17:06Z

cmd/podman/images/scp.go

+				scpOpts.ToRemote = true
+				if len(strings.Split(args[0], "::")[1]) <= 1 { // is save->load w/ one image name or save-load w/ two image names
+					cliConnections = append(cliConnections, args[0])
+					cliConnections = append(cliConnections, args[1])


The loop only validates image name if it is not empty; it does nothing in the image scp host1:: host2:: case AFAICS.

cmd/podman/images/scp.go

mtrmac · 2021-07-30T20:22:36Z

cmd/podman/images/scp.go

+		if strings.Contains(args[0], "::") {
+			if !(strings.Contains(args[1], "::")) && len(strings.Split(args[0], "::")[1]) <= 1 { // if an image is specified, this mean we are loading from our client
+				cliConnections = append(cliConnections, args[0])
+				scpOpts.ToRemote = true


added a setting for SourceImageName I had never tested that path.

(That’s ultimately what unit tests are for, but that might truly be a later PR.)

mtrmac · 2021-07-30T20:35:52Z

@cdoern Thanks for all your work, it’s been an enormous amount.

At this point the code LGTM, though I realize that we might not actually be agreeing on what the CLI design is :) I’ll happily leave that part to others.

Note to self/others: outstanding:

Created image scp feature #10828 (comment)
Created image scp feature #10828 (comment)
and possibly things since Created image scp feature #10828 (review)

added functionality for image secure copying from local to remote. Also moved system connection add code around a bit so functions within that file can be used by scp. Signed-off-by: cdoern <[email protected]>

cdoern · 2021-07-30T21:32:32Z

Just made small doc changes and the suggested sematic changes above. Looking forward to showing this off at the community meeting 😀

cdoern · 2021-08-02T15:25:14Z

@rhatdan @TomSweeneyRedHat PTAL

rhatdan · 2021-08-02T17:15:23Z

/lgtm

aardbol · 2021-10-08T15:08:20Z

using scp has been discouraged since openssh 8.0 and should be replaced with sftp or rsync. See https://www.openssh.com/txt/release-8.0

Although recent 8.8 update mentions, so expect potential breaking changes:

A near-future release of OpenSSH will switch scp(1) from using the
legacy scp/rcp protocol to using SFTP by default.

Legacy scp/rcp performs wildcard expansion of remote filenames (e.g.
"scp host:* .") through the remote shell. This has the side effect of
requiring double quoting of shell meta-characters in file names
included on scp(1) command-lines, otherwise they could be interpreted
as shell commands on the remote side.

This creates one area of potential incompatibility: scp(1) when using
the SFTP protocol no longer requires this finicky and brittle quoting,
and attempts to use it may cause transfers to fail. We consider the
removal of the need for double-quoting shell characters in file names
to be a benefit and do not intend to introduce bug- compatibility for
legacy scp/rcp in scp(1) when using the SFTP protocol.

Another area of potential incompatibility relates to the use of remote
paths relative to other user's home directories, for example -
"scp host:~user/file /tmp". The SFTP protocol has no native way to
expand a ~user path. However, sftp-server(8) in OpenSSH 8.7 and later
support a protocol extension "[email protected]" to support

rhatdan · 2021-10-08T17:09:10Z

I don't believe we are actually using scp to copy the files between containers/storage. We are using the ssh protocol to copy them though and we called the command scp, to make it clearer what is happening, Calling this sftp or rsync would not have made that clear. But if you would like to rename it to some other command, I am all ears.

cdoern added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 30, 2021

cdoern force-pushed the scp branch 13 times, most recently from 99ca323 to 9c02a95 Compare June 30, 2021 20:43

rhatdan reviewed Jun 30, 2021

View reviewed changes

docs/source/markdown/links/podman-image-scp.1 Outdated Show resolved Hide resolved

rhatdan reviewed Jun 30, 2021

View reviewed changes

docs/source/markdown/podman-image.1.md Outdated Show resolved Hide resolved

vrothberg reviewed Jul 1, 2021

View reviewed changes

cmd/podman/images/scp.go Show resolved Hide resolved

cdoern force-pushed the scp branch from 9c02a95 to 6cab41b Compare July 1, 2021 17:18

cdoern force-pushed the scp branch 2 times, most recently from 34f3237 to 2b5c08e Compare July 1, 2021 18:09

cdoern force-pushed the scp branch from 59f8880 to 114d8d5 Compare July 29, 2021 21:33

cdoern force-pushed the scp branch 3 times, most recently from 4f523dd to 174fcd7 Compare July 30, 2021 03:39

mtrmac reviewed Jul 30, 2021

View reviewed changes

cdoern force-pushed the scp branch 3 times, most recently from 0934f78 to dc07f4f Compare July 30, 2021 15:47

mtrmac reviewed Jul 30, 2021

View reviewed changes

cmd/podman/images/scp.go Outdated Show resolved Hide resolved

cmd/podman/images/scp.go Outdated Show resolved Hide resolved

cmd/podman/images/scp.go Outdated Show resolved Hide resolved

cmd/podman/images/scp.go Outdated Show resolved Hide resolved

mtrmac reviewed Jul 30, 2021

View reviewed changes

cdoern force-pushed the scp branch from dc07f4f to 2984f2a Compare July 30, 2021 18:02

mtrmac reviewed Jul 30, 2021

View reviewed changes

Created scp.go image_scp_test.go and podman-image-scp.1.md

1d10ca7

added functionality for image secure copying from local to remote. Also moved system connection add code around a bit so functions within that file can be used by scp. Signed-off-by: cdoern <[email protected]>

cdoern force-pushed the scp branch from 2984f2a to 1d10ca7 Compare July 30, 2021 21:19

openshift-ci bot assigned rhatdan Aug 2, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2021

openshift-ci bot merged commit bdbc210 into containers:main Aug 2, 2021

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 22, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023

Created image scp feature #10828

Created image scp feature #10828

Conversation

cdoern commented Jun 30, 2021

rhatdan commented Jun 30, 2021

cdoern commented Jun 30, 2021 • edited Loading

rhatdan commented Jul 1, 2021

vrothberg commented Jul 1, 2021 • edited Loading

rhatdan commented Jul 1, 2021

rhatdan commented Jul 1, 2021

vrothberg commented Jul 1, 2021

cdoern commented Jul 1, 2021

rhatdan commented Jul 1, 2021

cdoern commented Jul 1, 2021

edsantiago commented Jul 29, 2021

cdoern commented Jul 29, 2021

mtrmac 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

cdoern Jul 30, 2021 • edited Loading

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

mtrmac commented Jul 30, 2021

cdoern commented Jul 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Jul 30, 2021

cdoern commented Jul 30, 2021

cdoern commented Aug 2, 2021

rhatdan commented Aug 2, 2021

aardbol commented Oct 8, 2021 • edited Loading

rhatdan commented Oct 8, 2021

cdoern commented Jun 30, 2021 •

edited

Loading

vrothberg commented Jul 1, 2021 •

edited

Loading

cdoern Jul 30, 2021 •

edited

Loading

aardbol commented Oct 8, 2021 •

edited

Loading