diff --git a/agent/backend/devicediscovery/device_discovery.go b/agent/backend/devicediscovery/device_discovery.go index 169bb46f..720a3df3 100644 --- a/agent/backend/devicediscovery/device_discovery.go +++ b/agent/backend/devicediscovery/device_discovery.go @@ -49,6 +49,7 @@ type deviceDiscoveryBackend struct { diodeTargetFromOtel bool diodeDryRun bool diodeDryRunOutputDir string + diodeLogLevel string startTime time.Time proc backend.Commander @@ -88,6 +89,34 @@ func (d *deviceDiscoveryBackend) Configure(logger *slog.Logger, repo policies.Po d.diodeDryRun = backend.ConfigValueOrDefault(config, "dry_run", common.Diode.DryRun) d.diodeDryRunOutputDir = backend.ConfigValueOrDefault(config, "dry_run_output_dir", common.Diode.DryRunOutputDir) + // Precedence: explicit log_level > per-backend debug: true > the agent + // itself running in debug mode. A non-string log_level (YAML + // `log_level: 3`) falls through the type assertion and cannot panic. + // + // The third branch deliberately reads common.Debug rather than + // logger.Enabled(ctx, slog.LevelDebug), which is what the other three + // discovery backends currently do. logger.Enabled is not a reliable proxy + // for debug mode here: when orb.backends.common.otlp.grpc is set, + // agent.go:165-166 replaces the agent logger with a telemetry.multiHandler + // wrapping both the console handler and the otelslog handler. multiHandler + // ORs Enabled across its handlers (telemetry/logs.go:52-59) and the + // otelslog handler applies no level filter, so Enabled(Debug) is true + // whenever OTLP log export is enabled -- with or without -d. That would + // silently start device-discovery at DEBUG and export the full traceback + // plus napalm/netmiko/paramiko/ncclient chatter to the collector. + // + // common.Debug is the explicit state: agent.go:160 sets it from a.debug, + // which cmd/main.go:95 derives as debugFlag || cfg.OrbAgent.Debug.Enable. + // The same fix is owed to networkdiscovery, snmpdiscovery and + // gnmidiscovery; tracked separately. + if logLevel, prs := config["log_level"].(string); prs { + d.diodeLogLevel = logLevel + } else if debug, prs := config["debug"].(bool); prs && debug { + d.diodeLogLevel = "debug" + } else if common.Debug { + d.diodeLogLevel = "debug" + } + if common.Otlp.Grpc != "" { d.diodeOtelEndpoint = common.Otlp.Grpc d.logger.Info("device-discovery using OTLP endpoint", @@ -136,6 +165,12 @@ func (d *deviceDiscoveryBackend) buildArgs() []string { dOptions = append(opts, dOptions...) } + if d.diodeLogLevel != "" { + dOptions = append(dOptions, "--log-level", d.diodeLogLevel) + d.logger.Info("device-discovery using log level", + "log_level", d.diodeLogLevel) + } + if d.diodeOtelEndpoint != "" { dOptions = append(dOptions, "--otel-endpoint", d.diodeOtelEndpoint) } diff --git a/agent/backend/devicediscovery/device_discovery_test.go b/agent/backend/devicediscovery/device_discovery_test.go index afa7222e..fd0bcf71 100644 --- a/agent/backend/devicediscovery/device_discovery_test.go +++ b/agent/backend/devicediscovery/device_discovery_test.go @@ -88,6 +88,8 @@ func TestDeviceDiscoveryBackendStart(t *testing.T) { assert.Contains(t, args, "device-secret", "Expected args to contain diode client secret") assert.Contains(t, args, "--diode-app-name-prefix", "Expected args to contain diode app name prefix flag") assert.Contains(t, args, "device-agent", "Expected args to contain diode app name prefix") + assert.Contains(t, args, "--log-level", "Expected args to contain log level flag") + assert.Contains(t, args, "debug", "Expected args to contain log level value") assert.Contains(t, args, "--otel-endpoint", "Expected args to contain otel endpoint flag") assert.Contains(t, args, "collector:4317", "Expected args to contain otel endpoint value") }) @@ -116,6 +118,7 @@ func TestDeviceDiscoveryBackendStart(t *testing.T) { "agent_name": "device-agent", "dry_run": false, "dry_run_output_dir": "/tmp/device", + "log_level": "debug", }, commons, nil) require.NoError(t, err) diff --git a/agent/backend/devicediscovery/log_level_test.go b/agent/backend/devicediscovery/log_level_test.go new file mode 100644 index 00000000..b56606a6 --- /dev/null +++ b/agent/backend/devicediscovery/log_level_test.go @@ -0,0 +1,196 @@ +package devicediscovery + +import ( + "context" + "io" + "log/slog" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netboxlabs/orb-agent/agent/config" +) + +// discardLogger is a plain Info-level logger; the level is irrelevant to the +// precedence chain now that it keys off common.Debug rather than the handler. +func discardLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{Level: slog.LevelInfo})) +} + +// otlpLikeHandler mimics the otelslog handler that +// telemetry.BuildOTLPLogExporter merges into the agent logger: no level filter, +// so Enabled always reports true. telemetry.multiHandler ORs Enabled across its +// handlers, which is why an OTLP-wrapped agent logger claims Debug is enabled +// even when the agent was never started with -d. +type otlpLikeHandler struct { + slog.Handler +} + +func (otlpLikeHandler) Enabled(context.Context, slog.Level) bool { return true } + +func otlpWrappedLogger() *slog.Logger { + return slog.New(otlpLikeHandler{Handler: slog.NewTextHandler(io.Discard, nil)}) +} + +// flagValue returns the argument immediately following flag. Asserting on +// adjacency is strictly stronger than two separate assert.Contains calls, which +// would pass even if the flag and its value were separated by another option. +func flagValue(t *testing.T, args []string, flag string) (string, bool) { + t.Helper() + for i, arg := range args { + if arg == flag { + require.Less(t, i+1, len(args), "flag %s has no value following it", flag) + return args[i+1], true + } + } + return "", false +} + +func TestConfigureLogLevelPrecedence(t *testing.T) { + tests := []struct { + name string + config map[string]any + agentDebug bool + expected string + }{ + { + name: "explicit log_level wins", + config: map[string]any{"log_level": "error"}, + expected: "error", + }, + { + name: "explicit log_level beats per-backend debug", + config: map[string]any{"log_level": "error", "debug": true}, + expected: "error", + }, + { + name: "explicit log_level beats agent debug", + config: map[string]any{"log_level": "error"}, + agentDebug: true, + expected: "error", + }, + { + name: "per-backend debug implies debug", + config: map[string]any{"debug": true}, + expected: "debug", + }, + { + name: "per-backend debug false does not imply debug", + config: map[string]any{"debug": false}, + expected: "", + }, + { + name: "agent debug implies debug", + config: map[string]any{}, + agentDebug: true, + expected: "debug", + }, + { + name: "nothing set leaves it empty", + config: map[string]any{}, + expected: "", + }, + { + // YAML `log_level: 3` must fall through the type assertion. + name: "non-string log_level does not panic", + config: map[string]any{"log_level": 3}, + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + commons := config.BackendCommons{} + commons.Debug = tt.agentDebug + + d := &deviceDiscoveryBackend{apiProtocol: "http"} + require.NotPanics(t, func() { + require.NoError(t, d.Configure(discardLogger(), nil, tt.config, commons, nil)) + }) + assert.Equal(t, tt.expected, d.diodeLogLevel) + }) + } +} + +// TestConfigureDoesNotTreatOtlpExportAsDebug is the regression guard for the +// review finding on PR #510. Enabling OTLP log export must not, by itself, put +// the backend into debug. +func TestConfigureDoesNotTreatOtlpExportAsDebug(t *testing.T) { + logger := otlpWrappedLogger() + require.True(t, logger.Enabled(context.Background(), slog.LevelDebug), + "precondition: an OTLP-wrapped agent logger reports Debug as enabled") + + commons := config.BackendCommons{} + commons.Otlp.Grpc = "collector:4317" + commons.Debug = false // no -d, no orb.debug.enable + + d := &deviceDiscoveryBackend{apiProtocol: "http"} + require.NoError(t, d.Configure(logger, nil, map[string]any{}, commons, nil)) + + assert.Empty(t, d.diodeLogLevel, + "OTLP log export alone must not enable debug — see telemetry/logs.go:52-59") + + _, found := flagValue(t, d.buildArgs(), "--log-level") + assert.False(t, found, "expected no --log-level when only OTLP export is configured") +} + +// TestConfigureOtlpPlusAgentDebugStillEnablesDebug is the other half: the fix +// must not have broken the legitimate case. +func TestConfigureOtlpPlusAgentDebugStillEnablesDebug(t *testing.T) { + commons := config.BackendCommons{} + commons.Otlp.Grpc = "collector:4317" + commons.Debug = true + + d := &deviceDiscoveryBackend{apiProtocol: "http"} + require.NoError(t, d.Configure(otlpWrappedLogger(), nil, map[string]any{}, commons, nil)) + + assert.Equal(t, "debug", d.diodeLogLevel) +} + +func TestBuildArgsIncludesLogLevel(t *testing.T) { + d := &deviceDiscoveryBackend{apiProtocol: "http"} + require.NoError(t, d.Configure(discardLogger(), nil, + map[string]any{"log_level": "debug"}, config.BackendCommons{}, nil)) + + args := d.buildArgs() + + value, found := flagValue(t, args, "--log-level") + require.True(t, found, "expected --log-level in %v", args) + assert.Equal(t, "debug", value, "--log-level must be immediately followed by its value") +} + +func TestBuildArgsOmitsLogLevelWhenUnset(t *testing.T) { + // Pins the byte-identical default command line: with no log_level and no + // debug anywhere, the arguments must be exactly what they were before this + // change. That is what bounds the blast radius. + d := &deviceDiscoveryBackend{apiProtocol: "http"} + require.NoError(t, d.Configure(discardLogger(), nil, + map[string]any{}, config.BackendCommons{}, nil)) + + args := d.buildArgs() + + _, found := flagValue(t, args, "--log-level") + assert.False(t, found, "expected no --log-level in %v", args) +} + +func TestBuildArgsLogLevelPrecedesOtelEndpoint(t *testing.T) { + // Order matters only in that both must survive; this catches an append that + // accidentally replaces rather than extends. + commons := config.BackendCommons{} + commons.Otlp.Grpc = "collector:4317" + + d := &deviceDiscoveryBackend{apiProtocol: "http"} + require.NoError(t, d.Configure(discardLogger(), nil, + map[string]any{"log_level": "warn"}, commons, nil)) + + args := d.buildArgs() + + logLevel, foundLevel := flagValue(t, args, "--log-level") + require.True(t, foundLevel) + assert.Equal(t, "warn", logLevel) + + endpoint, foundEndpoint := flagValue(t, args, "--otel-endpoint") + require.True(t, foundEndpoint) + assert.Equal(t, "collector:4317", endpoint) +} diff --git a/agent/backend/devicediscovery/normalize_test.go b/agent/backend/devicediscovery/normalize_test.go index ab90af27..de003fc7 100644 --- a/agent/backend/devicediscovery/normalize_test.go +++ b/agent/backend/devicediscovery/normalize_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestParseDeviceDiscoveryLevel(t *testing.T) { @@ -40,6 +41,36 @@ func TestParseDeviceDiscoveryLevel(t *testing.T) { } } +func TestNormalizeDeviceDiscoveryLine_ExpectedTargetFailureWarning(t *testing.T) { + // The verbatim line the Python backend now emits for an unreachable target + // (device_discovery/policy/runner.py, _log_target_failure). If this stops + // parsing, the agent silently falls back to assigning level by pipe and the + // #494 fix regresses from the operator's point of view. + line := "WARNING:device_discovery.policy.runner:Policy lab_mgmt_device_policy, " + + "Hostname 10.0.0.5: Cannot connect to 10.0.0.5" + + msg, attrs, level, ok := normalizeDeviceDiscoveryLine(line, slog.LevelError) + + assert.True(t, ok) + assert.Equal(t, slog.LevelWarn, level) + require.Len(t, attrs, 1) + assert.Equal(t, "module", attrs[0].Key) + assert.Equal(t, "device_discovery.policy.runner", attrs[0].Value.String()) + assert.Contains(t, msg, "Cannot connect to 10.0.0.5") +} + +func TestNormalizeDeviceDiscoveryLine_ContinuationStillFallsBackToStderrLevel(t *testing.T) { + // Known remaining behaviour, recorded as a test rather than left implicit. + // A traceback continuation line has no LEVEL: prefix, so it keeps the + // caller's fallback -- which logLineAdapter sets to ERROR for stderr. The + // generic per-line stderr amplifier is deliberately out of scope for #494; + // the Python side avoids it by emitting exactly one physical line. + _, _, level, ok := normalizeDeviceDiscoveryLine(" self._open()", slog.LevelError) + + assert.False(t, ok) + assert.Equal(t, slog.LevelError, level) +} + func TestNormalizeDeviceDiscoveryLine_Valid(t *testing.T) { msg, attrs, level, ok := normalizeDeviceDiscoveryLine("INFO: mymodule: hello world", slog.LevelDebug) assert.True(t, ok) diff --git a/agent/docker/Dockerfile b/agent/docker/Dockerfile index 39e384cc..5d7e1eed 100644 --- a/agent/docker/Dockerfile +++ b/agent/docker/Dockerfile @@ -88,10 +88,6 @@ FROM netboxlabs/pktvisor:${PKTVISOR_TAG} AS pktvisor ######################################################################## FROM python:3.14-alpine AS python-builder -# Upgrade pip to latest version with fewer vulnerabilities -RUN python -m pip install --upgrade --no-cache-dir pip==26.1.2 && \ - rm -rf /usr/local/lib/python3.14/ensurepip/_bundled/pip-*.whl - # Install the Python backends from in-repo source (was: published PyPI packages # netboxlabs-device-discovery / netboxlabs-orb-worker). Console scripts # device-discovery and orb-worker land in /usr/local/bin. @@ -109,7 +105,13 @@ RUN BUILD_COMMIT="${BUILD_COMMIT}" BUILD_TRACK="${BUILD_TRACK}" \ python /tmp/stamp_version.py "${DEVICE_DISCOVERY_VERSION}" /src/device-discovery/device_discovery/version.py && \ BUILD_COMMIT="${BUILD_COMMIT}" BUILD_TRACK="${BUILD_TRACK}" \ python /tmp/stamp_version.py "${WORKER_VERSION}" /src/worker/worker/version.py -RUN pip3 install --no-cache-dir /src/device-discovery /src/worker +# Uninstall pip once the packages are installed so the site-packages COPY into +# the final stage carries application code only. Copying pip across stages +# merges it file-by-file over the final stage's own pip — two versions with +# different module layouts produce a broken chimera install (pip3 dies with +# ImportError on every invocation). +RUN pip3 install --no-cache-dir /src/device-discovery /src/worker && \ + pip3 uninstall -y pip ######################################################################## # Stage name is referenced by no-cache-filters in the build workflows so @@ -141,7 +143,15 @@ ENV PATH=/opt/orb/files:$PATH ENV PIP_CACHE_DIR=/opt/orb/pip-cache ENV DEFAULT_CONFIG_PATH=/usr/local/share/orb-agent/default_config.yaml -# Copy Python site-packages and binaries from builder (includes pip and package executables) +# The runtime pip: used by the entrypoint's INSTALL_DRIVERS_PATH feature and +# what image scans see. Pinned to a recent release with fewer vulnerabilities; +# pip's self-upgrade cleanly removes the base image's copy first. +RUN python -m pip install --upgrade --no-cache-dir pip==26.2.1 && \ + rm -rf /usr/local/lib/python3.14/ensurepip/_bundled/pip-*.whl + +# Copy Python site-packages and binaries from builder (application packages and +# their console scripts; the builder uninstalled pip so it cannot leak into — +# and version-clash with — this stage's pip above) COPY --from=python-builder /usr/local/lib/python3.14/site-packages /usr/local/lib/python3.14/site-packages COPY --from=python-builder /usr/local/bin /usr/local/bin diff --git a/docs/backends/device_discovery/README.md b/docs/backends/device_discovery/README.md index 9f30e530..caec03f2 100644 --- a/docs/backends/device_discovery/README.md +++ b/docs/backends/device_discovery/README.md @@ -37,7 +37,7 @@ When a target is a modular chassis and the `discover_modules` policy option is e When the `discover_vrfs` policy option is enabled, device-discovery emits a `VRF` entity for each VRF configured on the device and attaches it to the IP addresses and prefixes of the interfaces inside that VRF — see [VRFs](#vrfs) below. Defaults to `false`, so existing operators see zero behaviour change. ## Configuration -The `device_discovery` backend does not require any special configuration, though overriding `host` and `port` values can be specified. The backend will use the `diode` settings specified in the `common` subsection to forward discovery results. +The `device_discovery` backend does not require any special configuration, though overriding `host`, `port` and `log_level` values can be specified. The backend will use the `diode` settings specified in the `common` subsection to forward discovery results. ```yaml @@ -50,11 +50,52 @@ orb: client_secret: ${DIODE_CLIENT_SECRET} agent_name: agent01 device_discovery: - host: 192.168.5.11 # default 0.0.0.0 + host: 192.168.5.11 # default localhost port: 8857 # default 8072 + log_level: ERROR # default INFO ``` +| Parameter | Type | Required | Description | +|:---------:|:----:|:--------:|:-----------:| +| host | str | no | REST API host (default `localhost`) | +| port | int | no | REST API port (default `8072`) | +| log_level | str | no | Log level (default `INFO`) - see [Log level and troubleshooting](#log-level-and-troubleshooting) | + +### Log level and troubleshooting + +`log_level` accepts `trace`, `debug`, `info`, `warn`/`warning`, `error`/`err`/`exception` +and `critical`/`fatal`, case-insensitively. The default is `INFO`. An unrecognised value +falls back to `INFO` and logs a warning rather than failing to start. + +Precedence, highest first: an explicit `log_level`, then `debug: true` on the backend, +then the agent already running at debug (`-d` or `orb.debug.enable`). + +**A host that is reachable but not a manageable device logs exactly one record.** When +a target answers on the scanned port but cannot be logged into, device-discovery emits a +single WARNING per target per run, with no traceback: + +``` +WARNING:device_discovery.policy.runner:Policy my_policy, Hostname 10.0.0.5: Cannot connect to 10.0.0.5 +``` + +Stable phrases to alert or grep on: `Cannot connect to` and `Authentication to device failed`. +A sweep over unreachable address space produces one line per host, not one per traceback frame. + +Tracebacks for these expected failures are emitted only at DEBUG, on a single line with +newlines escaped as `\n`. To read one back: + +```bash +printf '%b\n' "" +``` + +**`log_level: DEBUG` alone is not enough to see debug output.** The agent maps the +backend's `DEBUG:` lines to its own debug level and its root handler drops them at Info. +Use `orb.debug.enable` or `-d` as the single troubleshooting switch; use `log_level` only +to go *quieter* than INFO. Note that DEBUG also raises napalm, netmiko, paramiko and +ncclient verbosity considerably (ncclient dumps multi-line XML), so point it at one +target rather than a subnet. + ## Policy Device discovery policies are broken down into two subsections: `config` and `scope`. @@ -83,6 +124,7 @@ Current supported options: | propagate_defaults_to_prefix_scope | bool | When `True` AND no explicit `defaults.prefix.scope_*` is set, `defaults.site` cascades to `Prefix.scope_site` (the literal placeholder `"undefined"` is skipped) and `defaults.location` cascades to `Prefix.scope_location`. Defaults to `False`. Setting any explicit `defaults.prefix.scope_*` puts the operator in "explicit mode" and the cascade is skipped wholesale. | | discover_vrfs | bool | When `True`, discovers VRFs from the device via the driver's `get_network_instances()` and attaches each VRF to the IP addresses and prefixes of its member interfaces. A discovered VRF takes precedence over the `defaults.*.vrf` / `vrf_ipv4` / `vrf_ipv6` settings for those interfaces; interfaces in the default routing table keep the configured defaults. Defaults to `False`. Only drivers that implement `get_network_instances()` populate VRF data — see the [supported platforms page](./supported_platforms.md#vrfs). See [VRFs](#vrfs) for filtering rules and route-distinguisher handling. | | emit_host_prefixes | bool | Derive a `Prefix` from IPv4 `/32` and IPv6 `/128` addresses. Defaults to `False`: a host prefix only restates the address, which is already emitted as an `IPAddress` entity, so no prefix is derived for them. Set `True` to restore them, e.g. when loopback `/32`s are deliberately tracked as prefixes in NetBox. IPv6 link-local prefixes (`fe80::/10`) are never derived and are unaffected by this option. See [Prefix](#prefix). | +| emit_device_name | bool | Emit `Device.name` from the hostname the driver reported. Defaults to `True`. Set `False` to suppress the name on the matched device so continual discovery stops proposing a hostname rename when the discovered hostname differs from the NetBox name. **Only takes effect when the device is matchable another way** — a scope `netbox_id`, or `defaults.device.asset_tag`; otherwise the name is kept and a warning is logged, because `name` is a primary NetBox device matcher and dropping it unguarded would emit a device NetBox cannot resolve. Matching by `serial` alone does **not** qualify (`Device.serial` is not unique in NetBox). On a virtual-chassis stack only the master's name is suppressed; member names come from `stack_member_name_template`. Mirrors the snmp-discovery option of the same name. | #### Defaults Current supported defaults: diff --git a/docs/backends/snmp_discovery/README.md b/docs/backends/snmp_discovery/README.md index 43ac80a3..527e1550 100644 --- a/docs/backends/snmp_discovery/README.md +++ b/docs/backends/snmp_discovery/README.md @@ -26,7 +26,7 @@ When the `discover_vrfs` policy option is enabled, snmp-discovery emits a `VRF` When the `discover_asset_tags` policy option is enabled, snmp-discovery reads `ENTITY-MIB::entPhysicalAssetID` and populates `asset_tag` on each emitted device — including per-member tags on virtual-chassis stacks. Defaults to `false`, so existing operators see zero behaviour change. Note that NetBox `asset_tag` values are unique and act as the highest-precedence device matcher during ingestion: enable this only if the tags provisioned on your devices are trustworthy and unique. -When the `emit_device_name` policy option is set to `false`, snmp-discovery still walks `sysName` but does **not** emit `Device.name` on the matched device. Use this with a target `netbox_id` / `metadata.source_match` so Diode matches the existing NetBox record without proposing a hostname rename when the device's `sysName` differs from the NetBox name. The name is suppressed only when the device carries a matcher that also travels on its nested references — `source_match` (netbox_id) or `asset_tag`; if neither is present the name is kept and a warning is logged. (Matching by `serial` or `primary_ip` alone does not enable omission: `serial` is not a unique NetBox matcher, and `primary_ip` is stripped from the nested device stubs.) On a virtual-chassis stack the master device's name is suppressed across all its representations (the device and the shared virtual-chassis master reference); member names and the virtual-chassis name are unaffected. Defaults to `true`. +When the `emit_device_name` policy option is set to `false`, snmp-discovery still walks `sysName` but does **not** emit `Device.name` on the matched device. Use this with a target `netbox_id` / `metadata.source_match` so Diode matches the existing NetBox record without proposing a hostname rename when the device's `sysName` differs from the NetBox name. The name is suppressed only when the device carries a matcher that also travels on its nested references — `source_match` (netbox_id) or `asset_tag`; if neither is present the name is kept and a warning is logged. (Matching by `serial` or `primary_ip` alone does not enable omission: `serial` is not a unique NetBox matcher, and `primary_ip` is stripped from the nested device stubs.) The name is suppressed across every representation of the device that reaches the payload: the device itself, the shared virtual-chassis master reference, the nested device stubs on interfaces, and the device reference embedded in its own `primary_ip4`/`primary_ip6`. On a virtual-chassis stack, member names and the virtual-chassis name are unaffected. Defaults to `true`. When a device exposes the relevant MIBs, interfaces also carry their switching configuration: `mode` (`access` / `tagged` / `tagged-all` / unset for routed), the untagged (access/native) VLAN, and the list of tagged VLANs. VLANs referenced on an interface but not present in the device's VLAN database are auto-emitted as VLAN entities so the association is complete in NetBox; this behavior can be disabled via the `create_unknown_vlans` option (see below). Auto-emitted stubs use the placeholder name `VLAN` (e.g. `VLAN42`) because NetBox's `ipam.vlan.name` is required — operators or sibling switches can later overwrite the placeholder via the same vid+group matcher. VLAN discovery uses Q-BRIDGE-MIB (RFC 4363) as the generic source and a Cisco-specific overlay (CISCO-VLAN-MEMBERSHIP-MIB, CISCO-VOICE-VLAN-MIB) on Cisco devices that don't fully implement Q-BRIDGE — see [SNMP Discovery — Supported Platforms](./supported_platforms.md#interface--vlan-associations) for which device classes are covered. @@ -180,10 +180,11 @@ Each target in the `targets` list can include: | auth_passphrase | string | no | SNMPv3 authentication passphrase | | priv_protocol | string | no | SNMPv3 privacy protocol (see [SNMPv3 auth/priv protocols](#snmpv3-authpriv-protocols)) | | priv_passphrase | string | no | SNMPv3 privacy passphrase | +| context_name | string | no | SNMPv3 context name (equivalent to `snmpwalk -n`). Required by devices that expose MIB data in a named context; such devices return an empty walk when it is omitted. Rejected for SNMPv1/v2c. | *Required for SNMPv1/v2c, optional for SNMPv3 -**Note:** Authentication can be specified at the policy level (under `scope.authentication`) as a fallback, or per-target (under each target's `authentication` field). Targets without authentication use the policy-level authentication. Environment variables are supported using `${VAR}` syntax for `community`, `username`, `auth_passphrase`, and `priv_passphrase` fields. +**Note:** Authentication can be specified at the policy level (under `scope.authentication`) as a fallback, or per-target (under each target's `authentication` field). Targets without authentication use the policy-level authentication — this is a wholesale replacement, not a field-level merge: a target with its own `authentication` block does not inherit any individual field, such as `context_name`, from the policy-level block. Environment variables are supported using `${VAR}` syntax for `community`, `username`, `auth_passphrase`, `priv_passphrase`, and `context_name` fields. #### SNMPv3 auth/priv protocols Values are case-sensitive and must be passed as one of the strings in the tables below. @@ -312,7 +313,13 @@ When the target reports 2+ chassis rows in `ENTITY-MIB` (`entPhysicalTable`) wit 3. **N − 1 member `Device` entities** — each named `-` (matching the format `device_discovery` emits, so the same physical stack discovered by both services lands on the same NetBox rows), carrying `vc_position = ` and an inline `virtual_chassis` ref pointing to the same matcher block. Per-member serial comes from `entPhysicalSerialNum` on the member's chassis row; per-member model comes from `entPhysicalModelName` when populated. 4. **Interface / IPAddress entities** — routed to the member that physically owns them. Routing uses `entAliasMappingTable` (RFC 6933) when present, then falls back to ifName parsing: Cisco IOS/IOS-XE/NX-OS 3-tuple (`Gi1/0/1`, `Te2/1/0/3`, etc., including short forms `Te`/`Fo`/`Hu`/`Tw`/`Fi`/`Twe`), Junos FPC, Aruba CX numeric, H3C dashed. Subinterface unit suffixes (`Gi2/0/1.100`) strip to the parent before parsing. -**Member ID derivation.** When `entPhysicalParentRelPos` is populated (`> 0`) it provides the member id directly; otherwise the trailing integer of `entPhysicalName` (`Switch 2`) is used; the final fallback is the ordinal position of the chassis row in the inventory. Master identity is pinned to the **lowest member id present**, regardless of live role — this is required because the Diode plugin resolves an existing `VirtualChassis` via its `unique_master` matcher, and pinning to the lowest id keeps the master Device stable across live stack-role failovers so re-runs upsert the existing VC instead of creating a new one. The other matcher fields used for VC re-identification (asset_tag, primary_ip4/6, name+site+tenant, and `metadata.source_match`) are carried consistently on both the rich master Device and the inline VC `master` ref. +**Member ID derivation.** One scheme is chosen for the whole member set, first usable wins. When `entPhysicalParentRelPos` is populated (`> 0`) and distinct across members it provides the member id directly; otherwise the trailing integer of `entPhysicalName` (`Switch 2`) is used. When neither column can number the members — some stacks report the same position on every chassis row and name them all `Chassis` — the leading number on each chassis's **port descendants** is used, reached by walking `entPhysicalContainedIn` downward (ports are named in the same namespace as `ifName`, e.g. `2/1/24`). That tier is accepted only when every member yields exactly one distinct number, and those numbers are distinct across members and greater than zero; anything else falls through. The final fallback is the ordinal position of the chassis row in the inventory. + +Master identity is pinned to the **lowest member id present**, regardless of live role — this is required because the Diode plugin resolves an existing `VirtualChassis` via its `unique_master` matcher, and pinning to the lowest id keeps the master Device stable across live stack-role failovers so re-runs upsert the existing VC instead of creating a new one. The other matcher fields used for VC re-identification (asset_tag, primary_ip4/6, name+site+tenant, and `metadata.source_match`) are carried consistently on both the rich master Device and the inline VC `master` ref. + +**When the device contradicts itself.** A signal that is merely absent falls through to the next tier, and the ordinal fallback always yields ids. But a device that reports the *same* position on two chassis rows, or the same trailing number in two names, has asserted something impossible. If no other tier can resolve such a set, the stack is refused rather than guessed: no `VirtualChassis` and no member Devices are emitted, since inventing a numbering would put wrong `vc_position` values and wrong member Device names into NetBox. + +The master does still receive a serial in that case, taken from the lowest `entPhysicalIndex` chassis row. Note this is a different ordering from the lowest-member-id rule above, and necessarily so: a refused set has no member ids to pin to, and the row index is the only ordering that does not depend on the disputed numbering. Only the numbering was ambiguous — each chassis row's serial was unambiguous — so refusing the structure while dropping the serial would discard a fact the device reported plainly. **Member AssetTag is cleared.** Diode's highest-precedence matcher for `dcim.device` is `asset_tag` (unique). The master Device carries the policy `defaults.asset_tag` value if configured; member Devices have it explicitly cleared so multiple members do not collapse onto one NetBox row through a shared asset tag. Master / standalone AssetTag behaviour from `defaults.asset_tag` is unchanged. When the `discover_asset_tags` option is enabled, members instead receive their own per-row `entPhysicalAssetID` values — only the operator-supplied defaults tag is never replicated to members. diff --git a/docs/config_samples.md b/docs/config_samples.md index 649c692a..1f02cf0e 100644 --- a/docs/config_samples.md +++ b/docs/config_samples.md @@ -513,6 +513,7 @@ orb: # auth_passphrase: "auth-password" # Also supports resolving values from environment variables eg ${SNMP_AUTH_PASSPHRASE} # priv_protocol: "AES" # priv_passphrase: "priv-password"# Also supports resolving values from environment variables eg ${SNMP_PRIV_PASSPHRASE} + # context_name: "mfpdirect" # SNMPv3 context name (snmpwalk -n); required by devices whose MIB data lives in a named context. Rejected for SNMPv1/v2c. discover_once: # will run only once scope: targets: @@ -536,6 +537,7 @@ orb: - `username` - `auth_passphrase` - `priv_passphrase` +- `context_name` ### Device Model Lookup The `lookup_extensions_dir` specifies a directory containing device data YAML files that map SNMP device ObjectIDs (from querying `.1.3.6.1.2.1.1.2.0`) to human-readable device names. This allows snmp-discovery to provide meaningful device identification instead of raw ObjectID values. This only needs to be set if additional or modified files are being provided beyond the ones bundled with orb-agent (under `orb-discovery/snmp-discovery/data/lookup_extensions`). diff --git a/docs/configs/agent_yaml.md b/docs/configs/agent_yaml.md index c794c94a..7cb184a5 100644 --- a/docs/configs/agent_yaml.md +++ b/docs/configs/agent_yaml.md @@ -317,7 +317,7 @@ Values can reference environment variables using `${VAR_NAME}` syntax. Resolutio | Vault secrets manager `auth_args` | All fields | Go agent at startup | | CyberArk secrets manager | `url`, `app_id`, `reason`, `ca_bundle`, `client_cert`, `client_key` | Go agent at startup | | `device_discovery` policy (all fields) | Any string value in `scope` and `defaults` | Python backend at policy execution | -| `snmp_discovery` policy authentication | `community`, `username`, `auth_passphrase`, `priv_passphrase` | Go SNMP backend at policy execution | +| `snmp_discovery` policy authentication | `community`, `username`, `auth_passphrase`, `priv_passphrase`, `context_name` | Go SNMP backend at policy execution | ```yaml # Git config (resolved by Go agent) diff --git a/go.mod b/go.mod index 42d1b67a..b9dda541 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( github.com/eclipse/paho.golang v0.23.0 github.com/go-cmd/cmd v1.4.3 github.com/go-co-op/gocron/v2 v2.20.0 - github.com/go-git/go-git/v5 v5.19.1 + github.com/go-git/go-git/v5 v5.19.2 github.com/go-jose/go-jose/v4 v4.1.4 github.com/go-viper/mapstructure/v2 v2.5.0 github.com/google/uuid v1.6.0 @@ -26,7 +26,7 @@ require ( go.opentelemetry.io/otel/sdk v1.43.0 go.opentelemetry.io/otel/sdk/log v0.19.0 go.opentelemetry.io/proto/otlp v1.10.0 - golang.org/x/sys v0.45.0 + golang.org/x/sys v0.46.0 google.golang.org/grpc v1.82.1 google.golang.org/protobuf v1.36.11 gopkg.in/yaml.v3 v3.0.1 @@ -154,8 +154,8 @@ require ( go.opentelemetry.io/otel/sdk/metric v1.43.0 // indirect go.opentelemetry.io/otel/trace v1.43.0 // indirect go.uber.org/atomic v1.11.0 // indirect - golang.org/x/crypto v0.52.0 // indirect - golang.org/x/net v0.55.0 // indirect + golang.org/x/crypto v0.53.0 // indirect + golang.org/x/net v0.56.0 // indirect golang.org/x/oauth2 v0.36.0 // indirect golang.org/x/sync v0.21.0 // indirect golang.org/x/text v0.39.0 // indirect diff --git a/go.sum b/go.sum index 872e3487..38a561d5 100644 --- a/go.sum +++ b/go.sum @@ -145,8 +145,8 @@ github.com/go-git/go-billy/v5 v5.9.0 h1:jItGXszUDRtR/AlferWPTMN4j38BQ88XnXKbilmm github.com/go-git/go-billy/v5 v5.9.0/go.mod h1:jCnQMLj9eUgGU7+ludSTYoZL/GGmii14RxKFj7ROgHw= github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399 h1:eMje31YglSBqCdIqdhKBW8lokaMrL3uTkpGYlE2OOT4= github.com/go-git/go-git-fixtures/v4 v4.3.2-0.20231010084843-55a94097c399/go.mod h1:1OCfN199q1Jm3HZlxleg+Dw/mwps2Wbk9frAWm+4FII= -github.com/go-git/go-git/v5 v5.19.1 h1:nX27AnaU43/K5bKktKwgBmR9lawoYVe1Ckg0rgzzN00= -github.com/go-git/go-git/v5 v5.19.1/go.mod h1:Pb1v0c7/g8aGQJwx9Us09W85yGoyvSwuhEGMH7zjDKQ= +github.com/go-git/go-git/v5 v5.19.2 h1:wkfn7vOlUBu8ivAWKBWisTiwJK4jYHzTF8Ndv1LyGqY= +github.com/go-git/go-git/v5 v5.19.2/go.mod h1:QqCBE1EFN5ddFmrliLQ3/ntRCUjZU3EJuwuB/jWEHjk= github.com/go-jose/go-jose/v4 v4.1.4 h1:moDMcTHmvE6Groj34emNPLs/qtYXRVcd6S7NHbHz3kA= github.com/go-jose/go-jose/v4 v4.1.4/go.mod h1:x4oUasVrzR7071A4TnHLGSPpNOm2a21K9Kf04k1rs08= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= @@ -359,11 +359,11 @@ go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto= go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE= go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= -golang.org/x/crypto v0.52.0 h1:RMs7fP2rXdep0CftQlK8Uf+kibLm7qkCcradZWYz988= -golang.org/x/crypto v0.52.0/go.mod h1:1QgfPxDqh0T2M/elOJtp9RvuR95kVjir0e6/BvEmGbc= +golang.org/x/crypto v0.53.0 h1:QZ4Muo8THX6CizN2vPPd5fBGHyogrdK9fG4wLPFUsto= +golang.org/x/crypto v0.53.0/go.mod h1:DNLU434OwVakk9PzuwV8w62mAJpRJL3vsgcfp4Qnsio= golang.org/x/net v0.0.0-20211112202133-69e39bad7dc2/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y= -golang.org/x/net v0.55.0 h1:bcvxaJn3e1U6InsFWt1JUq1aSjnRxLzT2rtD2KfkDF8= -golang.org/x/net v0.55.0/go.mod h1:L5U2KuzuOe1lY7Z+aWVIKK6qEeJXnXV9yzGA+WCHJww= +golang.org/x/net v0.56.0 h1:Rw8j/hFzGvJUZwNBXnAtf5sVDVt+65SK2C7IxCxZt5o= +golang.org/x/net v0.56.0/go.mod h1:D3Ku6r+V6JROoZK144D2XfMHFcMq/0zSfLelVTCFKec= golang.org/x/oauth2 v0.36.0 h1:peZ/1z27fi9hUOFCAZaHyrpWG5lwe0RJEEEeH0ThlIs= golang.org/x/oauth2 v0.36.0/go.mod h1:YDBUJMTkDnJS+A4BP4eZBjCqtokkg1hODuPjwiGPO7Q= golang.org/x/sync v0.21.0 h1:HLII4xRRTtCRkxYp4HNFF0Js/Og6q2i++KXbg0gHCwM= @@ -380,11 +380,11 @@ golang.org/x/sys v0.0.0-20210927094055-39ccf1dd6fa6/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20220503163025-988cb79eb6c6/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.45.0 h1:dO4czNzziLiiXplLQgBCEpCvXQ3dnkn0SdaZSYdQ+FY= -golang.org/x/sys v0.45.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= +golang.org/x/sys v0.46.0 h1:noSf2Fq6F8DBgS+LysIkx7rIExoNHJsxOAtPp4rthXw= +golang.org/x/sys v0.46.0/go.mod h1:4GL1E5IUh+htKOUEOaiffhrAeqysfVGipDYzABqnCmw= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= -golang.org/x/term v0.43.0 h1:S4RLU2sB31O/NCl+zFN9Aru9A/Cq2aqKpTZJ6B+DwT4= -golang.org/x/term v0.43.0/go.mod h1:lrhlHNdQJHO+1qVYiHfFKVuVioJIheAc3fBSMFYEIsk= +golang.org/x/term v0.44.0 h1:0rLvDRCtNj0gZkyIXhCyOb2OAzEhLVqc4B+hrsBhrmc= +golang.org/x/term v0.44.0/go.mod h1:7ze4MdzUzLXpSAoFP1H0bOI9aXDqveSvatT5vKcFh2Y= golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= golang.org/x/text v0.39.0 h1:UbZz4pLOvn600D6Oh6GGEI6VAmndrEBLv8/6BEXzyus= golang.org/x/text v0.39.0/go.mod h1:3UwRclnC2g0TU9x8PZiyfOajCd1zaUNHF9cvqcQZ+ZM= diff --git a/orb-discovery/device-discovery/device_discovery/client.py b/orb-discovery/device-discovery/device_discovery/client.py index cced25e7..4b8238ed 100644 --- a/orb-discovery/device-discovery/device_discovery/client.py +++ b/orb-discovery/device-discovery/device_discovery/client.py @@ -15,6 +15,7 @@ ) from device_discovery.entity_metadata import apply_run_id_to_entities +from device_discovery.log_config import configure_default_logging from device_discovery.stubs import prune_nested_refs from device_discovery.translate import translate_data from device_discovery.version import version_semver @@ -24,7 +25,7 @@ MAX_MESSAGE_SIZE_BYTES = 3 * 1024 * 1024 # 3MB threshold for chunking # Set up logging -logging.basicConfig(level=logging.INFO) +configure_default_logging() logger = logging.getLogger(__name__) diff --git a/orb-discovery/device-discovery/device_discovery/device_name.py b/orb-discovery/device-discovery/device_discovery/device_name.py new file mode 100644 index 00000000..e2e37f04 --- /dev/null +++ b/orb-discovery/device-discovery/device_discovery/device_name.py @@ -0,0 +1,90 @@ +#!/usr/bin/env python +# Copyright 2026 NetBox Labs Inc +""" +The ``emit_device_name`` transform. + +device-discovery always emits ``Device.name`` from the hostname the driver +reported. When a policy matches an existing NetBox device by ``netbox_id`` and +that device's discovered hostname differs from its NetBox name, the match +succeeds but the emitted name is a deviation — so continual discovery proposes +a hostname rename on every cycle and it never settles. + +``emit_device_name: false`` suppresses the name while keeping the match. Mirrors +``mapping.ApplyDeviceNameEmission`` in snmp-discovery (see +``orb-discovery/snmp-discovery/mapping/device_name.py``), including its matcher +guard and its master-only scope on virtual-chassis stacks. +""" + +import logging + +from netboxlabs.diode.sdk.diode.v1 import ingester_pb2 as pb + +logger = logging.getLogger(__name__) + + +def device_has_alternative_matcher(device: pb.Device) -> bool: + """ + Report whether ``device`` can be matched without its name. + + Only matchers that also survive onto the nested device stubs built by + ``stubs._device_match_stub`` count: + + - ``metadata["source_match"]`` (written from a scope's ``netbox_id``) + - a non-empty ``asset_tag`` + + ``serial`` is excluded because NetBox ``Device.serial`` is not unique and + generates no matcher at all. ``primary_ip4``/``primary_ip6`` are excluded + because, although ``unique_primary_ip4``/``ip6`` match a top-level device, + the nested stubs drop primary_ip (setting it on a nested stub fails + ingest), so clearing the name would leave every nested reference + unmatchable. + """ + if "source_match" in device.metadata: + return True + return bool(device.asset_tag and device.asset_tag.strip()) + + +def apply_device_name_emission( + device: pb.Device, + emit: bool, + target_hostname: str | None = None, +) -> bool: + """ + Clear ``device.name`` when name emission is disabled. Returns True if cleared. + + No-op when emission is enabled (the default), when the device carries no + name, or when the device is a virtual-chassis member — suppression is + master-only, matching snmp-discovery. Member names come from + ``stack_member_name_template``, which is their own lever. + + When disabled but the device carries no alternative matcher, the name is + KEPT and a warning is logged: ``name`` is a primary device matcher, so + dropping it without a ``source_match`` / ``asset_tag`` would emit a device + NetBox cannot resolve. + + Uses ``ClearField`` rather than assigning ``""``. ``Device.name`` declares + explicit presence, and the Diode plugin treats an explicit empty string as + a deliberate clear — that would wipe the hostname in NetBox instead of + leaving it untouched, which is the opposite of the intent. + """ + if emit: + return False + if not device.HasField("name"): + return False + if device.HasField("vc_position"): + return False + if not device_has_alternative_matcher(device): + logger.warning( + "emit_device_name is disabled but %s has no alternative matcher " + "(netbox_id/source_match, or defaults.device.asset_tag); keeping the " + "name so the device stays resolvable in NetBox", + target_hostname or device.name, + ) + return False + logger.debug( + "emit_device_name: suppressing Device.name %r for %s", + device.name, + target_hostname or "target", + ) + device.ClearField("name") + return True diff --git a/orb-discovery/device-discovery/device_discovery/discovery.py b/orb-discovery/device-discovery/device_discovery/discovery.py index ec20a9db..b89e3887 100644 --- a/orb-discovery/device-discovery/device_discovery/discovery.py +++ b/orb-discovery/device-discovery/device_discovery/discovery.py @@ -12,10 +12,12 @@ from napalm import get_network_driver from napalm.base.base import NetworkDriver +from device_discovery.log_config import configure_default_logging + _DRIVER_MISMATCH_MARKERS = ["%", "Invalid input", "^"] # Set up logging -logging.basicConfig(level=logging.INFO) +configure_default_logging() logger = logging.getLogger(__name__) @@ -184,11 +186,11 @@ def discover_device_driver(info: dict, drivers: list[str] | None = None) -> str "Hostname %s: '%s' driver did not work", info.hostname, driver ) continue - set_napalm_logs_level(logging.INFO) + set_napalm_logs_level(logging.getLogger().getEffectiveLevel()) return driver except Exception as e: logger.info( "Hostname %s: '%s' driver did not work. Exception: %s", info.hostname, driver, str(e) ) - set_napalm_logs_level(logging.INFO) + set_napalm_logs_level(logging.getLogger().getEffectiveLevel()) return None diff --git a/orb-discovery/device-discovery/device_discovery/log_config.py b/orb-discovery/device-discovery/device_discovery/log_config.py new file mode 100644 index 00000000..e0cd4df4 --- /dev/null +++ b/orb-discovery/device-discovery/device_discovery/log_config.py @@ -0,0 +1,127 @@ +#!/usr/bin/env python +# Copyright 2026 NetBox Labs Inc +""" +Logging configuration for the device-discovery backend. + +Centralises the root-logger setup that used to be five independent +``logging.basicConfig`` calls, and provides the ``--log-level`` plumbing. + +Two things here are load-bearing and easy to undo by accident: + +1. ``LOG_FORMAT`` is byte-identical to ``logging.BASIC_FORMAT``. The emitted + shape is a wire contract with ``normalizeDeviceDiscoveryLine`` in + ``agent/backend/devicediscovery/device_discovery.go``, which parses + ``LEVEL:module:message``, and with operator grep habits. Changing it buys + nothing and breaks both. +2. ``configure_logging`` passes ``force=True``. Every module in this package + configures logging at import time, so by the time ``main()`` runs a root + handler already exists and a plain ``basicConfig`` is a silent no-op -- + which is exactly the "accepted and ignored" defect ``--log-level`` exists + to fix. Prior art: ``orb-discovery/worker/worker/main.py``. +""" + +import logging + +# Byte-identical to logging.BASIC_FORMAT. See the module docstring. +LOG_FORMAT = "%(levelname)s:%(name)s:%(message)s" + +DEFAULT_LEVEL = logging.INFO + +# Mirrors parseDeviceDiscoveryLevel in +# agent/backend/devicediscovery/device_discovery.go so that every token the +# agent can forward resolves here instead of silently falling back to INFO. +# +# Deliberately an explicit dict rather than logging.getLevelNamesMapping() +# (3.11+, while pyproject declares requires-python = ">=3.10") or +# logging.getLevelName(), which returns the string "Level TRACE" for trace, +# err and exception -- setLevel raises on those. +_LEVEL_ALIASES = { + "trace": logging.DEBUG, + "debug": logging.DEBUG, + "info": logging.INFO, + "warn": logging.WARNING, + "warning": logging.WARNING, + "error": logging.ERROR, + "err": logging.ERROR, + "exception": logging.ERROR, + "critical": logging.CRITICAL, + "fatal": logging.CRITICAL, +} + + +def flatten_message(value: object) -> str: + """ + Collapse a value's string form onto a single physical line. + + Not cosmetic. netmiko builds its timeout and authentication messages as + ten-line f-strings, and the agent splits the backend's stderr on every + newline and assigns ERROR by pipe rather than by content + (agent/backend/process.go and device_discovery.go). An unflattened + message therefore becomes one WARNING plus nine ERROR records. + """ + return " ".join(str(value).split()) + + +def resolve_log_level(value: object) -> tuple[int, str | None]: + """ + Resolve a log-level value to a logging level, never raising. + + Tolerates non-str input on purpose: the argparse namespace is a MagicMock + under test, and a YAML ``log_level: 3`` reaches us as an int. + + Args: + ---- + value: the raw log-level value, of any type. + + Returns: + ------- + tuple[int, str | None]: the resolved level, and a warning message when + the value was unrecognised (so a degraded setting is visible rather + than silent) or None when it was understood. + + """ + normalized = str(value).strip().lower() + level = _LEVEL_ALIASES.get(normalized) + if level is None: + return DEFAULT_LEVEL, f"unrecognised log level {value!r}, falling back to INFO" + return level, None + + +def configure_logging(value: object) -> int: + """ + Configure root logging from a --log-level value, replacing any existing setup. + + ``force=True`` is required: the import-time ``configure_default_logging`` + calls have already installed a root handler by the time this runs, and + ``basicConfig`` without ``force`` would be a silent no-op. + + Args: + ---- + value: the raw --log-level value. + + Returns: + ------- + int: the logging level that was applied. + + """ + level, warning = resolve_log_level(value) + logging.basicConfig(level=level, format=LOG_FORMAT, force=True) + if warning: + logging.getLogger(__name__).warning(warning) + return level + + +def configure_default_logging() -> None: + """ + Configure root logging at the package default of INFO, if not already configured. + + Called at import time by the modules that used to call ``basicConfig`` + directly. Behaviour is unchanged -- the first call still wins and it is + still INFO -- but the format is now stated once instead of being five + implicit copies of an unchecked wire contract. + + Not removable: these modules are imported standalone by the test suite and + by the FastAPI app, and without this the root logger is left unconfigured + and INFO records vanish under ``logging.lastResort``. + """ + logging.basicConfig(level=DEFAULT_LEVEL, format=LOG_FORMAT) diff --git a/orb-discovery/device-discovery/device_discovery/main.py b/orb-discovery/device-discovery/device_discovery/main.py index 3f064a50..bad11e1a 100644 --- a/orb-discovery/device-discovery/device_discovery/main.py +++ b/orb-discovery/device-discovery/device_discovery/main.py @@ -11,6 +11,7 @@ import uvicorn from device_discovery.client import Client +from device_discovery.log_config import configure_logging from device_discovery.metrics import setup_metrics_export from device_discovery.server import app from device_discovery.version import version_semver @@ -35,11 +36,15 @@ def resolve_env_var(value: str) -> str: return value -def main(): +def build_parser() -> argparse.ArgumentParser: """ - Main entry point for the Agent CLI. + Build the CLI argument parser. - Parses command-line arguments and starts the backend. + Extracted from main() so a test can feed it the exact argument vector the + Go agent's buildArgs emits, pinning both sides of that boundary without a + subprocess. See agent/backend/devicediscovery/device_discovery.go. + + Returns the configured argparse.ArgumentParser. """ parser = argparse.ArgumentParser(description="Orb Device Discovery Backend") parser.add_argument( @@ -129,8 +134,37 @@ def main(): required=False, ) + # Long flag only: -d is --dry-run, and -s -p -t -c -k -a -o -V are taken. + # + # Deliberately no choices=. A typo in the agent's YAML would make argparse + # exit(2), and the backend would crash-loop and never pass readiness -- + # the same class of defect as #494, inverted. Unrecognised values fall + # back to INFO and warn instead. + parser.add_argument( + "--log-level", + help="Log level: trace/debug/info/warn/error/critical (default: INFO)", + type=str, + default="INFO", + required=False, + ) + + return parser + + +def main(): + """ + Main entry point for the Agent CLI. + + Parses command-line arguments and starts the backend. + """ + parser = build_parser() + try: args = parser.parse_args() + # Before the dry-run validation and before setup_metrics_export, + # Client() and uvicorn.run: basicConfig(force=True) replaces the root + # handlers installed at import time. + configure_logging(args.log_level) if not args.dry_run: missing = [ name diff --git a/orb-discovery/device-discovery/device_discovery/metrics.py b/orb-discovery/device-discovery/device_discovery/metrics.py index 8dcf4f3c..22fbaa77 100644 --- a/orb-discovery/device-discovery/device_discovery/metrics.py +++ b/orb-discovery/device-discovery/device_discovery/metrics.py @@ -12,10 +12,11 @@ PeriodicExportingMetricReader, ) +from device_discovery.log_config import configure_default_logging from device_discovery.version import version_semver # Set up logging -logging.basicConfig(level=logging.INFO) +configure_default_logging() logger = logging.getLogger(__name__) # Global variables to store the provider and meter diff --git a/orb-discovery/device-discovery/device_discovery/policy/manager.py b/orb-discovery/device-discovery/device_discovery/policy/manager.py index 734fe1ee..1f3871e3 100644 --- a/orb-discovery/device-discovery/device_discovery/policy/manager.py +++ b/orb-discovery/device-discovery/device_discovery/policy/manager.py @@ -7,12 +7,13 @@ import yaml +from device_discovery.log_config import configure_default_logging from device_discovery.policy.models import Policy, PolicyRequest, PolicyStatus from device_discovery.policy.run import RunStore from device_discovery.policy.runner import PolicyRunner # Set up logging -logging.basicConfig(level=logging.INFO) +configure_default_logging() logger = logging.getLogger(__name__) diff --git a/orb-discovery/device-discovery/device_discovery/policy/models.py b/orb-discovery/device-discovery/device_discovery/policy/models.py index 4a3c1788..c4d22da0 100644 --- a/orb-discovery/device-discovery/device_discovery/policy/models.py +++ b/orb-discovery/device-discovery/device_discovery/policy/models.py @@ -307,6 +307,20 @@ class Options(BaseModel): "keep the configured defaults. Default False." ), ) + emit_device_name: bool = Field( + default=True, + description=( + "Emit Device.name from the hostname the driver reported. " + "Defaults to True. Set False to suppress the name on the matched " + "device so continual discovery stops proposing a hostname rename " + "when the discovered hostname differs from the NetBox name. Only " + "takes effect when the device is matchable another way (a scope " + "netbox_id, or defaults.device.asset_tag); otherwise the name is " + "kept and a warning is logged. On a virtual-chassis stack only the " + "master's name is suppressed; member names come from " + "stack_member_name_template." + ), + ) emit_host_prefixes: bool = Field( default=False, description=( diff --git a/orb-discovery/device-discovery/device_discovery/policy/run.py b/orb-discovery/device-discovery/device_discovery/policy/run.py index d3844dd6..d140b29d 100644 --- a/orb-discovery/device-discovery/device_discovery/policy/run.py +++ b/orb-discovery/device-discovery/device_discovery/policy/run.py @@ -8,6 +8,7 @@ import time from typing import Any +from device_discovery.log_config import flatten_message from device_discovery.policy.models import Run, RunStatus # Maximum number of runs to keep per target @@ -165,7 +166,9 @@ def update_run( run.updated_at = time.time_ns() if error: - run.reason = str(error) + # Flattened so the string in the JSON /api/v1/status + # response is byte-identical to the log line. + run.reason = flatten_message(error) else: run.reason = "" # Clear reason when no error diff --git a/orb-discovery/device-discovery/device_discovery/policy/runner.py b/orb-discovery/device-discovery/device_discovery/policy/runner.py index 4b120f25..49a0612a 100644 --- a/orb-discovery/device-discovery/device_discovery/policy/runner.py +++ b/orb-discovery/device-discovery/device_discovery/policy/runner.py @@ -4,6 +4,7 @@ import logging import time +import traceback import uuid from datetime import datetime, timedelta from typing import Any @@ -13,9 +14,12 @@ from apscheduler.triggers.cron import CronTrigger from apscheduler.triggers.date import DateTrigger from napalm import get_network_driver +from napalm.base.exceptions import ConnectionException +from netmiko.exceptions import NetmikoAuthenticationException from device_discovery.client import Client from device_discovery.discovery import discover_device_driver, supported_drivers +from device_discovery.log_config import configure_default_logging, flatten_message from device_discovery.metrics import get_metric from device_discovery.policy.models import Config, Defaults, Napalm, Options, Status from device_discovery.policy.portscan import ( @@ -25,9 +29,46 @@ from device_discovery.policy.run import RunStatus, RunStore # Set up logging -logging.basicConfig(level=logging.INFO) +configure_default_logging() logger = logging.getLogger(__name__) +# Per-target failures that are routine in a discovery sweep rather than bugs: +# a host that is alive on the scanned port but is not a manageable device. +# These log as ONE WARNING with no traceback; everything else keeps ERROR plus +# full exc_info so real defects stay diagnosable. +# +# Both entries are required. Do not collapse this tuple: +# * napalm's NetworkDriver._netmiko_open (napalm/base/base.py) catches ONLY +# NetMikoTimeoutException and re-raises ConnectionException. Nothing there +# catches authentication failure. +# * netmiko declares NetmikoAuthenticationException on paramiko's +# AuthenticationException (netmiko/exceptions.py), so it is not a +# NapalmException at all -- issubclass(..., ConnectionException) is False. +# A rejected credential therefore propagates raw, and dropping the second +# entry restores the full ~63-record traceback wall for exactly the case +# reported in #494 ("unable to log into the device with the provided +# credentials"). +# +# ConnectionException already subsumes ConnectAuthError, ConnectTimeoutError, +# ConnectionClosedException and UnsupportedVersion, so they are not listed. +# Builtin TimeoutError / ConnectionError / socket.gaierror are deliberately +# EXCLUDED: Client().ingest runs inside the same catch-all, so widening this +# would relabel a dead Diode endpoint as a per-host connection failure and +# send the operator hunting devices while ingestion is down. +_EXPECTED_TARGET_FAILURES = (ConnectionException, NetmikoAuthenticationException) + + +def _is_expected_target_failure(error: BaseException) -> bool: + """ + Report whether a per-target failure is an expected unreachable/unauthenticated host. + + Evaluated on the raised object only. The ``__cause__`` / ``__context__`` + chain is deliberately NOT walked: a driver bug that surfaces while a + connection error is being handled must stay UNEXPECTED and keep its + traceback. + """ + return isinstance(error, _EXPECTED_TARGET_FAILURES) + def _deep_merge(base: dict, override: dict) -> dict: """Recursively merge ``override`` into ``base``; override wins on non-dict conflicts.""" @@ -204,7 +245,7 @@ def _discover_driver(self, scope: Napalm, sanitized_hostname: str, discovery_dri scope.driver = discover_device_driver(scope, drivers=discovery_drivers) if scope.driver is None: self.status = Status.FAILED - logger.error( + logger.warning( f"Policy {self.name}, Hostname {sanitized_hostname}: Not able to discover device driver" ) return False @@ -376,6 +417,49 @@ def _collect_network_instances( "Continuing without VRF data." ) + def _log_target_failure(self, sanitized_hostname: str, error: Exception) -> None: + """ + Log a per-target discovery failure at a level matching how odd it is. + + Expected transport and authentication failures emit exactly ONE + physical line at WARNING with the message flattened and no traceback -- + the agent splits stderr per newline, so one logical record must be one + line. Their traceback is still recoverable at DEBUG, as a single + newline-escaped line. + + Anything unexpected keeps ERROR with full exc_info. Diagnosability of + real bugs must not regress in service of quieting a routine condition. + The discipline matches custom_napalm/junos.py and aruba_aoscx.py: + expected signal goes quiet, unexpected stays loud. + + Args: + ---- + sanitized_hostname: target the failure belongs to. + error: the exception that reached the per-target catch-all. + + """ + message = ( + f"Policy {self.name}, Hostname {sanitized_hostname}: " + f"{flatten_message(error)}" + ) + + if not _is_expected_target_failure(error): + # exc_info takes the instance, not True, so this helper does not + # depend on being called inside an active except block. + logger.error(message, exc_info=error) + return + + logger.warning(message) + if logger.isEnabledFor(logging.DEBUG): + formatted = "".join( + traceback.format_exception(type(error), error, error.__traceback__) + ) + # One physical line: newlines escaped, so the agent's per-line + # stderr split cannot fan this back out. Un-escape with + # printf '%b\n' "" + escaped = formatted.replace("\\", "\\\\").replace("\n", "\\n") + logger.debug(f"{message} | traceback: {escaped}") + def run_scan( self, hostnames: list[str], trigger: BaseTrigger, scope: Napalm, config: Config ): @@ -562,9 +646,7 @@ def run(self, id: str, scope: Napalm, config: Config): discovery_failure = get_metric("discovery_failure") if discovery_failure: discovery_failure.add(1, {"policy": self.name}) - logger.error( - f"Policy {self.name}, Hostname {sanitized_hostname}: {e}", exc_info=True - ) + self._log_target_failure(sanitized_hostname, e) # Still record discovery duration on failure discovery_latency = get_metric("discovery_latency") if discovery_latency: @@ -672,9 +754,7 @@ def run_with_parent( discovery_failure = get_metric("discovery_failure") if discovery_failure: discovery_failure.add(1, {"policy": self.name}) - logger.error( - f"Policy {self.name}, Hostname {sanitized_hostname}: {e}", exc_info=True - ) + self._log_target_failure(sanitized_hostname, e) # Still record discovery duration on failure discovery_latency = get_metric("discovery_latency") if discovery_latency: diff --git a/orb-discovery/device-discovery/device_discovery/stubs.py b/orb-discovery/device-discovery/device_discovery/stubs.py index bc3ab7f5..3866c961 100644 --- a/orb-discovery/device-discovery/device_discovery/stubs.py +++ b/orb-discovery/device-discovery/device_discovery/stubs.py @@ -101,7 +101,18 @@ def _index_top_level_devices(entities: list[Entity]) -> dict[str, pb.Device]: def _resolve_device( nested: pb.Device, index: dict[str, pb.Device] ) -> pb.Device | None: - """Look up the rich top-level Device for a nested reference. Name first, serial fallback.""" + """ + Look up the rich top-level Device for a nested reference. + + Tries, in order: name, serial, ``metadata["source_match"]``, ``asset_tag``. + + The last two exist because ``emit_device_name: false`` unsets ``Device.name`` + on the strength of exactly those two matchers. Without them here, a + suppressed device whose driver reported no serial resolves to nothing, so + every nested ref is left un-pruned and logs a warning — one per interface, + on every discovery cycle. Both fall back only on a unique match, so an + ambiguous index is left unresolved rather than mis-attributed. + """ if nested.name and nested.name in index: return index[nested.name] if nested.serial: @@ -115,6 +126,19 @@ def _resolve_device( matches[0].name, ) return matches[0] + if "source_match" in nested.metadata: + wanted = nested.metadata["source_match"] + matches = [ + d + for d in index.values() + if "source_match" in d.metadata and d.metadata["source_match"] == wanted + ] + if len(matches) == 1: + return matches[0] + if nested.asset_tag: + matches = [d for d in index.values() if d.asset_tag == nested.asset_tag] + if len(matches) == 1: + return matches[0] return None @@ -250,17 +274,36 @@ def _module_match_stub(rich: pb.Module, dev_stub: pb.Device) -> pb.Module: """ Build a matcher-only Module for use as a nested reference. - Keeps just the fields Diode needs to resolve a Module — the chassis - device (matcher-stubbed) and the serial — plus a positional - module_bay reference (device-stubbed) when the rich Module carries - one. Drops module_type, description, asset_tag, status, and any - rich device fields, so that copying this stub into hundreds of - Interface entities does not duplicate the running-config-bearing - Device proto per port. + ``dcim.module`` resolves on ``module_bay`` (its only unique field + besides asset_tag), so the positional module_bay reference — + device-stubbed — is the load-bearing part of this stub, not the + serial. Serial is carried for create fidelity only; it matches + nothing. Drops description, asset_tag, status, and any rich device + fields, so that copying this stub into hundreds of Interface + entities does not duplicate the running-config-bearing Device proto + per port. + + ``module_type`` is retained even though it plays no part in matching: + when a nested reference does not resolve, Diode falls back to + creating the Module, and NetBox rejects a Module without one + (``Field module_type is required``). Dropping it cost 48 failed + plans per run on a 6-linecard chassis, and those failures take every + other entity in the same batch down with them. Only the + ``(manufacturer, model)`` pair is copied — the complete matcher for + ``dcim.moduletype`` — rather than the rich ModuleType, so the + reference cannot grow as drivers start populating part_number, + attributes or profile. """ stub = pb.Module() copy_scalar_if_set(stub, rich, "serial") stub.device.CopyFrom(dev_stub) + if rich.HasField("module_type"): + stub.module_type.CopyFrom( + pb.ModuleType( + model=rich.module_type.model, + manufacturer=pb.Manufacturer(name=rich.module_type.manufacturer.name), + ), + ) if rich.HasField("module_bay"): bay_stub = pb.ModuleBay(name=rich.module_bay.name) copy_scalar_if_set(bay_stub, rich.module_bay, "position") diff --git a/orb-discovery/device-discovery/device_discovery/translate.py b/orb-discovery/device-discovery/device_discovery/translate.py index 8b74c150..9a6bb59d 100644 --- a/orb-discovery/device-discovery/device_discovery/translate.py +++ b/orb-discovery/device-discovery/device_discovery/translate.py @@ -25,6 +25,7 @@ slugify, ) +from device_discovery.device_name import apply_device_name_emission from device_discovery.interface import build_interface_entities from device_discovery.policy.models import ( Defaults, @@ -724,6 +725,14 @@ def translate_data(data: dict) -> Iterable[Entity]: device = translate_device( device_info, defaults, config_info, options, netbox_id=netbox_id ) + # Suppress Device.name (emit_device_name: false) BEFORE the deepcopy + # below, so every nested interface/IP reference derived from it inherits + # the suppression rather than carrying the name we just dropped. + # translate_device has already stamped metadata.source_match from + # netbox_id, so the matcher guard can see it. + apply_device_name_emission( + device, options.emit_device_name, target_hostname + ) device_for_interfaces = copy.deepcopy(device) device_for_interfaces.ClearField("config") # Emit Module / ModuleBay entities into a separate list so the diff --git a/orb-discovery/device-discovery/device_discovery/translate_chassis.py b/orb-discovery/device-discovery/device_discovery/translate_chassis.py index 79f61c4a..08b3cd51 100644 --- a/orb-discovery/device-discovery/device_discovery/translate_chassis.py +++ b/orb-discovery/device-discovery/device_discovery/translate_chassis.py @@ -28,6 +28,7 @@ from netboxlabs.diode.sdk.diode.v1 import ingester_pb2 as pb from netboxlabs.diode.sdk.ingester import Entity +from device_discovery.device_name import apply_device_name_emission from device_discovery.interface import build_interface_entities from device_discovery.policy.models import Defaults, Options from device_discovery.proto_presence import copy_scalar_if_set @@ -409,6 +410,18 @@ def translate_as_stack( if master_iface_entities: assign_primary_ip(master_dev, master_iface_entities, target_hostname) + # Suppress the master's name (emit_device_name: false) BEFORE deriving + # vc_master_ref. _master_device_ref copies the name through + # copy_scalar_if_set, which skips an unset field, so the shared VC master + # ref inherits the suppression instead of resurrecting the name and + # recreating the divergence _master_device_ref exists to prevent. + # + # Master only: apply_device_name_emission no-ops on any device carrying + # vc_position, so member names are untouched. + apply_device_name_emission( + master_dev, options.emit_device_name, target_hostname + ) + # NOW derive vc_master_ref — captures master's primary_ip4/6 if assigned. vc_master_ref = _master_device_ref(master_dev) diff --git a/orb-discovery/device-discovery/tests/conftest.py b/orb-discovery/device-discovery/tests/conftest.py new file mode 100644 index 00000000..adc358f3 --- /dev/null +++ b/orb-discovery/device-discovery/tests/conftest.py @@ -0,0 +1,31 @@ +#!/usr/bin/env python +# Copyright 2026 NetBox Labs Inc +"""Shared fixtures for the device-discovery test suite.""" + +import logging + +import pytest + +_LIBRARY_LOGGERS = ("napalm", "ncclient", "paramiko", "pyeapi") + + +@pytest.fixture +def preserve_root_logging(): + """ + Snapshot and restore process-global logging state. + + ``basicConfig(force=True)`` and ``set_napalm_logs_level`` both mutate + process-global state, so without this the "exactly one record" and + "exactly one line" assertions become order-dependent flakes. + """ + root = logging.getLogger() + saved_level = root.level + saved_handlers = root.handlers[:] + saved_library = {name: logging.getLogger(name).level for name in _LIBRARY_LOGGERS} + + yield + + root.handlers[:] = saved_handlers + root.setLevel(saved_level) + for name, level in saved_library.items(): + logging.getLogger(name).setLevel(level) diff --git a/orb-discovery/device-discovery/tests/policy/test_runner.py b/orb-discovery/device-discovery/tests/policy/test_runner.py index 71259c02..240f89e9 100644 --- a/orb-discovery/device-discovery/tests/policy/test_runner.py +++ b/orb-discovery/device-discovery/tests/policy/test_runner.py @@ -261,7 +261,7 @@ def test_run_discovered_driver_error( patch( "device_discovery.policy.runner.discover_device_driver", return_value=None ) as mock_discover, - patch("device_discovery.policy.runner.logger.error") as mock_logger_error, + patch("device_discovery.policy.runner.logger.warning") as mock_logger_warning, ): # Set up run_store policy_runner.run_store = run_store @@ -271,8 +271,8 @@ def test_run_discovered_driver_error( policy_runner.run("test_id", sample_scopes[0], sample_config) mock_discover.assert_called_once_with(sample_scopes[0], drivers=None) - mock_logger_error.assert_called_once() - assert "Not able to discover device driver" in mock_logger_error.call_args[0][0] + mock_logger_warning.assert_called_once() + assert "Not able to discover device driver" in mock_logger_warning.call_args[0][0] assert policy_runner.status == Status.FAILED failed_run = run_store.get_runs_for_policy(policy_runner.name)[0] assert failed_run.driver is None diff --git a/orb-discovery/device-discovery/tests/policy/test_target_errors.py b/orb-discovery/device-discovery/tests/policy/test_target_errors.py new file mode 100644 index 00000000..a9b981ef --- /dev/null +++ b/orb-discovery/device-discovery/tests/policy/test_target_errors.py @@ -0,0 +1,103 @@ +#!/usr/bin/env python +# Copyright 2026 NetBox Labs Inc +"""NetBox Labs - Per-target failure classification unit tests.""" + +import pytest +from napalm.base.exceptions import ( + CommandErrorException, + CommandTimeoutException, + ConnectAuthError, + ConnectionClosedException, + ConnectionException, + ConnectTimeoutError, + NapalmException, + UnsupportedVersion, + ValidationException, +) +from netmiko.exceptions import NetmikoAuthenticationException, NetmikoTimeoutException + +from device_discovery.policy.runner import ( + _EXPECTED_TARGET_FAILURES, + _is_expected_target_failure, +) + + +def test_tuple_membership_is_a_conscious_choice(): + """ + Pin the tuple so widening or narrowing it must be a deliberate test edit. + + Collapsing it to the ConnectionException anchor alone restores the full + traceback wall for rejected credentials, which is the reported case. + """ + assert _EXPECTED_TARGET_FAILURES == ( + ConnectionException, + NetmikoAuthenticationException, + ) + + +def test_netmiko_auth_is_not_reachable_via_the_anchor(): + """ + The reason the second entry exists. + + netmiko roots NetmikoAuthenticationException in paramiko, not napalm, so a + rejected credential propagates past napalm's timeout-only except clause. + """ + assert not issubclass(NetmikoAuthenticationException, ConnectionException) + assert not issubclass(NetmikoAuthenticationException, NapalmException) + + +@pytest.mark.parametrize( + "error", + [ + ConnectionException("Cannot connect to 10.0.0.5"), + ConnectAuthError("auth"), + ConnectTimeoutError("timeout"), + ConnectionClosedException("closed"), + UnsupportedVersion("version"), + NetmikoAuthenticationException("Authentication to device failed."), + ], +) +def test_expected_failures(error): + """Routine unreachable/unauthenticated hosts classify as expected.""" + assert _is_expected_target_failure(error) is True + + +@pytest.mark.parametrize( + "error", + [ + NapalmException("bare napalm"), + CommandErrorException("bad command"), + CommandTimeoutException("command timed out"), + ValidationException("invalid"), + NetmikoTimeoutException("normalized upstream, should not be listed"), + KeyError("missing"), + ValueError("bad value"), + NotImplementedError("driver gap"), + # Stand-in for a diode transport failure: Client().ingest runs inside + # the same catch-all, and a dead ingest endpoint must NOT be reported + # as a per-host connection failure. + ConnectionResetError("diode endpoint reset"), + TimeoutError("builtin timeout"), + OSError("dns"), + ], +) +def test_unexpected_failures(error): + """Everything else keeps ERROR and its traceback.""" + assert _is_expected_target_failure(error) is False + + +def test_the_exception_chain_is_deliberately_not_walked(): + """ + A driver bug raised while handling a connection error stays UNEXPECTED. + + Proves the chain exists and is ignored on purpose, rather than the test + passing because no chain was built. + """ + try: + try: + raise ConnectionException("Cannot connect to 10.0.0.5") + except ConnectionException: + raise KeyError("driver bug") + except KeyError as error: + assert _is_expected_target_failure(error) is False + assert isinstance(error.__context__, ConnectionException) diff --git a/orb-discovery/device-discovery/tests/policy/test_target_failure_logging.py b/orb-discovery/device-discovery/tests/policy/test_target_failure_logging.py new file mode 100644 index 00000000..210d855a --- /dev/null +++ b/orb-discovery/device-discovery/tests/policy/test_target_failure_logging.py @@ -0,0 +1,187 @@ +#!/usr/bin/env python +# Copyright 2026 NetBox Labs Inc +"""NetBox Labs - Per-target failure logging behaviour unit tests.""" + +import logging +from unittest.mock import patch + +import pytest +from napalm.base.exceptions import ConnectionException +from netmiko.exceptions import NetmikoAuthenticationException + +from device_discovery.policy.models import Config, Defaults, Napalm, Status +from device_discovery.policy.run import RunStatus, RunStore +from device_discovery.policy.runner import PolicyRunner + +RUNNER_LOGGER = "device_discovery.policy.runner" + +# The real ten-line message netmiko builds for a rejected credential. +AUTH_MESSAGE = """Authentication to device failed. + +Common causes of this problem are: +1. Invalid username and password +2. Incorrect SSH-key file +3. Connecting to the wrong device + +Device settings: cisco_ios 10.0.0.5:22 + +Authentication failed.""" + + +@pytest.fixture +def runner(): + """A PolicyRunner wired to a real RunStore.""" + instance = PolicyRunner() + instance.name = "test_policy" + instance.run_store = RunStore() + return instance + + +@pytest.fixture +def scope(): + """A scope with the driver pinned, so driver discovery is skipped.""" + return Napalm( + driver="ios", hostname="10.0.0.5", username="admin", password="password" + ) + + +@pytest.fixture +def config(): + """A minimal policy config.""" + return Config(schedule="0 * * * *", defaults=Defaults(site="Lab")) + + +def _invoke(runner, entry_point, scope, config): + if entry_point == "run": + runner.run("test_id", scope, config) + else: + runner.run_with_parent("test_id", scope, config, "10.0.0.0/24") + + +ENTRY_POINTS = ["run", "run_with_parent"] + + +@pytest.mark.parametrize("entry_point", ENTRY_POINTS) +@pytest.mark.parametrize( + "error", + [ + ConnectionException("Cannot connect to 10.0.0.5"), + NetmikoAuthenticationException(AUTH_MESSAGE), + ], +) +def test_expected_failure_is_one_flat_warning( + runner, scope, config, entry_point, error, caplog +): + """One WARNING, no traceback, no embedded newline -- on both entry points.""" + with ( + patch.object(PolicyRunner, "_collect_device_data", side_effect=error), + caplog.at_level(logging.INFO, logger=RUNNER_LOGGER), + ): + _invoke(runner, entry_point, scope, config) + + records = [r for r in caplog.records if r.name == RUNNER_LOGGER and r.levelno >= logging.WARNING] + assert len(records) == 1 + record = records[0] + assert record.levelno == logging.WARNING + assert record.exc_info is None + assert record.exc_text is None + assert "\n" not in record.getMessage() + assert "10.0.0.5" in record.getMessage() + + +@pytest.mark.parametrize("entry_point", ENTRY_POINTS) +def test_expected_failure_keeps_traceback_at_debug( + runner, scope, config, entry_point, caplog +): + """At DEBUG the traceback is recoverable, still on a single physical line.""" + error = ConnectionException("Cannot connect to 10.0.0.5") + with ( + patch.object(PolicyRunner, "_collect_device_data", side_effect=error), + caplog.at_level(logging.DEBUG, logger=RUNNER_LOGGER), + ): + _invoke(runner, entry_point, scope, config) + + relevant = [ + r + for r in caplog.records + if r.name == RUNNER_LOGGER and r.levelno in (logging.WARNING, logging.DEBUG) + ] + assert [r.levelno for r in relevant] == [logging.WARNING, logging.DEBUG] + debug_message = relevant[1].getMessage() + assert "Traceback" in debug_message + assert "\n" not in debug_message + + +@pytest.mark.parametrize("entry_point", ENTRY_POINTS) +def test_unexpected_failure_keeps_error_and_exc_info( + runner, scope, config, entry_point, caplog +): + """The decision-3 guard: real bugs must not lose their traceback.""" + error = RuntimeError("boom") + with ( + patch.object(PolicyRunner, "_collect_device_data", side_effect=error), + caplog.at_level(logging.INFO, logger=RUNNER_LOGGER), + ): + _invoke(runner, entry_point, scope, config) + + errors = [r for r in caplog.records if r.name == RUNNER_LOGGER and r.levelno == logging.ERROR] + warnings = [r for r in caplog.records if r.name == RUNNER_LOGGER and r.levelno == logging.WARNING] + assert len(errors) == 1 + assert errors[0].exc_info is not None + assert errors[0].exc_info[1] is error + assert warnings == [] + + +@pytest.mark.parametrize("entry_point", ENTRY_POINTS) +def test_non_log_behaviour_is_unchanged(runner, scope, config, entry_point): + """Run status, reason, and the failure metrics must be untouched.""" + error = NetmikoAuthenticationException(AUTH_MESSAGE) + with patch.object(PolicyRunner, "_collect_device_data", side_effect=error): + _invoke(runner, entry_point, scope, config) + + runs = runner.run_store.get_runs_for_policy("test_policy") + assert runs + run = runs[0] + assert run.status is RunStatus.FAILED + # run.reason is flattened too -- a multi-line string inside the JSON + # /api/v1/status response is a defect on its own. + assert "\n" not in run.reason + assert "Authentication to device failed." in run.reason + + +@pytest.mark.parametrize("entry_point", ENTRY_POINTS) +def test_both_entry_points_route_through_the_shared_emitter( + runner, scope, config, entry_point +): + """The failure rule must not be able to drift between run and run_with_parent.""" + error = ConnectionException("Cannot connect to 10.0.0.5") + with ( + patch.object(PolicyRunner, "_collect_device_data", side_effect=error), + patch.object(PolicyRunner, "_log_target_failure") as emitter, + ): + _invoke(runner, entry_point, scope, config) + + assert emitter.call_count == 1 + + +def test_driver_discovery_failure_warns_rather_than_errors(runner, scope, config, caplog): + """ + runner.py:207 is the same expected-failure class on the auto-discovery path. + + Status.FAILED is deliberately left alone -- see the follow-up issue. + """ + scope.driver = None + with ( + patch("device_discovery.policy.runner.discover_device_driver", return_value=None), + caplog.at_level(logging.INFO, logger=RUNNER_LOGGER), + ): + runner.run("test_id", scope, config) + + matching = [ + r + for r in caplog.records + if "Not able to discover device driver" in r.getMessage() + ] + assert len(matching) == 1 + assert matching[0].levelno == logging.WARNING + assert runner.status == Status.FAILED diff --git a/orb-discovery/device-discovery/tests/test_emit_device_name.py b/orb-discovery/device-discovery/tests/test_emit_device_name.py new file mode 100644 index 00000000..1ae17399 --- /dev/null +++ b/orb-discovery/device-discovery/tests/test_emit_device_name.py @@ -0,0 +1,287 @@ +#!/usr/bin/env python +# Copyright 2026 NetBox Labs Inc +"""Tests for the emit_device_name option.""" + +import logging + +import pytest + +from device_discovery.device_name import ( + apply_device_name_emission, + device_has_alternative_matcher, +) +from device_discovery.policy.models import Defaults, DeviceParameters, Options +from device_discovery.translate import translate_data, translate_device + + +@pytest.fixture +def device_info(): + """NAPALM get_facts output for a device whose hostname differs from NetBox.""" + return { + "hostname": "discovered-name", + "vendor": "Cisco", + "model": "C9300-24T", + "os_version": "17.9.4", + "serial_number": "FOC1111111", + "uptime": 1000.0, + "fqdn": "discovered-name.example.net", + "interface_list": ["GigabitEthernet1/0/1"], + } + + +def _defaults(**kw): + return Defaults(site="site-a", role="switch", **kw) + + +# --- the matcher guard ------------------------------------------------------- + + +def test_netbox_id_counts_as_an_alternative_matcher(device_info): + """A scope netbox_id becomes metadata.source_match, which survives onto stubs.""" + dev = translate_device(device_info, _defaults(), netbox_id=42) + assert device_has_alternative_matcher(dev) is True + + +def test_asset_tag_counts_as_an_alternative_matcher(device_info): + """asset_tag is the highest-precedence NetBox device matcher.""" + dev = translate_device( + device_info, _defaults(device=DeviceParameters(asset_tag="AT-0001")) + ) + assert device_has_alternative_matcher(dev) is True + + +def test_serial_alone_is_not_an_alternative_matcher(device_info): + """ + NetBox Device.serial is not unique and generates no matcher. + + The fixture always carries a serial, so this pins that serial alone does + not unlock suppression — otherwise every device would qualify and clearing + the name would emit something NetBox cannot resolve. + """ + dev = translate_device(device_info, _defaults()) + assert dev.serial == "FOC1111111" + assert device_has_alternative_matcher(dev) is False + + +# --- the transform ----------------------------------------------------------- + + +def test_default_emits_the_name(device_info): + """Defaults are unchanged: the discovered hostname is emitted.""" + assert Options().emit_device_name is True + dev = translate_device(device_info, _defaults(), netbox_id=42) + assert apply_device_name_emission(dev, Options().emit_device_name, "10.0.0.5") is False + assert dev.name == "discovered-name" + + +def test_disabled_with_a_matcher_unsets_the_name(device_info): + """ + Suppression must UNSET the field, not assign "". + + Device.name declares proto presence and the Diode plugin treats an explicit + empty string as a deliberate clear, which would wipe the hostname in NetBox + instead of leaving it alone. HasField is the assertion that catches that. + """ + dev = translate_device(device_info, _defaults(), netbox_id=42) + assert apply_device_name_emission(dev, False, "10.0.0.5") is True + assert dev.HasField("name") is False + + +def test_disabled_without_a_matcher_keeps_the_name_and_warns(device_info, caplog): + """Name is a primary matcher; dropping it unguarded emits an unresolvable device.""" + dev = translate_device(device_info, _defaults()) + with caplog.at_level(logging.DEBUG, logger="device_discovery.device_name"): + assert apply_device_name_emission(dev, False, "10.0.0.5") is False + assert dev.name == "discovered-name" + warnings = [r for r in caplog.records if r.levelno >= logging.WARNING] + assert len(warnings) == 1 + assert "no alternative matcher" in warnings[0].getMessage() + + +def test_virtual_chassis_member_is_never_suppressed(device_info): + """Suppression is master-only, matching snmp-discovery.""" + dev = translate_device(device_info, _defaults(), netbox_id=42) + dev.vc_position = 2 + assert apply_device_name_emission(dev, False, "10.0.0.5") is False + assert dev.name == "discovered-name" + + +def test_a_device_with_no_name_is_a_no_op(device_info): + """A driver that discovered no hostname leaves nothing to suppress.""" + info = dict(device_info, hostname="") + dev = translate_device(info, _defaults(), netbox_id=42) + assert dev.HasField("name") is False + assert apply_device_name_emission(dev, False, "10.0.0.5") is False + + +# --- end to end through translate_data -------------------------------------- + + +def _stack_payload(device_info, options): + return { + "device": device_info, + "interface": { + "GigabitEthernet1/0/1": { + "is_enabled": True, "mtu": 1500, "mac_address": "", + "speed": 1000, "description": "", + }, + "GigabitEthernet2/0/1": { + "is_enabled": True, "mtu": 1500, "mac_address": "", + "speed": 1000, "description": "", + }, + }, + "interface_ip": {}, + "driver": "ios", + "defaults": _defaults(), + "options": options, + "netbox_id": 42, + "chassis_members": { + "members": [ + {"id": 1, "serial": "FOC1111111", "model": "C9300-24T", "role": "active"}, + {"id": 2, "serial": "FOC2222222", "model": "C9300-24T", "role": "standby"}, + ], + "domain": None, + }, + } + + +def test_standalone_end_to_end_suppresses_and_keeps_nothing_empty(device_info): + """The emitted Device carries no name, and no entity carries an empty one.""" + data = { + "device": device_info, + "interface": {}, + "interface_ip": {}, + "driver": "ios", + "defaults": _defaults(), + "options": Options(emit_device_name=False), + "netbox_id": 42, + } + entities = list(translate_data(data)) + devices = [e.device for e in entities if e.HasField("device")] + assert devices, "expected a Device entity" + for d in devices: + assert d.HasField("name") is False, "name must be unset, never empty" + + +def test_stack_master_suppressed_but_members_keep_their_names(device_info): + """Master and the shared VC master ref lose the name; members do not.""" + data = _stack_payload(device_info, Options(emit_device_name=False)) + entities = list(translate_data(data)) + + devices = [e.device for e in entities if e.HasField("device")] + masters = [d for d in devices if not d.HasField("vc_position")] + members = [d for d in devices if d.HasField("vc_position")] + + assert len(masters) == 1 + assert masters[0].HasField("name") is False, "master name must be suppressed" + + assert members, "expected non-master member devices" + for m in members: + assert m.HasField("name") is True, "member names come from the template" + assert m.name + + vcs = [e.virtual_chassis for e in entities if e.HasField("virtual_chassis")] + assert len(vcs) == 1 + assert vcs[0].name, "the VirtualChassis name itself is not suppressed" + assert vcs[0].master.HasField("name") is False, ( + "the shared VC master ref must inherit the suppression, or it " + "resurrects the name _master_device_ref exists to keep in sync" + ) + for m in members: + if m.HasField("virtual_chassis") and m.virtual_chassis.HasField("master"): + assert m.virtual_chassis.master.HasField("name") is False + + +def test_stack_default_keeps_every_name(device_info): + """With the option at its default, stack emission is byte-identical to before.""" + data = _stack_payload(device_info, Options()) + entities = list(translate_data(data)) + devices = [e.device for e in entities if e.HasField("device")] + assert all(d.HasField("name") for d in devices) + vcs = [e.virtual_chassis for e in entities if e.HasField("virtual_chassis")] + assert vcs[0].master.HasField("name") is True + + +def test_standalone_suppression_precedes_the_nested_ref_deepcopy(device_info): + """ + Suppression must run BEFORE translate.py's copy.deepcopy(device). + + Every nested interface / IP device reference is derived from that copy, so + suppressing after it leaves them carrying the name that was just dropped. + This asserts on the pre-pruning shape deliberately: prune_nested_refs + regenerates nested refs at ingest and would mask the ordering. + """ + data = { + "device": device_info, + "interface": { + "GigabitEthernet1/0/1": { + "is_enabled": True, "mtu": 1500, "mac_address": "", + "speed": 1000, "description": "", + }, + }, + "interface_ip": {}, + "driver": "ios", + "defaults": _defaults(), + "options": Options(emit_device_name=False), + "netbox_id": 42, + } + entities = list(translate_data(data)) + nested = [ + e.interface.device for e in entities + if e.HasField("interface") and e.interface.HasField("device") + ] + assert nested, "expected interface entities carrying a nested device ref" + for ref in nested: + assert ref.HasField("name") is False, ( + "a nested device ref still carries the suppressed name — suppression " + "ran after copy.deepcopy(device)" + ) + + +def test_suppressed_name_without_a_serial_still_prunes(device_info, caplog): + """ + A suppressed device with no serial must still resolve during pruning. + + prune_nested_refs resolves nested refs by name, then serial. Suppression is + permitted on the strength of source_match / asset_tag, so a device with a + netbox_id but no driver-reported serial would otherwise resolve to nothing: + every nested ref left un-pruned, with one WARNING per interface on every + discovery cycle. + """ + from device_discovery.stubs import prune_nested_refs + + info = dict(device_info) + del info["serial_number"] + data = { + "device": info, + "interface": { + f"GigabitEthernet1/0/{i}": { + "is_enabled": True, "mtu": 1500, "mac_address": "", + "speed": 1000, "description": "", + } + for i in (1, 2, 3) + }, + "interface_ip": {}, + "driver": "ios", + "defaults": _defaults(), + "options": Options(emit_device_name=False), + "netbox_id": 42, + } + entities = list(translate_data(data)) + with caplog.at_level(logging.DEBUG, logger="device_discovery.stubs"): + prune_nested_refs(entities) + + unresolved = [r for r in caplog.records if "could not resolve" in r.getMessage()] + assert unresolved == [], ( + "nested refs failed to resolve for a suppressed, serial-less device" + ) + nested = [ + e.interface.device for e in entities + if e.HasField("interface") and e.interface.HasField("device") + ] + assert nested + for ref in nested: + # platform and status are stripped by _device_match_stub, so their + # presence proves the ref was left un-pruned. + assert not ref.HasField("platform") + assert not ref.HasField("status") diff --git a/orb-discovery/device-discovery/tests/test_log_amplification.py b/orb-discovery/device-discovery/tests/test_log_amplification.py new file mode 100644 index 00000000..b3a4cae0 --- /dev/null +++ b/orb-discovery/device-discovery/tests/test_log_amplification.py @@ -0,0 +1,135 @@ +#!/usr/bin/env python +# Copyright 2026 NetBox Labs Inc +""" +NetBox Labs - Log amplification regression tests. + +Measured in PHYSICAL STDERR LINES, not log records, and that is the point. +The agent's go-cmd stream splits stderr on every newline and assigns ERROR by +pipe rather than by content (agent/backend/process.go:83-103 and +devicediscovery/device_discovery.go:172-176). That amplifier is deliberately +out of scope for #494, so the Python side has to hold the one-record == +one-line invariant permanently. Record-count assertions alone would not catch +a regression that reintroduces embedded newlines. +""" + +import io +import logging + +import pytest +from napalm.base.exceptions import ConnectionException +from netmiko.exceptions import NetmikoAuthenticationException + +from device_discovery.log_config import LOG_FORMAT +from device_discovery.policy.run import RunStore +from device_discovery.policy.runner import PolicyRunner + +AUTH_MESSAGE = """Authentication to device failed. + +Common causes of this problem are: +1. Invalid username and password +2. Incorrect SSH-key file +3. Connecting to the wrong device + +Device settings: cisco_ios 10.0.0.5:22 + +Authentication failed.""" + + +@pytest.fixture +def stderr_capture(): + """Attach a formatted stream handler to the runner logger, as the backend does.""" + buffer = io.StringIO() + logger = logging.getLogger("device_discovery.policy.runner") + handler = logging.StreamHandler(buffer) + handler.setFormatter(logging.Formatter(LOG_FORMAT)) + + saved_handlers = logger.handlers[:] + saved_propagate = logger.propagate + saved_level = logger.level + + logger.handlers = [handler] + logger.propagate = False + logger.setLevel(logging.INFO) + + yield buffer, logger + + logger.handlers = saved_handlers + logger.propagate = saved_propagate + logger.setLevel(saved_level) + + +def _emit(logger, error): + runner = PolicyRunner() + runner.name = "test_policy" + runner.run_store = RunStore() + runner._log_target_failure("10.0.0.5", error) + + +@pytest.mark.parametrize( + "error", + [ + ConnectionException("Cannot connect to 10.0.0.5"), + NetmikoAuthenticationException(AUTH_MESSAGE), + ], +) +def test_expected_failure_is_exactly_one_physical_line(stderr_capture, error): + """ + The headline of #494: one line per unreachable target, not ~63. + + The auth case is the one that matters most -- dropping exc_info without + flattening leaves ten lines, nine of which the agent labels ERROR. + """ + buffer, logger = stderr_capture + _emit(logger, error) + output = buffer.getvalue() + assert output.count("\n") == 1 + assert "Traceback" not in output + + +def test_a_sweep_emits_one_line_per_host(stderr_capture): + """The reported symptom, at the reported scale: a /24 of unreachable hosts.""" + buffer, logger = stderr_capture + for octet in range(1, 255): + _emit(logger, ConnectionException(f"Cannot connect to 10.0.0.{octet}")) + assert buffer.getvalue().count("\n") == 254 + + +def test_debug_adds_exactly_one_more_line(stderr_capture): + """The escaped traceback is recoverable at DEBUG and still one line.""" + buffer, logger = stderr_capture + logger.setLevel(logging.DEBUG) + try: + raise ConnectionException("Cannot connect to 10.0.0.5") + except ConnectionException as error: + _emit(logger, error) + output = buffer.getvalue() + assert output.count("\n") == 2 + assert "Traceback" in output + + +def test_unexpected_failure_still_gets_a_real_traceback(stderr_capture): + """Quiet must not have been bought by destroying diagnosability.""" + buffer, logger = stderr_capture + try: + raise ValueError("boom") + except ValueError as error: + _emit(logger, error) + output = buffer.getvalue() + assert "Traceback" in output + assert output.count("\n") > 1 + + +def test_emitted_line_matches_the_shape_the_agent_parses(stderr_capture): + """ + The format is a wire contract. + + normalizeDeviceDiscoveryLine (device_discovery.go:325-368) splits on the + first two colons to recover LEVEL and module. If this shape changes, the + agent silently falls back to assigning level by pipe. + """ + buffer, logger = stderr_capture + _emit(logger, ConnectionException("Cannot connect to 10.0.0.5")) + line = buffer.getvalue().rstrip("\n") + level, module, _ = line.split(":", 2) + assert level == "WARNING" + assert module == "device_discovery.policy.runner" diff --git a/orb-discovery/device-discovery/tests/test_log_config.py b/orb-discovery/device-discovery/tests/test_log_config.py new file mode 100644 index 00000000..3670daec --- /dev/null +++ b/orb-discovery/device-discovery/tests/test_log_config.py @@ -0,0 +1,117 @@ +#!/usr/bin/env python +# Copyright 2026 NetBox Labs Inc +"""NetBox Labs - Log configuration unit tests.""" + +import logging +from unittest.mock import MagicMock + +import pytest + +from device_discovery.log_config import ( + _LEVEL_ALIASES, + LOG_FORMAT, + configure_logging, + flatten_message, + resolve_log_level, +) + + +def test_log_format_is_logging_basic_format(): + """The emitted shape is a wire contract with the Go-side normalizer.""" + assert LOG_FORMAT == logging.BASIC_FORMAT + + +def test_alias_vocabulary_matches_the_go_side(): + """ + Mirrors parseDeviceDiscoveryLevel in device_discovery.go. + + If the Go switch gains a token, this assertion is the thing that notices. + """ + assert set(_LEVEL_ALIASES) == { + "trace", + "debug", + "info", + "warn", + "warning", + "error", + "err", + "exception", + "critical", + "fatal", + } + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + ("trace", logging.DEBUG), + ("debug", logging.DEBUG), + ("DEBUG", logging.DEBUG), + (" Debug ", logging.DEBUG), + ("info", logging.INFO), + ("warn", logging.WARNING), + ("warning", logging.WARNING), + ("WARNING", logging.WARNING), + ("error", logging.ERROR), + ("err", logging.ERROR), + ("exception", logging.ERROR), + ("critical", logging.CRITICAL), + ("fatal", logging.CRITICAL), + ], +) +def test_resolve_known_levels(value, expected): + """Every accepted token resolves without a warning.""" + level, warning = resolve_log_level(value) + assert level == expected + assert warning is None + + +@pytest.mark.parametrize("value", [None, "", "verbose", 3, MagicMock()]) +def test_resolve_unknown_levels_fall_back_and_warn(value): + """ + An unusable value degrades to INFO and says so. + + Must never raise: argparse has no choices= (a YAML typo would exit(2) and + crash-loop the backend), and every existing main() test patches parse_args + to return a MagicMock. + """ + level, warning = resolve_log_level(value) + assert level == logging.INFO + assert warning is not None + assert "falling back to INFO" in warning + + +def test_configure_logging_overrides_import_time_basicconfig(preserve_root_logging): + """ + The load-bearing test: this FAILS on pre-#494 code. + + Importing device_discovery.client runs the import-time basicConfig, after + which a plain basicConfig is a silent no-op. Without force=True the root + level stays INFO and --log-level is accepted and ignored -- the exact + defect this work exists to kill. + """ + import device_discovery.client # noqa: F401 - imported for its import-time side effect + + configure_logging("debug") + + root = logging.getLogger() + assert root.level == logging.DEBUG + assert root.handlers + assert any( + handler.formatter is not None and handler.formatter._fmt == LOG_FORMAT + for handler in root.handlers + ) + + +def test_configure_logging_returns_applied_level(preserve_root_logging): + """configure_logging reports what it applied.""" + assert configure_logging("error") == logging.ERROR + assert logging.getLogger().level == logging.ERROR + + +def test_flatten_message_collapses_multiline_exceptions(): + """Netmiko messages are ten physical lines; one record must be one line.""" + error = ValueError("line one\n\nline two\n line three") + flattened = flatten_message(error) + assert "\n" not in flattened + assert flattened == "line one line two line three" diff --git a/orb-discovery/device-discovery/tests/test_no_error_traceback_regression.py b/orb-discovery/device-discovery/tests/test_no_error_traceback_regression.py new file mode 100644 index 00000000..0f591be2 --- /dev/null +++ b/orb-discovery/device-discovery/tests/test_no_error_traceback_regression.py @@ -0,0 +1,76 @@ +#!/usr/bin/env python +# Copyright 2026 NetBox Labs Inc +""" +NetBox Labs - Structural guard against reintroducing ERROR-level tracebacks. + +The generic Go stderr amplifier stays in place by decision, so nothing +downstream will ever catch a reintroduced ``exc_info=True`` on a routine +condition. This test is the backstop. +""" + +import ast +import pathlib + +RUNNER = ( + pathlib.Path(__file__).resolve().parents[1] + / "device_discovery" + / "policy" + / "runner.py" +) + + +def _calls(tree): + return [node for node in ast.walk(tree) if isinstance(node, ast.Call)] + + +def _emitter_line_range(tree): + for node in ast.walk(tree): + if isinstance(node, ast.FunctionDef) and node.name == "_log_target_failure": + return range(node.lineno, (node.end_lineno or node.lineno) + 1) + return range(0) + + +def _attribute_path(func): + parts = [] + while isinstance(func, ast.Attribute): + parts.append(func.attr) + func = func.value + if isinstance(func, ast.Name): + parts.append(func.id) + return ".".join(reversed(parts)) + + +def test_no_logger_error_carries_exc_info(): + """ + logger.error(..., exc_info=...) belongs only inside _log_target_failure. + + Anywhere else it means a routine per-target condition is emitting a full + chained traceback again, which the agent fans out into ~63 ERROR records. + """ + tree = ast.parse(RUNNER.read_text()) + offenders = [] + for call in _calls(tree): + if _attribute_path(call.func) != "logger.error": + continue + if any(keyword.arg == "exc_info" for keyword in call.keywords): + offenders.append(call.lineno) + + allowed = _emitter_line_range(tree) + unexpected = [line for line in offenders if line not in allowed] + assert unexpected == [], ( + f"logger.error(..., exc_info=...) outside _log_target_failure at lines {unexpected}" + ) + + +def test_the_emitter_exists_so_this_cannot_pass_vacuously(): + """A guard that passes because the thing it guards was deleted is not a guard.""" + tree = ast.parse(RUNNER.read_text()) + names = { + node.name for node in ast.walk(tree) if isinstance(node, ast.FunctionDef) + } + assert "_log_target_failure" in names + + source = RUNNER.read_text() + assert source.count("self._log_target_failure(") == 2, ( + "expected exactly two call sites: run() and run_with_parent()" + ) diff --git a/orb-discovery/device-discovery/tests/test_stubs.py b/orb-discovery/device-discovery/tests/test_stubs.py index 56250d4a..7414bfbb 100644 --- a/orb-discovery/device-discovery/tests/test_stubs.py +++ b/orb-discovery/device-discovery/tests/test_stubs.py @@ -384,7 +384,9 @@ def test_prune_nested_refs_stubs_interface_module_reference(): the slot. Without stubbing, a 48-port linecard duplicates that rich subtree 48 times in the ingest payload. The stub keeps only the fields Diode needs to resolve the module — device, serial, and a - positional module_bay shell — so per-interface wire cost is bounded. + positional module_bay shell — plus module_type, which NetBox requires + if the reference has to be created rather than matched — so + per-interface wire cost is bounded. """ rich_dev = pb.Device(name="sw1", serial="FCW123", status="active") rich_dev.device_type.CopyFrom( @@ -398,7 +400,13 @@ def test_prune_nested_refs_stubs_interface_module_reference(): rich_module.device.CopyFrom(rich_dev) rich_module.module_bay.CopyFrom(bay) rich_module.module_type.CopyFrom( - pb.ModuleType(model="C9400-LC-48U", manufacturer=pb.Manufacturer(name="Cisco")), + pb.ModuleType( + model="C9400-LC-48U", + manufacturer=pb.Manufacturer(name="Cisco", slug="cisco", description="vendor"), + part_number="C9400-LC-48U-A", + description="48 port UPOE+ line card", + comments="rich field a driver may start populating", + ), ) iface = pb.Interface(name="GigabitEthernet2/0/1", type="1000base-t") @@ -413,7 +421,17 @@ def test_prune_nested_refs_stubs_interface_module_reference(): assert pruned_iface.HasField("module") assert pruned_iface.module.serial == "JAE2401LC02" assert pruned_iface.module.description == "" - assert not pruned_iface.module.HasField("module_type") + # module_type is retained: a nested ref that fails to match is created + # instead, and NetBox rejects a Module without one. + assert pruned_iface.module.module_type.model == "C9400-LC-48U" + assert pruned_iface.module.module_type.manufacturer.name == "Cisco" + # ...but only the (manufacturer, model) matcher pair, so the reference cannot + # grow as drivers start populating the richer ModuleType fields. + assert pruned_iface.module.module_type.part_number == "" + assert pruned_iface.module.module_type.description == "" + assert pruned_iface.module.module_type.comments == "" + assert pruned_iface.module.module_type.manufacturer.slug == "" + assert pruned_iface.module.module_type.manufacturer.description == "" # Nested device on the module ref is also a stub. assert pruned_iface.module.device.name == "sw1" assert pruned_iface.module.device.serial == "" diff --git a/orb-discovery/snmp-discovery/config/config.go b/orb-discovery/snmp-discovery/config/config.go index 9be3cba7..1265f213 100644 --- a/orb-discovery/snmp-discovery/config/config.go +++ b/orb-discovery/snmp-discovery/config/config.go @@ -155,6 +155,11 @@ type Authentication struct { AuthPassphrase string `yaml:"auth_passphrase"` PrivProtocol string `yaml:"priv_protocol"` PrivPassphrase string `yaml:"priv_passphrase"` + // ContextName is the SNMPv3 context name (snmpwalk -n). Devices that expose + // their MIB data in a named context return nothing for the default context, + // so an absent value looks like a successful empty walk. Rejected for + // v1/v2c, which have no context concept. + ContextName string `yaml:"context_name,omitempty"` } // IPAddressDefaults represents default values for a specific entity type diff --git a/orb-discovery/snmp-discovery/config/config_test.go b/orb-discovery/snmp-discovery/config/config_test.go index 4149e06b..0cb90b41 100644 --- a/orb-discovery/snmp-discovery/config/config_test.go +++ b/orb-discovery/snmp-discovery/config/config_test.go @@ -439,6 +439,41 @@ func TestTargetNetboxID_optional(t *testing.T) { assert.Nil(t, target.NetboxID) } +func TestAuthentication_ContextName_ParsesFromScopeAndTarget(t *testing.T) { + input := ` +targets: + - host: "10.3.2.20" + authentication: + protocol_version: "3" + username: "umsnmp" + auth_protocol: "SHA" + auth_passphrase: "secret" + priv_protocol: "AES" + priv_passphrase: "secret" + context_name: "mfpdirect" +authentication: + protocol_version: "3" + username: "policyuser" + context_name: "policycontext" +` + var scope Scope + err := yaml.Unmarshal([]byte(input), &scope) + require.NoError(t, err) + + assert.Equal(t, "policycontext", scope.Authentication.ContextName) + require.Len(t, scope.Targets, 1) + require.NotNil(t, scope.Targets[0].Authentication) + assert.Equal(t, "mfpdirect", scope.Targets[0].Authentication.ContextName) +} + +func TestAuthentication_ContextName_Optional(t *testing.T) { + input := `protocol_version: "3"` + var auth Authentication + err := yaml.Unmarshal([]byte(input), &auth) + require.NoError(t, err) + assert.Empty(t, auth.ContextName) +} + func TestMappingEntry_IndexKind(t *testing.T) { yamlBody := []byte(` entries: diff --git a/orb-discovery/snmp-discovery/mapping/chassis.go b/orb-discovery/snmp-discovery/mapping/chassis.go index 36325732..c8dbe85e 100644 --- a/orb-discovery/snmp-discovery/mapping/chassis.go +++ b/orb-discovery/snmp-discovery/mapping/chassis.go @@ -44,6 +44,13 @@ func (m *ChassisInventoryMapper) Map( // derived logical member id (see assignMemberIDs); EntPhysicalIndex is // the raw row index used for entAliasMappingTable chain walks. // AssetTag is the trimmed entPhysicalAssetID value ("" when unset or not walked). +// +// DescendantIDs is derivation-only input for assignMemberIDs' descendant +// tier, in the same category as EntName and ParentRelPos (both exported, +// both read only by the id helpers). It holds the DISTINCT SORTED leading +// numbers found on this chassis's port descendants; nil means "no signal" +// and makes the tier decline. Populated only for multi-member sets — see +// collectDescendantIDs. type ChassisMember struct { ID int EntPhysicalIndex string @@ -52,6 +59,7 @@ type ChassisMember struct { EntName string ParentRelPos int AssetTag string + DescendantIDs []int } // ChassisInventory is the deduped, validated, member-id-sorted set of @@ -110,7 +118,8 @@ func isStackContainerParent(oids ObjectIDValueMap, idx string) bool { // // Returns members sorted ascending by ID. Member ids are derived // set-wide by assignMemberIDs (one scheme for all members: -// parentRelPos → entPhysicalName trailing int → ordinal). +// parentRelPos → entPhysicalName trailing int → port-descendant number +// → ordinal). func extractInventory(oids ObjectIDValueMap, logger *slog.Logger) ChassisInventory { candidates := []string{} for oid, v := range oids { @@ -156,6 +165,7 @@ func extractInventory(oids ObjectIDValueMap, logger *slog.Logger) ChassisInvento bi, _ := strconv.Atoi(b.EntPhysicalIndex) return ai - bi }) + collectDescendantIDs(members, oids) assignMemberIDs(members, logger) // Dedup pass 1: drop later-occurring duplicates of the same serial, // keep the lowest-id occurrence. Track dropped ids and their @@ -352,6 +362,14 @@ func trimSNMPString(s string) string { var trailingIntRe = regexp.MustCompile(`(\d+)\s*$`) +// leadingMemberNumRe matches the leading slash-delimited number of an +// interface-style entPhysicalName ("2/1/1" -> 2, "1/1/1:1" -> 1). Anchored so +// vendor-decorated names that merely contain the pattern ("GigabitEthernet1/0/1", +// "RPM sensor for fan Tray-2/1/1") yield nothing rather than a slot number. +// +// Not trailingIntRe: that reads the END of a member's own name ("Switch 2"). +var leadingMemberNumRe = regexp.MustCompile(`^(\d+)/`) + // prelState classifies the member set's entPhysicalParentRelPos values. type prelState int @@ -373,12 +391,20 @@ const ( // 2. entPhysicalName trailing int — usable iff every member has one and // they are distinct (0 is allowed: FPC-style members are genuinely // zero-numbered); -// 3. when parentRelPos is AMBIGUOUS (duplicate positive positions) or -// names are AMBIGUOUS (full coverage, duplicate numbers) and the other -// signal could not rescue, keep the colliding values so the caller's -// ambiguity dedup refuses those rows — silently renumbering them -// would mis-attribute ifName-routed interfaces; -// 4. ordinal 1..N in slice order (callers pass entPhysicalIndex-sorted +// 3. the leading number on each member's PORT descendants, reached through +// entPhysicalContainedIn, under descendantIDs' strict predicate. Resolves +// stacks that report the same position AND the same name on every chassis +// row while numbering their ports 1/1/x .. N/1/x. Port names are the +// namespace entAliasMappingTable maps to ifName, so this id and an +// ifName-derived id agree — on the vendors where that holds, which is why +// the predicate refuses rather than assumes; +// 4. when parentRelPos is AMBIGUOUS (duplicate positive positions) or +// names are AMBIGUOUS (full coverage, duplicate numbers) and neither +// the other signal nor the descendant tier could rescue, keep the +// colliding values so the caller's ambiguity dedup refuses those rows — +// silently renumbering them would mis-attribute ifName-routed +// interfaces; +// 5. ordinal 1..N in slice order (callers pass entPhysicalIndex-sorted // members, so this is walk order). // // Scheme selection runs over the pre-dedup member rows: a duplicate-serial @@ -416,10 +442,39 @@ func assignMemberIDs(members []ChassisMember, logger *slog.Logger) { applyIDs(members, names) return } + // Tier 3: the device's own containment tree, ahead of both refusals and + // the ordinal fallback. A device asserting one position twice has asserted + // nonsense; one with both columns silent has asserted nothing. Containment + // beats walk order in either case, and descendantIDs' predicate is what + // makes trusting it safe. + desc, dstate := descendantIDs(members) + if dstate == descendantUsable { + logger.Info("member id: using entPhysicalContainedIn descendant-derived ids", + "reason", prelStateReason(state), "name_reason", nameStateReason(nstate), + "ids", desc, "members", len(members)) + applyIDs(members, desc) + return + } + if dstate == descendantConflict { + // Warn only when this decline costs the device its stack: with + // ambiguous prel or names the next stop is refusal. Otherwise the + // ordinal fallback still emits a working stack, and a permanent Warn + // on every poll of a healthy device whose ports are merely + // slot-numbered ("1/1".."1/48" on every member) is noise. + log := logger.Debug + if state == prelAmbiguous || nstate == nameAmbiguous { + log = logger.Warn + } + log("member id: descendant-derived ids rejected as contradictory", + "sets", descendantSets(members), "members", len(members)) + } + // From here every outcome is lossy or low-confidence: Warn. Colliding // ids are deliberately KEPT so the caller's ambiguity dedup refuses // those rows — silently renumbering them would mis-attribute - // ifName-routed interfaces. + // ifName-routed interfaces on any device whose ports are absent from + // entAliasMappingTable (present-table devices route by containment and + // use the id only as a map key — see chassisRouter.routeIfIndex). if state == prelAmbiguous { logger.Warn("member id: duplicate positive entPhysicalParentRelPos and no usable names; keeping colliding ids for ambiguity refusal", "members", len(members)) @@ -447,6 +502,96 @@ func assignMemberIDs(members []ChassisMember, logger *slog.Logger) { applyIDs(members, ordinal) } +// childIndexFromOIDs inverts entPhysicalContainedIn into parent -> children, +// so the descendant walk can descend where routeIfIndex ascends. +// +// Values go through trimSNMPString, matching how extractInventory reads the +// same column: a NUL-padded parent pointer must still key to its chassis, or +// the subtree reads as empty and the descendant tier silently declines. +func childIndexFromOIDs(oids ObjectIDValueMap) map[string][]string { + out := make(map[string][]string, len(oids)/8) + for oid, v := range oids { + if !strings.HasPrefix(oid, oidEntPhysicalContainedIn) { + continue + } + parent := trimSNMPString(v.Value) + out[parent] = append(out[parent], strings.TrimPrefix(oid, oidEntPhysicalContainedIn)) + } + return out +} + +// collectDescendantIDs populates each member's DescendantIDs with the distinct +// sorted leading numbers on the port rows in its entPhysicalContainedIn +// subtree. Input to the descendant tier. +// +// Multi-member sets only, and the containment index is built AFTER that check +// so a standalone target pays nothing: a lone member trivially satisfies the +// tier's predicate, so a subtree could otherwise renumber the one chassis of a +// partially-reporting stack, and childIndexFromOIDs scans the entire map — +// which extractInventory would pay on every poll of every plain switch, twice +// per target when discover_modules re-derives the inventory. +func collectDescendantIDs(members []ChassisMember, oids ObjectIDValueMap) { + if len(members) < 2 { + return + } + collectDescendantIDsFrom(members, oids, childIndexFromOIDs(oids)) +} + +// collectDescendantIDsFrom is collectDescendantIDs over a supplied containment +// index. Split out so tests can inject a tree the single-parent entPhysical +// encoding cannot express, such as a cycle. +// +// Two properties are easy to get wrong: +// +// - The class filter gates COLLECTION, not traversal. The chain is +// port(10) -> module(9) -> chassis(3), so filtering the walk itself +// returns an empty set for every member. +// - The walk stops at any other chassis or stack row, member or not. +// Class-3 rows are excluded from the member set for a non-root parent or +// a missing serial; descending through one would merge a rejected +// sibling's ports in and veto an otherwise-clean device. +func collectDescendantIDsFrom(members []ChassisMember, oids ObjectIDValueMap, childrenOf map[string][]string) { + classOf := func(idx string) string { + return trimSNMPString(oids[oidEntPhysicalClass+idx].Value) + } + for i := range members { + found := map[int]struct{}{} + seen := map[string]struct{}{members[i].EntPhysicalIndex: {}} + queue := append([]string(nil), childrenOf[members[i].EntPhysicalIndex]...) + for len(queue) > 0 { + idx := queue[len(queue)-1] + queue = queue[:len(queue)-1] + if _, dup := seen[idx]; dup { + continue + } + seen[idx] = struct{}{} + switch classOf(idx) { + case entPhysicalClassChassis, entPhysicalClassStack: + // Another chassis's territory (or a nested stack) — do not + // descend, do not collect. + continue + case entPhysicalClassPort: + name := trimSNMPString(oids[oidEntPhysicalName+idx].Value) + if m := leadingMemberNumRe.FindStringSubmatch(name); m != nil { + if n, err := strconv.Atoi(m[1]); err == nil { + found[n] = struct{}{} + } + } + } + queue = append(queue, childrenOf[idx]...) + } + if len(found) == 0 { + continue // leave nil: no signal + } + nums := make([]int, 0, len(found)) + for n := range found { + nums = append(nums, n) + } + slices.Sort(nums) + members[i].DescendantIDs = nums + } +} + func hasPositivePrel(members []ChassisMember) bool { for _, m := range members { if m.ParentRelPos > 0 { @@ -473,6 +618,13 @@ func prelStateReason(s prelState) string { return "contains zero or negative positions" } +func nameStateReason(s nameState) string { + if s == nameAmbiguous { + return "duplicate name numbers" + } + return "name numbers missing on at least one member" +} + func applyIDs(members []ChassisMember, ids []int) { for i := range members { members[i].ID = ids[i] @@ -500,6 +652,76 @@ func parentRelIDs(members []ChassisMember) ([]int, prelState) { return ids, state } +// descendantState classifies the member set's descendant-derived numbers. +type descendantState int + +const ( + descendantUsable descendantState = iota // every member yields exactly one number, all distinct, all > 0 + descendantConflict // a member's ports disagree, or two members claim one number + descendantAbsent // at least one member has no numbered port descendants +) + +// descendantIDs returns member ids derived from the leading number on each +// chassis's port descendants. Mirrors parentRelIDs and nameIDs: pure over the +// member slice, meaningful only for descendantUsable. +// +// Strict on purpose — a wrong-but-distinct id is silent downstream. Any of +// these vetoes the whole set: a member with no numbered ports; a member whose +// ports carry more than one number; two members claiming one number; a number +// <= 0. +// +// The zero rule differs from nameIDs, which allows 0 because an FPC-style +// member really is zero-numbered. Here the leading field of a PORT name is a +// slot, and devices do name every port "0/N" under a chassis called "Unit 1", +// where 0 would set vc_position 0 and re-pin the master. +// +// Nothing in the MIB states that the leading field is a member rather than a +// slot, so distinctness across members carries that weight: identical chassis +// each populated in their own slot 1 all report "1/..." and collide, which +// declines. The residual case is a stack of modular chassis populated in +// DIFFERENT slots whose slot numbers are not the member numbers; there the +// derived ids would be wrong but distinct. Checking entPhysicalName against +// the aliased ifName would not catch it — a slot-prefixed name matches its own +// ifName perfectly. +// +// Walk order need not match id order: extractInventory re-sorts by id, so an +// agent enumerating chassis rows in join order is numbered, not refused. +func descendantIDs(members []ChassisMember) ([]int, descendantState) { + ids := make([]int, len(members)) + seen := make(map[int]struct{}, len(members)) + for i, m := range members { + if len(m.DescendantIDs) == 0 { + return nil, descendantAbsent + } + if len(m.DescendantIDs) > 1 { + return nil, descendantConflict + } + id := m.DescendantIDs[0] + if id <= 0 { + return nil, descendantConflict + } + if _, dup := seen[id]; dup { + return nil, descendantConflict + } + seen[id] = struct{}{} + ids[i] = id + } + return ids, descendantUsable +} + +// descendantSets renders the per-member descendant numbers for logging, so a +// conflict decline says WHICH members disagreed instead of only that one did. +func descendantSets(members []ChassisMember) string { + var b strings.Builder + for i, m := range members { + if i > 0 { + b.WriteString(" ") + } + fmt.Fprintf(&b, "%s=%v", m.EntPhysicalIndex, m.DescendantIDs) + } + return b.String() +} + // nameState classifies the member set's entPhysicalName trailing-int values. type nameState int @@ -609,11 +831,41 @@ func resolveAssetTags(members []ChassisMember, masterTag string, logger *slog.Lo return out } +// refusedMasterSerial returns the serial to put on the master when every +// chassis row was refused, or "" when there was nothing to refuse (a device +// with no chassis rows is simply not a stack, and must stay untouched). +// +// DroppedEntIndexes carries the refused rows' entPhysicalIndexes, so the +// serial is read back from oids: the inventory itself keeps no serial for a +// dropped row. Lowest index wins, matching the master-pinning convention. +func refusedMasterSerial(inv ChassisInventory, oids ObjectIDValueMap) string { + if len(inv.DroppedEntIndexes) == 0 { + return "" + } + idxs := make([]string, 0, len(inv.DroppedEntIndexes)) + for idx := range inv.DroppedEntIndexes { + idxs = append(idxs, idx) + } + slices.SortFunc(idxs, func(a, b string) int { + ai, _ := strconv.Atoi(a) + bi, _ := strconv.Atoi(b) + return ai - bi + }) + for _, idx := range idxs { + if s := trimSNMPString(oids[oidEntPhysicalSerialNum+idx].Value); s != "" { + return s + } + } + return "" +} + // TranslateAsStack inspects the raw oids map for ENTITY-MIB chassis // inventory. Three outcomes: // // - 0 chassis rows with non-empty serial -> entities returned -// unchanged (no Serial assignment possible). +// unchanged (no Serial assignment possible), EXCEPT when rows existed +// and were all refused as ambiguous, in which case the master keeps a +// serial but gets no VirtualChassis (see refusedMasterSerial). // - 1 chassis row -> set master.Serial on the existing Device, // return entities unchanged otherwise (standalone case). // - >= 2 chassis rows -> emit master + top-level VirtualChassis + @@ -661,6 +913,22 @@ func TranslateAsStack( inv := extractInventory(oids, logger) if len(inv.Members) == 0 { + // Two situations reach zero members: no chassis rows at all (not a + // stack, leave it alone) and every row refused as ambiguous. Only + // the member NUMBERING is ambiguous in the second case, so refuse + // the structure but keep the serial, taken from the lowest + // entPhysicalIndex — the one ordering that does not depend on the + // disputed ids, so it is stable across polls of the same data. + // + // Not guaranteed to equal what a later RESOLVING poll picks: that + // takes the lowest member id, which is the lowest index only when + // ids ascend with it. Still better than no serial, and serial is + // not a Diode matcher, so nothing re-keys. + if s := refusedMasterSerial(inv, oids); s != "" && master.Serial == nil { + master.Serial = &s + logger.Warn("stack refused: emitting master serial only, no virtual chassis", + "serial", s, "refused_ids", len(inv.DroppedIDs)) + } return entities } @@ -693,6 +961,14 @@ func TranslateAsStack( // Master also gets its per-member Model in case the rich // DeviceMapper picked a top-level chassis model that diverges // (or didn't resolve a DeviceType at all — sysObjectID miss). + // + // Matches buildMemberDevice: every member's device_type comes from its own + // chassis row, which is what makes a mixed-model stack right, and the + // master is just the lowest-id chassis row. This does override + // override_defaults.device.model, which the backend documents as + // highest-priority — a real contract bug, but it applies equally to the + // member Devices and cannot be fixed here (no access to config.Defaults), + // and guarding only the master would split one stack across two types. if lowest.Model != "" { var mfg *diode.Manufacturer if master.DeviceType != nil { diff --git a/orb-discovery/snmp-discovery/mapping/chassis_fixtures_test.go b/orb-discovery/snmp-discovery/mapping/chassis_fixtures_test.go index 75030697..78469b5b 100644 --- a/orb-discovery/snmp-discovery/mapping/chassis_fixtures_test.go +++ b/orb-discovery/snmp-discovery/mapping/chassis_fixtures_test.go @@ -32,27 +32,164 @@ func fixtureCisco3850TwoMemberStack() ObjectIDValueMap { } } -// fixtureArubaCX2MemberVSF returns an ObjectIDValueMap shaped like a -// 2-member Aruba CX VSF stack: -// - sysName "aruba-cx-stack" -// - entPhysical rows at indices 1 and 2 -// - both entPhysicalClass=3, entPhysicalContainedIn=0 -// - parentRelPos = 1 and 2 (numeric VSF member IDs) -// - model "Aruba-6300M-48G" on both members -func fixtureArubaCX2MemberVSF() ObjectIDValueMap { - return ObjectIDValueMap{ - ".1.3.6.1.2.1.1.5.0": {Value: "aruba-cx-stack"}, - ".1.3.6.1.2.1.47.1.1.1.1.4.1": {Value: "0"}, - ".1.3.6.1.2.1.47.1.1.1.1.5.1": {Value: "3"}, - ".1.3.6.1.2.1.47.1.1.1.1.6.1": {Value: "1"}, - ".1.3.6.1.2.1.47.1.1.1.1.11.1": {Value: "SG12345"}, - ".1.3.6.1.2.1.47.1.1.1.1.13.1": {Value: "Aruba-6300M-48G"}, - ".1.3.6.1.2.1.47.1.1.1.1.4.2": {Value: "0"}, - ".1.3.6.1.2.1.47.1.1.1.1.5.2": {Value: "3"}, - ".1.3.6.1.2.1.47.1.1.1.1.6.2": {Value: "2"}, - ".1.3.6.1.2.1.47.1.1.1.1.11.2": {Value: "SG12346"}, - ".1.3.6.1.2.1.47.1.1.1.1.13.2": {Value: "Aruba-6300M-48G"}, +// fixtureIndistinctChassisRowsStack returns an ObjectIDValueMap for a stack +// whose chassis rows are mutually indistinguishable, transcribed from a real +// 4-member walk and reduced to the smallest failing case, two members: +// +// - class=11 stack container at index 1, name "Stack", containedIn=0 +// - class=3 chassis at 101001 and 201001, both containedIn=1 +// - parentRelPos = 1 on BOTH rows, so the position column is ambiguous +// - entPhysicalName = "Chassis" on BOTH rows, with no trailing number, so +// the name column carries nothing either +// - distinct serials per chassis +// +// Neither position nor name can number these members. What can is the +// containment tree: each chassis owns a class=9 module whose ports are named +// "N/1/x". Note the ports are NOT direct children of the chassis — the real +// chain is port(10) -> module(9) -> chassis(3) -> stack(11) — which is why +// the descendant walk must traverse every class and filter only on collection. +// +// Sensor rows carrying a decorated number ("RPM sensor for fan Tray-2/1/1") +// are included deliberately: they must NOT be collected, both because the +// leading-number anchor rejects them and because they are not ports. +// +// entAliasMappingTable rows are present so member-owned interfaces route by +// containment rather than by ifName parsing. +func fixtureIndistinctChassisRowsStack() ObjectIDValueMap { + oids := ObjectIDValueMap{ + ".1.3.6.1.2.1.1.5.0": {Value: "stack-indistinct.example"}, + // Stack container (class=11) — never a member. + ".1.3.6.1.2.1.47.1.1.1.1.4.1": {Value: "0"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.1": {Value: "11"}, + ".1.3.6.1.2.1.47.1.1.1.1.6.1": {Value: "-1"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.1": {Value: "Stack"}, + } + serials := []string{"SN0000000101", "SN0000000201"} + for member := 1; member <= 2; member++ { + var ( + chassis = fmt.Sprintf("%d01001", member) + module = fmt.Sprintf("%d12001", member) + sensor = fmt.Sprintf("%d07101", member) + ) + set := func(col int, idx, val string) { + oids[fmt.Sprintf(".1.3.6.1.2.1.47.1.1.1.1.%d.%s", col, idx)] = Value{Value: val} + } + // Chassis row: ambiguous position, uninformative name. + set(4, chassis, "1") + set(5, chassis, "3") + set(6, chassis, "1") + set(7, chassis, "Chassis") + set(11, chassis, serials[member-1]) + set(13, chassis, "SWITCH-48G-4SFP") + // Line module under the chassis — the hop the ports hang off. + set(4, module, chassis) + set(5, module, "9") + set(7, module, fmt.Sprintf("%d/1", member)) + set(11, module, serials[member-1]) + // Fan sensor whose name merely CONTAINS a slash-number. + set(4, sensor, chassis) + set(5, sensor, "8") + set(7, sensor, fmt.Sprintf("RPM sensor for fan Tray-%d/1/1", member)) + // Two ports under the module, named in the ifName namespace. + // + // Port indexes use the real "12{member-1}{port}0" encoding, so + // member 2's ports are 1210x0 — NOT 2xxxxx. Every port on every + // member therefore divides to 1 at the 100000 scale that the chassis + // rows do encode, which is exactly why this tier reads containment + // instead of doing arithmetic on the index. + for port := 1; port <= 2; port++ { + idx := fmt.Sprintf("12%d%03d", member-1, port*10) + set(4, idx, module) + set(5, idx, "10") + set(7, idx, fmt.Sprintf("%d/1/%d", member, port)) + // entAliasMappingTable: entPhysicalIndex -> ifIndex. + ifIndex := (member-1)*100 + port + aliasOID := fmt.Sprintf(".1.3.6.1.2.1.47.1.3.2.1.2.%s.0", idx) + oids[aliasOID] = Value{Value: fmt.Sprintf(".1.3.6.1.2.1.2.2.1.1.%d", ifIndex)} + } + } + return oids +} + +// fixtureSingleMemberWrappedStack returns an ObjectIDValueMap transcribed +// from a real single-member VSF capture: a class=11 "Stack" container at +// index 1 wrapping exactly ONE class=3 "Chassis" at 101001 with +// parentRelPos=1. The wrapper is present even with one member, so this is +// the standalone path reached through the wrapped topology — and the shape +// that must NOT be renumbered by the descendant tier, since a lone member +// trivially satisfies its predicate. +func fixtureSingleMemberWrappedStack() ObjectIDValueMap { + oids := ObjectIDValueMap{ + ".1.3.6.1.2.1.1.5.0": {Value: "single-member.example"}, + // Stack container (class=11). + ".1.3.6.1.2.1.47.1.1.1.1.4.1": {Value: "0"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.1": {Value: "11"}, + ".1.3.6.1.2.1.47.1.1.1.1.6.1": {Value: "-1"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.1": {Value: "Stack"}, + // The only chassis. + ".1.3.6.1.2.1.47.1.1.1.1.4.101001": {Value: "1"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.101001": {Value: "3"}, + ".1.3.6.1.2.1.47.1.1.1.1.6.101001": {Value: "1"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.101001": {Value: "Chassis"}, + ".1.3.6.1.2.1.47.1.1.1.1.11.101001": {Value: "SN0000000101"}, + ".1.3.6.1.2.1.47.1.1.1.1.13.101001": {Value: "SWITCH-48G-4SFP"}, + } + // A module and two ports, so a descendant signal exists and the + // single-member gate is the only thing suppressing it. + oids[".1.3.6.1.2.1.47.1.1.1.1.4.112001"] = Value{Value: "101001"} + oids[".1.3.6.1.2.1.47.1.1.1.1.5.112001"] = Value{Value: "9"} + oids[".1.3.6.1.2.1.47.1.1.1.1.7.112001"] = Value{Value: "1/1"} + for port := 1; port <= 2; port++ { + idx := fmt.Sprintf("120%03d", port*10) + oids[".1.3.6.1.2.1.47.1.1.1.1.4."+idx] = Value{Value: "112001"} + oids[".1.3.6.1.2.1.47.1.1.1.1.5."+idx] = Value{Value: "10"} + oids[".1.3.6.1.2.1.47.1.1.1.1.7."+idx] = Value{Value: fmt.Sprintf("1/1/%d", port)} + } + return oids +} + +// fixtureIndistinctChassisRowsStackN returns the same shape with `members` +// chassis rows, all reporting parentRelPos=1 and all named "Chassis". +// +// serialedMembers bounds how many of them report a serial: the six-member +// capture this is derived from serials only its first chassis, and a row with +// no serial is dropped before id derivation, so passing 1 reproduces a real +// six-member stack silently ingesting as a single standalone Device. +func fixtureIndistinctChassisRowsStackN(members, serialedMembers int) ObjectIDValueMap { + oids := ObjectIDValueMap{ + ".1.3.6.1.2.1.1.5.0": {Value: "stack-indistinct-n.example"}, + ".1.3.6.1.2.1.47.1.1.1.1.4.1": {Value: "0"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.1": {Value: "11"}, + ".1.3.6.1.2.1.47.1.1.1.1.6.1": {Value: "-1"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.1": {Value: "Stack"}, } + for member := 1; member <= members; member++ { + var ( + chassis = fmt.Sprintf("%d01001", member) + module = fmt.Sprintf("%d12001", member) + ) + set := func(col int, idx, val string) { + oids[fmt.Sprintf(".1.3.6.1.2.1.47.1.1.1.1.%d.%s", col, idx)] = Value{Value: val} + } + set(4, chassis, "1") + set(5, chassis, "3") + set(6, chassis, "1") + set(7, chassis, "Chassis") + set(13, chassis, "SWITCH-48G-4SFP") + if member <= serialedMembers { + set(11, chassis, fmt.Sprintf("SN00000%05d", member*101)) + } + set(4, module, chassis) + set(5, module, "9") + set(7, module, fmt.Sprintf("%d/1", member)) + for port := 1; port <= 2; port++ { + idx := fmt.Sprintf("12%d%03d", member-1, port*10) + set(4, idx, module) + set(5, idx, "10") + set(7, idx, fmt.Sprintf("%d/1/%d", member, port)) + } + } + return oids } // fixtureJunosQFX4MemberVC builds a Junos QFX5100 4-member VC scenario: diff --git a/orb-discovery/snmp-discovery/mapping/chassis_test.go b/orb-discovery/snmp-discovery/mapping/chassis_test.go index 055e8ced..a6cf158a 100644 --- a/orb-discovery/snmp-discovery/mapping/chassis_test.go +++ b/orb-discovery/snmp-discovery/mapping/chassis_test.go @@ -4,6 +4,7 @@ import ( "fmt" "log/slog" "os" + "strconv" "strings" "testing" @@ -757,20 +758,24 @@ func TestTranslateAsStack_OrphanIPFiltered(t *testing.T) { } } -func TestTranslateAsStack_ArubaCX_2MemberVSF(t *testing.T) { +// Every chassis row reports parentRelPos=1 and is named "Chassis", so both +// primary id signals are useless and the stack collapsed to a single Device +// before the descendant tier existed. The containment tree numbers +// the members, so the VC and its member Devices are emitted. +func TestTranslateAsStack_IndistinctChassisRowsResolvedByDescendants(t *testing.T) { logger := slog.Default() master := &diode.Device{ - Name: strPtr("aruba-cx-stack"), + Name: strPtr("stack-indistinct.example"), Site: &diode.Site{Name: strPtr("dc1")}, DeviceType: &diode.DeviceType{ - Model: strPtr("Aruba-6300M-48G"), - Manufacturer: &diode.Manufacturer{Name: strPtr("HPE Aruba")}, + Model: strPtr("switch-48g-4sfp"), + Manufacturer: &diode.Manufacturer{Name: strPtr("Example Networks")}, }, } memberIface := &diode.Interface{Name: strPtr("2/1/24"), Device: master} entities := []diode.Entity{master, memberIface} - out := TranslateAsStack(entities, fixtureArubaCX2MemberVSF(), nil, nil, logger) + out := TranslateAsStack(entities, fixtureIndistinctChassisRowsStack(), nil, nil, logger) var members []*diode.Device for _, e := range out { @@ -779,8 +784,122 @@ func TestTranslateAsStack_ArubaCX_2MemberVSF(t *testing.T) { } } assert.Len(t, members, 1) - assert.Equal(t, "SG12346", *members[0].Serial) - assert.Equal(t, "aruba-cx-stack-2", *memberIface.Device.Name) + assert.Equal(t, "SN0000000201", *members[0].Serial) + assert.Equal(t, "SN0000000101", *master.Serial, "master pins to the lowest-index chassis") + assert.Equal(t, "stack-indistinct.example-2", *memberIface.Device.Name) + + // Master and members must land on ONE device_type, both taken from their + // own chassis row. Guarding only the master would split a stack across two. + assert.Equal(t, "SWITCH-48G-4SFP", *master.DeviceType.Model) + assert.Equal(t, "SWITCH-48G-4SFP", *members[0].DeviceType.Model) + assert.Equal(t, "Example Networks", *master.DeviceType.Manufacturer.Name) +} + +func TestExtractInventory_IndistinctChassisRowsNumberedByDescendants(t *testing.T) { + inv := extractInventory(fixtureIndistinctChassisRowsStack(), slog.Default()) + assert.True(t, inv.IsStack()) + assert.Len(t, inv.Members, 2) + assert.Equal(t, 1, inv.Members[0].ID) + assert.Equal(t, 2, inv.Members[1].ID) + assert.Empty(t, inv.DroppedIDs, "nothing may be refused once the tier resolves the set") +} + +func TestExtractInventory_SixIndistinctChassisRowsNumberedByDescendants(t *testing.T) { + // Scale check on the shape of a real six-member capture: the per-member + // partition must stay clean as the tree grows. + inv := extractInventory(fixtureIndistinctChassisRowsStackN(6, 6), slog.Default()) + assert.Len(t, inv.Members, 6) + for i, m := range inv.Members { + assert.Equal(t, i+1, m.ID) + } +} + +func TestExtractInventory_SixMemberStackReportingOneSerialCollapsesToStandalone(t *testing.T) { + // As captured: the agent serials only its first chassis. Rows with no + // serial are dropped before id derivation, so a six-member stack presents + // as a single standalone Device. Pinned because NetBox shows no trace of + // it — the five drops are visible only as "chassis row dropped: empty + // serial" lines in the agent log. + inv := extractInventory(fixtureIndistinctChassisRowsStackN(6, 1), slog.Default()) + assert.Len(t, inv.Members, 1) + assert.False(t, inv.IsStack()) + assert.Nil(t, inv.Members[0].DescendantIDs, "single-member sets carry no descendant signal") +} + +func TestTranslateAsStack_SingleMemberWrappedStackSetsSerialOnly(t *testing.T) { + logger := slog.Default() + master := &diode.Device{ + Name: strPtr("single-member.example"), + Site: &diode.Site{Name: strPtr("dc1")}, + } + entities := []diode.Entity{master} + + out := TranslateAsStack(entities, fixtureSingleMemberWrappedStack(), nil, nil, logger) + + assert.Len(t, out, 1, "one chassis row emits no VirtualChassis") + assert.Equal(t, "SN0000000101", *master.Serial) +} + +// A stack whose rows cannot be numbered by ANY tier still loses its structure, +// but must not lose its serial: only the member NUMBERING was ambiguous. The +// value comes from the lowest entPhysicalIndex — the one ordering independent +// of the disputed ids, so repeated polls of the same data agree. +func TestTranslateAsStack_RefusedStackKeepsMasterSerialWithoutVirtualChassis(t *testing.T) { + logger := slog.Default() + // Duplicate positive prel, uninformative names, and no port descendants + // for the tier to read: every row is refused. + oids := ObjectIDValueMap{ + ".1.3.6.1.2.1.1.5.0": {Value: "refused.example"}, + } + for _, m := range []struct{ idx, serial string }{ + {"201001", "SN0000000201"}, + {"101001", "SN0000000101"}, + } { + oids[".1.3.6.1.2.1.47.1.1.1.1.4."+m.idx] = Value{Value: "0"} + oids[".1.3.6.1.2.1.47.1.1.1.1.5."+m.idx] = Value{Value: "3"} + oids[".1.3.6.1.2.1.47.1.1.1.1.6."+m.idx] = Value{Value: "1"} + oids[".1.3.6.1.2.1.47.1.1.1.1.7."+m.idx] = Value{Value: "Chassis"} + oids[".1.3.6.1.2.1.47.1.1.1.1.11."+m.idx] = Value{Value: m.serial} + } + master := &diode.Device{Name: strPtr("refused.example"), Site: &diode.Site{Name: strPtr("dc1")}} + entities := []diode.Entity{master} + + out := TranslateAsStack(entities, oids, nil, nil, logger) + + assert.Len(t, out, 1, "refused stack emits no VirtualChassis and no member Devices") + require.NotNil(t, master.Serial, "the serial was never the ambiguous datum") + assert.Equal(t, "SN0000000101", *master.Serial, "lowest entPhysicalIndex wins, as on the resolved path") +} + +func TestTranslateAsStack_NoChassisRowsLeavesDeviceUntouched(t *testing.T) { + // The other reason member count reaches zero: not a stack at all. Nothing + // may be invented here. + master := &diode.Device{Name: strPtr("plain.example"), Site: &diode.Site{Name: strPtr("dc1")}} + out := TranslateAsStack([]diode.Entity{master}, + ObjectIDValueMap{".1.3.6.1.2.1.1.5.0": {Value: "plain.example"}}, nil, nil, slog.Default()) + assert.Len(t, out, 1) + assert.Nil(t, master.Serial) +} + +// Same fixture, but routing driven by entAliasMappingTable instead of ifName +// parsing: the alias row resolves the port's entPhysicalIndex, and the +// containedIn chain walks port -> module -> chassis to find the owner. +func TestTranslateAsStack_IndistinctChassisRowsRouteViaAliasTable(t *testing.T) { + logger := slog.Default() + master := &diode.Device{ + Name: strPtr("stack-indistinct.example"), + Site: &diode.Site{Name: strPtr("dc1")}, + } + // Name deliberately carries NO member prefix, so a pass would have to come + // from the alias table rather than from ParseMemberID. + memberIface := &diode.Interface{Name: strPtr("uplink-a"), Device: master} + entities := []diode.Entity{master, memberIface} + + out := TranslateAsStack(entities, fixtureIndistinctChassisRowsStack(), + map[*diode.Interface]int{memberIface: 101}, nil, logger) + + assert.NotEmpty(t, out) + assert.Equal(t, "stack-indistinct.example-2", *memberIface.Device.Name) } func TestTranslateAsStack_JunosQFX_4MemberVC(t *testing.T) { @@ -1739,3 +1858,344 @@ func TestAssignMemberIDs_OrdinalFallback(t *testing.T) { assert.Equal(t, 1, members[0].ID) assert.Equal(t, 2, members[1].ID) } + +// --- descendant tier --- + +func TestAssignMemberIDs_DescendantTierResolvesAmbiguousPrel(t *testing.T) { + // The reported shape: duplicate positive prel, no names, but the ports in + // each chassis's subtree number their owner. The tier resolves what would + // otherwise be refused, so the stack survives instead of collapsing. + members := []ChassisMember{ + {EntPhysicalIndex: "101001", ParentRelPos: 1, EntName: "Chassis", DescendantIDs: []int{1}}, + {EntPhysicalIndex: "201001", ParentRelPos: 1, EntName: "Chassis", DescendantIDs: []int{2}}, + } + assignMemberIDs(members, slog.Default()) + assert.Equal(t, 1, members[0].ID) + assert.Equal(t, 2, members[1].ID) +} + +func TestAssignMemberIDs_DescendantTierAcceptsWalkOrderUnrelatedToMemberOrder(t *testing.T) { + // An agent may enumerate chassis rows in stack-join order, so the + // lowest-index row need not be member 1. extractInventory re-sorts by id, + // so this must be numbered, not refused. + members := []ChassisMember{ + {EntPhysicalIndex: "1", ParentRelPos: 1, EntName: "Chassis", DescendantIDs: []int{2}}, + {EntPhysicalIndex: "1000", ParentRelPos: 1, EntName: "Chassis", DescendantIDs: []int{1}}, + } + assignMemberIDs(members, slog.Default()) + assert.Equal(t, 2, members[0].ID) + assert.Equal(t, 1, members[1].ID) +} + +func TestAssignMemberIDs_DescendantTierPreemptsOrdinalOnGappedSet(t *testing.T) { + // A stack missing member 3. Ordinal would number these 1,2 and silently + // mis-name the second device; the containment tree reports 2 and 4. + members := []ChassisMember{ + {EntPhysicalIndex: "201001", ParentRelPos: 0, EntName: "Chassis", DescendantIDs: []int{2}}, + {EntPhysicalIndex: "401001", ParentRelPos: 0, EntName: "Chassis", DescendantIDs: []int{4}}, + } + assignMemberIDs(members, slog.Default()) + assert.Equal(t, 2, members[0].ID) + assert.Equal(t, 4, members[1].ID) +} + +func TestAssignMemberIDs_DescendantTierDeclineKeepsRefusal(t *testing.T) { + // Every decline must leave the pre-existing outcome untouched. With + // ambiguous prel that means the colliding ids survive for the caller's + // ambiguity dedup — the tier may rescue, never weaken. + cases := []struct { + name string + members []ChassisMember + }{ + {"member contradicts itself", []ChassisMember{ + {ParentRelPos: 1, DescendantIDs: []int{1, 2}}, + {ParentRelPos: 1, DescendantIDs: []int{2}}, + }}, + {"two members claim one number", []ChassisMember{ + {ParentRelPos: 1, DescendantIDs: []int{2}}, + {ParentRelPos: 1, DescendantIDs: []int{2}}, + }}, + {"zero is a slot, not a member", []ChassisMember{ + {ParentRelPos: 1, DescendantIDs: []int{0}}, + {ParentRelPos: 1, DescendantIDs: []int{1}}, + }}, + {"one member has no numbered ports", []ChassisMember{ + {ParentRelPos: 1, DescendantIDs: []int{1}}, + {ParentRelPos: 1}, + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assignMemberIDs(tc.members, slog.Default()) + assert.Equal(t, 1, tc.members[0].ID) + assert.Equal(t, 1, tc.members[1].ID, "colliding ids must survive for the refusal") + }) + } +} + +func TestAssignMemberIDs_DescendantTierLogsInfoOnSuccessAndWarnOnConflict(t *testing.T) { + // A rescue is the device working correctly: Info, matching the name-tier + // rescue precedent. A contradictory signal is a discarded PRESENT signal, + // so it warns AND names the sets, since a silent decline is the hardest + // outcome to diagnose remotely. + logs := &strings.Builder{} + assignMemberIDs([]ChassisMember{ + {EntPhysicalIndex: "101001", ParentRelPos: 1, DescendantIDs: []int{1}}, + {EntPhysicalIndex: "201001", ParentRelPos: 1, DescendantIDs: []int{2}}, + }, slog.New(slog.NewTextHandler(logs, nil))) + assert.Contains(t, logs.String(), "descendant-derived ids") + assert.NotContains(t, logs.String(), "level=WARN") + + conflict := &strings.Builder{} + assignMemberIDs([]ChassisMember{ + {EntPhysicalIndex: "101001", ParentRelPos: 1, DescendantIDs: []int{1, 2}}, + {EntPhysicalIndex: "201001", ParentRelPos: 1, DescendantIDs: []int{2}}, + }, slog.New(slog.NewTextHandler(conflict, nil))) + assert.Contains(t, conflict.String(), "rejected as contradictory") + assert.Contains(t, conflict.String(), "101001=[1 2]") + + // Having NO port descendants is the overwhelmingly common case — every + // stack resolved by prel or name reaches this code with nil sets — so it + // must stay silent, or the tier would warn on healthy devices forever. + absent := &strings.Builder{} + assignMemberIDs([]ChassisMember{ + {EntPhysicalIndex: "101001", ParentRelPos: 1}, + {EntPhysicalIndex: "201001", ParentRelPos: 1}, + }, slog.New(slog.NewTextHandler(absent, nil))) + assert.NotContains(t, absent.String(), "rejected as contradictory") + + // A conflict that still ends in a working stack (ordinal) is Debug, not + // Warn: the outcome did not change, so a per-poll Warn is noise. + ordinal := &strings.Builder{} + assignMemberIDs([]ChassisMember{ + {EntPhysicalIndex: "101001", ParentRelPos: 0, DescendantIDs: []int{1}}, + {EntPhysicalIndex: "201001", ParentRelPos: 0, DescendantIDs: []int{1}}, + }, slog.New(slog.NewTextHandler(ordinal, nil))) + assert.NotContains(t, ordinal.String(), "level=WARN") +} + +// --- descendant collection --- + +func TestCollectDescendantIDs_TraversesNonPortRowsToReachPorts(t *testing.T) { + // The property that decides whether the fix works at all. Ports are NOT + // direct children of a chassis — the chain is port -> module -> chassis — + // so filtering on class during TRAVERSAL (rather than only when + // collecting) returns an empty set for every member and silently declines + // the tier on the very devices this exists to fix. + oids := fixtureIndistinctChassisRowsStack() + members := []ChassisMember{ + {EntPhysicalIndex: "101001"}, + {EntPhysicalIndex: "201001"}, + } + collectDescendantIDs(members, oids) + assert.Equal(t, []int{1}, members[0].DescendantIDs) + assert.Equal(t, []int{2}, members[1].DescendantIDs) +} + +func TestCollectDescendantIDs_StopsAtAnyChassisRowNotOnlyMembers(t *testing.T) { + // A class=3 row excluded from the member set (here: no serial) must still + // terminate the walk. Descending through it would merge a rejected + // sibling's ports into this member and veto an otherwise-clean device. + oids := ObjectIDValueMap{ + ".1.3.6.1.2.1.47.1.1.1.1.4.100": {Value: "0"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.100": {Value: "3"}, + // Own port -> 1. + ".1.3.6.1.2.1.47.1.1.1.1.4.110": {Value: "100"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.110": {Value: "10"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.110": {Value: "1/1/1"}, + // Nested serial-less chassis whose port claims member 9. + ".1.3.6.1.2.1.47.1.1.1.1.4.150": {Value: "100"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.150": {Value: "3"}, + ".1.3.6.1.2.1.47.1.1.1.1.4.160": {Value: "150"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.160": {Value: "10"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.160": {Value: "9/1/1"}, + // Second member, so the multi-member gate opens. + ".1.3.6.1.2.1.47.1.1.1.1.4.200": {Value: "0"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.200": {Value: "3"}, + ".1.3.6.1.2.1.47.1.1.1.1.4.210": {Value: "200"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.210": {Value: "10"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.210": {Value: "2/1/1"}, + } + members := []ChassisMember{{EntPhysicalIndex: "100"}, {EntPhysicalIndex: "200"}} + collectDescendantIDs(members, oids) + assert.Equal(t, []int{1}, members[0].DescendantIDs, "9/1/1 lives behind another chassis row") + assert.Equal(t, []int{2}, members[1].DescendantIDs) +} + +func TestCollectDescendantIDs_SkipsSingleMemberSets(t *testing.T) { + // A lone member trivially satisfies the tier's predicate, so collecting + // for it would let a subtree renumber the single surviving chassis of a + // partially-reporting stack. + oids := fixtureSingleMemberWrappedStack() + members := []ChassisMember{{EntPhysicalIndex: "101001"}} + collectDescendantIDs(members, oids) + assert.Nil(t, members[0].DescendantIDs) +} + +func TestCollectDescendantIDs_StandaloneTargetDoesNotBuildContainmentIndex(t *testing.T) { + // The containment index scans the whole oids map, so it must be built only + // after the multi-member check — otherwise every plain switch in a fleet + // pays for it on every poll. Compare a large standalone inventory against + // the same map with the chassis row removed: neither reaches the walk, so + // allocations must not scale with the map. + big := ObjectIDValueMap{ + ".1.3.6.1.2.1.47.1.1.1.1.4.1": {Value: "0"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.1": {Value: "3"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.1": {Value: "Chassis"}, + ".1.3.6.1.2.1.47.1.1.1.1.11.1": {Value: "SN0000000001"}, + } + for i := 2; i < 2000; i++ { + idx := strconv.Itoa(i) + big[".1.3.6.1.2.1.47.1.1.1.1.4."+idx] = Value{Value: "1"} + big[".1.3.6.1.2.1.47.1.1.1.1.5."+idx] = Value{Value: "10"} + big[".1.3.6.1.2.1.47.1.1.1.1.7."+idx] = Value{Value: "1/1/" + idx} + } + members := []ChassisMember{{EntPhysicalIndex: "1"}} + allocs := testing.AllocsPerRun(20, func() { collectDescendantIDs(members, big) }) + assert.Zero(t, allocs, "a single-member set must not build the containment index") + assert.Nil(t, members[0].DescendantIDs) +} + +func TestCollectDescendantIDs_IgnoresDecoratedAndNonPortNames(t *testing.T) { + // Sensors and fans carry the member number inside a longer string + // ("RPM sensor for fan Tray-2/1/1"); the anchor rejects them, and they are + // not ports either. Only the port rows contribute. + oids := fixtureIndistinctChassisRowsStack() + // A module row named like a port must still not be collected. + oids[".1.3.6.1.2.1.47.1.1.1.1.4.199999"] = Value{Value: "101001"} + oids[".1.3.6.1.2.1.47.1.1.1.1.5.199999"] = Value{Value: "9"} + oids[".1.3.6.1.2.1.47.1.1.1.1.7.199999"] = Value{Value: "7/1/1"} + members := []ChassisMember{{EntPhysicalIndex: "101001"}, {EntPhysicalIndex: "201001"}} + collectDescendantIDs(members, oids) + assert.Equal(t, []int{1}, members[0].DescendantIDs) +} + +func TestExtractInventory_SlotNumberedPortsDeclineRatherThanBecomeMemberIDs(t *testing.T) { + // The leading field of a port name is not always a member number — on a + // modular chassis it is a local slot. The distinctness requirement is what + // keeps that from being mistaken for a member id: identical chassis each + // populated in their own slot 1 both yield {1}, which collides and declines + // the tier, leaving the pre-existing outcome (here: refusal) untouched. + oids := ObjectIDValueMap{ + ".1.3.6.1.2.1.1.5.0": {Value: "modular.example"}, + } + for member := 1; member <= 2; member++ { + chassis := fmt.Sprintf("%d0000", member) + module := fmt.Sprintf("%d1000", member) + set := func(col int, idx, val string) { + oids[fmt.Sprintf(".1.3.6.1.2.1.47.1.1.1.1.%d.%s", col, idx)] = Value{Value: val} + } + set(4, chassis, "0") + set(5, chassis, "3") + set(6, chassis, "1") // duplicate positive position on both rows + set(7, chassis, "Chassis") + set(11, chassis, fmt.Sprintf("SN0000000%03d", member)) + set(4, module, chassis) + set(5, module, "9") + set(7, module, "1/1") // slot 1 on BOTH chassis + for port := 1; port <= 2; port++ { + idx := fmt.Sprintf("%d2%03d", member, port) + set(4, idx, module) + set(5, idx, "10") + set(7, idx, fmt.Sprintf("1/1/%d", port)) // slot-prefixed, not member-prefixed + } + } + inv := extractInventory(oids, slog.Default()) + assert.Empty(t, inv.Members, "slot numbers must not be adopted as member ids") + assert.Len(t, inv.DroppedIDs, 1, "the pre-existing ambiguity refusal still fires") +} + +func TestCollectDescendantIDs_LeadingNumberMustBeAnchored(t *testing.T) { + // Vendor-style port names embed a slash-number that is NOT a member + // ("GigabitEthernet1/0/1"). An unanchored pattern would read 1 out of both + // members' ports and either mis-number them or veto a device that should + // simply have declined. + oids := ObjectIDValueMap{} + for _, m := range []struct{ chassis, port, name string }{ + {"100", "110", "GigabitEthernet1/0/1"}, + {"200", "210", "GigabitEthernet2/0/1"}, + } { + oids[".1.3.6.1.2.1.47.1.1.1.1.4."+m.chassis] = Value{Value: "0"} + oids[".1.3.6.1.2.1.47.1.1.1.1.5."+m.chassis] = Value{Value: "3"} + oids[".1.3.6.1.2.1.47.1.1.1.1.4."+m.port] = Value{Value: m.chassis} + oids[".1.3.6.1.2.1.47.1.1.1.1.5."+m.port] = Value{Value: "10"} + oids[".1.3.6.1.2.1.47.1.1.1.1.7."+m.port] = Value{Value: m.name} + } + members := []ChassisMember{{EntPhysicalIndex: "100"}, {EntPhysicalIndex: "200"}} + collectDescendantIDs(members, oids) + assert.Nil(t, members[0].DescendantIDs) + assert.Nil(t, members[1].DescendantIDs) +} + +func TestCollectDescendantIDs_StopsAtNestedStackRow(t *testing.T) { + // The stop covers class 11 as well as class 3. A stack container reported + // INSIDE a chassis subtree must not have its ports absorbed by the chassis + // that contains it. + oids := ObjectIDValueMap{ + ".1.3.6.1.2.1.47.1.1.1.1.4.100": {Value: "0"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.100": {Value: "3"}, + ".1.3.6.1.2.1.47.1.1.1.1.4.110": {Value: "100"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.110": {Value: "10"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.110": {Value: "1/1/1"}, + // Nested stack container under member 1, owning a port of its own. + ".1.3.6.1.2.1.47.1.1.1.1.4.150": {Value: "100"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.150": {Value: "11"}, + ".1.3.6.1.2.1.47.1.1.1.1.4.160": {Value: "150"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.160": {Value: "10"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.160": {Value: "7/1/1"}, + ".1.3.6.1.2.1.47.1.1.1.1.4.200": {Value: "0"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.200": {Value: "3"}, + ".1.3.6.1.2.1.47.1.1.1.1.4.210": {Value: "200"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.210": {Value: "10"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.210": {Value: "2/1/1"}, + } + members := []ChassisMember{{EntPhysicalIndex: "100"}, {EntPhysicalIndex: "200"}} + collectDescendantIDs(members, oids) + assert.Equal(t, []int{1}, members[0].DescendantIDs, "7/1/1 lives behind a nested stack row") + assert.Equal(t, []int{2}, members[1].DescendantIDs) +} + +func TestCollectDescendantIDs_ToleratesNULPaddedValues(t *testing.T) { + // Many agents NUL-pad DisplayStrings. The containment keys and the class + // lookups must be sanitized the same way the chassis extractor sanitizes + // them, or the parent key never matches and the subtree reads as empty. + oids := ObjectIDValueMap{ + ".1.3.6.1.2.1.47.1.1.1.1.4.100": {Value: "0"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.100": {Value: "3\x00"}, + ".1.3.6.1.2.1.47.1.1.1.1.4.110": {Value: "100\x00"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.110": {Value: "10\x00"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.110": {Value: "1/1/1\x00"}, + ".1.3.6.1.2.1.47.1.1.1.1.4.200": {Value: "0"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.200": {Value: "3\x00"}, + ".1.3.6.1.2.1.47.1.1.1.1.4.210": {Value: "200\x00"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.210": {Value: "10\x00"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.210": {Value: "2/1/1\x00"}, + } + members := []ChassisMember{{EntPhysicalIndex: "100"}, {EntPhysicalIndex: "200"}} + collectDescendantIDs(members, oids) + assert.Equal(t, []int{1}, members[0].DescendantIDs) + assert.Equal(t, []int{2}, members[1].DescendantIDs) +} + +func TestCollectDescendantIDs_TerminatesOnContainmentCycle(t *testing.T) { + // A malformed agent can report a containment cycle. The walk must not spin. + oids := ObjectIDValueMap{ + ".1.3.6.1.2.1.47.1.1.1.1.4.100": {Value: "0"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.100": {Value: "3"}, + ".1.3.6.1.2.1.47.1.1.1.1.4.110": {Value: "100"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.110": {Value: "9"}, + ".1.3.6.1.2.1.47.1.1.1.1.4.120": {Value: "110"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.120": {Value: "10"}, + ".1.3.6.1.2.1.47.1.1.1.1.7.120": {Value: "1/1/1"}, + ".1.3.6.1.2.1.47.1.1.1.1.4.200": {Value: "0"}, + ".1.3.6.1.2.1.47.1.1.1.1.5.200": {Value: "3"}, + } + // 110's child 130 points back at 110's parent chain. + oids[".1.3.6.1.2.1.47.1.1.1.1.4.130"] = Value{Value: "120"} + oids[".1.3.6.1.2.1.47.1.1.1.1.5.130"] = Value{Value: "9"} + children := childIndexFromOIDs(oids) + children["130"] = append(children["130"], "110") // cycle + members := []ChassisMember{{EntPhysicalIndex: "100"}, {EntPhysicalIndex: "200"}} + assert.NotPanics(t, func() { collectDescendantIDsFrom(members, oids, children) }) + assert.Equal(t, []int{1}, members[0].DescendantIDs) +} diff --git a/orb-discovery/snmp-discovery/mapping/device_name_test.go b/orb-discovery/snmp-discovery/mapping/device_name_test.go index 3533db8c..e8020db0 100644 --- a/orb-discovery/snmp-discovery/mapping/device_name_test.go +++ b/orb-discovery/snmp-discovery/mapping/device_name_test.go @@ -156,6 +156,20 @@ func TestApplyDeviceNameEmission_StubsStayMatchable(t *testing.T) { dev.AssetTag = StringPtr("ASSET-9") } iface := &diode.Interface{Name: StringPtr("Gi0/1"), Device: dev} + // A primary IP, so the device's PrimaryIp4 snapshot is covered too. That + // snapshot is a shallow copy taken during mapping, before suppression + // runs, so it is the one derived copy that can re-supply the hostname. + addr := "10.0.0.1/24" + dev.PrimaryIp4 = detachForPrimaryIP(&diode.IPAddress{ + Address: &addr, + AssignedObject: &diode.Interface{Name: StringPtr("Vlan10"), Device: dev}, + }, dev) + // Both families, so the assertion loop below is not silently v4-only. + addr6 := "2001:db8::1/64" + dev.PrimaryIp6 = detachForPrimaryIP6(&diode.IPAddress{ + Address: &addr6, + AssignedObject: &diode.Interface{Name: StringPtr("Vlan10"), Device: dev}, + }, dev) return []diode.Entity{dev, iface} } @@ -165,16 +179,32 @@ func TestApplyDeviceNameEmission_StubsStayMatchable(t *testing.T) { ApplyDeviceNameEmission(entities, dev, false, host, logger) require.Nil(t, dev.Name, "top-level device name suppressed") PruneNestedRefs(entities, dev, nil) + assertStub := func(stub *diode.Device, where string) { + assert.Nil(t, stub.Name, where+": nested device stub must not carry a name once suppressed") + _, hasSM := stub.Metadata["source_match"] + hasTag := stub.AssetTag != nil && strings.TrimSpace(*stub.AssetTag) != "" + assert.True(t, hasSM || hasTag, where+": nested device stub must keep a surviving matcher (source_match or asset_tag)") + } for _, e := range entities { iface, ok := e.(*diode.Interface) if !ok || iface == nil || iface.Device == nil { continue } - stub := iface.Device - assert.Nil(t, stub.Name, "nested device stub must not carry a name once suppressed") - _, hasSM := stub.Metadata["source_match"] - hasTag := stub.AssetTag != nil && strings.TrimSpace(*stub.AssetTag) != "" - assert.True(t, hasSM || hasTag, "nested device stub must keep a surviving matcher (source_match or asset_tag)") + assertStub(iface.Device, "interface.device") + } + // The primary-IP snapshots are nested refs too, and are NOT reachable + // from the loop above: the IP-assigned interface is deliberately absent + // from the top-level entity slice. + for where, ip := range map[string]*diode.IPAddress{ + "primary_ip4": dev.PrimaryIp4, + "primary_ip6": dev.PrimaryIp6, + } { + if ip == nil { + continue + } + if iface, ok := ip.AssignedObject.(*diode.Interface); ok && iface != nil && iface.Device != nil { + assertStub(iface.Device, where) + } } } diff --git a/orb-discovery/snmp-discovery/mapping/mapping.go b/orb-discovery/snmp-discovery/mapping/mapping.go index 42df3130..d7aa3157 100644 --- a/orb-discovery/snmp-discovery/mapping/mapping.go +++ b/orb-discovery/snmp-discovery/mapping/mapping.go @@ -963,6 +963,13 @@ func pickPrimaryIPHit(logger *slog.Logger, registry *EntityRegistry, target stri // still carries PrimaryIp4, reintroducing the cycle. The standalone // emitted Interface entities keep their full graph; only the snapshot is // pruned. +// +// The Device copy is a snapshot: anything a later stage writes to the rich +// Device does not reach it. PruneNestedRefs.prunePrimarySnapshot rebuilds this +// subtree as stubs after every mutator has run, which is what keeps the two +// representations of the device consistent. Keep the clearing below anyway — +// mapping-level callers must not have to depend on the prune having run — but +// do not "simplify" either side without the other. func detachForPrimaryIP(ip *diode.IPAddress, owner *diode.Device) *diode.IPAddress { if ip == nil { return nil @@ -999,6 +1006,9 @@ func detachForPrimaryIP(ip *diode.IPAddress, owner *diode.Device) *diode.IPAddre // introducing a reference cycle. Both PrimaryIp4 and PrimaryIp6 are // cleared on the embedded device copy so the snapshot is independent // of evaluation order between the v4 and v6 passes. +// +// Same snapshot caveat as detachForPrimaryIP: PruneNestedRefs rebuilds this +// subtree once every mutator has run. Both families must be kept in step. func detachForPrimaryIP6(ip *diode.IPAddress, owner *diode.Device) *diode.IPAddress { if ip == nil { return nil diff --git a/orb-discovery/snmp-discovery/mapping/stubs.go b/orb-discovery/snmp-discovery/mapping/stubs.go index b114f5e8..a6cf6912 100644 --- a/orb-discovery/snmp-discovery/mapping/stubs.go +++ b/orb-discovery/snmp-discovery/mapping/stubs.go @@ -178,6 +178,13 @@ func newDeviceStubKeepingPrimary(owner *diode.Device, isV6 bool, primary *diode. // attributes the mapper computed, or they are silently lost. Pointer- // sharing them costs negligible bytes. Structural refs (parent/bridge/ // lag) are intentionally dropped; they carry their own nested payloads. +// +// Tags is deliberately NOT carried. Nested IP-assigned interface refs have +// never carried it, so adding it here would start tagging interfaces that are +// untagged today — a product change, not a fix. Diode applies updates with +// PATCH semantics, so omitting the field never strips tags from an interface +// that already has them; only a first-time creation comes up untagged, which +// is the existing behaviour for every other IP-assigned interface. func newInterfaceStub(iface *diode.Interface, deviceStub *diode.Device) *diode.Interface { if iface == nil { return nil @@ -255,6 +262,15 @@ func PruneNestedRefs(entities []diode.Entity, currentDevice *diode.Device, prima // ref by-name lookup is ambiguous, stubForIface skips the // owner-rewrite rather than rebinding to a wrong member. ifaceByName := map[string][]*diode.Interface{} + // liveIfaceByAddr maps an address to the LIVE interface its top-level + // IPAddress entity is assigned to. TranslateAsStack has already routed that + // interface to its owning stack member, whereas a primary-IP snapshot froze + // the owner back during mapping. The primary-IP interface is deliberately + // excluded from top-level Interface emission (see getAssignedInterfaces), so + // ifaceByName structurally cannot resolve it and the address is the only + // exact key. Stores ALL matches so an address claimed by more than one + // interface is treated as ambiguous and left alone, mirroring ifaceByName. + liveIfaceByAddr := map[string][]*diode.Interface{} for _, e := range entities { switch v := e.(type) { case *diode.Device: @@ -268,6 +284,12 @@ func PruneNestedRefs(entities []diode.Entity, currentDevice *diode.Device, prima if v.Name != nil { ifaceByName[*v.Name] = append(ifaceByName[*v.Name], v) } + case *diode.IPAddress: + if v.Address != nil { + if i, ok := v.AssignedObject.(*diode.Interface); ok && i != nil { + liveIfaceByAddr[*v.Address] = append(liveIfaceByAddr[*v.Address], i) + } + } } } @@ -332,11 +354,57 @@ func PruneNestedRefs(entities []diode.Entity, currentDevice *diode.Device, prima return newInterfaceStub(ref, stubFor(resolveIfaceOwner(ref))) } + // prunePrimarySnapshot stubs the device ref buried in a top-level Device's + // primary-IP snapshot. detachForPrimaryIP shallow-copies the Device during + // mapping to break the Device -> IP -> Interface -> Device cycle, so without + // this the snapshot keeps whatever the Device looked like BEFORE + // TranslateAsStack, the annotators and name suppression ran: a stale + // device_type, a hostname the operator asked to suppress, no source_match, + // and the master as owner where the live interface belongs to a member. + // + // Cannot reintroduce the cycle: newDeviceStub carries no PrimaryIp4/6 and + // newInterfaceStub carries no Parent/Bridge/Lag/Module, which is exactly what + // detachForPrimaryIP clears by hand. + prunePrimarySnapshot := func(ip *diode.IPAddress) { + if ip == nil { + return + } + iface, ok := ip.AssignedObject.(*diode.Interface) + if !ok || iface == nil { + return + } + // Resolve the owner from the live entity for this exact address when + // there is exactly one: it reflects TranslateAsStack's member routing, + // while the snapshot's own copy is frozen at the master. + if ip.Address != nil { + if live, ok := liveIfaceByAddr[*ip.Address]; ok && len(live) == 1 && live[0] != nil { + iface = live[0] + } + } + // Deliberately still routed through stubForIface rather than stubbing + // live[0].Device directly. The point of this function is that the + // snapshot and the live cycle-closer entity agree, and the live entity + // goes through resolveIfaceOwner too; bypassing it here would let the + // two diverge again whenever resolveIfaceOwner rewrites an owner. + stubbed := stubForIface(iface) + // stubFor has an escape hatch that returns the ref unchanged when no + // owning Device can be resolved. Writing a still-rich device back here + // would point the snapshot at a Device that carries this very primary + // IP, and the SDK's proto conversion does not detect cycles. + if stubbed != nil && stubbed.Device != nil && + (stubbed.Device.PrimaryIp4 != nil || stubbed.Device.PrimaryIp6 != nil) { + return + } + ip.AssignedObject = stubbed + } + for _, entity := range entities { switch e := entity.(type) { case *diode.Device: - // Top-level rich Devices stay rich. - continue + // The Device itself stays rich, but its primary-IP snapshots hold + // nested refs like any other entity and must be stubbed. + prunePrimarySnapshot(e.PrimaryIp4) + prunePrimarySnapshot(e.PrimaryIp6) case *diode.Interface: e.Device = stubFor(e.Device) if e.Parent != nil { @@ -349,31 +417,36 @@ func PruneNestedRefs(entities []diode.Entity, currentDevice *diode.Device, prima e.Lag = stubForIface(e.Lag) } if e.Module != nil { - // Reduce nested Interface.Module to a matcher-only ref: - // chassis Device stub + Serial (if known) + ModuleBay - // matcher (if known). The top-level Module entity - // carries the full record (module_type, description, - // status, etc.); this nested form lets the Diode - // reconciler resolve the ref to that top-level row via - // the (Device, ModuleBay) or (Device, Serial) match - // paths without re-creating it. + // Reduce nested Interface.Module to a matcher-only ref: chassis + // Device stub + ModuleBay matcher + ModuleType, plus Serial when + // known. The top-level Module entity carries the full record + // (description, status, etc.). + // + // ModuleBay is the load-bearing part: it is dcim.module's only + // unique matcher besides asset_tag. Serial is carried for create + // fidelity only — dcim.module has no serial matcher, so a + // serial-only stub resolves nothing. Vendors that omit transceiver + // serial in ENTITY-MIB (some Aruba and low-end OEMs) still populate + // the bay, so the bay alone must keep the ref resolvable. // - // Shape mirrors device-discovery's _module_match_stub - // (device-discovery/device_discovery/stubs.py - // `_module_match_stub`): emit Device unconditionally, - // then conditionally copy Serial and ModuleBay matcher - // fields when present on the rich Module. Vendors that - // omit transceiver serial in ENTITY-MIB (some Aruba - // and low-end OEMs) still populate the bay, so the - // (Device, ModuleBay) path alone must keep the ref - // resolvable. + // ModuleType is retained even though it matches nothing. A ref only + // resolves once the referenced ModuleBay row exists; until then + // Diode falls back to CREATING the Module, and NetBox rejects a + // Module without a module_type. Emitting module entities ahead of + // interfaces does not prevent that: bulk-plan-apply batches plans + // and applies them independently, so payload order is not + // reconciliation order, and one failed plan takes its whole batch + // down — including unrelated interfaces. Only the + // (Manufacturer, Model) pair is copied, which is the complete + // dcim.moduletype matcher, so the ref cannot grow as drivers + // populate richer ModuleType fields. // - // Only when BOTH Serial AND ModuleBay are unusable do - // we drop the ref entirely — at that point the stub - // carries no identifier and the reconciler would fall - // into creation mode and fail the - // "module_bay required, module_type required" - // validation (we also strip ModuleType). + // Mirrors device-discovery's _module_match_stub + // (device-discovery/device_discovery/stubs.py). + // + // Only when BOTH Serial AND ModuleBay are unusable do we drop the + // ref entirely: it then carries no identifier, and creating it + // would fail anyway because NetBox requires module_bay. devStub := stubFor(e.Module.Device) stub := &diode.Module{Device: devStub} if e.Module.Serial != nil && *e.Module.Serial != "" { @@ -386,6 +459,14 @@ func PruneNestedRefs(entities []diode.Entity, currentDevice *diode.Device, prima Position: e.Module.ModuleBay.Position, } } + if mt := e.Module.ModuleType; mt != nil { + stub.ModuleType = &diode.ModuleType{Model: mt.Model} + if mt.Manufacturer != nil { + stub.ModuleType.Manufacturer = &diode.Manufacturer{ + Name: mt.Manufacturer.Name, + } + } + } if stub.Serial == nil && stub.ModuleBay == nil { // Unresolvable: drop the ref so the reconciler // doesn't try to create a Module without the diff --git a/orb-discovery/snmp-discovery/mapping/stubs_multi_device_test.go b/orb-discovery/snmp-discovery/mapping/stubs_multi_device_test.go index 1f14dc23..bdbadbc5 100644 --- a/orb-discovery/snmp-discovery/mapping/stubs_multi_device_test.go +++ b/orb-discovery/snmp-discovery/mapping/stubs_multi_device_test.go @@ -178,3 +178,239 @@ func TestPruneNestedRefs_UniqueIfaceNameStillCorrectsStaleParent(t *testing.T) { assert.Equal(t, "sw1-2", *sub.Parent.Device.Name, "unique-name lookup must still rewrite stale Parent.Device to the correct member") } + +// --- primary-IP snapshot pruning --- +// +// detachForPrimaryIP shallow-copies the Device into Device.PrimaryIp4/6 during +// mapping, before TranslateAsStack, the annotators and name suppression run. +// PruneNestedRefs must rebuild that subtree or the payload describes one device +// twice with divergent values. + +func snapshotDevice(t *testing.T, ip *diode.IPAddress) *diode.Device { + t.Helper() + iface, ok := ip.AssignedObject.(*diode.Interface) + if !ok || iface == nil { + t.Fatal("primary-IP snapshot lost its assigned interface") + } + return iface.Device +} + +func TestPruneNestedRefs_PrimaryIPSnapshotUsesPostMutationDeviceType(t *testing.T) { + addr := "10.0.0.1/24" + dev := &diode.Device{ + Name: strPtr("sw1"), + Site: &diode.Site{Name: strPtr("dc1")}, + DeviceType: &diode.DeviceType{Model: strPtr("platform-family-label")}, + } + iface := &diode.Interface{Name: strPtr("Vlan12"), Device: dev} + dev.PrimaryIp4 = detachForPrimaryIP(&diode.IPAddress{Address: &addr, AssignedObject: iface}, dev) + + // TranslateAsStack replaces the pointer with the real product model. + dev.DeviceType = &diode.DeviceType{Model: strPtr("REAL-PRODUCT-MODEL")} + PruneNestedRefs([]diode.Entity{dev}, dev, nil) + + got := snapshotDevice(t, dev.PrimaryIp4) + assert.Equal(t, "REAL-PRODUCT-MODEL", *got.DeviceType.Model, + "snapshot must carry the model the rich device ended up with, not the one it started with") +} + +// Pins the postcondition BOTH mechanisms share: detachForPrimaryIP clears these +// by hand and the stubs lack them by construction. It therefore cannot fail when +// the prune is reverted — that revert is killed by the five tests around it. Its +// value is catching a stub constructor that starts carrying a structural ref. +func TestPruneNestedRefs_PrimaryIPSnapshotStaysCycleFree(t *testing.T) { + addr := "10.0.0.1/24" + v6 := "2001:db8::1/64" + dev := &diode.Device{ + Name: strPtr("sw1"), Site: &diode.Site{Name: strPtr("dc1")}, + DeviceType: &diode.DeviceType{Model: strPtr("M")}, + } + mk := func(name string) *diode.Interface { + return &diode.Interface{ + Name: strPtr(name), Device: dev, + Parent: &diode.Interface{Name: strPtr("parent"), Device: dev}, + Bridge: &diode.Interface{Name: strPtr("bridge"), Device: dev}, + Lag: &diode.Interface{Name: strPtr("lag"), Device: dev}, + Module: &diode.Module{Device: dev}, + } + } + dev.PrimaryIp4 = detachForPrimaryIP(&diode.IPAddress{Address: &addr, AssignedObject: mk("Vlan12")}, dev) + dev.PrimaryIp6 = detachForPrimaryIP6(&diode.IPAddress{Address: &v6, AssignedObject: mk("Vlan12")}, dev) + PruneNestedRefs([]diode.Entity{dev}, dev, nil) + + for label, ip := range map[string]*diode.IPAddress{"v4": dev.PrimaryIp4, "v6": dev.PrimaryIp6} { + iface := ip.AssignedObject.(*diode.Interface) + assert.Nil(t, iface.Device.PrimaryIp4, label+": nested device must not regain a primary IP") + assert.Nil(t, iface.Device.PrimaryIp6, label+": nested device must not regain a primary IP") + assert.Nil(t, iface.Parent, label+": relationship pointers reintroduce the cycle") + assert.Nil(t, iface.Bridge, label) + assert.Nil(t, iface.Lag, label) + assert.Nil(t, iface.Module, label) + } +} + +func TestPruneNestedRefs_PrimaryIPSnapshotCarriesSourceMatch(t *testing.T) { + addr := "10.0.0.1/24" + dev := &diode.Device{ + Name: strPtr("sw1"), Site: &diode.Site{Name: strPtr("dc1")}, + DeviceType: &diode.DeviceType{Model: strPtr("M")}, + } + iface := &diode.Interface{Name: strPtr("Vlan12"), Device: dev} + dev.PrimaryIp4 = detachForPrimaryIP(&diode.IPAddress{Address: &addr, AssignedObject: iface}, dev) + + // Stamped AFTER the snapshot, onto a freshly allocated map — the production + // order. Stamping before would share the map and pass without the fix. + dev.Metadata = diode.Metadata{"source_match": diode.Metadata{"netbox_id": 7}} + PruneNestedRefs([]diode.Entity{dev}, dev, nil) + + got := snapshotDevice(t, dev.PrimaryIp4) + _, ok := got.Metadata["source_match"] + assert.True(t, ok, + "both representations must resolve via the same matcher path, or they can create a duplicate device") +} + +// The snapshot's interface must be attributed to the member that owns it. +// +// The IP-assigned interface is deliberately absent from the top-level entity +// slice (MapObjectIDsToEntity excludes it), which is why the owner is resolved +// from the live IPAddress entity by address rather than by interface name. A +// fixture that registers it as a top-level Interface passes without the fix. +func TestPruneNestedRefs_PrimaryIPSnapshotRoutesToOwningMember(t *testing.T) { + addr := "10.0.0.1/24" + dtype := &diode.DeviceType{Model: strPtr("M")} + master := &diode.Device{Name: strPtr("sw1"), Site: &diode.Site{Name: strPtr("dc1")}, DeviceType: dtype} + member := &diode.Device{Name: strPtr("sw1-2"), Site: &diode.Site{Name: strPtr("dc1")}, DeviceType: dtype} + + master.PrimaryIp4 = detachForPrimaryIP(&diode.IPAddress{ + Address: &addr, + AssignedObject: &diode.Interface{Name: strPtr("Gi2/0/48"), Device: master}, + }, master) + + // The live entity, already routed to the member by TranslateAsStack. + liveIP := &diode.IPAddress{ + Address: &addr, + AssignedObject: &diode.Interface{Name: strPtr("Gi2/0/48"), Device: member}, + } + + PruneNestedRefs([]diode.Entity{master, member, liveIP}, master, + map[*diode.IPAddress]bool{liveIP: true}) + + assert.Equal(t, "sw1-2", *snapshotDevice(t, master.PrimaryIp4).Name, + "one port must not be claimed by two devices, and two devices must not claim one unique primary_ip4") +} + +func TestPruneNestedRefs_PrimaryIPSnapshotAmbiguousAddressLeftAlone(t *testing.T) { + addr := "10.0.0.1/24" + dtype := &diode.DeviceType{Model: strPtr("M")} + master := &diode.Device{Name: strPtr("sw1"), Site: &diode.Site{Name: strPtr("dc1")}, DeviceType: dtype} + m2 := &diode.Device{Name: strPtr("sw1-2"), Site: &diode.Site{Name: strPtr("dc1")}, DeviceType: dtype} + m3 := &diode.Device{Name: strPtr("sw1-3"), Site: &diode.Site{Name: strPtr("dc1")}, DeviceType: dtype} + + master.PrimaryIp4 = detachForPrimaryIP(&diode.IPAddress{ + Address: &addr, + AssignedObject: &diode.Interface{Name: strPtr("Vlan12"), Device: master}, + }, master) + ip2 := &diode.IPAddress{ + Address: &addr, + AssignedObject: &diode.Interface{Name: strPtr("Vlan12"), Device: m2}, + } + ip3 := &diode.IPAddress{ + Address: &addr, + AssignedObject: &diode.Interface{Name: strPtr("Vlan12"), Device: m3}, + } + + // A post-snapshot model change, so this also dies on a full revert rather + // than only on the len==1 -> len>0 weakening. + master.DeviceType = &diode.DeviceType{Model: strPtr("REAL-PRODUCT-MODEL")} + PruneNestedRefs([]diode.Entity{master, m2, m3, ip2, ip3}, master, nil) + + got := snapshotDevice(t, master.PrimaryIp4) + assert.Equal(t, "sw1", *got.Name, + "two interfaces claiming one address is ambiguous: keep the existing owner rather than guessing") + assert.Equal(t, "REAL-PRODUCT-MODEL", *got.DeviceType.Model, + "ambiguity suppresses only the owner rewrite, not the rebuild") +} + +// The v6 twin. Without this, a fix applied only to detachForPrimaryIP's subtree +// would ship green: the cycle-free assertions above hold either way, because the +// shallow copy also nils both primary families. +func TestPruneNestedRefs_PrimaryIP6SnapshotUsesPostMutationDeviceType(t *testing.T) { + v6 := "2001:db8::1/64" + dev := &diode.Device{ + Name: strPtr("sw1"), + Site: &diode.Site{Name: strPtr("dc1")}, + DeviceType: &diode.DeviceType{Model: strPtr("platform-family-label")}, + } + iface := &diode.Interface{Name: strPtr("Vlan12"), Device: dev} + dev.PrimaryIp6 = detachForPrimaryIP6(&diode.IPAddress{Address: &v6, AssignedObject: iface}, dev) + + dev.DeviceType = &diode.DeviceType{Model: strPtr("REAL-PRODUCT-MODEL")} + PruneNestedRefs([]diode.Entity{dev}, dev, nil) + + assert.Equal(t, "REAL-PRODUCT-MODEL", *snapshotDevice(t, dev.PrimaryIp6).DeviceType.Model, + "the v6 path must be fixed too; the capture that surfaced this had no IPv6 primary") +} + +// The index is keyed by ADDRESS, not by interface name, and that is load-bearing: +// two members can expose same-named interfaces (per-member mgmt ports), so a +// name-keyed index could bind the primary to an unrelated device. Re-keying +// liveIfaceByAddr by name leaves the rest of the suite green, so this is the only +// test that pins the decision. +func TestPruneNestedRefs_PrimaryIPSnapshotKeysOnAddressNotInterfaceName(t *testing.T) { + dtype := &diode.DeviceType{Model: strPtr("M")} + master := &diode.Device{Name: strPtr("sw1"), Site: &diode.Site{Name: strPtr("dc1")}, DeviceType: dtype} + m2 := &diode.Device{Name: strPtr("sw1-2"), Site: &diode.Site{Name: strPtr("dc1")}, DeviceType: dtype} + m3 := &diode.Device{Name: strPtr("sw1-3"), Site: &diode.Site{Name: strPtr("dc1")}, DeviceType: dtype} + + mine, theirs := "10.0.0.1/24", "10.0.0.2/24" + master.PrimaryIp4 = detachForPrimaryIP(&diode.IPAddress{ + Address: &mine, + AssignedObject: &diode.Interface{Name: strPtr("mgmt0"), Device: master}, + }, master) + + // Both members expose an interface called mgmt0; only one holds this address. + ipMine := &diode.IPAddress{ + Address: &mine, + AssignedObject: &diode.Interface{Name: strPtr("mgmt0"), Device: m2}, + } + ipTheirs := &diode.IPAddress{ + Address: &theirs, + AssignedObject: &diode.Interface{Name: strPtr("mgmt0"), Device: m3}, + } + + PruneNestedRefs([]diode.Entity{master, m2, m3, ipMine, ipTheirs}, master, nil) + + assert.Equal(t, "sw1-2", *snapshotDevice(t, master.PrimaryIp4).Name, + "the address must select the owner; a name-keyed lookup could pick sw1-3") +} + +// stubFor returns the ref unchanged when it can resolve no owning Device (no +// name/serial match and no currentDevice). Writing that rich ref into the +// snapshot would leave a Device carrying its own primary IP reachable from that +// same primary IP, and the SDK's proto conversion has no cycle detection, so it +// would be a hard crash rather than an ingest error. Unreachable from the runner, +// which always passes a currentDevice, but PruneNestedRefs is exported. +func TestPruneNestedRefs_PrimaryIPSnapshotNeverWritesBackARichDevice(t *testing.T) { + addr := "10.0.0.1/24" + dtype := &diode.DeviceType{Model: strPtr("M")} + dev := &diode.Device{Name: strPtr("sw1"), Site: &diode.Site{Name: strPtr("dc1")}, DeviceType: dtype} + dev.PrimaryIp4 = detachForPrimaryIP(&diode.IPAddress{ + Address: &addr, + AssignedObject: &diode.Interface{Name: strPtr("Vlan12"), Device: dev}, + }, dev) + + // A live entity whose owner is resolvable from neither index. + foreign := &diode.Device{Name: strPtr("absent-from-entities"), DeviceType: dtype} + foreign.PrimaryIp4 = &diode.IPAddress{Address: &addr} + liveIP := &diode.IPAddress{ + Address: &addr, + AssignedObject: &diode.Interface{Name: strPtr("Vlan12"), Device: foreign}, + } + + PruneNestedRefs([]diode.Entity{dev, liveIP}, nil, nil) + + got := snapshotDevice(t, dev.PrimaryIp4) + assert.Nil(t, got.PrimaryIp4, + "a device reachable from a primary IP must never itself carry one") + assert.Nil(t, got.PrimaryIp6, "same for the v6 family") +} diff --git a/orb-discovery/snmp-discovery/mapping/stubs_test.go b/orb-discovery/snmp-discovery/mapping/stubs_test.go index aed340bd..0bfae26b 100644 --- a/orb-discovery/snmp-discovery/mapping/stubs_test.go +++ b/orb-discovery/snmp-discovery/mapping/stubs_test.go @@ -603,16 +603,19 @@ func TestPruneNestedRefs_StubsModuleBayAndInterfaceModule(t *testing.T) { assert.Nil(t, bay.Device.Status, "ModuleBay.Device status stripped on stub") // Interface.Module reduced to a matcher-only ref: Device stub + Serial - // + ModuleBay matcher (name+position+device stub). Mirrors - // device-discovery's _module_match_stub so the Diode reconciler - // resolves the ref to the existing top-level Module instead of - // trying to create one and failing the "module_bay/module_type - // required" validation. ModuleType / Description / Status etc. stay - // dropped so the wire payload stays bounded. + // + ModuleBay matcher (name+position+device stub) + ModuleType. + // Mirrors device-discovery's _module_match_stub. ModuleType is not a + // matcher, but a ref that has not yet resolved is CREATED instead, and + // NetBox rejects a Module without one; only the (Manufacturer, Model) + // matcher pair is copied so the ref stays bounded. Description / + // Status etc. stay dropped. require.NotNil(t, iface.Module) assert.Equal(t, "FNS24010TR1", strDerefSafe(iface.Module.Serial)) - assert.Nil(t, iface.Module.ModuleType, - "Interface.Module stub must not carry ModuleType (drops large nested manufacturer)") + require.NotNil(t, iface.Module.ModuleType, + "Interface.Module stub must carry ModuleType: NetBox requires it when the ref is created") + assert.Equal(t, "SFP-10G-LR", strDerefSafe(iface.Module.ModuleType.Model)) + require.NotNil(t, iface.Module.ModuleType.Manufacturer) + assert.Equal(t, "Cisco", strDerefSafe(iface.Module.ModuleType.Manufacturer.Name)) assert.Nil(t, iface.Module.Description, "Interface.Module stub must not carry Description") @@ -675,7 +678,8 @@ func TestPruneNestedRefs_InterfaceModuleSurvivesWhenSerialNilButBayPresent(t *te assert.Equal(t, "TenGigabitEthernet1/0/1", strDerefSafe(iface.Module.ModuleBay.Name)) assert.Equal(t, "1", strDerefSafe(iface.Module.ModuleBay.Position)) assert.Nil(t, iface.Module.Serial, "serial stays nil — matcher uses Device + Bay") - assert.Nil(t, iface.Module.ModuleType, "ModuleType still dropped per Python parity") + require.NotNil(t, iface.Module.ModuleType, + "ModuleType retained per Python parity: required when the ref is created") require.NotNil(t, iface.Module.Device, "Interface.Module.Device must be a chassis stub") assert.Equal(t, "sw1", strDerefSafe(iface.Module.Device.Name)) assert.Nil(t, iface.Module.Device.Status, "Module.Device must be stubbed (no rich Status)") @@ -714,17 +718,64 @@ func TestPruneNestedRefs_InterfaceModuleStubDegradesToSerialOnly(t *testing.T) { require.NotNil(t, iface.Module, "must not be cleared when serial is present") assert.Equal(t, "FNS00000001", strDerefSafe(iface.Module.Serial)) assert.Nil(t, iface.Module.ModuleBay, "no bay was set on input, stays nil") - assert.Nil(t, iface.Module.ModuleType, "ModuleType still dropped per Python parity") + require.NotNil(t, iface.Module.ModuleType, + "ModuleType retained per Python parity: required when the ref is created") require.NotNil(t, iface.Module.Device) assert.Equal(t, "sw1", strDerefSafe(iface.Module.Device.Name)) } +// TestPruneNestedRefs_InterfaceModuleTypeCopyIsBounded — the nested ref keeps +// ModuleType because NetBox requires it when the ref is created, but only the +// (Manufacturer, Model) matcher pair is copied. This stub is duplicated into +// every interface on a linecard, so it must not grow as drivers start +// populating the richer ModuleType fields. +func TestPruneNestedRefs_InterfaceModuleTypeCopyIsBounded(t *testing.T) { + rich := &diode.Device{ + Name: strPtr("sw1"), + Site: &diode.Site{Name: strPtr("dc1")}, + Serial: strPtr("FCW123"), + } + bay := &diode.ModuleBay{Device: rich, Name: strPtr("Gi1/0/1"), Position: strPtr("1")} + mod := &diode.Module{ + Device: rich, + ModuleBay: bay, + Serial: strPtr("FNS24010TR1"), + ModuleType: &diode.ModuleType{ + Model: strPtr("SFP-10G-LR"), + PartNumber: strPtr("SFP-10G-LR-S"), + Description: strPtr("10G LR optic"), + Comments: strPtr("a field a driver may start populating"), + Manufacturer: &diode.Manufacturer{ + Name: strPtr("Cisco"), + Slug: strPtr("cisco"), + Description: strPtr("vendor"), + }, + }, + } + iface := &diode.Interface{Name: strPtr("Gi1/0/1"), Device: rich, Module: mod} + + PruneNestedRefs([]diode.Entity{rich, bay, mod, iface}, rich, nil) + + require.NotNil(t, iface.Module) + require.NotNil(t, iface.Module.ModuleType) + mt := iface.Module.ModuleType + assert.Equal(t, "SFP-10G-LR", strDerefSafe(mt.Model)) + require.NotNil(t, mt.Manufacturer) + assert.Equal(t, "Cisco", strDerefSafe(mt.Manufacturer.Name)) + // everything outside the matcher pair must be absent + assert.Nil(t, mt.PartNumber, "PartNumber must not ride along") + assert.Nil(t, mt.Description, "ModuleType.Description must not ride along") + assert.Nil(t, mt.Comments, "ModuleType.Comments must not ride along") + assert.Nil(t, mt.Manufacturer.Slug, "Manufacturer.Slug must not ride along") + assert.Nil(t, mt.Manufacturer.Description, "Manufacturer.Description must not ride along") +} + // TestPruneNestedRefs_InterfaceModuleClearedWhenSerialAndBayBothMissing — // the only legitimate clear path. Without Serial AND without // ModuleBay, no matcher field can resolve the ref. Shipping such a -// stub would force the reconciler into creation mode and fail -// validation ("module_bay required, module_type required") because -// the stub also strips ModuleType. +// stub would force the reconciler into creation mode, which fails +// because NetBox requires module_bay. Retaining ModuleType does not +// rescue this case, so the ref is dropped instead. func TestPruneNestedRefs_InterfaceModuleClearedWhenSerialAndBayBothMissing(t *testing.T) { rich := &diode.Device{ Name: strPtr("sw1"), diff --git a/orb-discovery/snmp-discovery/policy/manager.go b/orb-discovery/snmp-discovery/policy/manager.go index f0bc5db3..ea1ff531 100644 --- a/orb-discovery/snmp-discovery/policy/manager.go +++ b/orb-discovery/snmp-discovery/policy/manager.go @@ -152,6 +152,11 @@ func (m *Manager) validateAuthentication(auth *config.Authentication, context st return fmt.Errorf("%s: unsupported protocol version", context) } + if auth.ContextName != "" && auth.ProtocolVersion != snmp.ProtocolVersion3 { + return fmt.Errorf("%s: context_name is only valid for SNMPv3 (got %q)", + context, auth.ProtocolVersion) + } + if auth.ProtocolVersion == "SNMPv2c" || auth.ProtocolVersion == "SNMPv1" { if auth.Community == "" { return fmt.Errorf("%s: missing community", context) @@ -200,6 +205,15 @@ func (m *Manager) validatePolicy(policy config.Policy) error { if err := m.validateAuthentication(&policy.Scope.Authentication, "policy-level"); err != nil { return err } + } else if policy.Scope.Authentication.ContextName != "" { + // A scope.authentication block with a context_name but no + // protocol_version skips the check above entirely (hasPolicyAuth is + // false), yet it is still env-resolved and then silently discarded + // for any target with its own authentication block. Catch it here + // rather than letting it disappear the same way the missing-context + // bug this ticket exists to fix did. + return fmt.Errorf("policy-level: context_name is only valid for SNMPv3 (got %q)", + policy.Scope.Authentication.ProtocolVersion) } // Validate each target's authentication @@ -326,6 +340,7 @@ func (m *Manager) resolveAuthenticationEnvVarsForAuth(auth *config.Authenticatio {&auth.Username, "username"}, {&auth.AuthPassphrase, "auth_passphrase"}, {&auth.PrivPassphrase, "priv_passphrase"}, + {&auth.ContextName, "context_name"}, } // Iterate over the fields and resolve environment variables diff --git a/orb-discovery/snmp-discovery/policy/manager_test.go b/orb-discovery/snmp-discovery/policy/manager_test.go index 7d0512d1..1bbbf2e5 100644 --- a/orb-discovery/snmp-discovery/policy/manager_test.go +++ b/orb-discovery/snmp-discovery/policy/manager_test.go @@ -419,6 +419,152 @@ func TestManagerParsePolicies(t *testing.T) { }) } +func TestManagerParsePolicies_ContextName(t *testing.T) { + logger := slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug, AddSource: false})) + manager, err := policy.NewManager(context.Background(), logger, nil, nil) + assert.NoError(t, err) + + t.Run("Environment Variable Resolution - context_name", func(t *testing.T) { + err := os.Setenv("SNMP_CONTEXT_NAME", "mfpdirect") + require.NoError(t, err) + defer func() { + _ = os.Unsetenv("SNMP_CONTEXT_NAME") + }() + + yamlData := []byte(` + policies: + policy1: + config: + lookup_extensions_dir: /tmp/extensions + scope: + targets: + - host: 192.168.1.1 + authentication: + protocol_version: SNMPv3 + security_level: noAuthNoPriv + username: testuser + context_name: ${SNMP_CONTEXT_NAME} + `) + + policies, err := manager.ParsePolicies(yamlData) + assert.NoError(t, err) + assert.Contains(t, policies, "policy1") + assert.Equal(t, "mfpdirect", policies["policy1"].Scope.Authentication.ContextName) + }) + + t.Run("Environment Variable Resolution - Missing context_name Environment Variable", func(t *testing.T) { + err := os.Unsetenv("MISSING_SNMP_CONTEXT_NAME") + require.NoError(t, err) + + yamlData := []byte(` + policies: + policy1: + config: + lookup_extensions_dir: /tmp/extensions + scope: + targets: + - host: 192.168.1.1 + authentication: + protocol_version: SNMPv3 + security_level: noAuthNoPriv + username: testuser + context_name: ${MISSING_SNMP_CONTEXT_NAME} + `) + + _, err = manager.ParsePolicies(yamlData) + assert.Error(t, err) + assert.Contains(t, err.Error(), "policy1 : failed to resolve environment variables") + assert.Contains(t, err.Error(), "failed to resolve context_name environment variable") + assert.Contains(t, err.Error(), "environment variable MISSING_SNMP_CONTEXT_NAME is not set") + }) + + t.Run("Rejects context_name on SNMPv2c policy-level auth", func(t *testing.T) { + yamlData := []byte(` + policies: + policy1: + config: + lookup_extensions_dir: /tmp/extensions + scope: + targets: + - host: 192.168.1.1 + authentication: + protocol_version: SNMPv2c + community: public + context_name: mfpdirect + `) + + _, err := manager.ParsePolicies(yamlData) + assert.Error(t, err) + assert.Contains(t, err.Error(), "policy-level: context_name is only valid for SNMPv3") + }) + + t.Run("Rejects context_name on SNMPv2c per-target auth", func(t *testing.T) { + yamlData := []byte(` + policies: + policy1: + config: + lookup_extensions_dir: /tmp/extensions + scope: + targets: + - host: 192.168.1.1 + authentication: + protocol_version: SNMPv2c + community: public + context_name: mfpdirect + `) + + _, err := manager.ParsePolicies(yamlData) + assert.Error(t, err) + assert.Contains(t, err.Error(), "target 192.168.1.1: context_name is only valid for SNMPv3") + }) + + t.Run("Rejects context_name in scope.authentication with no protocol_version", func(t *testing.T) { + // hasPolicyAuth is false here (ProtocolVersion is unset), so the + // per-field validateAuthentication check never runs. Without the + // standalone check this block is silently discarded. + yamlData := []byte(` + policies: + policy1: + config: + lookup_extensions_dir: /tmp/extensions + scope: + targets: + - host: 192.168.1.1 + authentication: + protocol_version: SNMPv3 + security_level: noAuthNoPriv + username: testuser + authentication: + context_name: mfpdirect + `) + + _, err := manager.ParsePolicies(yamlData) + assert.Error(t, err) + assert.Contains(t, err.Error(), "policy-level: context_name is only valid for SNMPv3") + }) + + t.Run("Accepts context_name on SNMPv3 policy-level auth", func(t *testing.T) { + yamlData := []byte(` + policies: + policy1: + config: + lookup_extensions_dir: /tmp/extensions + scope: + targets: + - host: 192.168.1.1 + authentication: + protocol_version: SNMPv3 + security_level: noAuthNoPriv + username: testuser + context_name: mfpdirect + `) + + policies, err := manager.ParsePolicies(yamlData) + assert.NoError(t, err) + assert.Equal(t, "mfpdirect", policies["policy1"].Scope.Authentication.ContextName) + }) +} + func TestManagerPolicyLifecycle(t *testing.T) { logger := slog.New(slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug, AddSource: false})) manager, err := policy.NewManager(context.Background(), logger, nil, nil) diff --git a/orb-discovery/snmp-discovery/snmp/snmp.go b/orb-discovery/snmp-discovery/snmp/snmp.go index fbbbdb74..b253b0b6 100644 --- a/orb-discovery/snmp-discovery/snmp/snmp.go +++ b/orb-discovery/snmp-discovery/snmp/snmp.go @@ -246,6 +246,7 @@ func NewClient(host string, port uint16, retries int, timeout time.Duration, aut Retries: retries, MsgFlags: msgFlags, SecurityModel: gosnmp.UserSecurityModel, + ContextName: authentication.ContextName, Logger: gosnmpLogger, SecurityParameters: &gosnmp.UsmSecurityParameters{ UserName: authentication.Username, diff --git a/orb-discovery/snmp-discovery/snmp/snmp_test.go b/orb-discovery/snmp-discovery/snmp/snmp_test.go index 5bc3ed4f..8bd364c1 100644 --- a/orb-discovery/snmp-discovery/snmp/snmp_test.go +++ b/orb-discovery/snmp-discovery/snmp/snmp_test.go @@ -1,6 +1,7 @@ package snmp_test import ( + "bytes" "context" "fmt" "log/slog" @@ -13,6 +14,7 @@ import ( "github.com/netboxlabs/diode-sdk-go/diode/v1/diodepb" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" "github.com/netboxlabs/orb-agent/orb-discovery/snmp-discovery/config" "github.com/netboxlabs/orb-agent/orb-discovery/snmp-discovery/mapping" @@ -249,10 +251,18 @@ func TestSNMPHost(t *testing.T) { func TestNewClient(t *testing.T) { testCases := []struct { - name string - auth *config.Authentication - expectError bool - expectedErrMsg string + name string + auth *config.Authentication + expectError bool + expectedErrMsg string + expectedContextName string + // checkContextWire additionally encodes a packet and asserts the + // context name is (or isn't) present on the wire. Only exercised for + // the context-name-specific rows below: it requires SecurityLevel to + // be unset (msgFlags == NoAuthNoPriv), since authPriv needs a + // discovered engine and fails with "invalid key size 0", and relying + // on bytes.Contains with an empty expected value would always match. + checkContextWire bool }{ { name: "Creates SNMPv1 client successfully", @@ -262,6 +272,44 @@ func TestNewClient(t *testing.T) { }, expectError: false, }, + { + name: "Creates SNMPv3 client with context name", + auth: &config.Authentication{ + ProtocolVersion: snmp.ProtocolVersion3, + Username: "testuser", + AuthProtocol: "MD5", + AuthPassphrase: "testpass", + PrivProtocol: "AES", + PrivPassphrase: "testpass", + ContextName: "mfpdirect", + }, + expectError: false, + expectedContextName: "mfpdirect", + checkContextWire: true, + }, + { + name: "Creates SNMPv3 client without context name", + auth: &config.Authentication{ + ProtocolVersion: snmp.ProtocolVersion3, + Username: "testuser", + AuthProtocol: "MD5", + AuthPassphrase: "testpass", + PrivProtocol: "AES", + PrivPassphrase: "testpass", + }, + expectError: false, + expectedContextName: "", + }, + { + name: "SNMPv2c ignores context name field", + auth: &config.Authentication{ + ProtocolVersion: snmp.ProtocolVersion2c, + Community: "public", + ContextName: "mfpdirect", + }, + expectError: false, + expectedContextName: "", + }, { name: "Creates SNMPv2c client successfully", auth: &config.Authentication{ @@ -445,6 +493,23 @@ func TestNewClient(t *testing.T) { } else { assert.NoError(t, err) assert.NotNil(t, client) + + typed, ok := client.(*snmp.Client) + assert.True(t, ok) + if ok { + assert.Equal(t, tc.expectedContextName, typed.ContextName) + + if tc.checkContextWire { + // Assert at the wire, not just on the struct: SnmpEncodePacket runs + // the same mkSnmpPacket -> marshalMsg -> prepareV3ScopedPDU path as a + // live send, with no Connect() and no socket. + out, err := typed.SnmpEncodePacket(gosnmp.GetRequest, + []gosnmp.SnmpPDU{{Name: "1.3.6.1.2.1.1.1.0", Type: gosnmp.Null}}, 0, 0) + require.NoError(t, err) + assert.Equal(t, tc.expectedContextName != "", + bytes.Contains(out, []byte(tc.expectedContextName))) + } + } } }) }