DRAFT: tc flower support #111
Draft
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
daniel-noland
force-pushed
the
tc-flower
branch
15 times, most recently
from
4ef077d
to
80651ea
Compare
cathay4t
reviewed
Apr 24, 2024
src/tc/actions/message.rs
Outdated
| #[non_exhaustive] | ||
| pub struct TcActionMessage { | ||
| pub header: TcActionHeader, | ||
| pub attributes: Vec<TcActionMessageNla>, |
There was a problem hiding this comment.
We do not use Nla in public structure anymore, try TcActionAttribute
There was a problem hiding this comment.
Ah, that makes sense.
I'll finish up the last few flower keys and then adjust the names while rebasing and cleaning up the commit history into some kind of sane and more easily reviewed structure.
cathay4t
requested changes
Apr 24, 2024
Will do, and thanks for the review. I'll finish up the last few keys, restructure the code into a more easily reviewed and ergonomic form (I think I will also add tests as per the pattern used in the rest of this library. That may take a while because this is a fairly large feature and getting reasonable coverage will require a large number of tests. I have also been testing this as I go using a modified form of the downstream rtnetlink library. Those modifications and tests are currently quite hacky however. I will clean up my modifications to rtnetlink and push a draft PR to that repo. That way we can consider the API design impacts on rtnetlink resulting from this PR before we merge anything. |
daniel-noland
force-pushed
the
tc-flower
branch
3 times, most recently
from
412cebb
to
50886ed
Compare
2 tasks
daniel-noland
changed the title
daniel-noland
force-pushed
the
tc-flower
branch
2 times, most recently
from
c46b4b7
to
cf53d1d
Compare
Merged
# Summary of feature
This is easiest to explain by way of iproute2.
`tc` allows actions to be created independently of filters.
Once created, these actions may then
1. be associated with _zero or more_ filters,
2. live updated (and updates will be seen by all filters using that action),
3. be deleted (only after all filters using that action have been deleted).
For example, consider the following :
```bash
for i in x y z; do
ip link add dev "$i" type dummy
tc qdisc add dev "$i" clsact
done
tc actions add action mirred egress redirect dev y
tc actions add action gact drop
```
At this point, we could
1. list the `mirred` actions
```bash
$ tc actions list action mirred
total acts 1
action order 0: mirred (Egress Redirect to device y) stolen
index 1 ref 1 bind 0
not_in_hw
used_hw_stats disabled
```
2. list the `gact` actions
```bash
$ tc actions list action gact
total acts 1
action order 0: gact action drop
random type none pass val 0
index 1 ref 1 bind 0
not_in_hw
used_hw_stats disabled
```
3. create any number of filters using either or both of these actions by index
```bash
tc filter add dev x ingress pref 1000 proto ip flower dst_ip 8.8.8.8 action mirred index 1
tc filter add dev z ingress pref 1000 proto ip flower dst_ip 8.8.8.8 action mirred index 1 action gact index 1
```
4. display those filters as normal (with per-action statistics)
```bash
$ tc -s filter show dev z ingress
filter protocol ip pref 1000 flower chain 0
filter protocol ip pref 1000 flower chain 0 handle 0x1
eth_type ipv4
dst_ip 8.8.8.8
not_in_hw
action order 1: mirred (Egress Redirect to device y) stolen
index 1 ref 3 bind 2 installed 599 sec used 599 sec
Action statistics:
Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
action order 2: gact action drop
random type none pass val 0
index 1 ref 2 bind 1 installed 599 sec used 599 sec
Action statistics:
Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
```
5. centrally update those actions (e.g., change `drop` to `pass`)
```bash
$ tc actions change action gact pass index 1
$ tc -s filter show dev z ingress
filter protocol ip pref 1000 flower chain 0
filter protocol ip pref 1000 flower chain 0 handle 0x1
eth_type ipv4
dst_ip 8.8.8.8
not_in_hw
action order 1: mirred (Egress Redirect to device y) stolen
index 1 ref 3 bind 2 installed 838 sec used 838 sec
Action statistics:
Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
action order 2: gact action pass
random type none pass val 0
index 1 ref 2 bind 1 installed 838 sec used 838 sec
Action statistics:
Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
backlog 0b 0p requeues 0
```
6. attempts to delete those actions while in use will be rejected (albeit with a buggy error message from `iproute2`/`tc`)
```bash
$ tc actions delete action gact index 1
Error: Failed to delete TC action.
We have an error talking to the kernel
Command "action" is unknown, try "tc actions help"
```
7. Removing all filters that use an action will allow the action to be deleted
```bash
$ tc filter del dev z ingress pref 1000
$ tc actions delete action gact index 1
$ tc filter del dev x ingress pref 1000
$ tc actions delete action mirred index 1
```
daniel-noland
force-pushed
the
tc-flower
branch
2 times, most recently
from
00a06ea
to
61c14d4
Compare
|
This has been split into two PRs (see #122 for the other half) I rebased on top of #122 and will rebase again when and if #122 is merged. Leaving this in draft state for now. I will focus on writing unit tests and making sure everything is documented while #122 and rust-netlink/rtnetlink#64 go through review and merge cycles. |
daniel-noland
force-pushed
the
tc-flower
branch
2 times, most recently
from
e1170f2
to
8f8e93f
Compare
Summary of changes: 1. adjust use of `NLA_F_NESTED` flag in both tests and emitted messages to reflect the kernel's stated usage (i.e. that `NLA_F_NESTED` be set for the tc-actions options). This required adjusting the tests which seem to have captured iproute2's incorrectly formed messages (iproute2 does not always set the `NLA_F_NESTED` flag) 2. Remove the use of hex as a dev dependency and use a const slice of `u8` instead as per @cathay4t request 3. remove links to iproute2's github as per @cathay4t request 4. trivial documentation cleanup
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.
I think that the PR for flower is basically code complete at this point.
So far as I can see 100% of the keys you can match on with the
flowerfilter are now implemented (including a few that are missing from iproute2).The big steps so far have been:
Up to this point, tc-u32 was the only supported filter and in that context it made sense to implement actions alongside the u32 filter.
This PR introduces support for an additional filter.
Since all filters share actions, it makes sense to move the actions to a separate module.
Additionally, since actions can be installed into the kernel independently of filters, it makes sense to have a separate module for actions which can be exposed in the high-level API in rtnetlink.
I think the next steps should be:
parse_ipv4functionIn addition, I think a few optional steps are in order.
I think a draft pr to rtnetlink with the proposed high-level API for tc-flower.
I expect that this will slow things down a bit, but I really don't like breaking APIs immediately after they are created when I can avoid it.
If I have done something ill-considered in this low-level API which shows itself when we go to actually use it in the high-level API, then it would likely be better to fix it before this is merged.
Those steps would be: