You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2017/06/14 10:35:23 UTC
[jclouds/jclouds-labs] Improve duplicate ssh key check in Packet
(#397)
@andreaturli please review.
Apologies for the noise in the PR; I've formatted the class to 3-space indent. There is a small set of changes:
* Generate a unique name for the generated key pair if the user does not provide a key. We were generating them with repeated names.
* Improve the check for the key existence (keys can't be duplicated in Packet). previously we just checked the names. Now the code checks the fingerprints of existing keys to determine if it already exists or not.
* When cleaning up the ssh keys, just delete the ones generated by jclouds, but do not delete any pre-existing key that might be used. if it was there, we leave it there.
* Synchronized the key check and creation to avoid race conditions in concurrent environments.
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/397
-- Commit Summary --
* Improve duplicate ssh key check in Packet
-- File Changes --
M packet/src/main/java/org/jclouds/packet/compute/strategy/CreateSshKeysThenCreateNodes.java (289)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/397.patch
https://github.com/jclouds/jclouds-labs/pull/397.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/397
Re: [jclouds/jclouds-labs] Improve duplicate ssh key check in Packet
(#397)
Posted by Andrea Turli <no...@github.com>.
lgtm, thanks @nacx
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/397#issuecomment-308673244
Re: [jclouds/jclouds-labs] Improve duplicate ssh key check in Packet
(#397)
Posted by Ignasi Barrera <no...@github.com>.
Closed #397.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/397#event-1124850801
Re: [jclouds/jclouds-labs] Improve duplicate ssh key check in Packet
(#397)
Posted by Ignasi Barrera <no...@github.com>.
>why only against 2.0.x?
I developed it in 2.0.x, but will be merged to master too.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/397#issuecomment-308659972
Re: [jclouds/jclouds-labs] Improve duplicate ssh key check in Packet
(#397)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
>
+ PublicKey userKey = readPublicKey(options.getPublicKey());
+ final String fingerprint = computeFingerprint(userKey);
+
+ synchronized (CreateSshKeysThenCreateNodes.class) {
+ boolean keyExists = api.sshKeyApi().list().concat().anyMatch(new Predicate<SshKey>() {
ok makes sense
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/397#discussion_r122154437
Re: [jclouds/jclouds-labs] Improve duplicate ssh key check in Packet
(#397)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to [master](http://git-wip-us.apache.org/repos/asf/jclouds/commit/b77ea069) and [2.0,.x](http://git-wip-us.apache.org/repos/asf/jclouds-labs/commit/c1fb7620). Thanks @andreaturli!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/397#issuecomment-308693466
Re: [jclouds/jclouds-labs] Improve duplicate ssh key check in Packet
(#397)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
lgtm, some minor comments
also tested `PacketComputeServiceLiveTest` which look alright.
why only against `2.0.x`?
>
+ PublicKey userKey = readPublicKey(options.getPublicKey());
+ final String fingerprint = computeFingerprint(userKey);
+
+ synchronized (CreateSshKeysThenCreateNodes.class) {
why is it needed?
>
+ PublicKey userKey = readPublicKey(options.getPublicKey());
+ final String fingerprint = computeFingerprint(userKey);
+
+ synchronized (CreateSshKeysThenCreateNodes.class) {
+ boolean keyExists = api.sshKeyApi().list().concat().anyMatch(new Predicate<SshKey>() {
isn't `SshKey key = api.sshKeyApi().get(fingerprint);` more appropriate here?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/397#pullrequestreview-44120886
Re: [jclouds/jclouds-labs] Improve duplicate ssh key check in Packet
(#397)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
>
+ PublicKey userKey = readPublicKey(options.getPublicKey());
+ final String fingerprint = computeFingerprint(userKey);
+
+ synchronized (CreateSshKeysThenCreateNodes.class) {
+ boolean keyExists = api.sshKeyApi().list().concat().anyMatch(new Predicate<SshKey>() {
No. The parameter to the `get` method is the "name" of the key. It happens that we were setting the fingerprint as the name when creating the keys (for convenience), but the old implementation, that already did this, was not able to find keys with normal names. We need to inspect the fingerprint field of all keys.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/397#discussion_r122142625
Re: [jclouds/jclouds-labs] Improve duplicate ssh key check in Packet
(#397)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
>
+ PublicKey userKey = readPublicKey(options.getPublicKey());
+ final String fingerprint = computeFingerprint(userKey);
+
+ synchronized (CreateSshKeysThenCreateNodes.class) {
To avoid race conditions on this "getOrCreate" method. You could have apps/clients accessing the same project with different API keys (that is, two different jclouds contexts), and race conditions could appear here.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/397#discussion_r122142423