sui icon indicating copy to clipboard operation
sui copied to clipboard

[Traffic Control] Make it play nicely with proxy infra

Open williampsmith opened this issue 1 year ago • 1 comments

Description

We received reports from a community validator of error logs from failure to parse x-forwarded-for header value to a SocketAddr. Investigation revealed that this is because the validator is running HAProxy, which attempts to insert the header itself (and in this case the value inserted was -).

We are not using this value anyway, and even if we were, we would not want to be reading the value inserted by HAProxy as this would correspond to the fullnode rather than the client.

Note that this also should obviate the fact that running TrafficController or related on a validator that is running HAProxy can have bad unintended side effects - namely that a spammy fullnode may cause a validator running HAProxy to block its HAProxy instance rather than the fullnode itself.

To fix all of these issues, this PR introduces configuration of client-id-source, which by default will select the socket-addr (as today) to be treated as the "connection". Alternatively, the node operator can select x-forwarded-for, in which case we will search for this header key and use its contents to determine the client connection.

In the x-forwarded-for case, we have seen that this can be written as a domain name rather than an IP by load balancers such as HAProxy, therefore we must allow for both IP and domain name. If a node operator is running a proxy, then they will need to run with this option, lest they suffer from the issue mentioned above.

Note that in addition to infra proxies such as HAProxy, traffic controller also has the concept of proxy, which from the perspective of a validator would be a fullnode, as it is forwarding a client request to the entire committee. This is a different issue entirely, and as of yet is unsupported. Nevertheless, to reduce the chance for confusion, there is some liberal renaming in this PR.

TODOS

  • Handle client-id-source: x-forwarded-for on json rpc side. Currently we say "unsupported" and skip traffic control.
  • Add tests for when client-id-source: x-forwarded-for is set (via both IP and domain name)
  • Unit tests for domain name parsing

Test plan

Existing tests. More unit tests to come for the x-forwarded-for header case.


Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

  • [ ] Protocol:
  • [ ] Nodes (Validators and Full nodes):
  • [ ] Indexer:
  • [ ] JSON-RPC:
  • [ ] GraphQL:
  • [ ] CLI:
  • [ ] Rust SDK:

williampsmith avatar May 21 '24 19:05 williampsmith

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
sui-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 11, 2024 7:40pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
multisig-toolkit ⬜️ Ignored (Inspect) Visit Preview Jun 11, 2024 7:40pm
sui-kiosk ⬜️ Ignored (Inspect) Visit Preview Jun 11, 2024 7:40pm
sui-typescript-docs ⬜️ Ignored (Inspect) Visit Preview Jun 11, 2024 7:40pm

vercel[bot] avatar May 21 '24 19:05 vercel[bot]