Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement RPC rate limiting #13480

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

implement RPC rate limiting #13480

wants to merge 6 commits into from

Conversation

tgross
Copy link
Member

@tgross tgross commented Jun 24, 2022

Provide re-auth rate limiting at the Nomad servers, with the ability to observe
which users are creating the load.


Note to reviewers: this is a really chonky PR so I've broken it up into commits that can be reviewed individually:

  • documentation, including all the metrics reference updates (~500 lines)
  • the implementation of the rate limiter itself (most of the guts are here, ~550 lines)
  • returning an appropriate error from the HTTP server
  • getting ready to wire everything up by threading the RPCContext into all the RPC endpoints (+300/-100 line diff)
  • adding the rate limit check to all the handlers (~650 lines of boilerplate)
  • add some missing node secrets (which are needed for this but are currently safe without assuming mTLS)

I still need to do some more E2E testing of this PR but I wanted to get some reviewers of it so that we can pick up any remaining design or UX flaws in parallel.

cc @schmichael @jrasell @mikenomitch (ref RFC NMD-148)

@tgross tgross added theme/api HTTP API and SDK issues type/enhancement labels Jun 24, 2022
@tgross tgross added this to the 1.4.0 milestone Jun 24, 2022
Provide a facility for pre-auth rate limiting at the Nomad servers, with the
ability to observe which users are creating the load.
Add helper functions to each RPC endpoint so that the endpoint configuration
doesn't need to be repeated for individual handlers, and to extract node ID
where needed.
@tgross
Copy link
Member Author

tgross commented Nov 29, 2022

Nothing that a lot of the approach here is going to be superseded by #13480 and follow-up PRs. I may end up just closing this PR out once all the auth and metrics work has been split out.

@tgross tgross modified the milestones: 1.4.x, 1.5.0 Nov 29, 2022
@tgross tgross modified the milestones: 1.5.0, 1.6.0 Jan 20, 2023
@tgross
Copy link
Member Author

tgross commented Jan 20, 2023

Leaving a note here that much of this code has been pulled out into pre-forwarding authentication and RPC rate metrics work planned to ship in Nomad 1.5.0. Once that work is done I'll rebase this on main and start over on any actual limiting.

@tgross tgross removed this from the 1.6.0 milestone May 22, 2023
@tgross tgross added the stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows label May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/needs-rebase This PR needs to be rebased on main before it can be backported to pick up new BPA workflows theme/api HTTP API and SDK issues type/enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant