From c8e0de4dc13452c04973afcb892260013c387d8c Mon Sep 17 00:00:00 2001 From: cproudlock Date: Mon, 31 Aug 2026 17:40:21 -0400 Subject: [PATCH] Let the server decide which NIC is a PC's address, and keep the rest A CMM reported 100.0.0.1 as its address and never reported 10.48.255.210 at all. Both halves of that were the same mistake. THE BAY WAS CHOOSING. The collector schema took a single `ipaddress`, so the script picked one NIC and dropped the others - preferring an address inside a configured range, else whichever interface carried a default route. A bay cannot make that judgement: it does not know the site's addressing. On this CMM the Marvell AQtion 10Gbit link to its controller is a real hardware NIC, up, and carries a default route of its own, so it satisfied every test the script had. It won, and the corporate address was discarded before the request was even built - which is why it is not in ShopDB even as a second row. So the bay now reports EVERY connected hardware NIC with a routable IPv4, and takes no view. The server picks the primary from collector_corporate_ranges, a new per-site setting, and writes a Communication row for each of the others. Naming the ranges is what turns the answer from a guess into a decision; with none set it still falls back to the default-route flag and then to the first reported, both guesses, both better than before only because nothing is thrown away now. Rows are keyed on the MAC where the bay sends one, because an address moves between adapters and a MAC does not. A NIC that is down at check-in is left alone rather than deleted - being down is not evidence the record is wrong, and hand-entered gateway, DNS and serial settings live on those same rows. Exactly one row ends up flagged primary, and only a row this report touched can claim it, so a check-in cannot silently demote an address set by hand. -AllowedRanges and the CollectorRanges registry value are now accepted and ignored, logging a line that says where the setting moved. The parameter stays because a scheduled task still passing it would otherwise fail on an unknown argument. A bay on an older collector sends one address and no interfaces; that keeps working and is pinned by a test. Scope: product --- .../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