-
Notifications
You must be signed in to change notification settings - Fork 260
Dualstack Overlay CNI #1866
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
Dualstack Overlay CNI #1866
Conversation
cni/network/invoker_azure.go
Outdated
| if len(addResult.ipv4Result.IPs) > 0 { | ||
| if er := invoker.Delete(&addResult.ipv4Result.IPs[0].Address, addConfig.nwCfg, nil, addConfig.options); er != nil { | ||
| addresses := []*net.IPNet{} | ||
| for _, ip := range addResult.ipv4Result.IPs { |
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.
Here we are appending from ipv4 result. And in the delete function we are deleting the ipv6 addresses as well. Correct me if am wrong but shouldn't we have the ipv6 addresses in the list too ?
OR am i missing something ? we can add a comment describing what the intent of this function is.
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.
@paulyufan can you repond to this comment?
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.
Hi, I missed this comment, yes, we do need delete ipv6 addresses here as well
| if net.ParseIP(info.podIPAddress).To4() != nil { | ||
| ncgw = net.ParseIP(overlayGatewayV4IP) | ||
| } else { | ||
| ncgw = net.ParseIP(overlayGatewayV6IP) |
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.
Should we add a condition to check that the ipv6 address is returned ? or we are sure that if not ipv4 then ipv6 is the case.
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.
+1 on @vipul-21 comment. We should validate that it's ipv6 IP in 'else if' followed by 'esle' which returns error
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.
yes I added this fix
cni/network/invoker_cns.go
Outdated
| return errors.Wrap(err, fmt.Sprintf("failed to release IP %v with err ", address)+"%w") | ||
| if err := invoker.cnsClient.ReleaseIPs(context.TODO(), req); err != nil { | ||
| // if we fail a release with a 404 error try using the old API | ||
| if errors.Is(err, cnscli.ErrAPINotFound) { |
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.
In this case, what if the API fails a single time with 404 and we call the old API. ? Do we intend to call the old API for the subsequent calls ?
OR the old API is serving just as back up ? ( will probably produce the same results ?)
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.
yes, we do call old API if new API does not work
vipul-21
left a comment
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 add a bit more description for the PR as well.
Like Reason for change, design doc etc.
| } | ||
|
|
||
| if len(invoker.nwInfo.Subnets) > 0 { | ||
| nwCfg.IPAM.Subnet = invoker.nwInfo.Subnets[0].Prefix.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.
which subnet is this referring to?
cni/network/invoker_azure.go
Outdated
| nwCfgIpv6.IPAM.Environment = common.OptEnvironmentIPv6NodeIpam | ||
| nwCfgIpv6.IPAM.Type = ipamV6 | ||
| nwCfgIpv6.IPAM.Address = address.IP.String() | ||
| if len(invoker.nwInfo.Subnets) > 1 { |
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 understand you haven't added this logic but just rearranged it. Why is invoker.nwInfo.Subnets[1] used in ipv6 path? Is that a guarantee?
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 see that in the invoker_azure_test.go for ipv6 delete, it uses this nwInfo -
nwInfo: getNwInfo("10.0.0.0/24", "2001:db8:abcd:0012::0/64"),
with dualstack, do we always create nwInfo with such ipv4 and ipv6 order?
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.
thats old code. you are right we should not make that assumption. instead we should find based on ipaddress itself.
cni/network/invoker_cns.go
Outdated
| PodInterfaceID: GetEndpointID(addConfig.args), | ||
| InfraContainerID: addConfig.args.ContainerID, | ||
| } | ||
| log.Errorf("Failed to request IPs using RequestIPs from CNS, going to try RequestIPAddress. error: %v request: %v", err, ipconfig) |
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: Move this error log to line 84
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.
In the error log I would clearly specify the failure due to api not found.
log.Errorf("RequestIPs not supported by CNS. Invoking RequestIPAddress API. request: %v", ipconfig)
| if net.ParseIP(info.podIPAddress).To4() != nil { | ||
| ncgw = net.ParseIP(overlayGatewayV4IP) | ||
| } else { | ||
| ncgw = net.ParseIP(overlayGatewayV6IP) |
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.
+1 on @vipul-21 comment. We should validate that it's ipv6 IP in 'else if' followed by 'esle' which returns error
cni/network/invoker_cns.go
Outdated
| }, | ||
| }, | ||
| } | ||
| } else { |
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.
let's check if this is ipv6 IP here
cni/network/invoker_cns.go
Outdated
| OrchestratorContext: orchestratorContext, | ||
| PodInterfaceID: GetEndpointID(args), | ||
| InfraContainerID: args.ContainerID, | ||
| DesiredIPAddress: req.DesiredIPAddresses[0], |
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.
req.DesiredIPAddresses[0] can run into nil pointer deref?
what if len(addresses) check on line 289 fails? You will never initialize the slice on line 290.
Has this been tested with UT?
cni/network/invoker_cns.go
Outdated
| InfraContainerID: args.ContainerID, | ||
| DesiredIPAddress: req.DesiredIPAddresses[0], | ||
| } | ||
| log.Errorf("Failed to release IPs using ReleaseIPs from CNS, going to try ReleaseIPAddress. error: %v request: %v", err, req) |
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.
same comment as request func. Move this up and revisit the error message.
…working into dualstack-overlay-cni
…twork is created at first time
Makefile
Outdated
| $(MKDIR) $(CNI_DUALSTACK_BUILD_DIR) | ||
| cp cni/azure-$(GOOS)-swift-overlay-dualstack.conflist $(CNI_DUALSTACK_BUILD_DIR)/10-azure.conflist | ||
| cp telemetry/azure-vnet-telemetry.config $(CNI_DUALSTACK_BUILD_DIR)/azure-vnet-telemetry.config | ||
| cp $(CNI_BUILD_DIR)/azure-vnet$(EXE_EXT) $(CNI_BUILD_DIR)/azure-vnet-ipam$(EXE_EXT) $(CNI_BUILD_DIR)/azure-vnet-telemetry$(EXE_EXT) $(CNI_DUALSTACK_BUILD_DIR) |
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.
why do you need azure-vnet-ipam binary?
cni/network/invoker_azure.go
Outdated
| } | ||
| } else if len(addResult.ipv6Result.IPs) > 0 { | ||
| if er := invoker.Delete(&addResult.ipv6Result.IPs[0].Address, addConfig.nwCfg, nil, addConfig.options); er != nil { | ||
| err = invoker.plugin.Errorf("Failed to clean up IPv6 during Delete with error %v, after Add failed with error %w", er, err) |
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.
why this change is required in legacy ipam? I think dualstack supported is added only with swift which uses cns
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.
thanks Tamilmani, we do not need to change this in legacy ipam. remove these changes from legacy ipam in case to generate problems
cni/network/invoker_cns.go
Outdated
| // if RequestIPs call fails, we may receive API not Found error as new CNS is not supported, then try old API RequestIPAddress | ||
| log.Errorf("RequestIPs not supported by CNS. Invoking RequestIPAddress API with infracontainerid %s", ipconfig.InfraContainerID) | ||
| if cnscli.IsUnsupportedAPI(err) { | ||
| ipconfigs := cns.IPConfigRequest{ |
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.
why do we redefine ipconfigs? can we use the same defined in line 75?
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.
because they are using different structure:
ipConfigs: IPConfigsRequest
ipConfig: IPConfigRequest
cni/network/invoker_cns.go
Outdated
| } | ||
|
|
||
| ipconfig := cns.IPConfigRequest{ | ||
| ipconfig := cns.IPConfigsRequest{ |
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 it be ipconfigs instead?
| return IPAMAddResult{}, err | ||
| } | ||
| } else if net.ParseIP(info.podIPAddress).To16() != nil { | ||
| ncgw = net.ParseIP(overlayGatewayV6IP) |
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 confirm with hns team if gw ip configuration correct? when last checked for ipv4 overlay, sravanth mentioned gw address should be from same subnet of NC.
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.
yes, we got this fixed ipv6 gateway from HNS team
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.
yes, HNS team gave this and we do not have this requirement for now because Jaeryn fixed this before was to fix overlay gateway bug that prevented cloud-node-manager-windows from initializing node because it couldn't reach IMDS. An additional VFP rule was implicitly plumbed by HNS to add 169.254.1.1/16 to the exception list. This is most likely based off the RouteConfiguration in the "azure" HNS network. This additional VFP rule was skipping SNAT for any IP address in this range 169.254.0.0-169.254.255.255. We'll set the first .1 IP of podcidr as the gateway now.
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.
Regarding this "We'll set the first .1 IP of podcidr as the gateway now."
in this case, cni sets "fe80::1234:5678:9abc" as gw ip and its not first ip of podcidr. This is exactly the same problem with ipv4 and hns team said gateway ip should be within pod cidr.. i know jaeryn made this change (making gw ip as first ip of pod cidr) for imds to work but that's the right way to do it whether it works or not as per hns team
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.
setting this gw ip "fe80:🔢5678:9abc" is correct for linux but may not be for windows. if you have got approval from sravanth/hns team on this, im good
cni/network/network.go
Outdated
| } | ||
|
|
||
| log.Printf("[cni-net] ADD command completed for pod %v with IPs:%+v err:%v.", k8sPodName, ipamAddResult.ipv4Result.IPs, err) | ||
| if ipamAddResult.ipv4Result.IPs != 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.
isn't that behaviour same even before? we didn't see any crash before..
cni/network/network.go
Outdated
|
|
||
| setNetworkOptions(ipamAddResult.ncResponse, &nwInfo) | ||
|
|
||
| addNatIPV6SubnetInfo(ipamAddConfig.nwCfg, ipamAddResult.ipv6Result, &nwInfo) |
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.
it does the same thing. lets remove that.anyway we are not supporting ipv6nat mode
| ipv4Subnet := network.SubnetInfo{ | ||
| Family: platform.AfINET, | ||
| Prefix: *podSubnetPrefix, | ||
| Gateway: ipamAddResult.ipv4Result.IPs[0].Gateway, | ||
| } | ||
| nwInfo.Subnets = append(nwInfo.Subnets, ipv4Subnet) | ||
|
|
||
| if ipamAddResult.ipv6Result != nil && len(ipamAddResult.ipv6Result.IPs) > 0 { | ||
| ipv6Subnet := network.SubnetInfo{ | ||
| Family: platform.AfINET6, | ||
| Prefix: *podSubnetV6Prefix, | ||
| Gateway: ipamAddResult.ipv6Result.IPs[0].Gateway, | ||
| } | ||
| nwInfo.Subnets = append(nwInfo.Subnets, ipv6Subnet) | ||
| } |
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.
the function is growing bigger.. you can define internal function like constructNetworkInfo and move initializing network info and assigning these subnets under that?
cni/network/network.go
Outdated
|
|
||
| if opt.resultV6 != nil { | ||
| // inject routes to linux pod | ||
| epInfo.IPV6Mode = network.DualStackOverlay |
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.
why ipv6Mode is concluded as DualstackOverlay and how do we handle if we support podsubnet(vnet) for ipv6 tomorrow?
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.
also where we are using this field?
cni/network/network.go
Outdated
|
|
||
| if !nwCfg.MultiTenancy { | ||
| // Call into IPAM plugin to release the endpoint's addresses. | ||
| logAndSendEvent(plugin, fmt.Sprintf("Releasing ips:%+v", epInfo.IPAddresses)) |
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 would prefer not changing this. anyway it will add one more extra log which is fine imo.. it will give more clarity on ip has been released and what not
network/network_windows.go
Outdated
| log.Printf("[net] Successfully created hcn network with response: %+v", hnsResponse) | ||
|
|
||
| // only add net rules if it's dualStackOverlay mode and hnsNetwork is created at first time | ||
| if util.DualStackOverlay == DualStackOverlay { |
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.
trying to understand the significance of this check.. isn't these comparing 2 constant variables?
cni/network/network.go
Outdated
|
|
||
| if opt.resultV6 != nil { | ||
| // inject routes to linux pod | ||
| epInfo.IPV6Mode = network.DualStackOverlay |
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.
also where we are using this field?
cni/network/invoker_cns.go
Outdated
| log.Printf("Failed to get IP address from CNS with error %v, response: %v", err, response) | ||
| return IPAMAddResult{}, errors.Wrap(err, "Failed to get IP address from CNS with error: %w") | ||
| // if RequestIPs call fails, we may receive API not Found error as new CNS is not supported, then try old API RequestIPAddress | ||
| log.Errorf("RequestIPs not supported by CNS. Invoking RequestIPAddress API with infracontainerid %s", ipconfigs.InfraContainerID) |
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 error log should be moved after line 86. We shouldn't log this error if the error is not of the type UnSupportedAPI
cni/network/invoker_cns.go
Outdated
| if err != nil { | ||
| log.Printf("Failed to get IP address from CNS with error %v, response: %v", err, response) | ||
| return IPAMAddResult{}, errors.Wrap(err, "Failed to get IP address from CNS with error: %w") | ||
| // if RequestIPs call fails, we may receive API not Found error as new CNS is not supported, then try old API RequestIPAddress |
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 comment: If RequestIPs is not supported by CNS, use RequestIPAddress API
cni/network/invoker_cns.go
Outdated
|
|
||
| log.Printf("[cni-invoker-cns] Received info %+v for pod %v", info, podInfo) | ||
| // set the NC Primary IP in options | ||
| // SNATIPKey set is not for ipv6 |
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: is not set for
cni/network/invoker_cns.go
Outdated
| if ncgw == nil { | ||
| if invoker.ipamMode != util.V4Overlay { | ||
| return IPAMAddResult{}, errors.Wrap(errInvalidArgs, "%w: Gateway address "+info.ncGatewayIPAddress+" from response is invalid") | ||
| // set result ipconfigArgument from CNS Response Body |
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.
is this comment placed correctly?
// set result ipconfigArgument from CNS Response Body
not sure what it means
network/network_windows.go
Outdated
|
|
||
| // netsh interface ipv4 add route $subnetV4 $hostInterfaceAlias $gatewayV4 | ||
| netshV4GatewayRoute := fmt.Sprintf(netRouteCmd, "ipv4", prefix, ifName, gateway) | ||
| log.Printf("[net] Adding ipv4 gateway route failed: %v:%v", out, err) |
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 log line is just incorrect. Looks like copy paste error
| if out, err = nm.plClient.ExecuteCommand(netshV4GatewayRoute); err != nil { | ||
| log.Printf("[net] Adding ipv4 gateway route failed: %v:%v", out, err) | ||
| } | ||
| } else { |
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.
let's check
ip.To16() != 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.
ip, _, er := net.ParseCIDR(prefix)
ParseCIDR already handled whether prefix is valid or not, so we do not need check ip.To16() here
network/network_windows.go
Outdated
| log.Printf("[net] Successfully created hcn network with response: %+v", hnsResponse) | ||
|
|
||
| // only add net rules if it's dualStackOverlay mode and hnsNetwork is created at first time | ||
| if string(util.DualStackOverlay) != "" { |
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.
how is this check ensuring that CNI is operating in dualstack overlay mode?
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.
so confusing as you define multiple variables with dualstack. why don't you use epInfo.Ipv6mode
network/network_windows.go
Outdated
| // only add net rules if it's dualStackOverlay mode and hnsNetwork is created at first time | ||
| if string(util.DualStackOverlay) != "" { | ||
| if err := nm.addNewNetRules(nwInfo); err != nil { // nolint | ||
| return nil, err |
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.
let's please log error here
| } else { | ||
| // netsh interface ipv6 add route $subnetV6 $hostInterfaceAlias "::" metric=270 | ||
| netshV6DefaultRoute := fmt.Sprintf(netRouteCmd, "ipv6", prefix, ifName, ipv6DefaultHop, "270") | ||
| if out, err = nm.plClient.ExecuteCommand(netshV6DefaultRoute); err != 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.
+1 these are not idempotent. If the route is already present recreating the route will fail as it complains saying the object is already present.
@paulyufan2 as discussed earlier this week, you would need to check if the route exists before creating it.
cni/network/network.go
Outdated
|
|
||
| if opt.resultV6 != nil { | ||
| // get IPAM mode from conflist file and add ipv6 routes to linux pod if needed | ||
| epInfo.IPV6Mode = string(util.IpamMode(opt.nwCfg.IPAM.Mode)) |
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 you should only set the epInfo.IPV6Mode to that value if the string(util.IpamMode(opt.nwCfg.IPAM.Mode)) is dualStackOverlay. This will prevent setting the IPV6Mode if we are not operating in dualstack mode
| { | ||
| "type":"azure-vnet", | ||
| "mode":"transparent", | ||
| "executionMode":"v4swift", |
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.
shouldn't this be "dualstackswift"?
| "type": "azure-vnet", | ||
| "mode": "bridge", | ||
| "bridge": "azure0", | ||
| "executionMode": "v4swift", |
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.
same as above
| nwCfgIpv6.IPAM.Subnet = invoker.nwInfo.Subnets[1].Prefix.String() | ||
| for _, subnet := range invoker.nwInfo.Subnets { | ||
| if subnet.Prefix.IP.To4() == nil { | ||
| nwCfgIpv6.IPAM.Subnet = subnet.Prefix.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.
break from for loop after finding ipv6 subnet?
| return IPAMAddResult{}, err | ||
| } | ||
| } else if net.ParseIP(info.podIPAddress).To16() != nil { | ||
| ncgw = net.ParseIP(overlayGatewayV6IP) |
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.
Regarding this "We'll set the first .1 IP of podcidr as the gateway now."
in this case, cni sets "fe80::1234:5678:9abc" as gw ip and its not first ip of podcidr. This is exactly the same problem with ipv4 and hns team said gateway ip should be within pod cidr.. i know jaeryn made this change (making gw ip as first ip of pod cidr) for imds to work but that's the right way to do it whether it works or not as per hns team
| return IPAMAddResult{}, err | ||
| } | ||
| } else if net.ParseIP(info.podIPAddress).To16() != nil { | ||
| ncgw = net.ParseIP(overlayGatewayV6IP) |
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.
setting this gw ip "fe80:🔢5678:9abc" is correct for linux but may not be for windows. if you have got approval from sravanth/hns team on this, im good
|
|
||
| var ( | ||
| errUnsupportedAPI = errors.New("Unsupported API") | ||
| errNoRequestIPFound = errors.New("No Request IP Found") |
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 dont think we are testing actual code paths but creating these mock errors and returning from that in mock function. @paulyufan is that right?
cni/network/network.go
Outdated
| // parse the ipv6 address and only add it to nwInfo if it's dual stack mode | ||
| var podSubnetV6Prefix *net.IPNet | ||
| if ipamAddResult.ipv6Result != nil && len(ipamAddResult.ipv6Result.IPs) > 0 { | ||
| _, podSubnetV6Prefix, err = net.ParseCIDR(ipamAddResult.ipv6Result.IPs[0].Address.String()) | ||
| if err != nil { | ||
| return nwInfo, fmt.Errorf("Failed to ParseCIDR for pod subnet IPv6 prefix: %w", err) | ||
| } | ||
| } | ||
|
|
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 move this and ipv4 parse as well to addSubnetToNetworkInfo function?
| // inject ipv6 routes to Linux pod | ||
| ipamMode := string(util.IpamMode(opt.nwCfg.IPAM.Mode)) | ||
| if ipamMode == "dualStackOverlay" { | ||
| epInfo.IPV6Mode = ipamMode |
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 guess instead of checking ipammode, we should set executionmode to dualstackswift and set that in epInfo.ipamMode
| COPY --from=azure-ipam /azure-ipam/bin/* pkg/embed/fs | ||
| COPY --from=azure-vnet /azure-container-networking/cni/azure-$OS-swift.conflist pkg/embed/fs/azure-swift.conflist | ||
| COPY --from=azure-vnet /azure-container-networking/cni/azure-$OS-swift-overlay.conflist pkg/embed/fs/azure-swift-overlay.conflist | ||
| COPY --from=azure-vnet /azure-container-networking/cni/azure-$OS-swift-overlay-dualstack.conflist pkg/embed/fs/azure-swift-overlay-dualstack.conflist |
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.
we are moving away from copying conflist from dropgz instead cns should write conflist after NC creation
network/network_windows.go
Outdated
| log.Printf("[net] Successfully created hcn network with response: %+v", hnsResponse) | ||
|
|
||
| // only add net rules if it's dualStackOverlay mode and hnsNetwork is created at first time | ||
| if string(util.DualStackOverlay) != "" { |
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.
so confusing as you define multiple variables with dualstack. why don't you use epInfo.Ipv6mode
|
do not review this PR. Please use this PR: https://github.com/Azure/azure-container-networking/pull/1925/files |
Reason for Change:
This PR is to provide dualStack CNI changes to create a NC with both ipv4 and ipv6 address in dualStackOverlay mode on both Windows&Linux OS
The PR includes following UTs:
(1) RequestIPs() => request ipv4 & ipv6 ip addresses and make sure dual-stack NC creation
(2) RequestIPAddress() => request only ipv4 and make sure single-stack NC is working fine
(3) ReleaseIPs() => release ipv4 & ipv6 ip addresses and make sure they are released
(4) ReleaseIPAddress() => release ipv4 address only and make sure this ipv4 address can be released in single-tenancy
Tests we did manually(on both Linux and Windows):
(1) Network(ipv4+ipv6):
pod -> pod (one node and different nodes)
pod -> host
host -> pod
pod -> internet
host -> internet
(2) Launched dual stack overlay cluster in different regions: eastus, eastuseuap, centraluseuap
(3) Test Different Linux OSs (including Ubuntu 19, Ubunt22 and CLB-Mariner)
(4) Test different Linux processors (ARM/AMDH&I)
(5) NMAgent tests
(6) HNS tests: https://microsoftapc-my.sharepoint.com/personal/ppereira_microsoft_com/_layouts/15/onedrive.aspx?id=%2Fpersonal%2Fppereira%5Fmicrosoft%5Fcom%2FDocuments%2FDocuments%2FProjects%2FAKS%2DIPV6%2DSupport&ct=1680642656649&or=Teams%2DHL&ga=1
Issue Fixed:
Requirements:
Notes: