Skip to content
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

Update IP parsing based on netlink address attribute format #19

Merged
merged 4 commits into from
Apr 28, 2020

Conversation

andrewsykim
Copy link
Collaborator

The netlink address attribute returns IPv4 addresses in a way that is not easily consumable by Go's standard net library. For example, an IPv4 address like 10.0.0.1 is received from netlink as:

[]byte{10 0 0 1 0 0 0 0 0 0 0 0 0 0]

which Go's net library considers an IPv6 address based on this check.

Due to this limitation, this PR updates the address family check for destination servers (only in older kernels < 3.18) so that any address attribute value that has 12 leading 0s is considered an IPv4 address, otherwise we assume the address family is IPv6.

…s always existed

Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
Signed-off-by: Andrew Sy Kim <kim.andrewsy@gmail.com>
@@ -351,17 +352,6 @@ func assembleService(attrs []syscall.NetlinkRouteAttr) (*Service, error) {

}

// in older kernels (< 3.18), the svc address family attribute may not exist so we must
// assume it based on the svc address provided.
if s.AddressFamily == 0 {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially added this check cause it seemed harmless but given AddressFamily for IPVS services always existed I think it would be simpler to remove this.

// 10.0.0.1 -> [10 0 0 1 0 0 0 0 0 0 0 0 0 0 0 0]
// For AF_INET6, the full 16 byte array is used:
// 2001:db8:3c4d:15::1a00 -> [32 1 13 184 60 77 0 21 0 0 0 0 0 0 26 0]
func getIPFamily(address []byte) (uint16, error) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aojea @uablrek can I get your review on this? Is this a safe way to determine the address family? It wouldn't account for addresses like 2600:: but that seems okay?

Can't use Go's net package because it expects IPv4 address to be last 4 bytes but netlink returns it as first 4 bytes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for my question but I'm not much familiar with this code, but assuming there is no nat64 or nat46, is not possible to obtain the IP family from the virtual IP or in another place and pass it as parameter, instead of using the destination?

ipvs/netlink.go

Line 156 in e63ad1c

func getIPVSFamily() (int, error) {

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scratch my comment, this #19 (comment) has a point :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah worth clarifying that newer kernels pass the address family attribute so this code path would never run. But for old kernels we need some way to figure out the address family so we know how to parse the address into a net.IP.

@andrewsykim
Copy link
Collaborator Author

Ran tests on CentOS 7 (3.10 kernel):

[root@ipvs-test ipvs]# uname -a
Linux ipvs-test 3.10.0-1062.18.1.el7.x86_64 #1 SMP Tue Mar 17 23:49:17 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

[root@ipvs-test ipvs]# go test ./...
ok  	github.com/moby/ipvs	1.367s
?   	github.com/moby/ipvs/ns	[no test files]

@justincormack
Copy link
Contributor

I mean this seems pretty gross but its only for old kernels, someone should file a bug with Red Hat to add family into their broken franeknkernel.

@thaJeztah
Copy link
Member

ping @arkodg @kolyshkin PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants