[JENKINS-71509] Clean up of `GradleEnterpriseExceptionTaskListenerDecorator` #318

jglick · 2023-07-11T20:18:41Z

Amends #312 by @alextu. While that appears to have worked, the way in which it worked was not reassuring—an exception is still thrown, but it gets caught and does not break withCredentials masking. Better to avoid the exception to begin with. jenkinsci/workflow-durable-task-step-plugin#323 proposes to enable USE_WATCHING by default, in which case the scenario would apply to all gradle plugin users, not only those who are also running opentelemetry.

Running

git checkout gradle-2.8 -- src/main/java/hudson/plugins/gradle/injection/GradleEnterpriseExceptionTaskListenerDecoratorFactory.java
./gradlew -i :test --tests hudson.plugins.gradle.injection.BuildScanInjectionGradleWithDurableTaskStepUseWatchingIntegrationTest."credentials are always masked in logs"

you can see the problem:

… org.jenkinsci.plugins.workflow.log.TaskListenerDecorator$DecoratedTaskListener getLogger
WARNING: null
java.lang.IllegalStateException: Expected 1 instance of hudson.plugins.gradle.injection.InjectionConfig but got 0
    at hudson.ExtensionList.lookupSingleton(ExtensionList.java:452)
    at hudson.plugins.gradle.injection.InjectionConfig.get(InjectionConfig.java:81)
    at hudson.plugins.gradle.injection.GradleEnterpriseExceptionTaskListenerDecoratorFactory$GradleEnterpriseExceptionTaskListenerDecorator.decorate(GradleEnterpriseExceptionTaskListenerDecoratorFactory.java:43)
    at org.jenkinsci.plugins.workflow.log.TaskListenerDecorator$MergedTaskListenerDecorator.decorate(TaskListenerDecorator.java:176)
    at org.jenkinsci.plugins.workflow.log.TaskListenerDecorator$DecoratedTaskListener.getLogger(TaskListenerDecorator.java:240)
    at org.jenkinsci.plugins.workflow.log.TaskListenerDecorator$CloseableTaskListener.getLogger(TaskListenerDecorator.java:283)
    at org.jenkinsci.plugins.workflow.steps.durable_task.DurableTaskStep$Execution$NewlineSafeTaskListener.getLogger(DurableTaskStep.java:455)
    at org.jenkinsci.plugins.workflow.steps.durable_task.DurableTaskStep$HandlerImpl.output(DurableTaskStep.java:720)
    at org.jenkinsci.plugins.durabletask.FileMonitoringTask$Watcher.run(FileMonitoringTask.java:616)

As Javadoc says, trying to access controller-only facilities such as ExtensionList from within decorate is not permitted. In fact the current GradleEnterpriseExceptionTaskListenerDecorator could not possibly work remotely because GradleEnterpriseExceptionLogProcessor uses DefaultBuildAgentErrorListener which can only run inside the controller as currently designed (accessing Run objects and so forth). So unless and until that is redesigned to permit it to run inside the agent—ideally by streaming events directly to Gradle Enterprise, but failing that by calling back to the controller somewhat like TimeoutStep does (discussion)—this feature just needs to be suppressed on the log of sh steps running remotely.

CC @daniel-beck

…orator`

jglick · 2023-07-11T20:20:38Z

src/main/java/hudson/plugins/gradle/injection/GradleEnterpriseExceptionLogFilter.java

    public OutputStream decorateLogger(Run build, OutputStream logger) {
        InjectionConfig injectionConfig = InjectionConfig.get();
-        if (injectionConfig.isEnabled() && injectionConfig.isCheckForBuildAgentErrors()) {
+        if (injectionConfig.isEnabled() && injectionConfig.isCheckForBuildAgentErrors() && build != null) {


Probably unnecessary, but core method does not specify nullability of the build argument, so being defensive.

src/main/java/hudson/plugins/gradle/injection/GradleEnterpriseExceptionLogProcessor.java

jglick · 2023-07-11T20:22:49Z

...a/hudson/plugins/gradle/injection/GradleEnterpriseExceptionTaskListenerDecoratorFactory.java

        @Override
        public OutputStream decorate(@Nonnull OutputStream logger) {
-            if (isBuildAgentErrorsEnabled()) {
+            if (run != null) {


The fix. We cannot call isBuildAgentErrorsEnabled from the agent side, and even if this value were computed on the controller side and serialized, it would not help because run would still have been left null by deserialization and could not be used here. So simply disable the decorator when remoted.

Could we use a more expressive condition to "know" we're on the controller ? Maybe jenkins.util.JenkinsJVM#isJenkinsJVM ?
As an additional safety net, I think we also need to combine the condition with isBuildAgentErrorsEnabled() (so only executed on the controller) even if it's already called upfront in hudson.plugins.gradle.injection.GradleEnterpriseExceptionTaskListenerDecoratorFactory#of.
Btw, org.jenkinsci.plugins.workflow.log.TaskListenerDecorator.Factory#of implementations are executed only on the controller, right?

Could we use a more expressive condition to "know" we're on the controller ? Maybe jenkins.util.JenkinsJVM#isJenkinsJVM ?

We could, though it should not matter—if running on an agent, certainly run == null. A checkJenkinsJVM call could be added (inside the run != null block) as a matter of documentation, though it would perhaps just confuse the situation if later you do implement remote decoration (using some data structure that is safe in the agent JVM unlike Run).

we also need to combine the condition with isBuildAgentErrorsEnabled()

Seems redundant since of already runs this check.

#of implementations are executed only on the controller, right?

Right.

jglick · 2023-07-11T20:23:25Z

...radle/injection/BuildScanInjectionGradleWithDurableTaskStepUseWatchingIntegrationTest.groovy

+        cleanup:
+        System.err.println('---%<--- agent logs')
+        agent.computer.logText.writeLogTo(0, System.err)
+        System.err.println('--->%---')


Otherwise you would not notice the errors. Currently JenkinsRule.create[Online]Slave does not stream logs. (InboundAgentRule does.)

ooh I see, I wondered as well why I could not see the error

jglick · 2023-07-11T20:25:52Z

src/test/groovy/hudson/plugins/gradle/injection/GradleSnippets.groovy

        withCredentials([string(credentialsId: 'my-creds', variable: 'PASSWORD')]) {
          if (isUnix()) {
-            sh "echo password=\$PASSWORD"
+            sh 'echo password=$PASSWORD'


The original test was improperly interpolating the password in Groovy code on the controller, rather than using the environment variable, and so actually running the shell with e.g.

echo password=actual secret here

Pipeline prints a warning about this to the build log. Does not make much difference for purposes of the masking issue (since either way the shell step output contains the secret), but better to test the script that users are actually supposed to write.

I did not see this warning in the test logs 🤔 but I understand the issue, quite sneaky!

…ond constructor was used from `GradleEnterpriseExceptionLogProcessorTest` mocks

jglick · 2023-07-12T14:04:17Z

@alextu there are ten test failures here, but these seem to be failing already in trunk.

alextu · 2023-07-12T16:06:28Z

@alextu there are ten test failures here, but these seem to be failing already in trunk.

Thanks for your contribution! We will review it tomorrow, yes we have flakiness on ci.jenkins.io that we did not get a chance to tackle yet (we also build internally on our CI to make sure those are not "real" failures)

alextu

Looks good to me, @c00ler could you take a look next week ?

alextu · 2023-07-13T15:22:21Z

...a/hudson/plugins/gradle/injection/GradleEnterpriseExceptionTaskListenerDecoratorFactory.java

        @Override
        public OutputStream decorate(@Nonnull OutputStream logger) {
-            if (isBuildAgentErrorsEnabled()) {
+            if (run != null) {


Could we use a more expressive condition to "know" we're on the controller ? Maybe jenkins.util.JenkinsJVM#isJenkinsJVM ?
As an additional safety net, I think we also need to combine the condition with isBuildAgentErrorsEnabled() (so only executed on the controller) even if it's already called upfront in hudson.plugins.gradle.injection.GradleEnterpriseExceptionTaskListenerDecoratorFactory#of.
Btw, org.jenkinsci.plugins.workflow.log.TaskListenerDecorator.Factory#of implementations are executed only on the controller, right?

alextu · 2023-07-13T15:26:54Z

...radle/injection/BuildScanInjectionGradleWithDurableTaskStepUseWatchingIntegrationTest.groovy

+        cleanup:
+        System.err.println('---%<--- agent logs')
+        agent.computer.logText.writeLogTo(0, System.err)
+        System.err.println('--->%---')


ooh I see, I wondered as well why I could not see the error

alextu · 2023-07-13T15:46:50Z

src/test/groovy/hudson/plugins/gradle/injection/GradleSnippets.groovy

        withCredentials([string(credentialsId: 'my-creds', variable: 'PASSWORD')]) {
          if (isUnix()) {
-            sh "echo password=\$PASSWORD"
+            sh 'echo password=$PASSWORD'


I did not see this warning in the test logs 🤔 but I understand the issue, quite sneaky!

[JENKINS-71509] Clean up of `GradleEnterpriseExceptionTaskListenerDec…

6d6082f

…orator`

jglick commented Jul 11, 2023

View reviewed changes

jglick mentioned this pull request Jul 11, 2023

Catch errors in TaskListenerDecorator.decorate even when wrapped with merge jenkinsci/workflow-api-plugin#290

Merged

Reverting changes to GradleEnterpriseExceptionLogProcessor; the sec…

0c01119

…ond constructor was used from `GradleEnterpriseExceptionLogProcessorTest` mocks

alextu approved these changes Jul 13, 2023

View reviewed changes

c00ler added the bugfix label Jul 19, 2023

c00ler approved these changes Jul 19, 2023

View reviewed changes

c00ler merged commit 003c8b7 into jenkinsci:master Jul 21, 2023

jglick deleted the GradleEnterpriseExceptionTaskListenerDecorator-JENKINS-71509 branch July 21, 2023 12:03

Uh oh!

[JENKINS-71509] Clean up of GradleEnterpriseExceptionTaskListenerDecorator #318

[JENKINS-71509] Clean up of GradleEnterpriseExceptionTaskListenerDecorator #318

Uh oh!

Conversation

jglick commented Jul 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick commented Jul 12, 2023

Uh oh!

alextu commented Jul 12, 2023

Uh oh!

alextu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[JENKINS-71509] Clean up of `GradleEnterpriseExceptionTaskListenerDecorator` #318

[JENKINS-71509] Clean up of `GradleEnterpriseExceptionTaskListenerDecorator` #318

jglick commented Jul 11, 2023 •

edited

Loading

alextu left a comment •

edited

Loading