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.

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

Remove fallback to Docker for host port ranges assignment #3569

Merged
merged 1 commit into from
Feb 14, 2023

Conversation

chienhanlin
Copy link
Contributor

@chienhanlin chienhanlin commented Feb 13, 2023

Summary

Starting from ECS Agent version 1.68.0 , users can specify containerPortRange at ECS task level, and ECS_DYNAMIC_HOST_PORT_RANGE at the container instance level to customize 1:1 container-host port mappings for their applications.

With the configured environment variable ECS_DYNAMIC_HOST_PORT_RANGE, ECS Agent will try to find a set of contiguous host ports within the given range. As users are expected to have ports to be bound in the customized range, errors should be returned instead of falling back for Docker to process dynamic port assignment if a set of contiguous host ports cannot be found by ECS Agent; therefore, this PR removes the Docker fallback mechanism when a set of contiguous host ports cannot be found by ECS Agent.

Issue

Scenario: ECS_DYNAMIC_HOST_PORT_RANGE is set, but host ports within the given range are not available for a bridge network mode task to bind it's container ports specified in the containerPortRange field.

  • Expected result: Task failed or a container STOPPED with an error Reason HostConfigError: error retrieving docker port map: 2 contiguous host ports unavailable
  • Actual result: Task succeeded but networkBindings is empty, and ecs-agent.log shows level=error msg="Unable to find contiguous host ports for container, falling back to docker dynamic port assignment" container="xxx" containerPortRange="8085-8086" error="2 contiguous host ports unavailable" task="xxx"

Setup

  • ECS-optimized AMI: amzn2-ami-ecs-hvm-2.0.20230127-x86_64-ebs
  • ECS Agent version: 1.68.2
  • An ECS cluster with only one registered EC2 instance
  • Userdata
#!/bin/bash
echo ECS_DYNAMIC_HOST_PORT_RANGE=100-102 >> /etc/ecs/ecs.config
  • ECS Task definition
    {
      "networkMode": "bridge",
      "containerDefinitions": [
        {
          "portMappings": [
            {
              "protocol": "tcp",
              "containerPortRange": "8085-8086"
            }
          ],
          ...
        }
      ]
    }
  • Ran the first ECS task, the task reached to RUNNING state and thehostPortRange returned from ecs describe-tasks call is within the given ECS_DYNAMIC_HOST_PORT_RANGE value.
$ aws ecs describe-tasks --cluster xxx --tasks "xxx" --region xxx
{
            "containers": [
                {
                    "networkBindings": [
                        {
                            "bindIP": "0.0.0.0",
                            "protocol": "tcp",
                            "containerPortRange": "8085-8086",
                            "hostPortRange": "100-101"
                        }
                    ],
                    ...
                }
            ],
}
  • Ran the same ECS task again, the task reached to RUNNING state but the hostPortRange returned from ecs describe-tasks call is empty.
$ aws ecs describe-tasks --cluster xxx --tasks "xxx" --region xxx
{
            "containers": [
                {
                    "lastStatus": "RUNNING",
                    "networkBindings": [],
                 ...
                }
            ],
}

Implementation details

  1. agent/api/task/task.go
  • for containerPortRange case, return nil as docker port map along with an error in dockerPortMap() when a set of contiguous host ports cannot be found by ECS Agent.
  1. agent/api/task/task_test.go
  • Remove an invalid test after the change made in dockerPortMap()

Testing

  1. Replaced the ECS Agent with the fix on the host, run 2 tasks and confirmed the second tasks failed as expected. Details of setup are provided in the Issue section.
$ aws ecs describe-tasks --cluster xxx --tasks "xxx" --region xxx
{

            "containers": [
                {
                    "lastStatus": "STOPPED",
                    "reason": "HostConfigError: error retrieving docker port map: 2 contiguous host ports unavailable",
                    "networkInterfaces": [],
                }
}
ecs-agent.log

level=info time=xxx msg="Creating container" task="xxx" container="xxx"
level=error time=xxx msg="Unable to find contiguous host ports for container" containerPortRange="8085-8086" error="2 contiguous host ports unavailable" task="xxx" container="xxx"
...
level=error time=xxx msg="Error transitioning container" task="xxx" container="xxx" runtimeID="" nextState="CREATED" error="error retrieving docker port map: 2 contiguous host ports unavailable"
...
level=warn time=xxx msg="Error creating container; marking its desired status as STOPPED" task="xxx" container="xxx" error="error retrieving docker port map: 2 contiguous host ports unavailable"
...

New tests cover the changes: no

Description for the changelog

Bug - Remove fallback to Docker for host port ranges assignment

Licensing

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

@chienhanlin chienhanlin changed the title WIP Remove fallback to Docker for host port ranges assignment Remove fallback to Docker for host port ranges assignment Feb 13, 2023
@chienhanlin chienhanlin marked this pull request as ready for review February 13, 2023 21:32
@chienhanlin chienhanlin requested a review from a team as a code owner February 13, 2023 21:32
agent/api/task/task.go Outdated Show resolved Hide resolved
agent/api/task/task.go Outdated Show resolved Hide resolved
@singholt
Copy link
Contributor

does this need to be changed as well?

// we supply containerPortRange here in case we did not assign a host port range and ask docker to do so

@singholt
Copy link
Contributor

singholt commented Feb 13, 2023

does this need to be changed as well?

// we supply containerPortRange here in case we did not assign a host port range and ask docker to do so

I think with Yash's last PR, we still need those dockerExposedPorts() injections, but just that the comment is no longer a valid one?

@chienhanlin
Copy link
Contributor Author

chienhanlin commented Feb 13, 2023

does this need to be changed as well?

// we supply containerPortRange here in case we did not assign a host port range and ask docker to do so

#3569 (comment)

Yes, no change required in dockerExposedPorts(). I will update the comment to reflect the change in this PR. Thanks!

Copy link
Contributor

@YashdalfTheGray YashdalfTheGray left a comment

Choose a reason for hiding this comment

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

Great writeup for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants