netpol: Add dual-stack support #1280

vadorovsky · 2022-04-06T13:58:26Z

This change allows to define two cluster CIDRs for compatibility with
Kubernetes dual-stack, with an assumption that two CIDRs are usually
IPv4 and IPv6.

Signed-off-by: Michal Rostecki [email protected]

mrueg · 2022-04-06T16:03:04Z

docs/user-guide.md

@@ -56,6 +56,8 @@ Usage of kube-router:
      --disable-source-dest-check                     Disable the source-dest-check attribute for AWS EC2 instances. When this option is false, it must be set some other way. (default true)
      --enable-cni                                    Enable CNI plugin. Disable if you want to use kube-router features alongside another CNI plugin. (default true)
      --enable-ibgp                                   Enables peering with nodes with the same ASN, if disabled will only peer with external BGP peers (default true)
+      --enable-ipv4                                   Enables IPv4 support (default true)
+      --enable-ipv6                                   Enables IPv6 support (default true)


should we enable this by default?

I can disable it by default, if you want.

I'm just wondering on implications to existing systems that don't use ipv6 currently. Maybe introduce as disabled and enable later?

Alright, changed to false

@vadorovsky it looks like this is still set to enabled by default. Given that it's going to fail to initialize the NPC if IPv6 is enabled and there is not an IPv6 address on the node in the Kubernetes API, I think that enabling IPv6 is too risky as a default.

aauren · 2022-04-07T19:29:39Z

@vadorovsky Thanks for submitting, will try to look at it tomorrow.

aauren · 2022-04-11T17:23:33Z

@vadorovsky apologies for not getting to this on Friday like I wanted to!

This is a big PR and so I want to give it the time it deserves to review and test. Had a bunch of work stuff come up last week that is continuing into this week, but I wanted to let you know this is still on my radar and still very much something that I want to get into this week.

This change allows to define two cluster CIDRs for compatibility with Kubernetes dual-stack, with an assumption that two CIDRs are usually IPv4 and IPv6. Signed-off-by: Michal Rostecki <[email protected]>

aauren

@vadorovsky Again, sorry that it took so long to finish this review. I had to take it in parts because this change is so large.

First off, thanks very much for being willing to work on such a sizable change. Just reviewing your refactor here made my head hurt a bit, so I can imagine it was difficult to put all this together from scratch and keep all the pieces straight.

I think that almost all of my comments are minor things, so try not to get disappointed by the ~30 changes I'm requesting here. On the whole this seems very well done.

So far this review only covers the code changes, I haven't gotten a chance to run this. I intend to do so over the next couple of days and put it through its paces, but in the meantime I wanted to get you feedback as soon as possible.

A request, when you work on updates, please don't squash or amend them to the existing commit, please leave them as a separate commit so that I can compare the original work against the requested changes. It will really help the final approval go much faster.

Again, thanks for your work!

aauren · 2022-04-19T19:51:52Z

pkg/cmd/kube-router.go

@@ -184,8 +188,39 @@ func (kr *KubeRouter) Run() error {
 	}

 	if kr.Config.RunFirewall {
+		iptablesCmdHandlers := make(map[v1core.IPFamily]utils.IPTablesHandler, 2)


It doesn't seem like there a particular need to move iptables and ipset logic up here into the command handler. If not, it would be better to sink this down into the NPC, into the NewNetworkPolicyController() initializer.

aauren · 2022-04-19T20:03:00Z

pkg/utils/iptables.go

 )

 var hasWait bool

+// Interface based on the IPTables struct from github.com/coreos/go-iptables
+// which allows to mock it.
+type IPTablesHandler interface {


Its a pity that the upstream package doesn't provide this interface for us. 😞 Thanks for creating it.

aauren · 2022-04-19T20:10:24Z

pkg/utils/iptables.go

+	buffer.WriteString(ruleStr)
+}
+
+type IPTablesSaveRestore struct {


Can you see any reason not to make this an interface as well? It would essentially just expose the SaveInto and Restore functions, that way we could mock it other places in the NPC as well if needed?

aauren · 2022-04-19T20:31:04Z

pkg/utils/node.go

+func GetNodeIPDualStack(node *apiv1.Node, enableIPv4, enableIPv6 bool) (net.IP, net.IP, error) {
+	var ipAddrv4, ipAddrv6 net.IP
+	addresses := node.Status.Addresses
+	addressesPerType := make(addressMap)


It seems like this would be a good opportunity to refactor GetNodeIP() as well, and consolidate it to use this same logic.

Perhaps the add() method above could be changed into more of a GetNodeAddressesByType() method, and then it could do the iteration over addresses and just return the entire map to both GetNodeIP and GetNodeIPDualStack?

aauren · 2022-04-19T20:32:15Z

pkg/utils/node.go

+	if _, ok := m[address.Type]; ok {
+		m[address.Type] = append(m[address.Type], address)
+	} else {
+		// There can be at most 2 addresses of the same type.


Can you explain what you mean by this comment and the 2 sizing? Is this something that the Kubernetes API guarantees?

aauren · 2022-04-19T22:32:06Z

pkg/controllers/netpol/pod.go

 	for _, obj := range npc.podLister.List() {
 		pod := obj.(*api.Pod)
 		// ignore the pods running on the different node and pods that are not actionable
 		if strings.Compare(pod.Status.HostIP, nodeIP) != 0 || !isNetPolActionable(pod) {
 			continue
 		}
-		localPods[pod.Status.PodIP] = podInfo{ip: pod.Status.PodIP,
+		localPods[pod.Status.PodIP] = podInfo{ips: pod.Status.PodIPs,


See comment above, but it might be beneficial to load both the ip as well as ips for the pod object here.

aauren · 2022-04-19T22:36:14Z

pkg/controllers/netpol/utils.go

@@ -8,6 +8,8 @@ import (

 	"github.com/cloudnativelabs/kube-router/pkg/utils"
 	api "k8s.io/api/core/v1"
+	"k8s.io/klog/v2"
+	utilsnet "k8s.io/utils/net"


Can fix the imports to consistently use either netutils or utilsnet across all the imports? It makes it a little easier to understand.

aauren · 2022-04-20T13:54:46Z

pkg/controllers/netpol/policy.go

 	activePolicyChains := make(map[string]bool)
 	activePolicyIPSets := make(map[string]bool)

+	// for ipFamily, ipset := range npc.ipSetHandlers {


This can probably be removed.

aauren · 2022-04-20T14:06:45Z

pkg/controllers/netpol/policy.go

+				for _, podIP := range currentPodIPs[ipFamily] {
+					setEntries = append(setEntries, []string{podIP, utils.OptionTimeout, "0"})
+				}
+				ipset.RefreshSet(targetDestPodIPSetName, setEntries, utils.TypeHashIP)


Is there a reason not to use createGenericHashIPSet() here rather than building the entries yourself? It already accepts ipFamily as a parameter, so you would just pass that and it would end up in the correct set.

aauren · 2022-04-20T14:07:12Z

pkg/controllers/netpol/policy.go

+				for _, podIP := range currentPodIPs[ipFamily] {
+					setEntries = append(setEntries, []string{podIP, utils.OptionTimeout, "0"})
+				}
+				ipset.RefreshSet(targetSourcePodIPSetName, setEntries, utils.TypeHashIP)


Same here on using createGenericHashIPSet()

aauren · 2022-08-06T02:14:13Z

Closed in favor of #1343

vadorovsky marked this pull request as draft April 6, 2022 13:58

vadorovsky mentioned this pull request Apr 6, 2022

utils(iptables): Factor out a common function for executing commands #1277

Closed

mrueg reviewed Apr 6, 2022

View reviewed changes

vadorovsky force-pushed the netpol-dual-stack branch from edc1d4c to 632d42f Compare April 7, 2022 16:30

netpol: Add dual-stack support

24b7390

This change allows to define two cluster CIDRs for compatibility with Kubernetes dual-stack, with an assumption that two CIDRs are usually IPv4 and IPv6. Signed-off-by: Michal Rostecki <[email protected]>

vadorovsky force-pushed the netpol-dual-stack branch from 632d42f to 24b7390 Compare April 12, 2022 14:04

vadorovsky marked this pull request as ready for review April 19, 2022 11:34

aauren requested changes Apr 20, 2022

View reviewed changes

aauren mentioned this pull request May 27, 2022

change package name from v1core to corev1 #1281

Closed

thomasferrandiz mentioned this pull request Aug 2, 2022

netpol: Add dual-stack support (updated) #1343

Closed

aauren closed this Aug 6, 2022

aauren mentioned this pull request Oct 25, 2022

Dual Stack Additions #1386

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

netpol: Add dual-stack support #1280

netpol: Add dual-stack support #1280

vadorovsky commented Apr 6, 2022

mrueg Apr 6, 2022

vadorovsky Apr 6, 2022

mrueg Apr 6, 2022

vadorovsky Apr 7, 2022

aauren Apr 19, 2022

aauren commented Apr 7, 2022

aauren commented Apr 11, 2022

aauren left a comment

aauren Apr 19, 2022

aauren Apr 19, 2022

aauren Apr 19, 2022

aauren Apr 19, 2022

aauren Apr 19, 2022

aauren Apr 19, 2022

aauren Apr 19, 2022

aauren Apr 20, 2022

aauren Apr 20, 2022

aauren Apr 20, 2022

aauren commented Aug 6, 2022

netpol: Add dual-stack support #1280

netpol: Add dual-stack support #1280

Conversation

vadorovsky commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aauren commented Apr 7, 2022

aauren commented Apr 11, 2022

aauren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aauren commented Aug 6, 2022