From d9177c9280641a661c6f8b842d2078163907db44 Mon Sep 17 00:00:00 2001 From: paulyu Date: Sat, 1 Apr 2023 10:00:56 -0400 Subject: [PATCH 01/12] fix dualNIC log error and add an UT --- cni/network/multitenancy.go | 2 +- cni/network/multitenancy_test.go | 151 ++++++++++++++++++++++++++++++- 2 files changed, 151 insertions(+), 2 deletions(-) diff --git a/cni/network/multitenancy.go b/cni/network/multitenancy.go index f39a429bd2..8c5f5fdcf5 100644 --- a/cni/network/multitenancy.go +++ b/cni/network/multitenancy.go @@ -242,7 +242,7 @@ func (m *Multitenancy) getNetworkContainersInternal( if err != nil && client.IsUnsupportedAPI(err) { ncConfig, errGetNC := m.cnsclient.GetNetworkContainer(ctx, orchestratorContext) if errGetNC != nil { - return nil, []net.IPNet{}, fmt.Errorf("%w", err) + return nil, []net.IPNet{}, fmt.Errorf("%w", errGetNC) } ncConfigs = append(ncConfigs, *ncConfig) } else if err != nil { diff --git a/cni/network/multitenancy_test.go b/cni/network/multitenancy_test.go index 0d1d66c82d..62024773c2 100644 --- a/cni/network/multitenancy_test.go +++ b/cni/network/multitenancy_test.go @@ -3,11 +3,15 @@ package network import ( "context" "encoding/json" + "errors" "net" + "reflect" "testing" "github.com/Azure/azure-container-networking/cni" "github.com/Azure/azure-container-networking/cns" + "github.com/Azure/azure-container-networking/cns/client" + "github.com/Azure/azure-container-networking/cns/types" "github.com/Azure/azure-container-networking/network" cniTypes "github.com/containernetworking/cni/pkg/types" cniTypesCurr "github.com/containernetworking/cni/pkg/types/100" @@ -66,6 +70,16 @@ func (c *MockCNSClient) GetNetworkContainer(ctx context.Context, orchestratorCon } func (c *MockCNSClient) GetAllNetworkContainers(ctx context.Context, orchestratorContext []byte) ([]cns.GetNetworkContainerResponse, error) { + // check if getAllNetworkContainersConfiguration orchestratorContext is empty + if c.getAllNetworkContainersConfiguration.orchestratorContext == nil { + // if orchestratorContext obj is same, attach CNSClientError code and error with Unsupported API and try old CNS API GetNetworkContainer() later + if reflect.DeepEqual(c.getNetworkContainerConfiguration.orchestratorContext, orchestratorContext) { + e := &client.CNSClientError{} + e.Code = types.UnsupportedAPI + e.Err = errors.New("Unsupported API") + return nil, e + } + } c.require.Exactly(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext) return c.getAllNetworkContainersConfiguration.returnResponse, c.getAllNetworkContainersConfiguration.err } @@ -456,7 +470,7 @@ func TestGetMultiTenancyCNIResult(t *testing.T) { tt.args.k8sNamespace, tt.args.ifName) if (err != nil) != tt.wantErr { - t.Errorf("GetContainerNetworkConfiguration() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("GetAllNetworkContainers() error = %v, wantErr %v", err, tt.wantErr) return } if tt.wantErr { @@ -473,3 +487,138 @@ func TestGetMultiTenancyCNIResult(t *testing.T) { }) } } + +func TestGetMultiTenancyCNIResultWithOldCNS(t *testing.T) { + require := require.New(t) //nolint:gocritic + + ncResponse := cns.GetNetworkContainerResponse{ + PrimaryInterfaceIdentifier: "10.0.0.0/16", + LocalIPConfiguration: cns.IPConfiguration{ + IPSubnet: cns.IPSubnet{ + IPAddress: "10.0.0.5", + PrefixLength: 16, + }, + GatewayIPAddress: "", + }, + CnetAddressSpace: []cns.IPSubnet{ + { + IPAddress: "10.1.0.0", + PrefixLength: 16, + }, + }, + IPConfiguration: cns.IPConfiguration{ + IPSubnet: cns.IPSubnet{ + IPAddress: "10.1.0.5", + PrefixLength: 16, + }, + DNSServers: nil, + GatewayIPAddress: "10.1.0.1", + }, + Routes: []cns.Route{ + { + IPAddress: "10.1.0.0/16", + GatewayIPAddress: "10.1.0.1", + }, + }, + } + + type args struct { + ctx context.Context + enableInfraVnet bool + nwCfg *cni.NetworkConfig + plugin *NetPlugin + k8sPodName string + k8sNamespace string + ifName string + } + + tests := []struct { + name string + args args + want *cns.GetNetworkContainerResponse + wantErr bool + }{ + { + name: "test happy path with old CNS and Unsupported API", + args: args{ + enableInfraVnet: true, + nwCfg: &cni.NetworkConfig{ + MultiTenancy: true, + EnableSnatOnHost: true, + EnableExactMatchForPodName: true, + InfraVnetAddressSpace: "10.0.0.0/16", + IPAM: cni.IPAM{Type: "azure-vnet-ipam"}, + }, + plugin: &NetPlugin{ + ipamInvoker: NewMockIpamInvoker(false, false, false), + multitenancyClient: &Multitenancy{ + netioshim: &mockNetIOShim{}, + cnsclient: &MockCNSClient{ + require: require, + getNetworkContainerConfiguration: getNetworkContainerConfigurationHandler{ + orchestratorContext: marshallPodInfo(cns.KubernetesPodInfo{ + PodName: "testpod", + PodNamespace: "testnamespace", + }), + returnResponse: &ncResponse, + }, + }, + }, + }, + k8sPodName: "testpod", + k8sNamespace: "testnamespace", + ifName: "eth0", + }, + want: &cns.GetNetworkContainerResponse{ + PrimaryInterfaceIdentifier: "10.0.0.0/16", + LocalIPConfiguration: cns.IPConfiguration{ + IPSubnet: cns.IPSubnet{ + IPAddress: "10.0.0.5", + PrefixLength: 16, + }, + GatewayIPAddress: "", + }, + CnetAddressSpace: []cns.IPSubnet{ + { + IPAddress: "10.1.0.0", + PrefixLength: 16, + }, + }, + IPConfiguration: cns.IPConfiguration{ + IPSubnet: cns.IPSubnet{ + IPAddress: "10.1.0.5", + PrefixLength: 16, + }, + DNSServers: nil, + GatewayIPAddress: "10.1.0.1", + }, + Routes: []cns.Route{ + { + IPAddress: "10.1.0.0/16", + GatewayIPAddress: "10.1.0.1", + }, + }, + }, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + got, err := tt.args.plugin.multitenancyClient.GetAllNetworkContainers( + tt.args.ctx, + tt.args.nwCfg, + tt.args.k8sPodName, + tt.args.k8sNamespace, + tt.args.ifName) + if (err != nil) != tt.wantErr { + t.Errorf("GetAllNetworkContainers() error = %v, wantErr %v", err, tt.wantErr) + return + } + if tt.wantErr { + require.Error(err) + } + require.NoError(err) + require.Exactly(tt.want, got[0].ncResponse) + }) + } +} From 5426afbb7fdda5621ec6413d47fcd991c45184df Mon Sep 17 00:00:00 2001 From: paulyu Date: Sat, 1 Apr 2023 10:51:02 -0400 Subject: [PATCH 02/12] add an UT --- cni/network/multitenancy_test.go | 144 ++++++++++++++++++++++++++++++- 1 file changed, 140 insertions(+), 4 deletions(-) diff --git a/cni/network/multitenancy_test.go b/cni/network/multitenancy_test.go index 62024773c2..107fec5416 100644 --- a/cni/network/multitenancy_test.go +++ b/cni/network/multitenancy_test.go @@ -70,14 +70,16 @@ func (c *MockCNSClient) GetNetworkContainer(ctx context.Context, orchestratorCon } func (c *MockCNSClient) GetAllNetworkContainers(ctx context.Context, orchestratorContext []byte) ([]cns.GetNetworkContainerResponse, error) { - // check if getAllNetworkContainersConfiguration orchestratorContext is empty + // check if getAllNetworkContainersConfiguration orchestratorContext is nil using GetAllNetworkContainers() if c.getAllNetworkContainersConfiguration.orchestratorContext == nil { - // if orchestratorContext obj is same, attach CNSClientError code and error with Unsupported API and try old CNS API GetNetworkContainer() later + // if orchestratorContext is same, attach CNSClientError code and error with Unsupported API and try old CNS API GetNetworkContainer() later if reflect.DeepEqual(c.getNetworkContainerConfiguration.orchestratorContext, orchestratorContext) { e := &client.CNSClientError{} e.Code = types.UnsupportedAPI e.Err = errors.New("Unsupported API") return nil, e + } else { + return nil, errors.New("Failed to use GetNetworkContainer()") } } c.require.Exactly(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext) @@ -488,7 +490,7 @@ func TestGetMultiTenancyCNIResult(t *testing.T) { } } -func TestGetMultiTenancyCNIResultWithOldCNS(t *testing.T) { +func TestGetMultiTenancyCNIResultUnsupportedAPI(t *testing.T) { require := require.New(t) //nolint:gocritic ncResponse := cns.GetNetworkContainerResponse{ @@ -539,7 +541,7 @@ func TestGetMultiTenancyCNIResultWithOldCNS(t *testing.T) { wantErr bool }{ { - name: "test happy path with old CNS and Unsupported API", + name: "test happy path for Unsupported API with old CNS API", args: args{ enableInfraVnet: true, nwCfg: &cni.NetworkConfig{ @@ -622,3 +624,137 @@ func TestGetMultiTenancyCNIResultWithOldCNS(t *testing.T) { }) } } + +func TestGetMultiTenancyCNIResultNotUnsupportedAPIError(t *testing.T) { + require := require.New(t) //nolint:gocritic + + ncResponse := cns.GetNetworkContainerResponse{ + PrimaryInterfaceIdentifier: "10.0.0.0/16", + LocalIPConfiguration: cns.IPConfiguration{ + IPSubnet: cns.IPSubnet{ + IPAddress: "10.0.0.5", + PrefixLength: 16, + }, + GatewayIPAddress: "", + }, + CnetAddressSpace: []cns.IPSubnet{ + { + IPAddress: "10.1.0.0", + PrefixLength: 16, + }, + }, + IPConfiguration: cns.IPConfiguration{ + IPSubnet: cns.IPSubnet{ + IPAddress: "10.1.0.5", + PrefixLength: 16, + }, + DNSServers: nil, + GatewayIPAddress: "10.1.0.1", + }, + Routes: []cns.Route{ + { + IPAddress: "10.1.0.0/16", + GatewayIPAddress: "10.1.0.1", + }, + }, + } + + type args struct { + ctx context.Context + enableInfraVnet bool + nwCfg *cni.NetworkConfig + plugin *NetPlugin + k8sPodName string + k8sNamespace string + ifName string + } + + tests := []struct { + name string + args args + want *cns.GetNetworkContainerResponse + wantErr bool + }{ + { + name: "test happy path for non Unsupported API error", + args: args{ + enableInfraVnet: true, + nwCfg: &cni.NetworkConfig{ + MultiTenancy: true, + EnableSnatOnHost: true, + EnableExactMatchForPodName: true, + InfraVnetAddressSpace: "10.0.0.0/16", + IPAM: cni.IPAM{Type: "azure-vnet-ipam"}, + }, + plugin: &NetPlugin{ + ipamInvoker: NewMockIpamInvoker(false, false, false), + multitenancyClient: &Multitenancy{ + netioshim: &mockNetIOShim{}, + cnsclient: &MockCNSClient{ + require: require, + getNetworkContainerConfiguration: getNetworkContainerConfigurationHandler{ + orchestratorContext: marshallPodInfo(cns.KubernetesPodInfo{ + PodName: "testpod", + PodNamespace: "testnamespace", + }), + returnResponse: &ncResponse, + }, + }, + }, + }, + k8sPodName: "testpod1", + k8sNamespace: "testnamespace1", + ifName: "eth0", + }, + want: &cns.GetNetworkContainerResponse{ + PrimaryInterfaceIdentifier: "10.0.0.0/16", + LocalIPConfiguration: cns.IPConfiguration{ + IPSubnet: cns.IPSubnet{ + IPAddress: "10.0.0.5", + PrefixLength: 16, + }, + GatewayIPAddress: "", + }, + CnetAddressSpace: []cns.IPSubnet{ + { + IPAddress: "10.1.0.0", + PrefixLength: 16, + }, + }, + IPConfiguration: cns.IPConfiguration{ + IPSubnet: cns.IPSubnet{ + IPAddress: "10.1.0.5", + PrefixLength: 16, + }, + DNSServers: nil, + GatewayIPAddress: "10.1.0.1", + }, + Routes: []cns.Route{ + { + IPAddress: "10.1.0.0/16", + GatewayIPAddress: "10.1.0.1", + }, + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + _, err := tt.args.plugin.multitenancyClient.GetAllNetworkContainers( + tt.args.ctx, + tt.args.nwCfg, + tt.args.k8sPodName, + tt.args.k8sNamespace, + tt.args.ifName) + if (err != nil) != tt.wantErr { + t.Errorf("GetAllNetworkContainers() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !tt.wantErr && err != errors.New("Failed to use GetNetworkContainer()") { + require.Error(err) + } + }) + } +} From cccc81f41c9bb0edbed14ffa69951e05d563c1bf Mon Sep 17 00:00:00 2001 From: paulyu Date: Tue, 4 Apr 2023 17:26:09 -0400 Subject: [PATCH 03/12] fix UT --- cni/network/multitenancy_test.go | 36 +++++++++++++++++--------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/cni/network/multitenancy_test.go b/cni/network/multitenancy_test.go index 107fec5416..f732349c42 100644 --- a/cni/network/multitenancy_test.go +++ b/cni/network/multitenancy_test.go @@ -5,7 +5,6 @@ import ( "encoding/json" "errors" "net" - "reflect" "testing" "github.com/Azure/azure-container-networking/cni" @@ -47,6 +46,7 @@ type getAllNetworkContainersConfigurationHandler struct { } type MockCNSClient struct { + isCNSSupportingGetAllNcConfigsAPI bool require *require.Assertions request requestIPAddressHandler release releaseIPAddressHandler @@ -70,18 +70,13 @@ func (c *MockCNSClient) GetNetworkContainer(ctx context.Context, orchestratorCon } func (c *MockCNSClient) GetAllNetworkContainers(ctx context.Context, orchestratorContext []byte) ([]cns.GetNetworkContainerResponse, error) { - // check if getAllNetworkContainersConfiguration orchestratorContext is nil using GetAllNetworkContainers() - if c.getAllNetworkContainersConfiguration.orchestratorContext == nil { - // if orchestratorContext is same, attach CNSClientError code and error with Unsupported API and try old CNS API GetNetworkContainer() later - if reflect.DeepEqual(c.getNetworkContainerConfiguration.orchestratorContext, orchestratorContext) { - e := &client.CNSClientError{} - e.Code = types.UnsupportedAPI - e.Err = errors.New("Unsupported API") - return nil, e - } else { - return nil, errors.New("Failed to use GetNetworkContainer()") - } + if !c.isCNSSupportingGetAllNcConfigsAPI { + e := &client.CNSClientError{} + e.Code = types.UnsupportedAPI + e.Err = errors.New("Unsupported API") + return nil, e } + c.require.Exactly(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext) return c.getAllNetworkContainersConfiguration.returnResponse, c.getAllNetworkContainersConfiguration.err } @@ -348,7 +343,8 @@ func TestGetMultiTenancyCNIResult(t *testing.T) { multitenancyClient: &Multitenancy{ netioshim: &mockNetIOShim{}, cnsclient: &MockCNSClient{ - require: require, + isCNSSupportingGetAllNcConfigsAPI: true, + require: require, getAllNetworkContainersConfiguration: getAllNetworkContainersConfigurationHandler{ orchestratorContext: marshallPodInfo(cns.KubernetesPodInfo{ PodName: "testpod", @@ -490,6 +486,7 @@ func TestGetMultiTenancyCNIResult(t *testing.T) { } } +// this will test if new CNS API is not supported and old CNS API can handle to get ncConfig with "Unsupported API" error func TestGetMultiTenancyCNIResultUnsupportedAPI(t *testing.T) { require := require.New(t) //nolint:gocritic @@ -556,7 +553,8 @@ func TestGetMultiTenancyCNIResultUnsupportedAPI(t *testing.T) { multitenancyClient: &Multitenancy{ netioshim: &mockNetIOShim{}, cnsclient: &MockCNSClient{ - require: require, + isCNSSupportingGetAllNcConfigsAPI: false, + require: require, getNetworkContainerConfiguration: getNetworkContainerConfigurationHandler{ orchestratorContext: marshallPodInfo(cns.KubernetesPodInfo{ PodName: "testpod", @@ -625,7 +623,10 @@ func TestGetMultiTenancyCNIResultUnsupportedAPI(t *testing.T) { } } -func TestGetMultiTenancyCNIResultNotUnsupportedAPIError(t *testing.T) { +// this test case includes two sub test cases: +// 1. CNS supports new API and it does not have orchestratorContext info +// 2. CNS does not support new API and it does not have orchestratorContext info +func TestGetMultiTenancyCNIResultNotFound(t *testing.T) { require := require.New(t) //nolint:gocritic ncResponse := cns.GetNetworkContainerResponse{ @@ -676,7 +677,7 @@ func TestGetMultiTenancyCNIResultNotUnsupportedAPIError(t *testing.T) { wantErr bool }{ { - name: "test happy path for non Unsupported API error", + name: "test happy path, CNS does support new API", args: args{ enableInfraVnet: true, nwCfg: &cni.NetworkConfig{ @@ -691,7 +692,8 @@ func TestGetMultiTenancyCNIResultNotUnsupportedAPIError(t *testing.T) { multitenancyClient: &Multitenancy{ netioshim: &mockNetIOShim{}, cnsclient: &MockCNSClient{ - require: require, + isCNSSupportingGetAllNcConfigsAPI: true, + require: require, getNetworkContainerConfiguration: getNetworkContainerConfigurationHandler{ orchestratorContext: marshallPodInfo(cns.KubernetesPodInfo{ PodName: "testpod", From cfc862f468f12b8ca281f78fee730572cef13eb1 Mon Sep 17 00:00:00 2001 From: paulyu Date: Tue, 4 Apr 2023 19:10:21 -0400 Subject: [PATCH 04/12] fix UT --- cni/network/multitenancy_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/cni/network/multitenancy_test.go b/cni/network/multitenancy_test.go index f732349c42..59c231cc11 100644 --- a/cni/network/multitenancy_test.go +++ b/cni/network/multitenancy_test.go @@ -45,8 +45,14 @@ type getAllNetworkContainersConfigurationHandler struct { err error } +type cnsAPIName string + +const ( + GetAllNetworkContainers cnsAPIName = "GetAllNetworkContainers" +) + type MockCNSClient struct { - isCNSSupportingGetAllNcConfigsAPI bool + unsupportedAPIs map[cnsApiName]struct{} require *require.Assertions request requestIPAddressHandler release releaseIPAddressHandler @@ -70,7 +76,7 @@ func (c *MockCNSClient) GetNetworkContainer(ctx context.Context, orchestratorCon } func (c *MockCNSClient) GetAllNetworkContainers(ctx context.Context, orchestratorContext []byte) ([]cns.GetNetworkContainerResponse, error) { - if !c.isCNSSupportingGetAllNcConfigsAPI { + if _, isUnsupported := c.unsupportedAPIs[GetAllNetworkContainers]; isUnsupported { e := &client.CNSClientError{} e.Code = types.UnsupportedAPI e.Err = errors.New("Unsupported API") From 3384d2402a7136de9915f3a877af17bafb6c1b61 Mon Sep 17 00:00:00 2001 From: paulyu Date: Tue, 4 Apr 2023 23:15:25 -0400 Subject: [PATCH 05/12] fix UT --- cni/network/multitenancy_test.go | 92 ++++++++++++++++++-------------- 1 file changed, 53 insertions(+), 39 deletions(-) diff --git a/cni/network/multitenancy_test.go b/cni/network/multitenancy_test.go index 59c231cc11..22b7cb7d70 100644 --- a/cni/network/multitenancy_test.go +++ b/cni/network/multitenancy_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "net" + "reflect" "testing" "github.com/Azure/azure-container-networking/cni" @@ -52,7 +53,7 @@ const ( ) type MockCNSClient struct { - unsupportedAPIs map[cnsApiName]struct{} + unsupportedAPIs map[cnsAPIName]bool require *require.Assertions request requestIPAddressHandler release releaseIPAddressHandler @@ -71,7 +72,9 @@ func (c *MockCNSClient) ReleaseIPAddress(_ context.Context, ipconfig cns.IPConfi } func (c *MockCNSClient) GetNetworkContainer(ctx context.Context, orchestratorContext []byte) (*cns.GetNetworkContainerResponse, error) { - c.require.Exactly(c.getNetworkContainerConfiguration.orchestratorContext, orchestratorContext) + if !reflect.DeepEqual(c.getNetworkContainerConfiguration.orchestratorContext, orchestratorContext) { + return nil, errors.New("No CNI OrchestratorContext Found") + } return c.getNetworkContainerConfiguration.returnResponse, c.getNetworkContainerConfiguration.err } @@ -83,7 +86,9 @@ func (c *MockCNSClient) GetAllNetworkContainers(ctx context.Context, orchestrato return nil, e } - c.require.Exactly(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext) + if !reflect.DeepEqual(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext) { + return nil, errors.New("No CNI OrchestratorContext Found") + } return c.getAllNetworkContainersConfiguration.returnResponse, c.getAllNetworkContainersConfiguration.err } @@ -349,8 +354,7 @@ func TestGetMultiTenancyCNIResult(t *testing.T) { multitenancyClient: &Multitenancy{ netioshim: &mockNetIOShim{}, cnsclient: &MockCNSClient{ - isCNSSupportingGetAllNcConfigsAPI: true, - require: require, + require: require, getAllNetworkContainersConfiguration: getAllNetworkContainersConfigurationHandler{ orchestratorContext: marshallPodInfo(cns.KubernetesPodInfo{ PodName: "testpod", @@ -527,6 +531,10 @@ func TestGetMultiTenancyCNIResultUnsupportedAPI(t *testing.T) { }, } + // set new CNS API is not supported + unsupportedAPIs := make(map[cnsAPIName]bool) + unsupportedAPIs["GetAllNetworkContainers"] = true + type args struct { ctx context.Context enableInfraVnet bool @@ -559,8 +567,8 @@ func TestGetMultiTenancyCNIResultUnsupportedAPI(t *testing.T) { multitenancyClient: &Multitenancy{ netioshim: &mockNetIOShim{}, cnsclient: &MockCNSClient{ - isCNSSupportingGetAllNcConfigsAPI: false, - require: require, + unsupportedAPIs: unsupportedAPIs, + require: require, getNetworkContainerConfiguration: getNetworkContainerConfigurationHandler{ orchestratorContext: marshallPodInfo(cns.KubernetesPodInfo{ PodName: "testpod", @@ -666,6 +674,10 @@ func TestGetMultiTenancyCNIResultNotFound(t *testing.T) { }, } + // set new CNS API is not supported + unsupportedAPIs := make(map[cnsAPIName]bool) + unsupportedAPIs["GetAllNetworkContainers"] = true + type args struct { ctx context.Context enableInfraVnet bool @@ -683,7 +695,7 @@ func TestGetMultiTenancyCNIResultNotFound(t *testing.T) { wantErr bool }{ { - name: "test happy path, CNS does support new API", + name: "test happy path, CNS does not support new API without orchestratorContext found", args: args{ enableInfraVnet: true, nwCfg: &cni.NetworkConfig{ @@ -698,8 +710,8 @@ func TestGetMultiTenancyCNIResultNotFound(t *testing.T) { multitenancyClient: &Multitenancy{ netioshim: &mockNetIOShim{}, cnsclient: &MockCNSClient{ - isCNSSupportingGetAllNcConfigsAPI: true, - require: require, + unsupportedAPIs: unsupportedAPIs, + require: require, getNetworkContainerConfiguration: getNetworkContainerConfigurationHandler{ orchestratorContext: marshallPodInfo(cns.KubernetesPodInfo{ PodName: "testpod", @@ -710,39 +722,44 @@ func TestGetMultiTenancyCNIResultNotFound(t *testing.T) { }, }, }, + // use mismatched k8sPodName and k8sNamespace k8sPodName: "testpod1", k8sNamespace: "testnamespace1", ifName: "eth0", }, - want: &cns.GetNetworkContainerResponse{ - PrimaryInterfaceIdentifier: "10.0.0.0/16", - LocalIPConfiguration: cns.IPConfiguration{ - IPSubnet: cns.IPSubnet{ - IPAddress: "10.0.0.5", - PrefixLength: 16, - }, - GatewayIPAddress: "", - }, - CnetAddressSpace: []cns.IPSubnet{ - { - IPAddress: "10.1.0.0", - PrefixLength: 16, - }, - }, - IPConfiguration: cns.IPConfiguration{ - IPSubnet: cns.IPSubnet{ - IPAddress: "10.1.0.5", - PrefixLength: 16, - }, - DNSServers: nil, - GatewayIPAddress: "10.1.0.1", + wantErr: true, + }, + { + name: "test happy path, CNS does support new API without orchestratorContext found", + args: args{ + enableInfraVnet: true, + nwCfg: &cni.NetworkConfig{ + MultiTenancy: true, + EnableSnatOnHost: true, + EnableExactMatchForPodName: true, + InfraVnetAddressSpace: "10.0.0.0/16", + IPAM: cni.IPAM{Type: "azure-vnet-ipam"}, }, - Routes: []cns.Route{ - { - IPAddress: "10.1.0.0/16", - GatewayIPAddress: "10.1.0.1", + plugin: &NetPlugin{ + ipamInvoker: NewMockIpamInvoker(false, false, false), + multitenancyClient: &Multitenancy{ + netioshim: &mockNetIOShim{}, + cnsclient: &MockCNSClient{ + require: require, + getNetworkContainerConfiguration: getNetworkContainerConfigurationHandler{ + orchestratorContext: marshallPodInfo(cns.KubernetesPodInfo{ + PodName: "testpod", + PodNamespace: "testnamespace", + }), + returnResponse: &ncResponse, + }, + }, }, }, + // use mismatched k8sPodName and k8sNamespace + k8sPodName: "testpod1", + k8sNamespace: "testnamespace1", + ifName: "eth0", }, wantErr: true, }, @@ -760,9 +777,6 @@ func TestGetMultiTenancyCNIResultNotFound(t *testing.T) { t.Errorf("GetAllNetworkContainers() error = %v, wantErr %v", err, tt.wantErr) return } - if !tt.wantErr && err != errors.New("Failed to use GetNetworkContainer()") { - require.Error(err) - } }) } } From 8f8ec2c7e39ee75f646f3c0fd61a789ca510ccc6 Mon Sep 17 00:00:00 2001 From: paulyu Date: Tue, 4 Apr 2023 23:30:10 -0400 Subject: [PATCH 06/12] fix UTs --- cni/network/multitenancy_test.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/cni/network/multitenancy_test.go b/cni/network/multitenancy_test.go index 22b7cb7d70..a9dbad348c 100644 --- a/cni/network/multitenancy_test.go +++ b/cni/network/multitenancy_test.go @@ -52,6 +52,11 @@ const ( GetAllNetworkContainers cnsAPIName = "GetAllNetworkContainers" ) +var ( + errUnsupportedAPI = errors.New("Unsupported API") + errNoOrchestratorContextFound = errors.New("No CNI OrchestratorContext Found") +) + type MockCNSClient struct { unsupportedAPIs map[cnsAPIName]bool require *require.Assertions @@ -73,7 +78,7 @@ func (c *MockCNSClient) ReleaseIPAddress(_ context.Context, ipconfig cns.IPConfi func (c *MockCNSClient) GetNetworkContainer(ctx context.Context, orchestratorContext []byte) (*cns.GetNetworkContainerResponse, error) { if !reflect.DeepEqual(c.getNetworkContainerConfiguration.orchestratorContext, orchestratorContext) { - return nil, errors.New("No CNI OrchestratorContext Found") + return nil, errNoOrchestratorContextFound } return c.getNetworkContainerConfiguration.returnResponse, c.getNetworkContainerConfiguration.err } @@ -82,12 +87,12 @@ func (c *MockCNSClient) GetAllNetworkContainers(ctx context.Context, orchestrato if _, isUnsupported := c.unsupportedAPIs[GetAllNetworkContainers]; isUnsupported { e := &client.CNSClientError{} e.Code = types.UnsupportedAPI - e.Err = errors.New("Unsupported API") + e.Err = errUnsupportedAPI return nil, e } if !reflect.DeepEqual(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext) { - return nil, errors.New("No CNI OrchestratorContext Found") + return nil, errNoOrchestratorContextFound } return c.getAllNetworkContainersConfiguration.returnResponse, c.getAllNetworkContainersConfiguration.err } From 786fdea215f1fc6359b0a1076485cb58399a24ec Mon Sep 17 00:00:00 2001 From: paulyu Date: Wed, 5 Apr 2023 13:09:01 -0400 Subject: [PATCH 07/12] fix comments --- cni/network/multitenancy_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cni/network/multitenancy_test.go b/cni/network/multitenancy_test.go index a9dbad348c..2e645c10d8 100644 --- a/cni/network/multitenancy_test.go +++ b/cni/network/multitenancy_test.go @@ -782,6 +782,9 @@ func TestGetMultiTenancyCNIResultNotFound(t *testing.T) { t.Errorf("GetAllNetworkContainers() error = %v, wantErr %v", err, tt.wantErr) return } + if err != errNoOrchestratorContextFound { + require.Error(err) + } }) } } From c08af0e7b4bc7b6f788a56d3f0508fb5fe0ae09c Mon Sep 17 00:00:00 2001 From: paulyu Date: Wed, 5 Apr 2023 14:04:11 -0400 Subject: [PATCH 08/12] fix linter issue --- cni/network/multitenancy_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cni/network/multitenancy_test.go b/cni/network/multitenancy_test.go index 2e645c10d8..bdb3659469 100644 --- a/cni/network/multitenancy_test.go +++ b/cni/network/multitenancy_test.go @@ -782,7 +782,8 @@ func TestGetMultiTenancyCNIResultNotFound(t *testing.T) { t.Errorf("GetAllNetworkContainers() error = %v, wantErr %v", err, tt.wantErr) return } - if err != errNoOrchestratorContextFound { + + if errors.Is(err, errNoOrchestratorContextFound) { require.Error(err) } }) From ae34861cb75f1122295ecbf647e2e51a9eec721f Mon Sep 17 00:00:00 2001 From: paulyu Date: Wed, 5 Apr 2023 18:58:52 -0400 Subject: [PATCH 09/12] fix Ashvin's comments --- cni/network/multitenancy_test.go | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/cni/network/multitenancy_test.go b/cni/network/multitenancy_test.go index bdb3659469..f812194c34 100644 --- a/cni/network/multitenancy_test.go +++ b/cni/network/multitenancy_test.go @@ -58,7 +58,7 @@ var ( ) type MockCNSClient struct { - unsupportedAPIs map[cnsAPIName]bool + unsupportedAPIs map[cnsAPIName]struct{} require *require.Assertions request requestIPAddressHandler release releaseIPAddressHandler @@ -501,7 +501,7 @@ func TestGetMultiTenancyCNIResult(t *testing.T) { } } -// this will test if new CNS API is not supported and old CNS API can handle to get ncConfig with "Unsupported API" error +// TestGetMultiTenancyCNIResultUnsupportedAPI tests if new CNS API is not supported and old CNS API can handle to get ncConfig with "Unsupported API" error func TestGetMultiTenancyCNIResultUnsupportedAPI(t *testing.T) { require := require.New(t) //nolint:gocritic @@ -537,8 +537,8 @@ func TestGetMultiTenancyCNIResultUnsupportedAPI(t *testing.T) { } // set new CNS API is not supported - unsupportedAPIs := make(map[cnsAPIName]bool) - unsupportedAPIs["GetAllNetworkContainers"] = true + unsupportedAPIs := make(map[cnsAPIName]struct{}) + unsupportedAPIs["GetAllNetworkContainers"] = struct{}{} type args struct { ctx context.Context @@ -630,6 +630,7 @@ func TestGetMultiTenancyCNIResultUnsupportedAPI(t *testing.T) { tt.args.k8sNamespace, tt.args.ifName) if (err != nil) != tt.wantErr { + // expect an error that GetAllNetworkContainers() fails to get all network containers t.Errorf("GetAllNetworkContainers() error = %v, wantErr %v", err, tt.wantErr) return } @@ -642,7 +643,7 @@ func TestGetMultiTenancyCNIResultUnsupportedAPI(t *testing.T) { } } -// this test case includes two sub test cases: +// TestGetMultiTenancyCNIResultNotFound test includes two sub test cases: // 1. CNS supports new API and it does not have orchestratorContext info // 2. CNS does not support new API and it does not have orchestratorContext info func TestGetMultiTenancyCNIResultNotFound(t *testing.T) { @@ -680,8 +681,8 @@ func TestGetMultiTenancyCNIResultNotFound(t *testing.T) { } // set new CNS API is not supported - unsupportedAPIs := make(map[cnsAPIName]bool) - unsupportedAPIs["GetAllNetworkContainers"] = true + unsupportedAPIs := make(map[cnsAPIName]struct{}) + unsupportedAPIs["GetAllNetworkContainers"] = struct{}{} type args struct { ctx context.Context From f49eaa7fa38c52c8b7c4bb53be30e9ad42d1a605 Mon Sep 17 00:00:00 2001 From: paulyu Date: Thu, 6 Apr 2023 12:43:30 -0400 Subject: [PATCH 10/12] fix comments --- cni/network/multitenancy_test.go | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/cni/network/multitenancy_test.go b/cni/network/multitenancy_test.go index f812194c34..e266c7d59c 100644 --- a/cni/network/multitenancy_test.go +++ b/cni/network/multitenancy_test.go @@ -4,8 +4,8 @@ import ( "context" "encoding/json" "errors" + "github.com/google/go-cmp/cmp" "net" - "reflect" "testing" "github.com/Azure/azure-container-networking/cni" @@ -77,7 +77,7 @@ func (c *MockCNSClient) ReleaseIPAddress(_ context.Context, ipconfig cns.IPConfi } func (c *MockCNSClient) GetNetworkContainer(ctx context.Context, orchestratorContext []byte) (*cns.GetNetworkContainerResponse, error) { - if !reflect.DeepEqual(c.getNetworkContainerConfiguration.orchestratorContext, orchestratorContext) { + if !cmp.Equal(c.getNetworkContainerConfiguration.orchestratorContext, orchestratorContext) { return nil, errNoOrchestratorContextFound } return c.getNetworkContainerConfiguration.returnResponse, c.getNetworkContainerConfiguration.err @@ -91,7 +91,7 @@ func (c *MockCNSClient) GetAllNetworkContainers(ctx context.Context, orchestrato return nil, e } - if !reflect.DeepEqual(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext) { + if !cmp.Equal(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext) { return nil, errNoOrchestratorContextFound } return c.getAllNetworkContainersConfiguration.returnResponse, c.getAllNetworkContainersConfiguration.err @@ -629,13 +629,8 @@ func TestGetMultiTenancyCNIResultUnsupportedAPI(t *testing.T) { tt.args.k8sPodName, tt.args.k8sNamespace, tt.args.ifName) - if (err != nil) != tt.wantErr { - // expect an error that GetAllNetworkContainers() fails to get all network containers - t.Errorf("GetAllNetworkContainers() error = %v, wantErr %v", err, tt.wantErr) - return - } - if tt.wantErr { - require.Error(err) + if err != nil && tt.wantErr { + t.Fatalf("expected an error %+v but none received", err) } require.NoError(err) require.Exactly(tt.want, got[0].ncResponse) @@ -779,13 +774,12 @@ func TestGetMultiTenancyCNIResultNotFound(t *testing.T) { tt.args.k8sPodName, tt.args.k8sNamespace, tt.args.ifName) - if (err != nil) != tt.wantErr { - t.Errorf("GetAllNetworkContainers() error = %v, wantErr %v", err, tt.wantErr) - return + if err != nil && !tt.wantErr { + t.Fatalf("expected an error %+v but none received", err) } - if errors.Is(err, errNoOrchestratorContextFound) { - require.Error(err) + if !errors.Is(err, errNoOrchestratorContextFound) { + t.Fatalf("expected an error %s but %v received", errNoOrchestratorContextFound, err) } }) } From 58d93c3f2c7e07618d8bad130b3b3e20fde9b157 Mon Sep 17 00:00:00 2001 From: paulyu Date: Thu, 6 Apr 2023 13:51:23 -0400 Subject: [PATCH 11/12] fix comments --- cni/network/multitenancy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cni/network/multitenancy_test.go b/cni/network/multitenancy_test.go index e266c7d59c..20c3b8dd67 100644 --- a/cni/network/multitenancy_test.go +++ b/cni/network/multitenancy_test.go @@ -774,7 +774,7 @@ func TestGetMultiTenancyCNIResultNotFound(t *testing.T) { tt.args.k8sPodName, tt.args.k8sNamespace, tt.args.ifName) - if err != nil && !tt.wantErr { + if err == nil && tt.wantErr { t.Fatalf("expected an error %+v but none received", err) } From 8b262ac5c66af61e83895783f6ad535cf0640f07 Mon Sep 17 00:00:00 2001 From: paulyu Date: Thu, 6 Apr 2023 14:05:38 -0400 Subject: [PATCH 12/12] fix linter issue --- cni/network/multitenancy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cni/network/multitenancy_test.go b/cni/network/multitenancy_test.go index 20c3b8dd67..6e65f5994d 100644 --- a/cni/network/multitenancy_test.go +++ b/cni/network/multitenancy_test.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "errors" - "github.com/google/go-cmp/cmp" "net" "testing" @@ -15,6 +14,7 @@ import ( "github.com/Azure/azure-container-networking/network" cniTypes "github.com/containernetworking/cni/pkg/types" cniTypesCurr "github.com/containernetworking/cni/pkg/types/100" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/require" )