cloudflare / cloudflare/pingora

Draft Feature: Convert APIs using `&str` for addresses to `ToSocketAddr`

Open
#804 0 comments 0 reactions 0 assignees View on GitHub
ergonomics
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.