fix(network): IPv6 addresses sorting was using wrong references #997
Conversation
…ne rather than every state change
… to online rather than every state change" This reverts commit 86be6df.
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes a critical bug in IPv6 address sorting where the comparison functions were incorrectly referencing elements from opposite slices instead of comparing within the same slice. The PR also adds detailed change reason tracking and logging for interface state changes, and includes an early return optimization for OTA updates.
- Fixed IPv6 address sorting bug by extracting stable sort logic into a separate function
- Added comprehensive change reason tracking and logging for network interface state changes
- Added early return when no OTA updates are available
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/nmlite/utils.go | Fixed IPv6 address sorting bug by introducing sortIPv6AddressSlicesStable() that correctly compares elements within the same slice |
| pkg/nmlite/interface_state.go | Added changeReason/stateChangeReason variables to track and log specific state changes for debugging |
| internal/ota/ota.go | Added early return logic when no updates are pending to avoid unnecessary processing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if im.state.Up != isUp { | ||
| im.state.Up = isUp | ||
| stateChanged = true | ||
| changeReason = "oper state changed" | ||
| } | ||
|
|
||
| // Check if the interface is online | ||
| isOnline := isUp && nl.HasGlobalUnicastAddress() | ||
| if im.state.Online != isOnline { | ||
| im.state.Online = isOnline | ||
| stateChanged = true | ||
| changeReason = "online state changed" | ||
| } | ||
|
|
||
| // Check if the MAC address has changed | ||
| if im.state.MACAddress != attrs.HardwareAddr.String() { | ||
| im.state.MACAddress = attrs.HardwareAddr.String() | ||
| stateChanged = true | ||
| changeReason = "MAC address changed" | ||
| } | ||
|
|
||
| // Update IP addresses | ||
| if ipChanged, err := im.updateInterfaceStateAddresses(nl); err != nil { | ||
| im.logger.Error().Err(err).Msg("failed to update IP addresses") | ||
| } else if ipChanged { | ||
| stateChanged = true | ||
| changeReason = "IP addresses changed" | ||
| } |
There was a problem hiding this comment.
When multiple state changes occur in the same update cycle, only the last change reason will be preserved and logged. For example, if both the oper state and MAC address change, only "MAC address changed" will be logged. Consider accumulating all change reasons (e.g., using a slice) or logging each change individually to aid in debugging.
| if !sortAndCompareStringSlices(im.state.IPv4Addresses, ipv4Addresses) { | ||
| im.state.IPv4Addresses = ipv4Addresses | ||
| stateChanged = true | ||
| stateChangeReason = "IPv4 addresses changed" | ||
| } | ||
|
|
||
| if !sortAndCompareIPv6AddressSlices(im.state.IPv6Addresses, ipv6Addresses) { | ||
| im.state.IPv6Addresses = ipv6Addresses | ||
| stateChanged = true | ||
| stateChangeReason = "IPv6 addresses changed" | ||
| } | ||
|
|
||
| if im.state.IPv4Address != ipv4Addr { | ||
| im.state.IPv4Address = ipv4Addr | ||
| stateChanged = true | ||
| stateChangeReason = "IPv4 address changed" | ||
| } | ||
|
|
||
| if im.state.IPv6Address != ipv6Addr { | ||
| im.state.IPv6Address = ipv6Addr | ||
| stateChanged = true | ||
| stateChangeReason = "IPv6 address changed" | ||
| } | ||
| if im.state.IPv6LinkLocal != ipv6LinkLocal { | ||
| im.state.IPv6LinkLocal = ipv6LinkLocal | ||
| stateChanged = true | ||
| stateChangeReason = "IPv6 link local address changed" | ||
| } | ||
|
|
||
| if im.state.IPv6Gateway != ipv6Gateway { | ||
| im.state.IPv6Gateway = ipv6Gateway | ||
| stateChanged = true | ||
| stateChangeReason = "IPv6 gateway changed" | ||
| } | ||
|
|
||
| if im.state.IPv4Ready != ipv4Ready { | ||
| im.state.IPv4Ready = ipv4Ready | ||
| stateChanged = true | ||
| stateChangeReason = "IPv4 ready state changed" | ||
| } | ||
|
|
||
| if im.state.IPv6Ready != ipv6Ready { | ||
| im.state.IPv6Ready = ipv6Ready | ||
| stateChanged = true | ||
| stateChangeReason = "IPv6 ready state changed" | ||
| } |
There was a problem hiding this comment.
When multiple state changes occur in the same update cycle, only the last change reason will be preserved and logged. For example, if both IPv4 and IPv6 addresses change, only "IPv6 addresses changed" will be logged. Consider accumulating all change reasons (e.g., using a slice) or logging each change individually to aid in debugging.
| } | ||
|
|
||
| sort.SliceStable(a, func(i, j int) bool { | ||
| return a[i].Address.String() < b[j].Address.String() |
There was a problem hiding this comment.
... so should have been
return a[i].Address.String() < a[j].Address.String()
(that b should have been a)... oops
| return a[i].Address.String() < b[j].Address.String() | ||
| }) | ||
| sort.SliceStable(b, func(i, j int) bool { | ||
| return b[i].Address.String() < a[j].Address.String() |
There was a problem hiding this comment.
... so should have been
return a[i].Address.String() < a[j].Address.String()
(that a should have been b)... oops
| return b[i].Address.String() < a[j].Address.String() | ||
| }) | ||
| sortIPv6AddressSlicesStable(a) | ||
| sortIPv6AddressSlicesStable(b) |
There was a problem hiding this comment.
Sorry I didn't notice this error :(
No description provided.