From 588de11a3608a711c550f393f48bdd2715842449 Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Thu, 10 Feb 2022 14:50:12 -0800 Subject: [PATCH 1/5] Allow cns to register node if no NCs are present --- cns/restserver/api.go | 2 +- cns/restserver/util.go | 14 ++++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/cns/restserver/api.go b/cns/restserver/api.go index b44c98e36b..ddb042dd13 100644 --- a/cns/restserver/api.go +++ b/cns/restserver/api.go @@ -746,7 +746,7 @@ func (service *HTTPRestService) setOrchestratorType(w http.ResponseWriter, r *ht service.dncPartitionKey = req.DncPartitionKey nodeID = service.state.NodeID - if nodeID == "" || nodeID == req.NodeID { + if nodeID == "" || nodeID == req.NodeID || !service.areNCsPresent() { switch req.OrchestratorType { case cns.ServiceFabric: fallthrough diff --git a/cns/restserver/util.go b/cns/restserver/util.go index f64999fcfe..987e925140 100644 --- a/cns/restserver/util.go +++ b/cns/restserver/util.go @@ -600,6 +600,20 @@ func (service *HTTPRestService) getNetworkContainerDetails(networkContainerID st return containerDetails, containerExists } +// areNCsPresent returns true if NCs are present in CNS, false if no NCs are present +func (service *HTTPRestService) areNCsPresent() bool { + service.RLock() + defer service.RUnlock() + + if len(service.state.ContainerStatus) == 0 { + if len(service.state.ContainerIDByOrchestratorContext) == 0 { + return false + } + } + + return true +} + // Check if the network is joined func (service *HTTPRestService) isNetworkJoined(networkID string) bool { namedLock.LockAcquire(stateJoinedNetworks) From e69a08e0552188cd5d3bc6ea2936ccf0aa03a777 Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Wed, 16 Feb 2022 10:01:05 -0800 Subject: [PATCH 2/5] Remove service lock --- cns/restserver/util.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/cns/restserver/util.go b/cns/restserver/util.go index 987e925140..34e95eef34 100644 --- a/cns/restserver/util.go +++ b/cns/restserver/util.go @@ -602,15 +602,11 @@ func (service *HTTPRestService) getNetworkContainerDetails(networkContainerID st // areNCsPresent returns true if NCs are present in CNS, false if no NCs are present func (service *HTTPRestService) areNCsPresent() bool { - service.RLock() - defer service.RUnlock() - if len(service.state.ContainerStatus) == 0 { if len(service.state.ContainerIDByOrchestratorContext) == 0 { return false } } - return true } From c2967df0e8ac72f96805b6e5d78465a2395fede0 Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Wed, 16 Feb 2022 13:51:09 -0800 Subject: [PATCH 3/5] unit tests --- cns/restserver/api_test.go | 91 +++++++++++++++++++++++++++++++++++++ cns/restserver/util_test.go | 52 +++++++++++++++++++++ 2 files changed, 143 insertions(+) create mode 100644 cns/restserver/util_test.go diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index 844cd0d115..6ae2c9782e 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -26,6 +26,7 @@ import ( acncommon "github.com/Azure/azure-container-networking/common" "github.com/Azure/azure-container-networking/processlock" "github.com/Azure/azure-container-networking/store" + "github.com/stretchr/testify/assert" ) const ( @@ -195,6 +196,96 @@ func TestSetOrchestratorType(t *testing.T) { } } +func FirstByte(b []byte, err error) []byte { + if err != nil { + panic(err) + } + return b +} + +func FirstRequest(req *http.Request, err error) *http.Request { + if err != nil { + panic(err) + } + return req +} + +func TestSetOrchestratorType_NCsPresent(t *testing.T) { + tests := []struct { + name string + service *HTTPRestService + writer *httptest.ResponseRecorder + request *http.Request + response cns.Response + wanthttperror bool + }{ + { + name: "Node already set, and has NCs, so registration should fail", + service: &HTTPRestService{ + state: &httpRestServiceState{ + NodeID: "node1", + ContainerStatus: map[string]containerstatus{ + "nc1": {}, + }, + ContainerIDByOrchestratorContext: map[string]string{ + "nc1": "present", + }, + }, + }, + writer: httptest.NewRecorder(), + request: FirstRequest(http.NewRequest(http.MethodPost, cns.SetOrchestratorType, bytes.NewReader(FirstByte(json.Marshal(cns.SetOrchestratorTypeRequest{ + OrchestratorType: "Kubernetes", + DncPartitionKey: "partition1", + NodeID: "node2", + }))))), + response: cns.Response{ + ReturnCode: types.InvalidRequest, + Message: "Invalid request since this node has already been registered as node1", + }, + wanthttperror: false, + }, + { + name: "Node already set, with no NCs, so registration should succeed", + service: &HTTPRestService{ + state: &httpRestServiceState{ + NodeID: "node1", + }, + }, + writer: httptest.NewRecorder(), + request: FirstRequest(http.NewRequest(http.MethodPost, cns.SetOrchestratorType, bytes.NewReader(FirstByte(json.Marshal(cns.SetOrchestratorTypeRequest{ + OrchestratorType: "Kubernetes", + DncPartitionKey: "partition1", + NodeID: "node2", + }))))), + response: cns.Response{ + ReturnCode: types.Success, + Message: "", + }, + wanthttperror: false, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + var resp cns.Response + // Since this is global, we have to replace the state + oldstate := svc.state + svc.state = tt.service.state + mux.ServeHTTP(tt.writer, tt.request) + // Replace back old state + svc.state = oldstate + + err := decodeResponse(tt.writer, &resp) + if tt.wanthttperror { + assert.NotNil(t, err) + } else { + assert.NoError(t, err) + } + assert.Equal(t, tt.response, resp) + }) + } +} + func TestCreateNetworkContainer(t *testing.T) { // requires more than 30 seconds to run fmt.Println("Test: TestCreateNetworkContainer") diff --git a/cns/restserver/util_test.go b/cns/restserver/util_test.go new file mode 100644 index 0000000000..981b98b39b --- /dev/null +++ b/cns/restserver/util_test.go @@ -0,0 +1,52 @@ +package restserver + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestAreNCsPresent(t *testing.T) { + tests := []struct { + name string + service HTTPRestService + want bool + }{ + { + name: "container status present", + service: HTTPRestService{ + state: &httpRestServiceState{ + ContainerStatus: map[string]containerstatus{ + "nc1": {}, + }, + }, + }, + want: true, + }, + { + name: "containerIDByOrchestorContext present", + service: HTTPRestService{ + state: &httpRestServiceState{ + ContainerIDByOrchestratorContext: map[string]string{ + "nc1": "present", + }, + }, + }, + want: true, + }, + { + name: "neither containerStatus nor containerIDByOrchestratorContext present", + service: HTTPRestService{ + state: &httpRestServiceState{}, + }, + want: false, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + got := tt.service.areNCsPresent() + assert.Equal(t, got, tt.want) + }) + } +} From 7b8c45a746320258d8b268a3d77723beddf7e1cf Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Wed, 16 Feb 2022 17:58:09 -0800 Subject: [PATCH 4/5] Address feedback --- cns/restserver/api_test.go | 26 ++++++++++++++++---------- cns/restserver/util.go | 6 ++---- cns/restserver/util_test.go | 4 ++-- 3 files changed, 20 insertions(+), 16 deletions(-) diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index 6ae2c9782e..78c18eefd1 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -233,11 +233,14 @@ func TestSetOrchestratorType_NCsPresent(t *testing.T) { }, }, writer: httptest.NewRecorder(), - request: FirstRequest(http.NewRequest(http.MethodPost, cns.SetOrchestratorType, bytes.NewReader(FirstByte(json.Marshal(cns.SetOrchestratorTypeRequest{ - OrchestratorType: "Kubernetes", - DncPartitionKey: "partition1", - NodeID: "node2", - }))))), + request: FirstRequest(http.NewRequestWithContext( + context.TODO(), http.MethodPost, cns.SetOrchestratorType, bytes.NewReader( + FirstByte(json.Marshal( + cns.SetOrchestratorTypeRequest{ + OrchestratorType: "Kubernetes", + DncPartitionKey: "partition1", + NodeID: "node2", + }))))), response: cns.Response{ ReturnCode: types.InvalidRequest, Message: "Invalid request since this node has already been registered as node1", @@ -252,11 +255,14 @@ func TestSetOrchestratorType_NCsPresent(t *testing.T) { }, }, writer: httptest.NewRecorder(), - request: FirstRequest(http.NewRequest(http.MethodPost, cns.SetOrchestratorType, bytes.NewReader(FirstByte(json.Marshal(cns.SetOrchestratorTypeRequest{ - OrchestratorType: "Kubernetes", - DncPartitionKey: "partition1", - NodeID: "node2", - }))))), + request: FirstRequest(http.NewRequestWithContext( + context.TODO(), http.MethodPost, cns.SetOrchestratorType, bytes.NewReader( + FirstByte(json.Marshal( + cns.SetOrchestratorTypeRequest{ + OrchestratorType: "Kubernetes", + DncPartitionKey: "partition1", + NodeID: "node2", + }))))), response: cns.Response{ ReturnCode: types.Success, Message: "", diff --git a/cns/restserver/util.go b/cns/restserver/util.go index 34e95eef34..7711272f3b 100644 --- a/cns/restserver/util.go +++ b/cns/restserver/util.go @@ -602,10 +602,8 @@ func (service *HTTPRestService) getNetworkContainerDetails(networkContainerID st // areNCsPresent returns true if NCs are present in CNS, false if no NCs are present func (service *HTTPRestService) areNCsPresent() bool { - if len(service.state.ContainerStatus) == 0 { - if len(service.state.ContainerIDByOrchestratorContext) == 0 { - return false - } + if len(service.state.ContainerStatus) == 0 && len(service.state.ContainerIDByOrchestratorContext) == 0 { + return false } return true } diff --git a/cns/restserver/util_test.go b/cns/restserver/util_test.go index 981b98b39b..9415fe2c57 100644 --- a/cns/restserver/util_test.go +++ b/cns/restserver/util_test.go @@ -42,8 +42,8 @@ func TestAreNCsPresent(t *testing.T) { want: false, }, } - for _, tt := range tests { - tt := tt + for _, tt := range tests { //nolint:govet // this mutex copy is to keep a local reference to this variable in the test func closure, and is ok + tt := tt //nolint:govet // this mutex copy is to keep a local reference to this variable in the test func closure, and is ok t.Run(tt.name, func(t *testing.T) { got := tt.service.areNCsPresent() assert.Equal(t, got, tt.want) From 998d88d3f83f8045c9b970c0b57fb7c29edf9c50 Mon Sep 17 00:00:00 2001 From: Paul Johnston Date: Wed, 16 Feb 2022 18:00:23 -0800 Subject: [PATCH 5/5] Linter still mad --- cns/restserver/api_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go index 78c18eefd1..749b0b9e73 100644 --- a/cns/restserver/api_test.go +++ b/cns/restserver/api_test.go @@ -235,7 +235,7 @@ func TestSetOrchestratorType_NCsPresent(t *testing.T) { writer: httptest.NewRecorder(), request: FirstRequest(http.NewRequestWithContext( context.TODO(), http.MethodPost, cns.SetOrchestratorType, bytes.NewReader( - FirstByte(json.Marshal( + FirstByte(json.Marshal( //nolint:errchkjson //inline map, only using returned bytes cns.SetOrchestratorTypeRequest{ OrchestratorType: "Kubernetes", DncPartitionKey: "partition1", @@ -257,7 +257,7 @@ func TestSetOrchestratorType_NCsPresent(t *testing.T) { writer: httptest.NewRecorder(), request: FirstRequest(http.NewRequestWithContext( context.TODO(), http.MethodPost, cns.SetOrchestratorType, bytes.NewReader( - FirstByte(json.Marshal( + FirstByte(json.Marshal( //nolint:errchkjson //inline map, only using returned bytes cns.SetOrchestratorTypeRequest{ OrchestratorType: "Kubernetes", DncPartitionKey: "partition1",