corndog: use systemd-sysctl for sysctl options #333

aetimmes · 2024-12-29T15:30:02Z

Issue number:

Closes bottlerocket-os/bottlerocket#4314

Description of changes:

Change corndog's sysctl function to utilize systemd-sysctl instead of writing values ephemerally to tmpfs, so that other system functions (eg Ciliums sysctl-fixup) don't completely clobber the sysctls set by corndog.

Testing done:

Unit tests added inline

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

aetimmes · 2024-12-29T15:35:32Z

I don't speak Rust natively so if there's anything in here that's not idiomatic, feel free to let me know.

aetimmes · 2024-12-31T14:18:23Z

sources/api/corndog/src/main.rs

-            // We don't fail because sysctl keys can vary between kernel versions and depend on
-            // loaded modules.  It wouldn't be possible to deploy settings to a mixed-kernel fleet
-            // if newer sysctl values failed on your older kernels, for example, and we believe
-            // it's too cumbersome to have to specify in settings which keys are allowed to fail.


The current implementation of this PR doesn't maintain the functionality mentioned in this comment, but we could pretty easily achieve this by prepending - to all the sysctls passed in by default (which would then remove the need for the backup-modify-restore shuffle, which might make things simpler overall), or by documenting the ability of users to do so. Open to feedback/direction here.

aetimmes · 2024-12-31T14:20:30Z

sources/api/corndog/src/main.rs

+        config.push_str(&format!("{}={}\n", key.as_ref(), value));
+    }
+
+    fs::write(SYSCTL_CONFIG, config).context(error::WriteSysctlConfigSnafu)?;


I need to do some double-checking around:

Does this actually append to the file as written, or does it clobber it?

If we append and we write duplicate sysctls to the same config file, how does systemd-sysctl handle that? (Does the most recent sysctl win?)

I haven't yet been able to get a local build working (turns out my hard drive is smaller than I thought!) but I'll investigate these shortly.

Answers:

This clobbers the file (must fix)

systemd-sysctl will use the last value in a file for the same sysctl. For example, a config file with the contents:

dev.raid.speed_limit_max = 199999 dev.raid.speed_limit_max = 199998 garbage dev.raid.speed_limit_max = 199997

and running systemctl restart systemd-sysctl will set dev.raid.speed_limit_max to 199997 and report an error when attempting to parse the "garbage" line - the return code of the systemd unit will be 1 but all of the correctly specified sysctl k/v pairs will have been applied.

systemd-sysctl also will deduplicate multiple lines and only write when there are changes to be made, so in theory endlessly appending to a file should be a reasonable way to do this.

Output from running systemd-sysctl with the above file in support of the above conclusions:

bash-5.1# SYSTEMD_LOG_LEVEL=debug /x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/systemd/systemd-sysctl Parsing /x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/sysctl.d/50-default.conf Parsing /x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/sysctl.d/50-pid-max.conf Parsing /x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/sysctl.d/80-release.conf Parsing /x86_64-bottlerocket-linux-gnu/sys-root/usr/lib/sysctl.d/90-kubelet.conf Parsing /etc/sysctl.d/90-primary_interface.conf Parsing /etc/sysctl.d/99-zzz-override_cilium.conf Overwriting earlier assignment of net/ipv4/conf/all/rp_filter at '/etc/sysctl.d/99-zzz-override_cilium.conf:7'. Parsing /etc/sysctl.d/99-zzz-zzz-testing.conf Overwriting earlier assignment of dev/raid/speed_limit_max at '/etc/sysctl.d/99-zzz-zzz-testing.conf:2'. /etc/sysctl.d/99-zzz-zzz-testing.conf:3: Line is not an assignment, ignoring: garbage Overwriting earlier assignment of dev/raid/speed_limit_max at '/etc/sysctl.d/99-zzz-zzz-testing.conf:4'. ... Setting '/proc/sys/dev/raid/speed_limit_max' to '199997' No change in value '199997', suppressing write

ytsssun · 2025-01-06T23:38:35Z

Sorry for the delay, @aetimmes. And thanks so much for opening the PR for this. I will take a look at this PR and get back to you soon. I will also help verify the behavior.

ytsssun · 2025-01-10T20:42:31Z

sources/api/corndog/src/main.rs

+fn validate_sysctl_entry<K>(key: K, value: &str) -> Result<()>
+where
+    K: AsRef<str>,
+{
+    let key = key.as_ref();
+
+    // Basic validation of key format
+    if !key.chars().all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '_' || c == '-' || c == '*') {
+        return error::InvalidSysctlKeySnafu { key: key.to_string() }.fail();
+    }
+
+    // Ensure value doesn't contain newlines which could break the config file format
+    if value.contains('\n') {
+        return error::InvalidSysctlValueSnafu {
+            key: key.to_string(),
+            value: value.to_string()
+        }.fail();
+    }
+
+    Ok(())
+}


We perform validation when parsing the key from userdata already. See https://github.com/bottlerocket-os/bottlerocket-settings-sdk/blob/0cf857357bc799bf036b5f5bf865d3c4af4e9256/bottlerocket-settings-models/modeled-types/src/shared.rs#L703-L733.

We may need to account for a more strict validation there though.

We probably need to adjust in a couple directions - systemd-sysctl also allows for wildcards in the keys, which it looks like the current format doesn't allow for. I can open a separate PR for that.

What is the purpose of the additional validation here? Is something being allowed in as a setting that would be rejected by systemd-sysctl?

The settings API is intended to be the first and best line of defense - the only time checks should be needed at apply-time is if there's invalid input that the API isn't currently capable of detecting, like two mutually exclusive or dependent settings.

If we do need some extra validation for compatibility, then it'd be better to warn and remove the setting from the output, so that it's not a hard error. But even that is a potential compatibility break on upgrade that's best avoided, if there were sysctls that would have been applied before but that can't be applied now because of more restrictive rules.

What is the purpose of the additional validation here

No purpose - I was unaware of the bottlerocket-settings-sdk repo when I took the first pass at this PR; I agree that the validation in this MR should be removed.

I would still like to update the validation in the bottlerocket-settings-sdk repo to allow for wildcards where possible since they're supported by systemd-sysctl in general, but given that that PR would be allowing for extra functionality, I don't consider it a blocker for this change.

ytsssun · 2025-01-10T20:45:59Z

sources/api/corndog/src/main.rs

 where
    K: AsRef<str>,
 {
+    // TODO: make the priority configurable?
+    const SYSCTL_CONFIG: &str = "/etc/sysctl.d/95-corndog.conf";


Let's move this constant up (somewhere in L26-L28) so that we don't recreate the constant every time we invoke this function.

Also maybe add _PATH as the suffix suggesting it is the path of the config not the actual content of the config?

ytsssun · 2025-01-10T20:47:19Z

sources/api/corndog/src/main.rs

+    for (key, value) in &sysctls {
+        validate_sysctl_entry(key, value)?;
+    }


Again validation already happened when parsing the keys.

ytsssun · 2025-01-10T20:56:08Z

sources/api/corndog/src/main.rs

+    }
+
+    // Create config string
+    let mut config = String::new();
    for (key, value) in sysctls {


We had conversation that iterating through the map entry is not ideal since it does not guarantee an ordering here.

One way to address that is to sort the sysctl entries first by the keys and then iterate based on the sorted keys. You can do something like

let mut sysctl_vec: Vec<_> = sysctls .iter() .map(|(k,v)| (k.as_ref(), v)) .collect(); sysctl_vec.sort(); for (key, value) in sysctl_vec { ... }

On another note, we may also want to preserve the order the entries are specified in the userdata. But that would require a different struct for sysctls that can preserve the order. For example BTreeMap, but it has performance trade-offs since the insertions, lookups, and deletions would not be as fast.

Good point - we definitely want to maintain ordering becuase of the interaction between wildcards and overrides. I'm not overly worried about performance here since the upper bound of the size of this structure is probably on the order of thousands.

So it looks like using a BTreeMap would complicate the serde bits a fair amount since BTreeMap doesn't implement several of the interfaces needed by serde - how do we feel about a Vec here instead?

ytsssun · 2025-01-10T21:00:37Z

sources/api/corndog/src/main.rs

+    let mut file = fs::OpenOptions::new()
+        .create(true)
+        .append(true)
+        .open(SYSCTL_CONFIG)
+        .context(error::OpenSysctlConfigSnafu)?;
+
+    file.write_all(config.as_bytes())
+        .context(error::WriteSysctlConfigSnafu)?;


fs::write("path/to/file.txt", config).context(error::WriteSysctlConfigSnafu)?;

This would try to create the file if it does not exist.

ytsssun · 2025-01-10T21:05:36Z

sources/api/corndog/src/main.rs

+    let status = Command::new("systemctl")
+        .args(["restart", "systemd-sysctl"])
+        .status()
+        .context(error::SystemctlSnafu)?;


You don't really need to restart the whole sysctld service.

/usr/lib/systemd/systemd-sysctl /etc/sysctl.d/99-corndog.conf

Would apply this specific file to sysctld.

And could you wrap the binary path and args into constants? Something like below and put it along with the definition of the SYSCTL_CONFIG_PATH.

const SYSTEMD_SYSCTL_BIN: &str = "/usr/lib/systemd/systemd-sysctl";

I'll need to confirm this, but I believe only applying the specific file may not necessarily respect the ordering of the rest of the config files, so sysctl restart systemd-sysctl is correct IMO.

For example, the Cilium sysctl.conf file has a prefix of 99-zzz, so applying the 95 priority corndog file by itself may clobber any entries that are also set in the Cilium conf file.

If we kept sysctl restart systemd-ctl, would we still want these wrapped into constants?

ytsssun · 2025-01-10T21:07:42Z

sources/api/corndog/src/main.rs

+    if !status.success() {
+        return error::SystemctlFailedSnafu.fail();
+    }


When hitting this case, it would be also good to print the errors from the output. So I would get the output of the command and check the status and the stderr.

let output = Command::new(SYSTEMD_SYSCTL_BIN) .arg(SYSCTL_CONFIG_PATH) .output() .context(ExecutionSnafu{command: format!("{} {}", SYSTEMD_SYSCTL_BIN, SYSCTL_CONFIG_PATH)})?; if !output.status.success() { warn!( "Failed to apply sysctl config: {}", String::from_utf8_lossy(&output.stderr) # account for invalid utf8 encoding ); }

ytsssun · 2025-01-10T21:09:04Z

Left some comments, the change looks good overall. I will run some test with the change to verify the behavior. I will get back to you on that.

bcressey · 2025-01-14T18:09:06Z

sources/api/corndog/src/main.rs

+fn validate_sysctl_entry<K>(key: K, value: &str) -> Result<()>
+where
+    K: AsRef<str>,
+{
+    let key = key.as_ref();
+
+    // Basic validation of key format
+    if !key.chars().all(|c| c.is_ascii_alphanumeric() || c == '.' || c == '_' || c == '-' || c == '*') {
+        return error::InvalidSysctlKeySnafu { key: key.to_string() }.fail();
+    }
+
+    // Ensure value doesn't contain newlines which could break the config file format
+    if value.contains('\n') {
+        return error::InvalidSysctlValueSnafu {
+            key: key.to_string(),
+            value: value.to_string()
+        }.fail();
+    }
+
+    Ok(())
+}


What is the purpose of the additional validation here? Is something being allowed in as a setting that would be rejected by systemd-sysctl?

The settings API is intended to be the first and best line of defense - the only time checks should be needed at apply-time is if there's invalid input that the API isn't currently capable of detecting, like two mutually exclusive or dependent settings.

If we do need some extra validation for compatibility, then it'd be better to warn and remove the setting from the output, so that it's not a hard error. But even that is a potential compatibility break on upgrade that's best avoided, if there were sysctls that would have been applied before but that can't be applied now because of more restrictive rules.

bcressey · 2025-01-14T18:14:34Z

sources/api/corndog/src/main.rs

+/// Restarts systemd-sysctl service
+fn restart_sysctl() -> Result<()> {
+    let status = Command::new("systemctl")
+        .args(["restart", "systemd-sysctl"])
+        .status()
+        .context(error::SystemctlSnafu)?;
+
+    if !status.success() {
+        return error::SystemctlFailedSnafu.fail();
+    }
+
+    Ok(())
+}


Restarting a service can lead to unexpected behavior if units depend on that service, or if for whatever reason it hasn't started yet or failed to start successfully. This is also using the blocking form which can lead to trouble in early boot.

It'd be better to avoid all of those issues and just invoke systemd-sysctl directly. I read your other comment and understand that we can't pass just the one config file, which makes sense, but the unit just runs /usr/lib/systemd/systemd-sysctl with no arguments which we can do here also.

That makes sense to me; I'll adjust accordingly.

bcressey · 2025-01-14T18:18:54Z

sources/api/corndog/src/main.rs

@@ -18,6 +18,11 @@ use std::path::{Path, PathBuf};
 use std::str::FromStr;
 use std::string::String;
 use std::{env, process};
+use std::process::Command;
+use tempfile::NamedTempFile;


This doesn't appear to be used, but it would be good to use it and follow the usual pattern:

create a tempfile in the same directory as the target file

write all the content to it

persist the tempfile to the final target once it's ready

(We want to avoid changing the original file if some other process might have it open, and we also want to avoid repeatedly appending to the same file on subsequent runs, which could lead to unexpected behavior or resource exhaustion.)

want to avoid repeatedly appending to the same file on subsequent runs

To be explicitly clear - if I make a new api client request to set sysctl userdata on a host that already has an existing /etc/sysctl.d/95-corndog.conf file, which of these is the desired behavior?

append the new sysctls to the existing list of sysctls (allowing for duplicates; gets expensive if many calls are made but systemd-sysctl handles deduping for us)

upsert the new sysctls into the existing list of sysctls (if a previously detected entry exists, remove the existing key and append the new k/v - more expensive but no duplicates)

replace the old sysctls with the new ones (seems like a breaking change)

In the case of the first two options, is the suggestion above to make a copy of the existing sysctl conf file as the tempfile (as opposed to operating on the original)?

(Apologies for being pedantic; I just want to make sure I understand the requirements here)

aetimmes · 2025-01-14T21:44:11Z

Thanks for the reviews! The feedback all makes sense (modulo the couple of open questions I have) - I'm weirdly struggling to get my local Rust install to play nice with the pre-existing tests so my follow-up will be slightly delayed while I debug my machine.

corndog: use systemd-sysctl for sysctl options

b165389

aetimmes mentioned this pull request Dec 29, 2024

Persist userdata sysctl changes to /etc/sysctl.d bottlerocket-os/bottlerocket#4314

Open

aetimmes commented Dec 31, 2024

View reviewed changes

refactor (to squash)

6b0507a

ytsssun reviewed Jan 10, 2025

View reviewed changes

bcressey reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

corndog: use systemd-sysctl for sysctl options #333

corndog: use systemd-sysctl for sysctl options #333

aetimmes commented Dec 29, 2024

aetimmes commented Dec 29, 2024

aetimmes Dec 31, 2024 •

edited

Loading

aetimmes Dec 31, 2024

aetimmes Dec 31, 2024

aetimmes Dec 31, 2024

ytsssun commented Jan 6, 2025 •

edited

Loading

ytsssun Jan 10, 2025

aetimmes Jan 11, 2025

bcressey Jan 14, 2025

aetimmes Jan 14, 2025 •

edited

Loading

ytsssun Jan 10, 2025

ytsssun Jan 10, 2025

ytsssun Jan 10, 2025

aetimmes Jan 11, 2025

aetimmes Jan 13, 2025

ytsssun Jan 10, 2025

ytsssun Jan 10, 2025

aetimmes Jan 11, 2025

ytsssun Jan 10, 2025

ytsssun commented Jan 10, 2025

bcressey Jan 14, 2025

bcressey Jan 14, 2025

aetimmes Jan 14, 2025

bcressey Jan 14, 2025

aetimmes Jan 14, 2025

aetimmes commented Jan 14, 2025

corndog: use systemd-sysctl for sysctl options #333

Are you sure you want to change the base?

corndog: use systemd-sysctl for sysctl options #333

Conversation

aetimmes commented Dec 29, 2024

aetimmes commented Dec 29, 2024

aetimmes Dec 31, 2024 • edited Loading

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

ytsssun commented Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aetimmes Jan 14, 2025 • edited Loading

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

ytsssun commented Jan 10, 2025

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

aetimmes commented Jan 14, 2025

aetimmes Dec 31, 2024 •

edited

Loading

ytsssun commented Jan 6, 2025 •

edited

Loading

aetimmes Jan 14, 2025 •

edited

Loading