carverauto/serviceradar

Fork 0

2016 bugsync not picking up correct sweepjson in demo namespace #2478

Merged

mfreeman451 merged 3 commits from refs/pull/2478/head into main

2025-11-25 03:41:33 +00:00

mfreeman451 commented

2025-11-25 03:07:53 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2017
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2017
Original created: 2025-11-25T03:07:53Z
Original updated: 2025-11-25T03:41:36Z
Original head: carverauto/serviceradar:2016-bugsync-not-picking-up-correct-sweepjson-in-demo-namespace
Original base: main
Original merged: 2025-11-25T03:41:33Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Bug fix, Enhancement

Description

Simplified device counting logic in stats aggregator to include all non-nil records, preventing legitimate discovered devices from being hidden in UI statistics
Added unified devices upsert logic and periodic cleanup of stale devices not seen within retention period (default 3 days)
Enhanced checker configuration lookup to use checker config's Address field as fallback when request details are empty
Implemented device retention configuration with defaults and integrated cleanup into metrics cleanup routine
Added comprehensive SRQL query support for device_updates entity with filtering, time range, and ordering capabilities
Regenerated mocks with improved parameter naming and new methods for better code maintainability
Extended Helm configuration with new gRPC checker for sync service

Diagram Walkthrough

flowchart LR
  A["Stats Aggregator<br/>Count all records"] --> B["Unified Devices<br/>Upsert & Track"]
  B --> C["Cleanup Routine<br/>Remove stale devices"]
  D["Checker Config<br/>Address fallback"] --> E["Agent Server<br/>Enhanced lookup"]
  F["Device Retention<br/>Config defaults"] --> C
  G["SRQL Parser<br/>device_updates entity"] --> H["Query Engine<br/>Execute filters"]
  H --> I["Device Updates<br/>Query results"]

File Walkthrough

Relevant files

Miscellaneous

1 files

mock_db.go

Regenerate mocks with improved parameter naming and new methods

pkg/db/mock_db.go

Regenerated mock file with improved parameter naming (e.g., arg0 →
ctx, arg1 → pollerID) for better code readability
Updated mockgen command to use -source=pkg/db/interfaces.go instead of
module path for more maintainable mock generation
Added new MockQueryExecutor interface implementation with ExecuteQuery
method
Added new mock methods CleanupStaleUnifiedDevices and
CountUnifiedDevices to MockService
Added new mock method GetDeviceMetricTypes to MockService
Reorganized method order in mock file for better logical grouping

+395/-380

Enhancement

10 files

cnpg_unified_devices.go `Add unified devices upsert and stale device cleanup` pkg/db/cnpg_unified_devices.go Added upsert logic into `unified_devices` table alongside existing `device_updates` insert to maintain current device state Implemented `CleanupStaleUnifiedDevices` function to periodically remove devices not seen within retention period Added comment explaining the dual-write pattern for device history and current state Upsert merges discovery sources and metadata while preserving existing values for empty fields	+64/-0
server.go `Use checker config address as fallback for details` pkg/agent/server.go Enhanced checker configuration lookup to use checker config's `Address` field as fallback when request details are empty Added logging when using checker config address as details Refactored conditional logic to check for both security configuration and address availability	+21/-9
server.go `Add periodic stale device cleanup to metrics cleanup routine` pkg/core/server.go Added device retention configuration from metrics config with 3-day default Implemented periodic cleanup of stale unified devices in `runMetricsCleanup` goroutine Added logging for cleanup operations including deleted device count Cleanup runs hourly alongside existing metrics cleanup	+21/-0
db.go `Add device retention days field to metrics config` pkg/models/db.go Added `DeviceRetentionDays` field to `Metrics` struct with JSON tag and omitempty option Field defaults to 3 days and controls how long devices are kept in `unified_devices` table	+4/-3
interfaces.go `Add cleanup method to service interface` pkg/db/interfaces.go Added `CleanupStaleUnifiedDevices` method signature to `Service` interface Method accepts context and retention duration, returns deleted count and error	+1/-0
device_updates.rs `Add device updates query support to SRQL` rust/srql/src/query/device_updates.rs New file implementing query execution for `device_updates` entity Supports filtering by device_id, ip, mac, hostname, poller_id, agent_id, partition, discovery_source, and availability Implements time range filtering and ordering by observed_at or device_id Provides query building and debug SQL generation capabilities	+169/-0
mod.rs `Add device_updates module integration to query engine` rust/srql/src/query/mod.rs Added module declaration for `device_updates` Integrated `DeviceUpdates` entity handling in query execution match statement Integrated `DeviceUpdates` entity handling in debug SQL generation match statement	+3/-0
models.rs `Add DeviceUpdateRow model with JSON serialization` rust/srql/src/models.rs Defined new `DeviceUpdateRow` struct with Diesel queryable derive macros Implemented `into_json()` method to convert `DeviceUpdateRow` to JSON representation Includes fields for device metadata, timestamps, and discovery information	+36/-0
parser.rs `Add DeviceUpdates entity type to parser` rust/srql/src/parser.rs Added `DeviceUpdates` variant to `Entity` enum Extended entity name parsing to recognize "device_updates", "device_update", and "updates" aliases	+2/-0
schema.rs `Add device_updates table schema definition` rust/srql/src/schema.rs Defined new `device_updates` Diesel table schema with composite primary key Includes fields for device tracking, discovery source, and optional metadata	+19/-0

Configuration changes

2 files

config.go `Add device retention configuration defaults` pkg/core/config.go Added `defaultDeviceRetentionDays` constant set to 3 days Added normalization logic to set default device retention if not configured Ensures device retention defaults are applied during config initialization	+8/-3
serviceradar-config.yaml `Add sync service gRPC checker configuration` helm/serviceradar/files/serviceradar-config.yaml Added new gRPC checker configuration for `sync` service with 5-minute results interval Extends existing checker configurations in the Helm values file	+5/-0

Bug fix

1 files

stats_aggregator.go

Simplify device counting logic to include all records

pkg/core/stats_aggregator.go

Simplified shouldCountRecord function to count all non-nil records
without filtering sweep-only devices
Changed logic to rely on database as source of truth for device
filtering
Prevents legitimate discovered devices from being hidden in UI
statistics

+5/-7

Imported from GitHub pull request. Original GitHub pull request: #2017 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2017 Original created: 2025-11-25T03:07:53Z Original updated: 2025-11-25T03:41:36Z Original head: carverauto/serviceradar:2016-bugsync-not-picking-up-correct-sweepjson-in-demo-namespace Original base: main Original merged: 2025-11-25T03:41:33Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix, Enhancement ___ ### **Description** - Simplified device counting logic in stats aggregator to include all non-nil records, preventing legitimate discovered devices from being hidden in UI statistics - Added unified devices upsert logic and periodic cleanup of stale devices not seen within retention period (default 3 days) - Enhanced checker configuration lookup to use checker config's `Address` field as fallback when request details are empty - Implemented device retention configuration with defaults and integrated cleanup into metrics cleanup routine - Added comprehensive SRQL query support for `device_updates` entity with filtering, time range, and ordering capabilities - Regenerated mocks with improved parameter naming and new methods for better code maintainability - Extended Helm configuration with new gRPC checker for sync service ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Stats Aggregator Count all records"] --> B["Unified Devices Upsert & Track"] B --> C["Cleanup Routine Remove stale devices"] D["Checker Config Address fallback"] --> E["Agent Server Enhanced lookup"] F["Device Retention Config defaults"] --> C G["SRQL Parser device_updates entity"] --> H["Query Engine Execute filters"] H --> I["Device Updates Query results"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Miscellaneous</td><td><details><summary>1 files</summary><table> <tr> <td> <details> <summary>mock_db.go<dd><code>Regenerate mocks with improved parameter naming and new methods</code></dd></summary> <hr> pkg/db/mock_db.go <ul><li>Regenerated mock file with improved parameter naming (e.g., <code>arg0</code> → <code>ctx</code>, <code>arg1</code> → <code>pollerID</code>) for better code readability <li> Updated mockgen command to use <code>-source=pkg/db/interfaces.go</code> instead of module path for more maintainable mock generation <li> Added new <code>MockQueryExecutor</code> interface implementation with <code>ExecuteQuery</code> method <li> Added new mock methods <code>CleanupStaleUnifiedDevices</code> and <code>CountUnifiedDevices</code> to <code>MockService</code> <li> Added new mock method <code>GetDeviceMetricTypes</code> to <code>MockService</code> <li> Reorganized method order in mock file for better logical grouping</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-30e38f888d4849fc40d7ebb1559c2a84c43aa8cd13b3b89fd7ec6cf873b243c7">+395/-380</a></td> </tr> </table></details></td></tr><tr><td>Enhancement</td><td><details><summary>10 files</summary><table> <tr> <td> <details> <summary>cnpg_unified_devices.go<dd><code>Add unified devices upsert and stale device cleanup</code>            </dd></summary> <hr> pkg/db/cnpg_unified_devices.go <ul><li>Added upsert logic into <code>unified_devices</code> table alongside existing <code>device_updates</code> insert to maintain current device state <li> Implemented <code>CleanupStaleUnifiedDevices</code> function to periodically remove devices not seen within retention period <li> Added comment explaining the dual-write pattern for device history and current state <li> Upsert merges discovery sources and metadata while preserving existing values for empty fields</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-ab3bf557cf9bb1b281a315a73abee38748de1654941c2471542e5e9bfc1716d8">+64/-0</a>    </td> </tr> <tr> <td> <details> <summary>server.go<dd><code>Use checker config address as fallback for details</code>              </dd></summary> <hr> pkg/agent/server.go <ul><li>Enhanced checker configuration lookup to use checker config's <code>Address</code> field as fallback when request details are empty <li> Added logging when using checker config address as details <li> Refactored conditional logic to check for both security configuration and address availability</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-3c51e5356a25859a3300ab62ed2494462feb2567ef69e6db3aa2bbc1032c1c5d">+21/-9</a>    </td> </tr> <tr> <td> <details> <summary>server.go<dd><code>Add periodic stale device cleanup to metrics cleanup routine</code></dd></summary> <hr> pkg/core/server.go <ul><li>Added device retention configuration from metrics config with 3-day default <li> Implemented periodic cleanup of stale unified devices in <code>runMetricsCleanup</code> goroutine <li> Added logging for cleanup operations including deleted device count <li> Cleanup runs hourly alongside existing metrics cleanup</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-a12d34beb8640efbbedad28f8610fd58afeea457572d82e4fd295ab2a6bb8ee6">+21/-0</a>    </td> </tr> <tr> <td> <details> <summary>db.go<dd><code>Add device retention days field to metrics config</code>                </dd></summary> <hr> pkg/models/db.go <ul><li>Added <code>DeviceRetentionDays</code> field to <code>Metrics</code> struct with JSON tag and omitempty option <li> Field defaults to 3 days and controls how long devices are kept in <code>unified_devices</code> table</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-0ae965d146dd5aa35ea3cf7713541c1f2e4c92e1bdc339fbd84296aa625d5f5e">+4/-3</a>      </td> </tr> <tr> <td> <details> <summary>interfaces.go<dd><code>Add cleanup method to service interface</code>                                    </dd></summary> <hr> pkg/db/interfaces.go <ul><li>Added <code>CleanupStaleUnifiedDevices</code> method signature to <code>Service</code> interface <li> Method accepts context and retention duration, returns deleted count and error</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-c230fe0c315251837357bfde4ae7f7b34080398d8e48af6bf78badb2124271f3">+1/-0</a>      </td> </tr> <tr> <td> <details> <summary>device_updates.rs<dd><code>Add device updates query support to SRQL</code>                                  </dd></summary> <hr> rust/srql/src/query/device_updates.rs <ul><li>New file implementing query execution for <code>device_updates</code> entity <li> Supports filtering by device_id, ip, mac, hostname, poller_id, agent_id, partition, discovery_source, and availability <li> Implements time range filtering and ordering by observed_at or device_id <li> Provides query building and debug SQL generation capabilities</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-77b99f0ef409a6a3e4ecc65800ae841231a82b27a64e8dc1d4667db6698a539d">+169/-0</a>  </td> </tr> <tr> <td> <details> <summary>mod.rs<dd><code>Add device_updates module integration to query engine</code>        </dd></summary> <hr> rust/srql/src/query/mod.rs <ul><li>Added module declaration for <code>device_updates</code> <li> Integrated <code>DeviceUpdates</code> entity handling in query execution match statement <li> Integrated <code>DeviceUpdates</code> entity handling in debug SQL generation match statement</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-393e3fa3d0e41741834cd7cd398a06111ab7b141ae6caca7a5dcc0e036172491">+3/-0</a>      </td> </tr> <tr> <td> <details> <summary>models.rs<dd><code>Add DeviceUpdateRow model with JSON serialization</code>                </dd></summary> <hr> rust/srql/src/models.rs <ul><li>Defined new <code>DeviceUpdateRow</code> struct with Diesel queryable derive macros <li> Implemented <code>into_json()</code> method to convert <code>DeviceUpdateRow</code> to JSON representation <li> Includes fields for device metadata, timestamps, and discovery information</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-57c82b01f92e9bd40063d0b0178c12d452771ac133f2121fb0ac008b167da367">+36/-0</a>    </td> </tr> <tr> <td> <details> <summary>parser.rs<dd><code>Add DeviceUpdates entity type to parser</code>                                    </dd></summary> <hr> rust/srql/src/parser.rs <ul><li>Added <code>DeviceUpdates</code> variant to <code>Entity</code> enum <li> Extended entity name parsing to recognize "device_updates", "device_update", and "updates" aliases</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-b2edf55d1721185349ecddb2f4eacc42e0dfcae19b6c2bc638602f187da67e66">+2/-0</a>      </td> </tr> <tr> <td> <details> <summary>schema.rs<dd><code>Add device_updates table schema definition</code>                              </dd></summary> <hr> rust/srql/src/schema.rs <ul><li>Defined new <code>device_updates</code> Diesel table schema with composite primary key <li> Includes fields for device tracking, discovery source, and optional metadata</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-2fe4906f42b52f96b7bc2d3431885b996b6701cfb88416905ae130b472d536ea">+19/-0</a>    </td> </tr> </table></details></td></tr><tr><td>Configuration changes</td><td><details><summary>2 files</summary><table> <tr> <td> <details> <summary>config.go<dd><code>Add device retention configuration defaults</code>                            </dd></summary> <hr> pkg/core/config.go <ul><li>Added <code>defaultDeviceRetentionDays</code> constant set to 3 days <li> Added normalization logic to set default device retention if not configured <li> Ensures device retention defaults are applied during config initialization</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-a2dcdf3bac7ca59032d52f7a0635b5dbcfcb83a6f07cc45db8f57fba0cc33c30">+8/-3</a>      </td> </tr> <tr> <td> <details> <summary>serviceradar-config.yaml<dd><code>Add sync service gRPC checker configuration</code>                            </dd></summary> <hr> helm/serviceradar/files/serviceradar-config.yaml <ul><li>Added new gRPC checker configuration for <code>sync</code> service with 5-minute results interval <li> Extends existing checker configurations in the Helm values file</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-b8c8d2484103b11c396bc60d290c81df63c30a0f81103eceb5852a17e1d2b5e3">+5/-0</a>      </td> </tr> </table></details></td></tr><tr><td>Bug fix</td><td><details><summary>1 files</summary><table> <tr> <td> <details> <summary>stats_aggregator.go<dd><code>Simplify device counting logic to include all records</code>        </dd></summary> <hr> pkg/core/stats_aggregator.go <ul><li>Simplified <code>shouldCountRecord</code> function to count all non-nil records without filtering sweep-only devices <li> Changed logic to rely on database as source of truth for device filtering <li> Prevents legitimate discovered devices from being hidden in UI statistics</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-09304e5f27f54a3a8fa2b57cc6a5485b9f7074d7554babde62b51a75753205e0">+5/-7</a>      </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-11-25 03:08:38 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2017#issuecomment-3573602250
Original created: 2025-11-25T03:08:38Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

⚪

Data deletion risk

Description: Deleting from unified_devices solely by last_seen < now - retention without partitioning
or safety checks can allow an attacker or misconfiguration to prematurely purge active
devices if clocks or observed times are manipulated, leading to data loss and potential
denial of service.
cnpg_unified_devices.go [421-432]

Referred Code

	return 0, nil
}

result, err := db.pgPool.Exec(ctx,
	`DELETE FROM unified_devices WHERE last_seen < $1`,
	time.Now().UTC().Add(-retention),
)
if err != nil {
	return 0, fmt.Errorf("failed to cleanup stale unified devices: %w", err)
}

return result.RowsAffected(), nil

SSRF risk

Description: Falling back to use checker config 'Address' for request details may cause unintended
service endpoints to be queried if misconfigured, enabling SSRF-like behavior or probing
internal addresses through the agent.
server.go [1749-1761]

Referred Code

		checkSecurity = conf.Security
	}
	// Use the checker config's Address if the request doesn't provide details
	if details == "" && conf.Address != "" {
		details = conf.Address
		s.logger.Info().
			Str("service", req.ServiceName).
			Str("address", conf.Address).
			Msg("Using checker config address as details")
	}
}

check, err = s.registry.Get(ctx, req.ServiceType, req.ServiceName, details, checkSecurity)

Ticket Compliance

🟡

🎫 #2016

🟢	Fix counting/aggregation so legitimate discovered devices are not hidden in UI totals.
🔴	Ensure demo namespace agents use sweep.json generated by serviceradar-sync/faker integration via objectstore blob referenced in NATS KV, not the default local sweep.json.
🔴	Agent should respond with scan results derived from the KV/objectstore-based sweep.json so core/device registry processes them.
⚪	Make discovered devices from the faker-driven sweep visible in inventory and reflected accurately in UI stats and analytics counts.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

🟢

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit logs: New critical DB actions (upsert into unified_devices and cleanup deletions) do not emit
audit logs identifying actor/context, making reconstruction of actions difficult.

Referred Code

// Upsert into unified_devices (current state table)
// This maintains the source of truth for device inventory
discoverySources := []string{arbitrarySource}
batch.Queue(
	`INSERT INTO unified_devices (
		device_id,
		ip,
		poller_id,
		agent_id,
		hostname,
		mac,
		discovery_sources,
		is_available,
		first_seen,
		last_seen,
		metadata,
		updated_at
	) VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$9,$10::jsonb,NOW())
	ON CONFLICT (device_id) DO UPDATE SET
		ip = COALESCE(NULLIF(EXCLUDED.ip, ''), unified_devices.ip),
		poller_id = COALESCE(NULLIF(EXCLUDED.poller_id, ''), unified_devices.poller_id),


 ... (clipped 23 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Partial failure handling: Batch upsert and insert operations lack per-item error context and retry/compensation, and
cleanup deletion does not validate inputs like zero/negative retention beyond early return
paths.

Referred Code

// Upsert into unified_devices (current state table)
// This maintains the source of truth for device inventory
discoverySources := []string{arbitrarySource}
batch.Queue(
	`INSERT INTO unified_devices (
		device_id,
		ip,
		poller_id,
		agent_id,
		hostname,
		mac,
		discovery_sources,
		is_available,
		first_seen,
		last_seen,
		metadata,
		updated_at
	) VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$9,$10::jsonb,NOW())
	ON CONFLICT (device_id) DO UPDATE SET
		ip = COALESCE(NULLIF(EXCLUDED.ip, ''), unified_devices.ip),
		poller_id = COALESCE(NULLIF(EXCLUDED.poller_id, ''), unified_devices.poller_id),


 ... (clipped 23 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Log data sensitivity: Newly added logs include address and service identifiers which could expose sensitive
endpoint details if logs are externally visible; absence of redaction is unclear.

Referred Code

	checkSecurity = conf.Security
}
// Use the checker config's Address if the request doesn't provide details
if details == "" && conf.Address != "" {
	details = conf.Address
	s.logger.Info().
		Str("service", req.ServiceName).
		Str("address", conf.Address).
		Msg("Using checker config address as details")
}

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Input validation: SRQL filters map directly into Diesel query builders; while parameterized, field
validation is limited to a whitelist and error messages may echo user fields, needing
confirmation no raw SQL or sensitive data exposure occurs.

Referred Code

other => {
    return Err(ServiceError::InvalidRequest(format!(
        "unsupported filter field for device_updates: '{other}'"
    )));
}

Learn more about managing compliance generic rules or creating your own custom rules

Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2017#issuecomment-3573602250 Original created: 2025-11-25T03:08:38Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR: <table><tbody><tr><td colspan='2'>Security Compliance</td></tr> <tr><td rowspan=2>⚪</td> <td><details><summary>Data deletion risk </summary> Description: Deleting from unified_devices solely by last_seen < now - retention without partitioning or safety checks can allow an attacker or misconfiguration to prematurely purge active devices if clocks or observed times are manipulated, leading to data loss and potential denial of service. <a href='https://github.com/carverauto/serviceradar/pull/2017/files#diff-ab3bf557cf9bb1b281a315a73abee38748de1654941c2471542e5e9bfc1716d8R421-R432'>cnpg_unified_devices.go [421-432]</a> <details open><summary>Referred Code</summary> ```go return 0, nil } result, err := db.pgPool.Exec(ctx, `DELETE FROM unified_devices WHERE last_seen < $1`, time.Now().UTC().Add(-retention), ) if err != nil { return 0, fmt.Errorf("failed to cleanup stale unified devices: %w", err) } return result.RowsAffected(), nil ``` </details></details></td></tr> <tr><td><details><summary>SSRF risk </summary> Description: Falling back to use checker config 'Address' for request details may cause unintended service endpoints to be queried if misconfigured, enabling SSRF-like behavior or probing internal addresses through the agent. <a href='https://github.com/carverauto/serviceradar/pull/2017/files#diff-3c51e5356a25859a3300ab62ed2494462feb2567ef69e6db3aa2bbc1032c1c5dR1749-R1761'>server.go [1749-1761]</a> <details open><summary>Referred Code</summary> ```go checkSecurity = conf.Security } // Use the checker config's Address if the request doesn't provide details if details == "" && conf.Address != "" { details = conf.Address s.logger.Info(). Str("service", req.ServiceName). Str("address", conf.Address). Msg("Using checker config address as details") } } check, err = s.registry.Get(ctx, req.ServiceType, req.ServiceName, details, checkSecurity) ``` </details></details></td></tr> <tr><td colspan='2'>Ticket Compliance</td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2016>#2016</a></summary> <table width='100%'><tbody> <tr><td rowspan=1>🟢</td> <td>Fix counting/aggregation so legitimate discovered devices are not hidden in UI totals.</td></tr> <tr><td rowspan=2>🔴</td> <td>Ensure demo namespace agents use sweep.json generated by serviceradar-sync/faker integration via objectstore blob referenced in NATS KV, not the default local sweep.json.</td></tr> <tr><td>Agent should respond with scan results derived from the KV/objectstore-based sweep.json so core/device registry processes them.</td></tr> <tr><td rowspan=1>⚪</td> <td>Make discovered devices from the faker-driven sweep visible in inventory and reflected accurately in UI stats and analytics counts.</td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'>Codebase Duplication Compliance</td></tr> <tr><td>⚪</td><td><details><summary>Codebase context is not defined </summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'>Custom Compliance</td></tr> <tr><td rowspan=2>🟢</td><td> <details><summary>Generic: Meaningful Naming and Self-Documenting Code</summary> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code self-documenting **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=4>⚪</td> <td><details> <summary>Generic: Comprehensive Audit Trails</summary> **Objective:** To create a detailed and reliable record of critical system actions for security analysis and compliance. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2017/files#diff-ab3bf557cf9bb1b281a315a73abee38748de1654941c2471542e5e9bfc1716d8R121-R164'>Missing audit logs</a>: New critical DB actions (upsert into <code>unified_devices</code> and cleanup deletions) do not emit audit logs identifying actor/context, making reconstruction of actions difficult. <details open><summary>Referred Code</summary> ```go // Upsert into unified_devices (current state table) // This maintains the source of truth for device inventory discoverySources := []string{arbitrarySource} batch.Queue( `INSERT INTO unified_devices ( device_id, ip, poller_id, agent_id, hostname, mac, discovery_sources, is_available, first_seen, last_seen, metadata, updated_at ) VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$9,$10::jsonb,NOW()) ON CONFLICT (device_id) DO UPDATE SET ip = COALESCE(NULLIF(EXCLUDED.ip, ''), unified_devices.ip), poller_id = COALESCE(NULLIF(EXCLUDED.poller_id, ''), unified_devices.poller_id), ... (clipped 23 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2017/files#diff-ab3bf557cf9bb1b281a315a73abee38748de1654941c2471542e5e9bfc1716d8R121-R164'>Partial failure handling</a>: Batch upsert and insert operations lack per-item error context and retry/compensation, and cleanup deletion does not validate inputs like zero/negative retention beyond early return paths. <details open><summary>Referred Code</summary> ```go // Upsert into unified_devices (current state table) // This maintains the source of truth for device inventory discoverySources := []string{arbitrarySource} batch.Queue( `INSERT INTO unified_devices ( device_id, ip, poller_id, agent_id, hostname, mac, discovery_sources, is_available, first_seen, last_seen, metadata, updated_at ) VALUES ($1,$2,$3,$4,$5,$6,$7,$8,$9,$9,$10::jsonb,NOW()) ON CONFLICT (device_id) DO UPDATE SET ip = COALESCE(NULLIF(EXCLUDED.ip, ''), unified_devices.ip), poller_id = COALESCE(NULLIF(EXCLUDED.poller_id, ''), unified_devices.poller_id), ... (clipped 23 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Secure Logging Practices</summary> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2017/files#diff-3c51e5356a25859a3300ab62ed2494462feb2567ef69e6db3aa2bbc1032c1c5dR1749-R1758'>Log data sensitivity</a>: Newly added logs include <code>address</code> and service identifiers which could expose sensitive endpoint details if logs are externally visible; absence of redaction is unclear. <details open><summary>Referred Code</summary> ```go checkSecurity = conf.Security } // Use the checker config's Address if the request doesn't provide details if details == "" && conf.Address != "" { details = conf.Address s.logger.Info(). Str("service", req.ServiceName). Str("address", conf.Address). Msg("Using checker config address as details") } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Security-First Input Validation and Data Handling</summary> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2017/files#diff-77b99f0ef409a6a3e4ecc65800ae841231a82b27a64e8dc1d4667db6698a539dR120-R124'>Input validation</a>: SRQL filters map directly into Diesel query builders; while parameterized, field validation is limited to a whitelist and error messages may echo user fields, needing confirmation no raw SQL or sensitive data exposure occurs. <details open><summary>Referred Code</summary> ```rust other => { return Err(ServiceError::InvalidRequest(format!( "unsupported filter field for device_updates: '{other}'" ))); } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant 🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label </details>

qodo-code-review[bot] commented

2025-11-25 03:09:39 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2017#issuecomment-3573604090
Original created: 2025-11-25T03:09:39Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Consider a simpler device management design The suggestion proposes simplifying the device management design by removing the dual-write pattern. Instead of writing to both `device_updates` and `unified_devices`, it recommends using only the `device_updates` hypertable and deriving the current state with a materialized view or a `DISTINCT ON` query. Examples: pkg/db/cnpg_unified_devices.go [94-164] batch.Queue( `INSERT INTO device_updates ( observed_at, agent_id, poller_id, partition, device_id, discovery_source, ip, mac, ... (clipped 61 lines) pkg/core/server.go [514-522] `// Cleanup stale unified devices if s.DB != nil { deleted, err := s.DB.CleanupStaleUnifiedDevices(ctx, unifiedDevicesRetention) if err != nil { s.logger.Error().Err(err).Msg("Failed to cleanup stale unified devices") } else if deleted > 0 { s.logger.Info().Int64("deleted_count", deleted).Msg("Cleaned up stale unified devices") } }` Solution Walkthrough: Before: // pkg/db/cnpg_unified_devices.go func cnpgInsertDeviceUpdates(ctx, updates) { batch = new batch() for update in updates { // 1. Insert into history table batch.Queue("INSERT INTO device_updates (...) VALUES (...)") // 2. Upsert into current state table batch.Queue( `INSERT INTO unified_devices (...) VALUES (...) ON CONFLICT (device_id) DO UPDATE SET ...` ) } db.SendBatch(ctx, batch) } // pkg/core/server.go func runMetricsCleanup(ctx) { // Periodically clean up the current state table db.CleanupStaleUnifiedDevices(ctx, retention) } After: // pkg/db/cnpg_unified_devices.go func cnpgInsertDeviceUpdates(ctx, updates) { batch = new batch() for update in updates { // Only insert into the history hypertable batch.Queue("INSERT INTO device_updates (...) VALUES (...)") } db.SendBatch(ctx, batch) } /* -- To get current device state, use a query or materialized view -- No separate `unified_devices` table or Go cleanup logic is needed. CREATE MATERIALIZED VIEW unified_devices_latest AS SELECT DISTINCT ON (device_id) * FROM device_updates ORDER BY device_id, observed_at DESC; */ Suggestion importance[1-10]: 9 __ Why: This is a high-impact architectural suggestion that correctly identifies the complexity of the dual-write pattern and proposes a simpler, more robust alternative using standard database features, which could significantly reduce code complexity and potential data inconsistencies.	High
General	Remove redundant default value logic Remove the redundant conditional block that sets a default value for `deviceRetentionDays`. pkg/core/server.go [492-497] `// Get device retention from config (defaults to 3 days in normalizeConfig) deviceRetentionDays := s.config.Metrics.DeviceRetentionDays - if deviceRetentionDays <= 0 { - deviceRetentionDays = defaultDeviceRetentionDays - } unifiedDevicesRetention := time.Duration(deviceRetentionDays) * 24 * time.Hour` Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies redundant logic for setting a default value that is already handled in `normalizeConfig`, and removing it improves code maintainability by ensuring a single source of truth.	Low
General	Refactor ordering logic to reduce duplication Refactor the `apply_ordering` function to remove duplicated logic for applying the first and subsequent ordering clauses by using an indexed loop. rust/srql/src/query/device_updates.rs [130-169] fn apply_ordering<'a>( mut query: DeviceUpdatesQuery<'a>, order: &[OrderClause], ) -> DeviceUpdatesQuery<'a> { - let mut applied = false; - for clause in order { - query = if !applied { - applied = true; - match clause.field.as_str() { - "observed_at" \| "timestamp" => match clause.direction { - OrderDirection::Asc => query.order(col_observed_at.asc()), - OrderDirection::Desc => query.order(col_observed_at.desc()), - }, - "device_id" => match clause.direction { - OrderDirection::Asc => query.order(col_device_id.asc()), - OrderDirection::Desc => query.order(col_device_id.desc()), - }, - _ => query, - } - } else { - match clause.field.as_str() { - "observed_at" \| "timestamp" => match clause.direction { - OrderDirection::Asc => query.then_order_by(col_observed_at.asc()), - OrderDirection::Desc => query.then_order_by(col_observed_at.desc()), - }, - "device_id" => match clause.direction { - OrderDirection::Asc => query.then_order_by(col_device_id.asc()), - OrderDirection::Desc => query.then_order_by(col_device_id.desc()), - }, - _ => query, - } - }; + if order.is_empty() { + return query.order(col_observed_at.desc()); } - if !applied { - query = query.order(col_observed_at.desc()); + for (i, clause) in order.iter().enumerate() { + let is_first = i == 0; + query = match clause.field.as_str() { + "observed_at" \| "timestamp" => match clause.direction { + OrderDirection::Asc if is_first => query.order(col_observed_at.asc()), + OrderDirection::Desc if is_first => query.order(col_observed_at.desc()), + OrderDirection::Asc => query.then_order_by(col_observed_at.asc()), + OrderDirection::Desc => query.then_order_by(col_observed_at.desc()), + }, + "device_id" => match clause.direction { + OrderDirection::Asc if is_first => query.order(col_device_id.asc()), + OrderDirection::Desc if is_first => query.order(col_device_id.desc()), + OrderDirection::Asc => query.then_order_by(col_device_id.asc()), + OrderDirection::Desc => query.then_order_by(col_device_id.desc()), + }, + _ => query, + }; } query } Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies code duplication and proposes a valid refactoring that improves maintainability, although the proposed implementation is still somewhat verbose.	Low
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2017#issuecomment-3573604090 Original created: 2025-11-25T03:09:39Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td>Category</td><td align=left>Suggestion                                                                                                                                    </td><td align=center>Impact</td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>Consider a simpler device management design</summary> ___ **The suggestion proposes simplifying the device management design by removing the dual-write pattern. Instead of writing to both <code>device_updates</code> and <code>unified_devices</code>, it recommends using only the <code>device_updates</code> hypertable and deriving the current state with a materialized view or a <code>DISTINCT ON</code> query.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-ab3bf557cf9bb1b281a315a73abee38748de1654941c2471542e5e9bfc1716d8R94-R164">pkg/db/cnpg_unified_devices.go [94-164]</a> </summary> ```go batch.Queue( `INSERT INTO device_updates ( observed_at, agent_id, poller_id, partition, device_id, discovery_source, ip, mac, ... (clipped 61 lines) ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2017/files#diff-a12d34beb8640efbbedad28f8610fd58afeea457572d82e4fd295ab2a6bb8ee6R514-R522">pkg/core/server.go [514-522]</a> </summary> ```go // Cleanup stale unified devices if s.DB != nil { deleted, err := s.DB.CleanupStaleUnifiedDevices(ctx, unifiedDevicesRetention) if err != nil { s.logger.Error().Err(err).Msg("Failed to cleanup stale unified devices") } else if deleted > 0 { s.logger.Info().Int64("deleted_count", deleted).Msg("Cleaned up stale unified devices") } } ``` </details> ### Solution Walkthrough: #### Before: ```go // pkg/db/cnpg_unified_devices.go func cnpgInsertDeviceUpdates(ctx, updates) { batch = new batch() for update in updates { // 1. Insert into history table batch.Queue("INSERT INTO device_updates (...) VALUES (...)") // 2. Upsert into current state table batch.Queue( `INSERT INTO unified_devices (...) VALUES (...) ON CONFLICT (device_id) DO UPDATE SET ...` ) } db.SendBatch(ctx, batch) } // pkg/core/server.go func runMetricsCleanup(ctx) { // Periodically clean up the current state table db.CleanupStaleUnifiedDevices(ctx, retention) } ``` #### After: ```go // pkg/db/cnpg_unified_devices.go func cnpgInsertDeviceUpdates(ctx, updates) { batch = new batch() for update in updates { // Only insert into the history hypertable batch.Queue("INSERT INTO device_updates (...) VALUES (...)") } db.SendBatch(ctx, batch) } /* -- To get current device state, use a query or materialized view -- No separate `unified_devices` table or Go cleanup logic is needed. CREATE MATERIALIZED VIEW unified_devices_latest AS SELECT DISTINCT ON (device_id) * FROM device_updates ORDER BY device_id, observed_at DESC; */ ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: This is a high-impact architectural suggestion that correctly identifies the complexity of the dual-write pattern and proposes a simpler, more robust alternative using standard database features, which could significantly reduce code complexity and potential data inconsistencies. </details></details></td><td align=center>High </td></tr><tr><td rowspan=2>General</td> <td> <details><summary>Remove redundant default value logic</summary> ___ **Remove the redundant conditional block that sets a default value for <code>deviceRetentionDays</code>.** [pkg/core/server.go [492-497]](https://github.com/carverauto/serviceradar/pull/2017/files#diff-a12d34beb8640efbbedad28f8610fd58afeea457572d82e4fd295ab2a6bb8ee6R492-R497) ```diff // Get device retention from config (defaults to 3 days in normalizeConfig) deviceRetentionDays := s.config.Metrics.DeviceRetentionDays - if deviceRetentionDays <= 0 { - deviceRetentionDays = defaultDeviceRetentionDays - } unifiedDevicesRetention := time.Duration(deviceRetentionDays) * 24 * time.Hour ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly identifies redundant logic for setting a default value that is already handled in `normalizeConfig`, and removing it improves code maintainability by ensuring a single source of truth. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>Refactor ordering logic to reduce duplication</summary> ___ **Refactor the <code>apply_ordering</code> function to remove duplicated logic for applying the first and subsequent ordering clauses by using an indexed loop.** [rust/srql/src/query/device_updates.rs [130-169]](https://github.com/carverauto/serviceradar/pull/2017/files#diff-77b99f0ef409a6a3e4ecc65800ae841231a82b27a64e8dc1d4667db6698a539dR130-R169) ```diff fn apply_ordering<'a>( mut query: DeviceUpdatesQuery<'a>, order: &[OrderClause], ) -> DeviceUpdatesQuery<'a> { - let mut applied = false; - for clause in order { - query = if !applied { - applied = true; - match clause.field.as_str() { - "observed_at" | "timestamp" => match clause.direction { - OrderDirection::Asc => query.order(col_observed_at.asc()), - OrderDirection::Desc => query.order(col_observed_at.desc()), - }, - "device_id" => match clause.direction { - OrderDirection::Asc => query.order(col_device_id.asc()), - OrderDirection::Desc => query.order(col_device_id.desc()), - }, - _ => query, - } - } else { - match clause.field.as_str() { - "observed_at" | "timestamp" => match clause.direction { - OrderDirection::Asc => query.then_order_by(col_observed_at.asc()), - OrderDirection::Desc => query.then_order_by(col_observed_at.desc()), - }, - "device_id" => match clause.direction { - OrderDirection::Asc => query.then_order_by(col_device_id.asc()), - OrderDirection::Desc => query.then_order_by(col_device_id.desc()), - }, - _ => query, - } - }; + if order.is_empty() { + return query.order(col_observed_at.desc()); } - if !applied { - query = query.order(col_observed_at.desc()); + for (i, clause) in order.iter().enumerate() { + let is_first = i == 0; + query = match clause.field.as_str() { + "observed_at" | "timestamp" => match clause.direction { + OrderDirection::Asc if is_first => query.order(col_observed_at.asc()), + OrderDirection::Desc if is_first => query.order(col_observed_at.desc()), + OrderDirection::Asc => query.then_order_by(col_observed_at.asc()), + OrderDirection::Desc => query.then_order_by(col_observed_at.desc()), + }, + "device_id" => match clause.direction { + OrderDirection::Asc if is_first => query.order(col_device_id.asc()), + OrderDirection::Desc if is_first => query.order(col_device_id.desc()), + OrderDirection::Asc => query.then_order_by(col_device_id.asc()), + OrderDirection::Desc => query.then_order_by(col_device_id.desc()), + }, + _ => query, + }; } query } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly identifies code duplication and proposes a valid refactoring that improves maintainability, although the proposed implementation is still somewhat verbose. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>