fides icon indicating copy to clipboard operation
fides copied to clipboard

Integrate redis rate limiter with saas connectors

Open earmenda opened this issue 3 years ago • 1 comments

Closes #1221

Code Changes

  • [x] Added RateLimitPeriod and RateLimitConfig to schema package as the model to be used by our connector config
  • [x] Added rate_limits to SaaSRequest and SaaSConfig models
  • [x] Changed RateLimiter implementation to work with a list of RateLimitRequest. This means that one call to limit can respect multiple limits. Because of this we now decrement counts when a limit is breached.
  • [x] Changed AuthenticatedClient to take depend on a SaasRequest instead of ClientConfig
  • [x] Implemented build_rate_limit_requests in AuthenticatedClient to build RateLimitRequest based on config
  • [x] Call RateLimiter.limit within AuthenticatedClient

Steps to Confirm

  • [ ] Manual testing
  • [ ] Current tests for regression(could still add some more automated testing I think)

Pre-Merge Checklist

  • [ ] All CI Pipelines Succeeded
  • Documentation Updated:
    • [ ] documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • [ ] documentation issue created (tag docs-team to complete issue separately)
  • [ ] Issue Requirements are Met
  • [ ] Relevant Follow-Up Issues Created
  • [ ] Update CHANGELOG.md

Description Of Changes

Model and integration changes for rate limiter from #1189.

We decided to allow the rate limiter to allow multiple limits which meant changing current limiter implementation. If we want to respect multiple limits simultaneously then it is not enough to just increment the rate limiter as it did before. If one limit succeeds and another fails, then the rate limiter should decrement the succeeding limit as a call will not be made.

One place where I could see us having further discussion is whether request level limits should override or add on to connector rate limits. This is implemented in build_rate_limit_requests.

earmenda avatar Oct 13 '22 14:10 earmenda

@adamsachs n @galvana, I think this is in a pretty good place to get some feedback. I'll also add some comments for some things which I think need more attention.

earmenda avatar Oct 13 '22 16:10 earmenda

@earmenda thanks for addressing the feedback - i think this looks just about good to go, besides one mypy issue it seems.

i also put down one minor comment about a potential enhancement of one of your tests - but that's not a blocker.

adamsachs avatar Oct 19 '22 18:10 adamsachs

sorry about the deleted comment, was trying to understand if that's why I couldn't resolve all conversations.

earmenda avatar Oct 25 '22 06:10 earmenda

@adamsachs @galvana I made some major changes since we reviewed this last week. As I was writing documentation I realized that there was some confusing user experience with my original idea. I decided that if a rate limit config is set on the client config AND at the request endpoint then it should work as an override. Some reasons for this:

  • More inline the current design of client config override. From an UX perspective and implementation as well.
  • Adding the limits at both levels doesn't really add much usefulness for users. I couldn't think of a use case which wasn't possible by treating it as an override instead.
  • Allows for more use cases. By treating this as an override we can define a rate limit in client config and disable the limit for a specific request endpoint. To allow for this I changed the model of RateLimitConfig to have an enabled field. Limits are now defined within a limits field.

I also decided to redo the refactoring that I had undone because I wanted to simplify the SaasConnector and AuthenticatedClient a little bit. Here is the changes to each:

SaasConnector:

  • No longer sets client_config in constructor. Created a new method get_client_config which returns the client config based on the current class state.
  • Changes to class state are now only done through set_privacy_request_state and set_saas_request_state methods. This should make it more obvious to the developer where the class state is being modified.
  • Creating a client can now just be done by calling create_client without any arguments or additional methods to change state in AuthenticatedClient.
  • Removed as many usages of # type: ignore as I could.

AuthenticatedClient

  • Removed logic for determining correct client config. It now only uses the client config passed in the constructor. This works because SaasConnector now sends only the correct config by using its get_client_config method.

Sorry for the refactoring to this PR but it was closely tied to the implementation of this feature. I am pretty comfortable merging this now after review. Documentation is also done!

earmenda avatar Oct 25 '22 07:10 earmenda

@conceptualshark do you have some time to take a look at new docs I wrote for saas connector rate limiting?

earmenda avatar Oct 25 '22 07:10 earmenda

@adamsachs Thanks again for the thorough review, your comments have been really helpful.

earmenda avatar Oct 27 '22 17:10 earmenda