search

saltstack / salt

Software to automate the management and configuration of any infrastructure or application at scale. Install Salt from the Salt package repositories here:
https://docs.saltproject.io/salt/install-guide/en/latest/
Apache License 2.0
14.19k stars 5.48k forks source link

Include issue with absolute and relative path result in conflicting IDs #54179

Open gigi206 opened 5 years ago

gigi206 commented 5 years ago

Description of Issue

I think include does not work as expected when include relative and absolute path

Setup

include:
  - .bug
  - .include
bug:
  test.succeed_without_changes:
    - name: bug

include_bug: test.succeed_without_changes:

Steps to Reproduce Issue

$ salt-call state.apply salt/bug
local:
    Data failed to compile:
----------
    Detected conflicting IDs, SLS IDs need to be globally unique.
    The conflicting ID is 'bug' and is found in SLS 'base:salt/bug.bug' and SLS 'base:salt/bug/bug'

Now if I replace with full path in salt/bug/init.sls:

include:
  - salt/bug/bug
  - salt/bug/include
$ salt-call state.apply salt/bug
local:
----------
          ID: bug
    Function: test.succeed_without_changes
      Result: True
     Comment: Success!
     Started: 10:20:43.032602
    Duration: 0.0 ms
     Changes:
----------
          ID: include_bug
    Function: test.succeed_without_changes
      Result: True
     Comment: Success!
     Started: 10:20:43.033596
    Duration: 0.0 ms
     Changes:

Summary for local
------------
Succeeded: 2
Failed:    0
------------

Versions Report

Salt Version:
           Salt: 2019.2.0

Dependency Versions:
           cffi: 1.11.5
       cherrypy: 17.4.1
       dateutil: 2.7.5
      docker-py: Not Installed
          gitdb: 2.0.5
      gitpython: 2.1.10
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: 0.28.2
        libnacl: 1.6.1
       M2Crypto: Not Installed
           Mako: 1.0.7
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: 2.17
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: 0.28.2
         Python: 3.5.4 (v3.5.4:3f56838, Aug  8 2017, 02:17:05) [MSC v.1900 64 bit (AMD64)]
   python-gnupg: 0.4.3
         PyYAML: 3.13
          PyZMQ: 17.1.2
           RAET: Not Installed
          smmap: 2.0.5
        timelib: 0.2.4
        Tornado: 4.5.3
            ZMQ: 4.2.5

System Versions:
           dist:
         locale: cp1252
        machine: AMD64
        release: 10
         system: Windows
        version: 10 10.0.18362 SP0 Multiprocessor Free
dmurphy18 commented 5 years ago

@gigi206 There was a bug in Salt 2019.2.0 where a leading '.' (dot) for an included path failed. It broke the nightly builds too. This has been fixed in the current 2019.2 branch and will be released in the upcoming Salt 2019.2.1 which is undergoing final QA.

Thanks for bringing it to SaltStack's attention, and if you would like to close this issue due to the above explanation, or keep it open until after Salt 2019.2.1 is released, and tested against your environment, which ever suits.

gigi206 commented 5 years ago

Please keep this issue open until the 2019.2.1 release.

dmurphy18 commented 5 years ago

Waiting to test once 2019.2.1 is released

martintamare commented 4 years ago

Any update on this ? Still not working on 2019.2.2+ds-1

thusoy commented 4 years ago

I'm also seeing this on 2019.2.3. My problem is that

# salt/state/init.sls
state:
    test.nop: []

and

# salt/state/foo.sls
include:
    - .

Doesn't work alongside a regular state: The conflicting ID is 'state' and is found in SLS 'base:state' and SLS 'base:state.' (note the trailing dot).

sagetherage commented 4 years ago

@dmurphy18 can you please review this and follow up, doesn't looking like it is fixed, removing the label.

sblaisot commented 4 years ago

Oh, I didn't see this bug before openning #56720

I can confirm this is not fixed as of 2019.2.3

sblaisot commented 4 years ago

@sagetherage can this bug be considered for sodium?

sblaisot commented 4 years ago

I can confirm I had this reproduced in 3000.1

sagetherage commented 3 years ago

Apologies! Getting back to this and looks like we need to have discussion on moving forward, here. I have pressed the Core team for that discussion and will follow up.

dmurphy18 commented 3 years ago

Confirm occurring with Salt 3003, with absolute paths, there is no problem with ID resolutions but with the relative paths there is, even though individually everything is fine.

Thinking there is something problematic in the relative path creation process to the file that does not execute with an absolute path.

salt-call --local state.show_sls salt/bug

is enough to demonstrate the issue

dmurphy18 commented 3 years ago

Problem appears to be due to re-rendering salt/bug/bug.sls in relative case which doesn't occur in the absolute case, as shown

538 [DEBUG   ] Rendered data from file: /var/cache/salt/minion/files/base/salt/bug/include.sls:
539 include:
540   - salt/bug/bug
541 
542 include_bug:
543   test.succeed_without_changes:
544     - name: include_bug
545     - require:
546       - id: bug
547 
548 [DEBUG   ] Results of YAML rendering:
549 OrderedDict([('include', ['salt/bug/bug']), ('include_bug', OrderedDict([('test.succeed_without_changes', [OrderedDict([('name', 'include_bug')]), OrderedDict([('require', [OrderedDict([('id', 'bug')])])])])]))])
550 [PROFILE ] Time (in seconds) to render '/var/cache/salt/minion/files/base/salt/bug/include.sls' using 'yaml' renderer: 0.0002703666687011719
551 [DEBUG   ] Using pkg_resources to load entry points
552 [DEBUG   ] LazyLoaded nested.output

but in relative, re-render and detect conflicting IDs

538 [DEBUG   ] Rendered data from file: /var/cache/salt/minion/files/base/salt/bug/include.sls:
539 include:
540   - salt/bug/bug
541 
542 include_bug:
543   test.succeed_without_changes:
544     - name: include_bug
545     - require:
546       - id: bug
547 
548 [DEBUG   ] Results of YAML rendering:
549 OrderedDict([('include', ['salt/bug/bug']), ('include_bug', OrderedDict([('test.succeed_without_changes', [OrderedDict([('name', 'include_bug')]), OrderedDict([('require', [OrderedDict([('id', 'bug')])])])])]))])
550 [PROFILE ] Time (in seconds) to render '/var/cache/salt/minion/files/base/salt/bug/include.sls' using 'yaml' renderer: 0.00026154518127441406
551 [DEBUG   ] In saltenv 'base', looking at rel_path 'salt/bug/bug.sls' to resolve 'salt://salt/bug/bug.sls'
552 [DEBUG   ] In saltenv 'base', ** considering ** path '/var/cache/salt/minion/files/base/salt/bug/bug.sls' to resolve 'salt://salt/bug/bug.sls'
553 [DEBUG   ] compile template: /var/cache/salt/minion/files/base/salt/bug/bug.sls
554 [DEBUG   ] Jinja search path: ['/var/cache/salt/minion/files/base']
555 [PROFILE ] Time (in seconds) to render '/var/cache/salt/minion/files/base/salt/bug/bug.sls' using 'jinja' renderer: 0.0005478858947753906
556 [DEBUG   ] Rendered data from file: /var/cache/salt/minion/files/base/salt/bug/bug.sls:
557 bug:
558   test.succeed_without_changes:
559     - name: bug
560 
561 [DEBUG   ] Results of YAML rendering:
562 OrderedDict([('bug', OrderedDict([('test.succeed_without_changes', [OrderedDict([('name', 'bug')])])]))])
563 [PROFILE ] Time (in seconds) to render '/var/cache/salt/minion/files/base/salt/bug/bug.sls' using 'yaml' renderer: 0.00020074844360351562
564 [DEBUG   ] Using pkg_resources to load entry points
565 [DEBUG   ] LazyLoaded nested.output
566 [TRACE   ] data = {'local': ["Detected conflicting IDs, SLS IDs need to be globally unique.\n    The conflicting ID is 'bug' and is found in SLS 'base:salt/bug.bug' and SLS 'base:salt/bug/bug'"]}
567 local:
568     - Detected conflicting IDs, SLS IDs need to be globally unique.
569           The conflicting ID is 'bug' and is found in SLS 'base:salt/bug.bug' and SLS 'base:salt/bug/bug'

If we replace the relative path for .bug in init.sls with an absolute path, it all works as in absolute paths case.

root@Unknown:/srv/salt/salt/bug# cat init.sls 
include:
##  - .bug
  - salt/bug/bug
  - .include
##  - salt/bug/bug
##  - salt/bug/include
root@Unknown:/srv/salt/salt/bug#

If we swap so include is absolute path and .bug is relative, the conflicting IDs problem occurs since it is re-rendering since the 'require' is for a relative path and it is then detecting the conflicting ID's. Where the require is for an absolute path the re-render isn't occurring probably to being able to obtain the cached information (will check).

dmurphy18 commented 2 years ago

Back working this issue (sorry for delay) and the problem still exists on master branch as of 2022-02-13.

dmurphy18 commented 2 years ago

looks like the issue is due to the re-rendering of the relative path and the then ordering , that the re-rendered .bug results in a different order number, see debugger screen shot

Screenshot from 2022-02-15 18-32-25

where highstate[id_] has {'order': 10000} and stateid] has {'order': 10001} for the same id_ of 'bug'

So state.py method _handleiorder and its assignment of order numbers is based on the state given to it, and doesn't know highstate already having that id. This implies that method merge_included_statues needs improvement

dmurphy18 commented 2 years ago

Note: apart form order number, dictionary key for dunder sls diff too

highstate[id_]['__sls__'] is 'salt/bug.bug'  the relative '.'

state[id_]['__sls__'] is 'salt/bug/bug' is the absolute '/'

playing around in the debugger to make state same as highstate for dunder sls and this passed regardless of order number, and passed through to update highstate, which means an issue, since don't want to update highstate with duplicate essentially if fix the relative path.

dmurphy18 commented 2 years ago

Solution appears to be to ensure all relative paths are converted to absolute paths when processed initially. This way everything is an absolute path when processing and no longer have to allow for '.' or '/', analogous to converting all local time to UTC, then everything is at the same level.

baby-gnu commented 2 years ago

Using / as a separator is not documented :-/

dmurphy18 commented 2 years ago

@baby-gnu the '/' is the separator for the file paths, take a look at its use with absolute paths, and this is internally when rendering the state

dmurphy18 commented 2 years ago

@myii with formula testing of master branch found issue where the fix only addresses relative paths/IDs beginning with '.' but runs into issues comparing paths/IDs which don't being with '.', see https://github.com/myii/test-formula/blob/feat/add-mre-for-includes/TEST/test_includes/notification.sls

For example: 

  {#- Use Jinja to construct the relevant include using dots or slashes,
      where `TEST.test_includes.include_notation` is either `.` or `/`.
      Dot notation is used throughout the SaltStack Formulas organisation, i.e.:
      - TEST.test_includes.package
      Slash notation is mentioned in the bug report (https://github.com/saltstack/salt/pull/61659), i.e.
      - TEST/test_includes/package
  #}

And from a failing formula test

       [PROFILE ] Time (in seconds) to render '/tmp/kitchen/var/cache/salt/minion/files/base/apache/config/modules/server_status.sls' using 'yaml' renderer: 0.01028585433959961
       [DEBUG   ] Using importlib_metadata to load entry points
       [DEBUG   ] LazyLoaded highstate.output
       local:
           Data failed to compile:
       ----------
           Detected conflicting IDs, SLS IDs need to be globally unique.
           The conflicting ID is 'apache-package-install-deps-pkg-installed' and is found in SLS 'base:apache/package/install' and SLS 'base:apache.package.install'
       ----------
           Detected conflicting IDs, SLS IDs need to be globally unique.
           The conflicting ID is 'apache-package-install-pkg-installed' and is found in SLS 'base:apache/package/install' and SLS 'base:apache.package.install'
       ----------
dmurphy18 commented 2 years ago

@gigi206 Have to go back to the drawing board, as my fix was incomplete, removed the fix I made since it was breaking others. But at least the next fix will have better tests

baby-gnu commented 2 years ago

@baby-gnu the '/' is the separator for the file paths, take a look at its use with absolute paths, and this is internally when rendering the state

I was disappointed by the use of / in the state.apply example $ salt-call state.apply salt/bug and in the include examples. To me (and the documentation), only $ salt-call state.apply salt.bug was working :sweat_smile:

dmurphy18 commented 2 years ago

@baby-gnu Well fix pulled since I broke things which my tests didn't catch, back to the drawing board :) , and things that I thought were only internal, had external consequences. At least it was not in released code.

dmurphy18 commented 2 years ago

Moved to Sulphur Release since this is going to require an exhaustive amount of testing, and runway getting to small.