Nonterminating rules #200
Merged
Nonterminating rules #200
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ol-imorozko
force-pushed
the
check-state
branch
2 times, most recently
from
c002ebd to
ef0183f
Compare
ol-imorozko
force-pushed
the
check-state
branch
6 times, most recently
from
f78b993 to
61db068
Compare
ol-imorozko
force-pushed
the
check-state
branch
7 times, most recently
from
10f8875 to
9309dfe
Compare
ol-imorozko
force-pushed
the
check-state
branch
2 times, most recently
from
0e3f90c to
5fafefc
Compare
autotest/units/001_one_port/036_firewall_keepstate/firewall.txt
Outdated
Show resolved
Hide resolved
| @@ -199,20 +200,26 @@ struct Action | |||
| } | |||
| }; | |||
|
|
|||
| namespace acl | |||
There was a problem hiding this comment.
The idea is not clear to me so the question is why we need to have two kinds on action list instead of include check-state into the list.
Additional point here - we want to introduce more different actions (like dump, count, log, etc.) and I would like to process them all in a common way.
There was a problem hiding this comment.
TLDR: that is to avoid extra paths in dataplane.
check-state is a unique action cause it should return some value, which we need to check. If we were to introduce that in the common handler function, this can drastically slow down the dataplane cause we will need to perform checks like if (!retval).
When we create Actions objects before passing them to the dataplane, we know whether the check-state rule will be in the actions to be executed. Therefore, the Actions itself is a std::variant denoting whether we will execute check-state or not.
When we get matched onto a group with potential check-state in either case we have to, well, check the state, so this is done at the very beginning. If the check is successful, we get flow and execute the actions before check-state. This is implemented via a separate vector, because that eliminates the need for unnecessary checks in dataplane like if current_action != check-state action. If the check fails, we go to the standard path.
Well, we already process all actions in common way -- by calling the respective execute function. To introduce a new action is to just add it to a parser and a common code for parsing actions, and implement that execute function.
|
Could the PR be split into two parts:
|
This was
linked to
issues
Aug 27, 2024
ol-imorozko
force-pushed
the
check-state
branch
3 times, most recently
from
b838992 to
953ee35
Compare
TheRandomCharacter
previously requested changes
Aug 28, 2024
ol-imorozko
force-pushed
the
check-state
branch
from
9d947f2 to
6131b7d
Compare
I have no idea how that worked previously, we don't have an "src" folder in controlplane.
First of all, is_nonterm makes implementation a bit more complex, as everywhere we use !is_nonterm, which does not makes sense. It would be better to just use is_term instead. Next, we don't need this as a static function defined in acl.cpp, as acl_compiler.cpp also wants the same functionality to fill `rule.terminating` field. So we make this a rule_t method.
Previous code was ignoring rules with too large dump_id at value::compile step, but this can be done way earlier
Introduce rule_action alias for std::variant to simplify code that will be in the next commits. Removed code duplication by extracting common initialization logic into a separate method.
It will be better to have all action-related definitions in a separate file
Conveniently, parser tokens were already present in the codebase, so we don't have to change them. Although we discussed that we might want to enrich our ipfw syntax by allowing the check-state action to go with filters like `add check-state tcp from X to Y`, this is a task for a future improvement. For now, check-state rules do not hold any information aside from the fact that a check-state should occur. Therefore, the related structure does not have any fields and is effectively a minimal one-byte struct. Consequently, we need some value to uniquely identify and hash it.
The class offers serialization/deserialization but is too restrictive. The `pop` method interface, which returns objects by reference in an argument, prevents usage with non-DefaultConstructible objects. This limitation means we cannot deserialize an object directly without first creating a default instance.
The previous implementation used vector's swap method, which was a strange way to pass a vector without copying. Using std::move achieves the same result more appropriately by transferring ownership of the vector.
These fields are unused so remove them
There are no such class anywhere whatsoever, so we don't need these lines too
This class definition is provided by common/type.h anyway
The value_t class previously represented a sequence of actions to be performed in the dataplane upon packet match. It contained a list of dump_ids and a flow (allow/deny/etc.), which was awkward as not all fields were necessary if certain rule types weren't matched. Additionally, adding new classes for new rule types was inconvenient. The new Actions class addresses these issues by storing a vector of "abstract" Actions that are matched and executed in the dataplane one by one, as intended. This makes the compilation of total_table (mapping from group_id to the actions to be performed for the packet) readable and efficient.
Total_table compile will use this. Idk if it's already presented. probably. Also, I think that we don't need groups to be ordered, we can just use vector, I guess (need to check this when semantics will be done, check whether all tests pass)
The previous implementation used a `value_t` structure to represent
actions required for packet processing. This approach was cumbersome
when adding new functionality, as it required modifications
to a monolithic structure.
Changes:
- Replaced `value_t` with a more flexible `Actions` structure.
- Removed the outdated `value_t` definition and its associated methods.
- Simplified the algorithm for filling {group_id, acl_id} by directly iterating
over groups and not through each rule_id (time complexity has not changed,
as we still need to iterate over all rule_ids, but we do that on "prepare" step now)
Actions are now managed in a more modular and decoupled manner.
This structure lost it's meaning as one struct to store all non-terminating rule data. This is not a good thing anyway as tHis will pollute the abstraction and the class size will grow when we will introduce new nonterminating kind of rules. Defining a new struct/class for each rule kind and then using it in an std::variant is a preffered approach. Therefore this class should not be called "action_t" but rather "dump_t"
Since value_t changed from a monolithic class with all data required to perform all matched actions on a packet to a vector of individual actions, we need to adjust unittests to use the new get_flow() method.
Add ActionDispatcher class to implement specific semantics for executing actions within the dataplane. FIXME: There is an uncertainty regarding the best way to execute actions in the dataplane, it involves passing numerous parameters to the execute method.
This change will help in implementing proper check-state semantics in the future. Previously, the code was checking state and immediately performing egress/ingress flow. With this update, we will retrieve the flow and perform it after executing nonterminating rules.
Well, they are checking the state, so let's call them like that
This commit implements proper check-state semantics for the packet processing logic: Introduced the `BaseActions` class with two specializations: - `BaseActions<true>`: Handles action sequences that contain a check-state action. - `BaseActions<false>`: Handles action sequences that do not contain a check-state action. Updated the `ActionDispatcher` to use `std::variant` to differentiate between `BaseActions<true>` and `BaseActions<false>`. This reduces runtime branching by making compile-time decisions. Refactored the `execute_impl` method in `ActionDispatcher` to check the state and decide the action path based on the presence of a check-state action. If a check-state action is present and the state check succeeds, it executes the check-state path. The final flow action is handled by the CheckStateAction execute method.
check-state is no longer an ignored option
Changed unittests to use std::visit for accessing get_flow() due to acl_values now being a std::variant of BaseActions<true> and BaseActions<false>.
These tests ensure that: - The BaseActions<true> specialization is used when check-state is present. - The check-state path includes the check-state action at the end. - The regular path does not include the check-state action. - We're ignoring multiple check-states and only looking at the first one.
Makes it easier to debug
ol-imorozko
force-pushed
the
check-state
branch
from
6131b7d to
5c5e711
Compare
GeorgyKirichenko
approved these changes
Sep 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.