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

pkg/ipam: convert dots to slashes in interface names for sysctl #585

Merged
merged 1 commit into from
Feb 24, 2021

Conversation

dverbeir
Copy link
Contributor

Since the sysctl key syntax already uses the dot as separator, dots in interface names that appear in sysctl keys, as is the case for the net.ipv6.conf..* entries, must be replaced by slashes.

@@ -64,12 +65,16 @@ func ConfigureIface(ifName string, res *current.Result) error {
// Enabled IPv6 for loopback "lo" and the interface
// being configured
for _, iface := range [2]string{"lo", ifName} {
ipv6SysctlValueName := fmt.Sprintf(DisableIPv6SysctlTemplate, iface)
// Cannot have dots in interface name for sysctl, must replace by /
ipv6SysctlValueName := fmt.Sprintf(DisableIPv6SysctlTemplate, strings.ReplaceAll(iface, ".", "/"))
Copy link
Member

Choose a reason for hiding this comment

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

IMO, since dots and slashes can be interchanged in sysctl, is it better to make DisableIPv6SysctlTemplate to use slashes as separators?

Copy link
Member

Choose a reason for hiding this comment

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

👍 for leaving dots in interface names and instead making template slash separated.
Also: https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/lib/utils.c?id=1f420318bda3cc62156e89e1b56d60cc744b48ad#n827 - iproute2 is checking if interface does not have slash in the name.

Copy link
Member

@jellonek jellonek Feb 22, 2021

Choose a reason for hiding this comment

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

Also please take a look on our utils in cni lib.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also please take a look on our utils in cni lib.

Did you mean to suggest adding a check using utils.ValidateInterfaceName()? I believe such check is already performed in skel.pluginMain().

@@ -64,12 +65,16 @@ func ConfigureIface(ifName string, res *current.Result) error {
// Enabled IPv6 for loopback "lo" and the interface
// being configured
for _, iface := range [2]string{"lo", ifName} {
ipv6SysctlValueName := fmt.Sprintf(DisableIPv6SysctlTemplate, iface)
// Cannot have dots in interface name for sysctl, must replace by /
ipv6SysctlValueName := fmt.Sprintf(DisableIPv6SysctlTemplate, strings.ReplaceAll(iface, ".", "/"))
Copy link
Member

Choose a reason for hiding this comment

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

👍 for leaving dots in interface names and instead making template slash separated.
Also: https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/lib/utils.c?id=1f420318bda3cc62156e89e1b56d60cc744b48ad#n827 - iproute2 is checking if interface does not have slash in the name.

A dot is a valid character in interface names and is often used in the
names of VLAN interfaces. The sysctl net.ipv6.conf.<ifname>.disable_ipv6
key path cannot use dots both in the ifname and as path separator.
We switch to using / as key path separator so dots are allowed in the
ifname.

This works because sysctl.Sysctl() accepts key paths with either dots
or slashes as separators.

Also, print error message to stderr in case sysctl cannot be read
instead of silently hiding the error.

Signed-off-by: David Verbeiren <[email protected]>
Copy link
Member

@mars1024 mars1024 left a comment

Choose a reason for hiding this comment

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

/lgtm, thanks!

@dcbw
Copy link
Member

dcbw commented Feb 24, 2021

/lgtm

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.

5 participants