Fix incorrect error "UnsupportedAPI" returned by CNI #1889

rjdenney · 2023-04-04T15:23:14Z

Reason for Change:

This PR is to fix DualNIC log issue:
(1) If new CNS API(GetAllNetworkContainers) is failed to get ncConfigs, then it will try old CNS API(GetNetworkContainer)
(2) If old CNS API is also failed to get called, then it should return corresponding error instead of "Unsupported API" error

This PR addes three UT test cases:

TestGetMultiTenancyCNIResultUnsupportedAPI() => This will test if new CNS API is not supported and old CNS API can handle to get ncConfig with "Unsupported API" error
TestGetMultiTenancyCNIResultNotFound() => 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

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests

Notes:

cni/network/multitenancy_test.go

ashvindeodhar · 2023-04-04T16:47:50Z

cni/network/multitenancy_test.go

+			e.Code = types.UnsupportedAPI
+			e.Err = errors.New("Unsupported API")
+			return nil, e
+		} else {


remove else. You are already returning in if

cni/network/multitenancy_test.go

ashvindeodhar · 2023-04-04T16:50:12Z

cni/network/multitenancy_test.go

+	}
+}
+
+func TestGetMultiTenancyCNIResultNotUnsupportedAPIError(t *testing.T) {


// TestGetMultiTenancyCNIResultNotFound tests the scenario where CNS doesn't have the NC goal state for the provided orchestrator context
TestGetMultiTenancyCNIResultNotFound(

tamilmani1989 · 2023-04-04T17:35:12Z

@paulyufan Please add description to your PR describing what's the issue and what you are fixing. That is must before we do any review

rjdenney · 2023-04-04T21:25:18Z

@tamilmani1989 Description has been updated.

…working into 17777760

tamilmani1989 · 2023-04-05T17:00:29Z

cni/network/multitenancy_test.go

+				tt.args.k8sPodName,
+				tt.args.k8sNamespace,
+				tt.args.ifName)
+			if (err != nil) != tt.wantErr {


the test case missing validating the actual error. we should validate if GetAllNetworkcontainers returning expected error message

ashvindeodhar · 2023-04-05T22:40:38Z

cni/network/multitenancy_test.go

+)
+
 type MockCNSClient struct {
+	unsupportedAPIs                      map[cnsAPIName]bool


why do we need bool? just empty struct would do, no?

all we need to check if the key exists. We don;t really need a true / false value in the map

cni/network/multitenancy_test.go

ashvindeodhar · 2023-04-05T22:43:49Z

cni/network/multitenancy_test.go

+
+	// set new CNS API is not supported
+	unsupportedAPIs := make(map[cnsAPIName]bool)
+	unsupportedAPIs["GetAllNetworkContainers"] = true


just need to add empty struct struct{}{}

cni/network/multitenancy_test.go

ashvindeodhar · 2023-04-05T23:31:28Z

cni/network/multitenancy_test.go

+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := tt.args.plugin.multitenancyClient.GetAllNetworkContainers(


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

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.

ashvindeodhar · 2023-04-05T23:32:04Z

cni/network/multitenancy_test.go

+				return
+			}
+
+			if errors.Is(err, errNoOrchestratorContextFound) {


what if the err is not errNoOrchestratorContextFound, it will still pass the test, right? Which is what is happening for the first comment I made

timraymond

I have some strongly suggested suggestions.

timraymond · 2023-04-06T00:55:53Z

cni/network/multitenancy_test.go

+	if _, isUnsupported := c.unsupportedAPIs[GetAllNetworkContainers]; isUnsupported {
+		e := &client.CNSClientError{}
+		e.Code = types.UnsupportedAPI
+		e.Err = errUnsupportedAPI
+		return nil, e
+	}
+
+	if !reflect.DeepEqual(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext) {
+		return nil, errNoOrchestratorContextFound
+	}


This stub cares too much about the data that it's handed. In the test we want to guarantee specific behavior from this stub so that specific reactive behavior can be tested. Behavior is the key, not the data. The problem with setting data and attempting to provide matching data to produce an expected stubbed behavior is that we basically end up with more untested code with bugs all of its own. It becomes difficult to modify the stub because there are other tests that depend on it.

Instead the tests should provide the implementation:

type MockCNSClient struct { GetAllNetworkContainersF(context.Context, []byte) ([]cns.GetNetworkContainersResponse, error) } func (m *MockCNSClient) GetAllNetworkContainers(ctx context.Context, ochestratorContext []byte) ([]cns.GetNetworkContainersResponse, error) { return m.GetAllNetworkContainersF(ctx, orchestratorContext) }

That way when you create the mock client, you provide the exact implementation you expect the stubbed method call to do:

func TestSomething(t *testing.T) { cnsClient := &MockCNSClient{ GetNetworkContainersF: func(_ context.Context, _ []byte) ([]cns.GetNetworkContainersResponse, error) { return nil, errNoOrchestratorContextFound }, } }

Your tests become simpler because you don't have another source of complexity from the logic in the stub. I only mention this because there's enough change to the function of MockCNSClient to make this a worthwhile change.

thanks Tim. @paulyufan2 I agree with Tim, this is a better approach.

timraymond · 2023-04-06T01:02:30Z

cni/network/multitenancy_test.go

+				tt.args.k8sPodName,
+				tt.args.k8sNamespace,
+				tt.args.ifName)
+			if (err != nil) != tt.wantErr {


This is a bit of a puzzling construction for future readers. Clear is better than clever. Prefer this:

if err != nil && !tt.WantErr { t.Fatal("unexpected error: err:", err) } if err == nil && tt.WantErr { t.Fatal("expected an error but none received") }

The first can be read literally: "if the error is not present and I expected one..." the second "if no error was present but I did expect one...".

Fixed with this style

timraymond · 2023-04-06T01:05:38Z

cni/network/multitenancy_test.go

+			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


Use t.Fatal instead of t.Error if you want to stop the test from proceeding. It has the same effect through some special runtime tricks. return is always a smell in tests.

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

timraymond · 2023-04-06T01:06:17Z

cni/network/multitenancy_test.go

+			if tt.wantErr {
+				require.Error(err)
+			}
+			require.NoError(err)


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

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

timraymond · 2023-04-06T01:10:35Z

cni/network/multitenancy_test.go

+		return nil, e
+	}
+
+	if !reflect.DeepEqual(c.getAllNetworkContainersConfiguration.orchestratorContext, orchestratorContext) {


reflect.DeepEqual is rarely the correct func in a *_test.go file these days. The trouble is that it compares everything--unexported fields included. This is fine if you don't have any unexported fields, but that may only be true today, at the time you're committing the code. It makes the tests more brittle because they make it more difficult for compared structs to have internals that are invisible to outside observers. Use github.com/google/go-cmp/cmp and the cmp.Equal func instead (which is a drop-in replacement). The upside is that you can provide diffs when they aren't the same using cmp.Diff. That makes it much easier to hunt down broken things.

fixed, use cmp.Equal

…working into 17777760

paulyufan added 2 commits April 1, 2023 10:00

fix dualNIC log error and add an UT

d9177c9

add an UT

5426afb

ashvindeodhar requested changes Apr 4, 2023

View reviewed changes

tamilmani1989 added cni Related to CNI. fix Fixes something. labels Apr 4, 2023

fix UT

cccc81f

tamilmani1989 changed the title ~~Fix dualNIC log issue and add UTs~~ Fix incorrect error "UnsupportedAPI" returned by CNI Apr 4, 2023

paulyufan added 4 commits April 4, 2023 19:10

fix UT

cfc862f

fix UT

3384d24

Merge branch 'master' of https://github.com/Azure/azure-container-net…

4753c6c

…working into 17777760

fix UTs

8f8ec2c

tamilmani1989 reviewed Apr 5, 2023

View reviewed changes

paulyufan added 2 commits April 5, 2023 13:09

fix comments

786fdea

fix linter issue

c08af0e

ashvindeodhar requested changes Apr 5, 2023

View reviewed changes

fix Ashvin's comments

ae34861

ashvindeodhar requested changes Apr 5, 2023

View reviewed changes

timraymond reviewed Apr 6, 2023

View reviewed changes

paulyufan added 4 commits April 6, 2023 12:43

fix comments

f49eaa7

fix comments

58d93c3

fix linter issue

8b262ac

Merge branch 'master' of https://github.com/Azure/azure-container-net…

3fe14f0

…working into 17777760

ashvindeodhar approved these changes Apr 14, 2023

View reviewed changes

paulyufan added 2 commits April 14, 2023 13:23

Merge branch 'master' of https://github.com/Azure/azure-container-net…

d91073c

…working into 17777760

Merge branch 'master' of https://github.com/Azure/azure-container-net…

8c55e65

…working into 17777760

vipul-21 merged commit a024d7d into Azure:master Apr 15, 2023

Fix incorrect error "UnsupportedAPI" returned by CNI #1889

Fix incorrect error "UnsupportedAPI" returned by CNI #1889

Uh oh!

Conversation

rjdenney commented Apr 4, 2023 • edited by tamilmani1989 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ashvindeodhar Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tamilmani1989 commented Apr 4, 2023

Uh oh!

rjdenney commented Apr 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timraymond left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rjdenney commented Apr 4, 2023 •

edited by tamilmani1989

Loading

ashvindeodhar Apr 4, 2023 •

edited

Loading