Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from
Feb 24, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 49 additions & 10 deletions agent/api/task/task.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2338,8 +2338,21 @@ func (task *Task) dockerLinks(container *apicontainer.Container, dockerContainer
}

var getHostPortRange = utils.GetHostPortRange
var getHostPort = utils.GetHostPort

// dockerPortMap creates a port binding map for
// (1) Ingress listeners for the service connect AppNet container in the service connect enabled bridge network mode task.
// (2) Port mapping definied by customers in the task definition.
mye956 marked this conversation as resolved.
Show resolved Hide resolved
//
// 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,
// (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) {
hostPortStr := ""
dockerPortMap := nat.PortMap{}
scContainer := task.GetServiceConnectContainer()
containerToCheck := container
Expand Down Expand Up @@ -2382,15 +2395,41 @@ func (task *Task) dockerPortMap(container *apicontainer.Container, dynamicHostPo
}
}

// For each port binding config, either one of containerPort or containerPortRange is set.
// (1) containerPort is the port number on the container that's bound to the user-specified host port or the host port assigned by ECS Agent.
// (2) containerPortRange is the port number range on the container that's bound to the mapped host port range found by ECS Agent.
var err error
for _, portBinding := range containerToCheck.Ports {
// for each port binding config, either one of containerPort or containerPortRange is set
if portBinding.ContainerPort != 0 {
containerPort := int(portBinding.ContainerPort)
protocolStr := portBinding.Protocol.String()
dockerPort := nat.Port(strconv.Itoa(containerPort) + "/" + protocolStr)

dockerPort := nat.Port(strconv.Itoa(containerPort) + "/" + portBinding.Protocol.String())
dockerPortMap[dockerPort] = append(dockerPortMap[dockerPort], nat.PortBinding{HostPort: strconv.Itoa(int(portBinding.HostPort))})
if portBinding.HostPort != 0 {
// An User-specified host port exists
hostPortStr = strconv.Itoa(int(portBinding.HostPort))
fierlion marked this conversation as resolved.
Show resolved Hide resolved
} else {
// If there is no user-specified host port, ECS Agent will find an available host port
// within the given dynamic host port range. And if no host port is available within the range,
// an error will be returned.
logger.Debug("No user-specified host port, ECS Agent will find an available host port within the given dynamic host port range", logger.Fields{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An example ecs-agent.log for line 2415 - 2427

level=info time=xxx msg="Creating container" task=“xxx” container="containerPort"
level=debug time=xxx msg="No user-specified host port, ECS Agent will find an available host port within the given dynamic host port range" container="containerPort" dynamicHostPortRange="50000-50001"
level=debug time=xxx msg="Port 50000 is unavailable or an error occurred while listening on the local tcp network" module=ephemeral_ports.go
level=debug time=xxx msg="Port 50001 is unavailable or an error occurred while listening on the local tcp network" module=ephemeral_ports.go
level=error time=xxx msg="Unable to find a host port for container within the given dynamic host port range" task=“xxx” container="containerPort" dynamicHostPortRange="50000-50001" error="a host port is unavailable"
level=error time=xxx msg="Error transitioning container" runtimeID="" nextState="CREATED" error="error retrieving docker port map: a host port is unavailable" task=“xxx” container="containerPort"
level=info time=xxx msg="Handling container change event" status="CREATED" task=“xxx” container="containerPort" runtimeID=""
level=warn time=xxx msg="Error creating container; marking its desired status as STOPPED" task=“xxx” container="containerPort" error="error retrieving docker port map: a host port is unavailable"
…
level=debug time=xxx msg="Submitted state change to ECS" eventType="task" eventData="TaskChange: [xxx -> STOPPED, Known Sent: STOPPED, PullStartedAt: xxx, PullStoppedAt: xxx, ExecutionStoppedAt: xxx, container change: xxx containerPort -> STOPPED, Reason HostConfigError: error retrieving docker port map: a host port is unavailable, Known Sent: STOPPED] sent: true"

field.Container: containerToCheck.Name,
"dynamicHostPortRange": dynamicHostPortRange,
})
hostPortStr, err = getHostPort(protocolStr, dynamicHostPortRange)
if err != nil {
logger.Error("Unable to find a host port for container within the given dynamic host port range", logger.Fields{
field.TaskID: task.GetID(),
field.Container: container.Name,
"dynamicHostPortRange": dynamicHostPortRange,
field.Error: err,
})
return nil, err
}
}
dockerPortMap[dockerPort] = append(dockerPortMap[dockerPort], nat.PortBinding{HostPort: hostPortStr})

// append non-range, singular container port to the containerPortSet
// For the containerPort case, append a non-range, singular container port to the containerPortSet.
containerPortSet[containerPort] = struct{}{}
} else if portBinding.ContainerPortRange != "" {
containerToCheck.SetContainerHasPortRange(true)
Expand All @@ -2404,8 +2443,8 @@ func (task *Task) dockerPortMap(container *apicontainer.Container, dynamicHostPo

numberOfPorts := endContainerPort - startContainerPort + 1
protocol := portBinding.Protocol.String()
// we will try to get a contiguous set of host ports from the ephemeral host port range.
// this is to ensure that docker maps host ports in a contiguous manner, and
// We will try to get a contiguous set of host ports from the ephemeral host port range.
// This is to ensure that docker maps host ports in a contiguous manner, and
// we are guaranteed to have the entire hostPortRange in a single network binding while sending this info to ECS;
// therefore, an error will be returned if we cannot find a contiguous set of host ports.
hostPortRange, err := getHostPortRange(numberOfPorts, protocol, dynamicHostPortRange)
Expand All @@ -2419,7 +2458,7 @@ func (task *Task) dockerPortMap(container *apicontainer.Container, dynamicHostPo
return nil, err
}

// append ranges to the dockerPortMap
// For the ContainerPortRange case, append ranges to the dockerPortMap.
// nat.ParsePortSpec returns a list of port mappings in a format that docker likes
mappings, err := nat.ParsePortSpec(hostPortRange + ":" + containerPortRange + "/" + protocol)
if err != nil {
fierlion marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -2430,13 +2469,13 @@ func (task *Task) dockerPortMap(container *apicontainer.Container, dynamicHostPo
dockerPortMap[mapping.Port] = append(dockerPortMap[mapping.Port], mapping.Binding)
}

// append containerPortRange and associated hostPortRange to the containerPortRangeMap
// this will ensure that we consolidate range into 1 network binding while sending it to ECS
// For the ContainerPortRange case, append containerPortRange and associated hostPortRange to the containerPortRangeMap.
// This will ensure that we consolidate range into 1 network binding while sending it to ECS
containerPortRangeMap[containerPortRange] = hostPortRange
}
}

// set Container.ContainerPortSet and Container.ContainerPortRangeMap to be used during network binding creation
// Set Container.ContainerPortSet and Container.ContainerPortRangeMap to be used during network binding creation
containerToCheck.SetContainerPortSet(containerPortSet)
containerToCheck.SetContainerPortRangeMap(containerPortRangeMap)
return dockerPortMap, nil
Expand Down
135 changes: 103 additions & 32 deletions agent/api/task/task_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,13 +240,13 @@ func TestDockerHostConfigPortBinding(t *testing.T) {
Ports: []apicontainer.PortBinding{
{
ContainerPort: 10,
HostPort: 10,
HostPort: 20,
BindIP: "",
Protocol: apicontainer.TransportProtocolTCP,
},
{
ContainerPort: 20,
HostPort: 20,
HostPort: 30,
BindIP: "",
Protocol: apicontainer.TransportProtocolUDP,
},
Expand All @@ -256,6 +256,31 @@ func TestDockerHostConfigPortBinding(t *testing.T) {
}

testTask2 := &Task{
Containers: []*apicontainer.Container{
{
Name: "c1",
Ports: []apicontainer.PortBinding{
{
ContainerPort: 10,
BindIP: "",
Protocol: apicontainer.TransportProtocolTCP,
},
{
ContainerPort: 20,
BindIP: "",
Protocol: apicontainer.TransportProtocolUDP,
},
{
ContainerPort: 30,
BindIP: "",
Protocol: apicontainer.TransportProtocolUDP,
},
},
},
},
}

testTask3 := &Task{
Containers: []*apicontainer.Container{
{
Name: "c1",
Expand All @@ -278,17 +303,20 @@ func TestDockerHostConfigPortBinding(t *testing.T) {
testCases := []struct {
testName string
testTask *Task
getHostPortRange func(numberOfPorts int, protocol string, dynamicHostPortRange string) (string, error)
testDynamicHostPortRange string
testContainerPortRange string
expectedPortBinding nat.PortMap
expectedContainerPortSet map[int]struct{}
expectedContainerPortRangeMap map[string]string
expectedError bool
}{
{
testName: "2 port bindings, each with singular container port - host port",
testTask: testTask1,
testName: "user-specified container ports and host ports",
testTask: testTask1,
testDynamicHostPortRange: "40000-60000",
expectedPortBinding: nat.PortMap{
nat.Port("10/tcp"): []nat.PortBinding{{HostPort: "10"}},
nat.Port("20/udp"): []nat.PortBinding{{HostPort: "20"}},
nat.Port("10/tcp"): []nat.PortBinding{{HostPort: "20"}},
nat.Port("20/udp"): []nat.PortBinding{{HostPort: "30"}},
},
expectedContainerPortSet: map[int]struct{}{
10: {},
Expand All @@ -297,23 +325,37 @@ func TestDockerHostConfigPortBinding(t *testing.T) {
expectedContainerPortRangeMap: map[string]string{},
},
{
testName: "2 port bindings, one with container port range, other with singular container port",
testTask: testTask2,
getHostPortRange: func(numberOfPorts int, protocol string, dynamicHostPortRange string) (string, error) {
return "155-157", nil
},
expectedPortBinding: nat.PortMap{
nat.Port("55/udp"): []nat.PortBinding{{HostPort: "155"}},
nat.Port("56/udp"): []nat.PortBinding{{HostPort: "156"}},
nat.Port("57/udp"): []nat.PortBinding{{HostPort: "157"}},
nat.Port("80/tcp"): []nat.PortBinding{{HostPort: "0"}},
testName: "user-specified container ports with a ideal dynamicHostPortRange",
testTask: testTask2,
testDynamicHostPortRange: "40000-60000",
expectedContainerPortSet: map[int]struct{}{
10: {},
20: {},
30: {},
},
expectedContainerPortRangeMap: map[string]string{},
},
{
testName: "user-specified container ports with a bad dynamicHostPortRange",
testTask: testTask2,
testDynamicHostPortRange: "100-101",
expectedError: true,
},
{
testName: "user-specified container port and container port range with a ideal dynamicHostPortRange",
testTask: testTask3,
testDynamicHostPortRange: "40000-60000",
testContainerPortRange: "55-57",
expectedContainerPortSet: map[int]struct{}{
80: {},
},
expectedContainerPortRangeMap: map[string]string{
"55-57": "155-157",
},
},
{
testName: "user-specified container port and container port range with a bad user-specified dynamicHostPortRange",
testTask: testTask3,
testDynamicHostPortRange: "40000-40001",
testContainerPortRange: "55-57",
expectedError: true,
},
}

Expand All @@ -322,22 +364,51 @@ func TestDockerHostConfigPortBinding(t *testing.T) {
defer func() {
getHostPortRange = utils.GetHostPortRange
}()
getHostPortRange = tc.getHostPortRange

config, err := tc.testTask.DockerHostConfig(tc.testTask.Containers[0], dockerMap(tc.testTask), defaultDockerClientAPIVersion,
&config.Config{})
assert.Nil(t, err)
// Get the Docker host config for the task container
config, err := tc.testTask.DockerHostConfig(tc.testTask.Containers[0], dockerMap(tc.testTask),
defaultDockerClientAPIVersion, &config.Config{DynamicHostPortRange: tc.testDynamicHostPortRange})
if !tc.expectedError {
assert.Nil(t, err)

if !reflect.DeepEqual(config.PortBindings, tc.expectedPortBinding) {
t.Error("Expected port bindings to be resolved, was: ", config.PortBindings)
}
// Verify PortBindings
if tc.expectedPortBinding != nil {
if !reflect.DeepEqual(config.PortBindings, tc.expectedPortBinding) {
t.Error("Expected port bindings to be resolved, was: ", config.PortBindings)
}
} else {
// Verify ECS Agent assigned host ports are within the dynamic host port range
eStartPort, eEndPort, _ := nat.ParsePortRangeToInt(tc.testDynamicHostPortRange)
for _, hostPortBinding := range config.PortBindings {
hostPort, _ := strconv.Atoi(hostPortBinding[0].HostPort)
result := utils.PortIsInRange(hostPort, eStartPort, eEndPort)
if !result {
t.Error("Actual host port is not in the dynamicHostPortRange: ", hostPort)
break
}
}
}

if !reflect.DeepEqual(tc.testTask.Containers[0].ContainerPortSet, tc.expectedContainerPortSet) {
t.Error("Expected container port set to be resolved, was: ", tc.testTask.Containers[0].GetContainerPortSet())
}
// Verify ContainerPortSet
if !reflect.DeepEqual(tc.testTask.Containers[0].ContainerPortSet, tc.expectedContainerPortSet) {
t.Error("Expected container port set to be resolved, was: ", tc.testTask.Containers[0].GetContainerPortSet())
}

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())
// Verify ContainerPortRangeMap
if tc.expectedContainerPortRangeMap != nil {
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
hostPortRange := tc.testTask.Containers[0].ContainerPortRangeMap[tc.testContainerPortRange]
result := utils.VerifyPortsWithinRange(hostPortRange, tc.testDynamicHostPortRange)
if !result {
t.Error("Expected host port range should be in the dynamicHostPortRange, but the actual host port range is: ", hostPortRange)
}
}
} else {
assert.NotNil(t, err)
}
})
}
Expand Down
2 changes: 2 additions & 0 deletions agent/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,7 @@ func (cfg *Config) String() string {
"DependentContainersPullUpfront: %v, "+
"TaskCPUMemLimit: %v, "+
"ShouldExcludeIPv6PortBinding: %v, "+
"DynamicHostPortRange: %v"+
"%s",
cfg.Cluster,
cfg.AWSRegion,
Expand All @@ -648,6 +649,7 @@ func (cfg *Config) String() string {
cfg.DependentContainersPullUpfront,
cfg.TaskCPUMemLimit,
cfg.ShouldExcludeIPv6PortBinding,
cfg.DynamicHostPortRange,
cfg.platformString(),
)
}
29 changes: 29 additions & 0 deletions agent/utils/ephemeral_ports.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,3 +214,32 @@ func getHostPortRange(numberOfPorts, start, end int, protocol string) (string, i

return fmt.Sprintf("%d-%d", resultStartPort, resultEndPort), resultEndPort, nil
}

// PortIsInRange returns true if the given port is within the start-end port range;
// otherwise, returns false.
func PortIsInRange(port, start, end int) bool {
if (port >= start) && (port <= end) {
return true
}
return false
}

// VerifyPortsWithinRange returns true if the actualPortRange is within the expectedPortRange;
// otherwise, returns false.
func VerifyPortsWithinRange(actualPortRange, expectedPortRange string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QQ. Is this method used anywhere other than in task_test.go ? If not, could you move it to the test file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also do we verify that a port is within range in our taskdef validation or elsewhere? Where is this same validation done outside of our tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QQ. Is this method used anywhere other than in task_test.go ? If not, could you move it to the test file?

Also do we verify that a port is within range in our taskdef validation or elsewhere? Where is this same validation done outside of our tests?

This method is used in both agent/utils/ephemeral_ports_test.go and agent/api/task/task_test.go in this PR. As it's using in different packages, I have to make it as a public method.

And as fierlion@ suggested, we should validate a host port or a host port range assigned by ECS Agent after calling getHostPort() or getHostPortRange(). Therefore, I would like to keep VerifyPortsWithinRange() in agent/utils/ephemeral_ports.go, and have a follow up PR to implement host port validations outside of our tests.

Copy link
Contributor Author

@chienhanlin chienhanlin Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are made in https://github.com/aws/amazon-ecs-agent/pull/3589/files#diff-d475c5ecd1be9e633e4fde242318c1369112ce30a3a122288779c5cc277a7b93.

// Get the actual start port and end port
aStartPort, aEndPort, _ := nat.ParsePortRangeToInt(actualPortRange)
// Get the expected start port and end port
eStartPort, eEndPort, _ := nat.ParsePortRangeToInt(expectedPortRange)
// Check the actual start port is in the expected range or not
aStartIsInRange := PortIsInRange(aStartPort, eStartPort, eEndPort)
// Check the actual end port is in the expected range or not
aEndIsInRange := PortIsInRange(aEndPort, eStartPort, eEndPort)

// Return true if both actual start port and end port are in the expected range
if aStartIsInRange && aEndIsInRange {
return true
}

return false
}
Loading