Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests for podman image scp (the sudo form) #12797

Merged
merged 2 commits into from
Jan 18, 2022

Conversation

edsantiago
Copy link
Member

Start inching our way back to having tests for the sudo form
of podman image scp. Basically, copy an image to another user
and then back, using a pseudorandom name. Confirm that the
image makes it to the remote end, and that when we copy it
back, the original image digest is preserved.

When scp'ing as root, we identify the destination rootless
user account via the $PODMAN_ROOTLESS_USER envariable. Setting
this and creating the account is left as an exercise for the
CI framework (be it github, or Fedora/CentOS/RHEL gating, or
other).

Also: amend hack/bats to set and relay $PODMAN_ROOTLESS_USER,
so developers can test locally.

Also: remove what I'm 99% sure is a debugging printf.

Signed-off-by: Ed Santiago [email protected]

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 10, 2022
@edsantiago
Copy link
Member Author

Root-to-rootless is currently untested. It's much more complicated than I had expected, because our Cirrus CI only creates the rootless account on-demand, just before using it to run rootless tests. We need to amend

case "$PRIV_NAME" in
root) ;;
rootless)
# load kernel modules since the rootless user has no permission to do so
modprobe ip6_tables || :
modprobe ip6table_nat || :
# Needs to exist for setup_rootless()
ROOTLESS_USER="${ROOTLESS_USER:-some${RANDOM}dude}"
echo "ROOTLESS_USER=$ROOTLESS_USER" >> /etc/ci_environment
setup_rootless
;;
so it always runs setup_rootless(), every time, and sets $PODMAN_ROOTLESS_USER (note the PODMAN_ prefix). This is left as an exercise for tomorrow.

Root-to-rootless is also not working anyway, at least not on my laptop:

   # .../podman tag quay.io/libpod/testimage:20210610 foo.bar/nonesuch/c_tckzsgu9nu:mytag
   # .../podman image scp foo.bar/nonesuch/c_tckzsgu9nu:mytag esm@localhost::
   Copying blob f36118df491f done
   Copying config 9f9ec7f2fd done
   Writing manifest to image destination
   Storing signatures
   Error: payload does not match any of the supported image formats:
    * oci: initializing destination containers-storage:[overlay@/home/esm/.local/share/containers/storage+/run/user/14904/containers]localhost/var/tmp/podman541448797:latest: creating a temporary directory: stat /this/is/set/in/containers.conf: no such file or directory
    * oci-archive: loading index: open /var/tmp/oci445817820/index.json: no such file or directory
    * docker-archive: initializing destination containers-storage:[overlay@/home/esm/.local/share/containers/storage+/run/user/14904/containers]foo.bar/nonesuch/c_tckzsgu9nu:mytag: creating a temporary directory: stat /this/is/set/in/containers.conf: no such file or directory
    * dir: open /var/tmp/podman541448797/manifest.json: not a directory

Again, I'm done worrying for today. The problem will still be there tomorrow.

Comment on lines 127 to 128
# FIXME FIXME FIXME: -q is unimplemented
#is "$output" "" "-q silences output"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this in your capable hands, @cdoern

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

patch added below

@edsantiago
Copy link
Member Author

Rootless f34, f35, and ubuntu all ran and passed. (As opposed to getting skipped). That was the purpose of this WIP run: making sure that all rootless environments are able to sudo to root.

I haven't looked at root tests, because I know those were all skipped. Remote, too. This CI run has served its purpose. Tomorrow, we look at sudo'ing from root to rootless.

@cdoern
Copy link
Contributor

cdoern commented Jan 11, 2022

@edsantiago please apply this patch for the quiet issue, it will still print the load result as podman load does: scp.patch.txt

also we don't need to worry about remote tests as this feature is not supported on remote at the moment

if err != nil {
return err
}
fmt.Printf("SOURCE: %s\nDEST: %s\n", string(src), string(dst))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was meant as a permanent output to tell the user some concrete information about what they transferred and who they transferred to, @rhatdan would you be opposed to removing this? I think having some sort of scp specific output is a good idea.

Comment on lines 127 to 128
# FIXME FIXME FIXME: -q is unimplemented
#is "$output" "" "-q silences output"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

patch added below

@cdoern
Copy link
Contributor

cdoern commented Jan 11, 2022

Root-to-rootless is currently untested. It's much more complicated than I had expected, because our Cirrus CI only creates the rootless account on-demand, just before using it to run rootless tests. We need to amend

case "$PRIV_NAME" in
root) ;;
rootless)
# load kernel modules since the rootless user has no permission to do so
modprobe ip6_tables || :
modprobe ip6table_nat || :
# Needs to exist for setup_rootless()
ROOTLESS_USER="${ROOTLESS_USER:-some${RANDOM}dude}"
echo "ROOTLESS_USER=$ROOTLESS_USER" >> /etc/ci_environment
setup_rootless
;;

so it always runs setup_rootless(), every time, and sets $PODMAN_ROOTLESS_USER (note the PODMAN_ prefix). This is left as an exercise for tomorrow.

Root-to-rootless is also not working anyway, at least not on my laptop:

   # .../podman tag quay.io/libpod/testimage:20210610 foo.bar/nonesuch/c_tckzsgu9nu:mytag
   # .../podman image scp foo.bar/nonesuch/c_tckzsgu9nu:mytag esm@localhost::
   Copying blob f36118df491f done
   Copying config 9f9ec7f2fd done
   Writing manifest to image destination
   Storing signatures
   Error: payload does not match any of the supported image formats:
    * oci: initializing destination containers-storage:[overlay@/home/esm/.local/share/containers/storage+/run/user/14904/containers]localhost/var/tmp/podman541448797:latest: creating a temporary directory: stat /this/is/set/in/containers.conf: no such file or directory
    * oci-archive: loading index: open /var/tmp/oci445817820/index.json: no such file or directory
    * docker-archive: initializing destination containers-storage:[overlay@/home/esm/.local/share/containers/storage+/run/user/14904/containers]foo.bar/nonesuch/c_tckzsgu9nu:mytag: creating a temporary directory: stat /this/is/set/in/containers.conf: no such file or directory
    * dir: open /var/tmp/podman541448797/manifest.json: not a directory

Again, I'm done worrying for today. The problem will still be there tomorrow.

I think yours fails due to limitations of podman save only accepting certain formats. I will look into this tomorrow as well to see if there is a way I can reformat the image on the fly.

@edsantiago
Copy link
Member Author

Thanks for the -q patch. With it, output is just:

$ bin/podman image scp -q quay.io/libpod/testimage:20210610 root@localhost::
Loaded image(s): quay.io/libpod/testimage:20210610

...which I'll confess to being ambivalent about. I don't have a good sense for what -q output should look like, but I would expect something more like:

root@localhost::quay.io/libpod/testimage:20210610

...or perhaps even no output at all. I think this merits wider discussion.

As for the SOURCE/DEST, I don't see those providing any value whatsoever to anyone -- I genuinely thought they were just debugging printfs that you forgot to remove. For reference, here are command results as they stand today:

$ bin/podman image scp -q quay.io/libpod/testimage:20210610 root@localhost::
Copying blob f36118df491f done
Copying config 9f9ec7f2fd done
Writing manifest to image destination
Storing signatures
Getting image source signatures
Copying blob f36118df491f skipped: already exists
Copying config 9f9ec7f2fd done
Writing manifest to image destination
Storing signatures
Loaded image(s): quay.io/libpod/testimage:20210610
SOURCE: {
    "file": "/dev/shm/podman120188115",
    "image": "quay.io/libpod/testimage:20210610",
    "user": "esm"
}
DEST: {
    "file": "/dev/shm/podman120188115",
    "user": "root"
}

I find that uncomfortably verbose. I accept your point about offering scp-specific output, but I really don't think that's it.

@rhatdan
Copy link
Member

rhatdan commented Jan 11, 2022

Yes I think this should become less verbose.

$ scp dwalsh@localhost:/tmp/dan /tmp/walsh
dan 100% 0 0.0KB/s 00:00

So

$ podman image scp quay.io/libpod/testimage:20210610 root@localhost::
Copying blob f36118df491f done
Copying config 9f9ec7f2fd done
Writing manifest to image destination
Storing signatures
Getting image source signatures
Copying blob f36118df491f skipped: already exists
Copying config 9f9ec7f2fd done
Writing manifest to image destination
Storing signatures
quay.io/libpod/testimage:20210610    SIZE
$ podman image scp -q quay.io/libpod/testimage:20210610 root@localhost::
quay.io/libpod/testimage:20210610    SIZE

@edsantiago edsantiago force-pushed the test_image_scp_sudo branch 3 times, most recently from ddac05c to c737e52 Compare January 11, 2022 18:46
@edsantiago
Copy link
Member Author

Today's non-progress:

  • in enabling the rootless account, root's ssh authorized_hosts got set up; which then
  • made the podman system connection - ssh test decide to run (it has been skipped since inception); which then
  • failed all podman-remote tests, because podman system connection does not work.

I know. It works on Macs, maybe, or at least it's supposed to. It does not work on Linux. Not root nor rootless. I've spent most of today on an f35 VM trying every combination I can (identity file, agent, command-line permutations).

# podman system service -t 0 unix:/tmp/foo &
[1] 22873
# ssh-agent bash
# ssh-add
Identity added: /root/.ssh/id_rsa (/root/.ssh/id_rsa)
# ssh localhost date
Tue Jan 11 05:27:44 PM EST 2022
# podman-remote system connection add --socket-path /tmp/foo mycon localhost
# podman-remote --log-level=debug info
INFO[0000] podman-remote filtering at log level debug
DEBU[0000] Called info.PersistentPreRunE(podman-remote --log-level=debug info)
DEBU[0000] Found SSH_AUTH_SOCK "/tmp/ssh-XXXXXXM82z3s/agent.22915", ssh-agent signer(s) enabled
DEBU[0000] SSH Agent Key SHA256:9j1blwt3wcrRiGYZQ7ZGu9axm3cDklH6/z4c+Ee8CzE ssh-rsa
Cannot connect to Podman. Please verify your connection to the Linux system using `podman system connection list`, or try `podman machine init` and `podman machine start` to manage a new Linux VM
Error: unable to connect to Podman. failed to create sshClient: Connection to bastion host (ssh://root@localhost:22/tmp/foo) failed.: ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain

The socket is fine:

# podman-remote --url unix:/tmp/foo info
...works fine

Tomorrow is another day, and this will wait.

edsantiago added a commit to edsantiago/libpod that referenced this pull request Jan 12, 2022
Relay --quiet to save & load commands, in both Rootless
and Rootful transfer functions.

Also, a little cleanup:
- remove unuseful SOURCE/DEST printfs
- refactor duplication in execMachine()
- fix Debug("Executing") statements to include the actual
  command they're executing

[NO NEW TESTS NEEDED] : Tests are being slowly implemented in containers#12797

Signed-off-by: Charlie Doern <[email protected]>
Signed-off-by: Ed Santiago <[email protected]>
@edsantiago edsantiago force-pushed the test_image_scp_sudo branch 3 times, most recently from e5bb11b to 77d4847 Compare January 13, 2022 18:16
test/system/120-load.bats Show resolved Hide resolved
run_podman image scp -q ${notme}@localhost::$newname

expect="Loaded image(s): $newname"
# FIXME: root uses machinectl, which emits useless \n and \r (#12829)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to look into this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will pause work on this PR, and wait for a solution, because working around machinectl plus the warnings on Ubuntu is proving time-consuming.

@edsantiago edsantiago removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 17, 2022
@edsantiago
Copy link
Member Author

@containers/podman-maintainers @cdoern @cevich PTAL

I've removed the WIP label and would like to merge this ASAP even though it's complete rubbish. Reason: #12867 is in rapid churn, but it's being submitted without any testing whatsoever, and every iteration I've (manually) tested has been completely nonfunctional. I believe it is urgent that #12867 be blocked until this merges, and then rebase on this, and then fix all (ok, some) of the horrible FIXMEs in here but at least get some testing.

Start inching our way back to having tests for the sudo form
of podman image scp. Basically, copy an image to another user
and then back, using a pseudorandom name. Confirm that the
image makes it to the remote end, and that when we copy it
back, the original image digest is preserved.

When scp'ing as root, we identify the destination rootless
user account via the $PODMAN_ROOTLESS_USER envariable. Setting
this and creating the account is left as an exercise for the
CI framework (be it github, or Fedora/CentOS/RHEL gating, or
other).

Also: amend hack/bats to set and relay $PODMAN_ROOTLESS_USER,
so developers can test locally.

Also: remove what I'm 99% sure is a debugging printf.

Signed-off-by: Ed Santiago <[email protected]>
viz, rootful system tests. The rootless account will be
used by image-scp tests.

Unfortunately, having ssh available means the system-connection
tests will start running, which is very bad because they will
fail, because system connection doesn't actually work (long story).
Add a few more checks to prevent this test from running.

Signed-off-by: Ed Santiago <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Jan 18, 2022

Label still says [WIP]?

@edsantiago edsantiago changed the title [WIP] Tests for podman image scp (the sudo form) Tests for podman image scp (the sudo form) Jan 18, 2022
@edsantiago
Copy link
Member Author

Oops. Fixed, thanks.

@rhatdan
Copy link
Member

rhatdan commented Jan 18, 2022

LGTM
Will leave thisto @cdoern to commit.

@edsantiago
Copy link
Member Author

I'd like @cevich to review also, since this mucks with the Cirrus setup

Copy link
Contributor

@cdoern cdoern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a general comment about future ubuntu work. I don't have approve or merging ability I don't think.

@@ -80,26 +80,94 @@ verify_iid_and_name() {

@test "podman image scp transfer" {
skip_if_remote "only applicable under local podman"
if is_ubuntu; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ubuntu gave me some issues on my first go as well, I can circle back in a separate PR and try to fix this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, I keep getting "warning: no machinectl, trying su", which wreaks havoc with my expected-output tests. Since I have no access to Ubuntu other than in CI, it did not seem worth my time to fiddle with that. I haven't tested against your latest PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my PR no longer uses machinectl to execute the commands but does use it to log in which is an issue... I am looking into how runuser can be used instead in the rootful form (which is all that matters in this case since rootless just uses an exec.Command(podman...))

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 18, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, edsantiago

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhatdan
Copy link
Member

rhatdan commented Jan 18, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2022
@openshift-merge-robot openshift-merge-robot merged commit 0bbf8fa into containers:main Jan 18, 2022
@edsantiago edsantiago deleted the test_image_scp_sudo branch January 18, 2022 15:31
@cevich
Copy link
Member

cevich commented Jan 18, 2022

I'd like @cevich to review also, since this mucks with the Cirrus setup

So...these changes are okay. I guess my only (minor) issue is possible confusion due to "All-roads lead to setup_rootless()", so it could just be called from the top-level instead of every case statement.

@edsantiago
Copy link
Member Author

Sorry about the confusion: all roads do not lead to setup_rootless. In root, only when tests are sys. I tried calling it from the top level but that failed super early, it didn't even make it to tests. I don't remember the failure, but it was maybe in smoke or validate or one of the very early steps.

@cevich
Copy link
Member

cevich commented Jan 18, 2022

Ahh ok, well I'll defer to your actual experience then, rather than my hypothesized guesses 😀

@github-actions 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 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants