Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jamesmunns opened this issue Jun 11, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@jamesmunns
Copy link
Contributor

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 memorysafety/river#39

@drcaramelsyrup
Copy link
Contributor

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.

@drcaramelsyrup drcaramelsyrup added the enhancement New feature or request label Jun 13, 2024
@jamesmunns
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants