From b78ddb0a6815a9551f47769889ec2afc354763b5 Mon Sep 17 00:00:00 2001 From: Yufei Wang Date: Mon, 24 Apr 2023 15:58:20 -0400 Subject: [PATCH] refactor - create egressContext new constructor for add/del --- cmd/egress-cni-plugin/egressContext.go | 101 +++++++++++++++++++++++-- cmd/egress-cni-plugin/main.go | 8 +- cmd/egress-cni-plugin/main_test.go | 33 ++++---- cmd/egress-cni-plugin/test_utils.go | 2 +- pkg/networkutils/network.go | 2 +- 5 files changed, 119 insertions(+), 27 deletions(-) diff --git a/cmd/egress-cni-plugin/egressContext.go b/cmd/egress-cni-plugin/egressContext.go index f97042dfa1..6db2d4bb34 100644 --- a/cmd/egress-cni-plugin/egressContext.go +++ b/cmd/egress-cni-plugin/egressContext.go @@ -57,9 +57,100 @@ type EgressContext struct { TmpResult *current.Result Log logger.Logger - Mtu int - Chain string - Comment string + Mtu int + // SnatChain is the chain name for iptables rules + SnatChain string + // SnatComment is the comment for iptables rules + SnatComment string +} + +// EgressContextAddOption has all necessary fields for container egress setup +type EgressContextAddOption struct { + Procsys procsyswrapper.ProcSys + Ipam hostipamwrapper.HostIpam + Link netlinkwrapper.NetLink + Ns nswrapper.NS + NsPath string + ArgsIfName string + Veth vethwrapper.Veth + IpTablesIface iptableswrapper.IPTablesIface + IptCreator func(iptables.Protocol) (iptableswrapper.IPTablesIface, error) +} + +// EgressContextDelOption has all necessary fields for container egress removal +type EgressContextDelOption struct { + Ipam hostipamwrapper.HostIpam + Link netlinkwrapper.NetLink + Ns nswrapper.NS + NsPath string + IpTablesIface iptableswrapper.IPTablesIface + IptCreator func(iptables.Protocol) (iptableswrapper.IPTablesIface, error) +} + +// NewEgressAddContext create a context for container egress traffic +func NewEgressAddContext(option EgressContextAddOption) (*EgressContext, error) { + if option.Procsys == nil { + return nil, fmt.Errorf("prosys is nil in EgressContextAddOption") + } + if option.Ipam == nil { + return nil, fmt.Errorf("ipam is nil in EgressContextAddOption") + } + if option.Link == nil { + return nil, fmt.Errorf("link is nil in EgressContextAddOption") + } + if option.Ns == nil { + return nil, fmt.Errorf("ns is nil in EgressContextAddOption") + } + if option.NsPath == "" { + return nil, fmt.Errorf("nsPath is empty in EgressContextAddOption") + } + if option.ArgsIfName == "" { + return nil, fmt.Errorf("argsIfName is empty in EgressContextAddOption") + } + if option.Veth == nil { + return nil, fmt.Errorf("veth is nil in EgressContextAddOption") + } + if option.IpTablesIface == nil && option.IptCreator == nil { + return nil, fmt.Errorf("both ipTablesIface and iptCreator are nil in EgressContextAddOption") + } + return &EgressContext{ + Procsys: option.Procsys, + Ipam: option.Ipam, + Link: option.Link, + Ns: option.Ns, + NsPath: option.NsPath, + ArgsIfName: option.ArgsIfName, + Veth: option.Veth, + IpTablesIface: option.IpTablesIface, + IptCreator: option.IptCreator, + }, nil +} + +// NewEgressDelContext create a context for container egress traffic +func NewEgressDelContext(option EgressContextDelOption) (*EgressContext, error) { + if option.Ipam == nil { + return nil, fmt.Errorf("ipam is nil in EgressContextAddOption") + } + if option.Link == nil { + return nil, fmt.Errorf("link is nil in EgressContextAddOption") + } + if option.Ns == nil { + return nil, fmt.Errorf("ns is nil in EgressContextAddOption") + } + if option.NsPath == "" { + return nil, fmt.Errorf("nsPath is empty in EgressContextAddOption") + } + if option.IpTablesIface == nil && option.IptCreator == nil { + return nil, fmt.Errorf("both ipTablesIface and iptCreator are nil in EgressContextAddOption") + } + return &EgressContext{ + Ipam: option.Ipam, + Link: option.Link, + Ns: option.Ns, + NsPath: option.NsPath, + IpTablesIface: option.IpTablesIface, + IptCreator: option.IptCreator, + }, nil } func (c *EgressContext) SetupContainerVeth() (*current.Interface, *current.Interface, error) { @@ -225,7 +316,7 @@ func (c *EgressContext) CmdAddEgressV4() error { for _, ipc := range c.TmpResult.IPs { if ipc.Address.IP.To4() != nil { // add SNAT chain/rules necessary for the container IPv6 egress traffic - if err = snat.Add(c.IpTablesIface, c.NetConf.NodeIP, ipc.Address.IP, ipv4MulticastRange, c.Chain, c.Comment, c.NetConf.RandomizeSNAT); err != nil { + if err = snat.Add(c.IpTablesIface, c.NetConf.NodeIP, ipc.Address.IP, ipv4MulticastRange, c.SnatChain, c.SnatComment, c.NetConf.RandomizeSNAT); err != nil { return err } } @@ -292,7 +383,7 @@ func (c *EgressContext) CmdDelEgressV4() error { if c.NetConf.NodeIP != nil { c.Log.Debugf("DEL: SNAT setup, let's clean them up. Size of ipnets: %d", len(ipnets)) for _, ipn := range ipnets { - if err := snat.Del(c.IpTablesIface, ipn.IP, c.Chain, c.Comment); err != nil { + if err := snat.Del(c.IpTablesIface, ipn.IP, c.SnatChain, c.SnatComment); err != nil { return err } } diff --git a/cmd/egress-cni-plugin/main.go b/cmd/egress-cni-plugin/main.go index f5d263a430..491a1ba0f6 100644 --- a/cmd/egress-cni-plugin/main.go +++ b/cmd/egress-cni-plugin/main.go @@ -104,8 +104,8 @@ func _cmdAdd(args *skel.CmdArgs, c *EgressContext) (err error) { } } - c.Chain = utils.MustFormatChainNameWithPrefix(c.NetConf.Name, args.ContainerID, "E4-") - c.Comment = utils.FormatComment(c.NetConf.Name, args.ContainerID) + c.SnatChain = utils.MustFormatChainNameWithPrefix(c.NetConf.Name, args.ContainerID, "E4-") + c.SnatComment = utils.FormatComment(c.NetConf.Name, args.ContainerID) // Invoke ipam del if err to avoid ip leak defer func() { @@ -174,8 +174,8 @@ func _cmdDel(args *skel.CmdArgs, c *EgressContext) (err error) { return fmt.Errorf("running IPAM plugin failed: %v", err) } - c.Chain = utils.MustFormatChainNameWithPrefix(c.NetConf.Name, args.ContainerID, "E4-") - c.Comment = utils.FormatComment(c.NetConf.Name, args.ContainerID) + c.SnatChain = utils.MustFormatChainNameWithPrefix(c.NetConf.Name, args.ContainerID, "E4-") + c.SnatComment = utils.FormatComment(c.NetConf.Name, args.ContainerID) // IPv4 egress c.NetConf.IfName = EgressIPv4InterfaceName diff --git a/cmd/egress-cni-plugin/main_test.go b/cmd/egress-cni-plugin/main_test.go index 536e01d1a8..f319968586 100644 --- a/cmd/egress-cni-plugin/main_test.go +++ b/cmd/egress-cni-plugin/main_test.go @@ -30,7 +30,7 @@ import ( ) const ( - chainV4 = "CNI-E4-5740307710ebfd5fb24f5" + snatChainV4 = "CNI-E4-5740307710ebfd5fb24f5" ) func TestCmdAddV4(t *testing.T) { @@ -66,7 +66,7 @@ func TestCmdAddV4(t *testing.T) { }`), } - c := &EgressContext{ + addOption := EgressContextAddOption{ Procsys: mock_procsyswrapper.NewMockProcSys(ctrl), Ns: mock_nswrapper.NewMockNS(ctrl), NsPath: "/var/run/netns/cni-xxxx", @@ -75,20 +75,22 @@ func TestCmdAddV4(t *testing.T) { Ipam: mock_ipamwrapper.NewMockHostIpam(ctrl), Link: mock_netlinkwrapper.NewMockNetLink(ctrl), Veth: mock_veth.NewMockVeth(ctrl), - Mtu: 9001, } + c, err := NewEgressAddContext(addOption) + assert.Nil(t, err) + var actualIptablesRules, actualRouteAdd, actualRouteDel []string - err := SetupAddExpectV4(*c, chainV4, &actualIptablesRules, &actualRouteAdd, &actualRouteDel) + err = SetupAddExpectV4(*c, snatChainV4, &actualIptablesRules, &actualRouteAdd, &actualRouteDel) assert.Nil(t, err) err = _cmdAdd(args, c) assert.Nil(t, err) expectIptablesRules := []string{ - fmt.Sprintf("nat %s -d 224.0.0.0/4 -j ACCEPT -m comment --comment name: \"aws-cni\" id: \"containerId-123\"", chainV4), - fmt.Sprintf("nat %s -j SNAT --to-source 192.168.1.123 -m comment --comment name: \"aws-cni\" id: \"containerId-123\" --random-fully", chainV4), - fmt.Sprintf("nat POSTROUTING -s 169.254.172.10 -j %s -m comment --comment name: \"aws-cni\" id: \"containerId-123\"", chainV4)} + fmt.Sprintf("nat %s -d 224.0.0.0/4 -j ACCEPT -m comment --comment name: \"aws-cni\" id: \"containerId-123\"", snatChainV4), + fmt.Sprintf("nat %s -j SNAT --to-source 192.168.1.123 -m comment --comment name: \"aws-cni\" id: \"containerId-123\" --random-fully", snatChainV4), + fmt.Sprintf("nat POSTROUTING -s 169.254.172.10 -j %s -m comment --comment name: \"aws-cni\" id: \"containerId-123\"", snatChainV4)} assert.EqualValues(t, expectIptablesRules, actualIptablesRules) expectRouteDel := []string{"route del: {Ifindex: 2 Dst: 169.254.172.0/22 Src: Gw: Flags: [] Table: 0}"} @@ -137,20 +139,19 @@ func TestCmdDelV4(t *testing.T) { "vethPrefix":"eni" }`), } - c := &EgressContext{ - Procsys: mock_procsyswrapper.NewMockProcSys(ctrl), + delOption := EgressContextDelOption{ Ns: mock_nswrapper.NewMockNS(ctrl), NsPath: "/var/run/netns/cni-xxxx", - ArgsIfName: args.IfName, IpTablesIface: mock_iptables.NewMockIPTablesIface(ctrl), Ipam: mock_ipamwrapper.NewMockHostIpam(ctrl), Link: mock_netlinkwrapper.NewMockNetLink(ctrl), - Veth: mock_veth.NewMockVeth(ctrl), - Mtu: 9001, } + c, err := NewEgressDelContext(delOption) + assert.Nil(t, err) + var actualLinkDel, actualIptablesDel []string - err := SetupDelExpectV4(*c, &actualLinkDel, &actualIptablesDel) + err = SetupDelExpectV4(*c, &actualLinkDel, &actualIptablesDel) assert.Nil(t, err) err = _cmdDel(args, c) @@ -160,8 +161,8 @@ func TestCmdDelV4(t *testing.T) { assert.EqualValues(t, expectLinkDel, actualLinkDel) expectIptablesDel := []string{ - fmt.Sprintf("nat POSTROUTING -s 169.254.172.10 -j %s -m comment --comment name: \"aws-cni\" id: \"containerId-123\"", chainV4), - fmt.Sprintf("clear chain nat %s", chainV4), - fmt.Sprintf("del chain nat %s", chainV4)} + fmt.Sprintf("nat POSTROUTING -s 169.254.172.10 -j %s -m comment --comment name: \"aws-cni\" id: \"containerId-123\"", snatChainV4), + fmt.Sprintf("clear chain nat %s", snatChainV4), + fmt.Sprintf("del chain nat %s", snatChainV4)} assert.EqualValues(t, expectIptablesDel, actualIptablesDel) } diff --git a/cmd/egress-cni-plugin/test_utils.go b/cmd/egress-cni-plugin/test_utils.go index 3e516f4efa..900f1d9a2a 100644 --- a/cmd/egress-cni-plugin/test_utils.go +++ b/cmd/egress-cni-plugin/test_utils.go @@ -65,7 +65,7 @@ func SetupAddExpectV4(c EgressContext, chain string, actualIptablesRules, actual f(nsParent) }).Return(nil) - c.Veth.(*mock_veth.MockVeth).EXPECT().Setup(EgressIPv4InterfaceName, c.Mtu, gomock.Any()).Return( + c.Veth.(*mock_veth.MockVeth).EXPECT().Setup(EgressIPv4InterfaceName, 9001, gomock.Any()).Return( net.Interface{ Name: HostIfName, HardwareAddr: macHost[:], diff --git a/pkg/networkutils/network.go b/pkg/networkutils/network.go index a5109061ff..b9da0d3306 100644 --- a/pkg/networkutils/network.go +++ b/pkg/networkutils/network.go @@ -717,7 +717,7 @@ func computeStaleIptablesRules(ipt iptableswrapper.IPTablesIface, table, chainPr } func containChainExistErr(err error) bool { - return strings.Contains(err.Error(), "Chain already exists") + return strings.Contains(err.Error(), "SnatChain already exists") } type iptablesRule struct {