config: test deterministic glob include ordering - #3151
Conversation
3cd34e5 to
3dbb6dd
Compare
|
The 3 failing builds here are expected for now. This test exercises glob expansion in the config include directive, which is added by Suricata PR OISF/suricata#15574 and is not in suricata master yet. When suricata-verify builds suricata from master, the include is read as a literal filename, the file is not found, and suricata exits 1. That suricata PR sets SV_BRANCH to this branch, so its own CI builds the glob feature and runs this test against it, where it passes. Running the same command locally against the feature build also exits 0 with the expected lexicographic ordering. This PR is meant to land together with or right after #15574, and once the feature is in master these builds will pass with no further changes. |
| # Ordering is deterministic: glob(3) returns matches sorted lexicographically, | ||
| # so the highest numeric prefix (99) is included last and wins. | ||
| - shell: | ||
| args: grep '^glob-order-test = file-99' stdout | wc -l |
There was a problem hiding this comment.
How does this check test the order ?
There was a problem hiding this comment.
Oh, this looks bad to have the test rely on duplicated keys behavior...
There was a problem hiding this comment.
Why should we care about the order ?
There was a problem hiding this comment.
Why did you mark this conversation as resolved without commenting ?
There was a problem hiding this comment.
Sorry for resolving without replying first. The test has been rewritten since those comments. Each drop-in file now sets its own unique marker key, and the final shell check greps the merged dump for those keys in sequence and expects them in sorted-prefix order. So the test now checks the actual glob expansion order without depending on which value wins when keys collide, and the README explains why ordered drop-ins matter for the numbered conf.d-style convention.
There was a problem hiding this comment.
If the team's position is that Suricata should not guarantee include order, I'm happy to go the other way, but then the right change is to drop the "lexicographic order" sentence from the user guide in #15574 and reduce this test to "all matching files are included." I think guaranteeing order is the more useful contract for users, so I'd prefer to keep both.
Predictable order should be a must. Probably much like udev rules, or older sysv init. 50-zzz.yaml should be included before 90-aaa.yaml in a globbing pattern.
There was a problem hiding this comment.
Why so @jasonish ?
For me the yaml is like a set of key/values without order (like a golang map)
There was a problem hiding this comment.
Thats true for pure YAML @catenacyber, but once you introduce includes, its no longer pure YAML.
So if you have one include:
sensor-name: foobar
and a second include
sensor-name: barfoo
the last include is going to overwrite the previous key, so deterministic with some level of user control would be desired when building up includes. This is already easily done by ordering the includes, but if we're going to glob them, we should carry over some way to order them as well.
There was a problem hiding this comment.
I do not agree with this desired behavior : I think we should fail if 2 includes redefine some key
But you may know better, so I defer the decision to you
There was a problem hiding this comment.
We already support this, and a warning is output when you redefine a key.
A subsequent, new object like vars redefines the whole previous vars section. You can do vars.address-groups.HOME_NET: ... later in the suricata.yaml, or in an include to override just the field.
As this is in use in production these days, by integrators, it's not something we can change. It also means predictable ordering of includes can be very beneficial.
The work was done pre-SV so there aren't many tests about it, and its only lightly documented here: https://docs.suricata.io/en/suricata-8.0.6/configuration/suricata-yaml.html#splitting-configuration-in-multiple-files
d531ad0 to
b8e7d55
Compare
Add config-includes-glob-order, which checks that a glob pattern in an include directive expands to all matching files and includes them in sorted order. glob.yaml includes *-order.yaml, matching three drop-in files (01/50/99) that each define their own unique marker key. The test asserts that all three keys are present in --dump-config output and that they appear in sorted-filename order, observable because the config tree preserves insertion order. No key is defined twice, so the test does not rely on duplicate-key override behavior. Include order matters because settings from later includes override earlier ones, which is what makes the numbered 01..99 drop-in convention predictable for users. The glob config is kept in glob.yaml and run via command: rather than a shipped suricata.yaml, because suricata-verify runs "suricata -c suricata.yaml --dump-config" during test setup before the min-version check. A glob include in suricata.yaml would break that setup step on Suricata versions without glob support. Until the Suricata side lands, a requires script greps src/conf-yaml-loader.c for the glob expansion code and skips the test on builds without it, the same way the eve-alert-metadata tests probe for METADATA_DEFAULTS. This keeps CI green against main and the release branches. Exercises the include glob support added in OISF/suricata for this ticket. Ticket: #8427
b8e7d55 to
c8f1b45
Compare
|
@jasonish that's exactly the scenario this test is meant to protect. I kept the marker keys unique per @catenacyber's earlier feedback, so the test pins just the sort order without relying on override behavior, but sorted expansion is what makes your sensor-name example predictable when the includes come from a glob. On the red CI: the main jobs were failing because the glob support itself (OISF/suricata#15574) isn't merged yet, not because of the test logic. I've updated the test with a requires script that greps conf-yaml-loader.c for the glob expansion code and skips on builds without it, same approach the eve-alert-metadata tests use for METADATA_DEFAULTS. Verified locally: skips against current main (no match on 7.0.x/8.0.x either), passes against a build of the 15574 branch. |
|
Depending on OISF/suricata#15574 (review), this might need a not runs on Windows requirement. |
|
Superseded by #3250 (v2), which adds the Windows requirement @jasonish asked about:
It gates on the build rather than the platform — Everything else is unchanged from this PR, including the ordering checks discussed above. The paired suricata PR is now OISF/suricata#15937 (v3 of OISF/suricata#15574), which is what A new PR rather than a push here, per the workflow: a branch should not be updated after its pull request is opened. Closing in favour of #3250. Thanks @catenacyber and @jasonish for the review and the discussion on ordering. |
3 participants
Important
Depends on OISF/suricata#15574 (the
include:glob feature, Redmine #8427). This test exercises that feature, so the CI here will stay red until it is merged intoOISF/suricata: suricata-verify CI builds suricata from the official branches (master/main-7.0.x/main-8.0.x), none of which have the feature yet, sosuricata --dump-configfails on the globinclude:. The pair is validated in #15574's CI (which builds the feature and runs this exact branch viaSV_BRANCH) and locally.Adds
config-includes-glob-order, a test for theinclude:glob-pattern support added in OISF/suricata#15574 (Redmine #8427).suricata.yamlincludes*-order.yaml, which matches three drop-in files (01-order.yaml,50-order.yaml,99-order.yaml) that each set the same keyglob-order-testto a different value. Becauseglob(3)returns matches in sorted (lexicographic) order, the files are processed in ascending numeric-prefix order and the highest-numbered file is applied last — matching the common01..99drop-in directory convention.Using
--dump-config, the test asserts:glob-order-testisfile-99(last in sorted order), not the baseline or an earlier file.This addresses review feedback on the suricata PR (victorjulien/jasonish) requesting the include tests live in suricata-verify, with an explicit check that include ordering is deterministic.
Ticket: https://redmine.openinfosecfoundation.org/issues/8427
Verified locally against a
9.0.0-devbuild with the glob feature:===> config-includes-glob-order: OK.