Grain targeting matches any minion (when it's cache directory is missing)

[adamlamers]

Description of Issue

When targeting minions with the -G flag in the salt CLI, salt will match any minion that does not have an existing directory in /var/cache/salt/master/minions. This leads to many unwanted minions being targeted when attempting to match a grain.

Setup

• Have an offline salt minion with an accepted key on the master.
• Delete directory /var/cache/salt/master/minions/<minion_id>

Steps to Reproduce Issue

• Run salt -G 'asd:def' test.ping
• The offline minion will be targeted by this, even though it most likely doesn't have a grain of "asd" with value "def". These can be arbitrary values.
• Create a directory /var/cache/salt/master/minions/<minion_id> and run the same grain targeting call, and it will no longer be targeted.

Versions Report

(My main salt master versions, however it appears to also happen in 2019.2)

Salt Version:
Salt: 2017.7.2

Dependency Versions:
cffi: 1.11.2
cherrypy: 3.2.3
dateutil: 2.2
docker-py: Not Installed
gitdb: 0.5.4
gitpython: 0.3.2 RC1
ioflo: Not Installed
Jinja2: 2.9.6
libgit2: Not Installed
libnacl: Not Installed
M2Crypto: Not Installed
Mako: Not Installed
msgpack-pure: Not Installed
msgpack-python: 0.4.8
mysql-python: 1.2.3
pycparser: 2.18
pycrypto: 2.6.1
pycryptodome: Not Installed
pygit2: Not Installed
Python: 2.7.9 (default, Jun 29 2016, 13:08:31)
python-gnupg: Not Installed
PyYAML: 3.12
PyZMQ: 16.0.2
RAET: Not Installed
smmap: 0.8.2
timelib: Not Installed
Tornado: 4.5.2
ZMQ: 4.1.6

System Versions:
dist: debian 8.1
locale: UTF-8
machine: x86_64
release: 3.16.0-4-amd64
system: Linux
version: debian 8.1

[cmcmarrow]

We no longer support 2017. Does this happen in 2019.2.1?

[adamlamers]

It does still happen in 2019.2.1.

It's very obvious when you copy over accepted keys from another salt master but haven't had all the minions connected to it yet. However, it also still happens if you just delete a previously connected minion's cache dir.

This seems to happen naturally with minions that are briefly online after having their key accepted as well, causing them to get matched (in grain targeting) until they come online for a period of time and salt has a chance to create that cache.

[stale[bot]]

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.

[wojaczek]

Hey team!
We're experiencing this on 2019.02 and it's truly annoying. I'm only the user, not the Salt admin, so don't have access to the master config, but what I see is when we're using grain or compound matching, the servers, that are unreachable are targeted even though the grain does not exists for them and the state is failing because of these unreachable nodes.
While this bug really exists, do you know if there is any switch for state.sls that can make it not failing on unresponsive nodes?

[kibi]

Hi salt supporters!

This issue still exists in 3002.6

I was able to make a workaround by creating /var/cache/salt/master/minions/<minion_id> directory for every minion returned by salt-key utility.

[ep5000]

Why was this closed? This bug still exists in 3004.2.

[kobus-v-schoor]

Just confirming that this is still happening, using the workaround that @kibi posted

[L4rS6]

Please reopen the issue as problem still exists in 3006.1. Workaround by @kibi is working well.

[L4rS6]

@Ch3LL Could you please reopen this issue?

[Ch3LL]

@MKLeb mind taking a look at this one for 3007.0

[marco-kusa]

Just noticed this on 3005 and amazed it's been around for so long, why has it been marked as a feature? Surely returning unmatching minions is a bug?

[terminalmage]

This is being caused by the fact that greedy matching is the default in salt.utils.minions.CkMinions.check_minions(), and this call in the master does not override this. When greedy matching is used, minions absent from the cache (i.e. minions for which there are accepted keys, but no cache data) are included in the match.

[iuooip]

Can we get this prioritised? This known bug is currently impacting production environments, and it makes Salt look like a tool that can't be trusted to work as documented.

The fix:

+++ b/salt/utils/minions.py
@@ -343,8 +343,8 @@ class CkMinions:
                     continue
                 mdata = self.cache.fetch(search_type, id_)
                 if mdata is None:
-                    if not greedy:
-                        minions.remove(id_)
+                    # mdata is None when the minion does not exist in the cache
+                    minions.remove(id_)
                     continue
                 if not salt.utils.data.subdict_match(
                     mdata,

[iuooip]

Correction: if mdata is None or not mdata: fixes the instance startup scenario but for instance going down you still need to do a version of sudo salt-run manage.down removekeys=True.
It seems that saltmaster needs to be notified about shutting down event. You can do this in various ways depending on your environment.
Possible solutions:

• crontab with removekeys=True
• run removekeys=True before your deployment in a pipeline
• reactor state in tandem with a service that sends ExecStart=/usr/bin/salt-call event.send 'minion/terminating' id=%H Before=shutdown.target halt.target
• reading from a queue with reactor state