New system connection add test#24340
New system connection add test#24340openshift-merge-bot[bot] merged 3 commits intocontainers:mainfrom
system connection add test#24340Conversation
openshift-ci
Bot
added
do-not-merge/work-in-progress
l0rd
force-pushed
the
ssh-knownhosts-test
branch
6 times, most recently
from
2c43969 to
a52d965
Compare
l0rd
force-pushed
the
ssh-knownhosts-test
branch
6 times, most recently
from
361aed9 to
bbf8ffc
Compare
l0rd
force-pushed
the
ssh-knownhosts-test
branch
3 times, most recently
from
1745ea4 to
ea0f53d
Compare
|
#24447 is merged which should have your latest changes in there. So you can rebase and drop the vendor changes and only add the tests here |
l0rd
force-pushed
the
ssh-knownhosts-test
branch
from
ea0f53d to
1f10a61
Compare
Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
Version 2.3.0 of codespell allows using inline comments to ignore some spelling errors. codespell-project/codespell#2400 Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
These tests verify that podman successfully adds (or fails to add) a connection to an SSH server based on the entries in the `~/.ssh/known_hosts` file. In particular `system connection add` should succeed if: - there is no `know_hosts` file - `known_hosts` has an entry that matches the first protocol/key returned by the SSH server - `known_hosts` has an entry that matches the first protocol/key returned by the SSH server - `known_hosts` has an entry for another SSH server, not for the target server It should fail if the `known_host` file has an entry for the target server that matches the protocol but not the key. Depends on containers/common#2212 Fixes containers#23575 Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
l0rd
force-pushed
the
ssh-knownhosts-test
branch
from
1f10a61 to
b20960b
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
I have removed the vendoring commit and rebased. All tests are passing. @containers/podman-maintainers please have a look. |
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
The int tests fail locally when I start sshd, that doesn't seem to be new here so I am fine to let this slide but these tests make a lot of (incorrect?) assumption about the hosts ssh key setup.
I really do not like that tests just decide rename known_hosts as this might have other unwanted side effects when other ssh things are running in parallel to the tests and if the tests are killed you get left with the renamed file.
I really think all of these tests must be rewritten in a way to not mess with the actul users .ssh. It should be safe to run tests on any system without having to worry about them messing up your home dir.
In any case this is pre-existing so I am fine with it. I did double checked that CI is running the tests on the rootless int tests so CI should catch regressions.
LGTM
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: l0rd, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
I agree with you @Luap99 I was able to run the tests locally after adding an To avoid modifying the local known_host file we should add a new flag to configure it's path. This is something I can work on when I am back from KubeCon. |
Well I do have such a key but I have a passphrase on it so the system connection add fails with I also did not setup such a key to allow localhost ssh connection like the test wants so that would be the next issue I don't think any attempt to make it work on a normal host setup is sane. IMO these tests need to be completely to not assume anything. I think the best way would be to run a container with ssh inside and then setup our own ssh keys so we do not pollute anything on the host ssh setup |
|
/lgtm |
stale-locking-app
Bot
added
the
locked - please file new issue/PR
3 participants
containers/commonfork (until containers/common#2212 get merged)The changes included:
containers/commonto include Use skeema/knownhosts, not x/crypto/ssh/knownhosts common#2212codespellso that the new tests can useAfterAll(func() { // codespell:ignore afterallknown_hostsfilesDepends on containers/common#2212
Fixes #23575