From 0f2cd58115ebb6dcbe791ce5f0cd639f74fae869 Mon Sep 17 00:00:00 2001 From: Xing Zheng Date: Tue, 3 Sep 2024 19:12:42 +0000 Subject: [PATCH] Remove ENI ID which is used for RWMutex --- agent/handlers/v4/tmdsstate.go | 1 - .../handlers/fault/v1/handlers/handlers.go | 62 ++++++------------ .../tmds/handlers/v4/state/response.go | 2 - .../handlers/fault/v1/handlers/handlers.go | 65 +++++++------------ .../fault/v1/handlers/handlers_test.go | 44 ++----------- ecs-agent/tmds/handlers/v4/handlers_test.go | 1 - ecs-agent/tmds/handlers/v4/state/response.go | 2 - 7 files changed, 49 insertions(+), 128 deletions(-) diff --git a/agent/handlers/v4/tmdsstate.go b/agent/handlers/v4/tmdsstate.go index 36f9c45f4b2..01b2b1c3858 100644 --- a/agent/handlers/v4/tmdsstate.go +++ b/agent/handlers/v4/tmdsstate.go @@ -161,7 +161,6 @@ func (s *TMDSAgentState) getTaskMetadata(v3EndpointID string, includeTags bool) Path: task.GetNetworkNamespace(), NetworkInterfaces: []*tmdsv4.NetworkInterface{ { - ENIID: "eni-fake-id", DeviceName: "ethx", }, }, diff --git a/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/tmds/handlers/fault/v1/handlers/handlers.go b/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/tmds/handlers/fault/v1/handlers/handlers.go index b38a959cf2e..ecc88300b06 100644 --- a/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/tmds/handlers/fault/v1/handlers/handlers.go +++ b/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/tmds/handlers/fault/v1/handlers/handlers.go @@ -96,8 +96,7 @@ func (h *FaultHandler) StartNetworkBlackholePort() func(http.ResponseWriter, *ht return } - // the network blackhole port fault would be injected to given task network - // namespace. + // To avoid multiple requests to manipulate same network resource networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path rwMu := h.loadLock(networkNSPath) rwMu.Lock() @@ -148,6 +147,7 @@ func (h *FaultHandler) StopNetworkBlackHolePort() func(http.ResponseWriter, *htt return } + // To avoid multiple requests to manipulate same network resource networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path rwMu := h.loadLock(networkNSPath) rwMu.Lock() @@ -198,6 +198,7 @@ func (h *FaultHandler) CheckNetworkBlackHolePort() func(http.ResponseWriter, *ht return } + // To avoid multiple requests to manipulate same network resource networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path rwMu := h.loadLock(networkNSPath) rwMu.RLock() @@ -243,13 +244,9 @@ func (h *FaultHandler) StartNetworkLatency() func(http.ResponseWriter, *http.Req return } - // the network latency fault would be injected to given elastic network - // interface/ENI. - eniID := taskMetadata.TaskNetworkConfig. - NetworkNamespaces[0]. - NetworkInterfaces[0]. - ENIID - rwMu := h.loadLock(eniID) + // To avoid multiple requests to manipulate same network resource + networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path + rwMu := h.loadLock(networkNSPath) rwMu.Lock() defer rwMu.Unlock() @@ -294,11 +291,9 @@ func (h *FaultHandler) StopNetworkLatency() func(http.ResponseWriter, *http.Requ return } - eniID := taskMetadata.TaskNetworkConfig. - NetworkNamespaces[0]. - NetworkInterfaces[0]. - ENIID - rwMu := h.loadLock(eniID) + // To avoid multiple requests to manipulate same network resource + networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path + rwMu := h.loadLock(networkNSPath) rwMu.Lock() defer rwMu.Unlock() @@ -343,11 +338,9 @@ func (h *FaultHandler) CheckNetworkLatency() func(http.ResponseWriter, *http.Req return } - eniID := taskMetadata.TaskNetworkConfig. - NetworkNamespaces[0]. - NetworkInterfaces[0]. - ENIID - rwMu := h.loadLock(eniID) + // To avoid multiple requests to manipulate same network resource + networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path + rwMu := h.loadLock(networkNSPath) rwMu.RLock() defer rwMu.RUnlock() @@ -391,13 +384,9 @@ func (h *FaultHandler) StartNetworkPacketLoss() func(http.ResponseWriter, *http. return } - // the network packet loss fault would be injected to given elastic network - // interface/ENI. - eniID := taskMetadata.TaskNetworkConfig. - NetworkNamespaces[0]. - NetworkInterfaces[0]. - ENIID - rwMu := h.loadLock(eniID) + // To avoid multiple requests to manipulate same network resource + networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path + rwMu := h.loadLock(networkNSPath) rwMu.Lock() defer rwMu.Unlock() @@ -442,11 +431,9 @@ func (h *FaultHandler) StopNetworkPacketLoss() func(http.ResponseWriter, *http.R return } - eniID := taskMetadata.TaskNetworkConfig. - NetworkNamespaces[0]. - NetworkInterfaces[0]. - ENIID - rwMu := h.loadLock(eniID) + // To avoid multiple requests to manipulate same network resource + networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path + rwMu := h.loadLock(networkNSPath) rwMu.Lock() defer rwMu.Unlock() @@ -492,11 +479,9 @@ func (h *FaultHandler) CheckNetworkPacketLoss() func(http.ResponseWriter, *http. return } - eniID := taskMetadata.TaskNetworkConfig. - NetworkNamespaces[0]. - NetworkInterfaces[0]. - ENIID - rwMu := h.loadLock(eniID) + // To avoid multiple requests to manipulate same network resource + networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path + rwMu := h.loadLock(networkNSPath) rwMu.RLock() defer rwMu.RUnlock() @@ -706,10 +691,5 @@ func validateTaskNetworkConfig(taskNetworkConfig *state.TaskNetworkConfig) error return errors.New("no ENI device name in the network namespace within task network config") } - // ENIID is required to avoid race condition where multiple requests are manunipuating same ENI. - if taskNetworkConfig.NetworkNamespaces[0].NetworkInterfaces[0].ENIID == "" { - return errors.New("no ENI ID in the network namespace within task network config") - } - return nil } diff --git a/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/tmds/handlers/v4/state/response.go b/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/tmds/handlers/v4/state/response.go index 124c2f961c7..2e1b9b8e468 100644 --- a/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/tmds/handlers/v4/state/response.go +++ b/agent/vendor/github.com/aws/amazon-ecs-agent/ecs-agent/tmds/handlers/v4/state/response.go @@ -56,8 +56,6 @@ type NetworkNamespace struct { type NetworkInterface struct { // DeviceName is the device name on the host. DeviceName string - // ENIID is the id of eni. - ENIID string } // Instance's clock drift status diff --git a/ecs-agent/tmds/handlers/fault/v1/handlers/handlers.go b/ecs-agent/tmds/handlers/fault/v1/handlers/handlers.go index b38a959cf2e..438d0cdbc74 100644 --- a/ecs-agent/tmds/handlers/fault/v1/handlers/handlers.go +++ b/ecs-agent/tmds/handlers/fault/v1/handlers/handlers.go @@ -44,8 +44,7 @@ const ( type FaultHandler struct { // mutexMap is used to avoid multiple clients to manipulate same resource at same - // time. The 'key' is the ENI ID or the network namespace path and 'value' is the - // RWMutex. + // time. The 'key' is the the network namespace path and 'value' is the RWMutex. // Using concurrent map here because the handler is shared by all requests. mutexMap sync.Map AgentState state.AgentState @@ -96,8 +95,7 @@ func (h *FaultHandler) StartNetworkBlackholePort() func(http.ResponseWriter, *ht return } - // the network blackhole port fault would be injected to given task network - // namespace. + // To avoid multiple requests to manipulate same network resource networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path rwMu := h.loadLock(networkNSPath) rwMu.Lock() @@ -148,6 +146,7 @@ func (h *FaultHandler) StopNetworkBlackHolePort() func(http.ResponseWriter, *htt return } + // To avoid multiple requests to manipulate same network resource networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path rwMu := h.loadLock(networkNSPath) rwMu.Lock() @@ -198,6 +197,7 @@ func (h *FaultHandler) CheckNetworkBlackHolePort() func(http.ResponseWriter, *ht return } + // To avoid multiple requests to manipulate same network resource networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path rwMu := h.loadLock(networkNSPath) rwMu.RLock() @@ -243,13 +243,9 @@ func (h *FaultHandler) StartNetworkLatency() func(http.ResponseWriter, *http.Req return } - // the network latency fault would be injected to given elastic network - // interface/ENI. - eniID := taskMetadata.TaskNetworkConfig. - NetworkNamespaces[0]. - NetworkInterfaces[0]. - ENIID - rwMu := h.loadLock(eniID) + // To avoid multiple requests to manipulate same network resource + networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path + rwMu := h.loadLock(networkNSPath) rwMu.Lock() defer rwMu.Unlock() @@ -294,11 +290,9 @@ func (h *FaultHandler) StopNetworkLatency() func(http.ResponseWriter, *http.Requ return } - eniID := taskMetadata.TaskNetworkConfig. - NetworkNamespaces[0]. - NetworkInterfaces[0]. - ENIID - rwMu := h.loadLock(eniID) + // To avoid multiple requests to manipulate same network resource + networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path + rwMu := h.loadLock(networkNSPath) rwMu.Lock() defer rwMu.Unlock() @@ -343,11 +337,9 @@ func (h *FaultHandler) CheckNetworkLatency() func(http.ResponseWriter, *http.Req return } - eniID := taskMetadata.TaskNetworkConfig. - NetworkNamespaces[0]. - NetworkInterfaces[0]. - ENIID - rwMu := h.loadLock(eniID) + // To avoid multiple requests to manipulate same network resource + networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path + rwMu := h.loadLock(networkNSPath) rwMu.RLock() defer rwMu.RUnlock() @@ -391,13 +383,9 @@ func (h *FaultHandler) StartNetworkPacketLoss() func(http.ResponseWriter, *http. return } - // the network packet loss fault would be injected to given elastic network - // interface/ENI. - eniID := taskMetadata.TaskNetworkConfig. - NetworkNamespaces[0]. - NetworkInterfaces[0]. - ENIID - rwMu := h.loadLock(eniID) + // To avoid multiple requests to manipulate same network resource + networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path + rwMu := h.loadLock(networkNSPath) rwMu.Lock() defer rwMu.Unlock() @@ -442,11 +430,9 @@ func (h *FaultHandler) StopNetworkPacketLoss() func(http.ResponseWriter, *http.R return } - eniID := taskMetadata.TaskNetworkConfig. - NetworkNamespaces[0]. - NetworkInterfaces[0]. - ENIID - rwMu := h.loadLock(eniID) + // To avoid multiple requests to manipulate same network resource + networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path + rwMu := h.loadLock(networkNSPath) rwMu.Lock() defer rwMu.Unlock() @@ -492,11 +478,9 @@ func (h *FaultHandler) CheckNetworkPacketLoss() func(http.ResponseWriter, *http. return } - eniID := taskMetadata.TaskNetworkConfig. - NetworkNamespaces[0]. - NetworkInterfaces[0]. - ENIID - rwMu := h.loadLock(eniID) + // To avoid multiple requests to manipulate same network resource + networkNSPath := taskMetadata.TaskNetworkConfig.NetworkNamespaces[0].Path + rwMu := h.loadLock(networkNSPath) rwMu.RLock() defer rwMu.RUnlock() @@ -706,10 +690,5 @@ func validateTaskNetworkConfig(taskNetworkConfig *state.TaskNetworkConfig) error return errors.New("no ENI device name in the network namespace within task network config") } - // ENIID is required to avoid race condition where multiple requests are manunipuating same ENI. - if taskNetworkConfig.NetworkNamespaces[0].NetworkInterfaces[0].ENIID == "" { - return errors.New("no ENI ID in the network namespace within task network config") - } - return nil } diff --git a/ecs-agent/tmds/handlers/fault/v1/handlers/handlers_test.go b/ecs-agent/tmds/handlers/fault/v1/handlers/handlers_test.go index 47b17ad592b..c810a48b94c 100644 --- a/ecs-agent/tmds/handlers/fault/v1/handlers/handlers_test.go +++ b/ecs-agent/tmds/handlers/fault/v1/handlers/handlers_test.go @@ -50,28 +50,18 @@ const ( awsvpcNetworkMode = "awsvpc" deviceName = "eth0" invalidNetworkMode = "invalid" - eniID = "eni-123" ) var ( noDeviceNameInNetworkInterfaces = []*state.NetworkInterface{ { DeviceName: "", - ENIID: eniID, - }, - } - - noENIIDInNetworkInterfaces = []*state.NetworkInterface{ - { - DeviceName: deviceName, - ENIID: "", }, } happyNetworkInterfaces = []*state.NetworkInterface{ { DeviceName: deviceName, - ENIID: eniID, }, } @@ -264,7 +254,7 @@ func generateNetworkBlackHolePortTestCases(name string, expectedHappyResponseBod agentState.EXPECT().GetTaskMetadata(endpointId).Return(state.TaskResponse{ TaskResponse: &v2.TaskResponse{TaskARN: taskARN}, FaultInjectionEnabled: false, - }, nil) + }, nil).Times(1) }, }, { @@ -280,7 +270,7 @@ func generateNetworkBlackHolePortTestCases(name string, expectedHappyResponseBod NetworkMode: invalidNetworkMode, NetworkNamespaces: happyNetworkNamespaces, }, - }, nil) + }, nil).Times(1) }, }, { @@ -294,7 +284,7 @@ func generateNetworkBlackHolePortTestCases(name string, expectedHappyResponseBod TaskResponse: &v2.TaskResponse{TaskARN: taskARN}, FaultInjectionEnabled: true, TaskNetworkConfig: nil, - }, nil) + }, nil).Times(1) }, }, { @@ -311,7 +301,7 @@ func generateNetworkBlackHolePortTestCases(name string, expectedHappyResponseBod NetworkMode: awsvpcNetworkMode, NetworkNamespaces: nil, }, - }, nil) + }, nil).Times(1) }, }, { @@ -328,7 +318,7 @@ func generateNetworkBlackHolePortTestCases(name string, expectedHappyResponseBod NetworkMode: awsvpcNetworkMode, NetworkNamespaces: noPathInNetworkNamespaces, }, - }, nil) + }, nil).Times(1) }, }, { @@ -350,29 +340,7 @@ func generateNetworkBlackHolePortTestCases(name string, expectedHappyResponseBod }, }, }, - }, nil) - }, - }, - { - name: fmt.Sprintf("%s no ENI ID in task network config", name), - expectedStatusCode: 500, - requestBody: happyBlackHolePortReqBody, - expectedResponseBody: types.NewNetworkFaultInjectionErrorResponse( - fmt.Sprintf("failed to get task metadata due to internal server error for container: %s", endpointId)), - setAgentStateExpectations: func(agentState *mock_state.MockAgentState) { - agentState.EXPECT().GetTaskMetadata(endpointId).Return(state.TaskResponse{ - TaskResponse: &v2.TaskResponse{TaskARN: taskARN}, - FaultInjectionEnabled: true, - TaskNetworkConfig: &state.TaskNetworkConfig{ - NetworkMode: awsvpcNetworkMode, - NetworkNamespaces: []*state.NetworkNamespace{ - &state.NetworkNamespace{ - Path: "/path", - NetworkInterfaces: noENIIDInNetworkInterfaces, - }, - }, - }, - }, nil) + }, nil).Times(1) }, }, } diff --git a/ecs-agent/tmds/handlers/v4/handlers_test.go b/ecs-agent/tmds/handlers/v4/handlers_test.go index 55d7a5012ec..388170f7244 100644 --- a/ecs-agent/tmds/handlers/v4/handlers_test.go +++ b/ecs-agent/tmds/handlers/v4/handlers_test.go @@ -167,7 +167,6 @@ func taskResponse() *state.TaskResponse { NetworkInterfaces: []*state.NetworkInterface{ &state.NetworkInterface{ DeviceName: "eth1", - ENIID: "eni-013ff4ad5747a0f6a", }, }, }, diff --git a/ecs-agent/tmds/handlers/v4/state/response.go b/ecs-agent/tmds/handlers/v4/state/response.go index 124c2f961c7..2e1b9b8e468 100644 --- a/ecs-agent/tmds/handlers/v4/state/response.go +++ b/ecs-agent/tmds/handlers/v4/state/response.go @@ -56,8 +56,6 @@ type NetworkNamespace struct { type NetworkInterface struct { // DeviceName is the device name on the host. DeviceName string - // ENIID is the id of eni. - ENIID string } // Instance's clock drift status