AMQP URL configuration consistency

[SwooshyCueb]

At present, we construct the AMQP URL from two configuration options, amqp_location and amqp_topic. amqp_location contains the hostname, port (optional), scheme (optional), username (optional), and password (optional).

Qpid Proton's C++ API gives us a better way of specifying username and password, which I have implemented in #105 via two new configuration options. Given that there seem to be multiple standards for supplying username and password in an AMQP URL^[1][2], I think this was the right decision.

For consistency's sake, and to discourage users from specifying credentials as part of the location as is currently done, I suggest we deprecate amqp_location in favor of separate options amqp_scheme, amqp_host, and amqp_port.

[korydraughn]

Sounds reasonable. We have to continue to support the old style until we define a deprecation policy though.

Does your change still support passing all of the bits of information via amqp_location?

[trel]

yes, seems we should honor them in an order, and then trim 2 from the list when the time is right...

1. amqp_scheme, amqp_host, amqp_port, amqp_username, amqp_password (if any are defined, then ignore 2 if it's defined, with a warning logged)
2. amqp_location (possibly with all the things)

[SwooshyCueb]

We have to continue to support the old style until we define a deprecation policy though.

I'm fine with continuing to support amqp_location for some amount of time, but we should log a warning if it is present.

Does your change still support passing all of the bits of information via amqp_location?

Yes, everything can still be passed via amqp_location. I think connection_options takes precedent over the credentials in amqp_location (the documentation isn't super clear on this) so non-empty amqp_user/amqp_password will override any credentials present in amqp_location.

yes, seems we should honor them in an order, and then trim 2 from the list when the time is right...

1. amqp_scheme, amqp_host, amqp_port, amqp_username, amqp_password (if any are defined, then ignore 2 if it's defined, with a warning logged)
2. amqp_location (possibly with all the things)

It's amqp_user, not amqp_username, unless you're suggesting I change it. Also, given that everything except the host is optional, I think we should only ignore amqp_location if amqp_host is present.

[korydraughn]

This is sounding really good.

I'm fine with continuing to support amqp_location for some amount of time, but we should log a warning if it is present.

Absolutely. Let the admins know they need to use the new options.

Yes, everything can still be passed via amqp_location. I think connection_options takes precedent over the credentials in amqp_location (the documentation isn't super clear on this) so non-empty amqp_user/amqp_password will override any credentials present in amqp_location.

Excellent. We should verify whether connection_options takes precedence over the creds in amqp_location before this is merged. That can be added to the README.

Also, given that everything except the host is optional, I think we should only ignore amqp_location if amqp_host is present.

If you only define a host, what does the AMQP/RabbitMQ server do? Does it not care about the user?

[SwooshyCueb]

If you only define a host, what does the AMQP/RabbitMQ server do? Does it not care about the user?

You get an anonymous connection to the default port. It's up to the endpoint to decide what to do with anonymous connections. Our ELK stack is configured to accept them.

(Clarification: the topic is also required)

[trel]

I like all of this.

Definitely not suggesting to change to amqp_username.

[trel]

Should this configuration (or just host) information also live in an array, in service of upcoming work on #106 ?

Consider the flow...

If there is an array/list of connection information (host/port/user/password/etc)
 - Walk the list and attempt to connect to each
 - Fail if the end of the list is reached without a successful connection

If there is only one stanza (current schema), use it.

It could look like this...

a)

"plugin_specific_configuration" : {
    "endpoints": [
        {
            "amqp_user" : "alice"
            "amqp_password" : "apass"
            "amqp_host" : "a.example.org"
            "amqp_port" : "5672"
        },
        {
            "amqp_user" : "bobby"
            "amqp_password" : "bpass"
            "amqp_host" : "b.example.org"
            "amqp_port" : "5672"
        }
     ]
     "amqp_options" : "",
     "amqp_topic" : "audit_messages",
     "pep_regex_to_match" : "audit_.*"
}

OR...

just the hostnames themselves in an array/list...

b)

"plugin_specific_configuration" : {
    "amqp_hosts": [
        "a.example.org",
        "b.example.org"
     ]
     "amqp_user" : "alice"
     "amqp_password" : "apass"
     "amqp_port" : "5672"
     "amqp_options" : "",
     "amqp_topic" : "audit_messages",
     "pep_regex_to_match" : "audit_.*"
}

Then, for backwards compatibility...

• if "amqp_host" is defined, use that and show warning
• if "amqp_location" is defined, use that and show warning

[SwooshyCueb]

My plan for this is similar to your "a" proposal, but with amqp_topic inside the array entries as well. Gives users plenty of flexibility. Additionally, we continue to support reading amqp_location and amqp_topic outside endpoints if endpoints does not exist, and log deprecation warnings. I don't want to support, in any capacity, any of the new endpoint configuration options outside of the endpoints array.

As discussed in standup this morning, I will be removing the implementation of amqp_user and amqp_password from my current PR, so that we can tackle endpoint configuration changes all in one go.

[trel]

cool cool.