Skip to content

Commit

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



* fix tests



* add logs and test



---------

Signed-off-by: Evan Baker <rbtr@users.noreply.github.com>
  • Loading branch information
@rbtr
rbtr authored Nov 8, 2023
1 parent b33b79b commit 441d94a
Show file tree
Hide file tree
Showing 8 changed files with 148 additions and 39 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 @@ -75,6 +76,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 @@ -96,6 +99,16 @@ type CreateNetworkContainerRequest struct {
NCStatus v1alpha.NCStatus
}

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 @@ -374,6 +387,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 TestNewPodInfoFromIPConfigRequest(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 @@ -797,14 +797,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
28 changes: 14 additions & 14 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 Down Expand Up @@ -323,7 +323,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 @@ -348,7 +348,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 @@ -363,7 +363,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 @@ -387,7 +387,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 @@ -418,7 +418,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 @@ -480,7 +480,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 @@ -548,7 +548,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 @@ -596,7 +596,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 @@ -635,7 +635,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 @@ -677,7 +677,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 @@ -1290,7 +1290,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
10 changes: 5 additions & 5 deletions cns/restserver/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -635,9 +635,7 @@ func (service *HTTPRestService) getNetPluginDetails() *networkcontainers.NetPlug
func (service *HTTPRestService) getNetworkContainerDetails(networkContainerID string) (containerstatus, bool) {
service.RLock()
defer service.RUnlock()

containerDetails, containerExists := service.state.ContainerStatus[networkContainerID]

return containerDetails, containerExists
}

Expand All @@ -653,17 +651,14 @@ func (service *HTTPRestService) areNCsPresent() bool {
func (service *HTTPRestService) isNetworkJoined(networkID string) bool {
namedLock.LockAcquire(stateJoinedNetworks)
defer namedLock.LockRelease(stateJoinedNetworks)

_, exists := service.state.joinedNetworks[networkID]

return exists
}

// Set the network as joined
func (service *HTTPRestService) setNetworkStateJoined(networkID string) {
namedLock.LockAcquire(stateJoinedNetworks)
defer namedLock.LockRelease(stateJoinedNetworks)

service.state.joinedNetworks[networkID] = struct{}{}
}

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

createNCsResp := service.createNetworkContainers(req.CreateNetworkContainerRequests)
response := cns.PostNetworkContainersResponse{
Expand Down

0 comments on commit 441d94a

Please sign in to comment.