Skip to content

Commit

Permalink
fix: validate that NCIDs are well-formed GUIDs (#2359)
Browse files Browse the repository at this point in the history 
* fix: validate that NCIDs are well-formed GUIDs

Signed-off-by: Evan Baker <[email protected]>

* fix tests

Signed-off-by: Evan Baker <[email protected]>

* add logs and test

Signed-off-by: Evan Baker <[email protected]>

---------

Signed-off-by: Evan Baker <[email protected]>
  • Loading branch information
@rbtr @matmerr
rbtr authored and matmerr committed Jan 16, 2024
1 parent e4900cb commit b1f90ba
Show file tree
Hide file tree
Showing 8 changed files with 152 additions and 43 deletions.
22 changes: 22 additions & 0 deletions cns/NetworkContainerContract.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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
Expand All @@ -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"+
Expand Down Expand Up @@ -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
Expand Down
95 changes: 95 additions & 0 deletions cns/NetworkContainerContract_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
}
}
6 changes: 1 addition & 5 deletions cns/networkcontainers/networkcontainers.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
7 changes: 1 addition & 6 deletions cns/networkcontainers/networkcontainers_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
7 changes: 1 addition & 6 deletions cns/networkcontainers/networkcontainers_windows.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
12 changes: 9 additions & 3 deletions cns/restserver/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
36 changes: 18 additions & 18 deletions cns/restserver/api_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"context"
"encoding/json"
"encoding/xml"
"errors"
"fmt"
"io"
"net/http"
Expand All @@ -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"
)

Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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")
}

Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down
Loading

0 comments on commit b1f90ba

Please sign in to comment.