Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compound targeting includes unexpected targets in the result #49205

Open
ereslibre opened this issue Aug 20, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@ereslibre
Copy link

commented Aug 20, 2018

Description of Issue/Question

Compound targeting includes unexpected (even nonexistant) targets in the result.

Setup

This was triggered by an orchestration that rejects a salt minion in the middle of the orchestration. The tgt used after the salt minion key is rejected included an explicit filter to not take this minion into consideration; even if salt guarantees this, it was nice to have this in a explicit way.

Consider the following minions:

# salt-key
Accepted Keys:
3db64d6583f548cd91336526b7733b90
6c1d7bdd4bb64196b3644429ed7f81e3
admin
ca
Denied Keys:
Unaccepted Keys:
Rejected Keys:

Now, we can write an orchestration:

do-something:
  salt.function:
    - tgt: 'not L@3db64d6583f548cd91336526b7733b90'
    - tgt_type: compound
    - name: grains.get
    - arg:
        - os

Steps to Reproduce Issue

Run the orchestration. It works as expected:

# salt-run state.orchestrate orch.some_test
admin_master:
----------
          ID: do-something
    Function: salt.function
        Name: grains.get
      Result: True
     Comment: Function ran successfully. Function grains.get ran on admin, ca, 6c1d7bdd4bb64196b3644429ed7f81e3.
     Started: 10:40:03.119885
    Duration: 315.809 ms
     Changes:   
              admin:
                  SUSE
              ca:
                  SUSE
              6c1d7bdd4bb64196b3644429ed7f81e3:
                  SUSE

Summary for admin_master
------------
Succeeded: 1 (changed=1)
Failed:    0
------------
Total states run:     1
Total run time: 315.809 ms

Now, if we reject this minion:

# salt-key -r 3db64d6583f548cd91336526b7733b90 --include-accepted
The following keys are going to be rejected:
Accepted Keys:
3db64d6583f548cd91336526b7733b90
Proceed? [n/Y] Y
Key for minion 3db64d6583f548cd91336526b7733b90 rejected.

Now let's try to run the same orchestration again:

# salt-run state.orchestrate orch.some_test
[ERROR   ] {u'ret': {u'admin': u'SUSE', u'ca': u'SUSE', u'6c1d7bdd4bb64196b3644429ed7f81e3': u'SUSE', u'3db64d6583f548cd91336526b7733b90': False}, u'out': u'highstate'}
admin_master:
----------
          ID: do-something
    Function: salt.function
        Name: grains.get
      Result: False
     Comment: Running function grains.get failed on minions: 3db64d6583f548cd91336526b7733b90 Function grains.get ran on admin, ca, 6c1d7bdd4bb64196b3644429ed7f81e3, 3db64d6583f548cd91336526b7733b90.
     Started: 10:43:03.357988
    Duration: 335.56 ms
     Changes:   
              admin:
                  SUSE
              ca:
                  SUSE
              6c1d7bdd4bb64196b3644429ed7f81e3:
                  SUSE
              3db64d6583f548cd91336526b7733b90:
                  False

Summary for admin_master
------------
Succeeded: 0 (changed=1)
Failed:    1
------------
Total states run:     1
Total run time: 335.560 ms

And this is where this behavior differs from 2016.11.0. Note that this is not related to salt.function, but rather to the matcher itself (keep reading :). If we are explicitly filtering out 3db64d6583f548cd91336526b7733b90, this minion should not be included in the response, and of course should not be a reason for the orchestration to fail.

In general, and as discovered later when debugging this issue if you filter out any non-existing minion id, it will be included in the response, as:

# salt -C 'not L@THIS_IS_NOT_A_MINION_ID,THIS_IS_NOT_A_MINION_ID_EITHER' grains.get os
admin:
    SUSE
6c1d7bdd4bb64196b3644429ed7f81e3:
    SUSE
ca:
    SUSE
THIS_IS_NOT_A_MINION_ID:
    Minion did not return. [No response]
THIS_IS_NOT_A_MINION_ID_EITHER:
    Minion did not return. [No response]

Versions Report

Salt Version:
           Salt: 2018.3.0
 
Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.8
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.6
   mysql-python: 1.2.5
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.13 (default, Jan 11 2017, 10:56:06) [GCC]
   python-gnupg: Not Installed
         PyYAML: 3.12
          PyZMQ: 14.0.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.4
 
System Versions:
           dist: SuSE 12 x86_64
         locale: ANSI_X3.4-1968
        machine: x86_64
        release: 4.4.132-94.33-default
         system: Linux
        version: SUSE Linux Enterprise Server  12 x86_64

Master and minions are all in the same version.

@gtmanfred gtmanfred added this to the Approved milestone Aug 20, 2018

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

This was probably added when trying to solve an issue with the list matcher.

Do you see this problem with any other matchers, like just globs? or is it only with list inside of compound matching?

@gtmanfred gtmanfred modified the milestones: Approved, Blocked Aug 20, 2018

@ereslibre

This comment has been minimized.

Copy link
Author

commented Aug 20, 2018

@gtmanfred Thanks for your fast response! I've recreated the environment, so the minion ids won't match the initial issue report, sorry for that. I rejected minion id 20bbfa446701426d9313d53373d7a0c3 this time.

  • Glob matching (fine):
# salt '*' grains.get os
admin:
    SUSE
4e49b27dabca4d37be9f81d8df94afe1:
    SUSE
ca:
    SUSE
  • PCRE matching (fine):
# salt -E '\w{32}' grains.get os
4e49b27dabca4d37be9f81d8df94afe1:
    SUSE
  • PCRE grains matching (fine):
# salt -P 'os:SUSE' grains.get os
ca:
    SUSE
4e49b27dabca4d37be9f81d8df94afe1:
    SUSE
admin:
    SUSE
  • List of minions (fine -- only if the list does not include non-existant minions):
# salt -L '4e49b27dabca4d37be9f81d8df94afe1' grains.get os
4e49b27dabca4d37be9f81d8df94afe1:
    SUSE
  • List of minions (wrong -- if the list includes non-existant minions):
# salt -L '4e49b27dabca4d37be9f81d8df94afe1,THIS_IS_NOT_A_MINION_ID' grains.get os
4e49b27dabca4d37be9f81d8df94afe1:
    SUSE
THIS_IS_NOT_A_MINION_ID:
    Minion did not return. [No response]

I fear the only way to include "new" (as in unknown to salt) minion ids is using the list matcher and the compound matcher by extension. The rest of matchers seem to only work on the subset of well-known minion ids, and thus, are not affected from what I can see.

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

so, the last one you did there, is the expected behavior, that was added in 2018.3.

If a minion is targeted in a list, and it does not exist, we print an error. That was a feature request from someone else.

The bug that I see is when using a not matcher inside of a compound, it should not be matching that minion at all, so when you did a not L@MINION_ID even if MINION_ID does not exist, it should also not be matched.

@ereslibre

This comment has been minimized.

Copy link
Author

commented Aug 20, 2018

If a minion is targeted in a list, and it does not exist, we print an error. That was a feature request from someone else.

This makes a lot of sense, yes.

The bug that I see is when using a not matcher inside of a compound, it should not be matching that minion at all, so when you did a not L@MINION_ID even if MINION_ID does not exist, it should also not be matched.

Exactly. The issue I'm referring to explicitly targets the use of compound matcher, not the raw list matcher; I just noticed I kind of inadvertibly implied that the last case was wrong, didn't mean to.

So, I completely agree that the issue is with the compound matcher using a list with excludes (not).

@gtmanfred

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2018

Cool, @saltstack/team-core who was it that added the feature that the list matcher would print a no response from a minion if it was included, but didn't actually exist?

I think there might have been a bug added when that was used with not in a compound matcher?

Thanks,
Daniel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.