[JENKINS-34755] Migrate to SystemProperties and restrict access to the engine

[oleg-nenashev]

Finalizes #2337

• Do not require null checks in methods with default values
• Migrate the newly created properties to SystemProperties
• Restrict the access to SystemProperties to the Jenkins core only

If #2332 and #2323 address SystemProperties in their files, the entire core will be covered.

@jenkinsci/code-reviewers @reviewbybees

[jtnord]

seems like you could just pass "" for the defautt and clean up the code below?

[oleg-nenashev]

maybe

[ghost]

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.

[jtnord]

should be @Nullable then?

[jtnord]

@olivergondza had a comment on the original PR #1914 that the properties where accessed before the context was initialized

Running slightly modified version of the PR it seems that some properties are read before the context is initialized (and AFAICT those can not be configured from context.xml). It is surprising and undocumented.

GETTING hudson.util.RingBufferLogHandler.defaultSize
GETTING jenkins.model.Jenkins.logStartupPerformance
GETTING hudson.model.Hudson.logStartupPerformance
GETTING jenkins.model.Jenkins.parallelLoad
GETTING hudson.model.Hudson.parallelLoad
GETTING jenkins.model.Jenkins.killAfterLoad
GETTING hudson.model.Hudson.killAfterLoad
GETTING jenkins.model.Jenkins.workspaceDirName
GETTING hudson.model.Hudson.workspaceDirName
CONTEXT INITIALIZED
...

and @evernat commented

The read of system properties before the context is initialized in that class could be fixed by adding a new servletContextListener (the SystemProperties class for example) at the top of the web.xml

Do you have plans to address this?

There was also a comment about the name SystemProperties being wrong and possibly should have been renamed JenkinsConfig or something like that do make it more obvious that they are not System properties in the System.getProiperties sence. (before the code gets into an actual release - if it is not too late already :-( )

[oleg-nenashev]

@jtnord That's I was pinging people heavily in PRs :)

had a comment on the original PR #1914 that the properties where accessed before the context was initialized .... Do you have plans to address this?

It has been addressed in Javadoc: {@link ServletContext} check will be skipped if the context is not initialized.

There was also a comment about the name SystemProperties being wrong and possibly should have been renamed JenkinsConfig or something like that do make it more obvious that they are not System properties in the System.getProiperties sence. (before the code gets into an actual release - if it is not too late already

We have about 24 hours to do it if you feel strongly.
JenkinsConfig is also not a perfect name, because it may say that properties are configurable somewhere in Jenkins configuration. It's not true right now, unfortunately.

But I agree this name would be better

[jtnord]

It has been addressed in Javadoc: {@link ServletContext} check will be skipped if the context is not initialized.

Would seem better to make it just work by adding a servelt listener that runs before Jenkins?
After all there is nothing in this code that should rely in Jenkins being initialized - and the initialization of Jenkins should be able to get config that it needs whilst it is being initialized...

JenkinsConfig is also not a perfect name, because it may say that properties are configurable somewhere in Jenkins configuration. It's not true right now, unfortunately.

that could be done in the future but changing the class name (although possible with extends and deprecates) would be more messy.

[oleg-nenashev]

Would seem better to make it just work by adding a servelt listener that runs before Jenkins?
After all there is nothing in this code that should rely in Jenkins being initialized - and the initialization of Jenkins should be able to get config that it needs whilst it is being initialized...

You know, a pull-request will be appreciated ;)
I can do it, but unlikely we can get it reviewed and tested by 2.5. So maybe a bug to submit

that could be done in the future but changing the class name (although possible with extends and deprecates) would be more messy.

I think we will need to do a "messy thing" in any case, because such engine in the core does not allow to apply it to remoting, stapler and Jenkins modules.

But if we want to rename it in the core, let's do it before the release

[jtnord]

Would seem better to make it just work by adding a servlet listener that runs before Jenkins?

I see that we are called pretty early on in the WebAppMain.contextInitialized so possibly making this a listeners may not help...

[oleg-nenashev]

According to the discussion with @jtnord , I've marked SystemProperties as restricted. We will be able to define the approach later

[jtnord]

🐝 👍

[evernat]

Would seem better to make it just work by adding a servlet listener that runs before Jenkins?

I have created PR #2347 for that.
PR not tested. But I am quite sure that this PR will fix read of 'SystemProperties' values before init of the context. (The issue is which classes are loaded first.)

[oleg-nenashev]

@evernat
Thanks! I'm merging this PR just to secure SystemProperties in the release