Skip to content

Commit

Permalink
address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@M00nF1sh
M00nF1sh committed Mar 22, 2022
1 parent dc394da commit 8e6c69c
Show file tree
Hide file tree
Showing 10 changed files with 271 additions and 239 deletions.
2 changes: 2 additions & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,8 @@ Once `ENABLE_POD_ENI` is set to `true`, this value controls how the traffic of p
* if externalSNAT enabled, traffic won't be SNATed, thus will be enforced by security group rules.
* if externalSNAT disabled, traffic will be SNATed via eth0, thus will only be enforced by security group associated with eth0.

**NOTE!**: To make new behavior be in effect after switching the mode, existing pods with security group must be recycled. Alternatively you can restart the nodes as well.

---

#### `DISABLE_TCP_EARLY_DEMUX` (v1.7.3+)
Expand Down
33 changes: 8 additions & 25 deletions cmd/routed-eni-cni-plugin/cni.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
"strconv"
"strings"

"github.com/aws/amazon-vpc-cni-k8s/pkg/utils/cniutils"

"github.com/aws/amazon-vpc-cni-k8s/pkg/sgpp"

"github.com/containernetworking/cni/pkg/skel"
Expand Down Expand Up @@ -427,22 +429,22 @@ func tryDelWithPrevResult(driverClient driver.NetworkAPIs, conf *NetConf, k8sArg
}

dummyIfaceName := generateHostVethName(dummyVlanInterfacePrefix, string(k8sArgs.K8S_POD_NAMESPACE), string(k8sArgs.K8S_POD_NAME))
_, dummyIface, found := findCNIInterfaceByName(prevResult.Interfaces, dummyIfaceName)
_, dummyIface, found := cniutils.FindInterfaceByName(prevResult.Interfaces, dummyIfaceName)
if !found {
return false, nil
}
podVlanID, err := strconv.Atoi(dummyIface.Mac)
if err != nil || podVlanID == 0 {
return true, errors.Errorf("malformed vlanID from prevResult: %s", dummyIface.Mac)
return true, errors.Errorf("malformed vlanID in prevResult: %s", dummyIface.Mac)
}

containerIfaceIndex, _, found := findCNIInterfaceByName(prevResult.Interfaces, contVethName)
containerIfaceIndex, _, found := cniutils.FindInterfaceByName(prevResult.Interfaces, contVethName)
if !found {
return false, nil
return false, errors.Errorf("cannot find contVethName %s in prevResult", contVethName)
}
containerIPs := findCNIIPConfigsByIfaceIndex(prevResult.IPs, containerIfaceIndex)
containerIPs := cniutils.FindIPConfigsByIfaceIndex(prevResult.IPs, containerIfaceIndex)
if len(containerIPs) != 1 {
return false, nil
return false, errors.Errorf("found %d containerIP for %v in prevResult", len(containerIPs), contVethName)
}
containerIP := containerIPs[0].Address

Expand All @@ -452,25 +454,6 @@ func tryDelWithPrevResult(driverClient driver.NetworkAPIs, conf *NetConf, k8sArg
return true, nil
}

func findCNIInterfaceByName(ifaceList []*current.Interface, ifaceName string) (ifaceIndex int, iface *current.Interface, found bool) {
for ifaceIndex, iface := range ifaceList {
if iface.Name == ifaceName {
return ifaceIndex, iface, true
}
}
return 0, nil, false
}

func findCNIIPConfigsByIfaceIndex(ipConfigs []*current.IPConfig, ifaceIndex int) []*current.IPConfig {
var matchedIPConfigs []*current.IPConfig
for _, ipConfig := range ipConfigs {
if ipConfig.Interface != nil && *ipConfig.Interface == ifaceIndex {
matchedIPConfigs = append(matchedIPConfigs, ipConfig)
}
}
return matchedIPConfigs
}

func main() {
log := logger.DefaultLogger()
about := fmt.Sprintf("AWS CNI %s", version)
Expand Down
217 changes: 9 additions & 208 deletions cmd/routed-eni-cni-plugin/cni_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ package main
import (
"encoding/json"
"errors"
"net"
"testing"

"github.com/aws/amazon-vpc-cni-k8s/pkg/sgpp"
"github.com/aws/amazon-vpc-cni-k8s/pkg/utils/logger"
"github.com/aws/aws-sdk-go/aws"
"github.com/containernetworking/cni/pkg/types/current"
"net"
"testing"

"github.com/containernetworking/cni/pkg/skel"
"github.com/containernetworking/cni/pkg/types"
Expand Down Expand Up @@ -600,7 +601,7 @@ func Test_tryDelWithPrevResult(t *testing.T) {
},
contVethName: "eth0",
},
wantErr: errors.New("malformed vlanID from prevResult: xxx"),
wantErr: errors.New("malformed vlanID in prevResult: xxx"),
},
{
name: "malformed vlanID in prevResult - 0",
Expand Down Expand Up @@ -642,10 +643,10 @@ func Test_tryDelWithPrevResult(t *testing.T) {
},
contVethName: "eth0",
},
wantErr: errors.New("malformed vlanID from prevResult: 0"),
wantErr: errors.New("malformed vlanID in prevResult: 0"),
},
{
name: "confVeth don't exists",
name: "confVeth don't exists",
fields: fields{},
args: args{
conf: &NetConf{
Expand Down Expand Up @@ -680,10 +681,10 @@ func Test_tryDelWithPrevResult(t *testing.T) {
},
contVethName: "eth0",
},
want: false,
wantErr: errors.New("cannot find contVethName eth0 in prevResult"),
},
{
name: "container IP don't exists",
name: "container IP don't exists",
fields: fields{},
args: args{
conf: &NetConf{
Expand Down Expand Up @@ -713,7 +714,7 @@ func Test_tryDelWithPrevResult(t *testing.T) {
},
contVethName: "eth0",
},
want: false,
wantErr: errors.New("found 0 containerIP for eth0 in prevResult"),
},
}
for _, tt := range tests {
Expand Down Expand Up @@ -742,203 +743,3 @@ func Test_tryDelWithPrevResult(t *testing.T) {
})
}
}

func Test_findCNIInterfaceByName(t *testing.T) {
type args struct {
ifaceList []*current.Interface
ifaceName string
}
tests := []struct {
name string
args args
wantIfaceIndex int
wantIface *current.Interface
wantFound bool
}{
{
name: "found the CNI interface at index 0",
args: args{
ifaceList: []*current.Interface{
{
Name: "eni8ea2c11fe35",
},
{
Name: "eth0",
},
},
ifaceName: "eni8ea2c11fe35",
},
wantIfaceIndex: 0,
wantIface: &current.Interface{
Name: "eni8ea2c11fe35",
},
wantFound: true,
},
{
name: "found the CNI interface at index 1",
args: args{
ifaceList: []*current.Interface{
{
Name: "eth0",
},
{
Name: "eni8ea2c11fe35",
},
},
ifaceName: "eni8ea2c11fe35",
},
wantIfaceIndex: 1,
wantIface: &current.Interface{
Name: "eni8ea2c11fe35",
},
wantFound: true,
},
{
name: "didn't found CNI interface",
args: args{
ifaceList: []*current.Interface{
{
Name: "eth0",
},
{
Name: "eni8ea2c11fe35",
},
},
ifaceName: "enixxxxx",
},
wantFound: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
gotIfaceIndex, gotIface, gotFound := findCNIInterfaceByName(tt.args.ifaceList, tt.args.ifaceName)
assert.Equal(t, tt.wantFound, gotFound)
if tt.wantFound {
assert.Equal(t, tt.wantIfaceIndex, gotIfaceIndex)
assert.Equal(t, tt.wantIface, gotIface)
}
})
}
}

func Test_findCNIIPConfigsByIfaceIndex(t *testing.T) {
type args struct {
ipConfigs []*current.IPConfig
ifaceIndex int
}
tests := []struct {
name string
args args
want []*current.IPConfig
}{
{
name: "single matched IPConfig",
args: args{
ipConfigs: []*current.IPConfig{
{
Interface: aws.Int(1),
Address: net.IPNet{
IP: net.ParseIP("192.168.1.1"),
},
},
{
Interface: aws.Int(2),
Address: net.IPNet{
IP: net.ParseIP("192.168.1.2"),
},
},
},
ifaceIndex: 1,
},
want: []*current.IPConfig{
{
Interface: aws.Int(1),
Address: net.IPNet{
IP: net.ParseIP("192.168.1.1"),
},
},
},
},
{
name: "multiple matched IPConfig",
args: args{
ipConfigs: []*current.IPConfig{
{
Interface: aws.Int(1),
Address: net.IPNet{
IP: net.ParseIP("192.168.1.1"),
},
},
{
Interface: aws.Int(1),
Address: net.IPNet{
IP: net.ParseIP("192.168.1.2"),
},
},
{
Interface: aws.Int(2),
Address: net.IPNet{
IP: net.ParseIP("192.168.1.3"),
},
},
},
ifaceIndex: 1,
},
want: []*current.IPConfig{
{
Interface: aws.Int(1),
Address: net.IPNet{
IP: net.ParseIP("192.168.1.1"),
},
},
{
Interface: aws.Int(1),
Address: net.IPNet{
IP: net.ParseIP("192.168.1.2"),
},
},
},
},
{
name: "none matched IPConfig",
args: args{
ipConfigs: []*current.IPConfig{
{
Interface: aws.Int(2),
Address: net.IPNet{
IP: net.ParseIP("192.168.1.1"),
},
},
{
Interface: aws.Int(2),
Address: net.IPNet{
IP: net.ParseIP("192.168.1.2"),
},
},
},
ifaceIndex: 1,
},
want: nil,
},
{
name: "interface is not set",
args: args{
ipConfigs: []*current.IPConfig{
{
Address: net.IPNet{
IP: net.ParseIP("192.168.1.1"),
},
},
},
ifaceIndex: 1,
},
want: nil,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := findCNIIPConfigsByIfaceIndex(tt.args.ipConfigs, tt.args.ifaceIndex)
assert.Equal(t, tt.want, got)
})
}
}
11 changes: 10 additions & 1 deletion cmd/routed-eni-cni-plugin/driver/driver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,7 @@ func waitForAddressesToBeStable(netLink netlinkwrapper.NetLink, ifName string, t
}

// SetupPodNetwork wires up linux networking for a pod's network
// we expect v4Addr and v6Addr to have correct IPAddress Family.
func (n *linuxNetwork) SetupPodNetwork(hostVethName string, contVethName string, netnsPath string, v4Addr *net.IPNet, v6Addr *net.IPNet,
deviceNumber int, mtu int, log logger.Logger) error {
log.Debugf("SetupPodNetwork: hostVethName=%s, contVethName=%s, netnsPath=%s, v4Addr=%v, v6Addr=%v, deviceNumber=%d, mtu=%d",
Expand Down Expand Up @@ -314,6 +315,7 @@ func (n *linuxNetwork) TeardownPodNetwork(containerAddr *net.IPNet, deviceNumber
}

// SetupBranchENIPodNetwork sets up the network ns for pods requesting its own security group
// we expect v4Addr and v6Addr to have correct IPAddress Family.
func (n *linuxNetwork) SetupBranchENIPodNetwork(hostVethName string, contVethName string, netnsPath string, v4Addr *net.IPNet, v6Addr *net.IPNet,
vlanID int, eniMAC string, subnetGW string, parentIfIndex int, mtu int, podSGEnforcingMode sgpp.EnforcingMode, log logger.Logger) error {
log.Debugf("SetupBranchENIPodNetwork: hostVethName=%s, contVethName=%s, netnsPath=%s, v4Addr=%v, v6Addr=%v, vlanID=%d, eniMAC=%s, subnetGW=%s, parentIfIndex=%d, mtu=%d, podSGEnforcingMode=%v",
Expand All @@ -324,7 +326,11 @@ func (n *linuxNetwork) SetupBranchENIPodNetwork(hostVethName string, contVethNam
return errors.Wrapf(err, "SetupBranchENIPodNetwork: failed to setup veth pair")
}

// clean up any previous hostVeth ip rule recursively. (when pod with same name are recreated multiple times)
// clean up any previous hostVeth ip rule recursively. (when pod with same name are recreated multiple times).
//
// per our understanding, previous we obtain vlanID from pod spec, it could be possible the vlanID is already updated when deleting old pod, thus the hostVeth been cleaned up during oldPod deletion is incorrect.
// now since we obtain vlanID from prevResult during pod deletion, we should be able to correctly purge hostVeth during pod deletion and thus don't need this logic.
// this logic is kept here for safety purpose.
oldFromHostVethRule := n.netLink.NewRule()
oldFromHostVethRule.IifName = hostVethName
oldFromHostVethRule.Priority = vlanRulePriority
Expand All @@ -344,6 +350,7 @@ func (n *linuxNetwork) SetupBranchENIPodNetwork(hostVethName string, contVethNam
} else if v6Addr != nil {
containerAddr = v6Addr
}

switch podSGEnforcingMode {
case sgpp.EnforcingModeStrict:
if err := n.setupIIFBasedContainerRouteRules(hostVeth, containerAddr, vlanLink, rtTable, log); err != nil {
Expand Down Expand Up @@ -537,6 +544,8 @@ func (n *linuxNetwork) teardownIPBasedContainerRouteRules(containerAddr *net.IPN
// we try to delete route and only log a warning even deletion failed.
if err := n.netLink.RouteDel(&route); err != nil && !netlinkwrapper.IsNotExistsError(err) {
log.Warnf("failed to delete container route, containerAddr=%s, rtTable=%v: %v", containerAddr.String(), "main", err)
} else {
log.Debugf("Successfully deleted container route, containerAddr=%s, rtTable=%v", containerAddr.String(), "main")
}

return nil
Expand Down
5 changes: 3 additions & 2 deletions cmd/routed-eni-cni-plugin/driver/mocks/driver_mocks.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 8e6c69c

Please sign in to comment.