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

Provide regex ID for API #1607

Merged
merged 4 commits into from
Sep 15, 2023
Merged

Provide regex ID for API #1607

merged 4 commits into from
Sep 15, 2023

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Jul 28, 2023

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


See title. This was so far missing.

@DL6ER DL6ER requested a review from a team July 28, 2023 21:57
@DL6ER DL6ER force-pushed the tweak/query_details branch 2 times, most recently from e5a5aa8 to 22e0e82 Compare July 28, 2023 22:11
@DL6ER
Copy link
Member Author

DL6ER commented Jul 30, 2023

Rebased on latest development-v6

Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

Why did you drop all the ttl stuff?

You're creating a new column in the database for the regex_id. So far, this was part of additional_info. Why did you change that? Do you plan to migrate the existing data from additional_info?

@DL6ER
Copy link
Member Author

DL6ER commented Aug 3, 2023

Why did you drop all the ttl stuff?

Pi-hole handles queries by "question" (a single domain) but TTL applies to replies and there may actually be multiple. Plus things get really difficult to disentangle when CNAMEs are there as well and even worse when a query is partially replied to from cache. This little extra bit of information is hard to get, not always unique and overall complicates code too much.

That's why I decided to drop it - this code was never used (it was commented out before).


You're creating a new column in the database for the regex_id. So far, this was part of additional_info. Why did you change that? Do you plan to migrate the existing data from additional_info?

Yes, it is in additional_info but only most of the time not always. The information about the regex ID is actually lost when blocking happens during deep CNAME inspection because we then use additional_info to store the domains that actually led to blocking in this case. This issue can only be resolved when adding a new column. Fortunately, this will not affect the database size by much as empty columns take only one byte per row. Integers up to 127 use two bytes per row, integers up to 32767 take three bytes and so on. This seems affordable given that the database indices will typically be rather small here.

src/database/query-table.c Outdated Show resolved Hide resolved
@yubiuser
Copy link
Member

yubiuser commented Aug 3, 2023

Needs documentation changes in https://deploy-preview-338--pihole-docs.netlify.app/database/ftl/

Co-authored-by: yubiuser <[email protected]>
Signed-off-by: DL6ER <[email protected]>
@DL6ER
Copy link
Member Author

DL6ER commented Sep 8, 2023

@yubiuser See pi-hole/docs#928

@DL6ER DL6ER requested review from a team and yubiuser September 11, 2023 08:47
yubiuser
yubiuser previously approved these changes Sep 12, 2023
@yubiuser
Copy link
Member

yubiuser commented Sep 12, 2023

The information about the regex ID is actually lost when blocking happens during deep CNAME inspection because we then use additional_info to store the domains that actually led to blocking in this case. This issue can only be resolved when adding a new column.

Is this implemented with this PR as well?
I tried a quick check by setting up a local CNAME and used a deny regex to block the CNAME. It's not stored in the database nor shown in the web interface as blocked, but only as CNAME.
Or is this expected as it is a local CNAME not a CNAME from upstream?

Add:

Or is this expected as it is a local CNAME not a CNAME from upstream?

I just tested it: it's the same.

@DL6ER DL6ER merged commit 392db95 into development-v6 Sep 15, 2023
18 checks passed
@DL6ER DL6ER deleted the tweak/query_details branch September 15, 2023 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants