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 aea5fae7fa7a7e7627f3bac37e0e0ce476d0f0de Author: Alex Heneveld <[email protected]> AuthorDate: Thu Nov 10 14:59:47 2022 +0000 better persistence and deserialization of dsl expressions to guarantee they are not read into json-pass-through steps as json --- .../spi/dsl/BrooklynDslDeferredSupplier.java | 20 +- .../camp/brooklyn/DslAndRebindYamlTest.java | 3 +- .../camp/brooklyn/WorkflowYamlRebindTest.java | 209 +++++++++++++++++++++ .../brooklyn/camp/brooklyn/WorkflowYamlTest.java | 12 +- .../jackson/BrooklynJacksonSerializationUtils.java | 6 + .../util/json/BrooklynJacksonSerializerTest.java | 27 +++ 6 files changed, 263 insertions(+), 14 deletions(-) diff --git a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/BrooklynDslDeferredSupplier.java b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/BrooklynDslDeferredSupplier.java index e3bb0548c4..04eee1cb34 100644 --- a/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/BrooklynDslDeferredSupplier.java +++ b/camp/camp-brooklyn/src/main/java/org/apache/brooklyn/camp/brooklyn/spi/dsl/BrooklynDslDeferredSupplier.java @@ -39,6 +39,7 @@ import org.apache.brooklyn.util.core.task.ImmediateSupplier; import org.apache.brooklyn.util.core.task.TaskTags; import org.apache.brooklyn.util.core.task.Tasks; import org.apache.brooklyn.util.exceptions.Exceptions; +import org.apache.commons.lang3.tuple.Pair; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -76,15 +77,26 @@ public abstract class BrooklynDslDeferredSupplier<T> implements DeferredSupplier /** The original DSL which generated the expression, if available. * Note the {@link #toString()} should create an equivalent expression. */ - // not required in persistence; potentially interesting, but increases size significantly - // xstream deserialization should use the toString, and skips transients; + // now included in xstream persistence, so that jackson reconstruction (for steps) has access to it; + // but cleaned up so it is only applied to the outermost DSL object (last one created for given parse node); // jackson deserialization includes this, and relies on it if reading an Object, // but if reading to a Supplier it will correctly instantiate based on the type field. - protected transient Object dsl = null; + protected Object dsl = null; + protected static ThreadLocal<Pair<Integer,BrooklynDslDeferredSupplier>> lastSourceNode = new ThreadLocal<>(); public BrooklynDslDeferredSupplier() { PlanInterpretationNode sourceNode = BrooklynDslInterpreter.currentNode(); - dsl = sourceNode!=null ? sourceNode.getOriginalValue() : null; + if (sourceNode!=null && sourceNode.getOriginalValue()!=null) { + Pair<Integer, BrooklynDslDeferredSupplier> last = lastSourceNode.get(); + if (last!=null && last.getLeft().equals(sourceNode.hashCode())) { + // set DSL on the last object created, and clear on previous ones; + // a bit of a hack, might be better to do on callers, ideally ensure all calls come from same CAMP parse caller + // (which i think they probably do) + last.getRight().dsl = null; + } + dsl = sourceNode.getOriginalValue(); + lastSourceNode.set(Pair.of(sourceNode.hashCode(), this)); + } } /** returns the current entity; for use in implementations of {@link #get()} */ diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/DslAndRebindYamlTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/DslAndRebindYamlTest.java index 67cb48fe88..14964937b3 100644 --- a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/DslAndRebindYamlTest.java +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/DslAndRebindYamlTest.java @@ -220,6 +220,7 @@ public class DslAndRebindYamlTest extends AbstractYamlRebindTest { // // <test.confName> // <org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.DslComponent_-AttributeWhenReady> + // <dsl class="string">$brooklyn:component("x").attributeWhenReady("foo")</dsl> // <component> // <componentId>x</componentId> // <scope>GLOBAL</scope> @@ -228,7 +229,7 @@ public class DslAndRebindYamlTest extends AbstractYamlRebindTest { // </org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.DslComponent_-AttributeWhenReady> // </test.confName> - Assert.assertTrue(testConfNamePersistedState.length() < 400, "persisted state too long: " + testConfNamePersistedState); + Assert.assertTrue(testConfNamePersistedState.length() < 500, "persisted state too long: " + testConfNamePersistedState); Assert.assertFalse(testConfNamePersistedState.contains("bar"), "value 'bar' leaked in persisted state"); } diff --git a/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowYamlRebindTest.java b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowYamlRebindTest.java new file mode 100644 index 0000000000..71922a792c --- /dev/null +++ b/camp/camp-brooklyn/src/test/java/org/apache/brooklyn/camp/brooklyn/WorkflowYamlRebindTest.java @@ -0,0 +1,209 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.brooklyn.camp.brooklyn; + +import com.google.common.base.Stopwatch; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; +import org.apache.brooklyn.api.entity.Entity; +import org.apache.brooklyn.api.entity.EntityLocal; +import org.apache.brooklyn.api.entity.EntitySpec; +import org.apache.brooklyn.api.location.LocationSpec; +import org.apache.brooklyn.api.mgmt.Task; +import org.apache.brooklyn.api.sensor.AttributeSensor; +import org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.BrooklynDslCommon; +import org.apache.brooklyn.core.config.ConfigKeys; +import org.apache.brooklyn.core.entity.Entities; +import org.apache.brooklyn.core.entity.EntityAsserts; +import org.apache.brooklyn.core.entity.StartableApplication; +import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext; +import org.apache.brooklyn.core.mgmt.rebind.RebindOptions; +import org.apache.brooklyn.core.sensor.Sensors; +import org.apache.brooklyn.core.test.entity.TestApplication; +import org.apache.brooklyn.core.test.entity.TestEntity; +import org.apache.brooklyn.core.workflow.WorkflowBasicTest; +import org.apache.brooklyn.core.workflow.WorkflowEffector; +import org.apache.brooklyn.entity.software.base.EmptySoftwareProcess; +import org.apache.brooklyn.entity.stock.BasicApplication; +import org.apache.brooklyn.entity.stock.BasicEntity; +import org.apache.brooklyn.location.localhost.LocalhostMachineProvisioningLocation; +import org.apache.brooklyn.tasks.kubectl.ContainerEffectorTest; +import org.apache.brooklyn.tasks.kubectl.ContainerWorkflowStep; +import org.apache.brooklyn.test.Asserts; +import org.apache.brooklyn.util.collections.MutableList; +import org.apache.brooklyn.util.collections.MutableMap; +import org.apache.brooklyn.util.core.config.ConfigBag; +import org.apache.brooklyn.util.text.Strings; +import org.apache.brooklyn.util.time.Duration; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import java.io.File; +import java.util.Map; +import java.util.function.Predicate; + +public class WorkflowYamlRebindTest extends AbstractYamlRebindTest { + + @BeforeMethod(alwaysRun = true) + @Override + public void setUp() throws Exception { + super.setUp(); + WorkflowYamlTest.addWorkflowTypes(mgmt()); + } + + @Override + protected LocalManagementContext createNewManagementContext(File mementoDir, Map<?, ?> additionalProperties) { + LocalManagementContext result = super.createNewManagementContext(mementoDir, additionalProperties); + WorkflowYamlTest.addWorkflowTypes(result); + return result; + } + + @Test + public void testEffectorArgDslInMap() throws Exception { + BrooklynDslCommon.registerSerializationHooks(); + BasicApplication app = mgmt().getEntityManager().createEntity(EntitySpec.create(BasicApplication.class).configure("z", "Z")); + + WorkflowEffector eff = new WorkflowEffector(ConfigBag.newInstance() + .configure(WorkflowEffector.EFFECTOR_NAME, "myWorkflowEffector3") + .configure(WorkflowEffector.EFFECTOR_PARAMETER_DEFS, MutableMap.of("x", MutableMap.of("type", "map"))) + .configure(WorkflowEffector.STEPS, MutableList.of("return ${x}"))); + eff.apply((EntityLocal)app); + eff = new WorkflowEffector(ConfigBag.newInstance() + .configure(WorkflowEffector.EFFECTOR_NAME, "myWorkflowEffector2") + .configure(WorkflowEffector.EFFECTOR_PARAMETER_DEFS, MutableMap.of("x", MutableMap.of())) + .configure(WorkflowEffector.STEPS, MutableList.of(MutableMap.of("step", "invoke-effector myWorkflowEffector3", + "args", MutableMap.of("x", "${x}"))))); + eff.apply((EntityLocal)app); + eff = new WorkflowEffector(ConfigBag.newInstance() + .configure(WorkflowEffector.EFFECTOR_NAME, "myWorkflowEffector1") + .configure(WorkflowEffector.EFFECTOR_PARAMETER_DEFS, MutableMap.of("x", MutableMap.of())) + .configure(WorkflowEffector.STEPS, MutableList.of(MutableMap.of("step", "invoke-effector myWorkflowEffector2", + "args", MutableMap.of("x", MutableMap.of("y", "$brooklyn:config(\"z\")")))))); + eff.apply((EntityLocal)app); + + Task<?> invocation = app.invoke(app.getEntityType().getEffectorByName("myWorkflowEffector1").get(), MutableMap.of()); + Asserts.assertEquals(invocation.getUnchecked(), MutableMap.of("y","Z")); + + app = (BasicApplication) rebind(); + + invocation = app.invoke(app.getEntityType().getEffectorByName("myWorkflowEffector1").get(), MutableMap.of()); + Asserts.assertEquals(invocation.getUnchecked(), MutableMap.of("y","Z")); + } + + @Test(groups="Live") + public void testEffectorSshEnvArgDslInMap() throws Exception { + BrooklynDslCommon.registerSerializationHooks(); + TestApplication app = mgmt().getEntityManager().createEntity(EntitySpec.create(TestApplication.class).configure("z", "Z")); + + EmptySoftwareProcess child = app.createAndManageChild(EntitySpec.create(EmptySoftwareProcess.class).location(LocationSpec.create(LocalhostMachineProvisioningLocation.class))); + app.start(ImmutableList.of()); + + WorkflowEffector eff = new WorkflowEffector(ConfigBag.newInstance() + .configure(WorkflowEffector.EFFECTOR_NAME, "myWorkflowEffector3") + .configure(WorkflowEffector.EFFECTOR_PARAMETER_DEFS, MutableMap.of("script", MutableMap.of(), "env", MutableMap.of("defaultValue", MutableMap.of()))) + .configure(WorkflowEffector.STEPS, MutableList.of( + MutableMap.of("type", "ssh", + "command", "bash -c \"${script}\"", + "env", "${env}"), + "return ${stdout}"))); + eff.apply((EntityLocal)child); + eff = new WorkflowEffector(ConfigBag.newInstance() + .configure(WorkflowEffector.EFFECTOR_NAME, "myWorkflowEffector2") + .configure(WorkflowEffector.EFFECTOR_PARAMETER_DEFS, MutableMap.of("script", MutableMap.of(), "env", MutableMap.of("defaultValue", MutableMap.of()))) + .configure(WorkflowEffector.STEPS, MutableList.of(MutableMap.of("step", "invoke-effector myWorkflowEffector3", + "args", MutableMap.of("script", "${script}", "env", "${env}"))))); + eff.apply((EntityLocal)child); + eff = new WorkflowEffector(ConfigBag.newInstance() + .configure(WorkflowEffector.EFFECTOR_NAME, "myWorkflowEffector1") + .configure(WorkflowEffector.STEPS, MutableList.of(MutableMap.of("step", "invoke-effector myWorkflowEffector2", + "args", MutableMap.of("script", "echo Y is $Y", "env", MutableMap.of("Y", "$brooklyn:config(\"z\")")))))); + eff.apply((EntityLocal)child); + + Task<?> invocation = child.invoke(child.getEntityType().getEffectorByName("myWorkflowEffector1").get(), MutableMap.of()); + Asserts.assertEquals(invocation.getUnchecked().toString().trim(), "Y is Z"); + + app = (TestApplication) rebind(); + child = (EmptySoftwareProcess) Iterables.getLast(app.getChildren()); + + invocation = child.invoke(child.getEntityType().getEffectorByName("myWorkflowEffector1").get(), MutableMap.of()); + Asserts.assertEquals(invocation.getUnchecked().toString().trim(), "Y is Z"); + } + + @Test(groups="Live") + public void testContainerEchoBashCommandAsWorkflowEffectorWithVarFromConfig() 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); + + Object output = ContainerEffectorTest.doTestEchoBashCommand(app, () -> { + ConfigBag parameters = ConfigBag.newInstance(ImmutableMap.of( + WorkflowEffector.EFFECTOR_NAME, "test-container-effector", + WorkflowEffector.STEPS, MutableList.of( + 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}")))); + + return new WorkflowEffector(parameters); + }, entity -> entity.config().set(ConfigKeys.newStringConfigKey("hello"), "world")); + Asserts.assertEquals(output.toString().trim(), message + " world"); + + app = (TestApplication) rebind(); + + TestEntity child = (TestEntity) Iterables.getLast(app.getChildren()); + + output = Entities.invokeEffector(app, child, child.getEffector("test-container-effector")).getUnchecked(Duration.ONE_MINUTE); + Asserts.assertEquals(output.toString().trim(), message + " world"); + } + + @Test + void testWorkflowSensorRebind() throws Exception { + Entity app = createAndStartApplication( + "services:", + "- type: " + BasicEntity.class.getName(), + " brooklyn.initializers:", + " - type: workflow-sensor", + " brooklyn.config:", + " sensor: myWorkflowSensor", + " triggers:", + " - trig", + " steps:", + " - type: return", + " value:", + " n: $brooklyn:attributeWhenReady(\"trig\")", + ""); + + waitForApplicationTasks(app); + Entity child = Iterables.getOnlyElement(app.getChildren()); + + child.sensors().set(Sensors.newIntegerSensor("trig"), 1); + EntityAsserts.assertAttributeEqualsEventually(child, Sensors.newSensor(Object.class, "myWorkflowSensor"), MutableMap.of("n", 1)); + + app = rebind(); + child = Iterables.getOnlyElement(app.getChildren()); + + child.sensors().set(Sensors.newIntegerSensor("trig"), 2); + EntityAsserts.assertAttributeEqualsEventually(child, Sensors.newSensor(Object.class, "myWorkflowSensor"), MutableMap.of("n", 2)); + } + +} 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 e043c93b03..597c22bbf0 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 @@ -761,10 +761,7 @@ public class WorkflowYamlTest extends AbstractYamlTest { "args", MutableMap.of("x", MutableMap.of("y", "$brooklyn:config(\"z\")")))))); eff.apply((EntityLocal)app); - Task<?> invocation = app.invoke(app.getEntityType().getEffectorByName("myWorkflowEffector1").get(), - MutableMap.of( -// "x", MutableMap.of("y", DslUtils.parseBrooklynDsl(mgmt(), "$brooklyn:config(\"z\")")) - )); + Task<?> invocation = app.invoke(app.getEntityType().getEffectorByName("myWorkflowEffector1").get(), MutableMap.of()); Asserts.assertEquals(invocation.getUnchecked(), MutableMap.of("y","Z")); } @@ -789,7 +786,7 @@ public class WorkflowYamlTest extends AbstractYamlTest { .configure(WorkflowEffector.EFFECTOR_NAME, "myWorkflowEffector2") .configure(WorkflowEffector.EFFECTOR_PARAMETER_DEFS, MutableMap.of("script", MutableMap.of(), "env", MutableMap.of("defaultValue", MutableMap.of()))) .configure(WorkflowEffector.STEPS, MutableList.of(MutableMap.of("step", "invoke-effector myWorkflowEffector3", - "args", MutableMap.of("script", "${scriot}", "env", "${env}"))))); + "args", MutableMap.of("script", "${script}", "env", "${env}"))))); eff.apply((EntityLocal)child); eff = new WorkflowEffector(ConfigBag.newInstance() .configure(WorkflowEffector.EFFECTOR_NAME, "myWorkflowEffector1") @@ -797,10 +794,7 @@ public class WorkflowYamlTest extends AbstractYamlTest { "args", MutableMap.of("script", "echo Y is $Y", "env", MutableMap.of("Y", "$brooklyn:config(\"z\")")))))); eff.apply((EntityLocal)child); - Task<?> invocation = child.invoke(child.getEntityType().getEffectorByName("myWorkflowEffector1").get(), - MutableMap.of( -// "x", MutableMap.of("y", DslUtils.parseBrooklynDsl(mgmt(), "$brooklyn:config(\"z\")")) - )); + Task<?> invocation = child.invoke(child.getEntityType().getEffectorByName("myWorkflowEffector1").get(), MutableMap.of()); Asserts.assertEquals(invocation.getUnchecked().toString().trim(), "Y is Z"); } diff --git a/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/BrooklynJacksonSerializationUtils.java b/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/BrooklynJacksonSerializationUtils.java index bce8842ec2..aba5f9ac63 100644 --- a/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/BrooklynJacksonSerializationUtils.java +++ b/core/src/main/java/org/apache/brooklyn/core/resolve/jackson/BrooklynJacksonSerializationUtils.java @@ -236,6 +236,12 @@ public class BrooklynJacksonSerializationUtils { Object brooklynLiteral = rm.get("$brooklyn:literal"); if (brooklynLiteral != null) { return BROOKLYN_PARSE_DSL_FUNCTION.apply(mgmt, brooklynLiteral); + } else { + Object type = rm.get("type"); + if (type instanceof String && ((String)type).startsWith("org.apache.brooklyn.camp.brooklyn.spi.dsl.")) { + // should always have a brooklyn:literal on outermost type, so that even with json pass through (used for steps etc) dsl gets preserved across convertDeeply + log.warn("Failed to deserialize "+result+" as DSL; will treat as map"); + } } } } diff --git a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/json/BrooklynJacksonSerializerTest.java b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/json/BrooklynJacksonSerializerTest.java index 9bcdd785c3..18ebb5c030 100644 --- a/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/json/BrooklynJacksonSerializerTest.java +++ b/rest/rest-resources/src/test/java/org/apache/brooklyn/rest/util/json/BrooklynJacksonSerializerTest.java @@ -30,6 +30,7 @@ import org.apache.brooklyn.api.entity.EntitySpec; import org.apache.brooklyn.api.mgmt.ManagementContext; import org.apache.brooklyn.api.objs.BrooklynObject; import org.apache.brooklyn.camp.brooklyn.BrooklynCampPlatform; +import org.apache.brooklyn.camp.brooklyn.spi.dsl.BrooklynDslDeferredSupplier; import org.apache.brooklyn.camp.brooklyn.spi.dsl.DslUtils; import org.apache.brooklyn.camp.brooklyn.spi.dsl.methods.DslComponent; import org.apache.brooklyn.camp.spi.PlatformRootSummary; @@ -38,8 +39,11 @@ import org.apache.brooklyn.core.entity.Entities; import org.apache.brooklyn.core.mgmt.BrooklynTaskTags; import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext; import org.apache.brooklyn.core.resolve.jackson.*; +import org.apache.brooklyn.core.test.BrooklynAppUnitTestSupport; import org.apache.brooklyn.core.test.entity.LocalManagementContextForTests; import org.apache.brooklyn.core.test.entity.TestEntity; +import org.apache.brooklyn.core.workflow.WorkflowStepDefinition; +import org.apache.brooklyn.core.workflow.steps.ReturnWorkflowStep; import org.apache.brooklyn.entity.stock.BasicApplication; import org.apache.brooklyn.test.Asserts; import org.apache.brooklyn.util.collections.MutableList; @@ -685,4 +689,27 @@ public class BrooklynJacksonSerializerTest { Entities.destroyAll(mgmt); } } + + @Test + public void testDslDeepConversionIntoMap() throws Exception { + LocalManagementContext mgmt = LocalManagementContextForTests.newInstance(); + try { + BrooklynCampPlatform platform = new BrooklynCampPlatform( + PlatformRootSummary.builder().name("Brooklyn CAMP Platform").build(), + mgmt) + .setConfigKeyAtManagmentContext(); + BrooklynAppUnitTestSupport.addRegisteredTypeBean(mgmt, "return", "1", ReturnWorkflowStep.class); + + Object v = DslUtils.parseBrooklynDsl(mgmt, "$brooklyn:config(\"x\")"); + + Object obj = MutableMap.of("type", "return", "value", MutableMap.of("n", v)); + WorkflowStepDefinition result = BeanWithTypeUtils.convert(mgmt, obj, TypeToken.of(WorkflowStepDefinition.class), true, null, true); + Asserts.assertInstanceOf(result, ReturnWorkflowStep.class); + Object n = ((Map) result.getInput().get("value")).get("n"); + Asserts.assertInstanceOf(n, BrooklynDslDeferredSupplier.class); + + } finally { + Entities.destroyAll(mgmt); + } + } }
