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

ssh: Add retries to setupProxy #308

Merged
merged 2 commits into from
Jan 11, 2024
Merged

Conversation

cfergeau
Copy link
Collaborator

When using podman-machine with applehv, sometimes gvproxy would die shortly
after being started. The following happens:

  • podman starts gvproxy with --listen-vfkit
  • gvproxy starts and waits for vfkit to create a network connection
  • podman starts vfkit
  • vfkit creates the VM and connects to gvproxy
  • gvproxy resumes its execution, and tries to create the ssh forwards
    podman asked for on the command line
  • gvproxy fails to create the ssh forward and exits

This happens because setupProxy fails in (*Bastion).reconnect with
"ssh: handshake failed: EOF". This is related to
https://www.man7.org/linux/man-pages/man8/systemd-user-sessions.8.html even
if it's possible to create a TCP connection to the ssh port, sshd/pam won't
necessarily allow you to connect at the ssh level.

This commit fixes this bug by adding a retry to the calls to
CreateBastion() to complement the retries already present in
initialConnection().

initialConnection retries multiple times to establish the TCP connection
which will be used for ssh communication.
This commit adds a generic helper to handle the retry which will be
useful in the next commits.

Signed-off-by: Christophe Fergeau <[email protected]>
@gbraad
Copy link
Member

gbraad commented Jan 11, 2024

/lgtm with the exception why we use nil instead of Bastion{}. but you told me just now you add a comment.

When using podman-machine with applehv, sometimes gvproxy would die
shortly after being started.
The following happens:
- podman starts gvproxy with --listen-vfkit
- gvproxy starts and waits for vfkit to create a network connection
- podman starts vfkit
- vfkit creates the VM and connects to gvproxy
- gvproxy resumes its execution, and tries to create the ssh forwards
  podman asked for on the command line
- gvproxy fails to create the ssh forward and exits

This happens because setupProxy fails in (*Bastion).reconnect with
"ssh: handshake failed: EOF". This is related to
https://www.man7.org/linux/man-pages/man8/systemd-user-sessions.8.html
even if it's possible to create a TCP connection to the ssh port,
sshd/pam won't necessarily allow you to connect at the ssh level.

This commit fixes this bug by adding a retry to the calls to
CreateBastion() to complement the retries already present in
initialConnection().

CreateBastion now returns *Bastion instead of Bastion as the latter type
is not `comparable` which is required by the generic `retry` function.

Signed-off-by: Christophe Fergeau <[email protected]>
@cfergeau
Copy link
Collaborator Author

cfergeau commented Jan 11, 2024

/lgtm with the exception why we use nil instead of Bastion{}. but you told me just now you add a comment.

I added this to the commit log:

CreateBastion now returns *Bastion instead of Bastion as the latter type is not comparable which is required by the generic retry function.

@gbraad
Copy link
Member

gbraad commented Jan 11, 2024

/approve
/lgtm

Copy link
Contributor

openshift-ci bot commented Jan 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau, gbraad

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

@openshift-merge-bot openshift-merge-bot bot merged commit 3cb88d9 into containers:main Jan 11, 2024
13 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants