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

Removing configurability of Jenkins.agentProtocols #9903

Merged
merged 1 commit into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 4 additions & 17 deletions core/src/main/java/hudson/TcpSlaveAgentListener.java
Original file line number Diff line number Diff line change
Expand Up @@ -271,14 +271,11 @@ public void run() {
String protocol = s.substring(9);
AgentProtocol p = AgentProtocol.of(protocol);
if (p != null) {
if (Jenkins.get().getAgentProtocols().contains(protocol)) {
LOGGER.log(p instanceof PingAgentProtocol ? Level.FINE : Level.INFO, () -> "Accepted " + protocol + " connection " + connectionInfo);
p.handle(this.s);
} else {
error("Disabled protocol:" + s, this.s);
}
} else
LOGGER.log(p instanceof PingAgentProtocol ? Level.FINE : Level.INFO, () -> "Accepted " + protocol + " connection " + connectionInfo);
p.handle(this.s);
} else {
error("Unknown protocol:", this.s);
}
} else {
error("Unrecognized protocol: " + s, this.s);
}
Expand Down Expand Up @@ -364,21 +361,11 @@ public PingAgentProtocol() {
ping = "Ping\n".getBytes(StandardCharsets.UTF_8);
}

@Override
public boolean isRequired() {
return true;
}

@Override
public String getName() {
return "Ping";
}

@Override
public String getDisplayName() {
return Messages.TcpSlaveAgentListener_PingAgentProtocol_displayName();
}

@Override
public void handle(Socket socket) throws IOException, InterruptedException {
try (socket) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,12 @@
import hudson.util.FormApply;
import jakarta.servlet.ServletException;
import java.io.IOException;
import java.util.Set;
import java.util.TreeSet;
import java.util.function.Predicate;
import java.util.logging.Level;
import java.util.logging.Logger;
import jenkins.model.GlobalConfigurationCategory;
import jenkins.model.Jenkins;
import jenkins.util.ServerTcpPort;
import net.sf.json.JSONArray;
import net.sf.json.JSONException;
import net.sf.json.JSONObject;
import org.jenkinsci.Symbol;
Expand Down Expand Up @@ -92,11 +89,6 @@ public boolean isSlaveAgentPortEnforced() {
return Jenkins.get().isSlaveAgentPortEnforced();
}

@NonNull
public Set<String> getAgentProtocols() {
return Jenkins.get().getAgentProtocols();
}

public boolean isDisableRememberMe() {
return Jenkins.get().isDisableRememberMe();
}
Expand Down Expand Up @@ -149,18 +141,6 @@ public boolean configure(StaplerRequest2 req, JSONObject json) throws FormExcept
throw new FormException(e, "slaveAgentPortType");
}
}
Set<String> agentProtocols = new TreeSet<>();
if (json.has("agentProtocol")) {
Object protocols = json.get("agentProtocol");
if (protocols instanceof JSONArray) {
for (int i = 0; i < ((JSONArray) protocols).size(); i++) {
agentProtocols.add(((JSONArray) protocols).getString(i));
}
} else {
agentProtocols.add(protocols.toString());
}
}
j.setAgentProtocols(agentProtocols);

// persist all the additional security configs
boolean result = true;
Expand Down
52 changes: 11 additions & 41 deletions core/src/main/java/jenkins/AgentProtocol.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
import hudson.TcpSlaveAgentListener;
import java.io.IOException;
import java.net.Socket;
import java.util.Set;
import jenkins.model.Jenkins;

/**
* Pluggable Jenkins TCP agent protocol handler called from {@link TcpSlaveAgentListener}.
Expand All @@ -18,57 +16,31 @@
* Implementations of this extension point is singleton, and its {@link #handle(Socket)} method
* gets invoked concurrently whenever a new connection comes in.
*
* <h2>Extending UI</h2>
* <dl>
* <dt>description.jelly</dt>
* <dd>Optional protocol description</dd>
* <dt>deprecationCause.jelly</dt>
* <dd>Optional. If the protocol is marked as {@link #isDeprecated()},
* clarifies the deprecation reason and provides extra documentation links</dd>
* </dl>
*
* @author Kohsuke Kawaguchi
* @since 1.467
* @see TcpSlaveAgentListener
*/
public abstract class AgentProtocol implements ExtensionPoint {
/**
* Allow experimental {@link AgentProtocol} implementations to declare being opt-in.
* Note that {@link Jenkins#setAgentProtocols(Set)} only records the protocols where the admin has made a
* conscious decision thus:
* <ul>
* <li>if a protocol is opt-in, it records the admin enabling it</li>
* <li>if a protocol is opt-out, it records the admin disabling it</li>
* </ul>
* Implementations should not transition rapidly from {@code opt-in -> opt-out -> opt-in}.
* Implementations should never flip-flop: {@code opt-in -> opt-out -> opt-in -> opt-out} as that will basically
* clear any preference that an admin has set. This latter restriction should be ok as we only ever will be
* adding new protocols and retiring old ones.
*
* @return {@code true} if the protocol requires explicit opt-in.
* @since 2.16
* @see Jenkins#setAgentProtocols(Set)
* @deprecated no longer used
*/
@Deprecated
public boolean isOptIn() {
return false;
}

/**
* Allow essential {@link AgentProtocol} implementations (basically {@link TcpSlaveAgentListener.PingAgentProtocol})
* to be always enabled.
*
* @return {@code true} if the protocol can never be disabled.
* @since 2.16
* @deprecated no longer used
*/

@Deprecated
public boolean isRequired() {
return false;
}

/**
* Checks if the protocol is deprecated.
*
* @since 2.75
* @deprecated no longer used
*/
@Deprecated
public boolean isDeprecated() {
return false;
}
Expand All @@ -79,17 +51,15 @@ public boolean isDeprecated() {
* This is a short string that consists of printable ASCII chars. Sent by the client to select the protocol.
*
* @return
* null to be disabled. This is useful for avoiding getting used
* until the protocol is properly configured.
* null to be disabled
*/
@CheckForNull
public abstract String getName();

/**
* Returns the human readable protocol display name.
*
* @return the human readable protocol display name.
* @since 2.16
* @deprecated no longer used
*/
@Deprecated
public String getDisplayName() {
return getName();
}
Expand Down
129 changes: 4 additions & 125 deletions core/src/main/java/jenkins/model/Jenkins.java
Original file line number Diff line number Diff line change
Expand Up @@ -656,47 +656,6 @@
*/
private static final boolean SLAVE_AGENT_PORT_ENFORCE = SystemProperties.getBoolean(Jenkins.class.getName() + ".slaveAgentPortEnforce", false);

/**
* The TCP agent protocols that are explicitly disabled (we store the disabled ones so that newer protocols
* are enabled by default). Will be {@code null} instead of empty to simplify XML format.
*
* @since 2.16
*/
@CheckForNull
@GuardedBy("this")
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.
* Will be {@code null} instead of empty to simplify XML format.
*
* @since 2.16
*/
@CheckForNull
@GuardedBy("this")
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
* {@link #enabledAgentProtocols}.
*
* @since 2.16
* @see #setAgentProtocols(Set)
* @see #getAgentProtocols()
*/
@GuardedBy("this")
private transient Set<String> agentProtocols;

/**
* Whitespace-separated labels assigned to the built-in node as a {@link Node}.
*/
Expand Down Expand Up @@ -1096,18 +1055,6 @@
if (SLAVE_AGENT_PORT_ENFORCE) {
slaveAgentPort = getSlaveAgentPortInitialValue(slaveAgentPort);
}
synchronized (this) {
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;
}

// no longer persisted
installStateName = null;
Expand Down Expand Up @@ -1282,81 +1229,15 @@
*/
@NonNull
public synchronized Set<String> getAgentProtocols() {
if (agentProtocols == null) {
Set<String> result = new TreeSet<>();
Set<String> disabled = new TreeSet<>();
for (String p : Util.fixNull(disabledAgentProtocols)) {
disabled.add(p.trim());
}
Set<String> enabled = new TreeSet<>();
for (String p : Util.fixNull(enabledAgentProtocols)) {
enabled.add(p.trim());
}
for (AgentProtocol p : AgentProtocol.all()) {
String name = p.getName();
if (name != null && (p.isRequired()
|| (!disabled.contains(name) && (!p.isOptIn() || enabled.contains(name))))) {
result.add(name);
}
}
/*
* An empty result is almost never valid, but it can happen due to JENKINS-70206. Since we know the result
* is likely incorrect, at least decline to cache it so that a correct result can be computed later on
* rather than continuing to deliver the incorrect result indefinitely.
*/
if (!result.isEmpty()) {
agentProtocols = result;
}
return result;
}
return agentProtocols;
return AgentProtocol.all().stream().map(AgentProtocol::getName).filter(Objects::nonNull).collect(Collectors.toCollection(TreeSet::new));
}

/**
* Sets the enabled agent protocols.
*
* @param protocols the enabled agent protocols.
* @since 2.16
* @deprecated No longer does anything.
*/
@Deprecated
public synchronized void setAgentProtocols(@NonNull Set<String> protocols) {
Set<String> disabled = new TreeSet<>();
Set<String> enabled = new TreeSet<>();
for (AgentProtocol p : AgentProtocol.all()) {
String name = p.getName();
if (name != null && !p.isRequired()) {
// we want to record the protocols where the admin has made a conscious decision
// thus, if a protocol is opt-in, we record the admin enabling it
// if a protocol is opt-out, we record the admin disabling it
// We should not transition rapidly from opt-in -> opt-out -> opt-in
// the scenario we want to have work is:
// 1. We introduce a new protocol, it starts off as opt-in. Some admins decide to test and opt-in
// 2. We decide that the protocol is ready for general use. It gets marked as opt-out. Any admins
// that took part in early testing now have their config switched to not mention the new protocol
// at all when they save their config as the protocol is now opt-out. Any admins that want to
// disable it can do so and will have their preference recorded.
// 3. We decide that the protocol needs to be retired. It gets switched back to opt-in. At this point
// the initial opt-in admins, assuming they visited an upgrade to a controller with step 2, will
// have the protocol disabled for them. This is what we want. If they didn't upgrade to a controller
// with step 2, well there is not much we can do to differentiate them from somebody who is upgrading
// from a previous step 3 controller and had needed to keep the protocol turned on.
//
// What we should never do is flip-flop: opt-in -> opt-out -> opt-in -> opt-out as that will basically
// clear any preference that an admin has set, but this should be ok as we only ever will be
// adding new protocols and retiring old ones.
if (p.isOptIn()) {
if (protocols.contains(name)) {
enabled.add(name);
}
} else {
if (!protocols.contains(name)) {
disabled.add(name);
}
}
}
}
disabledAgentProtocols = disabled.isEmpty() ? null : new ArrayList<>(disabled);
enabledAgentProtocols = enabled.isEmpty() ? null : new ArrayList<>(enabled);
agentProtocols = null;
LOGGER.log(Level.WARNING, null, new IllegalStateException("Jenkins.agentProtocols no longer configurable"));

Check warning on line 1240 in core/src/main/java/jenkins/model/Jenkins.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line

Line 1240 is not covered by tests
}

private void launchTcpSlaveAgentListener() throws IOException {
Expand Down Expand Up @@ -6000,8 +5881,6 @@
// for backward compatibility with <1.75, recognize the tag name "view" as well.
XSTREAM.alias("view", ListView.class);
XSTREAM.alias("listView", ListView.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
10 changes: 0 additions & 10 deletions core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol4.java
Original file line number Diff line number Diff line change
Expand Up @@ -137,16 +137,6 @@ private char[] constructPassword() {
return "password".toCharArray();
}

@Override
public boolean isOptIn() {
return false;
}

@Override
public String getDisplayName() {
return Messages.JnlpSlaveAgentProtocol4_displayName();
}

@Override
public String getName() {
return "JNLP4-connect"; // matches JnlpProtocol4Handler.getName
Expand Down
1 change: 0 additions & 1 deletion core/src/main/resources/hudson/Messages.properties
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,6 @@ PluginWrapper.Plugin.Has.Dependent=The plugin ''{0}'' has, at least, one depende
PluginWrapper.Plugin.Disabled=Plugin ''{0}'' disabled
PluginWrapper.NoSuchPlugin=No such plugin found with the name ''{0}''
PluginWrapper.Error.Disabling=There was an error disabling the ''{0}'' plugin. Error: ''{1}''
TcpSlaveAgentListener.PingAgentProtocol.displayName=Ping protocol

ProxyConfigurationManager.DisplayName=Proxy Configuration
ProxyConfigurationManager.Description=Configure the http proxy used by Jenkins
3 changes: 0 additions & 3 deletions core/src/main/resources/hudson/Messages_bg.properties
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,6 @@ PluginWrapper.disabledAndObsolete=\
# {0} is disabled. To fix, enable it.
PluginWrapper.disabled=\
„{0}“ е изключена. Включете я.
# Ping protocol
TcpSlaveAgentListener.PingAgentProtocol.displayName=\
Протокол „ping“
# {0} v{1} failed to load. Fix this plugin first.
PluginWrapper.failed_to_load_dependency=\
„{0}“, версия {1} не се зареди. Оправете приставката.
Expand Down
2 changes: 0 additions & 2 deletions core/src/main/resources/hudson/Messages_de.properties
Original file line number Diff line number Diff line change
Expand Up @@ -75,5 +75,3 @@ AboutJenkins.DisplayName=Über Jenkins
AboutJenkins.Description=Versions- und Lizenzinformationen anzeigen.

Functions.NoExceptionDetails=Keine Details zum Ausnahmefehler

TcpSlaveAgentListener.PingAgentProtocol.displayName=Ping-Protokoll
1 change: 0 additions & 1 deletion core/src/main/resources/hudson/Messages_es.properties
Original file line number Diff line number Diff line change
Expand Up @@ -117,4 +117,3 @@ PluginWrapper.Plugin.Has.Dependant=El plugin {0} tiene, al menos, un plugin depe
PluginWrapper.Plugin.Disabled=Plugin {0} deshabilitado
PluginWrapper.NoSuchPlugin=No se encuentra un plugin con el nombre {0}
PluginWrapper.Error.Disabling=Hubo un error al desactivar el plugin ''{0}''. Error: ''{1}''
TcpSlaveAgentListener.PingAgentProtocol.displayName=Protocolo ping
1 change: 0 additions & 1 deletion core/src/main/resources/hudson/Messages_fr.properties
Original file line number Diff line number Diff line change
Expand Up @@ -126,4 +126,3 @@ PluginWrapper.Plugin.Has.Dependent=Le plugin "{0}" a au moins un plugin dépenda
PluginWrapper.Plugin.Disabled=Plugin "{0}" désactivé
PluginWrapper.NoSuchPlugin=Aucun plugin trouvé avec le nom "{0}"
PluginWrapper.Error.Disabling=Une erreur a été relevée lors de la désactivation du plugin "{0}". Erreur : "{1}"
TcpSlaveAgentListener.PingAgentProtocol.displayName=Protocole de ping
Loading
Loading