The docker-java library is not a direct dependency of the docker plugin but is pulled in transitively through the docker-java-api plugin. Would it make more sense to whitelist the classes in that plugin instead if they can be safely serialized?

Note that the GitHub issue was reported a few days before docker-plugin:1.1.3 was released which included some significant changes to the classes in the stack trace, so I think this issue should be reinvestigated in 1.1.3. In particular I think that 4ae1f17 and/or 5a2a123 might eliminate the specific error in the description, or at least will alter where the error is thrown.

I think that whitelisting the classes in the com.github.dockerjava.api.model package is probably ok since they have been marked as serializable in the library they come from and all appear to be simple wrappers, but serializing the 2 commands in the com.github.dockerjava.api.command package seem like an accident where an inner class picks up a parameter or local variable. If the commands are fixed than I think that would eliminate all of the places where the model classes are serialized as well (but am not 100% sure, there might be some uses I missed).

> The docker-java library is not a direct dependency of the docker plugin but is pulled in transitively through the docker-java-api plugin. Would it make more sense to whitelist the classes in that plugin instead if they can be safely serialized?

This library is generally a risky thing, because it does not consistently retain binary compatibility. That's why it has been originally shaded in plugins like Docker, Yet Another Docker Plugin, Docker Traceability, etc. So a user of such API library may find his plugin broken by a library update. Maybe the situation became better over years, integer improved Docker Java's lifecycle a lot

Apart from that, +1 for moving whitelist to a library in the current state.

> I think that whitelisting the classes in the com.github.dockerjava.api.model package is probably ok

I have reviewed all classes, and yes they are OK in the current version.

OTOH the whitelist does not seem to be enough. com.github.dockerjava.api.command.InspectContainerResponse$Node is not whitelisted. The serialization of InspectContainerResponse may fail if the field is not null

> but serializing the 2 commands in the com.github.dockerjava.api.command package seem like an accident where an inner class picks up a parameter or local variable

Yes, InspectContainerResponse$ContainerState and InspectContainerResponse$Node inner classes are not static. Making them static would break binary compatibility. It may be acceptable for Docker Java tho.

Probably it's fine to keep it whitelisted. E.g. InspectContainerResponse is also explicitly persisted in the Docker Traceability plugin (within https://github.com/jenkinsci/docker-traceability-plugin/blob/49141a86d41269799e00161a02ac72e9aa9a3a15/docker-traceability-api/src/main/java/org/jenkinsci/plugins/docker/traceability/api/DockerTraceabilityReport.java#L51). This does seem to be a valid use-case for serialization though it makes the plugin affected by JEP-200 for sure. I will create a ticket

BTW DockerComputerJNLPConnector$1#val$inspect sounds like it will trigger some anonymous inner class warning in new cores, too (and rightly).

I cannot find any such class in the master version of the plugin, or for that matter in the current 1.1.3 release. Which plugin version did the abovementioned stack trace come from? The PR seems to be fixing some problem without demonstrating that problem in a test case, so it is difficult to evaluate the fix.

The mentioned class seems to come from 1.1.2 or earlier, according to Git history. So, what errors, if any, are thrown from 1.1.3?

Mind you, io.jenkins.docker.connector.DockerComputerConnector$1 remains a mess, and current Jenkins cores will warn that it should factored into a static nested class with consciously selected fields. But it does not seem to serialize anything from com.github.dockerjava.api according to code inspection.

Of course there may well be plenty of other JEP-200-related errors in this plugin. Which is why we need to check if there are existing functional tests which exercise sufficiently realistic use cases to turn up these errors when run against 2.107.x, or if new ones need to be written.

See PR 641: the test suite seems to reproduce the warning, but it does not cause a test failure.

Going by

diff --git a/src/main/java/io/jenkins/docker/DockerTransientNode.java b/src/main/java/io/jenkins/docker/DockerTransientNode.java
index eb3cf8c..3ee4916 100644
--- a/src/main/java/io/jenkins/docker/DockerTransientNode.java
+++ b/src/main/java/io/jenkins/docker/DockerTransientNode.java
@@ -84,6 +84,10 @@ public class DockerTransientNode extends Slave {
     }
 
     public void terminate(TaskListener listener) {
+        if (this != null) {
+            System.err.println("===> would terminate now");
+            return;
+        }
         try {
             toComputer().disconnect(new DockerOfflineCause());
             listener.getLogger().println("Disconnected computer");
diff --git a/src/test/java/io/jenkins/docker/connector/DockerComputerJNLPConnectorTest.java b/src/test/java/io/jenkins/docker/connector/DockerComputerJNLPConnectorTest.java
index 7c3bda3..0e7633f 100644
--- a/src/test/java/io/jenkins/docker/connector/DockerComputerJNLPConnectorTest.java
+++ b/src/test/java/io/jenkins/docker/connector/DockerComputerJNLPConnectorTest.java
@@ -41,6 +41,7 @@ public class DockerComputerJNLPConnectorTest extends DockerComputerConnectorTest
         }
 
         should_connect_agent(template);
+        Thread.sleep(Long.MAX_VALUE);
     }
 

it seems that the agent is in fact launched, and the build succeeds, despite the fact that the agent configuration is not stored on disk. If true, the problem could perhaps be reproduced using RestartableJenkinsRule with a WorkflowJob; it seems that there is no Pipeline test of DockerCloud at all (only DockerNodeStepTest, unrelated). This could arguably be considered a bug in Jenkins core: Nodes.addNode throws up any exception from persistNode, but then neglects to remove the faulty node from its list, whereas you would expect Jenkins.addNode to succeed or fail atomically.

Of course the bug should also be reproducible by using LoggerRule in the existing test to verify that there are no warnings (at least, about class-filter), but this is less satisfying since it would not catch other JEP-200 bugs in other tests. We would prefer for the Node serialization failure to cause a natural test failure.

I apologize for duplicating comment from https://github.com/jenkinsci/docker-plugin/pull/641, but I'm getting a bit lost among JIRA and GitHub issues and pull requests.

This is the exception trace we get with Jenkins 2.107.1.2 and docker-plugin 1.1.3 when the plugin tries to provision new docker agent

Mar 29, 2018 1:17:45 PM hudson.slaves.NodeProvisioner$2 run
WARNING: Unexpected exception encountered while provisioning agent Image of containerhub:6666/jenkins-agent:latest
java.lang.RuntimeException: java.io.IOException: java.lang.RuntimeException: Failed to serialize hudson.model.Slave#launcher for class com.nirima.jenkins.plugins.docker.DockerSlave
        at com.google.common.base.Throwables.propagate(Throwables.java:156)
        at com.nirima.jenkins.plugins.docker.DockerCloud$1.call(DockerCloud.java:304)
        at com.nirima.jenkins.plugins.docker.DockerCloud$1.call(DockerCloud.java:296)
        at jenkins.util.ContextResettingExecutorService$2.call(ContextResettingExecutorService.java:46)
        at java.util.concurrent.FutureTask.run(FutureTask.java:266)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
        at java.lang.Thread.run(Thread.java:748)
Caused by: java.io.IOException: java.lang.RuntimeException: Failed to serialize hudson.model.Slave#launcher for class com.nirima.jenkins.plugins.docker.DockerSlave
        at hudson.XmlFile.write(XmlFile.java:200)
        at jenkins.model.Nodes.save(Nodes.java:274)
        at hudson.util.PersistedList.onModified(PersistedList.java:173)
        at hudson.util.PersistedList.replaceBy(PersistedList.java:85)
        at hudson.model.Slave.<init>(Slave.java:198)
        at hudson.slaves.AbstractCloudSlave.<init>(AbstractCloudSlave.java:51)
        at com.nirima.jenkins.plugins.docker.DockerSlave.<init>(DockerSlave.java:62)
        at com.nirima.jenkins.plugins.docker.DockerCloud.provisionFromTemplate(DockerCloud.java:406)
        at com.nirima.jenkins.plugins.docker.DockerCloud.access$000(DockerCloud.java:81)
        at com.nirima.jenkins.plugins.docker.DockerCloud$1.call(DockerCloud.java:300)
        ... 6 more
Caused by: java.lang.RuntimeException: Failed to serialize hudson.model.Slave#launcher for class com.nirima.jenkins.plugins.docker.DockerSlave
        at hudson.util.RobustReflectionConverter$2.writeField(RobustReflectionConverter.java:256)
        at hudson.util.RobustReflectionConverter$2.visit(RobustReflectionConverter.java:224)
        at com.thoughtworks.xstream.converters.reflection.PureJavaReflectionProvider.visitSerializableFields(PureJavaReflectionProvider.java:138)
        at hudson.util.RobustReflectionConverter.doMarshal(RobustReflectionConverter.java:209)
        at hudson.util.RobustReflectionConverter.marshal(RobustReflectionConverter.java:150)
        at com.thoughtworks.xstream.core.AbstractReferenceMarshaller.convert(AbstractReferenceMarshaller.java:69)
        at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:58)
        at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:43)
        at com.thoughtworks.xstream.core.TreeMarshaller.start(TreeMarshaller.java:82)
        at com.thoughtworks.xstream.core.AbstractTreeMarshallingStrategy.marshal(AbstractTreeMarshallingStrategy.java:37)
        at com.thoughtworks.xstream.XStream.marshal(XStream.java:1026)
        at com.thoughtworks.xstream.XStream.marshal(XStream.java:1015)
        at com.thoughtworks.xstream.XStream.toXML(XStream.java:988)
        at hudson.XmlFile.write(XmlFile.java:193)
        ... 15 more
Caused by: java.lang.RuntimeException: Failed to serialize io.jenkins.docker.connector.DockerComputerJNLPConnector$1#val$inspect for class io.jenkins.docker.connector.DockerComputerJNLPConnector$1
        at hudson.util.RobustReflectionConverter$2.writeField(RobustReflectionConverter.java:256)
        at hudson.util.RobustReflectionConverter$2.visit(RobustReflectionConverter.java:224)
        at com.thoughtworks.xstream.converters.reflection.PureJavaReflectionProvider.visitSerializableFields(PureJavaReflectionProvider.java:138)
        at hudson.util.RobustReflectionConverter.doMarshal(RobustReflectionConverter.java:209)
        at hudson.util.RobustReflectionConverter.marshal(RobustReflectionConverter.java:150)
        at com.thoughtworks.xstream.core.AbstractReferenceMarshaller.convert(AbstractReferenceMarshaller.java:69)
        at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:58)
        at com.thoughtworks.xstream.core.AbstractReferenceMarshaller$1.convertAnother(AbstractReferenceMarshaller.java:84)
        at hudson.util.RobustReflectionConverter.marshallField(RobustReflectionConverter.java:265)
        at hudson.util.RobustReflectionConverter$2.writeField(RobustReflectionConverter.java:252)
        ... 28 more
Caused by: java.lang.UnsupportedOperationException: Refusing to marshal com.github.dockerjava.api.command.InspectContainerResponse for security reasons; see https://jenkins.io/redirect/class-filter/
        at hudson.util.XStream2$BlacklistedTypesConverter.marshal(XStream2.java:543)
        at com.thoughtworks.xstream.core.AbstractReferenceMarshaller.convert(AbstractReferenceMarshaller.java:69)
        at com.thoughtworks.xstream.core.TreeMarshaller.convertAnother(TreeMarshaller.java:58)
        at com.thoughtworks.xstream.core.AbstractReferenceMarshaller$1.convertAnother(AbstractReferenceMarshaller.java:84)
        at hudson.util.RobustReflectionConverter.marshallField(RobustReflectionConverter.java:265)
        at hudson.util.RobustReflectionConverter$2.writeField(RobustReflectionConverter.java:252)
        ... 37 more

Sorry, we have got a split-brain in the discussions.
We will sync-up with dnusbaum jglick ndeloof and then summarize the discussions

Seems I have fired a false alarm. We assemble docker image with Jenkins Master and our script leaves libraries from docker-plugin 1.0.4 on the classpath.
I'll correct the script and will re-test tomorrow.

It was a mistake on my side : old libraries of the plugin were still available. Clean installation of 1.1.3 (and upgrading its dependencies) resolved the problem.
Though now I'm not sure about the purpose of https://github.com/jenkinsci/docker-plugin/pull/619, which was merged after 1.1.3 is released

sadovnikov this PR has been reverted, so 1.1.4 won't include this code anyway

Version 1.1.3 and newer should not be affected by this issue, so I am marking the ticket as resolved.