tpldir calculation is broken in some cases

[mlasevich]

Description of Issue

Buggy code is here:

salt/salt/utils/templates.py

Lines 127 to 131 in 84c6070

	template = tmplpath.replace('\\', '/')
	i = template.rfind(slspath.replace('.', '/'))
	if i != -1:
	template = template[i:]
	tpldir = os.path.dirname(template).replace('\\', '/')

Short version, if name of the directory containing the sls is contained in sls name, tpldir calculation will return wrong result.

Slightly longer version, it seems to be trying to find the tplpath by looking for last occurrence of slspath in template name - without realizing it may occur later in the full template path.

This can be fixed by simply adding the basename of the template to the slspath - although I am not really sure what is the point of this code - seems like a complicated way to get what you already have in slspath....

Setup

setup as file x/x.sls

test:
  test.nop:
    - name: {{ tpldir }}

Steps to Reproduce Issue

• if you run the state above, you will get output of name to be . - which is incorrect
• if you rename that state file to anything without "x" in the name, it will return name 'x' (correct)
• if you name it anything with x in the name, you will have a problem
• if you move it to x/x/x.sls - you will get "x" - which is, again, incorrect
• if you move it to x/x/y.sls - you will get "x/x" - which is correct

Versions Report

bug is still present in the master branch as evident by code link above

[mlasevich]

Any news on this? Still hitting this bug in random places - driving people crazy where things unexpectedly fail

[sagetherage]

@mlasevich there is a long discussion on this and I can put this into the Magnesium release cycle as a possibility of fixing, but I cannot commit unless there is some help from the community to submit a PR perhaps @terminalmage?

[mlasevich]

What is the concern? I can take a stab, but I am a bit unclear about the larger context of this function and worried that its intertwined with other things I am not seeing... Thus would prefer to have someone who is more familiar with the code look at it.

[sagetherage]

@mlasevich you are of course welcome to take a stab at it! I was looking through the comments on the closed issue you linked and there was talk of many things, so it seems to have gone in many directions, and yes I also want to be sure you have someone available to look through the code with you, that is my entire intent. I hope that is suitable.

[terminalmage]

I tried to tackle this (or something similar) but ended up breaking behavior that others relied upon. I'll try to dig up the PR and issue on Tuesday. Want to make sure we don't make the same mistakes this time around. 😄

[sagetherage]

Thank you @terminalmage and if @mlasevich you two want to work together I am happy to help set up time and use our Community Slack or Zoom to help facilitate or @cassandrafaris can help as well :)

[sagetherage]

I tried to tackle this (or something similar) but ended up breaking behavior that others relied upon. I'll try to dig up the PR and issue on Tuesday. Want to make sure we don't make the same mistakes this time around. 😄

This is the original PR, right? #55434 and this is commit that it reverts 7c28995

[terminalmage]

@sagetherage No, that commit is actually from my original PR (#55434) which was reverted by @dwoz in #56341 per the conversation in #56119 (comment).

Note that the act of reverting added some documentation. If any changes are made, we want to make sure that we are not changing behavior in such a way that it will create further confusion (or conflict with the docs).

[sagetherage]

oh gosh! look at that, I am not correct! Thank you for setting me straight. Yes, I agree.

[mlasevich]

Ok, I actually just started poking at it (without looking at the above first) - I think that entire block of logic is way more complicated than it needs to be.

From what I see the core issue is how to determine the "root" of the sls template in tmplpath - and there are all sorts of tricks used to do that - which are source of the bugs - when the reality is - there are only two possibilities here - either it is whatever is in sls or whatever is in sls plus "init" - am I missing something? Are there any scenarios where sls and tmplpath are completely different?

[mlasevich]

My first stab at it, I need to run out for a bit - may not be ready for a PR yet, but wanted any opinions:

made the PR instead