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

Record internal calls to what look like Jenkins APIs made during the course of Pipeline builds #228

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@jglick
Copy link
Member

jglick commented May 17, 2018

Mainly useful for non-sandboxed scripts, though for now it captures events whether running in the sandbox or not. (And will report method calls even if they are @Whitelisted.)

Only lightly tested, and will miss anything happening inside a @NonCPS block, but might be a good way to get a rough sense of how crazy an installation’s trusted libraries are.

When JEP-300 gets general-purpose telemetry (not just exceptions), then we could start to tally calls like this from the field.

@jglick jglick requested review from abayer and svanoort May 17, 2018

@svanoort
Copy link
Member

svanoort left a comment

Since this does not include testcases, I'm assuming it's W.I.P. and you just wanted a review. That said: this is a super-clever idea and effectively demonstrates this is not as complex as I'd expected -- but I'm not quite convinced it actually accomplishes the goal (see comments). I think it could be adapted to get it there -- testcases would help a lot.

@@ -171,7 +171,7 @@ private Continuable createContinuable(CpsThread currentThread, CpsCallableInvoca

FunctionCallEnv caller = new FunctionCallEnv(null, onSuccess, null, null);
if (currentThread.getExecution().isSandbox())
caller.setInvoker(new SandboxInvoker());
caller.setInvoker(new LoggingInvoker(new SandboxInvoker()));

This comment has been minimized.

Copy link
@svanoort

svanoort May 23, 2018

Member

I think you actually need to catch cases where internal calls are being made via out-of-sandbox execution though, no? Seems like that is where the majority of unsafe calls would be done (otherwise you have to whitelist them).

This comment has been minimized.

Copy link
@svanoort

svanoort May 23, 2018

Member

Also: should have a System Property to control this, in case it introduces issues such as security concerns or stability problems (so users can runtime-disable the use of the LoggingInvoker).

This comment has been minimized.

Copy link
@jglick

jglick May 24, 2018

Author Member

Not sure offhand if this code path is used much, but yeah this should probably be amended to say

} else {
    caller.setInvoker(new LoggingInvoker(new DefaultInvoker()));
}
// would falsely mark third-party libs bundled in Jenkins plugins)
String name = clazz.getName();
// acc. to `find …/jenkinsci/*/src/main/java -type f -exec egrep -h '^package ' {} \; | sort | uniq` this is decent
return name.contains("jenkins") || name.contains("hudson") || name.contains("cloudbees");

This comment has been minimized.

Copy link
@svanoort

svanoort May 23, 2018

Member

I'm having a hard time telling if this is actually going to work -- seems like it might hit false positives for Pipeline classes. Would definitely need some test cases.

This comment has been minimized.

Copy link
@jglick

jglick May 24, 2018

Author Member

From some very simple examples I did not see any false positives. Definitely agreed that we would want some test coverage.

As noted in the PR description, there are cases such as RunWrapper.getNumber that you do want to record. A true false positive would be some call to a class inside groovy-sandbox, groovy-cps, or workflow-cps which is not actually done from the Pipeline script text but is just an artifact of some implementation hack. I would think that any such cases should not be going through Invoker to begin with, though.

/**
* Captures CPS-transformed events.
*/
final class LoggingInvoker implements Invoker {

This comment has been minimized.

Copy link
@svanoort

svanoort May 23, 2018

Member

A better-performing strategy here would be to check for isInternal first, then worry about constructing strings to record. Less terse, but since this code will be on the hot path for Pipeline execution, performance is somewhat important.

This comment has been minimized.

Copy link
@jglick

jglick May 24, 2018

Author Member

check for isInternal first, then worry about constructing strings to record

That is exactly what it already does.

* Mark a call to an internal API made by this build.
* @param call a representation of the call site; for example, {@code hudson.model.Run.setDescription}
*/
synchronized void recordInternalCall(String call) {

This comment has been minimized.

Copy link
@svanoort

svanoort May 23, 2018

Member

May be better to use a threadsafe type vs. global synchronization? Consider also the overheads of the data structure if we generate a lot of results.

This comment has been minimized.

Copy link
@jglick

jglick May 24, 2018

Author Member

There should not be a lot of calls—a typical build will never call this.

@@ -1603,6 +1618,9 @@ public void marshal(Object source, HierarchicalStreamWriter w, MarshallingContex
if (e.timings != null) {
writeChild(w, context, "timings", e.timings, Map.class);
}
if (e.internalCalls != null) {
writeChild(w, context, "internalCalls", e.internalCalls, Set.class);

This comment has been minimized.

Copy link
@svanoort

svanoort May 23, 2018

Member

Potentially clever solution to avoid use of synchronization: de/serialize to/from a trivial Set implementation, but use a thread-safe representation (such as ConcurrentHashMap-backed impls) in-memory.

This comment has been minimized.

Copy link
@jglick

jglick May 24, 2018

Author Member

Again, probably overkill for an infrequently-modified collection.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented May 24, 2018

I'm assuming it's W.I.P. and you just wanted a review.

Not really a W.i.P., no. I do not know if I would have time to develop it further, but wanted to demonstrate that this is possible and maybe “someone” can pick it up and productize it.

@svanoort

This comment has been minimized.

Copy link
Member

svanoort commented May 24, 2018

I would have time to develop it further, but wanted to demonstrate that this is possible and maybe “someone” can pick it up and productize it.

@jglick That's pretty much the definition of a good W.I.P. PR. I doubt anybody is likely to pick this up and carry it through to a merge-ready feature unless you can at least show it achieves its main goal of capturing the Jenkins-internal API calls (vs. the much rare in-sandbox uses). c.f. #228 (comment)

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented May 24, 2018

BTW current output for

currentBuild.rawBuild.description = ''
jenkins.model.Jenkins.instance.systemMessage = ''

is:

Internal calls for make-internal-calls #10:
  hudson.model.Hudson.systemMessage
  org.jenkinsci.plugins.workflow.job.WorkflowRun.description
  org.jenkinsci.plugins.workflow.support.steps.build.RunWrapper.rawBuild

Seems to be missing jenkins.model.Jenkins.instance (or .getInstance) for whatever reason.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Oct 1, 2018

Needs to be integrated with JEP-304.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Mar 11, 2019

@dwnusbaum would this still be wanted?

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