Skip to content

Commit

Permalink
refactor egressContext new constructor per PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@wanyufe
wanyufe committed Apr 25, 2023
1 parent 5e7c7c0 commit 2c98649
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 136 deletions.
6 changes: 6 additions & 0 deletions cmd/egress-cni-plugin/egress-plugin.log
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{"level":"info","ts":"2023-04-25T15:31:56.852-0400","caller":"egress-cni-plugin/netconf.go:74","msg":"Constructed new logger instance"}
{"level":"debug","ts":"2023-04-25T15:31:56.852-0400","caller":"egress-cni-plugin/main_test.go:84","msg":"Received an ADD request for: conf=&{{0.4.0 aws-cni aws-cni map[] {host-local} {[] [] []} map[] 0xc000222b00} 9001 true 192.168.1.123 egress-plugin.log DEBUG}; Plugin enabled=true"}
{"level":"debug","ts":"2023-04-25T15:31:56.853-0400","caller":"egress-cni-plugin/main.go:113","msg":"Node IP: 192.168.1.123"}
{"level":"info","ts":"2023-04-25T15:31:56.853-0400","caller":"egress-cni-plugin/netconf.go:74","msg":"Constructed new logger instance"}
{"level":"debug","ts":"2023-04-25T15:31:56.853-0400","caller":"egress-cni-plugin/main_test.go:152","msg":"Received Del Request: nsPath: /var/run/netns/cni-xxxx conf=&{{0.4.0 aws-cni aws-cni map[] {host-local} {[] [] []} map[] 0xc000116580} 9001 true 192.168.1.123 egress-plugin.log DEBUG}"}
{"level":"debug","ts":"2023-04-25T15:31:56.853-0400","caller":"egress-cni-plugin/main.go:151","msg":"DEL: SNAT setup, let's clean them up. Size of ipnets: 1"}
102 changes: 21 additions & 81 deletions cmd/egress-cni-plugin/egressContext.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
47 changes: 4 additions & 43 deletions cmd/egress-cni-plugin/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down
19 changes: 7 additions & 12 deletions cmd/egress-cni-plugin/main_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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{
Expand Down Expand Up @@ -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"}
Expand Down

0 comments on commit 2c98649

Please sign in to comment.