#585 #1229 #1598 #1915 Rate limiting global configuration #2294

MiladRv · 2025-05-18T20:19:44Z

Closes #585 #1229 #1915

Proposed Changes

This PR implements global rate limiting per #1229.

Adds new GlobalRateLimitOptions in Configuration
Adds new FileGlobalConfiguration in File

coveralls · 2025-05-19T09:39:39Z

coverage: 85.256% (-0.7%) from 85.965%
when pulling 4dd103f on MiladRv:feature/add-global-rate-limit
into 0b794b3 on ThreeMammals:develop.

raman-m · 2025-05-19T10:01:21Z

Hello, Milad!
Thank you for this great PR! I'll be reviewing the code shortly...
However, since Coveralls has reported a decrease in coverage by -0.7%, we need to write unit tests—ideally covering all new code. Unit tests are a crucial part of our development process ❕ Read points 4 and 5.

raman-m

Yellow card penalty! 🟨

Thank once again for submitting this PR However, after the code review, I consider it a draft.
The primary concern is the lack both unit and acceptance tests ❗
While your intention to add documentation is appreciated, please note that documentation should be written upon the completion of the code (at the end of development).

Several minor but important issues need to be addressed as listed below 👇

I recommend starting with the file models design.
Please be aware that the linked #1229 issue requires grouping routes by a key. Prioritize this type of grouping initially, and upon completion, proceed with the Patterns concept.
Presenting your ideas using patterns and method grouping is also necessary. However, I do not support your idea of including the "Methods" feature, as it mixes concerns.

P.S. I will provide my vision of the model's design at a later stage.
P.P.S. I anticipate several rounds of code review. However, this feat has high priority, and I will support you through multiple pair-programming sessions.
P.P.P.S. Other members from the Ocelot development team could review this PR and provide separate code feedback 😉
Life, indeed, not easy 😛

raman-m · 2025-05-19T11:09:50Z

docs/features/ratelimiting.rst

+
+.. code-block:: json
+
+   "GlobalRateLimitRules": [


To which section does it belong?
It would be preferable to display hierarchy within the GlobalConfiguration section.

raman-m · 2025-05-19T11:26:41Z

docs/features/ratelimiting.rst

+    * - ``Pattern``
+      - The downstream path template pattern to match (using Ocelot’s syntax).


Is grouping based on path patterns your idea? 🤔
It seems this approach was not discussed in #1229, where @neetra requested Ocelot’s standard method of grouping based on keys, aka the Key property of a route.
Therefore, grouping by a pattern appears to be a more advanced approach. That said, I have no objections to introducing this feature, provided that grouping by a key is implemented as well.

raman-m · 2025-05-19T11:44:52Z

src/Ocelot/Configuration/File/FileGlobalConfiguration.cs

@@ -42,6 +43,7 @@ public FileGlobalConfiguration()
    public FileMetadataOptions MetadataOptions { get; set; }
    public FileQoSOptions QoSOptions { get; set; }
    public FileRateLimitOptions RateLimitOptions { get; set; }
+    public IEnumerable<FileGlobalRateLimit> GlobalRateLimitRules { get; init; }


The property is defined to return IEnumerable<FileGlobalRateLimit>, but in the constructor, you assign it as List<FileGlobalRateLimit> ❗ This approach is suboptimal for performance. Please note that File* models contain deeply encapsulated data and properties. Thus, it is advisable define a more specific type, with options being List<T> and Dictionary<TK, TV>.

Another concern arises. Why is the "Global" prefix used in the name of a property that is already part of GlobalConfiguration? Kindly rename the property by omitting the "Global" prefix.

raman-m · 2025-05-19T11:58:36Z

src/Ocelot/Configuration/Creator/GlobalRateLimitOptionsCreator.cs

+
+namespace Ocelot.Configuration.Creator
+{
+    public sealed class GlobalRateLimitOptionsCreator : IGlobalRateLimitOptionsCreator


The creator's logic heavily relies on the chosen models. Therefore, I am postponing this class review until we reach an agreement on the model design.

raman-m · 2025-05-19T12:07:24Z

src/Ocelot/Configuration/Creator/RoutesCreator.cs

+    private readonly IVersionPolicyCreator _versionPolicyCreator;
+    private readonly IMetadataCreator _metadataCreator;


No actual changes, but see a GitHub diff. Why? Line endings?
Avoid altering line endings (EOL) if you are not making any changes to the line.
Ensure that you understand the EOL Gotchas in the development process.

raman-m · 2025-05-19T12:21:44Z

src/Ocelot/Configuration/File/FileGlobalRateLimit.cs

+    {
+        public string Name { get; init; }
+        public string Pattern { get; init; } 
+        public List<string> Methods { get; init; }


Pardon me? Why methods?
Are you genuinely suggesting implementing rate limiting based on a method? Astonishing!

raman-m · 2025-05-19T12:30:49Z

src/Ocelot/Configuration/File/FileGlobalRateLimit.cs

+    public sealed record FileGlobalRateLimit
+    {
+        public string Name { get; init; }
+        public string Pattern { get; init; } 


You propose to use Pattern property for grouping routes to apply the rule. 👌
I suggest defining this property within the FileRateLimitOptions. However, the final design of the models requires further discussion.

raman-m · 2025-05-19T12:38:21Z

src/Ocelot/Configuration/GlobalRateLimitOptions.cs

+    {
+        public string Name { get; set; }
+        public Regex Pattern { get; init; } // wildcard → regex
+        public HashSet<string> Methods { get; init; }


The Methods property appears to be a new feature ❕
Please remove all code associated with methods until the idea is implemented using patterns.
First, fully develop the feature and deliver it to the develop branch with complete code.

raman-m · 2025-05-19T12:40:30Z

src/Ocelot/Configuration/InternalConfiguration.cs

No changes detected! This file was mistakenly included in a commit. I will assist you in removing files with such fictitious changes caused by end-of-line (EOL).

raman-m · 2025-05-19T12:46:08Z

test/Ocelot.ManualTest/ocelot.json

@@ -355,6 +355,32 @@
  ],

  "GlobalConfiguration": {
-    "RequestIdKey": "ot-traceid"
+    "RequestIdKey": "ot-traceid",
+    "GlobalRateLimitRules": [


Conducting manual tests a valuable practice during development. However, Ocelot team doesn't accept pull requests without accompanying acceptance tests. Therefore, it is essential to create comprehensive tests that demonstrate the proper functionality of your feature.

P.S. Acceptance tests should be added to the RateLimiting folder.

ggnaegi · 2025-05-20T06:45:24Z

@raman-m Looking at the code, please don't merge

raman-m · 2025-05-20T06:55:48Z

@raman-m Looking at the code, please don't merge

@ggnaegi LoL 😆 Don't worry! In my opinion, this PR is of poor quality, so I don’t think it will be merged until June. I've requested changes, but I have no idea when the author will start fixing them. Yes, please review 🙏 FYI, the author hasn't implemented grouping by a key as we do in the multiplexer. However, issue #1229 requires this, yet the author ignored the requirements and implemented their own ideas.

The main issue with this PR is the design of the file models. Another concern is the inclusion of multiple proposed features. I don’t want to introduce models that will be either useless or difficult to maintain.

raman-m · 2025-05-21T09:38:20Z

@MiladRv Hi Milad !
When do you plan to begin addressing the code review issues?

MiladRv · 2025-05-23T11:30:27Z

@raman-m Hi Raman,
Thanks for your time to review my changes, I'll fix them ASAP.

raman-m · 2025-05-23T14:32:04Z

I am pleased to welcome your return to development 👍
Based on the feedback from my code review, I suggest concentrating on the design of the models.

OK We have this documentation → Global Configuration
The JSON issue lies in the different types of rate limiting. At present, Ocelot implements rate limiting based on the client's header. In the near future—hopefully within this year—we aim to introduce rate limiting based on the client's IP. I propose separating these types of rate limiting in this pull request, as it is a task that should be prioritized and completed ASAP.

Preserve RateLimitOptions for dynamic routing to maintain backward compatibility, while considering potential improvements. Refer to point 2 for static routing and grouping.
Introduce new RateLimiting section →

"GlobalConfiguration": {
  // This is the old section for dynamic routing with service discovery
  "RateLimitOptions": {
    "ClientIdHeader": "MyRateLimiting",
    "DisableRateLimitHeaders": false,
    "HttpStatusCode": 418, // I'm a teapot
    "QuotaExceededMessage": "Customize Tips!",
    "RateLimitCounterPrefix": "ocelot"
  },
  // I propose the following section ->
  "RateLimiting": {
    "ByHeader": {},
    "ByMethod": {},
    "ByIP": {},
    "ByAspNet": {},
    // and more...
    // Common props should go here ->
    "StatusCode": 418,
    "Message": "Customize Tips!",
    "CounterPrefix": "rate-limiting",
    // Also I propose to add separate metadata section
    "Metadata": {}
  }
}

Please share your thoughts regarding the new design of the section.

RaynaldM · 2025-05-26T12:42:19Z

src/Ocelot/Configuration/Creator/GlobalRateLimitOptionsCreator.cs

+                    DisableRateLimitHeaders = g.DisableRateLimitHeaders,
+                    EnableRateLimiting = g.EnableRateLimiting,
+                })
+                .ToList();


Suggested change

.ToList();

.ToArray();

RaynaldM · 2025-05-26T12:46:17Z

src/Ocelot/Configuration/File/FileGlobalConfiguration.cs

@@ -42,6 +43,7 @@ public FileGlobalConfiguration()
    public FileMetadataOptions MetadataOptions { get; set; }
    public FileQoSOptions QoSOptions { get; set; }
    public FileRateLimitOptions RateLimitOptions { get; set; }
+    public IEnumerable<FileGlobalRateLimit> GlobalRateLimitRules { get; init; }


the property is init-only, so it's presumed immutable, and in this case, I think an IReadOnlyCollection would be more appropriate

Correct! However, in such cases of collection, we use IList, IDictionary or Array objects.

RaynaldM · 2025-05-26T12:47:45Z

src/Ocelot/Configuration/File/FileGlobalRateLimit.cs

+        public List<string> Methods { get; init; }
+        public int Limit { get; init; }
+        public string Period { get; init; } 
+        public int HttpStatusCode { get; init; }= 429;


Suggested change

public int HttpStatusCode { get; init; }= 429;

public int HttpStatusCode { get; init; }= HttpStatusCode.TooManyRequests;

RaynaldM · 2025-05-26T12:48:39Z

src/Ocelot/Configuration/GlobalRateLimitOptions.cs

+    {
+        public string Name { get; set; }
+        public Regex Pattern { get; init; } // wildcard → regex
+        public HashSet<string> Methods { get; init; }


Suggested change

public HashSet<string> Methods { get; init; }

public IReadOnlySet<string> Methods { get; init; }

Apologies! Is it truly necessary to hash sets in this context? believe a straightforward collection, such as a List, could suffice.
This property is initialized by the framework's configuration provider, either JSON data or the internal storage of the provider. The specific collection type that will be assigned here remains unknown: it is needed to debug in run-time.

Another point, it seems we must remove Methods entirely. This is a mixing concerns issue I mentioned in my review feedback. The author did not implement the requirement from the linked issue and began implementing their own ideas. I believe this approach leads to coding inefficiency. Therefore, we must remove all code related to Methods and reintroduce it in upcoming PRs as a new feature.

raman-m · 2025-05-27T17:24:39Z

@yjorayev Welcome to code review!
Please write a short message here.
My understanding is that this PR should be merged first because we will develop a universal model schema for the RateLimiting JSON section. For ASP.NET rate limiting I propose to configure with ByAspNet subsection.

raman-m · 2025-05-27T17:56:22Z

@MiladRv wrote on May 23

@raman-m Hi Raman, Thanks for your time to review my changes, I'll fix them ASAP.

Milad, when you mention "ASAP," could you provide us with a timeline for your feedback or commits?
FYI Our Development Process → Best Practices → Remain online after submitting a pull request ❕
Are you busy?

yjorayev · 2025-05-31T02:10:01Z

docs/features/ratelimiting.rst

+
+.. code-block:: json
+
+   "GlobalRateLimitRules": [


If I understood the feature correctly, this is a rate limiter that only applies if a route does not have an explicit rate limiter configured. In that case would it make sense to have "fallback" in the name? Maybe FallbackRateLimitingRules?

The property names are excessively long, necessitating a larger JSON file!

FYI, I will be handling this PR because it seems the author doesn't care about their piece of coding art. My proposal for the JSON schema in the global section is the RateLimiting subsection which will describe multiple types of limiters.

yjorayev · 2025-05-31T02:17:24Z

src/Ocelot/Configuration/Creator/GlobalRateLimitOptionsCreator.cs

+        }
+    }
+
+    public interface IGlobalRateLimitOptionsCreator


I recommend to keep interfaces and implementations separate: each in its own file.

Fair enough! The interface will be moved to a separate file.

yjorayev · 2025-05-31T02:30:18Z

src/Ocelot/Configuration/DownstreamRoute.cs

+
+        if (globalRateLimit == null || RateLimitOptions.EnableRateLimiting)
+        {
+            return;


Not sure how I feel about return in ctor 😆

I have pointed out the same problem of implementing business logic inside the constructor... However, the author continues to soar within the realm of his dreams... 😄
I will address this design issue when I begin working on this pull request.

raman-m

Red card penalty 🟥

@MiladRv Milad,
You have received a red card 🟥 penalty because earlier, you got a yellow 🟨 one 2 weeks ago! 😜
Could you kindly begin addressing the review issues? Otherwise, the Ocelot team will take over this PR themselves.

Milad.Rv added 11 commits May 18, 2025 23:25

feat: implement and configure global rate limit

8e7c99f

fix: fix nullability at constructor of DownstreamRoute.cs

a6b516d

chore: update docs

9002e96

chore: update docs

e08310a

chore: update docs

42723fd

chore: update docs

1599a6f

chore: update docs

3202da7

chore: update docs

c810cde

chore: update docs

4a8c506

chore: update docs

e66ffa9

chore: update docs

26dd1cb

MiladRv temporarily deployed to build.cake May 19, 2025 09:35 — with GitHub Actions Inactive

raman-m assigned raman-m and MiladRv May 19, 2025

raman-m added feature A new feature Rate Limiting Ocelot feature: Rate Limiting Configuration Ocelot feature: Configuration labels May 19, 2025

raman-m added this to the Spring'25 milestone May 19, 2025

raman-m changed the title ~~Feature: implement configuration to enable RateLimit globaly~~ Rate limiting global configuration May 19, 2025

raman-m requested review from RaynaldM, raman-m and ggnaegi May 19, 2025 10:04

raman-m changed the title ~~Rate limiting global configuration~~ #585 #1229 #1598 #1915 Rate limiting global configuration May 19, 2025

raman-m added the high High priority label May 19, 2025

raman-m linked an issue May 19, 2025 that may be closed by this pull request

Apply rate limiting globally #1915

Open

raman-m mentioned this pull request May 19, 2025

Support Global configuration of load balancer and rate limiting options etc for ReRoutes #585

Open

raman-m requested changes May 19, 2025

View reviewed changes

Fix EOL

6519b0c

raman-m had a problem deploying to build.cake May 19, 2025 14:09 — with GitHub Actions Failure

raman-m temporarily deployed to build.cake May 19, 2025 14:12 — with GitHub Actions Inactive

Merge branch 'develop' into feature/add-global-rate-limit

69531a5

raman-m had a problem deploying to build.cake May 23, 2025 10:24 — with GitHub Actions Failure

raman-m temporarily deployed to build.cake May 23, 2025 10:30 — with GitHub Actions Inactive

RaynaldM reviewed May 26, 2025

View reviewed changes

raman-m mentioned this pull request May 27, 2025

#2138 Introduce ASP.NET rate limiting #2188

Open

Merge branch 'develop' into feature/add-global-rate-limit

4dd103f

raman-m temporarily deployed to build.cake May 27, 2025 17:28 — with GitHub Actions Inactive

raman-m added the Spring'25 Spring 2025 release label May 27, 2025

yjorayev reviewed May 31, 2025

View reviewed changes

raman-m requested changes Jun 2, 2025

View reviewed changes

		* - ``Pattern``
		- The downstream path template pattern to match (using Ocelot’s syntax).

		private readonly IVersionPolicyCreator _versionPolicyCreator;
		private readonly IMetadataCreator _metadataCreator;

	public int HttpStatusCode { get; init; }= 429;
	public int HttpStatusCode { get; init; }= HttpStatusCode.TooManyRequests;

	public HashSet<string> Methods { get; init; }
	public IReadOnlySet<string> Methods { get; init; }

#585 #1229 #1598 #1915 Rate limiting global configuration #2294

Are you sure you want to change the base?

#585 #1229 #1598 #1915 Rate limiting global configuration #2294

Uh oh!

Conversation

MiladRv commented May 18, 2025 • edited by raman-m Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Closes #585 #1229 #1915

Proposed Changes

Uh oh!

coveralls commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raman-m commented May 19, 2025

Uh oh!

raman-m left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Yellow card penalty! 🟨

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raman-m May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raman-m May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raman-m May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raman-m May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggnaegi commented May 20, 2025

Uh oh!

raman-m commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raman-m commented May 21, 2025

Uh oh!

MiladRv commented May 23, 2025 • edited by raman-m Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raman-m commented May 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raman-m commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raman-m commented May 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

MiladRv commented May 18, 2025 •

edited by raman-m

Loading

coveralls commented May 19, 2025 •

edited

Loading

raman-m left a comment •

edited

Loading

raman-m May 19, 2025 •

edited

Loading

raman-m May 19, 2025 •

edited

Loading

raman-m May 19, 2025 •

edited

Loading

raman-m May 19, 2025 •

edited

Loading

raman-m commented May 20, 2025 •

edited

Loading

MiladRv commented May 23, 2025 •

edited by raman-m

Loading

raman-m commented May 27, 2025 •

edited

Loading

raman-m left a comment •

edited

Loading