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

macvlan: add bclim option #698

Merged
merged 2 commits into from
May 25, 2023
Merged

macvlan: add bclim option #698

merged 2 commits into from
May 25, 2023

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented May 24, 2023

Expose the netlink IFLA_MACVLAN_BC_CUTOFF option as bclim like the ip
command does. We still need c/common and podman patches to allow this
option for podman network create.

Also I created a PR[1] upstream in the netlink lib to allow setting
these options directly instead of using the DefaultNla work around as I
do here.

TODO: Add a test but for now this option is not in a released version of
iproute so there is no way to check for it right now.

[1] rust-netlink/netlink-packet-route#32

This is needed for https://bugzilla.redhat.com/show_bug.cgi?id=2183896

@Luap99
Copy link
Member Author

Luap99 commented May 24, 2023

@mheon @flouthoc PTAL

Fortunately netlink upstream changes are not required for this feature so I implemented it like this for now. That should help back porting. I tested it on RHEL at it seemed to work correctly.

@mheon
Copy link
Member

mheon commented May 24, 2023

LGTM

Luap99 added a commit to Luap99/common that referenced this pull request May 24, 2023
Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

Needs fmt

diff --git a/src/network/bridge.rs b/src/network/bridge.rs
index 88fbf23..20afed0 100644
--- a/src/network/bridge.rs
+++ b/src/network/bridge.rs
@@ -75,7 +75,8 @@ impl driver::NetworkDriver for Bridge<'_> {
         let ipam = get_ipam_addresses(self.info.per_network_opts, self.info.network)?;
 
         let mtu: u32 = parse_option(&self.info.network.options, OPTION_MTU)?.unwrap_or(0);
-        let isolate: bool = parse_option(&self.info.network.options, OPTION_ISOLATE)?.unwrap_or(false);
+        let isolate: bool =
+            parse_option(&self.info.network.options, OPTION_ISOLATE)?.unwrap_or(false);
         let metric: u32 = parse_option(&self.info.network.options, OPTION_METRIC)?.unwrap_or(100);
         let no_default_route: bool =
             parse_option(&self.info.network.options, OPTION_NO_DEFAULT_ROUTE)?.unwrap_or(false);
@@ -389,8 +390,7 @@ impl<'a> Bridge<'a> {
             Some(d) => (&d.ipam.container_addresses, &d.ipam.nameservers, d.isolate),
             None => {
                 // options are not yet parsed
-                let isolate:bool = match parse_option(&self.info.network.options, OPTION_ISOLATE)
-                {
+                let isolate: bool = match parse_option(&self.info.network.options, OPTION_ISOLATE) {
                     Ok(i) => i.unwrap_or(false),
                     Err(e) => {
                         // just log we still try to do as much as possible for cleanup

While for these callers we can set defaults there are some options were
we actually want to know if it was set or not, thus return an Option<T>.

It will be required for the bclim type as it is not clear what the
default is so we do not want to set it at all if the option was not set.

Signed-off-by: Paul Holzinger <[email protected]>
Luap99 added a commit to Luap99/common that referenced this pull request May 24, 2023
Expose the netlink IFLA_MACVLAN_BC_CUTOFF option as bclim like the ip
command does. We still need c/common and podman patches to allow this
option for podman network create.

Also I created a PR[1] upstream in the netlink lib to allow setting
these options directly instead of using the DefaultNla work around as I
do here.

TODO: Add a test but for now this option is not in a released version of
iproute so there is no way to check for it right now.

[1] rust-netlink/netlink-packet-route#32

This is needed for https://bugzilla.redhat.com/show_bug.cgi?id=2183896

Signed-off-by: Paul Holzinger <[email protected]>
Luap99 added a commit to Luap99/common that referenced this pull request May 25, 2023
@Luap99
Copy link
Member Author

Luap99 commented May 25, 2023

@flouthoc PTAL again

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@flouthoc flouthoc merged commit 5ab1d08 into containers:main May 25, 2023
@Luap99 Luap99 deleted the bclim branch May 25, 2023 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants