support ratelimit shadow mode

huzlak commented 2 years ago

Version

1.11.x (latest stable)

Is your feature request related to a problem? Please describe.

Customer would like to enable shadow_mode and use it per vs/authconfig, to be able to easilly monitor new ratelimit configuration before actually limiting the users.

Describe the solution you'd like

shadow_mode keyword would be recognized by ratelimit.api.solo.io.Descriptor, e.g.

spec:
  raw:
    descriptors:
    - key: generic_key
      value: counter
      rateLimit:
        requestsPerUnit: 10
        unit: MINUTE
      shadow_mode: true
...

Describe alternatives you've considered

No response

Additional Context

No response

jbohanon commented 2 years ago

One possible area of hidden complexity here is supporting the numerous ways available to configure rate limits. While they all distill down to raw descriptors, it is not immediately obvious to me that translating the shadow_mode will be simple. A possible example for how to do this might be the alwaysApply setting.

sam-heilbron commented 2 years ago

This seems like a valuable addition.

The work to support this seems like it will fit into the following areas:

Support processing the shadow_mode API in rate-limiter with statistics, api changes, and tests (3 days)
Propagate the rate-limit shadow_mode API to the Gloo Edge API (1 day)
Translate the Edge API into ratelimit xds config in Gloo Edge Enterprise (2 days). As Jacob points out we'll need to support rate limit CRs, global settings, and ingress rate limits.
Improve e2e testing of rate-limiter includeing shadow mode (1 day).

There is a reference in the Envoy docs to a "global" shadow mode. Is that something that is necessary? If so, what might that look like from a UX?

dpashkevich commented 1 year ago

We would also like this feature. Changing rate limit settings can have an immediate severe impact on API consumers, so "trialing" out your configuration with live traffic is indispensible. There are just way too many nuances (descriptor matching rules, precedence, presence of the matching actions/setActions, presence of the actual request headers that we depend on, etc.), coupled with a tricky, non-straightforward syntax, that make it very easy to mess things up.

The proposed configuration options outlined by @huzlak and @sam-heilbron make perfect sense to me, as Gloo's descriptor config format generally mirrors Envoy's. Please make sure to make it work with both descriptors and setDescriptors config styles. We're currently using the former, but planning on migrating to the Set-Style rate limit APIs in near future.

I personally only see marginal value of the Global Shadow Mode, as it can be accomplished via enabling shadow mode on all corresponding descriptors. I guess it safeguards you from unintentionally adding a new descriptor without shadow_mode set on it. Maybe Global Shadow mode can be released as a separate follow-up feature if there is demand? If you do support it, I imagine it would be configured on the global settings object in Gloo.

I do believe it's very important to be able to toggle shadow mode on each descriptor individually, because we would want to shadow-test not only the initial introduction of rate limits in a place where there were none, but every following change to an existing rate limit system in place, if we want to tread carefully. I would shadow-test anything from a trivial change of the limit (e.g. 150 rps to 100 rps) to more complex changes like switching from descriptors to setDescriptors, or from simple to compound/nested descriptors, etc.

github-actions[bot] commented 5 months ago

This issue has been marked as stale because of no activity in the last 180 days. It will be closed in the next 180 days unless it is tagged "no stalebot" or other activity occurs.

solo-io / gloo