From a0ba4dfc6aa7158e15477e30b5f0db9e4b6043f1 Mon Sep 17 00:00:00 2001 From: Cam Proudlock Date: Mon, 31 Aug 2026 17:40:31 -0400 Subject: [PATCH] Server-side NIC selection for the collector --- .../computers/client/Report-AssetToShopDB.ps1 | 108 ++++------- plugins/computers/plugin.py | 172 +++++++++++++++++- .../test_collector_network_interfaces.py | 111 +++++++++++ 3 files changed, 312 insertions(+), 79 deletions(-) create mode 100644 tests/test_plugins/test_collector_network_interfaces.py diff --git a/plugins/computers/client/Report-AssetToShopDB.ps1 b/plugins/computers/client/Report-AssetToShopDB.ps1 index 40b3079e..a673dc9d 100644 --- a/plugins/computers/client/Report-AssetToShopDB.ps1 +++ b/plugins/computers/client/Report-AssetToShopDB.ps1 @@ -31,10 +31,11 @@ # # WHY ONE NIC ONLY: # Some bays carry two NICs - a private controller NIC and the routable -# corporate NIC. Only the routable one belongs in ShopDB. A site may name its -# corporate ranges (-AllowedRanges, or the CollectorRanges registry value); with -# none configured the NIC carrying the DEFAULT ROUTE is used, which expresses -# the same intent without knowing any site's addressing. +# corporate NIC. This script reports ALL of them and lets the server decide +# which is the PC's address, from the collector_corporate_ranges setting. +# Choosing here was wrong: a bay cannot know a site's addressing, and a CMM +# whose 10Gbit controller link carries a default route reported the +# machine-side address as the PC's - with the corporate one never sent. param( # Flask collector endpoint for the computers plugin. Empty resolves from @@ -90,18 +91,6 @@ if (-not $ApiUrl) { exit 0 } -# A site may name its routable ranges rather than rely on the default route. -if (-not $AllowedRanges) { - foreach ($regPath in @('HKLM:\SOFTWARE\GE\ShopDB', 'HKLM:\SOFTWARE\WOW6432Node\GE\ShopDB')) { - try { - if (Test-Path $regPath) { - $v = [string](Get-ItemProperty -Path $regPath -Name CollectorRanges -ErrorAction Stop).CollectorRanges - if ($v -and $v.Trim()) { $AllowedRanges = $v.Trim(); break } - } - } catch {} - } -} - # collector key from the GE-Enforce secret store when not passed via Args. if (-not $ApiKey) { foreach ($regPath in @('HKLM:\SOFTWARE\GE\ShopDB', 'HKLM:\SOFTWARE\WOW6432Node\GE\ShopDB')) { @@ -118,38 +107,13 @@ if (-not $ApiKey) { exit 0 } -# Routable ranges, if this site named any. NO SITE ADDRESSING IS BAKED IN: an -# unconfigured site falls through to the default-route NIC below (ADR-015). -# -# NOT named $allowedRanges: PowerShell variable names are case-insensitive, so -# that collides with the [string] parameter above and the array is silently -# COERCED to a string. .Count on a string is 1, so the script then believes a -# range is configured and never falls back to the default route. -$rangeList = @() -foreach ($cidr in ($AllowedRanges -split ',')) { - $cidr = $cidr.Trim() - if (-not $cidr) { continue } - $parts = $cidr -split '/' - if ($parts.Count -ne 2) { Log "WARN ignoring malformed range '$cidr'"; continue } - $rangeList += @{ Network = $parts[0].Trim(); PrefixLen = [int]$parts[1] } -} - -function ConvertTo-Uint32([string]$ip) { - $bytes = ([System.Net.IPAddress]::Parse($ip)).GetAddressBytes() - [Array]::Reverse($bytes) - return [BitConverter]::ToUInt32($bytes, 0) -} - -function Test-InAllowedRange([string]$ip) { - try { - $ipInt = ConvertTo-Uint32 $ip - foreach ($r in $rangeList) { - $netInt = ConvertTo-Uint32 $r.Network - $mask = [uint32]([math]::Pow(2, 32) - [math]::Pow(2, 32 - $r.PrefixLen)) - if (($ipInt -band $mask) -eq ($netInt -band $mask)) { return $true } - } - } catch {} - return $false +# -AllowedRanges and the CollectorRanges registry value are ACCEPTED AND +# IGNORED. Deciding which NIC is the PC's address moved to the server, where +# the site's addressing actually lives: this script now reports every NIC and +# takes no view. The parameter stays so a scheduled task still passing it does +# not fail on an unknown argument. +if ($AllowedRanges) { + Log 'NOTE -AllowedRanges is ignored; set collector_corporate_ranges in ShopDB settings instead.' } Log '=== Report asset to ShopDB (collector) ===' @@ -299,42 +263,48 @@ try { Log "WARN could not read make/model: $($_.Exception.Message)" } -# The routable IPv4 - a physical, connected NIC. The collector schema takes a -# single ipaddress string, so report the corporate NIC and drop any -# controller/machine-LAN NIC. With ranges configured the IP must be in one; -# with none, the NIC carrying the default route is the routable one by -# definition, which needs no knowledge of a site's addressing. -$corpIp = '' +# EVERY connected hardware NIC with a routable IPv4, and no opinion about +# which one matters. Choosing used to happen here and this was the wrong place +# for it: a bay cannot know a site's addressing. A CMM whose Marvell AQtion +# 10Gbit link to its controller sits on 100.0.0.1 reported THAT as the PC's +# address - the link carries a default route too - and the corporate address +# was never sent at all, so ShopDB could not even show it as a second row. +# +# The server picks the primary now, from the site's configured corporate +# ranges, and records the rest. Report what is here; let it decide. +$interfaces = @() $defaultRouteIfIndexes = @() -if ($rangeList.Count -eq 0) { - try { - $defaultRouteIfIndexes = @(Get-NetRoute -DestinationPrefix '0.0.0.0/0' -ErrorAction Stop | - Sort-Object RouteMetric | - Select-Object -ExpandProperty InterfaceIndex -Unique) - } catch { - Log "WARN could not read the route table: $($_.Exception.Message)" - } +try { + $defaultRouteIfIndexes = @(Get-NetRoute -DestinationPrefix '0.0.0.0/0' -ErrorAction Stop | + Select-Object -ExpandProperty InterfaceIndex -Unique) +} catch { + Log "WARN could not read the route table: $($_.Exception.Message)" } try { $ipObjs = Get-NetIPAddress -AddressFamily IPv4 -ErrorAction Stop | Where-Object { $_.IPAddress -notmatch '^169\.254' -and $_.IPAddress -ne '127.0.0.1' } foreach ($ipo in $ipObjs) { - if ($corpIp) { break } $adapter = $null try { $adapter = Get-NetAdapter -InterfaceIndex $ipo.InterfaceIndex -ErrorAction Stop } catch {} if (-not $adapter) { continue } if (-not $adapter.HardwareInterface) { continue } if ($adapter.Status -ne 'Up') { continue } - if ($rangeList.Count -gt 0) { - if (Test-InAllowedRange $ipo.IPAddress) { $corpIp = $ipo.IPAddress } - } elseif ($defaultRouteIfIndexes -contains $ipo.InterfaceIndex) { - $corpIp = $ipo.IPAddress + $interfaces += @{ + ipaddress = $ipo.IPAddress + macaddress = $adapter.MacAddress + adaptername = $adapter.InterfaceDescription + isdefaultroute = [bool]($defaultRouteIfIndexes -contains $ipo.InterfaceIndex) } } } catch { Log "WARN interface enumeration failed: $($_.Exception.Message)" } -if (-not $corpIp) { Log 'WARN no routable IPv4 NIC found; posting identity without ipaddress.' } +if ($interfaces.Count -eq 0) { + Log 'WARN no routable IPv4 NIC found; posting identity without any interface.' +} else { + Log ("Reporting {0} NIC(s): {1}" -f $interfaces.Count, + (($interfaces | ForEach-Object { $_.ipaddress }) -join ', ')) +} # collector schema fields (lowercase concatenated). hostname is required; the # rest are sent only when present so a partial read never blanks a good value. @@ -348,7 +318,7 @@ if ($model) { $body['modelnumber'] = $model } if ($osVersion) { $body['osname'] = $osVersion } if ($lastBootTime) { $body['lastboottime'] = $lastBootTime } if ($loggedInUser) { $body['loggedinuser'] = $loggedInUser } -if ($corpIp) { $body['ipaddress'] = $corpIp } +if ($interfaces.Count -gt 0) { $body['interfaces'] = $interfaces } $body['lastcheckin'] = (Get-Date).ToUniversalTime().ToString('yyyy-MM-ddTHH:mm:ss') $json = $body | ConvertTo-Json -Compress -Depth 4 diff --git a/plugins/computers/plugin.py b/plugins/computers/plugin.py index da4eb5aa..5e44aef6 100644 --- a/plugins/computers/plugin.py +++ b/plugins/computers/plugin.py @@ -1,5 +1,6 @@ """Computers plugin main class.""" +import ipaddress import json import logging from pathlib import Path @@ -59,6 +60,115 @@ PARTMARKER_LINK_ORIGIN = 'collector:partmarker' MACHINE_CLAIM_QUIET_HOURS = 24 +def _corporate_ranges(): + """CIDRs this site considers its routable network, from settings. + + Empty means the site has not said, and the fallback below applies. Kept + server-side rather than pushed to the bay: a bay cannot know a site's + addressing, and baking a range into the collector script would be an + ADR-015 violation the moment a second site ran it. + """ + from shopdb.api import Setting + raw = '' + try: + raw = Setting.get('collector_corporate_ranges') or '' + except Exception: + return [] + ranges = [] + for cidr in str(raw).split(','): + cidr = cidr.strip() + if not cidr: + continue + try: + ranges.append(ipaddress.ip_network(cidr, strict=False)) + except ValueError: + continue + return ranges + + +def _pick_primary(interfaces, ranges): + """Which reported NIC is the PC's address. + + In order: inside a configured corporate range; else the one the bay says + carries the default route; else the first. The last is a guess and is the + reason configuring the ranges is worth doing - it is what a site sets so + the answer stops being a guess. + """ + def addr(entry): + try: + return ipaddress.ip_address((entry.get('ipaddress') or '').strip()) + except ValueError: + return None + + if ranges: + for entry in interfaces: + parsed = addr(entry) + if parsed and any(parsed in net for net in ranges): + return entry + for entry in interfaces: + if entry.get('isdefaultroute'): + return entry + return interfaces[0] if interfaces else None + + +def _record_interfaces(comp, interfaces): + """Write one Communication row per reported NIC, flagging exactly one. + + Keyed on the MAC where the bay sent one, because an address moves between + adapters and a MAC does not. Falls back to the address for a bay that + reports no MAC, which is what the single-ipaddress payload looks like. + + Rows this PC has that the bay did NOT report are left alone: a NIC that is + down at check-in time is not evidence the record is wrong, and hand-entered + detail (gateway, DNS, serial settings) lives on these same rows. + """ + # Imported here for the same reason the caller does: these come through the + # shopdb.api contract surface, not from core internals. + from shopdb.api import Communication, CommunicationType, db + + warnings = [] + ip_comtype = CommunicationType.query.filter_by(comtype='IP').first() + if not ip_comtype: + return ['no IP communication type seeded; network not recorded'] + + usable = [entry for entry in interfaces if (entry.get('ipaddress') or '').strip()] + if not usable: + return warnings + + primary = _pick_primary(usable, _corporate_ranges()) + existing = Communication.query.filter_by(assetid=comp.assetid).all() + bymac = {(c.macaddress or '').strip().lower(): c for c in existing if c.macaddress} + byip = {(c.ipaddress or '').strip(): c for c in existing if c.ipaddress} + + touched = [] + for entry in usable: + addr = (entry.get('ipaddress') or '').strip() + mac = (entry.get('macaddress') or '').strip() + row = bymac.get(mac.lower()) if mac else None + if row is None: + row = byip.get(addr) + if row is None: + row = Communication(assetid=comp.assetid, comtypeid=ip_comtype.comtypeid) + db.session.add(row) + row.ipaddress = addr + if mac: + row.macaddress = mac + touched.append((row, entry is primary)) + + # Exactly one primary, and only among rows this report touched. An untouched + # row keeps its flag only if nothing here claimed it, which is what stops a + # check-in from silently demoting an address somebody set by hand. + for row, isprimary in touched: + row.isprimary = isprimary + if any(isprimary for _, isprimary in touched): + claimed = {id(row) for row, _ in touched} + for row in existing: + if id(row) not in claimed: + row.isprimary = False + + return warnings + + class ComputersPlugin(BasePlugin): """ Computers plugin - manages PC, server, and workstation assets. @@ -362,7 +472,27 @@ class ComputersPlugin(BasePlugin): 'loggedinuser': {'type': 'string'}, 'lastboottime': {'type': 'string', 'format': 'date-time'}, 'lastcheckin': {'type': 'string', 'format': 'date-time'}, + # Kept for bays on an older collector. A payload carrying + # both wins on `interfaces`. 'ipaddress': {'type': 'string'}, + # EVERY routable IPv4 the bay can see, and no opinion about + # which one matters. Choosing was the bay's job and it was the + # wrong place for it: the bay cannot know a site's addressing, + # so a CMM with a 10Gbit link to its controller reported the + # machine-side address as the PC's, and the corporate address + # was never sent at all. The server picks the primary now, + # using collector_corporate_ranges, and records the rest. + 'interfaces': { + 'type': 'array', + 'items': { + 'ipaddress': 'string', + 'macaddress': 'string', + 'adaptername': 'string', + 'isdefaultroute': 'boolean', + }, + 'description': ('All connected hardware NICs with a ' + 'routable IPv4. Order carries no meaning.'), + }, 'vendorname': {'type': 'string'}, 'modelnumber': {'type': 'string'}, 'osname': {'type': 'string'}, @@ -446,6 +576,26 @@ class ComputersPlugin(BasePlugin): site that installed an earlier one. """ return [ + { + # Which addresses are THIS site's routable network. The + # collector reports every NIC and takes no view; this decides + # which one is the PC's address. + # + # Empty falls back to the NIC the bay says carries the default + # route, then to the first reported - both guesses. A CMM with + # a 10Gbit link to its controller is the case that shows why: + # that link can carry a default route too, and the machine-side + # address then wins over the corporate one. + # + # Comma-separated CIDRs, e.g. '10.48.0.0/16,10.20.0.0/23'. + 'key': 'collector_corporate_ranges', + 'value': '', + 'valuetype': 'string', + 'category': 'computers', + 'description': ('Comma-separated CIDRs naming this site routable ' + 'network. The collector reports every NIC; this ' + 'picks which is the primary address.'), + }, { 'key': 'computers_quietreporthours', 'value': '24', @@ -646,16 +796,18 @@ class ComputersPlugin(BasePlugin): if payload.get('pcsubtype'): warnings.append('pcsubtype received but not stored (no model field)') - if payload.get('ipaddress'): - ip_comtype = CommunicationType.query.filter_by(comtype='IP').first() - primary = Communication.query.filter_by( - assetid=comp.assetid, isprimary=True).first() - if primary: - primary.ipaddress = payload['ipaddress'] - elif ip_comtype: - db.session.add(Communication( - assetid=comp.assetid, comtypeid=ip_comtype.comtypeid, - ipaddress=payload['ipaddress'], isprimary=True)) + # NETWORK. A bay reports every NIC it can see; THIS decides which one + # is the PC's address, because only the server knows the site's + # addressing. A CMM whose 10Gbit link to its controller sits on + # 100.0.0.1 used to report that as the PC's address and never send the + # corporate one at all - the bay had no way to tell them apart. + interfaces = payload.get('interfaces') or [] + if not interfaces and payload.get('ipaddress'): + # An older bay sends one address and no opinion beyond it. + interfaces = [{'ipaddress': payload['ipaddress']}] + + if interfaces: + warnings.extend(_record_interfaces(comp, interfaces)) for app_data in payload.get('installedsoftware', []) or []: name = app_data.get('name') diff --git a/tests/test_plugins/test_collector_network_interfaces.py b/tests/test_plugins/test_collector_network_interfaces.py new file mode 100644 index 00000000..3f6039c9 --- /dev/null +++ b/tests/test_plugins/test_collector_network_interfaces.py @@ -0,0 +1,111 @@ +"""Which NIC becomes the PC's address, and what happens to the others. + +The bay used to choose. It cannot: it does not know the site's addressing. A +CMM whose Marvell AQtion 10Gbit link to its controller sits on 100.0.0.1 +reported THAT as the PC's address - the link carries a default route too - and +the corporate address was never sent at all, so ShopDB could not show it even +as a second row. + +The bay now reports every NIC and takes no view. These pin the server side. +""" + +import pytest + +from shopdb.core.models import Asset, AssetType, Communication, CommunicationType + +KEY = 'network-key' +CORP = '10.48.255.210' +MACHINE_LINK = '100.0.0.1' + + +@pytest.fixture +def collector_key(app): + old = app.config.get('COLLECTOR_API_KEY') + app.config['COLLECTOR_API_KEY'] = KEY + yield KEY + app.config['COLLECTOR_API_KEY'] = old + + +@pytest.fixture +def rig(db): + if not AssetType.query.filter_by(assettype='computer').first(): + db.session.add(AssetType(assettype='computer')) + if not CommunicationType.query.filter_by(comtype='IP').first(): + db.session.add(CommunicationType(comtype='IP')) + db.session.commit() + + +def _report(client, key, hostname, interfaces): + return client.post('/api/collector/computers', + json={'hostname': hostname, 'interfaces': interfaces}, + headers={'X-API-Key': key}) + + +def _comms(hostname): + asset = Asset.query.filter_by(assetnumber=hostname).first() + return Communication.query.filter_by(assetid=asset.assetid).all() + + +BENS_CMM = [ + {'ipaddress': MACHINE_LINK, 'macaddress': 'AA:BB:CC:00:00:01', + 'adaptername': 'Marvell AQtion 10Gbit Network Adapter', 'isdefaultroute': True}, + {'ipaddress': CORP, 'macaddress': 'AA:BB:CC:00:00:02', + 'adaptername': 'Intel I219-LM', 'isdefaultroute': True}, +] + + +def test_both_addresses_are_recorded(client, db, rig, collector_key): + """The one that is not primary must still EXIST. It used to be discarded.""" + _report(client, collector_key, 'CMM01', BENS_CMM) + addresses = {c.ipaddress for c in _comms('CMM01')} + assert addresses == {MACHINE_LINK, CORP} + + +def test_the_configured_range_decides_the_primary(client, db, rig, collector_key): + """With the site's range named, the machine link cannot win.""" + from shopdb.core.models import Setting + Setting.set('collector_corporate_ranges', '10.48.0.0/16', + valuetype='string', category='computers') + db.session.commit() + + _report(client, collector_key, 'CMM02', BENS_CMM) + primary = [c for c in _comms('CMM02') if c.isprimary] + assert len(primary) == 1 + assert primary[0].ipaddress == CORP + + +def test_exactly_one_primary_even_when_both_claim_a_default_route( + client, db, rig, collector_key): + """Ben's CMM: two NICs, both routing. Ambiguous, but never two primaries.""" + _report(client, collector_key, 'CMM03', BENS_CMM) + assert sum(1 for c in _comms('CMM03') if c.isprimary) == 1 + + +def test_a_second_checkin_does_not_duplicate_rows(client, db, rig, collector_key): + """Keyed on the MAC, so the same NIC updates rather than accumulating.""" + _report(client, collector_key, 'CMM04', BENS_CMM) + _report(client, collector_key, 'CMM04', BENS_CMM) + assert len(_comms('CMM04')) == 2 + + +def test_an_address_moving_to_another_nic_follows_the_mac( + client, db, rig, collector_key): + """A MAC outlives an address, which is why it is the key.""" + _report(client, collector_key, 'CMM05', BENS_CMM) + moved = [dict(entry) for entry in BENS_CMM] + moved[1]['ipaddress'] = '10.48.255.211' + _report(client, collector_key, 'CMM05', moved) + + rows = _comms('CMM05') + assert len(rows) == 2, 'a changed address must not mint a second row' + assert '10.48.255.211' in {c.ipaddress for c in rows} + + +def test_an_older_bay_sending_one_address_still_works(client, db, rig, collector_key): + """Bays run whatever collector they were imaged with.""" + client.post('/api/collector/computers', + json={'hostname': 'CMM06', 'ipaddress': CORP}, + headers={'X-API-Key': collector_key}) + rows = _comms('CMM06') + assert len(rows) == 1 + assert rows[0].ipaddress == CORP and rows[0].isprimary