carverauto/serviceradar

Fork 0

Chore/podman updates #2533

Merged

mfreeman451 merged 19 commits from refs/pull/2533/head into main

2025-12-09 05:05:35 +00:00

mfreeman451 commented

2025-12-08 23:35:36 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2092
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2092
Original created: 2025-12-08T23:35:36Z
Original updated: 2025-12-09T05:05:50Z
Original head: carverauto/serviceradar:chore/podman_updates
Original base: main
Original merged: 2025-12-09T05:05:35Z 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

Enhancement, Documentation

Description

Add Podman support with dedicated startup script for dependency ordering
Implement container health checks and sequential service startup phases
Document Podman installation and configuration for multiple Linux distributions
Include SELinux troubleshooting guidance for RHEL/AlmaLinux environments

Diagram Walkthrough

flowchart LR
  A["Podman Setup"] --> B["podman-start.sh Script"]
  B --> C["9-Phase Startup Sequence"]
  C --> D["Health Checks & Logging"]
  E["README Updates"] --> F["Installation Instructions"]
  F --> G["SELinux Configuration"]
  F --> H["Troubleshooting Guide"]

File Walkthrough

Relevant files

Enhancement

podman-start.sh

Podman startup orchestration script with health checks

podman-start.sh

New bash script implementing 9-phase startup sequence for ServiceRadar
containers
Includes helper functions for waiting on container health, exit
status, and running state
Handles dependency ordering between certificate generation, database,
messaging, and application services
Provides colored logging output and optional cleanup with --clean flag
Retrieves and displays admin credentials after successful startup

+232/-0

Documentation

README-Docker.md

Add Podman installation and configuration documentation

README-Docker.md

Updated prerequisites section to include Podman 4.0+ as alternative to
Docker
Added comprehensive Podman installation instructions for
AlmaLinux/RHEL/Rocky and Ubuntu/Debian
Documented rootful mode requirement with explanation of privileged
containers and low port bindings
Included SELinux configuration options with three alternative
approaches for bind mount access
Added usage examples for startup script, log viewing, and service
management

+62/-2

Imported from GitHub pull request. Original GitHub pull request: #2092 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2092 Original created: 2025-12-08T23:35:36Z Original updated: 2025-12-09T05:05:50Z Original head: carverauto/serviceradar:chore/podman_updates Original base: main Original merged: 2025-12-09T05:05:35Z 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** Enhancement, Documentation ___ ### **Description** - Add Podman support with dedicated startup script for dependency ordering - Implement container health checks and sequential service startup phases - Document Podman installation and configuration for multiple Linux distributions - Include SELinux troubleshooting guidance for RHEL/AlmaLinux environments ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Podman Setup"] --> B["podman-start.sh Script"] B --> C["9-Phase Startup Sequence"] C --> D["Health Checks & Logging"] E["README Updates"] --> F["Installation Instructions"] F --> G["SELinux Configuration"] F --> H["Troubleshooting Guide"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Enhancement</td><td><table> <tr> <td> <details> <summary>podman-start.sh<dd><code>Podman startup orchestration script with health checks</code>      </dd></summary> <hr> podman-start.sh <ul><li>New bash script implementing 9-phase startup sequence for ServiceRadar containers <li> Includes helper functions for waiting on container health, exit status, and running state <li> Handles dependency ordering between certificate generation, database, messaging, and application services <li> Provides colored logging output and optional cleanup with <code>--clean</code> flag <li> Retrieves and displays admin credentials after successful startup</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2092/files#diff-b535a8cc079d7b11e8171badcb8d2e359d82db6b2903c688fa463798f9e7a154">+232/-0</a>  </td> </tr> </table></td></tr><tr><td>Documentation</td><td><table> <tr> <td> <details> <summary>README-Docker.md<dd><code>Add Podman installation and configuration documentation</code>    </dd></summary> <hr> README-Docker.md <ul><li>Updated prerequisites section to include Podman 4.0+ as alternative to Docker <li> Added comprehensive Podman installation instructions for AlmaLinux/RHEL/Rocky and Ubuntu/Debian <li> Documented rootful mode requirement with explanation of privileged containers and low port bindings <li> Included SELinux configuration options with three alternative approaches for bind mount access <li> Added usage examples for startup script, log viewing, and service management</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2092/files#diff-9fd61d24482efe68c22d8d41e2a1dcc440f39195aa56e7a050f2abe598179efd">+62/-2</a>    </td> </tr> </table></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-08 23:36:00 +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/2092#issuecomment-3629497338
Original created: 2025-12-08T23:36:00Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Excessive privileges Description: The script hard-requires root (EUID==0) and instructs running with sudo, increasing blast radius for podman operations and log/grep actions; consider minimizing privileges or using rootless Podman where possible. podman-start.sh [107-111] Referred Code `# Check if running as root if [ "$EUID" -ne 0 ]; then error "Please run as root (sudo $0)" exit 1 fi`
	Sensitive info exposure Description: Admin credentials are scraped from container logs and echoed to the console, risking sensitive information exposure via terminal history or multi-user systems. podman-start.sh [224-226] Referred Code `sleep 2 podman logs serviceradar-config-updater-mtls 2>&1 \| grep -E "(Username\|Password)" \|\| warn "Could not retrieve credentials" log ""`
	Weakening SELinux policy Description: Documentation advises setting SELinux to permissive mode (setenforce 0), which broadly weakens host security and may lead to unintended access to bind mounts and host files. README-Docker.md [96-105] Referred Code `# Option 1: Temporarily set SELinux to permissive (easiest) sudo setenforce 0 # Option 2: Relabel the project directory for container access sudo chcon -Rt svirt_sandbox_file_t /path/to/serviceradar/docker sudo chcon -Rt svirt_sandbox_file_t /path/to/serviceradar/packaging # Option 3: Allow container cgroup management (may still need option 1 or 2) sudo setsebool -P container_manage_cgroup on` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </strong></summary> - [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true --> </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=1>🟢</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 rowspan=2>🔴</td> <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: <br><a href='https://github.com/carverauto/serviceradar/pull/2092/files#diff-b535a8cc079d7b11e8171badcb8d2e359d82db6b2903c688fa463798f9e7a154R223-R226'><strong>Sensitive logs</strong></a>: The script prints admin credentials by grepping container logs and echoes errors/warnings <br>directly to the console, which can expose sensitive information to user-facing output.<br> <details open><summary>Referred Code</summary> ```shell log "Getting admin credentials..." sleep 2 podman logs serviceradar-config-updater-mtls 2>&1 \| grep -E "(Username\|Password)" \|\| warn "Could not retrieve credentials" log "" 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: Secrets in logs: The script retrieves and displays admin Username/Password from container logs and recommends grepping Password in README, which violates secure logging by exposing credentials. Referred Code `log "Getting admin credentials..." sleep 2 podman logs serviceradar-config-updater-mtls 2>&1 \| grep -E "(Username\|Password)" \|\| warn "Could not retrieve credentials" log "" log "Access ServiceRadar at: http://localhost" log "" log "Check status with: sudo podman ps"` 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 auditing: The startup script performs critical operational actions (starting/stopping services, cleanup) without emitting structured audit logs that include user identity and action outcomes. Referred Code `start_service() { local service=$1 log "Starting $service..." $PC up -d "$service" 2>/dev/null \|\| true } # Main startup sequence main() { log "ServiceRadar Podman Startup" log "===========================" # Check if running as root if [ "$EUID" -ne 0 ]; then error "Please run as root (sudo $0)" exit 1 fi cd "$(dirname "$0")" # Phase 0: Cleanup if requested if [ "$1" = "--clean" ] \|\| [ "$1" = "-c" ]; then ... (clipped 7 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: Timeout handling: Functions like wait_for_container_healthy continue after health-check timeouts and the script uses set -e with broad try/ignore patterns, leading to potential silent partial failures without clear remediation. Referred Code wait_for_container_healthy() { local container=$1 local timeout=${2:-120} local count=0 log "Waiting for $container to be healthy..." while [ $count -lt $timeout ]; do health=$(podman inspect --format '{{.State.Health.Status}}' "$container" 2>/dev/null \|\| echo "unknown") status=$(podman inspect --format '{{.State.Status}}' "$container" 2>/dev/null \|\| echo "not_found") if [ "$health" = "healthy" ]; then log "$container is healthy" return 0 elif [ "$status" = "exited" ]; then error "$container exited unexpectedly" podman logs "$container" \| tail -20 return 1 fi sleep 2 ((count+=2)) done warn "$container health check timed out, continuing anyway..." return 0 ... (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: Rootful guidance: Documentation instructs running all containers rootful with privileged services and suggests disabling/enabling SELinux constraints without discussing security implications or safer alternatives. Referred Code Why rootful mode is required: - The `agent` service uses `privileged: true` for network scanning - Ports 80, 514, and 162 require root to bind (< 1024) - Some init containers run as `user: "0:0"` SELinux considerations (RHEL/AlmaLinux): SELinux blocks bind mounts by default. Choose one of these options: ```bash # Option 1: Temporarily set SELinux to permissive (easiest) sudo setenforce 0 # Option 2: Relabel the project directory for container access sudo chcon -Rt svirt_sandbox_file_t /path/to/serviceradar/docker sudo chcon -Rt svirt_sandbox_file_t /path/to/serviceradar/packaging # Option 3: Allow container cgroup management (may still need option 1 or 2) sudo setsebool -P container_manage_cgroup on </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 <!-- /compliance --update_compliance=true --> </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>

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2092#issuecomment-3629497338 Original created: 2025-12-08T23:36:00Z --- ## 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=3>⚪</td> <td><details><summary>Excessive privileges </summary> Description: The script hard-requires root (EUID==0) and instructs running with sudo, increasing blast radius for podman operations and log/grep actions; consider minimizing privileges or using rootless Podman where possible. <a href='https://github.com/carverauto/serviceradar/pull/2092/files#diff-b535a8cc079d7b11e8171badcb8d2e359d82db6b2903c688fa463798f9e7a154R107-R111'>podman-start.sh [107-111]</a> <details open><summary>Referred Code</summary> ```shell # Check if running as root if [ "$EUID" -ne 0 ]; then error "Please run as root (sudo $0)" exit 1 fi ``` </details></details></td></tr> <tr><td><details><summary>Sensitive info exposure </summary> Description: Admin credentials are scraped from container logs and echoed to the console, risking sensitive information exposure via terminal history or multi-user systems. <a href='https://github.com/carverauto/serviceradar/pull/2092/files#diff-b535a8cc079d7b11e8171badcb8d2e359d82db6b2903c688fa463798f9e7a154R224-R226'>podman-start.sh [224-226]</a> <details open><summary>Referred Code</summary> ```shell sleep 2 podman logs serviceradar-config-updater-mtls 2>&1 | grep -E "(Username|Password)" || warn "Could not retrieve credentials" log "" ``` </details></details></td></tr> <tr><td><details><summary>Weakening SELinux policy </summary> Description: Documentation advises setting SELinux to permissive mode (setenforce 0), which broadly weakens host security and may lead to unintended access to bind mounts and host files. <a href='https://github.com/carverauto/serviceradar/pull/2092/files#diff-9fd61d24482efe68c22d8d41e2a1dcc440f39195aa56e7a050f2abe598179efdR96-R105'>README-Docker.md [96-105]</a> <details open><summary>Referred Code</summary> ```markdown # Option 1: Temporarily set SELinux to permissive (easiest) sudo setenforce 0 # Option 2: Relabel the project directory for container access sudo chcon -Rt svirt_sandbox_file_t /path/to/serviceradar/docker sudo chcon -Rt svirt_sandbox_file_t /path/to/serviceradar/packaging # Option 3: Allow container cgroup management (may still need option 1 or 2) sudo setsebool -P container_manage_cgroup on ``` ``` </details></details></td></tr> <tr><td colspan='2'>Ticket Compliance</td></tr> <tr><td>⚪</td><td><details><summary>🎫 No ticket provided </summary> - [ ] Create ticket/issue  </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=1>🟢</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 rowspan=2>🔴</td> <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:** <a href='https://github.com/carverauto/serviceradar/pull/2092/files#diff-b535a8cc079d7b11e8171badcb8d2e359d82db6b2903c688fa463798f9e7a154R223-R226'>Sensitive logs</a>: The script prints admin credentials by grepping container logs and echoes errors/warnings directly to the console, which can expose sensitive information to user-facing output. <details open><summary>Referred Code</summary> ```shell log "Getting admin credentials..." sleep 2 podman logs serviceradar-config-updater-mtls 2>&1 | grep -E "(Username|Password)" || warn "Could not retrieve credentials" log "" ``` </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/2092/files#diff-b535a8cc079d7b11e8171badcb8d2e359d82db6b2903c688fa463798f9e7a154R223-R229'>Secrets in logs</a>: The script retrieves and displays admin Username/Password from container logs and recommends grepping Password in README, which violates secure logging by exposing credentials. <details open><summary>Referred Code</summary> ```shell log "Getting admin credentials..." sleep 2 podman logs serviceradar-config-updater-mtls 2>&1 | grep -E "(Username|Password)" || warn "Could not retrieve credentials" log "" log "Access ServiceRadar at: http://localhost" log "" log "Check status with: sudo podman ps" ``` </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 rowspan=3>⚪</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/2092/files#diff-b535a8cc079d7b11e8171badcb8d2e359d82db6b2903c688fa463798f9e7a154R96-R123'>Missing auditing</a>: The startup script performs critical operational actions (starting/stopping services, cleanup) without emitting structured audit logs that include user identity and action outcomes. <details open><summary>Referred Code</summary> ```shell start_service() { local service=$1 log "Starting $service..." $PC up -d "$service" 2>/dev/null || true } # Main startup sequence main() { log "ServiceRadar Podman Startup" log "===========================" # Check if running as root if [ "$EUID" -ne 0 ]; then error "Please run as root (sudo $0)" exit 1 fi cd "$(dirname "$0")" # Phase 0: Cleanup if requested if [ "$1" = "--clean" ] || [ "$1" = "-c" ]; then ... (clipped 7 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/2092/files#diff-b535a8cc079d7b11e8171badcb8d2e359d82db6b2903c688fa463798f9e7a154R45-R66'>Timeout handling</a>: Functions like wait_for_container_healthy continue after health-check timeouts and the script uses set -e with broad try/ignore patterns, leading to potential silent partial failures without clear remediation. <details open><summary>Referred Code</summary> ```shell wait_for_container_healthy() { local container=$1 local timeout=${2:-120} local count=0 log "Waiting for $container to be healthy..." while [ $count -lt $timeout ]; do health=$(podman inspect --format '{{.State.Health.Status}}' "$container" 2>/dev/null || echo "unknown") status=$(podman inspect --format '{{.State.Status}}' "$container" 2>/dev/null || echo "not_found") if [ "$health" = "healthy" ]; then log "$container is healthy" return 0 elif [ "$status" = "exited" ]; then error "$container exited unexpectedly" podman logs "$container" | tail -20 return 1 fi sleep 2 ((count+=2)) done warn "$container health check timed out, continuing anyway..." return 0 ... (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>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/2092/files#diff-9fd61d24482efe68c22d8d41e2a1dcc440f39195aa56e7a050f2abe598179efdR86-R105'>Rootful guidance</a>: Documentation instructs running all containers rootful with privileged services and suggests disabling/enabling SELinux constraints without discussing security implications or safer alternatives. <details open><summary>Referred Code</summary> ```markdown **Why rootful mode is required:** - The `agent` service uses `privileged: true` for network scanning - Ports 80, 514, and 162 require root to bind (< 1024) - Some init containers run as `user: "0:0"` **SELinux considerations (RHEL/AlmaLinux):** SELinux blocks bind mounts by default. Choose one of these options: ```bash # Option 1: Temporarily set SELinux to permissive (easiest) sudo setenforce 0 # Option 2: Relabel the project directory for container access sudo chcon -Rt svirt_sandbox_file_t /path/to/serviceradar/docker sudo chcon -Rt svirt_sandbox_file_t /path/to/serviceradar/packaging # Option 3: Allow container cgroup management (may still need option 1 or 2) sudo setsebool -P container_manage_cgroup on ``` ``` </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-12-08 23:37:01 +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/2092#issuecomment-3629499701
Original created: 2025-12-08T23:37:01Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Replace startup script with declarative orchestration The `podman-start.sh` script is a fragile, imperative approach to service orchestration. It should be replaced with a more robust, declarative method like using Kubernetes manifests with `podman play kube` or generating systemd unit files. Examples: podman-start.sh [103-230] `main() { log "ServiceRadar Podman Startup" log "===========================" # Check if running as root if [ "$EUID" -ne 0 ]; then error "Please run as root (sudo $0)" exit 1 fi ... (clipped 118 lines)` Solution Walkthrough: Before: `#!/bin/bash # ... (helper functions for waiting) ... main() { # Phase 1: Certificate Generation start_service cert-generator wait_for_container_exit serviceradar-cert-generator-mtls 120 # Phase 2: Database start_service cnpg wait_for_container_healthy serviceradar-cnpg-mtls 120 # Phase 3: Config Generation start_service config-updater wait_for_container_exit serviceradar-config-updater-mtls 60 # ... (and so on for 9 manual phases) ... } main "$@"` After: `# Alternative 1: Use 'podman play kube' with a Kubernetes manifest # File: serviceradar-pod.yaml apiVersion: v1 kind: Pod metadata: name: serviceradar spec: initContainers: - name: cert-generator image: ... - name: config-updater image: ... containers: - name: cnpg # Database image: ... readinessProbe: # Declarative health check exec: command: ["pg_isready"] - name: core image: ... # Depends on DB being ready` Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a major architectural weakness in using a long, imperative shell script for service orchestration, which is brittle and hard to maintain. Proposing declarative alternatives like `podman play kube` or systemd units is a significant improvement for robustness and maintainability.	High
Possible issue	Avoid removing unrelated containers In the cleanup phase, replace the destructive `podman rm -af` command with a more targeted command that only removes containers associated with the ServiceRadar project to avoid deleting unrelated containers. podman-start.sh [115-122] `# Phase 0: Cleanup if requested if [ "$1" = "--clean" ] \|\| [ "$1" = "-c" ]; then log "Cleaning up existing containers and volumes..." $PC down -v 2>/dev/null \|\| true - podman rm -af 2>/dev/null \|\| true + podman rm -f $(podman ps -a --filter "label=io.podman.compose.project=serviceradar" -q) 2>/dev/null \|\| true podman volume prune -f 2>/dev/null \|\| true log "Cleanup complete" fi` Apply / Chat Suggestion importance[1-10]: 8 __ Why: This is a critical suggestion that prevents the script from causing unintended data loss by deleting all containers on the host, rather than just those related to the project.	Medium
Possible issue	✅ ~~Handle container not found scenario~~ Suggestion Impact: The commit added explicit handling of the "not_found" status, attempting to resolve the container name and, if still not found, continuing the loop distinctly. This addresses the suggested not_found check, though it does not immediately fail; it improves responsiveness and clarity around missing containers. code diff: + # Try exact name first, then pattern match status=$(podman inspect --format '{{.State.Status}}' "$container" 2>/dev/null \|\| echo "not_found") + if [ "$status" = "not_found" ]; then + # Try to find container by pattern + actual_name=$(podman ps -a --format "{{.Names}}" \| grep -E "${container}\|${container//-/_}" \| head -1) + if [ -n "$actual_name" ]; then + container="$actual_name" + status=$(podman inspect --format '{{.State.Status}}' "$container" 2>/dev/null \|\| echo "not_found") + fi + fi if [ "$status" = "exited" ]; then exit_code=$(podman inspect --format '{{.State.ExitCode}}' "$container" 2>/dev/null \|\| echo "1") if [ "$exit_code" = "0" ]; then @@ -31,14 +40,21 @@ return 0 else error "$container failed with exit code $exit_code" - podman logs "$container" \| tail -20 + podman logs "$container" 2>&1 \| tail -20 return 1 fi + elif [ "$status" = "not_found" ]; then + sleep 1 + ((count++)) + continue fi sleep 1 ((count++)) done error "$container timed out after ${timeout}s" + # Show what containers exist for debugging + log "Available containers:" + podman ps -a --format "{{.Names}} {{.Status}}" \| head -10 return 1 In the `wait_for_container_exit` function, add a check for the "not_found" status to fail early if a container does not exist, instead of waiting for a timeout. podman-start.sh [20-43] wait_for_container_exit() { local container=$1 local timeout=${2:-60} local count=0 log "Waiting for $container to complete..." while [ $count -lt $timeout ]; do status=$(podman inspect --format '{{.State.Status}}' "$container" 2>/dev/null \|\| echo "not_found") - if [ "$status" = "exited" ]; then + if [ "$status" = "not_found" ]; then + if [ $count -gt 5 ]; then + error "$container not found after ${count}s" + return 1 + fi + elif [ "$status" = "exited" ]; then exit_code=$(podman inspect --format '{{.State.ExitCode}}' "$container" 2>/dev/null \|\| echo "1") if [ "$exit_code" = "0" ]; then log "$container completed successfully" return 0 else error "$container failed with exit code $exit_code" podman logs "$container" \| tail -20 return 1 fi fi sleep 1 ((count++)) done error "$container timed out after ${timeout}s" return 1 } `[Suggestion processed]` Suggestion importance[1-10]: 6 __ Why: The suggestion improves the script's robustness by adding an early exit condition, which provides faster and more specific feedback if a container fails to start.	Low
General	Detect service startup failures In the `start_service` function, remove `\|\| true` and add error handling to check the exit code of `podman-compose up`, ensuring that service startup failures are detected and reported. podman-start.sh [96-100] `start_service() { local service=$1 log "Starting $service..." - $PC up -d "$service" 2>/dev/null \|\| true + if ! $PC up -d "$service" 2>&1 \| tee /tmp/pc-output.log \| grep -v "WARNING"; then + error "Failed to start $service" + cat /tmp/pc-output.log + return 1 + fi + return 0 }` Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that suppressing errors with `\|\| true` can hide important startup failures, and proposes a more robust error handling mechanism to improve script reliability and debuggability.	Medium
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2092#issuecomment-3629499701 Original created: 2025-12-08T23:37:01Z --- ## 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>Replace startup script with declarative orchestration</summary> ___ **The <code>podman-start.sh</code> script is a fragile, imperative approach to service orchestration. It should be replaced with a more robust, declarative method like using Kubernetes manifests with <code>podman play kube</code> or generating systemd unit files.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2092/files#diff-b535a8cc079d7b11e8171badcb8d2e359d82db6b2903c688fa463798f9e7a154R103-R230">podman-start.sh [103-230]</a> </summary> ```bash main() { log "ServiceRadar Podman Startup" log "===========================" # Check if running as root if [ "$EUID" -ne 0 ]; then error "Please run as root (sudo $0)" exit 1 fi ... (clipped 118 lines) ``` </details> ### Solution Walkthrough: #### Before: ```bash #!/bin/bash # ... (helper functions for waiting) ... main() { # Phase 1: Certificate Generation start_service cert-generator wait_for_container_exit serviceradar-cert-generator-mtls 120 # Phase 2: Database start_service cnpg wait_for_container_healthy serviceradar-cnpg-mtls 120 # Phase 3: Config Generation start_service config-updater wait_for_container_exit serviceradar-config-updater-mtls 60 # ... (and so on for 9 manual phases) ... } main "$@" ``` #### After: ```bash # Alternative 1: Use 'podman play kube' with a Kubernetes manifest # File: serviceradar-pod.yaml apiVersion: v1 kind: Pod metadata: name: serviceradar spec: initContainers: - name: cert-generator image: ... - name: config-updater image: ... containers: - name: cnpg # Database image: ... readinessProbe: # Declarative health check exec: command: ["pg_isready"] - name: core image: ... # Depends on DB being ready ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies a major architectural weakness in using a long, imperative shell script for service orchestration, which is brittle and hard to maintain. Proposing declarative alternatives like `podman play kube` or systemd units is a significant improvement for robustness and maintainability. </details></details></td><td align=center>High </td></tr><tr><td rowspan=2>Possible issue</td> <td> <details><summary>Avoid removing unrelated containers</summary> ___ **In the cleanup phase, replace the destructive <code>podman rm -af</code> command with a more targeted command that only removes containers associated with the ServiceRadar project to avoid deleting unrelated containers.** [podman-start.sh [115-122]](https://github.com/carverauto/serviceradar/pull/2092/files#diff-b535a8cc079d7b11e8171badcb8d2e359d82db6b2903c688fa463798f9e7a154R115-R122) ```diff # Phase 0: Cleanup if requested if [ "$1" = "--clean" ] || [ "$1" = "-c" ]; then log "Cleaning up existing containers and volumes..." $PC down -v 2>/dev/null || true - podman rm -af 2>/dev/null || true + podman rm -f $(podman ps -a --filter "label=io.podman.compose.project=serviceradar" -q) 2>/dev/null || true podman volume prune -f 2>/dev/null || true log "Cleanup complete" fi ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: This is a critical suggestion that prevents the script from causing unintended data loss by deleting all containers on the host, rather than just those related to the project. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>✅ <s>Handle container not found scenario</s></summary> ___ <details><summary>Suggestion Impact:</summary>The commit added explicit handling of the "not_found" status, attempting to resolve the container name and, if still not found, continuing the loop distinctly. This addresses the suggested not_found check, though it does not immediately fail; it improves responsiveness and clarity around missing containers. code diff: ```diff + # Try exact name first, then pattern match status=$(podman inspect --format '{{.State.Status}}' "$container" 2>/dev/null || echo "not_found") + if [ "$status" = "not_found" ]; then + # Try to find container by pattern + actual_name=$(podman ps -a --format "{{.Names}}" | grep -E "${container}|${container//-/_}" | head -1) + if [ -n "$actual_name" ]; then + container="$actual_name" + status=$(podman inspect --format '{{.State.Status}}' "$container" 2>/dev/null || echo "not_found") + fi + fi if [ "$status" = "exited" ]; then exit_code=$(podman inspect --format '{{.State.ExitCode}}' "$container" 2>/dev/null || echo "1") if [ "$exit_code" = "0" ]; then @@ -31,14 +40,21 @@ return 0 else error "$container failed with exit code $exit_code" - podman logs "$container" | tail -20 + podman logs "$container" 2>&1 | tail -20 return 1 fi + elif [ "$status" = "not_found" ]; then + sleep 1 + ((count++)) + continue fi sleep 1 ((count++)) done error "$container timed out after ${timeout}s" + # Show what containers exist for debugging + log "Available containers:" + podman ps -a --format "{{.Names}} {{.Status}}" | head -10 return 1 ``` </details> ___ **In the <code>wait_for_container_exit</code> function, add a check for the "not_found" status to fail early if a container does not exist, instead of waiting for a timeout.** [podman-start.sh [20-43]](https://github.com/carverauto/serviceradar/pull/2092/files#diff-b535a8cc079d7b11e8171badcb8d2e359d82db6b2903c688fa463798f9e7a154R20-R43) ```diff wait_for_container_exit() { local container=$1 local timeout=${2:-60} local count=0 log "Waiting for $container to complete..." while [ $count -lt $timeout ]; do status=$(podman inspect --format '{{.State.Status}}' "$container" 2>/dev/null || echo "not_found") - if [ "$status" = "exited" ]; then + if [ "$status" = "not_found" ]; then + if [ $count -gt 5 ]; then + error "$container not found after ${count}s" + return 1 + fi + elif [ "$status" = "exited" ]; then exit_code=$(podman inspect --format '{{.State.ExitCode}}' "$container" 2>/dev/null || echo "1") if [ "$exit_code" = "0" ]; then log "$container completed successfully" return 0 else error "$container failed with exit code $exit_code" podman logs "$container" | tail -20 return 1 fi fi sleep 1 ((count++)) done error "$container timed out after ${timeout}s" return 1 } ``` `[Suggestion processed]` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion improves the script's robustness by adding an early exit condition, which provides faster and more specific feedback if a container fails to start. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Detect service startup failures</summary> ___ **In the <code>start_service</code> function, remove <code>|| true</code> and add error handling to check the exit code of <code>podman-compose up</code>, ensuring that service startup failures are detected and reported.** [podman-start.sh [96-100]](https://github.com/carverauto/serviceradar/pull/2092/files#diff-b535a8cc079d7b11e8171badcb8d2e359d82db6b2903c688fa463798f9e7a154R96-R100) ```diff start_service() { local service=$1 log "Starting $service..." - $PC up -d "$service" 2>/dev/null || true + if ! $PC up -d "$service" 2>&1 | tee /tmp/pc-output.log | grep -v "WARNING"; then + error "Failed to start $service" + cat /tmp/pc-output.log + return 1 + fi + return 0 } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies that suppressing errors with `|| true` can hide important startup failures, and proposes a more robust error handling mechanism to improve script reliability and debuggability. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>

No reviewers

No labels

1week

2weeks

Failed compliance check

IP cameras

NATS

Possible security concern

serviceradar-agent-gateway

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

carverauto/serviceradar!2533

No description provided.

Security Compliance
⚪	Excessive privileges Description: The script hard-requires root (EUID==0) and instructs running with sudo, increasing blast radius for podman operations and log/grep actions; consider minimizing privileges or using rootless Podman where possible. podman-start.sh [107-111] Referred Code `# Check if running as root if [ "$EUID" -ne 0 ]; then error "Please run as root (sudo $0)" exit 1 fi`
	Sensitive info exposure Description: Admin credentials are scraped from container logs and echoed to the console, risking sensitive information exposure via terminal history or multi-user systems. podman-start.sh [224-226] Referred Code `sleep 2 podman logs serviceradar-config-updater-mtls 2>&1 \| grep -E "(Username\|Password)" \|\| warn "Could not retrieve credentials" log ""`
	Weakening SELinux policy Description: Documentation advises setting SELinux to permissive mode (setenforce 0), which broadly weakens host security and may lead to unintended access to bind mounts and host files. README-Docker.md [96-105] Referred Code `# Option 1: Temporarily set SELinux to permissive (easiest) sudo setenforce 0 # Option 2: Relabel the project directory for container access sudo chcon -Rt svirt_sandbox_file_t /path/to/serviceradar/docker sudo chcon -Rt svirt_sandbox_file_t /path/to/serviceradar/packaging # Option 3: Allow container cgroup management (may still need option 1 or 2) sudo setsebool -P container_manage_cgroup on` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </strong></summary> - [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true --> </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=1>🟢</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 rowspan=2>🔴</td> <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: <br><a href='https://github.com/carverauto/serviceradar/pull/2092/files#diff-b535a8cc079d7b11e8171badcb8d2e359d82db6b2903c688fa463798f9e7a154R223-R226'><strong>Sensitive logs</strong></a>: The script prints admin credentials by grepping container logs and echoes errors/warnings <br>directly to the console, which can expose sensitive information to user-facing output.<br> <details open><summary>Referred Code</summary> ```shell log "Getting admin credentials..." sleep 2 podman logs serviceradar-config-updater-mtls 2>&1 \| grep -E "(Username\|Password)" \|\| warn "Could not retrieve credentials" log "" 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: Secrets in logs: The script retrieves and displays admin Username/Password from container logs and recommends grepping Password in README, which violates secure logging by exposing credentials. Referred Code `log "Getting admin credentials..." sleep 2 podman logs serviceradar-config-updater-mtls 2>&1 \| grep -E "(Username\|Password)" \|\| warn "Could not retrieve credentials" log "" log "Access ServiceRadar at: http://localhost" log "" log "Check status with: sudo podman ps"` 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 auditing: The startup script performs critical operational actions (starting/stopping services, cleanup) without emitting structured audit logs that include user identity and action outcomes. Referred Code `start_service() { local service=$1 log "Starting $service..." $PC up -d "$service" 2>/dev/null \|\| true } # Main startup sequence main() { log "ServiceRadar Podman Startup" log "===========================" # Check if running as root if [ "$EUID" -ne 0 ]; then error "Please run as root (sudo $0)" exit 1 fi cd "$(dirname "$0")" # Phase 0: Cleanup if requested if [ "$1" = "--clean" ] \|\| [ "$1" = "-c" ]; then ... (clipped 7 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: Timeout handling: Functions like wait_for_container_healthy continue after health-check timeouts and the script uses set -e with broad try/ignore patterns, leading to potential silent partial failures without clear remediation. Referred Code wait_for_container_healthy() { local container=$1 local timeout=${2:-120} local count=0 log "Waiting for $container to be healthy..." while [ $count -lt $timeout ]; do health=$(podman inspect --format '{{.State.Health.Status}}' "$container" 2>/dev/null \|\| echo "unknown") status=$(podman inspect --format '{{.State.Status}}' "$container" 2>/dev/null \|\| echo "not_found") if [ "$health" = "healthy" ]; then log "$container is healthy" return 0 elif [ "$status" = "exited" ]; then error "$container exited unexpectedly" podman logs "$container" \| tail -20 return 1 fi sleep 2 ((count+=2)) done warn "$container health check timed out, continuing anyway..." return 0 ... (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: Rootful guidance: Documentation instructs running all containers rootful with privileged services and suggests disabling/enabling SELinux constraints without discussing security implications or safer alternatives. Referred Code Why rootful mode is required: - The `agent` service uses `privileged: true` for network scanning - Ports 80, 514, and 162 require root to bind (< 1024) - Some init containers run as `user: "0:0"` SELinux considerations (RHEL/AlmaLinux): SELinux blocks bind mounts by default. Choose one of these options: ```bash # Option 1: Temporarily set SELinux to permissive (easiest) sudo setenforce 0 # Option 2: Relabel the project directory for container access sudo chcon -Rt svirt_sandbox_file_t /path/to/serviceradar/docker sudo chcon -Rt svirt_sandbox_file_t /path/to/serviceradar/packaging # Option 3: Allow container cgroup management (may still need option 1 or 2) sudo setsebool -P container_manage_cgroup on </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 <!-- /compliance --update_compliance=true --> </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>

Rows
Columns