Add join VNET call for every AZR NC unpublish call with fixed UTs (#3016

) * add join vnet call for every AZR nc unpublish call * add join vnet call for every AZR nc unpublish call * modify comment with explanation * declare vnet variable * linter fix * address comments
Azure · Sep 16, 2024 · ed2b57a · ed2b57a
1 parent 64c6c11
commit ed2b57a
Show file tree

Hide file tree

Showing 2 changed files with 82 additions and 7 deletions.
diff --git a/cns/restserver/api.go b/cns/restserver/api.go
@@ -4,6 +4,7 @@
 package restserver
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"fmt"
@@ -20,6 +21,7 @@ import (
 	"github.com/Azure/azure-container-networking/cns/types"
 	"github.com/Azure/azure-container-networking/cns/wireserver"
 	"github.com/Azure/azure-container-networking/common"
+	"github.com/Azure/azure-container-networking/nmagent"
 	"github.com/pkg/errors"
 )
 
@@ -1029,7 +1031,28 @@ func (service *HTTPRestService) unpublishNetworkContainer(w http.ResponseWriter,
 
 	ctx := r.Context()
 
-	if !service.isNetworkJoined(req.NetworkID) {
+	var unpublishBody nmagent.DeleteContainerRequest
+	var azrNC bool
+	err = json.Unmarshal(req.DeleteNetworkContainerRequestBody, &unpublishBody)
+	if err != nil {
+		// If the body contains only `""\n`, it is non-AZR NC
+		// In this case, we should not return an error
+		// However, if the body is not `""\n`, it is invalid and therefore, we must return an error
+		// []byte{34, 34, 10} here represents []byte(`""`+"\n")
+		if !bytes.Equal(req.DeleteNetworkContainerRequestBody, []byte{34, 34, 10}) {
+			http.Error(w, fmt.Sprintf("could not unmarshal delete network container body: %v", err), http.StatusBadRequest)
+			return
+		}
+	} else {
+		// If unmarshalling was successful, it is an AZR NC
+		azrNC = true
+	}
+
+	/* For AZR scenarios, if NMAgent is restarted, it loses state and does not know what VNETs to subscribe to.
+	As it no longer has VNET state, delete nc calls would fail. We need to add join VNET call for all AZR
+	nc unpublish calls just like publish nc calls.
+	*/
+	if azrNC || !service.isNetworkJoined(req.NetworkID) {
 		joinResp, err := service.wsproxy.JoinNetwork(ctx, req.NetworkID) //nolint:govet // ok to shadow
 		if err != nil {
 			resp := cns.UnpublishNetworkContainerResponse{
@@ -1062,7 +1085,7 @@ func (service *HTTPRestService) unpublishNetworkContainer(w http.ResponseWriter,
 		}
 
 		service.setNetworkStateJoined(req.NetworkID)
-		logger.Printf("[Azure-CNS] joined vnet %s during nc %s unpublish. wireserver response: %v", req.NetworkID, req.NetworkContainerID, string(joinBytes))
+		logger.Printf("[Azure-CNS] joined vnet %s during nc %s unpublish. AZREnabled: %t, wireserver response: %v", req.NetworkID, req.NetworkContainerID, unpublishBody.AZREnabled, string(joinBytes))
 	}
 
 	publishResp, err := service.wsproxy.UnpublishNC(ctx, ncParams, req.DeleteNetworkContainerRequestBody)

diff --git a/cns/restserver/api_test.go b/cns/restserver/api_test.go
@@ -1057,7 +1057,7 @@ func TestUnpublishNCViaCNS(t *testing.T) {
 		"/machine/plugins/?comp=nmagent&type=NetworkManagement/interfaces/dummyIntf/networkContainers/dummyNCURL/authenticationToke/" +
 		"8636c99d-7861-401f-b0d3-7e5b7dc8183c" +
 		"/api-version/1/method/DELETE"
-	err = unpublishNCViaCNS("vnet1", "ethWebApp", deleteNetworkContainerURL)
+	err = unpublishNCViaCNS("vnet1", "ethWebApp", deleteNetworkContainerURL, []byte(`""`+"\n"))
 	if err == nil {
 		t.Fatal("Expected a bad request error due to delete network url being incorrect")
 	}
@@ -1068,20 +1068,72 @@ func TestUnpublishNCViaCNS(t *testing.T) {
 		"/machine/plugins/?comp=nmagent&NetworkManagement/interfaces/dummyIntf/networkContainers/dummyNCURL/authenticationToken/" +
 		"8636c99d-7861-401f-b0d3-7e5b7dc8183c8636c99d-7861-401f-b0d3-7e5b7dc8183c" +
 		"/api-version/1/method/DELETE"
-	err = unpublishNCViaCNS("vnet1", "ethWebApp", deleteNetworkContainerURL)
+	err = unpublishNCViaCNS("vnet1", "ethWebApp", deleteNetworkContainerURL, []byte(`""`+"\n"))
 	if err == nil {
 		t.Fatal("Expected a bad request error due to create network url having more characters than permitted in auth token")
 	}
 
 	// now actually perform the deletion:
 	deleteNetworkContainerURL = "http://" + nmagentEndpoint +
 		"/machine/plugins/?comp=nmagent&type=NetworkManagement/interfaces/dummyIntf/networkContainers/dummyNCURL/authenticationToken/dummyT/api-version/1/method/DELETE"
-	err = unpublishNCViaCNS("vnet1", "ethWebApp", deleteNetworkContainerURL)
+	err = unpublishNCViaCNS("vnet1", "ethWebApp", deleteNetworkContainerURL, []byte(`""`+"\n"))
 	if err != nil {
 		t.Fatal(err)
 	}
 }
 
+func TestUnpublishViaCNSRequestBody(t *testing.T) {
+	createNetworkContainerURL := "http://" + nmagentEndpoint +
+		"/machine/plugins/?comp=nmagent&type=NetworkManagement/interfaces/dummyIntf/networkContainers/dummyNCURL/authenticationToken/dummyT/api-version/1"
+	deleteNetworkContainerURL := "http://" + nmagentEndpoint +
+		"/machine/plugins/?comp=nmagent&type=NetworkManagement/interfaces/dummyIntf/networkContainers/dummyNCURL/authenticationToken/dummyT/api-version/1/method/DELETE"
+	vnet := "vnet1"
+	wsProxy := fakes.WireserverProxyFake{}
+	cleanup := setWireserverProxy(svc, &wsProxy)
+	defer cleanup()
+
+	tests := []struct {
+		name         string
+		ncID         string
+		body         []byte
+		requireError bool
+	}{
+		{
+			name:         "Delete NC with invalid body",
+			ncID:         "ncID1",
+			body:         []byte(`invalid` + "\n"),
+			requireError: true,
+		},
+		{
+			name:         "Delete NC with valid non-AZR body",
+			ncID:         "ncID2",
+			body:         []byte(`""` + "\n"),
+			requireError: false,
+		},
+		{
+			name:         "Delete NC with valid AZR body",
+			ncID:         "ncID3",
+			body:         []byte(`{"azID":1,"azrEnabled":true}`),
+			requireError: false,
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		t.Run(tt.name, func(t *testing.T) {
+			errPublish := publishNCViaCNS(vnet, tt.ncID, createNetworkContainerURL)
+			require.NoError(t, errPublish)
+			errUnpublish := unpublishNCViaCNS(vnet, tt.ncID, deleteNetworkContainerURL, tt.body)
+			if tt.requireError {
+				require.Error(t, errUnpublish)
+				require.Contains(t, errUnpublish.Error(), "error decoding json")
+			} else {
+				require.NoError(t, errUnpublish)
+			}
+		})
+	}
+}
+
 func TestUnpublishNCViaCNS401(t *testing.T) {
 	wsproxy := fakes.WireserverProxyFake{
 		UnpublishNCFunc: func(_ context.Context, _ cns.NetworkContainerParameters, i []byte) (*http.Response, error) {
@@ -1161,15 +1213,15 @@ func TestUnpublishNCViaCNS401(t *testing.T) {
 	}
 }
 
-func unpublishNCViaCNS(networkID, networkContainerID, deleteNetworkContainerURL string) error {
+func unpublishNCViaCNS(networkID, networkContainerID, deleteNetworkContainerURL string, bodyBytes []byte) error {
 	joinNetworkURL := "http://" + nmagentEndpoint + "/dummyVnetURL"
 
 	unpublishNCRequest := &cns.UnpublishNetworkContainerRequest{
 		NetworkID:                         networkID,
 		NetworkContainerID:                networkContainerID,
 		JoinNetworkURL:                    joinNetworkURL,
 		DeleteNetworkContainerURL:         deleteNetworkContainerURL,
-		DeleteNetworkContainerRequestBody: []byte("{}"),
+		DeleteNetworkContainerRequestBody: bodyBytes,
 	}
 
 	var body bytes.Buffer