-
Notifications
You must be signed in to change notification settings - Fork 616
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
Introduce a new environment variable ECS_EXCLUDE_IPV6_PORTBINDING #3025
Conversation
f25098d
to
113c257
Compare
113c257
to
762dca1
Compare
just wondering - does this filtering change anything for container in host and awsvpc network mode? i'm assuming no, but would be good to do a manual test to verify |
841a440
to
9ea0155
Compare
This filter does not change anything for containers in host and awsvpc network mode. Will add the manual test results to the PR, and also add following info as a reference. Thanks! For Task definition:
For Amazon ECS IPv6 support |
9ea0155
to
c9504d2
Compare
agent/api/ecsclient/client_test.go
Outdated
Cluster: configuredCluster, | ||
AWSRegion: "us-east-1", | ||
InstanceAttributes: additionalAttributes, | ||
IPv6PortBindingExcluded: config.BooleanDefaultTrue{Value: config.ExplicitlyEnabled}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - maybe a more appropriate name would be ShouldExcludeIPv6PortBinding
? Technically at the moment that this variable is checked it's not excluded yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the config variable to ShouldExcludeIPv6PortBinding
as recommended. Thanks!
agent/api/ecsclient/client.go
Outdated
networkBindings := []*ecs.NetworkBinding{} | ||
for _, binding := range change.PortBindings { | ||
if binding.BindIP == "::" && iPv6PortBindingExcluded { | ||
seelog.Debugf("To exclude IPv6 port bindings: %v as IPv6 port bindings is requested to be excluded", binding) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - we can probably make it shorter
"Excluded IPv6 port binding: %v"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the debug message as recommended. Thanks!
aa70721
to
2c3e4c0
Compare
Note: TestHandlerDoesntLeakGoroutines failed in the windows unit tests check, which is not introduced by code changes in this PR. |
That test has been flaky for a while on Windows. Can we update or skip that test for a more consistent signal? |
@@ -454,7 +454,7 @@ func (client *APIECSClient) buildManagedAgentStateChangePayload(change api.Manag | |||
} | |||
} | |||
|
|||
func (client *APIECSClient) buildContainerStateChangePayload(change api.ContainerStateChange) *ecs.ContainerStateChange { | |||
func (client *APIECSClient) buildContainerStateChangePayload(change api.ContainerStateChange, shouldExcludeIPv6PortBinding bool) *ecs.ContainerStateChange { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any reason why ipv6 ports are excluded only in state change calls?
Wouldn't the ipv6 ports show up in task metadata endpoint otherwise?
its better to filter it here to avoid any discrepancies:
container.SetKnownPortBindings(metadata.PortBindings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update README file and the description for the changelog to make the purpose/impact of this PR more clear. Thanks!
2c3e4c0
to
0a92d7a
Compare
README.md
Outdated
@@ -194,7 +194,8 @@ additional details on each available environment variable. | |||
| `ECS_ENABLE_AWSLOGS_EXECUTIONROLE_OVERRIDE` | `true` | Whether to enable awslogs log driver to authenticate via credentials of task execution IAM role. Needs to be true if you want to use awslogs log driver in a task that has task execution IAM role specified. When using the ecs-init RPM with version equal or later than V1.16.0-1, this env is set to true by default. | `false` | `false` | | |||
| `ECS_FSX_WINDOWS_FILE_SERVER_SUPPORTED` | `true` | Whether FSx for Windows File Server volume type is supported on the container instance. This variable is only supported on agent versions 1.47.0 and later. | `false` | `true` | | |||
| `ECS_ENABLE_RUNTIME_STATS` | `true` | Determines if [pprof](https://pkg.go.dev/net/http/pprof) is enabled for the agent. If enabled, the different profiles can be accessed through the agent's introspection port (e.g. `curl http://localhost:51678/debug/pprof/heap > heap.pprof`). In addition, agent's [runtime stats](https://pkg.go.dev/runtime#ReadMemStats) are logged to `/var/log/ecs/runtime-stats.log` file. | `false` | `false` | | |||
|
|||
| `ECS_EXCLUDE_IPV6_PORTBINDING` | `true` | Determines if agent should exclude IPv6 port binding. If enabled, IPv6 port binding will be filtered out, and the response of DescribeTasks API call will not show tasks' IPv6 port bindings, but it is still included in Task metadata endpoint. | `true` | `true` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may want to indicate this is only for bridge mode tasks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the description with the network mode as follows,
"Determines if agent should exclude IPv6 port binding using default network mode. If enabled, IPv6 port binding will be filtered out, and the response of DescribeTasks API call will not show tasks' IPv6 port bindings, but it is still included in Task metadata endpoint."
Thanks!
agent/api/ecsclient/client.go
Outdated
networkBindings := []*ecs.NetworkBinding{} | ||
for _, binding := range change.PortBindings { | ||
if binding.BindIP == "::" && shouldExcludeIPv6PortBinding { | ||
seelog.Debugf("Excluded IPv6 port binding: %v", binding) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we also log the container name/task ID if possible, so this is more useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the container name and the task ARN to the debug log, as
level=debug time=xxx msg="Exclude IPv6 port binding {80 49155 :: 0} for container web in task xxxxx" module=client.go
0a92d7a
to
7ec9685
Compare
Skipped Windows unit tests in favor of #3030 |
Just noticed this behavior today - has this recently regressed? I'm on Docker 20.10.7 and ecs-agent 1.57.1 and am still noticing IPV6 binding being created, even after setting |
Is this working? I have also added |
Summary
A behavioral change in docker container port binding, where both IPv4 and IPv6 bindings will be exposed starting from Docker version 20.10.6, is reported in following issues and PR:
To mitigate this issue, this PR introduces a new agent environment variable
ECS_EXCLUDE_IPV6_PORTBINDING
to filter out IPv6 port bindings for default network mode tasks. This config variable istrue
by default, which means the IPv6 port bindings will be filtered out in the Container State Change Payload. To include IPv6 port bindings, setECS_EXCLUDE_IPV6_PORTBINDING=false
in the/etc/ecs/ecs.config
file and then restart the agent.No change is made on the metadata. Customers can still find both IPv4 and IPv6 port bindings from the task metadata endpoint even when
ECS_EXCLUDE_IPV6_PORTBINDING=true
. This raw data, which is retrieved fromdocker inspect
, is kept for further debug operation.Implementation details
ShouldExcludeIPv6PortBinding
and a config variableECS_EXCLUDE_IPV6_PORTBINDING
to agent and istrue
by default.ECS_EXCLUDE_IPV6_PORTBINDING
.ShouldExcludeIPv6PortBinding
is true.Testing
Following two docker versions are used for testing:
Manual test cases:
Steps:
docker ps
anddocker inspect
to get the docker container port mappings.Results of each test case are listed as follows:
true
.false
.Test cases 1-3 have the same responses.
true
.Test cases 4-5 have the same responses.
false
.awsvpc
andhost
network modeawsvpc
andhost
modeNew tests cover the changes: no, but existing config unit tests are updated.
Description for the changelog
Enhancement - Introduce a new environment variable
ECS_EXCLUDE_IPV6_PORTBINDING
, which istrue
by default. When this filter is enabled, the response of DescribeTasks API call will not show the IPv6 port bindings for default network mode tasks, but it is still included in the Task metadata endpoint. This is a workaround for docker's bug as detailed in #2870.Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.