Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Display more meaningful exit message if dig command fails during os_check #3702

Merged
merged 3 commits into from
Aug 23, 2020

Conversation

PromoFaux
Copy link
Member

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes, and have included unit tests where possible.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)

What does this PR aim to accomplish?:

Fixes #3694

Alternative to #3695

Outputs as following:

image

Note: I couldn't be bothered with the rigmaroll of blocking outbound port 53 on my network so I changed the @ns1.pi-hole.net to @ns141@pi-hole.net to simulate being unable to resolve @ns1.pi-hole.net

yubiuser
yubiuser previously approved these changes Aug 20, 2020
@PromoFaux PromoFaux marked this pull request as draft August 20, 2020 21:02
@PromoFaux
Copy link
Member Author

Got some more stuff to do here...

@PromoFaux PromoFaux marked this pull request as ready for review August 22, 2020 14:15
@PromoFaux PromoFaux requested review from dschaper and DL6ER and removed request for dschaper and DL6ER August 22, 2020 14:15
@PromoFaux

This comment has been minimized.

Copy link
Member

@DL6ER DL6ER left a comment

Choose a reason for hiding this comment

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

This runs the identical dig two times and wouldn't catch it the second fails. I wonder if we should, instead, rather check for an invalid or empty response from

output="$(dig +short -t txt versions.pi-hole.net @ns1.pi-hole.net)"

rather than trying to extract the exact exit code?

In this case, we could show what was returned ("expired", empty or whatever) and still catch the error.

@PromoFaux

This comment has been minimized.

@DL6ER
Copy link
Member

DL6ER commented Aug 23, 2020

In the case of the first failure (ns1.pi-hole.net = NXDOMAIN), the supportedOS array contains one element of FAILED

In the case of the second (a timeout - hence random IP) the supportedOS array contains the entire ;; connection timed out... message, but not the FAILED

We could maybe simply redirect stderr into stdout to resolve this.

Example:

$ out="$(dig +short -t txt versions.pi-hole.net @use-application-dns.net || echo "FAILED")"
dig: couldn't get address for 'use-application-dns.net': not found

$ echo "$out"
FAILED

With 2>&1:

$ out="$(dig +short -t txt versions.pi-hole.net @use-application-dns.net 2>&1 || echo "FAILED")"
[no output]

$ echo "$out"
dig: couldn't get address for 'use-application-dns.net': not found
FAILED

Also:

$ out="$(dig +short -t txt versions.pi-hole.net @1.2.3.4 2>&1 || echo "FAILED")"
[no output]

$ echo "$out"
;; connection timed out; no servers could be reached
FAILED

Note: use-application-dns.net is always NXDOMAIN in Pi-hole environments to prevent Firefox's inbuilt DoH from taking over.

@PromoFaux
Copy link
Member Author

In the case of a failure, we only want the "failed" part (or more helpfully, just the exit code). Hence the double-hit of the function. With the 2>&1 it is nice because it hides the error message from immediate display, but we don't want the error message in the variable :)

Otherwise we are in the territory of string comparison for actual error messages, which obviously will cause issues internationally

@DL6ER
Copy link
Member

DL6ER commented Aug 23, 2020

I was thinking more along the lines of doing exactly this || echo "FAILED" and then check only for this FAILED being the last line - or not. Then no international stuff comes into play. For this, we'd obviously have to do the dig before the loop, then do some checking and then iterate over the variable instead of the immediate dig output.
More like

out="$(dig +short -t txt versions.pi-hole.net || echo "FAILED")"

# last line should be
echo "${out##*$'\n'}"
# or
echo "$out" | tail -n1

# Test based on these lines being == "FAILED"
# Only run loop if this is not the case!

while IFS= read -r line; do
    # Check if $line is empty, in this case, break out of the loop and print some error?
    echo "... $line ..."
done <<< "$out"

Sorry, I don't have a Pi-hole with me right now or I'd propose a more working code for you. I'm just thinking out aloud here, trusting you will do something amazing with it :-)

@PromoFaux
Copy link
Member Author

Hold my beer...

@PromoFaux
Copy link
Member Author

out="$(dig +short -t txt +timeout=1 versions.pi-hole.net @ns1.pi-hole.net 2>&1|| echo $?)"
echo "ResultFromWorking:"
echo "${out##*$'\n'}"
out="$(dig +short -t txt +timeout=1 versions.pi-hole.net @10.1.2.3 2>&1 || echo $?)"
echo "ResultFromTimeout:"
echo "${out##*$'\n'}"
out="$(dig +short -t txt +timeout=1 versions.pi-hole.net @use-application-dns.net 2>&1 || echo $?)"
echo "ResultFromNXDomain:"
echo "${out##*$'\n'}"

results:

ResultFromWorking:
"Raspbian=9,10 Ubuntu=16,18,20 Debian=9,10 Fedora=31,32 CentOS=7,8"
ResultFromTimeout:
9
ResultFromNXDomain:
10

OK, I should be able to work with this, just need to check if the result is a number. According to this page it should only be one of 5 numbers:

Dig return codes are:

  • 0: Everything went well, including things like NXDOMAIN
  • 1: Usage error
  • 8: Couldn't open batch file
  • 9: No reply from server
  • 10: Internal error

Include dig return code and response in debug run

Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@PromoFaux
Copy link
Member Author

Right, totally redid it (force pushed changes), and made some tweaks to the copy

Example outputs:

Blank response
image

dig returns non-zero
image

Debug info:
image

Full error output:
image

@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pihole-fails-to-update-on-ubuntu-18-04-lts-says-unsupported-os/37454/15


# Dig returned 0 code, so get the actual response, and loop through it to determine if the detected variables above are valid
Copy link
Member

Choose a reason for hiding this comment

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

This comment is wrong here (copy-paste from the installer)

automated install/basic-install.sh Outdated Show resolved Hide resolved
PromoFaux and others added 2 commits August 23, 2020 14:35
Co-authored-by: DL6ER <DL6ER@users.noreply.github.com>
Signed-off-by: Adam Warner <me@adamwarner.co.uk>
@DL6ER DL6ER merged commit b81cbaa into development Aug 23, 2020
@DL6ER DL6ER deleted the tweak/os_check_output branch August 23, 2020 14:08
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-core-web-v5-2-and-ftl-v5-3-released/40909/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants