control/controlclient: fix sending peer capmap changes #11457
Conversation
There was a problem hiding this comment.
I believe this is safe because it only applies to patchifying PeerChanged and MapCap is unused in peers for all previous versions. It fixes the integration test that was failing. Please wait for @maisem to also review before merge.
control/controlclient/map.go
Outdated
| if n.CapMap != nil { | ||
| pc().CapMap = n.CapMap |
There was a problem hiding this comment.
But now that I see this code, this is wrong. If these are equal, it shouldn't be a diff.
|
Let's try to find a better word than "improve" ... all changes are improvements or we wouldn't be making them :) Maybe "fix CapMap handling in peerChangeDiff" |
clairew
force-pushed
the
clairew/peerchangediff-capmap
branch
3 times, most recently
from
a5d2cdf
to
9dfe21b
Compare
clairew
changed the title
Instead of just checking if a peer capmap is nil, compare the previous state peer capmap with the new peer capmap. Updates tailscale/corp#17516 Co-authored-by: Adrian Dewhurst <adrian@tailscale.com> Signed-off-by: Claire Wang <claire@tailscale.com>
clairew
force-pushed
the
clairew/peerchangediff-capmap
branch
from
9dfe21b
to
6dbb67b
Compare
| if n.CapMap == nil { | ||
| pc().CapMap = make(tailcfg.NodeCapMap) | ||
| } else { | ||
| pc().CapMap = n.CapMap |
There was a problem hiding this comment.
looks like all the other return paths returning a diff don't alias memory provided by the caller
(line 678 too)
There was a problem hiding this comment.
Can I make that a follow on PR to unblock Claire? The existing code was already aliasing memory.
There was a problem hiding this comment.
@sailorfrag pointed out that since the value type for NodeCapMap is of type tailcfg.RawMessage, which is a string thus immutable. So - aliasing memory is ok?
There was a problem hiding this comment.
The RawMessage yes, the map itself, no
Instead of just checking if a peer capmap is nil, also check if the previous state's capmap was nil.
Updates tailscale/corp#17516