JENKINS-33925 Sample realization of the cps invoker interceptor

[sparshev]

fixes: #106

Realization:

JenkinsRule allows to run actual jenkins with actual plugins, but it's hard to intercept the calls. That could be done through CpsScript & CPSClosure (workflow-cps-plugin) or through Invoker (groovy-cps). Intercepting of CpsScript is hard (final method, too much logic...), but intercepting the Invoker is easier and allows better control of the execution, so the best candidate to intercept.

The main idea behind the changes is to provide a relatively easy way for custom interceptions. I think it’s the most secure way that requires a minimal number of changes and allows to prepare a custom logic for the ones who want to prepare flexible unit/integration tests of the CPS logic.
It changes the interface for the invokers which could be intercepted, but should not break not break the basic Invoker interface.
I thought about a number of interceptions to the existing code:

1. Create overrides of com.cloudbees.groovy.cps.sandbox.DefaultInvoker and com.cloudbees.groovy.cps.sandbox.SandboxInvoker - looks ok, but not for debugging - it’s hard to understand what the class is used - the original one or the overridden one.
2. Overriding of com.cloudbees.groovy.cps.sandbox.Invoker and com.cloudbees.groovy.cps.impl.FunctionCallEnv with creating extensions MPLDefaultInvoker & MPLSandboxInvoker - the traces looks better and showing where the issue and that the Invoker classes are overridden, but still requires too much original code from groovy-cps.
3. Generating override classes dynamically - looks complicated, but it is another way to implement the idea. Will require to get sources of groovy-cps, unpack them and apply patches to the required classes.
4. This one with modifying the groovy-cps code.

Usage

With this changes - all the user need to do - is to add an override the original InvokerInterceptor:

• test/java/com/cloudbees/groovy/cps/sandbox/InvokerInterceptor.java:

  package com.cloudbees.groovy.cps.sandbox;
  import com.griddynamics.devops.mpl.testing.MPLInterceptor
  
  public abstract class InvokerInterceptor implements Invoker {
      public Object methodCall(Object receiver, String method, Object[] args) throws Throwable {
          return MPLInterceptor.instance.processMethodCall(this.&doMethodCall, this.class.getSimpleName(), receiver, method, args);
      }
      abstract Object doMethodCall(Object receiver, String method, Object[] args) throws Throwable;
      private static final long serialVersionUID = 1L;
  }
  

Notice

Quite the same was prepared for MPL (right now using the second solution)

[dwnusbaum]

@sparshev Can you use GroovyInterceptor from groovy-sandbox for this use case already? Something like this:

import com.griddynamics.devops.mpl.testing.MPLInterceptor
import org.kohsuke.groovy.sandbox.GroovyInterceptor;

public class MyInterceptor extends GroovyInterceptor {
    public Object onMethodCall(Invoker invoker, Object receiver, String method, Object... args) {
        if (... intercept this call...) {
            return MPLInterceptor.instance.processMethodCall(...);
        } else {
            return super.onMethodCall(invoker, receiver, method, args);
        }
    }
}

And then in setup code you call new MyInterceptor().register() to enable your interceptor. This is how the sandbox works, script-security implements GroovyInterceptor to intercept all operations. All it does is check if they are allowed, but you can do whatever you want when an operation is intercepted.

[sparshev]

Hi @dwnusbaum

When I checked the possible ways to intercept the call I thought I saw this way only for the SandboxInvoker (probably it's not working for DefaultInvoker which is used to call the trusted sources like shared libraries), but I will check it again to confirm. Thank you)

[sparshev]

So, my experiments showing the next results:

• Registration of the interceptor during creating the unit test class is not working
• Registration during the test start is not working

Right now not sure how to properly register my invoker outside of the executing thread, but looks like scriptsecurity plugin made it - so it's possible. Let me try to investigate this way, for sure it's much better to use the existing intercepting method - for example by Extension.

[sparshev]

@dwnusbaum probably there is no such extension - I checked stacktraces from the interesting registration of GroovyInterceptor by GroovySandbox.enter():

-> GroovySandbox: GroovySandbox() create
java.lang.Throwable
        at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.GroovySandbox.<init>(GroovySandbox.java:78)
        at org.jenkinsci.plugins.workflow.cps.CpsGroovyShell.doParse(CpsGroovyShell.java:131)
        at org.jenkinsci.plugins.workflow.cps.CpsGroovyShell.reparse(CpsGroovyShell.java:127)
        at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.parseScript(CpsFlowExecution.java:561)
        at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.start(CpsFlowExecution.java:522)
        at org.jenkinsci.plugins.workflow.job.WorkflowRun.run(WorkflowRun.java:327)
        at hudson.model.ResourceController.execute(ResourceController.java:97)
        at hudson.model.Executor.run(Executor.java:427)
-> GroovySandbox: enter()
java.lang.Throwable
        at org.jenkinsci.plugins.scriptsecurity.sandbox.groovy.GroovySandbox.enter(GroovySandbox.java:122)
        at org.jenkinsci.plugins.workflow.cps.CpsGroovyShell.doParse(CpsGroovyShell.java:139)
        at org.jenkinsci.plugins.workflow.cps.CpsGroovyShell.reparse(CpsGroovyShell.java:127)
        at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.parseScript(CpsFlowExecution.java:561)
        at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.start(CpsFlowExecution.java:522)
        at org.jenkinsci.plugins.workflow.job.WorkflowRun.run(WorkflowRun.java:327)
        at hudson.model.ResourceController.execute(ResourceController.java:97)
        at hudson.model.Executor.run(Executor.java:427)

Looks like it's hardcoded in the CpsGroovyShell

Maybe you see any way to run GroovyInterceptor.register() in the running thread of JenkinsRule?

[sparshev]

Hello @dwnusbaum, I suppose there was no movement in such direction? How I can help with introducing such tracing for jenkins pipeline jobs? The hardcode looks not great I think...

[dwnusbaum]

@sparshev Is this something that you still care about? In general, we do not really have the bandwidth to support any direct usage of this library outside of workflow-cps itself. The general recommendation (which I understand is not ideal), is that you should do everything possible to move complex logic out of your Pipeline into external scripts that can be tested without having to worry about the behavior of this library. That includes avoiding complex Pipeline libraries, especially using the src folder in a library to effectively define an entire application using Groovy logic.

[sparshev]

Hi @dwnusbaum ,

Yeah I agree they should be simpler, but it will not help if I will give such recommendation to the users. The need of unit testing will not go anyway even with simpler pipelines (you know, they are not simple in the wild anyway). The only thing I can do is to simplify their lives with better interfaces and tools to handle them.

Unfortunately pipelines become more and more complicated (because they are actually useful) and I suppose there should be a way to properly test the libraries and validate their steps (even if they are simple ) for security and stability reasons.

For sure there is much more important work (like Jenkins clusterization or at least database hooks/plugins) - but I believe this kind of proposed interface will make the life of regular pipeline users much easier if they would be able to run the same Jenkins with needed plugins and dry-run the pipelines they have to check the unit-tests asserts. Maybe there is another way to dry-run the pipelines - but the proposed change is the best I was able to find for now...

Thank you

[dwnusbaum]

@sparshev Sorry I should have been more specific: are you actively using this patch and/or still trying to get it merged? I do not quite understand your concerns related to extending from DefaultInvoker, SandboxInvoker, or Invoker directly that you mentioned in this PR's description. MPLDefaultInvoker.groovy and MPLSandboxInvoker.groovy in griddynamics/mpl#49 seem fine to me (besides the fact that we do not support direct use of this library outside of workflow-cps). I do not see how this PR would simplify that PR in a meaningful way.

[sparshev]

Yeah, the change was born to show how to simplify the pipeline steps interception which is handy for the tracers - right now in griddynamics/mpl#49 I have to override the entire Invoker and FunctionCallEnv java classes - which basically means potential issues and incompatibility in the future versions of jenkins/groovy-cps (who knows when it will be changed).

So I don't use this way until it will be stabilized - too dangerous and still hope for merge or some another way to have such pipeline dry-run / interception capabilities built in jenkins.

The proposed changes creates "class interface" for easy override of the simple interface-purposed InvokerInterceptor.java (I showed how to in Usage in description) without hacking into above big classes (and I suppose still quite safe from secure perspective) - so I can just add this override instead of those 2 above big java files. This is the best way I found for unit tests and my knowledge of java doesn't allow me to see the other ways - probably it exists, but my exploring of security plugin showed it's just hard-coded into jenkins to make interceptions.

[dwnusbaum]

The proposed changes creates "class interface" for easy override of the simple interface-purposed InvokerInterceptor.java (I showed how to in Usage in description)

Sorry if I am missing something obvious, but your usage just shows a definition of an abstract class. It's not clear to me how this does anything at all. It's also not clear to me what you mean by "overriding" FunctionCallEnv and Invoker in griddynamics/mpl#49 - are you redefining classes via JVM APIs or Groovy metaprogramming or something unusual like that? Have you vendored the sources of groovy-cps into your tool directly? I see that you have redefined the classes with some modifications, but what is actually picking up those modifications and using them? For us to even consider merging something that adds new APIs we would need to see a PR that demonstrates a consumer of the new API.

To say it another way, how does the new InvokerInterceptor abstract class allow you to do anything that you could not already do with Invoker? What exactly does it look like to consume this new "API"? Is the idea that you can define your own version of InvokerInterceptor and then use some kind of build-level trick to have your class take precedence over the one from this library?

[sparshev]

@dwnusbaum for sure it's not obvious, sorry my explanations are misleading. Let me try to deep dive into the changes:

When I put something in test dir with the same path as in some jenkins library - during MPL unit tests execution those classes are replacing the ones we have in Jenkins - and since we replaced InvokerInterceptor abstract class in Usage - now it contains our code to call MPLInterceptor logic because later it will be extended by DefaultInvoker (in proposed changes). That's how MPL gets in control and can see what's happening.

In griddynamics/mpl#49 - I override the DefaultInvoker by extending it in MPLDefaultInvoker and SandboxInvokerbyMPLSandboxInvoker- and have to overrideFunctionCallEnv&Invoker` to do it properly - those files are modified in comparison to groovy-cps origins to make possible to intercept the calls.

Hopefully now it's more clear. Just keep in mind - this change & mpl#49 implements different logic to achieve almost the same results - if this change will be accepted I will be able to make mpl#49 a bit simpler with much more confidence in the near future.

[dwnusbaum]

When I put something in test dir with the same path as in some jenkins library - during MPL unit tests execution those classes are replacing the ones we have in Jenkins

Ok, thanks for explaining! That seems like a somewhat unusual approach, and I would not really consider it an API even with this PR which would simplify things for you.

In general we just do not have a good answer here. I do not think it makes sense to merge a PR like this when we are not willing to provide any guarantees about supporting it. In recent PRs like #121 we are actively trying to get rid of anything in this library that appears to be unused by workflow-cps. In general I think that a better approach would be to find some way to add an ExtensionPoint in workflow-cps so that you can create an @Extension for your behavior, for example maybe code like this could return an Invoker dynamically based on an extension lookup.

Maybe you see any way to run GroovyInterceptor.register() in the running thread of JenkinsRule?

Maybe you could prepend code to the beginning of the Pipeline for each test case to instantiate some custom GroovyInterceptor and call .register from inside the Pipeline itself (since it will run on the thread you care about). You might need to tweak script-security configuration or create a custom Whitelist depending on how realistic your test harness is, but I think that would allow you to register a new interceptor.

That said, I do not think the current state of griddynamics/mpl#49 is all that bad. Sure, you might have to update it once a year or something to adapt to code changes, but you will already need to spend some effort updating your tool to to new versions of Jenkins and workflow-cps regularly anyway, and generally speaking, no one is actively working on this library outside of security fixes and short bursts of activity right afterwards, so I do not anticipate frequent breaking changes. I think that if you had merged that PR back in 2020, you would not have needed to make any changes to it until this commit was released around 2 weeks ago, for what that's worth.

For now I am going to close this PR, just because our current position is that the only supported consumer of this library is workflow-cps, and any other usage is unsupported.

[sparshev]

Thank you @dwnusbaum , will check the ways you've described and maybe will come with another approach :)