Change Conntrack to delete by Both Port And IP#52423
Change Conntrack to delete by Both Port And IP#52423
Conversation
|
I marked the DCO check to pass, because it was complaining about the name not matching the metadata from your commit;
If you want Git to automatically set the correct name for your sign-offs, you can update your Git config with the right name; git config --global user.name "Carson Lenze" |
| log.G(context.TODO()).Warnf("Failed to delete conntrack state for %s port %d: %v", proto.String(), port, err) | ||
| continue | ||
| } | ||
| if err := filter.AddIP(netlink.ConntrackOrigDstIP, hostIPs[i]); err != nil { |
There was a problem hiding this comment.
Having 2 separate slices (one for ports and one for ip-addresses) looks brittle; it now becomes a contract to make sure they're the same length.
| for _, pb := range ep.portMapping { | ||
| if pb.Proto == types.UDP { | ||
| udpPorts = append(udpPorts, pb.HostPort) | ||
| udpHostIPs = append(udpHostIPs, pb.HostIP) |
There was a problem hiding this comment.
(related to my other comment) and it looks like those slices are created by deconstructing the list of port-mappings.
I wonder if we should make DeleteConntrackEntriesByPort accept a list of port-bindings, so that all combination needed to perform the delete is combined.
moby/daemon/libnetwork/types/types.go
Lines 51 to 59 in af677b6
At a quick glance, this is the only location the function is used, so we can adjust it for this purpose.
And; it looks like the ep.portMapping slice contains a list of portmapperapi.PortBinding, which already contains that type;
moby/daemon/libnetwork/portmapperapi/api.go
Lines 94 to 97 in fc045ad
So, we can collect that info;
var udpPorts []types.PortBinding
for _, pb := range ep.portMapping {
if pb.Proto == types.UDP {
udpPorts = append(udpPorts, pb.PortBinding)
}
}Then pass it to DeleteConntrackEntriesByPort as-is;
iptables.DeleteConntrackEntriesByPort(nlh, types.UDP, udpPorts)I even wonder if we then still need the proto argument; it looks like it's only used for a log-message outside of the loop inside that function, but we can keep that for a follow-up cleanup;
|
Sounds good to me - thank you. The original code came from #44742 ... there's no sign the current behaviour was intentional, and I can't think why it would have been. But cc @akerouanton, @corhere. Passing in the port binding rather than two separate lists sounds good - it'd also be great to have a regression test that demonstrates the issue and the fix. |
|
Hey @thaJeztah, I made the changes as requested. I also tried to update my DCO and its still not wanting to add a space in my name, sorry about that. @robmry I'm not too familiar with regression tests. I went ahead and tested it locally by just checking to see what Conntrack entires get deleted on containers stopping, and all looks good. If you could help me or point me in the right direction I can attempt to create those tests. |
|
Oh! Looks like one of your commits is missing a While doing so, could you also squash the two commits? (Let me know if you need help doing so) |
Signed-off-by: CarsonLenze <carsonlenze@gmail.com> Change to use PortBinding Signed-off-by: CarsonLenze <carsonlenze@gmail.com>
|
Got those done, Let me know if you need me to do anything else! |
| if err := filter.AddIP(netlink.ConntrackOrigDstIP, port.HostIP); err != nil { | ||
| log.G(context.TODO()).Warnf("Failed to delete conntrack state for %s port %d: %v", proto.String(), port.Port, err) |
There was a problem hiding this comment.
@AkihiroSuda I just noticed there's also a ChildHostIP field on the wrapper portmapperapi.PortBinding struct - is that relevant in this context?
moby/daemon/libnetwork/portmapperapi/api.go
Lines 118 to 127 in fc045ad
(and an accessor);
moby/daemon/libnetwork/portmapperapi/api.go
Lines 139 to 146 in fc045ad
|
This change does not work if you don't bind to a specific IP address as the IP would be |
|
Proposed fix here: #52640 |
- What I did
Changed DeleteConntrackEntriesByPort to delete by both dest IP And dest Port.
I was running into issues where I was running two diffrent containers with the same port but diffrent IPs. When restarting one container it would delete all the conntrack entires for both containers since it was only deleting by port.
Im not sure if there are any issues open about this, or if this is even the best way to fix this, but It was causing issues for us and seemed like a fairly major issue.
- How I did it
Passed the HostIP from portMapping into DeleteConntrackEntriesByPort and added the IPs as a filter by ConntrackOrigDstIP
- How to verify it
Start up a udp listener in one container on one IP, then start up another udp listener in another container on the same port but with a diffrent IP. Restarting one container shouldn't cause the conntrack entires from the other container to get deleted.
- Human readable description for the release notes
- A picture of a cute animal (not mandatory but encouraged)
Here is one of my pet turtles :)