pkg/bindings: add dial-stdio fallback for SSH connections#28100
pkg/bindings: add dial-stdio fallback for SSH connections#28100parmisean wants to merge 2 commits intocontainers:mainfrom
Conversation
|
Thanks for the review @jankaluza, I appreciate the feedback! I'll dive into these comments this evening after work and push a new commit to address them |
d47401f to
a2fc9e5
Compare
|
@jankaluza the implementation significantly changed with the introduction of I'm noticing a CICD error due to binary size growth, which I think is likely due to the
Let me know what you think, or if there's another approach I'm not considering that would match maintainer goals better. |
|
@containers/podman-maintainers , What do you think? I would probably go with option 2). |
|
@jankaluza I pushed an updated commit that uses no-op deadline methods to implement the It looks like I am still triggering the binary growth check. When running
|
804dc44 to
f5d82f0
Compare
Fixes: containers#27814 Signed-off-by: Sean Prouty <[email protected]>
f5d82f0 to
673f9a6
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
3 similar comments
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
@jankaluza Apologies for the force push again. I was still hitting binary growth CI failures because the check diffs against the original I rebased |
| } | ||
|
|
||
| type sshStdioConn struct { | ||
| path string |
There was a problem hiding this comment.
Is the path actually used anywhere?
It seems to be only used in the RemoteAddr, but this is called only in the tests. This looks suspicious to me.
There was a problem hiding this comment.
I see it now I think. The url is passed to ssh.DialNet.
There was a problem hiding this comment.
The primary path passes the URL directly to ssh.DialNet, but the dial-stdio fallback was relying on the servers configuration to determine the socket path. After some digging, I realized we can easily fix this by leveraging the existing global --url flag, updating the ssh command to podman --url <path> system dial-stdio.
I've pushed a new commit to fix this and verified locally that custom socket paths now route correctly through the fallback
| return nil, err | ||
| } | ||
|
|
||
| stdout, err := session.StdoutPipe() |
There was a problem hiding this comment.
I was thinking if we need to handle also stderr here. But it seems the podman system dial-stdio only output stdout, so that's probably fine.
There was a problem hiding this comment.
Yep, from what I can tell podman system dial-stdio only communicates over stdin/stdout, so we wouldn't capture anything from stderr
|
I think this looks good. It would be really nice to have some functional tests for that, but I admit these would not be trivial to do and even our |
Signed-off-by: Sean Prouty <[email protected]>
d63bd7e to
57dd3e8
Compare
I agree, having functional tests for this would be ideal even if it would definitely be a heavy lift. Given the complexity and current state of the dial-stdio tests, it may make sense to defer on heavier automated testing here if you are comfortable with proceeding as is. I'm happy to open a separate issue to track adding comprehensive SSH/connection tests in the future if that's something the project wants to pursue. |

Adds a fallback mechanism for remote SSH connections when the
[email protected]channel type is unsupported, such as when using Tailscale SSH or certain hardened SSH configurations.When the primary connection method fails, Podman will now fallback to executing
podman system dial-stdioover an SSH session to establish the bridge. The fallback state is stored after the first failure to prevent redundant failing connection attempts during command execution.Fixes #27814.
Testing
The following commands were manually verified using a local build of this branch to ensure functionality, targeting remote tailscale SSH with default and custom socket paths, and remote OpenSSHD hosts to verify against regressions.
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?