Going to be bold and append to the task description with what we discussed in the cachebust WG meeting today (so that anyone can update it and tick boxes as we go).

Change 832268 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] C:varnish: Add cluster_fe_hit and cluster_fe_ratelimit_hits subroutines

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

Change 832631 had a related patch set uploaded (by Jbond; author: jbond):

[operations/puppet@production] P:cache::varnish::frontend: Add parameter to enable requestctl on hits

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

Change 832268 merged by Jbond:

[operations/puppet@production] C:varnish: Add cluster_fe_hit and cluster_fe_ratelimit_hits subroutines

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

While implementing the the dry-run version of the current hotlinking patch i found out that its not possible to update the resp object in the vcl_hit hook. As such its not possible to update the X-Analytics header with the current suggested approach. This impacts two aspects of the design

Patch requestctl to check the value of internal cache disposition header req.http.X-CDIS against the action's cache_miss_only as part of evaluation. Within our VCL, this will be hit on a hit, miss on a miss, etc.

With the above approach i think the idea is that we could use the exact same bit of vcl code in both the hit and miss paths but control which rule is hit based on the X-CDIS. however the current vcl code that we drop in sets req.http.X-Requestctl on every rule match which will not be possible in the vcl_hit block of code. I think we could resolve this by, instead of mapping the hit/miss parameter to the vcl X-CDIS header, we create two functions, requestcl_ratelimit_miss` and requestcl_ratelimit_hit, in requestctl-filters.inc.vcl. we can then call requestcl_ratelimit_* from the correct blocks(s) in the main vcl code. e.g.

sub requestcl_ratelimit_hit {
  <% rule.filter(cache_miss_only == false).each |rule| %>
  // all the hit rules
  <%= rule %>
}
sub requestcl_ratelimit_miss {
  // all the miss rules
   <% rule.filter(cache_miss_only == true).each |rule| %>
  // all the hit rules
  <%= rule %>
}

include "requestctl-filters.inc.vcl";
...
sub vcl_hit {
  call requestcl_ratelimit_hit;
  ...
}
sub vcl_miss {
  call requestcl_ratelimit_miss;
  ...
}

This still doesn't help with how we can record the rules match in the vcl_hit block. from what i could see its just not possible to modify the request at this point in the vcl code so im not sure what to do to resolve this. however..

Create placeholder-for-now cluster_fe_{ratelimit_,}hit subroutines in our VCL https://gerrit.wikimedia.org/r/c/operations/puppet/+/832268

this is not in implemented but i wonder if vcl_hit is the best place to block cache hits. Specifically i dont think we want to rate-limit cache misses vs cache hits, what we want to do is ratelimit cache misses vs anything else. As such i wonder if we should instead put hooks in vcl_miss for cache misses and in vcl_recv for everything else. This has the benefit that we start ratlimiting the everything else requests e.g. hotlinks much earlier in the processing and although we still cant modify resp we can modify req which may be enough to add some analytics signalling

to be explicit the proposal would be to move to something like

sub requestcl_ratelimit_recv {
  <% rule.filter(cache_miss_only == false).each |rule| %>
  // all the hit rules
  <%= rule %>
}
sub requestcl_ratelimit_miss {
  // all the miss rules
   <% rule.filter(cache_miss_only == true).each |rule| %>
  // all the hit rules
  <%= rule %>
}

include "requestctl-filters.inc.vcl";
...
sub vcl_recv {
  call requestcl_ratelimit_recv;
  ...
}
sub vcl_miss {
  call requestcl_ratelimit_miss;
  ...
}

Removing SRE, has already been triaged to a more specific SRE subteam

In T317794#8289928, @jbond wrote:

While implementing the the dry-run version of the current hotlinking patch i found out that its not possible to update the resp object in the vcl_hit hook. As such its not possible to update the X-Analytics header with the current suggested approach. This impacts two aspects of the design

This is apparently not true, as we now have code that does exactly that. And the varnish documentation suggests the same:

req.http.*
    Type: HEADER
    Readable from: client
    Writable from: client
    Unsetable from: client

    The headers of request, things like req.http.date.

    The RFCs allow multiple headers with the same name, and both set and unset will remove all headers with the name given.

and client is defined as:

Which operations are possible on each variable is described below, often with the shorthand “backend” which covers the vcl_backend_* methods and “client” which covers the rest, except vcl_init and vcl_fini

oblivian opened https://gitlab.wikimedia.org/repos/sre/conftool/-/merge_requests/6

Add support for cache_miss_only to cli.commit()

cdanis merged https://gitlab.wikimedia.org/repos/sre/conftool/-/merge_requests/6

Add support for cache_miss_only to cli.commit()

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

[operations/puppet@production] varnish: add requestctl filters for cache hits

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

To clarify a bit, I didn't take the route described in the task. In fact, we want:

• Rules with cache_miss_only: true to only be processed in vcl_pass/vcl_miss
• Rules with cache_miss_only:false to be processed both in vcl_pass/vcl_miss and vcl_hit

So we've just separated rules so that they have a different patch on etcd - /requestctl-vcl/cache-text/global for miss-only things and /requestctl-vcl/cache-text/hit-global. This way we can filter which rules we include where and we don't need to mess with X-CDIS.

I also don't think we need protection against errors and merging in just a few servers, but I can be convinced otherwise.

Change #1054081 merged by Giuseppe Lavagetto:

[operations/puppet@production] varnish: add requestctl filters for cache hits

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