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 Signed-off-by: Arnab Ghosh <[email protected]>
ovn-org · Jun 4, 2024 · 672c0d3 · 672c0d3
1 parent aebd8c3
commit 672c0d3
Show file tree

Hide file tree

Showing 4 changed files with 186 additions and 9 deletions.
diff --git a/go-controller/pkg/node/gateway_iptables.go b/go-controller/pkg/node/gateway_iptables.go
@@ -64,6 +64,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 {
@@ -377,6 +382,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
@@ -385,6 +396,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
@@ -31,6 +31,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 {
@@ -75,6 +76,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{
@@ -2638,4 +2662,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
@@ -1728,7 +1728,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)
 				}
 			}
 		}
@@ -1833,17 +1837,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)
 		}
 	}