-
Notifications
You must be signed in to change notification settings - Fork 259
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
Add unit tests for assignIPToLink in GCS network setup #2309
base: main
Are you sure you want to change the base?
Add unit tests for assignIPToLink in GCS network setup #2309
Conversation
Signed-off-by: Kathryn Baldauf <[email protected]>
func Test_AssignIPToLink_GatewayOutsideSubnet(t *testing.T) { | ||
ctx := context.Background() | ||
|
||
var assignIPToLinkTestsGateway = []assignIPToLinkTest{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
am I seeing this wrong or is this same as defaultAssignIPToLinkTests
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one has gateway addresses that are outside the subnet of the IP addresses. I figured it was easier to just make new tests than try to change the gateway address based on if the IP was ipv4 or 6.
ifStr: "eth0", | ||
allocatedIP: "192.168.0.5", | ||
gatewayIP: "192.168.0.100", | ||
prefixLen: uint8(24), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Don't need the uint8
qualifier here.
func standardNetlinkAddrAdd(expectedIP string, prefixLen, totalMaskSize int) func(_ netlink.Link, _ *netlink.Addr) error { | ||
return func(link netlink.Link, addr *netlink.Addr) error { | ||
if addr.IP.String() != expectedIP { | ||
return fmt.Errorf("expected to add address %s, instead got %s", expectedIP, addr.IP.String()) | ||
} | ||
expectedMask := net.CIDRMask(prefixLen, totalMaskSize) | ||
if !bytes.Equal(addr.Mask, expectedMask) { | ||
return fmt.Errorf("expected mask to be %s, instead got %s", expectedMask, addr.Mask) | ||
} | ||
return nil | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we only care about inspecting what values are passed to AddrAdd/RuleAdd/RouteAdd, or do we actually need the ability to modify their return values?
If it's the former, I would suggest an alternate approach here which may be more flexible and readable: Have a single set of AddrAdd/RuleAdd/RouteAdd implementations, which add the arguments they were called with to a tracking slice. Then, after assignIPToLink returns, inspect the tracking slice to ensure the right set of calls were made.
This will help getting rid of the awkward variables that track how many times AddrAdd has been called, as instead you can just do some asserts like:
if len(addrAddCalls) != 2 {
t.Failf("AddrAdd called %d times instead of 2", len(addrAddCalls))
}
if addrAddCalls[0].IP.String() != tt.allocatedIP {
t.Errorf("First AddrAdd call has IP %s instead of %s", addrAddCalls[0].IP.String(), tt.allocatedIP)
}
expectedMask := net.CIDRMask(int(tt.prefixLen), tt.totalMaskSize)
if !bytes.Equal(addrAddCalls[0].Mask, expectedMask) {
t.Errorf("First AddrAdd call has mask %s instead of %s", addrAddCalls[0].Mask, expectedMask)
}
if addrAddCalls[1].IP.String() != tt.gatewayIP {
t.Errorf("Second AddrAdd call has IP %s instead of %s", addrAddCalls[1].IP.String(), tt.gatewayIP)
}
expectedMask = net.CIDRMask(tt.totalMaskSize, tt.totalMaskSize)
if !bytes.Equal(addrAddCalls[1].Mask, expectedMask) {
t.Errorf("Second AddrAdd call has mask %s instead of %s", addrAddCalls[1].Mask, expectedMask)
}
You could even generalize further and just add a slice of expected call data to your test matrix, and then validate that the actual calls match that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want both -- though in these tests, the ability to change the return value is not used. It's still in draft, but I completely redid all of these tests for the work to move to HCN v2 endpoints and support custom routes #2320. In that PR, the tests are set up similar to what you have suggested here and there is a case where we modify the return value from a netlink call as well.
The tests in this current PR were meant to add test coverage in the interim while the changes in my draft PR above are tested and reviewed. Would you still like me to modify the tests here with your suggestion?
This PR adds a series of unit tests for the function
assignIPToLink
which is run when setting up the networking for a pod. These tests were inspired by the issue caught here #2305.PS: These tests may need to change somewhat drastically in the near future when we move to use the v2 HNS endpoint.