search

coderanger / kitchen-sync

Test Kitchen transport plugin to speed up tests.
Apache License 2.0
81 stars 24 forks source link

Do not fail when ssh agent is not available #21

Closed ytsarev closed 8 years ago

coderanger commented 8 years ago

Erm no? If the agent is running you aren't going to be able to log in to the target.

ytsarev commented 8 years ago

Your assumption is wrong. If the agent is not available then we can use preconfigured keypair from main session. In case of kitchen-docker it works flawlessly. I have a working setup of kitchen-docker + rsync transport + this patch. Please consider reopen the PR as you have a hard dependency on agent where it is not really necessary.

coderanger commented 8 years ago

Only kitchen-docker supports that though, and I'm reticent to support too much deep intermixing of features. More likely what we should do is add similar code to what kitchen-docker does but generate our own key locally if an agent isn't running. As it is, this patch is too much of a footgun for everything except the use case you describe.

ytsarev commented 8 years ago

I do not really understand. With my patch the worst case scenario is that it will gracefully degrade to SCP (and it will do it verbosely enough). If the ssh agent is available it will use it original way. Conversely - with current state of the code (e.g. without my patch) - it will abnormally fail with exception when ssh agent is not started at all. So my change brings only benefits and no deviation from original behavior.

coderanger commented 8 years ago

The fallback with SCP will make things go back to being slow, which is generally not desired if you configured the rsync provider in the first place.

coderanger commented 8 years ago

Also if you want something that works autonomously already, use the SFTP provider. It's slightly slower than rsync, but not by much.

ytsarev commented 8 years ago

I actually switched from sftp to rsync because of preserving original file permissions got crucial in my scenario ( custom puppet facts ). Sorry for insisting, but let's compare scenarios . With and without my patch if you have ssh agent enabled it will use ssh agent - so full status quo here. Now when ssh agent is not available we have to distinct scenarios: My case - usage of keys from kitchen-docker session ( or any other driver that provides key injection functionality ) or fallback to scp in the worst case. Your case - just failure with uncaught exception. What is better?

coderanger commented 8 years ago

Yes, failure so the user knows they incorrectly configured the plugin and have to go fix things. Silent fallback to slowness is not a feature, it would be a bug.

ytsarev commented 8 years ago

Ok. But the fallback is not related to my code, rather to https://github.com/coderanger/kitchen-sync/blob/master/lib/kitchen/transport/rsync.rb#L65 . Feel free to remove it. But it does not justify not merging this PR

coderanger commented 8 years ago

Yes, that is intentional to catch things that aren't configuration errors. I've already said the correct way to fix this if you want the plugin to auto-negotiate an SSH key without using ssh-agent. I don't forsee having time to add that but you are welcome to.

ytsarev commented 8 years ago

Please enlighten me why inheriting ssh configuration from driver is not a proper way ? Why do you need to generate additional keypair in transport ? FYI the change also works with kitchen-openstack driver properly and is not limited to kitchen-docker case

coderanger commented 8 years ago

The underlying issue is that we have no way to convert the configured Kitchen::SSH object so that rsync can use it. The goal of that transport is to make sure that you have a working password-less SSH link and that is hard.

ytsarev commented 8 years ago

Right. So to compensate this we are reusing existing information about ssh keys in https://github.com/coderanger/kitchen-sync/blob/master/lib/kitchen/transport/rsync.rb#L100 Btw with hardwired ssh agent in your case this line of code is effectively dead

coderanger commented 8 years ago

Seriously it would take less time to fix this properly than I've already spent arguing with you. Look at the key generation code in kitchen-docker and follow that example.

ytsarev commented 8 years ago

What you suggest to me is just redundant keypair generation on transport level and does not belong to my use case. I understand why one would need it for some other drivers, but it's irrelevant to my change. Why you do not accept the change is beyond my understanding, so I will probably don't bother anymore and continue to use my fork , good luck.