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

Flocker deploy ssh config 71 #194

Merged
merged 32 commits into from
Jul 7, 2014
Merged

Flocker deploy ssh config 71 #194

merged 32 commits into from
Jul 7, 2014

Conversation

exarkun
Copy link
Contributor

@exarkun exarkun commented Jul 2, 2014

Fixes #71

exarkun added 24 commits June 26, 2014 14:57
@exarkun
Start of an API for getting SSH keys installed in the right place.
20a677c
@exarkun
Some more of the test suite.
e9835db
@exarkun
Merge remote-tracking branch 'origin/master' into flocker-deploy-ssh-…
868c653 
…config-71
@exarkun
Merge remote-tracking branch 'origin/master' into flocker-deploy-ssh-…
23f2ab9 
…config-71
@exarkun
Merge branch 'reusable-conch-test-server' into flocker-deploy-ssh-con…
ad953ad 
…fig-71
@exarkun
Generate the key.
c7e0388
@exarkun
Start trying to get keys into place on the node.
b0f2a85
@exarkun
Merge branch 'reusable-conch-test-server' into flocker-deploy-ssh-con…
63b572b 
…fig-71
@exarkun
mess up the tests a bunch
b78d864 
this is what happens when you don't make APIs asynchronous, folks.

also run a bunch of child processes, wee.
@exarkun
Merge branch 'reusable-conch-test-server' into flocker-deploy-ssh-con…
63bfe74 
…fig-71
@exarkun
Merge remote-tracking branch 'origin/master' into flocker-deploy-ssh-…
2fa9fc9 
…config-72
@exarkun
configure_ssh assumes you can already authenticate against the no…
d6ada7c 
…des.
@exarkun
Create the directories which hold the files we want to write, if nece…
bb84b88 
…ssary.
@exarkun
Clean up the whitespace in the key material.
b933278
@exarkun
These will succeed just as the rest will succeed. There is an SSH ser…
2549ea7 
…ver running, after all.
@exarkun
Properly run the blocking configure_ssh function in a separate th…
bdc6148 
…read in one more place that was missed previously. Also suppress output from ``ssh-add`` and do better key comparison in one more place.
@exarkun
Do not write duplicate entries to authorized_keys.
500cd6c
@exarkun
Put these in a more sensible location.
ad206b6
@exarkun
Adjust imports to reflect move.
3032e2d
@exarkun
expose the interface to the public
4b3e42f
@exarkun
minor doc cleanup
34284f9
@exarkun
Fix the mistaken double privacy. Add tests for defaults and the globa…
c3b6d8e 
…l configure_ssh.
@exarkun
Move these things to the *actually* correct package.
78df5b4
@exarkun
fix the name of the module being tested. explain why we do stuff with…
9548611 
… agents.
adamtheturtle
adamtheturtle reviewed Jul 2, 2014
View reviewed changes
flocker/cli/_sshconfig.py
each other.

This code runs in the Flocker client - that is, in an administrator's
environment, such as a laptop or desktop, not on Flocker nodes themselves.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean that it cannot be run on the nodes? Or that there is no point running them on the nodes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no expectation that folks will want to use the tool that way. I guess we might not even try very hard to specifically support that use. That said, it's not as if loopback ssh needs any extra special support - so chances are good this will just work by accident.

@adamtheturtle
Copy link
Contributor

There are linting errors to fix.

wallrj
wallrj reviewed Jul 4, 2014
View reviewed changes
flocker/cli/_sshconfig.py
if not local_private_path.exists():
with open(devnull, "w") as discard:
check_call(
[b"ssh-keygen", b"-N", b"", b"-f",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use long options here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ignore that. ssh-keygen doesn't support long options! At least they're not documented in the man page.

wallrj
wallrj reviewed Jul 4, 2014
View reviewed changes
flocker/cli/functional/test_sshconfig.py
sock = output[0].split()[2][:-1]
pid = output[1].split()[2][:-1]
environ[b"SSH_AUTH_SOCK"] = sock
environ[b"SSH_AGENT_PID"] = pid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will modify the environment for the entire remaining test run, right? Should we clean these up after each test? Or pass a custom environment dictionary to each subprocess?

@wallrj
Copy link
Contributor

wallrj commented Jul 4, 2014

Thanks @exarkun

This is all clever stuff, especially how you use the ConchTesting server. But I guess it was frustrating working with the ssh command line client.
My understanding is that this is going to be called from flocker-deploy to install a common set of private and public keys all the nodes in the cluster. After which they'll be able to ssh to one another without a password. And they'll do that when they need to push volumes to one another.

Is flocker-deploy going to do this for all nodes, everytime? Sounds like it could be slow? In the tests, you're calling configure_ssh via deferToThread, but do you think it would have been worth making configure_ssh an async function, using a ProcessEndpoint ? Which would have allowed us to call it in parallel without deferToThread

Anyway:

@exarkun
Copy link
Contributor Author

exarkun commented Jul 4, 2014

This is all clever stuff, especially how you use the ConchTesting server.

Mostly just re-used what @itamarst already built there. 😄

But I guess it was frustrating working with the ssh command line client.

Yes, definitely. I wonder what gave it away?

My understanding is that this is going to be called from flocker-deploy to install a common set of private and public keys all the nodes in the cluster. After which they'll be able to ssh to one another without a password. And they'll do that when they need to push volumes to one another.

Correct.

Is flocker-deploy going to do this for all nodes, everytime? Sounds like it could be slow?

At least initially, yes. If it's too slow then maybe we can explore fixes - for example, remember where we've deployed keys and not try to re-deploy them unless explicitly requested by the user.

In the tests, you're calling configure_ssh via deferToThread, but do you think it would have been worth making configure_ssh an async function, using a ProcessEndpoint ?

I suspect that would have been better, yes.

Thanks for the review.

@exarkun
Copy link
Contributor Author

exarkun commented Jul 7, 2014

Only spurious build failures, merging now.

exarkun added a commit that referenced this pull request Jul 7, 2014
@exarkun
Merge pull request #194 from ClusterHQ/flocker-deploy-ssh-config-71
9daebf4 
Add an API which can deploy the necessary SSH configuration to all of the Flocker cluster nodes and the administrator's client environment.
@exarkun exarkun merged commit 9daebf4 into master Jul 7, 2014
@exarkun exarkun deleted the flocker-deploy-ssh-config-71 branch July 7, 2014 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

flocker go should setup all nodes to be able to SSH to each other
3 participants
@exarkun @adamtheturtle @wallrj