Skip to content

Update existing lists #144

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Update existing lists #144

wants to merge 1 commit into from
+149 −7

Conversation

bsyk
Copy link
Contributor

@bsyk bsyk commented Dec 1, 2024

To avoid the need to delete all lists and recreate them, we can update existing lists only when their contents had changed.

This processes the diffs between the desired list of domains and the existing lists. Removing entires that are no longer in the desired list and appending any new entries. This prefers to minimize the number of PATCH calls by appending entries to the lists we're already patching for the removals.

The priority for additions is:

  1. Add to lists we're already patching for removals, filling up to LIST_ITEM_SIZE entries.
  2. Add to existing lists with fewer than LIST_ITEM_SIZE entries.
  3. Create a new list.

@bsyk
Copy link
Contributor Author

bsyk commented Dec 1, 2024

This is quite a large diff. Happy to iterate if there are things you want to change.

@vietthedev
Copy link
Contributor

I'm not sure list patching can work reliably. A domain can be in another list instead of the previous list after blocklist update and would mess up the patching process.

For example: We have two lists and the first one is full. If a new domain is added to the first list after the blocklist update, the last domain would end up being in the second list. We wouldn't be able to check its existence without going through all the current lists.

Before blocklist update

1st list
domainA
domainAA
...
domainB

2nd list
domainC
domainD

After blocklist update

1st list
domainA
domainAA
domainAAA
...

2nd list
domainB
domainC
domainD

Things become even more complicated if the next update is from a different set of blockklists.

Therefore, to make it work reliably, we can only go through all the current lists but that would defeat the purpose to save time and requests because there isn't an API to get all items of all the lists.

@bsyk
Copy link
Contributor Author

bsyk commented Dec 3, 2024

This change does check all existing lists. It is a reliable operation and idempotent such that should an error occur, the same command can be used again to complete any missing changes.

The benefits of this change are not only (slightly) faster operations, but avoiding any period where the lists or rules are unapplied which would leave the network and users vulnerable to accessing otherwise blocked hosts.

The maximum requests in this flow is 2x number_of_lists (GET + PATCH to every list) in the worst case.
The normal case is > 1x < 2x number_of_lists where it would be unusual to have to patch every list.

The requests in the current flow is 2x number_of_lists (DELETE + POST to recreate every list) in the normal case.

@bsyk bsyk mentioned this pull request Dec 11, 2024
Open
kieranbrown added a commit to kieranbrown/dns that referenced this pull request Dec 27, 2024
@kieranbrown
ci: test mrrfv/cloudflare-gateway-pihole-scripts#144 performance
2a6dec9
@kieranbrown
Copy link

I've been using this PR and the performance difference is pretty huge, using only OISD small it takes the average runtime from 2m45s to about 30s. (Comparing against a full delete+create & this PR to compare+patch).

I have came across a bug though, the script errors when no lists currently exist, it only works where at least 1 list already exists.

Example workflow:
https://github.com/kieranbrown/dns/actions/runs/12520357125/job/34925911614

file:///home/runner/work/dns/dns/lib/api.js:78
  const cgpsLists = lists.filter(({ name }) => name.startsWith("CGPS List"));
                          ^

TypeError: Cannot read properties of null (reading 'filter')
    at synchronizeZeroTrustLists (file:///home/runner/work/dns/dns/lib/api.js:78:27)
    at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
    at async file:///home/runner/work/dns/dns/cf_list_create.js:143:3

@bsyk
Copy link
Contributor Author

bsyk commented Dec 30, 2024 via email

@kieranbrown
Copy link

Thanks for the feedback and bug report. Will get that bug patched today.

Can confirm that issue is resolved 🙏

I've just encountered another issue when adding a new blocklist. Running a full delete+create fixed the issue so it seems to be related to adding a new Cloudflare list during synchronisation.

Should be able to reproduce by starting off with a small blocklist, let it create at least 1 Cloudflare list, then add another blocklist causing it to create more Cloudflare lists.

Example workflow:
https://github.com/kieranbrown/dns/actions/runs/12583072895/job/35069887006

Could not create "CGPS List - Chunk NaN" - Error: undefined - Error: HTTP error! Status: 409
file:///home/runner/work/dns/dns/lib/helpers.js:59
    throw new Error(`${(data && 'errors' in data) ? data.errors[0].message : data} - ${error}`);
          ^

Error: undefined - Error: HTTP error! Status: 409
    at request (file:///home/runner/work/dns/dns/lib/helpers.js:59:11)
    at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
    at async createZeroTrustListsOneByOne (file:///home/runner/work/dns/dns/lib/api.js:188:7)
    at async synchronizeZeroTrustLists (file:///home/runner/work/dns/dns/lib/api.js:169:5)
    at async file:///home/runner/work/dns/dns/cf_list_create.js:[143](https://github.com/kieranbrown/dns/actions/runs/12583072895/job/35069887006#step:8:144):3
    ```

@bsyk
Copy link
Contributor Author

bsyk commented Jan 2, 2025

Apologies for this oversight and thanks for the continued testing and feedback.
Patched the issue in the above commit.

@mrrfv
Copy link
Owner

mrrfv commented Jan 2, 2025 •
edited
Loading

Magnificent work; thank you for being patient and quickly fixing new problems as they come up. Because there are so many moving parts involved in this process, I'd continue testing the new code further before merging.
I've noticed 3 minor issues so far:

  1. Switching from a large blocklist (e.g., the current default) to a smaller one (this to be exact) causes the script to remove all items from some lists; this means a lot of lists end up completely empty. Such lists should probably be removed, as long as that wouldn't cause additional issues.
  2. After the same operation, some lists only contain a handful of items and could be consolidated into one for simplicity.
  3. If there are only additions to a list, the log entry will end with a comma, which ends up looking awkward:

    cloudflare-gateway-pihole-scripts/lib/api.js

    Line 161 in 5f7fb1a

    console.log(`Updating list "${cgpsLists.find(list => list.id === listId).name}", ${appends ? `${appends} additions, ` : ''}${removals ? `${removals} removals` : ''}`);

Aside from what I've mentioned, it works beautifully. Thanks again.

@bsyk
Copy link
Contributor Author

bsyk commented Jan 3, 2025

Fixed your 3. comment since it's an easy change.

For 1. and 2. the change would be more intrusive and other than organization, doesn't change the outcome much.
I think we can address that in a separate PR and possibly as a separate functionality. Some kind of "defrag".
There would be benefits to keeping lists more stable, i.e. hosts that have been consistently in the block list should be grouped into lists together so that any adds/removes affect as few lists as possible.

@kieranbrown
Copy link

I’m still coming across the Chuck NaN problem when the script wants to create a new list

https://github.com/kieranbrown/dns/actions/runs/12702722844/job/35409402648

@bsyk
Copy link
Contributor Author

bsyk commented Jan 10, 2025 via email

@umpire7777777
Copy link

umpire7777777 commented Jan 14, 2025 •
edited
Loading

I would like to test this out myself.
After merging this branch with my own repo, what do I need to change in order to try this? Do I need to update the action in workflow/main.yml?

ps: I'm very sorry to hear you are impacted by the CA fires. That is terrible and I wish you all the best.

@bsyk bsyk force-pushed the update-lists branch 2 times, most recently from 2865475 to bf94073 Compare February 14, 2025 02:57
@bsyk
Copy link
Contributor Author

bsyk commented Feb 14, 2025

Sorry for the delay. I have addressed the NaN issue. It was a bug on my part, forgot to spread the array of numbers so it was trying to do Math.max on an array rather than a list of numbers.

@bsyk
Copy link
Contributor Author

bsyk commented Feb 14, 2025

@umpire7777777 the easiest way to test out this branch is to edit your workflow to checkout this branch instead of the parent repo's v1 branch.

e.g.
Change

name: Checkout
        uses: actions/checkout@v4
        with:
          repository: "mrrfv/cloudflare-gateway-pihole-scripts"
          ref: "v1"

to

name: Checkout
        uses: actions/checkout@v4
        with:
          repository: "bsyk/cloudflare-gateway-pihole-scripts"
          ref: "update-lists"

Or point to your own repo if you do pull this branch into one of your own.

@umpire7777777
Copy link

@umpire7777777 the easiest way to test out this branch is to edit your workflow to checkout this branch instead of the parent repo's v1 branch.

e.g. Change

name: Checkout
        uses: actions/checkout@v4
        with:
          repository: "mrrfv/cloudflare-gateway-pihole-scripts"
          ref: "v1"

to

name: Checkout
        uses: actions/checkout@v4
        with:
          repository: "bsyk/cloudflare-gateway-pihole-scripts"
          ref: "update-lists"

Or point to your own repo if you do pull this branch into one of your own.

I made this change to test. It failed during the create new rules step. It failed on the first list, then repeated until the final failure.

Number of blocked domains: 182391
Number of lists to be created: 183
Creating 183 lists for 182391 domains...
Checking existing lists...
Found 0 existing lists. Calculating diffs...
0 removals, 182391 additions to make
Created "CGPS List - Chunk -Infinity" list - 182 left
An error occured while making a web request: "Error: HTTP error! Status: 429", retrying. Attempt 1 of 50.
THIS IS NORMAL IN MOST CIRCUMSTANCES. Refrain from reporting this as a bug unless the script doesn't automatically recover after several attempts.
Waiting for 2 minutes to avoid rate limiting.
An error occured while making a web request: "Error: HTTP error! Status: 409", retrying. Attempt 2 of 50.
---- repeated
An error occured while making a web request: "Error: HTTP error! Status: 409", retrying. Attempt 50 of 50.
THIS IS NORMAL IN MOST CIRCUMSTANCES. Refrain from reporting this as a bug unless the script doesn't automatically recover after several attempts.
Could not create "CGPS List - Chunk -Infinity" - Error: undefined - Error: HTTP error! Status: 409
file:///home/runner/work/cloudflare-gateway-pihole-scripts/cloudflare-gateway-pihole-scripts/lib/helpers.js:59
throw new Error(${(data && 'errors' in data) ? data.errors[0].message : data} - ${error});
^
Error: undefined - Error: HTTP error! Status: 409
at request (file:///home/runner/work/cloudflare-gateway-pihole-scripts/cloudflare-gateway-pihole-scripts/lib/helpers.js:59:11)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
at async createZeroTrustListsOneByOne (file:///home/runner/work/cloudflare-gateway-pihole-scripts/cloudflare-gateway-pihole-scripts/lib/api.js:188:7)
at async synchronizeZeroTrustLists (file:///home/runner/work/cloudflare-gateway-pihole-scripts/cloudflare-gateway-pihole-scripts/lib/api.js:169:5)
at async file:///home/runner/work/cloudflare-gateway-pihole-scripts/cloudflare-gateway-pihole-scripts/cf_list_create.js:143:3
Node.js v22.13.1
Error: Process completed with exit code 1.

@bsyk
Copy link
Contributor Author

bsyk commented Feb 14, 2025 via email

@bsyk bsyk force-pushed the update-lists branch 2 times, most recently from d8d9c76 to 293a3f5 Compare February 14, 2025 19:29
@bsyk
Copy link
Contributor Author

bsyk commented Feb 14, 2025

@umpire7777777 I believe this is now fixed. Sorry for the issue.

@umpire7777777
Copy link

I'm not geting it, sorry. I made the change above, but I think there is another change needed in main.yml but cannot figure it out. When I run the workflow Update Filter Lists, it still references "Delete Lists - Run npm run cloudflare-delete" and Create Lists -npm run cloudflare-create".

Can you share your main.yml so I can compare?

@bsyk
Copy link
Contributor Author

bsyk commented Feb 18, 2025 •
edited
Loading

Sorry. I forgot I'd removed some steps.
Here's my workflow:

name: Update Filter Lists

on:
  schedule:
    - cron: "0 3 * * *"
  push:
    branches:
      - update-lists
  workflow_dispatch:

concurrency:
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

env:
  NODE_ENV: production

jobs:
  cgps:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout
        uses: actions/checkout@v4
        with:
          repository: "bsyk/cloudflare-gateway-pihole-scripts"
          ref: "update-lists"

      - name: Install Node.js
        uses: actions/setup-node@v4
        with:
          node-version-file: ".node-version"

      - name: Install npm dependencies
        run: npm ci

      - name: Download blocklists
        run: npm run download:blocklist
        env:
          BLOCKLIST_URLS: |
            https://raw.githubusercontent.com/Cauchon/pi-hole-blacklist/master/hostlist.txt
            https://raw.githubusercontent.com/StevenBlack/hosts/master/hosts
            https://s3.amazonaws.com/lists.disconnect.me/simple_tracking.txt
            https://s3.amazonaws.com/lists.disconnect.me/simple_ad.txt

      - name: Upsert rules and lists
        run: npm run cloudflare-create
        env:
          CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN }}
          CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }}

  keepalive:
    runs-on: ubuntu-latest
    permissions:
      actions: write
      contents: read
    steps:
      - uses: actions/checkout@v4
      - uses: gautamkrishnar/keepalive-workflow@v2

@raetha
Copy link
Contributor

raetha commented Mar 8, 2025

Thinking about the docker work I submitted recently, once this PR merges it won't end up applying for docker containers due to the entrypoint script using the "npm start" command which runs "download", "delete", then "create" in order.

I see a couple options that make sense to me:

  1. Change the entrypoint script to only call "download" and "create" as you are doing above. This might require future changes to the script if other things change, and removes the simplicity of just using a single command to trigger all the things.
  2. Update the "start" script in package.json to only call "download" and "create". This only works if there isn't any scenario where "delete" might need to be called during an automated run. I would leave the delete available as it's faster than manually removing entries if someone wanted to cleanup all traces.
  3. Reworking things to somehow be smart if there are situations that might require/want a delete, and otherwise call an "update" routine instead. This would be the most user friendly, though I'm not sure how complicated as I don't actually know JS and Node well at all.

@bsyk @mrrfv any thoughts if this is still planned to be merged at some point? Would be nice to have the docker containers take advantage when it is.

@bsyk
Update existing lists
b7b3a5a 
To avoid the need to delete all lists and recreate them, we can update existing lists only when their contents had changed.
This processes the diffs between the desired list of domains and the existing lists. Removing entires that are no longer in the desired lists and appending any new entries. This prefers to minimize the number of PATCH calls by appending entries to the lists we're already patching for the removals.
The priority for additions is:
1. Add to lists we're already patching for removals.
2. Add to existing lists with fewer than LIST_ITEM_SIZE entries.
3. Create a new list.
@bsyk
Copy link
Contributor Author

bsyk commented Mar 30, 2025

I've addressed some of the concerns about small/empty lists in this second PR (not sure how to add non-repo reviewers to that one)
bsyk#1

The npm run cloudflare-defragment could be called in a cron GitHub action every so often to clean up.
I'll be running this configuration going forward so it will get some test runs.

@bsyk
Copy link
Contributor Author

bsyk commented Mar 30, 2025

@raetha Regarding the entrypoint, it should be sufficient to run npm run cloudflare-refresh:blocklist
i.e. change the cron expression to /usr/local/bin/npm run cloudflare-refresh:blocklist --prefix /app/

Note: There may be a need to init the allowlist.txt first since the refresh:blocklist does not touch that.

@raetha
Copy link
Contributor

raetha commented Apr 17, 2025

Hi @bsyk. Got everything working in my fork with this PR, the allowlist one, and pulled in your defrag branch. It all looks good, but I found one issue that I think stems back to this PR.

My allowlist is no longer being excluded. It took me a little while to figure out what is happening, but somewhere between your PRs it seems to have stopped excluding domains from a supplied allowlist. Specifically I am using Hagezi's Multi Normal blocklist, which includes joinhoney.com. I've added parts of the PayPal Honey whitelist into my own allowlist.txt file which can be found at:
https://github.com/raetha/cloudflare-gateway-pihole-scripts/blob/main/lists/joinhoney.txt

I'm finding that the joinhoney.com domain is still being added to a CF. It's possible I'm doing something wrong, but I believe things look right in my configuration, and I don't see anything in the code that jumps out at me given I've never worked directly with JS before.

Could you take a look and see if you notice anything that could be breaking allowlist processing?

@bsyk bsyk mentioned this pull request Apr 18, 2025
Merged
@bsyk
Copy link
Contributor Author

bsyk commented Apr 18, 2025

I don't have any issue with allowlist processing.
The format of your allowlist doesn't seem to work with the processing. This was not changed by any of my PRs.

One issue is in the normalizeDomain function.
Your allowlist has entries such as @@||joinhoney.com^
While it looks like @@|| should be removed, it's not because the earlier normalized value has already removed the || so there's only the @@ left.
If that were fixed, it would work because these would be normalized.

@@||joinhoney.com^
@@||honey.io^

However, any with the trailing bits, e.g. *$document will not be valid domains still.

PR here to fix the @@|| issue

Here are some additional debugging steps.

  1. Is your allowlist.txt getting populated correctly?
  2. If you're running cloudflare-refresh:blocklist it won't touch the allowlist.txt it assumes you have a committed version. You can instead run cloudflare-refresh which will regenerate allowlist.txt from your configured URLs.
  3. Try using DEBUG/DRY_RUN mode. DEBUG=1 DRY_RUN=1 npm run cloudflare-create:list. You should see detailed logging for skipping blocklist entries that are in the allowlist. Found ${domain} in allowlist - Skipping or Found parent domain ${item} in allowlist - Skipping ${domain}

@raetha
Copy link
Contributor

raetha commented Apr 22, 2025

Awesome, thanks for that fix it seems to be working as expected now.

As for the allowlist I shared though, that is basically just a stripped down version of the list provided directly by PayPal Honey for use with ad blockers. The full version can be found at https://www.joinhoney.com/whitelist/honey-smart-shopping.txt. I just kept only the entries for their main domain as I didn't want overrides for the others that look more questionable.

Ultimately I've just been in a loop lately where my main blocklist blocks these domains and was preventing downloading the official version of the list. So I looked at it for the first time and pulled out just what I needed into the one stored in my repo at the moment. I plan to find a better way to handle it eventually.

Thanks for the quick fix. All the improvements you've made are greatly appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bsyk @vietthedev @kieranbrown @mrrfv @umpire7777777 @raetha