[JENKINS-39232] Walk the DelegatingComputerLauncher instances when checking if JNLPComputerLauncher #2602

stephenc · 2016-10-26T09:49:00Z

@jenkinsci/code-reviewers @reviewbybees

Replaces #2601

…ecking if JNLPComputerLauncher

ghost · 2016-10-26T09:57:04Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

oleg-nenashev · 2016-10-26T11:03:24Z

I do not believe it's a perfect fix since anybody can implement his own launcher using ComputerLauncher API. And there are many examples: https://github.com/search?q=org%3Ajenkinsci+%22extends+computerlauncher%22&type=Code

I agree it worths adding support of Delegating launchers, but it does not fix the issue

…tinue to work until they get fixed

daniel-beck · 2016-10-26T12:00:37Z

core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java

+     * {@link ComputerLauncher} when then should have extended {@link DelegatingComputerLauncher}
+     *
+     * @since FIXME
+     */


@Extension(NoExternalUse.class)

oleg-nenashev · 2016-10-26T12:25:21Z

core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java

    @Override
    public boolean owns(String clientName) {
        Computer computer = Jenkins.getInstance().getComputer(clientName);
        return computer != null;
    }

+    private static ComputerLauncher getDelegate(ComputerLauncher launcher) {


CheckForNull

comment wasn't addressed?

oleg-nenashev · 2016-10-26T12:26:30Z

core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java

+            Method getDelegate = launcher.getClass().getMethod("getDelegate");
+            return ComputerLauncher.class.isAssignableFrom(getDelegate.getReturnType()) ? (ComputerLauncher) getDelegate
+                    .invoke(launcher) : null;
+        } catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {


ReflectiveOperationException?

oleg-nenashev · 2016-10-26T12:29:02Z

core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java

+                launcher = ((DelegatingComputerLauncher) launcher).getLauncher();
+            } else if (launcher instanceof ComputerLauncherFilter) {
+                launcher = ((ComputerLauncherFilter) launcher).getCore();
+            } else if (null != (l = getDelegate(launcher))) {  // TODO remove when all plugins are fixed


Does not guarantee a success for old plugins from what I see

oleg-nenashev · 2016-10-26T12:29:27Z

core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java

+            } else {
+                if (disableStrictVerification) {
+                    LOGGER.log(Level.WARNING, "Connecting {0} as a JNLP agent where the launcher does not mark itself "
+                            + "correctly as being a JNLP agent", clientName);


Please explicitly provide a class of the launcher

oleg-nenashev · 2016-10-26T12:30:36Z

core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java

+                            + "correctly as being a JNLP agent", clientName);
+                    break;
+                } else {
+                    event.reject(new ConnectionRefusalException(String.format("%s is not a JNLP agent", clientName)));


🐛 In the case of enforcement must contain more details. E.g. link to the Wiki page providing a workaround info. Or just in-text help

oleg-nenashev

If we want to go in such way, we need better diagnostic messages. 🐛 for it.

I still have concerns about such approach due to impact on plugins, but it's a subject for voting by @jenkinsci/core

…d contract

- Since the failure of strict verification is an issue, we should log it as a warning always

oleg-nenashev · 2016-10-26T13:23:04Z

I approve the code change itself since it makes the situation better that it is in 2.27. So 🐝 and 👍 as a partial fix.

But I still do not feel it's a right approach to nuke so many instances. Hence as a community member I keep 👎 regarding the change as a complete fix of JENKINS-39232

oleg-nenashev · 2016-10-27T16:31:56Z

@reviewbybees done
@jenkinsci/code-reviewers We really really really want to get it in the next weekly

oleg-nenashev · 2016-10-27T16:32:29Z

P.S: I will follow-up on the default behavior

oleg-nenashev · 2016-10-28T12:36:21Z

I will merge it into the next weekly if nobody shouts

KostyaSha · 2016-11-20T22:13:19Z

core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java

+                return (ComputerLauncher) getDelegate.invoke(launcher);
+            }
+        } catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {
+            // ignore


you can name ignore instead e to disable warnings static analysing tools.

KostyaSha · 2016-11-20T22:14:05Z

core/src/main/java/jenkins/slaves/DefaultJnlpSlaveReceiver.java

+                launcher = ((DelegatingComputerLauncher) launcher).getLauncher();
+            } else if (launcher instanceof ComputerLauncherFilter) {
+                launcher = ((ComputerLauncherFilter) launcher).getCore();
+            } else if (null != (l = getDelegate(launcher))) {  // TODO remove when all plugins are fixed


TODO jenkins 3.x and don't break existing plugins.

While technically, it doesn't qualify as an S, there are issues of that nature removed by enforcing that only agents explicitly configured to use a JNLP launcher get to use a JNLP launcher... especially as only a JNLP launcher will have a cookie and hence any existing connection will be dropped in favour of an incoming JNLP connection with a valid secret (it's the valid that stops it being an S... because if you've let somebody else get the master key driving the JNLP secrets... well that's your problem right there)

[JENKINS-39232] Walk the DelegatingComputerLauncher instances when ch…

facc9d5

…ecking if JNLPComputerLauncher

stephenc mentioned this pull request Oct 26, 2016

[JENKINS-39232] Make vSphereCloudLauncher inherit from DelegatingComputerLauncher jenkinsci/vsphere-cloud-plugin#59

Merged

daniel-beck added the needs-more-reviews Complex change, which would benefit from more eyes label Oct 26, 2016

[JENKINS-39232] Add a system property to enable legacy plugins to con…

0d7570a

…tinue to work until they get fixed

daniel-beck reviewed Oct 26, 2016

View reviewed changes

[JENKINS-39232] Be more forgiving of plugins

fe920c4

oleg-nenashev reviewed Oct 26, 2016

View reviewed changes

oleg-nenashev requested changes Oct 26, 2016

View reviewed changes

stephenc added 2 commits October 26, 2016 14:02

[JENKINS-39232] Yet more forgiveness of plugins but define the forwar…

b68ea09

…d contract

[JENKINS-39232] Provide diagnostics when this issue is encountered

3413cfe

- Since the failure of strict verification is an issue, we should log it as a warning always

oleg-nenashev approved these changes Oct 26, 2016

View reviewed changes

oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Oct 28, 2016

oleg-nenashev merged commit 1d16ce0 into jenkinsci:master Oct 29, 2016

oleg-nenashev mentioned this pull request Oct 29, 2016

[FIXED JENKINS-39232] - DefaultJnlpSlaveReceiver should not reject connections from non-JNLP launchers #2601

Closed

oleg-nenashev added a commit that referenced this pull request Oct 30, 2016

Changellog: Noting #2556, #2598, #2597, #2568, #2602, #2518, #2593

d5ffe44

KostyaSha reviewed Nov 20, 2016

View reviewed changes

stephenc deleted the jenkins-39232 branch February 15, 2017 09:29

sanjaydhawangale mentioned this pull request Apr 1, 2017

[FIXED JENKINS-38005] Properly log failure due to empty archive in Pipeline #2823

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-39232] Walk the DelegatingComputerLauncher instances when checking if JNLPComputerLauncher #2602

[JENKINS-39232] Walk the DelegatingComputerLauncher instances when checking if JNLPComputerLauncher #2602

stephenc commented Oct 26, 2016

ghost commented Oct 26, 2016

oleg-nenashev commented Oct 26, 2016

daniel-beck Oct 26, 2016

oleg-nenashev Oct 26, 2016

oleg-nenashev Oct 26, 2016

KostyaSha Nov 20, 2016

oleg-nenashev Oct 26, 2016

oleg-nenashev Oct 26, 2016

oleg-nenashev Oct 26, 2016

oleg-nenashev Oct 26, 2016

oleg-nenashev left a comment

oleg-nenashev commented Oct 26, 2016

oleg-nenashev commented Oct 27, 2016

oleg-nenashev commented Oct 27, 2016

oleg-nenashev commented Oct 28, 2016

KostyaSha Nov 20, 2016

KostyaSha Nov 20, 2016

stephenc Nov 21, 2016

[JENKINS-39232] Walk the DelegatingComputerLauncher instances when checking if JNLPComputerLauncher #2602

[JENKINS-39232] Walk the DelegatingComputerLauncher instances when checking if JNLPComputerLauncher #2602

Conversation

stephenc commented Oct 26, 2016

ghost commented Oct 26, 2016

oleg-nenashev commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Oct 26, 2016

oleg-nenashev commented Oct 27, 2016

oleg-nenashev commented Oct 27, 2016

oleg-nenashev commented Oct 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment