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

[DHPA] Validate the host port/host port range found by ECS Agent before returning it #40

Closed
wants to merge 3 commits into from

Conversation

chienhanlin
Copy link
Owner

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 to validate the host port/host port range found by ECS Agent before returning it through GetHostPort()/GetHostPortRange().

Implementation details

agent/utils/ephemeral_ports.go

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

agent/utils/ephemeral_ports_test.go

  • Remove host port validation in TestGetHostPort as the port should be validated before returning back from utils.GetHostPort()

agent/api/task/task.go

  • Update comments
  • Remove unclear logger.Debug entry from dockerPortMap()

agent/api/task/task_test.go

  • 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

Unit test

Re-ran the existing unit tests.

-- 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)
--- 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)
--- 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)
--- 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

N/A - Update comments and validate a host port or a host port range before returning from GetHostPort()/GetHostPortRange().

Licensing

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

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.

1 participant