Fix to allow equality of hostname #3381
Conversation
| @@ -145,7 +145,7 @@ internal class AMQPChannelHandler(private val serverMode: Boolean, | |||
| // Transfers application packet into the AMQP engine. | |||
| is SendableMessageImpl -> { | |||
| val inetAddress = InetSocketAddress(msg.destinationLink.host, msg.destinationLink.port) | |||
| require(inetAddress == remoteAddress) { | |||
| require(inetAddress.hostName == remoteAddress.hostName) { | |||
There was a problem hiding this comment.
I am not totally sure if this is correct and I could do with understanding the scenario in which there are distinct IP addresses resolved from the same name.The original expectation was that you were initiating the link, so the resolution should be consistent. I am also not quite sure that it will work if you have an IPv4 address specified in p2pAddress rather than a DNS name.
Can you do a bit more testing and report back? I am tempted to lose the require test completely if it is unreliable in various valid scenarios.
There was a problem hiding this comment.
Thanks! We'd like to use an AWS Load Balancer to front an EC2 instance (a corda node) for DR and CD. In our scenario, an EC2 Corda node can be given a unique IP on cloud formation, which is why we'd like to always use the load balancer hostname.
Right, this forked require fix is working for us. I also tested it in a deployed region using an IP specified as the p2pAddress (as well as localhost setup). In those cases the InetAddress gets resolved to:
remoteAddress.hostname: ip-10-224-9-82.aws.lmig.com and inetAddress.hostname: ip-10-224-9-82.aws.lmig.com so the require hostName test still works.
However, right, eliminating that require test completely may arguably be better. Thoughts?
There was a problem hiding this comment.
Can you change to code to print a log.debug {} rather than update the require. If the test is dependent upon complex network setup then it probably is't a good test.
|
Thanks @mnesbit, I updated to log.debug {} rather than update the require |
|
Thanks for the contribution, but unfortunately I'm not sure this is the right fix. DNS LB isn't how Corda is meant to be used. You can publish multiple IP addresses in NodeInfo structures. Nodes are supposed to pick one randomly if multiple are provided. I'm not sure how much of this is implemented, though if anything is missing it can only be a few lines of code (e.g. config file parsing). I can imagine that various things may break or not work correctly in future in setups with DNS-LB. |
|
I had a chat with Matthew. Apparently the logging fix is fine as is regardless of the wider issue. |
* Fix to allow equality of hostname * Remove unreliable require test per pull 3381 * Remove unreliable require test per pull 3381
We would like to front our Corda nodes with a hostname as IP addresses could change.
If I specify my p2pAddress as my hostname, after successful startup, I received the "Message for incorrect endpoint" validation error when attempting an IOU in the IOU example.
Liberty Corda Node node.conf:
p2pAddress : "liberty-corda-node-dev.aws.lmig.com:10002"The error was thrown from AMQPChannelHandler.kt, line 148, when doing validation for the AMQ destination:
require(inetAddress == remoteAddress) { "Message for incorrect endpoint" }The InetSocketAddress.java equality returns false because the IP changes. Debugging properties:
inetAddress.hostString: liberty-corda-node-dev.aws.lmig.com,inetAddress.address: liberty-corda-node-dev.aws.lmig.com/10.224.8.233inetAddress.port: 10002inetAddress.isUnresolved: falseremoteAddress.hostString: liberty-corda-node-dev.aws.lmig.comremoteAddress.address: liberty-corda-node-dev.aws.lmig.com/10.224.10.46remoteAddress.port: 10002remoteAddress.isUnresolved: falseSuggesting fix to update the validation to check against resolved hostnames.