verdi setup: improve validation and help string of broker virtual host
#4408
Conversation
The help string of the `--broker-virtual-host` option of `verdi setup` incorrectly said that forward slashes have to be escaped but this is not true. The code will escape any characters necessary when constructing the URL to connect to RabbitMQ. On top of that, slashes would fail the validation outright, even though these are common in virtual hosts. For example the virtual host always starts with a leading forward slash, but our validation would reject it. Also the leading slash will be added by the code and so does not have to be used in the setup phase. The help string and the documentation now reflect this. The exacti naming rules for virtual hosts, imposed by RabbitMQ or other implemenatations of the AMQP protocol, are not fully clear. But instead of putting an explicit validation on AiiDA's side and running the risk that we incorrectly reject valid virtual host names, we simply accept all strings. In any case, any non-default virtual host will have to be created through RabbitMQ's control interface, which will perform the validation itself.
Codecov Report
@@ Coverage Diff @@
## develop #4408 +/- ##
===========================================
+ Coverage 79.23% 79.23% +0.01%
===========================================
Files 475 475
Lines 34827 34827
===========================================
+ Hits 27590 27592 +2
+ Misses 7237 7235 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -315,10 +315,10 @@ def decorator(command): | |||
|
|
|||
| BROKER_VIRTUAL_HOST = OverridableOption( | |||
| '--broker-virtual-host', | |||
| type=types.HostnameType(), | |||
| type=click.types.StringParamType(), | |||
There was a problem hiding this comment.
Just to understand: Didn't the HOSTNAME_REGEX already exclude leading slashes?
https://github.com/sphuber/aiida-core/blob/dbbea0e3582894f8ef88e0f534db7560a1d8994d/aiida/cmdline/params/types/strings.py#L62
Why not keep the HostnameType?
There was a problem hiding this comment.
Just to understand: Didn't the HOSTNAME_REGEX already exclude leading slashes?
Yes, which is exactly the problem. A virtual host with one or more slashes is perfectly valid. See also my commit message:
On top of that, slashes would fail the validation outright, even though these are common in virtual hosts.
In the final paragraph of the commit message I explain why we resort to a type with as little restrictions as possible, namely a simple string type. I checked the RabbitMQ docs for virtual host naming rules but couldn't find it, so I asked on the mailing list and experimented: https://groups.google.com/u/1/g/rabbitmq-users/c/ZT56iwGbxy0 The last response is what I found by experimenting, almost anything goes. So I think it is best not to restrict anything in our interface.
There was a problem hiding this comment.
Yes, which is exactly the problem. A virtual host with one or more slashes is perfectly valid.
Well, I was going by the help string, in which you ask people not to add the leading slash.
But I see now that these virtual hosts can contain almost any string anywhere, thanks for the info!
| @@ -315,10 +315,10 @@ def decorator(command): | |||
|
|
|||
| BROKER_VIRTUAL_HOST = OverridableOption( | |||
| '--broker-virtual-host', | |||
| type=types.HostnameType(), | |||
| type=click.types.StringParamType(), | |||
There was a problem hiding this comment.
Yes, which is exactly the problem. A virtual host with one or more slashes is perfectly valid.
Well, I was going by the help string, in which you ask people not to add the leading slash.
But I see now that these virtual hosts can contain almost any string anywhere, thanks for the info!
…ost (aiidateam#4408) The help string of the `--broker-virtual-host` option of `verdi setup` incorrectly said that forward slashes have to be escaped but this is not true. The code will escape any characters necessary when constructing the URL to connect to RabbitMQ. On top of that, slashes would fail the validation outright, even though these are common in virtual hosts. For example the virtual host always starts with a leading forward slash, but our validation would reject it. Also the leading slash will be added by the code and so does not have to be used in the setup phase. The help string and the documentation now reflect this. The exacti naming rules for virtual hosts, imposed by RabbitMQ or other implemenatations of the AMQP protocol, are not fully clear. But instead of putting an explicit validation on AiiDA's side and running the risk that we incorrectly reject valid virtual host names, we simply accept all strings. In any case, any non-default virtual host will have to be created through RabbitMQ's control interface, which will perform the validation itself.
Fixes #4403
The help string of the
--broker-virtual-hostoption ofverdi setupincorrectly said that forward slashes have to be escaped but this is not
true. The code will escape any characters necessary when constructing
the URL to connect to RabbitMQ. On top of that, slashes would fail the
validation outright, even though these are common in virtual hosts. For
example the virtual host always starts with a leading forward slash, but
our validation would reject it. Also the leading slash will be added by
the code and so does not have to be used in the setup phase. The help
string and the documentation now reflect this.
The exact naming rules for virtual hosts, imposed by RabbitMQ or other
implementations of the AMQP protocol, are not fully clear. But instead
of putting an explicit validation on AiiDA's side and running the risk
that we incorrectly reject valid virtual host names, we simply accept
all strings. In any case, any non-default virtual host will have to be
created through RabbitMQ's control interface, which will perform the
validation itself.