linkerd2
linkerd2 copied to clipboard
Fix destination staleness issue when adding EndpointSlices
When updating the portPublisher's address set when a new EndpointSlice creation event is received, its addresses where getting overwritten with stale data whenever its IDs already existed in the current pp's address set.
This bug would be manifested for example when dealing with dual-stack services. We'd have two ES with different IPs but with the same TargetRef pointing to the same pod, so their IDs would be the same. When the creation event for the second ES is received, its addresses would get overridden with the ones from the first ES.
Note however that dual-stack services are not supported yet, but we have unit tests for them in server_test.go
although this particular failure case wasn't covered. Upcoming dual-stack support will modify this logic a bit so there's no need to add that case.
Nevertheless, there can also be pathological cases in single-stack where this can be a problem. For example when ES get recycled but the deletion event is not caught for some reason, when the addition event is received its Address data will be overwritten by the old stale entry.
Other Changes
- Remove overriding
newAddressSet.LocalTrafficPolicy
as that is already taken care insidepp.endpointSliceToAddresses(slice)
. - When there are no Add events to send, return early without updating state nor updating metrics.
- Finally, the IPv6 address for the "name1-ipv6" ES test fixture was updated to match the associated pod.
This scenario occurs when two different EndpointSlices contain the same pod. Currently, the address from the older EndpointSlice is used for that pod and this PR changes it so that the address from the newer EndpointSlice is used instead. But it's not clear to me why one would be more correct than the other. Is such a conflict a valid state? Missing delete events will put the index into a incorrect state regardless so I'm not sure it makes sense to prefer the newer address just in case we have missed a delete.
For dual-stack, it seems that we legitimately will have multiple addresses for a single pod. Do we need to refactor this code so that a pod can have multiple addresses instead of just preferring one over the other?
Thanks for the review @adleong. I think I'm gonna put this one into draft and let's instead focus on #12428 first so that we have a clearer picture of the dual-stack handling before discussing the current issue.
I have rebased with the latest main, which now includes the IP family in the Addresses index key so the first failure case is no longer relevant here. I haven't been able to come up with a reproducible scenario where the overwriting I described would cause staleness. I still don't see though the reasoning for this overwriting...