Skip to content

Add bind_intf argument to specify network interface for LAN commands#371

Open
dvanallen wants to merge 4 commits intoipmitool:masterfrom
dvanallen:add_bind_intf_option
Open

Add bind_intf argument to specify network interface for LAN commands#371
dvanallen wants to merge 4 commits intoipmitool:masterfrom
dvanallen:add_bind_intf_option

Conversation

@dvanallen
Copy link
Copy Markdown

Optionally sets the SO_BINDTODEVICE socket option for LAN connections, to allow binding to a specific interface specified on the command line. This has been tested successfully on bare-metal Linux servers.

@AlexanderAmelkin
Copy link
Copy Markdown
Contributor

Could you please explain why would anyone need that? Are the Linux routing rules not enough to establish the outbound interface? Why?

@dvanallen
Copy link
Copy Markdown
Author

Hi Alexander,

This is helpful when using a non-default VRF, because we can pass the VRF interface name to this new option, and then the Linux routing rules for that specific VRF will be used.

This is similar to the support provided by ip vrf exec, except that only works in environments with cgroups v2, and we'd like to support environments with only cgroups v1 support.

Some supporting docs are at https://docs.kernel.org/networking/vrf.html, which also notes "Processes can be “VRF aware” by binding a socket to the VRF device."

Happy to take feedback on this change! I saw there are a few other places in the README / man page where the options are described, so I can work on getting those updated as part of this PR, if this is a feature you'd be open to accepting.

Thanks for taking a look.

@AlexanderAmelkin
Copy link
Copy Markdown
Contributor

Oh, I see now. Thank you.

@AlexanderAmelkin
Copy link
Copy Markdown
Contributor

It would be nice to add that explanation (maybe briefer version) to the commit log as well.

@AlexanderAmelkin
Copy link
Copy Markdown
Contributor

Also, it appears like your change is not compatible with macos and windows and must be conditionally disabled there.

@dvanallen dvanallen force-pushed the add_bind_intf_option branch from 1790baf to a3f118a Compare October 6, 2022 15:50
@dvanallen
Copy link
Copy Markdown
Author

Force-pushed to get the commit description updated. Added conditional disablement and docs for the new option. Could you please kick off the test workflows again?

@dvanallen dvanallen marked this pull request as ready for review October 6, 2022 16:18
}

if (params->bind_intf != NULL) {
#ifdef SO_BINDTODEVICE
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

first, please place the #ifdef at the leftmost column.

second, if SO_BINDTODEVICE will ever change from a macro to an enum or const, this check will break. It's better to go the conventional feature-checking way, alter configure.ac with some extra AC_TRY_COMPILE to check if the code using SO_BINDTODEVICE can be compiled on the platform, then AC_DEFINE() some HAS_BINDTODEVICE macro, and then rely on that macro here.

Additional benefit to that approach is that you can check that autoconf definition in ipmitool.1.in as well and don't list in the man the option that is not supported on the platform.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the guidance here, and apologies for the delay in getting back.

I added the suggested AC_TRY_COMPILE macro, and used that throughout the source files, but I haven't found a good way to do a conditional change of the man page. I considered adding an output variable based on the same AC_TRY_COMPILE check, which would be empty or hold the option help. That doesn't seem to work for multi-line strings, and would move the option help string all the way into configure.ac, which seems odd.

I note that we have an existing similar case with the dual bridge support for the open interface, where we check for feature support with an AC_TRY_COMPILE at configure time, but we always include the related options in the man page.

Would it be OK to tackle the platform-specific man page request in another PR (or do you have some more advice on how I should tackle this here)?

@dvanallen dvanallen force-pushed the add_bind_intf_option branch from 1f25acf to 3740fbc Compare December 5, 2022 02:28
@dvanallen dvanallen force-pushed the add_bind_intf_option branch from 3740fbc to 6903862 Compare December 19, 2022 20:34
This is helpful when using a non-default VRF, because we can pass the
VRF interface name to this new option, and then the Linux routing rules
for that specific VRF will be used.

Some supporting docs are at https://docs.kernel.org/networking/vrf.html,
which also notes "Processes can be “VRF aware” by binding a socket to
the VRF device."
@dvanallen dvanallen force-pushed the add_bind_intf_option branch from 6903862 to 000aba4 Compare December 21, 2022 19:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants