Skip to content

Commit

Permalink
Fix CNI crashing when there is no available IP addresses. (#1499)
Browse files Browse the repository at this point in the history 
* Fix two bug in CNI/IPamd code path
1. when IPAMD returns "no ip available error", the err variable get overwritten to nil when get VPCCIDRs
2. when DelNetwork returns err, the r.Success will cause nil-pointer exception and crash CNI

* fix test cases

* address commits
  • Loading branch information
@M00nF1sh
M00nF1sh authored Jun 9, 2021
1 parent 62f1e7a commit a2c966e
Show file tree
Hide file tree
Showing 3 changed files with 142 additions and 59 deletions.
7 changes: 2 additions & 5 deletions cmd/routed-eni-cni-plugin/cni.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,11 +207,8 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
if delErr != nil {
log.Errorf("Error received from DelNetwork grpc call for container %s: %v",
args.ContainerID, delErr)
}

if !r.Success {
log.Errorf("Failed to release IP of container %s: %v",
args.ContainerID, delErr)
} else if !r.Success {
log.Errorf("Failed to release IP of container %s", args.ContainerID)
}
return errors.Wrap(err, "add command: failed to setup network")
}
Expand Down
8 changes: 7 additions & 1 deletion pkg/ipamd/rpc_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,16 @@ func (s *server) AddNetwork(ctx context.Context, in *rpc.AddNetworkRequest) (*rp
NetworkName: in.NetworkName,
}
addr, deviceNumber, err = s.ipamContext.dataStore.AssignPodIPv4Address(ipamKey)
if err != nil {
log.Warnf("Send AddNetworkReply: unable to assign IPv4 address for pod, err: %v", err)
return &failureResponse, nil
}
}

pbVPCcidrs, err := s.ipamContext.awsClient.GetVPCIPv4CIDRs()
if err != nil {
return nil, err
log.Errorf("Send AddNetworkReply: unable to obtain VPC CIDRs, err: %v", err)
return &failureResponse, nil
}
for _, cidr := range pbVPCcidrs {
log.Debugf("VPC CIDR %s", cidr)
Expand Down
186 changes: 133 additions & 53 deletions pkg/ipamd/rpc_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ func TestServer_VersionCheck(t *testing.T) {
networkClient: m.network,
dataStore: datastore.NewDataStore(log, datastore.NullCheckpoint{}),
}
m.awsutils.EXPECT().GetVPCIPv4CIDRs().Return([]string{}, nil)
m.network.EXPECT().UseExternalSNAT().Return(true)
m.awsutils.EXPECT().GetVPCIPv4CIDRs().Return([]string{}, nil).AnyTimes()
m.network.EXPECT().UseExternalSNAT().Return(true).AnyTimes()

rpcServer := server{
version: "1.2.3",
Expand Down Expand Up @@ -79,66 +79,146 @@ func TestServer_VersionCheck(t *testing.T) {
}

func TestServer_AddNetwork(t *testing.T) {
m := setup(t)
defer m.ctrl.Finish()

mockContext := &IPAMContext{
awsClient: m.awsutils,
maxIPsPerENI: 14,
maxENI: 4,
warmENITarget: 1,
warmIPTarget: 3,
networkClient: m.network,
dataStore: datastore.NewDataStore(log, datastore.NullCheckpoint{}),
type getVPCIPv4CIDRsCall struct {
cidrs []string
err error
}

rpcServer := server{
version: "1.2.3",
ipamContext: mockContext,
type useExternalSNATCall struct {
useExternalSNAT bool
}

addNetworkRequest := &pb.AddNetworkRequest{
ClientVersion: "1.2.3",
Netns: "netns",
NetworkName: "net0",
ContainerID: "cid",
IfName: "eni",
type getExcludeSNATCIDRsCall struct {
snatExclusionCIDRs []string
}

vpcCIDRs := []string{vpcCIDR}
testCases := []struct {
name string
useExternalSNAT bool
vpcCIDRs []string
snatExclusionCIDRs []string
type fields struct {
ipV4AddressByENIID map[string][]string
getVPCIPv4CIDRsCalls []getVPCIPv4CIDRsCall
useExternalSNATCalls []useExternalSNATCall
getExcludeSNATCIDRsCalls []getExcludeSNATCIDRsCall
}
tests := []struct {
name string
fields fields
want *pb.AddNetworkReply
wantErr error
}{
{
"VPC CIDRs",
true,
vpcCIDRs,
nil,
name: "successfully allocated IPAddress & use externalSNAT",
fields: fields{
ipV4AddressByENIID: map[string][]string{
"eni-1": {"192.168.1.100"},
},
getVPCIPv4CIDRsCalls: []getVPCIPv4CIDRsCall{
{
cidrs: []string{"10.10.0.0/16"},
},
},
useExternalSNATCalls: []useExternalSNATCall{
{
useExternalSNAT: true,
},
},
},
want: &pb.AddNetworkReply{
Success: true,
IPv4Addr: "192.168.1.100",
DeviceNumber: int32(0),
UseExternalSNAT: true,
VPCcidrs: []string{"10.10.0.0/16"},
},
},
{
name: "successfully allocated IPAddress & not use externalSNAT",
fields: fields{
ipV4AddressByENIID: map[string][]string{
"eni-1": {"192.168.1.100"},
},
getVPCIPv4CIDRsCalls: []getVPCIPv4CIDRsCall{
{
cidrs: []string{"10.10.0.0/16"},
},
},
useExternalSNATCalls: []useExternalSNATCall{
{
useExternalSNAT: false,
},
},
getExcludeSNATCIDRsCalls: []getExcludeSNATCIDRsCall{
{
snatExclusionCIDRs: []string{"10.12.0.0/16", "10.13.0.0/16"},
},
},
},
want: &pb.AddNetworkReply{
Success: true,
IPv4Addr: "192.168.1.100",
DeviceNumber: int32(0),
UseExternalSNAT: false,
VPCcidrs: []string{"10.10.0.0/16", "10.12.0.0/16", "10.13.0.0/16"},
},
},
{
"SNAT Exclusion CIDRs",
false,
vpcCIDRs,
[]string{"10.12.0.0/16", "10.13.0.0/16"},
name: "failed allocated IPAddress ",
fields: fields{
ipV4AddressByENIID: map[string][]string{},
},
want: &pb.AddNetworkReply{
Success: false,
},
},
}
for _, tc := range testCases {
m.awsutils.EXPECT().GetVPCIPv4CIDRs().Return(tc.vpcCIDRs, nil)
m.network.EXPECT().UseExternalSNAT().Return(tc.useExternalSNAT)
if !tc.useExternalSNAT {
m.network.EXPECT().GetExcludeSNATCIDRs().Return(tc.snatExclusionCIDRs)
}

addNetworkReply, err := rpcServer.AddNetwork(context.TODO(), addNetworkRequest)
if assert.NoError(t, err, tc.name) {

assert.Equal(t, tc.useExternalSNAT, addNetworkReply.UseExternalSNAT, tc.name)

expectedCIDRs := append([]string{vpcCIDR}, tc.snatExclusionCIDRs...)
assert.Equal(t, expectedCIDRs, addNetworkReply.VPCcidrs, tc.name)
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
m := setup(t)
defer m.ctrl.Finish()

for _, call := range tt.fields.getVPCIPv4CIDRsCalls {
m.awsutils.EXPECT().GetVPCIPv4CIDRs().Return(call.cidrs, call.err)
}
for _, call := range tt.fields.useExternalSNATCalls {
m.network.EXPECT().UseExternalSNAT().Return(call.useExternalSNAT)
}
for _, call := range tt.fields.getExcludeSNATCIDRsCalls {
m.network.EXPECT().GetExcludeSNATCIDRs().Return(call.snatExclusionCIDRs)
}
ds := datastore.NewDataStore(log, datastore.NullCheckpoint{})
for eniID, ipv4Addresses := range tt.fields.ipV4AddressByENIID {
ds.AddENI(eniID, 0, false, false, false)
for _, ipv4Address := range ipv4Addresses {
ds.AddIPv4AddressToStore(eniID, ipv4Address)
}
}

mockContext := &IPAMContext{
awsClient: m.awsutils,
maxIPsPerENI: 14,
maxENI: 4,
warmENITarget: 1,
warmIPTarget: 3,
networkClient: m.network,
dataStore: ds,
}

s := &server{
version: "1.2.3",
ipamContext: mockContext,
}

req := &pb.AddNetworkRequest{
ClientVersion: "1.2.3",
Netns: "netns",
NetworkName: "net0",
ContainerID: "cid",
IfName: "eni",
}

resp, err := s.AddNetwork(context.Background(), req)
if tt.wantErr != nil {
assert.EqualError(t, err, tt.wantErr.Error())
} else {
assert.NoError(t, err)
assert.Equal(t, tt.want, resp)
}
})
}
}

0 comments on commit a2c966e

Please sign in to comment.