Salt: [BUG] state.highstate test=True saltenv=None against a Salt GitFS server causes modules from random branches to be fetched rather than from the configured saltenv

[Rudd-O]

(With latest Salt 3001)

I encountered a curious situation with Salt. The setup:

• Salt master with GitFS and hundreds of branches.
• Salt API receiving requests.
• API client requesting state.highstate test=True from Salt minions.
• Clients all configured with topfile merge strategy same and with saltenv = base.

I was encountering the weirdest thing. Every time the API client hit the endpoint, test-mode highstate started, modules were synced by the minions (this by itself is weird, but that's another matter), and after that, all the minions had the wrong modules. It took a saltutil.sync_all to correct that bad situation.

It was only when we discovered that our API client was sending the kwarg saltenv: null as part of the kwarg dictionary, that we connected the dots. Whenever the API client sends the request for test highstate with that specific kwarg, the saltenv: null parameter is relayed as-is by the Salt master bus to the minions, which in turn causes the minions to hit every single branch known to the GitFS-enabled master, fetching modules from the wrong branch, rather than from the configured base branch.

This is weird. The code for state.highstate clearly states:

    if "saltenv" in kwargs:
        opts["saltenv"] = kwargs["saltenv"]

So in this case the saltenv key/value in the opts dictionary gets set to None. This should be expanded to be a check for if "saltenv" in kwargs and kwargs["saltenv"]. Anywhere saltenv and pillarenv are accepted, that is the check that should be done.

I get the feeling that invalid values should not cause malfunctions of this sort.

[alexey-zhukovin]

@Rudd-O Thank you
Could you provide steps to duplicate this issue?
salt version report

[Rudd-O]

Oh it's simple to replicate: use the Salt API to submit a state.highstate and send saltenv=None in the kwargs of the API payload. That's all you need to do.

The version is the very latest stable Salt on Ubuntu Xenial as published in the official Salt repository.

[sagetherage]

Oh it's simple to replicate: use the Salt API to submit a state.highstate and send saltenv=None in the kwargs of the API payload. That's all you need to do.

The version is the very latest stable Salt on Ubuntu Xenial as published in the official Salt repository.

Yes, we ask for the report so that it isn't a hunt for the exact version at the time of writing the issue. On July 23, 2020 the latest version likely was 3001, can you confirm @Rudd-O?

[Rudd-O]

Confirmed.

[jlsjonas]

It appears that the same (if not a very similar) issue appears when using the cli to do a state apply without explicitly defining a saltenv; the minion was reporting lacking top files in several branches (branches that predate our salt setup); while I (also) expected it to use the base branch (default_top: base is set, along with top_file_merging_strategy: same)
noticed the issue on 3002.2