From 9db6ad283c65b1c4707fcc7a7e1518648c758cd0 Mon Sep 17 00:00:00 2001 From: Zac Mrowicki Date: Thu, 1 Jun 2023 17:03:34 +0000 Subject: [PATCH 1/2] netdog: Add wicked IPv6 config structures to support RA This change adds the necessary structures to wicked in order to support setting the `accept_ra` IPv6 option via interface config. Doing this in config allows wicked to manage this option and avoids any race conditions that may occur between wicked and the kernel when attempting to set this via sysctl. (cherry picked from commit 91e2cc986764fd0967c10182bcfe086097a1bba6) --- sources/api/netdog/src/wicked/mod.rs | 60 +++++++++++++++++-- .../api/netdog/test_data/wicked/eno1-ra.xml | 1 + .../api/netdog/test_data/wicked/eno10-ra.xml | 1 + .../api/netdog/test_data/wicked/eno2-ra.xml | 1 + .../api/netdog/test_data/wicked/eno5-ra.xml | 1 + .../api/netdog/test_data/wicked/eno7-ra.xml | 1 + .../api/netdog/test_data/wicked/eno8-ra.xml | 1 + .../api/netdog/test_data/wicked/eno9-ra.xml | 1 + 8 files changed, 62 insertions(+), 5 deletions(-) create mode 100644 sources/api/netdog/test_data/wicked/eno1-ra.xml create mode 100644 sources/api/netdog/test_data/wicked/eno10-ra.xml create mode 100644 sources/api/netdog/test_data/wicked/eno2-ra.xml create mode 100644 sources/api/netdog/test_data/wicked/eno5-ra.xml create mode 100644 sources/api/netdog/test_data/wicked/eno7-ra.xml create mode 100644 sources/api/netdog/test_data/wicked/eno8-ra.xml create mode 100644 sources/api/netdog/test_data/wicked/eno9-ra.xml diff --git a/sources/api/netdog/src/wicked/mod.rs b/sources/api/netdog/src/wicked/mod.rs index 94c3b327d9a..54901454dd0 100644 --- a/sources/api/netdog/src/wicked/mod.rs +++ b/sources/api/netdog/src/wicked/mod.rs @@ -78,6 +78,9 @@ pub(crate) struct WickedInterface { #[serde(rename = "ipv6:static")] pub(crate) ipv6_static: Option, #[serde(skip_serializing_if = "Option::is_none")] + #[serde(rename = "ipv6")] + pub(crate) ipv6: Option, + #[serde(skip_serializing_if = "Option::is_none")] #[serde(rename = "vlan")] pub(crate) vlan_tag: Option, #[serde(skip_serializing_if = "Option::is_none")] @@ -162,6 +165,27 @@ struct LinkDetection { require_link: (), } +#[derive(Debug, Serialize, PartialEq)] +pub(crate) struct WickedIpv6 { + #[serde(rename = "$unflatten=accept-ra")] + accept_ra: WickedIpv6AcceptRA, +} + +// There are technically a few options here, but currently we only use "router" +#[derive(Debug, Clone, Serialize, PartialEq)] +pub(crate) enum WickedIpv6AcceptRA { + #[serde(rename = "$primitive=router")] + Router, +} + +impl Default for WickedIpv6 { + fn default() -> Self { + WickedIpv6 { + accept_ra: WickedIpv6AcceptRA::Router, + } + } +} + impl WickedInterface { pub(crate) fn new(id: I) -> Self where @@ -175,12 +199,19 @@ impl WickedInterface { ipv6_dhcp: None, ipv4_static: None, ipv6_static: None, + ipv6: None, vlan_tag: None, bond: None, link: None, } } + /// Add config to accept IPv6 router advertisements + // TODO: expose a network config option for this + pub(crate) fn accept_ra(&mut self) { + self.ipv6 = Some(WickedIpv6::default()) + } + /// Serialize the interface's configuration file pub(crate) fn write_config_file(&self) -> Result<()> { let mut cfg_path = Path::new(WICKED_CONFIG_DIR).join(self.name.to_string()); @@ -330,15 +361,34 @@ mod tests { for ok_str in ok { let net_config = NetConfigV1::from_str(ok_str).unwrap(); - let wicked_interfaces = net_config.as_wicked_interfaces(); - for interface in wicked_interfaces { + let mut wicked_interfaces = net_config.as_wicked_interfaces(); + for interface in &mut wicked_interfaces { let generated = quick_xml::se::to_string(&interface).unwrap(); - let mut path = wicked_config().join(interface.name.to_string()); path.set_extension("xml"); - let expected = fs::read_to_string(path).unwrap(); + let expected = fs::read_to_string(&path).unwrap(); + + assert_eq!( + expected.trim(), + generated, + "Generated output does not match file: {}", + path.display() + ); - assert_eq!(expected.trim(), generated) + // Add IPv6 `accept-ra` config to the interface, regenerate it, and ensure the + // generated config contains the added IPv6 option + interface.accept_ra(); + let generated = quick_xml::se::to_string(&interface).unwrap(); + let mut path = wicked_config().join(format!("{}-ra", interface.name.to_string())); + path.set_extension("xml"); + let expected = fs::read_to_string(&path).unwrap(); + + assert_eq!( + expected.trim(), + generated, + "Generated output does not match file: {}", + path.display() + ) } } } diff --git a/sources/api/netdog/test_data/wicked/eno1-ra.xml b/sources/api/netdog/test_data/wicked/eno1-ra.xml new file mode 100644 index 00000000000..9a75bc3eecf --- /dev/null +++ b/sources/api/netdog/test_data/wicked/eno1-ra.xml @@ -0,0 +1 @@ +eno1boottruerouter diff --git a/sources/api/netdog/test_data/wicked/eno10-ra.xml b/sources/api/netdog/test_data/wicked/eno10-ra.xml new file mode 100644 index 00000000000..3faf58cc33e --- /dev/null +++ b/sources/api/netdog/test_data/wicked/eno10-ra.xml @@ -0,0 +1 @@ +eno10boottrue1router diff --git a/sources/api/netdog/test_data/wicked/eno2-ra.xml b/sources/api/netdog/test_data/wicked/eno2-ra.xml new file mode 100644 index 00000000000..ac25582b65d --- /dev/null +++ b/sources/api/netdog/test_data/wicked/eno2-ra.xml @@ -0,0 +1 @@ +eno2boottruerouter diff --git a/sources/api/netdog/test_data/wicked/eno5-ra.xml b/sources/api/netdog/test_data/wicked/eno5-ra.xml new file mode 100644 index 00000000000..3a51b2749f7 --- /dev/null +++ b/sources/api/netdog/test_data/wicked/eno5-ra.xml @@ -0,0 +1 @@ +eno5boottruetruerouter diff --git a/sources/api/netdog/test_data/wicked/eno7-ra.xml b/sources/api/netdog/test_data/wicked/eno7-ra.xml new file mode 100644 index 00000000000..4a9880b2035 --- /dev/null +++ b/sources/api/netdog/test_data/wicked/eno7-ra.xml @@ -0,0 +1 @@ +eno7boottruetrue1router diff --git a/sources/api/netdog/test_data/wicked/eno8-ra.xml b/sources/api/netdog/test_data/wicked/eno8-ra.xml new file mode 100644 index 00000000000..263ae1f7cf0 --- /dev/null +++ b/sources/api/netdog/test_data/wicked/eno8-ra.xml @@ -0,0 +1 @@ +eno8boottrue1true1router diff --git a/sources/api/netdog/test_data/wicked/eno9-ra.xml b/sources/api/netdog/test_data/wicked/eno9-ra.xml new file mode 100644 index 00000000000..d7f06d0aa41 --- /dev/null +++ b/sources/api/netdog/test_data/wicked/eno9-ra.xml @@ -0,0 +1 @@ +eno9boottrue1router From cd01813feca248d9474864d0359ddd7d027fdb8b Mon Sep 17 00:00:00 2001 From: Zac Mrowicki Date: Mon, 5 Jun 2023 22:27:12 +0000 Subject: [PATCH 2/2] netdog: Set default accept-ra setting via config rather than sysctl Previously, we set the IPv6 accept-ra setting for the primary interface via `systemd-sysctl` during the wicked install helper. Setting this sysctl via the helper can cause a race condition between the kernel and wicked. The kernel sets a flag indicating a router advertisement has been received (`IF_RA_RCVD`), but only after it completes duplicate address detection and decides whether to send a router solicitation. If the sysctl isn't set by the time duplicate address detection completes, the solicitation doesn't happen and the `IF_RA_RCVD` flag doesn't get set. wicked uses this flag to decide whether or not to kick off the state machine that handles DHCP6. This change adds the accept-ra setting to the interface config if the primary interface is set up via kernel command line. The primary interface is supplied via kernel command line for AWS and VMware variants. This change also removes the accept-ra setting from the `systemd-sysctl` config. Allowing wicked to manage this setting eliminates any chance of races between sysctl/kernel/wicked. (cherry picked from commit 5ed812d1a0115b214f5a6e0c9dbac3d7675404e2) --- sources/api/netdog/src/cli/generate_net_config.rs | 14 ++++++++++++-- sources/api/netdog/src/cli/mod.rs | 5 +---- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/sources/api/netdog/src/cli/generate_net_config.rs b/sources/api/netdog/src/cli/generate_net_config.rs index a848b5876fb..4c9c2354731 100644 --- a/sources/api/netdog/src/cli/generate_net_config.rs +++ b/sources/api/netdog/src/cli/generate_net_config.rs @@ -17,6 +17,7 @@ pub(crate) struct GenerateNetConfigArgs {} /// Generate configuration for network interfaces. pub(crate) fn run() -> Result<()> { + let mut from_cmd_line = false; let maybe_net_config = if Path::exists(Path::new(OVERRIDE_NET_CONFIG_FILE)) { net_config::from_path(OVERRIDE_NET_CONFIG_FILE).context(error::NetConfigParseSnafu { path: OVERRIDE_NET_CONFIG_FILE, @@ -26,6 +27,7 @@ pub(crate) fn run() -> Result<()> { path: DEFAULT_NET_CONFIG_FILE, })? } else { + from_cmd_line = true; net_config::from_command_line(KERNEL_CMDLINE).context(error::NetConfigParseSnafu { path: KERNEL_CMDLINE, })? @@ -48,8 +50,16 @@ pub(crate) fn run() -> Result<()> { remove_old_primary_interface()?; write_primary_interface(&primary_interface)?; - let wicked_interfaces = net_config.as_wicked_interfaces(); - for interface in wicked_interfaces { + let mut wicked_interfaces = net_config.as_wicked_interfaces(); + for interface in &mut wicked_interfaces { + // The kernel command line is too limited to fully specify an interface's configuration; + // fix some defaults to match legacy behavior. + // Note: we only allow 1 interface to be listed via kernel command line, so this will only + // be added to a single interface + if from_cmd_line { + interface.accept_ra(); + } + interface .write_config_file() .context(error::InterfaceConfigWriteSnafu)?; diff --git a/sources/api/netdog/src/cli/mod.rs b/sources/api/netdog/src/cli/mod.rs index c7bee7eea8c..7857df72695 100644 --- a/sources/api/netdog/src/cli/mod.rs +++ b/sources/api/netdog/src/cli/mod.rs @@ -155,13 +155,10 @@ where // Note: The dash (-) preceding the "net..." variable assignment below is important; it // ensures failure to set the variable for any reason will be logged, but not cause the sysctl // service to fail - // Accept router advertisement (RA) packets even if IPv6 forwarding is enabled on interface - let ipv6_accept_ra = format!("-net.ipv6.conf.{}.accept_ra = 2", interface); // Enable loose mode for reverse path filter let ipv4_rp_filter = format!("-net.ipv4.conf.{}.rp_filter = 2", interface); let mut output = String::new(); - writeln!(output, "{}", ipv6_accept_ra).context(error::SysctlConfBuildSnafu)?; writeln!(output, "{}", ipv4_rp_filter).context(error::SysctlConfBuildSnafu)?; fs::write(path, output).context(error::SysctlConfWriteSnafu { path })?; @@ -176,7 +173,7 @@ mod tests { fn default_sysctls() { let interface = "eno1"; let fake_file = tempfile::NamedTempFile::new().unwrap(); - let expected = "-net.ipv6.conf.eno1.accept_ra = 2\n-net.ipv4.conf.eno1.rp_filter = 2\n"; + let expected = "-net.ipv4.conf.eno1.rp_filter = 2\n"; write_interface_sysctl(interface, &fake_file).unwrap(); assert_eq!(std::fs::read_to_string(&fake_file).unwrap(), expected); }