----------------------------------------------------------- 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 > >
