MTV-5323 | Preserve advanced Windows network settings during migration

[Hazanel]

Add a REGF registry parser to extract per-adapter network settings
from the Windows SYSTEM hive after virt-v2v conversion.
MAC addresses are resolved directly from the registry, eliminating any dependency on
provider-supplied NIC MAC lists.

Settings preserved:

• InterfaceMetric
• DNS registration,
• NetBIOS over TCP/IP,
• LanmanServer start type, and File & Printer Sharing bindings.

Ref: https://redhat.atlassian.net/browse/MTV-5323
Resolves: MTV-5323

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR adds comprehensive support for extracting and applying Windows advanced network settings during VM migration. It includes a complete Windows Registry REGF parser to read the SYSTEM hive, utilities to extract network configuration (interfaces, metrics, DNS, NetBIOS, File & Printer Sharing state), entrypoint integration to read settings before customization, refactored template injection to render and upload a firstboot PowerShell script, and extensive test coverage with synthetic hive builders.

Changes

Windows Advanced Network Settings Feature

Layer / File(s)	Summary
Windows Registry REGF Parser Library pkg/virt-v2v/customize/advancednet/regf.go	Complete hive parser with NK/VK cell parsing, subkey traversal with recursion guards, typed REG value readers (DWORD/SZ/BINARY/MULTI_SZ), and UTF-16LE decoding supporting both inline and offset-based data.
Registry Parser Test Suite pkg/virt-v2v/customize/advancednet/regf_test.go	Synthetic hiveBuilder utility constructing in-memory REGF structures; tests cover header validation, key/value traversal, type mismatches, inline data, UTF-16LE, and corrupt hive error handling.
Advanced Network Settings Data Model pkg/virt-v2v/customize/advancednet/types.go	JSON-serializable AdvancedNetSettings, AdapterRef, InterfaceSettings structs; Windows network defaults; HasNonDefaultSettings() predicate detecting deviations from defaults.
Settings Extraction from Disk pkg/virt-v2v/customize/advancednet/reader.go	Reads SYSTEM hive via virt-cat, parses it, builds GUID→MAC mapping via NetworkSetup2/device-class fallback, extracts per-interface settings and LanmanServer state, determines File & Printer Sharing disabled adapters, persists as JSON.
Entrypoint: Pre-Customization Settings Read cmd/virt-v2v/entrypoint.go	After loading inspection output, conditionally reads advanced network settings from first disk when source OS is Windows; logs failures non-fatally; writes settings file when non-default values detected.
Template Rendering and Customization Integration pkg/virt-v2v/customize/customize.go	New generic renderTemplate helper; injectAdvancedNetworkSettingsTemplate reads JSON, renders PowerShell script, returns sentinel ErrNoAdvancedNetSettings for absent/default settings; integrated into addWinFirstbootScripts with conditional upload.
Customization Tests pkg/virt-v2v/customize/customize_test.go	Standard testing-based suite validating renderTemplate, injectAdvancedNetworkSettingsTemplate sentinel errors, JSON payload round-tripping in rendered scripts, and proper propagation of non-sentinel errors.
Firstboot PowerShell Script pkg/virt-v2v/customize/scripts/windows/9998-advanced-network-settings.ps1.tmpl	Parses embedded JSON settings, resolves adapters by MAC (polling up to 30s), applies per-interface metric/DNS/NetBIOS configuration, sets LanmanServer startup, unbinds File & Printer Sharing on disabled adapters, reports completion.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• kubev2v/forklift#6186: Updates Windows network configuration script injection in customizeWindows, gating the legacy 9999-network-config.ps1.tmpl injection alongside the new advanced-network settings script flow.

Suggested reviewers

• mnecas
• yaacov
• mrnold
• solenoci

---

🐰 Registry hives we parse with care,
Windows nets configured fair,
JSON whispers through the hive,
PowerShell makes networks thrive! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically summarizes the main change: preserving advanced Windows network settings during migration, which aligns with the primary focus of the changeset.
Description check	✅ Passed	The description is directly related to the changeset, detailing the REGF registry parser, MAC address resolution, and specific settings preserved during migration.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/virt-v2v/customize/advancednet/reader.go`:
- Around line 113-126: The buildGUIDToMAC function currently swallows hive
errors when calling hive.EnumerateSubkeys(ns2Key), hive.OpenKey(ns2Path + "\\" +
guid + "\\Kernel"), and hive.ReadBinary(kernelKey,
"CurrentAddress"/"PermanentAddress"); change these to check and wrap each error
with contextual information (include guid and key path/value name) and propagate
the error out of buildGUIDToMAC instead of continuing silently. Specifically,
replace the ignored error returns from guidKeys, err :=
hive.EnumerateSubkeys(ns2Key), kernelKey, err := hive.OpenKey(...), and the
ReadBinary calls with proper if err != nil { return nil,
fmt.Errorf("buildGUIDToMAC: failed to <operation> for guid %s at %s: %w", guid,
<path-or-value>, err) } (or accumulate structured warnings to return alongside
the mapping), ensuring callers receive the error so data-loss conditions are
visible and handled.
- Around line 253-259: The check building hasCustom incorrectly treats only
RegistrationEnabled == 0 as custom; update the condition to mirror
HasNonDefaultSettings() semantics by checking iface.RegistrationEnabled !=
DNSRegistrationEnabled instead of == 0, so that any explicit non-default values
(e.g., 2) mark hasCustom true; adjust the block where hasCustom is computed (the
code referencing iface.InterfaceMetricAuto, iface.InterfaceMetric and
iface.RegistrationEnabled) to use the != comparison.

In `@pkg/virt-v2v/customize/advancednet/regf.go`:
- Around line 243-283: The looped element reads in findInSubkeyListDepth
(handling sigLF/sigLH, sigLI, sigRI) use count without checking that the full
array fits the buffer; before each switch case iterate, compute the required
bytes (sigLF/LH: 4 + int(count)*8, sigLI/sigRI: 4 + int(count)*4) and guard by
validating that abs + required <= len(h.data) (or h.size) and return a safe
error if not; apply the same pattern in FindValue before iterating over
nk.valuesCount (compute required = 4 + int(nk.valuesCount)*sizePerElement and
validate buffer bounds) so all element reads (readInt32, parseNK, child sublist
offsets) are protected from malformed counts.
- Around line 131-137: The code calls cellOffset(off) and only checks upper
bounds, but cellOffset can return negative values; update both locations where
abs := cellOffset(off) is used (the NK offset/record checks in regf.go around
the NK parsing) to validate abs >= 0 before any slicing or adding
cellSizeLen/nkMinSize, returning a descriptive error (e.g., "regf: NK offset
0x%x out of range") when abs < 0; ensure the same check is applied in the second
occurrence (the block handling NK record size further down) to prevent
negative-index panics when reading h.data.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d1c6328f-1197-44b3-b612-b8d6f3f74846

📥 Commits

Reviewing files that changed from the base of the PR and between dc5d977 and 92d7de1.

📒 Files selected for processing (12)

• cmd/virt-v2v/entrypoint.go
• pkg/controller/plan/adapter/base/network_test.go
• pkg/controller/plan/adapter/hyperv/builder.go
• pkg/controller/plan/adapter/ovfbase/builder.go
• pkg/controller/plan/adapter/vsphere/builder.go
• pkg/virt-v2v/customize/advancednet/reader.go
• pkg/virt-v2v/customize/advancednet/regf.go
• pkg/virt-v2v/customize/advancednet/regf_test.go
• pkg/virt-v2v/customize/advancednet/types.go
• pkg/virt-v2v/customize/customize.go
• pkg/virt-v2v/customize/customize_test.go
• pkg/virt-v2v/customize/scripts/windows/9998-advanced-network-settings.ps1.tmpl

[Hazanel]

@coderabbitai please take a look and also share your thought whether it's a good approach or you think we can come up with a better way

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the core new files
cat pkg/virt-v2v/customize/advancednet/types.go
echo "---SEPARATOR---"
cat pkg/virt-v2v/customize/advancednet/regf.go
echo "---SEPARATOR---"
cat pkg/virt-v2v/customize/advancednet/reader.go
echo "---SEPARATOR---"
cat pkg/virt-v2v/customize/customize.go
echo "---SEPARATOR---"
cat cmd/virt-v2v/entrypoint.go
echo "---SEPARATOR---"
cat pkg/virt-v2v/customize/scripts/windows/9998-advanced-network-settings.ps1.tmpl

Length of output: 61106

---

🏁 Script executed:

#!/bin/bash
# Check how AddArgs handles empty strings
ast-grep --pattern 'func ($_ $_) AddArgs($$$) {
  $$$
}'

# Also check the CommandBuilder interface
rg -n "AddArgs" pkg/virt-v2v/utils/ -C3

Length of output: 7968

---

@Hazanel, great PR! I went through the full implementation. Here's my take on the overall approach and a few things worth considering:

---

Overall Approach Assessment ✅

The approach is well-chosen for the environment. The two main design decisions are sound:

1. Custom REGF Parser vs. Using Existing Tools

The natural alternative to writing your own parser would be to shell out to hivex CLI tools (hivexget, hivexls) which are already present in the libguestfs container. However, the custom parser wins here for a few reasons:

• hivexget/hivexls output is text — awkward for REG_BINARY (MAC address bytes need hex-to-bytes conversion with potential formatting variance)
• virt-win-reg emits .reg format which still needs non-trivial parsing
• The custom parser is fully unit-testable with synthetic hives (as demonstrated by the test suite) — no process spawning in tests
• It has no additional Go module dependencies

The implementation handles all the important edge cases: lf/lh/li/ri subkey list structures, compressed vs UTF-16LE names, inline/resident data for small values, bounded recursion depth (maxSubkeyListDepth = 10).

2. MAC Resolution from Registry (vs. Provider-Supplied List)

This is a clear improvement over the previous approach. Reading from NetworkSetup2\Kernel\CurrentAddress (REG_BINARY, 6 bytes) as primary and Control\Class\{GUID}\NetworkAddress as fallback is the right strategy. The two-source design handles the common case where NetworkAddress isn't set (most adapters don't have a user-defined address override).

---

Specific Concerns

🔴 virt-cat --format=raw hardcoded — may fail on qcow2 disks

// reader.go
cmd := exec.Command("virt-cat",
    "--format=raw",   // ← hardcoded
    "-a", diskPath,
    systemHivePath,
)

If the disk is qcow2 (e.g., in some vSphere or OVA migrations), this will fail. The disk format should be determined dynamically or passed in as a parameter. You could detect it from the disk extension or thread it through from config.AppConfig where the format is likely already known.

🔴 Only first disk is read — no fallback for Windows spanning multiple disks

// entrypoint.go
settings, advErr := advancednet.ReadAdvancedNetworkSettings(
    convert.Disks[0].Link, env.Workdir,
)

Windows always places C:\Windows\System32\config\SYSTEM on the boot disk, which is typically Disks[0], so this is usually correct. But virt-cat will silently fail with "file not found" if the disk layout differs. It would be safer to either document this assumption explicitly with a comment, or iterate through disks until virt-cat succeeds.

🟡 injectComplementryStaticIPTemplate was not refactored to use the new renderTemplate helper

The new renderTemplate helper is great, and injectStaticIPTemplate was already refactored to use it. But injectComplementryStaticIPTemplate still carries its own duplicated template parsing/rendering logic (~20 lines). It could be simplified too, though it has the extra funcMap which would need passing through — perhaps renderTemplate could accept an optional template.FuncMap.

🟡 PowerShell JSON injection — single-quote assumption

# 9998-advanced-network-settings.ps1.tmpl
$settingsJson = '{{.SettingsJSON}}'

JSON uses double quotes, so single-quote wrapping is safe in practice. However, if a field ever contains a single-quote character (e.g., a NIC GUID or MAC with unusual formatting), it would break the PowerShell string. Wrapping in a here-string (@' ... '@) would be more robust:

$settingsJson = @'
{{.SettingsJSON}}
'@

🟢 AddArgs empty-string handling — already handled

I checked command.go:88 — AddArgs already skips empty values (if value != "" {), so passing an empty uploadAdvancedNetPath is safe. No issue here.

🟢 Non-fatal error handling

The pattern of logging errors without blocking migration is correct for a best-effort feature like this. Settings that can't be read just won't be applied, and the migration continues.

---

Summary

Custom REGF parser	✅ Right choice for this environment
MAC from registry	✅ Better than provider-supplied list
--format=raw hardcoded	🔴 Can break on qcow2
First disk only	🟡 Works in practice, worth a comment
injectComplementryStaticIPTemplate not refactored	🟡 Minor tech debt
PS1 JSON injection	🟡 Consider here-string for safety

The main actionable item before merge is the --format=raw hardcoding. The rest are lower priority.

[Hazanel]

Hey @coderabbitai

Thanks for the thorough review:

🔴 virt-cat --format=raw hardcoded

By the time this code runs, virt-v2v has already completed conversion — all disks are raw at this point regardless of the source format (vSphere, OVA, etc.). The --format=raw flag is correct and intentional.

🔴 Only first disk is read — no fallback

Windows always places C:\Windows\System32\config\SYSTEM on the boot disk, which is Disks[0]. Added a comment documenting this assumption explicitly. Iterating through all disks would add complexity with no practical benefit — if the boot disk isn't Disks[0], we have bigger problems than advanced network settings.

🟡 injectComplementryStaticIPTemplate not refactored to use renderTemplate

Good catch. Refactored — extended renderTemplate with a variadic funcMaps ...template.FuncMap parameter so the custom funcs (lower, add, len, formatIPs, formatDNS) pass through cleanly.

🟡 PowerShell JSON injection — single-quote assumption

The JSON is produced by json.MarshalIndent from our own Go structs (AdvancedNetSettings), which contain only MAC addresses (AA:BB:CC:DD:EE:FF), GUIDs ({...}), and uint32 values. None of these can contain single quotes.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@Hazanel — all four points addressed, thanks for the detailed responses!

• --format=raw: Makes complete sense — I missed that virt-v2v's output is always raw. The flag is correct.
• Disks[0] assumption: Agreed, a comment is the right call here. If the boot disk isn't first, the migration itself would be broken long before advanced network settings matter.
• injectComplementryStaticIPTemplate refactor: Variadic funcMaps ...template.FuncMap is a clean extension — nicely done.
• PS1 single-quote: Valid. The data model is constrained to hex MACs, {GUID} strings, and uint32 — no injection surface.

The implementation looks solid. 🐇

---

✏️ Learnings added

Learnt from: Hazanel
URL: https://github.com/kubev2v/forklift/pull/6414

Timestamp: 2026-05-17T06:47:39.639Z
Learning: In the forklift/kubev2v project, by the time advancednet.ReadAdvancedNetworkSettings runs (after virt-v2v conversion in cmd/virt-v2v/entrypoint.go), all output disks are already in raw format regardless of the source format (vSphere, OVA, qcow2, etc.). The `--format=raw` flag in the `virt-cat` call inside pkg/virt-v2v/customize/advancednet/reader.go is therefore correct and intentional.

---

Learnt from: Hazanel
URL: https://github.com/kubev2v/forklift/pull/6414

Timestamp: 2026-05-17T06:47:39.639Z
Learning: In the forklift/kubev2v project, Windows always places C:\Windows\System32\config\SYSTEM on the boot disk, which is always Disks[0] in the virt-v2v output. Reading only Disks[0] for the Windows SYSTEM hive in advancednet.ReadAdvancedNetworkSettings (cmd/virt-v2v/entrypoint.go) is correct by design.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 52.87714% with 303 lines in your changes missing coverage. Please review.
✅ Project coverage is 10.52%. Comparing base (f1fe5d0) to head (d07c164).
⚠️ Report is 2505 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/virt-v2v/customize/advancednet/reader.go	48.61%	95 Missing and 35 partials ⚠️
pkg/virt-v2v/customize/advancednet/regf.go	59.49%	84 Missing and 44 partials ⚠️
pkg/virt-v2v/customize/customize.go	38.29%	25 Missing and 4 partials ⚠️
cmd/virt-v2v/entrypoint.go	0.00%	14 Missing ⚠️
pkg/virt-v2v/customize/advancednet/types.go	84.61%	1 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6414      +/-   ##
==========================================
- Coverage   15.45%   10.52%   -4.94%     
==========================================
  Files         112      522     +410     
  Lines       23377    62366   +38989     
==========================================
+ Hits         3613     6564    +2951     
- Misses      19479    55172   +35693     
- Partials      285      630     +345     

Flag	Coverage Δ
unittests	10.52% <52.87%> (-4.94%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
7.4% Duplication on New Code

See analysis details on SonarQube Cloud