Skip to content

NM-273: Vnat pool assignments fix#3926

Merged
abhishek9686 merged 4 commits intodevelopfrom
NM-273
Mar 20, 2026
Merged

NM-273: Vnat pool assignments fix#3926
abhishek9686 merged 4 commits intodevelopfrom
NM-273

Conversation

@abhishek9686
Copy link
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

tenki-reviewer bot commented Mar 19, 2026

Tenki Code Review - Complete

Files Reviewed: 4
Findings: 2

By Severity:

  • 🟠 Medium: 2

This PR refactors Virtual NAT (VNAT) pool allocation by centralizing the logic in logic/networks.go and reusing it from both migrate/migrate.go and controllers/network.go, while also fixing the updateNetwork handler to return the correct server-side network object. Two minor correctness issues were identified: a missing overflow truncation in the new bigIntToIP helper (compared to the equivalent in pro/logic/egress.go), and a potentially unintentional removal of a feature-flag guard in the migration path.

Files Reviewed (4 files)
controllers/network.go
logic/networks.go
migrate/migrate.go
pro/logic/egress.go

Copy link

@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

The PR consolidates VNAT pool allocation logic — previously duplicated between migrate/migrate.go and pro/logic/egress.go — into exportable functions in logic/networks.go (AllocateUniqueVNATPool, AllocateUniquePoolFromFallback, NthSubnet). It also fixes a response encoding bug in updateNetwork and cleans up the migration function.

Positive Changes

  • updateNetwork response fix (controllers/network.go:826): The handler previously returned the raw client payload, missing server-managed fields (ID, CreatedAt, VNAT settings). Switching to netOld — which is mutated in-place by UpdateNetwork() — correctly returns the persisted DB state.
  • Refactoring: Centralizing AllocateUniquePoolFromFallback, NthSubnet, and related helpers avoids duplication between migrate/ and logic/.
  • Uniqueness guarantee: AllocateUniqueVNATPool now loads all existing network pools before allocating, preventing conflicts at network creation time.

Issues Found

bigIntToIP missing overflow guard (logic/networks.go:374)

The bigIntToIP function in logic/networks.go pads short byte slices but does not truncate when big.Int.Bytes() returns more bytes than expected (overflow). The identical function in pro/logic/egress.go includes this truncation guard. Without it, an overflowed IPv4 address could produce a nil result from To4(), causing NthSubnet to return a nil IP — which the pool allocation loop correctly skips, but means a valid pool slot is silently lost.

Migration no longer gated by feature flag (migrate/migrate.go:79)

The PR removes the check if !logic.GetFeatureFlags().EnableOverlappingEgressRanges { return } from initializeVirtualNATSettings(). This means the migration now populates VNAT fields for all Pro networks on startup, regardless of whether the overlapping egress feature is enabled. If intentional (pre-populating for future feature activation), this should be documented with a comment. If unintentional, the guard should be restored.

@abhishek9686 abhishek9686 merged commit 79c56b0 into develop Mar 20, 2026
5 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