Skip to content

Rate Limit Revamp #41

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

Merged
merged 44 commits into from
Jul 3, 2024
Merged

Rate Limit Revamp #41

merged 44 commits into from
Jul 3, 2024

Conversation

klaidliadon
Copy link
Contributor

@klaidliadon klaidliadon commented Jun 7, 2024

This PR accomplishes a few things:

  • It simplifies the rate limit. Instead of having a middleware for QuotaControl and one for other requests, now we only have one.
  • It removes the error handler as an argument, which was used for legacy versions, since all services have migrated to the most recent version of WebRPC.
  • The errors are handled directly in the client, instead that the error handling function passed as an argument. When the client has an unexpected error, we log the error but don't propagate it.
  • Added a series of test cases of weird scenarios, where we have timeouts or server errors in any method.
  • SetCredentials middleware allows to pass a jwtauth.JWTAuth, to check for the JWT in the context and use the project claim, to set the project in the context.
  • VerifyQuota uses the project from the context to get the project quota, or the access ket to get the access quota.
  • Added a Session middleware that it sets the session type in the context, depending on JWT and AccessKey
  • Added an AccessControl middleware that receives a list of service/endpoints + permissions and credits cost, verifies that the session type from Session is allowed and sets the compute units for EnsureQuota and SpendQuota

TODO:

  • Testing JWT Token, and JWT Token+Access
  • Showcase the new version in an existing service

Copy link
Contributor

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Can you provide an example usage from the app perspective, please?

Is this PR backward-compatible, or do we need to deploy all apps at the same time?

@klaidliadon
Copy link
Contributor Author

klaidliadon commented Jun 9, 2024

Can you provide an example usage from the app perspective, please?

Yes, I will provide an example soon.

Is this PR backward-compatible, or do we need to deploy all apps at the same time?

There is no change on the way the server client communication (see ridl file), so client apps should continue to work as before. Upgrading an existing app will require a some changes, but the resulting code should be cleaner, and a few of the issue we encountered should also be taken care of.

Copy link
Contributor

@david-littlefarmer david-littlefarmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few comments

Copy link
Contributor

@VojtechVitek VojtechVitek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great in overall. Minor suggestions but no blockers.

LGTM.

return cfg, true
}

type rcpRequest struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rpc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a WebRPC request that is parsed and checked in AccessControl middleware.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rcp => rpc :)

// Session middleware that detects the session type and sets the account or service on the context.
func Session() func(http.Handler) http.Handler {
return func(next http.Handler) http.Handler {
return session{Next: next}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow why instantiating session struct is necessary

why not write the logic directly in this MW?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a struct for each middleware. Just for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed -- but I don't think it's necessary :)
You can create local variables in the first function returning the MW function :)

func Session() func(http.Handler) http.Handler {
	// create vars here
	return func(next http.Handler) http.Handler {
		// MW
	}
}

Anyway, not a blocker ;)

@klaidliadon klaidliadon merged commit aa2b30d into master Jul 3, 2024
1 check passed
@klaidliadon klaidliadon deleted the ratelimit-revamp branch July 3, 2024 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants