bpf: allow L7LB TCP close packets on stale endpoint marks - #46508
Conversation
4c10047 to
11491b9
Compare
maintainer-s-little-helper
Bot
added
the
dont-merge/needs-release-note-label
11491b9 to
7d5a7d3
Compare
|
/test |
Envoy L7LB upstream sockets can outlive the source endpoint whose ID is stored in the proxy egress mark. Once that endpoint's policy program is removed, TCP close/control packets on the existing socket can miss the egress policy tail call and get dropped with DROP_EP_NOT_READY. To allow Envoy to terminate connections to backends, allow FIN/RST segments and payloadless ACKs if the egress policy has already been removed. SYNs and ACKs with payload will still be dropped. Fixes: cilium#41970 Signed-off-by: Tim Horner <[email protected]>
7d5a7d3 to
0b2ccf8
Compare
|
/test |
asauber
added
the
release-note/bug
maintainer-s-little-helper
Bot
removed
the
dont-merge/needs-release-note-label
|
@jrife gentle ping for review |
|
oops, missed this. I'll take a look soon. |
|
The scenario described and fix makes sense overall. Have you done any work to repro #41970 to make sure that this fixes it? |
|
@jrife yes locally in kind. I created a gist with the details here. Here's the results from the most recent run On main: On this branch: |
maintainer-s-little-helper
Bot
added
the
ready-to-merge
julianwiedmann
added
the
dont-merge/waiting-for-review
julianwiedmann
added
area/datapath
julianwiedmann
left a comment
julianwiedmann
left a comment
There was a problem hiding this comment.
Besides the issue noted below - this is a lot of additional detail, in an already complex code path. Are we sure that the problem is severe enough to justify this additional complexity?
| ret = tail_call_egress_policy(ctx, lxc_id); | ||
| ret = l7lb_tail_call_egress_policy(ctx, lxc_id, proto); | ||
| if (ret == CTX_ACT_OK) | ||
| return ret; | ||
| return send_drop_notify(ctx, lxc_id, sec_label, LXC_ID, | ||
| ret, METRIC_INGRESS); |
There was a problem hiding this comment.
This now skips all ingress processing for the destination endpoint, no? I think that's not ok.
There was a problem hiding this comment.
Ah is this because we are now returning from cil_to_container if this is an "accetpable" tail call miss? Could we continue only on CTX_ACT_OK instead?
ret = l7lb_tail_call_egress_policy(ctx, lxc_id, proto);
if (ret != CTX_ACT_OK)
return send_drop_notify(ctx, lxc_id, sec_label, LXC_ID,
ret, METRIC_INGRESS);
|
@julianwiedmann That's a good question. The problem seems rather benign, since nothing is functionally broken here. I think this is more of a "nice-to-have" improvement for signal:noise, not triggering a drop for an expected behavior. There's a few others that have +1 on the issue, but i'm not sure they are actually a direct match to the report. I'll defer to your opinion on this one. If this is adding a lot of overhead it might be better suited as a caveat in documentation |
If this is purely a cosmetic improvement (suppressing drop notifications during client teardown), then I don't think it's worth the maintenance & complexity overhead. This code path is complicated enough, no need to add one more variable to it. |
|
Closed in favor of documenting this behavior: #47587 |
5 participants
Envoy L7LB upstream sockets can outlive the source endpoint whose ID is stored in the proxy egress mark. Once that endpoint's policy program is removed, TCP close/control packets on the existing socket can miss the egress policy tail call and get dropped with DROP_EP_NOT_READY.
To allow Envoy to terminate connections to backends, allow only FIN/RST segments and payloadless ACKs if the egress policy has already been removed. SYNs and ACKs with payload will still be dropped.
Fixes: #41970