Only delete conntrack entries for orig-src and reply-src. #1500
Conversation
This reduces the number of scans of the conntrack table we need to do.
| // deleting entries for local workload endpoints, either the endpoint originated the traffic, or it | ||
| // received the traffic and replied to it. In the originating case, the "original source" will be | ||
| // set to the endpoint's IP; in the other case, the "reply source". Hence, it's sufficient to only | ||
| // look in those two fields. |
There was a problem hiding this comment.
This comment is very helpful, but I think it may need even a bit more in order to explain why only looking at orig-src and reply-src is watertight for our purposes.
The concerns are, I think that:
- There could be a (partial) flow with
orig-dst==<endpoint IP>but noreply-src, if the endpoint chooses not to respond at all. At least with UDP that would be quite easy to arrange; probably more tricky with TCP. - Is there any way to get a flow with
reply-dst==<endpoint IP>!=orig-src?
I think the possible answers are that:
- It isn't possible to get conntrack state at all for such cases, because they fall outside the definition of what conntrack considers to be a flow; and/or
- There can be conntrack state for such cases, but it naturally expires so quickly that we don't care about Felix deleting it here. (Hypothesis:) Conntrack state is only long-lived for complete flows with
reply-src==orig-dstandreply-dst==orig-src, and it's only that long-lived state that we are concerned about deleting here.
Feel free to cut and paste if any of this is right or useful.
There was a problem hiding this comment.
I think the conntrack tuples are produced as a result of the NAT calculation; it doesn't matter that there hasn't yet been a reply; the reply direction would still exist and have the expected reply source IP in it.
There was a problem hiding this comment.
Ah, I see now, I had completely misunderstood. (So this exchange was very useful!)
There was a problem hiding this comment.
To double-check my new understanding, then:
-
If
orig-dstis different fromreply-src, that would only be because of a DNAT affecting connection to the local workload, withorig-dstbeing the service or floating IP andreply-srcbeing the endpoint IP. Since we are deleting by endpoint IP,reply-srcis the only useful lookup. -
If
reply-dstis different fromorig-src, that would only be because of an SNAT affecting outgoing connections from the local workload, withorig-srcbeing the endpoint IP - and again, because we are deleting by endpoint IP,orig-srcis the only useful lookup.
That all seems watertight to me, so I'll approve now.
There was a problem hiding this comment.
That's my understanding. I can't think of any other cases.
| // deleting entries for local workload endpoints, either the endpoint originated the traffic, or it | ||
| // received the traffic and replied to it. In the originating case, the "original source" will be | ||
| // set to the endpoint's IP; in the other case, the "reply source". Hence, it's sufficient to only | ||
| // look in those two fields. |
There was a problem hiding this comment.
To double-check my new understanding, then:
-
If
orig-dstis different fromreply-src, that would only be because of a DNAT affecting connection to the local workload, withorig-dstbeing the service or floating IP andreply-srcbeing the endpoint IP. Since we are deleting by endpoint IP,reply-srcis the only useful lookup. -
If
reply-dstis different fromorig-src, that would only be because of an SNAT affecting outgoing connections from the local workload, withorig-srcbeing the endpoint IP - and again, because we are deleting by endpoint IP,orig-srcis the only useful lookup.
That all seems watertight to me, so I'll approve now.
Only delete conntrack entries for orig-src and reply-src.
This reduces the number of (expensive) scans of the conntrack table we need to do.
Description
This was one of the ideas that @doublek / @matthewdupre / I came up with to reduce the time spent removing conntrack flows. After closer inspection of the conntrack tuple, I'm convinced that we don't need to look in the other two fields when looking for flows to delete.
Todos
Improves https://github.com/projectcalico/felix/issues/1501
Release Note