acl +fold #3563

nigoroll · 2021-03-27T17:20:13Z

Adds an optional feature to optimize ACLs by removing, for positive matches only, subnets already included in other supernets and merging adjacent entries.
See the second commit's message for detail.
Predecessors to these patches have been in production use since late 2019. This version has been adjusted to recent changes in the VCC ACL code: We now keep the acl entries in a colo^Wrank balanced tree instead of a list, which actually helped the clarity of this patch's code.
Because the changes applied by the vcc_acl_merge feature are not obvious from the test case, I add here the VCC output from 8f64b0d#diff-4f98af9b0a5f60a191eac6e289d0e649f7b9c280f3c61cc8b23fbee0b669f98c

The ACL deliberately is constructed to contain various variants of the supernet & adjacent cases. I will show here the ACL entries and the relevant VCC output:

The second entry is a subnet of the first

                # bad notation (confusing)
                "1.2.3.0"/24;
                "1.2.3.64"/26;

ACL entry:
('<vcl.inline>' Line 9 Pos 17)
                "1.2.3.0"/24;
----------------#########----

supersedes / removes:
('<vcl.inline>' Line 10 Pos 17)
                "1.2.3.64"/26;
----------------##########----

Entry inserted as is, becomes relevant later

                "1.4.4.0"/22;

Another subnet case

                "1.3.4.0"/23;
                "1.3.5.0"/26;

ACL entry:
('<vcl.inline>' Line 14 Pos 17)
                "1.3.4.0"/23;
----------------#########----

supersedes / removes:
('<vcl.inline>' Line 15 Pos 17)
                "1.3.5.0"/26;
----------------#########----

Adjacent networks, get merged into `1.3.6.0/24`:

                "1.3.6.0"/25;
                "1.3.6.128"/25;

ACL entry:
('<vcl.inline>' Line 16 Pos 17)
                "1.3.6.0"/25;
----------------#########----

left of:
('<vcl.inline>' Line 17 Pos 17)
                "1.3.6.128"/25;
----------------###########----

removing the latter and expanding mask of the former by one to /24

Supernet of two previous entries

                "1.3.0.0"/21;

Addition of this network removes 1.3.4.0/23 from above and 1.3.6.0/24 which we just merged. Note that the VCC reference is still to the source line, even though we changed the netmask. I think this is most POLA, but I am open to naming the changed entry, alternatively:

ACL entry:
('<vcl.inline>' Line 18 Pos 17)
                "1.3.0.0"/21;
----------------#########----

supersedes / removes:
('<vcl.inline>' Line 16 Pos 17)
                "1.3.6.0"/25;
----------------#########----

(That was just a warning)
ACL entry:
('<vcl.inline>' Line 18 Pos 17)
                "1.3.0.0"/21;
----------------#########----

supersedes / removes:
('<vcl.inline>' Line 14 Pos 17)
                "1.3.4.0"/23;
----------------#########----

(That was just a warning)

If you wonder about the order of the messages, this is because more specific ACL entries are found to the left of least specific ones and we walk right to left from the newly inserted entry.

New entry superseded by previously added one

The next Two ACL entries tests that we properly remove new entries by one inserted much earier:

                "1.4.7";
                "1.4.6.0"/24;

ACL entry:
('<vcl.inline>' Line 13 Pos 17)
                "1.4.4.0"/22;
----------------#########----

supersedes / removes:
('<vcl.inline>' Line 19 Pos 17)
                "1.4.7";
----------------#######-

(That was just a warning)
ACL entry:
('<vcl.inline>' Line 13 Pos 17)
                "1.4.4.0"/22;
----------------#########----

supersedes / removes:
('<vcl.inline>' Line 20 Pos 17)
                "1.4.6.0"/24;
----------------#########----

Merges of adjaent entries

The final four ACL entries test that we merge adjacent entries as given, but also the result of merges:

                # right,left adjacent
                "2.3.2.0"/23;
                "2.3.0.0"/23;

ACL entry:
('<vcl.inline>' Line 24 Pos 17)
                "2.3.0.0"/23;
----------------#########----

left of:
('<vcl.inline>' Line 23 Pos 17)
                "2.3.2.0"/23;
----------------#########----

removing the latter and expanding mask of the former by one to /22

                # left,right adjacent
                "2.3.4.0"/23;
                "2.3.6.0"/23;

ACL entry:
('<vcl.inline>' Line 26 Pos 17)
                "2.3.4.0"/23;
----------------#########----

left of:
('<vcl.inline>' Line 27 Pos 17)
                "2.3.6.0"/23;
----------------#########----

removing the latter and expanding mask of the former by one to /22

At this point we have 2.3.0.0/22 and 2.3.4.0/22. Now these two ge merged into 2.3.0.0/21:

ACL entry:
('<vcl.inline>' Line 24 Pos 17)
                "2.3.0.0"/23;
----------------#########----

left of:
('<vcl.inline>' Line 26 Pos 17)
                "2.3.4.0"/23;
----------------#########----

removing the latter and expanding mask of the former by one to /21

Again, the original ACL lines are quotes, despite having been changed to /22. Let me know if you think giving the changed entries would be better.

bsdphk · 2021-03-29T06:13:41Z

Until now we have not done this, under the assumption that users wanted the VSL records to precisely report the entries in the acl.

bsdphk · 2021-04-12T12:13:44Z

I think "prune" is a better name than "merge" ?

Given that ACL's no longer VSL log by default, it is not obvious to me if this optimization is worth doing for performance reasons.

Any performance gain must outweigh the risk of getting things wrong, and the effort to write tests that this is not the case.

nigoroll · 2021-04-12T13:26:05Z

I do not care much about prune/merge. Technically it is actually both: For adjacent networks, It is a merge, for subnets, prune.
I need to report some numbers.

dridi · 2021-04-12T13:35:25Z

How about "fold" instead of merge or ~~purge~~prune?

nigoroll · 2021-04-12T14:01:04Z

I like fold

bsdphk · 2021-11-30T09:28:48Z

I'm very paranoid about our ACL's being correct, so I did a couple of manual tests, and there seems to be an "off-by-8" kind of error:

    "10.0.0.0"/23;
    "10.0.2.0"/23;

    "10.1.0.0"/24;
    "10.1.1.0"/24;

    "10.2.0.0"/25;
    "10.2.0.128"/25;

Gives only:

Message from VCC-compiler:
ACL entry:
('/home/phk/Varnish/trunk/varnish-cache/bin/varnishd/a.vcl' Line 6 Pos 9)
        "10.0.0.0"/23;
--------##########----

left of:
('/home/phk/Varnish/trunk/varnish-cache/bin/varnishd/a.vcl' Line 7 Pos 9)
        "10.0.2.0"/23;
--------##########----

removing the latter and expanding mask of the former by one to /22
(That was just a warning)
ACL entry:
('/home/phk/Varnish/trunk/varnish-cache/bin/varnishd/a.vcl' Line 12 Pos 9)
        "10.2.0.0"/25;
--------##########----

left of:
('/home/phk/Varnish/trunk/varnish-cache/bin/varnishd/a.vcl' Line 13 Pos 9)
        "10.2.0.128"/25;
--------############----

removing the latter and expanding mask of the former by one to /24

nigoroll · 2022-09-17T13:08:38Z

@bsdphk excellent find! I did not sufficiently test this case. :(
I would hope that this did not render the ACL implementation wrong, but certainly a merge opportunity was missed.
I have pushed a fix, but I am not yet sure if I am happy with it, so I will switch this PR to Draft for now.

nigoroll · 2022-09-18T13:00:12Z

@bsdphk I found a much simpler (2-line) fix, which I am happy with. I am leaving it as an extra commit for you to review easily, to be squashed before merge.

nigoroll · 2023-06-13T14:08:35Z

Friendly ping to @bsdphk

nigoroll · 2023-07-11T20:33:17Z

I returned to this old PR after a long time to hopefully finally get it off the list and found a bug which was hidden from my production test by the missed optimization found by phk. The bug was that the left of relation did not consider the fact that the left network always needs to be even to qualify for a mask expansion. The fix is shown specifically in a17140e

I have also renamed the feature to fold as shown in 31c087a

In the latest force-push d2929fa
I have squashed the above, rebased onto d99f62a and polished the commit messages.

This commit is going to production now.

nigoroll

@simonvik's finds from vdd

nigoroll · 2023-09-27T11:35:27Z

doc/sphinx/reference/vcl.rst

@@ -299,6 +299,27 @@ individually:
  However, if the name resolves to both IPv4 and IPv6 you will still
  get an error.

+* `+fold` - Fold ACL supernets and adjacent networks.


needs to be ``

nigoroll · 2023-09-27T11:35:45Z

bin/varnishtest/tests/c00005.vtc

+		set req.http.ip = "10.0.3.255";	call t;
+		set req.http.ip = "10.1.1.255";	call t;
+		set req.http.ip = "10.2.0.255";	call t;
+}


indentation error

nigoroll · 2023-09-27T11:37:25Z

vdd: polish, add ipv6 test, merge

Reverts d89674d Reinstantiates bbb5db4

This is in preparation of a follow-up commit to merge acl entries and detect supersedes from supernets, but these changes are backwards compatible with the previous CMP() if being used as a comparison function for which only negative, zero and positive result are relevant. The A in CMPA() stands for "adjacent". CMPA() returns -3/3 for left of/right of.

Function: With the fold acl flag enabled (default: disabled, keeping the existing behavior), ACLs are optimized in that subnets contained in other entries are skipped (e.g. if 1.2.3.0/24 is part of the ACL, an entry for 1.2.3.128/25 will not be added) and adjacent entries get folded (e.g. if both 1.2.3.0/25 and 1.2.3.128/25 are added, they will be folded to 1.2.3.0/24). Skip and fold operations on VCL entries are output as warnings during VCL compilation as entries from the VCL are processed in order. Logging under the VCL_acl tag can change with this parameter enabled: Matches on skipped subnet entries are now logged as matches on the respective supernet entry. Matches on folded entries are logged with a shorter netmask which might not be contained in the original ACL as defined in VCL. Such log entries are marked by "fixed: folded". Negated ACL entries are excluded from folds. Implementation: The sort functions are changed such that the previous semantics are preserved: negative return values signify "a < b", positive return values signify "a > b". But additionally the values -2/2 and -3/3 are introduced (and given enums) to signify "contained in supernet" and "directly adjacent to". This allows for mostly unchanged code with vcc_acl_fold disabled. For the "subnet contained in supernet" case, all contained subnets are removed. By sort order, caontained subnets are always to be found left of supernets. For the "fold adjacent" case, the netmask of the entry with the smaller network number is decreased by one and the other entry removed. Because changing the netmask might affect sort order, we reinsert the changed entry.

nigoroll added the a=RunByUPLEX This PR is being run by UPLEX in production label Mar 27, 2021

nigoroll force-pushed the acl_merge branch from 8f64b0d to 7c53c8a Compare March 27, 2021 18:13

nigoroll force-pushed the acl_merge branch 2 times, most recently from 99bd954 to 0bf47e1 Compare May 10, 2021 15:28

nigoroll force-pushed the acl_merge branch 6 times, most recently from e275eab to ff27739 Compare November 1, 2021 11:00

nigoroll force-pushed the acl_merge branch 5 times, most recently from 17796fe to 518dc4e Compare February 1, 2022 09:29

nigoroll force-pushed the acl_merge branch 3 times, most recently from df3ae7c to d1baff3 Compare April 27, 2022 11:43

nigoroll force-pushed the acl_merge branch from d1baff3 to f3f0f82 Compare September 17, 2022 13:07

nigoroll marked this pull request as draft September 17, 2022 13:09

nigoroll force-pushed the acl_merge branch from f3f0f82 to cdb5ba5 Compare September 17, 2022 16:42

nigoroll marked this pull request as ready for review September 18, 2022 13:00

nigoroll force-pushed the acl_merge branch 2 times, most recently from 168dcfd to 2f91e8f Compare June 17, 2023 17:11

nigoroll force-pushed the acl_merge branch 3 times, most recently from 1bb9d0d to 3741db7 Compare June 26, 2023 19:24

nigoroll force-pushed the acl_merge branch 4 times, most recently from 82a9afd to b28a84d Compare July 6, 2023 17:21

nigoroll force-pushed the acl_merge branch from 97da9f9 to 31c087a Compare July 11, 2023 13:34

nigoroll force-pushed the acl_merge branch from 31c087a to d2929fa Compare July 11, 2023 20:33

nigoroll changed the title ~~Add vcc_acl_merge~~ acl +fold Jul 11, 2023

nigoroll force-pushed the acl_merge branch from d2929fa to 05b377a Compare July 17, 2023 14:34

nigoroll force-pushed the acl_merge branch 2 times, most recently from 8335a43 to 4427a0b Compare August 1, 2023 16:35

nigoroll force-pushed the acl_merge branch from 4427a0b to 2c8b73f Compare August 8, 2023 11:22

nigoroll force-pushed the acl_merge branch from 2c8b73f to bbd5fd5 Compare August 21, 2023 12:30

nigoroll force-pushed the acl_merge branch 2 times, most recently from dd9bf45 to d91c286 Compare September 14, 2023 14:41

nigoroll commented Sep 27, 2023

View reviewed changes

nigoroll force-pushed the acl_merge branch from d91c286 to f64e9ce Compare October 2, 2023 19:58

nigoroll force-pushed the acl_merge branch from f64e9ce to cde22ac Compare December 4, 2023 11:31

nigoroll added 3 commits December 4, 2023 12:31

Split out vcc_acl_insert_entry() again

245c768

Reverts d89674d Reinstantiates bbb5db4

nigoroll force-pushed the acl_merge branch from cde22ac to 01bf0dd Compare December 4, 2023 11:31

nigoroll merged commit 01bf0dd into varnishcache:master Dec 4, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

acl +fold #3563

acl +fold #3563

nigoroll commented Mar 27, 2021

bsdphk commented Mar 29, 2021

bsdphk commented Apr 12, 2021

nigoroll commented Apr 12, 2021

dridi commented Apr 12, 2021 •

edited

Loading

nigoroll commented Apr 12, 2021

bsdphk commented Nov 30, 2021

nigoroll commented Sep 17, 2022

nigoroll commented Sep 18, 2022

nigoroll commented Jun 13, 2023

nigoroll commented Jul 11, 2023

nigoroll left a comment

nigoroll Sep 27, 2023

nigoroll Sep 27, 2023

nigoroll commented Sep 27, 2023

acl +fold #3563

acl +fold #3563

Conversation

nigoroll commented Mar 27, 2021

The second entry is a subnet of the first

Entry inserted as is, becomes relevant later

Another subnet case

Adjacent networks, get merged into 1.3.6.0/24:

Supernet of two previous entries

New entry superseded by previously added one

Merges of adjaent entries

bsdphk commented Mar 29, 2021

bsdphk commented Apr 12, 2021

nigoroll commented Apr 12, 2021

dridi commented Apr 12, 2021 • edited Loading

nigoroll commented Apr 12, 2021

bsdphk commented Nov 30, 2021

nigoroll commented Sep 17, 2022

nigoroll commented Sep 18, 2022

nigoroll commented Jun 13, 2023

nigoroll commented Jul 11, 2023

nigoroll left a comment

Choose a reason for hiding this comment

nigoroll Sep 27, 2023

Choose a reason for hiding this comment

nigoroll Sep 27, 2023

Choose a reason for hiding this comment

nigoroll commented Sep 27, 2023

Adjacent networks, get merged into `1.3.6.0/24`:

dridi commented Apr 12, 2021 •

edited

Loading