fix: validate that NCIDs are well-formed GUIDs

cns/NetworkContainerContract.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/Azure/azure-container-networking/cns/types"
 	"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
+	"github.com/google/uuid"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 )
@@ -90,6 +91,8 @@ const (
 	MultiTenantCRD = "MultiTenantCRD"
 )
 
+var ErrInvalidNCID = errors.New("invalid NetworkContainerID")
+
 // CreateNetworkContainerRequest specifies request to create a network container or network isolation boundary.
 type CreateNetworkContainerRequest struct {
 	HostPrimaryIP              string
@@ -112,6 +115,16 @@ type CreateNetworkContainerRequest struct {
 	NetworkInterfaceInfo       NetworkInterfaceInfo //nolint // introducing new field for backendnic, to be used later by cni code
 }
 
+func (req *CreateNetworkContainerRequest) Validate() error {
+	if req.NetworkContainerid == "" {
+		return errors.Wrap(ErrInvalidNCID, "NetworkContainerID is empty")
+	}
+	if _, err := uuid.Parse(strings.TrimPrefix(req.NetworkContainerid, SwiftPrefix)); err != nil {
+		return errors.Wrapf(ErrInvalidNCID, "NetworkContainerID %s is not a valid UUID: %s", req.NetworkContainerid, err.Error())
+	}
+	return nil
+}
+
 // CreateNetworkContainerRequest implements fmt.Stringer for logging
 func (req *CreateNetworkContainerRequest) String() string {
 	return fmt.Sprintf("CreateNetworkContainerRequest"+
@@ -404,6 +417,15 @@ type PostNetworkContainersRequest struct {
 	CreateNetworkContainerRequests []CreateNetworkContainerRequest
 }
 
+func (req *PostNetworkContainersRequest) Validate() error {
+	for i := range req.CreateNetworkContainerRequests {
+		if err := req.CreateNetworkContainerRequests[i].Validate(); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // PostNetworkContainersResponse specifies response of creating all NCs that are sent from DNC.
 type PostNetworkContainersResponse struct {
 	Response Response

cns/NetworkContainerContract_test.go

@@ -105,3 +105,98 @@ func TestNewPodInfoFromIPConfigsRequest(t *testing.T) {
 		})
 	}
 }
+
+func TestCreateNetworkContainerRequestValidate(t *testing.T) {
+	tests := []struct {
+		name    string
+		req     CreateNetworkContainerRequest
+		wantErr bool
+	}{
+		{
+			name: "valid",
+			req: CreateNetworkContainerRequest{
+				NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid",
+			req: CreateNetworkContainerRequest{
+				NetworkContainerid: SwiftPrefix + "f47ac10b-58cc-0372-8567-0e02b2c3d479",
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalid",
+			req: CreateNetworkContainerRequest{
+				NetworkContainerid: "-f47ac10b-58cc-0372-8567-0e02b2c3d479",
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := tt.req.Validate(); (err != nil) != tt.wantErr {
+				t.Errorf("CreateNetworkContainerRequest.Validate() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}
+
+func TestPostNetworkContainersRequest_Validate(t *testing.T) {
+	tests := []struct {
+		name    string
+		req     PostNetworkContainersRequest
+		wantErr bool
+	}{
+		{
+			name: "valid",
+			req: PostNetworkContainersRequest{
+				CreateNetworkContainerRequests: []CreateNetworkContainerRequest{
+					{
+						NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
+					},
+					{
+						NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d478",
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid",
+			req: PostNetworkContainersRequest{
+				CreateNetworkContainerRequests: []CreateNetworkContainerRequest{
+					{
+						NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
+					},
+					{
+						NetworkContainerid: SwiftPrefix + "f47ac10b-58cc-0372-8567-0e02b2c3d478",
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "invalid",
+			req: PostNetworkContainersRequest{
+				CreateNetworkContainerRequests: []CreateNetworkContainerRequest{
+					{
+						NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
+					},
+					{
+						NetworkContainerid: "-f47ac10b-58cc-0372-8567-0e02b2c3d478",
+					},
+				},
+			},
+			wantErr: true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := tt.req.Validate(); (err != nil) != tt.wantErr {
+				t.Errorf("PostNetworkContainersRequest.Validate() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}

cns/networkcontainers/networkcontainers.go

@@ -91,11 +91,7 @@ func (cn *NetworkContainers) Delete(networkContainerID string) error {
 }
 
 // CreateLoopbackAdapter creates a loopback adapter with the specified settings
-func CreateLoopbackAdapter(
-	adapterName string,
-	ipConfig cns.IPConfiguration,
-	setWeakHostOnInterface bool,
-	primaryInterfaceIdentifier string) error {
+func CreateLoopbackAdapter(adapterName string, ipConfig cns.IPConfiguration, setWeakHostOnInterface bool, primaryInterfaceIdentifier string) error {
 	return createOrUpdateWithOperation(
 		adapterName,
 		ipConfig,

cns/networkcontainers/networkcontainers_linux.go

@@ -89,11 +89,6 @@ func configureNetworkContainerNetworking(operation, podName, podNamespace, docke
 	return fmt.Errorf("[Azure CNS] Operation is not supported in linux.")
 }
 
-func createOrUpdateWithOperation(
-	adapterName string,
-	ipConfig cns.IPConfiguration,
-	setWeakHost bool,
-	primaryInterfaceIdentifier string,
-	operation string) error {
+func createOrUpdateWithOperation(string, cns.IPConfiguration, bool, string, string) error {
 	return nil
 }

cns/networkcontainers/networkcontainers_windows.go

@@ -126,12 +126,7 @@ func setWeakHostOnInterface(ipAddress, ncID string) error {
 	return nil
 }
 
-func createOrUpdateWithOperation(
-	adapterName string,
-	ipConfig cns.IPConfiguration,
-	setWeakHost bool,
-	primaryInterfaceIdentifier string,
-	operation string) error {
+func createOrUpdateWithOperation(adapterName string, ipConfig cns.IPConfiguration, setWeakHost bool, primaryInterfaceIdentifier, operation string) error {
 	acnBinaryPath, err := getAzureNetworkContainerBinaryPath()
 	if err != nil {
 		return err

cns/restserver/api.go

@@ -786,14 +786,20 @@ func (service *HTTPRestService) createOrUpdateNetworkContainer(w http.ResponseWr
 	logger.Printf("[Azure CNS] createOrUpdateNetworkContainer")
 
 	var req cns.CreateNetworkContainerRequest
-	err := service.Listener.Decode(w, r, &req)
-	logger.Request(service.Name, req.String(), err)
-	if err != nil {
+	if err := service.Listener.Decode(w, r, &req); err != nil {
+		w.WriteHeader(http.StatusBadRequest)
+		return
+	}
+	if err := req.Validate(); err != nil {
+		logger.Errorf("[Azure CNS] invalid request %+v: %s", req, err)
+		w.WriteHeader(http.StatusBadRequest)
 		return
 	}
 
+	logger.Request(service.Name, req.String(), nil)
 	var returnCode types.ResponseCode
 	var returnMessage string
+	var err error
 	switch r.Method {
 	case http.MethodPost:
 		if req.NetworkContainerType == cns.WebApps {

cns/restserver/api_test.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"encoding/json"
 	"encoding/xml"
-	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -28,6 +27,7 @@ import (
 	"github.com/Azure/azure-container-networking/nmagent"
 	"github.com/Azure/azure-container-networking/processlock"
 	"github.com/Azure/azure-container-networking/store"
+	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -86,15 +86,15 @@ var (
 	}
 
 	nc1 = createOrUpdateNetworkContainerParams{
-		ncID:         "ethWebApp1",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d479",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
 		podName:      "testpod",
 		podNamespace: "testpodnamespace",
 	}
 	nc2 = createOrUpdateNetworkContainerParams{
-		ncID:         "ethWebApp2",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d478",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
@@ -105,15 +105,15 @@ var (
 	errMismatchedNCs = errors.New("GetNetworkContainers failed because NCs not matched")
 
 	nc3 = createOrUpdateNetworkContainerParams{
-		ncID:         "1abc",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d477",
 		ncIP:         "10.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
 		podName:      "testpod",
 		podNamespace: "testpodnamespace",
 	}
 	nc4 = createOrUpdateNetworkContainerParams{
-		ncID:         "2abc",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d476",
 		ncIP:         "20.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
@@ -421,7 +421,7 @@ func TestDeleteNetworkContainers(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	if ncResponse.NetworkContainerID != "Swift_2abc" {
+	if strings.TrimPrefix(ncResponse.NetworkContainerID, cns.SwiftPrefix) != nc4.ncID {
 		t.Fatal("failed to check second nc")
 	}
 
@@ -448,7 +448,7 @@ func TestCreateNetworkContainer(t *testing.T) {
 	fmt.Println("TestCreateNetworkContainer: JobObject")
 
 	params := createOrUpdateNetworkContainerParams{
-		ncID:         "testJobObject",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d476",
 		ncIP:         "10.1.0.5",
 		ncType:       "JobObject",
 		ncVersion:    "0",
@@ -473,7 +473,7 @@ func TestCreateNetworkContainer(t *testing.T) {
 	// Test create network container of type WebApps
 	fmt.Println("TestCreateNetworkContainer: WebApps")
 	params = createOrUpdateNetworkContainerParams{
-		ncID:         "ethWebApp",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d475",
 		ncIP:         "192.0.0.5",
 		ncType:       "WebApps",
 		ncVersion:    "0",
@@ -488,7 +488,7 @@ func TestCreateNetworkContainer(t *testing.T) {
 	}
 
 	params = createOrUpdateNetworkContainerParams{
-		ncID:         "ethWebApp",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d475",
 		ncIP:         "192.0.0.6",
 		ncType:       "WebApps",
 		ncVersion:    "0",
@@ -512,7 +512,7 @@ func TestCreateNetworkContainer(t *testing.T) {
 
 	// Test create network container of type COW
 	params = createOrUpdateNetworkContainerParams{
-		ncID:         "testCOWContainer",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d474",
 		ncIP:         "10.0.0.5",
 		ncType:       "COW",
 		ncVersion:    "0",
@@ -543,7 +543,7 @@ func TestGetNetworkContainerByOrchestratorContext(t *testing.T) {
 	setOrchestratorType(t, cns.Kubernetes)
 
 	params := createOrUpdateNetworkContainerParams{
-		ncID:         "ethWebApp",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d471",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
@@ -605,7 +605,7 @@ func TestGetInterfaceForNetworkContainer(t *testing.T) {
 	setOrchestratorType(t, cns.Kubernetes)
 
 	params := createOrUpdateNetworkContainerParams{
-		ncID:         "ethWebApp",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d479",
 		ncIP:         "11.0.0.5",
 		ncType:       "WebApps",
 		ncVersion:    "0",
@@ -673,7 +673,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
 	defer cleanupWSP()
 
 	params := createOrUpdateNetworkContainerParams{
-		ncID:         "nc-nma-success",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d475",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
@@ -721,7 +721,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
 	// Testing the path where the NC version with CNS is higher than the one with NMAgent.
 	// This indicates that the NMAgent is yet to program the NC version.
 	params = createOrUpdateNetworkContainerParams{
-		ncID:         "nc-nma-fail-version-mismatch",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d474",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "1",
@@ -760,7 +760,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
 
 	// Testing the path where NMAgent response status code is not 200.
 	params = createOrUpdateNetworkContainerParams{
-		ncID:         "nc-nma-fail-500",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d473",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
@@ -802,7 +802,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
 
 	// Testing the path where NMAgent response status code is 200 but embedded response is 401
 	params = createOrUpdateNetworkContainerParams{
-		ncID:         "nc-nma-fail-unavailable",
+		ncID:         "f47ac10b-58cc-0372-8567-0e02b2c3d472",
 		ncIP:         "11.0.0.5",
 		ncType:       cns.AzureContainerInstance,
 		ncVersion:    "0",
@@ -1362,7 +1362,7 @@ func getAllNetworkContainers(t *testing.T, ncParams []createOrUpdateNetworkConta
 	var resp cns.GetAllNetworkContainersResponse
 	err = decodeResponse(w, &resp)
 	if err != nil || resp.Response.ReturnCode != types.Success || len(resp.NetworkContainers) != len(ncParams) {
-		return cns.GetAllNetworkContainersResponse{}, fmt.Errorf("GetNetworkContainers failed with response %+v Err: %w", resp, err)
+		return cns.GetAllNetworkContainersResponse{}, errors.Wrapf(err, "GetNetworkContainers failed with response %+v", resp)
 	}
 
 	// If any NC in response is not found in ncParams, it means get all NCs failed
@@ -1446,7 +1446,7 @@ func postAllNetworkContainers(t *testing.T, ncParams []createOrUpdateNetworkCont
 	err = decodeResponse(w, &resp)
 
 	if err != nil || resp.Response.ReturnCode != types.Success {
-		return fmt.Errorf("post Network Containers failed with response %+v Err:  %w", resp, err)
+		return fmt.Errorf("post Network Containers failed with response %+v: %w", resp, err)
 	}
 	t.Logf("Post Network Containers succeeded with response %+v\n", resp)