Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 cns/restserver/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

i know you didn't change this, but this fallthrough pattern is very unusual in go. if all of these switch tests should actually execute the same case, it's better to do:

case cns.ServiceFabric, cns.Kubernetes, cns.KubernetesCRD, [... other tests ...]:
    // the case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point

fallthrough
Expand Down
97 changes: 97 additions & 0 deletions cns/restserver/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -195,6 +196,102 @@ 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.NewRequestWithContext(
context.TODO(), http.MethodPost, cns.SetOrchestratorType, bytes.NewReader(
FirstByte(json.Marshal( //nolint:errchkjson //inline map, only using returned bytes
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.NewRequestWithContext(
context.TODO(), http.MethodPost, cns.SetOrchestratorType, bytes.NewReader(
FirstByte(json.Marshal( //nolint:errchkjson //inline map, only using returned bytes
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")
Expand Down
8 changes: 8 additions & 0 deletions cns/restserver/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,14 @@ 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 {
if len(service.state.ContainerStatus) == 0 && len(service.state.ContainerIDByOrchestratorContext) == 0 {
return false
}
return true
Comment on lines +605 to +608
Copy link
Collaborator

Choose a reason for hiding this comment

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

just a thought, this pattern is fundamentally

if (bool) { return false }
return true

which is really

return !bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I didn't think about that

}

// Check if the network is joined
func (service *HTTPRestService) isNetworkJoined(networkID string) bool {
namedLock.LockAcquire(stateJoinedNetworks)
Expand Down
52 changes: 52 additions & 0 deletions cns/restserver/util_test.go
Original file line number Diff line number Diff line change
@@ -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 { //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)
})
}
}