Skip to content
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

Draft: Nftables sets #1017

Merged
merged 17 commits into from
Oct 27, 2022
Merged

Draft: Nftables sets #1017

merged 17 commits into from
Oct 27, 2022

Conversation

inemajo
Copy link
Contributor

@inemajo inemajo commented Sep 5, 2022

Hello @svinota, this is a draft for the issue #1013 (implementation of sets and set_elements for Nftables)

@@ -46,6 +47,55 @@
NFT_MSG_GETFLOWTABLE = 23
NFT_MSG_DELFLOWTABLE = 24

# from nftables/include/datatype.h
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment, what do you think — would we benefit of using enum in these cases instead of a plain definition of variables?

Not starting with this PR, but in some future maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I think it would be better

else:
kwarg.pop("elements")

kwarg["elements"] = [
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I already started to work on kwarg filters for IPRoute, see /pyroute2/requests/; eventually we could make some common API that solves common transformations like str 255.255.255.0 ⇒ int 24, or iterable ( int interface_index ) ⇒ int interface_index

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will see that thanks !

@svinota
Copy link
Owner

svinota commented Sep 5, 2022

LGTM, if you could fix pep8 (run make format) — would be nice.

I see no issues so far, but tonigh I'll look more closer and start some test routines as well.

@inemajo inemajo force-pushed the nftables_sets branch 5 times, most recently from 6b16545 to 6767826 Compare September 16, 2022 21:26
@inemajo inemajo force-pushed the nftables_sets branch 2 times, most recently from d2e52d5 to 096da94 Compare September 20, 2022 16:00
@svinota
Copy link
Owner

svinota commented Sep 21, 2022

@inemajo does the functional test succeeds on your system?

@inemajo
Copy link
Contributor Author

inemajo commented Sep 21, 2022

Yes, and I try on a VM with ubuntu-18 and they pass too, so I don't understand why they don't pass on the CI

@inemajo
Copy link
Contributor Author

inemajo commented Sep 21, 2022

I put some new features on sets (timeout, counter...) I want to add userdata (for the comment entry) and after passing the MR to ready

@svinota
Copy link
Owner

svinota commented Sep 21, 2022

Is it possible that the test has some prerequisites set up on your machines but on the CI VM:s?

I'll take a look tonight, maybe will be lucky to find out.

@inemajo
Copy link
Contributor Author

inemajo commented Sep 21, 2022

that's what I thought, the test create the filter table but it had no effect.

Thanks for the help

@inemajo inemajo marked this pull request as ready for review October 27, 2022 09:11
@inemajo
Copy link
Contributor Author

inemajo commented Oct 27, 2022

Hello @svinota, I move the tests into examples for unlock the CI, do you think it's ok for you ?

@svinota
Copy link
Owner

svinota commented Oct 27, 2022

Sure thing, thanks.

@svinota svinota merged commit dc05a0e into svinota:master Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants