Conversation
SSHSocket.send()/recv() were writing to subprocess pipes instead of the real forwarded unix socket established by connect(). Additionally, exec_run() now sends Connection: Upgrade headers so the server returns a 101 response, allowing multiplexed output to be read directly from the raw socket. Without the upgrade, http.client closes the connection before the body can be read because no Content-Length or Transfer-Encoding is set. Fixes containers#506 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Max Ammann <max.ammann@zoo.dev>
f272447 to
a1ae74d
Compare
|
@jwhonce tyi this should fix podman on macOS as its using SSH connections there |
Honny1
left a comment
Honny1
left a comment
There was a problem hiding this comment.
Thanks for your contribution. I have some comments. Please rebase onto the main upstream.
| reach the underlying transport socket. | ||
| """ | ||
| if self.base_url.scheme == "http+ssh": | ||
| sock = response.raw._fp.fp.raw._sock |
There was a problem hiding this comment.
I don't like to see using that many private attributes.
| return data | ||
|
|
||
|
|
||
| def _frames_iter(sock): |
There was a problem hiding this comment.
_frames_iter() and _consume_frames() duplicate logic already in podman/api/output_utils.py (demux_output()) and api.stream_frames().
| logger = logging.getLogger("podman.containers") | ||
|
|
||
|
|
||
| def _read_from_socket(sock, n=4096): |
There was a problem hiding this comment.
_read_from_socket(), _read_exactly(), _frames_iter(), _consume_frames() are transport/protocol utilities. They belong in podman/api/output_utils.py or a similar shared module, not in the domain model file.
| else: | ||
| poll = select.poll() | ||
| poll.register(sock, select.POLLIN | select.POLLPRI) | ||
| if not poll.poll(5000): |
There was a problem hiding this comment.
I don't like a hardcoded 5-second timeout.
| response.raise_for_status() | ||
| exec_id = response.json()['Id'] | ||
| # start the exec instance, this will store command output | ||
| # Upgrade headers are required for SSH connections: without them the |
There was a problem hiding this comment.
Changes in exec_run() always send Upgrade headers and always expect a 101 response. even for non-SSH connections where the existing start_resp.content path works fine today. The fallback else branch (lines for non-101) is good defensive coding, but the default behavior should not change for connections that work correctly today. After rebasing, the Upgrade+raw-socket path should be gated on self.client.base_url.scheme == "http+ssh" for the non-socket case.
2 participants
SSHSocket.send()/recv() were writing to subprocess pipes instead of the real forwarded unix socket established by connect(). Additionally, exec_run() now sends Connection: Upgrade headers so the server returns a 101 response, allowing multiplexed output to be read directly from the raw socket. Without the upgrade, http.client closes the connection before the body can be read because no Content-Length or Transfer-Encoding is set.
Fixes #506
This debugging was heavily Claude supported. In the end after it figured it out I manually provided the correct and minimal fixes.