feat: Add Connector trait

[zu1k]

I need to get a tcpStream established from a specific source IP, but I didn't find a proper bind_connect implementation in ureq.

I checked the historical issue and found a lot of discussions on related content. #301 #419 #251

At the beginning, I added the bind_connect implementation to ureq, but I quickly relized that ureq needs to be kept lightweight, and there may be similar requirements in the future, such as establishing connections from socks files, or other special requirements, so finally I abstracted a most basic implementation of the Connector trait, and implemented BindConnector based on this.

The definition of the Connector trait is the same as the connect and connect_timeout of std TcpStream. I think it may need to be improved according to the needs in the future, but it should be enough for now.

Signed-off-by: zu1k [email protected]

[mcr]

I am concerned about how this might interact with TLS.
Maybe the example could do both TLS and connect()?

[zu1k]

I am concerned about how this might interact with TLS.
Maybe the example could do both TLS and connect()?

Sorry I didn't understand you. Do you mean that the Connector also should do the work of the TLS handshake and stripping? Or maybe I should rename it to TcpConnector to avoid confusion with TlsConnector?

I don't think it's a good idea to abstract TcpConnector and TlsConnector into one Connector, for this makes external use of ureq more complicated.

[algesten]

Overall I'm really positive to this abstraction, however, I want to tie it to the ReadWrite trait instead of working with std::net::TcpStream directly.

That potentially makes the task a lot harder, because it has wider ramifications for the handling of set_read_timeout as well as some potentially tricky trait bounds to work out.

[amunra]

I've been redirected here from #692. My requirement is pretty simple: Set the local bind interface address before establishing the connection, something that std::net::TcpStream can't do, thus it would be nice to allow the ureq user to do this by setting a connector on the agent.

It's probably a good idea to start summarising everything and document it in one go before jumping to a propoal.

Reading through the comments above there seem to be two candidate approaches with different tradeoffs and a set of desired features.

I'll start with the desired features (aka constraints):

• (C1) No breaking API changes.
• (C2) The ability to construct a custom TcpStream, for example with .bind() calls or custom TCP flags.
• (C3) Is composable with ureq::stream::TlsConnector.
• (C4) Is compatible with ureq::stream::Stream:
  • Stream exposes pub(crate) remote_addr: SocketAddr,
• (C5) Is compatible with ureq::stream::ReadWrite:
  • ReadWrite exposes fn socket(&self) -> Option<&TcpStream>
• (C6) The ability to more generically connect a URL to something that can read and write, thus supporting other protocols such as unix domain sockets.

So far, there are two proposed approaches with partial coverage of these constraints:

• (A1) support a new tcp-specific connection trait that yields a connected TcpStream.
  • Satisfies (C1), (C2), (C3), (C4), (C5).
• (A2) support a new generic connection trait that returns a dyn ureq::stream::ReadWrite.
  • Satisfies (C1), (C2), (C4), (C6).
  • Incompatible with (C3) as there may not be a DNS available to perform TLS auth.
  • Possibly incompatible with (C5)? [Feedback desired]
    • Unclear if can fully work with ReadWrite, since all examples I could see return Some(socket) from fn socket: I don't fully understand if it's OK for it this field to be None.

TL;DR of the two approaches:

• (A1):
  • Is simple to implement and slots right into the existing code.
  • Isn't generic enough for (C6).
  • Easier to use for most users who mostly care about TCP.
  • Easily composes with TLS.
• (A2):
  • Is trickier to implement due to the rest of the code relying on socket.
  • Is harder to use for users just caring about sockets (but examples can help here).
  • Is not (necessarily) composable with TLS, unless a DNS can be exposed.

@algesten (and others), kindly let me know where I'm going wrong and what my misunderstandings are: I've only stared at the code for 20 minutes :-).

[algesten]

@amunra I think you captured it well!

I think .socket() call can return None. It's used to implement the timeouts, since we delegate that to the underlying socket. However if we do return None, we need to handle the timeout differently.

[amunra]

Are you referring to this?

    pub(crate) fn set_read_timeout(&self, timeout: Option<Duration>) -> io::Result<()> {
        if let Some(socket) = self.socket() {
            socket.set_read_timeout(timeout)
        } else {
            Ok(())
        }
    }

I don't really see how it would be possible to bolt timeouts onto the ReadWrite trait (which itself would it make it a breaking API change).

For approach (A2), the timeouts would need to be passed to the connector trait: They're supported for unix domain sockets.

[amunra]

I've taken a quick look at what would be involved in (A2).
The first step would be to decouple struct Stream from std::net::TcpStream.
main...amunra:ureq:connector

This is enough to compile the main code, but not the tests and doesn't even introduce a connnector trait yet.

Personally, I feel like sticking to TCP (i.e. implementing (A1)) may be a more pragmatic apprach, at the cost of dropping (C6).

I think (A2) can be brought to completion, but would complicate the existing logic a fair bit: The code I hacked so far feels very much square-peg-round-hole and cleaning it up (changing the ReadWrite trait instead of introducing a new parallel StreamConnection trait) would require breaking API compatibility (C1).

@algesten I'd appreciate your thoughts.

[algesten]

Fair enough. Let's go with the pragmatic choice, A1.

[amunra]

@zu1k, I wonder if it would make sensible to rename the trait to TcpConnector, this would allow more scope for a more general non-tcp specific connector to be written afterwards, should there be renewed scope for it.

[zu1k]

@zu1k, I wonder if it would make sensible to rename the trait to TcpConnector, this would allow more scope for a more general non-tcp specific connector to be written afterwards, should there be renewed scope for it.

It should be so.