ICMP errors generated by tracked flows treated as related traffic #2247
Conversation
tomastigera
force-pushed
the
tomas-icmp-related
branch
2 times, most recently
from
82cf3e2
to
cda10e9
Compare
| @@ -327,6 +332,14 @@ enum calico_ct_result_type { | |||
| CALI_CT_INVALID, | |||
| }; | |||
|
|
|||
| #define CALI_CT_RELATED (1 << 8) | |||
There was a problem hiding this comment.
i decided to pass the flag as part of the code (a) the result struct is part of the state and I did not want to change it and (b) it effectively creates a whole set of return values, which was another option, but they would only differ in related or not
| static CALI_BPF_INLINE struct calico_ct_result calico_ct_v4_lookup(struct ct_ctx *ctx) | ||
| { | ||
| __u8 proto = ctx->proto; | ||
| __u8 proto_orig = ctx->proto; |
There was a problem hiding this comment.
ctx->proto changes once we decode the icmp payload
| if (state->ip_proto == IPPROTO_ICMP && ct_related) { | ||
| /* do not fix up embedded L4 checksum for related ICMP */ | ||
| } else { | ||
| switch (ip_header->protocol) { |
There was a problem hiding this comment.
state carries information about the entire packet, the state->ip_proto is still icmp, but we have moved the ip_header to point to the inner header. All the editing it done where point to, so we should use that info even in the case of regular NAT
tomastigera
force-pushed
the
tomas-icmp-related
branch
3 times, most recently
from
09677d0
to
b9e82c7
Compare
Can be reused in other locations.
This is a prerequisite for letting host to handle TTL exceeded.
Sets up endpoints, routes etc, but does not run
to complement the fact that we can create an inactive workload
Check that the ports are fixed up after NATing the ICMP related back
Needs to update csum in the inner IP header instead of the outer. When icmp generated by the next hop node, it needs to be placed in the tunnel.
tomastigera
force-pushed
the
tomas-icmp-related
branch
from
b9e82c7
to
52acb1b
Compare
| } | ||
|
|
||
| if (!icmp_skb_get_hdr(skb, &icmp)) { | ||
| CALI_DEBUG("Ooops, we already passed one such a check!!!\n"); |
There was a problem hiding this comment.
Worth making the log say what failed
There was a problem hiding this comment.
perhaps, but the function above also logs and this is more like this never happens, I'd panic if I could but did not feel like I should let this go completely unhandled.
This is triggered by missing tcpdump or bad filter
only felix-test image has tcpdump installed, extrnal client has not
if SNAT, we also need to fix up the source in the outer IP FV tests when ICMP is returned from host and from the backing workload
When we generate an ICMP in a workload or at a host in response to traffic that originated through a NP tunnel ammend the outer source IP as if the reponse was from the original node as all the rest is internal to the cluster.
tomastigera
force-pushed
the
tomas-icmp-related
branch
from
06bda79
to
14b7721
Compare
We need to treat related ICMP from tunnel the same way as we do the original traffic, otherwise it would go through the conntrack on the way out and would create ICMP tracking record.
Description
Todos
Release Note