refactor: allow custom db connection in deploy.sh#649
refactor: allow custom db connection in deploy.sh#649yu-teo wants to merge 3 commits intoopendatahub-io:mainfrom
deploy.sh#649Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yu-teo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThese changes introduce support for external PostgreSQL databases in the deployment flow. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Security concerns
Action: apply the fixes above before accepting secrets from untrusted inputs or exposing credentials via process args. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
scripts/deploy.sh (1)
684-691: Validation only checks URL scheme prefix; malformed URLs pass through.The regex
^postgres(ql)?://accepts strings likepostgresql://(empty host/db) orpostgres://user:@:5432/(missing components). While downstream PostgreSQL will reject these, failing early with a clearer message improves UX.Optional: More thorough URL validation
validate_postgres_connection() { local conn="$1" if [[ ! "$conn" =~ ^postgres(ql)?:// ]]; then log_error "Invalid PostgreSQL connection string format" log_error "Expected: postgresql://USER:PASSWORD@HOST:PORT/DATABASE?sslmode=require" exit 1 fi + # Basic structure check: scheme://something@host/database + if [[ ! "$conn" =~ ^postgres(ql)?://[^@]+@[^/]+/.+ ]]; then + log_warn "Connection string may be incomplete (expected user@host/database)" + fi }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/deploy.sh` around lines 684 - 691, The validate_postgres_connection function currently only checks the scheme; update it to validate the full connection string structure by ensuring the conn variable contains user, password, host, port and database segments (e.g. user:pass@host:port/db) and optionally query params like sslmode; modify the regex or parsing logic inside validate_postgres_connection to require non-empty username/password, host, numeric port and database name (and fail with a clear log_error message if any part is missing), so malformed URLs like "postgresql://", "postgres://user:@:5432/" or strings missing components are rejected early.scripts/deployment-helpers.sh (1)
1387-1401: Missing parameter validation; potential YAML injection via special characters in connection URL.The function doesn't validate that
namespaceandconnection_urlare non-empty. If called with empty values,kubectl apply -n ""may target the wrong namespace.Additionally, if the connection URL contains YAML special characters (e.g.,
:,#, or newlines), the heredoc could produce malformed YAML. Consider quoting the value or usingkubectl create secretwith--from-literal.Proposed fix
create_maas_db_config_secret() { - local namespace="$1" - local connection_url="$2" + local namespace="${1:?namespace is required}" + local connection_url="${2:?connection_url is required}" - kubectl apply -n "$namespace" -f - <<EOF + kubectl create secret generic maas-db-config \ + -n "$namespace" \ + --from-literal="DB_CONNECTION_URL=${connection_url}" \ + --dry-run=client -o yaml | \ + kubectl apply -f - --server-side=true -apiVersion: v1 -kind: Secret -metadata: - name: maas-db-config - labels: - app: maas-api -stringData: - DB_CONNECTION_URL: "${connection_url}" -EOF }The
--from-literalapproach properly escapes special characters. Add--dry-run=client -o yaml | kubectl applyto preserve the idempotent apply behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/deployment-helpers.sh` around lines 1387 - 1401, The create_maas_db_config_secret function lacks validation and is vulnerable to YAML injection via the heredoc; validate that both namespace and connection_url are non-empty (return non-zero and log an error if missing) and replace the raw heredoc with a safe creation pattern: use kubectl create secret generic maas-db-config --from-literal=DB_CONNECTION_URL="$connection_url" --dry-run=client -o yaml | kubectl apply -n "$namespace" -f - so special characters are escaped and apply remains idempotent; ensure the implementation references the existing function name create_maas_db_config_secret and checks the same parameters before running kubectl.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/setup-database.sh`:
- Around line 161-163: The DB connection URL built into DB_CONNECTION_URL embeds
the raw POSTGRES_PASSWORD which can contain URL-reserved characters (e.g., @, :,
/, ?); update the script to URL-encode POSTGRES_PASSWORD before constructing
DB_CONNECTION_URL so the resulting postgresql://... URL is valid, then pass the
encoded value into create_maas_db_config_secret; locate the password usage
around the DB_CONNECTION_URL variable and the create_maas_db_config_secret call
to apply the encoding.
---
Nitpick comments:
In `@scripts/deploy.sh`:
- Around line 684-691: The validate_postgres_connection function currently only
checks the scheme; update it to validate the full connection string structure by
ensuring the conn variable contains user, password, host, port and database
segments (e.g. user:pass@host:port/db) and optionally query params like sslmode;
modify the regex or parsing logic inside validate_postgres_connection to require
non-empty username/password, host, numeric port and database name (and fail with
a clear log_error message if any part is missing), so malformed URLs like
"postgresql://", "postgres://user:@:5432/" or strings missing components are
rejected early.
In `@scripts/deployment-helpers.sh`:
- Around line 1387-1401: The create_maas_db_config_secret function lacks
validation and is vulnerable to YAML injection via the heredoc; validate that
both namespace and connection_url are non-empty (return non-zero and log an
error if missing) and replace the raw heredoc with a safe creation pattern: use
kubectl create secret generic maas-db-config
--from-literal=DB_CONNECTION_URL="$connection_url" --dry-run=client -o yaml |
kubectl apply -n "$namespace" -f - so special characters are escaped and apply
remains idempotent; ensure the implementation references the existing function
name create_maas_db_config_secret and checks the same parameters before running
kubectl.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 1411e4b8-8385-4e31-aeab-4d1d79011a96
📒 Files selected for processing (4)
docs/content/install/maas-setup.mdscripts/deploy.shscripts/deployment-helpers.shscripts/setup-database.sh
| # Create the maas-db-config secret used by maas-api | ||
| DB_CONNECTION_URL="postgresql://${POSTGRES_USER}:${POSTGRES_PASSWORD}@postgres:5432/${POSTGRES_DB}?sslmode=disable" | ||
| create_maas_db_config_secret "$NAMESPACE" "$DB_CONNECTION_URL" |
There was a problem hiding this comment.
Externally provided POSTGRES_PASSWORD may contain URL-special characters.
Line 67 generates a safe password when not provided, but if POSTGRES_PASSWORD is set via environment variable and contains @, :, /, or ?, the connection URL will be malformed.
Proposed fix: URL-encode the password
# Create the maas-db-config secret used by maas-api
+# URL-encode password to handle special characters
+ENCODED_PASSWORD=$(python3 -c "import urllib.parse; print(urllib.parse.quote('${POSTGRES_PASSWORD}', safe=''))" 2>/dev/null || echo "${POSTGRES_PASSWORD}")
-DB_CONNECTION_URL="postgresql://${POSTGRES_USER}:${POSTGRES_PASSWORD}@postgres:5432/${POSTGRES_DB}?sslmode=disable"
+DB_CONNECTION_URL="postgresql://${POSTGRES_USER}:${ENCODED_PASSWORD}@postgres:5432/${POSTGRES_DB}?sslmode=disable"
create_maas_db_config_secret "$NAMESPACE" "$DB_CONNECTION_URL"Alternatively, document that POSTGRES_PASSWORD must not contain URL-reserved characters.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Create the maas-db-config secret used by maas-api | |
| DB_CONNECTION_URL="postgresql://${POSTGRES_USER}:${POSTGRES_PASSWORD}@postgres:5432/${POSTGRES_DB}?sslmode=disable" | |
| create_maas_db_config_secret "$NAMESPACE" "$DB_CONNECTION_URL" | |
| # Create the maas-db-config secret used by maas-api | |
| # URL-encode password to handle special characters | |
| ENCODED_PASSWORD=$(python3 -c "import urllib.parse; print(urllib.parse.quote('${POSTGRES_PASSWORD}', safe=''))" 2>/dev/null || echo "${POSTGRES_PASSWORD}") | |
| DB_CONNECTION_URL="postgresql://${POSTGRES_USER}:${ENCODED_PASSWORD}@postgres:5432/${POSTGRES_DB}?sslmode=disable" | |
| create_maas_db_config_secret "$NAMESPACE" "$DB_CONNECTION_URL" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup-database.sh` around lines 161 - 163, The DB connection URL
built into DB_CONNECTION_URL embeds the raw POSTGRES_PASSWORD which can contain
URL-reserved characters (e.g., @, :, /, ?); update the script to URL-encode
POSTGRES_PASSWORD before constructing DB_CONNECTION_URL so the resulting
postgresql://... URL is valid, then pass the encoded value into
create_maas_db_config_secret; locate the password usage around the
DB_CONNECTION_URL variable and the create_maas_db_config_secret call to apply
the encoding.
|
@yu-teo: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
https://redhat.atlassian.net/browse/RHOAIENG-52044
Description
Added
create_maas_db_config_secret()helper function that creates themaas-db-configSecret with a given connection URL. All of those changes tie into updatedscripts/deploy.shto allowPOSTGRES_CONNECTIONconfig variable with--postgres-connectionflag.Additionally, I have added validation of the postgras URL to fail early.
For clarity, I have added a warning banner like so:
How Has This Been Tested?
Merge criteria:
Summary by CodeRabbit
New Features
Documentation