-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Objects implementation refactor #259
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it’s very helpful to see the entire change laid out like that. I left a few detail questions.
"github.com/mdlayher/netlink" | ||
"golang.org/x/sys/unix" | ||
) | ||
|
||
// CounterObj implements Obj. | ||
// Deprecated: Use ObjAttr instead |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of deprecating these types, would it be possible to convert to ObjAttr under the covers? Maybe I’m not fully understanding the before/after, though. Would you mind showing an example change that users of CounterObj need to do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've already changed CounterObj and QuoteObj to implement Obj interface as a form of compatibility with functions in obj.go
. With this change, the only thing that users of CounterObj should change is the conversion of the returned object that implements the Obj interface.
Here are a few examples:
// This will work since CounterObj implements Obj
obj, err := c.ResetObject(&nftables.CounterObj{
Table: filter,
Name: "fwded",
})
// This will work as well
counter1 := c.AddObj(&nftables.CounterObj{
Table: table,
Name: "fwded1",
Bytes: 1,
Packets: 1,
})
// Also works, although counter1 is technically ObjAttr
obj1, err := c.GetObject(counter1)
// This wouldn't work anymore, obj1 is ObjAttr
rcounter1, ok := obj1.(*nftables.CounterObj)
// Previous line should be changed to this
rcounter1, ok := obj1.(*nftables.ObjAttr)
We could add a special case when we unmarshal ObjTypeCounter and ObjTypeQuota to return CounterObj and QuotaObj instead of ObjAttr, but this approach kind of beats the purpose of using ObjAttr for these two types since we would always leverage legacy types instead of new ObjAttr.
Let me know how to approach this.
Refactored obj.go to a more generic approach Added object support for already implemented expressions Added test for limit object Fixes google#253
54b203b
to
b77f1a9
Compare
Hi,
as per our discussion in #253, I have implemented a change for a more generic implementation of nftables objects. In addition to the refactor I have added a test to demonstrate that already implemented expressions are now supported as objects and made a few lint changes. Note that some of the expressions are currently not implemented in the lib (i.e. synproxy, ct_helper, ct_expect, ...). These can now be easily added to the
expr
package to offer support. I'll see to adding them after we agree on this refactor change.Let me know what you think.