feature/conn25: expire connector state - #20841
Conversation
The connector struct holds a map of peer+transitIP -> destinationIP that it uses for routing traffic. The client registers new entries in the map over the peer API. Stop the transitIPs map from growing indefinitely by expiring entries after 1 hour. Updates tailscale/corp#38261 Signed-off-by: Fran Bull <fran@tailscale.com>
12fc34d to
1ef6a84
Compare
mzbenami
left a comment
There was a problem hiding this comment.
The important thing to me is lowering the maximum per sweep and storing the value in a constant.
I'd also like to not use list.List.Init() in the reset().
The others are typos or explicitly marked as optional.
| // handle at most 100000 entries, don't just keep going if we have an | ||
| // unexpectedly large number of expiries, give up the lock and expect we will | ||
| // handle the backlog over time. | ||
| for i := 0; i < 100000; i++ { |
There was a problem hiding this comment.
c.mu is technically on the packet path, because the per-packet filter checks packetFilterAllow. Claude did some benchmarking and a full sweep added 20ms, while a limit of 1000 added 76 microseconds.
I'd prefer starting at 1000, which matches the flowtable (other place a sweeper is used).
tailscale/feature/conn25/flowtable.go
Line 91 in d31c6cd
There was a problem hiding this comment.
I also think this should be a named constant, not only for style consistency, but also because it can then be referenced from tests, and the tests are self-updating if/when the value changes.
| } | ||
| c.connector.mu.Lock() | ||
| if c.connector.expiryQueue.Len() != 3 { | ||
| t.Fatalf("expected 2 items remaining in queue") |
|
|
||
| c.connector.mu.Lock() | ||
| if _, ok := c.connector.transitIPs[peerA][tipOne]; ok { | ||
| t.Fatalf("expected oldTip %v to be removed from the map", tipOne) |
| t.Fatalf("expected oldTip %v to be removed from the map", tipOne) | ||
| } | ||
| if _, ok := c.connector.transitIPs[peerA][tipTwo]; !ok { | ||
| t.Fatalf("expected freshTip %v to remain in the map", tipTwo) |
| return netip.AddrFrom4(buf) | ||
| } | ||
| var i uint32 | ||
| for i = 0; i < 100003; i++ { |
There was a problem hiding this comment.
making this a named constant, e.g. maxPerSweep and referencing it here, e.g. maxPerSweep+3, would make tests self-updating when the value changes.
| defer c.mu.Unlock() | ||
|
|
||
| c.transitIPs = make(map[netip.Addr]map[netip.Addr]appAddr) | ||
| c.expiryQueue.Init() |
There was a problem hiding this comment.
Claude told me that making this c.expiryQueue = list.New() would be provide a little extra safety here if somehow a list.Element survives the reset. If code tries to remove the old element on a re-initialized list, funky things can happen like like the list having a length of -1.
You can see the relevant code here:
https://github.com/golang/go/blob/master/src/container/list/list.go#L134-L141
| type transitIPExpiryEntry struct { | ||
| peerIP netip.Addr | ||
| transitIP netip.Addr | ||
| createdAt time.Time |
There was a problem hiding this comment.
optional: mono.Time provides the same use cases at a smaller memory footprint. It's what we use on the flowtable side. We could wait until we observe some actual memory problems before making the change, or we can just do it now while we're here.
| return v, ok | ||
| } | ||
|
|
||
| // expireTransitIPs expires entries in the connectors transitIPs map that are |
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| removed := 0 | ||
| // handle at most 100000 entries, don't just keep going if we have an |
There was a problem hiding this comment.
double space between "keep" and "going"
| } | ||
| e := front.Value.(*transitIPExpiryEntry) | ||
| if now.Sub(e.createdAt) < connectorTransitIPExpiry { | ||
| // the list is ordered by createdAt there will be no entries to expire after this |
There was a problem hiding this comment.
punctuation between "createdAt" and "there will be" would be helpful for readability.
The connector struct holds a map of peer+transitIP -> destinationIP that it uses for routing traffic. The client registers new entries in the map over the peer API.
Stop the transitIPs map from growing indefinitely by expiring entries after 1 hour.
Updates tailscale/corp#38261