You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by bostko <gi...@git.apache.org> on 2016/06/10 14:26:50 UTC
[GitHub] brooklyn-server pull request #195: Sanitize effector parameters when reporti...
GitHub user bostko opened a pull request:
https://github.com/apache/brooklyn-server/pull/195
Sanitize effector parameters when reporting an exception
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bostko/brooklyn-server effector_utils_sanitize_logs
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/195.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #195
----
commit 3c0c8af2d591b6026945dd112f55450dbc752947
Author: Valentin Aitken <bo...@gmail.com>
Date: 2016-06-10T14:25:46Z
Sanitize effector parameters when reporting an exception
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #195: Sanitize effector parameters when reporting an e...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/195
@bostko I've created https://github.com/bostko/brooklyn-server/pull/1 - please review. If you merge it into your branch, then it should automatically appear in this PR.
For the apache record, that PR does:
Also refactors EffectorUtils.prepareArgsForEffector (deprecating old
method), and updates how MethodEffector calls it so we have the
parameter names.
The previous calls to `sanitizeArgs(args)` where args was not of type `Map` would do nothing.
Therefore if this was ever called with an array etc, we'd log all of the parameter values.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #195: Sanitize effector parameters when reporting an e...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/195
Whoever merges this, please look at marking https://issues.apache.org/jira/browse/BROOKLYN-282 as resolved.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #195: Sanitize effector parameters when reporti...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/195#discussion_r67142271
--- Diff: core/src/test/java/org/apache/brooklyn/core/mgmt/internal/TestEntityWithEffectors.java ---
@@ -21,15 +21,77 @@
import org.apache.brooklyn.api.entity.ImplementedBy;
import org.apache.brooklyn.core.annotation.Effector;
import org.apache.brooklyn.core.annotation.EffectorParam;
+import org.apache.brooklyn.core.effector.MethodEffector;
import org.apache.brooklyn.core.test.entity.TestEntity;
+/**
+ * Entity for testing that secret effector parameters are:
+ * <ul>
+ * <li>excluded from the activities view
+ * <li>not logged
+ * <li>masked out in the UI
+ * </ul>
+ * Of those, only the first is unit-tested.
+ *
+ * To test manually...
+ *
+ * Configure logback to log everything at trace:
+ * <pre>
+ * {@code
+ * <configuration>
+ * <include resource="logback-main.xml"/>
+ * <logger name="org.apache.brooklyn" level="TRACE"/>
+ * <logger name="brooklyn" level="TRACE"/>
+ * </configuration>
+ * }
+ * </pre>
+ *
+ * Run Brooklyn with the above log configuration file:
+ * <pre>
+ * {@code
+ * export JAVA_OPTS="-Xms256m -Xmx1g -XX:MaxPermSize=256m -Dlogback.configurationFile=/path/to/logback-trace.xml"
+ * ./bin/brooklyn launch --persist auto --persistenceDir /path/to/persistedState
+ * }
+ * </pre>
+ *
+ * Deploy the blueprint below:
+ * <pre>
+ * {@code
+ * services:
+ * - type: org.apache.brooklyn.core.mgmt.internal.TestEntityWithEffectors
--- End diff --
The way I built it was to export TestEntityWithEffectors*.java as a jar from my IDE, and add it to ./lib/patch of a pre-installed Brooklyn. That didn't require adding the pom.
Which pom did you add that to? Maybe we should add something more general like "You'll have to ensure this is on the classpath of your brooklyn server, before testing". I'll update my PR accordingly.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #195: Sanitize effector parameters when reporting an e...
Posted by ygy <gi...@git.apache.org>.
Github user ygy commented on the issue:
https://github.com/apache/brooklyn-server/pull/195
LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #195: Sanitize effector parameters when reporting an e...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/195
Unfortunately jenkins is not running PR builds. I ran `mvn clean install` manually, and it worked. So merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #195: Sanitize effector parameters when reporti...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/195
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #195: Sanitize effector parameters when reporti...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/195#discussion_r67141981
--- Diff: core/src/test/java/org/apache/brooklyn/core/mgmt/internal/TestEntityWithEffectors.java ---
@@ -21,15 +21,77 @@
import org.apache.brooklyn.api.entity.ImplementedBy;
import org.apache.brooklyn.core.annotation.Effector;
import org.apache.brooklyn.core.annotation.EffectorParam;
+import org.apache.brooklyn.core.effector.MethodEffector;
import org.apache.brooklyn.core.test.entity.TestEntity;
+/**
+ * Entity for testing that secret effector parameters are:
+ * <ul>
+ * <li>excluded from the activities view
+ * <li>not logged
+ * <li>masked out in the UI
+ * </ul>
+ * Of those, only the first is unit-tested.
+ *
+ * To test manually...
+ *
+ * Configure logback to log everything at trace:
+ * <pre>
+ * {@code
+ * <configuration>
+ * <include resource="logback-main.xml"/>
+ * <logger name="org.apache.brooklyn" level="TRACE"/>
+ * <logger name="brooklyn" level="TRACE"/>
+ * </configuration>
+ * }
+ * </pre>
+ *
+ * Run Brooklyn with the above log configuration file:
+ * <pre>
+ * {@code
+ * export JAVA_OPTS="-Xms256m -Xmx1g -XX:MaxPermSize=256m -Dlogback.configurationFile=/path/to/logback-trace.xml"
+ * ./bin/brooklyn launch --persist auto --persistenceDir /path/to/persistedState
+ * }
+ * </pre>
+ *
+ * Deploy the blueprint below:
+ * <pre>
+ * {@code
+ * services:
+ * - type: org.apache.brooklyn.core.mgmt.internal.TestEntityWithEffectors
--- End diff --
I had to add
```xml
<dependency>
<groupId>org.apache.brooklyn</groupId>
<artifactId>brooklyn-core</artifactId>
<version>${project.version}</version>
<classifier>tests</classifier>
</dependency>
```
In my maven file to use this entity.
@aledsage do you think we should this note here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #195: Sanitize effector parameters when reporting an e...
Posted by bostko <gi...@git.apache.org>.
Github user bostko commented on the issue:
https://github.com/apache/brooklyn-server/pull/195
@aledsage thank you for the changes!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---