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

Add new query retried status #901

Merged
merged 4 commits into from
Oct 22, 2020
Merged

Add new query retried status #901

merged 4 commits into from
Oct 22, 2020

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Oct 8, 2020

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

10


Add new status RETRIED (12) to be used for queries which were retried. If a query was retried five times until we received a reply form upstream, queries 1-4 will be marked as RETRIED and only query 5 will stay in status FORWARDED. This does not affect the statistics because all five queries where send upstream, so five upstream packages are counted, even when only one query stays in status FORWARDED.

edit Add another new status RETRIED_DNSSEC (13) to be used for queries for which which automatic DNSSEC queries were retried. If we've already got an answer to a query, but we're awaiting keys for validation, there's no point retrying the query, so we're retrying the key query instead.

The web interface will need updating to support this status. This will be done in a follow-up PR.

…. If a query was retried five times before it suceeded, queries 1-4 will be marked as RETRIED and only query 5 will stay in status FORWARDED.

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER added the Bugfix label Oct 8, 2020
@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/some-websites-are-showing-up-as-unknown-in-status-while-others-work/38916/44

@DL6ER DL6ER added the PR: Approval Required Open Pull Request, needs approval label Oct 14, 2020
@DL6ER DL6ER marked this pull request as ready for review October 14, 2020 19:09
…me of when we already have the upstream response but DNSSEC validation is still ongoing

Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER DL6ER changed the title Add new query status RETRIED Add new query retried statuses Oct 18, 2020
@DL6ER DL6ER changed the title Add new query retried statuses Add new query retried status Oct 18, 2020
@DL6ER
Copy link
Member Author

DL6ER commented Oct 18, 2020

The failed arm-qemu build is expected because the container was updated meanwhile and the cross-build doesn't use the versioned containers. This will be fixed by #903 and is of no concern here.

The failed CodeFactor issue is something I will address separately, however, the mentioned possibly security-relevant race condition doesn't apply to our code at all. It is only meant for ensuring that the ownership is correct when FTL is started as root and later drops to pihole as a security measure.

This PR can go in as-is. The two 🔴 can be ignored.

…ed normal queries take over, we have to flat the original query.

Signed-off-by: DL6ER <dl6er@dl6er.de>
@dschaper dschaper added PR: Approved Open Pull Request, Approved by required number of reviewers and removed PR: Approval Required Open Pull Request, needs approval labels Oct 22, 2020
@DL6ER DL6ER merged commit baf7b2a into development Oct 22, 2020
@DL6ER DL6ER deleted the fix/query_retries branch October 22, 2020 20:37
@gie69
Copy link

gie69 commented Oct 25, 2020 via email

@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

@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-ftl-log-found-unknown-status-12-in-long-term-database/41048/1

@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/frage-zum-query-log/41140/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bugfix PR: Approved Open Pull Request, Approved by required number of reviewers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants