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.

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

[JENKINS-48761] - Restore binary compatibility with agents running old Remoting versions #3212

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1,6 @@
# Remoting version, which is embedded into the core
# This version MAY differ from what is really classloaded (see the "Pluggable Core Components", JENKINS-41196)
remoting.embedded.version=${remoting.version}
Copy link
Member

Choose a reason for hiding this comment

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

Or this could be computed from remoting*.jar!/META-INF/MANIFEST.MF#Version using Which.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would prefer to keep it as is.

  • At some point we may want to write a "Remoting Compatibility Tester" tool or so
  • Not sure how which is going to behave once we have pluggable core components in place


# Minimal Remoting version on external agents which is supported by the core
remoting.minimal.supported.version=${remoting.minimal.supported.version}
Copy link
Member

Choose a reason for hiding this comment

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

s/minimal/minimum/g

8 changes: 6 additions & 2 deletions core/src/main/java/hudson/Launcher.java
Expand Up @@ -1289,7 +1289,7 @@ private static class RemoteLaunchCallable extends MasterToSlaveCallable<RemotePr
}

public RemoteProcess call() throws IOException {
final Channel channel = getOpenChannelOrFail();
final Channel channel = _getOpenChannelOrFail();
Launcher.ProcStarter ps = new LocalLauncher(listener).launch();
ps.cmds(cmd).masks(masks).envs(env).stdin(in).stdout(out).stderr(err).quiet(quiet);
if(workDir!=null) ps.pwd(workDir);
Expand All @@ -1308,7 +1308,11 @@ public int join() throws InterruptedException, IOException {
Channel taskChannel = null;
try {
// Sync IO will fail automatically if the channel is being closed, no need to use getOpenChannelOrFail()
taskChannel = Channel.currentOrFail();
// TODOL Replace by Channel#currentOrFail() when Remoting version allows
Copy link
Member

Choose a reason for hiding this comment

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

typo

taskChannel = Channel.current();
if (taskChannel == null) {
throw new IOException("No Remoting channel associated with this thread");
}
taskChannel.syncIO();
} catch (Throwable t) {
// this includes a failure to sync, agent.jar too old, etc
Expand Down
1 change: 1 addition & 0 deletions core/src/main/java/hudson/TcpSlaveAgentListener.java
Expand Up @@ -290,6 +290,7 @@ private void respondHello(String header, Socket s) throws IOException {
try {
Writer o = new OutputStreamWriter(s.getOutputStream(), "UTF-8");

//TODO: expose version about minimal supported Remoting version (JENKINS-48766)
if (header.startsWith("GET / ")) {
o.write("HTTP/1.0 200 OK\r\n");
o.write("Content-Type: text/plain;charset=UTF-8\r\n");
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/model/Computer.java
Expand Up @@ -1426,7 +1426,7 @@ public void doDumpExportTable( StaplerRequest req, StaplerResponse rsp ) throws

private static final class DumpExportTableTask extends MasterToSlaveCallable<String,IOException> {
public String call() throws IOException {
final Channel ch = getChannelOrFail();
final Channel ch = _getChannelOrFail();
StringWriter sw = new StringWriter();
try (PrintWriter pw = new PrintWriter(sw)) {
ch.dumpExportTable(pw);
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/hudson/slaves/ChannelPinger.java
Expand Up @@ -134,7 +134,7 @@ public void install(Channel channel) {
@Override
public Void call() throws IOException {
// No sense in setting up channel pinger if the channel is being closed
setUpPingForChannel(getOpenChannelOrFail(), null, pingTimeoutSeconds, pingIntervalSeconds, false);
setUpPingForChannel(_getOpenChannelOrFail(), null, pingTimeoutSeconds, pingIntervalSeconds, false);
return null;
}

Expand Down
7 changes: 6 additions & 1 deletion core/src/main/java/hudson/slaves/SlaveComputer.java
Expand Up @@ -38,6 +38,7 @@
import hudson.model.User;
import hudson.remoting.Channel;
import hudson.remoting.ChannelBuilder;
import hudson.remoting.ChannelClosedException;
import hudson.remoting.Launcher;
import hudson.remoting.VirtualChannel;
import hudson.security.ACL;
Expand Down Expand Up @@ -867,7 +868,11 @@ public Void call() {
// ignore this error.
}

Channel.currentOrFail().setProperty("slave",Boolean.TRUE); // indicate that this side of the channel is the slave side.
try {
_getChannelOrFail().setProperty("slave",Boolean.TRUE); // indicate that this side of the channel is the slave side.
} catch (ChannelClosedException e) {
throw new IllegalStateException(e);
}

return null;
}
Expand Down
29 changes: 28 additions & 1 deletion core/src/main/java/jenkins/security/MasterToSlaveCallable.java
@@ -1,7 +1,13 @@
package jenkins.security;

import hudson.remoting.Callable;
import hudson.remoting.Channel;
import hudson.remoting.ChannelClosedException;
import org.jenkinsci.remoting.RoleChecker;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import javax.annotation.Nonnull;


/**
Expand All @@ -11,10 +17,31 @@
* @since 1.587 / 1.580.1
*/
public abstract class MasterToSlaveCallable<V, T extends Throwable> implements Callable<V,T> {

private static final long serialVersionUID = 1L;

@Override
public void checkRoles(RoleChecker checker) throws SecurityException {
checker.check(this,Roles.SLAVE);
}

private static final long serialVersionUID = 1L;
//TODO: replace by Callable#getChannelOrFail() once Minimal supported Remoting version is 3.15 or above
@Restricted(NoExternalUse.class)
Copy link
Member

Choose a reason for hiding this comment

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

I think you could do something like

@Override
public Channel getChannelOrFail() throws ChannelStateException {
    // copy implementation from Callable
}

which would avoid polluting the API with even a restricted function, while still fixing the problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was not sure it's 100% safe for defaults and decided not to try due to the urgency. Can try it now since I have a test which should reveal the problem if any

Copy link
Member

Choose a reason for hiding this comment

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

Not at all urgent, just an idea which might simplify the patch.

protected static Channel _getChannelOrFail() throws ChannelClosedException {
final Channel ch = Channel.current();
if (ch == null) {
throw new ChannelClosedException(new IllegalStateException("No channel associated with the thread"));
}
return ch;
}

//TODO: replace by Callable#getOpenChannelOrFail() once Minimal supported Remoting version is 3.15 or above
@Restricted(NoExternalUse.class)
protected static Channel _getOpenChannelOrFail() throws ChannelClosedException {
final Channel ch = _getChannelOrFail();
if (ch.isClosingOrClosed()) { // TODO: Since Remoting 2.33, we still need to explicitly declare minimal Remoting version
throw new ChannelClosedException(new IllegalStateException("The associated channel " + ch + " is closing down or has closed down", ch.getCloseRequestCause()));
}
return ch;
}
}
104 changes: 104 additions & 0 deletions core/src/main/java/jenkins/slaves/RemotingVersionInfo.java
@@ -0,0 +1,104 @@
/*
* The MIT License
*
* Copyright (c) 2018, CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
package jenkins.slaves;

import hudson.util.VersionNumber;
import org.kohsuke.accmod.Restricted;
import org.kohsuke.accmod.restrictions.NoExternalUse;

import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
import java.io.IOException;
import java.io.InputStream;
import java.util.Properties;
import java.util.logging.Level;
import java.util.logging.Logger;

// TODO: Make the API public (JENKINS-48766)
/**
* Provides information about Remoting versions used withing the core.
Copy link
Member

Choose a reason for hiding this comment

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

typo

* @author Oleg Nenashev
* @since TODO
Copy link
Member

Choose a reason for hiding this comment

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

Restricted, so delete.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a TODO for making it public above

Copy link
Member

Choose a reason for hiding this comment

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

So if and when that happens, you will add a @since with that version, as with adding any new API. We should only have @since TODO comments in the code base for those things which we have forgotten to fill in after merging.

*/
@Restricted(NoExternalUse.class)
public class RemotingVersionInfo {

private static final Logger LOGGER = Logger.getLogger(RemotingVersionInfo.class.getName());
private static final String RESOURCE_NAME="remoting-info.properties";

@CheckForNull
private static VersionNumber EMBEDDED_VERSION;

@CheckForNull
private static VersionNumber MINIMAL_SUPPORTED_VERSION;

private RemotingVersionInfo() {}

static {
Properties props = new Properties();
try (InputStream is = RemotingVersionInfo.class.getResourceAsStream(RESOURCE_NAME)) {
if(is!=null) {
Copy link
Member

Choose a reason for hiding this comment

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

No need to check for null. If the file is missing, core is corrupt, so die now.

Copy link
Member Author

Choose a reason for hiding this comment

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

The implementation is similar to jenkins.model.Jenkins's appoarch to reading jenkins-version.properties. I agree this should not be a problem, but there may be edge cases (e.g. custom packaging). I see no problem with having a fault-tolerant code since it is a regression fix

Copy link
Member

Choose a reason for hiding this comment

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

In the case of custom packaging, it is the responsibility of the packager to not make mistakes. If they do, their WAR will be broken. We should not engineer in robustness for something that is not a user error and is trivially detected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, another potential use-case is calling this class from the agent. I doubt we would be dare enough to send Jenkins core over Remoting. Do we do that in the case of such requests? I hope not

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's keep it as is. I will improve checks when exposing it to public API

props.load(is);
}
} catch (IOException e) {
LOGGER.log(Level.WARNING, "Failed to load Remoting Info from " + RESOURCE_NAME, e);
Copy link
Member

Choose a reason for hiding this comment

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

Ditto—just rethrow as ExceptionInInitializerError.

}

EMBEDDED_VERSION = tryExtractVersion(props, "remoting.embedded.version");
MINIMAL_SUPPORTED_VERSION = tryExtractVersion(props, "remoting.minimal.supported.version");
}

@CheckForNull
private static VersionNumber tryExtractVersion(@Nonnull Properties props, @Nonnull String propertyName) {
String prop = props.getProperty(propertyName);
if (prop == null) {
LOGGER.log(Level.FINE, "Property {0} is not defined in {1}", new Object[] {propertyName, RESOURCE_NAME});
Copy link
Member

Choose a reason for hiding this comment

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

Throw an IllegalStateException.

return null;
}

if(prop.contains("${")) { // Due to whatever reason, Maven does not nullify them
LOGGER.log(Level.WARNING, "Property {0} in {1} has unresolved variable(s). Raw value: {2}",
Copy link
Member

Choose a reason for hiding this comment

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

ditto

new Object[] {propertyName, RESOURCE_NAME, prop});
return null;
}

try {
return new VersionNumber(prop);
} catch (RuntimeException ex) {
LOGGER.log(Level.WARNING, String.format("Failed to parse version for for property %s in %s. Raw Value: %s",
Copy link
Member

Choose a reason for hiding this comment

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

Ditto. We are in full control of this file, so there is no reason to tolerate any errors.

propertyName, RESOURCE_NAME, prop), ex);
return null;
}
}

@CheckForNull
Copy link
Member

Choose a reason for hiding this comment

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

@Nonnull since we know these will be defined.

public static VersionNumber getEmbeddedVersion() {
return EMBEDDED_VERSION;
}

@CheckForNull
public static VersionNumber getMinimalSupportedVersion() {
return MINIMAL_SUPPORTED_VERSION;
}
}
Expand Up @@ -39,7 +39,7 @@ public void preOnline(Computer c, Channel channel, FilePath root, TaskListener l
private static final class ChannelSwapper extends MasterToSlaveCallable<Boolean,Exception> {
public Boolean call() throws Exception {
if (File.pathSeparatorChar==';') return false; // Windows
Channel c = getOpenChannelOrFail();
Channel c = _getOpenChannelOrFail();
StandardOutputStream sos = (StandardOutputStream) c.getProperty(StandardOutputStream.class);
if (sos!=null) {
swap(sos);
Expand Down
47 changes: 47 additions & 0 deletions core/src/test/java/jenkins/slaves/RemotingVersionInfoTest.java
@@ -0,0 +1,47 @@
/*
* The MIT License
*
* Copyright (c) 2018, CloudBees, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*/
package jenkins.slaves;

import org.junit.Test;

import static org.hamcrest.CoreMatchers.*;
import static org.junit.Assert.assertThat;

/**
* Tests for {@link RemotingVersionInfo}.
*/
public class RemotingVersionInfoTest {

@Test
public void shouldLoadEmbeddedVersionByDefault() {
assertThat("Remoting Embedded version is not defined",
RemotingVersionInfo.getEmbeddedVersion(), notNullValue());
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary if they are just @Nonnull to begin with, since other tests would die if someone screwed this up.

}

@Test
public void shouldLoadMinimalSupportedVersionByDefault() {
assertThat("Remoting Minimal supported version is not defined",
RemotingVersionInfo.getMinimalSupportedVersion(), notNullValue());
}
}
6 changes: 5 additions & 1 deletion pom.xml
Expand Up @@ -104,6 +104,10 @@ THE SOFTWARE.

<maven-war-plugin.version>3.0.0</maven-war-plugin.version> <!-- JENKINS-47127 bump when 3.2.0 is out. Cf. MWAR-407 -->

<!-- Minimal Remoting version, which is tested for API compatibility -->
Copy link
Member Author

Choose a reason for hiding this comment

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

Just picked first version with excessive stability improvements and available changelog: https://github.com/jenkinsci/remoting/blob/master/CHANGELOG.md#260 . This version has been released on June 10, 2016 => old enough.

IMHO the version should be bumped to 3.0 once https://issues.jenkins-ci.org/browse/JENKINS-48766 is implemented. It would unblock #3188 from @Vlatombe

<remoting.version>3.15</remoting.version>
<remoting.minimal.supported.version>2.60</remoting.minimal.supported.version>

</properties>

<!-- Note that the 'repositories' and 'pluginRepositories' blocks below are actually copy-pasted
Expand Down Expand Up @@ -175,7 +179,7 @@ THE SOFTWARE.
<dependency>
<groupId>org.jenkins-ci.main</groupId>
<artifactId>remoting</artifactId>
<version>3.15</version>
<version>${remoting.version}</version>
</dependency>

<dependency>
Expand Down
25 changes: 25 additions & 0 deletions test/pom.xml
Expand Up @@ -224,6 +224,31 @@ THE SOFTWARE.
<!-- version specified in grandparent pom -->
<extensions>true</extensions>
</plugin>
<plugin>
<artifactId>maven-dependency-plugin</artifactId>
<executions>
<execution>
<id>old-remoting-for-test</id>
<phase>generate-test-resources</phase>
<goals>
<!-- we use copy as this is a dependency from outside the reactor -->
<goal>copy</goal>
</goals>
<configuration>
<artifactItems>
<artifactItem>
<groupId>org.jenkins-ci.main</groupId>
<artifactId>remoting</artifactId>
<version>${remoting.minimal.supported.version}</version>
<type>jar</type>
<outputDirectory>${project.build.outputDirectory}/old-remoting</outputDirectory>
<destFileName>remoting-minimal-supported.jar</destFileName>
Copy link
Member

Choose a reason for hiding this comment

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

BTW may want to s/minimal/minimum/ here too for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will address it in a follow-up

</artifactItem>
</artifactItems>
</configuration>
</execution>
</executions>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
Expand Down