Patch 1

.github/PULL_REQUEST_TEMPLATE.md

@@ -14,6 +14,18 @@ If the issue is not fully described in Jira, add more information here (justific
  * Major new features should have a Jira issue.
 -->
 
+### Testing done
+
+<!-- Comment:
+Provide a clear description of how this change was tested.
+At minimum this should include proof that a computer has executed the changed lines.
+Ideally this should include an automated test or an explanation as to why this change has no tests.
+Note that automated test coverage is less than complete, so a successful PR build does not necessarily imply that a computer has executed the changed lines.
+If automated test coverage does not exist for the lines you are changing, you must describe the scenario(s) in which you manually tested the change.
+For frontend changes, include screenshots of the relevant page(s) before and after the change.
+For refactoring and code cleanup changes, exercise the code before and after the change and verify the behavior remains the same.
+-->
+
 ### Proposed changelog entries
 
 - Entry 1: Issue, human-readable text

test/pom.xml

@@ -93,7 +93,7 @@ THE SOFTWARE.
     <dependency>
       <groupId>${project.groupId}</groupId>
       <artifactId>jenkins-test-harness</artifactId>
-      <version>1854.v92f6cb_540509</version>
+      <version>1856.v943040a_5cd13</version>
       <scope>test</scope>
       <exclusions>
         <exclusion>

test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java

@@ -30,33 +30,22 @@
 import com.gargoylesoftware.htmlunit.Page;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
 import com.gargoylesoftware.htmlunit.xml.XmlPage;
-import hudson.Launcher;
-import hudson.Proc;
-import hudson.model.Node.Mode;
 import hudson.model.Slave;
 import hudson.model.User;
 import hudson.remoting.Channel;
-import hudson.slaves.DumbSlave;
-import hudson.slaves.JNLPLauncher;
-import hudson.slaves.RetentionStrategy;
-import hudson.util.StreamTaskListener;
 import java.io.File;
 import java.net.HttpURLConnection;
 import java.net.URL;
-import java.util.Collections;
-import java.util.List;
 import java.util.Locale;
 import jenkins.security.apitoken.ApiTokenTestHelper;
 import jenkins.security.s2m.AdminWhitelistRule;
-import org.apache.commons.io.FileUtils;
-import org.apache.tools.ant.util.JavaEnvUtils;
 import org.dom4j.Document;
 import org.dom4j.Element;
 import org.dom4j.io.DOMReader;
 import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
 import org.jvnet.hudson.test.Email;
+import org.jvnet.hudson.test.InboundAgentRule;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.recipes.PresetData;
 import org.jvnet.hudson.test.recipes.PresetData.DataSet;
@@ -72,22 +61,15 @@ public class JnlpAccessWithSecuredHudsonTest {
     public JenkinsRule r = new JenkinsRule();
 
     @Rule
-    public TemporaryFolder tmp = new TemporaryFolder();
-
-    /**
-     * Creates a new agent that needs to be launched via JNLP.
-     */
-    protected Slave createNewJnlpSlave(String name) throws Exception {
-        return new DumbSlave(name, "", System.getProperty("java.io.tmpdir") + '/' + name, "2", Mode.NORMAL, "", new JNLPLauncher(true), RetentionStrategy.INSTANCE, Collections.EMPTY_LIST);
-    }
+    public InboundAgentRule inboundAgents = new InboundAgentRule();
 
     @PresetData(DataSet.NO_ANONYMOUS_READACCESS)
     @Email("http://markmail.org/message/on4wkjdaldwi2atx")
     @Test
     public void anonymousCanAlwaysLoadJARs() throws Exception {
         ApiTokenTestHelper.enableLegacyBehavior();
 
-        r.jenkins.setNodes(List.of(createNewJnlpSlave("test")));
+        inboundAgents.createAgent(r, InboundAgentRule.Options.newBuilder().name("test").skipStart().build());
         JenkinsRule.WebClient wc = r.createWebClient();
         HtmlPage p = wc.withBasicApiToken(User.getById("alice", true)).goTo("computer/test/");
 
@@ -111,35 +93,21 @@ public void anonymousCanAlwaysLoadJARs() throws Exception {
     @PresetData(DataSet.ANONYMOUS_READONLY)
     @Test
     public void anonymousCannotGetSecrets() throws Exception {
-        r.jenkins.setNodes(List.of(createNewJnlpSlave("test")));
+        inboundAgents.createAgent(r, InboundAgentRule.Options.newBuilder().name("test").skipStart().build());
         r.createWebClient().assertFails("computer/test/jenkins-agent.jnlp", HttpURLConnection.HTTP_FORBIDDEN);
     }
 
     @PresetData(DataSet.NO_ANONYMOUS_READACCESS)
-    @SuppressWarnings("SleepWhileInLoop")
     @Test
     public void serviceUsingDirectSecret() throws Exception {
-        Slave slave = createNewJnlpSlave("test");
-        r.jenkins.setNodes(List.of(slave));
+        Slave slave = inboundAgents.createAgent(r, InboundAgentRule.Options.newBuilder().name("test").secret().build());
+        r.waitOnline(slave);
         r.createWebClient().goTo("computer/test/jenkins-agent.jnlp?encrypt=true", "application/octet-stream");
-        String secret = slave.getComputer().getJnlpMac();
-        // To watch it fail: secret = secret.replace('1', '2');
-        File slaveJar = tmp.newFile();
-        FileUtils.copyURLToFile(new Slave.JnlpJar("agent.jar").getURL(), slaveJar);
-        Proc p = new Launcher.LocalLauncher(StreamTaskListener.fromStderr()).launch().
-            stdout(System.out).stderr(System.err).
-            cmds(JavaEnvUtils.getJreExecutable("java"), "-jar", slaveJar.getAbsolutePath(), "-jnlpUrl", r.getURL() + "computer/test/jenkins-agent.jnlp", "-secret", secret).
-            start();
-        try {
-            r.waitOnline(slave);
             Channel channel = slave.getComputer().getChannel();
             assertFalse("SECURITY-206", channel.isRemoteClassLoadingAllowed());
             r.jenkins.getExtensionList(AdminWhitelistRule.class).get(AdminWhitelistRule.class).setMasterKillSwitch(false);
             final File f = new File(r.jenkins.getRootDir(), "config.xml");
             assertTrue(f.exists());
-        } finally {
-            p.kill();
-        }
     }
 
 

test/src/test/java/hudson/cli/OfflineNodeCommandTest.java

@@ -27,8 +27,6 @@
 import static hudson.cli.CLICommandInvoker.Matcher.failedWith;
 import static hudson.cli.CLICommandInvoker.Matcher.hasNoStandardOutput;
 import static hudson.cli.CLICommandInvoker.Matcher.succeededSilently;
-import static org.awaitility.Awaitility.await;
-import static org.hamcrest.CoreMatchers.notNullValue;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
@@ -39,18 +37,16 @@
 import hudson.model.Computer;
 import hudson.model.FreeStyleBuild;
 import hudson.model.FreeStyleProject;
+import hudson.model.Slave;
 import hudson.slaves.DumbSlave;
-import hudson.slaves.JNLPLauncher;
 import hudson.slaves.OfflineCause;
-import hudson.slaves.RetentionStrategy;
 import hudson.util.OneShotEvent;
-import java.io.File;
 import java.util.concurrent.Future;
-import java.util.concurrent.TimeUnit;
 import jenkins.model.Jenkins;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.jvnet.hudson.test.InboundAgentRule;
 import org.jvnet.hudson.test.JenkinsRule;
 
 /**
@@ -63,6 +59,9 @@ public class OfflineNodeCommandTest {
     @Rule
     public final JenkinsRule j = new JenkinsRule();
 
+    @Rule
+    public InboundAgentRule inboundAgents = new InboundAgentRule();
+
     @Before
     public void setUp() {
         command = new CLICommandInvoker(j, "offline-node");
@@ -112,15 +111,7 @@ public void offlineNodeShouldSucceedOnOnlineNode() throws Exception {
 
     @Test
     public void offlineNodeShouldSucceedOnOfflineNode() throws Exception {
-        DumbSlave slave = new DumbSlave(
-                "aNode",
-                new File(j.jenkins.getRootDir(), "agent-work-dirs/aNode").getAbsolutePath(),
-                new JNLPLauncher(true));
-        slave.setRetentionStrategy(RetentionStrategy.NOOP);
-        j.jenkins.addNode(slave);
-        await().pollInterval(250, TimeUnit.MILLISECONDS)
-                .atMost(10, TimeUnit.SECONDS)
-                .until(() -> slave.toComputer().getOfflineCause(), notNullValue());
+        Slave slave = inboundAgents.createAgent(j, InboundAgentRule.Options.newBuilder().name("aNode").skipStart().build());
         slave.toComputer().setTemporarilyOffline(true, null);
         assertThat(slave.toComputer().isOffline(), equalTo(true));
         assertThat(slave.toComputer().isTemporarilyOffline(), equalTo(true));
@@ -179,15 +170,7 @@ public void offlineNodeShouldSucceedOnOnlineNodeWithCause() throws Exception {
 
     @Test
     public void offlineNodeShouldSucceedOnOfflineNodeWithCause() throws Exception {
-        DumbSlave slave = new DumbSlave(
-                "aNode",
-                new File(j.jenkins.getRootDir(), "agent-work-dirs/aNode").getAbsolutePath(),
-                new JNLPLauncher(true));
-        slave.setRetentionStrategy(RetentionStrategy.NOOP);
-        j.jenkins.addNode(slave);
-        await().pollInterval(250, TimeUnit.MILLISECONDS)
-                .atMost(10, TimeUnit.SECONDS)
-                .until(() -> slave.toComputer().getOfflineCause(), notNullValue());
+        Slave slave = inboundAgents.createAgent(j, InboundAgentRule.Options.newBuilder().name("aNode").skipStart().build());
         slave.toComputer().setTemporarilyOffline(true, null);
         assertThat(slave.toComputer().isOffline(), equalTo(true));
         assertThat(slave.toComputer().isTemporarilyOffline(), equalTo(true));

test/src/test/java/hudson/slaves/AgentInboundUrlTest.java

@@ -25,16 +25,9 @@
 package hudson.slaves;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
 
 import com.gargoylesoftware.htmlunit.xml.XmlPage;
-import hudson.Util;
-import hudson.model.Computer;
-import hudson.model.Node;
 import hudson.model.Slave;
-import java.io.File;
-import java.util.ArrayList;
-import java.util.List;
 import java.util.logging.Level;
 import jenkins.model.Jenkins;
 import org.dom4j.Document;
@@ -43,6 +36,7 @@
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.FlagRule;
+import org.jvnet.hudson.test.InboundAgentRule;
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.LoggerRule;
@@ -55,6 +49,9 @@ public class AgentInboundUrlTest {
     @Rule
     public JenkinsRule j = new JenkinsRule();
 
+    @Rule
+    public InboundAgentRule inboundAgents = new InboundAgentRule();
+
     @Rule
     public LoggerRule logging = new LoggerRule().record(Slave.class, Level.FINE);
 
@@ -73,34 +70,14 @@ public void testInboundAgentUrlOverride() throws Exception {
         j.jenkins.setAuthorizationStrategy(authorizationStrategy);
 
         // Create an agent
-        addTestAgent();
+        inboundAgents.createAgent(j, InboundAgentRule.Options.newBuilder().name("test").skipStart().build());
 
         // parse the JNLP page into DOM to inspect the jnlp url argument.
         JenkinsRule.WebClient agent = j.createWebClient();
         XmlPage jnlp = (XmlPage) agent.goTo("computer/test/jenkins-agent.jnlp", "application/x-java-jnlp-file");
         Document dom = new DOMReader().read(jnlp.getXmlDocument());
-        Object arg = dom.selectSingleNode("//application-desc/argument[3]/following-sibling::argument[1]");
+        Object arg = dom.selectSingleNode("//application-desc/argument[7]/following-sibling::argument[1]");
         String val = ((Element) arg).getText();
         assertEquals(customInboundUrl, val);
     }
-
-    /**
-     * Adds an Inbound TCP agent to the system and returns it.
-     */
-    private void addTestAgent() throws Exception {
-        addTestAgent(new JNLPLauncher(false));
-    }
-
-    /**
-     * Adds an Inbound TCP agent to the system and returns it.
-     */
-    private void addTestAgent(ComputerLauncher launcher) throws Exception {
-        List<Node> agents = new ArrayList<>(j.jenkins.getNodes());
-        File dir = Util.createTempDir();
-        agents.add(new DumbSlave("test", "dummy", dir.getAbsolutePath(), "1", Node.Mode.NORMAL, "",
-                launcher, RetentionStrategy.INSTANCE, new ArrayList<>()));
-        j.jenkins.setNodes(agents);
-        Computer c = j.jenkins.getComputer("test");
-        assertNotNull(c);
-    }
 }

test/src/test/java/hudson/slaves/JNLPLauncherRealTest.java

@@ -29,21 +29,13 @@
 
 import hudson.ExtensionList;
 import hudson.PluginWrapper;
-import hudson.Proc;
 import hudson.model.FreeStyleBuild;
 import hudson.model.FreeStyleProject;
 import hudson.model.Slave;
 import hudson.util.FormValidation;
-import io.jenkins.lib.support_log_formatter.SupportLogFormatter;
 import java.io.File;
-import java.util.logging.ConsoleHandler;
-import java.util.logging.Handler;
-import java.util.logging.Level;
-import java.util.logging.Logger;
 import jenkins.agents.WebSocketAgentsTest;
 import jenkins.slaves.JnlpSlaveAgentProtocol4;
-import org.apache.commons.io.FileUtils;
-import org.apache.tools.ant.util.JavaEnvUtils;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.Description;
@@ -64,69 +56,51 @@ public class JNLPLauncherRealTest {
         /* Since RealJenkinsRuleInit.jpi will load detached plugins, to reproduce a failure use:
         FileUtils.touch(new File(rr.getHome(), "plugins/instance-identity.jpi.disabled"));
         */
-        rr.then(JNLPLauncherRealTest::_smokes);
+        rr.then(new ConnectStep(false));
     }
 
-    private static void _smokes(JenkinsRule r) throws Throwable {
+    private static class ConnectStep implements RealJenkinsRule.Step {
+
+      private final boolean webSocket;
+
+      private ConnectStep(boolean webSocket) {
+        this.webSocket = webSocket;
+      }
+
+      @Override
+      public void run(JenkinsRule r) throws Throwable {
         InboundAgentRule inboundAgents = new InboundAgentRule(); // cannot use @Rule since it would not be accessible from the controller JVM
         inboundAgents.apply(new Statement() {
             @Override public void evaluate() throws Throwable {
                 for (PluginWrapper plugin : r.jenkins.pluginManager.getPlugins()) {
                     System.err.println(plugin + " active=" + plugin.isActive() + " enabled=" + plugin.isEnabled());
                 }
-                assertThat(ExtensionList.lookupSingleton(JNLPLauncher.DescriptorImpl.class).doCheckWebSocket(false, null).kind, is(FormValidation.Kind.OK));
-                Slave agent = inboundAgents.createAgent(r, "static");
+                assertThat(ExtensionList.lookupSingleton(JNLPLauncher.DescriptorImpl.class).doCheckWebSocket(webSocket, null).kind, is(FormValidation.Kind.OK));
+                InboundAgentRule.Options.Builder builder = InboundAgentRule.Options.newBuilder().name("static").secret();
+                if (webSocket) {
+                    builder.webSocket();
+                }
+                Slave agent = inboundAgents.createAgent(r, builder.build());
+                r.waitOnline(agent);
                 FreeStyleProject p = r.createFreeStyleProject();
                 p.setAssignedNode(agent);
                 FreeStyleBuild b = r.buildAndAssertSuccess(p);
+                if (webSocket) {
+                    assertThat(agent.toComputer().getSystemProperties().get("java.class.path"), is(new File(r.jenkins.root, "agent.jar").getAbsolutePath()));
+                }
                 System.err.println(JenkinsRule.getLog(b));
             }
         }, Description.EMPTY).evaluate();
 
+      }
     }
 
     /**
      * Simplified version of {@link WebSocketAgentsTest#smokes} just checking Jetty/Winstone.
      */
     @Issue("JENKINS-68933")
     @Test public void webSocket() throws Throwable {
-        rr.then(JNLPLauncherRealTest::_webSocket);
-    }
-
-    private static void _webSocket(JenkinsRule r) throws Throwable {
-        // TODO RealJenkinsRule does not yet support LoggerRule
-        Handler handler = new ConsoleHandler();
-        handler.setFormatter(new SupportLogFormatter());
-        handler.setLevel(Level.FINE);
-        Logger logger = Logger.getLogger("jenkins.websocket");
-        logger.setLevel(Level.FINE);
-        logger.addHandler(handler);
-        assertThat(ExtensionList.lookupSingleton(JNLPLauncher.DescriptorImpl.class).doCheckWebSocket(true, null).kind, is(FormValidation.Kind.OK));
-        // TODO InboundAgentRule does not yet support WebSocket
-        JNLPLauncher launcher = new JNLPLauncher(true);
-        launcher.setWebSocket(true);
-        DumbSlave s = new DumbSlave("remote", new File(r.jenkins.root, "agent").getAbsolutePath(), launcher);
-        r.jenkins.addNode(s);
-        String secret = ((SlaveComputer) s.toComputer()).getJnlpMac();
-        File slaveJar = new File(r.jenkins.root, "agent.jar");
-        FileUtils.copyURLToFile(new Slave.JnlpJar("agent.jar").getURL(), slaveJar);
-        Proc proc = r.createLocalLauncher().launch().cmds(
-            JavaEnvUtils.getJreExecutable("java"), "-jar", slaveJar.getAbsolutePath(),
-            "-jnlpUrl", r.getURL() + "computer/remote/jenkins-agent.jnlp",
-            "-secret", secret
-        ).stdout(System.out).start();
-        try {
-            FreeStyleProject p = r.createFreeStyleProject();
-            p.setAssignedNode(s);
-            r.buildAndAssertSuccess(p);
-            assertThat(s.toComputer().getSystemProperties().get("java.class.path"), is(slaveJar.getAbsolutePath()));
-        } finally {
-            proc.kill();
-            while (r.jenkins.getComputer("remote").isOnline()) {
-                System.err.println("waiting for computer to go offline");
-                Thread.sleep(250);
-            }
-        }
+        rr.then(new ConnectStep(true));
     }
 
 }