search

cloudflare / pingora

A library for building fast, reliable and evolvable network services.
Apache License 2.0
20.21k stars 1.1k forks source link

`LoadBalancer` and `Backends` interface are `SocketAddr` oriented, while `ProxyHttp::upstream_peer()` is `HttpPeer` oriented #276

Open jamesmunns opened 2 weeks ago

jamesmunns commented 2 weeks ago

What is the problem your feature solves, or the need it fulfills?

The examples of LoadBalancer are oriented around SocketAddrs, e.g. addr+ports. This includes the constructor of LoadBalancer, as well as the interfaces of Backends.

However, when using LoadBalancer as a part of a ProxyHttp implementation, the Service is expected to provide an HttpPeer, which includes additional information, such as TLS+SNI information.

Is the recommended usage of LoadBalancer to obtain the socketaddr from the selection process, then do a second mapping of SocketAddr to HttpPeers?

Describe the solution you'd like

I'm guessing the intent here is to support both TCP and HTTP load balancing, though I'm unsure what the intended use was for this.

I'm going to start looking into this, but wanted to raise it in case I'm just looking at it incorrectly.

Additional context

CC https://github.com/memorysafety/river/issues/39

drcaramelsyrup commented 2 weeks ago

Right now yes, the usage is that we obtain the SocketAddr and then map to an HttpPeer. Because we want to keep LoadBalancer relatively generic, we probably want to be able to return optional metadata along with the SocketAddr instead of having the balancer return peers.

jamesmunns commented 2 weeks ago

@drcaramelsyrup that makes sense! River could probably assume the metadata should always be there (or ignore HTTP peers without metadata), which would leave the door open for that.

Open to either having "optional" methods, like .get_metadata() -> Option<PeerMetadata>, or having a separate "with metadata" trait which we could bound on for our purposes (so we don't need to do that extra option check).

Let me know if you have any API proposals or have a branch with an API for me to try out, I can hack on it and report back, or open a PR that matches the API you propose.

drcaramelsyrup commented 2 weeks ago

Do you mean including optional metadata in the Backend? I think that might be the easiest way to go, you may want some of that metadata to affect the Backend hash as well. If you have bandwidth to try that out, happy to look at a PR.

jamesmunns commented 1 week ago

Now that I've got #275 figured out, I'll look into this as an alternative to doing a two-stage lookup.