Skip to content

Fix incorrect error "UnsupportedAPI" returned by CNI #1889

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Apr 15, 2023
Merged

Fix incorrect error "UnsupportedAPI" returned by CNI #1889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d9177c9
fix dualNIC log error and add an UT
paulyufan Apr 1, 2023
5426afb
add an UT
paulyufan Apr 1, 2023
cccc81f
fix UT
paulyufan Apr 4, 2023
cfc862f
fix UT
paulyufan Apr 4, 2023
3384d24
fix UT
paulyufan Apr 5, 2023
4753c6c
Merge branch 'master' of https://github.com/Azure/azure-container-net…
paulyufan Apr 5, 2023
8f8ec2c
fix UTs
paulyufan Apr 5, 2023
786fdea
fix comments
paulyufan Apr 5, 2023
c08af0e
fix linter issue
paulyufan Apr 5, 2023
ae34861
fix Ashvin's comments
paulyufan Apr 5, 2023
f49eaa7
fix comments
paulyufan Apr 6, 2023
58d93c3
fix comments
paulyufan Apr 6, 2023
8b262ac
fix linter issue
paulyufan Apr 6, 2023
3fe14f0
Merge branch 'master' of https://github.com/Azure/azure-container-net…
paulyufan Apr 13, 2023
d91073c
Merge branch 'master' of https://github.com/Azure/azure-container-net…
paulyufan Apr 14, 2023
8c55e65
Merge branch 'master' of https://github.com/Azure/azure-container-net…
paulyufan Apr 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cni/network/multitenancy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
317 changes: 314 additions & 3 deletions cni/network/multitenancy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,18 @@ package network
import (
"context"
"encoding/json"
"errors"
"net"
"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"
"github.com/google/go-cmp/cmp"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -42,7 +46,19 @@ type getAllNetworkContainersConfigurationHandler struct {
err error
}

type cnsAPIName string

const (
GetAllNetworkContainers cnsAPIName = "GetAllNetworkContainers"
)

var (
errUnsupportedAPI = errors.New("Unsupported API")
errNoOrchestratorContextFound = errors.New("No CNI OrchestratorContext Found")
)

type MockCNSClient struct {
unsupportedAPIs map[cnsAPIName]struct{}
require *require.Assertions
request requestIPAddressHandler
release releaseIPAddressHandler
Expand All @@ -61,12 +77,23 @@ 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 !cmp.Equal(c.getNetworkContainerConfiguration.orchestratorContext, orchestratorContext) {
return nil, errNoOrchestratorContextFound
}
return c.getNetworkContainerConfiguration.returnResponse, c.getNetworkContainerConfiguration.err
}

func (c *MockCNSClient) GetAllNetworkContainers(ctx context.Context, orchestratorContext []byte) ([]cns.GetNetworkContainerResponse, error) {
c.require.Exactly(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext)
if _, isUnsupported := c.unsupportedAPIs[GetAllNetworkContainers]; isUnsupported {
e := &client.CNSClientError{}
e.Code = types.UnsupportedAPI
e.Err = errUnsupportedAPI
return nil, e
}

if !cmp.Equal(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext) {
return nil, errNoOrchestratorContextFound
}
return c.getAllNetworkContainersConfiguration.returnResponse, c.getAllNetworkContainersConfiguration.err
}

Expand Down Expand Up @@ -456,7 +483,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 {
Expand All @@ -473,3 +500,287 @@ func TestGetMultiTenancyCNIResult(t *testing.T) {
})
}
}

// 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

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",
},
},
}

// set new CNS API is not supported
unsupportedAPIs := make(map[cnsAPIName]struct{})
unsupportedAPIs["GetAllNetworkContainers"] = struct{}{}

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 Unsupported API with old CNS 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{
unsupportedAPIs: unsupportedAPIs,
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.Fatalf("expected an error %+v but none received", err)
}
require.NoError(err)
Copy link
Member

@timraymond timraymond Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use either t.Fatal / t.Error or require in tests... seeing both can be confusing for future readers.

Copy link
Contributor

@paulyufan2 paulyufan2 Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, use t.Fatal() and t.Fatalf() instead

require.Exactly(tt.want, got[0].ncResponse)
})
}
}

// 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) {
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",
},
},
}

// set new CNS API is not supported
unsupportedAPIs := make(map[cnsAPIName]struct{})
unsupportedAPIs["GetAllNetworkContainers"] = struct{}{}

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, CNS does not 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"},
},
plugin: &NetPlugin{
ipamInvoker: NewMockIpamInvoker(false, false, false),
multitenancyClient: &Multitenancy{
netioshim: &mockNetIOShim{},
cnsclient: &MockCNSClient{
unsupportedAPIs: unsupportedAPIs,
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,
},
{
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"},
},
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,
},
}
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
_, err := tt.args.plugin.multitenancyClient.GetAllNetworkContainers(
Copy link
Member

@ashvindeodhar ashvindeodhar Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is wrong -
if you call this func with GetAllNetworkContainers func in unsupported list, it will return ApiNotSupported error.

Copy link
Contributor

@paulyufan2 paulyufan2 Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine. Logic is:
(1) Use new API GetAllNetworkContainers() => it goes to UnsupportedAPI
(2) Use old API GetNetworkContainer() => orchestratorContext is not same => errored.

tt.args.ctx,
tt.args.nwCfg,
tt.args.k8sPodName,
tt.args.k8sNamespace,
tt.args.ifName)
if err == nil && tt.wantErr {
t.Fatalf("expected an error %+v but none received", err)
}

if !errors.Is(err, errNoOrchestratorContextFound) {
t.Fatalf("expected an error %s but %v received", errNoOrchestratorContextFound, err)
}
})
}
}