Skip to content

Allow to reset state#436

Merged
grzuy merged 1 commit into
rack:masterfrom
fatkodima:reset-method
Oct 23, 2019
Merged

Allow to reset state#436
grzuy merged 1 commit into
rack:masterfrom
fatkodima:reset-method

Conversation

@fatkodima
Copy link
Copy Markdown
Contributor

#249 (comment)

I can think of 2 (not functionally equal) implementations:

  1. Rack::Attack.reset! which really resets (deletes) specific keys in cache store and can be used for testing as well as production. But, while this can be implemented for most of cache stores (using https://api.rubyonrails.org/classes/ActiveSupport/Cache/Store.html#method-i-delete_matched), this is not (and can't be) supported by memcache (the second most popular cache store), because to reset state, we should know exact key names, which is impossible for production considering rack-attack's dynamic key names nature. Unfortunately, memcache does not support deleting based on patterns/prefixes, like in redis. And need to figure out how to use this with custom, created by user, cache stores.
    Also, personally, I don't think this is a very popular usecase to reset production rack-attack cache. I have used this once to shrink redis memory usage, implemented as a simple rake task executing scan and del on redis. I think, for general use, changing keys prefix and let the backed store prune old entries here is a valid solution.

  2. Implementation only to support user tests. I implemented this version here. Example of usage:

# test_helper.rb
Rack::Attack.test_mode
# some_test_file.rb
...
setup do
  Rack::Attack.cache.store.clear
end
...

After agreeing on implementation, will add missing tests and documentation.

Copy link
Copy Markdown
Collaborator

@grzuy grzuy left a comment

Choose a reason for hiding this comment

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

#249 (comment)

I can think of 2 (not functionally equal) implementations:

1. `Rack::Attack.reset!` which really resets (deletes) specific keys in cache store and can be used for testing as well as production. But, while this can be implemented for most of cache stores (using https://api.rubyonrails.org/classes/ActiveSupport/Cache/Store.html#method-i-delete_matched), this is not (and can't be) supported by memcache (the second most popular cache store), because to reset state, we should know exact key names, which is impossible for production considering rack-attack's dynamic key names nature. Unfortunately, memcache does not support deleting based on patterns/prefixes, like in redis. And need to figure out how to use this with custom, created by user, cache stores.
   Also, personally, I don't think this is a very popular usecase to reset _production_ rack-attack cache. I have used this once to shrink redis memory usage, implemented as a simple rake task executing `scan` and `del` on redis. I think, for general use, changing keys prefix and let the backed store prune old entries here is a valid solution.

Good call.

I would still consider providing a Rack::Attack.reset! method that depends on the store #delete_matched method (passing the prefix) which will work for redis and memory_store.

For mem_cache users wanting to reset in their test suites will have the option to:

  1. use the memory_store for the test suite and use this new .reset! method
  2. clear all mem_cache during tests
2. Implementation only to support user tests. I implemented this version here. Example of usage:
# test_helper.rb
Rack::Attack.test_mode
# some_test_file.rb
...
setup do
  Rack::Attack.cache.store.clear
end
...

After agreeing on implementation, will add missing tests and documentation.

@fatkodima
Copy link
Copy Markdown
Contributor Author

@grzuy Updated with your suggestions. I haven't added any readme notes. I think it is better to add all of them as third subtask of #301

@fatkodima fatkodima changed the title Allow to reset state between tests Allow to reset state Oct 22, 2019
Copy link
Copy Markdown
Collaborator

@grzuy grzuy left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

Comment thread lib/rack/attack.rb Outdated
else
raise(
MisconfiguredStoreError,
"Configured store #{store.class.name} doesn't respond to #delete_matched method"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

MisconfiguredStoreError was conceived to let a user know it need to reconfigure rack-attack or nothing will work.

In this particular case, not sure we're exactly there. This would more like "Everything will work fine with your current store but you won't be able to use the #reset! method"? Maybe IncompatibleStoreError for the constant?

Comment thread lib/rack/attack.rb Outdated

if store.respond_to?(:delete_matched)
store.delete_matched("#{cache.prefix}*")
else
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sounds like everything in this method implementation is interacting with cache-only stuff (store and prefix).
Should we move reset! inside Cache and make the implementation here just cache.reset!?

@fatkodima
Copy link
Copy Markdown
Contributor Author

Updated.

@grzuy grzuy merged commit a4ea214 into rack:master Oct 23, 2019
@grzuy
Copy link
Copy Markdown
Collaborator

grzuy commented Oct 23, 2019

Awesome, thank you!

@camilova
Copy link
Copy Markdown

Should Rack::Attack::reset! remove current IPs that I have listed with safelist_ip ?
Because I'm trying to remove the IPs, but the new reset! method doesnt remove them and does not raise any error. Is there a way to remove from cache only one specific IP that I was added through safelist_ip?

@jdelStrother
Copy link
Copy Markdown
Contributor

Any chance of getting a new release with this in?

@grzuy
Copy link
Copy Markdown
Collaborator

grzuy commented Apr 26, 2020

Released in v6.3.0.

@grzuy
Copy link
Copy Markdown
Collaborator

grzuy commented Apr 26, 2020

Should Rack::Attack::reset! remove current IPs that I have listed with safelist_ip ?
Because I'm trying to remove the IPs, but the new reset! method doesnt remove them and does not raise any error. Is there a way to remove from cache only one specific IP that I was added through safelist_ip?

Hi @camilova , #safelist_ip doesn't write anything to cache.

@camilova
Copy link
Copy Markdown

camilova commented Apr 26, 2020 via email

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