CHE-2059 : Each time a workspace is created, register/associate ssh key #2949

benoitf · 2016-10-31T16:08:51Z

What does this PR do?

Adds ssh key for creator of workspace. This ssh key can be used to connect on machines

What issues does this PR fix or reference?

#2059

Previous behavior

no ssh key by default

New behavior

Create a workspace
SSH key is associated
You can use then this key to connect to the workspace machines.

Change-Id: Ifddfe5398cffd1af31aa93beb5d0674b29270f4f
Signed-off-by: Florent BENOIT fbenoit@codenvy.com

benoitf · 2016-10-31T18:12:20Z

@sleshchenko @garagatyi if you could review
issue link is #2059

codenvy-ci · 2016-10-31T18:25:07Z

Build # 864 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/864/ to view the results.

codenvy-ci · 2016-10-31T21:10:34Z

Build # 868 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/868/ to view the results.

sleshchenko · 2016-11-01T06:30:47Z

...achine-ext-server/src/main/java/org/eclipse/che/ide/ext/machine/server/ssh/KeysInjector.java

+                        // build list of all pairs.
+                        final List<String> publicKeys;
+                        if (sshWorkspacePair != null && sshWorkspacePair.getPublicKey() != null) {
+                            publicKeys = new ArrayList<>(publicMachineKeys.size() + 1);


Why do you create list for publicMachineKeys.size() + 1 but put there only one element publicKeys.add(sshWorkspacePair.getPublicKey());? Am I correct?

You're correct
I need to fix my PR it is missing the other keys

also tests are not on my PR I think I failed to merge changes to master

Looks like list creation and addition of publicMachineKeys should be out of if clause. Then size + 1 is fine.

sleshchenko · 2016-11-01T06:31:43Z

...achine-ext-server/src/main/java/org/eclipse/che/ide/ext/machine/server/ssh/KeysInjector.java

+
+                        // build list of all pairs.
+                        final List<String> publicKeys;
+                        if (sshWorkspacePair != null && sshWorkspacePair.getPublicKey() != null) {


sshWorkspacePair can't be null https://github.com/eclipse/che/blob/master/wsmaster/che-core-api-ssh/src/main/java/org/eclipse/che/api/ssh/server/SshManager.java#L106

sleshchenko · 2016-11-01T06:31:58Z

...achine-ext-server/src/main/java/org/eclipse/che/ide/ext/machine/server/ssh/KeysInjector.java

                                                             .filter(sshPair -> sshPair.getPublicKey() != null)
                                                             .map(SshPairImpl::getPublicKey)
                                                             .collect(Collectors.toList());

+                        // get workspace keypair
+                        SshPairImpl sshWorkspacePair = sshManager.getPair(machine.getOwner(), "workspace", event.getWorkspaceId());


I think you should catch NotFoundException here

I have to catch yes, else it won't work

garagatyi · 2016-11-01T08:21:54Z

...achine-ext-server/src/main/java/org/eclipse/che/ide/ext/machine/server/ssh/KeysInjector.java

+                        // build list of all pairs.
+                        final List<String> publicKeys;
+                        if (sshWorkspacePair != null && sshWorkspacePair.getPublicKey() != null) {
+                            publicKeys = new ArrayList<>(publicMachineKeys.size() + 1);


Looks like list creation and addition of publicMachineKeys should be out of if clause. Then size + 1 is fine.

garagatyi · 2016-11-01T08:40:52Z

...-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java


    @Inject
    public WorkspaceManager(WorkspaceDao workspaceDao,
                            WorkspaceRuntimes workspaceRegistry,
                            EventService eventService,
                            AccountManager accountManager,
-                            @Named("che.workspace.auto_snapshot") boolean defaultAutoSnapshot,


I think that correct names were changed, please check

garagatyi · 2016-11-01T08:42:20Z

...-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java

@@ -877,6 +882,10 @@ private WorkspaceImpl doCreateWorkspace(WorkspaceConfig config,
                                                     .setAttributes(attributes)
                                                     .setTemporary(isTemporary)
                                                     .build();
+
+        // Register default SSH keypair for this workspace
+        SshPairImpl sshPair = this.sshManager.generatePair(EnvironmentContext.getCurrent().getSubject().getUserId(), "workspace", workspace.getId());


Can you elaborate what this operation does, because it is not obvious why result of this operation is never used.

We don't need to capture the result (forgot to cleanup)

garagatyi · 2016-11-01T08:42:42Z

...e-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java

@@ -117,6 +118,8 @@
    private AccountManager                     accountManager;
    @Mock
    private SnapshotDao                        snapshotDao;
+    @Mock
+    private SshManager                    sshManager;


Please align variables, because author kept them aligned

garagatyi · 2016-11-01T08:43:34Z

...e-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java

@@ -784,7 +789,8 @@ public void shouldStartWorkspaceFromSnapshotUsingDefaultValueForAutoRestore() th
                                                    accountManager,
                                                    false,
                                                    true,
-                                                    snapshotDao));
+                                                    snapshotDao,
+                                                    sshManager));
        final WorkspaceImpl workspace = workspaceManager.createWorkspace(createConfig(), NAMESPACE);
        when(workspaceDao.get(workspace.getId())).thenReturn(workspace);
        when(runtimes.get(any())).thenThrow(new NotFoundException(""));


Please add tests to changed code

Change-Id: Ifddfe5398cffd1af31aa93beb5d0674b29270f4f Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

…e ssh key Change-Id: I92b699f2d0744b7789d6b3d4a6cd68fc23ca8777 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

garagatyi

LGTM

codenvy-ci · 2016-11-02T11:42:27Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/894/

sleshchenko · 2016-11-02T12:54:29Z

...achine-ext-server/src/main/java/org/eclipse/che/ide/ext/machine/server/ssh/KeysInjector.java

+                        // build list of all pairs.
+                        final List<String> publicKeys;
+                        if (sshWorkspacePair != null && sshWorkspacePair.getPublicKey() != null) {
+                            publicKeys = new ArrayList<>(publicMachineKeys.size() + 1);


Why should we create new list? Why we can't just add sshWorkspacePair to publicMachineKeys list?

existing list may be immutable no ?

sleshchenko · 2016-11-02T14:36:21Z

...-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/WorkspaceManager.java

+
+        // Register default SSH keypair for this workspace.
+        try {
+            this.sshManager.generatePair(EnvironmentContext.getCurrent().getSubject().getUserId(), "workspace", workspace.getId());


I think it would better to rewrite this logic by events in ...api-ssh... or che-plugin-machine-ext-server module? Because then ssh module will contain logic related to ssh and workspace module won't contain logic related to ssh :)
@garagatyi WDYT?

Also should we remove default ssh key when workspace is removed?

I like the idea.

Also should we remove default ssh key when workspace is removed?

I think so

about events yes why not but then in api-ssh machine-ext-server is not at the right "level" as it needs to be at workspace level
it would mean that api-ssh depends on api-workspace (in my PR it's the opposite)

on removal of ssh key yes make sense

api-ssh machine-ext-server is not at the right "level" as it needs to be at workspace level

The main problem is that machine-ext-server module has wrong name :)
We can see here only KeysInjector and Module with binding for it and for RecipeScriptDownloadService. So RecipeScriptDownloadService was moved to api-workspace but binding wasn't. So we should move binding to module in api-workspace or to OnpremisesIdeApiModule
Then machine-ext-server will contains only classes related to ssh keys and you can rename it as you wish, I think :)
WDYT?

…e ssh key Change-Id: I65d149e973f6ebd77ac3fc4c5539373c0b3d672e Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf · 2016-11-02T17:14:30Z

@sleshchenko updated with events

…e ssh key Change-Id: If2f6a5827ed162cd2f5a2d661fed77242ecf6c67 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

…e ssh key Change-Id: I1e3d39ea38feb286f904161f46d77b1b53eb4315 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

codenvy-ci · 2016-11-02T18:00:46Z

Build # 903 - FAILED

Please check console output at https://ci.codenvycorp.com/job/che-pullrequests-build/903/ to view the results.

…e ssh key Change-Id: Icbd591ddc13c7e9ee4740dee41e674cc7c5cee45 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

codenvy-ci · 2016-11-02T21:07:16Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/905/

sleshchenko · 2016-11-03T08:58:02Z

...ne-ext-server/src/main/java/org/eclipse/che/ide/ext/machine/server/ssh/WorkspaceSshKeys.java

+                    sshManager.removePair(EnvironmentContext.getCurrent().getSubject().getUserId(), "workspace",
+                                          workspaceRemovedEvent.getWorkspace().getId());
+                } catch (NotFoundException e) {
+                    LOG.error(String.format("Do not remove default keypair from workspace %s as it is not existing (workspace ID %s)",


I think we should not log this case as error

ah yes copy/paste error fixed

sleshchenko · 2016-11-03T09:03:53Z

Small question: when user 1 create workspace A and share it for user2. When user2 starts this workspace, he is not able to see this default ssh key. So he is not able to connect to it by ssh.
@benoitf It is OK?

…e ssh key Change-Id: I96c1470f8ca973f50b6d12218cd6d419127228c1 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf · 2016-11-03T09:06:36Z

@sleshchenko I discussed that a couple of weeks ago with Tyler. It was OK that only owner of workspace has the key. Not all people to which we shared the workspace.

garagatyi · 2016-11-03T09:20:35Z

...achine-ext-server/src/main/java/org/eclipse/che/ide/ext/machine/server/ssh/KeysInjector.java

                    try {
-                        final Instance machine = environmentEngine.getMachine(event.getWorkspaceId(),
-                                                                              event.getMachineId());
+                        machine = environmentEngine.getMachine(event.getWorkspaceId(),


Did we plan to inject keys into each machine or dev-machines only?

each machine

codenvy-ci · 2016-11-03T10:20:07Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/909/

garagatyi · 2016-11-03T09:24:32Z

...ne-ext-server/src/main/java/org/eclipse/che/ide/ext/machine/server/ssh/WorkspaceSshKeys.java

+                                            workspaceRemovedEvent.getWorkspace().getId()));
+                } catch (ServerException e) {
+                    LOG.error(String.format(
+                            "Error when trying to remove default ssh pair for the workspace %s as it is not existing (workspace ID %s)",


Why message says that "it is not existing" on server exception?

garagatyi · 2016-11-03T09:25:13Z

...ne-ext-server/src/main/java/org/eclipse/che/ide/ext/machine/server/ssh/WorkspaceSshKeys.java

+                                            workspaceRemovedEvent.getWorkspace().getConfig().getName(),
+                                            workspaceRemovedEvent.getWorkspace().getId()));
+                } catch (ServerException e) {
+                    LOG.error(String.format(


Please use logger format with curly brackets instead of String.format()

garagatyi · 2016-11-03T09:26:28Z

...ne-ext-server/src/main/java/org/eclipse/che/ide/ext/machine/server/ssh/WorkspaceSshKeys.java

+ * @author Florent Benoit
+ */
+@Singleton // must be eager
+public class WorkspaceSshKeys {


Purpose of this class is not clear from its name, can you rename it?

For me the name is clear.
Also there is javadoc explaining the purpose so it's subjective

garagatyi · 2016-11-03T09:28:02Z

...ne-ext-server/src/test/java/org/eclipse/che/ide/ext/machine/server/ssh/KeysInjectorTest.java

+        verifyZeroInteractions(docker, environmentEngine, sshManager);
+    }
+
+


Please remove not needed empty lines

garagatyi · 2016-11-03T09:31:01Z

...xt-server/src/test/java/org/eclipse/che/ide/ext/machine/server/ssh/WorkspaceSshKeysTest.java

+        EnvironmentContext.setCurrent(new EnvironmentContext() {
+            @Override
+            public Subject getSubject() {
+                return new SubjectImpl(NAMESPACE, OWNER_ID, "token", false);


Don't forget to revert changes in EnvironmentContext to not affect other tests

garagatyi · 2016-11-03T10:53:24Z

...e-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/WorkspaceManagerTest.java

@@ -1051,6 +1051,8 @@ public void shouldBeAbleToGetMachineInstanceIfWorkspaceIsStarting() throws Excep
        verify(runtimes).getMachine(workspace.getId(), machine.getId());
    }

+


Please remove these extra lines

…e ssh key Change-Id: Ia3acda4e8cdf3edbcdb6c17536a37b7310736f0f Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf · 2016-11-03T13:11:40Z

@garagatyi fixed

codenvy-ci · 2016-11-03T13:12:07Z

Build success. https://ci.codenvycorp.com/job/che-pullrequests-build/912/

…ey (eclipse-che#2949) * CHE-2059 : Each time a workspace is created, register/associate ssh key Change-Id: Ifddfe5398cffd1af31aa93beb5d0674b29270f4f Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. kind/enhancement A feature request - must adhere to the feature request template. sprint/current labels Oct 31, 2016

benoitf self-assigned this Oct 31, 2016

benoitf force-pushed the CHE-2059-2 branch from 9c03199 to bf618ba Compare October 31, 2016 19:27

sleshchenko reviewed Nov 1, 2016

View reviewed changes

benoitf changed the title ~~CHE-2059 : Each time a workspace is created, register/associate ssh key~~ [WiP] CHE-2059 : Each time a workspace is created, register/associate ssh key Nov 1, 2016

garagatyi suggested changes Nov 1, 2016

View reviewed changes

benoitf added 2 commits November 2, 2016 09:17

CHE-2059 : Each time a workspace is created, register/associate ssh key

670f6f7

Change-Id: Ifddfe5398cffd1af31aa93beb5d0674b29270f4f Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

fixup! CHE-2059 : Each time a workspace is created, register/associat…

185aa14

…e ssh key Change-Id: I92b699f2d0744b7789d6b3d4a6cd68fc23ca8777 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf force-pushed the CHE-2059-2 branch from bf618ba to 185aa14 Compare November 2, 2016 09:38

benoitf changed the title ~~[WiP] CHE-2059 : Each time a workspace is created, register/associate ssh key~~ CHE-2059 : Each time a workspace is created, register/associate ssh key Nov 2, 2016

garagatyi approved these changes Nov 2, 2016

View reviewed changes

sleshchenko reviewed Nov 2, 2016

View reviewed changes

fixup! CHE-2059 : Each time a workspace is created, register/associat…

458fde6

…e ssh key Change-Id: I65d149e973f6ebd77ac3fc4c5539373c0b3d672e Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

benoitf added 2 commits November 2, 2016 18:18

fixup! CHE-2059 : Each time a workspace is created, register/associat…

9518c25

…e ssh key Change-Id: If2f6a5827ed162cd2f5a2d661fed77242ecf6c67 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

fixup! CHE-2059 : Each time a workspace is created, register/associat…

9f25c6c

…e ssh key Change-Id: I1e3d39ea38feb286f904161f46d77b1b53eb4315 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

fixup! CHE-2059 : Each time a workspace is created, register/associat…

af94079

…e ssh key Change-Id: Icbd591ddc13c7e9ee4740dee41e674cc7c5cee45 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

sleshchenko reviewed Nov 3, 2016

View reviewed changes

fixup! CHE-2059 : Each time a workspace is created, register/associat…

947d67e

…e ssh key Change-Id: I96c1470f8ca973f50b6d12218cd6d419127228c1 Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

sleshchenko approved these changes Nov 3, 2016

View reviewed changes

garagatyi reviewed Nov 3, 2016

View reviewed changes

garagatyi suggested changes Nov 3, 2016

View reviewed changes

fixup! CHE-2059 : Each time a workspace is created, register/associat…

a62eeaf

…e ssh key Change-Id: Ia3acda4e8cdf3edbcdb6c17536a37b7310736f0f Signed-off-by: Florent BENOIT <fbenoit@codenvy.com>

garagatyi approved these changes Nov 4, 2016

View reviewed changes

benoitf merged commit 4a86b5e into master Nov 4, 2016

benoitf deleted the CHE-2059-2 branch November 4, 2016 09:31

benoitf added this to the 5.0.0-M7 milestone Nov 4, 2016

benoitf removed status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. sprint/current labels Nov 4, 2016

bmicklea mentioned this pull request Nov 16, 2016

5.0.0-M7 Milestone Plan #2935

Closed

66 tasks

		verifyZeroInteractions(docker, environmentEngine, sshManager);
		}

		@@ -1051,6 +1051,8 @@ public void shouldBeAbleToGetMachineInstanceIfWorkspaceIsStarting() throws Excep
		verify(runtimes).getMachine(workspace.getId(), machine.getId());
		}

CHE-2059 : Each time a workspace is created, register/associate ssh key #2949

CHE-2059 : Each time a workspace is created, register/associate ssh key #2949

Conversation

benoitf commented Oct 31, 2016

What does this PR do?

What issues does this PR fix or reference?

Previous behavior

New behavior

benoitf commented Oct 31, 2016

codenvy-ci commented Oct 31, 2016

codenvy-ci commented Oct 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garagatyi left a comment

Choose a reason for hiding this comment

codenvy-ci commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garagatyi Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleshchenko Nov 2, 2016 • edited Loading

Choose a reason for hiding this comment

benoitf commented Nov 2, 2016

codenvy-ci commented Nov 2, 2016

codenvy-ci commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleshchenko commented Nov 3, 2016

benoitf commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codenvy-ci commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benoitf commented Nov 3, 2016

codenvy-ci commented Nov 3, 2016

garagatyi Nov 2, 2016 •

edited

Loading

sleshchenko Nov 2, 2016 •

edited

Loading