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

Enumerate port ranges into docker config #3558

Merged

Conversation

YashdalfTheGray
Copy link
Contributor

@YashdalfTheGray YashdalfTheGray commented Feb 6, 2023

Summary

When using docker run with a port range exposed, Docker enumerates every port within the range into the Config.ExposedPorts map but the ECS Agent just puts the range as a string into the config. This becomes a problem when containers themselves aren't exposing ports (the Dockerfile doesn't contain an EXPOSE instruction for example) but customers are exposing ports using ECS task definitions and also using dynamic host port binding.

For example, assume that a container was built without an EXPOSE instruction in the Dockerfile but the customer's task definition lists a container port range of 80-90 with the protocol being TCP. Currently, there would be no way of reporting the bound ports since "80-90/tcp" gets added to the Config.ExposedPorts map and Docker seems to ignore anything that is not a singular port in the above map when it is reporting the host ports bound through the NetworkSettings.Ports map.

Implementation details

Using nat.ParsePortRangeToInt(), we get the start and the end of the port range. Once we have that, we individually build a nat.Port instance for each of the ports in the range and then we add them to the list of Docker exposed ports.

Testing

Unit tests were run and manual testing was also done by building the new agent with the changes and then running tasks on the instance with the new agent running.

Manual testing results

Config.ExposedPorts after $ docker run -d -p 9090 --name dr-sp public.ecr.aws/kulshres/simple-container

$ docker inspect dr-sp | jq '.[0] | .Config | .ExposedPorts'
{
  "8080/tcp": {},
  "9090/tcp": {}
}

Config.ExposedPorts after $ docker run -d -p 9090-9095 --name dr-pr public.ecr.aws/kulshres/ranges

$ docker inspect dr-pr | jq '.[0] | .Config | .ExposedPorts'
{
  "8080/tcp": {},
  "8081/tcp": {},
  "8082/tcp": {},
  "8083/tcp": {},
  "8084/tcp": {},
  "8085/tcp": {},
  "9090/tcp": {},
  "9091/tcp": {},
  "9092/tcp": {},
  "9093/tcp": {},
  "9094/tcp": {},
  "9095/tcp": {}
}

Config.ExposedPorts after running $ aws ecs run-task --task-definition SimpleContainer --launch-type EC2 --count 1

$ docker inspect ecs-SimpleContainer-3-container-... | jq '.[0] | .Config | .ExposedPorts'
{
  "8080/tcp": {},
  "9090/tcp": {}
}

Config.ExposedPorts after running $ aws ecs run-task --task-definition Ranges --launch-type EC2 --count 1

$ docker inspect ecs-Ranges-1-container-... | jq '.[0] | .Config | .ExposedPorts'
{
  "8080/tcp": {},
  "8081/tcp": {},
  "8082/tcp": {},
  "8083/tcp": {},
  "8084/tcp": {},
  "8085/tcp": {},
  "9090-9095/tcp": {}
}

Config.ExposedPorts after running $ aws ecs run-task --task-definition SimpleContainer --launch-type EC2 --count 1 and after making the change

$ docker inspect ecs-SimpleContainer-3-container-... | jq '.[0] | .Config | .ExposedPorts'
{
  "8080/tcp": {},
  "9090/tcp": {}
}

Config.ExposedPorts after running $ aws ecs run-task --task-definition Ranges --launch-type EC2 --count 1 and after making the change

$ docker inspect ecs-Ranges-1-container-... | jq '.[0] | .Config | .ExposedPorts'
{
  "8080/tcp": {},
  "8081/tcp": {},
  "8082/tcp": {},
  "8083/tcp": {},
  "8084/tcp": {},
  "8085/tcp": {},
  "9090/tcp": {},
  "9091/tcp": {},
  "9092/tcp": {},
  "9093/tcp": {},
  "9094/tcp": {},
  "9095/tcp": {}
}

New tests cover the changes: yes

Description for the changelog

Fix: enumerate port ranges into the docker config

Licensing

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

@danehlim
Copy link
Contributor

danehlim commented Feb 6, 2023

Looks good to me, thanks for the clear but concise writeup. Just one nit comment: if I'm not mistaken, I believe the second to last entry under "Manual testing results" should read as

Config.ExposedPorts after running $ aws ecs run-task --task-definition SimpleContainer --launch-type EC2 --count 1 and after making the change

instead of

Config.ExposedPorts after running $ aws ecs run-task --task-definition Ranges --launch-type EC2 --count 1 and after making the change

@YashdalfTheGray
Copy link
Contributor Author

Thanks for pointing it out Dane! Fixed that. And reran the test just to make sure too.

@YashdalfTheGray
Copy link
Contributor Author

According to this comment, the *kernel5dot10* tests are currently hanging on pending status but the tests are actually successful.

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