cloudflare / cloudflare/pingora
Draft Feature: Convert APIs using `&str` for addresses to `ToSocketAddr`
- Dominant language
- Rust
- Stars
- 27.4k
- Forks
- 1.7k
- Avg merge
- 6h 22m
- Merged PRs (30d)
- 3
Description
Currently Pingora clients specify TCP addresses add `&str` which are converted internally. However Rust std already has types for addresses/sockets and their conversion i.e. [`SocketAddr`](https://doc.rust-lang.org/std/net/enum.SocketAddr.html) and [`ToSocketAddrs`](https://doc.rust-lang.org/std/net/trait.ToSocketAddrs.html). Most importantly, it already has blanket implementations for a number of conversions, including `&str`.
I propose converting all uses of `&str` with `ToSocketAddrs` where a `SocketAddr` would be appropriate, and performs conversion internally. Note that we do not use [`IpAddr`](https://doc.rust-lang.org/beta/std/net/enum.IpAddr.html) as it does not contain all required information for IPv6 (specifically `scope_id` for link-local addresses).
As `ToSocketAddrs` is already implemented for `&str` this is not a breaking API change; existing API calls providing `&str` addresses will be automatically converted. However it would be useful to modify some APIs that currently have no return type (e.g. `add_tls(addr)`) to return a Result. There may also be the option for e.g. `add_tls()` to be provided with multiple addresses, that that is out of scope here.
I have already have a [draft branch](https://github.com/tarka/pingora/tree/sockaddr) for this work (I thought it was going to be necessary for a project but managed to work around it; see below).
**Use case**
In [Vicarian](https://github.com/tarka/vicarian/) I allow the admin to specify listening on an interface e.g. `eth0`. The provided interface is expanded to all addresses using [`getifaddrs()`](https://docs.rs/nix/latest/nix/ifaddrs/fn.getifaddrs.html) with returns a SocketAddr-like list. These are converted into `SocketAddr` to preserve information (`scope_id`), but need to be serialised to `&str` for Pingora, which then converts them back. This is all a bit redundant and easy to get wrong (just serialising the IP portion of an IPv6 address will break with link-local addresses). Providing `ToSocketAddr`-compatible types would be more idiomatic Rust.
Contributor guide
Assessment
This issue has not been assessed yet.