From d996e9c9eee0f075ffa844044d86a62932193374 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 24 Jan 2019 09:28:09 +0100 Subject: [PATCH 01/12] Fix inconsistent handling of binary variable. get_binary_variable() stores the result into a global variable we pass later on as argument to FTLinstall() and define a local variable with the same name. This is fixed by only using the globa variable in all places not. This is still not a very elegant solution but it is also not subject of the current PR. Signed-off-by: DL6ER --- automated install/basic-install.sh | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/automated install/basic-install.sh b/automated install/basic-install.sh index 990b1f34..f91363c9 100755 --- a/automated install/basic-install.sh +++ b/automated install/basic-install.sh @@ -115,6 +115,9 @@ else OVER="\\r\\033[K" fi +# Define global binary variable +binary="tbd" + # A simple function that just echoes out our logo in ASCII format # This lets users know that it is a Pi-hole, LLC product show_ascii_berry() { @@ -2130,7 +2133,6 @@ clone_or_update_repos() { # Download FTL binary to random temp directory and install FTL binary FTLinstall() { # Local, named variables - local binary="${1}" local latesttag local str="Downloading and Installing FTL" printf " %b %s..." "${INFO}" "${str}" @@ -2377,7 +2379,7 @@ FTLdetect() { printf "\\n %b FTL Checks...\\n\\n" "${INFO}" if FTLcheckUpdate ; then - FTLinstall "${binary}" || return 1 + FTLinstall || return 1 fi } From 3cdd6204c562254b9368ce5b3153fe99bdf788a2 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 24 Jan 2019 09:31:02 +0100 Subject: [PATCH 02/12] Move dnsmasq disabling and config file rewriting into a dedicated subroutine Signed-off-by: DL6ER --- automated install/basic-install.sh | 40 ++++++++++++++++-------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/automated install/basic-install.sh b/automated install/basic-install.sh index f91363c9..092dafef 100755 --- a/automated install/basic-install.sh +++ b/automated install/basic-install.sh @@ -2184,25 +2184,6 @@ FTLinstall() { popd > /dev/null || { printf "Unable to return to original directory after FTL binary download.\\n"; return 1; } # Install the FTL service printf "%b %b %s\\n" "${OVER}" "${TICK}" "${str}" - # dnsmasq can now be stopped and disabled if it exists - if which dnsmasq &> /dev/null; then - if check_service_active "dnsmasq";then - printf " %b FTL can now resolve DNS Queries without dnsmasq running separately\\n" "${INFO}" - stop_service dnsmasq - disable_service dnsmasq - fi - fi - - # Backup existing /etc/dnsmasq.conf if present and ensure that - # /etc/dnsmasq.conf contains only "conf-dir=/etc/dnsmasq.d" - local conffile="/etc/dnsmasq.conf" - if [[ -f "${conffile}" ]]; then - printf " %b Backing up %s to %s.old\\n" "${INFO}" "${conffile}" "${conffile}" - mv "${conffile}" "${conffile}.old" - fi - # Create /etc/dnsmasq.conf - echo "conf-dir=/etc/dnsmasq.d" > "${conffile}" - return 0 # Otherwise, else @@ -2222,6 +2203,27 @@ FTLinstall() { fi } +disable_dnsmasq() { + # dnsmasq can now be stopped and disabled if it exists + if which dnsmasq &> /dev/null; then + if check_service_active "dnsmasq";then + printf " %b FTL can now resolve DNS Queries without dnsmasq running separately\\n" "${INFO}" + stop_service dnsmasq + disable_service dnsmasq + fi + fi + + # Backup existing /etc/dnsmasq.conf if present and ensure that + # /etc/dnsmasq.conf contains only "conf-dir=/etc/dnsmasq.d" + local conffile="/etc/dnsmasq.conf" + if [[ -f "${conffile}" ]]; then + printf " %b Backing up %s to %s.old\\n" "${INFO}" "${conffile}" "${conffile}" + mv "${conffile}" "${conffile}.old" + fi + # Create /etc/dnsmasq.conf + echo "conf-dir=/etc/dnsmasq.d" > "${conffile}" +} + get_binary_name() { # This gives the machine architecture which may be different from the OS architecture... local machine From de6dc90575df32800155fb2ecd3c075503388bb0 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 24 Jan 2019 09:31:53 +0100 Subject: [PATCH 03/12] Don't diable pihole-FTL before calling install. root should be able to overwrite the binary in any case. The binary has already been downloaded and hash-verified here so we don't expect any errors in this binary. Signed-off-by: DL6ER --- automated install/basic-install.sh | 2 -- 1 file changed, 2 deletions(-) diff --git a/automated install/basic-install.sh b/automated install/basic-install.sh index 092dafef..03214f1a 100755 --- a/automated install/basic-install.sh +++ b/automated install/basic-install.sh @@ -2176,8 +2176,6 @@ FTLinstall() { # If we downloaded binary file (as opposed to text), if sha1sum --status --quiet -c "${binary}".sha1; then printf "transferred... " - # Stop FTL - stop_service pihole-FTL &> /dev/null # Install the new version with the correct permissions install -T -m 0755 "${binary}" /usr/bin/pihole-FTL # Move back into the original directory the user was in From d90d7b69273997be0285e79417b36a02f99f1485 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 24 Jan 2019 09:36:27 +0100 Subject: [PATCH 04/12] Call FTL download/installation early on in the installation process. Signed-off-by: DL6ER --- automated install/basic-install.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/automated install/basic-install.sh b/automated install/basic-install.sh index 03214f1a..83abc49e 100755 --- a/automated install/basic-install.sh +++ b/automated install/basic-install.sh @@ -1909,8 +1909,9 @@ installPihole() { installCron # Install the logrotate file installLogrotate - # Check if FTL is installed - FTLdetect || printf " %b FTL Engine not installed\\n" "${CROSS}" + # Check if dnsmasq is present. If so, disable it and back up any possible + # config file + disable_dnsmasq # Configure the firewall if [[ "${useUpdateVars}" == false ]]; then configureFirewall @@ -2539,6 +2540,8 @@ main() { else LIGHTTPD_ENABLED=false fi + # Check if FTL is installed - do this early on as FTL is a hard dependency for Pi-hole + FTLdetect || printf " %b FTL Engine not installed\\n" "${CROSS}" # Install and log everything to a file installPihole | tee -a /proc/$$/fd/3 From cc657c0c263485c6c16e476db29236027ce0f598 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 24 Jan 2019 09:37:03 +0100 Subject: [PATCH 05/12] Rename subroutine start_service() to restart_service() because this is what it does Signed-off-by: DL6ER --- automated install/basic-install.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/automated install/basic-install.sh b/automated install/basic-install.sh index 83abc49e..d823e923 100755 --- a/automated install/basic-install.sh +++ b/automated install/basic-install.sh @@ -1437,7 +1437,7 @@ stop_service() { } # Start/Restart service passed in as argument -start_service() { +restart_service() { # Local, named variables local str="Starting ${1} service" printf " %b %s..." "${INFO}" "${str}" @@ -2572,7 +2572,7 @@ main() { if [[ "${INSTALL_WEB_SERVER}" == true ]]; then if [[ "${LIGHTTPD_ENABLED}" == true ]]; then - start_service lighttpd + restart_service lighttpd enable_service lighttpd else printf " %b Lighttpd is disabled, skipping service restart\\n" "${INFO}" @@ -2587,7 +2587,7 @@ main() { # Fixes a problem reported on Ubuntu 18.04 where trying to start # the service before enabling causes installer to exit enable_service pihole-FTL - start_service pihole-FTL + restart_service pihole-FTL # Download and compile the aggregated block list runGravity From d26f2dcb2c4e865a4e69c72e7e2c2db3778b1d40 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 24 Jan 2019 09:47:28 +0100 Subject: [PATCH 06/12] Use global binary variable in tests for automated install Signed-off-by: DL6ER --- test/test_automated_install.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/test_automated_install.py b/test/test_automated_install.py index a2593d83..ce0abfd2 100644 --- a/test/test_automated_install.py +++ b/test/test_automated_install.py @@ -484,7 +484,8 @@ def test_FTL_download_aarch64_no_errors(Pihole): # mock uname to return generic platform download_binary = Pihole.run(''' source /opt/pihole/basic-install.sh - FTLinstall pihole-FTL-aarch64-linux-gnu + binary="pihole-FTL-aarch64-linux-gnu" + FTLinstall ''') expected_stdout = tick_box + ' Downloading and Installing FTL' assert expected_stdout in download_binary.stdout @@ -498,7 +499,8 @@ def test_FTL_download_unknown_fails_no_errors(Pihole): # mock uname to return generic platform download_binary = Pihole.run(''' source /opt/pihole/basic-install.sh - FTLinstall pihole-FTL-mips + binary="pihole-FTL-mips" + FTLinstall ''') expected_stdout = cross_box + ' Downloading and Installing FTL' assert expected_stdout in download_binary.stdout From 7479485d4518e0fda8ea46d255e504d193013722 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 31 Jan 2019 20:00:26 +0100 Subject: [PATCH 07/12] Add test for the case the binary variable is unset (defaults to "tbd" in this case) Signed-off-by: DL6ER --- test/test_automated_install.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/test/test_automated_install.py b/test/test_automated_install.py index ce0abfd2..853048d1 100644 --- a/test/test_automated_install.py +++ b/test/test_automated_install.py @@ -481,7 +481,6 @@ def test_FTL_download_aarch64_no_errors(Pihole): ''' confirms only aarch64 package is downloaded for FTL engine ''' - # mock uname to return generic platform download_binary = Pihole.run(''' source /opt/pihole/basic-install.sh binary="pihole-FTL-aarch64-linux-gnu" @@ -496,7 +495,6 @@ def test_FTL_download_unknown_fails_no_errors(Pihole): ''' confirms unknown binary is not downloaded for FTL engine ''' - # mock uname to return generic platform download_binary = Pihole.run(''' source /opt/pihole/basic-install.sh binary="pihole-FTL-mips" @@ -510,6 +508,22 @@ def test_FTL_download_unknown_fails_no_errors(Pihole): assert error2 in download_binary.stdout +def test_FTL_download_binary_unset_no_errors(Pihole): + ''' + confirms unset binary variable does not download FTL engine + ''' + download_binary = Pihole.run(''' + source /opt/pihole/basic-install.sh + FTLinstall + ''') + expected_stdout = cross_box + ' Downloading and Installing FTL' + assert expected_stdout in download_binary.stdout + error1 = 'Error: URL https://github.com/pi-hole/FTL/releases/download/' + assert error1 in download_binary.stdout + error2 = 'not found' + assert error2 in download_binary.stdout + + def test_FTL_binary_installed_and_responsive_no_errors(Pihole): ''' confirms FTL binary is copied and functional in installed location From 4948862dced5a0ae9f9dba13f119277c533815db Mon Sep 17 00:00:00 2001 From: DL6ER Date: Thu, 31 Jan 2019 20:10:52 +0100 Subject: [PATCH 08/12] Fail hard if FTL binary could not be installed Signed-off-by: DL6ER --- automated install/basic-install.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/automated install/basic-install.sh b/automated install/basic-install.sh index d823e923..ac61783c 100755 --- a/automated install/basic-install.sh +++ b/automated install/basic-install.sh @@ -2541,7 +2541,10 @@ main() { LIGHTTPD_ENABLED=false fi # Check if FTL is installed - do this early on as FTL is a hard dependency for Pi-hole - FTLdetect || printf " %b FTL Engine not installed\\n" "${CROSS}" + if ! FTLdetect; then + printf " %b FTL Engine not installed\\n" "${CROSS}" + exit 1 + fi # Install and log everything to a file installPihole | tee -a /proc/$$/fd/3 From 9c0de0f73128da47378c18958bd04d37793be853 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Fri, 1 Feb 2019 17:39:12 +0100 Subject: [PATCH 09/12] Try to stop pihole-FTL before and (re)start pihole-FTL after the install command Signed-off-by: DL6ER --- automated install/basic-install.sh | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/automated install/basic-install.sh b/automated install/basic-install.sh index ac61783c..2022a7bf 100755 --- a/automated install/basic-install.sh +++ b/automated install/basic-install.sh @@ -2177,11 +2177,24 @@ FTLinstall() { # If we downloaded binary file (as opposed to text), if sha1sum --status --quiet -c "${binary}".sha1; then printf "transferred... " + + # Stop pihole-FTL service if available + # Allow failing without tripping set -e as the + # service might not be available (e.g. on first install) + service pihole-FTL stop > /dev/null 2>&1 || true + # Install the new version with the correct permissions install -T -m 0755 "${binary}" /usr/bin/pihole-FTL + + # Start pihole-FTL service if available + # Allow failing without tripping set -e as the + # service might not be available (e.g. on first install) + service pihole-FTL restart > /dev/null 2>&1 || true + # Move back into the original directory the user was in popd > /dev/null || { printf "Unable to return to original directory after FTL binary download.\\n"; return 1; } - # Install the FTL service + + # Installed the FTL service printf "%b %b %s\\n" "${OVER}" "${TICK}" "${str}" return 0 # Otherwise, From e8dabc71af52238726da2a0a9454a09dca5e66bb Mon Sep 17 00:00:00 2001 From: DL6ER Date: Fri, 1 Feb 2019 17:54:21 +0100 Subject: [PATCH 10/12] Don't try to start pihole-FTL after it has been installed. This will be done a few moments later when gravity is invoked. Signed-off-by: DL6ER --- automated install/basic-install.sh | 5 ----- 1 file changed, 5 deletions(-) diff --git a/automated install/basic-install.sh b/automated install/basic-install.sh index 2022a7bf..865750ad 100755 --- a/automated install/basic-install.sh +++ b/automated install/basic-install.sh @@ -2186,11 +2186,6 @@ FTLinstall() { # Install the new version with the correct permissions install -T -m 0755 "${binary}" /usr/bin/pihole-FTL - # Start pihole-FTL service if available - # Allow failing without tripping set -e as the - # service might not be available (e.g. on first install) - service pihole-FTL restart > /dev/null 2>&1 || true - # Move back into the original directory the user was in popd > /dev/null || { printf "Unable to return to original directory after FTL binary download.\\n"; return 1; } From ec79e86bee709cbbf94b677a5c515212081e70a6 Mon Sep 17 00:00:00 2001 From: DL6ER Date: Fri, 1 Feb 2019 18:06:57 +0100 Subject: [PATCH 11/12] We should really use stop_service as it also includes some nice output Signed-off-by: DL6ER --- automated install/basic-install.sh | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/automated install/basic-install.sh b/automated install/basic-install.sh index 865750ad..cf73864b 100755 --- a/automated install/basic-install.sh +++ b/automated install/basic-install.sh @@ -2179,9 +2179,7 @@ FTLinstall() { printf "transferred... " # Stop pihole-FTL service if available - # Allow failing without tripping set -e as the - # service might not be available (e.g. on first install) - service pihole-FTL stop > /dev/null 2>&1 || true + stop_service pihole-FTL &> /dev/null # Install the new version with the correct permissions install -T -m 0755 "${binary}" /usr/bin/pihole-FTL From ae654730c4a54d7b7c9a7734e010bff1fda97c1a Mon Sep 17 00:00:00 2001 From: Adam Warner Date: Sat, 2 Feb 2019 08:00:58 +0100 Subject: [PATCH 12/12] Update automated install/basic-install.sh Co-Authored-By: DL6ER --- automated install/basic-install.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/automated install/basic-install.sh b/automated install/basic-install.sh index cf73864b..51fb2810 100755 --- a/automated install/basic-install.sh +++ b/automated install/basic-install.sh @@ -1439,7 +1439,7 @@ stop_service() { # Start/Restart service passed in as argument restart_service() { # Local, named variables - local str="Starting ${1} service" + local str="Restarting ${1} service" printf " %b %s..." "${INFO}" "${str}" # If systemctl exists, if is_command systemctl ; then