Sysctl keys can contain some more non-alphanumeric characters

[mchf]

Tries to address https://github.com/hercules-team/augeas/issues/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]

@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]

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

[georgehansper]

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]

@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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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.

[mchf]

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

[georgehansper]

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]

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

[waipeng]

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!