search

tompave / fun_with_flags

Feature Flags/Toggles for Elixir
https://hexdocs.pm/fun_with_flags/FunWithFlags.html
MIT License
1.08k stars 79 forks source link

Add support for Redis config in the tuple form #145

Closed iamvery closed 2 years ago

iamvery commented 2 years ago

Problem

We have an app that's deployed on Fly.io. In order to connect to a Redis database provided by upstash.com, we must include the socket_opts: [:inet6] option in our Redis config. Unfortunately, the two configuration formats supported by this library do not enable us to easily configure app in this way.

  1. The URI-only form, e.g. config :fun_with_flags, :redis, {:system, "REDIS_URI"} does not give us the ability to specify the socket options
  2. The opt-list form, e.g. config :fun_with_flags, :redis, host: ..., username: ..., password: ..., socket_opts: [:inet6] requires us to parse our Redis URI and then the user/pass part in order to configure the Redis connection

Solution

Add support for the Redix configuration in the form of {url, connection_options} so that we can configure apps directly within the constraints of its environment.

Note

I noticed that the URI-only form appears to exercise an untested code path. Assuming I am seeing that correctly, I opted to add my own new codepath without a test. I realize that this is somewhat risky, so please advise if there is a way to ensure the new code path is tested.

tompave commented 2 years ago

Hello, thank you for reporting the issue and opening the PR!

I want to make sure I understand the problem correctly. You say:

Unfortunately, the two configuration formats supported by this library do not enable us to easily configure app in this way. ...

  1. The opt-list form, e.g. config :fun_with_flags, :redis, host: ..., username: ..., password: ..., socket_opts: [:inet6] requires us to parse our Redis URI and then the user/pass part in order to configure the Redis connection ... Add support for the Redix configuration in the form of {url, connection_options} so that we can configure apps directly within the constraints of its environment. (emphasis added)

Does that mean that technically you can configure the package to use your Redis, but your vendor (upstash.com) gives you the connection info as a URL string, and you wish to configure it using that URL string to keep your config code simple? What would the alternative be? A call to URI.parse/1 inside one of your config .exs files?

If that's the case, I'm a bit hesitant. It sounds like you're suggesting to push complexity into the package in order to keep your config simple. Perhaps I'm missing something, in that case please clarify.

It would also be helpful if you could provide a link to the redix documentation (or any of the lower-level packages and libs it uses) where it explains what the extra option does, as that's something we'd have to document somewhere here in FWF.

On this:

I noticed that the URI-only form appears to exercise an untested code path. Assuming I am seeing that correctly, I opted to add my own new codepath without a test. I realize that this is somewhat risky, so please advise if there is a way to ensure the new code path is tested.

Good spot. The different ways to configure Redis are tested in the Config module. That is, the package tests that they all produce something that can then be passed to Redix. Then the Persistent.Redis module during a test run uses the Redis config for the test env, which is just a URL. Basically the alternative would be to re-run some of the Redis tests all the different ways to configure the Redis connection, and I decided to avoid that complexity and place a lot of trust that the redix package would work.

On this:

I had some difficulty running the entire suite

If there is a problem running the tests, I'd like to figure it out and fix it. At the moment "they run on my machine" 😅. Quick sanity check: have you followed the instructions in the readme? If you have Postgres, MySQL and Redis running on localhost on their default ports, you should be able to just run mix test.all without issues. If you want you can also run the test suite targeting only the Redis configuration, with mix test.redis. The tests are 99% the same, just the config changes.

iamvery commented 2 years ago

Thank you so much for the quick response to this issue! And thank you for the great questions.

Does that mean that technically you can configure the package to use your Redis, but your vendor (upstash.com) gives you the connection info as a URL string, and you wish to configure it using that URL string to keep your config code simple? What would the alternative be? A call to URI.parse/1 inside one of your config .exs files?

If that's the case, I'm a bit hesitant. It sounds like you're suggesting to push complexity into the package in order to keep your config simple. Perhaps I'm missing something, in that case please clarify.

That is exactly correct, and I can understand your hesitation. However, in my experience URI-based configurations are common and in many cases standard. Here's the solution we're using for our app.

uri = System.get_env("REDIS_URI") |> URI.parse()
[user, pass] = String.split(uri.userinfo, ":")

config :fun_with_flags, :redis,
  host: uri.host,
  username: user,
  password: pass,
  socket_opts: [:inet6]

It reads to me like necessary app complexity for a common problem the solution of which is already supported by the underlying Redis library.

It would also be helpful if you could provide a link to the redix documentation (or any of the lower-level packages and libs it uses) where it explains what the extra option does, as that's something we'd have to document somewhere here in FWF.

I apologize for not including this in the original message. The particular issue that we resolved using these additional options is described here https://github.com/whatyouhide/redix/issues/222. My understanding is that because Fly/Upstash using IPv6, the option must be passed along to the lower-level networking tooling in order to make a connection.

More information:

Basically the alternative would be to re-run some of the Redis tests all the different ways to configure the Redis connection, and I decided to avoid that complexity and place a lot of trust that the redix package would work.

That makes a ton of sense to me, and I appreciate the tradeoff. I just wanted to mention it to note that there's always a possibility of issues cropping up through untested code paths (of which I was adding another).

If there is a problem running the tests, I'd like to figure it out and fix it. At the moment "they run on my machine" 😅. Quick sanity check: have you followed the instructions in the readme? If you have Postgres, MySQL and Redis running on localhost on their default ports, you should be able to just run mix test.all without issues. If you want you can also run the test suite targeting only the Redis configuration, with mix test.redis. The tests are 99% the same, just the config changes.

No, admittedly I saw the amount of setup required and balked. However, I am happy to do my due diligence and give this a try. I would expect the same of anyone I work with. I will report back with the results soon.

Fwiw

All this did strike me as a little strange as to not have come up yet. I haven't been using Elixir in production for a very long time, so I've certainly considered whether this is a case of beginner inexperience. That said, it does feel like a real problem within the confines of my current understanding, so I wanted to start the conversation to either learn or contribute.

Thanks again for your thoughtful response and collaboration on this issue!

iamvery commented 2 years ago

Update: I was able to get the full test suite running by following your instructions along with a couple small changes:

  1. I had to remove the MySQL password as the default brew installation did not include a password.
  2. I had to address a Redis misconfiguration (details).

There was one failing test, but it appears to be an ordering issue, not related to the changes in this PR. Here's the full output: https://gist.github.com/iamvery/68e2f803a5aa2175b8cfa0b51252271c (failure here).

tompave commented 2 years ago

However, in my experience URI-based configurations are common and in many cases standard. Here's the solution we're using for our app.

I see. Yes, that's what I had in mind.

I think you make some good points. I'm convinced, thank you for insisting. Let's get this merged!

All this did strike me as a little strange as to not have come up yet. I haven't been using Elixir in production for a very long time, so I've certainly considered whether this is a case of beginner inexperience. That said, it does feel like a real problem within the confines of my current understanding, so I wanted to start the conversation to either learn or contribute.

You mean the IPv6 issue? I think that most projects use FWF with Ecto because they use Phoenix and that's easier to setup, so the Redis adapter is less battle tested than the Ecto one. I might be wrong.

If you mean the issue with configuring {"redis_url", [opts..]}, I still think that it wasn't really a blocker. Here we're just polishing the UX of the configuration API and surfacing something that Redix supports.

tompave commented 2 years ago

This has been released on Hex with version 1.10.0.

iamvery commented 2 years ago

Thank you so much! I really appreciate the time and thought that you gave to this issue. I will update our system with this change as one more point of data for the use case :) Blessings to you ❤️

iamvery commented 2 years ago

And thank you so much for taking care of the documentation update. I just realized that I should have included that, but you already got it :) https://github.com/tompave/fun_with_flags/commit/8baaa5cf0e3c3b6f2c09f339c5aabb5734e3ef51