-
Notifications
You must be signed in to change notification settings - Fork 45
Fix AXIS2-5858 #842
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix AXIS2-5858 #842
Conversation
|
@robertlazarski can you restart the build? From the stacktrace it seems like this to just be some dns resolution failure, but I don't see how I can restart the build. |
|
If possible I'd like to also add tests for this, but I'm unsure where/how. If someone has an idea that would be great. |
|
There seems to be a problem in the |
|
@cortlepp I reviewed the builds and code, with a conclusion that map(InetAddress::getHostAddress) instead of map(InetAddress::toString) in getIpAddress() will fix the issue as the latter includes the hostname or simply / as a leading char. The logging of unit tests can be problematic, so I temporarily insert System.out.println() statements and found the problem that way. |
|
@robertlazarski Thanks so much for looking into this! Do you perhaps have any idea how/where I can add a good test for this? |
|
See this print statement that you can see in the unit test logs at modules/transport/tcp/target/surefire-reports/org.apache.axis2.transport.tcp.TCPEchoRawXMLTest-output.txt |
|
@robertlazarski I'm not quite sure if I understand what you're saying: does the new |
|
@cortlepp this Linux command below shows a localhost ipv6 address and an ipv6 address assigned from a router. ip a | grep inet6 inet6 ::1/128 scope host noprefixroute That last IP should be returned in the List from this new method as one of the IP's, correct? While I did not commit those println statements in the unit tests, I use them liberally while debugging Axis2 unit tests and in the right place you can see the output in the unit tests as described. I saw my ipv6 address assigned from my router in the beginning of the method but somehow it got filtered out. Your code does return an ipv4 address as the first in the List. From what I understand of the code, the List should contain my ipv6 address from my router but probably the first entry should be ipv4 as the priority? I suppose the point here is that if there was no ipv4 address on my localhost and only ipv6, that would be returned. |
Well, yes, but actually no (I think). The current implementation was somewhat incorrect because it checked the site-local attribute for IPv6 which has been deprecated. Your address there is a unique local address which (if I understood the definition correctly) is also not something we want. In the revised implementation I am filtering these out, but because the standard library does not have this check implemented I had to do this myself.
I'm guessing the order of network interfaces is jvm/platform specific, the revised code returns an IPv6 address first on my machine. Not sure whether that is wise, or if we should prioritize (for backwards compatibility) IPv4 addresses. Maybe it would also be good to respect The revised implementation on my machine returns 3 IP addresses, one IPv4 and two IPv6 (and according to my router these addresses are also actually being used). |
995e26f to
1cfb14f
Compare
|
@robertlazarski I thought a bit more about this decided that my special case for IPv6 unique local addresses might not be correct, so I removed it. I also implemented that by default we prefer IPv4 addresses of IPv6 addresses for backwards compatibility. Please do give this another try, but I think this should be safe to merge now. Nothing changes if you still have a usable IPv4 address and for IPv6 users this PR should provide at least some initial support. |
|
This looks great. Thanks @cortlepp for sticking with this and coming up with code good enough to merge. |
See discussion in Jira.