beater: even more refactoring #5502
Merged
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
- rate limiting middleware is now installed for both RUM and backend agent APIs, but only applies for anonymous clients (currently only RUM) - rate limiting middleware now performs an initial Allow check at the request level, for consistent request rate limiting of those endpoints that are rate limited - agent config now restricts "insecure" (RUM) agents on the basis that they are anonymous, rather than being RUM specifically. The list of insecure agent names (those allowed for anonymous auth) is now passed in
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
simitt
reviewed
Jun 21, 2021
| middleware.CORSMiddleware(cfg.RumConfig.AllowOrigins, cfg.RumConfig.AllowHeaders), | ||
| middleware.AnonymousAuthorizationMiddleware(), | ||
| middleware.KillSwitchMiddleware(cfg.RumConfig.Enabled, msg), | ||
| middleware.AnonymousRateLimitMiddleware(ratelimitStore), |
There was a problem hiding this comment.
Isn't the order wrong now for Authorization and RateLimiting middleware? (the AnonymousRateLimitMiddleware checks c.AuthResult.Anonymous which will only be set afterwards)
There was a problem hiding this comment.
middleware.Wrap calls the Middleware functions in reverse order:
So Wrap(handler, Auth, RateLimit) leads to Auth(RateLimit(handler))
i.e. the first in the chain is outermost, last is innermost after the wrapped handler
There was a problem hiding this comment.
right, I still get confused with the KillSwitch not being the first middleware in the chain; looks good then
There was a problem hiding this comment.
I found it pretty confusing too. Probably something to come back and clarify later.
simitt
approved these changes
Jun 21, 2021
mergify bot
pushed a commit
that referenced
this pull request
Jun 21, 2021
* beater: even more refactoring - rate limiting middleware is now installed for both RUM and backend agent APIs, but only applies for anonymous clients (currently only RUM) - rate limiting middleware now performs an initial Allow check at the request level, for consistent request rate limiting of those endpoints that are rate limited - agent config now restricts "insecure" (RUM) agents on the basis that they are anonymous, rather than being RUM specifically. The list of insecure agent names (those allowed for anonymous auth) is now passed in * make gofmt * beater/api/profile: remove unused field (cherry picked from commit d577ec8)
axw
added a commit
that referenced
this pull request
Jun 22, 2021
* beater: even more refactoring - rate limiting middleware is now installed for both RUM and backend agent APIs, but only applies for anonymous clients (currently only RUM) - rate limiting middleware now performs an initial Allow check at the request level, for consistent request rate limiting of those endpoints that are rate limited - agent config now restricts "insecure" (RUM) agents on the basis that they are anonymous, rather than being RUM specifically. The list of insecure agent names (those allowed for anonymous auth) is now passed in * make gofmt * beater/api/profile: remove unused field (cherry picked from commit d577ec8) Co-authored-by: Andrew Wilkins <[email protected]>
stuartnelson3
pushed a commit
to stuartnelson3/apm-server
that referenced
this pull request
Jun 22, 2021
* beater: even more refactoring - rate limiting middleware is now installed for both RUM and backend agent APIs, but only applies for anonymous clients (currently only RUM) - rate limiting middleware now performs an initial Allow check at the request level, for consistent request rate limiting of those endpoints that are rate limited - agent config now restricts "insecure" (RUM) agents on the basis that they are anonymous, rather than being RUM specifically. The list of insecure agent names (those allowed for anonymous auth) is now passed in * make gofmt * beater/api/profile: remove unused field
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.
Motivation/summary
How to test these changes
The only functional change is the response body for rate-limited RUM intake requests has changed slightly. This is not a problem in practice as the RUM agent does not parse the response.
Related issues
Part of #5347