Setup v4 and v6 allowing rules when using bridges (#2401)

* refactor iptables/nftables rules to support setting up rules for arbitrary bridges

* some renaming

* renaming and log messages

* added iptables support

* typo fix

* use the common functions to setup iptables rules when using a bridge

* remove unused delete function

docs/manual/kinds/bridge.md

@@ -15,7 +15,7 @@ For example, by connecting a lab node to a bridge we can:
 3. scale out containerlab labs by running separate labs in different hosts and get network reachability between them
 4. wiring nodes' data interfaces via a broadcast domain (linux bridge) and use vlans to making dynamic connections
 
-<div class="mxgraph" style="max-width:100%;border:1px solid transparent;margin:0 auto; display:block;" data-mxgraph="{&quot;page&quot;:8,&quot;zoom&quot;:1.5,&quot;highlight&quot;:&quot;#0000ff&quot;,&quot;nav&quot;:true,&quot;check-visible-state&quot;:true,&quot;resize&quot;:true,&quot;url&quot;:&quot;https://raw.githubusercontent.com/srl-labs/containerlab/diagrams/containerlab.drawio&quot;}"></div>
+-{{diagram(url='srl-labs/containerlab/diagrams/containerlab.drawio', page='8', title='Using bridges')}}-
 
 ## Using bridge kind
 
@@ -53,9 +53,11 @@ In the example above, node `br-clab` of kind `bridge` tells containerlab to iden
 
 When connecting other nodes to a bridge, the bridge endpoint must be present in the `links` section.
 
-!!!note
-    When choosing names of the interfaces that need to be connected to the bridge make sure that these names are not clashing with existing interfaces.  
-    In the example above we named interfaces `eth1`, `eth2`, `eth3` accordingly and ensured that none of these interfaces existed before in the root netns.  
+/// admonition
+    type: subtle-note
+When choosing names of the interfaces that need to be connected to the bridge make sure that these names are not clashing with existing interfaces.  
+In the example above we named interfaces `eth1`, `eth2`, `eth3` accordingly and ensured that none of these interfaces existed before in the root netns.  
+///
 
 As a result of such topology definition, you will see bridge `br-clab` with three interfaces attached to it:
 
@@ -66,12 +68,12 @@ br-clab         8000.6281eb7133d2       no              eth1
                                                         eth3
 ```
 
-Containerlab automatically adds an iptables rule for the referenced bridges to allow forwarding over them. Namely, for a given bridge named `br-clab` containerlab will attempt to call the following iptables command during the lab deployment:
+Containerlab automatically adds iptables rules for the referenced bridges (v4 and v6) to allow traffic ingressing to the bridges. Namely, for a given bridge named `br-clab` containerlab will attempt to create the allowing rule in the filter table, FORWARD chain like this:
 
 ```
 iptables -I FORWARD -i br-clab -j ACCEPT
 ```
 
-This will ensure that traffic is forwarded when passing this particular bridge. Note, that once you destroy the lab, the rule will stay.
+This will ensure that traffic is forwarded when passing this particular bridge. Note, that once you destroy the lab, the rule will stay, if you wish to remove it, you will have to do it manually.
 
 Check out ["External bridge"](../../lab-examples/ext-bridge.md) lab for a ready-made example on how to use bridges.

nodes/bridge/bridge.go

@@ -6,10 +6,6 @@ package bridge
 
 import (
 	"context"
-	"fmt"
-	"os/exec"
-	"regexp"
-	"strings"
 
 	"github.com/containernetworking/plugins/pkg/ns"
 	log "github.com/sirupsen/logrus"
@@ -18,6 +14,8 @@ import (
 	"github.com/srl-labs/containerlab/nodes"
 	"github.com/srl-labs/containerlab/nodes/state"
 	"github.com/srl-labs/containerlab/runtime"
+	"github.com/srl-labs/containerlab/runtime/docker/firewall"
+	"github.com/srl-labs/containerlab/runtime/docker/firewall/definitions"
 	"github.com/srl-labs/containerlab/types"
 	"github.com/srl-labs/containerlab/utils"
 	"github.com/vishvananda/netlink"
@@ -26,9 +24,6 @@ import (
 var kindNames = []string{"bridge"}
 
 const (
-	iptCheckCmd = "-vL FORWARD -w 5"
-	iptAllowCmd = "-I FORWARD -i %s -j ACCEPT -w 5"
-
 	generateable     = true
 	generateIfFormat = "eth%d"
 )
@@ -64,7 +59,12 @@ func (n *bridge) Deploy(_ context.Context, _ *nodes.DeployParams) error {
 	return nil
 }
 
-func (*bridge) Delete(_ context.Context) error                { return nil }
+func (*bridge) Delete(_ context.Context) error {
+	// we are not deleting iptables rules set up in the post deploy stage
+	// because we can't guarantee that the bridge is not used by another topology.
+	return nil
+}
+
 func (*bridge) GetImages(_ context.Context) map[string]string { return map[string]string{} }
 
 // DeleteNetnsSymlink is a noop for bridge nodes.
@@ -87,38 +87,6 @@ func (b *bridge) CheckDeploymentConditions(_ context.Context) error {
 	return nil
 }
 
-// installIPTablesBridgeFwdRule calls iptables to install `allow` rule for traffic passing through the bridge
-// otherwise, communication over the bridge is not permitted.
-func (b *bridge) installIPTablesBridgeFwdRule() (err error) {
-	// first check if a rule already exists for this bridge to not create duplicates
-	res, err := exec.Command("iptables", strings.Split(iptCheckCmd, " ")...).Output()
-
-	re, _ := regexp.Compile(fmt.Sprintf("ACCEPT[^\n]+%s", b.Cfg.ShortName))
-
-	if re.Match(res) {
-		log.Debugf("found iptables forwarding rule targeting the bridge %q. Skipping creation of the forwarding rule.", b.Cfg.ShortName)
-		return err
-	}
-	if err != nil {
-		return fmt.Errorf("failed to add iptables forwarding rule for bridge %q: %w", b.Cfg.ShortName, err)
-	}
-
-	cmd := fmt.Sprintf(iptAllowCmd, b.Cfg.ShortName)
-
-	log.Debugf("Installing iptables rules for bridge %q", b.Cfg.ShortName)
-
-	stdOutErr, err := exec.Command("iptables", strings.Split(cmd, " ")...).CombinedOutput()
-
-	log.Debugf("iptables install stdout for bridge %s:%s", b.Cfg.ShortName, stdOutErr)
-
-	if err != nil {
-		log.Warnf("iptables install stdout/stderr result is: %s", stdOutErr)
-		return fmt.Errorf("unable to create iptables rules: %w", err)
-	}
-
-	return nil
-}
-
 func (*bridge) PullImage(_ context.Context) error { return nil }
 
 // UpdateConfigWithRuntimeInfo is a noop for bridges.
@@ -160,3 +128,15 @@ func (b *bridge) AddLinkToContainer(ctx context.Context, link netlink.Link, f fu
 func (b *bridge) GetLinkEndpointType() links.LinkEndpointType {
 	return links.LinkEndpointTypeBridge
 }
+
+// installIPTablesBridgeFwdRule installs `allow` rule for the traffic routed to ingress the bridge
+// otherwise, communication over the bridge is not permitted on most systems.
+func (b *bridge) installIPTablesBridgeFwdRule() (err error) {
+	f, err := firewall.NewFirewallClient()
+	if err != nil {
+		return err
+	}
+	log.Debugf("setting up bridge firewall rules using %s as the firewall interface", f.Name())
+
+	return f.InstallForwardingRules(b.Cfg.ShortName, "", definitions.ForwardChain)
+}

runtime/docker/docker.go

@@ -354,7 +354,7 @@ func (d *DockerRuntime) postCreateNetActions() (err error) {
 	if err != nil {
 		log.Warnf("failed to disable TX checksum offloading for the %s bridge interface: %v", d.mgmt.Bridge, err)
 	}
-	err = d.installFwdRule()
+	err = d.installMgmtNetworkFwdRule()
 	if err != nil {
 		log.Warnf("errors during iptables rules install: %v", err)
 	}
@@ -391,7 +391,7 @@ func (d *DockerRuntime) DeleteNet(ctx context.Context) (err error) {
 		return err
 	}
 
-	err = d.deleteFwdRule()
+	err = d.deleteMgmtNetworkFwdRule()
 	if err != nil {
 		log.Warnf("errors during iptables rules removal: %v", err)
 	}

runtime/docker/firewall.go

@@ -3,27 +3,30 @@ package docker
 import (
 	log "github.com/sirupsen/logrus"
 	"github.com/srl-labs/containerlab/runtime/docker/firewall"
+	"github.com/srl-labs/containerlab/runtime/docker/firewall/definitions"
 )
 
-// deleteFwdRule deletes `allow` rule installed with installFwdRule when the bridge interface doesn't exist anymore.
-func (d *DockerRuntime) deleteFwdRule() (err error) {
+// deleteMgmtNetworkFwdRule deletes `allow` rule installed with installFwdRule
+// when the containerlab management network (bridge) interface doesn't exist anymore.
+func (d *DockerRuntime) deleteMgmtNetworkFwdRule() (err error) {
 	if !*d.mgmt.ExternalAccess {
 		return
 	}
 
-	f, err := firewall.NewFirewallClient(d.mgmt.Bridge)
+	f, err := firewall.NewFirewallClient()
 	if err != nil {
 		return err
 	}
 
-	return f.DeleteForwardingRules()
+	return f.DeleteForwardingRules("", d.mgmt.Bridge, definitions.DockerUserChain)
 }
 
-// installFwdRule installs the `allow` rule for traffic destined to the nodes
+// installMgmtNetworkFwdRule installs the `allow` rule for traffic destined to the nodes
 // on the clab management network for v4 and v6.
 // This rule is required for external access to the nodes.
-func (d *DockerRuntime) installFwdRule() (err error) {
+func (d *DockerRuntime) installMgmtNetworkFwdRule() (err error) {
 	if !*d.mgmt.ExternalAccess {
+		log.Debug("skipping setup of forwarding rules for the management network since External Access is disabled by a user")
 		return
 	}
 
@@ -32,11 +35,13 @@ func (d *DockerRuntime) installFwdRule() (err error) {
 		return
 	}
 
-	f, err := firewall.NewFirewallClient(d.mgmt.Bridge)
+	f, err := firewall.NewFirewallClient()
 	if err != nil {
 		return err
 	}
 	log.Debugf("using %s as the firewall interface", f.Name())
 
-	return f.InstallForwardingRules()
+	// install the rules with the management bridge listed as the outgoing interface with the allow action
+	// in the DOCKER-USER chain
+	return f.InstallForwardingRules("", d.mgmt.Bridge, definitions.DockerUserChain)
 }

runtime/docker/firewall/definitions/definitions.go

@@ -5,8 +5,9 @@ import "errors"
 var ErrNotAvailable = errors.New("not available")
 
 const (
-	DockerFWUserChain = "DOCKER-USER"
-	DockerFWTable     = "filter"
+	DockerUserChain = "DOCKER-USER"
+	ForwardChain    = "FORWARD"
+	FilterTable     = "filter"
 
 	IPTablesRuleComment = "set by containerlab"
 
@@ -15,7 +16,7 @@ const (
 
 // ClabFirewall is the interface that all firewall clients must implement.
 type ClabFirewall interface {
-	DeleteForwardingRules() error
-	InstallForwardingRules() error
+	DeleteForwardingRules(inInterface, outInterface, chain string) error
+	InstallForwardingRules(inInterface, outInterface, chain string) error
 	Name() string
 }

runtime/docker/firewall/firewall.go

@@ -7,15 +7,15 @@ import (
 )
 
 // NewFirewallClient returns a firewall client based on the availability of nftables or iptables.
-func NewFirewallClient(bridgeName string) (definitions.ClabFirewall, error) {
+func NewFirewallClient() (definitions.ClabFirewall, error) {
 	var clf definitions.ClabFirewall
 
-	clf, err := nftables.NewNftablesClient(bridgeName)
+	clf, err := nftables.NewNftablesClient()
 	if err == nil {
 		return clf, nil
 	}
 
-	clf, err = iptables.NewIpTablesClient(bridgeName)
+	clf, err = iptables.NewIpTablesClient()
 	if err == nil {
 		return clf, nil
 	}