This is an automated email from the ASF dual-hosted git repository. heneveld pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/brooklyn-server.git
commit 6eed5b3ba0ea52be90d354e840e1b3270b733c1d Author: Alex Heneveld <[email protected]> AuthorDate: Wed Nov 9 08:42:00 2022 +0000 guard against recursive variable evaluation in workflow --- .../brooklyn/camp/brooklyn/WorkflowYamlTest.java | 47 ++++++++++++++++++---- .../workflow/WorkflowExpressionResolution.java | 41 ++++++++++++++----- .../apache/brooklyn/util/core/task/BasicTask.java | 25 +++++++++--- 3 files changed, 90 insertions(+), 23 deletions(-) diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowYamlTest.java index effd85b8af..9cdacacdbb 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowYamlTest.java @@ -25,6 +25,7 @@ import com.google.common.collect.Iterables; import org.apache.brooklyn.api.effector.Effector; import org.apache.brooklyn.api.entity.Application; import org.apache.brooklyn.api.entity.Entity; +import org.apache.brooklyn.api.entity.EntityInitializer; import org.apache.brooklyn.api.entity.EntitySpec; import org.apache.brooklyn.api.location.LocationSpec; import org.apache.brooklyn.api.location.MachineLocation; @@ -36,24 +37,24 @@ import org.apache.brooklyn.api.sensor.AttributeSensor; import org.apache.brooklyn.api.typereg.RegisteredType; import org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.BrooklynDslCommon; import org.apache.brooklyn.core.config.ConfigKeys; -import org.apache.brooklyn.core.entity.Attributes; -import org.apache.brooklyn.core.entity.BrooklynConfigKeys; -import org.apache.brooklyn.core.entity.Dumper; -import org.apache.brooklyn.core.entity.EntityAsserts; +import org.apache.brooklyn.core.entity.*; import org.apache.brooklyn.core.entity.lifecycle.Lifecycle; import org.apache.brooklyn.core.entity.trait.Startable; import org.apache.brooklyn.core.sensor.Sensors; import org.apache.brooklyn.core.test.BrooklynAppUnitTestSupport; import org.apache.brooklyn.core.test.entity.TestApplication; +import org.apache.brooklyn.core.test.entity.TestEntity; import org.apache.brooklyn.core.typereg.BasicTypeImplementationPlan; import org.apache.brooklyn.core.typereg.JavaClassNameTypePlanTransformer; import org.apache.brooklyn.core.typereg.RegisteredTypes; import org.apache.brooklyn.core.workflow.*; import org.apache.brooklyn.core.workflow.steps.LogWorkflowStep; import org.apache.brooklyn.core.workflow.store.WorkflowStatePersistenceViaSensors; +import org.apache.brooklyn.entity.software.base.EmptySoftwareProcess; import org.apache.brooklyn.entity.software.base.WorkflowSoftwareProcess; import org.apache.brooklyn.entity.stock.BasicEntity; import org.apache.brooklyn.location.byon.FixedListMachineProvisioningLocation; +import org.apache.brooklyn.location.localhost.LocalhostMachineProvisioningLocation; import org.apache.brooklyn.location.ssh.SshMachineLocation; import org.apache.brooklyn.location.winrm.WinrmWorkflowStep; import org.apache.brooklyn.tasks.kubectl.ContainerEffectorTest; @@ -664,8 +665,8 @@ public class WorkflowYamlTest extends AbstractYamlTest { Asserts.assertEquals(result, 11); } - @Test - public void testEchoBashCommandAsWorkflowEffectorWithVarFromConfig() throws Exception { + @Test(groups="Live") + public void testContainerEchoBashCommandAsWorkflowEffectorWithVarFromConfig() throws Exception { WorkflowBasicTest.addRegisteredTypeBean(mgmt(), "container", ContainerWorkflowStep.class); BrooklynDslCommon.registerSerializationHooks(); final String message = ("hello " + Strings.makeRandomId(10)).toLowerCase(); @@ -677,7 +678,7 @@ public class WorkflowYamlTest extends AbstractYamlTest { ConfigBag parameters = ConfigBag.newInstance(ImmutableMap.of( WorkflowEffector.EFFECTOR_NAME, "test-container-effector", WorkflowEffector.STEPS, MutableList.of( - MutableMap.<String, Object>of("s", "container " + ContainerEffectorTest.BASH_SCRIPT_CONTAINER + " echo " + message + " $VAR", + MutableMap.<String, Object>of("step", "container " + ContainerEffectorTest.BASH_SCRIPT_CONTAINER + " echo " + message + " $VAR", "input", MutableMap.of("env", MutableMap.of("VAR", "$brooklyn:config(\"hello\")")), "output", "${stdout}")))); @@ -687,5 +688,37 @@ public class WorkflowYamlTest extends AbstractYamlTest { Asserts.assertEquals(output.toString().trim(), message + " world"); } + @Test(groups="Live") + public void testSshEchoBashCommandAsWorkflowEffectorWithVarFromConfig() throws Exception { + WorkflowBasicTest.addRegisteredTypeBean(mgmt(), "container", ContainerWorkflowStep.class); + BrooklynDslCommon.registerSerializationHooks(); + final String message = ("hello " + Strings.makeRandomId(10)).toLowerCase(); + + EntitySpec<TestApplication> appSpec = EntitySpec.create(TestApplication.class); + TestApplication app = mgmt().getEntityManager().createEntity(appSpec); + + ConfigBag parameters = ConfigBag.newInstance(ImmutableMap.of( + WorkflowEffector.EFFECTOR_NAME, "test-ssh-effector", + WorkflowEffector.PARAMETER_DEFS, MutableMap.of("env", null), + WorkflowEffector.STEPS, MutableList.of( + MutableMap.of("step", "let map env_local", "value", MutableMap.of("VAR1", "$brooklyn:config(\"hello\")", "ENTITY_ID", "$brooklyn:entityId()")), + "let merge map env = ${env} ${env_local}", + MutableMap.<String, Object>of("step", "ssh echo "+ message+" Entity:$ENTITY_ID:$VAR1:$VAR2", + "input", + MutableMap.of("env", "${env}"), + "output", "${stdout}")))); + + WorkflowEffector initializer = new WorkflowEffector(parameters); + + EmptySoftwareProcess child = app.createAndManageChild(EntitySpec.create(EmptySoftwareProcess.class).location(LocationSpec.create(LocalhostMachineProvisioningLocation.class)). + addInitializer(initializer)); + app.start(ImmutableList.of()); + child.config().set(ConfigKeys.newStringConfigKey("hello"), "world"); + + Object output = Entities.invokeEffector(app, child, ((EntityInternal) child).getEffector("test-ssh-effector"), MutableMap.of("env", MutableMap.of("VAR2","Arg1"))).getUnchecked(Duration.ONE_MINUTE); + + Asserts.assertEquals(output.toString().trim(), message + " Entity:"+app.getChildren().iterator().next().getId()+":"+"world:Arg1"); + } + } diff --git a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowExpressionResolution.java b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowExpressionResolution.java index 5baf4b50d6..90aa7b6b05 100644 --- a/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowExpressionResolution.java +++ b/core/src/main/java/org/apache/brooklyn/core/workflow/WorkflowExpressionResolution.java @@ -27,7 +27,9 @@ import org.apache.brooklyn.core.resolve.jackson.BeanWithTypeUtils; import org.apache.brooklyn.core.resolve.jackson.BrooklynJacksonSerializationUtils; import org.apache.brooklyn.core.typereg.RegisteredTypes; import org.apache.brooklyn.util.collections.Jsonya; +import org.apache.brooklyn.util.collections.MutableList; import org.apache.brooklyn.util.collections.MutableMap; +import org.apache.brooklyn.util.collections.MutableSet; import org.apache.brooklyn.util.core.flags.TypeCoercions; import org.apache.brooklyn.util.core.predicates.ResolutionFailureTreatedAsAbsent; import org.apache.brooklyn.util.core.task.DeferredSupplier; @@ -38,14 +40,12 @@ import org.apache.brooklyn.util.core.text.TemplateProcessor; import org.apache.brooklyn.util.exceptions.Exceptions; import org.apache.brooklyn.util.guava.Maybe; import org.apache.brooklyn.util.javalang.Boxing; +import org.apache.commons.lang3.tuple.Pair; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import javax.swing.*; -import java.util.Collection; -import java.util.Map; -import java.util.Objects; -import java.util.Set; +import java.util.*; import java.util.stream.Collectors; public class WorkflowExpressionResolution { @@ -233,13 +233,34 @@ public class WorkflowExpressionResolution { } } + static ThreadLocal<Set<Pair<WorkflowExecutionContext,Object>>> RESOLVE_STACK = new ThreadLocal<>(); + public Object processTemplateExpression(Object expression) { - if (expression instanceof String) return processTemplateExpressionString((String)expression); - if (expression instanceof Map) return processTemplateExpressionMap((Map)expression); - if (expression instanceof Collection) return processTemplateExpressionCollection((Collection)expression); - if (expression==null || Boxing.isPrimitiveOrBoxedObject(expression)) return expression; - // otherwise resolve DSL - return resolveDsl(expression); + Set<Pair<WorkflowExecutionContext,Object>> stack = null; + try { + stack = RESOLVE_STACK.get(); + if (stack==null) { + stack = MutableSet.of(); + RESOLVE_STACK.set(stack); + } + if (!stack.add(Pair.of(context, expression))) { + throw new IllegalStateException("Recursive reference: "+stack.stream().map(p -> ""+p.getRight()).collect(Collectors.joining("->"))); + } + if (stack.size()>100) { + throw new IllegalStateException("Reference exceeded max depth 100: "+stack.stream().map(p -> ""+p.getRight()).collect(Collectors.joining("->"))); + } + + if (expression instanceof String) return processTemplateExpressionString((String) expression); + if (expression instanceof Map) return processTemplateExpressionMap((Map) expression); + if (expression instanceof Collection) return processTemplateExpressionCollection((Collection) expression); + if (expression == null || Boxing.isPrimitiveOrBoxedObject(expression)) return expression; + // otherwise resolve DSL + return resolveDsl(expression); + + } finally { + stack.remove(Pair.of(context, expression)); + if (stack.isEmpty()) RESOLVE_STACK.remove(); + } } private Object resolveDsl(Object expression) { diff --git a/core/src/main/java/org/apache/brooklyn/util/core/task/BasicTask.java b/core/src/main/java/org/apache/brooklyn/util/core/task/BasicTask.java index 8cf5c69b69..625bf89fbb 100644 --- a/core/src/main/java/org/apache/brooklyn/util/core/task/BasicTask.java +++ b/core/src/main/java/org/apache/brooklyn/util/core/task/BasicTask.java @@ -808,6 +808,8 @@ public class BasicTask<T> implements TaskInternal<T> { } } + private transient String loggedLongStack = null; + protected void computeStatusStringError(int verbosity, StatusStringData data) { Throwable error = Tasks.getError(this, false); if (error!=null) { @@ -819,13 +821,24 @@ public class BasicTask<T> implements TaskInternal<T> { boolean isCancelled = isCancelled() && error instanceof CancellationException; if (!isCancelled) data.oneLineData.add(": " + abbreviate(errorMessage)); if (verbosity >= 2) { - StringWriter sw = new StringWriter(); - error.printStackTrace(new PrintWriter(sw)); - String sws = sw.toString(); - if (isCancelled && sws.contains(BasicTask.class.getName()+"."+"computeStatusStringError")) { - // don't add the cancellation exception generated by this call + if (loggedLongStack!=null) { + data.multiLineData.add(loggedLongStack); } else { - data.multiLineData.add(sws); + StringWriter sw = new StringWriter(); + error.printStackTrace(new PrintWriter(sw)); + String sws = sw.toString(); + if (isCancelled && sws.contains(BasicTask.class.getName() + "." + "computeStatusStringError")) { + // don't add the cancellation exception generated by this call + } else { + if (sws.length() > 80 * 100) { + // shorten a bit, if long; esp if there is a circular reference + loggedLongStack = sws.substring(0, 40 * 100) + "\n ...\n ..." + sws.substring(sws.length() - 40 * 100); + log.warn("Long stack trace suppressed when reporting status of task " + getId() + ":\n" + sws); + data.multiLineData.add(loggedLongStack); + } else { + data.multiLineData.add(sws); + } + } } } }
