Swiftv1 dual stack prototype #4196
Conversation
estebancams
force-pushed
the
estebanca/dual-stack-prototype
branch
from
4cac76a to
f6977d7
Compare
There was a problem hiding this comment.
Pull request overview
This pull request implements dual-stack (IPv4 + IPv6) support for Swift network containers. The changes introduce IPv6 configuration handling across the CNS (Container Network Service) and CNI (Container Network Interface) layers, enabling network containers to support both IPv4 and IPv6 addresses simultaneously.
Changes:
- Added
IPV6Configurationfield toCreateNetworkContainerRequestfor specifying IPv6 subnet and gateway configuration - Implemented IPv6 validation logic in CNS RestServer for both NodeSubnet and Swift scenarios, including CIDR superset validation for configuration updates
- Extended CNI multitenancy layer to extract and process IPv6 addresses from
SecondaryIPConfigurations, create IPv6 network interfaces, and configure IPv6 routes with proper gateway handling
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| cns/NetworkContainerContract.go | Adds IPV6Configuration field to network container request and SecondaryIPConfigurations to response for dual-stack support; updates validation and string formatting |
| cns/restserver/internalapi.go | Implements IPv6 validation for NodeSubnet and Swift scenarios; adds IPv6 CIDR comparison logic for network container updates |
| cni/network/multitenancy.go | Adds validateAndGetIPv6FromSecondaryIPs function; implements IPv6 configuration extraction and processing from secondary IPs; creates IPv6 network interfaces and routes |
Comments suppressed due to low confidence (2)
cns/NetworkContainerContract.go:126
- The design uses two different mechanisms for IPv6 configuration: IPV6Configuration field in CreateNetworkContainerRequest and SecondaryIPConfigurations array in GetNetworkContainerResponse. This dual approach may confuse developers. Consider adding documentation explaining when each is used and how they relate to each other, or consider unifying the approach if possible.
SecondaryIPConfigs map[string]SecondaryIPConfig // uuid is key
cns/NetworkContainerContract.go:511
- The SecondaryIPConfigurations field lacks documentation explaining its purpose for dual-stack support. Add a comment explaining that this field is used to provide secondary IP configurations such as IPv6 addresses for dual-stack networking scenarios.
PodIPConfig IPSubnet
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| routes := make([]network.RouteInfo, 0) | ||
| if networkConfig.Routes != nil && len(networkConfig.Routes) > 0 { | ||
| if len(networkConfig.Routes) > 0 { |
There was a problem hiding this comment.
The removal of the nil check (networkConfig.Routes != nil) is unnecessary but not harmful since len() on a nil slice returns 0 in Go. However, the original check was more explicit and readable. Consider keeping the original condition for code clarity, or ensure this change is consistent with the project's style guidelines.
| ipv6IPConfig.Gateway = net.ParseIP(ipv6Config.GatewayIPAddress) | ||
| } else if ipv6Config.GatewayIPv6Address != "" { | ||
| ipv6IPConfig.Gateway = net.ParseIP(ipv6Config.GatewayIPv6Address) |
There was a problem hiding this comment.
The gateway address parsing at lines 259-263 lacks validation. After parsing with net.ParseIP, the code should verify that the parsed gateway is actually an IPv6 address (using .To4() == nil check). Currently, if GatewayIPAddress or GatewayIPv6Address contains an IPv4 address string, it would be accepted for an IPv6 configuration, which could lead to routing issues.
| ipv6IPConfig.Gateway = net.ParseIP(ipv6Config.GatewayIPAddress) | |
| } else if ipv6Config.GatewayIPv6Address != "" { | |
| ipv6IPConfig.Gateway = net.ParseIP(ipv6Config.GatewayIPv6Address) | |
| gw := net.ParseIP(ipv6Config.GatewayIPAddress) | |
| if gw == nil || gw.To4() != nil { | |
| logger.Error("Invalid IPv6 gateway address in GatewayIPAddress", | |
| zap.String("gateway", ipv6Config.GatewayIPAddress), | |
| zap.String("ncID", ncResponses[i].NetworkContainerID)) | |
| return IPAMAddResult{}, fmt.Errorf("invalid IPv6 gateway address %q for NC %s", ipv6Config.GatewayIPAddress, ncResponses[i].NetworkContainerID) | |
| } | |
| ipv6IPConfig.Gateway = gw | |
| } else if ipv6Config.GatewayIPv6Address != "" { | |
| gw := net.ParseIP(ipv6Config.GatewayIPv6Address) | |
| if gw == nil || gw.To4() != nil { | |
| logger.Error("Invalid IPv6 gateway address in GatewayIPv6Address", | |
| zap.String("gateway", ipv6Config.GatewayIPv6Address), | |
| zap.String("ncID", ncResponses[i].NetworkContainerID)) | |
| return IPAMAddResult{}, fmt.Errorf("invalid IPv6 gateway address %q for NC %s", ipv6Config.GatewayIPv6Address, ncResponses[i].NetworkContainerID) | |
| } | |
| ipv6IPConfig.Gateway = gw |
| _, defaultIPv6Net, _ := net.ParseCIDR("::/0") | ||
| dstIPv6 := net.IPNet{IP: net.ParseIP("::"), Mask: defaultIPv6Net.Mask} | ||
| ifInfo.Routes = append(ifInfo.Routes, network.RouteInfo{Dst: dstIPv6, Gw: ipv6IPConfig.Gateway}) |
There was a problem hiding this comment.
The error returned from net.ParseCIDR is being ignored. If the CIDR "::/0" cannot be parsed (which would be unusual but possible in certain environments), the code will continue with a nil defaultIPv6Net, leading to a potential nil pointer dereference when accessing defaultIPv6Net.Mask. While this is an unlikely scenario for a hardcoded well-formed CIDR, it's better to handle or at least check for errors for robustness.
| _, defaultIPv6Net, _ := net.ParseCIDR("::/0") | |
| dstIPv6 := net.IPNet{IP: net.ParseIP("::"), Mask: defaultIPv6Net.Mask} | |
| ifInfo.Routes = append(ifInfo.Routes, network.RouteInfo{Dst: dstIPv6, Gw: ipv6IPConfig.Gateway}) | |
| _, defaultIPv6Net, err := net.ParseCIDR("::/0") | |
| if err != nil { | |
| logger.Error("Failed to parse default IPv6 CIDR", zap.Error(err)) | |
| } else { | |
| dstIPv6 := net.IPNet{IP: net.ParseIP("::"), Mask: defaultIPv6Net.Mask} | |
| ifInfo.Routes = append(ifInfo.Routes, network.RouteInfo{Dst: dstIPv6, Gw: ipv6IPConfig.Gateway}) | |
| } |
| func validateAndGetIPv6FromSecondaryIPs(secondaryIPConfigs []cns.IPConfiguration) (*cns.IPConfiguration, error) { | ||
| if len(secondaryIPConfigs) == 0 { | ||
| return nil, nil | ||
| } | ||
|
|
||
| var ipv6Config *cns.IPConfiguration | ||
| ipv6Count := 0 | ||
|
|
||
| for i := range secondaryIPConfigs { | ||
| ipAddr := net.ParseIP(secondaryIPConfigs[i].IPSubnet.IPAddress) | ||
| if ipAddr == nil { | ||
| return nil, fmt.Errorf("%w: %s", errInvalidIPv6InSecondaryIPs, secondaryIPConfigs[i].IPSubnet.IPAddress) | ||
| } | ||
|
|
||
| // Check if this is an IPv6 address | ||
| if ipAddr.To4() == nil && ipAddr.To16() != nil { | ||
| ipv6Count++ | ||
| if ipv6Count > 1 { | ||
| return nil, errMultipleIPv6InSecondaryIPs | ||
| } | ||
| ipv6Config = &secondaryIPConfigs[i] | ||
| } | ||
| } | ||
|
|
||
| return ipv6Config, nil | ||
| } |
There was a problem hiding this comment.
The new function validateAndGetIPv6FromSecondaryIPs lacks test coverage. Given this is a critical validation function that determines whether IPv6 configurations are valid and properly handles dual-stack scenarios, comprehensive unit tests should be added to cover cases like: no IPv6 addresses, one IPv6 address, multiple IPv6 addresses (error case), mixed IPv4/IPv6 addresses, and invalid IP addresses.
| ipv6Addr := net.ParseIP(ipv6Config.IPSubnet.IPAddress) | ||
| ipv6IPConfig := &network.IPConfig{ | ||
| Address: net.IPNet{ | ||
| IP: ipv6Addr, | ||
| Mask: net.CIDRMask(int(ipv6Config.IPSubnet.PrefixLength), ipv6FullMask), | ||
| }, | ||
| } |
There was a problem hiding this comment.
The IPv6 configuration validation should check if ipAddr is actually an IPv6 address before attempting to create an IPConfig with it. Currently, if ipv6Config.IPSubnet.IPAddress contains an invalid IP string, net.ParseIP will return nil at line 250, but the code continues to use this nil value to create the Address field. This will cause issues. Add validation: if ipv6Addr == nil, return an error.
| // Validate SecondaryIPConfigurations contains at most one IPv6 | ||
| ipv6Config, err := validateAndGetIPv6FromSecondaryIPs(ncResponses[i].SecondaryIPConfigurations) | ||
| if err != nil { | ||
| logger.Error("Failed to validate SecondaryIPConfigurations", | ||
| zap.Int("ncResponseIndex", i), | ||
| zap.String("ncID", ncResponses[i].NetworkContainerID), | ||
| zap.Error(err)) | ||
| return IPAMAddResult{}, fmt.Errorf("validation failed for NC %s: %w", ncResponses[i].NetworkContainerID, err) | ||
| } | ||
|
|
||
| // one ncResponse gets you one interface info in the returned IPAMAddResult | ||
| ifInfo := network.InterfaceInfo{ | ||
| NCResponse: &ncResponses[i], | ||
| HostSubnetPrefix: hostSubnetPrefixes[i], | ||
| } | ||
|
|
||
| // Process primary IPv4 configuration | ||
| ipconfig, routes := convertToIPConfigAndRouteInfo(ifInfo.NCResponse) | ||
| ifInfo.IPConfigs = append(ifInfo.IPConfigs, ipconfig) | ||
| ifInfo.Routes = routes | ||
|
|
||
| // Process secondary IPv6 configuration if present | ||
| if ipv6Config != nil { | ||
| logger.Info("Processing IPv6 from SecondaryIPConfigurations", | ||
| zap.String("ipv6Address", ipv6Config.IPSubnet.IPAddress), | ||
| zap.String("ncID", ncResponses[i].NetworkContainerID)) | ||
|
|
||
| ipv6Addr := net.ParseIP(ipv6Config.IPSubnet.IPAddress) | ||
| ipv6IPConfig := &network.IPConfig{ | ||
| Address: net.IPNet{ | ||
| IP: ipv6Addr, | ||
| Mask: net.CIDRMask(int(ipv6Config.IPSubnet.PrefixLength), ipv6FullMask), | ||
| }, | ||
| } | ||
|
|
||
| // Set IPv6 gateway if provided | ||
| if ipv6Config.GatewayIPAddress != "" { | ||
| ipv6IPConfig.Gateway = net.ParseIP(ipv6Config.GatewayIPAddress) | ||
| } else if ipv6Config.GatewayIPv6Address != "" { | ||
| ipv6IPConfig.Gateway = net.ParseIP(ipv6Config.GatewayIPv6Address) | ||
| } | ||
|
|
||
| // Add IPv6 configuration to the interface | ||
| ifInfo.IPConfigs = append(ifInfo.IPConfigs, ipv6IPConfig) | ||
|
|
||
| // Add IPv6 routes if gateway is specified | ||
| if ipv6IPConfig.Gateway != nil { | ||
| _, defaultIPv6Net, _ := net.ParseCIDR("::/0") | ||
| dstIPv6 := net.IPNet{IP: net.ParseIP("::"), Mask: defaultIPv6Net.Mask} | ||
| ifInfo.Routes = append(ifInfo.Routes, network.RouteInfo{Dst: dstIPv6, Gw: ipv6IPConfig.Gateway}) | ||
| } | ||
| } |
There was a problem hiding this comment.
The IPv6 validation and processing logic added in lines 223-274 lacks test coverage. This is a significant new feature for dual-stack support that should have comprehensive tests covering scenarios such as: NC with IPv6 in SecondaryIPConfigurations, NC without IPv6, IPv6 with and without gateway configuration, and error handling for invalid IPv6 configurations.
| errSubnetOverlap = errors.New("subnet overlap error") | ||
| errIfaceNotFound = errors.New("Interface not found for this ip") | ||
| errMultipleIPv6InSecondaryIPs = errors.New("SecondaryIPConfigurations contains more than one IPv6") | ||
| errInvalidIPv6InSecondaryIPs = errors.New("SecondaryIPConfigurations contains invalid IPv6 address") |
There was a problem hiding this comment.
The error variable name errInvalidIPv6InSecondaryIPs is misleading. This error is used at line 379 to indicate any invalid IP address format (not specifically IPv6). The actual usage doesn't match the name since it's triggered before the IPv6-specific check at line 383. Consider renaming to errInvalidIPAddressInSecondaryIPs.
| errInvalidIPv6InSecondaryIPs = errors.New("SecondaryIPConfigurations contains invalid IPv6 address") | |
| errInvalidIPv6InSecondaryIPs = errors.New("SecondaryIPConfigurations contains invalid IP address") |
| result.IPs = append(result.IPs, resultIpconfig) | ||
|
|
||
| if networkConfig.Routes != nil && len(networkConfig.Routes) > 0 { | ||
| if len(networkConfig.Routes) > 0 { |
There was a problem hiding this comment.
The removal of the nil check (networkConfig.Routes != nil) is unnecessary but not harmful since len() on a nil slice returns 0 in Go. However, the original check was more explicit and readable. Consider keeping the original condition for code clarity, or ensure this change is consistent with the project's style guidelines.
Not to be merged
By now
Change aims at adding IPV6 specifically for swiftv1 multitenancy scenarios. This change is targeted only to CNI ADD flow (no CNS changes in this PR)
IPV6 will be passed to CNI using new field SecondaryIPConfigurations in GetNetworkConfigurationResponse from CNS