Page MenuHomePhabricator

Integrate requestctl haproxy rules into our TLS terminator
Closed, ResolvedPublic

Description

Integrating requestctl-generated haproxy dsl in our configuration isn't as simple as slapping the relevant code in a file, and include it from tls.cfg. In fact, haproxy doesn't have the concept of includes at all, but it can load multiple files.

I think we basically have two options:

  1. We rewrite the haproxy config as a confd::file, where we inject the requestctl rules in the right place
  2. We create an "admission backend" to haproxy, defined in a separated file and with all the requestctl ACLs, to which we rely the request, and which will take care of relying the request back to varnish.

I see pros and cons to both approaches, namely:

  • Single file: it makes the file more unreadable and slightly harder to maintain, and the template harder to reason about in general. On the other hand, nothing would change of our production setup, more or less. So while this solution is uglier, it has less unknowns.
  • Separate backend: of course it's more readable and easier to manage, and could even allow us to do fancier things over time, but at the cost of a significant change in how we serve things at the TLS layer and the risk of some issues arising from adding another http proxy.

I think ultimately this is the Traffic's team choice.

Event Timeline

If the requestctl rules are defined in a separate backend, they obviously need to be evaluated strictly after the ones in frontend (so, they are necessarily last ones). AFAIK there's no "decent" way to "redirect" a request back to the frontend. Also, is not possible to redirect a request from a backend to another (at least with the use_backend directive)

requestctl integration could be a great candidate to write a custom SPOA (Stream Processing Offload Agent) that handles all the requestctl rules and returns an OK/block/rate-limit response back to HAProxy, but given this is out of scope for this quarter I'm tempted to stick with option number 1 and keep this as simple as possible in terms of request flow at the expense of some extra configuration file complexity.

Change #1056875 had a related patch set uploaded (by Giuseppe Lavagetto; author: Giuseppe Lavagetto):

[operations/puppet@production] haproxy: add confd_file define

https://gerrit.wikimedia.org/r/1056875

Change #1056876 had a related patch set uploaded (by Giuseppe Lavagetto; author: Giuseppe Lavagetto):

[operations/puppet@production] haproxy: add ability to inject requestctl rules

https://gerrit.wikimedia.org/r/1056876

Change #1056875 merged by Giuseppe Lavagetto:

[operations/puppet@production] haproxy: add confd_file define

https://gerrit.wikimedia.org/r/1056875

Change #1056876 merged by Giuseppe Lavagetto:

[operations/puppet@production] haproxy: add ability to inject requestctl rules

https://gerrit.wikimedia.org/r/1056876

Change #1059457 had a related patch set uploaded (by Giuseppe Lavagetto; author: Giuseppe Lavagetto):

[operations/puppet@production] haproxy: add confd files for ipblock maps

https://gerrit.wikimedia.org/r/1059457

Change #1059459 had a related patch set uploaded (by Giuseppe Lavagetto; author: Giuseppe Lavagetto):

[operations/puppet@production] cache: test requestctl rules in haproxy on cp4044

https://gerrit.wikimedia.org/r/1059459

Joe triaged this task as High priority.Aug 5 2024, 5:42 AM

Change #1059457 merged by Giuseppe Lavagetto:

[operations/puppet@production] haproxy: add confd files for ipblock maps

https://gerrit.wikimedia.org/r/1059457

Change #1059459 merged by Giuseppe Lavagetto:

[operations/puppet@production] cache: test requestctl rules in haproxy on cp4044

https://gerrit.wikimedia.org/r/1059459

Change #1060198 had a related patch set uploaded (by Giuseppe Lavagetto; author: Giuseppe Lavagetto):

[operations/puppet@production] haproxy: change behaviour for requestctl filters

https://gerrit.wikimedia.org/r/1060198

Change #1060198 merged by Giuseppe Lavagetto:

[operations/puppet@production] haproxy: change behaviour for requestctl filters

https://gerrit.wikimedia.org/r/1060198

I'm reopening this to track the rollout of this feature beyond cp4044.

Change #1092914 had a related patch set uploaded (by CDanis; author: CDanis):

[operations/puppet@production] haproxy+requestctl: enable in codfw

https://gerrit.wikimedia.org/r/1092914

Change #1092914 merged by CDanis:

[operations/puppet@production] haproxy+requestctl: enable in codfw

https://gerrit.wikimedia.org/r/1092914

Now deployed on all cp hosts in codfw.

This of course caused a minor dashboard issue, in all plots that use haproxy_backend_ metrics and look only at {proxy="tls"}:

image.png (2,136×1,263 px, 298 KB)

The fix is to use the requestctl_filters proxy instead:

image.png (1,876×1,356 px, 192 KB)

I fixed the instances of this I found (this panel)

sum(rate(haproxy_backend_bytes_out_total{proxy=~"tls|requestctl_filters", site="$site", cluster=~"cache_$cluster"}[5m])) by (instance)

and this panel

Change #1093940 had a related patch set uploaded (by CDanis; author: CDanis):

[operations/puppet@production] Move x-requestctl ingress scrub from Varnish to haproxy

https://gerrit.wikimedia.org/r/1093940

Change #1093940 merged by CDanis:

[operations/puppet@production] Move x-requestctl ingress scrub from Varnish to haproxy

https://gerrit.wikimedia.org/r/1093940

The good news: this works, we now see some hap: prefix rules appearing in x-requestctl.

The bad news: especially on text, we aren't reusing as many connections. https://grafana.wikimedia.org/goto/o_lT6OnNR?orgId=1
I'm not sure how best to evaluate the user impact of this?

Reedy renamed this task from Integrate requestctl haproxy rules into our TLS terminator to Integrate requestctl haproxy rules into our TLS terminator.Nov 21 2024, 8:41 PM

@Fabfur I have no idea where we are on this task, specifically regarding the worry chris expressed above. Given the functionality is deployed, I'd resolve this task.

@Fabfur I have no idea where we are on this task, specifically regarding the worry chris expressed above. Given the functionality is deployed, I'd resolve this task.

👍