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

mine.get returns different dictionary format in version 3000 #56118

Open
ryanmilne-digicert opened this issue Feb 10, 2020 · 21 comments · May be fixed by #56172
Open

mine.get returns different dictionary format in version 3000 #56118

ryanmilne-digicert opened this issue Feb 10, 2020 · 21 comments · May be fixed by #56172

Comments

@ryanmilne-digicert
Copy link

@ryanmilne-digicert ryanmilne-digicert commented Feb 10, 2020

I created couple new servers today and noticed that the output of the mine function I use for getting the ip address of the minion is returning in a different format. I can find documentation about the change in format for defining a mine function, but nothing noting the difference in output.

Am I overlooking something?

Set

mine function definition (via pillar):

mine_functions:
  internal_ip:
    - mine_function: grains.get
    - fqdn_ip4
  internal_ips:
    - mine_function: grains.get
    - ipv4

command to get mine data:

salt saltmaster mine.get '*' internal_ip

expected output:

    minion_name:
        - 10.1.2.3

received output:

    minion_name:
        ----------
        __data__:
            - 10.1.2.3
        __saltmine_acl__:
            1

When used in a jinja template I receive an error that there is not an element 0

{% for server, addr in salt['mine.get'](match, 'internal_ip', tgt_type='compound').items() | sort(case_sensitive=False) -%}
{{ host }}     IN    A   {{ addr[0] }}
{% endfor -%}
          ID: zone_file
    Function: file.managed
        Name: /var/named/chroot/etc/named/name.zone
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python2.7/site-packages/salt/state.py", line 1933, in call
                  **cdata['kwargs'])
                File "/usr/lib/python2.7/site-packages/salt/loader.py", line 1951, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python2.7/site-packages/salt/states/file.py", line 2769, in managed
                  **kwargs
                File "/usr/lib/python2.7/site-packages/salt/modules/file.py", line 4824, in check_managed_changes
                  **kwargs)
                File "/usr/lib/python2.7/site-packages/salt/modules/file.py", line 4265, in get_managed
                  **kwargs)
                File "/usr/lib/python2.7/site-packages/salt/utils/templates.py", line 169, in render_tmpl
                  output = render_str(tmplstr, context, tmplpath)
                File "/usr/lib/python2.7/site-packages/salt/utils/templates.py", line 404, in render_jinja_tmpl
                  buf=tmplstr)
              SaltRenderError: Jinja variable dict object has no element 0
     Started: 21:07:23.444192
    Duration: 1053.174 ms
     Changes:

Versions Report

Salt Version:
           Salt: 3000

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.6.2
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.5 (default, Aug  7 2019, 00:51:29)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.3.0
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.1.4

System Versions:
           dist: centos 7.7.1908 Core
         locale: UTF-8
        machine: x86_64
        release: 3.10.0-1062.12.1.el7.x86_64
         system: Linux
        version: CentOS Linux 7.7.1908 Core
@max-arnold

This comment has been minimized.

Copy link
Contributor

@max-arnold max-arnold commented Feb 11, 2020

Probably introduced in #55760

CC: @github-abcde

@OrangeDog

This comment has been minimized.

Copy link
Contributor

@OrangeDog OrangeDog commented Feb 11, 2020

I can find documentation about the change in format for defining a mine function

Where? It's not mentioned in the release notes.

@github-abcde

This comment has been minimized.

Copy link
Contributor

@github-abcde github-abcde commented Feb 11, 2020

Interesting. The underscore-variables returned are for internal use only and should not be returned. The code at https://github.com/saltstack/salt/pull/55760/files#diff-b63dd508d9b7489d1f22527bda3f300dR613 detects the __saltmine_acl__-key and determines that the mine item is in the "new" (with minion-side ACL) format, and then only returns the contents of the __data__-key (line 614). Except that this apparently does not happen in your case. I will look into this.
Edit: Correct spelling error

@github-abcde

This comment has been minimized.

Copy link
Contributor

@github-abcde github-abcde commented Feb 11, 2020

@ryanmilne-digicert Just a thought, is your salt-master running on version 3000 as well? Because if the versions are mismatched, the minion will send the new datastructure (including minion-side-acl metadata), but the master will not expect this and just return all the data the minion has provided.

@bitskri3g

This comment has been minimized.

Copy link

@bitskri3g bitskri3g commented Feb 11, 2020

@ryanmilne-digicert Just a thought, is your salt-master running on version 3000 as well? Because if the versions are mismatched, the minion will send the new datastructure (including minion-side-acl metadata), but the master will not expect this and just return all the data the minion has provided.

I ran into this as well - updating the master to 3000 allows the mine to display as expected.

@ryanmilne-digicert

This comment has been minimized.

Copy link
Author

@ryanmilne-digicert ryanmilne-digicert commented Feb 11, 2020

I just checked and my saltmaster is still on 2019.2.2. However, I am using the docker images and there does not appear to be anything newer published to docker hub.

https://hub.docker.com/r/saltstack/salt/tags

@ryanmilne-digicert

This comment has been minimized.

Copy link
Author

@ryanmilne-digicert ryanmilne-digicert commented Feb 11, 2020

I can find documentation about the change in format for defining a mine function

Where? It's not mentioned in the release notes.
I was only pointing out that there was documentation of changed around mine functions, but nothing about a change to the output.

https://docs.saltstack.com/en/latest/topics/mine/

Changed in version 3000.

The format to define mine_functions has been changed to allow the same format as used for module.run. The old format (above) will still be supported.
@max-arnold

This comment has been minimized.

Copy link
Contributor

@max-arnold max-arnold commented Feb 12, 2020

https://docs.saltstack.com/en/latest/faq.html#can-i-run-different-versions-of-salt-on-my-master-and-minion

When upgrading Salt, the master(s) should always be upgraded first. Backwards compatibility for minions running newer versions of salt than their masters is not guaranteed.

However, it looks like it is quite easy to temporarily patch an older master:

diff --git a/salt/daemons/masterapi.py b/salt/daemons/masterapi.py
index 8fc5bdc0af..61067bef56 100644
--- a/salt/daemons/masterapi.py
+++ b/salt/daemons/masterapi.py
@@ -591,7 +591,10 @@ class RemoteFuncs(object):
             if isinstance(fdata, dict):
                 fdata = fdata.get(load['fun'])
                 if fdata:
-                    ret[minion] = fdata
+                    if isinstance(fdata, dict) and '__data__' in fdata:
+                        ret[minion] = fdata['__data__']
+                    else:
+                        ret[minion] = fdata
         return ret

     def _mine(self, load, skip_verify=False):
diff --git a/salt/utils/minions.py b/salt/utils/minions.py
index a24f293701..21bea126e8 100644
--- a/salt/utils/minions.py
+++ b/salt/utils/minions.py
@@ -1157,5 +1157,8 @@ def mine_get(tgt, fun, tgt_type='glob', opts=None):
             continue
         fdata = mdata.get(fun)
         if fdata:
-            ret[minion] = fdata
+            if isinstance(fdata, dict) and '__data__' in fdata:
+                ret[minion] = fdata['__data__']
+            else:
+                ret[minion] = fdata
     return ret
@sagetherage sagetherage added the Bug label Feb 12, 2020
@sagetherage sagetherage added this to the Approved milestone Feb 12, 2020
@Ch3LL

This comment has been minimized.

Copy link
Contributor

@Ch3LL Ch3LL commented Feb 12, 2020

@max-arnold is completely right that you should always upgrade your master first, so this is definitely the recommended approach. Although we do not guarantee backwards compatibility of newer minions with older masters we want to try to make a best effort to keep that compatibility. Diving in now to see if its even possible.

@ryanmilne-digicert

This comment has been minimized.

Copy link
Author

@ryanmilne-digicert ryanmilne-digicert commented Feb 12, 2020

It makes sense to upgrade the master first, however I ran into this as I created some new servers with Salt-Cloud and did not see that a new version of salt had been release.

Another problem though is I am using the docker images on docker hub and those have not been updated for 2 months.
https://hub.docker.com/r/saltstack/salt/tags
Is there a timeframe when a new image will be published on dockerhub?

@max-arnold

This comment has been minimized.

Copy link
Contributor

@max-arnold max-arnold commented Feb 12, 2020

For Salt Cloud it is generally a good idea to pin the salt version using script_args: #54857 (comment)

@Ch3LL

This comment has been minimized.

Copy link
Contributor

@Ch3LL Ch3LL commented Feb 12, 2020

okay i believe we only really need to set the __data__ when we have allow_tgt set.

something like this:

diff --git a/salt/utils/mine.py b/salt/utils/mine.py
index fa1f4d9024..6a92194dd1 100644
--- a/salt/utils/mine.py
+++ b/salt/utils/mine.py
@@ -68,12 +68,15 @@ def wrap_acl_structure(
     :rtype: dict
     :return: Mine entry structured to include ACL data.
     '''
-    res = {
-        MINE_ITEM_ACL_DATA: function_data,
-        MINE_ITEM_ACL_ID: MINE_ITEM_ACL_VERSION,
-    }
+    res = function_data
+
     # Add minion-side ACL
     if allow_tgt:
+        res = {
+            MINE_ITEM_ACL_DATA: function_data,
+            MINE_ITEM_ACL_ID: MINE_ITEM_ACL_VERSION,
+        }
+
         res.update(salt.utils.data.filter_falsey({
             'allow_tgt': allow_tgt,
             'allow_tgt_type': allow_tgt_type,

I confirmed that works for working with new/old versions. @github-abcde see anything wrong with this approach? I'm writing some tests right now, so i might find something wrong while i'm working on this.

@Ch3LL

This comment has been minimized.

Copy link
Contributor

@Ch3LL Ch3LL commented Feb 12, 2020

@ryanmilne-digicert let me ask around about docker-hub

@github-abcde

This comment has been minimized.

Copy link
Contributor

@github-abcde github-abcde commented Feb 13, 2020

@Ch3LL Syntactically there is nothing wrong, however semantically I would prefer a different solution. With your proposed change this function's actions no longer completely match its name (since it will not always wrap the function_data). I would suggest implementing the check for allow_tgt in the places where this function is called: https://github.com/saltstack/salt/blob/master/salt/modules/mine.py#L197 and https://github.com/saltstack/salt/blob/master/salt/modules/mine.py#L255 and to leave the wrap_acl_structure-function as-is.

@Ch3LL

This comment has been minimized.

Copy link
Contributor

@Ch3LL Ch3LL commented Feb 13, 2020

thanks i'll take a look at that approach. don't see why that wouldn't work

@Ch3LL

This comment has been minimized.

Copy link
Contributor

@Ch3LL Ch3LL commented Feb 13, 2020

ping @github-abcde another follow up question. Can we not just remove adding __data__ here, regardless of it using allow_tgt ? https://github.com/saltstack/salt/pull/55760/files#diff-298e79edaa330ebd37da69a4742863acR19

I'm trying to understand the purpose behind adding this?

@github-abcde

This comment has been minimized.

Copy link
Contributor

@github-abcde github-abcde commented Feb 14, 2020

@Ch3LL The added functionality of allowing minion-side ACL had a side-effect in what is stored in the salt mine. Before, only the raw data (function_data) was stored. But now, as minions can push ACL metadata, the need arose for each mine item to store not just the function_data, but the metadata as well. Since the function_data can be anything, including a dict, some mechanism was needed to determine whether we're dealing with a "new style" mine-item, which contains the ACL metadata and the function_data, or the old one. For this, I introduced the magic https://github.com/saltstack/salt/pull/55760/files#diff-298e79edaa330ebd37da69a4742863acR17 identifier, which, when present, indicates a "new style" mine-item. Of course, the function_data obviously needed to be stored as well, but since the "new style" mine-item can now contain anything besides the metadata, I opted to store the function_data under the __data__-key, which is another magic identifier and thus must be defined somewhere instead of being hardcoded everywhere.

TL;DR: In the case that minion-side ACL is used (allow_tgt), the __data__-key is where the function_data of the mine function is stored in the mine-item, removing it will complicate things a lot. If you're not using minion-side ACL, it is sufficient to store the function_data as-is.

@Ch3LL

This comment has been minimized.

Copy link
Contributor

@Ch3LL Ch3LL commented Feb 14, 2020

thanks for the clarification :)

@Ch3LL

This comment has been minimized.

Copy link
Contributor

@Ch3LL Ch3LL commented Feb 14, 2020

@ryanmilne-digicert i was able to find out those containers are managed here: https://github.com/saltstack/saltdocker/

looks like there is an issue with the cron job on travis that should have automatically uploaded that to docker hub so i'll dive into that next week.

@ryanmilne-digicert

This comment has been minimized.

Copy link
Author

@ryanmilne-digicert ryanmilne-digicert commented Feb 14, 2020

@Ch3LL Thank you. I did find that repo and used it to create my own updated image, but I would like to switch back to official images once you have resolved the issue.

@Ch3LL

This comment has been minimized.

Copy link
Contributor

@Ch3LL Ch3LL commented Feb 14, 2020

Fix here: #56172

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants
You can’t perform that action at this time.