Skip to content

Commit 441d94a

Browse files
rbtrrbtr
authored
fix: validate that NCIDs are well-formed GUIDs (#2359) (#2364)
* fix: validate that NCIDs are well-formed GUIDs * fix tests * add logs and test --------- Signed-off-by: Evan Baker <[email protected]>
1 parent b33b79b commit 441d94a

8 files changed

+148
-39
lines changed

‎cns/NetworkContainerContract.go

+22
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
)
@@ -75,6 +76,8 @@ const (
7576
MultiTenantCRD = "MultiTenantCRD"
7677
)
7778

79+
var ErrInvalidNCID = errors.New("invalid NetworkContainerID")
80+
7881
// CreateNetworkContainerRequest specifies request to create a network container or network isolation boundary.
7982
type CreateNetworkContainerRequest struct {
8083
HostPrimaryIP string
@@ -96,6 +99,16 @@ type CreateNetworkContainerRequest struct {
9699
NCStatus v1alpha.NCStatus
97100
}
98101

102+
func (req *CreateNetworkContainerRequest) Validate() error {
103+
if req.NetworkContainerid == "" {
104+
return errors.Wrap(ErrInvalidNCID, "NetworkContainerID is empty")
105+
}
106+
if _, err := uuid.Parse(strings.TrimPrefix(req.NetworkContainerid, SwiftPrefix)); err != nil {
107+
return errors.Wrapf(ErrInvalidNCID, "NetworkContainerID %s is not a valid UUID: %s", req.NetworkContainerid, err.Error())
108+
}
109+
return nil
110+
}
111+
99112
// CreateNetworkContainerRequest implements fmt.Stringer for logging
100113
func (req *CreateNetworkContainerRequest) String() string {
101114
return fmt.Sprintf("CreateNetworkContainerRequest"+
@@ -374,6 +387,15 @@ type PostNetworkContainersRequest struct {
374387
CreateNetworkContainerRequests []CreateNetworkContainerRequest
375388
}
376389

390+
func (req *PostNetworkContainersRequest) Validate() error {
391+
for i := range req.CreateNetworkContainerRequests {
392+
if err := req.CreateNetworkContainerRequests[i].Validate(); err != nil {
393+
return err
394+
}
395+
}
396+
return nil
397+
}
398+
377399
// PostNetworkContainersResponse specifies response of creating all NCs that are sent from DNC.
378400
type PostNetworkContainersResponse struct {
379401
Response Response

‎cns/NetworkContainerContract_test.go

+95
Original file line numberDiff line numberDiff line change
@@ -105,3 +105,98 @@ func TestNewPodInfoFromIPConfigRequest(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

+1-5
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

+1-6
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

+1-6
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

+9-3
Original file line numberDiff line numberDiff line change
@@ -797,14 +797,20 @@ func (service *HTTPRestService) createOrUpdateNetworkContainer(w http.ResponseWr
797797
logger.Printf("[Azure CNS] createOrUpdateNetworkContainer")
798798

799799
var req cns.CreateNetworkContainerRequest
800-
err := service.Listener.Decode(w, r, &req)
801-
logger.Request(service.Name, req.String(), err)
802-
if err != nil {
800+
if err := service.Listener.Decode(w, r, &req); err != nil {
801+
w.WriteHeader(http.StatusBadRequest)
802+
return
803+
}
804+
if err := req.Validate(); err != nil {
805+
logger.Errorf("[Azure CNS] invalid request %+v: %s", req, err)
806+
w.WriteHeader(http.StatusBadRequest)
803807
return
804808
}
805809

810+
logger.Request(service.Name, req.String(), nil)
806811
var returnCode types.ResponseCode
807812
var returnMessage string
813+
var err error
808814
switch r.Method {
809815
case http.MethodPost:
810816
if req.NetworkContainerType == cns.WebApps {

‎cns/restserver/api_test.go

+14-14
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",
@@ -323,7 +323,7 @@ func TestCreateNetworkContainer(t *testing.T) {
323323
fmt.Println("TestCreateNetworkContainer: JobObject")
324324

325325
params := createOrUpdateNetworkContainerParams{
326-
ncID: "testJobObject",
326+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d476",
327327
ncIP: "10.1.0.5",
328328
ncType: "JobObject",
329329
ncVersion: "0",
@@ -348,7 +348,7 @@ func TestCreateNetworkContainer(t *testing.T) {
348348
// Test create network container of type WebApps
349349
fmt.Println("TestCreateNetworkContainer: WebApps")
350350
params = createOrUpdateNetworkContainerParams{
351-
ncID: "ethWebApp",
351+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d475",
352352
ncIP: "192.0.0.5",
353353
ncType: "WebApps",
354354
ncVersion: "0",
@@ -363,7 +363,7 @@ func TestCreateNetworkContainer(t *testing.T) {
363363
}
364364

365365
params = createOrUpdateNetworkContainerParams{
366-
ncID: "ethWebApp",
366+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d475",
367367
ncIP: "192.0.0.6",
368368
ncType: "WebApps",
369369
ncVersion: "0",
@@ -387,7 +387,7 @@ func TestCreateNetworkContainer(t *testing.T) {
387387

388388
// Test create network container of type COW
389389
params = createOrUpdateNetworkContainerParams{
390-
ncID: "testCOWContainer",
390+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d474",
391391
ncIP: "10.0.0.5",
392392
ncType: "COW",
393393
ncVersion: "0",
@@ -418,7 +418,7 @@ func TestGetNetworkContainerByOrchestratorContext(t *testing.T) {
418418
setOrchestratorType(t, cns.Kubernetes)
419419

420420
params := createOrUpdateNetworkContainerParams{
421-
ncID: "ethWebApp",
421+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d471",
422422
ncIP: "11.0.0.5",
423423
ncType: cns.AzureContainerInstance,
424424
ncVersion: "0",
@@ -480,7 +480,7 @@ func TestGetInterfaceForNetworkContainer(t *testing.T) {
480480
setOrchestratorType(t, cns.Kubernetes)
481481

482482
params := createOrUpdateNetworkContainerParams{
483-
ncID: "ethWebApp",
483+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d479",
484484
ncIP: "11.0.0.5",
485485
ncType: "WebApps",
486486
ncVersion: "0",
@@ -548,7 +548,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
548548
defer cleanupWSP()
549549

550550
params := createOrUpdateNetworkContainerParams{
551-
ncID: "nc-nma-success",
551+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d475",
552552
ncIP: "11.0.0.5",
553553
ncType: cns.AzureContainerInstance,
554554
ncVersion: "0",
@@ -596,7 +596,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
596596
// Testing the path where the NC version with CNS is higher than the one with NMAgent.
597597
// This indicates that the NMAgent is yet to program the NC version.
598598
params = createOrUpdateNetworkContainerParams{
599-
ncID: "nc-nma-fail-version-mismatch",
599+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d474",
600600
ncIP: "11.0.0.5",
601601
ncType: cns.AzureContainerInstance,
602602
ncVersion: "1",
@@ -635,7 +635,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
635635

636636
// Testing the path where NMAgent response status code is not 200.
637637
params = createOrUpdateNetworkContainerParams{
638-
ncID: "nc-nma-fail-500",
638+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d473",
639639
ncIP: "11.0.0.5",
640640
ncType: cns.AzureContainerInstance,
641641
ncVersion: "0",
@@ -677,7 +677,7 @@ func TestGetNetworkContainerVersionStatus(t *testing.T) {
677677

678678
// Testing the path where NMAgent response status code is 200 but embedded response is 401
679679
params = createOrUpdateNetworkContainerParams{
680-
ncID: "nc-nma-fail-unavailable",
680+
ncID: "f47ac10b-58cc-0372-8567-0e02b2c3d472",
681681
ncIP: "11.0.0.5",
682682
ncType: cns.AzureContainerInstance,
683683
ncVersion: "0",
@@ -1290,7 +1290,7 @@ func postAllNetworkContainers(t *testing.T, ncParams []createOrUpdateNetworkCont
12901290
err = decodeResponse(w, &resp)
12911291

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

‎cns/restserver/util.go

+5-5
Original file line numberDiff line numberDiff line change
@@ -635,9 +635,7 @@ func (service *HTTPRestService) getNetPluginDetails() *networkcontainers.NetPlug
635635
func (service *HTTPRestService) getNetworkContainerDetails(networkContainerID string) (containerstatus, bool) {
636636
service.RLock()
637637
defer service.RUnlock()
638-
639638
containerDetails, containerExists := service.state.ContainerStatus[networkContainerID]
640-
641639
return containerDetails, containerExists
642640
}
643641

@@ -653,17 +651,14 @@ func (service *HTTPRestService) areNCsPresent() bool {
653651
func (service *HTTPRestService) isNetworkJoined(networkID string) bool {
654652
namedLock.LockAcquire(stateJoinedNetworks)
655653
defer namedLock.LockRelease(stateJoinedNetworks)
656-
657654
_, exists := service.state.joinedNetworks[networkID]
658-
659655
return exists
660656
}
661657

662658
// Set the network as joined
663659
func (service *HTTPRestService) setNetworkStateJoined(networkID string) {
664660
namedLock.LockAcquire(stateJoinedNetworks)
665661
defer namedLock.LockRelease(stateJoinedNetworks)
666-
667662
service.state.joinedNetworks[networkID] = struct{}{}
668663
}
669664

@@ -886,6 +881,11 @@ func (service *HTTPRestService) handlePostNetworkContainers(w http.ResponseWrite
886881
logger.Response(service.Name, response, response.Response.ReturnCode, err)
887882
return
888883
}
884+
if err := req.Validate(); err != nil { //nolint:govet // shadow okay
885+
logger.Errorf("[Azure CNS] handlePostNetworkContainers failed with error: %s", err.Error())
886+
w.WriteHeader(http.StatusBadRequest)
887+
return
888+
}
889889

890890
createNCsResp := service.createNetworkContainers(req.CreateNetworkContainerRequests)
891891
response := cns.PostNetworkContainersResponse{

0 commit comments

Comments
 (0)