[JENKINS-26481] [JENKINS-27421] Use GroovyCategorySupport to invoke CpsDefaultGroovyMethods (w/o DGMPatcher) & IteratorHack #124

jglick · 2017-05-11T20:18:06Z

JENKINS-26481: supersedes #14 and most of cloudbees/groovy-cps#51. Seems to demonstrate that we never actually needed all the horrifying stuff in DGMPatcher. And seems to work in any Groovy version we support. (Pending integration of 2.5.0 in Jenkins core, we will however hit GROOVY-6263.)

Also JENKINS-27421: a more maintainable workaround for serialization issues.

Really the technique is quite general: any time you find a (non-static) method somewhere (typically in either the Java Platform or the GDK) which is not amenable to being called from Pipeline script (particularly CPS-transformed parts), we can replace its implementation with something else of our choice.

Tasks, including upstream:

using CpsDefaultGroovyMethods
adding coverage for most Closure-based methods
fixing List.iterator
fixing Map.entrySet
fixing Set.iterator
fixing List.listIterator
test coverage for miscellaneous new Closure-based methods
GroovyClassLoaderWhitelist to check only Closure-based methods defined outside the supported receivers, as in [JENKINS-34064] Making fix work in Jenkins 2 #14
some manual integration checking just because this is all so scary

Subsumes #135.

@reviewbybees if you are feeling adventurous

…sDefaultGroovyMethods without hacks like DGMPatcher.

jglick · 2017-05-11T20:18:28Z

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThread.java

@@ -146,10 +157,33 @@ public StepExecution getStep() {
        this.step = step;
    }

+    /** TODO pending full coverage in {@link CpsDefaultGroovyMethods} */


Such as is added in cloudbees/groovy-cps#52, but that mixes in other stuff which we do not need.

jglick · 2017-05-11T20:19:53Z

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThread.java

-            outcome = program.run0(o);
+            outcome = GroovyCategorySupport.use(Arrays.<Class>asList(CpsDefaultGroovyMethods.class, CpsDefaultGroovyMethodsExt.class), new Closure<Outcome>(null) {
+                @Override public Outcome call() {
+                    return program.run0(o);


This is just for experimentation; probably we would rather want to have a change in groovy-cps which

fleshes out CpsDefaultGroovyMethods

calls GroovyCategorySupport.use from inside Continuable.run0

jglick · 2017-05-11T20:20:55Z

src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java

-        for (int i = 0; i < args.length; i++) {
-            if (args[i] instanceof CpsClosure && parameterTypes.length > i && parameterTypes[i] == Closure.class) {
-                throw new UnsupportedOperationException("Calling " + method + " on a CPS-transformed closure is not yet supported (JENKINS-26481); encapsulate in a @NonCPS method, or use Java-style loops");
+        if (false) { // TODO be more discriminating


Suppressing the main change in #15 for the moment; #14 has something closer to what we need, though ideally we would check the actual method and determine if we have CPS coverage for that method (and overload) now.

jglick · 2017-05-11T20:21:38Z

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThread.java

+            Builder b = new Builder(loc("each"));
+            CpsFunction f = new CpsFunction(Arrays.asList("self", "closure"), b.block(
+                    b.staticCall(-1, CpsDefaultGroovyMethods.class, "each",
+                            b.staticCall(-1, InvokerHelper.class, "asIterator",


Just delegates to the Iterator overload.

…work around unserializable iterators, map entries, etc.

jglick · 2017-05-11T21:05:03Z

src/main/java/org/jenkinsci/plugins/workflow/cps/persistence/IteratorHack.java

-
-    @Override public Pickle writeReplace(Object object) {
-        if (ACCEPTED_TYPES.contains(object.getClass().getName())) {
-            return new Replacement(object);


Supersedes jenkinsci/pipeline-plugin#372.

jglick · 2017-05-11T21:05:43Z

src/test/java/org/jenkinsci/plugins/workflow/SerializationTest.java

@@ -219,12 +219,8 @@ public void stop(Throwable cause) throws Exception {
                SemaphoreStep.success("new-one/1", null);
                SemaphoreStep.success("new-two/1", null);
                story.j.waitForCompletion(b);
-                /* TODO desired behavior:


Now we can have the “desired behavior” from #77.

jglick · 2017-05-11T21:23:50Z

Hmm, seems load step calls and the like fail with

groovy.lang.MissingMethodException: No signature of method: com.cloudbees.groovy.cps.SandboxCpsTransformer.copy() is applicable for argument types: (java.util.ArrayList) values: [[MethodNode@…[void main([Ljava.lang.String;)], ...]]
Possible solutions: any(), any(groovy.lang.Closure), notify(), wait(), find(), dump()

Looking into that.

jglick · 2017-05-12T12:43:21Z

Seems that merely calling GroovyCategorySupport.use—even with no categories—triggers this. So some of the global behavioral switches turned on by the presence of category support seems to be breaking the compiler. I think I have a workaround.

abayer

This is some nifty

abayer · 2017-05-12T18:30:42Z

src/main/java/org/jenkinsci/plugins/workflow/cps/CpsThread.java

            resumeValue = null;
-            outcome = program.run0(o);
+            outcome = GroovyCategorySupport.use(Arrays.<Class>asList(CpsDefaultGroovyMethods.class, CpsDefaultGroovyMethodsExt.class, IteratorHack.class), new Closure<Outcome>(null) {


Given the black magic here, it'd be nice to have some comments explaining what exactly is going on. =)

abayer

Overall, this is great, but it needs some more tests for additional methods like Map.each, collect, collectEntries, eachLine... I can get those added today, though. Expect a PR to @jglick's branch.

jglick · 2017-05-16T14:10:45Z

Absolutely we would like more test coverage.

As to

Could not find artifact com.cloudbees:groovy-cps:jar:1.13-20170515.220420-1 in repo.jenkins-ci.org (https://repo.jenkins-ci.org/public/)

seems I forgot to deploy to the Jenkins repo. Will fix.

abayer · 2017-05-16T14:16:54Z

@jglick I'm getting test failures locally - CpsFlowExecutionTest#trustedShell_control, CpsScriptTest#evaluateShallSandbox, SnippetizerTest#doDslRef...are you seeing those too or is this possibly a Java 8 issue? (since I'm using Java 8 locally with #126 merged in)

jglick · 2017-05-16T14:30:52Z

CpsScriptTest.evaluateShallSandbox indeed fails for me. Will investigate.

abayer · 2017-05-16T14:33:40Z

Had the same failures on Java 7 (and again with the right Java 8 version) so they look legit.

jglick · 2017-05-16T14:35:56Z

Yes, somehow these changes seem to be suppressing the sandbox, so will need to figure out why that is.

abayer · 2017-05-16T14:46:54Z

Map.each is failing with

groovy.lang.MissingMethodException: No signature of method: static com.cloudbees.groovy.cps.CpsDefaultGroovyMethods.$callClosureForMapEntry__groovy_lang_Closure__java_util_Map_Entry() is applicable for argument types: (org.jenkinsci.plugins.workflow.cps.CpsClosure2, java.util.AbstractMap$SimpleImmutableEntry) values: [org.jenkinsci.plugins.workflow.cps.CpsClosure2@3f6cd2de, a=1]

And it looks like callClosureForMapEntry doesn't exist in CpsDefaultGroovyMethods for real.

jglick · 2017-05-16T16:05:39Z

npm errors are making CI more or less useless in this repository, so I need to take a detour to try using yarn instead.

jglick · 2017-05-16T18:53:09Z

Seems that as of the last build, all tests passed, and we just bombed out due to npm (cf. #128).

I think this was in reference to the upstream PR, and those tests were already added.

jglick · 2017-05-23T20:14:03Z

Seems to be causing a failure in LibraryAdderTest.loaderReleased which I will look into.

jglick · 2017-05-23T20:26:48Z

Never mind, that seems to be a regression in 2.31; I will look into it, but not a blocker for this.

ghost · 2017-05-23T20:29:40Z

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.

jglick · 2017-05-23T22:35:43Z

pom.xml

@@ -62,8 +62,7 @@
        </pluginRepository>
    </pluginRepositories>
    <properties>
-        <jenkins.version>1.642.3</jenkins.version>
-        <jenkins-test-harness.version>2.19</jenkins-test-harness.version>
+        <jenkins.version>2.7.3</jenkins.version>


Did not seem practical to support Groovy 1.x, since we have to choose a Groovy release to generate CpsDefaultGroovyMethods from. Too old, and we do not cover enough methods; too new, and it may not link against an older Groovy core.

+1 strongly. Especially given that Blue Ocean and Declarative require 2.7.3, so we've got that as the "default" baseline these days anyway.

jglick · 2017-05-23T22:36:11Z

pom.xml

@@ -234,6 +233,12 @@
                </exclusion>
            </exclusions>
        </dependency>
+        <dependency>
+            <groupId>org.jenkins-ci.plugins</groupId>
+            <artifactId>credentials</artifactId>


This is #135.

jglick · 2017-05-23T22:38:33Z

src/main/java/org/jenkinsci/plugins/workflow/cps/GroovyClassLoaderWhitelist.java

@@ -76,21 +68,4 @@ private boolean permits(Class<?> declaringClass) {
        return permits(field.getDeclaringClass()) || delegate.permitsStaticFieldSet(field, value);
    }

-    /**
-     * Blocks accesses to some {@link DefaultGroovyMethods}, or any other binary method taking a closure, until JENKINS-26481 is fixed.


Yes there could be other Closure-taking binary methods which we are not translating, but it is too cumbersome to track them. Better to spend time implementing the more general, and precise, JENKINS-31314.

jglick · 2017-05-23T22:39:41Z

src/test/java/org/jenkinsci/plugins/workflow/cps/SnippetizerTest.java

-        assertThat("SubversionSCM.workspaceUpdater is mentioned as an attribute of a value of GenericSCMStep.delegate", html, containsString("workspaceUpdater"));
-        assertThat("CheckoutUpdater is mentioned as an option", html, containsString("CheckoutUpdater"));
+        assertThat("GitSCM.submoduleCfg is mentioned as an attribute of a value of GenericSCMStep.scm", html, containsString("submoduleCfg"));
+        assertThat("CleanBeforeCheckout is mentioned as an option", html, containsString("CleanBeforeCheckout"));


Just related to jenkins-test-harness bump (no more detached plugins).

jglick · 2017-05-23T22:40:35Z

src/main/java/org/jenkinsci/plugins/workflow/cps/persistence/IteratorHack.java

+        }
+        Set<Map.Entry<K, V>> entries = new LinkedHashSet<>();
+        for (Map.Entry<K, V> entry : map.entrySet()) {
+            // TODO return an object holding references to the map and the key and delegating all calls accordingly


These impls are not intended to be perfect or complete, just good enough to let typical script usages pass, where previously they would not.

I forget - does this all enable for (a : listOfA) { ... } and the like?

Yes, see IteratorHackTest. Prior to this PR, ArrayList.iterator was safe but nothing else (and in particular I had no idea how to support iterating Maps and the like—the trick did not generalize well). Now most common interface methods in this category should work, and on any implementation class.

abayer · 2017-05-23T23:18:28Z

pom.xml

@@ -62,8 +62,7 @@
        </pluginRepository>
    </pluginRepositories>
    <properties>
-        <jenkins.version>1.642.3</jenkins.version>
-        <jenkins-test-harness.version>2.19</jenkins-test-harness.version>
+        <jenkins.version>2.7.3</jenkins.version>


+1 strongly. Especially given that Blue Ocean and Declarative require 2.7.3, so we've got that as the "default" baseline these days anyway.

abayer · 2017-05-23T23:20:56Z

src/main/java/org/jenkinsci/plugins/workflow/cps/persistence/IteratorHack.java

+        }
+        Set<Map.Entry<K, V>> entries = new LinkedHashSet<>();
+        for (Map.Entry<K, V> entry : map.entrySet()) {
+            // TODO return an object holding references to the map and the key and delegating all calls accordingly


I forget - does this all enable for (a : listOfA) { ... } and the like?

jglick · 2017-05-24T19:02:07Z

@reviewbybees done

@kohsuke are you out there?

[JENKINS-26481] Prototype of using GroovyCategorySupport to invoke Cp…

ab9a154

…sDefaultGroovyMethods without hacks like DGMPatcher.

jglick added the work-in-progress label May 11, 2017

jglick requested review from kohsuke and abayer May 11, 2017 20:18

jglick commented May 11, 2017

View reviewed changes

[JENKINS-27421] Categories also seem to provide a far simpler way to …

ee84510

…work around unserializable iterators, map entries, etc.

jglick commented May 11, 2017

View reviewed changes

jglick changed the title ~~[JENKINS-26481] Prototype of using GroovyCategorySupport to invoke CpsDefaultGroovyMethods without hacks like DGMPatcher~~ [JENKINS-26481] [JENKINS-27421] Use GroovyCategorySupport to invoke CpsDefaultGroovyMethods (w/o DGMPatcher) & IteratorHack May 11, 2017

Picking up cloudbees/groovy-cps#53 to fix problems with nested parsing.

76ec12a

jglick mentioned this pull request May 12, 2017

[JENKINS-26481] Working on supporting CPS-transformed closures passed to DefaultGroovyMethods cloudbees/groovy-cps#54

Merged

abayer approved these changes May 12, 2017

View reviewed changes

Picking up work from cloudbees/groovy-cps-dgm-builder#4.

ce087e2

abayer previously requested changes May 16, 2017

View reviewed changes

Redeployed snapshot, to the right repository this time.

2403513

The sandbox was not being applied.

56edbee

As of jenkins-test-harness 2.20 subversion is not loaded implicitly.

39d9382

jglick mentioned this pull request May 16, 2017

[JENKINS-26481] Generate CpsDefaultGroovyMethods, invoke using GroovyCategorySupport cloudbees/groovy-cps#56

Merged

jglick added 3 commits May 16, 2017 17:41

Now on cloudbees/groovy-cps#56.

3127422

Updated.

2f6c0ec

Merge branch 'master' into GroovyCategorySupport-JENKINS-26481

843aebb

InjectedTest failure.

ff0d5bb

jglick added 3 commits May 23, 2017 14:57

Fleshing out IteratorHack.

0b1a8cc

Separating tests for IteratorHack into its own suite for clarity.

da7f149

Cleaning up whitelist logic related to CpsDefaultGroovyMethods.

bb70ddb

jglick removed the work-in-progress label May 23, 2017

jglick mentioned this pull request May 23, 2017

[JENKINS-34064] Making fix work in Jenkins 2 #14

Closed

jglick commented May 23, 2017

View reviewed changes

abayer approved these changes May 23, 2017

View reviewed changes

jglick added 2 commits May 30, 2017 17:30

Merge branch 'master' into GroovyCategorySupport-JENKINS-26481

de716ee

workflow-api 2.16

eb0ad8f

jglick merged commit 3ec591d into jenkinsci:master May 30, 2017

jglick deleted the GroovyCategorySupport-JENKINS-26481 branch May 30, 2017 22:51

jglick mentioned this pull request Jun 1, 2017

Adding whitelist methods needed for DefaultGroovyMethods jenkinsci/script-security-plugin#124

Merged

abayer mentioned this pull request Jun 1, 2017

Tests for DGM running through script security in actual jobs. #139

Merged

This was referenced Nov 7, 2019

MPL-32 Added working implementation of the JenkinsRule unit tests griddynamics/mpl#49

Open

Improve unit test framework to execute in near sandboxed jenkins environment griddynamics/mpl#32

Open

jglick mentioned this pull request Mar 29, 2024

More efficient use of GroovyCategorySupport #877

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-26481] [JENKINS-27421] Use GroovyCategorySupport to invoke CpsDefaultGroovyMethods (w/o DGMPatcher) & IteratorHack #124

[JENKINS-26481] [JENKINS-27421] Use GroovyCategorySupport to invoke CpsDefaultGroovyMethods (w/o DGMPatcher) & IteratorHack #124

jglick commented May 11, 2017 •

edited

jglick May 11, 2017

jglick May 11, 2017

jglick May 11, 2017

jglick May 11, 2017

jglick May 11, 2017

jglick May 11, 2017

jglick commented May 11, 2017 •

edited

jglick commented May 12, 2017

abayer left a comment

abayer May 12, 2017

abayer left a comment

jglick commented May 16, 2017

abayer commented May 16, 2017

jglick commented May 16, 2017

abayer commented May 16, 2017

jglick commented May 16, 2017

abayer commented May 16, 2017 •

edited

jglick commented May 16, 2017

jglick commented May 16, 2017

jglick commented May 23, 2017

jglick commented May 23, 2017

ghost commented May 23, 2017

jglick May 23, 2017

abayer May 23, 2017

jglick May 23, 2017

jglick May 23, 2017

jglick May 23, 2017

jglick May 23, 2017

abayer May 23, 2017

jglick May 24, 2017

abayer May 23, 2017

abayer May 23, 2017

jglick commented May 24, 2017

[JENKINS-26481] [JENKINS-27421] Use GroovyCategorySupport to invoke CpsDefaultGroovyMethods (w/o DGMPatcher) & IteratorHack #124

[JENKINS-26481] [JENKINS-27421] Use GroovyCategorySupport to invoke CpsDefaultGroovyMethods (w/o DGMPatcher) & IteratorHack #124

Conversation

jglick commented May 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented May 11, 2017 • edited

jglick commented May 12, 2017

abayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abayer left a comment

Choose a reason for hiding this comment

jglick commented May 16, 2017

abayer commented May 16, 2017

jglick commented May 16, 2017

abayer commented May 16, 2017

jglick commented May 16, 2017

abayer commented May 16, 2017 • edited

jglick commented May 16, 2017

jglick commented May 16, 2017

jglick commented May 23, 2017

jglick commented May 23, 2017

ghost commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jglick commented May 24, 2017

jglick commented May 11, 2017 •

edited

jglick commented May 11, 2017 •

edited

abayer commented May 16, 2017 •

edited