ssh package #1094
ssh package #1094
Conversation
cdoern
force-pushed
the
ssh
branch
4 times, most recently
from
fb75000 to
7f12fff
Compare
|
just realized I reintroduced pkg/errors. will remove that. |
There was a problem hiding this comment.
I do not like inheriting the entities and infra structure from Podman.
I always had a hard time interpreting the meaning of the two names as they are very generic. We also have naming conflicts when importing them into the Podman code.
Do we need to separate the types and interfaces from the implementations? If so, I prefer to keep the types and interfaces in pkg/ssh and the implementations in pkg/ssh/{foo,bar,...}. It feels more idiomatic to me.
|
I agree, Do not use entities and infra |
|
i'll try to change up the structure @vrothberg the best I can seem to do is: ssh/
otherwise we get cyclical imports and the interface does not work. I know this is bulky but this is a bulky addition to common and one that we do not want to have to re work over and over |
cdoern
force-pushed
the
ssh
branch
3 times, most recently
from
0a492f5 to
c36c2e9
Compare
cdoern
force-pushed
the
ssh
branch
4 times, most recently
from
a8bd3ae to
205142c
Compare
There was a problem hiding this comment.
Thanks, @cdoern. I like the package structure much more :)
I wished that more external types and APIs would have comments but it's probably not fair to ask you to do that given it's inherited from Podman.
Before merging, I suggest to open a PR in Podman and vendor this code here. That's a nice way to figure out which types and parts of the API Podman needs. At the moment, I think that we expose more than needed but I am not familiar with the ssh code at all.
cdoern
force-pushed
the
ssh
branch
7 times, most recently
from
aef9298 to
932b17f
Compare
|
@cdoern is this still a draft? |
not anymore :) |
There was a problem hiding this comment.
I’m sorry, I probably won’t be able to review this carefully, at least this week.
I’ve only been looking at the knownhosts uses.
I continue to be a bit worried about the impact of the verification enforcement on users. To an extent I’m very fine with that impact, but I’d be happier if someone could actively check that the most important cases (is that podman machine? Some localhost use?) are not terribly user-unfriendly (e.g. that we don’t ourselves create a VM and then immediately faili accessing it, or something like that).
|
@containers/podman-maintainers PTAL |
|
Just curious, are we aiming this for Podman v4.3 or 5.0? |
@TomSweeneyRedHat I was thinking 4.3 The overall impact on podman should be nothing, unless one wants to use |
|
This should definitely be in podman 4.3 |
initial implementation of the ssh package including: - nativeConnectionCreate() / golangConnectionCreate() - n/a / golangConnectionDial() - nativeConnectionScp() / golangConnectionScp() - nativeConnectionExec() / golangConnectionExeC() the way this works, is there are publicly accessible functions Create, Exec, Dial and Scp. podman will have a new global flag --ssh` that will allow users to choose native or golang based ssh functions. The functionality in containers/common (here) also checks if you have the necessary binaries installed closes containers#1091 Signed-off-by: Charlie Doern <cdoern@redhat.com>
|
@mtrmac @containers/podman-maintainers PTAL and merge |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdoern, giuseppe, rhatdan 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 |
| cfg.Engine.ActiveService = options.Name | ||
| } | ||
|
|
||
| if cfg.Engine.ServiceDestinations == nil { |
There was a problem hiding this comment.
AFAICS this code, and the code creating dst, is ~the same here and in connection_golang.go.
And the only parts of Podman that actually care about sshMode are scp (which I’m ignoring here), and connection.add.
So how does this work? How can the user set up a connection to use the native mode (and ideally to do it every time for that connection without having to set a flag)?
What am I missing about how this works?
| if options.Default { | ||
| cfg.Engine.ActiveService = options.Name | ||
| } |
There was a problem hiding this comment.
This part does not exist in golangConnectionCreate. Isn’t that a bug?
(Really it seems to me that the level of abstraction of this function is incorrect — a lot of the code should not be duplicated between native/golang here, and should not be duplicated with Podman’s connection/add.)
initial implementation of the ssh package including:
the way this works, is there are publicly accessible functions
CreateDialandScp. podman will have a new global flag--sshthat will allow users to choose native or golang based ssh functions. The functionality in containers/common (here) also checks if you have the necessary binaries installedcloses #1091
see https://github.com/cdoern/podman/tree/ssh for podman implementation
Signed-off-by: Charlie Doern cdoern@redhat.com