Hi Paul and Maciej, I have another question: I would expect the timeout to be CT_SYN_TIMEOUT instead of CT_CONNECTION_LIFETIME_TCP for the reopened case. This can be fixed with the following change.
What do you think? If you agree, I can add the following change on top of the current commit.

--- a/bpf/lib/conntrack.h
+++ b/bpf/lib/conntrack.h
@@ -244,6 +244,7 @@ static __always_inline __u8 __ct_lookup(const void *map, struct __ctx_buff *ctx,
                        reopen |= seen_flags.value & TCP_FLAG_SYN;
                        if (unlikely(reopen == (TCP_FLAG_SYN|0x1))) {
                                ct_reset_closing(entry);
+                               entry->seen_non_syn = 0;
                                *monitor = ct_update_timeout(entry, is_tcp, dir, seen_flags);
                                return CT_REOPENED;
                        }

@lzang Regarding the timeout question, do we know for sure that such a connection will actually be new vs. a partially closed connection which the peers are reopening?

I wonder whether anyone has published some real-world observations or recommendations for handling TCP connection reopening, I've always seen it as something that is theoretically possible but am not familiar with common use cases or what kind of expectations apps might have in this scenario (and hence for instance how our selection of timeouts might impact such apps). It's easy to trigger such codepaths with a testing tool that just reuses ports, but in that case I think it's rare that there's any real impact from the timeouts we select since the test will often open & close the session within a short window.

@lzang Regarding the timeout question, do we know for sure that such a connection will actually be new vs. a partially closed connection which the peers are reopening?

The code path was triggered only on SYN request. What flag will be sent if it is a partially closed connection but the peers are reopening? Also the timeout would be updated on next packet that hits the same conntrack entry.

I wonder whether anyone has published some real-world observations or recommendations for handling TCP connection reopening, I've always seen it as something that is theoretically possible but am not familiar with common use cases or what kind of expectations apps might have in this scenario (and hence for instance how our selection of timeouts might impact such apps). It's easy to trigger such codepaths with a testing tool that just reuses ports, but in that case I think it's rare that there's any real impact from the timeouts we select since the test will often open & close the session within a short window.

I am not aware of any published work there. I think selection of timeout mainly affects how fast you can recycle the resource. But I agree the scenario here is a corner case and its impact to the real system should be small.

K8s-1.12-Kernel-netnext test fails with the following error - is it a test flakiness?

Stderr:
error: unable to upgrade connection: Authorization error (user=kube-apiserver-kubelet-client, verb=create, resource=nodes, subresource=proxy)

do we know for sure that such a connection will actually be new vs. a partially closed connection which the peers are reopening?

Is there such a thing as reopening a close-wait TCP connection? RFC793 mentions the following on reception of a SYN packet while in close-wait state (page 71):

If the SYN is in the window it is an error, send a reset, any
outstanding RECEIVEs and SEND should receive "reset" responses,
all segment queues should be flushed, the user should also
receive an unsolicited general "connection reset" signal, enter
the CLOSED state, delete the TCB, and return.

@pchaigno Hmm, I was thinking more about time-wait rather than close-wait, see RFC1122 Section 4.2.2.13:

When a connection is closed actively, it MUST linger in
TIME-WAIT state for a time 2xMSL (Maximum Segment Lifetime).
However, it MAY accept a new SYN from the remote TCP to
reopen the connection directly from TIME-WAIT state, if it:

       (1)  assigns its initial sequence number for the new
            connection to be larger than the largest sequence
            number it used on the previous connection incarnation,
            and

       (2)  returns to TIME-WAIT state if the SYN turns out to be
            an old duplicate.

Is there such a thing as reopening a close-wait TCP connection? RFC793 mentions the following on reception of a SYN packet while in close-wait state (page 71):

If the SYN is in the window it is an error, send a reset, any
outstanding RECEIVEs and SEND should receive "reset" responses,
all segment queues should be flushed, the user should also
receive an unsolicited general "connection reset" signal, enter
the CLOSED state, delete the TCB, and return.

I think we are not talking about the TCP layer here, and the re-open is just a re-use of the old entry from conntrack's view. The connection may have been considered as closed from TCP layer's view.

/test-me-please

I think we are not talking about the TCP layer here, and the re-open is just a re-use of the old entry from conntrack's view. The connection may have been considered as closed from TCP layer's view.

Before this PR, both connection reopens from time-wait and new connections reusing the same tuple would be considered as the existing connection and no policy verdict would be returned. So this is correct for the time-wait/re-open case but wrong for the tuple re-use case (which should be considered a new connection).

With the PR currently, both cases above would be considered new, which is correct for the tuple re-use case but could be considered wrong for the time-wait/reopen case since it's the same connection, just being reopened..

The only way to accurately determine when the connection is new or being reused for both cases AFAICT would be to also track sequence numbers, which is probably more work than we really care to do.

I think the argument of this PR is that overall, Cilium is more likely to send policy verdict notifications more accurately across these cases because (particularly in local testing), reuse of the tuple for a new connection is more likely than reopen of the existing time-wait connection? Or maybe it could be argued that even following the time-wait case above, in some sense it is triggering another "opening" of a connection and therefore we should log the policy verdict. I'm fine with either of these assessments.

Regarding the timeouts, my reading on the above time-wait would be that for that case, we should resume the regular connection timeouts. But as you say, the timeout would be updated on the next connection update anyway so it's probably not that consequential?

Given the comments above and inside __ct_update_timeout(), I would probably err on the side of not clearing the seen_non_syn bit unless we have some pretty clear guidance to describe why it's important or necessary.

Thanks Joe for the explanation! Now I got what you mean. Not sure how frequently would a tcp being re-opened with a SYN after entering the time-wait state, as most time time-wait is used to catch data packets that arrive later than the FIN. Also time-wait with 2xMSL is usually less than a couple of minutes, but the conntrack entry can stick for quite long in CiIlium due to the GC mechanism (please correct me if I am wrong), so the chance of hitting the tuple-reuse case could be larger. Agree that this is not a perfect solution, but it should work for most cases, and we probably don't need to be 100% accurate here :-)

CI is green except for checkpatch but we can ignore that particular warning (would require a lot more changes to fix).