FQDN: fix incorrect reaping of newly-expired IPs

[squeed]

A bug was found where a low-TTL name was incorrectly reaped despite
being part of an active connection. After looking at logs, and
reproducing locally, it was determined that there is an unfortunate
interleaving between the DNS and CT GC loops.

The code attempts to prevent this issue by ensuring that names inserted
after CT GC has started are exempt from reaping. However, we don't
actually track the insertion time, we track the DNS TTL expiration time,
which is strictly in the past. In fact, it can be up to a minute in the
past. We shouldn't rely on timestamps anyways, as the scheduler can
always play tricks on us.

So, if a CT GC run has started and finished in the time between name
expiration and insertion in to zombies, the IP address is immediately
considered dead and unnecessarily reaped.

Timeline:

T1. name expires
T2. CT GC starts and finishes
T3. Zombies.SetCTGCTime(T2)
T4. Zombies.Upsert(name, T1)
T5. Zombies.GC()

At T5, zombies.GC will remove IPs associated with name, because T2 > T1.

The solution is to use an explicit serial number to ensure that CTGC has
completed a full run before we are allowed to delete an IP. We actually
need to let CT GC run twice, as it may have started before this zombie
was added and thus not marked it alive.

Additionally, we already have a grace period, the idle connection
timeout, that gives applications a chance to re-use an expired IP.
However, we did not respect this grace period if the IP in question did
not have an entry in conntrack. So, pad deletion time by this grace
period as well, just to be sure this grace period applies to all
possible deletions.

Separately, don't tell FQDN that CT GC is complete if it encountered an error, or didn't do a full pass.

Fixes a bug where ToFQDN IPs may be garbage collected too early, disrupting existing connections.

[christarazi]

Thanks for the fix. I was sort of wondering about alternatives that avoid add extra fields that are not exported to DNSZombieMapping and ignored by the marshaller. Given the timeline you've detailed:

T1. name expires
T2. GC starts and finishes
T3. Zombies.SetCTGCTime(T2)
T4. Zombies.Upsert(name, T1)
T5. Zombies.GC()

Have you considered modifying step T4 to pass T4 instead of T1 to the Upsert call? It seems a bit suspicious that the time value passed in is subject to a race.

To clarify, is T2 CT GC or FQDN GC?

[squeed]

Have you considered modifying step T4 to pass T4 instead of T1 to the Upsert call?

I initially didn't do this, because the expiration time is relevant for determining which name to reap in the event of an overflow. However, I did wind up taking this suggestion with a bit of extra logic to handle the expiration-in-the-future case (i.e. over limit).

It seems a bit suspicious that the time value passed in is subject to a race.

Be careful, lest the ghost of Leslie Lamport come down and cause race conditions in all of your code that relies on clocks :-). Remember, the scheduler can and will break all your assumptions. Besides, I am uncomfortable with the fiddliness of exactly how the CT GC timestamp is interpreted. If we want to ensure CT GC runs twice, I'd rather use a counter.

[squeed]

/test

[squeed]

Have you considered modifying step T4 to pass T4 instead of T1 to the Upsert call?

I initially didn't do this, because the expiration time is relevant for determining which name to reap in the event of an overflow. However, I did wind up taking this suggestion with a bit of extra logic to handle the expiration-in-the-future case (i.e. over limit).

And I just tried this, and it broke tests because of a subtle interaction with timestamps and cilium-dbg fqdn cache clean. And this is why I dislike relying on timestamps. So we can't change the expiration time from T1 to T4 without breaking fqdn cache clean, which tests rely on extensively.

Nevermind, I'm wrong. We can insert a time of T4, but not a time of T4 + grace. Complicated.

[squeed]

/test

[squeed]

It seems a bit suspicious that the time value passed in is subject to a race.

Be careful, lest the ghost of Leslie Lamport come down and cause race conditions in all of your code that relies on clocks :-).

Here's one way it could happen:

1. FQDN.GC(T1) starts
2. CTGCStart := T2. GC finishes.
3. Zombie.Upsert(T1)
4. Zombie.SetCTGCTime(T2)
5. Zombie.GC() removes the IP, because T1 < T2

So, we really need the explicit conntrack generation -- it ensures a full CT GC pass has run, giving us a chance to mark the IP as alive. We also need the timestamps, so we can correctly compute grace period and prioritize newer connections in the event of overflow.

[chancez]

This is probably not useful as a drive-by comment, so feel free to ignore me, but don't forget to think about time going backwards too.

[stensonb]

Would this impact v1.14.2 and above? I'm pretty sure I'm seeing this in my clusters running v1.14.2.

Excited to see this fixed, either way. Thanks for your efforts @squeed

[squeed]

Would this impact v1.14.2 and above? I'm pretty sure I'm seeing this in my clusters running v1.14.2.

I’m not sure of the exact point version, but I’ve seen this in v1.14.5 clusters for sure.

[pippolo84]

Great, thanks! 💯
Just a non-blocking nit left inline.

[squeed]

/test

[stensonb]

@squeed just wanted to thank you for this! 1.14.8 works great!