[JENKINS-59656] check build id before interrupting from the executors widget

[thomasgl-orange]

See JENKINS-59656

The approach I propose here is to add a new stopBuild method to the executor API (Executor#doStopBuild(String)), which takes an identifier for the target build as parameter. It's value (if set) should match the externalizable id of the executor's current executable. If it doesn't match, the stopBuild request does nothing (similar to the case of the user not having the right permissions). Of course, the executors view adds the right build ids to the target URL of its stop buttons.

The behaviour of the existing stop API (which takes no parameter) is left unchanged. Usage as an HTTP API method should be avoided (and the one I've substituted is the only one I've found in Jenkins code base), because it's error-prone (it can stop a build which is not the intended one), but I think it's usage as a Java method is still okay (see discussion below about implementation of AbstractBuild#doStop()), so I don't think it should be deprecated.

Note that I've assumed that a Queue.Executable was also always a Run (on which I could call getExternalizableId()). Please let me know if it's not true.

Proposed changelog entries

• improved stop button behavior in the executors widget of the classical GUI, to avoid accidentally interrupting the wrong job

Submitter checklist

• JIRA issue is well described
• Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change)
• Appropriate autotests or explanation to why this change has no tests

Desired reviewers

Maybe @daniel-beck, for the test case? (because it's based on something you wrote in Security400Test)

[jvz]

LGTM

[alecharp]

is this still a work in progress?

[thomasgl-orange]

is this still a work in progress?

Yes, I'm waiting for #4278 to be merged, to shorten the jelly code.
Also, I should probably add a test case, something very similar to Security400Test.ensureDoStopStillReachable().

[thomasgl-orange]

is this still a work in progress?

Yes, I'm waiting for #4278 to be merged, to shorten the jelly code.
Also, I should probably add a test case, something very similar to Security400Test.ensureDoStopStillReachable().

Now using h.urlEncode from #4278, and added test case. I will remove the WIP tag, I think the PR is ready.

[res0nance]

LGTM, AFAIK queue executables are builds, freestylebuilds, abstractbuilds and workflowruns all inherit from run
https://javadoc.jenkins.io/hudson/model/Queue.Executable.html

[rsandell]

I think you need to change the stop button on the build page as well?

[thomasgl-orange]

I think you need to change the stop button on the build page as well?

I don't think so. This button already POSTs to the stop API of the build itself (/job/something/NNN/stop), not the one of an executor, so it's much less error-prone:
https://github.com/jenkinsci/jenkins/blob/jenkins-2.203/core/src/main/resources/lib/hudson/buildCaption.jelly#L41
Same for the one in a job's history widget:
https://github.com/jenkinsci/jenkins/blob/jenkins-2.203/core/src/main/resources/hudson/widgets/HistoryWidget/entry.jelly#L65

This doStop() method (on a build) stops the (OneOff)Executor for which the build is the current executable:
https://github.com/jenkinsci/jenkins/blob/jenkins-2.203/core/src/main/java/hudson/model/AbstractBuild.java#L1307
https://github.com/jenkinsci/jenkins/blob/jenkins-2.203/core/src/main/java/hudson/model/Executor.java#L935

Of course, we could change AbstractBuild#doStop() implementation to make it call Executor#doStopBuild(runExtId) rather than Executor#doStop(), but to me it seems a bit redundant (it would then become something like "find the right executor via its current executable, and then check again that its current executable is the right one").

[thomasgl-orange]

Updated the PR text, because it was still describing the first version of the changes (with an optional parameter to doStop instead of the new doStopBuild method).

[rsandell]

I think that h.urlEncode expects a non null object. But if exe is not a Run that method is not defined on the object and I think jelly will then just pass in null causing a NullPointerException

[thomasgl-orange]

You're right. I've addressed that in db5d364, by making h.urlEncode tolerant to null. It will then return an empty string, which is, I think, more convenient than returning null for the intended use case (encoding URL query parameters).

[thomasgl-orange]

Oh, and you're right about jelly passing null for unknown properties, I tested it to be sure.

[MRamonLeon]

Do you agree to keep the former behavior if the executable is not a Run?

[thomasgl-orange]

Do you agree to keep the former behavior if the executable is not a Run?

Agreed. I was kind of assuming AbstractBuild (a Run) was the only implementation of Queue.Executable, but it's true only in Jenkins core.
Searching in plugins code reveals several other implementations, like for instance this one:

https://github.com/jenkinsci/slave-squatter-plugin/blob/master/src/main/java/hudson/plugins/slave_squatter/ReservationExecutable.java

You and @rsandell are absolutely right that the change I propose should not break these cases (by throwing an NPE from the jelly view, or by not interrupting the current executable).

[varyvol]

@thomasgl-orange thanks for your changes! I'm not sure about changing the behaviour in a public method in such an extended class. What do you think about checking in the Jelly file if the externalizableId is null or not and call the proper method depending on that, given you already implemented the two methods (the one receiving the id and the one not receiving it).

[fcojfernandez]

I have the same concern than @varyvol and I think its suggestion makes sense

[thomasgl-orange]

@varyvol @fcojfernandez : While I would agree, in general, that changing behavior of an existing public method is not good, I think here we should tolerate it.

The Functions.urlEncode(String) method has been introduced very recently, in PR #4278, in order to simplify the Jelly code of this PR. The whole point of puting it in Functions was to encourage proper URL-encoding of requests parameters from Jelly code, by providing something which would be simple enough to be actually used. See for example this fix, in the same PR:
https://github.com/jenkinsci/jenkins/pull/4278/files#diff-757c85f964f10fece894a97dd912e2a4L34
Having to guard its usages with null-check conditionals in Jelly code defeats this purpose.

Also, the current implementation, when called with a null parameter, raises an NPE (sorry about that, didn't think it through enough, obviously, when I've submitted this implementation). It is hard to believe that, in the last five weeks, someone has already discovered this new method, and has started using it in a way which actually relies on this NPE being thrown.

So, my opinion is still that "improving" this method, thus slightly changing its behavior on this corner case, is the way to go.

The best alternative being, in my opinion, to add a new Functions.urlEncode2(String) with the null handling change, and to deprecate of Functions.urlEncode(String). But it sounds like a very heavy way to fix what sounds, to me, like a purely theoretical retro-compatibility issue.

[fcojfernandez]

@thomasgl-orange my concern is because if I make a quick search, I can see uses of that method: https://github.com/search?q=org%3Ajenkinsci+Functions.urlEncode&type=Code

At a very first glance, the change does not seem to have a great impact, but maybe it's wort to have a deeper look at those uses.

[timja]

@thomasgl-orange my concern is because if I make a quick search, I can see uses of that method: github.com/search?q=org%3Ajenkinsci+Functions.urlEncode&type=Code

At a very first glance, the change does not seem to have a great impact, but maybe it's wort to have a deeper look at those uses.

The only use from that search I see is https://github.com/jenkinsci/jenkins/blob/96cc9ce0c57fe25e1538c9e8601672eed4b059db/core/src/main/resources/hudson/search/Search/search-failed.jelly and that's in core

[thomasgl-orange]

@fcojfernandez @timja : I went through results from https://github.com/search?q=org%3Ajenkinsci+"urlEncode"&type=Code (without "Functions" in the query, because it could hide results in Jelly files), and the two relevant results I find are usages I had introduced in #4278:

jenkins/core/src/main/resources/hudson/search/Search/search-failed.jelly

Line 52 in 96cc9ce

<em>result has been truncated, <a href="?q=${h.urlEncode(q)}&amp;max=${max+100}">see 100 more</a></em>

jenkins/core/src/main/resources/lib/hudson/project/upstream-downstream.jelly

Line 34 in 96cc9ce

<a href="${rootURL}/projectRelationship?lhs=${h.urlEncode(lhs.name)}&amp;rhs=${h.urlEncode(rhs.name)}">

(and these ones are alright with the null change)

One more thing, if it counts: Functions.urlEncode(String) is not yet in an LTS version, so less likely to have ever been used, but in core.

[fcojfernandez]

Fair enough then!

[varyvol]

@thomasgl-orange makes sense, thanks for the explanation and the in-depth verification.

[oleg-nenashev]

The implementation looks great, thanks a lot!
One minor comment about making the new API public, but it does not block the merge.

[oleg-nenashev]

Does it have to be a public API? We could mark it as @Restricted(NoExternalUse.class) which is a good practice for stapler endpoints IMHO

[thomasgl-orange]

I think you're right, fixed in f01385e.
FYI, to make sure this restriction was alright, I've searched through all existing uses of the original Executor#doStop() method by plugins in the jenkinsci organisation, in case some would have a reason to be changed to Executor#doStopBuild(String) instead. But the few call sites I've found are always similar to what is done in AbstractBuild#doStop(), with Executor#doStop() being called immediately on an executor which has just been retrieved as the current one for a specific build, so it would be redundant to check it's the "right" one twice.