tc actions support #122

daniel-noland · 2024-05-07T23:38:21Z

Support for tc-actions

Add the building blocks for tc-actions support.

This PR is in the same spirit as my (in progress) work on florianl/go-tc#145

I broke out this functionality from my other PR #111 based on conversation and feedback from @cathay4t.

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

be associated with zero or more filters,
live updated (and updates will be seen by all filters using that action),
be deleted (only after all filters using that action have been deleted).

For example, consider the following :

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

list the mirred actions

$ 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

list the gact actions

$ 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

create any number of filters using either or both of these actions by index

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

display those filters as normal (with per-action statistics)

$ 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

centrally update those actions (e.g., change drop to pass)

$ 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

attempts to delete those actions while in use will be rejected (albeit with a buggy error message from iproute2/tc)

$ 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"

Removing all filters that use an action will allow the action to be deleted

$ 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 · 2024-05-08T21:08:54Z

I'm moving this out of draft state now because I think it can now be meaningfully reviewed.

I will work on squashing the remaining tiny TODO involving documentation, making sure all the doc links work correctly, and the other notes I left for myself. I will be very surprised if addressing any of those significantly change the code.

As things stand I

Wrote more documentation for all of the tc-actions functionality.
~~Added support for tc-tunnel_key~~ EDIT: already broken out into another (future) pr. I will submit that after this is merged as it is a logically distinct unit of work.

in addition to what is described in the issue.

~~If you like I will break this into three commits to account for that (or even three PRs if you feel it is necessary).~~ EDIT: I put the documentation for the existing functionality in a distinct commit from the rest of the PR in case the actions functionality needs to be reverted.

daniel-noland · 2024-05-08T21:23:45Z

I also made a slight amendment to the testing pattern by

pulling in the hex library as a dev dependency.
adding additional tests as per my thoughts in Testing strategy improvement ideas #121

I am happy to adapt that back into the existing testing pattern with hard coded vec![...] statements and or remove the "parse back" style tests if you don't think they belong here.

# 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 ```

wllenyj

Thanks for your documentation.
The unit tests is nice too.

src/tc/actions/action.rs

wllenyj · 2024-05-10T04:22:23Z

src/message.rs

+                    &TcActionMessageBuffer::new_checked(&buf.inner())
+                        .context(err)?,
+                )
+                .context(err)?;


Is context required, if only return errors to caller. Or write some context message?

good catch. No idea how that got there. Will fix

It looks like I was just following an existing pattern when I did this?

If we change this pattern here we should likely change this pattern for all the other arms to remain consistent.

Thoughts?

It's ok to keep origin pattern.
But the context is used to trace error path. I haven't tried what context(err) behaves like.

Ok. Then I suggest we leave it as is for now.

The use of anyhow need to be debated in another PR or issue. I don't like anyhow, but let's follow the precedent

daniel-noland · 2024-05-10T15:04:02Z

Thanks for your documentation. The unit tests is nice too.

Thank you for the review 😄

I will write fixups based on your comments.

If you think that flavor of unit tests is helpful (as I do, see #121) then we may also consider something like proptest or bolero to really expand on that "parse-back" strategy.
We could even use bolero+kani if we wanna get really hardcore, but things like that are best done one step at a time.

It may also be worth looking at the rather unfortunate state of the higher level API this PR results in (see rust-netlink/rtnetlink#64) before we agree to merge this.
If changes to this PR are needed to smooth out the rtnetlink api then that is best done before anybody (including rtnetlink) starts depending on this change set.
Especially if we are going to try to 1.0 this library this year 😄 (something I really look forward to)

This segment of test code is especially unfortunate

This segment of test code is almost as sad

I have thoughts about what can be done to fix this.
I will write them up shortly.

Thanks again 🎉

cathay4t

Besides the lints I mentioned, all looks good to me.

We have document in README.md for capture netlink data and convert it into Vec, which could be better than decode a long string.

cathay4t · 2024-05-14T01:00:56Z

src/tc/actions/action.rs

+                // This is heuristically trying to match the kernel's behavior
+                // but may not be correct.
+                if opts.len() == 1 {
+                    TCA_ACT_OPTIONS | NLA_F_NESTED


In kernel we have

static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = { [TCA_ACT_KIND] = { .type = NLA_STRING }, [TCA_ACT_INDEX] = { .type = NLA_U32 }, [TCA_ACT_COOKIE] = { .type = NLA_BINARY, .len = TC_COOKIE_MAX_SIZE }, [TCA_ACT_OPTIONS] = { .type = NLA_NESTED },

And nest = nla_nest_start_noflag(skb, TCA_ACT_OPTIONS);.

So it should always carry with NLA_F_NESTED.

Ah, it must be iproute2 just being wrong sometimes. I will change our code to always set NLA_F_NESTED

cathay4t · 2024-05-14T01:04:11Z

src/tc/actions/message.rs

+        ///   All dump responses will contain the number of actions being dumped
+        ///   stored in for user app's consumption in TCA_ROOT_COUNT
+        ///
+        /// [`rtnetlink.h`]: https://github.com/iproute2/iproute2/blob/89210b9ec1c445ae963d181b5816d12a0cdafbb6/include/uapi/linux/rtnetlink.h#L803-L806


Those link might not accessible in the future. No need to mention it.

iproute2 might be the de-factor netlink client, but I prefer explains stuff using kernel codes or function name.

Ok. Will fix.

cathay4t · 2024-05-14T01:14:39Z

src/tc/tests/action_nat.rs

@@ -1,164 +0,0 @@
-// SPDX-License-Identifier: MIT


Why this file is removed instead of amended?

Hmm, I thought I moved the contents of this file into the same file as the other nat tests.

Likely that change was messed up in a rebase. Will fix.

The test in this file has been moved now. It lives with the rest of the NAT tests

cathay4t · 2024-05-14T01:15:50Z

If you find it hard to capture netlink package as Vec, please let me know, I am happy to amend(and also validate) your test codes.

daniel-noland · 2024-05-14T01:56:04Z

If you find it hard to capture netlink package as Vec, please let me know, I am happy to amend(and also validate) your test codes.

I can capture as a Vec if that is better. It does remove the dev-dependency on hex. Will change

daniel-noland · 2024-05-14T03:02:36Z

Feedback responses posted.

Summary of changes:

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)
Remove the use of hex as a dev dependency
and use a const slice of u8 instead as per @cathay4t request
remove links to iproute2's github as per @cathay4t request
trivial documentation cleanup
restore deleted test

Will mark as ready for next review.
Thanks for review.

I left the review response fixups as the last commit in this series to make review easier. I'm happy to squash that commit into the prior one before merge if you wish to keep the commit history clean

@cathay4t

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 5. restore previously deleted test

daniel-noland · 2024-06-10T21:52:14Z

Any update on this?

cathay4t

Nicely done!

This was referenced May 7, 2024

RFC: tc flower support #112

Open

DRAFT: tc flower support #111

Draft

daniel-noland force-pushed the tc-actions branch 2 times, most recently from 522f60e to 4dfa69e Compare May 8, 2024 21:00

daniel-noland changed the title ~~Draft: tc actions support~~ tc actions support May 8, 2024

daniel-noland marked this pull request as ready for review May 8, 2024 21:09

daniel-noland force-pushed the tc-actions branch 5 times, most recently from 4e99be4 to 902ac77 Compare May 8, 2024 21:19

daniel-noland force-pushed the tc-actions branch 6 times, most recently from 134c77f to 524578e Compare May 9, 2024 15:43

Docs for existing tc-actions

186747e

daniel-noland force-pushed the tc-actions branch 4 times, most recently from 3c5c87d to fb346b9 Compare May 10, 2024 00:36

daniel-noland force-pushed the tc-actions branch from fb346b9 to 8f2d6ec Compare May 10, 2024 00:50

daniel-noland mentioned this pull request May 10, 2024

DRAFT: Add support for tc-actions rust-netlink/rtnetlink#64

Draft

wllenyj approved these changes May 10, 2024

View reviewed changes

daniel-noland requested a review from wllenyj May 13, 2024 16:45

cathay4t requested changes May 14, 2024

View reviewed changes

daniel-noland requested a review from cathay4t May 14, 2024 03:04

daniel-noland force-pushed the tc-actions branch from 18e884c to 6a28989 Compare May 14, 2024 03:59

daniel-noland mentioned this pull request May 18, 2024

RFC: Support for tc actions like functionality florianl/go-tc#145

Closed

cathay4t approved these changes Jun 29, 2024

View reviewed changes

cathay4t merged commit 0227234 into rust-netlink:main Jun 29, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tc actions support #122

tc actions support #122

daniel-noland commented May 7, 2024 •

edited

Loading

daniel-noland commented May 8, 2024 •

edited

Loading

daniel-noland commented May 8, 2024

wllenyj left a comment

wllenyj May 10, 2024 •

edited

Loading

daniel-noland May 10, 2024

daniel-noland May 10, 2024

wllenyj May 11, 2024

daniel-noland May 13, 2024

cathay4t Jun 29, 2024

daniel-noland commented May 10, 2024

cathay4t left a comment

cathay4t May 14, 2024

daniel-noland May 14, 2024 •

edited

Loading

cathay4t May 14, 2024

daniel-noland May 14, 2024

cathay4t May 14, 2024

daniel-noland May 14, 2024

daniel-noland May 14, 2024

cathay4t commented May 14, 2024

daniel-noland commented May 14, 2024

daniel-noland commented May 14, 2024 •

edited

Loading

daniel-noland commented Jun 10, 2024

cathay4t left a comment

tc actions support #122

tc actions support #122

Conversation

daniel-noland commented May 7, 2024 • edited Loading

Support for tc-actions

Summary of feature

daniel-noland commented May 8, 2024 • edited Loading

daniel-noland commented May 8, 2024

wllenyj left a comment

Choose a reason for hiding this comment

wllenyj May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-noland commented May 10, 2024

cathay4t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-noland May 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cathay4t commented May 14, 2024

daniel-noland commented May 14, 2024

daniel-noland commented May 14, 2024 • edited Loading

daniel-noland commented Jun 10, 2024

cathay4t left a comment

Choose a reason for hiding this comment

daniel-noland commented May 7, 2024 •

edited

Loading

daniel-noland commented May 8, 2024 •

edited

Loading

wllenyj May 10, 2024 •

edited

Loading

daniel-noland May 14, 2024 •

edited

Loading

daniel-noland commented May 14, 2024 •

edited

Loading