Update nat discovery to support IP family #3311
Conversation
yboaron
requested review from
Oats87,
skitt,
sridhargaddam,
tpantelis and
vthapar
as code owners
|
🤖 Created branch: z_pr3311/yboaron/natd_v6 |
Current code assumes 1:1 mapping between endpoint to nat-discovery and endpoint to submariner inter-cluster tunnel. Since we decided that we continue using a single endpoint for each cluster also for dual-stack, it is necessary to support mapping endpoint up to 2 nat-discovery and inter-cluster tunnels. Additionally, nat-discovery and submariner-tunnel also need to support IP family parameter, as both IPv4 and IPv6 should be supported. This change update nat-discovery to support IP family,to make it clear, only IPV4 nat-discovery is supported even after this change. Signed-off-by: Yossi Boaron <[email protected]>
| @@ -156,13 +157,13 @@ func (i *engine) installCableWithNATInfo(rnat *natdiscovery.NATEndpointInfo) err | |||
| i.Lock() | |||
| defer i.Unlock() | |||
|
|
|||
| if _, ok := i.natDiscoveryPending[rnat.Endpoint.Spec.CableName]; !ok { | |||
| if _, ok := i.natDiscoveryPending[rnat.Endpoint.Spec.GetFamilyCableName(rnat.Family)]; !ok { | |||
There was a problem hiding this comment.
We could extract GetFamilyCableName to a var as it's referenced several times.
|
|
||
| i.natDiscovery.RemoveEndpoint(endpoint.Spec.CableName) | ||
| i.natDiscovery.RemoveEndpoint(endpoint.Spec.GetFamilyCableName(family)) |
There was a problem hiding this comment.
Same here re: extract GetFamilyCableName to a var.
| @@ -274,9 +275,9 @@ func (i *engine) RemoveCable(endpoint *v1.Endpoint) error { | |||
| return errors.Wrapf(err, "error disconnecting Endpoint cable %q", endpoint.Spec.CableName) | |||
| } | |||
|
|
|||
| delete(i.installedCables, endpoint.Spec.CableName) | |||
| delete(i.installedCables, endpoint.Spec.GetFamilyCableName(family)) | |||
There was a problem hiding this comment.
We need to be consistent and also index by GetFamilyCableName everywhere we access installedCables (L269 above and in installCableWithNATInfo).
| delete(i.natDiscoveryPending, rnat.Endpoint.Spec.CableName) | ||
| i.natDiscoveryPending[rnat.Endpoint.Spec.GetFamilyCableName(rnat.Family)]-- | ||
| if i.natDiscoveryPending[rnat.Endpoint.Spec.GetFamilyCableName(rnat.Family)] == 0 { | ||
| delete(i.natDiscoveryPending, rnat.Endpoint.Spec.GetFamilyCableName(rnat.Family)) |
There was a problem hiding this comment.
The code below where we check for an active connection is going to also have to take into account the family. I think we're going to have to add a UsingIPFamily field to the Connection struct. But we can address this later when the cable drivers are modified to use the family.
| @@ -172,3 +172,15 @@ func (ep *EndpointSpec) GetPrivateIP(family k8snet.IPFamily) string { | |||
| func (ep *EndpointSpec) SetPrivateIP(ip string) { | |||
| ep.PrivateIPs, ep.PrivateIP = setIP(ep.PrivateIPs, ep.PrivateIP, ip) | |||
| } | |||
|
|
|||
| func (ep *EndpointSpec) GetIPFamilies() [2]k8snet.IPFamily { | |||
There was a problem hiding this comment.
This actually returns an array of length 2 with the second element empty: ["4", ""]. We should return []k8snet.IPFamily since it can have 0, 1, or 2 elements.
We should be able to implement this method for real now and add unit tests since we only populate Subnets with IP4.
ipFamilies := set.New[k8snet.IPFamily]()
for _, cidr := range ep.Subnets {
f := k8snet.IPFamilyOfCIDRString(cidr)
if f != k8snet.IPFamilyUnknown {
ipFamilies.Insert(f)
}
}
return ipFamilies.UnsortedList()
| return ipFamilies | ||
| } | ||
|
|
||
| func (ep *EndpointSpec) GetFamilyCableName(family k8snet.IPFamily) string { |
There was a problem hiding this comment.
Please add a unit test for this function so we don't lose any coverage percentage. You can simply check that the string starts with the cable name and ends with the family.
| } | ||
|
|
||
| var logger = log.Logger{Logger: logf.Log.WithName("Tunnel")} | ||
|
|
||
| func findCommonIPFamilies(local, remote [2]k8snet.IPFamily) []k8snet.IPFamily { |
There was a problem hiding this comment.
| func findCommonIPFamilies(local, remote [2]k8snet.IPFamily) []k8snet.IPFamily { | |
| func findCommonIPFamilies(local, remote []k8snet.IPFamily) []k8snet.IPFamily { |
| common := []k8snet.IPFamily{} | ||
|
|
||
| for _, lf := range local { | ||
| for _, rf := range remote { | ||
| if lf == rf { | ||
| common = append(common, lf) | ||
| break | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return common |
There was a problem hiding this comment.
You can simplify to (import "github.com/submariner-io/admiral/pkg/slices"):
| common := []k8snet.IPFamily{} | |
| for _, lf := range local { | |
| for _, rf := range remote { | |
| if lf == rf { | |
| common = append(common, lf) | |
| break | |
| } | |
| } | |
| } | |
| return common | |
| return slices.Intersect(local, remote, func(e k8snet.IPFamily) k8snet.IPFamily { | |
| return e | |
| }) |
| } | ||
|
|
||
| return "" | ||
| } |
There was a problem hiding this comment.
We can restructure and simply these functions as follows:
func getLocalIPv4ForDestination(dst string) string {
...
}
func GetLocalIPForDestination(dst string, family k8snet.IPFamily) string {
switch family {
case k8snet.IPv4:
return getLocalIPv4ForDestination(dst)
case k8snet.IPv6:
// TODO_IPV6: add V6 healthcheck IP
case k8snet.IPFamilyUnknown:
}
return ""
}
func GetLocalIP(family k8snet.IPFamily) string {
return GetLocalIPForDestination("8.8.8.8", family)
}
| @@ -64,7 +69,7 @@ func (nd *natDiscovery) handleRequestFromAddress(req *proto.SubmarinerNATDiscove | |||
| return nd.sendResponseToAddress(&response, addr) | |||
| } | |||
|
|
|||
| if req.GetReceiver().GetEndpointId() != localEndpointSpec.CableName { | |||
| if req.GetReceiver().GetEndpointId() != localEndpointSpec.GetFamilyCableName(k8snet.IPv4) { | |||
There was a problem hiding this comment.
Should we assume v4 here?
Current code assumes 1:1 mapping between endpoint to nat-discovery and
endpoint to submariner inter-cluster tunnel.
Since we decided that we continue using a single endpoint for each cluster
also for dual-stack, it is necessary to support mapping endpoint up to 2
nat-discovery and inter-cluster tunnels.
Additionally, nat-discovery and submariner-tunnel also need
to support IP family parameter, as both IPv4 and IPv6 should be supported.
This change update nat-discovery to support IP family, to make it clear,
only IPV4 nat-discovery is supported even after this change.