Salt: [BUG] sysctl.present broken in FreeBSD

[ari]

          ID: security.jail.sysvipc_allowed
    Function: sysctl.present
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/local/lib/python3.7/site-packages/salt/state.py", line 2154, in call
                  *cdata["args"], **cdata["kwargs"]
                File "/usr/local/lib/python3.7/site-packages/salt/loader.py", line 2085, in wrapper
                  return f(*args, **kwargs)
                File "/usr/local/lib/python3.7/site-packages/salt/states/sysctl.py", line 117, in present
                  update = __salt__["sysctl.persist"](name, value, config, ignore)
              TypeError: persist() takes from 2 to 3 positional arguments but 4 were given

Looks like it was broken by this https://github.com/saltstack/salt/pull/55719 since the author of that commit didn't do any testing or review on any platform other than linux.

[ari]

The breakage was introduced in 3001.

[ari]

@mchugh19 and @dwoz Please review your commits which broke a basic feature in FreeBSD.

[krionbsd]

@ari thanks for submitting it, we'll officially run tests on FreeBSD very soon. Do you want to provide the patch to fix it?

[ari]

Well, if it were me I'd revert the whole of the patch that broke it so that author could rewrite it a safe way without assuming that Linux is the only operating system. I don't understand why this feature would be added at all to the generic sysctl.py codebase. What problem did it solve, why does it exist, what does it do?

[asomers]

See also downstream bug report https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=247627 .

[mchugh19]

Hi all,

@ari this was obviously an unintended breakage, and would have been helped by having a test suite running for non linux OSs. I apologize for the breakage and disruption. A long time ago I was a Solaris sysadmin, so I understand wanting to keep things supported everywhere.

I don't understand why this feature would be added at all to the generic sysctl.py codebase. What problem did it solve, why does it exist, what does it do?

The problem it solves is in the linked bug report: #45195 as well as brought up in the community here: https://www.reddit.com/r/saltstack/comments/ebgxjh/saltstatessysctl/

I've just submitted a possible fix in #57841. Can you try it out to verify the solution?

[ari]

Hi @mchugh19,

Yes, some better CI servers for the salt project would certainly help, although they didn't catch the OSX errors here. And keeping salt OS agnostic is great for the health of the project.

The problems I see with your patch are this:

• it is going to break in the future when someone wants to add another option. Is there are reason named params are not used?
• it is fragile in that it will be easily broken by someone who doesn't understand the details of this hack in the future
• if someone adds the ignore keyword by mistake, the breakage error message/stacktrace makes no sense
• 'ignore' is a confusing name for the feature. More common salt syntax might be "skip_errors".
• the feature could be easily implemented for all platforms by just ignoring the response code or testing the value first. Is there a need for a specific platform implementation?
• There needs to be more documentation about what this feature does without reading source code.

Overall, I guess I don't see the point of this whole feature since my salt is broken up into states which only apply to servers where they are relevant. What is the use-case of applying sysctls to the wrong machines? But I acknowledge everyone is different...

[mchugh19]

Hi @ari

You are correct, there are other ways to handle restoration of the sysctl state for non-linux OSs.
Do you have a suggestion for a less brittle or more clear method for handling the sysctl ignore case?

1. I'm not sure what you are asking about named params. Both the linux sysctl state and module are using named params where ignore is a boolean.
2. What's the suggestion? Where supported, the ignore bool will be passed along to the sysctl state.
3. True. Is the suggestion to add ignore as a noop to other OSs sysctl files?
4. It was based on the linux sysctl command line. But this could be extended to other OSs and the name made more generic.
5. It was based on the linux sysctl command line. Ignoring retcodes on other OSs could be an option.
6. Where do you suggest adding documentation? The ignore option was documented in the linux sysctl state and module in #55719
   Module:
   > :param ignore: Optional boolean to pass --ignore to sysctl (Default: False)
   State:
   >ignore
   ..versionadded:: neon
   Adds --ignore to sysctl commands. This suppresses errors in environments
   where sysctl settings may have been disabled in kernel boot configuration.
   Defaults to False

It was a mistake and unfortunate that this broke non-linux sysctl states. The patch in #57841 was intended as a quick fix which could be added to 3001.1 to restore functionality quickly. I see possible next steps as:

1. Allow non-linux OSs to ignore sysctl setting failures by ignoring the sysctl command retcode
2. Update the documentation and support the more generic sounding "skip_errors"

Do you have any other suggestions?

[ari]

1. update = __salt__"sysctl.persist" is the method call you are patching here. Four parameters passed by position. Are you anticipating that another param will never be added? Ever.
   3-5. Yes, implementing this feature properly across all OS makes sense. A user is interacting with salt, not the OS directly, so its salt's job to do things consistently. If I only wanted to interact with OS commands, every state would be cmd.run.

I like your possible next steps.

[sagetherage]

de-scoping from Magnesium as the regression test has not yet been written

[bgdnlp]

So, sysctl is going to remain broken on non-Linux for at least another release?

[krionbsd]

So, sysctl is going to remain broken on non-Linux for at least another release?

FreeBSD port and package contain the fix

[asomers]

That's an untenable situation. Separately maintaining that change will be maintenance-intensive. Why won't you simply revert the change that introduced the breakage?