From 526f58d51072d04c414f6bfad4f8384905687104 Mon Sep 17 00:00:00 2001 From: Chien-Han Lin Date: Tue, 21 Feb 2023 08:24:27 +0000 Subject: [PATCH 1/2] Upudate dockerPortMap() in task.go with dynamic host port range support part 2 --- agent/api/task/task.go | 99 ++++++++++++++---- agent/api/task/task_test.go | 149 ++++++++++++++++++---------- agent/utils/ephemeral_ports.go | 5 + agent/utils/ephemeral_ports_test.go | 13 ++- 4 files changed, 191 insertions(+), 75 deletions(-) diff --git a/agent/api/task/task.go b/agent/api/task/task.go index adeaab04962..996394fcf66 100644 --- a/agent/api/task/task.go +++ b/agent/api/task/task.go @@ -2340,10 +2340,53 @@ func (task *Task) dockerLinks(container *apicontainer.Container, dockerContainer var getHostPortRange = utils.GetHostPortRange var getHostPort = utils.GetHostPort +// In buildPortMapWithSCIngressConfig, the dockerPortMap and the containerPortSet will be constructed +// for ingress listeners under two service connect bridge mode cases: +// (1) non-default bridge mode service connect experience: customers specify host ports for listeners in the ingress config. +// (2) default bridge mode service connect experience: customers do not specify host ports for listeners in the ingress config. +// 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) { + var err error + ingressDockerPortMap := nat.PortMap{} + ingressContainerPortSet := make(map[int]struct{}) + protocolStr := "tcp" + for _, ic := range task.ServiceConnectConfig.IngressConfig { + listenerPortInt := int(ic.ListenerPort) + dockerPort := nat.Port(strconv.Itoa(listenerPortInt) + "/" + protocolStr) + hostPortStr := "" + if ic.HostPort != nil { + // For non-default bridge mode service connect experience, a host port is specified by customers + hostPortStr = strconv.Itoa(int(*ic.HostPort)) + } else { + // For default bridge mode service connect experience, customers do not specify a host port + // thus the host port will be assigned by ECS Agent. + // ECS Agent will find an available host port within the given dynamic host port range, + // 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 + } + } + + ingressDockerPortMap[dockerPort] = append(ingressDockerPortMap[dockerPort], nat.PortBinding{HostPort: hostPortStr}) + // Append non-range, singular container port to the ingressContainerPortSet + ingressContainerPortSet[listenerPortInt] = struct{}{} + } + return ingressDockerPortMap, ingressContainerPortSet, err +} + // 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. // +// For service connect bridge mode task, we will create port bindings for customers' application containers +// and service connect AppNet container, and let them to be published by the associated pause containers. +// (a) For default bridge service connect experience, ECS Agent will assign a host port within the +// default/user-specified dynamic host port range for the ingress listener. If no available host port can be +// found by ECS Agent, an error will be returned. +// (b) For non-default bridge service connect experience, ECS Agent will use the user-defined host port for the ingress listener. +// // 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. @@ -2354,43 +2397,49 @@ var getHostPort = utils.GetHostPort func (task *Task) dockerPortMap(container *apicontainer.Container, dynamicHostPortRange string) (nat.PortMap, error) { hostPortStr := "" dockerPortMap := nat.PortMap{} - scContainer := task.GetServiceConnectContainer() containerToCheck := container containerPortSet := make(map[int]struct{}) containerPortRangeMap := make(map[string]string) + + // For service connect bridge network mode task, we will create port bindings for task containers, + // including both application containers and service connect AppNet container, and let them to be published + // by the associated pause containers. if task.IsServiceConnectEnabled() && task.IsNetworkModeBridge() { if container.Type == apicontainer.ContainerCNIPause { - // we will create bindings for task containers (including both customer containers and SC Appnet container) - // 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 } + + scContainer := task.GetServiceConnectContainer() if taskContainer == scContainer { - // create bindings for all ingress listener ports - // no need to create binding for egress listener port as it won't be access from host level or from outside - for _, ic := range task.ServiceConnectConfig.IngressConfig { - listenerPortInt := int(ic.ListenerPort) - dockerPort := nat.Port(strconv.Itoa(listenerPortInt)) + "/tcp" - hostPort := 0 // default bridge-mode SC experience - host port will be an ephemeral port assigned by docker - if ic.HostPort != nil { // non-default bridge-mode SC experience - host port specified by customer - hostPort = int(*ic.HostPort) - } - dockerPortMap[dockerPort] = append(dockerPortMap[dockerPort], nat.PortBinding{HostPort: strconv.Itoa(hostPort)}) - // append non-range, singular container port to the containerPortSet - containerPortSet[listenerPortInt] = struct{}{} - // set taskContainer.ContainerPortSet to be used during network binding creation - taskContainer.SetContainerPortSet(containerPortSet) + // If the associated task container is the service connect AppNet container, + // create port binding(s) for ingress listener ports based on its ingress config. + // Note that, there is no need to do this for egress listener port as it won't be accessed + // from host level or from outside. + dockerPortMap, containerPortSet, err := task.buildPortMapWithSCIngressConfig(dynamicHostPortRange) + if err != nil { + logger.Error("Failed to build a port map with service connect ingress config", logger.Fields{ + field.TaskID: task.GetID(), + field.Container: taskContainer.Name, + "dynamicHostPortRange": dynamicHostPortRange, + field.Error: err, + }) + return nil, err } + // Set taskContainer.ContainerPortSet to be used during network binding creation + taskContainer.SetContainerPortSet(containerPortSet) return dockerPortMap, nil } containerToCheck = taskContainer } else { - // 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 neither service connect AppNet container nor pause container, and it is a regular task container. + // Its port bindings(s) are published by the associated pause container, and we will leave the map empty here + // (docker would actually complain otherwise). + // + // 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. return dockerPortMap, nil } } @@ -2478,6 +2527,12 @@ func (task *Task) dockerPortMap(container *apicontainer.Container, dynamicHostPo // Set Container.ContainerPortSet and Container.ContainerPortRangeMap to be used during network binding creation containerToCheck.SetContainerPortSet(containerPortSet) containerToCheck.SetContainerPortRangeMap(containerPortRangeMap) + logger.Debug("containerToCheck is", logger.Fields{ + field.Container: containerToCheck.Name, + "dockerPortMap": dockerPortMap, + "containerPortSet": containerPortSet, + "containerPortRangeMap": containerPortRangeMap, + }) return dockerPortMap, nil } diff --git a/agent/api/task/task_test.go b/agent/api/task/task_test.go index cc70a648c77..eef53aeb2e7 100644 --- a/agent/api/task/task_test.go +++ b/agent/api/task/task_test.go @@ -424,6 +424,14 @@ var ( defaultSCProtocol = "/tcp" ) +func getDefaultDynamicHostPortRange() (start int, end int) { + startHostPortRange, endHostPortRange, err := utils.GetDynamicHostPortRange() + if err != nil { + return utils.DefaultPortRangeStart, utils.DefaultPortRangeEnd + } + return startHostPortRange, endHostPortRange +} + func getTestTaskServiceConnectBridgeMode() *Task { testTask := &Task{ NetworkMode: BridgeNetworkMode, @@ -479,61 +487,100 @@ 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) { testTask := getTestTaskServiceConnectBridgeMode() - // task container and SC container should both get empty port binding map and "container" network mode - actualConfig, err := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), defaultDockerClientAPIVersion, - &config.Config{}) - assert.Nil(t, err) - assert.NotNil(t, actualConfig) - assert.Equal(t, dockercontainer.NetworkMode(fmt.Sprintf("%s-%s", // e.g. "container:dockerid-~internal~ecs~pause-C1" - dockerMappingContainerPrefix+dockerIDPrefix+NetworkPauseContainerName, "C1")), actualConfig.NetworkMode) - assert.Empty(t, actualConfig.PortBindings, "Task container port binding should be empty") - - actualConfig, err = testTask.DockerHostConfig(testTask.Containers[2], dockerMap(testTask), defaultDockerClientAPIVersion, - &config.Config{}) - assert.Nil(t, err) - assert.NotNil(t, actualConfig) - assert.Equal(t, dockercontainer.NetworkMode(fmt.Sprintf("%s-%s", // e.g. "container:dockerid-~internal~ecs~pause-C1" - dockerMappingContainerPrefix+dockerIDPrefix+NetworkPauseContainerName, serviceConnectContainerTestName)), actualConfig.NetworkMode) - assert.Empty(t, actualConfig.PortBindings, "SC container port binding should be empty") + testCases := []struct { + testStartHostPort int + testEndHostPort int + testName string + testError bool + }{ + { + testStartHostPort: 0, + testEndHostPort: 0, + testName: "with default dynamic host port range", + }, + { + testStartHostPort: 50000, + testEndHostPort: 60000, + testName: "with user-specified dynamic host port range", + }, + } - // task pause container should get port binding map of the task container - actualConfig, err = testTask.DockerHostConfig(testTask.Containers[1], dockerMap(testTask), defaultDockerClientAPIVersion, - &config.Config{}) - assert.Nil(t, err) - assert.NotNil(t, actualConfig) - assert.Equal(t, dockercontainer.NetworkMode(BridgeNetworkMode), actualConfig.NetworkMode) - bindings, ok := actualConfig.PortBindings[convertSCPort(SCTaskContainerPort1)] - assert.True(t, ok, "Could not get port bindings") - assert.Equal(t, 1, len(bindings), "Wrong number of bindings") - assert.Equal(t, "0", bindings[0].HostPort, "Wrong hostport") - bindings, ok = actualConfig.PortBindings[convertSCPort(SCTaskContainerPort2)] - assert.True(t, ok, "Could not get port bindings") - assert.Equal(t, 1, len(bindings), "Wrong number of bindings") - assert.Equal(t, "0", bindings[0].HostPort, "Wrong hostport") - - // SC pause container should get port binding map of all ingress listeners - actualConfig, err = testTask.DockerHostConfig(testTask.Containers[3], dockerMap(testTask), defaultDockerClientAPIVersion, - &config.Config{}) - assert.Nil(t, err) - assert.NotNil(t, actualConfig) - assert.Equal(t, dockercontainer.NetworkMode(BridgeNetworkMode), actualConfig.NetworkMode) - // SC - ingress listener 1 - default experience - bindings, ok = actualConfig.PortBindings[convertSCPort(SCIngressListener1ContainerPort)] - assert.True(t, ok, "Could not get port bindings") - assert.Equal(t, 1, len(bindings), "Wrong number of bindings") - assert.Equal(t, "0", bindings[0].HostPort, "Wrong hostport") - // SC - ingress listener 2 - non-default host port - bindings, ok = actualConfig.PortBindings[convertSCPort(SCIngressListener2ContainerPort)] - assert.True(t, ok, "Could not get port bindings") - assert.Equal(t, 1, len(bindings), "Wrong number of bindings") - assert.Equal(t, strconv.Itoa(int(SCIngressListener2HostPort)), bindings[0].HostPort, "Wrong hostport") - // SC - egress listener - should not have port binding - bindings, ok = actualConfig.PortBindings[convertSCPort(SCEgressListenerContainerPort)] - assert.False(t, ok, "egress listener has port binding but it shouldn't") + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + // need to reset the tracker to avoid getting data from previous test cases + utils.ResetTracker() + if tc.testStartHostPort == 0 && tc.testEndHostPort == 0 { + tc.testStartHostPort, tc.testEndHostPort = getDefaultDynamicHostPortRange() + } + testDynamicHostPortRange := fmt.Sprintf("%d-%d", tc.testStartHostPort, tc.testEndHostPort) + testConfig := &config.Config{DynamicHostPortRange: testDynamicHostPortRange} + + // task container and SC container should both get empty port binding map and "container" network mode + + // the task container + actualConfig, err := testTask.DockerHostConfig(testTask.Containers[0], dockerMap(testTask), defaultDockerClientAPIVersion, testConfig) + assert.Nil(t, err) + assert.NotNil(t, actualConfig) + assert.Equal(t, dockercontainer.NetworkMode(fmt.Sprintf("%s-%s", // e.g. "container:dockerid-~internal~ecs~pause-C1" + dockerMappingContainerPrefix+dockerIDPrefix+NetworkPauseContainerName, "C1")), actualConfig.NetworkMode) + assert.Empty(t, actualConfig.PortBindings, "Task container port binding should be empty") + + // the service connect container + actualConfig, err = testTask.DockerHostConfig(testTask.Containers[2], dockerMap(testTask), defaultDockerClientAPIVersion, testConfig) + assert.Nil(t, err) + assert.NotNil(t, actualConfig) + assert.Equal(t, dockercontainer.NetworkMode(fmt.Sprintf("%s-%s", // e.g. "container:dockerid-~internal~ecs~pause-C1" + dockerMappingContainerPrefix+dockerIDPrefix+NetworkPauseContainerName, serviceConnectContainerTestName)), actualConfig.NetworkMode) + assert.Empty(t, actualConfig.PortBindings, "SC container port binding should be empty") + + // task pause container should get port binding map of the task container + actualConfig, err = testTask.DockerHostConfig(testTask.Containers[1], dockerMap(testTask), defaultDockerClientAPIVersion, testConfig) + assert.Nil(t, err) + assert.NotNil(t, actualConfig) + assert.Equal(t, dockercontainer.NetworkMode(BridgeNetworkMode), actualConfig.NetworkMode) + bindings, ok := actualConfig.PortBindings[convertSCPort(SCTaskContainerPort1)] + assert.True(t, ok, "Could not get port bindings") + assert.Equal(t, 1, len(bindings), "Wrong number of bindings") + hostPort, _ := strconv.Atoi(bindings[0].HostPort) + result := utils.PortIsInRange(hostPort, tc.testStartHostPort, tc.testEndHostPort) + assert.True(t, result, "hostport is not in the dynamic host port range") + + bindings, ok = actualConfig.PortBindings[convertSCPort(SCTaskContainerPort2)] + assert.True(t, ok, "Could not get port bindings") + assert.Equal(t, 1, len(bindings), "Wrong number of bindings") + hostPort, _ = strconv.Atoi(bindings[0].HostPort) + result = utils.PortIsInRange(hostPort, tc.testStartHostPort, tc.testEndHostPort) + assert.True(t, result, "hostport is not in the dynamic host port range") + + // SC pause container should get port binding map of all ingress listeners + actualConfig, err = testTask.DockerHostConfig(testTask.Containers[3], dockerMap(testTask), defaultDockerClientAPIVersion, testConfig) + assert.Nil(t, err) + assert.NotNil(t, actualConfig) + assert.Equal(t, dockercontainer.NetworkMode(BridgeNetworkMode), actualConfig.NetworkMode) + + // SC - ingress listener 1 - default experience + bindings, ok = actualConfig.PortBindings[convertSCPort(SCIngressListener1ContainerPort)] + assert.True(t, ok, "Could not get port bindings") + hostPort, _ = strconv.Atoi(bindings[0].HostPort) + result = utils.PortIsInRange(hostPort, tc.testStartHostPort, tc.testEndHostPort) + assert.True(t, result, "hostport is not in the dynamic host port range") + + // SC - ingress listener 2 - non-default host port + bindings, ok = actualConfig.PortBindings[convertSCPort(SCIngressListener2ContainerPort)] + assert.True(t, ok, "Could not get port bindings") + assert.Equal(t, 1, len(bindings), "Wrong number of bindings") + assert.Equal(t, strconv.Itoa(int(SCIngressListener2HostPort)), bindings[0].HostPort, "Wrong hostport") + + // SC - egress listener - should not have port binding + bindings, ok = actualConfig.PortBindings[convertSCPort(SCEgressListenerContainerPort)] + assert.False(t, ok, "egress listener has port binding but it shouldn't") + }) + } } // TestDockerHostConfigSCBridgeMode_getPortBindingFailure verifies that when we can't find the task diff --git a/agent/utils/ephemeral_ports.go b/agent/utils/ephemeral_ports.go index efcf8477466..40a90969939 100644 --- a/agent/utils/ephemeral_ports.go +++ b/agent/utils/ephemeral_ports.go @@ -96,6 +96,11 @@ func (pt *safePortTracker) GetLastAssignedHostPort() int { var tracker safePortTracker +// ResetTracker resets the last assigned host port to 0. +func ResetTracker() { + tracker.SetLastAssignedHostPort(0) +} + // 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. diff --git a/agent/utils/ephemeral_ports_test.go b/agent/utils/ephemeral_ports_test.go index 211474d98ce..404c8fe53ea 100644 --- a/agent/utils/ephemeral_ports_test.go +++ b/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() hostPortRange, err := GetHostPortRange(tc.numberOfPorts, tc.protocol, tc.testDynamicHostPortRange) assert.Equal(t, tc.expectedError, err) @@ -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() } t.Run(tc.testName, func(t *testing.T) { @@ -275,6 +276,14 @@ func TestVerifyPortsWithinRange(t *testing.T) { } } +func TestResetTracker(t *testing.T) { + tracker.SetLastAssignedHostPort(100) + ResetTracker() + expectedResetVal := 0 + actualResult := tracker.GetLastAssignedHostPort() + assert.Equal(t, expectedResetVal, actualResult) +} + func getPortRangeLength(portRange string) (int, error) { startPort, endPort, err := nat.ParsePortRangeToInt(portRange) if err != nil { From cfdbaee7320e92dbe0feb9135b33339f2e7ecfb4 Mon Sep 17 00:00:00 2001 From: Chien-Han Lin Date: Fri, 24 Feb 2023 00:15:11 +0000 Subject: [PATCH 2/2] Validate the host port/host port range found by ECS Agent before returning it --- agent/api/task/task.go | 54 +++++++++++++++-------------- agent/api/task/task_test.go | 27 --------------- agent/utils/ephemeral_ports.go | 28 +++++++++++---- agent/utils/ephemeral_ports_test.go | 7 ++-- 4 files changed, 51 insertions(+), 65 deletions(-) diff --git a/agent/api/task/task.go b/agent/api/task/task.go index 996394fcf66..731e71572cc 100644 --- a/agent/api/task/task.go +++ b/agent/api/task/task.go @@ -2344,8 +2344,9 @@ var getHostPort = utils.GetHostPort // for ingress listeners under two service connect bridge mode cases: // (1) non-default bridge mode service connect experience: customers specify host ports for listeners in the ingress config. // (2) default bridge mode service connect experience: customers do not specify host ports for listeners in the ingress config. -// 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. +// +// 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) { var err error ingressDockerPortMap := nat.PortMap{} @@ -2357,6 +2358,9 @@ func (task *Task) buildPortMapWithSCIngressConfig(dynamicHostPortRange string) ( hostPortStr := "" if ic.HostPort != nil { // For non-default bridge mode service connect experience, a host port is specified by customers + // Note that service connect ingress config has been validated in service_connect_validator.go, + // where host ports will be validated to ensure user-definied ports are within a valid port range (1 to 65535) + // 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 @@ -2377,8 +2381,8 @@ func (task *Task) buildPortMapWithSCIngressConfig(dynamicHostPortRange string) ( } // 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. +// (1) Ingress listeners for the service connect AppNet container in the service connect bridge network mode task. +// (2) Port mapping configured by customers in the task definition. // // For service connect bridge mode task, we will create port bindings for customers' application containers // and service connect AppNet container, and let them to be published by the associated pause containers. @@ -2391,7 +2395,7 @@ func (task *Task) buildPortMapWithSCIngressConfig(dynamicHostPortRange string) ( // 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) { @@ -2414,9 +2418,9 @@ func (task *Task) dockerPortMap(container *apicontainer.Container, dynamicHostPo scContainer := task.GetServiceConnectContainer() if taskContainer == scContainer { - // If the associated task container is the service connect AppNet container, + // If the associated task container to this pause container is the service connect AppNet container, // create port binding(s) for ingress listener ports based on its ingress config. - // Note that, there is no need to do this for egress listener port as it won't be accessed + // Note that there is no need to do this for egress listener ports as they won't be accessed // from host level or from outside. dockerPortMap, containerPortSet, err := task.buildPortMapWithSCIngressConfig(dynamicHostPortRange) if err != nil { @@ -2432,21 +2436,23 @@ func (task *Task) dockerPortMap(container *apicontainer.Container, dynamicHostPo taskContainer.SetContainerPortSet(containerPortSet) 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. containerToCheck = taskContainer } else { - // If the container is neither service connect AppNet container nor pause container, and it is a regular task container. - // Its port bindings(s) are published by the associated pause container, and we will leave the map empty here - // (docker would actually complain otherwise). - // - // 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. + // If the container is not a pause container, then it is a regular customers' application container + // or a service connect AppNet container. We will leave the map empty and return it as its port bindings(s) + // are published by the associated pause container. return dockerPortMap, nil } } // 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. + // (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 { if portBinding.ContainerPort != 0 { @@ -2455,7 +2461,9 @@ func (task *Task) dockerPortMap(container *apicontainer.Container, dynamicHostPo dockerPort := nat.Port(strconv.Itoa(containerPort) + "/" + protocolStr) if portBinding.HostPort != 0 { - // An User-specified host port exists + // An user-specified host port exists. + // Note that the host port value has been validated by ECS front end service; + // thus only an valid host port value will be streamed down to ECS Agent. hostPortStr = strconv.Itoa(int(portBinding.HostPort)) } else { // If there is no user-specified host port, ECS Agent will find an available host port @@ -2484,7 +2492,7 @@ func (task *Task) dockerPortMap(container *apicontainer.Container, dynamicHostPo containerToCheck.SetContainerHasPortRange(true) containerPortRange := portBinding.ContainerPortRange - // nat.ParsePortRangeToInt validates a port range; if valid, it returns start and end ports as integers + // nat.ParsePortRangeToInt validates a port range; if valid, it returns start and end ports as integers. startContainerPort, endContainerPort, err := nat.ParsePortRangeToInt(containerPortRange) if err != nil { return nil, err @@ -2508,7 +2516,7 @@ func (task *Task) dockerPortMap(container *apicontainer.Container, dynamicHostPo } // For the ContainerPortRange case, append ranges to the dockerPortMap. - // nat.ParsePortSpec returns a list of port mappings in a format that docker likes + // nat.ParsePortSpec returns a list of port mappings in a format that Docker likes. mappings, err := nat.ParsePortSpec(hostPortRange + ":" + containerPortRange + "/" + protocol) if err != nil { return nil, err @@ -2519,20 +2527,14 @@ func (task *Task) dockerPortMap(container *apicontainer.Container, dynamicHostPo } // 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 + // 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) - logger.Debug("containerToCheck is", logger.Fields{ - field.Container: containerToCheck.Name, - "dockerPortMap": dockerPortMap, - "containerPortSet": containerPortSet, - "containerPortRangeMap": containerPortRangeMap, - }) return dockerPortMap, nil } diff --git a/agent/api/task/task_test.go b/agent/api/task/task_test.go index eef53aeb2e7..97441600fef 100644 --- a/agent/api/task/task_test.go +++ b/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 { - // 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 - } - } } // Verify ContainerPortSet @@ -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 - 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) @@ -546,16 +528,10 @@ func TestDockerHostConfigSCBridgeMode(t *testing.T) { bindings, ok := actualConfig.PortBindings[convertSCPort(SCTaskContainerPort1)] assert.True(t, ok, "Could not get port bindings") assert.Equal(t, 1, len(bindings), "Wrong number of bindings") - hostPort, _ := strconv.Atoi(bindings[0].HostPort) - result := utils.PortIsInRange(hostPort, tc.testStartHostPort, tc.testEndHostPort) - assert.True(t, result, "hostport is not in the dynamic host port range") bindings, ok = actualConfig.PortBindings[convertSCPort(SCTaskContainerPort2)] assert.True(t, ok, "Could not get port bindings") assert.Equal(t, 1, len(bindings), "Wrong number of bindings") - hostPort, _ = strconv.Atoi(bindings[0].HostPort) - result = utils.PortIsInRange(hostPort, tc.testStartHostPort, tc.testEndHostPort) - assert.True(t, result, "hostport is not in the dynamic host port range") // SC pause container should get port binding map of all ingress listeners actualConfig, err = testTask.DockerHostConfig(testTask.Containers[3], dockerMap(testTask), defaultDockerClientAPIVersion, testConfig) @@ -566,9 +542,6 @@ func TestDockerHostConfigSCBridgeMode(t *testing.T) { // SC - ingress listener 1 - default experience bindings, ok = actualConfig.PortBindings[convertSCPort(SCIngressListener1ContainerPort)] assert.True(t, ok, "Could not get port bindings") - hostPort, _ = strconv.Atoi(bindings[0].HostPort) - result = utils.PortIsInRange(hostPort, tc.testStartHostPort, tc.testEndHostPort) - assert.True(t, result, "hostport is not in the dynamic host port range") // SC - ingress listener 2 - non-default host port bindings, ok = actualConfig.PortBindings[convertSCPort(SCIngressListener2ContainerPort)] diff --git a/agent/utils/ephemeral_ports.go b/agent/utils/ephemeral_ports.go index 40a90969939..1ee4399fa4e 100644 --- a/agent/utils/ephemeral_ports.go +++ b/agent/utils/ephemeral_ports.go @@ -34,6 +34,8 @@ const ( portUnavailableMsg = "Port %v is unavailable or an error occurred while listening on the local %v network" portNotFoundErrMsg = "a host port is unavailable" portsNotFoundErrMsg = "%v contiguous host ports are unavailable" + portRangeErrMsg = "The host port range: %s found by ECS Agent is not within the expected host port range: %s" + portErrMsg = "The host port: %s found by ECS Agent is not within the expected host port range: %s" ) var ( @@ -108,6 +110,13 @@ func GetHostPortRange(numberOfPorts int, protocol string, dynamicHostPortRange s portLock.Lock() defer portLock.Unlock() result, err := getNumOfHostPorts(numberOfPorts, protocol, dynamicHostPortRange) + if err == nil { + // Verify the found host port range is within the given dynamic host port range + if isInRange := verifyPortsWithinRange(result, dynamicHostPortRange); !isInRange { + errMsg := fmt.Errorf(portRangeErrMsg, result, dynamicHostPortRange) + return "", errMsg + } + } return result, err } @@ -119,10 +128,15 @@ func GetHostPort(protocol string, dynamicHostPortRange string) (string, error) { defer portLock.Unlock() numberOfPorts := 1 result, err := getNumOfHostPorts(numberOfPorts, protocol, dynamicHostPortRange) + foundHostPort := strings.Split(result, "-")[0] if err == nil { - result = strings.Split(result, "-")[0] + // Verify the found host port is within the given dynamic host port range + if isInRange := verifyPortsWithinRange(result, dynamicHostPortRange); !isInRange { + errMsg := fmt.Errorf(portErrMsg, foundHostPort, dynamicHostPortRange) + return "", errMsg + } } - return result, err + return foundHostPort, err } // getNumOfHostPorts returns the requested number of host ports using the given dynamic host port range @@ -220,9 +234,9 @@ 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; +// portIsInRange returns true if the given port is within the start-end port range; // otherwise, returns false. -func PortIsInRange(port, start, end int) bool { +func portIsInRange(port, start, end int) bool { if (port >= start) && (port <= end) { return true } @@ -231,15 +245,15 @@ func PortIsInRange(port, start, end int) bool { // VerifyPortsWithinRange returns true if the actualPortRange is within the expectedPortRange; // otherwise, returns false. -func VerifyPortsWithinRange(actualPortRange, expectedPortRange string) bool { +func verifyPortsWithinRange(actualPortRange, expectedPortRange string) bool { // 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) + aStartIsInRange := portIsInRange(aStartPort, eStartPort, eEndPort) // Check the actual end port is in the expected range or not - aEndIsInRange := PortIsInRange(aEndPort, eStartPort, eEndPort) + aEndIsInRange := portIsInRange(aEndPort, eStartPort, eEndPort) // Return true if both actual start port and end port are in the expected range if aStartIsInRange && aEndIsInRange { diff --git a/agent/utils/ephemeral_ports_test.go b/agent/utils/ephemeral_ports_test.go index 404c8fe53ea..c116e0a0ea3 100644 --- a/agent/utils/ephemeral_ports_test.go +++ b/agent/utils/ephemeral_ports_test.go @@ -207,9 +207,6 @@ func TestGetHostPort(t *testing.T) { numberOfHostPorts, err := getPortRangeLength(hostPortRange) assert.NoError(t, err) assert.Equal(t, numberOfPorts, numberOfHostPorts) - - actualResult := VerifyPortsWithinRange(hostPortRange, tc.testDynamicHostPortRange) - assert.True(t, actualResult) } }) } @@ -241,7 +238,7 @@ func TestPortIsInRange(t *testing.T) { for _, tc := range testCases { t.Run(tc.testName, func(t *testing.T) { - result := PortIsInRange(tc.testPort, tc.testStartPort, tc.testEndPort) + result := portIsInRange(tc.testPort, tc.testStartPort, tc.testEndPort) assert.Equal(t, tc.expectedResult, result) }) } @@ -270,7 +267,7 @@ func TestVerifyPortsWithinRange(t *testing.T) { for _, tc := range testCases { t.Run(tc.testName, func(t *testing.T) { - result := VerifyPortsWithinRange(tc.testActualRange, tc.testExpectedRange) + result := verifyPortsWithinRange(tc.testActualRange, tc.testExpectedRange) assert.Equal(t, tc.expectedResult, result) }) }