Merge pull request #929 from jglick/LoggingInvoker

jglick · web-flow · commit 19f11da8d2fa · 2024-08-27T17:11:57.000-04:00
False positives from `LoggingInvoker.isInternal`
diff --git a/plugin/pom.xml b/plugin/pom.xml
@@ -50,7 +50,7 @@
             <dependency>
                 <groupId>io.jenkins.tools.bom</groupId>
                 <artifactId>bom-2.426.x</artifactId>
-                <version>2982.vdce2153031a_0</version>
+                <version>3023.v02a_987a_b_3ff9</version>
                 <scope>import</scope>
                 <type>pom</type>
             </dependency>
@@ -80,7 +80,6 @@
         <dependency>
             <groupId>org.jenkins-ci.plugins</groupId>
             <artifactId>script-security</artifactId>
-            <version>1336.vf33a_a_9863911</version>
         </dependency>
         <dependency>
             <groupId>org.jenkins-ci.plugins</groupId>
diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecution.java
@@ -483,7 +483,7 @@ void recordInternalCall(@NonNull String call) {
     }
 
     @NonNull Set<String> getInternalCalls() {
-        return new TreeSet<>(internalCalls);
+        return internalCalls;
     }
 
     /**
@@ -2105,6 +2105,8 @@ public void autopersist(@NonNull FlowNode n) throws IOException {
             container.add(new Content("nodes/master/pipeline-internal-calls.txt") {
                 @Override public void writeTo(OutputStream outputStream) throws IOException {
                     PrintWriter pw = new PrintWriter(new OutputStreamWriter(outputStream, StandardCharsets.UTF_8));
+                    pw.println("Internal Jenkins API calls from the last build of any job (plus one example of such a build):");
+                    Map<String, String> internalCallsToExample = new TreeMap<>();
                     for (Job<?, ?> job : Jenkins.get().getAllItems(Job.class)) {
                         // TODO as above
                         if (job instanceof Queue.FlyweightTask) {
@@ -2114,19 +2116,17 @@ public void autopersist(@NonNull FlowNode n) throws IOException {
                                 if (owner != null) {
                                     FlowExecution exec = owner.getOrNull();
                                     if (exec instanceof CpsFlowExecution) {
-                                        Set<String> calls = ((CpsFlowExecution) exec).getInternalCalls();
-                                        if (!calls.isEmpty()) {
-                                            pw.println("Internal calls for " + run + ":");
-                                            for (String call : calls) {
-                                                pw.println("  " + call);
-                                            }
-                                            pw.println();
+                                        for (var call : ((CpsFlowExecution) exec).getInternalCalls()) {
+                                            internalCallsToExample.putIfAbsent(call, run.toString());
                                         }
                                     }
                                 }
                             }
                         }
                     }
+                    for (var entry : internalCallsToExample.entrySet()) {
+                        pw.println(entry.getKey() + " (" + entry.getValue() + ")");
+                    }
                     pw.flush();
                 }
             });
diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/IgnoredInternalClasses.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/IgnoredInternalClasses.java
@@ -0,0 +1,39 @@
+/*
+ * The MIT License
+ *
+ * Copyright 2024 CloudBees, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+package org.jenkinsci.plugins.workflow.cps;
+
+import hudson.ExtensionPoint;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.Beta;
+
+/**
+ * Allows selected classes to be ignored for purposes of {@link LoggingInvoker#isInternal}.
+ */
+@Restricted(value = Beta.class)
+public interface IgnoredInternalClasses extends ExtensionPoint {
+
+    boolean ignore(Class<?> clazz);
+
+}
diff --git a/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/LoggingInvoker.java b/plugin/src/main/java/org/jenkinsci/plugins/workflow/cps/LoggingInvoker.java
@@ -31,8 +31,11 @@
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
 import groovy.lang.GroovyClassLoader;
+import hudson.ExtensionList;
+import java.util.Set;
 import java.util.function.Supplier;
 import jenkins.util.SystemProperties;
+import org.jenkinsci.plugins.workflow.support.steps.build.RunWrapper;
 
 /**
  * Captures CPS-transformed events.
@@ -64,8 +67,17 @@ private void record(String call) {
         execution.recordInternalCall(call);
     }
 
+    private static final Set<Class<?>> IGNORED = Set.of(
+        Safepoint.class,
+        CpsClosure2.class,
+        EnvActionImpl.class,
+        RunWrapper.class);
+
     private static boolean isInternal(Class<?> clazz) {
-        if (clazz == Safepoint.class || clazz == CpsClosure2.class) {
+        if (IGNORED.contains(clazz)) {
+            return false;
+        }
+        if (ExtensionList.lookup(IgnoredInternalClasses.class).stream().anyMatch(iic -> iic.ignore(clazz))) {
             return false;
         }
         if (clazz.getClassLoader() instanceof GroovyClassLoader) { // similar to GroovyClassLoaderWhitelist
@@ -75,7 +87,7 @@ private static boolean isInternal(Class<?> clazz) {
         // (simply checking whether the class loader can “see”, say, jenkins/model/Jenkins.class
         // would falsely mark third-party libs bundled in Jenkins plugins)
         String name = clazz.getName();
-        if (name.startsWith("com.cloudbees.groovy.cps.")) {
+        if (name.startsWith("com.cloudbees.groovy.cps.") || name.startsWith("org.jenkinsci.plugins.workflow.cps.persistence.IteratorHack$")) {
             // Likely synthetic call, as to CpsDefaultGroovyMethods.
             return false;
         }
diff --git a/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java b/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/CpsFlowExecutionTest.java
@@ -24,8 +24,8 @@
 
 package org.jenkinsci.plugins.workflow.cps;
 
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.containsString;
+import com.cloudbees.jenkins.support.api.Container;
+import com.cloudbees.jenkins.support.api.Content;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.instanceOf;
@@ -49,6 +49,7 @@
 import hudson.model.Item;
 import hudson.model.Result;
 import hudson.model.TaskListener;
+import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.IOException;
 import java.io.Serializable;
@@ -72,10 +73,14 @@
 import java.util.stream.Collectors;
 import jenkins.model.Jenkins;
 import org.apache.commons.io.FileUtils;
-import org.hamcrest.Matchers;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.arrayContaining;
 import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsInRelativeOrder;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.empty;
+import static org.hamcrest.Matchers.not;
 import org.htmlunit.ElementNotFoundException;
 import org.htmlunit.FailingHttpStatusCodeException;
 import org.htmlunit.HttpMethod;
@@ -493,7 +498,7 @@ private static final class Execution extends StepExecution {
                     Thread.sleep(100); // apparently a race condition between CpsVmExecutorService.tearDown and WorkflowRun.finish
                 }
                 // TODO https://github.com/jenkinsci/workflow-cps-plugin/pull/570#issuecomment-1192679404 message can be duplicated
-                assertThat(logger.getRecords(), Matchers.not(Matchers.empty()));
+                assertThat(logger.getRecords(), not(empty()));
                 assertEquals(CpsFlowExecution.TimingKind.values().length, ((CpsFlowExecution) b.getExecution()).liveTimings.keySet().size());
         });
     }
@@ -511,16 +516,43 @@ private static final class Execution extends StepExecution {
             assertThat(new XmlFile(new File(b.getRootDir(), "build.xml")).asString(), containsString(
                 "<string>org.jenkinsci.plugins.workflow.job.WorkflowRun.description</string>"));
             CpsFlowExecution exec = (CpsFlowExecution) b.getExecution();
-            assertThat(exec.getInternalCalls(), contains(
-                "org.jenkinsci.plugins.workflow.job.WorkflowRun.description",
-                "org.jenkinsci.plugins.workflow.support.steps.build.RunWrapper.rawBuild"));
+            assertThat(exec.getInternalCalls(), containsInAnyOrder(
+                "org.jenkinsci.plugins.workflow.job.WorkflowRun.description"));
             SemaphoreStep.success("wait/1", null);
             r.assertBuildStatusSuccess(r.waitForCompletion(b));
-            assertThat(exec.getInternalCalls(), contains(
+            assertThat(exec.getInternalCalls(), containsInAnyOrder(
                 "hudson.model.Hudson.systemMessage",
                 "jenkins.model.Jenkins.instance",
-                "org.jenkinsci.plugins.workflow.job.WorkflowRun.description",
-                "org.jenkinsci.plugins.workflow.support.steps.build.RunWrapper.rawBuild"));
+                "org.jenkinsci.plugins.workflow.job.WorkflowRun.description"));
+        });
+    }
+
+    @Test public void internalCallsUniquified() throws Throwable {
+        sessions.then(r -> {
+            var p1 = r.jenkins.createProject(WorkflowJob.class, "project-1");
+            p1.setDefinition(new CpsFlowDefinition("currentBuild.rawBuild.description = 'XXX'; Jenkins.instance.systemMessage = 'XXX'", false));
+            r.buildAndAssertSuccess(p1);
+            var p2 = r.jenkins.createProject(WorkflowJob.class, "project-2");
+            p2.setDefinition(new CpsFlowDefinition("Jenkins.instance.systemMessage = Jenkins.VERSION", false));
+            r.buildAndAssertSuccess(p2);
+            var p3 = r.jenkins.createProject(WorkflowJob.class, "project-3");
+            p3.setDefinition(new CpsFlowDefinition("echo 'clean'", false));
+            r.buildAndAssertSuccess(p3);
+            var baos = new ByteArrayOutputStream();
+            new CpsFlowExecution.PipelineInternalCalls().addContents(new Container() {
+                @Override public void add(Content content) {
+                    try {
+                        content.writeTo(baos);
+                    } catch (IOException x) {
+                        assert false : x;
+                    }
+                }
+            });
+            assertThat(baos.toString().replaceFirst(".+\r?\n", "").split("\r?\n"), arrayContaining(
+                "hudson.model.Hudson.systemMessage (project-1 #1)",
+                "jenkins.model.Jenkins.VERSION (project-2 #1)",
+                "jenkins.model.Jenkins.instance (project-1 #1)",
+                "org.jenkinsci.plugins.workflow.job.WorkflowRun.description (project-1 #1)"));
         });
     }
 
diff --git a/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/LoggingInvokerTest.java b/plugin/src/test/java/org/jenkinsci/plugins/workflow/cps/LoggingInvokerTest.java
@@ -24,25 +24,25 @@
 
 package org.jenkinsci.plugins.workflow.cps;
 
-import java.util.Arrays;
-import java.util.TreeSet;
 import org.jenkinsci.plugins.workflow.job.WorkflowJob;
 import org.jenkinsci.plugins.workflow.job.WorkflowRun;
-import org.junit.Test;
-import static org.junit.Assert.*;
 import org.junit.ClassRule;
+import org.junit.Test;
+import org.jvnet.hudson.test.BuildWatcher;
 import org.jvnet.hudson.test.JenkinsRule;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsInAnyOrder;
 
 public class LoggingInvokerTest {
 
     @ClassRule public static JenkinsRule r = new JenkinsRule();
+    @ClassRule public static final BuildWatcher bw = new BuildWatcher();
 
     @Test public void smokes() throws Exception {
         assertInternalCalls("currentBuild.rawBuild.description = 'XXX'; Jenkins.instance.systemMessage = 'XXX'", false,
             "hudson.model.Hudson.systemMessage",
             "jenkins.model.Jenkins.instance",
-            "org.jenkinsci.plugins.workflow.job.WorkflowRun.description",
-            "org.jenkinsci.plugins.workflow.support.steps.build.RunWrapper.rawBuild");
+            "org.jenkinsci.plugins.workflow.job.WorkflowRun.description");
     }
 
     @Test public void groovyCalls() throws Exception {
@@ -60,12 +60,20 @@ public class LoggingInvokerTest {
         assertInternalCalls("class X extends hudson.lifecycle.Lifecycle {void onReady() {super.onReady()}}; new X().getHudsonWar(); new X().onReady()", false);
     }
 
+    @Test public void envAction() throws Exception {
+        assertInternalCalls("env.XXX = 'yyy'; echo XXX", true);
+    }
+
+    @Test public void runWrapper() throws Exception {
+        assertInternalCalls("echo currentBuild.displayName", true);
+    }
+
     private void assertInternalCalls(String script, boolean sandbox, String... calls) throws Exception {
         WorkflowJob p = r.createProject(WorkflowJob.class);
         p.setDefinition(new CpsFlowDefinition(script, sandbox));
         WorkflowRun b = r.buildAndAssertSuccess(p);
         CpsFlowExecution exec = (CpsFlowExecution) b.getExecution();
-        assertEquals(new TreeSet<>(Arrays.asList(calls)).toString(), exec.getInternalCalls().toString());
+        assertThat(exec.getInternalCalls(), containsInAnyOrder(calls));
     }
 
 }

Original file line number	Diff line number	Diff line change
`@@ -483,7 +483,7 @@ void recordInternalCall(@NonNull String call) {`
`483`	`483`	`}`
`484`	`484`
`485`	`485`	`@NonNull Set<String> getInternalCalls() {`
`486`		`- return new TreeSet<>(internalCalls);`
	`486`	`+ return internalCalls;`
`487`	`487`	`}`
`488`	`488`
`489`	`489`	`/**`
`@@ -2105,6 +2105,8 @@ public void autopersist(@NonNull FlowNode n) throws IOException {`
`2105`	`2105`	`container.add(new Content("nodes/master/pipeline-internal-calls.txt") {`
`2106`	`2106`	`@Override public void writeTo(OutputStream outputStream) throws IOException {`
`2107`	`2107`	`PrintWriter pw = new PrintWriter(new OutputStreamWriter(outputStream, StandardCharsets.UTF_8));`
	`2108`	`+ pw.println("Internal Jenkins API calls from the last build of any job (plus one example of such a build):");`
	`2109`	`+ Map<String, String> internalCallsToExample = new TreeMap<>();`
`2108`	`2110`	`for (Job<?, ?> job : Jenkins.get().getAllItems(Job.class)) {`
`2109`	`2111`	`// TODO as above`
`2110`	`2112`	`if (job instanceof Queue.FlyweightTask) {`
`@@ -2114,19 +2116,17 @@ public void autopersist(@NonNull FlowNode n) throws IOException {`
`2114`	`2116`	`if (owner != null) {`
`2115`	`2117`	`FlowExecution exec = owner.getOrNull();`
`2116`	`2118`	`if (exec instanceof CpsFlowExecution) {`
`2117`		`- Set<String> calls = ((CpsFlowExecution) exec).getInternalCalls();`
`2118`		`- if (!calls.isEmpty()) {`
`2119`		`- pw.println("Internal calls for " + run + ":");`
`2120`		`- for (String call : calls) {`
`2121`		`- pw.println(" " + call);`
`2122`		`- }`
`2123`		`- pw.println();`
	`2119`	`+ for (var call : ((CpsFlowExecution) exec).getInternalCalls()) {`
	`2120`	`+ internalCallsToExample.putIfAbsent(call, run.toString());`
`2124`	`2121`	`}`
`2125`	`2122`	`}`
`2126`	`2123`	`}`
`2127`	`2124`	`}`
`2128`	`2125`	`}`
`2129`	`2126`	`}`
	`2127`	`+ for (var entry : internalCallsToExample.entrySet()) {`
	`2128`	`+ pw.println(entry.getKey() + " (" + entry.getValue() + ")");`
	`2129`	`+ }`
`2130`	`2130`	`pw.flush();`
`2131`	`2131`	`}`
`2132`	`2132`	`});`