nft: Add support named counters and handlers. #58

bunert · 2023-08-02T07:36:53Z

Fixing docs typo and some minor code cleanup (package rename, field name in struct literal, error check in test cleanup).

Adding handlers to nftables structs (test comparison function removes them). Having the handlers in place when retrieving the configuration can be useful to execute the nft binary directly with the retrieved handlers. The rules already include the handle, this adds support for handlers in tables and chains.

Adding support for named counters. Currently, only anonymous counters in rules are supported.

EdDev · 2023-08-02T09:12:00Z

Hi @bunert , thank you for the contribution.
I will go over it in detail this week.

What is odd is that I cannot run the tests though GitHub. Usually for new contributors it asks to run it.

EdDev

Thank you very much for the contribution!

It will be great if you can add some text in the commit message that describes why this is useful (the reasoning).

I will not insist on the next point, but if you are able to split the PR into more commits it will be great. It will just help with the review.
I identified 3 topics of changes (each can potentially be a commit):

Fixing doc.
Adding the named counter.
Adding handlers to the table and chain.

If this last request complicates things for you, I'll be fine with no doing it.

EdDev · 2023-08-02T09:13:00Z

nft/config/config_test.go

@@ -23,7 +23,7 @@ import (
 "fmt"
 "testing"

- assert "github.com/stretchr/testify/require"
+ "github.com/stretchr/testify/assert"


Why this has changed? It changes the behavior of the tests.

Renaming require as assert seems unintentional, I removed the rename.

nft/config/counter.go

nft/config/counter_test.go

nft/counter.go

nft/schema/counter.go

EdDev · 2023-08-02T09:21:45Z

nft/schema/table.go

@@ -32,4 +32,5 @@ const (
 type Table struct {
 Family string `json:"family"`
 Name string `json:"name"`
+ Handle *int `json:"handle,omitempty"`


A handle has been added to the table and chain, could you please reason for adding them?

Most of the PR is about counters, and while it also merit a reasoning for adding, it seems pretty trivial (to me at least).
On the other hand, the reasoning for adding the handle to the tables and chains is less trivial.

For me, there is no reason to omit the handlers when reading the config. And it allows executing nft with the obtained handlers which is a use case for us.

Fair enough.

The fact that the tables and chains have their name unique made the use of handlers redundant when writing the configuration for me.
But if you find it useful, there is indeed no reason to omit it.

Do you know what will happen if one uses the original handler but changes the name of the table/chain? Will it fail to apply the configuration (a scenario when a user reads the config, changed the chain name and applied back).

Changes: - doc typo - require package renamed to assert - field name in struct literal - error check in test cleanup Signed-off-by: Tobias Buner <buner@anapaya.net>

Adding handlers to nftables structs and adjusted the test comparison function to remove them. Having the handlers in place when retrieving the configuration can be useful to execute the nft binary directly with the retrieved handlers. The rules already include the handle, this adds support for handlers in tables and chains. Signed-off-by: Tobias Buner <buner@anapaya.net>

Currently, only anonymous counters in rules are supported. This adds support also for named counters on the table level. Signed-off-by: Tobias Buner <buner@anapaya.net>

EdDev

Thank you, this looks really great!

There are a few inline comments, nothing major.
The build fails on the formatting of the imports: https://github.com/networkplumbing/go-nft/actions/runs/5738880629/job/15587712456?pr=58#step:5:11

EdDev · 2023-08-02T11:18:21Z

nft/config/config_test.go

@@ -23,7 +23,7 @@ import (
 "fmt"
 "testing"

- assert "github.com/stretchr/testify/require"
+ "github.com/stretchr/testify/require"


The rename was intentional [1] to use the more common assert word but behave like the testify require semantics.

This is the convention through the project and I would prefer to keep it.
I guess it is a bag I carry from other testing frameworks and different languages.

[1] 5d1c9c8

EdDev · 2023-08-02T11:32:57Z

tests/testlib/config.go

+ },
+ },
+ })
+ require.NoError(t, err)


This is a best effort operation, I do not think it fits asserting.
Perhaps we should make sure everything stops if this somehow happens (i.e. panic in an explicit manner). Continuing without this succeeding means the tests are dependent and the results are unpredictable.

Another alternative is to log it.

Have you seen cases this is failing?

EdDev · 2023-08-02T11:34:42Z

tests/testlib/config.go

 test(t)
 })
 }

-func flushRuleset() {


I find it easier to read it when it is not inlined.
What is the reason you inlined it?

EdDev · 2023-08-02T11:45:58Z

nft/schema/table.go

@@ -32,4 +32,5 @@ const (
 type Table struct {
 Family string `json:"family"`
 Name string `json:"name"`
+ Handle *int `json:"handle,omitempty"`


Fair enough.

The fact that the tables and chains have their name unique made the use of handlers redundant when writing the configuration for me.
But if you find it useful, there is indeed no reason to omit it.

Do you know what will happen if one uses the original handler but changes the name of the table/chain? Will it fail to apply the configuration (a scenario when a user reads the config, changed the chain name and applied back).

EdDev · 2023-08-03T12:52:31Z

nft/schema/rule.go

+ if c.Name != "" {
+ return json.Marshal(c.Name)
+ }
+ type counter Counter


You used a different naming convention from the existing.
This is a bit confusing, but not a blocker for sure.

EdDev · 2023-08-03T13:02:52Z

@bunert , if you can, it will help if you add this new change in the CHANGELOG so it will be ready for the next release. (as a new commit in the end)
Not a must.

Something like:

## [0.5.0] - 2023-??-??
### New Features
 - ...

EdDev · 2023-08-09T17:35:27Z

@bunert , can I assist you with anything here?

EdDev · 2023-11-14T09:36:17Z

@bunert , can I assist you with anything here?

@bunert , are you still available to complete this one?

bunert · 2023-12-05T17:41:57Z

Sorry, I don't have the time right now and we worked around the missing handlers for now. I hope to find some time to update it soon, sorry for the inconvenience.

bunert force-pushed the bunert/handle-and-counter-support branch 2 times, most recently from 844ee9d to 0085ba4 Compare August 2, 2023 07:57

EdDev self-requested a review August 2, 2023 09:08

EdDev reviewed Aug 2, 2023

View reviewed changes

bunert force-pushed the bunert/handle-and-counter-support branch from 0085ba4 to 208ff48 Compare August 2, 2023 11:06

bunert added 3 commits August 2, 2023 14:32

Fixing doc and some minor code cleanup

b43b64c

Changes: - doc typo - require package renamed to assert - field name in struct literal - error check in test cleanup Signed-off-by: Tobias Buner <buner@anapaya.net>

Adding named counter support

60dd966

Currently, only anonymous counters in rules are supported. This adds support also for named counters on the table level. Signed-off-by: Tobias Buner <buner@anapaya.net>

bunert force-pushed the bunert/handle-and-counter-support branch from 208ff48 to 60dd966 Compare August 2, 2023 12:36

EdDev reviewed Aug 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nft: Add support named counters and handlers. #58

nft: Add support named counters and handlers. #58

bunert commented Aug 2, 2023 •

edited

Loading

EdDev commented Aug 2, 2023

EdDev left a comment

EdDev Aug 2, 2023

bunert Aug 2, 2023

EdDev Aug 2, 2023

bunert Aug 2, 2023

EdDev Aug 2, 2023

EdDev left a comment

EdDev Aug 2, 2023

EdDev Aug 2, 2023

EdDev Aug 2, 2023

EdDev Aug 2, 2023

EdDev Aug 3, 2023

EdDev commented Aug 3, 2023

EdDev commented Aug 9, 2023

EdDev commented Nov 14, 2023

bunert commented Dec 5, 2023

nft: Add support named counters and handlers. #58

Are you sure you want to change the base?

nft: Add support named counters and handlers. #58

Conversation

bunert commented Aug 2, 2023 • edited Loading

EdDev commented Aug 2, 2023

EdDev left a 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

Choose a reason for hiding this comment

EdDev left a 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

Choose a reason for hiding this comment

EdDev commented Aug 3, 2023

EdDev commented Aug 9, 2023

EdDev commented Nov 14, 2023

bunert commented Dec 5, 2023

bunert commented Aug 2, 2023 •

edited

Loading