Server-side NIC selection for the collector

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 NIC. This script reports ALL of them and lets the server decide
-#   corporate ranges (-AllowedRanges, or the CollectorRanges registry value); with
+#   which is the PC's address, from the collector_corporate_ranges setting.
-#   none configured the NIC carrying the DEFAULT ROUTE is used, which expresses
+#   Choosing here was wrong: a bay cannot know a site's addressing, and a CMM
-#   the same intent without knowing any site's addressing.
+#   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
+# -AllowedRanges and the CollectorRanges registry value are ACCEPTED AND
-# unconfigured site falls through to the default-route NIC below (ADR-015).
+# 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
-# NOT named $allowedRanges: PowerShell variable names are case-insensitive, so
+# takes no view. The parameter stays so a scheduled task still passing it does
-# that collides with the [string] parameter above and the array is silently
+# not fail on an unknown argument.
-# COERCED to a string. .Count on a string is 1, so the script then believes a
+if ($AllowedRanges) {
-# range is configured and never falls back to the default route.
+    Log 'NOTE -AllowedRanges is ignored; set collector_corporate_ranges in ShopDB settings instead.'
-$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
 }
 
 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
+# EVERY connected hardware NIC with a routable IPv4, and no opinion about
-# single ipaddress string, so report the corporate NIC and drop any
+# which one matters. Choosing used to happen here and this was the wrong place
-# controller/machine-LAN NIC. With ranges configured the IP must be in one;
+# for it: a bay cannot know a site's addressing. A CMM whose Marvell AQtion
-# with none, the NIC carrying the default route is the routable one by
+# 10Gbit link to its controller sits on 100.0.0.1 reported THAT as the PC's
-# definition, which needs no knowledge of a site's addressing.
+# address - the link carries a default route too - and the corporate address
-$corpIp = ''
+# 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 {
-    try {
+    $defaultRouteIfIndexes = @(Get-NetRoute -DestinationPrefix '0.0.0.0/0' -ErrorAction Stop |
-        $defaultRouteIfIndexes = @(Get-NetRoute -DestinationPrefix '0.0.0.0/0' -ErrorAction Stop |
+                               Select-Object -ExpandProperty InterfaceIndex -Unique)
-                                   Sort-Object RouteMetric |
+} catch {
-                                   Select-Object -ExpandProperty InterfaceIndex -Unique)
+    Log "WARN could not read the route table: $($_.Exception.Message)"
-    } 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) {
+        $interfaces += @{
-            if (Test-InAllowedRange $ipo.IPAddress) { $corpIp = $ipo.IPAddress }
+            ipaddress      = $ipo.IPAddress
-        } elseif ($defaultRouteIfIndexes -contains $ipo.InterfaceIndex) {
+            macaddress     = $adapter.MacAddress
-            $corpIp = $ipo.IPAddress
+            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

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'):
+        # NETWORK. A bay reports every NIC it can see; THIS decides which one
-            ip_comtype = CommunicationType.query.filter_by(comtype='IP').first()
+        # is the PC's address, because only the server knows the site's
-            primary = Communication.query.filter_by(
+        # addressing. A CMM whose 10Gbit link to its controller sits on
-                assetid=comp.assetid, isprimary=True).first()
+        # 100.0.0.1 used to report that as the PC's address and never send the
-            if primary:
+        # corporate one at all - the bay had no way to tell them apart.
-                primary.ipaddress = payload['ipaddress']
+        interfaces = payload.get('interfaces') or []
-            elif ip_comtype:
+        if not interfaces and payload.get('ipaddress'):
-                db.session.add(Communication(
+            # An older bay sends one address and no opinion beyond it.
-                    assetid=comp.assetid, comtypeid=ip_comtype.comtypeid,
+            interfaces = [{'ipaddress': payload['ipaddress']}]
-                    ipaddress=payload['ipaddress'], isprimary=True))
+
+        if interfaces:
+            warnings.extend(_record_interfaces(comp, interfaces))
 
         for app_data in payload.get('installedsoftware', []) or []:
             name = app_data.get('name')

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