search

bk-rs / ssh-rs

https://docs.rs/async-ssh2-lite
Apache License 2.0
56 stars 21 forks source link

Support limit unauthenticated conns for bb8 #34

Closed vkill closed 1 year ago

netthier commented 1 year ago

You're effectively reimplementing https://docs.rs/tokio/latest/tokio/sync/struct.Semaphore.html Using a Semaphore would also have the advantage that you could just .await a permit instead of sleeping for an arbitrary amount of time. I believe that having AsyncSessionManagerWithTokioTcpStream own a Semaphore and acquiring a permit for the duration of an unauthenticated connection would be a cleaner way to solve this :thinking:

vkill commented 1 year ago

I have replaced it with Semaphore.

vkill commented 1 year ago

I will merge if it solves the problem.

netthier commented 1 year ago

Sorry, it seems like this was not the issue after all :( After testing against multiple branches and SSH config settings, I found out that the issue is not in fact related to MaxStartups, but MaxSessions instead. Increasing MaxSessions fixed the issue I was having, even on the main branch. However, this does not make any sense to me. Does your bb8-async-ssh2-lite crate multiplex all its sessions over a single TCP connection? I was under the impression it opened a new one each time. According to a few Google searches MaxSessions only applies to the number of sessions on a single TCP connection.

vkill commented 1 year ago

I was under the impression it opened a new one each time

No, please read bb8 README.

netthier commented 1 year ago

I was under the impression it opened a new one each time

No, please read bb8 README.

Sorry, let me rephrase. I meant that every time bb8 wants a new session, it also opens a new TCP connection. https://github.com/bk-rs/ssh-rs/blob/44a3332092ff70c99ed55fe9db6194c37056afe1/bb8-async-ssh2-lite/src/impl_tokio.rs#L142 As opposed to every new session being on the same TCP stream.

vkill commented 1 year ago

every time bb8 wants a new session, it also opens a new TCP connection.

Yes, So we should use max_size to limit the max TCP conns.

netthier commented 1 year ago

every time bb8 wants a new session, it also opens a new TCP connection.

Yes, So we should use max_size to limit the max TCP conns.

Theoretically yes, but the documentation for MaxSessions says this:

MaxSessions Specifies the maximum number of open shell, login or subsystem (e.g. sftp) sessions permitted per network connection. Multiple sessions may be established by clients that support connection multiplexing. Setting MaxSessions to 1 will effectively disable session multiplexing, whereas setting it to 0 will prevent all shell, login and subsystem sessions while still permitting for- warding. The default is 10.

It explicitly says that it limits the number of sessions per connection, but we are only ever using one session per connection? If I understand it correctly, changing that setting should have absolutely no effect on my program. Yet increasing it eliminates the errors I have. I will see what happens if I limit the number of connections in bb8 directly. If that fixes my issue, then there's either a very cursed bug or sshd's docs are weird.

vkill commented 1 year ago

So, it has nothing to do with bb8. But can set max_lifetime

vkill commented 1 year ago

Anyway, this PR can still solve some MaxStartups problems.

vkill commented 1 year ago

https://github.com/bk-rs/ssh-rs/blob/b006dbf7d835b633a57a7d409af9d236fd15d7b5/async-ssh2-lite/tests/integration_tests/tokio_spawn_session.rs#L25-L29

And the MaxSessions is 10. And log has 'no more sessions'.

vkill commented 1 year ago

https://github.com/bk-rs/ssh-rs/blob/79a092ba3c3f63130aa27ac72f69e6b9706f05fa/async-ssh2-lite/tests/integration_tests/tokio_spawn_session.rs#L16-L20

So, I still suggest https://github.com/bk-rs/ssh-rs/issues/28#issuecomment-1396405173 , enable tokio rt-multi-thread feature, don't care MaxSessions, set max_number_of_unauthenticated_conns is MaxStartups

The example