Coerce config values to binaries in message retainer

[michaelklishin]

See this mailing list thread.

[acogoluegnes]

Managed to reproduce with this configuration file:

[
  {rabbit,
    [
      {default_vhost, "/"}
    ]
  }
].

Message retainer supervisor fails to start its children (one per virtual host). It gets the list of virtual hosts with rabbit_vhost:list/0. Unfortunately, the harm is already done at that time, the default virtual host is a string in the broker and it's broken, it won't be much useful to convert string-ed virtual hosts to an atom everywhere in rabbit_mqtt_retainer_sup.

We'd better off coercing when inserting those default values.

WDYT @michaelklishin?

[michaelklishin]

@acogoluegnes those values have been strings for close to 10 years. What rabbit_vhost:list/0 can easily be coerced to binaries in the MQTT plugin.

[acogoluegnes]

Sure, but the broker is broken e.g. if we use an Erlang string for the default virtual host. I did a couple of tests with the configuration above on a fresh installation: the management plugin is broken on some pages, a Java client wouldn't connect to the default virtual host, etc.

We can coerce to binaries in the MQTT plugin, it may be working instead of crashing but the rest of the system would have a strange and hard-to-debug behavior in many other places. Hence my suggestion to coerce to binaries some of the default values in the broker itself. We can also coerce in both places, better safe than sorry.

[michaelklishin]

Fair enough, let's do both.

…

On Mon, Dec 5, 2016 at 2:41 PM, Arnaud Cogoluègnes ***@***.*** > wrote: Sure, but the broker is broken e.g. if we use an Erlang string for the default virtual host. I did a couple of tests with the configuration above on a fresh installation: the management plugin is broken on some pages, a Java client wouldn't connect to the default virtual host, etc. We can coerce to binaries in the MQTT plugin, it may be working instead of crashing but the rest of the system would have a strange and hard-to-debug behavior in many other places. Hence my suggestion to coerce to binaries some of the default values in the broker itself. We can also coerce in both places, better safe than sorry. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#86 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAEQvIxo16Q-3VACYJZCe_L3OANXOHbks5rE_h0gaJpZM4I_iR4> .

-- MK Staff Software Engineer, Pivotal/RabbitMQ

[acogoluegnes]

OK, I'll create the issue in the broker repo.