amqps is a valid scheme accepted by most RabbitMQ clients

[michaelklishin]

I've noticed that "amqps" is not recognized as a valid scheme. This is wrong: most popular RabbitMQ clients support it. RabbitMQ for Pivotal CF also uses it in VCAP_SERVICES.

[michaelklishin]

Any chance this can be prioritised a bit? It's currently not possible to use this project with RabbitMQ which is configured to use TLS, including on Pivotal CF.

[dsyer]

You said out of band that it isn't just a question of adding "amqps" as a valid prefix. If it really is supported by the Java client (the only one that is relevant here surely), then why is it more complicated?

[garyrussell]

To be honest, it's not clear to me why Spring is parsing the URI at all; simply wire up a rabbit connection factory and inject it into the Spring CCF:

<rabbit:connection-factory id="connectionFactory" connection-factory="rcf" />

<bean id="rcf" class="com.rabbitmq.client.ConnectionFactory">
    <property name="uri" value="amqps://10.0.0.3" />
</bean>

[garyrussell]

Also Spring AMQP 1.4 (will be released right after Spring Framework 4.1.2) adds a FactoryBean to make configuring advanced SSL options on the RCF more "springy", with the SSL properties in a Spring resource.

[michaelklishin]

I'm not sure why this validation is performed. I suspect that both Spring AMQP and RabbitMQ Java client do enough to reject invalid URIs. If not, I'd be happy to make the necessary changes to the Java client.

[markfisher]

In org.springframework.cloud.service.common.AmqpServiceInfo#validateAndCleanUriInfo, the following can most likely be removed:

if (!URI_SCHEME.equals(uriInfo.getScheme())) {
    throw new IllegalArgumentException("wrong scheme in amqp URI: " + uriInfo);
}

[dsyer]

@markfisher that's what I think we all thought, but @michaelklishin told me that it was more complicated. He seems to have some skin in the game and a demo that works, so if he makes the change we will know that it is correct. A pull request should be really easy.

[garyrussell]

Right; that's what I meant by "it's not clear to me why Spring is parsing the URI at all". I suspect Ramnivas did it because the Spring CCF doesn't support URIs at all; you have to configure an RCF and inject it into the CCF. My suggested fix a few comments up is the best IMHO. (I tested it before posting).

[markfisher]

well, the validateAndCleanUriInfo method takes a UriInfo and returns a UriInfo. It is simply validating that all the components of the URI are present and that the path doesn't contain more than one segment. It also provides the default port if no port was provided explicitly. It seems that the strict check for matching "amqp" as the scheme could be replaced with a simpler check that a scheme has been provided. Then let the actual validation of that occur where the URI is later used to establish a connection.

[michaelklishin]

@dsyer the idea I had in mind at first is to simply make "amqps" a valid schema. Removing the validation altogether hasn't crossed my mind. If you folks agree this is the best option, I'm willing to work on a pull request.

[garyrussell]

It also provides the default port if no port was provided explicitly.

Again, why would Spring do that rather then just letting rabbitmq do it?

[markfisher]

@garyrussell good point - I just quickly skimmed through the other *ServiceInfo implementations and most do not do validation, and the default implementation of the validateAndCleanUriInfo method in the UriBasedServiceInfo parent class is a no-op. In any case, the scheme itself could be passed along as is.

This is the method in question: https://github.com/spring-cloud/spring-cloud-connectors/blob/master/spring-cloud-core/src/main/java/org/springframework/cloud/service/common/AmqpServiceInfo.java#L33

[markfisher]

I just submitted this Pull Request: https://github.com/spring-cloud/spring-cloud-connectors/pull/94

[garyrussell]

Unfortunately the RabbitConnectionFactoryCreator also needs to be fixed to pass the URI to the underlying RCF instead of pulling out the component parts.

[markfisher]

@garyrussell I started in that direction but the com.rabbitmq.client.ConnectionFactory does not have a setUri method. So should we call connectionFactory.useSslProtocol() for the amqps case? (that means of course that we do need to check the URI scheme, even if just for this config setting).

[markfisher]

@garyrussell for example (not part of PR yet): https://github.com/markfisher/spring-cloud-connectors/commit/90aae2e6020e6e5e039338e223107d520ce0fb20

[garyrussell]

?

/**
 * Convenience method for setting the fields in an AMQP URI: host,
 * port, username, password and virtual host.  If any part of the
 * URI is ommited, the ConnectionFactory's corresponding variable
 * is left unchanged.  Note that not all valid AMQP URIs are
 * accepted; in particular, the hostname must be given if the
 * port, username or password are given, and escapes in the
 * hostname are not permitted.
 * @param uriString is the AMQP URI containing the data
 */
public void setUri(String uriString)
    throws URISyntaxException, NoSuchAlgorithmException, KeyManagementException
{
    setUri(new URI(uriString));
}

As I mentioned in my other post; that's how I tested it...

<rabbit:connection-factory id="connectionFactory" connection-factory="rcf" />

<bean id="rcf" class="com.rabbitmq.client.ConnectionFactory">
    <property name="uri" value="amqps://10.0.0.3" />
</bean>

[michaelklishin]

@markfisher ConnectionFactory does have a setUri method and it does enable TLS when the schema is "amqps". Is there a typo in your comment?

[garyrussell]

So I think all we need is to create the RCF, setUri(uri), then new CCF(rcf);

[markfisher]

There's no setUri in the version that is pulled in to my workspace for spring-cloud-connectors.

[garyrussell]

Ugh - it's ancient - 2.5.0; should be 3.4.1

[garyrussell]

Eeek - Spring AMQP is 1.0.0 !!!

[dsyer]

I think we need to bump everything and get a 1.2 release out the door (cc @scottfrederick and @chrisjs, who I think is supposed to be freeing up some time to work on this).

[markfisher]

Agreed. As soon as we have a green light to bump spring-amqp (and thus the rabbitmq client), I will update the PR.

[garyrussell]

Current Spring AMQP release is 1.3.6, which will pull in the 3.3.4 amqp-client transitively.

1.4 should be released next week (waiting for Spring 4.1.2 - Spring 4.1 is needed for annotation-based listeners but 1.4 will still work with older Springs if you're not using annotations).

It's quite strange that 1.0.0 was selected as the default but I see there's some gradle magic to pull in newer releases (but I guess the STS tooling is not aware of it)...

    "amqp11" : [springAmqpVersion: "1.1.4.RELEASE"],
    "amqp12" : [springAmqpVersion: "1.2.2.RELEASE"],
    "amqp13" : [springAmqpVersion: "1.3.3.RELEASE"],

[scottfrederick]

Removing the URI scheme validation as in @markfisher's PR is good. The URI scheme is also used for detection of the service type (in addition to service tags, which are unreliable). So it would still be good to change the base code to support more than one scheme for a service type.

[scottfrederick]

@markfisher wrote

Agreed. As soon as we have a green light to bump spring-amqp (and thus the rabbitmq client), I will update the PR.

Who needs to green-light that change? I say go for it. If you can make those changes and update the PR I will work on the change to support more than one URI scheme for detection.

[markfisher]

@scottfrederick I wasn't sure if there was a roadmap plan for upgrading the underlying versions. The current versions are really out-of-date though, so if we can go ahead with that upgrade, it will be better all around (and will make the solution simply a matter of calling setUri instead of what I did in that wip commit I posted above).

[scottfrederick]

@markfisher There's not much of a roadmap currently. I think we should get the Rabbit/AMQP stuff up to date with current releases now and deal with other spring.io dependencies when we have a better overall plan.

[scottfrederick]

@michaelklishin The latest commits should fix this issue. Can you test with your use cases and confirm?

[michaelklishin]

@scottfrederick thank you. Will give it a try next week.

[scottfrederick]

@michaelklishin Did you get a chance to verify that this works for you?