Skip to content

Conversation

@aayushchouhan09
Copy link
Member

@aayushchouhan09 aayushchouhan09 commented Oct 14, 2025

Describe the Problem

AGENT_CONFIG was supported as environment variable, not as volume-mounted secrets. Also the agent logs exposed sensitive configuration data.

Explain the Changes

  1. Added support for reading AGENT_CONFIG from volume-mounted secrets
  2. Removed sensitive data logging from agent pod logs

Issues: Fixed #xxx / Gap #xxx

  1. Bug: https://issues.redhat.com/browse/DFBUGS-2687
  2. Operator PR: Secure agent_config by using volume mounts instead of env vars noobaa-operator#1718

Testing Instructions:

  • Doc added/updated
  • Tests added

Summary by CodeRabbit

  • Bug Fixes
    • Improved agent configuration initialization with stricter validation and clearer error messages.
    • Now exits early if a valid configuration already exists, and resolves config from environment or default locations.
    • Supports raw JSON or base64-encoded JSON safely: only writes a new config after successful validation/decoding, and fails fast on invalid or missing config.

@aayushchouhan09 aayushchouhan09 requested review from a team, dannyzaken, jackyalbo, liranmauda and nbecker-cibot and removed request for a team October 14, 2025 14:11
@coderabbitai
Copy link

coderabbitai bot commented Oct 14, 2025

Walkthrough

noobaa_init.sh now early-exits if an existing agent_conf.json is present; otherwise it resolves AGENT_CONFIG from the environment or the file at AGENT_CONFIG_PATH (default /etc/agent-config/agent_config), validates JSON with jq, and if invalid attempts base64 decode before writing; explicit errors are emitted for missing or invalid config.

Changes

Cohort / File(s) Summary
AGENT_CONFIG initialization logic
src/deploy/NVA_build/noobaa_init.sh
Add early return when agent_conf.json exists; introduce AGENT_CONFIG_PATH (default /etc/agent-config/agent_config) and resolve AGENT_CONFIG from env or that file; exit if empty with a message referencing the path; validate AGENT_CONFIG via jq and write if valid; if not valid, attempt base64 decode and write; exit with clear errors if decode/validation fail.

Sequence Diagram(s)

sequenceDiagram
    participant Script as noobaa_init.sh
    participant FS as Filesystem
    participant ENV as Environment
    participant JQ as jq
    participant DEC as base64-decode

    Script->>FS: stat / check `agent_conf.json`
    alt exists
        FS-->>Script: file present
        Script->>Script: exit (early)
    else not exists
        Script->>ENV: read AGENT_CONFIG or file at AGENT_CONFIG_PATH (/etc/agent-config/agent_config)
        alt AGENT_CONFIG empty
            ENV-->>Script: empty/missing
            Script->>Script: exit with "missing AGENT_CONFIG" error (mentions path)
        else AGENT_CONFIG present
            ENV-->>Script: config payload
            Script->>JQ: attempt JSON parse
            alt jq success
                JQ-->>Script: valid JSON
                Script->>FS: write `agent_conf.json`
            else jq fails
                JQ-->>Script: invalid JSON
                Script->>DEC: attempt base64 decode
                alt decode success
                    DEC-->>Script: decoded JSON
                    Script->>FS: write `agent_conf.json`
                else decode fails
                    DEC-->>Script: decode failed
                    Script->>Script: exit with "invalid AGENT_CONFIG format" error
                end
            end
        end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Review branching for early exit and AGENT_CONFIG resolution.
  • Verify JSON validation and base64-decode error paths and messages.
  • Check file write atomicity/permissions and referenced default path.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Support AGENT_CONFIG from volume mounts and remove secret logging" directly aligns with the stated PR objectives and the changes implemented in the changeset. The first part of the title corresponds to the added support for reading AGENT_CONFIG from volume-mounted secrets, which is reflected in the changes to noobaa_init.sh that introduce AGENT_CONFIG_PATH resolution and file-based configuration reading. The second part, "remove secret logging," matches the objective to eliminate sensitive data exposure in agent pod logs. The title is concise, specific, and clearly communicates the two primary improvements without vague terminology or unnecessary details.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2367740 and 5b2fd04.

📒 Files selected for processing (2)
  • config.js (1 hunks)
  • src/deploy/NVA_build/noobaa_init.sh (1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
src/deploy/NVA_build/noobaa_init.sh

[warning] 128-128: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.

(SC2164)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Build Noobaa Image
  • GitHub Check: run-package-lock-validation
  • GitHub Check: run-jest-unit-tests
🔇 Additional comments (1)
config.js (1)

245-250: LGTM! Clean implementation following existing patterns.

The AGENT_CONFIG section follows the same pattern as other secret configurations (JWT_SECRET, SERVER_SECRET). Using _get_data_from_file is appropriate and handles missing files gracefully by returning undefined, which the shell script later validates.

@nimrod-becker
Copy link
Contributor

@dannyzaken @jackyalbo @tangledbytes PTAL

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 17b14ea and fb12872.

📒 Files selected for processing (1)
  • src/deploy/NVA_build/noobaa_init.sh (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: run-package-lock-validation
  • GitHub Check: Build Noobaa Image
  • GitHub Check: run-jest-unit-tests
🔇 Additional comments (1)
src/deploy/NVA_build/noobaa_init.sh (1)

129-135: Variable quoting and early-exit validation are properly addressed.

Lines 132–135 now correctly quote ${AGENT_CONFIG} in the test and provide a clear error message when the configuration is missing. Both past quoting concerns have been resolved.

Copy link
Member

@tangledbytes tangledbytes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/deploy/NVA_build/noobaa_init.sh (1)

138-143: Consider validating base64-decoded JSON before writing.

The current flow attempts JSON parsing first, then falls back to base64 decoding. However, after a successful base64 decode (line 139), the script doesn't validate that the decoded content is valid JSON. If AGENT_CONFIG is valid base64 that decodes to invalid JSON, the script will succeed but write corrupted config. The agent will then fail downstream.

Consider piping the decoded output through jq validation before writing to ensure end-to-end correctness.

Proposed diff:

  if ! echo "${AGENT_CONFIG}" | jq . >"$AGENT_CONF_FILE" 2>/dev/null; then
-   openssl enc -base64 -d -A <<<"${AGENT_CONFIG}" >"$AGENT_CONF_FILE" || {
+   if ! openssl enc -base64 -d -A <<<"${AGENT_CONFIG}" | jq . >"$AGENT_CONF_FILE" 2>/dev/null; then
      echo "AGENT_CONFIG format is invalid. AGENT_CONFIG must be valid JSON or base64 encoded JSON. Exit"
      exit 1
-   }
+   fi
  fi
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3297bd8 and b9bdfcf.

📒 Files selected for processing (1)
  • src/deploy/NVA_build/noobaa_init.sh (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Build Noobaa Image
  • GitHub Check: run-jest-unit-tests
  • GitHub Check: run-package-lock-validation
🔇 Additional comments (1)
src/deploy/NVA_build/noobaa_init.sh (1)

123-144: Changes look good—previous issues addressed and PR objectives met.

The function now correctly:

  • Supports reading AGENT_CONFIG from volume-mounted secrets via AGENT_CONFIG_PATH (line 130)
  • Removes unconditional logging of the raw config (no direct echo of AGENT_CONFIG)
  • Quotes variables properly (line 132, 139)
  • Handles errors from openssl base64 decode (lines 139–142)
  • Provides clear error messages linking to expected sources (lines 133, 140)

All three issues from previous review comments are resolved.

@aayushchouhan09 aayushchouhan09 merged commit a38bc13 into noobaa:master Oct 28, 2025
18 checks passed
@aayushchouhan09 aayushchouhan09 deleted the agent-fix branch October 28, 2025 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants