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.

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

PoC / WiP for #48067 and related #51636

Closed
wants to merge 6 commits into from
Closed

PoC / WiP for #48067 and related #51636

wants to merge 6 commits into from

Conversation

The-Loeki
Copy link
Contributor

@The-Loeki The-Loeki commented Feb 13, 2019

What does this PR do?

Salt-SSH 'wraps' around the cp module to enable transport over SSH. This wrap is however incomplete and is done by manually rewriting parts of the original function in the wrapper.
As a number of functions are missing, the direct invocation of salt-ssh will b0rk on them, subtly or less so for range of modules & functions.
See discussion in #48067.

This is a working PoC to take a slightly different tack on that problem, by injecting a modified fileclient.
The end result is that the function calls to the cp module can be left untouched & be called directly, increasing stability, reducing code dupliation and erroneous and unexpected behaviour from salt-ssh when calling modules directly.

I would really like some input on this solution before proceeding, as I'm not sure I fully understand the consequences of what I'm doing here code/namespace/injection/cleanliness wise.

What issues does this PR fix or reference?

#48067
#50525
#50196
#50351

Previous Behavior

Salt-SSH Wrapping functions in the cp module is cumbersome and difficult

New Behavior

Salt-SSH Wrapping functions in the cp module is a matter of pointing at the actual cp module and very maybe add/overload a function.

Tests written?

No

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

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

@waynew
Copy link
Contributor

waynew commented Feb 14, 2019

You haven't made any changes to the cmd.script part, right? At least when I try that (even after doing a cp.cache_file) it fails with a cache error.

I think what we'd want is something similar to what you've done (though maybe someone else in @saltstack/team-core who's more familiar with salt-ssh can weigh in on this), since this appears to actually be asking the master for the file. What I think we'd want is to have the file client injected into the fileclients on the client. If that's not too many clients :trollface:

If you need me to hunt it down, I can find the place I think it's in the cmd module, where it's checking fileclients - that's where we'd want this one, I believe.

@The-Loeki
Copy link
Contributor Author

The-Loeki commented Feb 14, 2019

Well I figured out the wrapper is actually only executed master-side upon direct invocation.
As you say, for that to work the fileclient/wrapper needs to be active during local execution. That and the minion would be pulling rather than the master pushing.

@waynew
Copy link
Contributor

waynew commented Feb 14, 2019

Ahh. That makes some sense, regarding what we've seen. It sounds like we just need to see if it's possible for the salt-ssh master to receive file requests, rather than the existing method of bundling up the file when we start the call.

@mchugh19
Copy link
Contributor

mchugh19 commented Apr 13, 2019

Some sort of wrapper or utility function like this would still be very handy for the modules which do include cp functions if they want to implement salt/client/ssh/wrapper/ capabilities.

I was looking at getting saltcheck to support salt-ssh, and it looks like it will need to override the cp.cache_dir calls with a salt/client/ssh/wrapper/saltcheck.py wrapper which copies over the needed files to the ssh-minion's cache directory, then calls the expected functions how that the files are on the minion.

Relatedly, when testing out this PR, I noticed that cp.cache_dir only copies to the ssh user's home directory, and not the salt minion's cache dir:

# salt-ssh -l debug minion-ssh cp.cache_dir salt://cron
...
[DEBUG   ] Terminal Command: /bin/sh -c scp -o KbdInteractiveAuthentication=no -o PasswordAuthentication=no -o GSSAPIAuthentication=no -o ConnectTimeout=65 -o Port=22 -o IdentityFile=/etc/salt/pki/master/ssh/salt-ssh.rsa -o User=vagrant  salt-ssh/minion-ssh/files/base/cron/service.sls 192.168.10.3:
...

So rather than populating /var/tmp/.vagrant_526c8b_salt/running_data/var/cache/salt/minion/files/cron/service.sls it's in /home/vagrant/service.sls

Also, any ssh-cp helper functions would need to support salt-ssh's sudo settings. If sudo=False, scp cache files to __opts__['cachedir'] if sudo=True then scp to home, then connect again and move to __opts__['cachedir']. After further testing, I think this might be fine!

@dwoz
Copy link
Contributor

dwoz commented Dec 9, 2019

@The-Loeki This needs to be re-opened against the master branch and link back to this issue for the discussion. We are no longer accepting PRs to 2018.3. Sorry for the confusion.

@dwoz dwoz closed this Dec 9, 2019
@The-Loeki
Copy link
Contributor Author

@The-Loeki This needs to be re-opened against the master branch and link back to this issue for the discussion. We are no longer accepting PRs to 2018.3. Sorry for the confusion.

Dude this was a test-case in last February while you guys were really busy upgrading your pipelines.
I've repeatedly asked for comments & suggestions in here, and I'll be happy to rebase/update if need be, because I guarantee this problem still exists and the discussion for the core solution needed too.

So please don't 'just' close this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants