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

sysctl.present replaces spaces by tabs #40054

Closed
bdrung opened this issue Mar 15, 2017 · 5 comments · Fixed by #61452
Closed

sysctl.present replaces spaces by tabs #40054

bdrung opened this issue Mar 15, 2017 · 5 comments · Fixed by #61452
Labels
Bug broken, incorrect, or confusing behavior P2 Priority 2 Platform Relates to OS, containers, platform-based utilities like FS, system based apps severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around stale
Milestone

Comments

@bdrung
Copy link
Contributor

bdrung commented Mar 15, 2017

Description of Issue/Question

salt/modules/linux_sysctl.py tries to be smart in persist() and replaces spaces by tabs:

        # On Linux procfs, files such as /proc/sys/net/ipv4/tcp_rmem or any
        # other sysctl with whitespace in it consistently uses 1 tab.  Lets
        # allow our users to put a space or tab between multi-value sysctls
        # and have salt not try to set it every single time.
        if isinstance(comps[1], string_types) and ' ' in comps[1]:
            comps[1] = re.sub(r'\s+', '\t', comps[1])

        # Do the same thing for the value 'just in case'
        if isinstance(value, string_types) and ' ' in value:
            value = re.sub(r'\s+', '\t', value)

This behavior exists since at least salt 0.11. Sadly following state:

kernel-core-pattern:
    sysctl.present:
        - config: /etc/sysctl.d/50-dump-core.conf
        - name: kernel.core_pattern
        - value: "|/usr/share/kdump-tools/dump-core %p %s %t %e"

leads to following setting:

$ hd /proc/sys/kernel/core_pattern 
00000000  7c 2f 75 73 72 2f 73 68  61 72 65 2f 6b 64 75 6d  ||/usr/share/kdum|
00000010  70 2d 74 6f 6f 6c 73 2f  64 75 6d 70 2d 63 6f 72  |p-tools/dump-cor|
00000020  65 09 25 70 09 25 73 09  25 74 09 25 65 0a        |e.%p.%s.%t.%e.|
0000002e

There is a difference between tabs and spaces for this setting!

Versions Report

$ salt-call --versions-report
Salt Version:
           Salt: 2016.11.2
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.2
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.3
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.2
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
         pygit2: Not Installed
         Python: 2.7.9 (default, Jun 29 2016, 13:08:31)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 14.4.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.5
 
System Versions:
           dist: debian 8.7 
        machine: x86_64
        release: 4.4.36-2-pserver
         system: Linux
        version: debian 8.7 
@gtmanfred
Copy link
Contributor

Thanks for reporting this.

Looks like this was added back in 0.9.8 545dca0

@SEJeff do you have any opinions about this?

Thanks,
Daniel

@gtmanfred gtmanfred added Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around P2 Priority 2 Platform Relates to OS, containers, platform-based utilities like FS, system based apps TEAM Platform labels Mar 15, 2017
@gtmanfred gtmanfred added this to the Approved milestone Mar 15, 2017
@stale
Copy link

stale bot commented Sep 16, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

@bdrung
Copy link
Contributor Author

bdrung commented Mar 26, 2020

please reopen

@gtmanfred
Copy link
Contributor

@sagetherage

@bdrung
Copy link
Contributor Author

bdrung commented Aug 5, 2020

Just confirmed that the behavior is still the same in salt 3000.2.

bdrung added a commit to bdrung/salt that referenced this issue Jan 13, 2022
`salt/modules/linux_sysctl.py` tries to be smart in `persist()` and
replaces spaces by tabs, which will be correct for keys like
`net.ipv4.tcp_rmem` but not for `kernel.core_pattern`.

So only sanitize the values for comparison, but not for persisting them
in the configuration file.

Bug: saltstack#40054
Forwarded: saltstack#61452
Signed-off-by: Benjamin Drung <[email protected]>
bdrung added a commit to bdrung/salt that referenced this issue Feb 4, 2022
`salt/modules/linux_sysctl.py` tries to be smart in `persist()` and
replaces spaces by tabs, which will be correct for keys like
`net.ipv4.tcp_rmem` but not for `kernel.core_pattern`.

So only sanitize the values for comparison, but not for persisting them
in the configuration file.

Bug: saltstack#40054
Signed-off-by: Benjamin Drung <[email protected]>
bdrung added a commit to bdrung/salt that referenced this issue Apr 15, 2022
`salt/modules/linux_sysctl.py` tries to be smart in `persist()` and
replaces spaces by tabs, which will be correct for keys like
`net.ipv4.tcp_rmem` but not for `kernel.core_pattern`.

So only sanitize the values for comparison, but not for persisting them
in the configuration file.

Bug: saltstack#40054
Forwarded: saltstack#61452
Signed-off-by: Benjamin Drung <[email protected]>
Gbp-Pq: Name linux_sysctl.present-Fix-replacing-spaces-by-tabs.patch
bdrung added a commit to bdrung/salt that referenced this issue Apr 16, 2022
`salt/modules/linux_sysctl.py` tries to be smart in `persist()` and
replaces spaces by tabs, which will be correct for keys like
`net.ipv4.tcp_rmem` but not for `kernel.core_pattern`.

So only sanitize the values for comparison, but not for persisting them
in the configuration file.

Bug: saltstack#40054
Forwarded: saltstack#61452
Signed-off-by: Benjamin Drung <[email protected]>
Gbp-Pq: Name linux_sysctl.present-Fix-replacing-spaces-by-tabs.patch
bdrung added a commit to bdrung/salt that referenced this issue Apr 16, 2022
`salt/modules/linux_sysctl.py` tries to be smart in `persist()` and
replaces spaces by tabs, which will be correct for keys like
`net.ipv4.tcp_rmem` but not for `kernel.core_pattern`.

So only sanitize the values for comparison, but not for persisting them
in the configuration file.

Bug: saltstack#40054
Forwarded: saltstack#61452
Signed-off-by: Benjamin Drung <[email protected]>
Gbp-Pq: Name linux_sysctl.present-Fix-replacing-spaces-by-tabs.patch
garethgreenaway added a commit that referenced this issue Sep 23, 2022
…conf files (#61452)

* test_linux_sysctl: Reduce indentation level

Reduce indentation level to improve readability.

Signed-off-by: Benjamin Drung <[email protected]>

* test_linux_sysctl: Fix test_persist_no_conf_failure

`test_persist_no_conf_failure` has different behaviour depending on the
operating system it is executed on. Mock all system calls and let it
fail in the first try to write the sysctl configuration file.

Signed-off-by: Benjamin Drung <[email protected]>

* linux_sysctl: Pass parameters as list to cmd.run

Signed-off-by: Benjamin Drung <[email protected]>

* linux_sysctl.show: Fix parsing of sysctl.conf files

The lines in sysctl.conf files can contain leading and trailing spaces.
The equal sign (that separates the key and value) might have zero to
multiple white spaces. Valid example:

```
kernel.core_pattern = |/usr/share/kdump-tools/dump-core %p %s %t %e
 # Stop low-level messages on console = less logging
 kernel.printk  = 3 4 1 3

net.ipv4.ip_forward=1
```

`sysctl.show` currently has two issues:
* all values contain a trailing newline
* Comments with leading spaces and equal sign in them are not skipped

Address both issues and simplify the code and cover them with unit tests.

Signed-off-by: Benjamin Drung <[email protected]>

* linux_sysctl.present: Fix replacing spaces by tabs

`salt/modules/linux_sysctl.py` tries to be smart in `persist()` and
replaces spaces by tabs, which will be correct for keys like
`net.ipv4.tcp_rmem` but not for `kernel.core_pattern`.

So only sanitize the values for comparison, but not for persisting them
in the configuration file.

Bug: #40054
Signed-off-by: Benjamin Drung <[email protected]>

* linux_sysctl: Drop unreachable code

The check `if "=" not in line` already ensures that the split by `=`
will return two elements.

Signed-off-by: Benjamin Drung <[email protected]>

* fixing failing tests

* need to mock which in test_assign_success

Signed-off-by: Benjamin Drung <[email protected]>
Co-authored-by: Benjamin Drung <[email protected]>
Co-authored-by: Gareth J. Greenaway <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior P2 Priority 2 Platform Relates to OS, containers, platform-based utilities like FS, system based apps severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants