Add support for HTTP local rate limiting #388
Draft
+690
−304
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
olix0r
reviewed
Oct 23, 2024
proto/inbound.proto
Outdated
Comment on lines
256
to
257
| // TargetRefs for traffic sources. Only ServiceAccounts supported for now. | ||
| repeated io.linkerd.proxy.meta.Metadata targets = 4; |
There was a problem hiding this comment.
Proxies shouldn't know anything about service accounts or how to translate between kubernetes resources and identity strings. This all belongs exclusively in the control plane.
We also should probably be careful about calling this 'targets' when it is... sources?
Doesn't this belong on an override? Or how does this work if it is specified at the whole policy level?
This is probably where we want to look at some of the authorization prior art to support identity matching while preserving future variants.
There was a problem hiding this comment.
Great points.
I've renamed that part to clients. And it does belong under overrides indeed, my mistake.
And you're right about service accounts, I've refactored it a bit to use identities instead.
alpeb
added a commit
to linkerd/linkerd2-proxy
that referenced
this pull request
Oct 23, 2024
Some caveats: - This doesn't abide yet to the agreed API as declared in linkerd/linkerd2-proxy-api#388 . Instead, this is based on an earlier API proposal using "specifiers" to define the buckets keys. - The limiting logic has been added directly into the inbound http policy middleware. It relies on [governor](https://docs.rs/governor/latest/governor/). If we're not allowing to configure bursting in the first implementation, we might reconsider implementing something simpler directly ourselves. - There is actually an additional middleware (`RateLimitPolicyService`) that is currently commented out (used in the initial demo), that implemented a simpler approach, inspired by Tower's own rate-limiting middleware.
alpeb
force-pushed
the
alpeb/rate-limiting
branch
from
843ba05
to
da8028e
Compare
olix0r
reviewed
Oct 23, 2024
proto/inbound.proto
Outdated
Comment on lines
265
to
268
| // A list of identity suffixes. | ||
| // | ||
| // If this contains an empty suffix, all identities are matched. | ||
| repeated IdentitySuffix suffixes = 2; |
There was a problem hiding this comment.
We're not planning on shipping suffix matches in the first version so it can be omitted.
There was a problem hiding this comment.
Ok, I guess I'll still leave the ClientIdentities wrapper to leave room to add this back again in the future.
alpeb
added a commit
to linkerd/linkerd2
that referenced
this pull request
Oct 25, 2024
This adds an entry for `HttpLocalRateLimit` under `ProxyProtocol.Detect`, `ProxyProtocol.Http1` and `ProxyProtocol.Http2` leaving the door open to add analogous support for protocols besides HTTP later on.
alpeb
force-pushed
the
alpeb/rate-limiting
branch
from
765b27c
to
1abdd2f
Compare
alpeb
added a commit
to linkerd/linkerd2
that referenced
this pull request
Oct 28, 2024
alpeb
added a commit
to linkerd/linkerd2
that referenced
this pull request
Oct 28, 2024
alpeb
added a commit
to linkerd/linkerd2
that referenced
this pull request
Oct 28, 2024
alpeb
added a commit
to linkerd/linkerd2-proxy
that referenced
this pull request
Oct 29, 2024
This adds a new field `local_rate_limit` to `ServerPolicy`, containing three optional rate-limiters: total, identity, overrides (this one is really a vector of limiters, one per configured override). I tried putting that under `Protocol` instead, but the `PartialEq` requirement made it very hard to follow. `Server` OTOH doesn't really require that trait, so I was able to remove it and accommodate the limiters. I made sure to avoid pulling the dashmap dependency in `governor`; I haven't checked yet the necessity of the "jitter" and "quanta" features. This temporarily overrides linkerd2-proxy-api dependency to pick changes from linkerd/linkerd2-proxy-api#388
alpeb
added a commit
to linkerd/linkerd2-proxy
that referenced
this pull request
Oct 29, 2024
This adds a new field `local_rate_limit` to `ServerPolicy`, containing three optional rate-limiters: total, identity, overrides (this one is really a vector of limiters, one per configured override). I tried putting that under `Protocol` instead, but the `PartialEq` requirement made it very hard to follow. `Server` OTOH doesn't really require that trait, so I was able to remove it and accommodate the limiters. I made sure to avoid pulling the dashmap dependency in `governor`; I haven't checked yet the necessity of the "jitter" and "quanta" features. This temporarily overrides linkerd2-proxy-api dependency to pick changes from linkerd/linkerd2-proxy-api#388
alpeb
added a commit
to linkerd/linkerd2-proxy
that referenced
this pull request
Oct 29, 2024
This adds a new field `local_rate_limit` to `ServerPolicy`, containing three optional rate-limiters: total, identity, overrides (this one is really a vector of limiters, one per configured override). I tried putting that under `Protocol` instead, but the `PartialEq` requirement made it very hard to follow. `Server` OTOH doesn't really require that trait, so I was able to remove it and accommodate the limiters. I made sure to avoid pulling the dashmap dependency in `governor`; I haven't checked yet the necessity of the "jitter" and "quanta" features. This temporarily overrides linkerd2-proxy-api dependency to pick changes from linkerd/linkerd2-proxy-api#388
alpeb
added a commit
to linkerd/linkerd2-proxy
that referenced
this pull request
Oct 29, 2024
This adds a new field `local_rate_limit` to `ServerPolicy`, containing three optional rate-limiters: total, identity, overrides (this one is really a vector of limiters, one per configured override). I tried putting that under `Protocol` instead, but the `PartialEq` requirement made it very hard to follow. `Server` OTOH doesn't really require that trait, so I was able to remove it and accommodate the limiters. I made sure to avoid pulling the dashmap dependency in `governor`; I haven't checked yet the necessity of the "jitter" and "quanta" features. This temporarily overrides linkerd2-proxy-api dependency to pick changes from linkerd/linkerd2-proxy-api#388
alpeb
added a commit
to linkerd/linkerd2-proxy
that referenced
this pull request
Oct 29, 2024
This adds the local_rate_limit module to the server-policy crate, that `ServerPolicy` uses for its new `local_rate_limit` field, containing three optional rate-limiters: total, identity, overrides (this one is really a vector of limiters, one per configured override). I tried putting that under `Protocol` instead, but the `PartialEq` requirement made it very hard to follow. `Server` OTOH doesn't really require that trait, so I was able to remove it and accommodate the limiters. I made sure to avoid pulling the dashmap dependency in `governor`; I haven't checked yet the necessity of the "jitter" and "quanta" features. This temporarily overrides linkerd2-proxy-api dependency to pick changes from linkerd/linkerd2-proxy-api#388
alpeb
added a commit
to linkerd/linkerd2-proxy
that referenced
this pull request
Oct 29, 2024
This adds the local_rate_limit module to the server-policy crate, that `ServerPolicy` uses for its new `local_rate_limit` field, containing three optional rate-limiters: total, identity, overrides (this one is really a vector of limiters, one per configured override). I tried putting that under `Protocol` instead, but the `PartialEq` requirement made it very hard to follow. `Server` OTOH doesn't really require that trait, so I was able to remove it and accommodate the limiters. I made sure to avoid pulling the dashmap dependency in `governor`; I haven't checked yet the necessity of the "jitter" and "quanta" features. This temporarily overrides linkerd2-proxy-api dependency to pick changes from linkerd/linkerd2-proxy-api#388
alpeb
added a commit
to linkerd/linkerd2-proxy
that referenced
this pull request
Oct 29, 2024
This adds the local_rate_limit module to the server-policy crate, that `ServerPolicy` uses for its new `local_rate_limit` field, containing three optional rate-limiters: total, identity, overrides (this one is really a vector of limiters, one per configured override). I tried putting that under `Protocol` instead, but the `PartialEq` requirement made it very hard to follow. `Server` OTOH doesn't really require that trait, so I was able to remove it and accommodate the limiters. I made sure to avoid pulling the dashmap dependency in `governor`; I haven't checked yet the necessity of the "jitter" and "quanta" features. This temporarily overrides linkerd2-proxy-api dependency to pick changes from linkerd/linkerd2-proxy-api#388
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds an entry for
HttpLocalRateLimitunderProxyProtocol.Detect,ProxyProtocol.Http1andProxyProtocol.Http2leaving the door open to add analogous support for protocols besides HTTP later on.Also note there are lot of smaller unrelated changes stemming from using protoc v3.20.3 (as per the dev:v43 container) instead of v3.12.4 which was used apparently by mistake in c9914c2