Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

marmarek
Copy link
Contributor

@marmarek marmarek commented Sep 7, 2022 •
edited
Loading

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.

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.

@marmarek marmarek requested a review from a team as a code owner September 7, 2022 02:50
@marmarek marmarek requested review from garethgreenaway and removed request for a team September 7, 2022 02:50
@garethgreenaway
Copy link
Contributor

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

@garethgreenaway 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
@github-actions
Copy link

github-actions bot commented Oct 3, 2022

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!

1 similar comment
@github-actions
Copy link

github-actions bot commented Oct 3, 2022

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
Copy link
Contributor Author

marmarek commented Oct 3, 2022 •
edited
Loading

@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 marmarek marked this pull request as draft October 3, 2022 15:37
@marmarek
Copy link
Contributor Author

marmarek commented Oct 3, 2022

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

@garethgreenaway
Copy link
Contributor

@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
Copy link
Contributor Author

@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...

@marmarek marmarek force-pushed the fileclient-cachedir branch 2 times, most recently from 8c4d057 to 7aedeb3 Compare November 15, 2022 03:55
@marmarek marmarek marked this pull request as ready for review November 15, 2022 03:58
@marmarek
Prevent reuse of file client if cachedir does not match
e10f359 
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
@marmarek
Make test_state.py use separate container
1247f3a 
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
Copy link
Contributor Author

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

@Ch3LL
Copy link
Contributor

Ch3LL commented Dec 2, 2022

@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
Copy link
Contributor

Ch3LL commented Dec 13, 2022

Closing in favor of #63184

@Ch3LL Ch3LL closed this Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garethgreenaway garethgreenaway Awaiting requested review from garethgreenaway garethgreenaway was automatically assigned from saltstack/team-core

Assignees
No one assigned
Labels
needs-changelog-file Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] salt-ssh support for extra_filerefs in Saltfile is broken
3 participants
@marmarek @garethgreenaway @Ch3LL