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

nftables: Add more operations and raise kernel errors #902

Merged
merged 4 commits into from
Apr 24, 2022

Conversation

blechschmidt
Copy link
Contributor

Hello,

this PR is motivated by the lack of error reporting for nftables write operations. I have seen that there are unfinished classes like BatchSocket and BatchAddrPool, which are probably supposed to tackle the issue which I am addressing here. So I hope that this PR is not too intrusive or diverging too much from your intention.

Currently, the handling of batch messages is difficult, as nlm_request sends each message separately and listens for responses from a single message sequence number only. Therefore, 39741a4 introduces nlm_request_batch, which can be supplied with a list of messages. The function will then listen for expected responses for any of the supplied messages and return/yield them to the caller.

This is leveraged by 729de2d, which introduces acknowledgements for write operations by default. With this commit, errors, e.g. caused by invalid rule expressions or missing NLAs, will no longer go unnoticed and silent failures will no longer happen. The commit also adds some additional commands for the table, chain and rule objects.

Before, the following would silently fail because the chain does not exist:
nft.rule('add', table='test', chain='doesnotexist', expressions=some_valid_expr)

Now it will fail as indicated by the errno from the kernel:
pr2modules.netlink.exceptions.NetlinkError: (2, 'No such file or directory')

Silent failures can still be enabled by setting the flags parameter to 0. If the parameter is not set, it will default to NLM_F_ACK, which will cause the batch request function to expect an acknowledgement:
nft.rule('add', table='test', chain='doesnotexist', expressions=some_expr, flags=0)

Kind regards.

Currently, the handling of batch messages is difficult as nlm_request
sends each message separately and listens for responses from a single
message sequence number only. Therefore, this commit introduces
`nlm_request_batch`, which can be supplied with a list of messages. The
function will then listen for expected responses for any of the supplied
messages and return/yield them to the caller.

In the future, this will allow for improved error handling. Since some
nftables write operations require batches, the current approach is to
accumulate messages in a batch and send them to the socket in a
fire-and-forget manner. This causes errors reported by the kernel to go
unnoticed.

This commit prepares improved error handling for nftables by allowing to
read those errors. It is related to issue svinota#892.
This commit adds more nftables operations and uses `nlm_request_batch`
to raise errors that are reported by the kernel. With this commit,
errors, e.g. caused by invalid rule expressions or missing NLAs, will no
longer go unnoticed.
@svinota
Copy link
Owner

svinota commented Apr 24, 2022

  • formatting issues — I have to update the contribution docs, thanks for the reminder
  • nlm_request_batch — there was a plan to introduce something like BatchSocket.commit() also

Let's merge your code first: It's better to have something working and used, and thereafter we can eventually employ other possible solutions, if there will be any.

Thanks for the contribution!

@svinota svinota merged commit 39c3e04 into svinota:master Apr 24, 2022
bmwiedemann pushed a commit to bmwiedemann/openSUSE that referenced this pull request Jul 7, 2022
https://build.opensuse.org/request/show/987191
by user dirkmueller + dimstar_suse
- update to 0.6.13:
  * iproute, ndb: use `pyroute2.requests` to filter and transform API call arguments
  * conntrack: fix exports: <svinota/pyroute2#900>
  * nftables: multiple fixes: <svinota/pyroute2#902>
  * tc: fix em_ipset crash: <svinota/pyroute2#905>
  * tests: integrated pre-commit, github actions and more
  * tests: support basic OpenBSD tests
  * pyroute2-cli: parser fixes

- Initial package (0.6.9)
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