Support more than one IP per interface and IPv6 for results returned by CNI #478
Conversation
|
@vvejell1 can you please help review this PR when you have a moment? Thank you. |
th0m
force-pushed
the
tlefebvre/upstream
branch
from
d88daf3 to
b8c8a86
Compare
|
@ginglis13 I am getting build errors that seem unrelated to my change https://buildkite.com/firecracker-microvm/firecracker-go-sdk/builds/3007#01869a5d-b0c1-4d19-ad6f-156ddcc120f3/17-1015 |
|
@ginglis13 can you please have a look? Thanks |
|
@th0m we've got an infrastructure bug we're addressing. As soon as it's resolved we can re-run and continue the review |
|
@th0m I have a fix on CI failure merged into main. You can rebase, push and trigger the build again. |
|
Thanks, build failures are on me now, I will fix them. |
th0m
force-pushed
the
tlefebvre/upstream
branch
2 times, most recently
from
964e4f4 to
e0b0c97
Compare
|
Ready for review @fangn2 thank you. |
network_test.go
Outdated
| err := staticNetworkConfig.validate() | ||
| assert.Error(t, err, "invalid network config hostdevname did not result in validation error") | ||
| } | ||
| // TestNetworkStaticValidationFails_IPConfiguration removed as IPv6 support was added in this fork |
There was a problem hiding this comment.
nit: could you add a note about this to the commit message and/or PR body? not sure we need the comment to remain
There was a problem hiding this comment.
Absolutely, I have just pushed the change.
th0m
force-pushed
the
tlefebvre/upstream
branch
from
e0b0c97 to
3381e0b
Compare
…by CNI Note that the TestNetworkStaticValidationFails_IPConfiguration test was removed since IPv6 support got added. Signed-off-by: Thomas Lefebvre <[email protected]>
th0m
force-pushed
the
tlefebvre/upstream
branch
from
3381e0b to
4df80f5
Compare
|
@ginglis13 I believe this is ready to go. |
| // the first two in the slice will be applied in the VM. | ||
| // - VMDomain, VMSearchDomains and VMResolverOptions will be ignored | ||
| // - Nameserver settings are also only set in /proc/net/pnp. Most applications will thus require | ||
| // /etc/resolv.conf to be a symlink to /proc/net/pnp in order to resolve names as expected. | ||
| func (c StaticNetworkConf) IPBootParam() string { | ||
| // See "ip=" section of kernel linked above for details on each field listed below. |
There was a problem hiding this comment.
Can we also add a check here to validate VMIPConfig has only 1 IP here, else error out?
| for _, ip := range []net.IP{ipConf.IPAddr.IP, ipConf.Gateway} { | ||
| if ip.To4() == nil { | ||
| return fmt.Errorf("invalid ip, only ipv4 addresses are supported: %+v", ip) | ||
| if ip.To4() == nil && ip.To16() == nil { |
There was a problem hiding this comment.
Can we add a test for this change in network_test.go?
| IPAddr: net.IPNet{ | ||
| IP: net.ParseIP("2001:db8:a0b:12f0::2"), | ||
| Mask: net.CIDRMask(24, 128), | ||
| invalidIPConfiguration = []*IPConfiguration{ |
There was a problem hiding this comment.
I don't see any tests using this invalid config. Can we add one and also validate IPv6 support?
Our use case is to:
SetupNetworkHandlerourselves to get the network configuration generated using a CNI pluginfirecracker.SetupNetworkHandlerNameandfirecracker.SetupKernelArgsHandlerNamefrom the list of defaultFcInithandlersWith that in mind we want to be able to have more than one IP on interfaces and to support IPv6, which is what this change intends on achieving while keeping backwards compatibility.