Merge pull request #61 from jglick/NPE-JENKINS-44830

[JENKINS-44830] NPE after upgrading caused by fix of JENKINS-42959
jenkinsci · Jun 12, 2017 · 367bfdf · 367bfdf
2 parents ce18ed6 + 7487db5
commit 367bfdf
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 16 deletions.
diff --git a/pom.xml b/pom.xml
@@ -80,6 +80,12 @@
       <artifactId>ssh-credentials</artifactId>
       <version>1.6.1</version>
     </dependency>
+    <dependency>
+      <groupId>org.jenkins-ci.test</groupId>
+      <artifactId>docker-fixtures</artifactId>
+      <version>1.0</version>
+      <scope>test</scope>
+    </dependency>
   </dependencies>
 
 </project>
diff --git a/src/main/java/hudson/plugins/sshslaves/SSHLauncher.java b/src/main/java/hudson/plugins/sshslaves/SSHLauncher.java
@@ -272,6 +272,7 @@ public Object readResolve() {
      * The verifier to use for checking the SSH key presented by the host
      * responding to the connection
      */
+    @CheckForNull
     private final SshHostKeyVerificationStrategy sshHostKeyVerificationStrategy;
 
     /**
@@ -553,10 +554,16 @@ public String getCredentialsId() {
         return credentialsId;
     }
 
+    @CheckForNull
     public SshHostKeyVerificationStrategy getSshHostKeyVerificationStrategy() {
         return sshHostKeyVerificationStrategy;
     }
 
+    @NonNull
+    SshHostKeyVerificationStrategy getSshHostKeyVerificationStrategyDefaulted() {
+        return sshHostKeyVerificationStrategy != null ? sshHostKeyVerificationStrategy : new NonVerifyingKeyVerificationStrategy();
+    }
+
     public StandardUsernameCredentials getCredentials() {
         String credentialsId = this.credentialsId == null
                 ? (this.credentials == null ? null : this.credentials.getId())
@@ -786,7 +793,7 @@ public synchronized void launch(final SlaveComputer computer, final TaskListener
             public Boolean call() throws InterruptedException {
                 Boolean rval = Boolean.FALSE;
                 try {
-                    connection.setServerHostKeyAlgorithms(sshHostKeyVerificationStrategy.getPreferredKeyAlgorithms(computer));
+                    connection.setServerHostKeyAlgorithms(getSshHostKeyVerificationStrategyDefaulted().getPreferredKeyAlgorithms(computer));
 
                     openConnection(listener, computer);
 
@@ -1270,14 +1277,7 @@ public boolean verifyServerHostKey(String hostname, int port, String serverHostK
 
                         final HostKey key = new HostKey(serverHostKeyAlgorithm, serverHostKey);
 
-                        final SshHostKeyVerificationStrategy hostKeyVerificationStrategy;
-                        if (null == sshHostKeyVerificationStrategy) {
-                            hostKeyVerificationStrategy = new NonVerifyingKeyVerificationStrategy();
-                        } else {
-                            hostKeyVerificationStrategy = sshHostKeyVerificationStrategy;
-                        }
-
-                        return hostKeyVerificationStrategy.verify(computer, key, listener);
+                        return getSshHostKeyVerificationStrategyDefaulted().verify(computer, key, listener);
                     }
                 });
                 break;

diff --git a/src/test/java/hudson/plugins/sshslaves/SSHLauncherTest.java b/src/test/java/hudson/plugins/sshslaves/SSHLauncherTest.java
@@ -37,23 +37,22 @@
 
 import com.cloudbees.jenkins.plugins.sshcredentials.impl.BasicSSHUserPrivateKey;
 import com.cloudbees.plugins.credentials.Credentials;
-import com.cloudbees.plugins.credentials.CredentialsProvider;
 import com.cloudbees.plugins.credentials.CredentialsScope;
 import com.cloudbees.plugins.credentials.SystemCredentialsProvider;
-import com.cloudbees.plugins.credentials.common.StandardUsernameCredentials;
 import com.cloudbees.plugins.credentials.domains.Domain;
 import com.cloudbees.plugins.credentials.domains.DomainSpecification;
-import com.cloudbees.plugins.credentials.domains.HostnamePortRequirement;
 import com.cloudbees.plugins.credentials.domains.HostnamePortSpecification;
-import hudson.model.Descriptor;
+import hudson.model.Computer;
+import hudson.model.FreeStyleProject;
+import hudson.model.Node;
 import hudson.model.Node.Mode;
 import hudson.model.Slave;
-import hudson.plugins.sshslaves.SSHLauncher.DefaultJDKInstaller;
-import hudson.security.ACL;
 import hudson.slaves.DumbSlave;
 import hudson.slaves.RetentionStrategy;
+import java.util.concurrent.ExecutionException;
+import org.jenkinsci.test.acceptance.docker.DockerRule;
+import org.jenkinsci.test.acceptance.docker.fixtures.JavaContainer;
 
-import hudson.util.ListBoxModel;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.JenkinsRule;
@@ -62,12 +61,21 @@
 import static org.junit.Assert.assertNotSame;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import org.junit.ClassRule;
+import org.jvnet.hudson.test.BuildWatcher;
+import org.jvnet.hudson.test.Issue;
 
 public class SSHLauncherTest {
 
+    @ClassRule
+    public static BuildWatcher buildWatcher = new BuildWatcher();
+
     @Rule
     public JenkinsRule j = new JenkinsRule();
 
+    @Rule
+    public DockerRule<JavaContainer> javaContainer = new DockerRule<>(JavaContainer.class);
+
     @Test
     public void checkJavaVersionOpenJDK7NetBSD() throws Exception {
         assertTrue("OpenJDK7 on NetBSD should be supported", checkSupported("openjdk-7-netbsd.version"));
@@ -158,4 +166,26 @@ public void fillCredentials() {
         assertEquals(1, desc.doFillCredentialsIdItems(j.jenkins, "", "forty two", "does-not-exist").size());
         assertEquals(1, desc.doFillCredentialsIdItems(j.jenkins, "", "", "does-not-exist").size());
     }
+
+    @Issue("JENKINS-44830")
+    @Test
+    public void upgrade() throws Exception {
+        JavaContainer c = javaContainer.get();
+        DumbSlave slave = new DumbSlave("slave" + j.jenkins.getNodes().size(),
+                "dummy", "/home/test/slave", "1", Node.Mode.NORMAL, "remote",
+                // Old constructor passes null sshHostKeyVerificationStrategy:
+                new SSHLauncher(c.ipBound(22), c.port(22), "test", "test", "", ""),
+                RetentionStrategy.INSTANCE, Collections.<NodeProperty<?>>emptyList());
+        j.jenkins.addNode(slave);
+        Computer computer = slave.toComputer();
+        try {
+            computer.connect(false).get();
+        } catch (ExecutionException x) {
+            throw new AssertionError("failed to connect: " + computer.getLog(), x);
+        }
+        FreeStyleProject p = j.createFreeStyleProject();
+        p.setAssignedNode(slave);
+        j.buildAndAssertSuccess(p);
+    }
+
 }