Unsupported query parameter sslmode

[darraghenright]

HI there.

This is really just a question at this point.

I deployed a Phoenix/Commanded app to Fly this evening. Fly makes a PG connection string available as an envvar named DATABASE_URL. I used this to configure the EventStore database.

It failed with the following error because of the presence of sslmode in the connection string:

shutdown: failed to start child: CRM.EventStore
        ** (EXIT) an exception was raised:
            ** (ArgumentError) unsupported query parameter `sslmode`
                (eventstore 1.4.1) lib/event_store/config/parser.ex:90: anonymous fn/2 in EventStore.Config.Parser.parse_uri_query/1

A quick look at EventStore.Config.Parser.parse_uri_query/1 reveals that this mode is indeed not supported. Is this an option that should be supported, silently discarded, or continue to raise an exception?

Cheers!

[darraghenright]

Happy to contribute a change for this if it's warranted btw — forgot to mention that!

[mudassirkhan19]

Hey @darraghenright ,
I'm facing the same issue, how did you resolve it?

[darraghenright]

Hi @mudassirkhan19

I totally forgot about this! Yeah, I ended up making a temporary hack to my configuration to address this.

For context, it's part of a Phoenix project and I am deploying to Fly.io (which is why the sslmode parameter is in the connection string in the first place).

I ended up making a change to runtime.exs to just strip the query string out of the connection string supplied by DATABASE_URL and create a new connection string, imaginatively called database_url_without_query_string 😆

if config_env() == :prod do
  database_url =
    System.get_env("DATABASE_URL") ||
      raise """
      environment variable DATABASE_URL is missing.
      For example: ecto://USER:PASS@HOST/DATABASE
      """

# EventStore doesn't like `sslmode` query string
database_url_without_query_string =
    database_url
    |> URI.new!()
    |> then(&%URI{&1 | query: nil, path: "#{&1.path}_events"})
    |> URI.to_string()

  # Configure write model database
  config :crm, CRM.EventStore,
    serializer: Commanded.Serialization.JsonSerializer,
    url: database_url_without_query_string,
    pool_size: String.to_integer(System.get_env("POOL_SIZE") || "10"),
    socket_options: maybe_ipv6

This works for me for now, but since you've reminded me of this, I will try to make the time to submit a PR to address this issue.

[mudassirkhan19]

Thanks for the reply, I think this kind of an approach will work for me.

[dvic]

I think we should add a PR for this, it should be supported right?

[darraghenright]

@mudassirkhan19 Great stuff. Glad it helped.

@dvic Agreed, seems like something that would be worth adding. Especially given popular hosting vendors like Fly are including this property in connection strings. I'd be happy to look into this in due course.

[darraghenright]

Looking at the code in question:

defp parse_uri_query(%URI{query: query}) do
  query
  |> URI.query_decoder()
  |> Enum.reduce([], fn
    {"ssl", "true"}, acc ->
      [{:ssl, true} | acc]

    {"ssl", "false"}, acc ->
      [{:ssl, false} | acc]

    {key, value}, acc when key in ["pool_size", "queue_target", "queue_interval", "timeout"] ->
      [{String.to_atom(key), parse_integer!(key, value)} | acc]

    {key, _value}, _acc ->
      raise ArgumentError, message: "unsupported query parameter `#{key}`"
  end)
end

And looking at the full list of currently recognised query string params documented by PostgreSQL — I wonder if it's worth considering doing something else rather than raising an ArgumentError here?

From this library's perspective, only a subset of all possible params are of interest; i.e: ssl, pool_size, queue_target, queue_interval, timeout. But this is a very small subset of the recognised params, so it's certainly possible for any of those to be present in a supplied connection string.

So maybe instead we could do one of the following:

a. Log and otherwise ignore the unsupported query param because it's not required by this library:

    {key, _value}, acc ->
      Logger.debug("Ignoring unsupported query parameter `#{key}`")
      acc
  end)

b. Just silently ignore any param that's not of interest:

    {_key, _value}, acc ->
      acc
  end)

c. Retain the current raise behaviour as default but allow it to be overridden in configuration/envvar:

    {key, _value}, acc ->
      if ignore_unsupported_query_parameter? do
        acc
      else
        raise ArgumentError, message: "unsupported query parameter `#{key}`"
      end
  end)

d. Last option would of course be, do nothing leave it as is.

---

@slashdotdash sorry about the random ping but I wonder if you'd have an opinion on this? :)

[dvic]

I believe we shouldn't skip "unknown" parameters, especially this ssl param because you expect it to use ssl when you pass this. Depending on the server settings, the connection might fail but it might also succeed and use an non-ssl connection, which is something you might not want.

The function parse_uri_query should allow whatever is listed at lib/event_store/config.ex:70:

@postgrex_connection_opts [
    :username,
    :password,
    :database,
    :hostname,
    :configure,
    :port,
    :types,
    :socket,
    :socket_dir,
    :ssl,
    :ssl_opts,
    :timeout,
    :pool,
    :pool_size,
    :queue_target,
    :queue_interval,
    :socket_options,
    :parameters
  ]

So I think sslmode should be reduced into the ssl_opts option? (it seems erlang, and thus postgrex, only supports :verify_none and :verify_peer, see https://www.erlang.org/doc/man/ssl.html#type-verify_type)

[darraghenright]

@dvic I guess the decision to support any possible well-known options is up to the library owners, but I'd tend to agree with you, especially your nice find of the list of options in config.ex.

As a side note, I noticed that :ssl isn't listed in PostgresSQL's documented list of Parameter Key Words. Thought that was strange.