Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion data/live-build-config/hooks/live/40-init-geoip-database.chroot
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,24 @@ DATE_SUFFIX=$(date +%Y-%m)
URL="https://download.db-ip.com/free/dbip-country-lite-${DATE_SUFFIX}.csv.gz"
OUT_PATH="/usr/share/vyos-geoip/dbip-country-lite.csv.gz"

ASN_URL="https://download.db-ip.com/free/dbip-asn-lite-${DATE_SUFFIX}.csv.gz"
ASN_OUT_PATH="/usr/share/vyos-geoip/dbip-asn-lite.csv.gz"

mkdir -p $(dirname $OUT_PATH)
wget -O - $URL > $OUT_PATH

if [ $? -ne 0 ]; then
echo "Failed to download GeoIP database"
echo "Failed to download GeoIP country-code database"
rm $OUT_PATH
fi
Comment on lines 16 to 19

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail fast when either GeoIP download fails.

Right now the hook logs and removes the partial file, but still continues to Line 30. That can leave the image in a partially initialized state. Make each failure path terminate with non-zero status.

Suggested patch
-mkdir -p $(dirname $OUT_PATH)
-curl -sSfL -o $OUT_PATH $URL
-
-if [ $? -ne 0 ]; then
+mkdir -p "$(dirname "$OUT_PATH")"
+if ! curl -sSfL -o "$OUT_PATH" "$URL"; then
     echo "Failed to download GeoIP country-code database"
-    rm $OUT_PATH
+    rm -f "$OUT_PATH"
+    exit 1
 fi
 
-mkdir -p $(dirname $ASN_OUT_PATH)
-curl -sSfL -o $ASN_OUT_PATH $ASN_URL
-
-if [ $? -ne 0 ]; then
+mkdir -p "$(dirname "$ASN_OUT_PATH")"
+if ! curl -sSfL -o "$ASN_OUT_PATH" "$ASN_URL"; then
     echo "Failed to download GeoIP ASN database"
-    rm $ASN_OUT_PATH
+    rm -f "$ASN_OUT_PATH"
+    exit 1
 fi

Also applies to: 24-27

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@data/live-build-config/hooks/live/40-init-geoip-database.chroot` around lines
16 - 19, The GeoIP download error handling at lines 16-19 logs the failure and
removes the partial file but fails to exit with a non-zero status, allowing the
script to continue and potentially leave the image in a partially initialized
state. After the rm $OUT_PATH command in the error handling block, add exit 1 to
terminate the script with a failure status. This same fix needs to be applied to
the other GeoIP download failure path mentioned at lines 24-27 as well, ensuring
all download failure paths terminate immediately with a non-zero exit code.


mkdir -p $(dirname $ASN_OUT_PATH)
wget -O - $ASN_URL > $ASN_OUT_PATH
Comment thread
sarthurdev marked this conversation as resolved.
Outdated

if [ $? -ne 0 ]; then
echo "Failed to download GeoIP ASN database"
rm $ASN_OUT_PATH
fi

# Generate sqlite database
python3 /usr/libexec/vyos/geoip-update.py --init
1 change: 1 addition & 0 deletions data/live-build-config/rootfs/excludes
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ usr/local/games/*

# T5511: We do not need any caches on the system (will be recreated when needed).
# T7278: We need directory created by python3-cracklib for password checks
# T7926: VyOS GeoIP database location
var/cache/!(cracklib|vyos)
Comment on lines +48 to 49

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify the T7926 comment to match the actual path context.

Line 48 says “VyOS GeoIP database location” in a /var/cache exclusion block, while the downloader writes to /usr/share/vyos-geoip in data/live-build-config/hooks/live/40-init-geoip-database.chroot (Lines 8 and 11). Please reword this to explicitly describe why /var/cache/vyos is preserved here.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@data/live-build-config/rootfs/excludes` around lines 48 - 49, The comment on
line 48 labeled "T7926: VyOS GeoIP database location" is misleading because the
exclusion pattern `var/cache/!(cracklib|vyos)` actually preserves the
`/var/cache/vyos` directory (the vyos cache directory), not the GeoIP database
itself which is located elsewhere at `/usr/share/vyos-geoip`. Reword the comment
next to the exclusion pattern to explicitly clarify that it preserves the vyos
cache directory by changing the description to something that accurately
reflects why `/var/cache/vyos` is being kept in the exclusion.


# T5511: We do not need any log-files on the system (will be recreated when needed).
Expand Down
Loading