carverauto/serviceradar

Fork 0

docker updates #2531

Merged

mfreeman451 merged 1 commit from refs/pull/2531/head into main

2025-12-08 22:17:11 +00:00

mfreeman451 commented

2025-12-08 22:15:00 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2090
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2090
Original created: 2025-12-08T22:15:00Z
Original updated: 2025-12-08T22:17:14Z
Original head: carverauto/serviceradar:chore/docker-doc-updates
Original base: main
Original merged: 2025-12-08T22:17:11Z 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

Migrate Docker Compose from SPIFFE to mTLS security model: Replaced SPIFFE-based inter-service communication with direct mTLS certificates, removing SPIRE services and simplifying security configuration
Create backward-compatible SPIFFE override file: Introduced docker-compose.spiffe.yml to preserve SPIFFE configuration for users who require it
Standardize image versioning: Replaced latest tag with ${APP_TAG:-v1.0.65} variable across all services and updated .env.example to use APP_TAG instead of SERVICERADAR_VERSION
Consolidate Docker documentation: Restructured README-Docker.md with OS-specific setup instructions, startup sequence, and troubleshooting guidance
Simplify main README: Streamlined README.md with condensed Docker Compose examples and references to detailed documentation
Update runbooks and documentation: Removed file-specific -f docker-compose.mtls.yml flags from all command examples in runbooks and deployment guides, simplifying to generic docker compose syntax
Remove deprecated file: Deleted docker-compose.mtls.yml as functionality is now in main docker-compose.yml

Diagram Walkthrough

flowchart LR
  A["SPIFFE-based<br/>Security"] -->|"Migrate to mTLS"| B["mTLS Certificates<br/>docker-compose.yml"]
  A -->|"Preserve for<br/>backward compatibility"| C["docker-compose.spiffe.yml"]
  D["SERVICERADAR_VERSION"] -->|"Replace with"| E["APP_TAG variable<br/>v1.0.65"]
  F["Scattered Docker<br/>Documentation"] -->|"Consolidate &<br/>Reorganize"| G["README-Docker.md<br/>with OS-specific setup"]
  H["File-specific<br/>docker-compose flags"] -->|"Simplify"| I["Generic docker compose<br/>commands"]

File Walkthrough

Relevant files

Configuration changes

3 files

docker-compose.yml `Migrate Docker Compose from SPIFFE to mTLS security model` docker-compose.yml Replaced SPIFFE-based security model with mTLS certificates for inter-service communication Removed SPIRE server, agent, and bootstrap services; replaced with direct certificate-based authentication Updated all service images to use `${APP_TAG:-v1.0.65}` variable instead of `latest` tag Simplified environment variables across services, removing SPIFFE-specific configurations (trust domains, workload sockets, SPIFFE IDs) Changed database configuration from KV store to direct CNPG PostgreSQL connections with mTLS Reorganized service order and dependencies to reflect mTLS architecture Updated container names to include `-mtls` suffix for clarity Removed SPIRE-related volumes (`spire-*`) and simplified volume definitions	+380/-743
docker-compose.spiffe.yml `Create SPIFFE-based Docker Compose override configuration` docker-compose.spiffe.yml Created new file containing the complete SPIFFE-based configuration previously in `docker-compose.yml` Includes all SPIRE services (spire-server, spire-bootstrap, spire-agent) with full SPIFFE trust domain setup Preserves original SPIFFE environment variables and workload socket configurations for all services Maintains SPIRE-related volumes and networking configurations Serves as backward-compatible override file for users requiring SPIFFE security model	+1132/-3
.env.example `Update environment configuration with APP_TAG variable` .env.example Replaced `SERVICERADAR_VERSION` variable with `APP_TAG` environment variable Updated default version from `latest` to `v1.0.65` Simplified comments to clarify the variable is used by docker-compose.yml Removed multiple version example options (latest, 1.0.53, 1.0.52, develop)	+3/-5

Documentation

5 files

README-Docker.md `Restructure Docker documentation with OS-specific setup` README-Docker.md Reorganized documentation with new "OS-Specific Setup" section covering AlmaLinux/RHEL/Rocky Linux, Ubuntu/Debian, and macOS installation instructions Moved "Build Images Locally (Bazel)" section to the end and added "Startup Sequence" section explaining service initialization order Updated Quick Start steps to include environment file creation and simplified docker-compose commands (removed `SERVICERADAR_VERSION` variable usage) Added comprehensive troubleshooting section with AlmaLinux-specific SELinux and firewall configuration guidance Enhanced documentation with clearer formatting, code block improvements, and additional security/TLS references	+125/-38
README.md `Streamline main README with Docker documentation references` README.md Simplified "Quick Installation" section with condensed Docker Compose example and removed redundant prerequisites Updated service descriptions to emphasize mTLS security and automatic certificate generation Consolidated Docker Compose services list and added "Common Commands" section with practical usage examples Replaced detailed inline instructions with references to README-Docker.md for OS-specific setup and troubleshooting Improved formatting and clarity of deployment overview	+31/-50
age-graph-readiness.md `Simplify docker compose commands in AGE readiness runbook` docs/docs/runbooks/age-graph-readiness.md Removed `-f docker-compose.mtls.yml` flag from all `docker compose` commands, simplifying to standard `docker compose` syntax Updated section header from "Docker Compose (docker-compose.mtls.yml)" to generic "Docker Compose" Maintained all command functionality while reducing file-specific references	+7/-7
AGENTS.md `Update docker compose commands in agents deployment runbook` AGENTS.md Updated section header from "Docker MTLS Compose Refresh" to "Docker Compose Refresh" Removed `-f docker-compose.mtls.yml` flag from all `docker compose` commands throughout the section Simplified command examples while maintaining the same operational procedures	+4/-4
compose-mtls-sysmonosx.md `Simplify Docker Compose prerequisites in sysmon-osx runbook` docs/docs/runbooks/compose-mtls-sysmonosx.md Updated prerequisites to reference generic Docker Compose stack instead of specific `docker-compose.mtls.yml` file Simplified prerequisite description to `docker compose up -d` command	+1/-1

Additional files

1 files

docker-compose.mtls.yml

+0/-770

Imported from GitHub pull request. Original GitHub pull request: #2090 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2090 Original created: 2025-12-08T22:15:00Z Original updated: 2025-12-08T22:17:14Z Original head: carverauto/serviceradar:chore/docker-doc-updates Original base: main Original merged: 2025-12-08T22:17:11Z 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** - **Migrate Docker Compose from SPIFFE to mTLS security model**: Replaced SPIFFE-based inter-service communication with direct mTLS certificates, removing SPIRE services and simplifying security configuration - **Create backward-compatible SPIFFE override file**: Introduced `docker-compose.spiffe.yml` to preserve SPIFFE configuration for users who require it - **Standardize image versioning**: Replaced `latest` tag with `${APP_TAG:-v1.0.65}` variable across all services and updated `.env.example` to use `APP_TAG` instead of `SERVICERADAR_VERSION` - **Consolidate Docker documentation**: Restructured `README-Docker.md` with OS-specific setup instructions, startup sequence, and troubleshooting guidance - **Simplify main README**: Streamlined `README.md` with condensed Docker Compose examples and references to detailed documentation - **Update runbooks and documentation**: Removed file-specific `-f docker-compose.mtls.yml` flags from all command examples in runbooks and deployment guides, simplifying to generic `docker compose` syntax - **Remove deprecated file**: Deleted `docker-compose.mtls.yml` as functionality is now in main `docker-compose.yml` ___ ### Diagram Walkthrough ```mermaid flowchart LR A["SPIFFE-based Security"] -->|"Migrate to mTLS"| B["mTLS Certificates docker-compose.yml"] A -->|"Preserve for backward compatibility"| C["docker-compose.spiffe.yml"] D["SERVICERADAR_VERSION"] -->|"Replace with"| E["APP_TAG variable v1.0.65"] F["Scattered Docker Documentation"] -->|"Consolidate & Reorganize"| G["README-Docker.md with OS-specific setup"] H["File-specific docker-compose flags"] -->|"Simplify"| I["Generic docker compose commands"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Configuration changes</td><td><details><summary>3 files</summary><table> <tr> <td> <details> <summary>docker-compose.yml<dd><code>Migrate Docker Compose from SPIFFE to mTLS security model</code></dd></summary> <hr> docker-compose.yml <ul><li>Replaced SPIFFE-based security model with mTLS certificates for inter-service communication <li> Removed SPIRE server, agent, and bootstrap services; replaced with direct certificate-based authentication <li> Updated all service images to use <code>${APP_TAG:-v1.0.65}</code> variable instead of <code>latest</code> tag <li> Simplified environment variables across services, removing SPIFFE-specific configurations (trust domains, workload sockets, SPIFFE IDs) <li> Changed database configuration from KV store to direct CNPG PostgreSQL connections with mTLS <li> Reorganized service order and dependencies to reflect mTLS architecture <li> Updated container names to include <code>-mtls</code> suffix for clarity <li> Removed SPIRE-related volumes (<code>spire-*</code>) and simplified volume definitions</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3">+380/-743</a></td> </tr> <tr> <td> <details> <summary>docker-compose.spiffe.yml<dd><code>Create SPIFFE-based Docker Compose override configuration</code></dd></summary> <hr> docker-compose.spiffe.yml <ul><li>Created new file containing the complete SPIFFE-based configuration previously in <code>docker-compose.yml</code> <li> Includes all SPIRE services (spire-server, spire-bootstrap, spire-agent) with full SPIFFE trust domain setup <li> Preserves original SPIFFE environment variables and workload socket configurations for all services <li> Maintains SPIRE-related volumes and networking configurations <li> Serves as backward-compatible override file for users requiring SPIFFE security model</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2090/files#diff-603fd9e7d40841d174f26b95d0cb0c9537430bf3f7a5da3ccbba4ea3d8ac66c9">+1132/-3</a></td> </tr> <tr> <td> <details> <summary>.env.example<dd><code>Update environment configuration with APP_TAG variable</code>      </dd></summary> <hr> .env.example <ul><li>Replaced <code>SERVICERADAR_VERSION</code> variable with <code>APP_TAG</code> environment variable <li> Updated default version from <code>latest</code> to <code>v1.0.65</code> <li> Simplified comments to clarify the variable is used by docker-compose.yml <li> Removed multiple version example options (latest, 1.0.53, 1.0.52, develop)</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2090/files#diff-a3046da0d15a27e89f2afe639b25748a7ad4d9290af3e7b1b6c1a5533c8f0a8c">+3/-5</a>      </td> </tr> </table></details></td></tr><tr><td>Documentation</td><td><details><summary>5 files</summary><table> <tr> <td> <details> <summary>README-Docker.md<dd><code>Restructure Docker documentation with OS-specific setup</code>    </dd></summary> <hr> README-Docker.md <ul><li>Reorganized documentation with new "OS-Specific Setup" section covering AlmaLinux/RHEL/Rocky Linux, Ubuntu/Debian, and macOS installation instructions <li> Moved "Build Images Locally (Bazel)" section to the end and added "Startup Sequence" section explaining service initialization order <li> Updated Quick Start steps to include environment file creation and simplified docker-compose commands (removed <code>SERVICERADAR_VERSION</code> variable usage) <li> Added comprehensive troubleshooting section with AlmaLinux-specific SELinux and firewall configuration guidance <li> Enhanced documentation with clearer formatting, code block improvements, and additional security/TLS references</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2090/files#diff-9fd61d24482efe68c22d8d41e2a1dcc440f39195aa56e7a050f2abe598179efd">+125/-38</a></td> </tr> <tr> <td> <details> <summary>README.md<dd><code>Streamline main README with Docker documentation references</code></dd></summary> <hr> README.md <ul><li>Simplified "Quick Installation" section with condensed Docker Compose example and removed redundant prerequisites <li> Updated service descriptions to emphasize mTLS security and automatic certificate generation <li> Consolidated Docker Compose services list and added "Common Commands" section with practical usage examples <li> Replaced detailed inline instructions with references to README-Docker.md for OS-specific setup and troubleshooting <li> Improved formatting and clarity of deployment overview</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2090/files#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5">+31/-50</a>  </td> </tr> <tr> <td> <details> <summary>age-graph-readiness.md<dd><code>Simplify docker compose commands in AGE readiness runbook</code></dd></summary> <hr> docs/docs/runbooks/age-graph-readiness.md <ul><li>Removed <code>-f docker-compose.mtls.yml</code> flag from all <code>docker compose</code> commands, simplifying to standard <code>docker compose</code> syntax <li> Updated section header from "Docker Compose (docker-compose.mtls.yml)" to generic "Docker Compose" <li> Maintained all command functionality while reducing file-specific references</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2090/files#diff-64fec6b9d2d878e0b9ffdf75868a063dd73d0980551622f567781240d41afbc2">+7/-7</a>      </td> </tr> <tr> <td> <details> <summary>AGENTS.md<dd><code>Update docker compose commands in agents deployment runbook</code></dd></summary> <hr> AGENTS.md <ul><li>Updated section header from "Docker MTLS Compose Refresh" to "Docker Compose Refresh" <li> Removed <code>-f docker-compose.mtls.yml</code> flag from all <code>docker compose</code> commands throughout the section <li> Simplified command examples while maintaining the same operational procedures</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2090/files#diff-a54ff182c7e8acf56acfd6e4b9c3ff41e2c41a31c9b211b2deb9df75d9a478f9">+4/-4</a>      </td> </tr> <tr> <td> <details> <summary>compose-mtls-sysmonosx.md<dd><code>Simplify Docker Compose prerequisites in sysmon-osx runbook</code></dd></summary> <hr> docs/docs/runbooks/compose-mtls-sysmonosx.md <ul><li>Updated prerequisites to reference generic Docker Compose stack instead of specific <code>docker-compose.mtls.yml</code> file <li> Simplified prerequisite description to <code>docker compose up -d</code> command</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2090/files#diff-6faf517c376b3509d0b14235f1b674a825f0399cd8a20791d8caf3b6ef16525b">+1/-1</a>      </td> </tr> </table></details></td></tr><tr><td>Additional files</td><td><details><summary>1 files</summary><table> <tr> <td>docker-compose.mtls.yml</td> <td><a href="https://github.com/carverauto/serviceradar/pull/2090/files#diff-6d953c3f91279bbdeea722524ee1069c8a367625a3970503232174e56a06bd48">+0/-770</a>  </td> </tr> </table></details></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-08 22:15:47 +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/2090#issuecomment-3629230790
Original created: 2025-12-08T22:15:47Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Unpinned image tag Description: The NATS service uses the floating 'nats:latest' image tag, which can lead to non-reproducible builds and unexpected security regressions when the upstream image changes; pin a specific digest or version tag to reduce supply-chain risk. docker-compose.yml [172-193] Referred Code image: nats:latest platform: linux/amd64 container_name: serviceradar-nats-mtls ports: - "4222:4222" - "8222:8222" - "6222:6222" volumes: - nats-data:/data - cert-data:/etc/serviceradar/certs:ro - ./docker/compose/nats.docker.conf:/etc/nats/nats-server.conf:ro command: ["--config", "/etc/nats/nats-server.conf"] depends_on: cert-generator: condition: service_completed_successfully cert-permissions-fixer: condition: service_completed_successfully networks: serviceradar-net: aliases: - nats.serviceradar ... (clipped 1 lines)
	Insecure default HTTP Description: The Nginx service exposes port 80 publicly without TLS termination configured, potentially encouraging plaintext access to the web UI and APIs; ensure TLS is enabled or provide clear guidance and defaults for HTTPS in production. docker-compose.yml [718-743] Referred Code image: ghcr.io/carverauto/serviceradar-nginx:latest container_name: serviceradar-nginx-mtls ports: - "${SERVICERADAR_HTTP_PORT:-80}:80" environment: API_UPSTREAM: ${API_UPSTREAM:-http://kong:8000} volumes: - cert-data:/etc/serviceradar/certs:ro - ./docker/compose/nginx.conf.template:/etc/nginx/templates/default.conf.template:ro - ./docker/compose/entrypoint-nginx.sh:/docker-entrypoint.d/50-serviceradar.sh:ro depends_on: web: condition: service_started core: condition: service_started kong: condition: service_started healthcheck: test: ["CMD", "wget", "--no-verbose", "--tries=1", "--spider", "http://127.0.0.1/"] interval: 30s timeout: 10s ... (clipped 5 lines)
	Disabled healthcheck Description: The 'rperf-client' healthcheck was replaced with a no-op 'true' command, which can mask unhealthy containers and degrade orchestration safety; restore a meaningful healthcheck to avoid blind spots. docker-compose.yml [612-616] Referred Code `otel: condition: service_started healthcheck: test: ["CMD", "true"] interval: 30s`
	Excessive container privileges Description: The 'agent' container runs with 'privileged: true' and NET_RAW/NET_ADMIN capabilities, expanding the attack surface; restrict capabilities to the minimum necessary and avoid full privilege where possible. docker-compose.yml [265-273] Referred Code `- NET_RAW - NET_ADMIN privileged: true depends_on: cert-generator: condition: service_completed_successfully config-updater: condition: service_completed_successfully command: ["/usr/local/bin/serviceradar-agent","-config","/etc/serviceradar/agent.json"]`
	Exposed internal ports Description: The 'core' service maps ports 8090, 50052, and 9090 to the host without documented network access controls, potentially exposing internal APIs; consider binding to localhost, using a reverse proxy only, or adding guidance to restrict exposure. docker-compose.yml [121-148] Referred Code - "8090:8090" - "50052:50052" - "9090:9090" volumes: - generated-config:/etc/serviceradar/config:ro - cert-data:/etc/serviceradar/certs:ro - credentials:/etc/serviceradar/credentials:ro - core-data:/var/lib/serviceradar - ./logs:/var/log/serviceradar environment: - INIT_DB=true - CONFIG_SOURCE=file - CONFIG_PATH=/etc/serviceradar/config/core.json - CONFIG_WATCH_ENABLED=${CONFIG_WATCH_ENABLED:-true} - CNPG_HOST=${CNPG_HOST:-cnpg} - CNPG_PORT=${CNPG_PORT:-5432} - CNPG_DATABASE=${CNPG_DATABASE:-serviceradar} - CNPG_USERNAME=${CNPG_USERNAME:-serviceradar} - CNPG_PASSWORD=${CNPG_PASSWORD:-serviceradar} - CNPG_SSL_MODE=${CNPG_SSL_MODE:-verify-full} - KV_ADDRESS=datasvc:50057 ... (clipped 7 lines)
	Credential leakage via logs Description: The documentation instructs removing '/etc/serviceradar/certs/password.txt' but does not clarify that the password may still be in container logs ('config-updater' logs show it), risking credential exposure if logs are retained; add guidance to scrub logs and rotate the initial admin password. README-Docker.md [221-223] Referred Code ```bash docker compose exec core rm /etc/serviceradar/certs/password.txt </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=2>🟢</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: 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 rowspan=4>⚪</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/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R195-R744'><strong>Service logging</strong></a>: The compose changes add many services and healthchecks but do not demonstrate that <br>critical security-relevant actions are logged with user IDs and outcomes, which cannot be <br>verified from deployment manifests alone.<br> <details open><summary>Referred Code</summary> ```yaml datasvc: image: ghcr.io/carverauto/serviceradar-datasvc:${APP_TAG:-v1.0.65} container_name: serviceradar-datasvc-mtls cpus: "2.0" ports: - "50057:50057" volumes: - ./docker/compose/datasvc.mtls.json:/etc/serviceradar/datasvc.json:ro - cert-data:/etc/serviceradar/certs:ro - datasvc-data:/var/lib/serviceradar - ./logs:/var/log/serviceradar environment: - CONFIG_PATH=/etc/serviceradar/datasvc.json - WAIT_FOR_NATS=true - LOG_LEVEL=${LOG_LEVEL:-info} depends_on: cert-generator: condition: service_completed_successfully nats: condition: service_started healthcheck: ... (clipped 529 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: Healthcheck stub: The rperf-client healthcheck was replaced with a no-op 'true' command which may hide runtime failures, reducing robustness assurances from the deployment perspective. Referred Code `otel: condition: service_started healthcheck: test: ["CMD", "true"] interval: 30s` 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: Potential secrets: The compose file defines database credentials and service passwords via environment variables and .env defaults, which could risk exposure in logs if applications print env values; verification requires application code review. Referred Code environment: - POSTGRES_USER=${CNPG_USERNAME:-serviceradar} - POSTGRES_PASSWORD=${CNPG_PASSWORD:-serviceradar} - POSTGRES_DB=${CNPG_DATABASE:-serviceradar} - TS_TUNE=false volumes: - cnpg-data:/var/lib/postgresql/data - ./docker/compose/cnpg-init.sql:/docker-entrypoint-initdb.d/001-init.sql:ro - cert-data:/etc/serviceradar/certs:ro healthcheck: test: [ "CMD-SHELL", "PGSSLMODE=require PGSSLROOTCERT=/etc/serviceradar/certs/root.pem pg_isready -h localhost -U ${CNPG_USERNAME:-serviceradar} -d ${CNPG_DATABASE:-serviceradar}" ] interval: 10s timeout: 5s retries: 12 start_period: 10s networks: serviceradar-net: ... (clipped 96 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: Creds in env: Credentials like CNPG username/password and service endpoints are passed via environment variables and defaults, but the diff cannot confirm validation, secret handling, or transport security beyond mTLS configuration. Referred Code environment: - POSTGRES_USER=${CNPG_USERNAME:-serviceradar} - POSTGRES_PASSWORD=${CNPG_PASSWORD:-serviceradar} - POSTGRES_DB=${CNPG_DATABASE:-serviceradar} - TS_TUNE=false volumes: - cnpg-data:/var/lib/postgresql/data - ./docker/compose/cnpg-init.sql:/docker-entrypoint-initdb.d/001-init.sql:ro - cert-data:/etc/serviceradar/certs:ro healthcheck: test: [ "CMD-SHELL", "PGSSLMODE=require PGSSLROOTCERT=/etc/serviceradar/certs/root.pem pg_isready -h localhost -U ${CNPG_USERNAME:-serviceradar} -d ${CNPG_DATABASE:-serviceradar}" ] interval: 10s timeout: 5s retries: 12 start_period: 10s networks: serviceradar-net: ... (clipped 96 lines) Learn more about managing compliance generic rules or creating your own custom rules

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/2090#issuecomment-3629230790 Original created: 2025-12-08T22:15:47Z --- ## 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=6>⚪</td> <td><details><summary>Unpinned image tag </summary> Description: The NATS service uses the floating 'nats:latest' image tag, which can lead to non-reproducible builds and unexpected security regressions when the upstream image changes; pin a specific digest or version tag to reduce supply-chain risk. <a href='https://github.com/carverauto/serviceradar/pull/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R172-R193'>docker-compose.yml [172-193]</a> <details open><summary>Referred Code</summary> ```yaml image: nats:latest platform: linux/amd64 container_name: serviceradar-nats-mtls ports: - "4222:4222" - "8222:8222" - "6222:6222" volumes: - nats-data:/data - cert-data:/etc/serviceradar/certs:ro - ./docker/compose/nats.docker.conf:/etc/nats/nats-server.conf:ro command: ["--config", "/etc/nats/nats-server.conf"] depends_on: cert-generator: condition: service_completed_successfully cert-permissions-fixer: condition: service_completed_successfully networks: serviceradar-net: aliases: - nats.serviceradar ... (clipped 1 lines) ``` </details></details></td></tr> <tr><td><details><summary>Insecure default HTTP </summary> Description: The Nginx service exposes port 80 publicly without TLS termination configured, potentially encouraging plaintext access to the web UI and APIs; ensure TLS is enabled or provide clear guidance and defaults for HTTPS in production. <a href='https://github.com/carverauto/serviceradar/pull/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R718-R743'>docker-compose.yml [718-743]</a> <details open><summary>Referred Code</summary> ```yaml image: ghcr.io/carverauto/serviceradar-nginx:latest container_name: serviceradar-nginx-mtls ports: - "${SERVICERADAR_HTTP_PORT:-80}:80" environment: API_UPSTREAM: ${API_UPSTREAM:-http://kong:8000} volumes: - cert-data:/etc/serviceradar/certs:ro - ./docker/compose/nginx.conf.template:/etc/nginx/templates/default.conf.template:ro - ./docker/compose/entrypoint-nginx.sh:/docker-entrypoint.d/50-serviceradar.sh:ro depends_on: web: condition: service_started core: condition: service_started kong: condition: service_started healthcheck: test: ["CMD", "wget", "--no-verbose", "--tries=1", "--spider", "http://127.0.0.1/"] interval: 30s timeout: 10s ... (clipped 5 lines) ``` </details></details></td></tr> <tr><td><details><summary>Disabled healthcheck </summary> Description: The 'rperf-client' healthcheck was replaced with a no-op 'true' command, which can mask unhealthy containers and degrade orchestration safety; restore a meaningful healthcheck to avoid blind spots. <a href='https://github.com/carverauto/serviceradar/pull/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R612-R616'>docker-compose.yml [612-616]</a> <details open><summary>Referred Code</summary> ```yaml otel: condition: service_started healthcheck: test: ["CMD", "true"] interval: 30s ``` </details></details></td></tr> <tr><td><details><summary>Excessive container privileges </summary> Description: The 'agent' container runs with 'privileged: true' and NET_RAW/NET_ADMIN capabilities, expanding the attack surface; restrict capabilities to the minimum necessary and avoid full privilege where possible. <a href='https://github.com/carverauto/serviceradar/pull/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R265-R273'>docker-compose.yml [265-273]</a> <details open><summary>Referred Code</summary> ```yaml - NET_RAW - NET_ADMIN privileged: true depends_on: cert-generator: condition: service_completed_successfully config-updater: condition: service_completed_successfully command: ["/usr/local/bin/serviceradar-agent","-config","/etc/serviceradar/agent.json"] ``` </details></details></td></tr> <tr><td><details><summary>Exposed internal ports </summary> Description: The 'core' service maps ports 8090, 50052, and 9090 to the host without documented network access controls, potentially exposing internal APIs; consider binding to localhost, using a reverse proxy only, or adding guidance to restrict exposure. <a href='https://github.com/carverauto/serviceradar/pull/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R121-R148'>docker-compose.yml [121-148]</a> <details open><summary>Referred Code</summary> ```yaml - "8090:8090" - "50052:50052" - "9090:9090" volumes: - generated-config:/etc/serviceradar/config:ro - cert-data:/etc/serviceradar/certs:ro - credentials:/etc/serviceradar/credentials:ro - core-data:/var/lib/serviceradar - ./logs:/var/log/serviceradar environment: - INIT_DB=true - CONFIG_SOURCE=file - CONFIG_PATH=/etc/serviceradar/config/core.json - CONFIG_WATCH_ENABLED=${CONFIG_WATCH_ENABLED:-true} - CNPG_HOST=${CNPG_HOST:-cnpg} - CNPG_PORT=${CNPG_PORT:-5432} - CNPG_DATABASE=${CNPG_DATABASE:-serviceradar} - CNPG_USERNAME=${CNPG_USERNAME:-serviceradar} - CNPG_PASSWORD=${CNPG_PASSWORD:-serviceradar} - CNPG_SSL_MODE=${CNPG_SSL_MODE:-verify-full} - KV_ADDRESS=datasvc:50057 ... (clipped 7 lines) ``` </details></details></td></tr> <tr><td><details><summary>Credential leakage via logs </summary> Description: The documentation instructs removing '/etc/serviceradar/certs/password.txt' but does not clarify that the password may still be in container logs ('config-updater' logs show it), risking credential exposure if logs are retained; add guidance to scrub logs and rotate the initial admin password. <a href='https://github.com/carverauto/serviceradar/pull/2090/files#diff-9fd61d24482efe68c22d8d41e2a1dcc440f39195aa56e7a050f2abe598179efdR221-R223'>README-Docker.md [221-223]</a> <details open><summary>Referred Code</summary> ```markdown ```bash docker compose exec core rm /etc/serviceradar/certs/password.txt ``` ``` </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=2>🟢</td><td> <details><summary>Generic: Meaningful Naming and Self-Documenting Code</summary> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code self-documenting **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=4>⚪</td> <td><details> <summary>Generic: Comprehensive Audit Trails</summary> **Objective:** To create a detailed and reliable record of critical system actions for security analysis and compliance. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R195-R744'>Service logging</a>: The compose changes add many services and healthchecks but do not demonstrate that critical security-relevant actions are logged with user IDs and outcomes, which cannot be verified from deployment manifests alone. <details open><summary>Referred Code</summary> ```yaml datasvc: image: ghcr.io/carverauto/serviceradar-datasvc:${APP_TAG:-v1.0.65} container_name: serviceradar-datasvc-mtls cpus: "2.0" ports: - "50057:50057" volumes: - ./docker/compose/datasvc.mtls.json:/etc/serviceradar/datasvc.json:ro - cert-data:/etc/serviceradar/certs:ro - datasvc-data:/var/lib/serviceradar - ./logs:/var/log/serviceradar environment: - CONFIG_PATH=/etc/serviceradar/datasvc.json - WAIT_FOR_NATS=true - LOG_LEVEL=${LOG_LEVEL:-info} depends_on: cert-generator: condition: service_completed_successfully nats: condition: service_started healthcheck: ... (clipped 529 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/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R612-R616'>Healthcheck stub</a>: The rperf-client healthcheck was replaced with a no-op 'true' command which may hide runtime failures, reducing robustness assurances from the deployment perspective. <details open><summary>Referred Code</summary> ```yaml otel: condition: service_started healthcheck: test: ["CMD", "true"] interval: 30s ``` </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/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R31-R147'>Potential secrets</a>: The compose file defines database credentials and service passwords via environment variables and .env defaults, which could risk exposure in logs if applications print env values; verification requires application code review. <details open><summary>Referred Code</summary> ```yaml environment: - POSTGRES_USER=${CNPG_USERNAME:-serviceradar} - POSTGRES_PASSWORD=${CNPG_PASSWORD:-serviceradar} - POSTGRES_DB=${CNPG_DATABASE:-serviceradar} - TS_TUNE=false volumes: - cnpg-data:/var/lib/postgresql/data - ./docker/compose/cnpg-init.sql:/docker-entrypoint-initdb.d/001-init.sql:ro - cert-data:/etc/serviceradar/certs:ro healthcheck: test: [ "CMD-SHELL", "PGSSLMODE=require PGSSLROOTCERT=/etc/serviceradar/certs/root.pem pg_isready -h localhost -U ${CNPG_USERNAME:-serviceradar} -d ${CNPG_DATABASE:-serviceradar}" ] interval: 10s timeout: 5s retries: 12 start_period: 10s networks: serviceradar-net: ... (clipped 96 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/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R31-R147'>Creds in env</a>: Credentials like CNPG username/password and service endpoints are passed via environment variables and defaults, but the diff cannot confirm validation, secret handling, or transport security beyond mTLS configuration. <details open><summary>Referred Code</summary> ```yaml environment: - POSTGRES_USER=${CNPG_USERNAME:-serviceradar} - POSTGRES_PASSWORD=${CNPG_PASSWORD:-serviceradar} - POSTGRES_DB=${CNPG_DATABASE:-serviceradar} - TS_TUNE=false volumes: - cnpg-data:/var/lib/postgresql/data - ./docker/compose/cnpg-init.sql:/docker-entrypoint-initdb.d/001-init.sql:ro - cert-data:/etc/serviceradar/certs:ro healthcheck: test: [ "CMD-SHELL", "PGSSLMODE=require PGSSLROOTCERT=/etc/serviceradar/certs/root.pem pg_isready -h localhost -U ${CNPG_USERNAME:-serviceradar} -d ${CNPG_DATABASE:-serviceradar}" ] interval: 10s timeout: 5s retries: 12 start_period: 10s networks: serviceradar-net: ... (clipped 96 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">   </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 22:16:49 +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/2090#issuecomment-3629234346
Original created: 2025-12-08T22:16:49Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Refactor Docker Compose for maintainability Refactor the new `docker-compose.spiffe.yml` file to be a lean override file instead of a full copy of the service configurations. This will improve maintainability and prevent configuration drift between the mTLS and SPIFFE setups. Examples: docker-compose.spiffe.yml [1-1133] `# ServiceRadar Docker Compose Configuration # # Usage: # Standard deployment (core stack only): # docker compose up -d # # Include testing services (faker): # docker compose -f docker-compose.yml -f docker-compose.testing.yml --profile testing up -d # # Available profiles: ... (clipped 1123 lines)` docker-compose.yml [1-770] `version: "3.9" services: cert-generator: image: alpine:3.20 container_name: serviceradar-cert-generator-mtls volumes: - cert-data:/certs - ./docker/compose/generate-certs.sh:/generate-certs.sh:ro command: sh -c "apk add --no-cache openssl bash && cp /generate-certs.sh /tmp/gen.sh && chmod +x /tmp/gen.sh && CERT_DIR=/certs bash /tmp/gen.sh" ... (clipped 760 lines)` Solution Walkthrough: Before: # docker-compose.yml (new mTLS default) services: core: image: ghcr.io/carverauto/serviceradar-core:${APP_TAG:-v1.0.65} environment: - CORE_SEC_MODE=mtls # ... other service definitions poller: image: ghcr.io/carverauto/serviceradar-poller:${APP_TAG:-v1.0.65} # ... other service definitions # docker-compose.spiffe.yml (complete duplication) services: spire-server: ... spire-agent: ... core: image: ghcr.io/carverauto/serviceradar-core:latest # <-- Inconsistent tag environment: - CORE_SEC_MODE=spiffe # ... full duplication of volumes, ports, etc. poller: image: ghcr.io/carverauto/serviceradar-poller:latest # <-- Inconsistent tag # ... full duplication of volumes, ports, etc. After: # docker-compose.yml (base mTLS config) services: core: image: ghcr.io/carverauto/serviceradar-core:${APP_TAG:-v1.0.65} environment: - CORE_SEC_MODE=mtls # ... other service definitions poller: image: ghcr.io/carverauto/serviceradar-poller:${APP_TAG:-v1.0.65} # ... other service definitions # docker-compose.spiffe.yml (lean override file) services: spire-server: # ... spire-server definition spire-agent: # ... spire-agent definition core: environment: - CONFIG_SOURCE=kv - KV_SEC_MODE=spiffe # ... other SPIFFE-specific overrides depends_on: - spire-agent poller: environment: - CORE_SEC_MODE=spiffe - KV_SEC_MODE=spiffe # ... other SPIFFE-specific overrides depends_on: - spire-agent Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical design flaw where `docker-compose.spiffe.yml` is a full copy, leading to immediate configuration drift (e.g., `image` tags) and a future maintenance burden.	High
Possible issue	Implement a meaningful service healthcheck Restore the meaningful `grpcurl` healthcheck for the `rperf-client` service instead of the current `["CMD", "true"]` which is ineffective. docker-compose.yml [614-619] `healthcheck: - test: ["CMD", "true"] + test: ["CMD", "grpcurl", "-cacert", "/etc/serviceradar/certs/root.pem", "-cert", "/etc/serviceradar/certs/rperf-client.pem", "-key", "/etc/serviceradar/certs/rperf-client-key.pem", "-servername", "rperf-client.serviceradar", "localhost:50059", "grpc.health.v1.Health/Check"] interval: 30s timeout: 10s retries: 3 start_period: 60s` Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that the `rperf-client` healthcheck was changed to a no-op, which can cause race conditions and instability during stack startup.	Medium
	Use application-level healthcheck instead of TCP Replace the current TCP port check for the `srql` service with a more reliable application-level healthcheck that queries a health endpoint. docker-compose.yml [354-359] `healthcheck: - test: ["CMD-SHELL", "bash -c '</dev/tcp/127.0.0.1/8080'"] + test: ["CMD", "curl", "-sf", "http://localhost:8080/healthz"] interval: 30s timeout: 5s retries: 5 start_period: 20s` Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that an application-level healthcheck is more robust than a TCP port check, improving the reliability of service dependency management.	Medium
	Fix incorrect exit status capturing Correct the shell command to properly capture the exit status using `$?` instead of the incorrect `?` for accurate error logging. docker-compose.spiffe.yml [327-340] command: - \| until /usr/local/bin/serviceradar-cli render-kong \ --jwks "${JWKS_URL:-http://core:8090/auth/jwks.json}" \ --service "${KONG_SERVICE_URL:-http://core:8090}" \ --path "${KONG_ROUTE_PATH:-/api}" \ --srql-service "${SRQL_SERVICE_URL:-http://srql:8080}" \ --srql-path "${SRQL_ROUTE_PATH:-/api/query}" \ --out /out/kong.yml do - status=$$? - echo "[kong-config] render failed (exit $$status), retrying in $${KONG_CONFIG_RETRY_SECONDS:-5}s" + status=$? + echo "[kong-config] render failed (exit $status), retrying in $${KONG_CONFIG_RETRY_SECONDS:-5}s" sleep $${KONG_CONFIG_RETRY_SECONDS:-5} done Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a shell scripting bug where `$$?` is used instead of `$?`, which would cause incorrect logging of the exit status, making debugging difficult.	Medium
General	Avoid installing packages at runtime Improve reliability and startup time by creating a custom Docker image for the `cert-generator` service with `openssl` and `bash` pre-installed, instead of installing them at runtime. docker-compose.spiffe.yml [107] `-command: sh -c "apk add --no-cache openssl bash && cp /generate-certs.sh /tmp/gen.sh && chmod +x /tmp/gen.sh && CERT_DIR=/certs bash /tmp/gen.sh" +command: sh -c "cp /generate-certs.sh /tmp/gen.sh && chmod +x /tmp/gen.sh && CERT_DIR=/certs bash /tmp/gen.sh"` Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a best practice violation by installing packages at runtime and proposes creating a custom image, which would improve startup reliability and performance.	Low
	Simplify command and fix path Remove the redundant `until` loop from the `kong` service command, as the `depends_on` condition already guarantees the file's existence, and correct a duplicated path in the `rm` command. docker-compose.spiffe.yml [373] -command: ["sh","-lc","if [ ! -f /opt/kong/kong.yml ]; then cp /default-kong.yml /opt/kong/kong.yml 2>/dev/null \|\| true; fi; until [ -f /opt/kong/kong.yml ]; do echo '[kong] waiting for /opt/kong/kong.yml'; sleep 2; done; rm -f /usr/local/kong/pids/nginx.pid /opt/kong/pids/nginx.pid 2>/dev/null \|\| true; exec kong start --vv"] +command: ["sh","-lc","cp -n /default-kong.yml /opt/kong/kong.yml 2>/dev/null \|\| true; rm -f /usr/local/kong/pids/nginx.pid /opt/kong/pids/nginx.pid 2>/dev/null \|\| true; exec kong start --vv"] Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies a redundant `until` loop due to an existing `depends_on` condition, and it also points out a duplicated path in an `rm` command, improving startup efficiency and correctness.	Low
	Simplify the service startup command Simplify the `cert-generator` service's startup command by directly executing the script from its mount point, removing the need to copy and change permissions. docker-compose.yml [10] `-command: sh -c "apk add --no-cache openssl bash && cp /generate-certs.sh /tmp/gen.sh && chmod +x /tmp/gen.sh && CERT_DIR=/certs bash /tmp/gen.sh" +command: sh -c "apk add --no-cache openssl bash && CERT_DIR=/certs bash /generate-certs.sh"` Apply / Chat Suggestion importance[1-10]: 4 __ Why: The suggestion offers a valid simplification of the `cert-generator` command, improving readability and removing an unnecessary file copy operation.	Low
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2090#issuecomment-3629234346 Original created: 2025-12-08T22:16:49Z --- ## 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>Refactor Docker Compose for maintainability</summary> ___ **Refactor the new <code>docker-compose.spiffe.yml</code> file to be a lean override file instead of a full copy of the service configurations. This will improve maintainability and prevent configuration drift between the mTLS and SPIFFE setups.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2090/files#diff-603fd9e7d40841d174f26b95d0cb0c9537430bf3f7a5da3ccbba4ea3d8ac66c9R1-R1133">docker-compose.spiffe.yml [1-1133]</a> </summary> ```yaml # ServiceRadar Docker Compose Configuration # # Usage: # Standard deployment (core stack only): # docker compose up -d # # Include testing services (faker): # docker compose -f docker-compose.yml -f docker-compose.testing.yml --profile testing up -d # # Available profiles: ... (clipped 1123 lines) ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R1-R770">docker-compose.yml [1-770]</a> </summary> ```yaml version: "3.9" services: cert-generator: image: alpine:3.20 container_name: serviceradar-cert-generator-mtls volumes: - cert-data:/certs - ./docker/compose/generate-certs.sh:/generate-certs.sh:ro command: sh -c "apk add --no-cache openssl bash && cp /generate-certs.sh /tmp/gen.sh && chmod +x /tmp/gen.sh && CERT_DIR=/certs bash /tmp/gen.sh" ... (clipped 760 lines) ``` </details> ### Solution Walkthrough: #### Before: ```yaml # docker-compose.yml (new mTLS default) services: core: image: ghcr.io/carverauto/serviceradar-core:${APP_TAG:-v1.0.65} environment: - CORE_SEC_MODE=mtls # ... other service definitions poller: image: ghcr.io/carverauto/serviceradar-poller:${APP_TAG:-v1.0.65} # ... other service definitions # docker-compose.spiffe.yml (complete duplication) services: spire-server: ... spire-agent: ... core: image: ghcr.io/carverauto/serviceradar-core:latest # <-- Inconsistent tag environment: - CORE_SEC_MODE=spiffe # ... full duplication of volumes, ports, etc. poller: image: ghcr.io/carverauto/serviceradar-poller:latest # <-- Inconsistent tag # ... full duplication of volumes, ports, etc. ``` #### After: ```yaml # docker-compose.yml (base mTLS config) services: core: image: ghcr.io/carverauto/serviceradar-core:${APP_TAG:-v1.0.65} environment: - CORE_SEC_MODE=mtls # ... other service definitions poller: image: ghcr.io/carverauto/serviceradar-poller:${APP_TAG:-v1.0.65} # ... other service definitions # docker-compose.spiffe.yml (lean override file) services: spire-server: # ... spire-server definition spire-agent: # ... spire-agent definition core: environment: - CONFIG_SOURCE=kv - KV_SEC_MODE=spiffe # ... other SPIFFE-specific overrides depends_on: - spire-agent poller: environment: - CORE_SEC_MODE=spiffe - KV_SEC_MODE=spiffe # ... other SPIFFE-specific overrides depends_on: - spire-agent ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies a critical design flaw where `docker-compose.spiffe.yml` is a full copy, leading to immediate configuration drift (e.g., `image` tags) and a future maintenance burden. </details></details></td><td align=center>High </td></tr><tr><td rowspan=3>Possible issue</td> <td> <details><summary>Implement a meaningful service healthcheck</summary> ___ **Restore the meaningful <code>grpcurl</code> healthcheck for the <code>rperf-client</code> service instead of the current <code>["CMD", "true"]</code> which is ineffective.** [docker-compose.yml [614-619]](https://github.com/carverauto/serviceradar/pull/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R614-R619) ```diff healthcheck: - test: ["CMD", "true"] + test: ["CMD", "grpcurl", "-cacert", "/etc/serviceradar/certs/root.pem", "-cert", "/etc/serviceradar/certs/rperf-client.pem", "-key", "/etc/serviceradar/certs/rperf-client-key.pem", "-servername", "rperf-client.serviceradar", "localhost:50059", "grpc.health.v1.Health/Check"] interval: 30s timeout: 10s retries: 3 start_period: 60s ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies that the `rperf-client` healthcheck was changed to a no-op, which can cause race conditions and instability during stack startup. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Use application-level healthcheck instead of TCP</summary> ___ **Replace the current TCP port check for the <code>srql</code> service with a more reliable application-level healthcheck that queries a health endpoint.** [docker-compose.yml [354-359]](https://github.com/carverauto/serviceradar/pull/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R354-R359) ```diff healthcheck: - test: ["CMD-SHELL", "bash -c '</dev/tcp/127.0.0.1/8080'"] + test: ["CMD", "curl", "-sf", "http://localhost:8080/healthz"] interval: 30s timeout: 5s retries: 5 start_period: 20s ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly points out that an application-level healthcheck is more robust than a TCP port check, improving the reliability of service dependency management. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Fix incorrect exit status capturing</summary> ___ **Correct the shell command to properly capture the exit status using <code>$?</code> instead of the incorrect <code>$$?</code> for accurate error logging.** [docker-compose.spiffe.yml [327-340]](https://github.com/carverauto/serviceradar/pull/2090/files#diff-603fd9e7d40841d174f26b95d0cb0c9537430bf3f7a5da3ccbba4ea3d8ac66c9R327-R340) ```diff command: - | until /usr/local/bin/serviceradar-cli render-kong \ --jwks "${JWKS_URL:-http://core:8090/auth/jwks.json}" \ --service "${KONG_SERVICE_URL:-http://core:8090}" \ --path "${KONG_ROUTE_PATH:-/api}" \ --srql-service "${SRQL_SERVICE_URL:-http://srql:8080}" \ --srql-path "${SRQL_ROUTE_PATH:-/api/query}" \ --out /out/kong.yml do - status=$$? - echo "[kong-config] render failed (exit $$status), retrying in $${KONG_CONFIG_RETRY_SECONDS:-5}s" + status=$? + echo "[kong-config] render failed (exit $status), retrying in $${KONG_CONFIG_RETRY_SECONDS:-5}s" sleep $${KONG_CONFIG_RETRY_SECONDS:-5} done ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a shell scripting bug where `$$?` is used instead of `$?`, which would cause incorrect logging of the exit status, making debugging difficult. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=3>General</td> <td> <details><summary>Avoid installing packages at runtime</summary> ___ **Improve reliability and startup time by creating a custom Docker image for the <code>cert-generator</code> service with <code>openssl</code> and <code>bash</code> pre-installed, instead of installing them at runtime.** [docker-compose.spiffe.yml [107]](https://github.com/carverauto/serviceradar/pull/2090/files#diff-603fd9e7d40841d174f26b95d0cb0c9537430bf3f7a5da3ccbba4ea3d8ac66c9R107-R107) ```diff -command: sh -c "apk add --no-cache openssl bash && cp /generate-certs.sh /tmp/gen.sh && chmod +x /tmp/gen.sh && CERT_DIR=/certs bash /tmp/gen.sh" +command: sh -c "cp /generate-certs.sh /tmp/gen.sh && chmod +x /tmp/gen.sh && CERT_DIR=/certs bash /tmp/gen.sh" ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies a best practice violation by installing packages at runtime and proposes creating a custom image, which would improve startup reliability and performance. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>Simplify command and fix path</summary> ___ **Remove the redundant <code>until</code> loop from the <code>kong</code> service command, as the <code>depends_on</code> condition already guarantees the file's existence, and correct a duplicated path in the <code>rm</code> command.** [docker-compose.spiffe.yml [373]](https://github.com/carverauto/serviceradar/pull/2090/files#diff-603fd9e7d40841d174f26b95d0cb0c9537430bf3f7a5da3ccbba4ea3d8ac66c9R373-R373) ```diff -command: ["sh","-lc","if [ ! -f /opt/kong/kong.yml ]; then cp /default-kong.yml /opt/kong/kong.yml 2>/dev/null || true; fi; until [ -f /opt/kong/kong.yml ]; do echo '[kong] waiting for /opt/kong/kong.yml'; sleep 2; done; rm -f /usr/local/kong/pids/nginx.pid /opt/kong/pids/nginx.pid 2>/dev/null || true; exec kong start --vv"] +command: ["sh","-lc","cp -n /default-kong.yml /opt/kong/kong.yml 2>/dev/null || true; rm -f /usr/local/kong/pids/nginx.pid /opt/kong/pids/nginx.pid 2>/dev/null || true; exec kong start --vv"] ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly identifies a redundant `until` loop due to an existing `depends_on` condition, and it also points out a duplicated path in an `rm` command, improving startup efficiency and correctness. </details></details></td><td align=center>Low </td></tr><tr><td> <details><summary>Simplify the service startup command</summary> ___ **Simplify the <code>cert-generator</code> service's startup command by directly executing the script from its mount point, removing the need to copy and change permissions.** [docker-compose.yml [10]](https://github.com/carverauto/serviceradar/pull/2090/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R10-R10) ```diff -command: sh -c "apk add --no-cache openssl bash && cp /generate-certs.sh /tmp/gen.sh && chmod +x /tmp/gen.sh && CERT_DIR=/certs bash /tmp/gen.sh" +command: sh -c "apk add --no-cache openssl bash && CERT_DIR=/certs bash /generate-certs.sh" ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 4</summary> __ Why: The suggestion offers a valid simplification of the `cert-generator` command, improving readability and removing an unnecessary file copy operation. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>