carverauto/serviceradar

Fork 0

2102 bugdocker docker compose stack not seeding kv with configs #2543

Merged

mfreeman451 merged 4 commits from refs/pull/2543/head into main

2025-12-11 06:41:43 +00:00

mfreeman451 commented

2025-12-11 06:41:17 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2104
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2104
Original created: 2025-12-11T06:41:17Z
Original updated: 2025-12-11T06:43:06Z
Original head: carverauto/serviceradar:2102-bugdocker-docker-compose-stack-not-seeding-kv-with-configs
Original base: main
Original merged: 2025-12-11T06:41:43Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

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

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

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

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

PR Type

Bug fix, Enhancement

Description

Enable KV-backed configuration in Docker Compose services to seed defaults into datasvc on first boot
Fix db-event-writer consumer initialization to retry on transient errors instead of exiting
Prevent zen consumer from restarting on initial KV watch event by skipping first sync
Add NATS context setup script and debugging tools to serviceradar-tools image
Update all service images to v1.0.67 and add comprehensive KV environment variables

Diagram Walkthrough

flowchart LR
  A["Docker Compose Services"] -->|CONFIG_SOURCE=kv| B["KV-backed Configuration"]
  B -->|seed defaults| C["datasvc bucket"]
  C -->|publish snapshots| D["Watcher Telemetry"]
  E["db-event-writer"] -->|retry on error| F["Resilient Consumer Init"]
  G["zen consumer"] -->|skip initial event| H["Stable Config Watch"]
  I["serviceradar-tools"] -->|bundled NATS context| J["Local Debugging"]

File Walkthrough

Relevant files

Bug fix

2 files

service.go `Implement resilient consumer initialization with retry logic`	+24/-20
main.rs `Skip initial KV watch event to prevent restart loop`	+6/-0

Enhancement

2 files

setup-nats-context.sh `Add NATS context configuration script for local debugging`	+46/-0
Dockerfile.tools `Bundle NATS context setup and debugging aliases for local tools`	+23/-8

Configuration changes

3 files

docker-compose.yml `Enable KV-backed config and update service images to v1.0.67`	+81/-28
docker-compose.podman.yml `Enable KV-backed config and update service images to v1.0.67`	+81/-28
poller-stack.compose.yml `Enable KV-backed configuration for poller stack agent service`	+8/-0

Documentation

5 files

tools-motd.txt `Update NATS context and certificate documentation in motd`	+4/-4
docker-setup.md `Document KV seeding verification and expected watcher telemetry`	+17/-0
proposal.md `Define KV seeding fix scope and impact analysis`	+15/-0
spec.md `Add Docker Compose KV bootstrap requirement specification`	+9/-0
tasks.md `Track implementation tasks for KV seeding fix`	+7/-0

Imported from GitHub pull request. Original GitHub pull request: #2104 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2104 Original created: 2025-12-11T06:41:17Z Original updated: 2025-12-11T06:43:06Z Original head: carverauto/serviceradar:2102-bugdocker-docker-compose-stack-not-seeding-kv-with-configs Original base: main Original merged: 2025-12-11T06:41:43Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix, Enhancement ___ ### **Description** - Enable KV-backed configuration in Docker Compose services to seed defaults into datasvc on first boot - Fix db-event-writer consumer initialization to retry on transient errors instead of exiting - Prevent zen consumer from restarting on initial KV watch event by skipping first sync - Add NATS context setup script and debugging tools to serviceradar-tools image - Update all service images to v1.0.67 and add comprehensive KV environment variables ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Docker Compose Services"] -->|CONFIG_SOURCE=kv| B["KV-backed Configuration"] B -->|seed defaults| C["datasvc bucket"] C -->|publish snapshots| D["Watcher Telemetry"] E["db-event-writer"] -->|retry on error| F["Resilient Consumer Init"] G["zen consumer"] -->|skip initial event| H["Stable Config Watch"] I["serviceradar-tools"] -->|bundled NATS context| J["Local Debugging"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><details><summary>2 files</summary><table> <tr> <td><strong>service.go</strong><dd><code>Implement resilient consumer initialization with retry logic</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2104/files#diff-9f9f48b11e7670c7ae374abc41327adf4617972b214f1c168e9da53d3cd7b609">+24/-20</a>  </td> </tr> <tr> <td><strong>main.rs</strong><dd><code>Skip initial KV watch event to prevent restart loop</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2104/files#diff-78494042ba6078c9704eeb6e374d97a3481a5a74426db8e95ed34da15f9133b2">+6/-0</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Enhancement</strong></td><td><details><summary>2 files</summary><table> <tr> <td><strong>setup-nats-context.sh</strong><dd><code>Add NATS context configuration script for local debugging</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2104/files#diff-201a8197495066d5f2189460b9c4a5fb49a423d11ec7c7a63ad575f12945b6a4">+46/-0</a>    </td> </tr> <tr> <td><strong>Dockerfile.tools</strong><dd><code>Bundle NATS context setup and debugging aliases for local tools</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2104/files#diff-0258db71e4070e342198965f1d046f3097640850b037df8a2287a7e239630add">+23/-8</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Configuration changes</strong></td><td><details><summary>3 files</summary><table> <tr> <td><strong>docker-compose.yml</strong><dd><code>Enable KV-backed config and update service images to v1.0.67</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2104/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3">+81/-28</a>  </td> </tr> <tr> <td><strong>docker-compose.podman.yml</strong><dd><code>Enable KV-backed config and update service images to v1.0.67</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2104/files#diff-77a7fc5fac1d23ddc900d0f5128aedde1fe22302ba66820a704a8e5d13a15dbb">+81/-28</a>  </td> </tr> <tr> <td><strong>poller-stack.compose.yml</strong><dd><code>Enable KV-backed configuration for poller stack agent service</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2104/files#diff-f3b5c991c2c1f7646db0ca4ed9bcb5df0f313ce6a05d8f3c890f80c873f776f5">+8/-0</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Documentation</strong></td><td><details><summary>5 files</summary><table> <tr> <td><strong>tools-motd.txt</strong><dd><code>Update NATS context and certificate documentation in motd</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2104/files#diff-b66b550fcd1772acb59e343e845225193c6b9601e78a29a1f124b7bcd9ca636a">+4/-4</a>      </td> </tr> <tr> <td><strong>docker-setup.md</strong><dd><code>Document KV seeding verification and expected watcher telemetry</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2104/files#diff-8604269dffb3ce4133e48cab374ca8e97745d0efbdef67cad792aeb5945fe5ec">+17/-0</a>    </td> </tr> <tr> <td><strong>proposal.md</strong><dd><code>Define KV seeding fix scope and impact analysis</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2104/files#diff-101e287b4968d5f8b7ad517d4b4e297f1220e566b5afa440b4f175635255061e">+15/-0</a>    </td> </tr> <tr> <td><strong>spec.md</strong><dd><code>Add Docker Compose KV bootstrap requirement specification</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2104/files#diff-b630673220a5759a8738dd243c6c56dcded486bc046bb2193b70c3ae57349c7e">+9/-0</a>      </td> </tr> <tr> <td><strong>tasks.md</strong><dd><code>Track implementation tasks for KV seeding fix</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/2104/files#diff-4eec00439bd8c35c743af0c8455b7797af00593c93c3016de2991b797453e8e5">+7/-0</a>      </td> </tr> </table></details></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-11 06:41:52 +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/2104#issuecomment-3640471855
Original created: 2025-12-11T06:41:52Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

⚪

CLI context misuse

Description: The script auto-creates and selects a NATS CLI context using filesystem TLS certs if
present, which could be misused interactively inside the tools container; ensure the
container does not mount broader host paths and that cert volumes are read-only as
configured.
setup-nats-context.sh [17-33]

Referred Code

if command -v nats >/dev/null 2>&1; then
  # Overwrite any stale context with the current endpoints/certs.
  if nats context ls >/dev/null 2>&1; then
    nats context rm "${CONTEXT_NAME}" >/dev/null 2>&1 || true
  fi

  nats context save "${CONTEXT_NAME}" \
    --description "${CONTEXT_DESC}" \
    --server "${NATS_URL}" \
    --tlsca "${NATS_CA}" \
    --tlscert "${NATS_CERT}" \
    --tlskey "${NATS_KEY}" \
    --select \
    >/dev/null 2>&1 || true

  echo "NATS context '${CONTEXT_NAME}' ready for ${NATS_URL}"
else

Secrets exposure via env

Description: Multiple services expose detailed KV mTLS certificate file paths via environment variables
which may be logged by container runtimes or crash reporters; confirm logging and
diagnostics do not leak sensitive paths or contents and that volumes are mounted read-only
as configured.
docker-compose.yml [543-559]

Referred Code

  - "50041:50041"
volumes:
  - ./docker/compose/db-event-writer.mtls.json:/etc/serviceradar/templates/db-event-writer.json:ro
  - cert-data:/etc/serviceradar/certs:ro
  - db-event-writer-data:/var/lib/serviceradar
  - db-event-writer-config:/etc/serviceradar/consumers
  - ./logs:/var/log/serviceradar
environment:
  - CONFIG_SOURCE=kv
  - CONFIG_PATH=/etc/serviceradar/templates/db-event-writer.json
  - KV_ADDRESS=datasvc:50057
  - KV_SEC_MODE=mtls
  - KV_CERT_DIR=/etc/serviceradar/certs
  - KV_CERT_FILE=/etc/serviceradar/certs/db-event-writer.pem
  - KV_KEY_FILE=/etc/serviceradar/certs/db-event-writer-key.pem
  - KV_CA_FILE=/etc/serviceradar/certs/root.pem
  - KV_SERVER_NAME=datasvc.serviceradar

Ticket Compliance

🟡

🎫 #2102

🟢	Ensure Docker Compose stack seeds configuration defaults into the KV (datasvc bucket) for each KV-managed service on first boot.
	Align Docker Compose behavior with Kubernetes where KV-backed configuration is already functioning.
	Provide documentation or guidance to verify KV seeding and watcher telemetry after first boot.
⚪	Verify at runtime that the datasvc KV bucket is actually populated on first boot in a clean environment and watcher telemetry shows expected services.

Codebase Duplication Compliance

⚪

Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance

🟢

Generic: Meaningful Naming and Self-Documenting Code

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

Status: Passed

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

Generic: Robust Error Handling and Edge Case Management

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

Status: Passed

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

⚪

Generic: Comprehensive Audit Trails

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

Status:
Logging context: New retry logic logs failures but it is unclear if critical actions (e.g., consumer
connection state changes) are captured with sufficient audit context across the system.

Referred Code

	s.logger.Info().
		Str("stream_name", s.cfg.StreamName).
		Str("consumer_name", s.cfg.ConsumerName).
		Msg("Connected to NATS consumer")
	return consumer, nil
}

// Treat transient connection/setup errors as retryable so we don't exit the process.
if ctx.Err() != nil {
	return nil, err
}

s.logger.Warn().Err(err).Msg("Failed to (re)establish NATS consumer; retrying")
if !sleepWithContext(ctx, s.retryDelay) {

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

Generic: Secure Error Handling

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

Status:
Verbose output: The script echoes connection details and certificate paths which may be sensitive in some
environments; verify this is not user-facing or exposed beyond internal debugging.

Referred Code

  echo "NATS context '${CONTEXT_NAME}' ready for ${NATS_URL}"
else
  # Fallback: write a JSON context file for manual selection.
  cat >"${CONTEXT_DIR}/${CONTEXT_NAME}.json" <<EOF
{
  "description": "${CONTEXT_DESC}",
  "url": "${NATS_URL}",
  "cert": "${NATS_CERT}",
  "key": "${NATS_KEY}",
  "ca": "${NATS_CA}",
  "inbox_prefix": "_INBOX"
}
EOF
  echo "nats CLI not found; wrote context to ${CONTEXT_DIR}/${CONTEXT_NAME}.json"

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 env: Environment variables such as CNPG credentials and KV certificate paths are configured;
ensure these are not logged at any log level by services when CONFIG_SOURCE=kv is enabled.

Referred Code

- CONFIG_SOURCE=kv
- CONFIG_PATH=/etc/serviceradar/templates/db-event-writer.json
- KV_ADDRESS=datasvc:50057
- KV_SEC_MODE=mtls
- KV_CERT_DIR=/etc/serviceradar/certs
- KV_CERT_FILE=/etc/serviceradar/certs/db-event-writer.pem
- KV_KEY_FILE=/etc/serviceradar/certs/db-event-writer-key.pem
- KV_CA_FILE=/etc/serviceradar/certs/root.pem
- KV_SERVER_NAME=datasvc.serviceradar
- LOG_LEVEL=${LOG_LEVEL:-info}
- 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}
- CNPG_CERT_DIR=/etc/serviceradar/certs

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:
Watcher logic: Skipping the first KV watch event may be correct, but verify that configuration
initialization and validation still handle empty or malformed initial states securely.

Referred Code

let mut is_initial = true;
while cfg_watcher.recv().await.is_some() {
    if is_initial {
        // First event is the current value; don't restart on initial sync.
        is_initial = false;
        continue;
    }
    restarter.trigger();
}

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/2104#issuecomment-3640471855 Original created: 2025-12-11T06:41:52Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td rowspan=2>⚪</td> <td><details><summary><strong>CLI context misuse </strong></summary><br> <b>Description:</b> The script auto-creates and selects a NATS CLI context using filesystem TLS certs if <br>present, which could be misused interactively inside the tools container; ensure the <br>container does not mount broader host paths and that cert volumes are read-only as <br>configured.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2104/files#diff-201a8197495066d5f2189460b9c4a5fb49a423d11ec7c7a63ad575f12945b6a4R17-R33'>setup-nats-context.sh [17-33]</a></strong><br> <details open><summary>Referred Code</summary> ```shell if command -v nats >/dev/null 2>&1; then # Overwrite any stale context with the current endpoints/certs. if nats context ls >/dev/null 2>&1; then nats context rm "${CONTEXT_NAME}" >/dev/null 2>&1 || true fi nats context save "${CONTEXT_NAME}" \ --description "${CONTEXT_DESC}" \ --server "${NATS_URL}" \ --tlsca "${NATS_CA}" \ --tlscert "${NATS_CERT}" \ --tlskey "${NATS_KEY}" \ --select \ >/dev/null 2>&1 || true echo "NATS context '${CONTEXT_NAME}' ready for ${NATS_URL}" else ``` </details></details></td></tr> <tr><td><details><summary><strong>Secrets exposure via env</strong></summary><br> <b>Description:</b> Multiple services expose detailed KV mTLS certificate file paths via environment variables <br>which may be logged by container runtimes or crash reporters; confirm logging and <br>diagnostics do not leak sensitive paths or contents and that volumes are mounted read-only <br>as configured.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/2104/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R543-R559'>docker-compose.yml [543-559]</a></strong><br> <details open><summary>Referred Code</summary> ```yaml - "50041:50041" volumes: - ./docker/compose/db-event-writer.mtls.json:/etc/serviceradar/templates/db-event-writer.json:ro - cert-data:/etc/serviceradar/certs:ro - db-event-writer-data:/var/lib/serviceradar - db-event-writer-config:/etc/serviceradar/consumers - ./logs:/var/log/serviceradar environment: - CONFIG_SOURCE=kv - CONFIG_PATH=/etc/serviceradar/templates/db-event-writer.json - KV_ADDRESS=datasvc:50057 - KV_SEC_MODE=mtls - KV_CERT_DIR=/etc/serviceradar/certs - KV_CERT_FILE=/etc/serviceradar/certs/db-event-writer.pem - KV_KEY_FILE=/etc/serviceradar/certs/db-event-writer-key.pem - KV_CA_FILE=/etc/serviceradar/certs/root.pem - KV_SERVER_NAME=datasvc.serviceradar ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2102>#2102</a></summary> <table width='100%'><tbody> <tr><td rowspan=3>🟢</td> <td>Ensure Docker Compose stack seeds configuration defaults into the KV (datasvc bucket) for <br>each KV-managed service on first boot.</td></tr> <tr><td>Align Docker Compose behavior with Kubernetes where KV-backed configuration is already <br>functioning.</td></tr> <tr><td>Provide documentation or guidance to verify KV seeding and watcher telemetry after first <br>boot.</td></tr> <tr><td rowspan=1>⚪</td> <td>Verify at runtime that the datasvc KV bucket is actually populated on first boot in a <br>clean environment and watcher telemetry shows expected services.</td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=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: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td 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/2104/files#diff-9f9f48b11e7670c7ae374abc41327adf4617972b214f1c168e9da53d3cd7b609R188-R201'><strong>Logging context</strong></a>: New retry logic logs failures but it is unclear if critical actions (e.g., consumer <br>connection state changes) are captured with sufficient audit context across the system.<br> <details open><summary>Referred Code</summary> ```go s.logger.Info(). Str("stream_name", s.cfg.StreamName). Str("consumer_name", s.cfg.ConsumerName). Msg("Connected to NATS consumer") return consumer, nil } // Treat transient connection/setup errors as retryable so we don't exit the process. if ctx.Err() != nil { return nil, err } s.logger.Warn().Err(err).Msg("Failed to (re)establish NATS consumer; retrying") if !sleepWithContext(ctx, s.retryDelay) { ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: 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/2104/files#diff-201a8197495066d5f2189460b9c4a5fb49a423d11ec7c7a63ad575f12945b6a4R32-R45'><strong>Verbose output</strong></a>: The script echoes connection details and certificate paths which may be sensitive in some <br>environments; verify this is not user-facing or exposed beyond internal debugging.<br> <details open><summary>Referred Code</summary> ```shell echo "NATS context '${CONTEXT_NAME}' ready for ${NATS_URL}" else # Fallback: write a JSON context file for manual selection. cat >"${CONTEXT_DIR}/${CONTEXT_NAME}.json" <<EOF { "description": "${CONTEXT_DESC}", "url": "${NATS_URL}", "cert": "${NATS_CERT}", "key": "${NATS_KEY}", "ca": "${NATS_CA}", "inbox_prefix": "_INBOX" } EOF echo "nats CLI not found; wrote context to ${CONTEXT_DIR}/${CONTEXT_NAME}.json" ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2104/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R551-R567'><strong>Secrets in env</strong></a>: Environment variables such as CNPG credentials and KV certificate paths are configured; <br>ensure these are not logged at any log level by services when CONFIG_SOURCE=kv is enabled.<br> <details open><summary>Referred Code</summary> ```yaml - CONFIG_SOURCE=kv - CONFIG_PATH=/etc/serviceradar/templates/db-event-writer.json - KV_ADDRESS=datasvc:50057 - KV_SEC_MODE=mtls - KV_CERT_DIR=/etc/serviceradar/certs - KV_CERT_FILE=/etc/serviceradar/certs/db-event-writer.pem - KV_KEY_FILE=/etc/serviceradar/certs/db-event-writer-key.pem - KV_CA_FILE=/etc/serviceradar/certs/root.pem - KV_SERVER_NAME=datasvc.serviceradar - LOG_LEVEL=${LOG_LEVEL:-info} - 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} - CNPG_CERT_DIR=/etc/serviceradar/certs ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2104/files#diff-78494042ba6078c9704eeb6e374d97a3481a5a74426db8e95ed34da15f9133b2R75-R83'><strong>Watcher logic</strong></a>: Skipping the first KV watch event may be correct, but verify that configuration <br>initialization and validation still handle empty or malformed initial states securely.<br> <details open><summary>Referred Code</summary> ```rust let mut is_initial = true; while cfg_watcher.recv().await.is_some() { if is_initial { // First event is the current value; don't restart on initial sync. is_initial = false; continue; } restarter.trigger(); } ``` </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<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>

qodo-code-review[bot] commented

2025-12-11 06:43:06 +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/2104#issuecomment-3640476028
Original created: 2025-12-11T06:43:06Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Remove duplicate environment variable definition In `docker/compose/poller-stack.compose.yml`, the `agent` service has `CONFIG_SOURCE` defined twice. Remove the second definition (`CONFIG_SOURCE=file`) to ensure the intended `kv` setting is used. docker/compose/poller-stack.compose.yml [103-113] `- CONFIG_SOURCE=kv - CONFIG_PATH=/etc/serviceradar/config/agent.json - KV_ADDRESS=datasvc:50057 - KV_SEC_MODE=mtls - KV_CERT_DIR=/etc/serviceradar/certs - KV_CERT_FILE=/etc/serviceradar/certs/agent.pem - KV_KEY_FILE=/etc/serviceradar/certs/agent-key.pem - KV_CA_FILE=/etc/serviceradar/certs/root.pem - KV_SERVER_NAME=datasvc.serviceradar - LOG_LEVEL=${LOG_LEVEL:-info} -- CONFIG_SOURCE=file` Apply / Chat Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical bug where a duplicate `CONFIG_SOURCE` variable would cause the service to use `file` instead of `kv`, directly contradicting the PR's main objective.	High
General	Update outdated image tag in documentation Update the `serviceradar-tools` image tag from `v1.0.65` to `v1.0.67` in the `docker-setup.md` documentation to match the version used elsewhere in the PR. docs/docs/docker-setup.md [154-161] `docker run --rm --network serviceradar_serviceradar-net \ -v serviceradar_cert-data:/etc/serviceradar/certs \ - ghcr.io/carverauto/serviceradar-tools:${APP_TAG:-v1.0.65} \ + ghcr.io/carverauto/serviceradar-tools:${APP_TAG:-v1.0.67} \ nats --server tls://nats:4222 \ --tlsca /etc/serviceradar/certs/root.pem \ --tlscert /etc/serviceradar/certs/poller.pem \ --tlskey /etc/serviceradar/certs/poller-key.pem \ kv ls serviceradar-datasvc` Apply / Chat Suggestion importance[1-10]: 5 __ Why: The suggestion correctly points out an outdated image tag in the documentation, which could cause confusion or errors for users following the instructions. Updating it improves documentation accuracy and consistency with the PR's changes.	Low
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2104#issuecomment-3640476028 Original created: 2025-12-11T06:43:06Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Remove duplicate environment variable definition</summary> ___ **In <code>docker/compose/poller-stack.compose.yml</code>, the <code>agent</code> service has <code>CONFIG_SOURCE</code> <br>defined twice. Remove the second definition (<code>CONFIG_SOURCE=file</code>) to ensure the <br>intended <code>kv</code> setting is used.** [docker/compose/poller-stack.compose.yml [103-113]](https://github.com/carverauto/serviceradar/pull/2104/files#diff-f3b5c991c2c1f7646db0ca4ed9bcb5df0f313ce6a05d8f3c890f80c873f776f5R103-R113) ```diff - CONFIG_SOURCE=kv - CONFIG_PATH=/etc/serviceradar/config/agent.json - KV_ADDRESS=datasvc:50057 - KV_SEC_MODE=mtls - KV_CERT_DIR=/etc/serviceradar/certs - KV_CERT_FILE=/etc/serviceradar/certs/agent.pem - KV_KEY_FILE=/etc/serviceradar/certs/agent-key.pem - KV_CA_FILE=/etc/serviceradar/certs/root.pem - KV_SERVER_NAME=datasvc.serviceradar - LOG_LEVEL=${LOG_LEVEL:-info} -- CONFIG_SOURCE=file ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies a critical bug where a duplicate `CONFIG_SOURCE` variable would cause the service to use `file` instead of `kv`, directly contradicting the PR's main objective. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Update outdated image tag in documentation</summary> ___ **Update the <code>serviceradar-tools</code> image tag from <code>v1.0.65</code> to <code>v1.0.67</code> in the <br><code>docker-setup.md</code> documentation to match the version used elsewhere in the PR.** [docs/docs/docker-setup.md [154-161]](https://github.com/carverauto/serviceradar/pull/2104/files#diff-8604269dffb3ce4133e48cab374ca8e97745d0efbdef67cad792aeb5945fe5ecR154-R161) ```diff docker run --rm --network serviceradar_serviceradar-net \ -v serviceradar_cert-data:/etc/serviceradar/certs \ - ghcr.io/carverauto/serviceradar-tools:${APP_TAG:-v1.0.65} \ + ghcr.io/carverauto/serviceradar-tools:${APP_TAG:-v1.0.67} \ nats --server tls://nats:4222 \ --tlsca /etc/serviceradar/certs/root.pem \ --tlscert /etc/serviceradar/certs/poller.pem \ --tlskey /etc/serviceradar/certs/poller-key.pem \ kv ls serviceradar-datasvc ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: The suggestion correctly points out an outdated image tag in the documentation, which could cause confusion or errors for users following the instructions. Updating it improves documentation accuracy and consistency with the PR's changes. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>