Fix __pillar__ access in modules in pillar render

[frebib]

Historically the __pillar__ dunder has never been available or worked in extension modules during pillar render, due to what I can only assume is a bug/oversight in the overly-complex mess of branches and logic in the pillar rendering code. Functionally there is very little reason this shouldn't work, and it even appears like it may have worked at some point, or at least was intended to work by the fact that the ext_pillar data was stuffed right back into opts before attempting to render the remainder of the pillar.

The missing piece to the puzzle here was that the partially-rendered ext_pillar data wasn't available in the opts dict packed into the salt.loader.minion_mods() call when it was generated, and the fact that the functions weren't reloaded after the pillar dict was updated. This commit fixes both of those things.

Ensure the minion_mods are always reloaded with the updated opts dict containing the partial pillar data when rendering pillar with ext_pillar_first enabled.

Signed-off-by: Joe Groocock jgroocock@cloudflare.com

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

• Docs
• Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
• Tests written/updated

Commits signed with GPG?

Yes

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

[frebib]

No idea why the builds are failing

>>>>>>     Failed to complete #create action: [undefined method `[]' for nil:NilClass in the specified region us-west-2. Please check this AMI is available in this region.] on py3-photon-3-x86-64

[Ch3LL]

What if there is already "pillar" in opts, wouldn't self.opts["pillar"] be writing that over?

[frebib]

Yes, probably. The whole "which opts should we use" thing here is an utter mess. It's very unclear what's going on regarding mutability and correctness of where pillar should be preserved and not. I can't really say for sure what the semantics of this should be, but if we remove this statement the thing that this PR attempts to fix will not work. The only reason this branch even exists is to not break compatibility with this ancient nonsense that makes no sense to me: 340aed8#diff-8b80a647365268f00563c1c4f3e787812a7ad3e07744c7fd71ffc21c7c75b4afR105
I could just drop this entirely, but seeing as there are no tests I have no idea if we'll break anything.

Another option here is to self.raw_opts.copy() but that then changes the semantics of the opts passed into the LazyLoader that we create on line 640.
The way I see it: the whole opts thing with respect to mutability and ownership is a joke that is beyond fixing, at least until someone states for definite how it should be handled.

I'm happy to change this to however will be the most compatible/sensible, but ultimately this is extremely fragile code with so many untested edge cases that people probably rely on, that something will inevitably break.

[Ch3LL]

Let me reach out to some other core team members and see what they think.

[Ch3LL]

@s0undt3ch or @dwoz any opinions here? ^

[frebib]

I'm lead to believe that this change breaks merging in pillar like so salt-call state.apply foo pillar='{"blah": False"}' (edit: from looking at the code, I don't see how that could happen because pillar overrides are merged in on the minion side)
I'm very tempted to just rip out the whole opts waffle here because it's so antiquated that leaving it in makes less sense than taking it out now.

[dwoz]

I would like to see pillar data removed from opts completely. I know @s0undt3ch has made some attempts to make opts immutable in the past.

[s0undt3ch]

I would like to see pillar data removed from opts completely. I know @s0undt3ch has made some attempts to make opts immutable in the past.

Yes, it's immutable, unless we copy it.
I'm 1000% with keeping opts for what they are supposed to provide, opts, just opts, and remove all runtime additions to it, in this case, pillar.

If the user provides pillar data in opts, then sure, those live in opts, but those should be merged against all pillar data, and not the other way around.

[dwoz]

@frebib I've created a patch with a different path to solve this issue. Can you please test my branch and see if it fixes the problem you are seeing? #64043

[frebib]

Apologies, I'm not sure how but I missed your comment here :(
I'll test this thoroughly tommorrow, but at least your PR passes my test with some minor tweaks (commented on the PR)

diff --git tests/pytests/unit/pillar/test_pillar.py tests/pytests/unit/pillar/test_pillar.py
index d375a37d47..75bd554a34 100644
--- tests/pytests/unit/pillar/test_pillar.py
+++ tests/pytests/unit/pillar/test_pillar.py
@@ -1,4 +1,7 @@
+from unittest.mock import patch
+
 import pytest
+
 import salt.loader
 import salt.pillar
 from salt.utils.odict import OrderedDict
@@ -122,3 +125,30 @@ def test_pillar_envs_path_substitution(env, temp_salt_minion, tmp_path):

     # The __env__ string in the path has been substituted for the actual env
     assert pillar.opts["pillar_roots"] == expected
+
+
+def test_ext_pillar_dunder_in_modules_in_pillar(temp_salt_minion):
+    """
+    Test that ext_pillar is available in __pillar__ inside execution modules
+    during pillar render when using ext_pillar_first=true
+    """
+    opts = temp_salt_minion.config.copy()
+    opts["ext_pillar_first"] = True
+
+    grains = salt.loader.grains(opts)
+    pillar = salt.pillar.Pillar(opts, grains, temp_salt_minion.id, "base")
+    # Pillar should be empty to start with
+    assert pillar.functions.pack["__pillar__"] == {}
+
+    ext_value = {"ext": "some ext value"}
+    pil_value = {"pillar": "some pillar value"}
+    combined = dict(**ext_value, **pil_value)
+    with (
+        patch.object(pillar, "ext_pillar", return_value=(ext_value, [])),
+        patch.object(pillar, "render_pillar", return_value=(pil_value, [])),
+    ):
+        compiled = pillar.compile_pillar()
+        assert compiled == combined
+
+    # Loader should pack the opts pillar dict from the ext_pillar() call
+    assert pillar.functions.pack["__pillar__"] == combined

I'm hopeful that your fix is an improvement over mine. Aside from the obvious hackery I had to go through to make my PR even work, the biggest problem is that somehow mine incurs a ~7 second slowdown reloading all of the modules. I'll measure how your change performs tomorrow and whether it works, but I'm hopeful, although with the ridiculous size of our pillar, I do worry that all of the dict.update() calls will be quite slow. We'll see

[Ch3LL]

Thanks for confirming @frebib I'll go ahead and close this PR in favor of #64043