Setting disabled = true on a route should disable the virtual host global rate limit policy

[shadialtarsha]

Envoy has extensions.filters.http.ratelimit.v3.RateLimitPerRoute API which allows control over the Vhost Rate Limits on the route level.

This is very useful to disable the vhost rate limits for example on specific routes like /health routes.

HTTPProxy example:

apiVersion: projectcontour.io/v1
kind: HTTPProxy
metadata:
  name: echo
spec:
  virtualhost:
    fqdn: local.projectcontour.io
    rateLimitPolicy:
      global:
        disabled: true
      local:
        requests: 100
        unit: hour
        burst: 20
  routes:
    - conditions:
        - prefix: /
      services:
        - name: ingress-conformance-echo
          port: 80
    - conditions:
        - prefix: /foo
      rateLimitPolicy:
        global:
          disabled: true
      services:
        - name: ingress-conformance-echo
          port: 80

Fixes #5685

[codecov]

Codecov Report

Merging #5657 (d77620f) into main (9ad7ac5) will increase coverage by 0.00%.
The diff coverage is 86.36%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5657   +/-   ##
=======================================
  Coverage   78.56%   78.57%           
=======================================
  Files         138      138           
  Lines       19164    19184   +20     
=======================================
+ Hits        15057    15074   +17     
- Misses       3820     3822    +2     
- Partials      287      288    +1     

Files Changed	Coverage Δ
internal/dag/dag.go	98.70% <ø> (ø)
internal/envoy/v3/route.go	80.37% <0.00%> (-0.31%)	⬇️
internal/dag/httpproxy_processor.go	91.91% <100.00%> (+0.06%)	⬆️
internal/envoy/v3/ratelimit.go	100.00% <100.00%> (ø)

[davinci26]

implementation lgtm, I am not quite sure about the API here and how it is different than the existing API

[clayton-gonsalves]

Question: doesnt this https://projectcontour.io/docs/main/config/api/#projectcontour.io/v1.GlobalRateLimitPolicy
disabled solve the use case you are trying to achieve?

[skriss]

Thanks @shadialtarsha, I think this makes sense, had a few clarifications for the changelog

[clayton-gonsalves]

I think we should be able to reuse RateLimitPolicy.disabled. which will allow us to not introduce new fields.

[shadialtarsha]

So dag. RateLimitPolicy doesn't really have disabled on it, this noob is only on the API.

I don't believe introducing this switch to DAG is a good idea as DAG is closer to Envoy than to the HTTPProxy API and also RateLimitPerRoute is native to Envoy's struct for doing exactly this.

[clayton-gonsalves]

Do we need VhRateLimitsInclude for this case?

[shadialtarsha]

I don't have a strong opinion here.
I kept them just in case someone wants to extend the implementation and supports another mode.

[shadialtarsha]

Sorry took me a bit of time to address changes here as we were busy with many other things.
will follow up immediately with the next round

[shadialtarsha]

@skriss would appreciate another round once you have time 🙏🏻

[shadialtarsha]

Hey folks sorry another ping for this @skriss and @sunjayBhatia

[skriss]

Sorry, but this file can be removed now since we included it in the 1.26 release notes and then cleared out the individual files from changelogs/unreleased.

[skriss]

Otherwise LGTM, thanks @shadialtarsha

[skriss]

LGTM, thanks @shadialtarsha