[BUG] salt-ssh does not merge complex pillar data provided in the command-line for jinja templates

[xmsk]

Description
I am running salt-ssh to apply salt states to a remote minion. I am providing custom pillar data in the command line to override values that I have defined in the pillar SLS files. The pillar data is properly merged for use in the SLS files but fails to properly merge for jinja templates in a file.managed state.

The issue only appears for 'complex' pillar data, i.e. (see example below) when there is a top-level key (complex) with an object value that has several key-value pairs (key1:val1, key2:val2), top-level pillar data is not affected (e.g. top_level).

Setup
I have provided a working example to reproduce the issue in this repo: https://github.com/xmsk/salt-ssh-jinja-pillar-merge.

defaults.sls

complex:
  key1: val1
  key2: val2

top_level: top_val

complex.sls

file managed:
  file.managed:
    - name: /tmp/salt-managed.txt
    - source: salt://files/salt.txt.j2
    - template: jinja
    - user: root
    - group: root
    - mode: 644

file append:
  file.append:
    - name: /tmp/salt-append.txt
    - text: |
        key1: {{ pillar.get('complex', {}).get('key1') }}
        key2: {{ pillar.get('complex', {}).get('key2') }}
        top_level: {{ pillar.get('top_level') }}

jinja text template salt.txt.j2

key1: {{ pillar.get('complex', {}).get('key1') }}
key2: {{ pillar.get('complex', {}).get('key2') }}
top_level: {{ pillar.get('top_level') }}

Steps to Reproduce the behavior
Run the following command

salt-ssh -i '*' state.apply complex pillar='{"complex": {"key2": "new val"}}'

Expected behavior
The following files are present on the minion

/tmp/salt-append.txt

key1: val1
key2: new val
top_level: top_val

/tmp/salt-managed.txt

key1: val1
key2: new val
top_level: top_val

Actual Behavior
The following files are present on the minion

(correct) /tmp/salt-append.txt

key1: val1
key2: new val
top_level: top_val

(key1 has value None instead of val1) /tmp/salt-managed.txt

key1: None
key2: new val
top_level: top_val

Versions Report
I tested using the latest commit on master and also tested with salt-ssh version 3002.5.

salt --versions-report

Salt Version:
          Salt: 3003rc1+1.g4af72e7875
 
Dependency Versions:
          cffi: Not Installed
      cherrypy: Not Installed
      dateutil: Not Installed
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 2.11.3
       libgit2: Not Installed
      M2Crypto: Not Installed
          Mako: Not Installed
       msgpack: 1.0.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: Not Installed
      pycrypto: Not Installed
  pycryptodome: 3.10.1
        pygit2: Not Installed
        Python: 3.9.2 (default, Feb 20 2021, 18:40:11)
  python-gnupg: Not Installed
        PyYAML: 5.4.1
         PyZMQ: 22.0.3
         smmap: Not Installed
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.4
 
System Versions:
          dist: arch  
        locale: utf-8
       machine: x86_64
       release: 5.11.2-arch1-1
        system: Linux
       version: Arch Linux

Additional context
I have also tested with a master/minion (version 3002.2) set up using the salt command and the problem is not present there.

For the salt command I have found a related issue #18429 which seems to have been fixed a while ago. Another issue that seems to be related is #27494.

[welcome]

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

• Community Wiki
• Salt’s Contributor Guide
• Join our Community Slack
• IRC on Freenode
• SaltStack YouTube channel
• SaltStackInc Twitch channel

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

[waynew]

Yep, looks like pillar merging doesn't work quite correctly for salt-ssh when it's passed in via the command line.

Verified that

salt-ssh some_minion state.apply pillar='{"complex": {"key2": "new val"}}'

will overwrite the pillar value, while

salt-call --local state.apply pillar='{"complex": {"key2": "new val"}}'

works as expected, merging the dictionaries as documented in https://docs.saltproject.io/en/latest/ref/pillar/all/salt.pillar.stack.html

I didn't track down why it's happening, but it definitely is, and appears only to affect salt-ssh.

[waynew]

I've double checked on 3002.2 and salt-ssh and I am unable to see correct behavior, so I'm thinking that this was always (at least as far back as 3002) an issue with salt-ssh

[vladaurosh]

Having exact same issue with salt-ssh 3004. Are there plans to fix this issue?

[lkubb]

This is caused by the state wrapper just updating __pillar__ with kwargs.get("pillar", {}):

salt/salt/client/ssh/wrapper/state.py

Line 175 in 560bacd

__pillar__.update(kwargs.get("pillar", {}))

The SSHState class does not support pillar overrides like the regular State:

salt/salt/client/ssh/state.py

Lines 29 to 34 in 560bacd

	class SSHState(salt.state.State):
	"""
	Create a State object which wraps the SSH functions for state operations
	"""
	def __init__(self, opts, pillar=None, wrapper=None, context=None):

salt/salt/state.py

Lines 732 to 735 in 560bacd

	def __init__(
	self,
	opts,
	pillar_override=None,

salt/salt/state.py

Lines 768 to 781 in 560bacd

	if initial_pillar and not self._pillar_override:
	self.opts["pillar"] = initial_pillar
	else:
	# Compile pillar data
	self.opts["pillar"] = self._gather_pillar()
	# Reapply overrides on top of compiled pillar
	if self._pillar_override:
	self.opts["pillar"] = salt.utils.dictupdate.merge(
	self.opts["pillar"],
	self._pillar_override,
	self.opts.get("pillar_source_merging_strategy", "smart"),
	self.opts.get("renderer", "yaml"),
	self.opts.get("pillar_merge_lists", False),
	)

Edit3:
I got sidetracked with the below. I can repro this, not in a rendered sls template, but when a state renders a template on the target. The below is valid, but the reason why it does not work on the target is that __opts__["pillar"] still contains the .update()d dict, not the one used during sls rendering on the master.
This btw also means that ext_pillars are not refreshed with the overridden pillar for the target.

Sidetrack

Edit: While writing a fix for this, I noticed I could not reproduce the failure. Part of the cause is that the SSHState in fact passes the whole pillar (which is the .update()'d __pillar__) as an override to the pillar compilation. Example:

(1) original pillar:

complex:
  value1: foo
  value2: bar

(2) override:

complex:
  value2: baz

pillar.update(override):

complex:
  value2: baz

and then salt.State passes this as pillar_override to salt.pillar.get_pillar, which usually compiles (1) and merges (2) recursively, keeping value1 intact.

I suspect this bug is caused by the combination of the aforementioned behavior PLUS the failure of properly compiling (1) for some reason - misalignment of master/minion opts somewhere?

On a sidenote:
The SSHState might better override _gather_pillar to ensure the correct opts are used for the compilation of (1), especially that the master opts are available for runners that might be called. From what I can tell, it currently uses the modified minion opts only (in simple cases), while salt.client.ssh.Single uses:

popts = {}
popts.update(opts["__master_opts__"])  # actual master __opts__ are embedded one level down
popts.update(opts)  # opts reported by the minion via salt-ssh minion test.opts_pkg, modified

In addition, the current behavior results in unnecessary duplicate pillar compilations when applying states without pillar overrides, which were fixed for salt-call in #41679.

A proper fix would thus include updating the state wrapper in a similar way.

Edit2: Pillar merging is no joke:

By default, when running 'state.apply' (with or without pillar={...}), this is roughly how the final pillar is made [let's call (1) singleopts and (2) singleopts_upd]:

  singleopts_upd + *.sls rendered   # render_pillar
+ singleopts_upd + ext_pillar       # ext_pillar
+ singleopts_upd                    # if ssh_merge_pillar
+ singleopts_upd                    # always, because it's passed as an override
+ singleopts_upd                    # because salt.state.State merges it again

This becomes even funnier when ext_pillar_first was enabled:
((((singleopts_upd + singleopts_upd + ext_pillars) + (singleopts_upd + *.sls)) + (singleopts_upd + singleopts_upd + ext_pillars)) + singleopts_upd) + singleopts_upd.

[dwoz]

Closing as fixed per #65067 (comment)