OCPBUGS-10695: Use subnet mask to detect IP stack #227
Conversation
|
@mkowalski: This pull request references Jira Issue OCPBUGS-10695, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (jhajyahy@redhat.com), skipping review request. The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/cc @cybertron |
|
Skipping CI for Draft Pull Request. |
54bd167 to
e29a176
Compare
|
/test all |
|
/retest |
pkg/utils/network.go
Outdated
| if IsIPv4(network.IP) { | ||
| // Sadly the fact that address has "." doesn't mean it's an IPv4 address. We need to check | ||
| // subnet masks to make sure this is really the case. If subnet mask is longer than 32 we | ||
| // are for sure dealing with IPv6 address. If it is longer, then we need to look at the |
There was a problem hiding this comment.
Should this say "If it is shorter"?
| // Sadly the fact that address has "." doesn't mean it's an IPv4 address. We need to check | ||
| // subnet masks to make sure this is really the case. If subnet mask is longer than 32 we | ||
| // are for sure dealing with IPv6 address. If it is longer, then we need to look at the | ||
| // total capacity of the mask as it can be simply an IPv4 address but could be as well an |
There was a problem hiding this comment.
I don't think we actually support less than /64 masks. I guess it doesn't hurt to check, but I wouldn't expect it to work anyway.
pkg/utils/network.go
Outdated
| if m > 8*net.IPv4len { | ||
| return true | ||
| } else { | ||
| if n > 8*net.IPv4len { |
There was a problem hiding this comment.
Why can't we just check total length in the first place? I know you mentioned some weirdness with the net library and mask lengths, but I'm trying to understand how we could ever have m > 32 and not have n > 32. We can't have more ones in the mask than we have total bits, right?
If the really problematic case here is something like ::ffff:1.1.1.1/16 where it's a v4-wrapped v6 address in a v4-like subnet mask, that still needs to be treated as a v6 address IIUC.
That said, you've done a lot more experimentation with this so maybe I'm missing a case because things are not sane when it comes to these addresses. :-)
There was a problem hiding this comment.
I'm trying to understand how we could ever have m > 32 and not have n > 32. We can't have more ones in the mask than we have total bits, right?
Of course; thanks to this I can merge two branches into one
Why can't we just check total length in the first place?
In principle we can. Input to this code can be either netlink library or net.parse* functions. As I can see from both, for IPv4 they will encode mask using 32 bits and for IPv6 they will encode it using 128. The only worrisome combination for me was a vanilla IPv4 address with mask encoded in 128 bits, but this just simply doesn't seem to happen. If it ever would, then our logic will treat such an address as IPv6. On the other hand it then becomes practically indistinguishable from IPv4-mapped-to-IPv6 so we are back at square 1.
Overall, I simplified the if-statements
e29a176 to
06e145b
Compare
This PR hardens the logic of selecting node IP by adding more sophisticated way of detecting IPv6 addresses. This is because there is a class of IPv4-mapped-to-IPv6 addresses which are represented in the same way as vanilla IPv4 addresses when using net.IP structures. In order to make our logic smarter, we now analyse subnet mask to decide if an address is IPv6 or IPv4. This is because we may have an address `::ffff:192.168.0.160/64` which contains both `:` and `.` making the regular logic unreliable. Knowing that the biggest mask for IPv4 is /32 we may detect if such an address is an IPv4-mapped-to-IPv6 address or vanilla IPv4. To solve the case of IPv4-mapped-to-IPv6 address in a huge v6 subnet (e.g. `::ffff:192.168.0.160/16`) we are also checking for the capacity of the struct storing subnet mask. Contributes-to: OCPBUGS-10695
06e145b to
dbe4cc3
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cybertron, mkowalski The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required |
|
@mkowalski: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@mkowalski: Jira Issue OCPBUGS-10695: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged: These pull request must merge or be unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-10695 has not been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This PR hardens the logic of selecting node IP by adding more sophisticated way of detecting IPv6 addresses. This is because there is a class of IPv4-mapped-to-IPv6 addresses which are represented in the same way as vanilla IPv4 addresses when using net.IP structures.
In order to make our logic smarter, we now analyse subnet mask to decide if an address is IPv6 or IPv4. This is because we may have an address
::ffff:192.168.0.160/64which contains both:and.making the regular logic unreliable. Knowing that the biggest mask for IPv4 is /32 we may detect if such an address is an IPv4-mapped-to-IPv6 address or vanilla IPv4.To solve the case of IPv4-mapped-to-IPv6 address in a huge v6 subnet (e.g.
::ffff:192.168.0.160/16) we are also checking for the capacity of the struct storing subnet mask.Contributes-to: OCPBUGS-10695