Small bug in IsIPInSubnet at pkg/unikontainers/unikernels/linux.go which I noticed when looking over the Linux unikernel boot configuration. Wanted to bring it up here before opening a PR in case I'm missing something obvious.
This function takes the container IP, gateway IP, and subnet mask as strings and returns whether they are all in the same subnet. The caller then uses this to decide whether to add the (URUNIT_DEFROUTE=1) kernel command line option which was added in #284 to deal with the case of k8s where IP and gateway can be in different subnets.
The bug: the function parses each string as an IP address using net.ParseIP, and doesn't check if it's nil on invalid input. Due to how the Go net package implements operations on nil IPs, all operations yield nil so the final comparison .Equal returns true if any input was malformed. So invalid/missing input like an empty mask string, a typo, an IPv6 literal, or a CIDR-style string passed instead of a dotted-decimal mask will quietly give the answer "yes, same subnet" and therefore the (URUNIT_DEFROUTE=1) flag won't be set.
The specific case that breaks: The setup that is supposed to use the flag, e.g. A k8s pod whose IP and gateway are in different subnets (e.g. CNI default setup with gateway on 169.254.1.1), passes in a malformed mask for whatever reason. The function will say "same subnet", drop URUNIT_DEFROUTE=1 and the unikernel will boot with no default route, which is precisely what #284 was supposed to fix.
Small bug in
IsIPInSubnetatpkg/unikontainers/unikernels/linux.gowhich I noticed when looking over the Linux unikernel boot configuration. Wanted to bring it up here before opening a PR in case I'm missing something obvious.This function takes the container IP, gateway IP, and subnet mask as strings and returns whether they are all in the same subnet. The caller then uses this to decide whether to add the (
URUNIT_DEFROUTE=1) kernel command line option which was added in #284 to deal with the case of k8s where IP and gateway can be in different subnets.The bug: the function parses each string as an IP address using net.ParseIP, and doesn't check if it's nil on invalid input. Due to how the Go net package implements operations on nil IPs, all operations yield nil so the final comparison .Equal returns true if any input was malformed. So invalid/missing input like an empty mask string, a typo, an IPv6 literal, or a CIDR-style string passed instead of a dotted-decimal mask will quietly give the answer "yes, same subnet" and therefore the (
URUNIT_DEFROUTE=1) flag won't be set.The specific case that breaks: The setup that is supposed to use the flag, e.g. A k8s pod whose IP and gateway are in different subnets (e.g. CNI default setup with gateway on 169.254.1.1), passes in a malformed mask for whatever reason. The function will say "same subnet", drop
URUNIT_DEFROUTE=1and the unikernel will boot with no default route, which is precisely what #284 was supposed to fix.