Skip to content

Commit b3faa2f

Browse files
rbtrrbtr
authored
fix: validate that NCIDs are well-formed GUIDs (#2359)
* 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]>
1 parent 9e07bd3 commit b3faa2f

File tree

8 files changed

+152
-43
lines changed

8 files changed

+152
-43
lines changed

‎cns/NetworkContainerContract.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99

1010
"github.com/Azure/azure-container-networking/cns/types"
1111
"github.com/Azure/azure-container-networking/crd/nodenetworkconfig/api/v1alpha"
12+
"github.com/google/uuid"
1213
"github.com/pkg/errors"
1314
corev1 "k8s.io/api/core/v1"
1415
)
@@ -90,6 +91,8 @@ const (
9091
MultiTenantCRD = "MultiTenantCRD"
9192
)
9293

94+
var ErrInvalidNCID = errors.New("invalid NetworkContainerID")
95+
9396
// CreateNetworkContainerRequest specifies request to create a network container or network isolation boundary.
9497
type CreateNetworkContainerRequest struct {
9598
HostPrimaryIP string
@@ -112,6 +115,16 @@ type CreateNetworkContainerRequest struct {
112115
NetworkInterfaceInfo NetworkInterfaceInfo //nolint // introducing new field for backendnic, to be used later by cni code
113116
}
114117

118+
func (req *CreateNetworkContainerRequest) Validate() error {
119+
if req.NetworkContainerid == "" {
120+
return errors.Wrap(ErrInvalidNCID, "NetworkContainerID is empty")
121+
}
122+
if _, err := uuid.Parse(strings.TrimPrefix(req.NetworkContainerid, SwiftPrefix)); err != nil {
123+
return errors.Wrapf(ErrInvalidNCID, "NetworkContainerID %s is not a valid UUID: %s", req.NetworkContainerid, err.Error())
124+
}
125+
return nil
126+
}
127+
115128
// CreateNetworkContainerRequest implements fmt.Stringer for logging
116129
func (req *CreateNetworkContainerRequest) String() string {
117130
return fmt.Sprintf("CreateNetworkContainerRequest"+
@@ -404,6 +417,15 @@ type PostNetworkContainersRequest struct {
404417
CreateNetworkContainerRequests []CreateNetworkContainerRequest
405418
}
406419

420+
func (req *PostNetworkContainersRequest) Validate() error {
421+
for i := range req.CreateNetworkContainerRequests {
422+
if err := req.CreateNetworkContainerRequests[i].Validate(); err != nil {
423+
return err
424+
}
425+
}
426+
return nil
427+
}
428+
407429
// PostNetworkContainersResponse specifies response of creating all NCs that are sent from DNC.
408430
type PostNetworkContainersResponse struct {
409431
Response Response

‎cns/NetworkContainerContract_test.go‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,3 +105,98 @@ func TestNewPodInfoFromIPConfigsRequest(t *testing.T) {
105105
})
106106
}
107107
}
108+
109+
func TestCreateNetworkContainerRequestValidate(t *testing.T) {
110+
tests := []struct {
111+
name string
112+
req CreateNetworkContainerRequest
113+
wantErr bool
114+
}{
115+
{
116+
name: "valid",
117+
req: CreateNetworkContainerRequest{
118+
NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
119+
},
120+
wantErr: false,
121+
},
122+
{
123+
name: "valid",
124+
req: CreateNetworkContainerRequest{
125+
NetworkContainerid: SwiftPrefix + "f47ac10b-58cc-0372-8567-0e02b2c3d479",
126+
},
127+
wantErr: false,
128+
},
129+
{
130+
name: "invalid",
131+
req: CreateNetworkContainerRequest{
132+
NetworkContainerid: "-f47ac10b-58cc-0372-8567-0e02b2c3d479",
133+
},
134+
wantErr: true,
135+
},
136+
}
137+
for _, tt := range tests {
138+
t.Run(tt.name, func(t *testing.T) {
139+
if err := tt.req.Validate(); (err != nil) != tt.wantErr {
140+
t.Errorf("CreateNetworkContainerRequest.Validate() error = %v, wantErr %v", err, tt.wantErr)
141+
}
142+
})
143+
}
144+
}
145+
146+
func TestPostNetworkContainersRequest_Validate(t *testing.T) {
147+
tests := []struct {
148+
name string
149+
req PostNetworkContainersRequest
150+
wantErr bool
151+
}{
152+
{
153+
name: "valid",
154+
req: PostNetworkContainersRequest{
155+
CreateNetworkContainerRequests: []CreateNetworkContainerRequest{
156+
{
157+
NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
158+
},
159+
{
160+
NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d478",
161+
},
162+
},
163+
},
164+
wantErr: false,
165+
},
166+
{
167+
name: "valid",
168+
req: PostNetworkContainersRequest{
169+
CreateNetworkContainerRequests: []CreateNetworkContainerRequest{
170+
{
171+
NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
172+
},
173+
{
174+
NetworkContainerid: SwiftPrefix + "f47ac10b-58cc-0372-8567-0e02b2c3d478",
175+
},
176+
},
177+
},
178+
wantErr: false,
179+
},
180+
{
181+
name: "invalid",
182+
req: PostNetworkContainersRequest{
183+
CreateNetworkContainerRequests: []CreateNetworkContainerRequest{
184+
{
185+
NetworkContainerid: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
186+
},
187+
{
188+
NetworkContainerid: "-f47ac10b-58cc-0372-8567-0e02b2c3d478",
189+
},
190+
},
191+
},
192+
wantErr: true,
193+
},
194+
}
195+
for _, tt := range tests {
196+
t.Run(tt.name, func(t *testing.T) {
197+
if err := tt.req.Validate(); (err != nil) != tt.wantErr {
198+
t.Errorf("PostNetworkContainersRequest.Validate() error = %v, wantErr %v", err, tt.wantErr)
199+
}
200+
})
201+
}
202+
}

‎cns/networkcontainers/networkcontainers.go‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -91,11 +91,7 @@ func (cn *NetworkContainers) Delete(networkContainerID string) error {
9191
}
9292

9393
// CreateLoopbackAdapter creates a loopback adapter with the specified settings
94-
func CreateLoopbackAdapter(
95-
adapterName string,
96-
ipConfig cns.IPConfiguration,
97-
setWeakHostOnInterface bool,
98-
primaryInterfaceIdentifier string) error {
94+
func CreateLoopbackAdapter(adapterName string, ipConfig cns.IPConfiguration, setWeakHostOnInterface bool, primaryInterfaceIdentifier string) error {
9995
return createOrUpdateWithOperation(
10096
adapterName,
10197
ipConfig,

‎cns/networkcontainers/networkcontainers_linux.go‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -89,11 +89,6 @@ func configureNetworkContainerNetworking(operation, podName, podNamespace, docke
8989
return fmt.Errorf("[Azure CNS] Operation is not supported in linux.")
9090
}
9191

92-
func createOrUpdateWithOperation(
93-
adapterName string,
94-
ipConfig cns.IPConfiguration,
95-
setWeakHost bool,
96-
primaryInterfaceIdentifier string,
97-
operation string) error {
92+
func createOrUpdateWithOperation(string, cns.IPConfiguration, bool, string, string) error {
9893
return nil
9994
}

‎cns/networkcontainers/networkcontainers_windows.go‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -126,12 +126,7 @@ func setWeakHostOnInterface(ipAddress, ncID string) error {
126126
return nil
127127
}
128128

129-
func createOrUpdateWithOperation(
130-
adapterName string,
131-
ipConfig cns.IPConfiguration,
132-
setWeakHost bool,
133-
primaryInterfaceIdentifier string,
134-
operation string) error {
129+
func createOrUpdateWithOperation(adapterName string, ipConfig cns.IPConfiguration, setWeakHost bool, primaryInterfaceIdentifier, operation string) error {
135130
acnBinaryPath, err := getAzureNetworkContainerBinaryPath()
136131
if err != nil {
137132
return err

‎cns/restserver/api.go‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -786,14 +786,20 @@ func (service *HTTPRestService) createOrUpdateNetworkContainer(w http.ResponseWr
786786
logger.Printf("[Azure CNS] createOrUpdateNetworkContainer")
787787

788788
var req cns.CreateNetworkContainerRequest
789-
err := service.Listener.Decode(w, r, &req)
790-
logger.Request(service.Name, req.String(), err)
791-
if err != nil {
789+
if err := service.Listener.Decode(w, r, &req); err != nil {
790+
w.WriteHeader(http.StatusBadRequest)
791+
return
792+
}
793+
if err := req.Validate(); err != nil {
794+
logger.Errorf("[Azure CNS] invalid request %+v: %s", req, err)
795+
w.WriteHeader(http.StatusBadRequest)
792796
return
793797
}
794798

799+
logger.Request(service.Name, req.String(), nil)
795800
var returnCode types.ResponseCode
796801
var returnMessage string
802+
var err error
797803
switch r.Method {
798804
case http.MethodPost:
799805
if req.NetworkContainerType == cns.WebApps {

‎cns/restserver/api_test.go‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"context"
99
"encoding/json"
1010
"encoding/xml"
11-
"errors"
1211
"fmt"
1312
"io"
1413
"net/http"
@@ -28,6 +27,7 @@ import (
2827
"github.com/Azure/azure-container-networking/nmagent"
2928
"github.com/Azure/azure-container-networking/processlock"
3029
"github.com/Azure/azure-container-networking/store"
30+
"github.com/pkg/errors"
3131
"github.com/stretchr/testify/assert"
3232
)
3333

@@ -86,15 +86,15 @@ var (
8686
}
8787

8888
nc1 = createOrUpdateNetworkContainerParams{
89-
ncID: "ethWebApp1",
89+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
9090
ncIP: "11.0.0.5",
9191
ncType: cns.AzureContainerInstance,
9292
ncVersion: "0",
9393
podName: "testpod",
9494
podNamespace: "testpodnamespace",
9595
}
9696
nc2 = createOrUpdateNetworkContainerParams{
97-
ncID: "ethWebApp2",
97+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d478",
9898
ncIP: "11.0.0.5",
9999
ncType: cns.AzureContainerInstance,
100100
ncVersion: "0",
@@ -105,15 +105,15 @@ var (
105105
errMismatchedNCs = errors.New("GetNetworkContainers failed because NCs not matched")
106106

107107
nc3 = createOrUpdateNetworkContainerParams{
108-
ncID: "1abc",
108+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d477",
109109
ncIP: "10.0.0.5",
110110
ncType: cns.AzureContainerInstance,
111111
ncVersion: "0",
112112
podName: "testpod",
113113
podNamespace: "testpodnamespace",
114114
}
115115
nc4 = createOrUpdateNetworkContainerParams{
116-
ncID: "2abc",
116+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d476",
117117
ncIP: "20.0.0.5",
118118
ncType: cns.AzureContainerInstance,
119119
ncVersion: "0",
@@ -421,7 +421,7 @@ func TestDeleteNetworkContainers(t *testing.T) {
421421
t.Fatal(err)
422422
}
423423

424-
if ncResponse.NetworkContainerID != "Swift_2abc" {
424+
if strings.TrimPrefix(ncResponse.NetworkContainerID, cns.SwiftPrefix) != nc4.ncID {
425425
t.Fatal("failed to check second nc")
426426
}
427427

@@ -448,7 +448,7 @@ func TestCreateNetworkContainer(t *testing.T) {
448448
fmt.Println("TestCreateNetworkContainer: JobObject")
449449

450450
params := createOrUpdateNetworkContainerParams{
451-
ncID: "testJobObject",
451+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d476",
452452
ncIP: "10.1.0.5",
453453
ncType: "JobObject",
454454
ncVersion: "0",
@@ -473,7 +473,7 @@ func TestCreateNetworkContainer(t *testing.T) {
473473
// Test create network container of type WebApps
474474
fmt.Println("TestCreateNetworkContainer: WebApps")
475475
params = createOrUpdateNetworkContainerParams{
476-
ncID: "ethWebApp",
476+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d475",
477477
ncIP: "192.0.0.5",
478478
ncType: "WebApps",
479479
ncVersion: "0",
@@ -488,7 +488,7 @@ func TestCreateNetworkContainer(t *testing.T) {
488488
}
489489

490490
params = createOrUpdateNetworkContainerParams{
491-
ncID: "ethWebApp",
491+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d475",
492492
ncIP: "192.0.0.6",
493493
ncType: "WebApps",
494494
ncVersion: "0",
@@ -512,7 +512,7 @@ func TestCreateNetworkContainer(t *testing.T) {
512512

513513
// Test create network container of type COW
514514
params = createOrUpdateNetworkContainerParams{
515-
ncID: "testCOWContainer",
515+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d474",
516516
ncIP: "10.0.0.5",
517517
ncType: "COW",
518518
ncVersion: "0",
@@ -543,7 +543,7 @@ func TestGetNetworkContainerByOrchestratorContext(t *testing.T) {
543543
setOrchestratorType(t, cns.Kubernetes)
544544

545545
params := createOrUpdateNetworkContainerParams{
546-
ncID: "ethWebApp",
546+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d471",
547547
ncIP: "11.0.0.5",
548548
ncType: cns.AzureContainerInstance,
549549
ncVersion: "0",
@@ -605,7 +605,7 @@ func TestGetInterfaceForNetworkContainer(t *testing.T) {
605605
setOrchestratorType(t, cns.Kubernetes)
606606

607607
params := createOrUpdateNetworkContainerParams{
608-
ncID: "ethWebApp",
608+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
609609
ncIP: "11.0.0.5",
610610
ncType: "WebApps",
611611
ncVersion: "0",
@@ -673,7 +673,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
673673
defer cleanupWSP()
674674

675675
params := createOrUpdateNetworkContainerParams{
676-
ncID: "nc-nma-success",
676+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d475",
677677
ncIP: "11.0.0.5",
678678
ncType: cns.AzureContainerInstance,
679679
ncVersion: "0",
@@ -721,7 +721,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
721721
// Testing the path where the NC version with CNS is higher than the one with NMAgent.
722722
// This indicates that the NMAgent is yet to program the NC version.
723723
params = createOrUpdateNetworkContainerParams{
724-
ncID: "nc-nma-fail-version-mismatch",
724+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d474",
725725
ncIP: "11.0.0.5",
726726
ncType: cns.AzureContainerInstance,
727727
ncVersion: "1",
@@ -760,7 +760,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
760760

761761
// Testing the path where NMAgent response status code is not 200.
762762
params = createOrUpdateNetworkContainerParams{
763-
ncID: "nc-nma-fail-500",
763+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d473",
764764
ncIP: "11.0.0.5",
765765
ncType: cns.AzureContainerInstance,
766766
ncVersion: "0",
@@ -802,7 +802,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
802802

803803
// Testing the path where NMAgent response status code is 200 but embedded response is 401
804804
params = createOrUpdateNetworkContainerParams{
805-
ncID: "nc-nma-fail-unavailable",
805+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d472",
806806
ncIP: "11.0.0.5",
807807
ncType: cns.AzureContainerInstance,
808808
ncVersion: "0",
@@ -1362,7 +1362,7 @@ func getAllNetworkContainers(t *testing.T, ncParams []createOrUpdateNetworkConta
13621362
var resp cns.GetAllNetworkContainersResponse
13631363
err = decodeResponse(w, &resp)
13641364
if err != nil || resp.Response.ReturnCode != types.Success || len(resp.NetworkContainers) != len(ncParams) {
1365-
return cns.GetAllNetworkContainersResponse{}, fmt.Errorf("GetNetworkContainers failed with response %+v Err: %w", resp, err)
1365+
return cns.GetAllNetworkContainersResponse{}, errors.Wrapf(err, "GetNetworkContainers failed with response %+v", resp)
13661366
}
13671367

13681368
// 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
14461446
err = decodeResponse(w, &resp)
14471447

14481448
if err != nil || resp.Response.ReturnCode != types.Success {
1449-
return fmt.Errorf("post Network Containers failed with response %+v Err: %w", resp, err)
1449+
return fmt.Errorf("post Network Containers failed with response %+v: %w", resp, err)
14501450
}
14511451
t.Logf("Post Network Containers succeeded with response %+v\n", resp)
14521452

0 commit comments

Comments
 (0)