Skip to content

Master#3994

Merged
abhishek9686 merged 3 commits into
developfrom
master
Apr 30, 2026
Merged

Master#3994
abhishek9686 merged 3 commits into
developfrom
master

Conversation

@abhishek9686
Copy link
Copy Markdown
Member

Describe your changes

Provide Issue ticket number if applicable/not in title

Provide testing steps

Checklist before requesting a review

  • My changes affect only 10 files or less.
  • I have performed a self-review of my code and tested it.
  • If it is a new feature, I have added thorough tests, my code is <= 1450 lines.
  • If it is a bugfix, my code is <= 200 lines.
  • My functions are <= 80 lines.
  • I have had my code reviewed by a peer.
  • My unit tests pass locally.
  • Netmaker is awesome.

@tenki-reviewer
Copy link
Copy Markdown
Contributor

tenki-reviewer Bot commented Apr 30, 2026

Tenki Code Review - Complete

Files Reviewed: 2
Findings: 1

By Severity:

  • 🟠 Medium: 1

The PR improves deduplicateEgressRoutes to merge (rather than discard) duplicate-key egress route entries, which is a correct behavioral improvement. One medium-confidence bug exists in mergeUniqueEgressRangeMetrics: using the full EgressRangeMetric struct as a map key causes incomplete deduplication when the same network range appears in metrics from different egress gateway sources (different EgressID/EgressName), creating an inconsistency with how EgressRanges strings are deduplicated.

Files Reviewed (2 files)
logic/peers.go
logic/peers_test.go

Copy link
Copy Markdown
Contributor

@tenki-reviewer tenki-reviewer Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overview

This PR changes deduplicateEgressRoutes in logic/peers.go from a simple drop-duplicate strategy to a merge strategy: when two EgressNetworkRoutes entries share the same PeerKey+Network key, their EgressRanges and EgressRangesWithMetric slices are now combined rather than the second entry being discarded. A new helper mergeUniqueEgressRangeMetrics handles the metrics merging, and a test file logic/peers_test.go is added to cover the new behavior.

Finding: Incomplete deduplication in mergeUniqueEgressRangeMetrics

In logic/peers.go (line 960), mergeUniqueEgressRangeMetrics uses the full models.EgressRangeMetric struct directly as a map key:

seen := make(map[models.EgressRangeMetric]struct{}, len(existing)+len(incoming))

EgressRangeMetric contains EgressID and EgressName fields (tagged json:"-" but actively populated in logic/egress.go with distinct per-egress-gateway values). This means two metrics representing the same network range but originating from different egress gateway sources will have different EgressID values, causing them to be treated as distinct entries and both retained in the merged slice.

By contrast, EgressRanges is deduplicated by UniqueStrings purely on the CIDR string — so after merging, EgressRanges will correctly contain one entry for a network range while EgressRangesWithMetric may contain multiple entries for the same range. This inconsistency can cause the client to receive duplicate routing metric entries for a single network.

The existing test does not exercise this case (its test metrics have EgressID: "" and EgressName: ""), so the test passes despite the latent inconsistency.

Fix: Change the map key from models.EgressRangeMetric to string (the Network field), mirroring the string-based deduplication of EgressRanges.

Comment thread logic/peers.go
@abhishek9686 abhishek9686 merged commit 201c818 into develop Apr 30, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants