network: bridge: add support for unmanaged mode #1126

M1cha · 2024-11-13T21:31:05Z

While Linux doesn't support modes on bridges, we use this concept to let the user tell us if they want podman/netavark to own the bridge or not. L3 behaves the same way as before this commit. L2 requires the bridge to exist already, will not setup any sysctls or firewall rules on the host and will not delete the bridge once all containers left.

Fixes #1090

Related: containers/common#2247

Luap99

So this is something I did not thought about. What does it mean to be a internal network with l2? There seems to be fair bit of overlap between internal l3 and l2 (except for a few sysctls and the delete the bridge thing).

My understanding is that routing would not be able to blocked without using the firewall in the l2 case normally you would have your l2 ethX interface connected to the bridge and your LAN so there is no ip routing happing on the host system as all the packages will be send to the routed normally vie the LAN.

I guess there is no good reason to support this but I wonder if we should make podman (rather c/common) error out if internal and l2 is used

Luap99 · 2024-11-14T09:53:37Z

src/network/bridge.rs

+    L2,
+    L3,


Can you add comments to the types what they do?

Luap99 · 2024-11-14T09:59:47Z

src/network/bridge.rs

-        setup_ipv4_fw_sysctl()?;
-        if data.ipam.ipv6_enabled {
-            setup_ipv6_fw_sysctl()?;
+        if let BridgeMode::L3 = data.mode {


mhh using if let means it will not fail to compile if we add a new enum variant, right?
If we ever were to add a third mode I think it would be cleaner to fail so we have to make a explicit decision

Also now that I look at this we should really not set the global sysctl for internal networks here (feel free to fix this in a second commit or I put it on my list)

If podman passed an unsupported mode it would fail in get_bridge_mode_from_string. In case you mean "in case we add support for the new mode, but forget to handle it in all places", we need to use match everywhere. It might make some conditions more complicated but it would cause compile-errors.

yes I meant we forget to handle it. However if we go unmanaged/managed for now I think it is safe enough that we do not add new modes often enough. And if we do we need to check what mode should do what anyway so let's keep the readable form.

makes sense.

Also now that I look at this we should really not set the global sysctl for internal networks here (feel free to fix this in a second commit or I put it on my list)

done

Luap99 · 2024-11-14T10:18:26Z

src/network/bridge.rs

@@ -225,7 +237,7 @@ impl driver::NetworkDriver for Bridge<'_> {
        };

        // if the network is internal block routing and do not setup firewall rules
-        if self.info.network.internal {
+        if self.info.network.internal && data.mode == BridgeMode::L3 {


if we wrap this entire part in if let BridgeMode::L3 = data.mode { up until the setup_firewall call we would only need to match once

good idea, done

Luap99 · 2024-11-14T10:19:54Z

src/network/bridge.rs

-                error_list.push(err);
-            }
-        };
+        if let BridgeMode::L3 = mode {


Can we simplify this together with the self.info.network.internal condition into one and then remove the early return from internal

good idea, done

Luap99 · 2024-11-14T10:22:29Z

src/network/bridge.rs

+                if let BridgeMode::L2 = data.mode {
+                    return Err(err).wrap("l2 bridge interface not found");
+                }


I wonder if user will be running into 730e1bf if we do not create the bridge. There seems to be bad things going on while/adding removing new interfaces.

I think the error here should say l2 requires the bridge to already exist on the host

Yes, but for an unmanaged bridge I'd let netavark move that responsibility to the creator of the bridge. The workaround is not needed if you manually set a mac address on the bridge, because it'll then keep that address forever.

Luap99 · 2024-11-14T10:23:20Z

src/network/bridge.rs

+                core_utils::CoreUtils::apply_sysctl_value(disable_dad_in_container, "0")?;
+            }
+            let enable_arp_notify = format!(
+                "/proc/sys/net/ipv4/conf/{}/arp_notify",


I think we may want to keep arp_notify even on l2?

I don't know, all of these can actually be useful for both managed and unmanaged mode. The question is just if we want to enforce our own defaults or rely on the linux kernel defaults. If we want to make this decision differently for each sysctl, we have to document why.

M1cha · 2024-11-14T11:37:13Z

So this is something I did not thought about. What does it mean to be a internal network with l2? There seems to be fair bit of overlap between internal l3 and l2 (except for a few sysctls and the delete the bridge thing).

It may just be me, but I always had trouble understanding what --internal does by just reading the documentation (or even just by it's name). The code is very simple though: Disable forwarding via sysctls, don't setup a firewall and don't add a default route. (1)

I guess there is no good reason to support this but I wonder if we should make podman (rather c/common) error out if internal and l2 is used

No matter if podman "manages" the bridge or not, having podman configure addresses and routes can be useful. You can compare that with static IPs and routes inside the network-config of a physical host.

Also, podmans firewall is currently external to the container, so it can be compared to a managed l3 switch with an integrated firewall. That is definitely needed for both managed and external bridges, the question is just if there is a need to have podman setup the firewall for external bridges. My guess is: no. Either way, podman currently does not have an option to disable the firewall for a single network, that's why I disabled it for L2 in this PR.

My understanding is that routing would not be able to blocked without using the firewall in the l2 case normally you would have your l2 ethX interface connected to the bridge and your LAN so there is no ip routing happing on the host system as all the packages will be send to the routed normally vie the LAN.

Without a firewall, the only way to block traffic going out the bridge to a different interface is by setting the forwarding sysctls to 0, yes. However, you could control traffic between ports on a bridge using an nftables bridge filter(or ebtables). This is like a managed L2-switch with a firewall. Currently, netavark does not support this and it's one of many reasons why I needed configurable veth names, so I can do this outside of podman. It might make sense to add this in future though, especially since this is kinda what the isolate option sounds like - just more fine-grained.

Also, there doesn't always have to be a physical port on an external bridge. For example, you could simply want to put both containers and libvirt VMs on the same bridge so they can communicate directly, but you don't want them to access the internet or the physical local network. Or your containers are running on the router itself and you have to forward packets to the wan interface instead of l2-switching them through the bridge to a different port.

Given my arguments above, implementing the external bridge feature as l2 vs l3 mode might be confusing. In the end it's an API choice and will not affect the functionality, but you might want to reconsider it.

(1) Not setting up a firewall might be wrong if netavark/podman would ever support firewalling between containers on the bridge.

Luap99 · 2024-11-14T13:06:07Z

Yeah I am to sure about the naming either. My think was l3 is routed while 2 is local LAN but l2 is just whatever the user configured so you are right that that naming does not really reflect what is going on.

Would something mode=managed/unmanaged or type=managed/unmanaged better? Where managed is what we to today and unmanaged is what you do here. Then it is easy to documented that unmanaged means no firewall and sysctl settings AND ONLY veth pair + ip assignment to the container interface.

M1cha · 2024-11-14T13:13:54Z

mode=managed/unmanaged sounds good to me, yes. We're already using mode for both macvlan and ipvlan (with different possible values) so reusing it makes sense to me. I also can't think of anything else we'd want to use mode for in future.

Luap99

code seems good, two minor nits on the test then LGTM

Luap99 · 2024-11-19T18:00:37Z

test/620-bridge-mode.bats

+    assert "$output" "==" 'Device "podman0" does not exist.'
+}
+
+@test bridge - l2 mode {


test need to be changed to unmanged

Luap99 · 2024-11-19T18:00:41Z

test/620-bridge-mode.bats

+
+    run_netavark --file ${TESTSDIR}/testfiles/bridge-unmanaged.json teardown $(get_container_netns_path)
+
+    # check if the interface gets removed


this is a bit confusion, it should say that we ensure the bridge is not removed.

While Linux doesn't support modes on bridges, we use this concept to let the user tell us if they want podman/netavark to own the bridge or not. Managed behaves the same way as before this commit. Unmanaged requires the bridge to exist already, will not setup any sysctls or firewall rules on the host and will not delete the bridge once all containers left. Fixes containers#1090 Signed-off-by: Michael Zimmermann <[email protected]>

That's simplfy not neccessary, because the whole point of internal networks is to not have any forwardings or firewall rules. Signed-off-by: Michael Zimmermann <[email protected]>

M1cha · 2024-11-19T20:49:28Z

thanks, fixed

Luap99

LGTM

openshift-ci · 2024-11-22T15:29:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, M1cha

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Luap99 · 2024-11-22T15:29:39Z

@mheon PTAL

mheon · 2024-11-22T15:53:21Z

/lgtm

M1cha mentioned this pull request Nov 13, 2024

libnetwork: support mode option for bridge containers/common#2247

Merged

Luap99 reviewed Nov 14, 2024

View reviewed changes

M1cha force-pushed the l2-bridge branch from 2f49fd1 to 85554bd Compare November 14, 2024 14:26

Luap99 reviewed Nov 19, 2024

View reviewed changes

M1cha force-pushed the l2-bridge branch from 85554bd to db36c4c Compare November 19, 2024 18:32

M1cha added 2 commits November 19, 2024 19:49

network: bridge: don't change forwarding sysctl for internal bridges

2900e92

That's simplfy not neccessary, because the whole point of internal networks is to not have any forwardings or firewall rules. Signed-off-by: Michael Zimmermann <[email protected]>

M1cha force-pushed the l2-bridge branch from db36c4c to 2900e92 Compare November 19, 2024 18:50

Luap99 approved these changes Nov 22, 2024

View reviewed changes

openshift-ci bot added the approved label Nov 22, 2024

M1cha changed the title ~~network: bridge: add support for l2 mode~~ network: bridge: add support for unmanaged mode Nov 22, 2024

openshift-ci bot assigned mheon Nov 22, 2024

openshift-ci bot added the lgtm label Nov 22, 2024

openshift-merge-bot bot merged commit d5358e4 into containers:main Nov 22, 2024
28 checks passed

M1cha deleted the l2-bridge branch November 22, 2024 16:00

M1cha mentioned this pull request Nov 25, 2024

Add support for bridge mode containers/podman#24677

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network: bridge: add support for unmanaged mode #1126

network: bridge: add support for unmanaged mode #1126

M1cha commented Nov 13, 2024 •

edited

Loading

Luap99 left a comment

Luap99 Nov 14, 2024

M1cha Nov 14, 2024

Luap99 Nov 14, 2024

M1cha Nov 14, 2024

Luap99 Nov 14, 2024

M1cha Nov 14, 2024

Luap99 Nov 14, 2024

M1cha Nov 14, 2024

Luap99 Nov 14, 2024

M1cha Nov 14, 2024

Luap99 Nov 14, 2024

M1cha Nov 14, 2024

Luap99 Nov 14, 2024

M1cha Nov 14, 2024

M1cha commented Nov 14, 2024

Luap99 commented Nov 14, 2024

M1cha commented Nov 14, 2024

Luap99 left a comment

Luap99 Nov 19, 2024

Luap99 Nov 19, 2024

M1cha commented Nov 19, 2024

Luap99 left a comment

openshift-ci bot commented Nov 22, 2024

Luap99 commented Nov 22, 2024

mheon commented Nov 22, 2024


		run_netavark --file ${TESTSDIR}/testfiles/bridge-unmanaged.json teardown $(get_container_netns_path)

		# check if the interface gets removed

network: bridge: add support for unmanaged mode #1126

network: bridge: add support for unmanaged mode #1126

Conversation

M1cha commented Nov 13, 2024 • edited Loading

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M1cha commented Nov 14, 2024

Luap99 commented Nov 14, 2024

M1cha commented Nov 14, 2024

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M1cha commented Nov 19, 2024

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 22, 2024

Luap99 commented Nov 22, 2024

mheon commented Nov 22, 2024

M1cha commented Nov 13, 2024 •

edited

Loading