From 74dd41c58040a8f4a6ecff26e431de142b3ba274 Mon Sep 17 00:00:00 2001 From: Yufei Wang Date: Tue, 25 Apr 2023 15:32:42 -0400 Subject: [PATCH] refactor egressContext new constructor per PR comments --- cmd/egress-cni-plugin/egressContext.go | 102 +++++-------------------- cmd/egress-cni-plugin/main.go | 47 +----------- cmd/egress-cni-plugin/main_test.go | 19 ++--- 3 files changed, 32 insertions(+), 136 deletions(-) diff --git a/cmd/egress-cni-plugin/egressContext.go b/cmd/egress-cni-plugin/egressContext.go index 6db2d4bb34..97d6af49ea 100644 --- a/cmd/egress-cni-plugin/egressContext.go +++ b/cmd/egress-cni-plugin/egressContext.go @@ -64,93 +64,33 @@ type EgressContext struct { 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") +func NewEgressAddContext(nsPath, ifName string) EgressContext { + return EgressContext{ + Procsys: procsyswrapper.NewProcSys(), + Ipam: hostipamwrapper.NewIpam(), + Link: netlinkwrapper.NewNetLink(), + Ns: nswrapper.NewNS(), + NsPath: nsPath, + ArgsIfName: ifName, + Veth: vethwrapper.NewSetupVeth(), + IptCreator: func(protocol iptables.Protocol) (iptableswrapper.IPTablesIface, error) { + return iptableswrapper.NewIPTables(protocol) + }, } - 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") +func NewEgressDelContext(nsPath string) EgressContext { + return EgressContext{ + Ipam: hostipamwrapper.NewIpam(), + Link: netlinkwrapper.NewNetLink(), + Ns: nswrapper.NewNS(), + NsPath: nsPath, + IptCreator: func(protocol iptables.Protocol) (iptableswrapper.IPTablesIface, error) { + return iptableswrapper.NewIPTables(protocol) + }, } - 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) { diff --git a/cmd/egress-cni-plugin/main.go b/cmd/egress-cni-plugin/main.go index b561bb15a2..69e1664e23 100644 --- a/cmd/egress-cni-plugin/main.go +++ b/cmd/egress-cni-plugin/main.go @@ -18,14 +18,6 @@ import ( "runtime" "strconv" - "github.com/aws/amazon-vpc-cni-k8s/pkg/iptableswrapper" - "github.com/aws/amazon-vpc-cni-k8s/pkg/vethwrapper" - - "github.com/aws/amazon-vpc-cni-k8s/pkg/hostipamwrapper" - "github.com/aws/amazon-vpc-cni-k8s/pkg/netlinkwrapper" - "github.com/aws/amazon-vpc-cni-k8s/pkg/nswrapper" - "github.com/aws/amazon-vpc-cni-k8s/pkg/procsyswrapper" - "github.com/coreos/go-iptables/iptables" "github.com/containernetworking/cni/pkg/skel" @@ -49,25 +41,8 @@ func main() { } func cmdAdd(args *skel.CmdArgs) error { - iptCreator := func(protocol iptables.Protocol) (iptableswrapper.IPTablesIface, error) { - return iptableswrapper.NewIPTables(protocol) - } - addOption := EgressContextAddOption{ - Procsys: procsyswrapper.NewProcSys(), - Ns: nswrapper.NewNS(), - NsPath: args.Netns, - ArgsIfName: args.IfName, - Ipam: hostipamwrapper.NewIpam(), - Link: netlinkwrapper.NewNetLink(), - Veth: vethwrapper.NewSetupVeth(), - IptCreator: iptCreator, - } - - ec, err := NewEgressAddContext(addOption) - if err != nil { - return err - } - return add(args, ec) + ec := NewEgressAddContext(args.Netns, args.IfName) + return add(args, &ec) } func add(args *skel.CmdArgs, ec *EgressContext) (err error) { @@ -140,22 +115,8 @@ func add(args *skel.CmdArgs, ec *EgressContext) (err error) { } func cmdDel(args *skel.CmdArgs) error { - iptCreator := func(protocol iptables.Protocol) (iptableswrapper.IPTablesIface, error) { - return iptableswrapper.NewIPTables(protocol) - } - delOption := EgressContextDelOption{ - Ns: nswrapper.NewNS(), - NsPath: args.Netns, - Ipam: hostipamwrapper.NewIpam(), - Link: netlinkwrapper.NewNetLink(), - IptCreator: iptCreator, - } - - ec, err := NewEgressDelContext(delOption) - if err != nil { - return err - } - return del(args, ec) + ec := NewEgressDelContext(args.Netns) + return del(args, &ec) } func del(args *skel.CmdArgs, ec *EgressContext) (err error) { diff --git a/cmd/egress-cni-plugin/main_test.go b/cmd/egress-cni-plugin/main_test.go index a98f9e92bd..57ec8290d7 100644 --- a/cmd/egress-cni-plugin/main_test.go +++ b/cmd/egress-cni-plugin/main_test.go @@ -66,7 +66,7 @@ func TestCmdAddV4(t *testing.T) { }`), } - addOption := EgressContextAddOption{ + ec := EgressContext{ Procsys: mock_procsyswrapper.NewMockProcSys(ctrl), Ns: mock_nswrapper.NewMockNS(ctrl), NsPath: "/var/run/netns/cni-xxxx", @@ -77,14 +77,11 @@ func TestCmdAddV4(t *testing.T) { Veth: mock_veth.NewMockVeth(ctrl), } - c, err := NewEgressAddContext(addOption) - assert.Nil(t, err) - var actualIptablesRules, actualRouteAdd, actualRouteDel []string - err = SetupAddExpectV4(*c, snatChainV4, &actualIptablesRules, &actualRouteAdd, &actualRouteDel) + err := SetupAddExpectV4(ec, snatChainV4, &actualIptablesRules, &actualRouteAdd, &actualRouteDel) assert.Nil(t, err) - err = add(args, c) + err = add(args, &ec) assert.Nil(t, err) expectIptablesRules := []string{ @@ -139,7 +136,8 @@ func TestCmdDelV4(t *testing.T) { "vethPrefix":"eni" }`), } - delOption := EgressContextDelOption{ + + ec := EgressContext{ Ns: mock_nswrapper.NewMockNS(ctrl), NsPath: "/var/run/netns/cni-xxxx", IpTablesIface: mock_iptables.NewMockIPTablesIface(ctrl), @@ -147,14 +145,11 @@ func TestCmdDelV4(t *testing.T) { Link: mock_netlinkwrapper.NewMockNetLink(ctrl), } - c, err := NewEgressDelContext(delOption) - assert.Nil(t, err) - var actualLinkDel, actualIptablesDel []string - err = SetupDelExpectV4(*c, &actualLinkDel, &actualIptablesDel) + err := SetupDelExpectV4(ec, &actualLinkDel, &actualIptablesDel) assert.Nil(t, err) - err = del(args, c) + err = del(args, &ec) assert.Nil(t, err) expectLinkDel := []string{"link del - name: v4if0"}