🐛 fix issue 34 when running Pi-Hole in Docker Container #40

thomasmerz · 2022-01-31T23:16:41Z

This PR will fix issue #34 by disabling regex-check when running Pi-hole in Docker Container 😁
To be honest to the user, we will print this info in options and when running 👍🏻

thomasmerz · 2022-01-31T23:20:08Z

Checks won't fail any more after PR #37 has beeen finished/done…

yubiuser · 2022-02-01T05:52:50Z

but it doesn't say command not found anymore - problem/issue solved wink

I'm not convinced if this is really a working solution. If it takes hours this is nothing I'd like to ship to users

yubiuser · 2022-02-01T05:59:05Z

Does it really work?

I did not test it, but expect it to fail also here:

pihole_adlist_tool/pihole_adlist_tool

Line 1006 in 890f454

    
           pihole-FTL regex-test $CURRENT_DOMAIN |grep -E -o "DB ID [0-9]*"|awk '{print $3}' | while read REGEX_ID; do

thomasmerz · 2022-02-01T08:21:57Z

Does it really work?

I did not test it, but expect it to fail also here:

pihole_adlist_tool/pihole_adlist_tool

Line 1006 in 890f454

pihole-FTL regex-test $CURRENT_DOMAIN |grep -E -o "DB ID [0-9]*"|awk '{print $3}' | while read REGEX_ID; do

I reviewed and a fix will come…

I'm not convinced if this is really a working solution. If it takes hours this is nothing I'd like to ship to users

You are right, do not merge this until we have an optimized version!
I have some ideas but have to test and check…

⚠️ Review and diff should be done when PR #37 has been merged because this PR depends on it! ⚠️

thomasmerz · 2022-02-02T00:02:50Z

💡 This is working with limit of 2000 (distinct) domains, but limited due to "Argument list too long". This array should be splitted by for example 500 domains and regex-tested in packages…

         mapfile -t all_domains < <(sqlite $TEMP_DB "SELECT domain FROM all_domains")

-        for CURRENT_DOMAIN in "${all_domains[@]}"; do
                 if [ "$PIHOLE_DOCKER" = 0 ];
                     then
-                        pihole-FTL regex-test $CURRENT_DOMAIN |grep -E -o "DB ID [0-9]*"|awk '{print $3}' | while read REGEX_ID; do
-                            sqlite $TEMP_DB "INSERT INTO domain_by_regex(domain, regex_id) VALUES ('$CURRENT_DOMAIN',$REGEX_ID);"
+                       for CURRENT_DOMAIN in "${all_domains[@]}"; do
+                            pihole-FTL regex-test "$CURRENT_DOMAIN" |grep -E -o "DB ID [0-9]*"|awk '{print $3}' | while read -r REGEX_ID; do
+                                sqlite $TEMP_DB "INSERT INTO domain_by_regex(domain, regex_id) VALUES ('$CURRENT_DOMAIN',$REGEX_ID);"
+                            done
                         done
                     else
-                        docker exec $CONTAINER_ID pihole-FTL regex-test $CURRENT_DOMAIN |grep -E -o "DB ID [0-9]*"|awk '{print $3}' | while read REGEX_ID; do
+                        ### TODO: remove limit!!!! ####### TODO ####### TODO #######
+                        docker exec "$CONTAINER_ID" bash -c \
+                          "$(for CURRENT_DOMAINS in $(sqlite $TEMP_DB 'SELECT domain FROM all_domains limit 2000'); do
+                              echo pihole-FTL regex-test "$CURRENT_DOMAINS" ; done)" \
+                                  |grep -E -o "DB ID [0-9]*"|awk '{print $3}' | while read -r REGEX_ID; do
                             sqlite $TEMP_DB "INSERT INTO domain_by_regex(domain, regex_id) VALUES ('$CURRENT_DOMAIN',$REGEX_ID);"
                         done
+                        #docker exec "$CONTAINER_ID" pihole-FTL regex-test "$CURRENT_DOMAIN" |grep -E -o "DB ID [0-9]*"|awk '{print $3}' | while read -r REGEX_ID; do
+                        #    sqlite $TEMP_DB "INSERT INTO domain_by_regex(domain, regex_id) VALUES ('$CURRENT_DOMAIN',$REGEX_ID);"
+                        #done
+                        ### TODO: this really needs some performance optimization to avoid 1000s of docker-execs
+                        :
                 fi
-        done

 # count for each RegEx_id how many domains are in domain_by_regex and store it in table regex_black

@@ -1014,9 +1023,17 @@ EOF
         mapfile -t all_exact_domains < <(sqlite $GRAVITY "SELECT domain FROM domainlist WHERE type in (0,1);")

         for CURRENT_DOMAIN in "${all_exact_domains[@]}"; do
-          pihole-FTL regex-test "$CURRENT_DOMAIN" |grep -E -o "DB ID [0-9]*"|awk '{print $3}' | while read -r REGEX_ID; do
-              sqlite $TEMP_DB "INSERT INTO domainlist_regex(domain, regex_id) VALUES ('$CURRENT_DOMAIN',$REGEX_ID);"
-            done
+                if [ "$PIHOLE_DOCKER" = 0 ];
+                    then
+                        pihole-FTL regex-test "$CURRENT_DOMAIN" |grep -E -o "DB ID [0-9]*"|awk '{print $3}' | while read -r REGEX_ID; do
+                            sqlite $TEMP_DB "INSERT INTO domainlist_regex(domain, regex_id) VALUES ('$CURRENT_DOMAIN',$REGEX_ID);"
+                        done
+                    else
+                        docker exec "$CONTAINER_ID" pihole-FTL regex-test "$CURRENT_DOMAIN" |grep -E -o "DB ID [0-9]*"|awk '{print $3}' | while read -r REGEX_ID; do
+                            sqlite $TEMP_DB "INSERT INTO domainlist_regex(domain, regex_id) VALUES ('$CURRENT_DOMAIN',$REGEX_ID);"
+                        done
+                        ### TODO: may need some performance optimization to avoid so much docker-execs
+                fi

❓ Which counts are relevant for the regex-mode?

  [i]  Since 01.02.2022	 0:58:13 522 different domains from your adlists have been blocked 15680 times in total
       (blocked directly by gravity or during deep CNAME inspection)
  [i]  Using you current adlist configuration 546 domains would have been blocked 16606 times
  [i]  Adlist coverage (sum of "hits covered" ca. 47.000)
  [i]  In total your adlists contain 189 visited (covered) unique domains - meaning those domains are contained only in a single adlist.
…
  [i]  Since 01.02.2022	 0:58:13 you have been visiting 2980 different domains.
       You have 22 blacklist RegEx configured (20 enabled)
       With your enabled blacklist RegEx you would have covered 1 different domains.
  [i]  Please note: the internal Pi-hole RegEx test used here only checks domains against enabled RegEx.
       Therefor, currently disabled RegEx will always have 0 domains covered.

yubiuser · 2022-02-02T06:21:09Z

Which counts are relevant for the regex-mode?

All regex (number not shown) are checked against the 2980 distinct domains

thomasmerz · 2022-02-16T00:08:45Z

@yubiuser , I changed my commit / PR:

This PR will disable regex-analysis when Pi-hole is running in a docker container and tells user about this.

So Regex analysis is still not supported on docker, but script isn't doing anything worthless/useless stuff anymore. Do you agree?

thomasmerz · 2022-02-16T00:14:11Z

I also added a .stickler.yml 😉

thomasmerz · 2022-02-16T21:17:12Z

The difficulty is that we have an unknown and possibly very big list of domains that we have to pass to the docker container for regex-checking. I really have no clue how to solve this efficiently… 🤷🏻‍♂️

yubiuser · 2022-02-17T19:25:24Z

I agree that we should probably disable regex-checking if Pi-hole is running on docker as long as we do not have a better solution.

Please remove the stickler stuff from this PR. I'm happy to include it, but don't mix unrelated changes in one PR

stickler-ci · 2022-02-18T18:53:44Z

I couldn't find a .stickler.yml file in this repository. I can make one for you, or you can create one by following the documentation.

…ned a string. adapt suggestion Co-authored-by: yubiuser <[email protected]>

Co-authored-by: yubiuser <[email protected]>

…te argument.

thomasmerz · 2022-02-20T00:09:42Z

Is this PR now ready for a merge? Or do you want a single commit rebased to commit 890f454 ?

Co-authored-by: yubiuser <[email protected]>

yubiuser · 2022-02-20T19:50:30Z

I guess best would be a rebase on current development. Please also target development. Do not modify .github/workflows/shellcheck.yml in this PR, draft a new one for this. One PR for one issue.

This reverts commit a1cabc7.

This reverts commit cb88f36 to not mix unrelated changes in one PR.

…ing in Docker Container

thomasmerz · 2022-02-20T21:44:35Z

rebase on current development
don't modify .github/workflows/shellcheck.yml (and .stickler.yml as well)
local run of GH action:

$ act
[Shellcheck Lint/Shellcheck Lint] 🚀  Start image=ghcr.io/catthehacker/ubuntu:act-latest
[Shellcheck Lint/Shellcheck Lint]   🐳  docker pull image=ghcr.io/catthehacker/ubuntu:act-latest platform= username= forcePull=false
[Shellcheck Lint/Shellcheck Lint]   🐳  docker create image=ghcr.io/catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[Shellcheck Lint/Shellcheck Lint]   🐳  docker run image=ghcr.io/catthehacker/ubuntu:act-latest platform= entrypoint=["/usr/bin/tail" "-f" "/dev/null"] cmd=[]
[Shellcheck Lint/Shellcheck Lint]   🐳  docker exec cmd=[mkdir -m 0777 -p /var/run/act] user=root workdir=
[Shellcheck Lint/Shellcheck Lint]   🐳  docker cp src=/home/thomas/temp/PRs/pihole_adlist_tool/. dst=/home/thomas/temp/PRs/pihole_adlist_tool
[Shellcheck Lint/Shellcheck Lint]   🐳  docker exec cmd=[mkdir -p /home/thomas/temp/PRs/pihole_adlist_tool] user= workdir=
[Shellcheck Lint/Shellcheck Lint] ⭐  Run actions/checkout@v2
[Shellcheck Lint/Shellcheck Lint]   ✅  Success - actions/checkout@v2
[Shellcheck Lint/Shellcheck Lint] ⭐  Run Download Shellcheck
[Shellcheck Lint/Shellcheck Lint]   🐳  docker exec cmd=[bash --noprofile --norc -e -o pipefail /home/thomas/temp/PRs/pihole_adlist_tool/workflow/1] user= workdir=
| shellcheck-stable/LICENSE.txt
| shellcheck-stable/README.txt
| shellcheck-stable/shellcheck
[Shellcheck Lint/Shellcheck Lint]   ✅  Success - Download Shellcheck
[Shellcheck Lint/Shellcheck Lint] ⭐  Run Check Shellcheck Version
[Shellcheck Lint/Shellcheck Lint]   🐳  docker exec cmd=[bash --noprofile --norc -e -o pipefail /home/thomas/temp/PRs/pihole_adlist_tool/workflow/2] user= workdir=
| ShellCheck - shell script analysis tool
| version: 0.8.0
| license: GNU General Public License, version 3
| website: https://www.shellcheck.net
[Shellcheck Lint/Shellcheck Lint]   ✅  Success - Check Shellcheck Version
[Shellcheck Lint/Shellcheck Lint] ⭐  Run Run Shellcheck
[Shellcheck Lint/Shellcheck Lint]   🐳  docker exec cmd=[bash --noprofile --norc -e -o pipefail /home/thomas/temp/PRs/pihole_adlist_tool/workflow/3] user= workdir=
[Shellcheck Lint/Shellcheck Lint]   ✅  Success - Run Shellcheck

testing before/after as done in PR #37 - with -r 😉

🦎🖥  ✔ ~/temp/PRs/pihole_adlist_tool [fix_issue_34|…4]
23:05 $ ./pihole_adlist_tool-fix_issue_34 -d 2 -t 100 -r -a > new.txt
🦎🖥  ✔ ~/temp/PRs/pihole_adlist_tool [fix_issue_34|…4]
23:06 $ ./pihole_adlist_tool-development -d 2 -t 100 -r -a > old.txt
./pihole_adlist_tool-development: line 991: pihole-FTL: command not found
…
./pihole_adlist_tool-development: line 991: pihole-FTL: command not found
^C./pihole_adlist_tool-development: line 991: pihole-FTL: command not found
🦎🖥  ^C
🦎🖥  ✔ ~/temp/PRs/pihole_adlist_tool [fix_issue_34|…4]
23:06 $ !sd
✔ ~/temp/PRs/pihole_adlist_tool [fix_issue_34|…4]
23:06 $ sdiff -s old.txt  new.txt
  *** Pihole Adlist Tool 2.6.1 ***		      |	  *** Pihole Adlist Tool 2.6.X ***
  [i]  REGEX_MODE: Enabled				      |	  [i]  REGEX_MODE: Disabled due to running Pi-hole in Docker
  [i]  Since 18.02.2022	23:06:35 893 different domains |	  [i]  Since 18.02.2022	23:06:06 893 different domains
       Those would have been the 100 top blocked adlist do |	       Those would have been the 100 top blocked adlist do
							      <
							      <
  [i]  Analysing RegEx .....				      <
  [i]  This might take some time (minutes!) - please be patie <
							      <
							      <
							      <
							      <
  [✗]  User-abort detected!			      <
🦎🖥  ✔ ~/temp/PRs/pihole_adlist_tool [fix_issue_34|…4]
23:06 $

target development

yubiuser · 2022-02-21T05:27:14Z

Whats's about the changes in line 859 and above?

thomasmerz · 2022-02-21T08:25:28Z

What changes do you mean? 🤷🏻‍♂️ git diff development shows me these changes:

Is there anything "wrong" or are you missing anything? If this is the case, it might be my fault while rebasing and resolving some merge conflicts…

yubiuser · 2022-02-26T18:53:46Z

Something went wrong during rebase I guess. These changes were introduced in your other PR but later reverted.

thomasmerz · 2022-02-26T23:26:26Z

I fixed the rebase now. git diff now shows only the "do not RegEx when running in docker container" parts for issue #34 - do you want another squash of all commits to a single one before merging?

pihole_adlist_tool

yubiuser

I can't test currently because I don't have a docker installation right now. But the code change seems right

thomasmerz · 2022-02-28T10:19:43Z

I'll add a final test later (in/after the lunch-break possibly)…

yubiuser · 2022-02-28T10:21:18Z

I just decided that it looks Ok ;-)
But will wait for your feedback before merging to master

thomasmerz · 2022-02-28T10:32:12Z

Short coffee-break before next meeting 😜

🦎🖥  ✔ ~/temp/PRs/pihole_adlist_tool [fix_issue_34|…2]
11:26 $ ./pihole_adlist_tool -d 2 -t 100 -r -a > new_with_r.txt
🦎🖥  ✔ ~/temp/PRs/pihole_adlist_tool [fix_issue_34|…3]
11:30 $ ./pihole_adlist_tool -d 2 -t 100 -a > new_without_r.txt
sdiff -s new_with_r.txt new_without_r.txt🦎🖥  ✔ ~/temp/PRs/pihole_adlist_tool [fix_issue_34|…4]
11:30 $ sdiff -s new_with_r.txt new_without_r.txt
  [i]  REGEX_MODE: Disabled due to running Pi-hole in Docker  |	  [i]  REGEX_MODE: Disabled
  [i]  Since 26.02.2022	11:30:20 982 different domains |	  [i]  Since 26.02.2022	11:30:45 982 different domains
  [i]  Using you current adlist configuration 998 domains|	  [i]  Using you current adlist configuration 998 domains
                                                                                                                                Those would have been the 100 top blocked adlist do |	       Those would have been the 100 top blocked adlist do
stats.gc.fe.apple-dns.net                                2225 |	stats.gc.fe.apple-dns.net                                2223
stats.gc-apple.com.akadns.net                            727  |	stats.gc-apple.com.akadns.net                            725
63  1        407420         697              38098         11 |	63  1        407420         697              38094         11
64  1        99381          28               1533          16 |	64  1        99381          28               1531          16
🦎🖥  ✔ ~/temp/PRs/pihole_adlist_tool [fix_issue_34|…4]
11:31 $

thomasmerz mentioned this pull request Jan 31, 2022

Regex analysis not supported on docker #24

Open

thomasmerz changed the title ~~🐛 fix issue 34 when running Pi-Hole in Docker Container~~ WIP: 🐛 fix issue 34 when running Pi-Hole in Docker Container Feb 1, 2022

thomasmerz marked this pull request as draft February 2, 2022 15:49

thomasmerz changed the title ~~WIP: 🐛 fix issue 34 when running Pi-Hole in Docker Container~~ 🐛 fix issue 34 when running Pi-Hole in Docker Container Feb 2, 2022

thomasmerz marked this pull request as ready for review February 16, 2022 00:09

thomasmerz mentioned this pull request Feb 18, 2022

Add sticker.yml #41

Merged

thomasmerz and others added 13 commits February 18, 2022 20:46

🚨 shellcheck'ed with some suggestions from @yubiuser

2866842

🚨 shellcheck'ed with some more suggestions from @yubiuser

36b9d32

undo the change in the sqlite query (mapfile) + keep the check mark

da3013c

Adapt some more real nice proposals from @yubiuser

7a8ae3d

Fix: SC2178 (warning): Variable was used as an array but is now assig…

d21d6bb

…ned a string. adapt suggestion Co-authored-by: yubiuser <[email protected]>

Update pihole_adlist_tool

6a97ca7

Co-authored-by: yubiuser <[email protected]>

Update pihole_adlist_tool

08830a6

Co-authored-by: yubiuser <[email protected]>

Update pihole_adlist_tool

bc3637c

Co-authored-by: yubiuser <[email protected]>

Update pihole_adlist_tool

c2320d1

Co-authored-by: yubiuser <[email protected]>

Update pihole_adlist_tool

dfa2a09

Co-authored-by: yubiuser <[email protected]>

Fix: SC2145 (error): Argument mixes string and array. Use * or separa…

f757ccb

…te argument.

Fix declaration of LEFT_DOMAINS

70c76e3

Revert unneccessary shellcheck'ed lines

c862156

Update pihole_adlist_tool

92b396d

Co-authored-by: yubiuser <[email protected]>

thomasmerz and others added 7 commits February 20, 2022 22:31

Merge branch 'yubiuser:development' into development

eac1e63

Revert "🚨 shellcheck'ed with some suggestions from @yubiuser"

1743c9e

This reverts commit a1cabc7.

🐛 fix issue 34 when running Pi-Hole in Docker Container

ffeaa8c

🚸 Disable Analyse RegEx when Pi-hole is running in a Docker Container

8e1abf0

Do not exclude any shellcheck (SC1090,SC1091) anymore

63c3f82

Revert "Add also a CI-Job for Stickler"

0b0de8f

This reverts commit cb88f36 to not mix unrelated changes in one PR.

📝 add info about regex-limitation (=disablement) when Pi-hole is runn…

41e5ed1

…ing in Docker Container

Rebased on development and don't modify GH action yaml-files

68ec7d8

thomasmerz changed the base branch from master to development February 20, 2022 21:46

.

58a1d52

yubiuser requested changes Feb 27, 2022

View reviewed changes

pihole_adlist_tool Outdated Show resolved Hide resolved

🐛 fix wrong logic

e5859f3

thomasmerz requested a review from yubiuser February 27, 2022 21:11

yubiuser requested changes Feb 28, 2022

View reviewed changes

pihole_adlist_tool Outdated Show resolved Hide resolved

pihole_adlist_tool Outdated Show resolved Hide resolved

🐛 rewrite logic of disabling regex-checks with docker

e1782f6

yubiuser approved these changes Feb 28, 2022

View reviewed changes

yubiuser merged commit 1fe2eb8 into yubiuser:development Feb 28, 2022

thomasmerz deleted the fix_issue_34 branch February 28, 2022 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 fix issue 34 when running Pi-Hole in Docker Container #40

🐛 fix issue 34 when running Pi-Hole in Docker Container #40

thomasmerz commented Jan 31, 2022 •

edited

Loading

thomasmerz commented Jan 31, 2022

yubiuser commented Feb 1, 2022

yubiuser commented Feb 1, 2022

thomasmerz commented Feb 1, 2022 •

edited

Loading

thomasmerz commented Feb 2, 2022

yubiuser commented Feb 2, 2022

thomasmerz commented Feb 16, 2022

thomasmerz commented Feb 16, 2022

thomasmerz commented Feb 16, 2022

yubiuser commented Feb 17, 2022

stickler-ci bot commented Feb 18, 2022

thomasmerz commented Feb 20, 2022

yubiuser commented Feb 20, 2022

thomasmerz commented Feb 20, 2022 •

edited

Loading

yubiuser commented Feb 21, 2022

thomasmerz commented Feb 21, 2022

yubiuser commented Feb 26, 2022

thomasmerz commented Feb 26, 2022

yubiuser left a comment

thomasmerz commented Feb 28, 2022

yubiuser commented Feb 28, 2022

thomasmerz commented Feb 28, 2022

🐛 fix issue 34 when running Pi-Hole in Docker Container #40

🐛 fix issue 34 when running Pi-Hole in Docker Container #40

Conversation

thomasmerz commented Jan 31, 2022 • edited Loading

thomasmerz commented Jan 31, 2022

yubiuser commented Feb 1, 2022

yubiuser commented Feb 1, 2022

thomasmerz commented Feb 1, 2022 • edited Loading

thomasmerz commented Feb 2, 2022

❓ Which counts are relevant for the regex-mode?

yubiuser commented Feb 2, 2022

thomasmerz commented Feb 16, 2022

thomasmerz commented Feb 16, 2022

thomasmerz commented Feb 16, 2022

yubiuser commented Feb 17, 2022

stickler-ci bot commented Feb 18, 2022

thomasmerz commented Feb 20, 2022

yubiuser commented Feb 20, 2022

thomasmerz commented Feb 20, 2022 • edited Loading

yubiuser commented Feb 21, 2022

thomasmerz commented Feb 21, 2022

yubiuser commented Feb 26, 2022

thomasmerz commented Feb 26, 2022

yubiuser left a comment

Choose a reason for hiding this comment

thomasmerz commented Feb 28, 2022

yubiuser commented Feb 28, 2022

thomasmerz commented Feb 28, 2022

thomasmerz commented Jan 31, 2022 •

edited

Loading

thomasmerz commented Feb 1, 2022 •

edited

Loading

thomasmerz commented Feb 20, 2022 •

edited

Loading