carverauto/serviceradar

Fork 0

2100 bugcore docker agent showing up as docker poller #2541

Merged

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

2025-12-11 03:59:40 +00:00

mfreeman451 commented

2025-12-11 02:40:31 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2101
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2101
Original created: 2025-12-11T02:40:31Z
Original updated: 2025-12-11T03:59:43Z
Original head: carverauto/serviceradar:2100-bugcore-docker-agent-showing-up-as-docker-poller
Original base: main
Original merged: 2025-12-11T03:59:40Z 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

Fix device details showing wrong device in Docker environments with shared IPs
- Prevent IP-based fallback for device ID lookups containing colons
- Add IP vs device ID detection helpers to distinguish lookup types
- Update API handler to fall back to database instead of IP lookup
Improve SRQL configuration with sensible defaults and environment variable support
- Add default base URL, path, timeout, and API key configuration
- Support SRQL_BASE_URL and NEXT_INTERNAL_SRQL_URL environment variables
- Ensure SRQL is enabled by default in Docker compose setup
Enhance search planner to handle device_id filters when SRQL unavailable
- Extract device_id from query using regex pattern matching
- Perform direct registry lookup for exact device ID matches
- Return empty results gracefully when device not found

Diagram Walkthrough

flowchart LR
  A["Device Lookup Request"] --> B{Input Type?}
  B -->|Device ID with colon| C["Exact Registry Lookup"]
  B -->|IP Address| D["IP-based Registry Lookup"]
  C -->|Not Found| E["Fall back to Database"]
  D -->|Multiple Devices| F["Return First Device + Warning"]
  E -->|Found| G["Return Device"]
  E -->|Not Found| H["Return 404"]
  D -->|Not Found| H

File Walkthrough

Relevant files

Bug fix

2 files

server.go `Implement database fallback for device registry misses`	+15/-8
registry.go `Add IP detection and prevent device ID IP fallback`	+61/-1

Enhancement

2 files

config.go `Add SRQL configuration defaults and environment variable support`	+40/-0
planner.go `Extract and handle device_id filters in registry search`	+35/-0

Tests

3 files

config_defaults_test.go `Test SRQL default configuration normalization`	+20/-0
registry_test.go `Test IP detection and device ID lookup behavior`	+157/-0
planner_test.go `Test device_id filter extraction and registry execution`	+80/-0

Configuration changes

2 files

update-config.sh `Ensure SRQL configuration in Docker compose setup`	+16/-0
core.docker.json `Add SRQL configuration to Docker default config`	+6/-0

Dependencies

1 files

BUILD.bazel
Add registry dependency to search package +1/-0

Documentation

4 files

design.md `Document design decisions for device lookup fix`	+110/-0
proposal.md `Document problem analysis and proposed solution`	+70/-0
spec.md `Define requirements for device ID and IP lookup behavior`	+39/-0
tasks.md `Track implementation tasks and testing requirements`	+77/-0

Imported from GitHub pull request. Original GitHub pull request: #2101 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2101 Original created: 2025-12-11T02:40:31Z Original updated: 2025-12-11T03:59:43Z Original head: carverauto/serviceradar:2100-bugcore-docker-agent-showing-up-as-docker-poller Original base: main Original merged: 2025-12-11T03:59:40Z 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** - Fix device details showing wrong device in Docker environments with shared IPs - Prevent IP-based fallback for device ID lookups containing colons - Add IP vs device ID detection helpers to distinguish lookup types - Update API handler to fall back to database instead of IP lookup - Improve SRQL configuration with sensible defaults and environment variable support - Add default base URL, path, timeout, and API key configuration - Support SRQL_BASE_URL and NEXT_INTERNAL_SRQL_URL environment variables - Ensure SRQL is enabled by default in Docker compose setup - Enhance search planner to handle device_id filters when SRQL unavailable - Extract device_id from query using regex pattern matching - Perform direct registry lookup for exact device ID matches - Return empty results gracefully when device not found ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Device Lookup Request"] --> B{Input Type?} B -->|Device ID with colon| C["Exact Registry Lookup"] B -->|IP Address| D["IP-based Registry Lookup"] C -->|Not Found| E["Fall back to Database"] D -->|Multiple Devices| F["Return First Device + Warning"] E -->|Found| G["Return Device"] E -->|Not Found| H["Return 404"] D -->|Not Found| H ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><details><summary>2 files</summary><table> <tr> <td><strong>server.go</strong><dd><code>Implement database fallback for device registry misses</code>      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021e">+15/-8</a>    </td> </tr> <tr> <td><strong>registry.go</strong><dd><code>Add IP detection and prevent device ID IP fallback</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ec">+61/-1</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Enhancement</strong></td><td><details><summary>2 files</summary><table> <tr> <td><strong>config.go</strong><dd><code>Add SRQL configuration defaults and environment variable support</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-a2dcdf3bac7ca59032d52f7a0635b5dbcfcb83a6f07cc45db8f57fba0cc33c30">+40/-0</a>    </td> </tr> <tr> <td><strong>planner.go</strong><dd><code>Extract and handle device_id filters in registry search</code>    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-e10a578c6efce8ae42e5d451f1b3bb978fd6c1e1dec254a21cdfab89685d2970">+35/-0</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>3 files</summary><table> <tr> <td><strong>config_defaults_test.go</strong><dd><code>Test SRQL default configuration normalization</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-da36d332e7ed42f5ef6bda1ce6806c69f902e644e17f9f6d78c3c1673e331d51">+20/-0</a>    </td> </tr> <tr> <td><strong>registry_test.go</strong><dd><code>Test IP detection and device ID lookup behavior</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-f010972d104404be52d2a8e6e784cb56e31194f90795a69571a12696bcbdc075">+157/-0</a>  </td> </tr> <tr> <td><strong>planner_test.go</strong><dd><code>Test device_id filter extraction and registry execution</code>    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-5e568fe0ba96be717ebc8337367ce66741d0d8f144f06f07b1a5ec002928d324">+80/-0</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Configuration changes</strong></td><td><details><summary>2 files</summary><table> <tr> <td><strong>update-config.sh</strong><dd><code>Ensure SRQL configuration in Docker compose setup</code>                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-9ae50be83a13010a038389c74407ba1bde8cabcea0944e238c4b3374133f78bf">+16/-0</a>    </td> </tr> <tr> <td><strong>core.docker.json</strong><dd><code>Add SRQL configuration to Docker default config</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-e8daaf647c9f7582595681307b2d56a0b0436bebb8e9112d9c894cacb3347a1f">+6/-0</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Dependencies</strong></td><td><details><summary>1 files</summary><table> <tr> <td><strong>BUILD.bazel</strong><dd><code>Add registry dependency to search package</code>                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-21c0fe512f70c4e838fd614612e1d59da6a8c235798a645fb69f578cf703994e">+1/-0</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Documentation</strong></td><td><details><summary>4 files</summary><table> <tr> <td><strong>design.md</strong><dd><code>Document design decisions for device lookup fix</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-ca352772fb9768d93ddd8d8c499698f07590f2065b73865903c041096dd0e61f">+110/-0</a>  </td> </tr> <tr> <td><strong>proposal.md</strong><dd><code>Document problem analysis and proposed solution</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-b5cffb01463e6fa96e05a3d900846771a2057f7324999836f74fe1350df9b7e7">+70/-0</a>    </td> </tr> <tr> <td><strong>spec.md</strong><dd><code>Define requirements for device ID and IP lookup behavior</code>  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-7daeb203b53784f13a5d0f413c75b01e21979d4e2dbab004a81dfb94f449da49">+39/-0</a>    </td> </tr> <tr> <td><strong>tasks.md</strong><dd><code>Track implementation tasks and testing requirements</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-64afad5a1a6b6937b8fcff6e2984c104f7083e3d12c79cb5a6a0eb9418bf4afe">+77/-0</a>    </td> </tr> </table></details></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-11 02:41:05 +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/2101#issuecomment-3639802201
Original created: 2025-12-11T02:41:05Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

⚪

Insecure configuration

Description: SRQL API key is sourced from environment variable without validation or restriction,
risking accidental enablement of external SRQL access if misconfigured; ensure SRQL is
internal-only and that API key usage and transport are secured.
config.go [126-151]

Referred Code

normalized.Enabled = true

if strings.TrimSpace(normalized.BaseURL) == "" {
	switch {
	case strings.TrimSpace(os.Getenv("SRQL_BASE_URL")) != "":
		normalized.BaseURL = strings.TrimSpace(os.Getenv("SRQL_BASE_URL"))
	case strings.TrimSpace(os.Getenv("NEXT_INTERNAL_SRQL_URL")) != "":
		normalized.BaseURL = strings.TrimSpace(os.Getenv("NEXT_INTERNAL_SRQL_URL"))
	default:
		normalized.BaseURL = defaultSRQLBaseURL
	}
}

if normalized.Timeout == 0 {
	normalized.Timeout = models.Duration(defaultSRQLTimeout)
}

if strings.TrimSpace(normalized.Path) == "" {
	normalized.Path = defaultSRQLPath
}



 ... (clipped 5 lines)

Sensitive information exposure

Description: Script writes SRQL base URL and API key into core.json using jq, which may commit
sensitive API keys to disk in plaintext; consider file permission hardening and avoiding
persisting secrets in config files.
update-config.sh [122-135]

Referred Code

if [ -f "$CORE_CONFIG" ]; then
    SRQL_BASE_URL="${SRQL_BASE_URL:-http://srql:8080}"
    jq --arg base "$SRQL_BASE_URL" \
       --arg api_key "${API_KEY:-}" \
       '.srql = (.srql // {}) |
        .srql.enabled = true |
        .srql.base_url = ($base // "http://srql:8080") |
        .srql.timeout = (.srql.timeout // "15s") |
        .srql.path = (.srql.path // "/api/query") |
        (if $api_key != "" then .srql.api_key = $api_key else . end)' \
       "$CORE_CONFIG" > "$CORE_CONFIG.tmp"
    mv "$CORE_CONFIG.tmp" "$CORE_CONFIG"
    echo "✅ Ensured SRQL config in core.json (base_url=$SRQL_BASE_URL)"
fi

Ticket Compliance

🟡

🎫 #2100

🟢	Ensure docker-agent device details do not incorrectly show docker-poller information in Docker environments.
🟢	Review and correct queries/lookup logic that conflate device ID and IP, particularly when multiple devices share an IP in Docker.
⚪	Maintain correct behavior in K8s where k8s-agent already shows the right device.

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: Robust Error Handling and Edge Case Management

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

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: Secure Logging Practices

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

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:
Logging Context: New error paths and fallbacks were added but there is no clear evidence in the diff that
these critical lookup outcomes are consistently logged with user/request context to form
an audit trail.

Referred Code

// If device not found in registry, fall back to database lookup (don't return 404 yet).
// This handles the case where the device exists in DB but isn't in the in-memory registry
// (e.g., due to timing/hydration issues in Docker environments).
switch {
case errors.Is(err, registry.ErrDeviceNotFound):
	s.logger.Debug().
		Str("device_id", deviceID).
		Msg("Device not found in registry cache, falling back to database lookup")
	// Fall through to database lookup below
case s.requireDeviceRegistry:
	s.logger.Error().
		Err(err).
		Str("device_id", deviceID).
		Msg("Device registry lookup failed and legacy fallback disabled")
	writeError(w, "Device registry unavailable", http.StatusServiceUnavailable)
	return
default:
	s.logger.Warn().
		Err(err).
		Str("device_id", deviceID).
		Msg("Device registry lookup failed, falling back to legacy")


 ... (clipped 1 lines)

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:
Env Input Use: New SRQL config reads environment variables directly into configuration without visible
validation or normalization beyond trimming, which may require further review for security
and correctness.

Referred Code


// WithGraphWriter wires an optional graph writer (e.g., AGE) for device relationship ingestion.
func WithGraphWriter(writer GraphWriter) Option {
	return func(r *DeviceRegistry) {
		r.graphWriter = writer
	}
}

// WithIdentityReconciliationConfig wires identity reconciliation feature gates and defaults.
func WithIdentityReconciliationConfig(cfg *models.IdentityReconciliationConfig) Option {
	return func(r *DeviceRegistry) {
		r.identityCfg = cfg
	}
}

// WithReconcileInterval configures how often background reconciliation should run.
func WithReconcileInterval(interval time.Duration) Option {
	return func(r *DeviceRegistry) {
		if interval > 0 {
			r.reconcileInterval = interval
		}


 ... (clipped 14 lines)

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/2101#issuecomment-3639802201 Original created: 2025-12-11T02:41:05Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=2>⚪</td> <td><details><summary><strong>Insecure configuration </strong></summary><br> <b>Description:</b> SRQL API key is sourced from environment variable without validation or restriction, <br>risking accidental enablement of external SRQL access if misconfigured; ensure SRQL is <br>internal-only and that API key usage and transport are secured.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2101/files#diff-a2dcdf3bac7ca59032d52f7a0635b5dbcfcb83a6f07cc45db8f57fba0cc33c30R126-R151'>config.go [126-151]</a></strong><br> <details open><summary>Referred Code</summary> ```go normalized.Enabled = true if strings.TrimSpace(normalized.BaseURL) == "" { switch { case strings.TrimSpace(os.Getenv("SRQL_BASE_URL")) != "": normalized.BaseURL = strings.TrimSpace(os.Getenv("SRQL_BASE_URL")) case strings.TrimSpace(os.Getenv("NEXT_INTERNAL_SRQL_URL")) != "": normalized.BaseURL = strings.TrimSpace(os.Getenv("NEXT_INTERNAL_SRQL_URL")) default: normalized.BaseURL = defaultSRQLBaseURL } } if normalized.Timeout == 0 { normalized.Timeout = models.Duration(defaultSRQLTimeout) } if strings.TrimSpace(normalized.Path) == "" { normalized.Path = defaultSRQLPath } ... (clipped 5 lines) ``` </details></details></td></tr> <tr><td><details><summary><strong>Sensitive information exposure </strong></summary><br> <b>Description:</b> Script writes SRQL base URL and API key into core.json using jq, which may commit <br>sensitive API keys to disk in plaintext; consider file permission hardening and avoiding <br>persisting secrets in config files.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2101/files#diff-9ae50be83a13010a038389c74407ba1bde8cabcea0944e238c4b3374133f78bfR122-R135'>update-config.sh [122-135]</a></strong><br> <details open><summary>Referred Code</summary> ```shell if [ -f "$CORE_CONFIG" ]; then SRQL_BASE_URL="${SRQL_BASE_URL:-http://srql:8080}" jq --arg base "$SRQL_BASE_URL" \ --arg api_key "${API_KEY:-}" \ '.srql = (.srql // {}) | .srql.enabled = true | .srql.base_url = ($base // "http://srql:8080") | .srql.timeout = (.srql.timeout // "15s") | .srql.path = (.srql.path // "/api/query") | (if $api_key != "" then .srql.api_key = $api_key else . end)' \ "$CORE_CONFIG" > "$CORE_CONFIG.tmp" mv "$CORE_CONFIG.tmp" "$CORE_CONFIG" echo "✅ Ensured SRQL config in core.json (base_url=$SRQL_BASE_URL)" fi ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2100>#2100</a></summary> <table width='100%'><tbody> <tr><td rowspan=2>🟢</td> <td>Ensure docker-agent device details do not incorrectly show docker-poller information in <br>Docker environments.</td></tr> <tr><td>Review and correct queries/lookup logic that conflate device ID and IP, particularly when <br>multiple devices share an IP in Docker.</td></tr> <tr><td rowspan=1>⚪</td> <td>Maintain correct behavior in K8s where k8s-agent already shows the right device.</td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></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'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=4>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > 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><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** Passed<br> > 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><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> > 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><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > 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=2>⚪</td> <td><details> <summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2101/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR2374-R2395'><strong>Logging Context</strong></a>: New error paths and fallbacks were added but there is no clear evidence in the diff that <br>these critical lookup outcomes are consistently logged with user/request context to form <br>an audit trail.<br> <details open><summary>Referred Code</summary> ```go // If device not found in registry, fall back to database lookup (don't return 404 yet). // This handles the case where the device exists in DB but isn't in the in-memory registry // (e.g., due to timing/hydration issues in Docker environments). switch { case errors.Is(err, registry.ErrDeviceNotFound): s.logger.Debug(). Str("device_id", deviceID). Msg("Device not found in registry cache, falling back to database lookup") // Fall through to database lookup below case s.requireDeviceRegistry: s.logger.Error(). Err(err). Str("device_id", deviceID). Msg("Device registry lookup failed and legacy fallback disabled") writeError(w, "Device registry unavailable", http.StatusServiceUnavailable) return default: s.logger.Warn(). Err(err). Str("device_id", deviceID). Msg("Device registry lookup failed, falling back to legacy") ... (clipped 1 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><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2101/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR120-R154'><strong>Env Input Use</strong></a>: New SRQL config reads environment variables directly into configuration without visible <br>validation or normalization beyond trimming, which may require further review for security <br>and correctness.<br> <details open><summary>Referred Code</summary> ```go // WithGraphWriter wires an optional graph writer (e.g., AGE) for device relationship ingestion. func WithGraphWriter(writer GraphWriter) Option { return func(r *DeviceRegistry) { r.graphWriter = writer } } // WithIdentityReconciliationConfig wires identity reconciliation feature gates and defaults. func WithIdentityReconciliationConfig(cfg *models.IdentityReconciliationConfig) Option { return func(r *DeviceRegistry) { r.identityCfg = cfg } } // WithReconcileInterval configures how often background reconciliation should run. func WithReconcileInterval(interval time.Duration) Option { return func(r *DeviceRegistry) { if interval > 0 { r.reconcileInterval = interval } ... (clipped 14 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 align="center" colspan="2"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>

qodo-code-review[bot] commented

2025-12-11 02:42:19 +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/2101#issuecomment-3639804435
Original created: 2025-12-11T02:42:19Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	✅ ~~Prioritize architectural cleanup over patching~~ Suggestion Impact: The commit removes GetMergedDevice, adds a new GetDeviceByIDStrict wrapper, updates the tasks to mark Phase 2 items as completed (deprecation, auditing callers, migration), and retains GetDevicesByIP—aligning with the suggested architectural cleanup rather than patching. code diff: +func (r DeviceRegistry) GetDeviceByIDStrict(ctx context.Context, deviceID string) (models.UnifiedDevice, error) { + return r.GetDevice(ctx, deviceID) } func (r DeviceRegistry) GetDevicesByIP(ctx context.Context, ip string) ([]models.UnifiedDevice, error) { @@ -2032,82 +2040,6 @@ return UnifiedDeviceSlice(matchedRecords) } -// looksLikeIP returns true if the input string appears to be an IP address (IPv4 or IPv6). -// Device IDs like "serviceradar:agent:docker-agent" or "default:10.0.0.1" contain colons -// but are NOT valid IP addresses and should not be treated as such. -func looksLikeIP(input string) bool { - input = strings.TrimSpace(input) - if input == "" { - return false - } - - // Try parsing as IP address - this handles both IPv4 and IPv6 - ip := net.ParseIP(input) - return ip != nil -} - -// looksLikeDeviceID returns true if the input string appears to be a device ID rather than an IP. -// Device IDs contain colons in patterns that don't match valid IP addresses. -func looksLikeDeviceID(input string) bool { - input = strings.TrimSpace(input) - if input == "" { - return false - } - - // If it parses as a valid IP, it's not a device ID - if net.ParseIP(input) != nil { - return false - } - - // Device IDs typically contain colons (e.g., "serviceradar:agent:X", "default:10.0.0.1") - // or are ServiceRadar UUIDs (e.g., "sr:...") - return strings.Contains(input, ":") -} - -func (r DeviceRegistry) GetMergedDevice(ctx context.Context, deviceIDOrIP string) (models.UnifiedDevice, error) { - // First, try exact device ID lookup in the in-memory registry - if device, err := r.GetDevice(ctx, deviceIDOrIP); err == nil { - return device, nil - } - - // If the input looks like a device ID (not an IP), do NOT fall back to IP lookup. - // This prevents returning the wrong device when multiple devices share an IP (e.g., Docker). - if looksLikeDeviceID(deviceIDOrIP) { - if r.logger != nil { - r.logger.Debug(). - Str("device_id", deviceIDOrIP). - Msg("Device ID lookup failed in registry; not falling back to IP lookup for device ID format") - } - return nil, fmt.Errorf("%w: %s", ErrDeviceNotFound, deviceIDOrIP) - } - - // Only fall back to IP lookup if the input actually looks like an IP address - if !looksLikeIP(deviceIDOrIP) { - return nil, fmt.Errorf("%w: %s", ErrDeviceNotFound, deviceIDOrIP) - } - - devices, err := r.GetDevicesByIP(ctx, deviceIDOrIP) - if err != nil { - return nil, fmt.Errorf("failed to get device by IP %s: %w", deviceIDOrIP, err) - } - - if len(devices) == 0 { - return nil, fmt.Errorf("%w: %s", ErrDeviceNotFound, deviceIDOrIP) - } - - // Note: IP lookup returns first device at that IP. This is only valid when - // there's a single device at that IP, which is typically the case in K8s - // but NOT in Docker where multiple containers may share the host IP. - if r.logger != nil && len(devices) > 1 { - r.logger.Warn(). - Str("ip", deviceIDOrIP). - Int("device_count", len(devices)). - Msg("Multiple devices found at IP; returning first device - consider using device ID for exact lookup") - } - - return devices[0], nil -} Instead of patching the flawed `GetMergedDevice` function, proceed with the "Phase 2" architectural cleanup outlined in the design documents. Deprecate `GetMergedDevice` and refactor its callers to use explicit `GetDeviceByID` and `GetDeviceByIP` functions to avoid accumulating technical debt. Examples: pkg/registry/registry.go [2067-2109] func (r DeviceRegistry) GetMergedDevice(ctx context.Context, deviceIDOrIP string) (models.UnifiedDevice, error) { // First, try exact device ID lookup in the in-memory registry if device, err := r.GetDevice(ctx, deviceIDOrIP); err == nil { return device, nil } // If the input looks like a device ID (not an IP), do NOT fall back to IP lookup. // This prevents returning the wrong device when multiple devices share an IP (e.g., Docker). if looksLikeDeviceID(deviceIDOrIP) { if r.logger != nil { ... (clipped 33 lines) openspec/changes/fix-device-details-wrong-device/tasks.md [22-38] ## Phase 2: Clean Architecture (Optional) ### 2.1 Deprecate `GetMergedDevice` - [ ] Mark `GetMergedDevice` as deprecated - [ ] Audit all callers of `GetMergedDevice` - [ ] Create migration plan for each caller ### 2.2 Create Explicit Functions - [ ] Add `GetDeviceByIDStrict(deviceID string)` - exact match only, returns error if not found - [ ] Keep existing `GetDevicesByIP(ip string)` for legitimate IP lookups ... (clipped 7 lines) Solution Walkthrough: Before: // pkg/registry/registry.go func looksLikeDeviceID(input string) bool { // ... logic to check for colons and not be a valid IP } func GetMergedDevice(ctx, deviceIDOrIP string) (UnifiedDevice, error) { // 1. Try to get by device ID device, err := r.GetDevice(ctx, deviceIDOrIP) if err == nil { return device, nil } // 2. If it looks like a device ID, fail now (this is the patch) if looksLikeDeviceID(deviceIDOrIP) { return nil, ErrDeviceNotFound } // 3. Fallback to IP lookup for things that look like IPs devices, err := r.GetDevicesByIP(ctx, deviceIDOrIP) // ... return first device } After: // pkg/registry/registry.go // @deprecated Use GetDeviceByID or GetDevicesByIP. func (r DeviceRegistry) GetMergedDevice(...) { ... } // New explicit function for ID lookup func (r DeviceRegistry) GetDeviceByID(ctx, deviceID string) (UnifiedDevice, error) { return r.GetDevice(ctx, deviceID) // No IP fallback } // pkg/core/api/server.go func (s APIServer) getDevice(w http.ResponseWriter, r http.Request) { // ... var unifiedDevice *models.UnifiedDevice var err error if looksLikeIP(deviceID) { devices, err := s.deviceRegistry.GetDevicesByIP(ctx, deviceID) if len(devices) > 0 { unifiedDevice = devices[0] } } else { unifiedDevice, err = s.deviceRegistry.GetDeviceByID(ctx, deviceID) } // ... } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that the PR implements a patch rather than the ideal architectural cleanup described in its own design documents, which adds complexity to a function slated for deprecation.	Medium
Possible issue	Allow registry to handle device_id filters Modify `supportsRegistry` to allow `device_id` filters, enabling the newly added logic in `executeRegistry` to handle them via the in-memory registry instead of incorrectly routing to SRQL. pkg/search/planner.go [312-362] func (p *Planner) supportsRegistry(query string, filters map[string]string) bool { // SRQL is required for any query that isn't a simple search or known filter. // This is a simple heuristic; more complex queries will be routed to SRQL. if strings.Contains(query, "or") \|\| strings.Contains(query, "OR") { - return false - } - if strings.Contains(query, "device_id:") { - return false - } - if strings.Contains(query, "device_id =") { return false } // Check for unsupported filters for k := range filters { switch k { case "search", "status", "capability": // supported default: + // Allow device_id filters to be handled by the registry path + if extractDeviceIDFilter(query) != "" { + continue + } return false } } return true } `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that `supportsRegistry` prevents the new `device_id` filter logic in `executeRegistry` from being used, which is a significant bug in the PR's implementation.	Medium
Possible issue	Respect explicit SRQL disable configuration Update `ensureSRQLConfig` to respect the user's choice to disable SRQL in the configuration file, instead of unconditionally enabling it. pkg/core/config.go [120-137] func ensureSRQLConfig(cfg models.SRQLConfig) models.SRQLConfig { normalized := cfg if normalized == nil { - normalized = &models.SRQLConfig{} + normalized = &models.SRQLConfig{ + Enabled: true, + } } - - normalized.Enabled = true if strings.TrimSpace(normalized.BaseURL) == "" { switch { case strings.TrimSpace(os.Getenv("SRQL_BASE_URL")) != "": normalized.BaseURL = strings.TrimSpace(os.Getenv("SRQL_BASE_URL")) case strings.TrimSpace(os.Getenv("NEXT_INTERNAL_SRQL_URL")) != "": normalized.BaseURL = strings.TrimSpace(os.Getenv("NEXT_INTERNAL_SRQL_URL")) default: normalized.BaseURL = defaultSRQLBaseURL } } ... `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that the current implementation prevents users from disabling the SRQL feature via configuration, which is a valid correctness and usability issue.	Medium
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2101#issuecomment-3639804435 Original created: 2025-12-11T02:42:19Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>✅ <s>Prioritize architectural cleanup over patching</s></summary> ___ <details><summary><b>Suggestion Impact:</b></summary>The commit removes GetMergedDevice, adds a new GetDeviceByIDStrict wrapper, updates the tasks to mark Phase 2 items as completed (deprecation, auditing callers, migration), and retains GetDevicesByIP—aligning with the suggested architectural cleanup rather than patching. code diff: ```diff +func (r *DeviceRegistry) GetDeviceByIDStrict(ctx context.Context, deviceID string) (*models.UnifiedDevice, error) { + return r.GetDevice(ctx, deviceID) } func (r *DeviceRegistry) GetDevicesByIP(ctx context.Context, ip string) ([]*models.UnifiedDevice, error) { @@ -2032,82 +2040,6 @@ return UnifiedDeviceSlice(matchedRecords) } -// looksLikeIP returns true if the input string appears to be an IP address (IPv4 or IPv6). -// Device IDs like "serviceradar:agent:docker-agent" or "default:10.0.0.1" contain colons -// but are NOT valid IP addresses and should not be treated as such. -func looksLikeIP(input string) bool { - input = strings.TrimSpace(input) - if input == "" { - return false - } - - // Try parsing as IP address - this handles both IPv4 and IPv6 - ip := net.ParseIP(input) - return ip != nil -} - -// looksLikeDeviceID returns true if the input string appears to be a device ID rather than an IP. -// Device IDs contain colons in patterns that don't match valid IP addresses. -func looksLikeDeviceID(input string) bool { - input = strings.TrimSpace(input) - if input == "" { - return false - } - - // If it parses as a valid IP, it's not a device ID - if net.ParseIP(input) != nil { - return false - } - - // Device IDs typically contain colons (e.g., "serviceradar:agent:X", "default:10.0.0.1") - // or are ServiceRadar UUIDs (e.g., "sr:...") - return strings.Contains(input, ":") -} - -func (r *DeviceRegistry) GetMergedDevice(ctx context.Context, deviceIDOrIP string) (*models.UnifiedDevice, error) { - // First, try exact device ID lookup in the in-memory registry - if device, err := r.GetDevice(ctx, deviceIDOrIP); err == nil { - return device, nil - } - - // If the input looks like a device ID (not an IP), do NOT fall back to IP lookup. - // This prevents returning the wrong device when multiple devices share an IP (e.g., Docker). - if looksLikeDeviceID(deviceIDOrIP) { - if r.logger != nil { - r.logger.Debug(). - Str("device_id", deviceIDOrIP). - Msg("Device ID lookup failed in registry; not falling back to IP lookup for device ID format") - } - return nil, fmt.Errorf("%w: %s", ErrDeviceNotFound, deviceIDOrIP) - } - - // Only fall back to IP lookup if the input actually looks like an IP address - if !looksLikeIP(deviceIDOrIP) { - return nil, fmt.Errorf("%w: %s", ErrDeviceNotFound, deviceIDOrIP) - } - - devices, err := r.GetDevicesByIP(ctx, deviceIDOrIP) - if err != nil { - return nil, fmt.Errorf("failed to get device by IP %s: %w", deviceIDOrIP, err) - } - - if len(devices) == 0 { - return nil, fmt.Errorf("%w: %s", ErrDeviceNotFound, deviceIDOrIP) - } - - // Note: IP lookup returns first device at that IP. This is only valid when - // there's a single device at that IP, which is typically the case in K8s - // but NOT in Docker where multiple containers may share the host IP. - if r.logger != nil && len(devices) > 1 { - r.logger.Warn(). - Str("ip", deviceIDOrIP). - Int("device_count", len(devices)). - Msg("Multiple devices found at IP; returning first device - consider using device ID for exact lookup") - } - - return devices[0], nil -} ``` </details> ___ **Instead of patching the flawed <code>GetMergedDevice</code> function, proceed with the "Phase <br>2" architectural cleanup outlined in the design documents. Deprecate <br><code>GetMergedDevice</code> and refactor its callers to use explicit <code>GetDeviceByID</code> and <br><code>GetDeviceByIP</code> functions to avoid accumulating technical debt.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ecR2067-R2109">pkg/registry/registry.go [2067-2109]</a> </summary> ```go func (r *DeviceRegistry) GetMergedDevice(ctx context.Context, deviceIDOrIP string) (*models.UnifiedDevice, error) { // First, try exact device ID lookup in the in-memory registry if device, err := r.GetDevice(ctx, deviceIDOrIP); err == nil { return device, nil } // If the input looks like a device ID (not an IP), do NOT fall back to IP lookup. // This prevents returning the wrong device when multiple devices share an IP (e.g., Docker). if looksLikeDeviceID(deviceIDOrIP) { if r.logger != nil { ... (clipped 33 lines) ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2101/files#diff-64afad5a1a6b6937b8fcff6e2984c104f7083e3d12c79cb5a6a0eb9418bf4afeR22-R38">openspec/changes/fix-device-details-wrong-device/tasks.md [22-38]</a> </summary> ```markdown ## Phase 2: Clean Architecture (Optional) ### 2.1 Deprecate `GetMergedDevice` - [ ] Mark `GetMergedDevice` as deprecated - [ ] Audit all callers of `GetMergedDevice` - [ ] Create migration plan for each caller ### 2.2 Create Explicit Functions - [ ] Add `GetDeviceByIDStrict(deviceID string)` - exact match only, returns error if not found - [ ] Keep existing `GetDevicesByIP(ip string)` for legitimate IP lookups ... (clipped 7 lines) ``` </details> ### Solution Walkthrough: #### Before: ```markdown // pkg/registry/registry.go func looksLikeDeviceID(input string) bool { // ... logic to check for colons and not be a valid IP } func GetMergedDevice(ctx, deviceIDOrIP string) (*UnifiedDevice, error) { // 1. Try to get by device ID device, err := r.GetDevice(ctx, deviceIDOrIP) if err == nil { return device, nil } // 2. If it looks like a device ID, fail now (this is the patch) if looksLikeDeviceID(deviceIDOrIP) { return nil, ErrDeviceNotFound } // 3. Fallback to IP lookup for things that look like IPs devices, err := r.GetDevicesByIP(ctx, deviceIDOrIP) // ... return first device } ``` #### After: ```markdown // pkg/registry/registry.go // @deprecated Use GetDeviceByID or GetDevicesByIP. func (r *DeviceRegistry) GetMergedDevice(...) { ... } // New explicit function for ID lookup func (r *DeviceRegistry) GetDeviceByID(ctx, deviceID string) (*UnifiedDevice, error) { return r.GetDevice(ctx, deviceID) // No IP fallback } // pkg/core/api/server.go func (s *APIServer) getDevice(w http.ResponseWriter, r *http.Request) { // ... var unifiedDevice *models.UnifiedDevice var err error if looksLikeIP(deviceID) { devices, err := s.deviceRegistry.GetDevicesByIP(ctx, deviceID) if len(devices) > 0 { unifiedDevice = devices[0] } } else { unifiedDevice, err = s.deviceRegistry.GetDeviceByID(ctx, deviceID) } // ... } ``` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies that the PR implements a patch rather than the ideal architectural cleanup described in its own design documents, which adds complexity to a function slated for deprecation. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=2>Possible issue</td> <td> <details><summary>Allow registry to handle device_id filters</summary> ___ **Modify <code>supportsRegistry</code> to allow <code>device_id</code> filters, enabling the newly added <br>logic in <code>executeRegistry</code> to handle them via the in-memory registry instead of <br>incorrectly routing to SRQL.** [pkg/search/planner.go [312-362]](https://github.com/carverauto/serviceradar/pull/2101/files#diff-e10a578c6efce8ae42e5d451f1b3bb978fd6c1e1dec254a21cdfab89685d2970R312-R362) ```diff func (p *Planner) supportsRegistry(query string, filters map[string]string) bool { // SRQL is required for any query that isn't a simple search or known filter. // This is a simple heuristic; more complex queries will be routed to SRQL. if strings.Contains(query, "or") || strings.Contains(query, "OR") { - return false - } - if strings.Contains(query, "device_id:") { - return false - } - if strings.Contains(query, "device_id =") { return false } // Check for unsupported filters for k := range filters { switch k { case "search", "status", "capability": // supported default: + // Allow device_id filters to be handled by the registry path + if extractDeviceIDFilter(query) != "" { + continue + } return false } } return true } ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies that `supportsRegistry` prevents the new `device_id` filter logic in `executeRegistry` from being used, which is a significant bug in the PR's implementation. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Respect explicit SRQL disable configuration</summary> ___ **Update <code>ensureSRQLConfig</code> to respect the user's choice to disable SRQL in the <br>configuration file, instead of unconditionally enabling it.** [pkg/core/config.go [120-137]](https://github.com/carverauto/serviceradar/pull/2101/files#diff-a2dcdf3bac7ca59032d52f7a0635b5dbcfcb83a6f07cc45db8f57fba0cc33c30R120-R137) ```diff func ensureSRQLConfig(cfg *models.SRQLConfig) *models.SRQLConfig { normalized := cfg if normalized == nil { - normalized = &models.SRQLConfig{} + normalized = &models.SRQLConfig{ + Enabled: true, + } } - - normalized.Enabled = true if strings.TrimSpace(normalized.BaseURL) == "" { switch { case strings.TrimSpace(os.Getenv("SRQL_BASE_URL")) != "": normalized.BaseURL = strings.TrimSpace(os.Getenv("SRQL_BASE_URL")) case strings.TrimSpace(os.Getenv("NEXT_INTERNAL_SRQL_URL")) != "": normalized.BaseURL = strings.TrimSpace(os.Getenv("NEXT_INTERNAL_SRQL_URL")) default: normalized.BaseURL = defaultSRQLBaseURL } } ... ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly points out that the current implementation prevents users from disabling the SRQL feature via configuration, which is a valid correctness and usability issue. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>