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

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

Closed

Conversation

@oleg-nenashev
Copy link
Member

commented Oct 26, 2016

In 2.27 we released Remoting 3. It required some patches in the core.

There was also compatibility check hardening:

if (computer == null || !(computer.getLauncher() instanceof JNLPLauncher)) {
event.reject(new ConnectionRefusalException(String.format("%s is not a JNLP agent", clientName)));
return;
}

From what I see the code does not process ComputerLauncherFilter or DelegatingComputerLauncher correctly, and it causes failures of plugins like Slave Setup Plugin (https://github.com/jenkinsci/slave-setup-plugin/blob/ba1a93e0d1a4a150c1cd1cda87a29930a2d60773/src/main/java/org/jenkinsci/plugins/slave_setup/SetupSlaveLauncher.java#L23).

It also breaks plugins like VSphere plugin, which use JNLP, but declare there own Launcher, which nests the JNLP one: https://github.com/jenkinsci/vsphere-cloud-plugin/blob/dd3f70f41b06ac2ab3e02996f924a73f15850911/src/main/java/org/jenkinsci/plugins/vSphereCloudLauncher.java#L31

https://issues.jenkins-ci.org/browse/JENKINS-39232

@reviewbybees @jenkinsci/code-reviewers @stephenc

@oleg-nenashev oleg-nenashev changed the title [FIXED JENKINS-39232] - DefaultJnlpSlaveReceiver should not reject connection of non-JNLP launchers [FIXED JENKINS-39232] - DefaultJnlpSlaveReceiver should not reject connections from non-JNLP launchers Oct 26, 2016

if (computer == null || !(computer.getLauncher() instanceof JNLPLauncher)) {
event.reject(new ConnectionRefusalException(String.format("%s is not a JNLP agent", clientName)));
if (computer == null) {
event.reject(new ConnectionRefusalException(String.format("%s is not a registered JNLP agent", clientName)));

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Oct 26, 2016

Author Member

Maybe it leaks the fact of the agent existense

@reviewbybees

This comment has been minimized.

Copy link

commented Oct 26, 2016

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.

@stephenc

This comment has been minimized.

Copy link
Member

commented Oct 26, 2016

🐛 👎 should not allow non-jnlp agents to connect on a jnlp launcher... correct solution would be to walk the delegating computer launcher instances to see if one of them is a JNLP launcher

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2016

@stephenc Well, this approach won't work with plugins, which extend ComputerLauncher on their own. And there are many of them, not only vSphere cloud plugin. See https://github.com/search?q=org%3Ajenkinsci+%22extends+computerlauncher%22&type=Code

The proposed fix contains TODO for a better replacement. I do not believe we can fix and test all plugins by the next weekly, hence I propose this PR as a temporary fix

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Oct 26, 2016

It does not help with vSphere Launcher I've referenced

On Oct 26, 2016 11:40 AM, "Stephen Connolly" notifications@github.com
wrote:

🐛 👎 should not allow non-jnlp agents to connect on a jnlp launcher...
correct solution would be to walk the delegating computer launcher
instances to see if one of them is a JNLP launcher


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#2601 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AC3IoP2nNuC9A9PzM9R_lH0FUf-OvGauks5q3yAEgaJpZM4Kg274
.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Oct 29, 2016

Closing in favor of #2602

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Nov 20, 2016

@stephenc so you are enforcing plugins to extend classes while java doesn't allow multiple inheritance?

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Nov 20, 2016

Please reopen

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2016

I will reopen it just to continue the discussion

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

@stephenc
Copy link
Member

left a comment

🐛 for reasons I have previously raised out of band

return;
}

final ComputerLauncher launcher = computer.getLauncher();
if (!(launcher instanceof JNLPLauncher) && LOGGER.isLoggable(Level.FINE)) {

This comment has been minimized.

Copy link
@stephenc

stephenc Nov 30, 2016

Member

If you are going to do this rather than walk the delegation chain, you need to reject a connection over JNLP where a connection is already established and connected.

@KostyaSha

This comment has been minimized.

Copy link
Member

commented Nov 30, 2016

@stephenc what is the reason? I will repeat my question, there are plugins that already do class inheritance. Now in non 3.0 you introduced breaking API behaviour and plugins doesn't work. People just adding ignore property. This is not acceptable in 2.x terms. Such behaviour could be planned for 3.X or any other breaking release.

@stephenc

This comment has been minimized.

Copy link
Member

commented Nov 30, 2016

@KostyaSha the reason starts with S and ends in Y.

@stephenc

This comment has been minimized.

Copy link
Member

commented Apr 2, 2017

@oleg-nenashev why is this still open?

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Apr 2, 2017

@stephenc Just because I do not feel that screwing up so many plugins was a good decisions. But since nobody else from @jenkinsci/code-reviewers or CloudBees bothers to provide feedback, I suppose we can close it with the "Nobody else GAF" resolution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.