-
Notifications
You must be signed in to change notification settings - Fork 127
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.
counter.go
Outdated
"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.
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.
Hi @stapelberg,
this discussion has gone stale a bit. I have refactored this feature to make it backwards compatible. The implementation now detects when user passes the "new" ObjAttr
type and then works with and returns the ObjAttr
value, otherwise we default to "old" QuotaObj
or CounterObj
. I have implemented tests using both variants to demonstrate how it works.
Let me know what you think.
54b203b
to
b77f1a9
Compare
bf1fedd
to
c6bcfd9
Compare
Refactored obj.go to a more generic approach Added object support for already implemented expressions Added test for limit object Fixes google#253
c6bcfd9
to
533a934
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.