Prevent reuse of file client if cachedir does not match #62631

marmarek · 2022-09-07T02:50:14Z

What does this PR do?

Complementary change to #62043, fixing final corner case in salt-ssh (using fileclient with mismatching cachedir)

What issues does this PR fix or reference?

Fixes: #60003

Previous Behavior

salt-ssh was looking for jinja includes in cache in /var/tmp/... on the master node.

New Behavior

salt-ssh is looking for jinja includes in cache in /var/cache/salt (or wherever salt is configured for salt-ssh).

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.

garethgreenaway · 2022-09-21T20:04:34Z

@marmarek Thanks for the PR! This change will require a test case and a changelog detailing the changes. Thanks!

github-actions · 2022-10-03T14:48:03Z

Hi! I'm your friendly PR bot!

You might be wondering what I'm doing commenting here on your PR.

Yes, as a matter of fact, I am...

I'm just here to help us improve the documentation. I can't respond to
questions or anything, but what I can do, I do well!

Okay... so what do you do?

I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
When I was created we had a lot of these. The documentation for these
modules need some love and attention to make Salt better for our users.

So what does that have to do with my PR?

I noticed that in this PR there are some files changed that have some of these
issues. So I'm leaving this comment to let you know your options.

Okay, what are they?

Well, my favorite, is that since you were making changes here I'm hoping that
you would be the most familiar with this module and be able to add some other
examples or fix any of the reported issues.

If I can, then what?

Well, you can either add them to this PR or add them to another PR. Either way is fine!

Well... what if I can't, or don't want to?

That's also fine! We appreciate all contributions to the Salt Project. If you
can't add those other examples, either because you're too busy, or unfamiliar,
or you just aren't interested, we still appreciate the contributions that
you've made already.

Whatever approach you decide to take, just drop a comment here letting us know!

Detected Issues (click me)

Check Known Missing Docstrings...........................................Failed
- hook id: invoke
- exit code: 1
/home/runner/.cache/pre-commit/repoxramzil0/py_env-python3/lib/python3.9/site-packages/_distutils_hack/init.py:33: UserWarning: Setuptools is replacing distutils.

warnings.warn("Setuptools is replacing distutils.")

The function 'jinja_raise' on 'salt/utils/jinja.py' does not have a docstring

The function 'regex_escape' on 'salt/utils/jinja.py' does not have a docstring

The function 'method_call' on 'salt/utils/jinja.py' does not have a docstring

The function 'show_full_context' on 'salt/utils/jinja.py' does not have a docstring

Found 4 errors

Thanks again!

github-actions · 2022-10-03T14:48:06Z

Hi! I'm your friendly PR bot!

You might be wondering what I'm doing commenting here on your PR.

Yes, as a matter of fact, I am...

I'm just here to help us improve the documentation. I can't respond to
questions or anything, but what I can do, I do well!

Okay... so what do you do?

I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
When I was created we had a lot of these. The documentation for these
modules need some love and attention to make Salt better for our users.

So what does that have to do with my PR?

I noticed that in this PR there are some files changed that have some of these
issues. So I'm leaving this comment to let you know your options.

Okay, what are they?

Well, my favorite, is that since you were making changes here I'm hoping that
you would be the most familiar with this module and be able to add some other
examples or fix any of the reported issues.

If I can, then what?

Well, you can either add them to this PR or add them to another PR. Either way is fine!

Well... what if I can't, or don't want to?

That's also fine! We appreciate all contributions to the Salt Project. If you
can't add those other examples, either because you're too busy, or unfamiliar,
or you just aren't interested, we still appreciate the contributions that
you've made already.

Whatever approach you decide to take, just drop a comment here letting us know!

Detected Issues (click me)

Check Known Missing Docstrings...........................................Failed
- hook id: invoke
- exit code: 1
/home/runner/.cache/pre-commit/repoxramzil0/py_env-python3/lib/python3.9/site-packages/_distutils_hack/init.py:33: UserWarning: Setuptools is replacing distutils.

warnings.warn("Setuptools is replacing distutils.")

The function 'jinja_raise' on 'salt/utils/jinja.py' does not have a docstring

The function 'regex_escape' on 'salt/utils/jinja.py' does not have a docstring

The function 'method_call' on 'salt/utils/jinja.py' does not have a docstring

The function 'show_full_context' on 'salt/utils/jinja.py' does not have a docstring

Found 4 errors

Thanks again!

marmarek · 2022-10-03T15:19:08Z

@marmarek Thanks for the PR! This change will require a test case and a changelog detailing the changes. Thanks!

Changelog pushed, but for the test I'll need some help. There is a test that (I think) already test this thing here: https://github.com/saltstack/salt/blob/master/tests/pytests/integration/ssh/test_state.py#L40-L68. But currently it passes because it ssh to localhost, so using paths from remote system locally works because they are the same system... I think using a fixture from test_ssh_setup.py (which seems to create a separate container) would be enough, but I'm not sure what's the best way to do it. I'll try some naive approach, but any hints are appreciated.

marmarek · 2022-10-03T18:00:15Z

Which job is supposed to run tests marked as "slow_test"?

garethgreenaway · 2022-10-17T19:58:27Z

@marmarek I'm looking at the changes for this PR and I only see the change to the test file? Were there additional changes?

marmarek · 2022-10-17T22:30:56Z

@marmarek I'm looking at the changes for this PR and I only see the change to the test file? Were there additional changes?

The "[DO NOT MERGE]" commit reverts the fix, to see if the test is effective. But it seems all "slow tests" are skipped in CI (or I can't find where it runs)". Any idea? The test makes sense only if it's going to be called...

salt-ssh constructs file client with cachedir set to /var/tmp/... already (as it would be on the minion side). Make sure it isn't used when looking for files on the master (with cachedir /var/cache/salt or similar), as it wouldn't find them. related to saltstack#60003

This makes it detect cases where remote/local paths gets confused (like in saltstack#60003 or saltstack#62043). Without using a container, remote paths works locally too, so tests do not detect an issue. Rename test_ssh_setup.py to test_ssh_setup_and_state.py to match its new content. pytest.mark.skip_on_windows is dropped as test_ssh_setup.py already has necessary mark (dockerd present).

marmarek · 2022-11-18T13:07:47Z

I think the centos-7 failure is unrelated to the changes in this PR.

Ch3LL · 2022-12-02T15:25:10Z

@marmarek My fix here #63184 is still a work in progress, but can you give it a try?

I'm worried with this fix it will lead to slow down to re-generate the fileclient.

This should resolve the state.highstate call. I'm going to review other state calls to ensure we are also explicitly passing in context.

Ch3LL · 2022-12-13T15:24:36Z

Closing in favor of #63184

marmarek requested a review from a team as a code owner September 7, 2022 02:50

marmarek requested review from garethgreenaway and removed request for a team September 7, 2022 02:50

garethgreenaway added Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases needs-changelog-file labels Sep 21, 2022

marmarek force-pushed the fileclient-cachedir branch from 4b9f426 to 57be89d Compare October 3, 2022 14:46

marmarek marked this pull request as draft October 3, 2022 15:37

marmarek force-pushed the fileclient-cachedir branch from 1e4664d to a1d7a26 Compare October 3, 2022 16:07

marmarek force-pushed the fileclient-cachedir branch from a1d7a26 to 1d41fe9 Compare October 13, 2022 10:41

marmarek force-pushed the fileclient-cachedir branch 2 times, most recently from 8c4d057 to 7aedeb3 Compare November 15, 2022 03:55

marmarek marked this pull request as ready for review November 15, 2022 03:58

marmarek added 2 commits November 17, 2022 15:17

marmarek force-pushed the fileclient-cachedir branch from 7aedeb3 to 1247f3a Compare November 17, 2022 14:17

Ch3LL closed this Dec 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent reuse of file client if cachedir does not match #62631

Prevent reuse of file client if cachedir does not match #62631

marmarek commented Sep 7, 2022 •

edited

Loading

garethgreenaway commented Sep 21, 2022

github-actions bot commented Oct 3, 2022

github-actions bot commented Oct 3, 2022

marmarek commented Oct 3, 2022 •

edited

Loading

marmarek commented Oct 3, 2022

garethgreenaway commented Oct 17, 2022

marmarek commented Oct 17, 2022

marmarek commented Nov 18, 2022

Ch3LL commented Dec 2, 2022

Ch3LL commented Dec 13, 2022

Prevent reuse of file client if cachedir does not match #62631

Prevent reuse of file client if cachedir does not match #62631

Conversation

marmarek commented Sep 7, 2022 • edited Loading

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

New Behavior

Merge requirements satisfied?

Commits signed with GPG?

garethgreenaway commented Sep 21, 2022

github-actions bot commented Oct 3, 2022

Hi! I'm your friendly PR bot!

github-actions bot commented Oct 3, 2022

Hi! I'm your friendly PR bot!

marmarek commented Oct 3, 2022 • edited Loading

marmarek commented Oct 3, 2022

garethgreenaway commented Oct 17, 2022

marmarek commented Oct 17, 2022

marmarek commented Nov 18, 2022

Ch3LL commented Dec 2, 2022

Ch3LL commented Dec 13, 2022

marmarek commented Sep 7, 2022 •

edited

Loading

marmarek commented Oct 3, 2022 •

edited

Loading