refactor egressContext new constructor per PR comments

aws · Apr 25, 2023 · 74dd41c · 74dd41c
1 parent 5e7c7c0
commit 74dd41c
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 136 deletions.
diff --git 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
@@ -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
@@ -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,22 +136,20 @@ 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),
 		Ipam:          mock_ipamwrapper.NewMockHostIpam(ctrl),
 		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"}