Skip to content

Commit

Permalink
networking: refactor some iptables for testability (#23856)
Browse files Browse the repository at this point in the history
  • Loading branch information
gulducat authored Aug 23, 2024
1 parent 36522ec commit a6e2905
Show file tree
Hide file tree
Showing 5 changed files with 256 additions and 106 deletions.
54 changes: 5 additions & 49 deletions client/allocrunner/networking_bridge_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"context"
"fmt"

"github.com/coreos/go-iptables/iptables"
hclog "github.com/hashicorp/go-hclog"
"github.com/hashicorp/nomad/client/allocrunner/cni"
"github.com/hashicorp/nomad/nomad/structs"
Expand All @@ -26,10 +25,6 @@ const (
// defaultNomadAllocSubnet is the subnet to use for host local ip address
// allocation when not specified by the client
defaultNomadAllocSubnet = "172.26.64.0/20" // end 172.26.79.255

// cniAdminChainName is the name of the admin iptables chain used to allow
// forwarding traffic to allocations
cniAdminChainName = "NOMAD-ADMIN"
)

// bridgeNetworkConfigurator is a NetworkConfigurator which adds the alloc to a
Expand All @@ -41,6 +36,8 @@ type bridgeNetworkConfigurator struct {
bridgeName string
hairpinMode bool

newIPTables func(structs.NodeNetworkAF) (IPTablesChain, error)

logger hclog.Logger
}

Expand All @@ -49,6 +46,7 @@ func newBridgeNetworkConfigurator(log hclog.Logger, alloc *structs.Allocation, b
bridgeName: bridgeName,
allocSubnet: ipRange,
hairpinMode: hairpinMode,
newIPTables: newIPTablesChain,
logger: log,
}

Expand Down Expand Up @@ -97,60 +95,18 @@ func newBridgeNetworkConfigurator(log hclog.Logger, alloc *structs.Allocation, b
// ensureForwardingRules ensures that a forwarding rule is added to iptables
// to allow traffic inbound to the bridge network
func (b *bridgeNetworkConfigurator) ensureForwardingRules() error {
ipt, err := iptables.New()
ipt, err := b.newIPTables(structs.NodeNetworkAF_IPv4)
if err != nil {
return err
}

if err = ensureChain(ipt, "filter", cniAdminChainName); err != nil {
return err
}

if err := appendChainRule(ipt, cniAdminChainName, b.generateAdminChainRule()); err != nil {
if err = ensureChainRule(ipt, b.bridgeName, b.allocSubnet); err != nil {
return err
}

return nil
}

// ensureChain ensures that the given chain exists, creating it if missing
func ensureChain(ipt *iptables.IPTables, table, chain string) error {
chains, err := ipt.ListChains(table)
if err != nil {
return fmt.Errorf("failed to list iptables chains: %v", err)
}
for _, ch := range chains {
if ch == chain {
return nil
}
}

err = ipt.NewChain(table, chain)

// if err is for chain already existing return as it is possible another
// goroutine created it first
if e, ok := err.(*iptables.Error); ok && e.ExitStatus() == 1 {
return nil
}

return err
}

// appendChainRule adds the given rule to the chain
func appendChainRule(ipt *iptables.IPTables, chain string, rule []string) error {
exists, err := ipt.Exists("filter", chain, rule...)
if !exists && err == nil {
err = ipt.Append("filter", chain, rule...)
}
return err
}

// generateAdminChainRule builds the iptables rule that is inserted into the
// CNI admin chain to ensure traffic forwarding to the bridge network
func (b *bridgeNetworkConfigurator) generateAdminChainRule() []string {
return []string{"-o", b.bridgeName, "-d", b.allocSubnet, "-j", "ACCEPT"}
}

// Setup calls the CNI plugins with the add action
func (b *bridgeNetworkConfigurator) Setup(ctx context.Context, alloc *structs.Allocation, spec *drivers.NetworkIsolationSpec) (*structs.AllocNetworkStatus, error) {
if err := b.ensureForwardingRules(); err != nil {
Expand Down
14 changes: 4 additions & 10 deletions client/allocrunner/networking_cni.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (

cni "github.com/containerd/go-cni"
cnilibrary "github.com/containernetworking/cni/libcni"
"github.com/coreos/go-iptables/iptables"
consulIPTables "github.com/hashicorp/consul/sdk/iptables"
log "github.com/hashicorp/go-hclog"
"github.com/hashicorp/go-set/v2"
Expand Down Expand Up @@ -59,6 +58,7 @@ type cniNetworkConfigurator struct {
rand *rand.Rand
logger log.Logger
nsOpts *nsOpts
newIPTables func(structs.NodeNetworkAF) (IPTablesCleanup, error)
}

func newCNINetworkConfigurator(logger log.Logger, cniPath, cniInterfacePrefix, cniConfDir, networkName string, ignorePortMappingHostIP bool, node *structs.Node) (*cniNetworkConfigurator, error) {
Expand All @@ -79,6 +79,7 @@ func newCNINetworkConfiguratorWithConf(logger log.Logger, cniPath, cniInterfaceP
nodeAttrs: node.Attributes,
nodeMeta: node.Meta,
nsOpts: &nsOpts{},
newIPTables: newIPTablesCleanup,
}
if cniPath == "" {
if cniPath = os.Getenv(envCNIPath); cniPath == "" {
Expand Down Expand Up @@ -513,7 +514,7 @@ func (c *cniNetworkConfigurator) Teardown(ctx context.Context, alloc *structs.Al

if err := c.cni.Remove(ctx, alloc.ID, spec.Path, cni.WithCapabilityPortMap(portMap.ports)); err != nil {
// create a real handle to iptables
ipt, iptErr := iptables.New()
ipt, iptErr := c.newIPTables(structs.NodeNetworkAF_IPv4)
if iptErr != nil {
return fmt.Errorf("failed to detect iptables: %w", iptErr)
}
Expand All @@ -524,13 +525,6 @@ func (c *cniNetworkConfigurator) Teardown(ctx context.Context, alloc *structs.Al
return nil
}

// IPTables is a subset of iptables.IPTables
type IPTables interface {
List(table, chain string) ([]string, error)
Delete(table, chain string, rule ...string) error
ClearAndDeleteChain(table, chain string) error
}

var (
// ipRuleRe is used to parse a postrouting iptables rule created by nomad, e.g.
// -A POSTROUTING -s 172.26.64.191/32 -m comment --comment "name: \"nomad\" id: \"6b235529-8111-4bbe-520b-d639b1d2a94e\"" -j CNI-50e58ea77dc52e0c731e3799
Expand All @@ -541,7 +535,7 @@ var (
// an allocation that was using bridge networking. The cni library refuses to handle a
// dirty state - e.g. the pause container is removed out of band, and so we must cleanup
// iptables ourselves to avoid leaking rules.
func (c *cniNetworkConfigurator) forceCleanup(ipt IPTables, allocID string) error {
func (c *cniNetworkConfigurator) forceCleanup(ipt IPTablesCleanup, allocID string) error {
const (
natTable = "nat"
postRoutingChain = "POSTROUTING"
Expand Down
65 changes: 18 additions & 47 deletions client/allocrunner/networking_cni_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -273,51 +273,10 @@ func TestSetup(t *testing.T) {
}
}

type mockIPTables struct {
listCall [2]string
listRules []string
listErr error

deleteCall [2]string
deleteErr error

clearCall [2]string
clearErr error
}

func (ipt *mockIPTables) List(table, chain string) ([]string, error) {
ipt.listCall[0], ipt.listCall[1] = table, chain
return ipt.listRules, ipt.listErr
}

func (ipt *mockIPTables) Delete(table, chain string, rule ...string) error {
ipt.deleteCall[0], ipt.deleteCall[1] = table, chain
return ipt.deleteErr
}

func (ipt *mockIPTables) ClearAndDeleteChain(table, chain string) error {
ipt.clearCall[0], ipt.clearCall[1] = table, chain
return ipt.clearErr
}

func (ipt *mockIPTables) assert(t *testing.T, jumpChain string) {
// List assertions
must.Eq(t, "nat", ipt.listCall[0])
must.Eq(t, "POSTROUTING", ipt.listCall[1])

// Delete assertions
must.Eq(t, "nat", ipt.deleteCall[0])
must.Eq(t, "POSTROUTING", ipt.deleteCall[1])

// Clear assertions
must.Eq(t, "nat", ipt.clearCall[0])
must.Eq(t, jumpChain, ipt.clearCall[1])
}

func TestCNI_forceCleanup(t *testing.T) {
t.Run("ok", func(t *testing.T) {
c := cniNetworkConfigurator{logger: testlog.HCLogger(t)}
ipt := &mockIPTables{
ipt := &mockIPTablesCleanup{
listRules: []string{
`-A POSTROUTING -m comment --comment "CNI portfwd requiring masquerade" -j CNI-HOSTPORT-MASQ`,
`-A POSTROUTING -s 172.17.0.0/16 ! -o docker0 -j MASQUERADE`,
Expand All @@ -330,12 +289,24 @@ func TestCNI_forceCleanup(t *testing.T) {
}
err := c.forceCleanup(ipt, "2dd71cac-2b1e-ff08-167c-735f7f9f4964")
must.NoError(t, err)
ipt.assert(t, "CNI-5d36f286cfbb35c5776509ec")

// List assertions
must.Eq(t, "nat", ipt.listCall[0])
must.Eq(t, "POSTROUTING", ipt.listCall[1])

// Delete assertions
must.Eq(t, "nat", ipt.deleteCall[0])
must.Eq(t, "POSTROUTING", ipt.deleteCall[1])

// Clear assertions
must.Eq(t, "nat", ipt.clearCall[0])
jumpChain := "CNI-5d36f286cfbb35c5776509ec"
must.Eq(t, jumpChain, ipt.clearCall[1])
})

t.Run("missing allocation", func(t *testing.T) {
c := cniNetworkConfigurator{logger: testlog.HCLogger(t)}
ipt := &mockIPTables{
ipt := &mockIPTablesCleanup{
listRules: []string{
`-A POSTROUTING -m comment --comment "CNI portfwd requiring masquerade" -j CNI-HOSTPORT-MASQ`,
`-A POSTROUTING -s 172.17.0.0/16 ! -o docker0 -j MASQUERADE`,
Expand All @@ -352,14 +323,14 @@ func TestCNI_forceCleanup(t *testing.T) {

t.Run("list error", func(t *testing.T) {
c := cniNetworkConfigurator{logger: testlog.HCLogger(t)}
ipt := &mockIPTables{listErr: errors.New("list error")}
ipt := &mockIPTablesCleanup{listErr: errors.New("list error")}
err := c.forceCleanup(ipt, "2dd71cac-2b1e-ff08-167c-735f7f9f4964")
must.EqError(t, err, "failed to list iptables rules: list error")
})

t.Run("delete error", func(t *testing.T) {
c := cniNetworkConfigurator{logger: testlog.HCLogger(t)}
ipt := &mockIPTables{
ipt := &mockIPTablesCleanup{
deleteErr: errors.New("delete error"),
listRules: []string{
`-A POSTROUTING -s 172.26.64.217/32 -m comment --comment "name: \"nomad\" id: \"2dd71cac-2b1e-ff08-167c-735f7f9f4964\"" -j CNI-5d36f286cfbb35c5776509ec`,
Expand All @@ -371,7 +342,7 @@ func TestCNI_forceCleanup(t *testing.T) {

t.Run("clear error", func(t *testing.T) {
c := cniNetworkConfigurator{logger: testlog.HCLogger(t)}
ipt := &mockIPTables{
ipt := &mockIPTablesCleanup{
clearErr: errors.New("clear error"),
listRules: []string{
`-A POSTROUTING -s 172.26.64.217/32 -m comment --comment "name: \"nomad\" id: \"2dd71cac-2b1e-ff08-167c-735f7f9f4964\"" -j CNI-5d36f286cfbb35c5776509ec`,
Expand Down
102 changes: 102 additions & 0 deletions client/allocrunner/networking_iptables.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: BUSL-1.1

//go:build linux

package allocrunner

import (
"fmt"

"github.com/coreos/go-iptables/iptables"
"github.com/hashicorp/nomad/nomad/structs"
)

const (
// cniAdminChainName is the name of the admin iptables chain used to allow
// forwarding traffic to allocations
cniAdminChainName = "NOMAD-ADMIN"
)

// newIPTables provides an *iptables.IPTables for the requested address family
// "ipv6" or "ipv4"
func newIPTables(family structs.NodeNetworkAF) (IPTables, error) {
if family == structs.NodeNetworkAF_IPv6 {
return iptables.New(iptables.IPFamily(iptables.ProtocolIPv6), iptables.Timeout(5))
}
return iptables.New()
}
func newIPTablesCleanup(family structs.NodeNetworkAF) (IPTablesCleanup, error) {
return newIPTables(family)
}
func newIPTablesChain(family structs.NodeNetworkAF) (IPTablesChain, error) {
return newIPTables(family)
}

// IPTables is a subset of iptables.IPTables
type IPTables interface {
IPTablesCleanup
IPTablesChain
}
type IPTablesCleanup interface {
List(table, chain string) ([]string, error)
Delete(table, chain string, rule ...string) error
ClearAndDeleteChain(table, chain string) error
}
type IPTablesChain interface {
ListChains(table string) ([]string, error)
NewChain(table string, chain string) error
Exists(table string, chain string, rulespec ...string) (bool, error)
Append(table string, chain string, rulespec ...string) error
}

// ensureChainRule ensures our admin chain exists and contains a rule to accept
// traffic to the bridge network
func ensureChainRule(ipt IPTablesChain, bridgeName, subnet string) error {
if err := ensureChain(ipt, "filter", cniAdminChainName); err != nil {
return err
}
rule := generateAdminChainRule(bridgeName, subnet)
if err := appendChainRule(ipt, cniAdminChainName, rule); err != nil {
return err
}
return nil
}

// ensureChain ensures that the given chain exists, creating it if missing
func ensureChain(ipt IPTablesChain, table, chain string) error {
chains, err := ipt.ListChains(table)
if err != nil {
return fmt.Errorf("failed to list iptables chains: %v", err)
}
for _, ch := range chains {
if ch == chain {
return nil
}
}

err = ipt.NewChain(table, chain)

// if err is for chain already existing return as it is possible another
// goroutine created it first
if e, ok := err.(*iptables.Error); ok && e.ExitStatus() == 1 {
return nil
}

return err
}

// appendChainRule adds the given rule to the chain
func appendChainRule(ipt IPTablesChain, chain string, rule []string) error {
exists, err := ipt.Exists("filter", chain, rule...)
if !exists && err == nil {
err = ipt.Append("filter", chain, rule...)
}
return err
}

// generateAdminChainRule builds the iptables rule that is inserted into the
// CNI admin chain to ensure traffic forwarding to the bridge network
func generateAdminChainRule(bridgeName, subnet string) []string {
return []string{"-o", bridgeName, "-d", subnet, "-j", "ACCEPT"}
}
Loading

0 comments on commit a6e2905

Please sign in to comment.