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

redis gem only expects one argument in the get method #190

Closed
monfresh opened this issue Jul 14, 2016 · 7 comments
Closed

redis gem only expects one argument in the get method #190

monfresh opened this issue Jul 14, 2016 · 7 comments

Comments

@monfresh
Copy link

Hi. I've noticed other similar issues, but I couldn't get a definitive answer. When I implement a Fail2Ban filter, I get this error:

ArgumentError - wrong number of arguments (given 2, expected 1):
13:05:52 web.1    |   redis (3.3.0) lib/redis.rb:860:in `get'
13:05:52 web.1    |   /Users/moncefbelyamani/.rvm/rubies/ruby-2.3.1/lib/ruby/2.3.0/delegate.rb:83:in `method_missing'
13:05:52 web.1    |   rack-attack (4.4.1) lib/rack/attack/store_proxy/redis_store_proxy.rb:16:in `read'
13:05:52 web.1    |   rack-attack (4.4.1) lib/rack/attack/cache.rb:23:in `read'
13:05:52 web.1    |   rack-attack (4.4.1) lib/rack/attack/fail2ban.rb:26:in `banned?'
13:05:52 web.1    |   rack-attack (4.4.1) lib/rack/attack/fail2ban.rb:10:in `filter'

The README implies that I should be able to use the redis gem (v3.0.0+), but it looks like it's not supported because RedisStoreProxy#read passes in 2 arguments. From reading the other issues, it seems like I would need to add another dependency to my app for this work, namely the redis-store gem.

What is the purpose of the raw: true option, and is it essential? Could it be made optional? Is this a bug in the redis gem, or is it not required of it to accept a second argument? If the second argument is optional, then could rack-attack please update the read method such that it's compatible with the redis gem?

Thanks!

@sgtpepper43
Copy link

I'm also seeing this issue with the setex command, you're passing in four arguments when it expects three. I don't know if it's actually a problem, but I'm using bugsnag and it logs that error out quite a bit. Would appreciate a little clarification.

@kgleong
Copy link

kgleong commented Oct 25, 2016

I'm also seeing a similar error with the setex method.

My rack-attack Redis store configuration in application.rb:

      redis = Redis.new(
        host: redis_config[:host],
        port: redis_config[:port],
        db: redis_config[:db],
      )

      Rack::Attack.cache.store =
        Rack::Attack::StoreProxy::RedisStoreProxy.new(redis)

@ryantm
Copy link

ryantm commented Nov 18, 2016

It looks like https://github.com/kickstarter/rack-attack/blob/master/lib/rack/attack/store_proxy/redis_store_proxy.rb#L22 is wrong because it passes raw: true to redis, and it doesn't seem like the redis gem supports that.

@ryantm
Copy link

ryantm commented Nov 18, 2016

@ktheory How are these proxies supposed to work anyway? They seem to swallow all the errors that might come from the proxied store without bothering to log them.

@ryantm
Copy link

ryantm commented Nov 18, 2016

I thought this was using the redis gem directly, but it is actually using the redis-store gem which has the appropriate interface.

bfad added a commit to bfad/rack-attack that referenced this issue Feb 6, 2018
While a cache-store proxy exists for the redis-store gem, no such proxy
existed for using the redis gem itself. This fills that gap by adding
such a proxy.

Resolves rack#190
@grzuy
Copy link
Collaborator

grzuy commented Jun 18, 2018

Hi @monfresh, thank you for the issue report.

Can you please share your rack-attack store config if you still facing this issue?

@grzuy
Copy link
Collaborator

grzuy commented Jul 2, 2018

For the record, plain Redis support added in rack-attack 5.4.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants