Skip to content

Commit

Permalink
[JENKINS-39465] - Fix the AgentProtocol settings persistency handling (…
Browse files Browse the repository at this point in the history 
…#2621)

* [JENKINS-39465] - Tweak processing of enabled and disabled protocols in Jenkins instance

Due to whatever reason, without a definition of an array recipient field the data goes to the disk in the following way:

```
<enabledAgentProtocol>JNLP3-connect</enabledAgentProtocol>
<enabledAgentProtocol>JNLP4-connect</enabledAgentProtocol>
```

It is supposed to processed by Implicit array correctly, but it does not actually happen.
With a fix the data is being stored in another format:

```
  <enabledAgentProtocols>
    <string>JNLP3-connect</string>
    <string>JNLP4-connect</string>
  </enabledAgentProtocols>
```

This data now works correctly and gets deserialized correctly. readResolve() just adds a fallback for the case when Implicit array handling starts behaving correctly (?).

* [JENKINS-39465] - Add configuration roundtrip tests

* [JENKINS-39465] - Jenkins#agentProtocols cache must be invalidated when we reload the configuration

* [JENKINS-39465] - Remove obsolete comment from Tests
  • Loading branch information
@oleg-nenashev
oleg-nenashev committed Nov 7, 2016
1 parent 5aed788 commit 3e2e017
Show file tree
Hide file tree
Showing 2 changed files with 143 additions and 2 deletions.
24 changes: 22 additions & 2 deletions core/src/main/java/jenkins/model/Jenkins.java
View file
Expand Up @@ -617,6 +617,11 @@ private static int getSlaveAgentPortInitialValue(int def) {
*/
@CheckForNull
private List<String> disabledAgentProtocols;
/**
* @deprecated Just a temporary buffer for XSTream migration code from JENKINS-39465, do not use
*/
@Deprecated
private transient String[] _disabledAgentProtocols;

/**
* The TCP agent protocols that are {@link AgentProtocol#isOptIn()} and explicitly enabled.
Expand All @@ -626,6 +631,11 @@ private static int getSlaveAgentPortInitialValue(int def) {
*/
@CheckForNull
private List<String> enabledAgentProtocols;
/**
* @deprecated Just a temporary buffer for XSTream migration code from JENKINS-39465, do not use
*/
@Deprecated
private transient String[] _enabledAgentProtocols;

/**
* The TCP agent protocols that are enabled. Built from {@link #disabledAgentProtocols} and
Expand Down Expand Up @@ -1010,6 +1020,16 @@ private Object readResolve() {
if (SLAVE_AGENT_PORT_ENFORCE) {
slaveAgentPort = getSlaveAgentPortInitialValue(slaveAgentPort);
}
if (disabledAgentProtocols == null && _disabledAgentProtocols != null) {
disabledAgentProtocols = Arrays.asList(_disabledAgentProtocols);
_disabledAgentProtocols = null;
}
if (enabledAgentProtocols == null && _enabledAgentProtocols != null) {
enabledAgentProtocols = Arrays.asList(_enabledAgentProtocols);
_enabledAgentProtocols = null;
}
// Invalidate the protocols cache after the reload
agentProtocols = null;
return this;
}

Expand Down Expand Up @@ -5027,8 +5047,8 @@ private static void computeVersion(ServletContext context) {
// for backward compatibility with <1.75, recognize the tag name "view" as well.
XSTREAM.alias("view", ListView.class);
XSTREAM.alias("listView", ListView.class);
XSTREAM.addImplicitCollection(Jenkins.class, "disabledAgentProtocols", "disabledAgentProtocol", String.class);
XSTREAM.addImplicitCollection(Jenkins.class, "enabledAgentProtocols", "enabledAgentProtocol", String.class);
XSTREAM.addImplicitArray(Jenkins.class, "_disabledAgentProtocols", "disabledAgentProtocol");
XSTREAM.addImplicitArray(Jenkins.class, "_enabledAgentProtocols", "enabledAgentProtocol");
XSTREAM2.addCriticalField(Jenkins.class, "securityRealm");
XSTREAM2.addCriticalField(Jenkins.class, "authorizationStrategy");
// this seems to be necessary to force registration of converter early enough
Expand Down
121 changes: 121 additions & 0 deletions test/src/test/java/jenkins/model/JenkinsTest.java
View file
Expand Up @@ -25,7 +25,10 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasItem;
import static org.hamcrest.Matchers.not;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertEquals;
Expand Down Expand Up @@ -75,6 +78,12 @@
import java.io.IOException;
import java.net.HttpURLConnection;
import java.net.URL;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.Iterator;
import java.util.Set;
import org.junit.Assume;

/**
* @author kingfai
Expand Down Expand Up @@ -471,4 +480,116 @@ public void onOnline(Computer c, TaskListener listener) throws IOException, Inte
throw new IOException("Something happened (the listener always throws this exception)");
}
}

@Test
@Issue("JENKINS-39465")
public void agentProtocols_singleEnable_roundtrip() throws Exception {
final Set<String> defaultProtocols = Collections.unmodifiableSet(j.jenkins.getAgentProtocols());
Assume.assumeThat("We assume that JNLP3-connect is disabled",
defaultProtocols, not(hasItem("JNLP3-connect")));

final Set<String> newProtocols = new HashSet<>(defaultProtocols);
newProtocols.add("JNLP3-connect");
j.jenkins.setAgentProtocols(newProtocols);
j.jenkins.save();
final Set<String> agentProtocolsBeforeReload = j.jenkins.getAgentProtocols();
assertThat("JNLP3-connect must be enabled before the roundtrip",
j.jenkins.getAgentProtocols(), hasItem("JNLP3-connect"));

j.jenkins.reload();

final Set<String> reloadedProtocols = j.jenkins.getAgentProtocols();
assertFalse("The protocol list must have been really reloaded", agentProtocolsBeforeReload == reloadedProtocols);
assertThat("We should have additional enabled protocol",
reloadedProtocols.size(), equalTo(defaultProtocols.size() + 1));
assertThat("JNLP3-connect must be enabled after the roundtrip",
reloadedProtocols, hasItem("JNLP3-connect"));
}

@Test
@Issue("JENKINS-39465")
public void agentProtocols_multipleDisable_roundtrip() throws Exception {
final Set<String> defaultProtocols = Collections.unmodifiableSet(j.jenkins.getAgentProtocols());
Assume.assumeThat("At least one protocol is enabled", defaultProtocols.size(), greaterThan(0));

final String protocolToDisable = defaultProtocols.iterator().next();

final Set<String> newProtocols = new HashSet<>(defaultProtocols);
newProtocols.remove(protocolToDisable);
j.jenkins.setAgentProtocols(newProtocols);
j.jenkins.save();
assertThat(protocolToDisable + " must be disabled before the roundtrip",
j.jenkins.getAgentProtocols(), not(hasItem(protocolToDisable)));
final Set<String> agentProtocolsBeforeReload = j.jenkins.getAgentProtocols();
j.jenkins.reload();

assertFalse("The protocol list must have been really refreshed", agentProtocolsBeforeReload == j.jenkins.getAgentProtocols());
assertThat("We should have disabled one protocol",
j.jenkins.getAgentProtocols().size(), equalTo(defaultProtocols.size() - 1));
assertThat(protocolToDisable + " must be disabled after the roundtrip",
j.jenkins.getAgentProtocols(), not(hasItem(protocolToDisable)));
}

@Test
@Issue("JENKINS-39465")
public void agentProtocols_multipleEnable_roundtrip() throws Exception {
final Set<String> defaultProtocols = Collections.unmodifiableSet(j.jenkins.getAgentProtocols());
Assume.assumeThat("We assume that JNLP3-connect is disabled",
defaultProtocols, not(hasItem("JNLP3-connect")));
Assume.assumeThat("We assume that JNLP4-connect is disabled",
defaultProtocols, not(hasItem("JNLP4-connect")));

final Set<String> newProtocols = new HashSet<>(defaultProtocols);
newProtocols.add("JNLP3-connect");
newProtocols.add("JNLP4-connect");
j.jenkins.setAgentProtocols(newProtocols);
j.jenkins.save();
final Set<String> agentProtocolsBeforeReload = j.jenkins.getAgentProtocols();
assertThat("JNLP3-connect must be enabled before the roundtrip",
j.jenkins.getAgentProtocols(), hasItem("JNLP3-connect"));
assertThat("JNLP4-connect must be enabled before the roundtrip",
j.jenkins.getAgentProtocols(), hasItem("JNLP4-connect"));

j.jenkins.reload();

final Set<String> reloadedProtocols = j.jenkins.getAgentProtocols();
assertFalse("The protocol list must have been really reloaded", agentProtocolsBeforeReload == reloadedProtocols);
assertThat("We should have two additional enabled protocols",
reloadedProtocols.size(), equalTo(defaultProtocols.size() + 2));
assertThat("JNLP3-connect must be enabled after the roundtrip",
reloadedProtocols, hasItem("JNLP3-connect"));
assertThat("JNLP3-connect must be enabled after the roundtrip",
reloadedProtocols, hasItem("JNLP4-connect"));
}

@Test
@Issue("JENKINS-39465")
public void agentProtocols_singleDisable_roundtrip() throws Exception {
final Set<String> defaultProtocols = Collections.unmodifiableSet(j.jenkins.getAgentProtocols());
Assume.assumeThat("At least two protocol should be enabled", defaultProtocols.size(), greaterThan(1));

Iterator<String> iterator = defaultProtocols.iterator();
final String protocolToDisable1 = iterator.next();
final String protocolToDisable2 = iterator.next();

final Set<String> newProtocols = new HashSet<>(defaultProtocols);
newProtocols.remove(protocolToDisable1);
newProtocols.remove(protocolToDisable2);
j.jenkins.setAgentProtocols(newProtocols);
j.jenkins.save();
assertThat(protocolToDisable1 + " must be disabled before the roundtrip",
j.jenkins.getAgentProtocols(), not(hasItem(protocolToDisable1)));
assertThat(protocolToDisable2 + " must be disabled before the roundtrip",
j.jenkins.getAgentProtocols(), not(hasItem(protocolToDisable2)));
final Set<String> agentProtocolsBeforeReload = j.jenkins.getAgentProtocols();
j.jenkins.reload();

assertFalse("The protocol list must have been really reloaded", agentProtocolsBeforeReload == j.jenkins.getAgentProtocols());
assertThat("We should have disabled two protocols",
j.jenkins.getAgentProtocols().size(), equalTo(defaultProtocols.size() - 2));
assertThat(protocolToDisable1 + " must be disaabled after the roundtrip",
j.jenkins.getAgentProtocols(), not(hasItem(protocolToDisable1)));
assertThat(protocolToDisable2 + " must be disaabled after the roundtrip",
j.jenkins.getAgentProtocols(), not(hasItem(protocolToDisable2)));
}
}

0 comments on commit 3e2e017

Please sign in to comment.