Repository: saltstack/salt
Pull request: handle config load error when cache is not available for first time
### What does this PR do?

When cache is not present, while copy we used cp_get_template, this return folder name and not the file path. Hence we got this error
```
[ERROR   ] {'out': False, 'message': 'Could not load configuration due to : "[Errno 21] Is a directory: \'/tmp/tmpgcohosan\'"', 'format': 'set'}
```
Hence first create tmp file in given dir, and use this file path as the location for the rendered config file.

### What issues does this PR fix or reference?
Fixes: https://github.com/saltstack/salt/issues/57667

### Previous Behavior
It used to work in older salt. With recent code changes in master/sodium, this got broken.

### New Behavior
Remove this section if not relevant

### Merge requirements satisfied?
**[NOTICE] Bug fixes or features added to Salt require tests.**
<!-- Please review the [test documentation](https://docs.saltstack.com/en/master/topics/tutorials/writing_tests.html) for details on how to implement tests into Salt's test suite. -->
- [ ] Docs
- [ ] Changelog - https://docs.saltstack.com/en/master/topics/development/changelog.html
- [ y ] Tests written/updated

### Commits signed with GPG?
Yes/No

Please review [Salt's Contributing Guide](https://docs.saltstack.com/en/master/topics/development/contributing.html) for best practices.

See GitHub's [page on GPG signing](https://help.github.com/articles/signing-commits-using-gpg/) for more information about signing commits with GPG.


Problem statement:
[BUG] [RC1 Sodium] Could not load configuration due to : "[Errno 21] Is a directory.."
**Description**
When cache is not present, while copy we used cp_get_template, this return folder name and not the file path. Hence we got this error
```
[ERROR   ] {'out': False, 'message': 'Could not load configuration due to : "[Errno 21] Is a directory: \'/tmp/tmpgcohosan\'"', 'format': 'set'}
```

**Setup**
```
root@iagent:/srv/salt# cat install_conf.sls
Install the mentioned config:
  junos.install_config:
    - name: salt://configs/test.set
```

**Steps to Reproduce the behavior**
Remove any cache file on proxy server.
it can be found using
```
salt xxx cp.is_cached salt://configs/test.set
```

**Expected behavior**
Should not throw error

**Screenshots**
If applicable, add screenshots to help explain your problem.

**Versions Report**
v3001rc1

```
PASTE HERE
```
</details>

**Additional context**
Add any other context about the problem here.

Hints from issue discussion:

Commit message: add unit test for bug fix

Patch to review:
diff --git a/salt/modules/junos.py b/salt/modules/junos.py
index 5efaf71e6f94..39f5cfda0086 100644
--- a/salt/modules/junos.py
+++ b/salt/modules/junos.py
@@ -131,8 +131,9 @@ def __enter__(self):
                 "Caching file {0} at {1}".format(self._file_path, self._cached_folder)
             )
             if self._kwargs:
-                self._cached_file = __salt__["cp.get_template"](
-                    self._file_path, self._cached_folder, **self._kwargs
+                self._cached_file = salt.utils.files.mkstemp(dir=self._cached_folder)
+                __salt__["cp.get_template"](
+                    self._file_path, self._cached_file, **self._kwargs
                 )
             else:
                 self._cached_file = __salt__["cp.get_file"](
diff --git a/tests/unit/modules/test_junos.py b/tests/unit/modules/test_junos.py
index 799e40830cc3..e90fccc2c16a 100644
--- a/tests/unit/modules/test_junos.py
+++ b/tests/unit/modules/test_junos.py
@@ -1261,6 +1261,49 @@ def test_install_config_text_file(self):
                 self.assertEqual(junos.install_config("salt://actual/path/config"), ret)
                 mock_load.assert_called_with(path="test/path/config", format="text")
 
+    def test_install_config_cache_not_exists(self):
+        with patch.dict(
+            junos.__salt__,
+            {
+                "cp.is_cached": MagicMock(return_value=None),
+                "file.rmdir": MagicMock(return_value="True"),
+            },
+        ):
+            with patch("jnpr.junos.utils.config.Config.commit") as mock_commit, patch(
+                "jnpr.junos.utils.config.Config.commit_check"
+            ) as mock_commit_check, patch(
+                "jnpr.junos.utils.config.Config.diff"
+            ) as mock_diff, patch(
+                "jnpr.junos.utils.config.Config.load"
+            ) as mock_load, patch(
+                "salt.utils.files.safe_rm"
+            ) as mock_safe_rm, patch(
+                "salt.utils.files.mkstemp"
+            ) as mock_mkstemp, patch(
+                "tempfile.mkdtemp"
+            ) as mock_mkdtemp, patch(
+                "os.path.isfile"
+            ) as mock_isfile, patch(
+                "os.path.getsize"
+            ) as mock_getsize:
+                mock_isfile.return_value = True
+                mock_getsize.return_value = 10
+                mock_mkstemp.return_value = "test/path/config"
+                mock_diff.return_value = "diff"
+                mock_commit_check.return_value = True
+                mock_mkdtemp.return_value = "/tmp/argr5351afd"
+
+                ret = dict()
+                ret["message"] = "Successfully loaded and committed!"
+                ret["out"] = True
+                self.assertEqual(
+                    junos.install_config(
+                        "salt://actual/path/config", template_vars=dict(temp_var_1=True)
+                    ),
+                    ret,
+                )
+                mock_mkstemp.assert_called_with(dir="/tmp/argr5351afd")
+
     def test_install_config_replace(self):
         with patch.dict(
             junos.__salt__,


Code review comments:
diff_hunk: @@ -131,8 +131,9 @@ def __enter__(self):
                 "Caching file {0} at {1}".format(self._file_path, self._cached_folder)
             )
             if self._kwargs:
-                self._cached_file = __salt__["cp.get_template"](
-                    self._file_path, self._cached_folder, **self._kwargs
+                self._cached_file = salt.utils.files.mkstemp(dir=self._cached_folder)
path: salt/modules/junos.py
text: @user1:
Wondering why the mkdirs=True parameter is not being used. See https://docs.saltstack.com/en/latest/ref/modules/all/salt.modules.cp.html#salt.modules.cp.get_template




@author:
As we need to mandatorily pass `dest` location to cp.get_template https://docs.saltstack.com/en/latest/ref/modules/all/salt.modules.cp.html#salt.modules.cp.get_template
The temporary dest (which gets cleaned after API work is done) is already created using https://github.com/saltstack/salt/blob/af051441aa70f8caa234e51bf423a71c8d66be8b/salt/modules/junos.py#L129 hence we don't need to pass mkdirs.

@user1:
@author My comment 'makedirs=True' is related to why that is not the proper fix for this, not 'well I am using mkdtemp'.  When there is already a solution for having directories created, it should be used, unless there is problem using 'mkdirs=True'.  Already have code using mkdtemp in the PR is not a relevant answer to a review question.

BTW this will not make it in the Sodium release as that is tagged, packaged, tested and QA and about to be released tomorrow. Salt point release 3000.1 is the earliest release that this PR could be applied to.



@author:
@user1, I am missing something it seems. 

I could just pass dir=/tmp and makedirs=True but that giving me error everytime (even if I delete the file it breaks for, it error for another file). This is irrespective of we use makedirs or not.

root@user2:/# salt proxy01 cp.get_template salt://configs/pankaj.set /tmp makedirs=True
```
proxy01:
    The minion function caused an exception: Traceback (most recent call last):
      File "/usr/local/lib/python3.6/dist-packages/salt/metaproxy/proxy.py", line 480, in thread_return
        opts, data, func, args, kwargs
      File "/usr/local/lib/python3.6/dist-packages/salt/executors/direct_call.py", line 12, in execute
        return func(*args, **kwargs)
      File "/usr/local/lib/python3.6/dist-packages/salt/modules/cp.py", line 327, in get_template
        return _client().get_template(path, dest, template, makedirs, saltenv, **kwargs)
      File "/usr/local/lib/python3.6/dist-packages/salt/fileclient.py", line 830, in get_template
        shutil.move(data["data"], dest)
      File "/usr/lib/python3.6/shutil.py", line 548, in move
        raise Error("Destination path '%s' already exists" % real_dst)
    shutil.Error: Destination path '/tmp/__salt.tmp.vgfq0pwc' already exists
ERROR: Minions returned with non-zero exit code
root@user2:/# rm /tmp/__salt.tmp.vgfq0pwc
root@user2:/# salt proxy01 cp.get_template salt://configs/pankaj.set /tmp makedirs=True
proxy01:
    The minion function caused an exception: Traceback (most recent call last):
      File "/usr/local/lib/python3.6/dist-packages/salt/metaproxy/proxy.py", line 480, in thread_return
        opts, data, func, args, kwargs
      File "/usr/local/lib/python3.6/dist-packages/salt/executors/direct_call.py", line 12, in execute
        return func(*args, **kwargs)
      File "/usr/local/lib/python3.6/dist-packages/salt/modules/cp.py", line 327, in get_template
        return _client().get_template(path, dest, template, makedirs, saltenv, **kwargs)
      File "/usr/local/lib/python3.6/dist-packages/salt/fileclient.py", line 830, in get_template
        shutil.move(data["data"], dest)
      File "/usr/lib/python3.6/shutil.py", line 548, in move
        raise Error("Destination path '%s' already exists" % real_dst)
    shutil.Error: Destination path '/tmp/__salt.tmp.ljdtvx6a' already exists
ERROR: Minions returned with non-zero exit code
```

Anyways, **I have changed the logic (with the last commit) and now don't use any dirs things for cp.get_template, and use tempfile itself. I hope this is the best we should have.
Let me know if we are good now.**

I am OK with this change going to whichever latest build it can (if not sodium)


@user1:
Added @user3 as reviewer


@user1:
@author Sorry for taking so long to get back to you on this, looks good