Skip to content

Commit a024d7d

Browse files
rjdenneypaulyufan
rjdenney
and
paulyufan
authored
Fix incorrect error "UnsupportedAPI" returned by CNI (#1889)
* fix dualNIC log error and add an UT * add an UT * fix UT * fix UT * fix UT * fix UTs * fix comments * fix linter issue * fix Ashvin's comments * fix comments * fix comments * fix linter issue --------- Co-authored-by: paulyu <[email protected]>
1 parent cf5d99b commit a024d7d

File tree

2 files changed

+315
-4
lines changed

2 files changed

+315
-4
lines changed

‎cni/network/multitenancy.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -242,7 +242,7 @@ func (m *Multitenancy) getNetworkContainersInternal(
242242
if err != nil && client.IsUnsupportedAPI(err) {
243243
ncConfig, errGetNC := m.cnsclient.GetNetworkContainer(ctx, orchestratorContext)
244244
if errGetNC != nil {
245-
return nil, []net.IPNet{}, fmt.Errorf("%w", err)
245+
return nil, []net.IPNet{}, fmt.Errorf("%w", errGetNC)
246246
}
247247
ncConfigs = append(ncConfigs, *ncConfig)
248248
} else if err != nil {

‎cni/network/multitenancy_test.go

+314-3
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,18 @@ package network
33
import (
44
"context"
55
"encoding/json"
6+
"errors"
67
"net"
78
"testing"
89

910
"github.com/Azure/azure-container-networking/cni"
1011
"github.com/Azure/azure-container-networking/cns"
12+
"github.com/Azure/azure-container-networking/cns/client"
13+
"github.com/Azure/azure-container-networking/cns/types"
1114
"github.com/Azure/azure-container-networking/network"
1215
cniTypes "github.com/containernetworking/cni/pkg/types"
1316
cniTypesCurr "github.com/containernetworking/cni/pkg/types/100"
17+
"github.com/google/go-cmp/cmp"
1418
"github.com/stretchr/testify/require"
1519
)
1620

@@ -42,7 +46,19 @@ type getAllNetworkContainersConfigurationHandler struct {
4246
err error
4347
}
4448

49+
type cnsAPIName string
50+
51+
const (
52+
GetAllNetworkContainers cnsAPIName = "GetAllNetworkContainers"
53+
)
54+
55+
var (
56+
errUnsupportedAPI = errors.New("Unsupported API")
57+
errNoOrchestratorContextFound = errors.New("No CNI OrchestratorContext Found")
58+
)
59+
4560
type MockCNSClient struct {
61+
unsupportedAPIs map[cnsAPIName]struct{}
4662
require *require.Assertions
4763
request requestIPAddressHandler
4864
release releaseIPAddressHandler
@@ -61,12 +77,23 @@ func (c *MockCNSClient) ReleaseIPAddress(_ context.Context, ipconfig cns.IPConfi
6177
}
6278

6379
func (c *MockCNSClient) GetNetworkContainer(ctx context.Context, orchestratorContext []byte) (*cns.GetNetworkContainerResponse, error) {
64-
c.require.Exactly(c.getNetworkContainerConfiguration.orchestratorContext, orchestratorContext)
80+
if !cmp.Equal(c.getNetworkContainerConfiguration.orchestratorContext, orchestratorContext) {
81+
return nil, errNoOrchestratorContextFound
82+
}
6583
return c.getNetworkContainerConfiguration.returnResponse, c.getNetworkContainerConfiguration.err
6684
}
6785

6886
func (c *MockCNSClient) GetAllNetworkContainers(ctx context.Context, orchestratorContext []byte) ([]cns.GetNetworkContainerResponse, error) {
69-
c.require.Exactly(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext)
87+
if _, isUnsupported := c.unsupportedAPIs[GetAllNetworkContainers]; isUnsupported {
88+
e := &client.CNSClientError{}
89+
e.Code = types.UnsupportedAPI
90+
e.Err = errUnsupportedAPI
91+
return nil, e
92+
}
93+
94+
if !cmp.Equal(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext) {
95+
return nil, errNoOrchestratorContextFound
96+
}
7097
return c.getAllNetworkContainersConfiguration.returnResponse, c.getAllNetworkContainersConfiguration.err
7198
}
7299

@@ -456,7 +483,7 @@ func TestGetMultiTenancyCNIResult(t *testing.T) {
456483
tt.args.k8sNamespace,
457484
tt.args.ifName)
458485
if (err != nil) != tt.wantErr {
459-
t.Errorf("GetContainerNetworkConfiguration() error = %v, wantErr %v", err, tt.wantErr)
486+
t.Errorf("GetAllNetworkContainers() error = %v, wantErr %v", err, tt.wantErr)
460487
return
461488
}
462489
if tt.wantErr {
@@ -473,3 +500,287 @@ func TestGetMultiTenancyCNIResult(t *testing.T) {
473500
})
474501
}
475502
}
503+
504+
// TestGetMultiTenancyCNIResultUnsupportedAPI tests if new CNS API is not supported and old CNS API can handle to get ncConfig with "Unsupported API" error
505+
func TestGetMultiTenancyCNIResultUnsupportedAPI(t *testing.T) {
506+
require := require.New(t) //nolint:gocritic
507+
508+
ncResponse := cns.GetNetworkContainerResponse{
509+
PrimaryInterfaceIdentifier: "10.0.0.0/16",
510+
LocalIPConfiguration: cns.IPConfiguration{
511+
IPSubnet: cns.IPSubnet{
512+
IPAddress: "10.0.0.5",
513+
PrefixLength: 16,
514+
},
515+
GatewayIPAddress: "",
516+
},
517+
CnetAddressSpace: []cns.IPSubnet{
518+
{
519+
IPAddress: "10.1.0.0",
520+
PrefixLength: 16,
521+
},
522+
},
523+
IPConfiguration: cns.IPConfiguration{
524+
IPSubnet: cns.IPSubnet{
525+
IPAddress: "10.1.0.5",
526+
PrefixLength: 16,
527+
},
528+
DNSServers: nil,
529+
GatewayIPAddress: "10.1.0.1",
530+
},
531+
Routes: []cns.Route{
532+
{
533+
IPAddress: "10.1.0.0/16",
534+
GatewayIPAddress: "10.1.0.1",
535+
},
536+
},
537+
}
538+
539+
// set new CNS API is not supported
540+
unsupportedAPIs := make(map[cnsAPIName]struct{})
541+
unsupportedAPIs["GetAllNetworkContainers"] = struct{}{}
542+
543+
type args struct {
544+
ctx context.Context
545+
enableInfraVnet bool
546+
nwCfg *cni.NetworkConfig
547+
plugin *NetPlugin
548+
k8sPodName string
549+
k8sNamespace string
550+
ifName string
551+
}
552+
553+
tests := []struct {
554+
name string
555+
args args
556+
want *cns.GetNetworkContainerResponse
557+
wantErr bool
558+
}{
559+
{
560+
name: "test happy path for Unsupported API with old CNS API",
561+
args: args{
562+
enableInfraVnet: true,
563+
nwCfg: &cni.NetworkConfig{
564+
MultiTenancy: true,
565+
EnableSnatOnHost: true,
566+
EnableExactMatchForPodName: true,
567+
InfraVnetAddressSpace: "10.0.0.0/16",
568+
IPAM: cni.IPAM{Type: "azure-vnet-ipam"},
569+
},
570+
plugin: &NetPlugin{
571+
ipamInvoker: NewMockIpamInvoker(false, false, false),
572+
multitenancyClient: &Multitenancy{
573+
netioshim: &mockNetIOShim{},
574+
cnsclient: &MockCNSClient{
575+
unsupportedAPIs: unsupportedAPIs,
576+
require: require,
577+
getNetworkContainerConfiguration: getNetworkContainerConfigurationHandler{
578+
orchestratorContext: marshallPodInfo(cns.KubernetesPodInfo{
579+
PodName: "testpod",
580+
PodNamespace: "testnamespace",
581+
}),
582+
returnResponse: &ncResponse,
583+
},
584+
},
585+
},
586+
},
587+
k8sPodName: "testpod",
588+
k8sNamespace: "testnamespace",
589+
ifName: "eth0",
590+
},
591+
want: &cns.GetNetworkContainerResponse{
592+
PrimaryInterfaceIdentifier: "10.0.0.0/16",
593+
LocalIPConfiguration: cns.IPConfiguration{
594+
IPSubnet: cns.IPSubnet{
595+
IPAddress: "10.0.0.5",
596+
PrefixLength: 16,
597+
},
598+
GatewayIPAddress: "",
599+
},
600+
CnetAddressSpace: []cns.IPSubnet{
601+
{
602+
IPAddress: "10.1.0.0",
603+
PrefixLength: 16,
604+
},
605+
},
606+
IPConfiguration: cns.IPConfiguration{
607+
IPSubnet: cns.IPSubnet{
608+
IPAddress: "10.1.0.5",
609+
PrefixLength: 16,
610+
},
611+
DNSServers: nil,
612+
GatewayIPAddress: "10.1.0.1",
613+
},
614+
Routes: []cns.Route{
615+
{
616+
IPAddress: "10.1.0.0/16",
617+
GatewayIPAddress: "10.1.0.1",
618+
},
619+
},
620+
},
621+
},
622+
}
623+
for _, tt := range tests {
624+
tt := tt
625+
t.Run(tt.name, func(t *testing.T) {
626+
got, err := tt.args.plugin.multitenancyClient.GetAllNetworkContainers(
627+
tt.args.ctx,
628+
tt.args.nwCfg,
629+
tt.args.k8sPodName,
630+
tt.args.k8sNamespace,
631+
tt.args.ifName)
632+
if err != nil && tt.wantErr {
633+
t.Fatalf("expected an error %+v but none received", err)
634+
}
635+
require.NoError(err)
636+
require.Exactly(tt.want, got[0].ncResponse)
637+
})
638+
}
639+
}
640+
641+
// TestGetMultiTenancyCNIResultNotFound test includes two sub test cases:
642+
// 1. CNS supports new API and it does not have orchestratorContext info
643+
// 2. CNS does not support new API and it does not have orchestratorContext info
644+
func TestGetMultiTenancyCNIResultNotFound(t *testing.T) {
645+
require := require.New(t) //nolint:gocritic
646+
647+
ncResponse := cns.GetNetworkContainerResponse{
648+
PrimaryInterfaceIdentifier: "10.0.0.0/16",
649+
LocalIPConfiguration: cns.IPConfiguration{
650+
IPSubnet: cns.IPSubnet{
651+
IPAddress: "10.0.0.5",
652+
PrefixLength: 16,
653+
},
654+
GatewayIPAddress: "",
655+
},
656+
CnetAddressSpace: []cns.IPSubnet{
657+
{
658+
IPAddress: "10.1.0.0",
659+
PrefixLength: 16,
660+
},
661+
},
662+
IPConfiguration: cns.IPConfiguration{
663+
IPSubnet: cns.IPSubnet{
664+
IPAddress: "10.1.0.5",
665+
PrefixLength: 16,
666+
},
667+
DNSServers: nil,
668+
GatewayIPAddress: "10.1.0.1",
669+
},
670+
Routes: []cns.Route{
671+
{
672+
IPAddress: "10.1.0.0/16",
673+
GatewayIPAddress: "10.1.0.1",
674+
},
675+
},
676+
}
677+
678+
// set new CNS API is not supported
679+
unsupportedAPIs := make(map[cnsAPIName]struct{})
680+
unsupportedAPIs["GetAllNetworkContainers"] = struct{}{}
681+
682+
type args struct {
683+
ctx context.Context
684+
enableInfraVnet bool
685+
nwCfg *cni.NetworkConfig
686+
plugin *NetPlugin
687+
k8sPodName string
688+
k8sNamespace string
689+
ifName string
690+
}
691+
692+
tests := []struct {
693+
name string
694+
args args
695+
want *cns.GetNetworkContainerResponse
696+
wantErr bool
697+
}{
698+
{
699+
name: "test happy path, CNS does not support new API without orchestratorContext found",
700+
args: args{
701+
enableInfraVnet: true,
702+
nwCfg: &cni.NetworkConfig{
703+
MultiTenancy: true,
704+
EnableSnatOnHost: true,
705+
EnableExactMatchForPodName: true,
706+
InfraVnetAddressSpace: "10.0.0.0/16",
707+
IPAM: cni.IPAM{Type: "azure-vnet-ipam"},
708+
},
709+
plugin: &NetPlugin{
710+
ipamInvoker: NewMockIpamInvoker(false, false, false),
711+
multitenancyClient: &Multitenancy{
712+
netioshim: &mockNetIOShim{},
713+
cnsclient: &MockCNSClient{
714+
unsupportedAPIs: unsupportedAPIs,
715+
require: require,
716+
getNetworkContainerConfiguration: getNetworkContainerConfigurationHandler{
717+
orchestratorContext: marshallPodInfo(cns.KubernetesPodInfo{
718+
PodName: "testpod",
719+
PodNamespace: "testnamespace",
720+
}),
721+
returnResponse: &ncResponse,
722+
},
723+
},
724+
},
725+
},
726+
// use mismatched k8sPodName and k8sNamespace
727+
k8sPodName: "testpod1",
728+
k8sNamespace: "testnamespace1",
729+
ifName: "eth0",
730+
},
731+
wantErr: true,
732+
},
733+
{
734+
name: "test happy path, CNS does support new API without orchestratorContext found",
735+
args: args{
736+
enableInfraVnet: true,
737+
nwCfg: &cni.NetworkConfig{
738+
MultiTenancy: true,
739+
EnableSnatOnHost: true,
740+
EnableExactMatchForPodName: true,
741+
InfraVnetAddressSpace: "10.0.0.0/16",
742+
IPAM: cni.IPAM{Type: "azure-vnet-ipam"},
743+
},
744+
plugin: &NetPlugin{
745+
ipamInvoker: NewMockIpamInvoker(false, false, false),
746+
multitenancyClient: &Multitenancy{
747+
netioshim: &mockNetIOShim{},
748+
cnsclient: &MockCNSClient{
749+
require: require,
750+
getNetworkContainerConfiguration: getNetworkContainerConfigurationHandler{
751+
orchestratorContext: marshallPodInfo(cns.KubernetesPodInfo{
752+
PodName: "testpod",
753+
PodNamespace: "testnamespace",
754+
}),
755+
returnResponse: &ncResponse,
756+
},
757+
},
758+
},
759+
},
760+
// use mismatched k8sPodName and k8sNamespace
761+
k8sPodName: "testpod1",
762+
k8sNamespace: "testnamespace1",
763+
ifName: "eth0",
764+
},
765+
wantErr: true,
766+
},
767+
}
768+
for _, tt := range tests {
769+
tt := tt
770+
t.Run(tt.name, func(t *testing.T) {
771+
_, err := tt.args.plugin.multitenancyClient.GetAllNetworkContainers(
772+
tt.args.ctx,
773+
tt.args.nwCfg,
774+
tt.args.k8sPodName,
775+
tt.args.k8sNamespace,
776+
tt.args.ifName)
777+
if err == nil && tt.wantErr {
778+
t.Fatalf("expected an error %+v but none received", err)
779+
}
780+
781+
if !errors.Is(err, errNoOrchestratorContextFound) {
782+
t.Fatalf("expected an error %s but %v received", errNoOrchestratorContextFound, err)
783+
}
784+
})
785+
}
786+
}

0 commit comments

Comments
 (0)