Skip to content

conf-yaml: support glob patterns in include directive (v2) - #15574

Closed
ssam18 wants to merge 1 commit into
OISF:mainfrom
ssam18:feat-8427-include-glob-v2
Closed

conf-yaml: support glob patterns in include directive (v2)#15574
ssam18 wants to merge 1 commit into
OISF:mainfrom
ssam18:feat-8427-include-glob-v2

Conversation

@ssam18

@ssam18 ssam18 commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Continuation of #15573 (v2). See "Changes since #15573" below.

Contribution style:

Our Contribution agreements:

Changes (if applicable):

Link to ticket: https://redmine.openinfosecfoundation.org/issues/8427

Describe changes:

SCConfYamlHandleInclude now expands shell-style glob patterns (*, ?, [) via glob(3) when the include path contains them. Each match is loaded in lexicographic (sorted) order; literal paths bypass glob() and take the existing per-file path. A pattern matching zero files is logged as a warning, not an error, so drop-in conf.d/-style directories may be empty. The precedent for the glob(3) shape and HAVE_GLOB_H gating is rule-files: in src/detect-engine-loader.c.

Changes since #15573

  • Moved the test coverage out of C unit tests and into suricata-verify, as requested in review (victorjulien, jasonish). The two HAVE_GLOB_H unit tests (ConfYamlFileIncludeGlobTest, ConfYamlFileIncludeGlobNoMatchTest) and their registration are removed.
  • The new suricata-verify test (config-includes-glob-order) specifically asserts that glob matches are included in deterministic lexicographic order (the 01..99 drop-in convention jasonish asked about): three drop-ins set the same key and the highest-prefix file wins under --dump-config.
  • Removing those C unit tests also resolves the two CodeQL "file created without restricting permissions (mode 0666)" High warnings, which were raised on the fopen(..., "w") calls inside the removed tests.

Provide values to any of the below to override the defaults.

SV_BRANCH=OISF/suricata-verify#3151

SCConfYamlHandleInclude only accepted explicit filenames. The
rule-files directive has supported shell-style glob expansion via
glob(3) for years, and asymmetry with include has been blocking
clean drop-in conf.d/ style configuration directories.

Refactor SCConfYamlHandleInclude into a thin wrapper that resolves
the path against conf_dirname and, if the input contains glob
metacharacters (*, ?, [), expands it with glob(3) and includes
each match in lexicographic order. A pattern that matches no
files is logged as a warning and not treated as an error, so a
drop-in directory can be empty without breaking startup. Literal
paths skip glob() and call the existing per-file inclusion logic
directly, preserving current behaviour.

The behaviour is verified by the config-includes-glob-order
suricata-verify test, which checks that every file matching a
pattern is loaded and that matches are included in deterministic
lexicographic order.

Document the new behaviour in doc/userguide/configuration/
includes.rst.

Feature: OISF#8427.
@codecov

codecov Bot commented Jun 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.75862% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.86%. Comparing base (899eb38) to head (8840a1f).
⚠️ Report is 235 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #15574      +/-   ##
==========================================
- Coverage   82.88%   82.86%   -0.03%     
==========================================
  Files         999      998       -1     
  Lines      272613   271884     -729     
==========================================
- Hits       225968   225298     -670     
+ Misses      46645    46586      -59     
Flag Coverage Δ
fuzzcorpus 61.48% <58.62%> (+<0.01%) ⬆️
livemode 18.46% <31.03%> (+0.03%) ⬆️
netns ?
pcap 45.24% <0.00%> (+0.01%) ⬆️
suricata-verify 66.60% <72.41%> (+<0.01%) ⬆️
unittests 58.51% <31.03%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.


Filenames in ``include`` may contain shell-style glob metacharacters
(``*``, ``?``, ``[...]``). Patterns are expanded at startup via
``glob(3)`` and each matching file is loaded in lexicographic order. A

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What does the 3 mean in glob(3) ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

man 3 glob
image

@catenacyber

Copy link
Copy Markdown
Contributor

This looks good to me but I left some remarks on the SV PR

@catenacyber

Copy link
Copy Markdown
Contributor

This looks good to me but I left some remarks on the SV PR

Jason and I do not agree on the desired behavior, so I leave the decision to him

@jasonish jasonish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The Windows failures are real:

checking for glob.h... no

There are a few options here.

  • Ideally figure out if we're just missing a package in msys2?
  • Document and accept that globbing doesn't work on Windows for now. Ideally with a warning message if the patterns are found.

Before 9 I plan to finish my Rust based loader and parser which would fix the Windows issue (I think), so we could just wait for that and accept no support on Windows for now.

@ssam18

ssam18 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #15937 (v3), which addresses @jasonish's review: an include pattern is now detected regardless of HAVE_GLOB_H and, on platforms without glob(3), logged with a warning and skipped instead of being opened as a literal filename. The platform limitation is documented in the user guide. v3 is also rebased onto current main.

A new PR rather than a push here, per the workflow: a branch should not be updated after its pull request is opened.

The paired suricata-verify PR is now OISF/suricata-verify#3250 (v2 of OISF/suricata-verify#3151).

For the record on the CI failures here: the three Windows/Ubuntu failures date from 2026-06-07 and are stale. The suricata-verify test was rewritten on 2026-06-12 to stop shipping a suricata.yaml, which is what broke suricata-verify's setup step on Windows, and the Ubuntu 24.04 (afpacket IPS tests in namespaces) failure was a transient Codecov CLI GPG error unrelated to the change.

Closing in favour of #15937. Thanks @catenacyber and @jasonish for the reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants