[chores] Added loud failure message if existing database is detected #562#568
[chores] Added loud failure message if existing database is detected #562#568Srinath0916 wants to merge 1 commit intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthroughAdds a pre-flight guard and conditional secret handling in deploy/auto-install.sh within setup_docker_openwisp. When no Sequence Diagram(s)sequenceDiagram
participant User
participant AutoInstall as "auto-install.sh"
participant FS as "Filesystem (.env / env_backup)"
participant Docker as "Docker Engine"
participant PostgresVol as "docker-openwisp_postgres_data"
User->>AutoInstall: run setup_docker_openwisp (no env_path or missing .env)
AutoInstall->>FS: check INSTALL_PATH/.env
FS-->>AutoInstall: .env missing
AutoInstall->>FS: check ENV_BACKUP
FS-->>AutoInstall: ENV_BACKUP present? / absent?
alt ENV_BACKUP absent
AutoInstall->>Docker: inspect volumes for docker-openwisp_postgres_data
Docker-->>PostgresVol: locate volume
PostgresVol-->>Docker: exists
Docker-->>AutoInstall: volume exists
AutoInstall->>User: log critical message and exit (code 1)
else ENV_BACKUP present
AutoInstall->>FS: read ENV_BACKUP -> DB_USER, DB_PASS, DJANGO_SECRET_KEY
FS-->>AutoInstall: credentials restored
AutoInstall->>AutoInstall: skip regenerating DB creds and secret key
AutoInstall->>User: continue with restored config
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/auto-install.sh (1)
127-143:⚠️ Potential issue | 🟠 MajorGuard fires after the full config wizard and
git clone— move it before prompting the userThe new check at lines 134–143 sits after
download_docker_openwisp(line 127), which means the user already:
- Answered all five domain / SSL / email prompts (lines 107–121), and
- Waited for a potentially slow
git clone(line 97 insidedownload_docker_openwisp)…before being told the installation cannot continue. In the worst case the clone also wipes
$INSTALL_PATHviarm -rf, making subsequent retries start clean from scratch while the volumes are still intact — another confusing state.Moving the guard to the very top of the
if [[ ! -f "$env_path" ]]block (before the first config prompt) fixes the UX and, as a bonus, makes the! -f "$INSTALL_PATH/.env"sub-condition meaningful: at that earlier point it correctly checks whether a previous installation's.envis still in place. In the current placement,$INSTALL_PATH/.envis always absent (freshly cloned repo never ships one), so only the! -f "$ENV_BACKUP"branch provides real protection.Additionally, the suggested remediation command (
docker compose down --volumes) does not specify a working directory. At the point the abort fires,$INSTALL_PATHhas been freshly cloned and holds the compose files, so the message should direct the user there.🛠️ Proposed fix — move guard before the config prompts and add directory context
if [[ ! -f "$env_path" ]]; then + # Pre-flight: abort before any prompts if existing DB volumes would break new credentials + if [[ ! -f "$INSTALL_PATH/.env" ]] && [[ ! -f "$ENV_BACKUP" ]]; then + if docker volume ls -q | grep -q "postgres_data"; then + echo -e "${RED}CRITICAL: Existing database data detected!${NON}" + echo -e "New credentials will break your connection to existing volumes." + echo -e "\nTo fix, either restore your .env or run:" + echo -e "${YLW}cd ${INSTALL_PATH} && docker compose down --volumes${NON}" + echo -e "\nAborting to prevent authentication failure." + exit 1 + fi + fi # Dashboard Domain echo -ne ${GRN}"(1/5) Enter dashboard domain: "${NON} read dashboard_domainThen remove the duplicate block that was at lines 134–143 (after
download_docker_openwisp):if [[ ! -f "$env_path" ]]; then - if [[ ! -f "$INSTALL_PATH/.env" ]] && [[ ! -f "$ENV_BACKUP" ]]; then - if docker volume ls -q | grep -q "postgres_data"; then - echo -e "${RED}CRITICAL: Existing database data detected!${NON}" - echo -e "New credentials will break your connection to existing volumes." - echo -e "\nTo fix, either restore your .env or run:" - echo -e "${YLW}docker compose down --volumes${NON}" - echo -e "\nAborting to prevent authentication failure." - exit 1 - fi - fi # Dashboard Domain set_env "DASHBOARD_DOMAIN" "$dashboard_domain"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/auto-install.sh` around lines 127 - 143, The guard that checks for existing postgres volume runs after download_docker_openwisp and the interactive prompts, causing wasted prompts/clone; move the entire volume-check block (the if [[ ! -f "$env_path" ]] ... check that inspects docker volume ls -q | grep -q "postgres_data" and exits) to before any prompting logic (i.e., before the domain/SSL/email prompts and before calling download_docker_openwisp) so the script aborts early; update the error text to include the installation directory context (referencing $INSTALL_PATH in the message) and remove the duplicate guard block that remains after download_docker_openwisp to avoid redundant checks.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
deploy/auto-install.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:43.808Z
Learning: In the docker-openwisp Makefile, `include .env` is intentionally mandatory (not `-include .env`) because the .env file contains critical configurations that must be present for safe operation. Silent failures with empty values would be more dangerous than failing explicitly when the file is missing.
Learnt from: pandafy
Repo: openwisp/docker-openwisp PR: 549
File: docker-compose.yml:171-171
Timestamp: 2026-01-06T11:28:41.789Z
Learning: In docker-openwisp, the OpenVPN container (images/common/init_command.sh) exits silently with exit code 0 when VPN_DOMAIN environment variable is not set. Combined with restart policy "on-failure" in docker-compose.yml, this prevents the container from restarting when VPN is disabled.
📚 Learning: 2026-02-17T16:59:43.808Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:43.808Z
Learning: In the docker-openwisp Makefile, `include .env` is intentionally mandatory (not `-include .env`) because the .env file contains critical configurations that must be present for safe operation. Silent failures with empty values would be more dangerous than failing explicitly when the file is missing.
Applied to files:
deploy/auto-install.sh
📚 Learning: 2026-02-17T17:08:12.144Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: .env:7-9
Timestamp: 2026-02-17T17:08:12.144Z
Learning: In docker-openwisp, OPENWISP_VERSION=edge is intentionally set as the default in .env to maintain consistency with the existing TAG=edge default in the Makefile. Production users are expected to override OPENWISP_VERSION in their .env file to pin to a specific stable release tag.
Applied to files:
deploy/auto-install.sh
⏰ 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). (1)
- GitHub Check: CI Build
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/auto-install.sh`:
- Line 135: The current check using `docker volume ls -q | grep -q
"postgres_data"` can match substrings and produce false positives; replace it
with an exact existence check using `docker volume inspect` against the
Compose-derived volume name `docker-openwisp_postgres_data` (derived from the
hardcoded install path `/opt/openwisp/docker-openwisp`) and branch on the
inspect command's exit status so only the exact OpenWISP Postgres volume
triggers the path in the install script.
---
Outside diff comments:
In `@deploy/auto-install.sh`:
- Around line 127-143: The guard that checks for existing postgres volume runs
after download_docker_openwisp and the interactive prompts, causing wasted
prompts/clone; move the entire volume-check block (the if [[ ! -f "$env_path" ]]
... check that inspects docker volume ls -q | grep -q "postgres_data" and exits)
to before any prompting logic (i.e., before the domain/SSL/email prompts and
before calling download_docker_openwisp) so the script aborts early; update the
error text to include the installation directory context (referencing
$INSTALL_PATH in the message) and remove the duplicate guard block that remains
after download_docker_openwisp to avoid redundant checks.
Srinath0916
force-pushed
the
issues/562-fatal-auth-path-fix
branch
from
701838a to
e1f27e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
deploy/auto-install.sh
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:43.808Z
Learning: In the docker-openwisp Makefile, `include .env` is intentionally mandatory (not `-include .env`) because the .env file contains critical configurations that must be present for safe operation. Silent failures with empty values would be more dangerous than failing explicitly when the file is missing.
📚 Learning: 2026-02-17T16:59:43.808Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:43.808Z
Learning: In the docker-openwisp Makefile, `include .env` is intentionally mandatory (not `-include .env`) because the .env file contains critical configurations that must be present for safe operation. Silent failures with empty values would be more dangerous than failing explicitly when the file is missing.
Applied to files:
deploy/auto-install.sh
⏰ 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). (1)
- GitHub Check: CI Build
🔇 Additional comments (1)
deploy/auto-install.sh (1)
106-115:docker volume inspectexact-match correctly implemented; hardcoded volume name verifiedThe previous review's suggestion to replace
docker volume ls -q | grep -q "postgres_data"with an exactdocker volume inspect "docker-openwisp_postgres_data"call has been correctly applied. The exit-code-based check is precise.Verification confirms:
- The postgres volume is named
postgres_dataindocker-compose.ymland will correctly becomedocker-openwisp_postgres_datawith the default project name.COMPOSE_PROJECT_NAMEis not hardcoded anywhere in the repository.The guard operates correctly for standard deployments. An edge case remains: if a user explicitly sets
COMPOSE_PROJECT_NAMEin their runtime environment or shell (outside the repository), the hardcoded volume name will no longer match and the guard will not detect the existing volume. This is a known limitation documented in your comment—not a code defect but a configuration scenario users should be aware of.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/auto-install.sh`:
- Around line 108-113: The abort block writes only to stdout and bypasses the
common logging path; change it to call error_msg() (or at minimum tee the
critical summary line into ${LOG_FILE}) so the failure is recorded in
${LOG_FILE} and includes the log reference; also make the suggested mitigation
robust by not relying solely on cd ${INSTALL_PATH} — keep the existing "cd
${INSTALL_PATH} && docker compose down --volumes" but add a path-independent
fallback that runs docker volume rm docker-openwisp_postgres_data (or equivalent
named volume) so volumes are removed even if INSTALL_PATH was deleted; update
the block that currently echoes the messages to use error_msg/error logging and
include the fallback volume remove command as described, referencing error_msg,
${LOG_FILE}, and ${INSTALL_PATH}.
e1f27e8 to
bd59264
Compare
|
Hey @nemesifier @pandafy , I've implemented the fix we discussed. The script now detects existing postgres volumes and aborts with a red warning instead of silently breaking the setup. No automatic volume deletion, just clear instructions for the user to handle it manually. |
nemesifier
changed the title
Srinath0916
force-pushed
the
issues/562-fatal-auth-path-fix
branch
from
bd59264 to
0736986
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/auto-install.sh`:
- Around line 106-119: The installer currently treats the presence of
$ENV_BACKUP as bypassing the critical .env safety check, which can lead
setup_docker_openwisp() to generate new DB credentials against the existing
docker-openwisp_postgres_data volume and break auth; change the logic so that if
$INSTALL_PATH/.env is missing the script either (a) aborts immediately with the
same critical error message (do not treat $ENV_BACKUP as a safe bypass), or (b)
restores $ENV_BACKUP into $INSTALL_PATH/.env before continuing; update the
branch around the INSTALL_PATH/.env / ENV_BACKUP check to implement one of these
two behaviors and ensure subsequent steps that generate credentials (the code
path currently referenced by setup_docker_openwisp()) run only after a live .env
has been restored or the script has aborted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6bdb4e0a-45a3-43af-ab12-ed68d11f1036
📒 Files selected for processing (1)
deploy/auto-install.sh
📜 Review details
⏰ 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). (1)
- GitHub Check: CI Build
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:43.808Z
Learning: In the docker-openwisp Makefile, `include .env` is intentionally mandatory (not `-include .env`) because the .env file contains critical configurations that must be present for safe operation. Silent failures with empty values would be more dangerous than failing explicitly when the file is missing.
📚 Learning: 2026-02-17T16:59:43.808Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:43.808Z
Learning: In the docker-openwisp Makefile, `include .env` is intentionally mandatory (not `-include .env`) because the .env file contains critical configurations that must be present for safe operation. Silent failures with empty values would be more dangerous than failing explicitly when the file is missing.
Applied to files:
deploy/auto-install.sh
Srinath0916
force-pushed
the
issues/562-fatal-auth-path-fix
branch
from
0736986 to
e05acc2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/auto-install.sh`:
- Around line 106-118: The script currently only aborts when INSTALL_PATH/.env
is missing and a docker volume "docker-openwisp_postgres_data" exists, but
misses the case where .env exists and will later be moved by
download_docker_openwisp() to ENV_BACKUP while DB credentials are regenerated;
change the logic to explicitly detect an existing .env plus an existing docker
volume and immediately abort with the same error/log message (use
INSTALL_PATH/.env and docker-openwisp_postgres_data checks), or alternatively
avoid moving INSTALL_PATH/.env to ENV_BACKUP in download_docker_openwisp() when
that docker volume exists; ensure any code that regenerates DB credentials (the
block around the DB credential generation lines) is skipped if the abort
condition is met so you never overwrite credentials tied to the existing volume.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80c02c5b-3d28-4189-918c-5d3857f15adf
📒 Files selected for processing (1)
deploy/auto-install.sh
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:43.808Z
Learning: In the docker-openwisp Makefile, `include .env` is intentionally mandatory (not `-include .env`) because the .env file contains critical configurations that must be present for safe operation. Silent failures with empty values would be more dangerous than failing explicitly when the file is missing.
📚 Learning: 2026-02-17T16:59:43.808Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:43.808Z
Learning: In the docker-openwisp Makefile, `include .env` is intentionally mandatory (not `-include .env`) because the .env file contains critical configurations that must be present for safe operation. Silent failures with empty values would be more dangerous than failing explicitly when the file is missing.
Applied to files:
deploy/auto-install.sh
deploy/auto-install.sh
Outdated
| if [[ ! -f "$INSTALL_PATH/.env" ]]; then | ||
| if docker volume inspect "docker-openwisp_postgres_data" &>/dev/null; then | ||
| { | ||
| echo -e "${RED}CRITICAL: Existing database data detected!${NON}" | ||
| echo -e "New credentials will break your connection to existing volumes." | ||
| echo -e "\nTo fix, either restore your .env or run:" | ||
| echo -e "${YLW}cd ${INSTALL_PATH} && docker compose down --volumes${NON}" | ||
| echo -e "or directly: ${YLW}docker volume rm docker-openwisp_postgres_data${NON}" | ||
| echo -e "\nAborting to prevent authentication failure." | ||
| echo -e "${RED}Check logs at $LOG_FILE${NON}" | ||
| } | tee -a "$LOG_FILE" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
This still misses the reinstall path where .env exists at prompt time.
If --install is rerun without env_path while $INSTALL_PATH/.env still exists, Line 106 skips the abort. But download_docker_openwisp() later moves that file to $ENV_BACKUP (Lines 85-88), and the ad-hoc branch still regenerates DB credentials (Lines 175-176) instead of restoring them. With an existing docker-openwisp_postgres_data volume, this recreates the same auth failure the PR is trying to prevent.
🛠️ Minimal safe fix
- if [[ ! -f "$env_path" ]]; then
- if [[ ! -f "$INSTALL_PATH/.env" ]]; then
- if docker volume inspect "docker-openwisp_postgres_data" &>/dev/null; then
+ if [[ ! -f "$env_path" ]]; then
+ if docker volume inspect "docker-openwisp_postgres_data" &>/dev/null; then
{
echo -e "${RED}CRITICAL: Existing database data detected!${NON}"
echo -e "New credentials will break your connection to existing volumes."
echo -e "\nTo fix, either restore your .env or run:"
echo -e "${YLW}cd ${INSTALL_PATH} && docker compose down --volumes${NON}"
echo -e "or directly: ${YLW}docker volume rm docker-openwisp_postgres_data${NON}"
echo -e "\nAborting to prevent authentication failure."
echo -e "${RED}Check logs at $LOG_FILE${NON}"
} | tee -a "$LOG_FILE"
exit 1
- fi
fiBased on learnings, the .env file carries critical configuration, so failing explicitly is safer than continuing after it will be sidelined and regenerated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/auto-install.sh` around lines 106 - 118, The script currently only
aborts when INSTALL_PATH/.env is missing and a docker volume
"docker-openwisp_postgres_data" exists, but misses the case where .env exists
and will later be moved by download_docker_openwisp() to ENV_BACKUP while DB
credentials are regenerated; change the logic to explicitly detect an existing
.env plus an existing docker volume and immediately abort with the same
error/log message (use INSTALL_PATH/.env and docker-openwisp_postgres_data
checks), or alternatively avoid moving INSTALL_PATH/.env to ENV_BACKUP in
download_docker_openwisp() when that docker volume exists; ensure any code that
regenerates DB credentials (the block around the DB credential generation lines)
is skipped if the abort condition is met so you never overwrite credentials tied
to the existing volume.
e05acc2 to
35cfe01
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| echo -e "or directly: ${YLW}docker volume rm docker-openwisp_postgres_data${NON}" | ||
| echo -e "\nAborting to prevent authentication failure." | ||
| echo -e "${RED}Check logs at $LOG_FILE${NON}" | ||
| } | tee -a "$LOG_FILE" |
There was a problem hiding this comment.
WARNING: ANSI escape codes will be written to the log file
The tee -a "$LOG_FILE" call pipes the raw output — including ANSI color codes (\033[1;31m, \033[1;33m, \033[0m) — directly into the log file. When users inspect the log with tail -n 50 -f /opt/openwisp/autoinstall.log (as documented in docs/user/quickstart.rst), they will see garbled escape sequences instead of readable text.
Strip the color codes before writing to the log:
| } | tee -a "$LOG_FILE" | |
| } | tee >(sed 's/\x1b\[[0-9;]*m//g' >> "$LOG_FILE") # strips ANSI color codes |
Alternatively, use script -q -c or simply write a plain-text version to the log separately.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issue alignment: The fix correctly addresses issue #562 — it detects an existing Pandafy's testing (scenario 1): The reviewer confirmed that when re-running the installer on an existing working installation (where Pandafy's testing (scenario 2): The guard correctly catches dangling volumes from a previous installation where Checklist gaps: The PR author's checklist shows both "written new test cases" and "updated the documentation" are unchecked. Per the project's custom review policy, both are required for a bug fix. Significance: The change is minimal (26 lines) and directly prevents a data-loss-adjacent failure mode. The fix is proportionate to the problem. Files Reviewed (1 file)
|
nemesifier
changed the title
nemesifier
changed the title
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
During manual testing of the auto-install.sh script, I tested the following two scenarios to verify how the new safeguard behaves.
1. Executing auto-install script on an existing OpenWISP installation
Steps to replicate
- Install OpenWISP using
./deploy/auto-install.sh. - Allow the installation to complete successfully.
- Run
./deploy/auto-install.shagain on the same system. - When prompted for
.env, leave the input blank (ad-hoc configuration).
Actual outcome
The script exits early with the following message:
OpenWISP Configuration:
OpenWISP Version (leave blank for latest):
Do you have .env file? Enter filepath (leave blank for ad-hoc configuration):
CRITICAL: Existing database data detected!
New credentials will break your connection to existing volumes.
To fix, either restore your .env or run:
cd /opt/openwisp/docker-openwisp && docker compose down --volumes
or directly: docker volume rm docker-openwisp_postgres_data
Aborting to prevent authentication failure.
Check logs at /opt/openwisp/autoinstall.log
Expected outcome
The script should prevent accidental credential regeneration (which it does), but ideally it should still allow the user to reconfigure or reuse the existing installation if the .env file already exists in the installation directory.
Currently, the script blocks execution entirely even though the installation is valid and running.
2. Executing auto-install script with dangling volumes from previous installation
Steps to replicate
- Install OpenWISP using
./deploy/auto-install.sh. - Stop the containers using:
docker compose down. - Remove the installation directory:
rm -rf /opt/openwisp. - Ensure the Docker volume from the previous installation still exists.
- Run the auto-install script again:
./deploy/auto-install.sh. - Leave the
.envprompt blank.
Actual outcome
The script exits early with the same "Existing database data detected" message because the PostgreSQL volume still exists.
Expected outcome
In this scenario the script correctly detects the existing database volume and prevents new credentials from being generated. It shows the correct commands for recovering.
Conclusion
The safeguard added in this PR correctly prevents accidental regeneration of database credentials when a PostgreSQL volume already exists. However, the current implementation completely blocks the auto-install script from proceeding, which means users cannot reconfigure or recover an installation even in legitimate scenarios.
It may be worth refining the logic so that the script:
- Prevents credential regeneration, but
- Still allows to re-configure the existing installation (e.g. allow changing DNS hostname for services)
deploy/auto-install.sh
Outdated
| if docker volume inspect "docker-openwisp_postgres_data" &>/dev/null; then | ||
| { | ||
| echo -e "${RED}CRITICAL: Existing database data detected!${NON}" | ||
| echo -e "New credentials will break your connection to existing volumes." | ||
| echo -e "\nTo fix, either restore your .env or run:" | ||
| echo -e "${YLW}cd ${INSTALL_PATH} && docker compose down --volumes${NON}" | ||
| echo -e "or directly: ${YLW}docker volume rm docker-openwisp_postgres_data${NON}" | ||
| echo -e "\nAborting to prevent authentication failure." | ||
| echo -e "${RED}Check logs at $LOG_FILE${NON}" | ||
| } | tee -a "$LOG_FILE" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
@Srinath0916 @nemesifier what do you think about the following message?
Yes, it is a bit long, but it cover all the bases.
CRITICAL: Existing database volume detected!
The Docker volume "docker-openwisp_postgres_data" already exists on this system.
This likely means there is database data from a previous OpenWISP installation.
The auto-install script generates new database credentials during fresh
installations. If it proceeds while this volume exists, the newly generated
credentials will not match the credentials stored in the existing database,
making the database inaccessible to OpenWISP.
⚠️ WARNING: The commands below will permanently delete the database volume
and all stored data. Run them only if you intentionally want to wipe the
previous installation or have a verified backup.
Proceed at your own discretion.
Cleanup commands:
cd /opt/openwisp/docker-openwisp && docker compose down --volumes
or
docker volume rm docker-openwisp_postgres_data
Aborting installation to prevent credential mismatch.
Check logs at /opt/openwisp/autoinstall.log
There was a problem hiding this comment.
@pandafy That's much clearer and covers all as you mentioned. I've updated it with your message. Thanks for the detailed feedback.
Srinath0916
force-pushed
the
issues/562-fatal-auth-path-fix
branch
from
b37cea6 to
bbe07e8
Compare
Thanks for testing this thoroughly, The current code should actually handle the first case correctly, it only aborts when there's no existing .env AND volumes exist. So if someone reruns the installer on a working installation (where .env is already there), it should back up and restore that .env instead of regenerating credentials. The second case is working as intended, it catches the dangling volume situation and shows the cleanup commands. |
deploy/auto-install.sh
Outdated
| echo -ne ${GRN}"Do you have .env file? Enter filepath (leave blank for ad-hoc configuration): "${NON} | ||
| read env_path | ||
| if [[ ! -f "$env_path" ]]; then | ||
| if [[ ! -f "$INSTALL_PATH/.env" ]] && docker volume inspect "docker-openwisp_postgres_data" &>/dev/null; then |
There was a problem hiding this comment.
WARNING: Missing $ENV_BACKUP check causes false-positive abort
The condition only tests ! -f "$INSTALL_PATH/.env" but not ! -f "$ENV_BACKUP". The download_docker_openwisp function (line 86) moves $INSTALL_PATH/.env to $ENV_BACKUP before cloning. If a previous run was interrupted after that move but before the new .env was written, the credentials are safely preserved in $ENV_BACKUP — yet this guard will still fire and abort with a "CRITICAL" message, blocking a legitimate recovery.
The condition should also check for the backup:
| if [[ ! -f "$INSTALL_PATH/.env" ]] && docker volume inspect "docker-openwisp_postgres_data" &>/dev/null; then | |
| if [[ ! -f "$INSTALL_PATH/.env" ]] && [[ ! -f "$ENV_BACKUP" ]] && docker volume inspect "docker-openwisp_postgres_data" &>/dev/null; then |
There was a problem hiding this comment.
@Srinath0916 @pandafy do you think this suggestion is valid?
There was a problem hiding this comment.
@kilo-code-bot[bot] the installation follow does not handle restoring of ENV_BACKUP file. It is only done for upgrades.
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
The current code should actually handle the first case correctly, it only aborts when there's no existing .env AND volumes exist. So if someone reruns the installer on a working installation (where .env is already there), it should back up and restore that .env instead of regenerating credentials.
@Srinath0916 the password changed when I invoked the auto-install script for the second time. I followed the exact steps that I shared in my previous review.
Steps to replicate
- Install OpenWISP using ./deploy/auto-install.sh.
- Allow the installation to complete successfully.
- Run ./deploy/auto-install.sh again on the same system.
- When prompted for .env, leave the input blank (ad-hoc configuration).
From my quick debugging, I concluded that ENV_BACKUP is only used for upgrades and not re-installs.
I think we should update the auto-install script to not change the credentials if they are already present in the .env file.
docker-openwisp/deploy/auto-install.sh
Lines 186 to 188 in bbe07e8
deploy/auto-install.sh
Outdated
| echo -ne ${GRN}"Do you have .env file? Enter filepath (leave blank for ad-hoc configuration): "${NON} | ||
| read env_path | ||
| if [[ ! -f "$env_path" ]]; then | ||
| if [[ ! -f "$INSTALL_PATH/.env" ]] && docker volume inspect "docker-openwisp_postgres_data" &>/dev/null; then |
There was a problem hiding this comment.
@kilo-code-bot[bot] the installation follow does not handle restoring of ENV_BACKUP file. It is only done for upgrades.
Prevents credential mismatch when reinstalling without clearing volumes. The script now detects existing postgres_data volumes and aborts with a clear warning instead of silently generating new credentials that break database connectivity. Fixes openwisp#562
Srinath0916
force-pushed
the
issues/562-fatal-auth-path-fix
branch
from
bbe07e8 to
fc9869a
Compare
|
@pandafy , updated the fix to restore credentials from ENV_BACKUP instead of regenerating them on reinstall. Let me know if this matches what you had in mind. |
pandafy
left a comment
pandafy
left a comment
There was a problem hiding this comment.
The current code should actually handle the first case correctly, it only aborts when there's no existing .env AND volumes exist. So if someone reruns the installer on a working installation (where .env is already there), it should back up and restore that .env instead of regenerating credentials.
@Srinath0916 the password changed when I invoked the auto-install script for the second time. I followed the exact steps that I shared in my previous review.
Steps to replicate
- Install OpenWISP using ./deploy/auto-install.sh.
- Allow the installation to complete successfully.
- Run ./deploy/auto-install.sh again on the same system.
- When prompted for .env, leave the input blank (ad-hoc configuration).
From my initial testing, I confirm that this issue has been resolved.
| # Set random secret values only if no previous credentials exist | ||
| if [[ ! -f "$ENV_BACKUP" ]]; then | ||
| python3 $INSTALL_PATH/build.py change-secret-key >/dev/null | ||
| python3 $INSTALL_PATH/build.py change-database-credentials >/dev/null |
There was a problem hiding this comment.
Can you help me understand why did you chose to rely on checking if the backup.env is present? Why didn't we just check if the variables are set in the .env file?
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/auto-install.sh`:
- Line 106: The script currently treats presence of ENV_BACKUP as sufficient to
restore .env; update the logic so before bypassing the abort/restore path you
validate that ENV_BACKUP actually contains non-empty DB_USER, DB_PASS and
DJANGO_SECRET_KEY (use the existing get_env helper to read those keys from
ENV_BACKUP and verify each is non-empty). Only if all three values are present
should you proceed to copy or source ENV_BACKUP into INSTALL_PATH/.env;
otherwise fail with an explicit abort and do not create a blank .env. Apply the
same validation where ENV_BACKUP is used later in the restore flow.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4bd0cbc8-c4ee-4f33-91ad-e9dc2114e9d2
📒 Files selected for processing (1)
deploy/auto-install.sh
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:43.808Z
Learning: In the docker-openwisp Makefile, `include .env` is intentionally mandatory (not `-include .env`) because the .env file contains critical configurations that must be present for safe operation. Silent failures with empty values would be more dangerous than failing explicitly when the file is missing.
📚 Learning: 2026-02-17T16:59:43.808Z
Learnt from: atif09
Repo: openwisp/docker-openwisp PR: 555
File: Makefile:4-5
Timestamp: 2026-02-17T16:59:43.808Z
Learning: In the docker-openwisp Makefile, `include .env` is intentionally mandatory (not `-include .env`) because the .env file contains critical configurations that must be present for safe operation. Silent failures with empty values would be more dangerous than failing explicitly when the file is missing.
Applied to files:
deploy/auto-install.sh
| echo -ne ${GRN}"Do you have .env file? Enter filepath (leave blank for ad-hoc configuration): "${NON} | ||
| read env_path | ||
| if [[ ! -f "$env_path" ]]; then | ||
| if [[ ! -f "$INSTALL_PATH/.env" ]] && [[ ! -f "$ENV_BACKUP" ]] && docker volume inspect "docker-openwisp_postgres_data" &>/dev/null; then |
There was a problem hiding this comment.
Validate backup.env before using it as the safe bypass.
Line 106 treats backup.env existence as enough proof that recovery is safe, but Lines 191-194 assume it actually contains DB_USER, DB_PASS, and DJANGO_SECRET_KEY. If that file is partial or stale, get_env returns empty values and the new .env gets blank credentials, recreating the same broken install with a less obvious cause. Only bypass the abort / restore path when all three keys are present.
🛠️ Proposed fix
if [[ ! -f "$env_path" ]]; then
+ backup_has_required_secrets=true
+ if [[ -f "$ENV_BACKUP" ]]; then
+ for config in DB_USER DB_PASS DJANGO_SECRET_KEY; do
+ [[ -n "$(get_env "$config" "$ENV_BACKUP")" ]] || backup_has_required_secrets=false
+ done
+ fi
- if [[ ! -f "$INSTALL_PATH/.env" ]] && [[ ! -f "$ENV_BACKUP" ]] && docker volume inspect "docker-openwisp_postgres_data" &>/dev/null; then
+ if [[ ! -f "$INSTALL_PATH/.env" ]] && { [[ ! -f "$ENV_BACKUP" ]] || [[ "$backup_has_required_secrets" != true ]]; } && docker volume inspect "docker-openwisp_postgres_data" &>/dev/null; then
{
...
} | tee -a "$LOG_FILE"
exit 1
fi
...
- if [[ ! -f "$ENV_BACKUP" ]]; then
+ if [[ ! -f "$ENV_BACKUP" ]] || [[ "$backup_has_required_secrets" != true ]]; then
python3 $INSTALL_PATH/build.py change-secret-key >/dev/null
python3 $INSTALL_PATH/build.py change-database-credentials >/dev/null
else
for config in DB_USER DB_PASS DJANGO_SECRET_KEY; do
value=$(get_env "$config" "$ENV_BACKUP")
set_env "$config" "$value"
done
fiBased on learnings, the .env file contains critical configurations that must be present for safe operation, and silent empty values are more dangerous than failing explicitly.
Also applies to: 186-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/auto-install.sh` at line 106, The script currently treats presence of
ENV_BACKUP as sufficient to restore .env; update the logic so before bypassing
the abort/restore path you validate that ENV_BACKUP actually contains non-empty
DB_USER, DB_PASS and DJANGO_SECRET_KEY (use the existing get_env helper to read
those keys from ENV_BACKUP and verify each is non-empty). Only if all three
values are present should you proceed to copy or source ENV_BACKUP into
INSTALL_PATH/.env; otherwise fail with an explicit abort and do not create a
blank .env. Apply the same validation where ENV_BACKUP is used later in the
restore flow.
3 participants
Checklist
Reference to Existing Issue
Closes #562
Description of Changes
The auto-install script was silently regenerating database credentials on every run, which broke existing setups when users reinstalled without clearing volumes. This caused the "FATAL: password authentication failed" error reported in #562.
I've added a simple check that detects when postgres volumes exist but the .env file is missing. When this happens, the script now stops and shows a clear warning with instructions on how to proceed safely.
The fix follows the approach discussed in the issue - no automatic volume deletion, just a red warning and the command users need to run manually if they want to start fresh.
Screenshot
N/A