routes: Add blackhole route support#1417
Conversation
|
Ephemeral COPR build failed. @containers/packit-build please check. |
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
I don't think that is a great design choice, there are many different route types that could be used in the same spot, i.e. unreachable or prohibit.
I think it would be smarter to add an explicit type field and then let users request blackhole explicitly, that type field should then accept the same names like ip route basically.
p12tic
force-pushed
the
blackhole-routes
branch
2 times, most recently
from
9b96f9a to
55660cc
Compare
|
@Luap99 Is the current version of the PR something what you had in mind? ip route additionally supports |
|
This feature has been exposed in podman via containers/podman#28230 |
|
@Luap99 Just a friendly ping :) |
|
Note I am super busy, in particular I like to get #1394 merged first then this PR can add the network creation validation logic for this option here as well. Of course we still need the type changes in container-libs and podman but when we port the creating logic we should not add even more features in parallel to not complicated that work so this may be a bit stalled for a few weeks. I try to ensure we still get this into netavark v2/podman v6 |
|
Thanks for explaining, I just wanted to make sure the PR is not forgotten for no reason :-) |
|
#1394 is merged This should allow you to rebase and add the create time validation here |
p12tic
force-pushed
the
blackhole-routes
branch
2 times, most recently
from
3930fc7 to
b5e9d91
Compare
|
@Luap99 I have updated the PR and re-tested together with containers/podman#28230 |
|
@Luap99 just a friendly ping :-) Is there anything left to do with this PR? |
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
Thanks, one minor thing. LGTM overall
Add an explicit route_type field to the Route type that accepts the same values as ip route: unicast, blackhole, unreachable, prohibit. Defaults to unicast when not specified. The gateway field is now optional: it is required for unicast routes and must not be set for blackhole/unreachable/prohibit routes. Closes containers#930 Signed-off-by: Povilas Kanapickas <povilas@radix.lt>
p12tic
force-pushed
the
blackhole-routes
branch
from
b5e9d91 to
230dd6e
Compare
|
@Luap99 Thanks for review. Applied your suggestion and rebased on top of latest |
|
One question for when this PR is being merged - how to integrate this feature in containers/podman#28230, in particular tests? Is it appropriate to do local tests with development version of netavark, leaving the tests requiring newer netavark disabled with a version check? Or should I wait until netavark 2.0 is released before that PR can land? |
|
We can spin new images with a Netavark built from source when this merges; all the Podman main stuff is already doing that after the other changes that have merged in preparation for 6 |
|
We have a dev version in podman CI right now due other netavark changes we need. So we just need to rebuild a new CI image but the current one is blocked on some crun failures so we need to wait a bit longer. @p12tic you do not have to do anything for the time being, I let you know once we have the new netavark in podman CI but it should happen before 6.0 final release |
3 participants
When the gateway field is omitted from a route definition, a blackhole route is created instead of a unicast route. Blackhole routes silently discard packets destined for the specified network.
The gateway field in the Route type is now Optional.
Closes #930