Don't attempt to resolve TCP sockets client side #120
Conversation
|
This change is |
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files
|
|
Thanks for the review, @NobodyXu. Sorry for the terse PR description last night; filed in a rush. Added some more context to the description now. |
|
@benesch Please run |
It may be important to have the SSH server perform DNS resolution, as the client often does not use the same DNS server as the server.
|
Whoops, sorry about that! Done. |
| pub fn new<'a, S>(host: S, port: u16) -> Socket<'a> | ||
| where | ||
| S: Into<Cow<'a, str>>, |
There was a problem hiding this comment.
This changes the public api, and so would end up requiring a major version bump. Would you mind making this a separate constructor instead?
cc @NobodyXu we have to be a little careful about these kinds of changes. Maybe we should add a CI step that invokes https://github.com/obi1kenobi/cargo-semver-checks or https://github.com/Enselic/cargo-public-api to catch these things automatically (at least in most cases).
There was a problem hiding this comment.
This changes the public api, and so would end up requiring a major version bump. Would you mind making this a separate constructor instead?
Sure, but with #131 we probably need a breaking change anyway.
cc @NobodyXu we have to be a little careful about these kinds of changes. Maybe we should add a CI step that invokes https://github.com/obi1kenobi/cargo-semver-checks or https://github.com/Enselic/cargo-public-api to catch these things automatically (at least in most cases).
Yeah, we definitely need semver-checking in CI.
There was a problem hiding this comment.
I'm sorry to chime in, I saw the stream and think there's common mistake regarding SemVer and the zero major version you might not be aware of (and I learned recently as well). :-)
My comment is just for info, choose whatever version you want. I just want to give some hint.
This changes the public api, and so would end up requiring a major version bump.
Technically, there's no need to bump the major version, because 1.0.0 hasn't been released yet. Therefore, everything so far is "initial development".
The following is cited from SemVer spec:
Major version zero (0.y.z) is for initial development. Anything MAY change at any time. The public API SHOULD NOT be considered stable.
There was a problem hiding this comment.
@stephan-cr in Rust, when the leftmost version number component is a 0, then the next digit over is effectively considered the major version. That's what I was referring to bumping. Updating x in 0.x has the same effect and implications as bumping x in x.0, and so it is still considered (and referred to) as a major version bump.
There was a problem hiding this comment.
Just a user here, so discount my opinion appropriately, but strong preference from me for a semver-breaking release with just the new API, rather than a backcompatible release that includes both APIs! The existing API is, AFAICT, not possible to use correctly with remote port forwarding, and so forcing users into the new API seems worth the breakage to me.
(Earlier discussion on this: #120 (comment))
It may be important to have the SSH server perform DNS resolution, as the client often does not use the same DNS server as the server.
We ran into this issue at @MaterializeInc, where we use
opensshto set up SSH tunnels via bastion hosts. Our customers often want to establish a tunnel to connect to a host whose name is not resolvable on the client side of the tunnel.