You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@qpid.apache.org by Andrew Stitcher <as...@apache.org> on 2013/01/14 21:35:28 UTC

Review Request: Improve qpid-route handling for interbroker ssl connections

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8943/
-----------------------------------------------------------

Review request for qpid, Gordon Sim and Ted Ross.

Description
-------

This change addresses 2 issues I found while using qpid-route to setup and inspect ssl federation routes:

1. It allows the source URL to use the amqps scheme and for this to be interpreted to use the ssl transport between the brokers.

2. It allows qpid-route itself to deal sensibly if it gets an amqps scheme from a broker during the "qpid-route route map" command.

I think the fix to 2. is a clear improvement over the previous behaviour as now qpid-route can at least try to contact the broker at the end of the URL (even if it won't always succeed due to authentication failure). Before it would always try to use the ssl port with a tcp transport which would almost never succeed (unless the broker was using multiplexed TCP/SSL).

It's not clear to me if the change to fix 1. is good or not. As it relies on the user understanding that that the Source URL is really the URL used by the destination to contact the source and so is the actual federation link. This is really not clear from the command itself. You can still use the --transport option to specify the protocol, so this is an additional way to specify the link.

So I'm seeking some confirmation (or not) that I should commit this change.

Diffs
-----

/trunk/qpid/tools/src/py/qpid-route 1433061

Diff: https://reviews.apache.org/r/8943/diff/

Testing
-------

Thanks,

Andrew Stitcher

Re: Review Request 8943: Improve qpid-route handling for interbroker ssl connections

Posted by Kenneth Giusti <kg...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8943/#review22509
-----------------------------------------------------------

Ship it!


I think this change is an improvement, but I'll raise a couple of points:

1) As the patch stands, if the "amqps:" scheme is given in the address, the '--transport' option is silently ignored.  Operationally, this is probably harmless, as specifying an "amqps:" url with '--transport=tcp' probably doesn't make sense.  Should --transport be ignored if a scheme is explicitly provided?  Should this raise an input error instead?

2) IIRC, we don't have decent test coverage of the Federation + SSL case.  Ideally this change should have a unit test, but that would entail writing more test code than the patch itself :I (or at least raise a JIRA to add such tests)

- Kenneth Giusti


On June 27, 2013, 8:19 p.m., Andrew Stitcher wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8943/
> -----------------------------------------------------------
> 
> (Updated June 27, 2013, 8:19 p.m.)
> 
> 
> Review request for qpid, Gordon Sim, Kenneth Giusti, and Ted Ross.
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> This change addresses 2 issues I found while using qpid-route to setup and inspect ssl federation routes:
> 
> 1. It allows the source URL to use the amqps scheme and for this to be interpreted to use the ssl transport between the brokers.
> 
> 2. It allows qpid-route itself to deal sensibly if it gets an amqps scheme from a broker during the "qpid-route route map" command.
> 
> I think the fix to 2. is a clear improvement over the previous behaviour as now qpid-route can at least try to contact the broker at the end of the URL (even if it won't always succeed due to authentication failure). Before it would always try to use the ssl port with a tcp transport which would almost never succeed (unless the broker was using multiplexed TCP/SSL).
> 
> It's not clear to me if the change to fix 1. is good or not. As it relies on the user understanding that that the Source URL is really the URL used by the destination to contact the source and so is the actual federation link. This is really not clear from the command itself. You can still use the --transport option to specify the protocol, so this is an additional way to specify the link.
> 
> So I'm seeking some confirmation (or not) that I should commit this change.
> 
> 
> Diffs
> -----
> 
>   /trunk/qpid/tools/src/py/qpid-route 1433061 
> 
> Diff: https://reviews.apache.org/r/8943/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrew Stitcher
> 
>

Re: Review Request 8943: Improve qpid-route handling for interbroker ssl connections

Posted by Andrew Stitcher <as...@apache.org>.

(Updated June 27, 2013, 8:19 p.m.)

Review request for qpid, Gordon Sim, Kenneth Giusti, and Ted Ross.

Changes
-------

I think this small change is still relevant, could someone have a quick look and say what they think.

Repository: qpid