[BUG] file.comment ignore_missing not working

[DaAwesomeP]

Description
The ignore_missing option on file.comment is not working.

Setup
(Please provide relevant configs and/or SLS files (be sure to remove sensitive info. There is no general set-up of Salt.)

Please be as specific as possible and give set-up details.

• on-prem machine
• VM (VMWare)
• VM running on a cloud service, please be explicit and add details
• container (Kubernetes, Docker, containerd, etc. please specify)
• or a combination, please be explicit
• jails if it is FreeBSD
• classic packaging
• onedir packaging
• used bootstrap to install

Steps to Reproduce the behavior

openssh-server_comment_permitrootlogin_sshd_config:
  file.comment:
    - name: "/etc/ssh/sshd_config"
    - regex: ^PermitRootLogin[ \t]+.*$
    - char: "# NEXT LINE COMMENT SALTSTACK openssh-server_comment_permitrootlogin_sshd_config\n# "
    - ignore_missing: True

Line in /etc/ssh/sshd_config is already commented out (not via Saltstack) so there is no match:

#PermitRootLogin prohibit-password

Expected behavior
State should pass.

Screenshots

----------
          ID: openssh-server_comment_permitrootlogin_sshd_config
    Function: file.comment
        Name: /etc/ssh/sshd_config
      Result: False
     Comment: Expected commented lines not found
     Started: 16:38:34.339699
    Duration: 2.78 ms
     Changes:   

Versions Report

salt --versions-report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)

Salt Version:
          Salt: 3006.4
 
Python Version:
        Python: 3.10.13 (main, Oct  4 2023, 21:54:22) [GCC 11.2.0]
 
Dependency Versions:
          cffi: 1.14.6
      cherrypy: unknown
      dateutil: 2.8.1
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.1.2
       libgit2: 1.7.1
  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: 1.13.1
  python-gnupg: 0.4.8
        PyYAML: 6.0.1
         PyZMQ: 23.2.0
        relenv: 0.13.12
         smmap: Not Installed
       timelib: 0.2.4
       Tornado: 4.5.3
           ZMQ: 4.3.4
 
System Versions:
          dist: debian 12 bookworm
        locale: utf-8
       machine: x86_64
       release: 6.1.0-13-amd64
        system: Linux
       version: Debian GNU/Linux 12 bookworm

Additional context
Related pull: #62045

[DaAwesomeP]

It seems that it is getting past this block where it would return early due to ignore_missing:

salt/salt/states/file.py

Lines 6177 to 6188 in 53f54d0

	# Make sure the pattern appears in the file before continuing
	if not __salt__["file.search"](name, uncomment_regex, multiline=True):
	if __salt__["file.search"](name, comment_regex, multiline=True):
	ret["comment"] = "Pattern already commented"
	ret["result"] = True
	return ret
	elif ignore_missing:
	ret["comment"] = "Pattern not found and ignore_missing set to True"
	ret["result"] = True
	return ret
	else:
	return _error(ret, f"{unanchor_regex}: Pattern not found")

@nicholasmhughes any ideas?

[nicholasmhughes]

I think the regex hates your char...

The uncomment_regex in the code ends up being:

^(?!\\s*# NEXT LINE COMMENT SALTSTACK openssh-server_comment_permitrootlogin_sshd_config\n# )*PermitRootLogin[ \\t]+.*

and matches the line even when it shouldn't because we're using a negative look-ahead (which I kinda hate, but can't think of a good way to avoid...)

The test cases are single characters without newline characters.

I'll have to play with this some more to see the best way to fix it for this use case.

[DaAwesomeP]

@nicholasmhughes ah, I see! To help with testing, here is another example:

openssh-server_comment_authenticationmethods_sshd_config:
  file.comment:
    - name: "/etc/ssh/sshd_config"
    - regex: ^AuthenticationMethods[ \t]+.*$
    - char: "# NEXT LINE COMMENT SALTSTACK openssh-server_comment_authenticationmethods_sshd_config\n# "
    - ignore_missing: True

Though in this case this match is not present in the file at all (commented or uncommented). Because of that maybe this one is not having the same issue for me.

[nicholasmhughes]

Near as I can tell, this might be best fixed by adding another parameter. Essentially what you're trying to do is comment the line with one char if found, but use a different char for search purposes. If we had comment_char and search_char (or something like that), would you be okay with a solution like that?

[DaAwesomeP]

Yeah, that would make sense. I can't immediately think of any caveats to decouple the search and find/replace. It'd probably be a good idea to include a newline example in the updated documentation so it is clear why you would separate them.

That's easy to make backward compatible too:

• If char is defined use it for both
• If char and either comment_char or search_char is defined, error
• If only one of search_char or comment_char is defined, error
• If both search_char and comment_char are defined, use them

[nicholasmhughes]

played around with the regex some more and I think .* was matching \n# in the "find the uncommented line" regex... which isn't what we want. #65552 should fix it.