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

[JENKINS-38187] Fix setting jnlp port through system property #2545

Merged
merged 7 commits into from Sep 27, 2016

Conversation

5 participants
@Vlatombe
Member

Vlatombe commented Sep 14, 2016

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

Fix setting up slave agent port through -Djenkins.model.Jenkins.slaveAgentPort
not only during first startup, but on any startup.

@reviewbybees
@jenkinsci/code-reviewers

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Sep 14, 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.

reviewbybees commented Sep 14, 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.

@Vlatombe

This comment has been minimized.

Show comment
Hide comment
@Vlatombe

Vlatombe Sep 14, 2016

Member

Reviewing test failures

Member

Vlatombe commented Sep 14, 2016

Reviewing test failures

@Vlatombe

This comment has been minimized.

Show comment
Hide comment
@Vlatombe

Vlatombe Sep 14, 2016

Member

Not sure what is wrong with the remaining test. It passes locally for me.

Member

Vlatombe commented Sep 14, 2016

Not sure what is wrong with the remaining test. It passes locally for me.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Sep 16, 2016

Member

I'm not sure about the change. I would not expect code modification here

Member

oleg-nenashev commented Sep 16, 2016

I'm not sure about the change. I would not expect code modification here

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Sep 17, 2016

Member

I would be extremely careful here: This looks like an effective regression for everyone skipping the wizard using system property.

Member

daniel-beck commented Sep 17, 2016

I would be extremely careful here: This looks like an effective regression for everyone skipping the wizard using system property.

@Vlatombe

This comment has been minimized.

Show comment
Hide comment
@Vlatombe

Vlatombe Sep 17, 2016

Member

I'm not specially attached to having the jnlp disabled directly in Jenkins. My primary focus is to fix setting up using -Djenkins.model.Jenkins.slaveAgentPort to avoid having to use hacks such as https://github.com/jenkinsci/docker/blob/master/init.groovy. So I can rework the PR in that way if the current one is too controversial.

Member

Vlatombe commented Sep 17, 2016

I'm not specially attached to having the jnlp disabled directly in Jenkins. My primary focus is to fix setting up using -Djenkins.model.Jenkins.slaveAgentPort to avoid having to use hacks such as https://github.com/jenkinsci/docker/blob/master/init.groovy. So I can rework the PR in that way if the current one is too controversial.

@Vlatombe

This comment has been minimized.

Show comment
Hide comment
@Vlatombe

Vlatombe Sep 18, 2016

Member

Pushed a second version without changing base behaviour of Jenkins.

Member

Vlatombe commented Sep 18, 2016

Pushed a second version without changing base behaviour of Jenkins.

@Vlatombe Vlatombe changed the title from [JENKINS-38187] Disable jnlp by default directly in Jenkins instead of SetupWizard to [JENKINS-38187] Fix setting jnlp port through system property Sep 19, 2016

@Vlatombe Vlatombe closed this Sep 20, 2016

@Vlatombe Vlatombe reopened this Sep 20, 2016

@Vlatombe

This comment has been minimized.

Show comment
Hide comment
@Vlatombe
Member

Vlatombe commented Sep 20, 2016

@daniel-beck

Looks much better now, although I have serious doubts about the readResolve.

It also changes the meaning of the system property. It was always only the initial state, to support automated setup, now it would be 'enforcing' on every restart.

Show outdated Hide outdated core/src/main/java/jenkins/model/Jenkins.java
@@ -982,6 +982,7 @@ private Object readResolve() {
if (jdks == null) {
jdks = new ArrayList<>();
}
slaveAgentPort = SystemProperties.getInteger(Jenkins.class.getName()+".slaveAgentPort", slaveAgentPort);

This comment has been minimized.

@daniel-beck

daniel-beck Sep 20, 2016

Member

Still not sure about keeping the system property "active" beyond initial setup.

@daniel-beck

daniel-beck Sep 20, 2016

Member

Still not sure about keeping the system property "active" beyond initial setup.

This comment has been minimized.

@Vlatombe

Vlatombe Sep 20, 2016

Member

If the port is not enforced on each start, then people will keep using 'hacks' such as https://github.com/jenkinsci/docker/blob/master/init.groovy, which is what I want to avoid.

I would actually go even further: if this property is set, the jnlp port shouldn't be editable through UI. Most cases this port is fixed because of firewall setup so you don't want anyone to change the value through UI once started.

@Vlatombe

Vlatombe Sep 20, 2016

Member

If the port is not enforced on each start, then people will keep using 'hacks' such as https://github.com/jenkinsci/docker/blob/master/init.groovy, which is what I want to avoid.

I would actually go even further: if this property is set, the jnlp port shouldn't be editable through UI. Most cases this port is fixed because of firewall setup so you don't want anyone to change the value through UI once started.

This comment has been minimized.

@daniel-beck

daniel-beck Sep 20, 2016

Member

Then this should have been the behavior from the start. Which it is neither in behavior nor intent, and it's documented differently as well.

Or are you claiming that there is absolutely nobody who needed a fixed port for the initial setup, but changed it on the UI later?

The system property was unfortunately not named initialPort or similar due to @kohsuke circumventing the usual PR process, but it's too late to change that.

My proposal if you want to have different behavior, do it properly: New system property, called enforcePort (or similar), that removes the option from the UI, skips setting the option on form submission, and readResolves.

@daniel-beck

daniel-beck Sep 20, 2016

Member

Then this should have been the behavior from the start. Which it is neither in behavior nor intent, and it's documented differently as well.

Or are you claiming that there is absolutely nobody who needed a fixed port for the initial setup, but changed it on the UI later?

The system property was unfortunately not named initialPort or similar due to @kohsuke circumventing the usual PR process, but it's too late to change that.

My proposal if you want to have different behavior, do it properly: New system property, called enforcePort (or similar), that removes the option from the UI, skips setting the option on form submission, and readResolves.

This comment has been minimized.

@KostyaSha

KostyaSha Sep 20, 2016

Member

Most cases this port is fixed because of firewall setup so you don't want anyone to change the value through UI once started.

During debug (that issues constantly appears with security and firewalls) you may want to change it without restarting master. Inconsistency could be checked and reported to administrative monitor.

@daniel-beck i made own hacks for this port by shipping piece of config.xml in tests that applies to stock docker-container.

PS. @kohsuke so would you use PRs next time?

@KostyaSha

KostyaSha Sep 20, 2016

Member

Most cases this port is fixed because of firewall setup so you don't want anyone to change the value through UI once started.

During debug (that issues constantly appears with security and firewalls) you may want to change it without restarting master. Inconsistency could be checked and reported to administrative monitor.

@daniel-beck i made own hacks for this port by shipping piece of config.xml in tests that applies to stock docker-container.

PS. @kohsuke so would you use PRs next time?

[JENKINS-38187] Honor System Properties to set up jnlp in SetupWizard
Fix setting up slave agent port through -Djenkins.model.Jenkins.slaveAgentPort on startup.
Add -Djenkins.model.Jenkins.slaveAgentPortEnforce=true to enforce slave agent port on every start,  and an administrative monitor in case the value is modified through UI.
@Vlatombe

This comment has been minimized.

Show comment
Hide comment
@Vlatombe

Vlatombe Sep 21, 2016

Member

I added a separate property to control enforcement (rather than a second property to set up a port value), and added an administrative monitor as suggested by @KostyaSha to warn in case the value is changed through UI.

Member

Vlatombe commented Sep 21, 2016

I added a separate property to control enforcement (rather than a second property to set up a port value), and added an administrative monitor as suggested by @KostyaSha to warn in case the value is changed through UI.

Show outdated Hide outdated core/src/main/java/jenkins/model/Jenkins.java
return Jenkins.getSlaveAgentPortInitialValue(slaveAgentPort);
}
public String getExpectedPortString() {

This comment has been minimized.

@daniel-beck

daniel-beck Sep 22, 2016

Member

Why? Can't you use the int for the message?

@daniel-beck

daniel-beck Sep 22, 2016

Member

Why? Can't you use the int for the message?

This comment has been minimized.

@Vlatombe

Vlatombe Sep 22, 2016

Member

Otherwise it will be formatted as 12 345 in the UI instead of 12345.

@Vlatombe

Vlatombe Sep 22, 2016

Member

Otherwise it will be formatted as 12 345 in the UI instead of 12345.

Show outdated Hide outdated core/src/main/java/jenkins/model/Jenkins.java
}
/**
* Depending on whether the user said "yes" or "no", send him to the right place.

This comment has been minimized.

@daniel-beck

daniel-beck Sep 22, 2016

Member

Lazy copy/paste with unnecessary code?

@daniel-beck

daniel-beck Sep 22, 2016

Member

Lazy copy/paste with unnecessary code?

This comment has been minimized.

@Vlatombe

Vlatombe Sep 22, 2016

Member

will remove

@Vlatombe

Vlatombe Sep 22, 2016

Member

will remove

@@ -1206,6 +1219,42 @@ private void launchTcpSlaveAgentListener() throws IOException {
}
}
@Extension

This comment has been minimized.

@daniel-beck

daniel-beck Sep 22, 2016

Member

@Restricted missing

@daniel-beck

daniel-beck Sep 22, 2016

Member

@Restricted missing

This comment has been minimized.

@oleg-nenashev
@oleg-nenashev
Show outdated Hide outdated ...s/model/Jenkins/EnforceSlaveAgentPortAdministrativeMonitor/message.jelly
<!--
The MIT License
Copyright (c) 2016, CloudBees

This comment has been minimized.

@daniel-beck

daniel-beck Sep 22, 2016

Member

CloudBees, Inc.

@daniel-beck

daniel-beck Sep 22, 2016

Member

CloudBees, Inc.

Show outdated Hide outdated ...s/model/Jenkins/EnforceSlaveAgentPortAdministrativeMonitor/message.jelly
${%description(it.expectedPortString)}
<form method="post" action="${rootURL}/${it.url}/act" name="${it.id}">
<div style="float:right">
<f:submit name="fix" value="${%Fix}"/>

This comment has been minimized.

@daniel-beck

daniel-beck Sep 22, 2016

Member

Should use a more descriptive label if it actually changes the value right then rather than navigates to a config page.

@daniel-beck

daniel-beck Sep 22, 2016

Member

Should use a more descriptive label if it actually changes the value right then rather than navigates to a config page.

@Vlatombe

This comment has been minimized.

Show comment
Hide comment
@Vlatombe

Vlatombe Sep 22, 2016

Member

@daniel-beck fixes done according to your last comments

Member

Vlatombe commented Sep 22, 2016

@daniel-beck fixes done according to your last comments

@daniel-beck

Can be merged now I think, but I recommend minor changes to the messages.

Show outdated Hide outdated ...el/Jenkins/EnforceSlaveAgentPortAdministrativeMonitor/message.properties
@@ -1 +1,2 @@
description=Slave Agent Port has been manually changed. Its value will be reset to {0} on restart.
description=Slave Agent Port has been manually changed. Its value will be reset to {0,number,#} on restart.
reset=Reset to {0,number,#}

This comment has been minimized.

@daniel-beck

daniel-beck Sep 22, 2016

Member

… now; to emphasize what it does as opposed to what happens automatically as mentioned in the description preceding this button.

@daniel-beck

daniel-beck Sep 22, 2016

Member

… now; to emphasize what it does as opposed to what happens automatically as mentioned in the description preceding this button.

Show outdated Hide outdated ...el/Jenkins/EnforceSlaveAgentPortAdministrativeMonitor/message.properties
@@ -1 +1,2 @@
description=Slave Agent Port has been manually changed. Its value will be reset to {0} on restart.
description=Slave Agent Port has been manually changed. Its value will be reset to {0,number,#} on restart.

This comment has been minimized.

@daniel-beck

daniel-beck Sep 22, 2016

Member

Should probably mention the system property that sets this, otherwise there's no reference to the reason for this or action to change this behavior.

@daniel-beck

daniel-beck Sep 22, 2016

Member

Should probably mention the system property that sets this, otherwise there's no reference to the reason for this or action to change this behavior.

Show outdated Hide outdated core/src/main/java/jenkins/model/Jenkins.java
public void doAct(StaplerRequest req, StaplerResponse rsp) throws IOException {
if (req.hasParameter("fix")) {
if (req.hasParameter("reset")) {

This comment has been minimized.

@daniel-beck

daniel-beck Sep 22, 2016

Member

🐜 Still weird as there's no way to have a submission that doesn't enter this branch. There's no second button.

@daniel-beck

daniel-beck Sep 22, 2016

Member

🐜 Still weird as there's no way to have a submission that doesn't enter this branch. There's no second button.

@oleg-nenashev

🐛 since SLAVE_AGENT_PORT_ENFORCE flag does not really enforce the port. Dare admins can change it and break the instance

@@ -596,7 +597,16 @@ protected void onModified() throws IOException {
* TCP agent port.
* 0 for random, -1 to disable.
*/
private int slaveAgentPort = SystemProperties.getInteger(Jenkins.class.getName()+".slaveAgentPort",0);
private int slaveAgentPort = getSlaveAgentPortInitialValue(0);

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 23, 2016

Member

Hmm. We were using -1 in SetupWizard as a default value. Is it just another inconsistency?

@oleg-nenashev

oleg-nenashev Sep 23, 2016

Member

Hmm. We were using -1 in SetupWizard as a default value. Is it just another inconsistency?

This comment has been minimized.

@Vlatombe

Vlatombe Sep 23, 2016

Member

Well, using -1 was my first proposal, but it got rejected by Daniel because this was changing behaviour for people skipping setupWizard.

@Vlatombe

Vlatombe Sep 23, 2016

Member

Well, using -1 was my first proposal, but it got rejected by Daniel because this was changing behaviour for people skipping setupWizard.

/**
* If -Djenkins.model.Jenkins.slaveAgentPort is defined, enforce it on every start instead of only the first one.
*/
private static final boolean SLAVE_AGENT_PORT_ENFORCE = SystemProperties.getBoolean(Jenkins.class.getName()+".slaveAgentPortEnforce", false);

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 23, 2016

Member

🐛 From what I see it does not really enforce it since a Jenkins admin can actually change it. IMHO configuration flow needs to be adjusted:

  • Values from the form submissions should be ignored
  • Ideally the field should be readonly when this flag is set
  • It should be possible forcing the disabled state (paranoid mode)
@oleg-nenashev

oleg-nenashev Sep 23, 2016

Member

🐛 From what I see it does not really enforce it since a Jenkins admin can actually change it. IMHO configuration flow needs to be adjusted:

  • Values from the form submissions should be ignored
  • Ideally the field should be readonly when this flag is set
  • It should be possible forcing the disabled state (paranoid mode)

This comment has been minimized.

@Vlatombe

Vlatombe Sep 23, 2016

Member

Thanks for the first bullet, I actually missed that comment from @daniel-beck earlier.

@Vlatombe

Vlatombe Sep 23, 2016

Member

Thanks for the first bullet, I actually missed that comment from @daniel-beck earlier.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 23, 2016

Member

me too, sorry for duplication :(

@oleg-nenashev

oleg-nenashev Sep 23, 2016

Member

me too, sorry for duplication :(

@@ -1206,6 +1219,42 @@ private void launchTcpSlaveAgentListener() throws IOException {
}
}
@Extension

This comment has been minimized.

@oleg-nenashev
@oleg-nenashev
@Override
public boolean isActivated() {
int slaveAgentPort = Jenkins.getInstance().slaveAgentPort;
return SLAVE_AGENT_PORT_ENFORCE && slaveAgentPort != Jenkins.getSlaveAgentPortInitialValue(slaveAgentPort);

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 23, 2016

Member

Well, would be better to really enforce it

@oleg-nenashev

oleg-nenashev Sep 23, 2016

Member

Well, would be better to really enforce it

This comment has been minimized.

@Vlatombe

Vlatombe Sep 26, 2016

Member

fixed

@Vlatombe
Show outdated Hide outdated core/src/main/java/hudson/security/GlobalSecurityConfiguration.java
@@ -72,6 +72,10 @@ public int getSlaveAgentPort() {
return Jenkins.getInstance().getSlaveAgentPort();
}
public boolean isSlaveAgentPortEnforced() {

This comment has been minimized.

@daniel-beck

daniel-beck Sep 23, 2016

Member

@since @restricted?

@daniel-beck

daniel-beck Sep 23, 2016

Member

@since @restricted?

This comment has been minimized.

@Vlatombe

Vlatombe Sep 23, 2016

Member

Done. Thanks for your continued feedback on this, I really appreciate it.

@Vlatombe

Vlatombe Sep 23, 2016

Member

Done. Thanks for your continued feedback on this, I really appreciate it.

This comment has been minimized.

@daniel-beck

daniel-beck Sep 26, 2016

Member

Nitpicking territory: We're not actually using XXX, at least not for @since, it's always TODO. Only change if there are further changes necessary anyway.

@daniel-beck

daniel-beck Sep 26, 2016

Member

Nitpicking territory: We're not actually using XXX, at least not for @since, it's always TODO. Only change if there are further changes necessary anyway.

Vlatombe added some commits Sep 23, 2016

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Sep 26, 2016

Member

FWIW I could have dealt with both approaches -- allowing to reconfigure temporarily (while showing a warning) can make sense in troubleshooting situations.

EnforceSlaveAgentPortAdministrativeMonitor still exists, I assume because of Groovy scripting?

Member

daniel-beck commented Sep 26, 2016

FWIW I could have dealt with both approaches -- allowing to reconfigure temporarily (while showing a warning) can make sense in troubleshooting situations.

EnforceSlaveAgentPortAdministrativeMonitor still exists, I assume because of Groovy scripting?

@Vlatombe

This comment has been minimized.

Show comment
Hide comment
@Vlatombe

Vlatombe Sep 27, 2016

Member

@daniel-beck indeed, it is still possible to change the value through groovy scripting.

Member

Vlatombe commented Sep 27, 2016

@daniel-beck indeed, it is still possible to change the value through groovy scripting.

Show outdated Hide outdated core/src/main/java/jenkins/model/Jenkins.java
/**
* @param port
* 0 to indicate random available TCP port. -1 to disable this service.
*/
public void setSlaveAgentPort(int port) throws IOException {
if (!SLAVE_AGENT_PORT_ENFORCE) {
forceSetSlaveAgentPort(port);
}

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Sep 27, 2016

Member

NIT: else Logging or IOException

@oleg-nenashev

oleg-nenashev Sep 27, 2016

Member

NIT: else Logging or IOException

This comment has been minimized.

@Vlatombe

Vlatombe Sep 27, 2016

Member

OK for logging, but I don't think an exception should be raised.

@Vlatombe

Vlatombe Sep 27, 2016

Member

OK for logging, but I don't think an exception should be raised.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev
Member

oleg-nenashev commented Sep 27, 2016

🐝

@daniel-beck daniel-beck merged commit 3f5fe7f into jenkinsci:master Sep 27, 2016

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@Vlatombe Vlatombe deleted the Vlatombe:JENKINS-38187 branch Sep 27, 2016

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

olivergondza added a commit that referenced this pull request Nov 3, 2016

Merge pull request #2545 from Vlatombe/JENKINS-38187
[FIX JENKINS-38187] Fix setting JNLP port through system property
(cherry picked from commit 3f5fe7f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment