Implementation required to enable Forwarding if it is already disabled

This PR is to remove iptables rules from FORWARD chain if config.Gateway.DisableForwarding is set to false. Following iptables rules gets removed from FORWARD chain given 10.1.0.0/16 is clusterNetwork CIDR and 10.96.0.0/16 is serviceNetwork CIDR. -A FORWARD -s 10.96.0.0/16 -j ACCEPT -A FORWARD -d 10.96.0.0/16 -j ACCEPT -A FORWARD -s 169.254.169.1 -j ACCEPT -A FORWARD -d 169.254.169.1 -j ACCEPT -A FORWARD -d 10.1.0.0/16 -j ACCEPT -A FORWARD -s 10.1.0.0/16 -j ACCEPT -A FORWARD -i breth1 -j DROP -A FORWARD -o breth1 -j DROP Jira: https://issues.redhat.com/browse/OCPBUGS-23758 Signed-off-by: Arnab Ghosh <[email protected]>
ovn-org · Jun 4, 2024 · 586aee8 · 586aee8
1 parent 47fcdd2
commit 586aee8
Show file tree

Hide file tree

Showing 6 changed files with 225 additions and 9 deletions.
diff --git a/docs/developer-guide/configuration.md b/docs/developer-guide/configuration.md
diff --git a/docs/getting-started/configuration.md b/docs/getting-started/configuration.md
@@ -0,0 +1,39 @@
+## Disable Forwarding Config
+
+OVN-Kubernetes allows to enable or disable IP forwarding for all traffic on OVN-Kubernetes managed interfaces (such as br-ex). By default forwarding is enabled and this allows host to forward traffic across OVN-Kubernetes managed interfaces. If forwarding is disabled then Kubernetes related traffic is still forwarded appropriately, but other IP traffic will not be routed by cluster nodes.
+
+IP forwarding is implemented at cluster node level by modifying both iptables `FORWARD` chain and IP forwarding `sysctl` parameters. 
+
+- If forwarding is enabled(default) then system administrator need to set following sysctl parameters. An operator can be built to manage forwarding sysctl parmeters based on forwarding mode. No extra iptables rules are added by OVN-Kubernetes to FORWARD chain while using this IP forwarding mode.
+
+```
+net.ipv4.ip_forward=1
+net.ipv6.conf.all.forwarding=1
+```
+
+- IP forwarding can be disabled either by setting `disable-forwarding` command line option to `true` while starting ovnkube or by setting `disable-forwarding` to `true` in config file. If forwarding is disabled then system administrator need to set following sysctl parameters to stop routing other IP traffic. An operator can be built to manage forwarding sysctl parmeters based on forwarding mode.
+
+```
+net.ipv4.ip_forward=0
+net.ipv6.conf.all.forwarding=0
+```
+
+When IP forwarding is disabled, following sysctl parameters are modified by OVN-Kubernetes to allow forwarding Kubernetes related traffic on OVN-Kubernetes managed bridge interfaces and management port interface.
+
+```
+net.ipv4.conf.br-ex.forwarding=1
+net.ipv4.conf.ovn-k8s-mp0.forwarding = 1
+```
+
+Additionally following iptables rules are added at FORWARD chain to forward clusterNetwork and serviceNetwork traffic to their intended destinations. 
+
+```
+-A FORWARD -s 10.128.0.0/14 -j ACCEPT
+-A FORWARD -d 10.128.0.0/14 -j ACCEPT
+-A FORWARD -s 169.254.169.1 -j ACCEPT
+-A FORWARD -d 169.254.169.1 -j ACCEPT
+-A FORWARD -d 172.16.1.0/24 -j ACCEPT
+-A FORWARD -s 172.16.1.0/24 -j ACCEPT
+-A FORWARD -i breth1 -j DROP
+-A FORWARD -o breth1 -j DROP
+```
diff --git a/go-controller/pkg/node/gateway_iptables.go b/go-controller/pkg/node/gateway_iptables.go
@@ -72,6 +72,11 @@ func appendIptRules(rules []nodeipt.Rule) error {
 	return nodeipt.AddRules(rules, true)
 }
 
+// deleteIptRules removes provided rules from the chain
+func deleteIptRules(rules []nodeipt.Rule) error {
+	return nodeipt.DelRules(rules)
+}
+
 func getGatewayInitRules(chain string, proto iptables.Protocol) []nodeipt.Rule {
 	iptRules := []nodeipt.Rule{}
 	if chain == egressservice.Chain {
@@ -396,6 +401,12 @@ func initExternalBridgeServiceForwardingRules(cidrs []*net.IPNet) error {
 	return insertIptRules(getGatewayForwardRules(cidrs))
 }
 
+// delExternalBridgeServiceForwardingRules removes iptables rules which might
+// have been added to disable forwarding
+func delExternalBridgeServiceForwardingRules(cidrs []*net.IPNet) error {
+	return deleteIptRules(getGatewayForwardRules(cidrs))
+}
+
 // initExternalBridgeDropRules sets up iptables rules to block forwarding
 // in br-* interfaces (also for 2ndary bridge) - we block for v4 and v6 based on clusterStack
 // -A FORWARD -i breth1 -j DROP
@@ -404,6 +415,12 @@ func initExternalBridgeDropForwardingRules(ifName string) error {
 	return appendIptRules(getGatewayDropRules(ifName))
 }
 
+// delExternalBridgeDropForwardingRules removes iptables rules which might
+// have been added to disable forwarding
+func delExternalBridgeDropForwardingRules(ifName string) error {
+	return deleteIptRules(getGatewayDropRules(ifName))
+}
+
 func getLocalGatewayFilterRules(ifname string, cidr *net.IPNet) []nodeipt.Rule {
 	// Allow packets to/from the gateway interface in case defaults deny
 	protocol := getIPTablesProtocol(cidr.IP.String())

diff --git a/go-controller/pkg/node/gateway_localnet.go b/go-controller/pkg/node/gateway_localnet.go
@@ -81,7 +81,11 @@ func newLocalGateway(nodeName string, hostSubnets []*net.IPNet, gwNextHops []net
 			}
 			if config.Gateway.DisableForwarding {
 				if err := initExternalBridgeDropForwardingRules(exGwBridge.bridgeName); err != nil {
-					return fmt.Errorf("failed to add forwarding block rules for bridge %s: err %v", exGwBridge.bridgeName, err)
+					return fmt.Errorf("failed to add drop rules in forwarding table for bridge %s: err %v", exGwBridge.bridgeName, err)
+				}
+			} else {
+				if err := delExternalBridgeDropForwardingRules(exGwBridge.bridgeName); err != nil {
+					return fmt.Errorf("failed to delete drop rules in forwarding table for bridge %s: err %v", exGwBridge.bridgeName, err)
 				}
 			}
 		}

diff --git a/go-controller/pkg/node/gateway_localnet_linux_test.go b/go-controller/pkg/node/gateway_localnet_linux_test.go
@@ -33,6 +33,7 @@ const (
 	v4localnetGatewayIP = "10.244.0.1"
 	v6localnetGatewayIP = "fd00:96:1::1"
 	gwMAC               = "0a:0b:0c:0d:0e:0f"
+	linkName            = "breth0"
 )
 
 func initFakeNodePortWatcher(iptV4, iptV6 util.IPTablesHelper) *nodePortWatcher {
@@ -77,6 +78,29 @@ func startNodePortWatcher(n *nodePortWatcher, fakeClient *util.OVNNodeClientset,
 	ip, ipnet, _ := net.ParseCIDR(localHostNetEp)
 	n.nodeIPManager.addAddr(net.IPNet{IP: ip, Mask: ipnet.Mask})
 
+	// Add or delete iptables rules from FORWARD chain based on DisableForwarding. This is
+	// to imitate addition or deletion of iptales rules done in newNodePortWatcher().
+	var subnets []*net.IPNet
+	for _, subnet := range config.Default.ClusterSubnets {
+		subnets = append(subnets, subnet.CIDR)
+	}
+	subnets = append(subnets, config.Kubernetes.ServiceCIDRs...)
+	if config.Gateway.DisableForwarding {
+		if err := initExternalBridgeServiceForwardingRules(subnets); err != nil {
+			return fmt.Errorf("failed to add accept rules in forwarding table for bridge %s: err %v", linkName, err)
+		}
+		if err := initExternalBridgeDropForwardingRules(linkName); err != nil {
+			return fmt.Errorf("failed to add drop rules in forwarding table for bridge %s: err %v", linkName, err)
+		}
+	} else {
+		if err := delExternalBridgeServiceForwardingRules(subnets); err != nil {
+			return fmt.Errorf("failed to delete accept rules in forwarding table for bridge %s: err %v", linkName, err)
+		}
+		if err := delExternalBridgeDropForwardingRules(linkName); err != nil {
+			return fmt.Errorf("failed to delete drop rules in forwarding table for bridge %s: err %v", linkName, err)
+		}
+	}
+
 	// set up a controller to handle events on services to mock the nodeportwatcher bits
 	// in gateway.go and trigger code in gateway_shared_intf.go
 	_, err := n.watchFactory.AddServiceHandler(cache.ResourceEventHandlerFuncs{
@@ -2758,4 +2782,125 @@ var _ = Describe("Node Operations", func() {
 			Expect(err).NotTo(HaveOccurred())
 		})
 	})
+
+	Context("disable-forwarding", func() {
+		It("adds or removes iptables rules upon change in forwarding mode", func() {
+			app.Action = func(ctx *cli.Context) error {
+				config.Gateway.DisableForwarding = true
+				fakeOvnNode.start(ctx)
+				fNPW.watchFactory = fakeOvnNode.watcher
+				Expect(startNodePortWatcher(fNPW, fakeOvnNode.fakeClient, &fakeMgmtPortConfig)).To(Succeed())
+				expectedTables := map[string]util.FakeTable{
+					"nat": {
+						"PREROUTING": []string{
+							"-j OVN-KUBE-ETP",
+							"-j OVN-KUBE-EXTERNALIP",
+							"-j OVN-KUBE-NODEPORT",
+						},
+						"OUTPUT": []string{
+							"-j OVN-KUBE-EXTERNALIP",
+							"-j OVN-KUBE-NODEPORT",
+							"-j OVN-KUBE-ITP",
+						},
+						"OVN-KUBE-NODEPORT":   []string{},
+						"OVN-KUBE-EXTERNALIP": []string{},
+						"POSTROUTING": []string{
+							"-j OVN-KUBE-EGRESS-SVC",
+						},
+						"OVN-KUBE-SNAT-MGMTPORT": []string{},
+						"OVN-KUBE-ETP":           []string{},
+						"OVN-KUBE-ITP":           []string{},
+						"OVN-KUBE-EGRESS-SVC":    []string{},
+					},
+					"filter": {
+						"FORWARD": []string{
+							"-d 169.254.169.1 -j ACCEPT",
+							"-s 169.254.169.1 -j ACCEPT",
+							"-d 172.16.1.0/24 -j ACCEPT",
+							"-s 172.16.1.0/24 -j ACCEPT",
+							"-d 10.1.0.0/16 -j ACCEPT",
+							"-s 10.1.0.0/16 -j ACCEPT",
+							"-i breth0 -j DROP",
+							"-o breth0 -j DROP",
+						},
+					},
+					"mangle": {
+						"OUTPUT": []string{
+							"-j OVN-KUBE-ITP",
+						},
+						"OVN-KUBE-ITP": []string{},
+					},
+				}
+
+				f4 := iptV4.(*util.FakeIPTables)
+				err := f4.MatchState(expectedTables)
+				Expect(err).NotTo(HaveOccurred())
+				expectedTables = map[string]util.FakeTable{
+					"nat":    {},
+					"filter": {},
+					"mangle": {},
+				}
+				f6 := iptV6.(*util.FakeIPTables)
+				err = f6.MatchState(expectedTables)
+				Expect(err).NotTo(HaveOccurred())
+
+				// Enable forwarding and test deletion of iptables rules from FORWARD chain
+				config.Gateway.DisableForwarding = false
+				fNPW.watchFactory = fakeOvnNode.watcher
+				Expect(startNodePortWatcher(fNPW, fakeOvnNode.fakeClient, &fakeMgmtPortConfig)).To(Succeed())
+				expectedTables = map[string]util.FakeTable{
+					"nat": {
+						"PREROUTING": []string{
+							"-j OVN-KUBE-ETP",
+							"-j OVN-KUBE-EXTERNALIP",
+							"-j OVN-KUBE-NODEPORT",
+						},
+						"OUTPUT": []string{
+							"-j OVN-KUBE-EXTERNALIP",
+							"-j OVN-KUBE-NODEPORT",
+							"-j OVN-KUBE-ITP",
+						},
+						"OVN-KUBE-NODEPORT":   []string{},
+						"OVN-KUBE-EXTERNALIP": []string{},
+						"POSTROUTING": []string{
+							"-j OVN-KUBE-EGRESS-SVC",
+						},
+						"OVN-KUBE-SNAT-MGMTPORT": []string{},
+						"OVN-KUBE-ETP":           []string{},
+						"OVN-KUBE-ITP":           []string{},
+						"OVN-KUBE-EGRESS-SVC":    []string{},
+					},
+					"filter": {
+						"FORWARD": []string{},
+					},
+					"mangle": {
+						"OUTPUT": []string{
+							"-j OVN-KUBE-ITP",
+						},
+						"OVN-KUBE-ITP": []string{},
+					},
+				}
+
+				f4 = iptV4.(*util.FakeIPTables)
+				err = f4.MatchState(expectedTables)
+				Expect(err).NotTo(HaveOccurred())
+				expectedTables = map[string]util.FakeTable{
+					"nat":    {},
+					"filter": {},
+					"mangle": {},
+				}
+				f6 = iptV6.(*util.FakeIPTables)
+				err = f6.MatchState(expectedTables)
+				Expect(err).NotTo(HaveOccurred())
+				return nil
+			}
+			err := app.Run([]string{
+				app.Name,
+				"--cluster-subnets=" + "10.1.0.0/16",
+				"--k8s-service-cidrs=" + "172.16.1.0/24",
+			})
+			Expect(err).NotTo(HaveOccurred())
+		})
+	})
+
 })
diff --git a/go-controller/pkg/node/gateway_shared_intf.go b/go-controller/pkg/node/gateway_shared_intf.go
@@ -1760,7 +1760,11 @@ func newSharedGateway(nodeName string, subnets []*net.IPNet, gwNextHops []net.IP
 			}
 			if config.Gateway.DisableForwarding {
 				if err := initExternalBridgeDropForwardingRules(exGwBridge.bridgeName); err != nil {
-					return fmt.Errorf("failed to add forwarding block rules for bridge %s: err %v", exGwBridge.bridgeName, err)
+					return fmt.Errorf("failed to add drop rules in forwarding table for bridge %s: err %v", exGwBridge.bridgeName, err)
+				}
+			} else {
+				if err := delExternalBridgeDropForwardingRules(exGwBridge.bridgeName); err != nil {
+					return fmt.Errorf("failed to delete drop rules in forwarding table for bridge %s: err %v", exGwBridge.bridgeName, err)
 				}
 			}
 		}
@@ -1865,17 +1869,24 @@ func newNodePortWatcher(gwBridge *bridgeConfiguration, ofm *openflowManager,
 		}
 	}
 
+	var subnets []*net.IPNet
+	for _, subnet := range config.Default.ClusterSubnets {
+		subnets = append(subnets, subnet.CIDR)
+	}
+	subnets = append(subnets, config.Kubernetes.ServiceCIDRs...)
 	if config.Gateway.DisableForwarding {
-		var subnets []*net.IPNet
-		for _, subnet := range config.Default.ClusterSubnets {
-			subnets = append(subnets, subnet.CIDR)
-		}
-		subnets = append(subnets, config.Kubernetes.ServiceCIDRs...)
 		if err := initExternalBridgeServiceForwardingRules(subnets); err != nil {
-			return nil, fmt.Errorf("failed to add forwarding rules for bridge %s: err %v", gwBridge.bridgeName, err)
+			return nil, fmt.Errorf("failed to add accept rules in forwarding table for bridge %s: err %v", gwBridge.bridgeName, err)
 		}
 		if err := initExternalBridgeDropForwardingRules(gwBridge.bridgeName); err != nil {
-			return nil, fmt.Errorf("failed to add forwarding rules for bridge %s: err %v", gwBridge.bridgeName, err)
+			return nil, fmt.Errorf("failed to add drop rules in forwarding table for bridge %s: err %v", gwBridge.bridgeName, err)
+		}
+	} else {
+		if err := delExternalBridgeServiceForwardingRules(subnets); err != nil {
+			return nil, fmt.Errorf("failed to delete accept rules in forwarding table for bridge %s: err %v", gwBridge.bridgeName, err)
+		}
+		if err := delExternalBridgeDropForwardingRules(gwBridge.bridgeName); err != nil {
+			return nil, fmt.Errorf("failed to delete drop rules in forwarding table for bridge %s: err %v", gwBridge.bridgeName, err)
 		}
 	}