[DHPA] Upudate dockerPortMap() in task.go with dynamic host port range support - part 2+ #3589

chienhanlin · 2023-02-24T17:28:36Z

Summary

DHPA - Dynamic Host Port Assignment

The target branch of this PR is feature/dynamicHostPortAssignment.

This is a follow-up PR for "Upudate dockerPortMap() in task.go with dynamic host port range support" part 1 and part 2 to

Implement dynamic host port assignment for default bridge mode service connect ingress listener ports in dockerPortMap(). Details of service connect ingress listener port bindings changes, comparing to part 1 can be found in part 2.
Validate the host port/host port range found by ECS Agent before returning it through GetHostPort()/GetHostPortRange() to address comments on the part 1 PR here.

Implementation details

agent/utils/ephemeral_ports.go

New! Add host port/host port range validation to GetHostPort()/GetHostPortRange()
New! Make portIsInRange() and verifyPortsWithinRange() to private function

agent/utils/ephemeral_ports_test.go

Use ResetTracker() in TestGetHostPort and TestGetHostPortRange
Add TestResetTracker
New! Remove host port validation in TestGetHostPort as the port should be validated before returning back from utils.GetHostPort()

agent/api/task/task.go

Add function buildPortMapWithSCIngressConfig() to build a dockerPortMap and a containerPortSet for ingress listener ports
Refactor how we create port bindings for service connect ingress listener ports and update comments for function dockerPortMap()
New! Update comments
New! Remove unclear logger.Debug entry from dockerPortMap()

agent/api/task/task_test.go

Update test cases in TestDockerHostConfigSCBridgeMode to test changes
New! Remove singular host port and host port range validation in TestDockerHostConfigPortBinding and TestDockerHostConfigSCBridgeMode as ports should be validated before returning back from utils.GetHostPort()/utils.GetHostPortRange()

Testing

New tests cover the changes: yes

Unit test

TestGetHostPortRange

-- PASS: TestGetHostPortRange (0.00s)
    --- PASS: TestGetHostPortRange/tcp_protocol,_contiguous_hostPortRange_found (0.00s)
    --- PASS: TestGetHostPortRange/udp_protocol,_contiguous_hostPortRange_found (0.00s)
    --- PASS: TestGetHostPortRange/2_requests_for_contiguous_hostPortRange_in_succession,_success (0.00s)
    --- PASS: TestGetHostPortRange/contiguous_hostPortRange_after_looping_back,_success (0.00s)
    --- PASS: TestGetHostPortRange/contiguous_hostPortRange_not_found (0.00s)

TestGetHostPort

--- PASS: TestGetHostPort (0.00s)
    --- PASS: TestGetHostPort/tcp_protocol,_a_host_port_found (0.00s)
    --- PASS: TestGetHostPort/udp_protocol,_a_host_port_found (0.00s)
    --- PASS: TestGetHostPort/5_requests_for_host_port_in_succession,_success (0.00s)
    --- PASS: TestGetHostPort/5_requests_for_host_port_in_succession,_success#01 (0.00s)

TestResetTracker

--- PASS: TestResetTracker (0.00s)

TestDockerHostConfigPortBinding

--- PASS: TestDockerHostConfigPortBinding (0.00s)
    --- PASS: TestDockerHostConfigPortBinding/user-specified_container_ports_and_host_ports (0.00s)
    --- PASS: TestDockerHostConfigPortBinding/user-specified_container_ports_with_a_ideal_dynamicHostPortRange (0.00s)
    --- PASS: TestDockerHostConfigPortBinding/user-specified_container_ports_with_a_bad_dynamicHostPortRange (0.00s)
    --- PASS: TestDockerHostConfigPortBinding/user-specified_container_port_and_container_port_range_with_a_ideal_dynamicHostPortRange (0.00s)
    --- PASS: TestDockerHostConfigPortBinding/user-specified_container_port_and_container_port_range_with_a_bad_user-specified_dynamicHostPortRange (0.00s)

TestDockerHostConfigSCBridgeMode

--- PASS: TestDockerHostConfigSCBridgeMode (0.00s)
    --- PASS: TestDockerHostConfigSCBridgeMode/with_default_dynamic_host_port_range (0.00s)
    --- PASS: TestDockerHostConfigSCBridgeMode/with_user-specified_dynamic_host_port_range (0.00s)

Manual testing

Re-ran test cases listed in Upudate dockerPortMap() in task.go with dynamic host port range support - part 1, and test results are the same as expected.

New tests cover the changes: no

Description for the changelog

[Enhancement] Support user-specified dynamic host port range for a singular port and default bridge mode service connect ingress listener port.

Related PRs

Licensing

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…rt part 2

…rning it

chienhanlin · 2023-02-24T17:29:36Z

agent/api/task/task.go

+//
+//	Instead, ECS Agent finds host ports within the given dynamic host port range. An error will be returned for case (2) if
+//	ECS Agent cannot find an available host port within range.
+func (task *Task) buildPortMapWithSCIngressConfig(dynamicHostPortRange string) (nat.PortMap, map[int]struct{}, error) {


This function buildPortMapWithSCIngressConfig() has been reviewed in #3585.

chienhanlin · 2023-02-24T17:34:16Z

agent/api/task/task.go

+			// and do not have port collisions.
+			hostPortStr = strconv.Itoa(int(*ic.HostPort))
+		} else {
+			// For default bridge mode service connect experience, customers do not specify a host port


Comments from line 2366-2369 are newly added in this PR.

chienhanlin · 2023-02-24T17:59:46Z

agent/api/task/task.go

 //
 // For non-service connect bridge network mode task, ECS Agent will assign a host port or a host port range
 // within the default/user-specified dynamic host port range. If no available host port or host port range can be
 // found by ECS Agent, an error will be returned.
 //
-// Note that,
+// Note that
 // (a) ECS Agent will not assign a new host port within the dynamic host port range for awsvpc network mode task
 // (b) ECS Agent will not assign a new host port within the dynamic host port range if the user-specified host port exists
 func (task *Task) dockerPortMap(container *apicontainer.Container, dynamicHostPortRange string) (nat.PortMap, error) {


This function dockerPortMap() has been reviewed in #3585.

chienhanlin · 2023-02-24T18:01:55Z

agent/api/task/task.go

-			// If container is neither SC container nor pause container, it's a regular task container. Its port bindings(s)
-			// are published by the associated pause container, and we leave the map empty here (docker would actually complain
-			// otherwise).
+			// If the container is not a pause container, then it is a regular customers' application container


Comments from line 2444-2446 are newly updated in this PR.

The original comment shown as follows is not valid; thus it is removed in this PR.

// Note - for service connect bridge mode, we do not allow customers to specify a host port for their application containers. // Additionally, AppNet will NOT proxy traffic to that port when an ephemeral host port is assigned.

chienhanlin · 2023-02-24T18:04:59Z

agent/api/task/task_test.go

@@ -376,17 +376,6 @@ func TestDockerHostConfigPortBinding(t *testing.T) {
 					if !reflect.DeepEqual(config.PortBindings, tc.expectedPortBinding) {
 						t.Error("Expected port bindings to be resolved, was: ", config.PortBindings)
 					}
-				} else {


NEW!: Remove singular host port validation in TestDockerHostConfigPortBinding as ports should be validated before returning back from utils.GetHostPort()

chienhanlin · 2023-02-24T18:05:36Z

agent/api/task/task_test.go

@@ -399,13 +388,6 @@ func TestDockerHostConfigPortBinding(t *testing.T) {
 					if !reflect.DeepEqual(tc.testTask.Containers[0].ContainerPortRangeMap, tc.expectedContainerPortRangeMap) {
 						t.Error("Expected container port range map to be resolved, was: ", tc.testTask.Containers[0].GetContainerPortRangeMap())
 					}
-				} else {
-					// Verify ECS Agent assigned host port range are within the dynamic host port range


NEW!: Remove host port range validation in TestDockerHostConfigSCBridgeMode as ports should be validated before returning back from utils.GetHostPortRange()

chienhanlin · 2023-02-24T18:06:26Z

agent/api/task/task_test.go

@@ -424,6 +406,14 @@ var (
 	defaultSCProtocol                      = "/tcp"
 )

+func getDefaultDynamicHostPortRange() (start int, end int) {


This function getDefaultDynamicHostPortRange() has been reviewed in #3585.

chienhanlin · 2023-02-24T18:06:49Z

agent/api/task/task_test.go

@@ -479,61 +469,91 @@ func convertSCPort(port uint16) nat.Port {
 }

 // TestDockerHostConfigSCBridgeMode verifies port bindings and network mode overrides for each
-// container in an SC-enabled bridge mode task. The test task is consisted of the SC container, a regular container,
+// container in an SC-enabled bridge mode task with default/user-specified dynamic host port range.
+// The test task is consisted of the SC container, a regular container,
 // and two pause containers associated with each.
 func TestDockerHostConfigSCBridgeMode(t *testing.T) {


This test TestDockerHostConfigSCBridgeMode() has been reviewed in #3585.

chienhanlin · 2023-02-24T18:07:28Z

agent/utils/ephemeral_ports.go

@@ -96,13 +98,25 @@ func (pt *safePortTracker) GetLastAssignedHostPort() int {

 var tracker safePortTracker

+// ResetTracker resets the last assigned host port to 0.
+func ResetTracker() {


This function ResetTracker() has been reviewed in #3585.

chienhanlin · 2023-02-24T18:07:54Z

agent/utils/ephemeral_ports_test.go

 			assert.Equal(t, tc.expectedResult, result)
 		})
 	}
 }

+func TestResetTracker(t *testing.T) {


This test TestResetTracker() has been reviewed in #3585.

chienhanlin · 2023-02-24T18:08:13Z

agent/utils/ephemeral_ports_test.go

@@ -196,7 +196,8 @@ func TestGetHostPort(t *testing.T) {

 	for _, tc := range testCases {
 		if tc.resetLastAssignedHostPort {
-			tracker.SetLastAssignedHostPort(0)
+			// need to reset the tracker to avoid getting data from previous test cases
+			ResetTracker()


This function ResetTracker() has been reviewed in #3585.

chienhanlin · 2023-02-24T18:08:30Z

agent/utils/ephemeral_ports_test.go

@@ -144,7 +144,7 @@ func TestGetHostPortRange(t *testing.T) {
 					assert.Equal(t, tc.expectedLastAssignedPort[i], actualLastAssignedHostPort)
 				} else {
 					// need to reset the tracker to avoid getting data from previous test cases
-					tracker.SetLastAssignedHostPort(0)
+					ResetTracker()


This function ResetTracker() has been reviewed in #3585.

chienhanlin · 2023-02-24T18:09:20Z

agent/utils/ephemeral_ports.go

 // GetHostPortRange gets N contiguous host ports from the ephemeral host port range defined on the host.
 // dynamicHostPortRange can be set by customers using ECS Agent environment variable ECS_DYNAMIC_HOST_PORT_RANGE;
 // otherwise, ECS Agent will use the default value returned from GetDynamicHostPortRange() in the utils package.
 func GetHostPortRange(numberOfPorts int, protocol string, dynamicHostPortRange string) (string, error) {
 	portLock.Lock()
 	defer portLock.Unlock()
 	result, err := getNumOfHostPorts(numberOfPorts, protocol, dynamicHostPortRange)
+	if err == nil {


NEW!: Add port range validation as ports should be validated before returning back from utils.GetHostPortRange()

NIt; generally, I would handle the case where err != nil in an if condition for the first thing that could return an error and then another if condition if the next thing could also return an error.

chienhanlin · 2023-02-24T18:09:46Z

agent/utils/ephemeral_ports.go

 	if err == nil {
-		result = strings.Split(result, "-")[0]
+		// Verify the found host port is within the given dynamic host port range


NEW!: Add port validation as ports should be validated before returning back from utils.GetHostPort()

mye956 · 2023-02-24T19:20:43Z

agent/api/task/task.go

+			// or return an error if no host port is available within the range.
+			hostPortStr, err = getHostPort(protocolStr, dynamicHostPortRange)
+			if err != nil {
+				return nil, nil, err


nit(non-blocking): maybe we can add a logging statement here saying no host port was available.

Don't have to add a logging statement here as the error returned from this function will be surfaced on line 2426 : )

mye956 · 2023-02-24T19:24:10Z

agent/api/task/task.go

-			// and let them be published by the associated pause container.
-			// Note - for SC bridge mode we do not allow customer to specify a host port for their containers. Additionally,
-			// When an ephemeral host port is assigned, Appnet will NOT proxy traffic to that port
+			// Find the task container associated with this particular pause container
 			taskContainer, err := task.getBridgeModeTaskContainerForPauseContainer(container)
 			if err != nil {
 				return nil, err


nit(non-blocking): can also maybe add a logging statement here

Thanks for the suggestion! I saw that getBridgeModeTaskContainerForPauseContainer() has been used in dockerExposedPorts() and PopulateServiceConnectContainerMappingEnvVar() as well. These functions are related to createContainer, thus the error returned from getBridgeModeTaskContainerForPauseContainer() will eventually be logged as dockerapi.DockerContainerMetadata{Error: XXX} in ecs-agent.log and also be reported back to ECS control plane.

SreeeS · 2023-02-24T20:31:49Z

agent/api/task/task.go

 				return dockerPortMap, nil
 			}
+			// If the associated task container to this pause container is NOT the service connect AppNet container,
+			// we will continue to update the dockerPortMap for the pause container using the port bindings
+			// configured for the application container since port bindings will be published by the pasue container.


nit: "pause"

Thanks for catching this. Will fix the typo when we are going in the PR to rebase against the dev branch.

YashdalfTheGray

The comments that serve as mile markers in this code review are really helpful. Thank you for putting those in.

YashdalfTheGray · 2023-02-24T20:36:57Z

agent/utils/ephemeral_ports.go

 // GetHostPortRange gets N contiguous host ports from the ephemeral host port range defined on the host.
 // dynamicHostPortRange can be set by customers using ECS Agent environment variable ECS_DYNAMIC_HOST_PORT_RANGE;
 // otherwise, ECS Agent will use the default value returned from GetDynamicHostPortRange() in the utils package.
 func GetHostPortRange(numberOfPorts int, protocol string, dynamicHostPortRange string) (string, error) {
 	portLock.Lock()
 	defer portLock.Unlock()
 	result, err := getNumOfHostPorts(numberOfPorts, protocol, dynamicHostPortRange)
+	if err == nil {


NIt; generally, I would handle the case where err != nil in an if condition for the first thing that could return an error and then another if condition if the next thing could also return an error.

1. Add GetHostPort() and update unit tests #3570 2. Upudate dockerPortMap() in task.go with dynamic host port range support part 1 #3584 3. Upudate dockerPortMap() in task.go with dynamic host port range support part 2 #3589 4. Validate the host port/host port range found by ECS Agent before returning it #3589

1. Add GetHostPort() and update unit tests aws#3570 2. Upudate dockerPortMap() in task.go with dynamic host port range support part 1 aws#3584 3. Upudate dockerPortMap() in task.go with dynamic host port range support part 2 aws#3589 4. Validate the host port/host port range found by ECS Agent before returning it aws#3589 5. Refactor buildPortMapWithSCIngressConfig() in task.go aws#3600

1. Add GetHostPort() and update unit tests #3570 2. Upudate dockerPortMap() in task.go with dynamic host port range support part 1 #3584 3. Upudate dockerPortMap() in task.go with dynamic host port range support part 2 #3589 4. Validate the host port/host port range found by ECS Agent before returning it #3589 5. Refactor buildPortMapWithSCIngressConfig() in task.go #3600

chienhanlin added 2 commits February 24, 2023 17:10

Upudate dockerPortMap() in task.go with dynamic host port range suppo…

526f58d

…rt part 2

Validate the host port/host port range found by ECS Agent before retu…

cfdbaee

…rning it

chienhanlin commented Feb 24, 2023

View reviewed changes

chienhanlin added the bot/test label Feb 24, 2023

amazon-ecs-bot removed the bot/test label Feb 24, 2023

chienhanlin marked this pull request as ready for review February 24, 2023 17:58

chienhanlin requested a review from a team as a code owner February 24, 2023 17:58

chienhanlin commented Feb 24, 2023

View reviewed changes

mye956 reviewed Feb 24, 2023

View reviewed changes

SreeeS reviewed Feb 24, 2023

View reviewed changes

YashdalfTheGray approved these changes Feb 24, 2023

View reviewed changes

SreeeS approved these changes Feb 24, 2023

View reviewed changes

mye956 approved these changes Feb 24, 2023

View reviewed changes

chienhanlin merged commit 5b7cbda into aws:feature/dynamicHostPortAssignment Feb 24, 2023

chienhanlin deleted the dhpa-3 branch February 24, 2023 22:16

chienhanlin mentioned this pull request Mar 3, 2023

[DHPA] Refactor buildPortMapWithSCIngressConfig() in task.go #3600

Merged

chienhanlin mentioned this pull request Mar 4, 2023

ECS Agent dynamic host port assignment #3601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DHPA] Upudate dockerPortMap() in task.go with dynamic host port range support - part 2+ #3589

[DHPA] Upudate dockerPortMap() in task.go with dynamic host port range support - part 2+ #3589

chienhanlin commented Feb 24, 2023 •

edited

Loading

chienhanlin Feb 24, 2023

chienhanlin Feb 24, 2023

chienhanlin Feb 24, 2023

chienhanlin Feb 24, 2023

chienhanlin Feb 24, 2023

chienhanlin Feb 24, 2023

chienhanlin Feb 24, 2023

chienhanlin Feb 24, 2023

chienhanlin Feb 24, 2023

chienhanlin Feb 24, 2023

chienhanlin Feb 24, 2023

chienhanlin Feb 24, 2023

chienhanlin Feb 24, 2023

YashdalfTheGray Feb 24, 2023

chienhanlin Feb 24, 2023

mye956 Feb 24, 2023

chienhanlin Feb 24, 2023

mye956 Feb 24, 2023

chienhanlin Feb 24, 2023 •

edited

Loading

SreeeS Feb 24, 2023

chienhanlin Feb 24, 2023

YashdalfTheGray left a comment

YashdalfTheGray Feb 24, 2023

[DHPA] Upudate dockerPortMap() in task.go with dynamic host port range support - part 2+ #3589

[DHPA] Upudate dockerPortMap() in task.go with dynamic host port range support - part 2+ #3589

Conversation

chienhanlin commented Feb 24, 2023 • edited Loading

Summary

Implementation details

Testing

Unit test

TestGetHostPortRange

TestGetHostPort

TestResetTracker

TestDockerHostConfigPortBinding

TestDockerHostConfigSCBridgeMode

Manual testing

Description for the changelog

Related PRs

Licensing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chienhanlin Feb 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YashdalfTheGray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chienhanlin commented Feb 24, 2023 •

edited

Loading

chienhanlin Feb 24, 2023 •

edited

Loading