Sysctl keys can contain some more non-alphanumeric characters #755

mchf · 2022-03-24T10:39:56Z

Tries to address #684 by supporting * and : in the key names.

See Example 4 in sysctl.d man page
Lines like net.ipv4.conf.*.rp_filter = 2 are valid in sysctl

mchf · 2022-03-28T06:55:40Z

@raphink
Could you review the patch? Thanks

Note to failing tests. I currently hardly doubt that both failing tests test-rwlock1 and test-thread_create are somehow influenced by this lens modification. Both failures seems to be related to a thread creation.

teclator · 2022-04-19T08:34:19Z

@raphink @lutter could you take a look / review it?

georgehansper · 2022-04-27T12:54:40Z

Can I ask - which OS are you using?

When I try to use the wildcard on Linux (Fedora, CentOS), I get:

# sysctl 'net.ipv4.conf.*.rp_filter=2'
sysctl: cannot stat /proc/sys/net/ipv4/conf/*/rp_filter: No such file or directory

and the values in sysctl remain unchanged.

teclator · 2022-04-28T16:28:59Z

@georgehansper this bug concretely it is for openSUSE (TW) but the new sysctl defaults are from upstream (see https://bugzilla.suse.com/show_bug.cgi?id=1197443#c7)

It seems that these new settings or format are for /usr/lib/systemd/systemd-sysctl (see https://man7.org/linux/man-pages/man5/sysctl.d.5.html) the configuration is read at boot by systemd-sysctl.service but these options are not supported by sysctl command.

mchf · 2022-04-28T17:05:33Z

in addition to what was already stated by @teclator ... In suse the old sysctl is part of procps package which was marked as deprecated. All sysctl files should use new systemd powered syntax (e.g. wildcards)

georgehansper · 2022-05-05T09:35:56Z

I'm looking over sysctl.d(5) and notice that the forward-slash character is also valid / within the key (left-hand-side of the assignment).

Can you please add the / to the regular-expression for word ?

I can't see any mention of the colon : character as being valid within a key.
Nor does it appear as a filename in my local /proc/sys tree.
It's probably harmless to leave it in the RE. Was there a specific reason you included it?

mchf · 2022-05-06T11:41:11Z

I'm looking over sysctl.d(5) and notice that the forward-slash character is also valid / within the key (left-hand-side of the assignment).

Can you please add the / to the regular-expression for word ?

should be easy enough, thanks for tip.

I can't see any mention of the colon : character as being valid within a key. Nor does it appear as a filename in my local /proc/sys tree. It's probably harmless to leave it in the RE. Was there a specific reason you included it?

it is kind of implicit. You can have double colon e.g. in network interface name (e.g. for virtual interfaces; again it might be distribution specific convention as in theory kernel has almost no requirements on network interface names). See referenced issue in the description for an example. So, whatever option which can include an interface name, can contain also double colon.

mvidner · 2022-05-12T09:19:13Z

Can you please add the / to the regular-expression for word ?

@mchf Could there be a problem with / being the separator in Augeas paths?

georgehansper · 2022-05-12T11:39:47Z

Could there be a problem with / being the separator in Augeas paths?

@mvidner That's a good point.

> augtool --load-file /etc/sysctl.d/pr755_test.star.conf 
augtool> print /files/etc/sysctl.d/pr755_test.star.conf/
/files/etc/sysctl.d/pr755_test.star.conf
/files/etc/sysctl.d/pr755_test.star.conf/net.ipv6.conf.*.forwarding = "1"
/files/etc/sysctl.d/pr755_test.star.conf/net\/ipv6\/conf\/*\/forwarding = "1"
augtool> set /files/etc/sysctl.d/pr755_test.star.conf/net\/ipv6\/conf\/*\/forwarding 0
augtool> print /files/etc/sysctl.d/pr755_test.star.conf/
/files/etc/sysctl.d/pr755_test.star.conf
/files/etc/sysctl.d/pr755_test.star.conf/net.ipv6.conf.*.forwarding = "1"
/files/etc/sysctl.d/pr755_test.star.conf/net\/ipv6\/conf\/*\/forwarding = "0"
augtool> save
Saved 1 file(s)
augtool>

augtool seems OK with it, so aug_set() must be OK too.

Since the / variant is mentioned in man sysctl.d I think augeas should allow it, otherwise some valid files would fail to load.

mchf · 2022-05-13T07:01:26Z

Can you please add the / to the regular-expression for word ?

@mchf Could there be a problem with / being the separator in Augeas paths?

it definitely is ... however it has to be somehow doable as we can expect '/' to be part of e.g. file paths which might be present across various config files. Also there might be a difference (for augeas) in having '/' in a key or in a value.

so, i'll keep trying for a while ;-)

georgehansper · 2022-08-28T10:31:10Z

Did you want to add the / character to the key ?

If not, we can address the / character in a seperate PR, and just use this PR as-is

mchf · 2022-10-05T09:14:31Z

Did you want to add the / character to the key ?

I tried but ended with The key regexp /[*:/A-Za-z0-9_.-]+/ matches a '/' and was not able to find a solution. Do you have a hint for me?

georgehansper · 2022-10-05T10:31:38Z

If I make an additional modification to sysctl.aug

diff --git a/lenses/sysctl.aug b/lenses/sysctl.aug
index 7ee0c736..f6245e37 100644
--- a/lenses/sysctl.aug
+++ b/lenses/sysctl.aug
@@ -38,7 +38,7 @@ let comment = Util.comment_generic /[ \t]*[#;][ \t]*/ "# "
 let entry =
      let some_value = Sep.space_equal . store Simplevars.to_comment_re
   (* Rx.word extended by * and : *)
-  in let word = /[*:A-Za-z0-9_.-]+/
+  in let word = /[*:\/A-Za-z0-9_.-]+/
   (* Avoid ambiguity in tree by making a subtree here *)
   in let empty_value = [del /[ \t]*=/ "="] . store ""
   in [ Util.indent . key word

And create a file /etc/sysctl.d/pr755_test_addenda.conf

key.name.under.proc.sys = some value
key/name/under/proc/sys = some value
key/middle.part.with.dots/foo = 123

This produces the following tree in augeas

/files/etc/sysctl.d/pr755_test_addenda.conf
/files/etc/sysctl.d/pr755_test_addenda.conf/key.name.under.proc.sys = "some value"
/files/etc/sysctl.d/pr755_test_addenda.conf/key\/name\/under\/proc\/sys = "some value"
/files/etc/sysctl.d/pr755_test_addenda.conf/key\/middle.part.with.dots\/foo = "123"

which can be used with set/get etc

augtool> set /files/etc/sysctl.d/pr755_test_addenda.conf/key\/middle.part.with.dots\/foo 456
augtool> get /files/etc/sysctl.d/pr755_test_addenda.conf/key\/middle.part.with.dots\/foo
/files/etc/sysctl.d/pr755_test_addenda.conf/key\/middle.part.with.dots\/foo = 456
augtool>

Backslash quoting like this tends to be tricky within some scripting environments like bash, but not too difficult when calling the API directly from python or ruby or C.

mchf · 2022-10-06T06:13:13Z

If I make an additional modification to sysctl.aug

diff --git a/lenses/sysctl.aug b/lenses/sysctl.aug
index 7ee0c736..f6245e37 100644
--- a/lenses/sysctl.aug
+++ b/lenses/sysctl.aug
@@ -38,7 +38,7 @@ let comment = Util.comment_generic /[ \t]*[#;][ \t]*/ "# "
 let entry =
      let some_value = Sep.space_equal . store Simplevars.to_comment_re
   (* Rx.word extended by * and : *)
-  in let word = /[*:A-Za-z0-9_.-]+/
+  in let word = /[*:\/A-Za-z0-9_.-]+/
   (* Avoid ambiguity in tree by making a subtree here *)
   in let empty_value = [del /[ \t]*=/ "="] . store ""
   in [ Util.indent . key word

sorry, I did a mistake when copy pasting my regexp. So, i have in let word = /[*:\/A-Za-z0-9_.-]+/ (the same as you have above) in sysctl.aug but when I try to check it with augparse -I lenses/ lenses/tests/test_sysctl.aug I receive this error output

Syntax error in lens definition
lenses/sysctl.aug:38.0-46.45:Failed to compile entry
lenses/sysctl.aug:44.21-.29:exception: The key regexp /[*:/A-Za-z0-9_.-]+/ matches a '/'

the test test_sysctl.aug was also modified to cover new character. However it shouldn't have any impact on the augparse run as it crashes on syntax check of the lense.

georgehansper · 2022-10-06T11:13:00Z

I was also getting this error from augparse initially:

lenses/tests/test_sysctl.aug:51.0-65.0:exception thrown in test
lenses/tests/test_sysctl.aug:51.5-56.21:exception: Failed to match tree under /net

     { "ipv4" }
<snip>

But the problem was due to me failing to quote the / in the set command
Without the quoting, the / is interpreted as a path-seperator
When quoted, it is treated as part of the label.

The following changes to test_sysctl.aug work without raising an error:

diff --git a/lenses/tests/test_sysctl.aug b/lenses/tests/test_sysctl.aug
index daec3dc8..0ab36177 100644
--- a/lenses/tests/test_sysctl.aug
+++ b/lenses/tests/test_sysctl.aug
@@ -9,6 +9,7 @@ module Test_sysctl =
 let default_sysctl = "# Kernel sysctl configuration file
 # Controls IP packet forwarding
 net.ipv4.ip_forward = 0
+net/ipv4/ip_nonlocal_bind = 0
 
 net.ipv4.conf.default.rp_filter = 1
 net.ipv4.conf.default.accept_source_route = \t0
@@ -30,6 +31,7 @@ test Sysctl.lns get default_sysctl =
     { "#comment" = "Kernel sysctl configuration file" }
     { "#comment" = "Controls IP packet forwarding"}
     { "net.ipv4.ip_forward" = "0" }
+    { "net/ipv4/ip_nonlocal_bind" = "0" }
     { }
     { "net.ipv4.conf.default.rp_filter" = "1" }
     { "net.ipv4.conf.default.accept_source_route" = "0" }
@@ -48,12 +50,14 @@ test Sysctl.lns get spec_chars_sysctl =
 (* Test: Sysctl.lns *)
 test Sysctl.lns put default_sysctl after
     set "net.ipv4.ip_forward" "1" ;
+    set "net\/ipv4\/ip_nonlocal_bind" "1" ;
     rm "net.ipv4.conf.default.rp_filter" ;
     rm "net.ipv4.conf.default.accept_source_route" ;
     rm "kernel.sysrq"
   = "# Kernel sysctl configuration file
 # Controls IP packet forwarding
 net.ipv4.ip_forward = 1
+net/ipv4/ip_nonlocal_bind = 1
 
 
 ; Semicolon comments are also allowed

Note the litteral / in the file-content, but the quoted \/ in the set-command

mchf · 2022-10-07T10:47:55Z

I was also getting this error from augparse initially:

lenses/tests/test_sysctl.aug:51.0-65.0:exception thrown in test
lenses/tests/test_sysctl.aug:51.5-56.21:exception: Failed to match tree under /net

     { "ipv4" }
<snip>

I think, my issue is little bit different. As far as I can understand it, the problem is that modified regexp matches /. At least that way i understand to the output of augparse

Syntax error in lens definition
lenses/sysctl.aug:38.0-46.45:Failed to compile entry
lenses/sysctl.aug:44.21-.29:exception: The key regexp /[*:/A-Za-z0-9_.-]+/ matches a '/'

It happens when I run augparse agains original test_sysctl.aug (without slashes) even modified one (with slashes). Moreover, I think that content of the test cannot have any impact on the issue as it fails in lens syntax check and the lens is not loaded at all. Because these messages follows

lenses/tests/test_sysctl.aug:37.5-.15:Could not load module Sysctl for Sysctl.lns
lenses/tests/test_sysctl.aug:37.5-.15:Undefined variable Sysctl.lns
lenses/tests/test_sysctl.aug:50.5-.15:Undefined variable Sysctl.lns
lenses/tests/test_sysctl.aug:65.5-.15:Undefined variable Sysctl.lns
lenses/tests/test_sysctl.aug:65.5-.15:Undefined variable Sysctl.lns
lenses/tests/test_sysctl.aug:80.5-.15:Undefined variable Sysctl.lns
lenses/tests/test_sysctl.aug:80.5-.15:Undefined variable Sysctl.lns

I don't understand why you are not experiencing this issue. So, may be there is a problem in my augparse / augeas version ?

Btw note that augparse reports the regexp as /[*:/A-Za-z0-9_.-]+/ but in the lens' source it is /[*:\/A-Za-z0-9_.-]+/, so the error report doesn't respect escaping /. It migth (or might not - just an issue in error output formatting) be symptom of an issue.

waipeng · 2022-10-17T07:36:42Z

First of all, thanks all for working on this! This has been bugging us for a long time, I am really glad there are more people helping out to solve this bug!

I don't understand why you are not experiencing this issue. So, may be there is a problem in my augparse / augeas version ?
@mchf - @georgehansper 's code worked for me. I am using compiling from his PR, can you try that?

waipeng · 2022-10-17T07:40:48Z

Thanks @georgehansper, that test worked.

+net/ipv4/ip_nonlocal_bind = 0

Just a suggestion, to reduce confusion, is it possible to use something that will happen in real life scenario in the test case?

net.ipv4.conf.bond0/15.forwarding = 0

where bond0.15 is a network interface for VLAN15 on bond0.

waipeng · 2022-10-17T07:43:16Z

Just to add, it might also be better to use this PR as is and address / in a separate PR. I've linked a bunch of issues that I have been tracking; having separate PRs solving different bugs might reduce confusion.

like net.ipv4.conf.*.rp_filter = 2

mchf · 2022-10-18T10:07:30Z

I've rebased the patch on top of current master.

georgehansper · 2022-10-22T03:32:24Z

That's looking great.

One last thing, the 'Changes' tab says:

Submodule .gnulib updated from 2f3892 to 7b8cbb

which is reverting .gnulib to a previous version.

On your branch, can merge in the latest version from github, eg.

% git remote -v
origin	https://github.com/hercules-team/augeas.git (fetch)
origin	https://github.com/hercules-team/augeas.git (push)

% git pull origin master

Check the result using:

git submodule status

Expected output:

 2f3892304bd432c5ca3f291b3ef7d8a912a85e96 .gnulib (v0.1-4400-g2f3892304b)

If that's OK, a simple git push to your branch will fix it.

If not, the simplest way to fix the submodule in your branch is to:

cd .gnulib
git checkout 2f3892304b
cd ..
git add .gnulib
git commit -m 'fix gnulib version'

Again, a git push to your branch should fix it

Let me know if this does not work as expected.
The desired end-result is that PR755 will no longer show the submodule in the changes tab.

mchf · 2022-10-24T09:16:02Z

Let me know if this does not work as expected.
The desired end-result is that PR755 will no longer show the submodule in the changes tab.

Simplier solution didn't work ... in fact it was what I did before rebasing the patch. So, I went longer way and hopefully everything is fine now. Thanks for step by step solution

georgehansper

All checks passing.

The update to man/augtool.1 is not really part of this change. It is just a side-effect of the make command.

waipeng · 2022-10-25T07:38:33Z

Thanks @georgehansper for merging this!

One more request - is it possible to tag a version, so that puppet-agent can bump their requirements and get this patch?

There might be a few more things before we can get sysctl handling '/' keys, but we are progressing!

This was referenced Oct 17, 2022

sysctl lens cannot parse values with / #176

Open

Please allow for the use of / as a key separator, as described in the sysctl man page voxpupuli/puppet-augeasproviders_sysctl#17

Open

mchf added 3 commits October 18, 2022 12:01

Sysctl keys can contain some more non-alphanumeric charackters

0da1c6e

like net.ipv4.conf.*.rp_filter = 2

Support / in sysctl key

308b7ad

Updated testsuite

90732ba

mchf force-pushed the fix-sysctl-wildcards branch from 6ddfca2 to 90732ba Compare October 18, 2022 10:07

fix gnulib version

458756a

georgehansper approved these changes Oct 24, 2022

View reviewed changes

georgehansper merged commit e9c7ce3 into hercules-team:master Oct 24, 2022

deric mentioned this pull request Feb 19, 2023

Update module metadata after transfer to Voxpupuli voxpupuli/puppet-augeasproviders_sysctl#69

Closed

johnwarburton mentioned this pull request Jul 12, 2023

quote slash (/) as a key separator when communicating with augeas voxpupuli/puppet-augeasproviders_sysctl#79

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sysctl keys can contain some more non-alphanumeric characters #755

Sysctl keys can contain some more non-alphanumeric characters #755

mchf commented Mar 24, 2022

mchf commented Mar 28, 2022

teclator commented Apr 19, 2022

georgehansper commented Apr 27, 2022 •

edited

Loading

teclator commented Apr 28, 2022

mchf commented Apr 28, 2022

georgehansper commented May 5, 2022

mchf commented May 6, 2022

mvidner commented May 12, 2022

georgehansper commented May 12, 2022

mchf commented May 13, 2022

georgehansper commented Aug 28, 2022

mchf commented Oct 5, 2022

georgehansper commented Oct 5, 2022

mchf commented Oct 6, 2022

georgehansper commented Oct 6, 2022

mchf commented Oct 7, 2022

waipeng commented Oct 17, 2022

waipeng commented Oct 17, 2022

waipeng commented Oct 17, 2022

mchf commented Oct 18, 2022

georgehansper commented Oct 22, 2022

mchf commented Oct 24, 2022

georgehansper left a comment

waipeng commented Oct 25, 2022

Sysctl keys can contain some more non-alphanumeric characters #755

Sysctl keys can contain some more non-alphanumeric characters #755

Conversation

mchf commented Mar 24, 2022

mchf commented Mar 28, 2022

teclator commented Apr 19, 2022

georgehansper commented Apr 27, 2022 • edited Loading

teclator commented Apr 28, 2022

mchf commented Apr 28, 2022

georgehansper commented May 5, 2022

mchf commented May 6, 2022

mvidner commented May 12, 2022

georgehansper commented May 12, 2022

mchf commented May 13, 2022

georgehansper commented Aug 28, 2022

mchf commented Oct 5, 2022

georgehansper commented Oct 5, 2022

mchf commented Oct 6, 2022

georgehansper commented Oct 6, 2022

mchf commented Oct 7, 2022

waipeng commented Oct 17, 2022

waipeng commented Oct 17, 2022

waipeng commented Oct 17, 2022

mchf commented Oct 18, 2022

georgehansper commented Oct 22, 2022

mchf commented Oct 24, 2022

georgehansper left a comment

Choose a reason for hiding this comment

waipeng commented Oct 25, 2022

georgehansper commented Apr 27, 2022 •

edited

Loading