[BUG] ini_manage always updates file even if there are no changes

[jcpearson]

Description
When using ini.options_present in a state, the ini file always gets updated even if there have been no changes

Setup
Using something like:

/tmp/test.ini:
  ini.options_present:
    - sections:
        A:
          a: 1
        B:
          b: 2
        C:
          c: 3
        D:
          d: 4

Steps to Reproduce the behavior
Every time the state is applied, the timestamp of /tmp/test.ini is always updated - even if the state hasn't changed

Expected behavior
/tmp/test.ini to remain untouched if no changes have been made to the state

Versions Report

Salt Version:
          Salt: 3006.3
 
Python Version:
        Python: 3.10.13 (main, Sep  6 2023, 02:11:27) [GCC 11.2.0]
 
Dependency Versions:
          cffi: 1.14.6
      cherrypy: 18.6.1
      dateutil: 2.8.1
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.1.2
       libgit2: Not Installed
  looseversion: 1.0.2
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     packaging: 22.0
     pycparser: 2.21
      pycrypto: Not Installed
  pycryptodome: 3.9.8
        pygit2: Not Installed
  python-gnupg: 0.4.8
        PyYAML: 6.0.1
         PyZMQ: 23.2.0
        relenv: 0.13.10
         smmap: Not Installed
       timelib: 0.2.4
       Tornado: 4.5.3
           ZMQ: 4.3.4
 
System Versions:
          dist: centos 7.9.2009 Core
        locale: utf-8
       machine: x86_64
       release: 3.10.0-1160.81.1.el7.x86_64

[OrangeDog]

Does the state report changes (and if so, what), or is it just the mtime being changed?

[jcpearson]

Running the following commands shows that the file contents don't update, but the atime/mtime/ctime is updated:

# stat /tmp/test.ini 
  File: ‘/tmp/test.ini’
  Size: 43              Blocks: 8          IO Block: 4096   regular file
Device: 803h/2051d      Inode: 34230479    Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2023-09-14 13:11:46.746909196 +0100
Modify: 2023-09-14 13:11:46.746909196 +0100
Change: 2023-09-14 13:11:46.746909196 +0100
 Birth: -

# md5sum /tmp/test.ini
5bf3e0fbca74537932b18d6a7f83f04e  /tmp/test.ini

# salt-call state.apply formula.test
local:
----------
          ID: /tmp/test.ini
    Function: ini.options_present
      Result: True
     Comment: No anomaly detected
     Started: 13:50:22.763280
    Duration: 3.862 ms
     Changes:   

Summary for local
------------
Succeeded: 1
Failed:    0
------------
Total states run:     1
Total run time:   3.862 ms

# stat /tmp/test.ini 
  File: ‘/tmp/test.ini’
  Size: 43              Blocks: 8          IO Block: 4096   regular file
Device: 803h/2051d      Inode: 34230479    Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2023-09-14 13:50:22.765941778 +0100
Modify: 2023-09-14 13:50:22.765941778 +0100
Change: 2023-09-14 13:50:22.765941778 +0100
 Birth: -

# md5sum /tmp/test.ini
5bf3e0fbca74537932b18d6a7f83f04e  /tmp/test.ini

[OrangeDog]

It's probably due to this section here, where it sets values without first checking if it needs to.

options_updated = __salt__["ini.set_option"](
    name, {section_name: section_body}, separator
)
if options_updated:
    changes[section_name].update(options_updated[section_name])
if not changes[section_name]:
    del changes[section_name]

Does it actually cause any problems though?

[jcpearson]

Everything else in Salt only updates files if there is a change

It's bad practice for a config management tool to change things it doesn't need to, so should really be fixed ...

[jcpearson]

Having a look at the code in states/ini_manage.py, it seems strange that it goes to a lot of trouble to see if anything has changed when in test mode, but not otherwise ?

I'm not a python developer, but I can 'fix' my issue with the following hack (I'm only using ini.options_present - without the 'strict' option set, so ignored anywhere else in the code it may update the file without checking for changes):

diff --git a/salt/states/ini_manage.py b/salt/states/ini_manage.py
index 9d39897..5eb6311 100644
--- a/salt/states/ini_manage.py
+++ b/salt/states/ini_manage.py
@@ -49,7 +49,8 @@ def options_present(name, sections=None, separator="=", strict=False):
         "result": True,
         "comment": "No anomaly detected",
     }
-    if __opts__["test"]:
+    testing = __opts__["test"]
+    if testing:
         ret["comment"] = ""
     # pylint: disable=too-many-nested-blocks
     try:
@@ -67,18 +68,26 @@ def options_present(name, sections=None, separator="=", strict=False):
                     original_sections.update({key: val})
                 else:
                     original_top_level_opts.update({key: val})
-            if __opts__["test"]:
-                for option in options:
-                    if option in original_top_level_opts:
-                        if str(original_top_level_opts[option]) == str(options[option]):
+
+            update = False
+            for option in options:
+                if option in original_top_level_opts:
+                    if str(original_top_level_opts[option]) == str(options[option]):
+                        if testing:
                             ret["comment"] += "Unchanged key {}.\n".format(option)
-                        else:
-                            ret["comment"] += "Changed key {}.\n".format(option)
-                            ret["result"] = None
-                    else:
+                    elif testing:
                         ret["comment"] += "Changed key {}.\n".format(option)
                         ret["result"] = None
-            else:
+                    else:
+                        update = True
+                        break
+                elif testing:
+                    ret["comment"] += "Changed key {}.\n".format(option)
+                    ret["result"] = None
+                else:
+                    update = True
+                    break
+            if update:
                 options_updated = __salt__["ini.set_option"](name, options, separator)
                 changes.update(options_updated)
             if strict:
@@ -126,20 +135,24 @@ def options_present(name, sections=None, separator="=", strict=False):
                             changes[section_name].update(
                                 {key_to_remove: {"before": orig_value, "after": None}}
                             )
-                if __opts__["test"]:
-                    for option in section_body:
-                        if str(section_body[option]) == str(
-                            original_sections.get(section_name, {}).get(option, "#-#-")
-                        ):
+                update = False
+                for option in section_body:
+                    if str(section_body[option]) == str(
+                        original_sections.get(section_name, {}).get(option, "#-#-")
+                    ):
+                        if testing:
                             ret["comment"] += "Unchanged key {}{}.\n".format(
                                 option, section_descr
                             )
-                        else:
-                            ret["comment"] += "Changed key {}{}.\n".format(
-                                option, section_descr
-                            )
-                            ret["result"] = None
-                else:
+                    elif testing:
+                        ret["comment"] += "Changed key {}{}.\n".format(
+                            option, section_descr
+                        )
+                        ret["result"] = None
+                    else:
+                        update = True
+                        break
+                if update:
                     options_updated = __salt__["ini.set_option"](
                         name, {section_name: section_body}, separator
                     )
@@ -148,7 +161,7 @@ def options_present(name, sections=None, separator="=", strict=False):
                     if not changes[section_name]:
                         del changes[section_name]
         else:
-            if not __opts__["test"]:
+            if not testing:
                 changes = __salt__["ini.set_option"](name, sections, separator)
     except (OSError, KeyError) as err:
         ret["comment"] = "{}".format(err)

[jcpearson]

I believe I have a patch that fixes this problem for all the states in ini_manage.py - but I don't have the knowledge/capabilities/resources to submit this via git etc - so I've attached it here

ini_manage.patch.gz