✨ Support IPv6/dual-stack routable pods with NSX-VPC#1763
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: silvery1622 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold |
67393ba to
45e7c24
Compare
| } | ||
|
|
||
| if !c.ipFamily.DualStack() { | ||
| if len(node.Spec.PodCIDRs) == 1 && node.Spec.PodCIDRs[0] == thisCIDR { |
There was a problem hiding this comment.
What if len(node.Spec.PodCIDRs) > 1 or node.Spec.PodCIDRs[0] != thisCIDR?
There was a problem hiding this comment.
If the node has multiple CIDRs or an incorrect CIDR in a single-stack cluster, that if condition evaluates to false. The code then falls through to utils.PatchNodeCIDRWithRetry, which uses a strategic merge patch to completely overwrite both PodCIDR and PodCIDRs with exactly []string{thisCIDR}.
e9922fe to
1c90c66
Compare
…NSX-VPC - hack/nsx-operator-apis-fork: local copy of nsx-operator API types with updated IPAddressAllocation (multi-family CIDR fields) - routablepod/core.go: thread ipv4Enabled/ipv6Enabled/primaryIPFamilyIsIPv4 through to IPAddressAllocation and node controllers - routablepod/ipaddressallocation: allocate separate IPv4/IPv6 CRs in dual-stack - nsxipmanager/nsx_vpc.go: reconcile IPv4/IPv6 IPAddressAllocation objects - routemanager: propagate per-family CIDRs to StaticRoute CRs - cloud.go: derive IP-family flags from --cluster-ip-family and pass to StartControllers
1c90c66 to
54c942f
Compare
| // We deliberately do NOT parse the CR name to decide the family: node names | ||
| // that themselves end in "-ipv6" would otherwise be misclassified. | ||
| func isIPv4Allocation(alloc *vpcapisv1.IPAddressAllocation) bool { | ||
| return alloc.Spec.IPAddressType != vpcapisv1.IPAllocationIPAddressTypeIPv6 |
There was a problem hiding this comment.
According to method name, alloc.Spec.IPAddressType == vpcapisv1.IPAllocationIPAddressTypeIPv4 looks better
| } else { | ||
| // Dual-stack: list all allocations for this node. | ||
| // Dual-stack clusters are new and guaranteed to have the nodeName label on their CRs. | ||
| selector := labels.SelectorFromSet(labels.Set{helper.LabelKeyNodeName: nodeName}) |
There was a problem hiding this comment.
Should we also add label for cluster name or vsphere cluster name? There could be same nodeName in different clusters, for example, below 2 clusters all render node name coffee-worker-beijing
cluster coffee nodepool worker-beijing
cluster coffee-worker nodepool beijing
| Namespace: m.svNamespace, | ||
| Labels: map[string]string{ | ||
| helper.LabelKeyNodeName: nodeName, | ||
| helper.LabelKeyIPFamily: familyLabel, |
There was a problem hiding this comment.
The spec.IPAddressType contains ipfamily so I don't think this label is used anywhere. We need to add a label for VSphereCluster
| ctx := context.TODO() | ||
|
|
||
| for _, ipv4 := range families { | ||
| crName := crNameForFamily(node.Name, ipv4) |
There was a problem hiding this comment.
We should use label to get IPAddressAnnotation rather than name.
| // deleteIPAddressAllocation deletes the IPAddressAllocation CR for the given node and | ||
| // IP family. NotFound is treated as success (idempotent delete). | ||
| func (m *NSXVPCIPManager) deleteIPAddressAllocation(ctx context.Context, nodeName string, ipv4 bool) error { | ||
| crName := crNameForFamily(nodeName, ipv4) |
| // destination is actually IPv6, so an IPv4 route on a node whose | ||
| // name happens to end in "-ipv6" is not silently truncated. | ||
| nodeName := staticroute.Labels[helper.LabelKeyNodeName] | ||
| if nodeName == "" { |
There was a problem hiding this comment.
Raise an error till label is added by node controller.
| } | ||
| if podCIDR == "" { | ||
| return fmt.Errorf("IPAddressAllocation %v does not get CIDR allocated", ipAddressAllocation.Name) | ||
| return helper.StripFamilySuffix(alloc.Name) |
There was a problem hiding this comment.
Do not fallback, Raise an error till label is added by node controller.
What this PR does / why we need it:
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
Release note: