Move initialization to common method for Swarm #693
Conversation
| @CheckForNull | ||
| private HostnameVerifier hostnameVerifier; |
There was a problem hiding this comment.
Following a general paradigm where everytime a boolean field exists to disable validation, a HostNameVerifier field exists beneath it. Following this convention consistently in all classes that do this allows for certain common utilities like JnlpAgentEndpointResolver#openURLConnection to be simplified.
| @@ -928,8 +937,8 @@ private JnlpEndpointResolver createEndpointResolver(List<String> jenkinsUrls, St | |||
| } catch (Exception e) { | |||
| events.error(e); | |||
| } | |||
| resolver = new JnlpAgentEndpointResolver(jenkinsUrls, credentials, proxyCredentials, tunnel, | |||
| sslSocketFactory, disableHttpsCertValidation, agentName); | |||
| resolver = new JnlpAgentEndpointResolver(jenkinsUrls, agentName, credentials, proxyCredentials, tunnel, | |||
There was a problem hiding this comment.
Following a general paradigm where user-specified flags come before computed objects and mandatory arguments come before optional arguments. Using a consistent order helps improve readability as you go from one file to the next, which otherwise used different ordering which was confusing.
| if (noCertificateCheck) { | ||
| hostnameVerifier = new NoCheckHostnameVerifier(); |
There was a problem hiding this comment.
Initializing this eagerly rather than lazily for consistency with other code. Of course, this necessitates that all public methods in this file now call initialization, which is why I factored out initialization into its own method.
| @@ -624,6 +643,7 @@ private void runAsInboundAgent() throws CmdLineException, IOException, Interrupt | |||
| */ | |||
| @SuppressFBWarnings(value = {"CIPHER_INTEGRITY", "STATIC_IV"}, justification = "Integrity not needed here. IV used for decryption only, loaded from encryptor.") | |||
| public List<String> parseJnlpArguments() throws ParserConfigurationException, SAXException, IOException, InterruptedException { | |||
| initialize(); // For Swarm, or anyone else who invokes this public method directly rather than going through main() or run() | |||
There was a problem hiding this comment.
With lazy initialization of the noCertificateCheck functionality, this was working, but only by accident. Now, we explicitly do all initialization in one place, and from every entrypoint.
| @@ -633,7 +653,7 @@ public List<String> parseJnlpArguments() throws ParserConfigurationException, SA | |||
| while (true) { | |||
| URLConnection con = null; | |||
| try { | |||
| con = JnlpAgentEndpointResolver.openURLConnection(agentJnlpURL, agentJnlpCredentials, proxyCredentials, sslSocketFactory, noCertificateCheck, null); | |||
| con = JnlpAgentEndpointResolver.openURLConnection(agentJnlpURL, null, agentJnlpCredentials, proxyCredentials, sslSocketFactory, hostnameVerifier); | |||
There was a problem hiding this comment.
As elsewhere, putting mandatory user-specified arguments before optional ones, and putting user-specified arguments before computed things like SSL socket factories and hostname verifiers.
| private final String agentName; | ||
|
|
||
| private String credentials; | ||
|
|
||
| private String proxyCredentials; | ||
|
|
||
| private String tunnel; | ||
|
|
||
| private SSLSocketFactory sslSocketFactory; | ||
|
|
||
| private boolean disableHttpsCertValidation; | ||
|
|
||
| private final String agentName; | ||
| private HostnameVerifier hostnameVerifier; |
There was a problem hiding this comment.
As elsewhere, putting mandatory user-specified arguments before optional ones, and putting user-specified arguments before computed things like SSL socket factories and hostname verifiers.
after calling `Launcher#parseJnlpArguments` in which case re-initialization needs to take place
I tested that nothing in Remoting or Swarm does this today.
|
|
| if (connectionTarget != null) { | ||
| initialize(); | ||
| runAsTcpClient(); | ||
| } else if (agentJnlpURL != null || !urls.isEmpty() || directConnection != null) { |
There was a problem hiding this comment.
Fortunately this now has test coverage! (Though jenkinsci/bom#2417 is of no help since the relevant tests require special setup to run. And anyway the change needs to propagate through remoting → docker-agent → docker-inbound-agent → kubernetes-plugin rather than the normal Maven dependency chain.)
Found a minor issue when trying to refactor Swarm to take advantage of these recent changes: Swarm directly calls
Launcher#parseJnlpArgumentsto get the secret from the server (and can't easily be changed to stop doing this), and that currently means that the initialization logic fromLauncher#rundoesn't get run, so things like-noCertificateCheckare working only by accident in that their initialization is done lazily rather than eagerly. This PR fixes that problem and makes the code more easy to understand by putting all initialization logic in one place and ensuring that all public entrypoints invoke it exactly once, and eagerly at the beginning of program launch.We also took the various initialization paradigms and made them consistent: eager rather than lazy, using the same set of fields everywhere, the same set of arguments everywhere in the same order, etc.
Testing done
Ran both Remoting and Swarm tests with these changes.