Allow overwriting existing macros with conditions defined after them

[wdoekes]

Thanks to @fntlnz for providing a minimal working example.

Now allowed:

- macro: something_happens
  condition: (evt.num=0)

- macro: something_else
  condition: (evt.num=1)

# This -- usually seen in a local file -- was previously not
# allowed, because during resolution of the /first/ something_happens,
# the /then/ unknown something_else would be encountered.
- macro: something_happens
  condition: (evt.num=2) or (something_else)

/kind bug

/area rules

Fixes #706

Does this PR introduce a user-facing change?:

fix: allow overwriting existing macros with conditions defined after them

--

P.S. Unrelated to this PR. When I opened a PR, I got these tips:

1. If this is your first time, please read our contributor guidelines in the github.com/falcosecurity/.github/blob/master/CONTRIBUTING.md file and learn how to compile Falco from source falco.org/docs/source

If I wanted tips on how to compile Falco, I think I wouldn't be at the pull request stage. So that's weird. Also, the mentioned URL gives a 404.

The tip that isn't mentioned, but is found in the CONTRIBUTING docs, is the -s signed-off that is apparently mandatory. Maybe put that here.

[poiana]

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

[poiana]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wdoekes
To complete the pull request process, please assign leogr after the PR has been reviewed.
You can assign the PR to them by writing /assign @leogr in a comment when ready.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[jasondellaluce]

This is a great step forward. I haven't tested this, but my only concern is that this would also allow cases like this to be accepted:

- macro: something_happens
  condition: (something_else)

- macro: something_else
  condition: (evt.num=1)

Which kind brokes the ordering semantics. Did you test this specific case?

[leogr]

Hey @wdoekes

Thank you for this PR!
I've just updated the PR description since we need the release-note block. Also, I added the release note text, since this PR is actually fixing an issue.

[leogr]

/milestone 0.31.0

[wdoekes]

This is a great step forward. I haven't tested this, but my only concern is that this would also allow cases like this to be accepted:

Yes. It accepts that case. I haven't tested it, but that's what the code (also) implies.

Is there a reason why we would not want that?

If there is, then the fix would've been to simply insert updated macros at the end of ordered_macros when loading them from yaml. Apparently there is a reason why you want to keep some kind of ordering. But I'm in the dark about what reasons you have. Keeping the original order whilst allowing the bugfix, but not your suggested case, seems to me like trying to fit a round peg through a square hole.

[leogr]

Although I like this fix, I believe we have to rethink the whole sematic a bit. So, I'm moving it to the next milestone
/milestone 0.32.0

[jasondellaluce]

Hey @wdoekes, I was finally able to work on a refactoring of the rule loader 👉🏼 #1966
Right now, all that is done is porting the existing loader logic from Lua to C++, but some bugs have been fixed in the meanwhile, including the one you're tackling here I think. It would be great if you could take a look!

[leogr]

I'm closing this PR in favor of #1966 (that's already merged).
/close

[poiana]

@leogr: Closed this PR.

In response to this:

I'm closing this PR in favor of #1966 (that's already merged).
/close

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/test-infra repository.

[wdoekes]

I was finally able to work on a refactoring of the rule loader 👉🏼 #1966

Okay. I was finally able to test it. It works 👍