-
Couldn't load subscription status.
- Fork 142
Support more than one IP per interface and IPv6 for results returned by CNI #478
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -141,19 +141,18 @@ func (networkInterfaces NetworkInterfaces) setupNetwork( | |
| MacAddress: vmNetConf.VMMacAddr, | ||
| } | ||
|
|
||
| if vmNetConf.VMIPConfig != nil { | ||
| for _, vmIPCfg := range vmNetConf.VMIPConfig { | ||
| if len(vmNetConf.VMNameservers) > 2 { | ||
| logger.Warnf("more than 2 nameservers provided from CNI result, only the first 2 %+v will be applied", | ||
| vmNetConf.VMNameservers[:2]) | ||
| vmNetConf.VMNameservers = vmNetConf.VMNameservers[:2] | ||
| } | ||
|
|
||
| cniNetworkInterface.StaticConfiguration.IPConfiguration = &IPConfiguration{ | ||
| IPAddr: vmNetConf.VMIPConfig.Address, | ||
| Gateway: vmNetConf.VMIPConfig.Gateway, | ||
| cniNetworkInterface.StaticConfiguration.IPConfiguration = append(cniNetworkInterface.StaticConfiguration.IPConfiguration, &IPConfiguration{ | ||
| IPAddr: vmIPCfg.Address, | ||
| Gateway: vmIPCfg.Gateway, | ||
| Nameservers: vmNetConf.VMNameservers, | ||
| IfName: cniNetworkInterface.CNIConfiguration.VMIfName, | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -484,16 +483,16 @@ type StaticNetworkConfiguration struct { | |
|
|
||
| // IPConfiguration (optional) allows a static IP, gateway and up to 2 DNS nameservers | ||
| // to be automatically configured within the VM upon startup. | ||
| IPConfiguration *IPConfiguration | ||
| IPConfiguration []*IPConfiguration | ||
| } | ||
|
|
||
| func (staticConf StaticNetworkConfiguration) validate() error { | ||
| if staticConf.HostDevName == "" { | ||
| return fmt.Errorf("HostDevName must be provided if StaticNetworkConfiguration is provided: %+v", staticConf) | ||
| } | ||
|
|
||
| if staticConf.IPConfiguration != nil { | ||
| err := staticConf.IPConfiguration.validate() | ||
| for _, ipCfg := range staticConf.IPConfiguration { | ||
| err := ipCfg.validate() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -522,10 +521,10 @@ type IPConfiguration struct { | |
| } | ||
|
|
||
| func (ipConf IPConfiguration) validate() error { | ||
| // Make sure only ipv4 is being provided (for now). | ||
| // Make sure it is a valid ip. | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a test for this change in |
||
| return fmt.Errorf("invalid ip: %+v", ip) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -538,11 +537,14 @@ func (ipConf IPConfiguration) validate() error { | |
|
|
||
| func (conf IPConfiguration) ipBootParam() string { | ||
| // the vmconf package already has a function for doing this, just re-use it | ||
|
|
||
| vmConf := vmconf.StaticNetworkConf{ | ||
| VMNameservers: conf.Nameservers, | ||
| VMIPConfig: ¤t.IPConfig{ | ||
| Address: conf.IPAddr, | ||
| Gateway: conf.Gateway, | ||
| VMIPConfig: []*current.IPConfig{ | ||
| { | ||
| Address: conf.IPAddr, | ||
| Gateway: conf.Gateway, | ||
| }, | ||
| }, | ||
| VMIfName: conf.IfName, | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,23 +44,27 @@ var ( | |
| kernelArgsWithIP = parseKernelArgs("foo=bar this=phony ip=whatevz") | ||
|
|
||
| // These RFC 5737 IPs are reserved for documentation, they are not usable | ||
| validIPConfiguration = &IPConfiguration{ | ||
| IPAddr: net.IPNet{ | ||
| IP: net.IPv4(198, 51, 100, 2), | ||
| Mask: net.IPv4Mask(255, 255, 255, 0), | ||
| validIPConfiguration = []*IPConfiguration{ | ||
| &IPConfiguration{ | ||
| IPAddr: net.IPNet{ | ||
| IP: net.IPv4(198, 51, 100, 2), | ||
| Mask: net.IPv4Mask(255, 255, 255, 0), | ||
| }, | ||
| Gateway: net.IPv4(198, 51, 100, 1), | ||
| Nameservers: []string{"192.0.2.1", "192.0.2.2"}, | ||
| }, | ||
| Gateway: net.IPv4(198, 51, 100, 1), | ||
| Nameservers: []string{"192.0.2.1", "192.0.2.2"}, | ||
| } | ||
|
|
||
| // IPv6 is currently invalid | ||
| // These RFC 3849 IPs are reserved for documentation, they are not usable | ||
| invalidIPConfiguration = &IPConfiguration{ | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see any tests using this invalid config. Can we add one and also validate IPv6 support? |
||
| &IPConfiguration{ | ||
| IPAddr: net.IPNet{ | ||
| IP: net.ParseIP("2001:db8:a0b:12f0::2"), | ||
| Mask: net.CIDRMask(24, 128), | ||
| }, | ||
| Gateway: net.ParseIP("2001:db8:a0b:12f0::1"), | ||
| }, | ||
| Gateway: net.ParseIP("2001:db8:a0b:12f0::1"), | ||
| } | ||
|
|
||
| validStaticNetworkInterface = NetworkInterface{ | ||
|
|
@@ -99,31 +103,22 @@ func TestNetworkStaticValidationFails_TooManyNameservers(t *testing.T) { | |
| staticNetworkConfig := StaticNetworkConfiguration{ | ||
| MacAddress: mockMacAddrString, | ||
| HostDevName: tapName, | ||
| IPConfiguration: &IPConfiguration{ | ||
| IPAddr: net.IPNet{ | ||
| IP: net.IPv4(198, 51, 100, 2), | ||
| Mask: net.IPv4Mask(255, 255, 255, 0), | ||
| IPConfiguration: []*IPConfiguration{ | ||
| &IPConfiguration{ | ||
| IPAddr: net.IPNet{ | ||
| IP: net.IPv4(198, 51, 100, 2), | ||
| Mask: net.IPv4Mask(255, 255, 255, 0), | ||
| }, | ||
| Gateway: net.IPv4(198, 51, 100, 1), | ||
| Nameservers: []string{"192.0.2.1", "192.0.2.2", "192.0.2.3"}, | ||
| }, | ||
| Gateway: net.IPv4(198, 51, 100, 1), | ||
| Nameservers: []string{"192.0.2.1", "192.0.2.2", "192.0.2.3"}, | ||
| }, | ||
| } | ||
|
|
||
| err := staticNetworkConfig.validate() | ||
| assert.Error(t, err, "network config with more than two nameservers did not result in validation error") | ||
| } | ||
|
|
||
| func TestNetworkStaticValidationFails_IPConfiguration(t *testing.T) { | ||
| staticNetworkConfig := StaticNetworkConfiguration{ | ||
| MacAddress: mockMacAddrString, | ||
| HostDevName: tapName, | ||
| IPConfiguration: invalidIPConfiguration, | ||
| } | ||
|
|
||
| err := staticNetworkConfig.validate() | ||
| assert.Error(t, err, "invalid network config hostdevname did not result in validation error") | ||
| } | ||
|
|
||
| func TestNetworkCNIValidation(t *testing.T) { | ||
| err := validCNIInterface.CNIConfiguration.validate() | ||
| assert.NoError(t, err, "valid cni network config unexpectedly returned validation error") | ||
|
|
@@ -448,12 +443,12 @@ func startCNIMachine(t *testing.T, ctx context.Context, m *Machine) string { | |
| assert.NotEmpty(t, staticConfig.HostDevName, | ||
| "static config should have host dev name set") | ||
|
|
||
| ipConfig := staticConfig.IPConfiguration | ||
| ipConfig := staticConfig.IPConfiguration[0] | ||
| require.NotNil(t, ipConfig, | ||
| "cni configuration should have updated network interface ip configuration") | ||
|
|
||
| require.Equal(t, m.Cfg.NetworkInterfaces[0].CNIConfiguration.VMIfName, | ||
| staticConfig.IPConfiguration.IfName, | ||
| staticConfig.IPConfiguration[0].IfName, | ||
| "interface name should be propagated to static conf") | ||
|
|
||
| return ipConfig.IPAddr.IP.String() | ||
|
|
||
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.
Can we also add a check here to validate
VMIPConfighas only 1 IP here, else error out?