ExHammer / hammer-backend-redis

A Redis backend for for the Hammer rate-limiter
https://hexdocs.pm/ExHammer/hammer-backend-redis
MIT License
49 stars 32 forks source link

Having trouble connecting with Redis on prod. #78

Open talhaazeemmughal opened 3 months ago

talhaazeemmughal commented 3 months ago

Describe the bug i have redix library defined in my config and i also have hammer one defined as below:

config :hammer,
  backend:
    {Hammer.Backend.Redis,
     [
       expiry_ms: :timer.hours(24),
       redix_config: [
         host: System.get_env("HOST", "127.0.0.1"),
         port: System.get_env("PORT", "6379") |> String.to_integer(),
         password: System.get_env("PASSWORD"),
         ssl: System.get_env("SSL", "false") == "true"
       ],
       pool_size: 30,
       timeout: 5000
     ]}

config :redix,
  url: System.get_env("REDIS_URL", "redis://127.0.0.1:6379"),
  pool_size: 30

i am also adding this to redix and hammer library while booting the app for socket_opts:

[
      customize_hostname_check: [
        match_fun: :public_key.pkix_verify_hostname_match_fun(:https)
      ]
]

now redix is getting connected properly as i ran redix ping command i got pong in response but when i execute Hammer.check_rate in prod iex. it gives redis error saying connection closed. i am kind of stuck here.

** Provide the following details

Expected behavior Hammer.check_rate should be connecting with redis and giving the required output like {:allow, count}, {:deny, limit}

Actual behavior It actually gives the error tuple {:error, err} and err variable says redis connection closed.

talhaazeemmughal commented 3 months ago

i think i need to pass this (since i am using AWS Elasticache Redis Instance)

[
      customize_hostname_check: [
        match_fun: :public_key.pkix_verify_hostname_match_fun(:https)
      ]
]

to the redix_config that i am passing to the hammer library, how can i do that?

image
epinault commented 3 months ago

Hello! interesting! I use also Elasticache and we don t have that issue. Maybe depends on some configuration you might have setup with the Elasticache but that seems to make sense . If you have time to make an MR, feel free to open one. I ll see tomorrow if I can block sometime to create one and release something

talhaazeemmughal commented 3 months ago

Hello! interesting! I use also Elasticache and we don t have that issue. Maybe depends on some configuration you might have setup with the Elasticache but that seems to make sense . If you have time to make an MR, feel free to open one. I ll see tomorrow if I can block sometime to create one and release something

@epinault also i think generating MR will not help me since i am on elixir 1.10.4-otp-22 and hammer 6.2.1 is using elixir 1.13 while the hammer_backend_redis 6.1.2 is using elixir 1.12 and we are upgrading it to use elixir 1.12.

another thing to note over here is that if i pass verify: :verify_none in socket_opts then it works fine.

talhaazeemmughal commented 2 months ago

@epinault , I updated the elixir, erlang and hammer libs:

and i still have the same issue.

epinault commented 1 month ago

do you have more details on the error? cause this is networking issue and I am not sure why that would fail. does it have that error all the time?

talhaazeemmughal commented 1 month ago

do you have more details on the error? cause this is networking issue and I am not sure why that would fail. does it have that error all the time?

@epinault I get this error only when i execute Hammer.check_rate for other redix library is working fine and is getting connected with redis.

talhaazeemmughal commented 1 month ago

@epinault if i pass verify: :verify_none in socket_opts then it works fine.

epinault commented 1 month ago

This is more likely an issue with redux itself or whether you have SSL properly setup. Do you have the certificate setup correctly? You need the proper set of certificate likely installed if you want verify on for ssl.

But might be worth asking on the redix package see what they say?

epinault commented 1 month ago

Also know that the PONG command in cluster is not implemented in redix, there is a ticket out there in that project too

talhaazeemmughal commented 1 month ago

@epinault I already have an issue on that repo. have a look

epinault commented 1 month ago

I see! Thanks that helps! Should have mentioned that early on. I ll See if I can upgrade it early next week

talhaazeemmughal commented 1 month ago

I see! Thanks that helps! Should have mentioned that early on. I ll See if I can upgrade it early next week

@epinault so you understand the issue?

epinault commented 1 month ago

maybe. seem they are pointing to an older version of Redix. I ll track this down and see if I can loosen up or update the version soon

epinault commented 1 month ago

I am looking at the deps I don t see why you have redix 0.11. Hammer redis require 1.1. at a minimum. did you try updating redix ? what is your package version for redix?

talhaazeemmughal commented 1 month ago

I am looking at the deps I don t see why you have redix 0.11. Hammer redis require 1.1. at a minimum. did you try updating redix ? what is your package version for redix?

@epinault i have updated the libraries recently and i am using redix 1.5.1 and i am still facing this issue

epinault commented 1 month ago

your issue is likely related to https://github.com/whatyouhide/redix/issues/240

talhaazeemmughal commented 1 month ago

your issue is likely related to whatyouhide/redix#240

@epinault but it is working fine with exq library as well as redix. It only gives me trouble with hammer.

epinault commented 1 month ago

yea I am not sure what is going on. if you check the init

 def init(args) do
    expiry_ms = Keyword.get(args, :expiry_ms)

    if !expiry_ms do
      raise RuntimeError, "Missing required config: expiry_ms"
    end

    key_prefix = Keyword.get(args, :key_prefix, "Hammer:Redis:")

    redix_config =
      Keyword.get(
        args,
        :redix_config,
        Keyword.get(args, :redis_config, [])
      )

    redis_url = Keyword.get(args, :redis_url, nil)

    {:ok, redix} =
      if is_binary(redis_url) && byte_size(redis_url) > 0 do
        Redix.start_link(redis_url, redix_config)
      else
        Redix.start_link(redix_config)
      end

    delete_buckets_timeout = Keyword.get(args, :delete_buckets_timeout, 5000)

    {:ok,
     %{
       redix: redix,
       expiry_ms: expiry_ms,
       delete_buckets_timeout: delete_buckets_timeout,
       key_prefix: key_prefix
     }}
  end

it s looking pretty correct ot pass options..

can you share snippet of code how you started? I see the config but you seem to override both? and you don t need to do that...

talhaazeemmughal commented 1 month ago

@epinault from what I have understood so far, if i pass verify: :verify_none in socket_opts then it works fine. I also saw that the socket opts are not getting passed when hammer tries to start the redis instance.

talhaazeemmughal commented 1 month ago

@epinault for rest of the libs like exq and redix itself. When i am starting them i am adding socket_opts as:

[
      customize_hostname_check: [
        match_fun: :public_key.pkix_verify_hostname_match_fun(:https)
      ]
    ]
talhaazeemmughal commented 1 month ago

also this is how i started redix itself:

talhaazeemmughal commented 1 month ago

@epinault this is the codebase:

defp configure_redis_ssl(redis_url) do
    socket_opts = redis_socket_opts(redis_url)

    Application.put_env(
      :exq,
      :redis_options,
      socket_opts: socket_opts
    )

    Application.put_env(
      :redix,
      :socket_opts,
      socket_opts
    )

    # Retrieve the current hammer configuration
    current_hammer_config = Application.get_env(:hammer, :backend)

    # Extract and update the `redix_config`
    {backend_module, backend_opts} = current_hammer_config

    # Update the redix_config with socket_opts
    updated_redix_config =
      backend_opts
      |> Keyword.get(:redix_config, [])
      |> Keyword.put(:socket_opts, socket_opts)

    # Rebuild the backend_opts with the updated redix_config
    updated_backend_opts = Keyword.put(backend_opts, :redix_config, updated_redix_config)

    # Put the updated configuration back into the application environment
    Application.put_env(:hammer, :backend, {backend_module, updated_backend_opts})

    :ok
  end

  # If SSL is enabled for the Redis connection, add additional socket_opts
  # in order to support AWS ElastiCache's wildcard SSL certificates
  defp redis_socket_opts("rediss://" <> _rest) do
    [
      customize_hostname_check: [
        match_fun: :public_key.pkix_verify_hostname_match_fun(:https)
      ]
    ]
  end

  defp redis_socket_opts(_non_ssl_url), do: []

I was not doing it for hammer before but it was not working at that time as well and now that i am actually passing it. I think hammer is not using while starting redix instance.

epinault commented 1 month ago

I am a little concern with. -> # Put the updated configuration back into the application environment Application.put_env(:hammer, :backend, {backend_module, updated_backend_opts})

this is an anti pattern in Elixir to be honest. Best is to get the config once from your application, and in your application.ex do the proper passing of the startup

If you look at the code I posted, I don t do anything about socket_opts. And it should be passed like any other fine as I am merely just passing the set of options you pass to Redix itself

what is concerning you are missing bunch of config together ->

Application.put_env(
  :redix,
  :socket_opts,
  socket_opts
)

I would encourage to manage the options a little differently into a config that only read but not put . Redix itself does not need to be configured via the application.put_env

talhaazeemmughal commented 1 month ago

I am a little concern with. -> # Put the updated configuration back into the application environment Application.put_env(:hammer, :backend, {backend_module, updated_backend_opts})

this is an anti pattern in Elixir to be honest. Best is to get the config once from your application, and in your application.ex do the proper passing of the startup

If you look at the code I posted, I don t do anything about socket_opts. And it should be passed like any other fine as I am merely just passing the set of options you pass to Redix itself

what is concerning you are missing bunch of config together ->

Application.put_env(
  :redix,
  :socket_opts,
  socket_opts
)

I would encourage to manage the options a little differently into a config that only read but not put . Redix itself does not need to be configured via the application.put_env

if i remove this

# Retrieve the current hammer configuration
    current_hammer_config = Application.get_env(:hammer, :backend)

    # Extract and update the `redix_config`
    {backend_module, backend_opts} = current_hammer_config

    # Update the redix_config with socket_opts
    updated_redix_config =
      backend_opts
      |> Keyword.get(:redix_config, [])
      |> Keyword.put(:socket_opts, socket_opts)

    # Rebuild the backend_opts with the updated redix_config
    updated_backend_opts = Keyword.put(backend_opts, :redix_config, updated_redix_config)

    # Put the updated configuration back into the application environment
    Application.put_env(:hammer, :backend, {backend_module, updated_backend_opts})

it still gives me issues for hammer and doing that for redix and exq is working fine since they are using the socket_opts that i passed to get connected with the AWS Elasticache instance

epinault commented 1 month ago

what does "current_hammer_config" do?

talhaazeemmughal commented 1 month ago

what does "current_hammer_config" do?

current hammer config is:

config :hammer,
  backend:
    {Hammer.Backend.Redis,
     [
       expiry_ms: :timer.hours(24),
       redix_config: [
         host: System.get_env("REDIS_HOST", "127.0.0.1"),
         port: System.get_env("REDIS_PORT", "6379") |> String.to_integer(),
         password: System.get_env("REDIS_PASSWORD"),
         ssl: System.get_env("REDIS_SSL", "false") == "true"
       ],
       pool_size: 30,
       timeout: 5000
     ]}

i was just passing redis_url before but then added redix_config. I am fetching the backend values and then updating the redix_config with the socket_opts

talhaazeemmughal commented 1 month ago

@epinault Any update on your end?

epinault commented 1 month ago

I don t have much update. I am looking at Redix and the code I pointed out seems to work fine. I have feeling that there is something specific to how you configure it that messes with it like the put_env and how redix would read that on startup

The company I work for uses it in production and we do not see any issues either with connecting

talhaazeemmughal commented 1 month ago

@epinault I found one thing by digging into the hammer code was that I told you initially as well that when hammer is initiating the redix instance. it is not adding this in the socket_opts:

[
      customize_hostname_check: [
        match_fun: :public_key.pkix_verify_hostname_match_fun(:https)
      ]
    ]
epinault commented 1 month ago

I cannot spot the problem at the moment but if you see it, feel free to suggest an MR with a fix .

As you can see the redis backend passes the options straight to redix. So as long as the init gets all the args it should just work.

I am not sure I ll have time today and definitely won't for the next few days due to work commitment. In the meantime showing a repro will speed up the resolution too. I only see snippet of your code and I see things that are a bit of an anti pattern already in the configuration. I want to help but a bit ensure what you see compare to what i see

talhaazeemmughal commented 1 month ago

I cannot spot the problem at the moment but if you see it, feel free to suggest an MR with a fix .

As you can see the redis backend passes the options straight to redix. So as long as the init gets all the args it should just work.

I am not sure I ll have time today and definitely won't for the next few days due to work commitment. In the meantime showing a repro will speed up the resolution too. I only see snippet of your code and I see things that are a bit of an anti pattern already in the configuration. I want to help but a bit ensure what you see compare to what i see

@epinault I will try to get a MR ready for you to have a look. Thank you

talhaazeemmughal commented 1 month ago

@epinault How do you want me to generate a MR? should I fork it? or will you give me access? i am unable to generate a MR

epinault commented 1 month ago

@talhaazeemmughal the proper way is fork it and propose a pull request, like all OSS on github. Also https://github.com/ExHammer/hammer-backend-redis/blob/master/CONTRIBUTING.md is documented here.

ruslandoga commented 1 week ago

👋

Just wanted to mention that in #56 we would start supporting all Redix options.

defmodule MyApp.RateLimit do
  use Hammer, backend: Hammer.Redis
end

redix_opts = [
  host: "example.com",
  port: 5050,
  ssl: true,
  socket_opts: [
    customize_hostname_check: [
      match_fun: :public_key.pkix_verify_hostname_match_fun(:https)
    ]
  ]
]

poolboy_opts = [size: 10, max_overflow: 2]
MyApp.RateLimit.start_link(redix_opts ++ poolboy_opts)