test(engine): cover rejection of empty appended rules by Jvlegod · Pull Request #3935 · falcosecurity/falco

Jvlegod · 2026-07-02T08:31:06Z

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area engine

/area tests

What this PR does / why we need it:

This PR adds a unit test covering the rejection of append: true rules that do not actually append anything (no condition and no exceptions).

The rule loader collector already rejects them with LOAD_ERR_VALIDATE ("Appended rule must have exceptions or condition property"), but the case had no test coverage. The commented-out reader-level check is left as is, since restoring it would only change the reported error code to LOAD_ERR_YAML_VALIDATE, which is not what falcosecurity/testing expects.

N.B. Scope reduced to the test on September 7th by the maintainers, as agreed in the review (see the comments below).

Which issue(s) this PR fixes:

N/A

Special notes for your reviewer:

Tested with:

cmake -S . -B build-agent -DMINIMAL_BUILD=On -DBUILD_DRIVER=Off -DBUILD_FALCO_UNIT_TESTS=On
cmake --build build-agent --target falco_unit_tests
sudo ./build-agent/unit_tests/falco_unit_tests

Does this PR introduce a user-facing change?:

NONE

poiana · 2026-07-02T08:31:16Z

Welcome @Jvlegod! It looks like this is your first PR to falcosecurity/falco 🎉

github-actions · 2026-07-02T08:31:23Z

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

Jvlegod · 2026-07-02T08:44:40Z

Maybe I don't think FALCO_ENGINE_VERSION needs to be bumped.

leogr · 2026-07-22T15:05:59Z

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

False positive
/hold cancel

poiana · 2026-09-03T18:57:57Z

LGTM label has been added.

Details

Git tree hash: c623d718650d68e1f58e51908cd3829069000546

leogr

Hey @Jvlegod

Thanks for this PR, and sorry for the long wait!

I had a closer look at the red test-packages jobs. Apart from the 11 UTF-8 tests (unrelated, they fail only because the merge commit predates #3942), there is one real failure caused by this change: TestFalco_Legacy_InvalidAppendRuleWithoutCondition in falcosecurity/testing expects LOAD_ERR_VALIDATE, while the THROW in the reader emits LOAD_ERR_YAML_VALIDATE.

For the record, master already rejects an appended rule without condition/exceptions: in the append branch the reader reads only those two properties, so the collector's has_any_value() check fails and it throws the very same message (see rule_loader_collector.cpp), with LOAD_ERR_VALIDATE. So re-enabling the check in the reader does not change what is accepted or rejected. It only changes the error code, and the order in which two errors are reported (an append to an unknown rule with no condition would now report "Appended rule must have..." instead of "no rule by that name already exists").

That said, the new unit test is valuable, since we had no coverage for this case. So, may you drop the rule_loader_reader.cpp change and keep the tests? They should pass on master as is, since check_error_message() matches the message text. See also my inline comment about the renamed test.

This way we also avoid a cross-repo dance with falcosecurity/testing, whose main is used by the CI of every Falco PR.

Thanks 🙏

leogr · 2026-09-04T13:22:35Z

+			THROW((!v.cond.has_value() && !v.exceptions.has_value()),
+			      "Appended rule must have exceptions or condition property",
+			      v.ctx);


This is where the error code changes: the THROW macro in this file uses LOAD_ERR_YAML_VALIDATE, while the collector already throws the same message with LOAD_ERR_VALIDATE for exactly this case (rule_loader_collector.cpp, has_any_value() check in append()).

May you drop this hunk, leaving the file as it is on master? Otherwise TestFalco_Legacy_InvalidAppendRuleWithoutCondition in falcosecurity/testing keeps failing, since it asserts LOAD_ERR_VALIDATE.

leogr · 2026-09-04T13:22:35Z


-// todo!: Probably we shouldn't allow this syntax
-TEST_F(test_falco_engine, rule_enabled_is_ignored_by_append) {
+TEST_F(test_falco_engine, rule_enabled_is_rejected_by_append) {


Nit. The new name says that enabled is rejected, but what gets rejected here is the missing condition/exceptions (an append with a condition and enabled: true still loads fine). This makes it nearly identical to empty_append_rule_is_rejected below.

May you restore rule_enabled_is_ignored_by_append as it is on master (it's the only test documenting that enabled is ignored on append), and keep empty_append_rule_is_rejected as the new coverage?

Jvlegod · 2026-09-04T15:19:31Z

Of course, I will submit the changes shortly.

leogr · 2026-09-07T07:10:43Z

Of course, I will submit the changes shortly.

Thanks, let us know! 🙏

An appended rule (`append: true`) with neither `condition` nor `exceptions` is already rejected by the rule loader collector, via the `has_any_value()` check in `rule_loader::collector::append()`, with error code `LOAD_ERR_VALIDATE`. However, no unit test covered this case. Add `empty_append_rule_is_rejected` to lock the behavior in. The commented-out reader-level check is left as is, since restoring it would only change the reported error code to `LOAD_ERR_YAML_VALIDATE`. Signed-off-by: Keke Ming <ming.jvle@gmail.com> Signed-off-by: Leonardo Grasso <me@leonardograsso.com>

leogr · 2026-09-07T11:24:25Z

Hey @Jvlegod

Since we'd like to have this in Falco 0.45.0 and the code freeze is close, I went ahead and applied the change we agreed on Sep 4 (see #3935 (review)), and force-pushed to your branch. You are still the author of the commit, I just added my sign-off next to yours. I hope you don't mind 🙏

What changed, in short:

dropped the rule_loader_reader.cpp hunk, since the collector already rejects an appended rule without condition/exceptions with the same message and with LOAD_ERR_VALIDATE, which is what falcosecurity/testing expects
kept your new empty_append_rule_is_rejected test as is
restored rule_enabled_is_ignored_by_append exactly as it is on master
rebased on the current master, so the unrelated UTF-8 test failures go away too
reworded the commit message to test(engine): cover rejection of empty appended rules, since the PR is now test-only, and updated the PR title and description accordingly

The new test is the valuable part here, since we had no coverage for this case. Thanks again for the contribution!

N.B. The force-push dropped the lgtm, so the PR needs a fresh round of approvals. Nothing needed on your side.

/kind test
/remove-kind bug

poiana · 2026-09-07T11:24:30Z

@leogr: The label(s) kind/test cannot be applied, because the repository doesn't have them.

Details

In response to this:

Hey @Jvlegod

Since we'd like to have this in Falco 0.45.0 and the code freeze is close, I went ahead and applied the change we agreed on Sep 4 (see #3935 (review)), and force-pushed to your branch. You are still the author of the commit, I just added my sign-off next to yours. I hope you don't mind 🙏

What changed, in short:

dropped the rule_loader_reader.cpp hunk, since the collector already rejects an appended rule without condition/exceptions with the same message and with LOAD_ERR_VALIDATE, which is what falcosecurity/testing expects

kept your new empty_append_rule_is_rejected test as is

restored rule_enabled_is_ignored_by_append exactly as it is on master

rebased on the current master, so the unrelated UTF-8 test failures go away too

reworded the commit message to test(engine): cover rejection of empty appended rules, since the PR is now test-only, and updated the PR title and description accordingly

The new test is the valuable part here, since we had no coverage for this case. Thanks again for the contribution!

N.B. The force-push dropped the lgtm, so the PR needs a fresh round of approvals. Nothing needed on your side.

/kind test
/remove-kind bug

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

leogr · 2026-09-07T11:26:02Z

/kind cleanup

poiana · 2026-09-07T11:39:06Z

LGTM label has been added.

Details

Git tree hash: aa541d297c0fcae6e4d66c6fe0d31f01484153d0

poiana · 2026-09-07T12:20:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jvlegod, leogr, sgaist

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [leogr,sgaist]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

github-project-automation Bot added this to Falco Roadmap Jul 2, 2026

github-project-automation Bot moved this to Todo in Falco Roadmap Jul 2, 2026

poiana added dco-signoff: yes kind/bug area/engine do-not-merge/release-note-label-needed labels Jul 2, 2026

poiana requested review from Kaizhe and irozzo-1A July 2, 2026 08:31

poiana added the area/tests label Jul 2, 2026

poiana added size/M do-not-merge/hold release-note-none and removed do-not-merge/release-note-label-needed labels Jul 2, 2026

poiana removed the do-not-merge/hold label Jul 22, 2026

leogr added this to the 0.45.0 milestone Aug 31, 2026

sgaist previously approved these changes Sep 3, 2026

View reviewed changes

poiana assigned sgaist Sep 3, 2026

poiana added lgtm approved labels Sep 3, 2026

leogr reviewed Sep 4, 2026

View reviewed changes

leogr mentioned this pull request Sep 7, 2026

[TRACKING] Falco 0.45.0 release #3967

Open

leogr dismissed sgaist’s stale review via 05bf224 September 7, 2026 11:23

leogr force-pushed the fix-empty-appended-rules branch from de50619 to 05bf224 Compare September 7, 2026 11:23

poiana removed the lgtm label Sep 7, 2026

poiana requested a review from sgaist September 7, 2026 11:23

poiana added size/S and removed approved size/M labels Sep 7, 2026

leogr changed the title ~~fix(engine): reject empty appended rules~~ test(engine): cover rejection of empty appended rules Sep 7, 2026

poiana removed the kind/bug label Sep 7, 2026

poiana added the kind/cleanup label Sep 7, 2026

sgaist approved these changes Sep 7, 2026

View reviewed changes

poiana added the lgtm label Sep 7, 2026

poiana added the approved label Sep 7, 2026

leogr approved these changes Sep 7, 2026

View reviewed changes

github-project-automation Bot moved this from Todo to In progress in Falco Roadmap Sep 7, 2026

poiana assigned leogr Sep 7, 2026

poiana merged commit e2acedb into falcosecurity:master Sep 7, 2026
34 checks passed

github-project-automation Bot moved this from In progress to Done in Falco Roadmap Sep 7, 2026

Conversation

Jvlegod commented Jul 2, 2026 • edited by leogr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poiana commented Jul 2, 2026

Uh oh!

github-actions Bot commented Jul 2, 2026

Uh oh!

Jvlegod commented Jul 2, 2026

Uh oh!

leogr commented Jul 22, 2026

Uh oh!

poiana commented Sep 3, 2026

Uh oh!

leogr left a comment

Choose a reason for hiding this comment

Uh oh!

leogr Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

leogr Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

Jvlegod commented Sep 4, 2026

Uh oh!

leogr commented Sep 7, 2026

Uh oh!

leogr commented Sep 7, 2026

Uh oh!

poiana commented Sep 7, 2026

Uh oh!

leogr commented Sep 7, 2026

Uh oh!

poiana commented Sep 7, 2026

Uh oh!

poiana commented Sep 7, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Jvlegod commented Jul 2, 2026 •

edited by leogr

Loading