fix: route issues on Swiftv2 Windows #3205
Conversation
paulyufan2
force-pushed
the
defaultroutelongtermsolution
branch
2 times, most recently
from
6683000 to
ec4f810
Compare
paulyufan2
force-pushed
the
defaultroutelongtermsolution
branch
from
cd29e26 to
8077cd5
Compare
paulyufan2
changed the title
paulyufan2
changed the title
|
/azp run Azure Container Networking PR |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days |
|
Pull request closed due to inactivity. |
paulyufan2
force-pushed
the
defaultroutelongtermsolution
branch
from
6b3eefc to
0213526
Compare
paulyufan2
force-pushed
the
defaultroutelongtermsolution
branch
4 times, most recently
from
1249211 to
272f829
Compare
|
/azp run Azure Container Networking PR |
|
Azure Pipelines successfully started running 1 pipeline(s). |
paulyufan2
force-pushed
the
defaultroutelongtermsolution
branch
from
e9442c3 to
4f8ef4f
Compare
|
/azp run Azure Container Networking PR |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| for i := range podIPInfo { | ||
| assert.DeepEqual(t, podIPInfo[i].Routes, desiredPodIPInfo[i].Routes) | ||
| reflect.DeepEqual(podIPInfo[i].Routes, desiredPodIPInfo[i].Routes) |
There was a problem hiding this comment.
Preference towards github.com/google/go-cmp/cmp.Equal over reflect.DeepEqual (it's a drop-in replacement)
| v4IPs = []string{} | ||
| v6IPs = []string{} |
There was a problem hiding this comment.
Don't use named returns please, they have surprising semantics. For example, these two lines are unnecessary since they are auto-initialized.
| v6IPs = append(v6IPs, v6PodIPs...) | ||
|
|
||
| if ip.Is4() { | ||
| routes = append(routes, k.addRoutes(v4IPs, overlayGatewayv4)...) |
There was a problem hiding this comment.
I'm comparing the getInfraRoutes code in the _linux.go file and the getInfraRoutes code in the _windows.go file and noticed that overlayGatewayv4 / v6 is added in linux-- could you provide background on this (does it make an appearance in windows, and what is it used for?). Also the casing is inconsistent for v4 and V6
There was a problem hiding this comment.
yes, I think I added comments somewhere;
To sum up:
Linux: always use overlayGatewayv4 as the default gateway IP (168.x.x.x) for IPv4 and overlayGatewayv6 for IPv6; note that ipv6 is not supported for both Linux and Windows right now but codes already implemented ipv6 gateway and I will not touch it.
Windows: always use 0.0.0.0 as the default gateway IP for containerd to program the routing table and ipv6 is not supported.
There was a problem hiding this comment.
thanks for clarifying, could we also add that note (regarding the gateway ip) above this function if it is still present after the refactor?
| func (k *K8sSWIFTv2Middleware) assignSubnetPrefixLengthFields(_ *cns.PodIpInfo, _ v1alpha1.InterfaceInfo, _ string) error { | ||
| return nil | ||
| } | ||
| func (k *K8sSWIFTv2Middleware) getInfraRoutes(podIPInfo *cns.PodIpInfo) ([]cns.Route, error) { |
There was a problem hiding this comment.
I noticed there is a lot of similar code between the windows and linux versions of getInfraRoutes-- would it be possible to move these to the common go file and then have another function that calls an OS specific implementation and returns all v4 and v6 cidrs for that particular OS?
There was a problem hiding this comment.
if not could you add a comment to this function mentioning what it does in each OS implementation, since the behavior differs (ex: presence of overlayGateway IP, get pod cidrs in linux only, get cidrs (meaning node+service cidrs are added)
There was a problem hiding this comment.
yes Riya's PR has refactored the middlewire and I am waiting for her PR to be checked in and I will make changes on top of her PR
| @@ -249,3 +249,59 @@ func (k *K8sSWIFTv2Middleware) getIPConfig(ctx context.Context, podInfo cns.PodI | |||
| func (k *K8sSWIFTv2Middleware) Type() cns.SWIFTV2Mode { | |||
| return cns.K8sSWIFTV2 | |||
| } | |||
|
|
|||
| // CNS gets pod CIDRs from configuration env and parse them to get the v4 and v6 IPs | |||
| // Containerd reassigns the IP to the adapter and kernel configures the pod cidr route by default, so windows swiftv2 does not require pod cidr | |||
There was a problem hiding this comment.
I feel like the comment here would be better above the OS-specific implementations of getInfraRoutes. GetPodCidrs (the function) by itself doesn't do anything os-specific.
Reason for Change:
This is the long term solution to fix Swiftv2 Windows Routes issues to make sure windows cns gets all required cidrs from AKS.
Fixes include:
1.Windows CNS gets infravnet/pod/node cidrs from configs
2.Add these routes and send them to the CNI
Issue Fixed:
Background of the issue:
In Linux Swiftv2, CNS fetches infravnet/pod/node cidrs from the node's envs set by AKS; we should do same thing for Windows;
Requirements:
Require to validate this PR after AKS-RP PR to be merged
Notes: