Fix IP address collection with IPv6 disabled #3190
Conversation
jirihnidek
left a comment
jirihnidek
left a comment
There was a problem hiding this comment.
Thanks for the PR. I miss some unit tests for this case and instructions for disabling/enabling IPv6 in the commit message or comment of PR. Something like:
# Disable IPv6
sudo sysctl -w net.ipv6.conf.all.disable_ipv6=1
sudo sysctl -w net.ipv6.conf.default.disable_ipv6=1
# Enable IPv6 again
sudo sysctl -w net.ipv6.conf.all.disable_ipv6=0
sudo sysctl -w net.ipv6.conf.default.disable_ipv6=0
And I have some other requests. More details in comments.
src/rhsmlib/facts/hwprobe.py
Outdated
| if ipv6_addresses: | ||
| net_info["network.ipv6_address"] = ", ".join(ipv6_addresses) | ||
| else: | ||
| log.debug(f"Error getting IPv6 addresses of {hostname} using socket inspection.") |
There was a problem hiding this comment.
I would not scare users with any message containing word: "Error"
| net_info["network.ipv6_address"] = ", ".join(ipv6_addresses) | ||
| else: | ||
| log.debug(f"Error getting IPv6 addresses of {hostname} using socket inspection.") | ||
| net_info["network.ipv6_address"] = ", ".join(self._get_ipv6_addr_list()) |
There was a problem hiding this comment.
When IPv6 is completely disabled, then the list of addresses still contains ::1 address, which is not correct. When IPv6 is disabled, then there should not be any IPv6 address in the list.
There was a problem hiding this comment.
I can't reproduce this. When I disable IPv6, no v6 addresses are listed, on both bare metal and in container.
There was a problem hiding this comment.
It is strange. I can see: network.ipv6_address: ::1 in system facts, because of this:
It is strange, because loopback interface contains only IPv4 addresses:
net.interface.lo.ipv4_address: 127.0.0.1
net.interface.lo.ipv4_address_list: 127.0.0.1
net.interface.lo.ipv4_broadcast: Unknown
net.interface.lo.ipv4_broadcast_list: Unknown
net.interface.lo.ipv4_netmask: 8
net.interface.lo.ipv4_netmask_list: 8
It was probably me, who has implemented that, but I think that it is wrong now, because we should not fabricate any non-existent facts in the code.
There was a problem hiding this comment.
Maybe we could fix it in other PR. It is only little bit related to this case. When I was talking about unit testing, then I had this network.ipv6_address: ::1 in my mind.
There was a problem hiding this comment.
I discovered it as well, the two paths could produce different facts (one with loopback, one without). I'd like to fix this later, either in following PR or in the upcoming re-implementation using ip.
When IPv6 is disabled, this code crashed Python with segfault, instead of rising an exception. This patch iterates over interfaces found by generic 'getaddrinfo()' call instead of querying v4 and v6 individually. To test this, you can disable IPv6 by calling # sysctl -w net.ipv6.conf.all.disable_ipv6=1 # sysctl -w net.ipv6.conf.default.disable_ipv6=1 (and after testing, enable it by passing 0s instead of 1s), or, when using containers, add --sysctl net.ipv6.conf.all.disable_ipv6=1 to podman/docker arguments.
|
I'm not sure how to unittest this. This PR targets some weird not-easily-reproducible issues we had that were causing Python to crash instead of raising OSError; you can't mock that. This is rather a workaround: proper solution is to switch all of this for |
m-horky
force-pushed
the
mhorky/ipv6-address-collection
branch
from
2e5e5f9 to
112880f
Compare
Coverage (computed on Fedora latest) •
|
||||||||||||||||||||||||||||||
|
TL;DR: I don't think we should include this patch. I did some testing of this patch, and I noticed the log message
The "either ... or" part is the key here: on a system that has both IPv4 and IPv6 set up, IPv4 is found as usable, and thus all the results of Hence, I don't think this change makes a lot of difference in terms of "code flow". The only problem here is Python 3.9 crashing, and I think it was fixed in stable branches of Python 3.10+ with python/cpython#101252 -- I will need to check that out. Also, I'm surprised nobody actually had/reported issues when registering a RHEL 9 system without IPv6... [1] https://man7.org/linux/man-pages/man3/getaddrinfo.3.html |
|
As a result of the problems, I'm closing this PR in favor of future re-implementation. |
When IPv6 is disabled, this code crashed Python with segfault, instead of rising an exception. This patch iterates over interfaces found by generic 'getaddrinfo()' call instead of querying v4 and v6 individually.