Bypasseable Errors for Rack Attack Proxy Instances

[mpenalozag]

Description

Some errors on proxy instances could lead to requests not being passed to the next middleware. This could be behaviour that we want to avoid, we wouldn't want our system to stop responding just because of unexpected issues between Rack::Attack and our external Cache instance.

Considerations

• This is a short draft to validate the approach. Happy to iterate it.
• I noticed there could be more tests on some behaviours. Happy to add those after we agree on a solution

[santib]

I like the idea to keep the existing behavior untouched, but maybe we can do something else to not have to override the should_bypass_error? method here?

maybe something like having this DalliProxy have bypassable_store_errors default to ['Dalli::DalliError'] as a String so that it doesn't fail if the gem is not installed?

[mpenalozag]

I think that's a good idea. Like you said, we could just define the bypassable_store_errors and let the BaseProxy class handle the rest.

[santib]

great, also we may need to decide if we want to let the rack attack user "remove" the default DalliError being rescued by default but still rescue other errors I guess..

[mpenalozag]

Here I'm open to options. I think we could define the DalliError as a predefined element in the bypassable_store_errors and be explicit that if the user defines the bypassable_store_errors then this would overwrite the default bypassable_errors

[santib]

Hey @mpenalozag, sorry for the delay and thanks for working on this PR. I'm happy with the solution you are proposing, left just one comment for now.

Would like to know what @grzuy thinks as well. And if we are all ok, then the PR would need tests and some changes to the README, of course.

Thank you!

[mpenalozag]

@santib Awesome! I'll add the tests for the concept after we have agreed on the concept. I'll wait for @grzuy comments

[santib]

wondering if it's better to have just one variable: bypassable_store_errors which can either be a Symbol (:all or :none), or an Array? Just to avoid the possible combinations of both variable sets:

• bypass_all_store_errors: false, bypassable_store_errors: [...]
• bypass_all_store_errors: true, bypassable_store_errors: []
• bypass_all_store_errors: false, bypassable_store_errors: [...]
• bypass_all_store_errors: true, bypassable_store_errors: []

[mpenalozag]

I think that's a good approach. We could also raise a configuration error if both variables are defined. I think the one you mention is cleaner

[mpenalozag]

@grzuy Hey! Just doing a follow up to this PR, hope you can check it soon

[santib]

Hey @mpenalozag, he may not be available at the time. So if you want we can go ahead with the PR 👍

[grzuy]

Hey there!

Sorry for the delay.

@mpenalozag thanks for you contribution!

Interested and eager to go a bit deeper on the issue/problem being addressed here and what others (if any) alternatives exit to address it, before considering adding a new feature that changes the public API in some way.

Can you elaborate on which particular proxy you're using and which errors in particular are making this a problem for you?

Thank you in advance!

[grzuy]

How would this be called from the caller?
Thinking about what the end user API would be.

[mpenalozag]

@grzuy Thanks for your response!

I'm specifically using the RedisCacheStoreProxy. I decided I needed to use rack-attack for the project I'm working on. Investigating on the gem I got to Issue #511 and realized that some people ran into some problems because of an external redis failure. I replicated this locally by filling up the memory of a redis instance and then trying to throttle requests using rack-attack, the requests wouldn't go through and the logged error would be OOM.

The project I'm working on handles payments, which is critical so I can't risk having this sort of issues. If anything fails I don't want the rack-attack middleware to stop any payments requests going through my app just because the dedicated external Redis Instance I'm using for rack-attack has an issue. That is why I found the need for this kind of feature.

From the perspective of how would this be called by the end user API, since it's an opt-in feature and compatible with current usage of rack-attack, the caller would do something like this when deciding to use it:

Rack::Attack.cache.store = ActiveSupport::Cache::RedisCacheStore.new(...),
                          bypass_all_store_errors: true,
                          bypassable_store_errors: [Redis::ConnectionError, Redis::TimeoutError]

As for the options, I'm currently handling this on my project by wrapping the rack-attack middleware on a custom middleware I created just for this purpose. Not sure if there are other options that are more minimalistic and have less impact on the API. However, I'm open to options and would be happy to hear if you have any suggestions.