From 85116872706057cd8063e4ec22deb19da32799f1 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Fri, 19 Jun 2026 13:30:52 +0200 Subject: [PATCH] [management] log peer meta diff (#6468) --- management/server/peer.go | 4 +- management/server/peer/peer.go | 167 +++++++++++++------ management/server/peer/peer_metadiff_test.go | 113 +++++++++++++ 3 files changed, 233 insertions(+), 51 deletions(-) create mode 100644 management/server/peer/peer_metadiff_test.go diff --git a/management/server/peer.go b/management/server/peer.go index 83236d961..c54c1dc7b 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -1010,7 +1010,7 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync types.PeerSy } oldHasIPv6Cap := peer.HasCapability(nbpeer.PeerCapabilityIPv6Overlay) - updated, versionChanged = peer.UpdateMetaIfNew(sync.Meta) + updated, versionChanged = peer.UpdateMetaIfNew(ctx, sync.Meta) ipv6CapabilityChanged = oldHasIPv6Cap != peer.HasCapability(nbpeer.PeerCapabilityIPv6Overlay) if updated { am.metrics.AccountManagerMetrics().CountPeerMetUpdate() @@ -1170,7 +1170,7 @@ func (am *DefaultAccountManager) LoginPeer(ctx context.Context, login types.Peer } // This is needed to keep in memory for the peer config. Otherwise browser client will end in a retry loop - peer.UpdateMetaIfNew(login.Meta) + peer.UpdateMetaIfNew(ctx, login.Meta) peerGroupIDs, err = getPeerGroupIDs(ctx, am.Store, accountID, peer.ID) if err != nil { diff --git a/management/server/peer/peer.go b/management/server/peer/peer.go index e5475c07d..591ac074e 100644 --- a/management/server/peer/peer.go +++ b/management/server/peer/peer.go @@ -1,12 +1,16 @@ package peer import ( + "context" + "fmt" "net" "net/netip" "slices" - "sort" + "strings" "time" + log "github.com/sirupsen/logrus" + "github.com/netbirdio/netbird/management/server/util" "github.com/netbirdio/netbird/shared/management/http/api" ) @@ -162,49 +166,7 @@ type PeerSystemMeta struct { //nolint:revive } func (p PeerSystemMeta) isEqual(other PeerSystemMeta) bool { - sort.Slice(p.NetworkAddresses, func(i, j int) bool { - return p.NetworkAddresses[i].Mac < p.NetworkAddresses[j].Mac - }) - sort.Slice(other.NetworkAddresses, func(i, j int) bool { - return other.NetworkAddresses[i].Mac < other.NetworkAddresses[j].Mac - }) - equalNetworkAddresses := slices.EqualFunc(p.NetworkAddresses, other.NetworkAddresses, func(addr NetworkAddress, oAddr NetworkAddress) bool { - return addr.Mac == oAddr.Mac && addr.NetIP == oAddr.NetIP - }) - if !equalNetworkAddresses { - return false - } - - sort.Slice(p.Files, func(i, j int) bool { - return p.Files[i].Path < p.Files[j].Path - }) - sort.Slice(other.Files, func(i, j int) bool { - return other.Files[i].Path < other.Files[j].Path - }) - equalFiles := slices.EqualFunc(p.Files, other.Files, func(file File, oFile File) bool { - return file.Path == oFile.Path && file.Exist == oFile.Exist && file.ProcessIsRunning == oFile.ProcessIsRunning - }) - if !equalFiles { - return false - } - - return p.Hostname == other.Hostname && - p.GoOS == other.GoOS && - p.Kernel == other.Kernel && - p.KernelVersion == other.KernelVersion && - p.Core == other.Core && - p.Platform == other.Platform && - p.OS == other.OS && - p.OSVersion == other.OSVersion && - p.WtVersion == other.WtVersion && - p.UIVersion == other.UIVersion && - p.SystemSerialNumber == other.SystemSerialNumber && - p.SystemProductName == other.SystemProductName && - p.SystemManufacturer == other.SystemManufacturer && - p.Environment.Cloud == other.Environment.Cloud && - p.Environment.Platform == other.Environment.Platform && - p.Flags.isEqual(other.Flags) && - capabilitiesEqual(p.Capabilities, other.Capabilities) + return len(metaDiff(p, other)) == 0 } func (p PeerSystemMeta) isEmpty() bool { @@ -296,7 +258,7 @@ func (p *Peer) Copy() *Peer { // UpdateMetaIfNew updates peer's system metadata if new information is provided // returns true if meta was updated, false otherwise -func (p *Peer) UpdateMetaIfNew(meta PeerSystemMeta) (updated, versionChanged bool) { +func (p *Peer) UpdateMetaIfNew(ctx context.Context, meta PeerSystemMeta) (updated, versionChanged bool) { if meta.isEmpty() { return updated, versionChanged } @@ -308,14 +270,121 @@ func (p *Peer) UpdateMetaIfNew(meta PeerSystemMeta) (updated, versionChanged boo meta.UIVersion = p.Meta.UIVersion } - if p.Meta.isEqual(meta) { - return updated, versionChanged + oldVersion := p.Meta.WtVersion + + diff := metaDiff(p.Meta, meta) + if len(diff) != 0 { + p.Meta = meta + updated = true } - p.Meta = meta - updated = true + + versionInfo := "" + if versionChanged { + versionInfo = fmt.Sprintf("version changed: %s -> %s, ", oldVersion, meta.WtVersion) + } + + if len(diff) > 0 || versionChanged { + log.WithContext(ctx). + Debugf("peer meta updated, %s%d field(s) changed: %s", versionInfo, len(diff), strings.Join(diff, ", ")) + } + return updated, versionChanged } +// metaDiff returns a human-readable list of the fields that differ between the +// old and new meta, each formatted as `field: -> `. It is the single +// source of truth for meta comparison: isEqual reports equality as an empty +// diff, so the log line can never disagree with the change decision. Slices are +// cloned before sorting, so callers' meta is not mutated. +func metaDiff(oldMeta, newMeta PeerSystemMeta) []string { + var diff []string + add := func(field string, oldVal, newVal any) { + diff = append(diff, fmt.Sprintf("%s: %v -> %v", field, oldVal, newVal)) + } + + if oldMeta.Hostname != newMeta.Hostname { + add("hostname", oldMeta.Hostname, newMeta.Hostname) + } + if oldMeta.GoOS != newMeta.GoOS { + add("goos", oldMeta.GoOS, newMeta.GoOS) + } + if oldMeta.Kernel != newMeta.Kernel { + add("kernel", oldMeta.Kernel, newMeta.Kernel) + } + if oldMeta.KernelVersion != newMeta.KernelVersion { + add("kernel_version", oldMeta.KernelVersion, newMeta.KernelVersion) + } + if oldMeta.Core != newMeta.Core { + add("core", oldMeta.Core, newMeta.Core) + } + if oldMeta.Platform != newMeta.Platform { + add("platform", oldMeta.Platform, newMeta.Platform) + } + if oldMeta.OS != newMeta.OS { + add("os", oldMeta.OS, newMeta.OS) + } + if oldMeta.OSVersion != newMeta.OSVersion { + add("os_version", oldMeta.OSVersion, newMeta.OSVersion) + } + if oldMeta.WtVersion != newMeta.WtVersion { + add("wt_version", oldMeta.WtVersion, newMeta.WtVersion) + } + if oldMeta.UIVersion != newMeta.UIVersion { + add("ui_version", oldMeta.UIVersion, newMeta.UIVersion) + } + if oldMeta.SystemSerialNumber != newMeta.SystemSerialNumber { + add("system_serial_number", oldMeta.SystemSerialNumber, newMeta.SystemSerialNumber) + } + if oldMeta.SystemProductName != newMeta.SystemProductName { + add("system_product_name", oldMeta.SystemProductName, newMeta.SystemProductName) + } + if oldMeta.SystemManufacturer != newMeta.SystemManufacturer { + add("system_manufacturer", oldMeta.SystemManufacturer, newMeta.SystemManufacturer) + } + if oldMeta.Environment.Cloud != newMeta.Environment.Cloud { + add("environment_cloud", oldMeta.Environment.Cloud, newMeta.Environment.Cloud) + } + if oldMeta.Environment.Platform != newMeta.Environment.Platform { + add("environment_platform", oldMeta.Environment.Platform, newMeta.Environment.Platform) + } + if !oldMeta.Flags.isEqual(newMeta.Flags) { + add("flags", fmt.Sprintf("%+v", oldMeta.Flags), fmt.Sprintf("%+v", newMeta.Flags)) + } + if !capabilitiesEqual(oldMeta.Capabilities, newMeta.Capabilities) { + add("capabilities", oldMeta.Capabilities, newMeta.Capabilities) + } + + if !sameMultiset(oldMeta.NetworkAddresses, newMeta.NetworkAddresses) { + add("network_addresses", fmt.Sprintf("%v", oldMeta.NetworkAddresses), fmt.Sprintf("%v", newMeta.NetworkAddresses)) + } + + if !sameMultiset(oldMeta.Files, newMeta.Files) { + add("files", fmt.Sprintf("%v", oldMeta.Files), fmt.Sprintf("%v", newMeta.Files)) + } + + return diff +} + +// sameMultiset reports whether two slices contain the same elements with the +// same multiplicity, ignoring order. The element type is the comparison key, so +// every field participates in equality. +func sameMultiset[T comparable](a, b []T) bool { + if len(a) != len(b) { + return false + } + counts := make(map[T]int, len(a)) + for _, v := range a { + counts[v]++ + } + for _, v := range b { + counts[v]-- + if counts[v] == 0 { + delete(counts, v) + } + } + return len(counts) == 0 +} + // GetLastLogin returns the last login time of the peer. func (p *Peer) GetLastLogin() time.Time { if p.LastLogin != nil { diff --git a/management/server/peer/peer_metadiff_test.go b/management/server/peer/peer_metadiff_test.go new file mode 100644 index 000000000..1256cdb02 --- /dev/null +++ b/management/server/peer/peer_metadiff_test.go @@ -0,0 +1,113 @@ +package peer + +import ( + "net/netip" + "reflect" + "testing" + + "github.com/stretchr/testify/require" +) + +// metaDiffExtraEntries accounts for PeerSystemMeta fields that metaDiff does not +// map 1:1 to a single diff entry. Today the only such field is Environment, which +// is exploded into two checks (Cloud, Platform) and therefore yields one extra +// entry beyond its single struct field. If you teach metaDiff to explode another +// field into N entries, bump this by N-1; if you collapse a field, lower it. +const metaDiffExtraEntries = 1 + +// TestMetaDiff_CoversAllFields fully populates a PeerSystemMeta with non-zero +// values and diffs it against the zero value, then asserts metaDiff emits exactly +// one entry per exported field (plus metaDiffExtraEntries for fields it explodes). +// +// The expected count is derived from the struct via reflection, so adding a field +// to PeerSystemMeta raises the expectation automatically — but the actual diff +// only grows if metaDiff was taught to compare the new field. A mismatch means +// someone changed the struct without updating metaDiff (or this test's +// extra-entry accounting), which is exactly what we want to catch. +func TestMetaDiff_CoversAllFields(t *testing.T) { + var full PeerSystemMeta + exported := populateAll(t, reflect.ValueOf(&full).Elem()) + require.NotZero(t, exported, "expected PeerSystemMeta to expose fields") + + diff := metaDiff(PeerSystemMeta{}, full) + + require.Len(t, diff, exported+metaDiffExtraEntries, + "metaDiff entry count no longer matches PeerSystemMeta's fields: a field was "+ + "likely added or removed without updating metaDiff (or metaDiffExtraEntries). "+ + "diff was: %v", diff) + + require.False(t, full.isEqual(PeerSystemMeta{}), + "isEqual must report a fully-populated meta as different from the zero value") +} + +// TestFlags_isEqualChecksEveryField guards the one field that the count-based +// TestMetaDiff_CoversAllFields cannot: metaDiff collapses all of Flags into a +// single "flags" diff entry, so a new Flags field that Flags.isEqual forgets to +// compare would not change the diff count. This flips each Flags field on its own +// and asserts Flags.isEqual notices, so adding a Flags field without comparing it +// fails here. +func TestFlags_isEqualChecksEveryField(t *testing.T) { + typ := reflect.TypeOf(Flags{}) + for i := 0; i < typ.NumField(); i++ { + f := typ.Field(i) + require.Equal(t, reflect.Bool, f.Type.Kind(), + "Flags.%s is not a bool; extend this test to set it non-zero", f.Name) + + var a, b Flags + reflect.ValueOf(&b).Elem().Field(i).SetBool(true) + require.False(t, a.isEqual(b), "Flags.isEqual ignores field %s", f.Name) + } +} + +// populateAll sets every exported field of the struct to a deterministic non-zero +// value, recursing into nested structs and the element type of struct slices so +// that each leaf differs from zero. It returns the number of exported fields on +// the top-level struct. netip.Prefix is treated as an opaque leaf (it has no +// settable exported fields and is comparable with ==). +func populateAll(t *testing.T, v reflect.Value) int { + t.Helper() + + typ := v.Type() + exported := 0 + for i := 0; i < typ.NumField(); i++ { + f := typ.Field(i) + if f.PkgPath != "" { // unexported + continue + } + exported++ + setNonZero(t, v.Field(i)) + } + return exported +} + +// setNonZero assigns a deterministic non-zero value to a field based on its kind, +// recursing into nested structs and populating one element of slice fields. +func setNonZero(t *testing.T, field reflect.Value) { + t.Helper() + + if field.Type() == reflect.TypeOf(netip.Prefix{}) { + field.Set(reflect.ValueOf(netip.MustParsePrefix("10.0.0.0/24"))) + return + } + + switch field.Kind() { + case reflect.String: + field.SetString("non-zero") + case reflect.Bool: + field.SetBool(true) + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64: + field.SetInt(7) + case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64: + field.SetUint(7) + case reflect.Float32, reflect.Float64: + field.SetFloat(7) + case reflect.Struct: + populateAll(t, field) + case reflect.Slice: + s := reflect.MakeSlice(field.Type(), 1, 1) + setNonZero(t, s.Index(0)) + field.Set(s) + default: + t.Fatalf("unhandled field kind %s; extend setNonZero", field.Kind()) + } +}