Shell escaping problems for Java system properties

[Richard Downer <ri...@apache.org>]

All,

When you copy-and-paste the YAML file near the top of
https://brooklyn.incubator.apache.org/quickstart/, the application
deploys as you expect it to, using the default of JBoss 7 for the
ControlledDynamicWebAppCluster.

If you modify the blueprint to deploy Tomcat instead of JBoss 7, it
seems to work, but when you try and run the hello-world web app it
fails to connect to the database.

The reason is this line here:

    java.sysprops:
      brooklyn.example.db.url: >
        $brooklyn:formatString("jdbc:%s%s?user=%s\\&password=%s",
        component("db").attributeWhenReady("datastore.url"),
        "visitors", "brooklyn", "br00k11n")

Note that the format string contains "\\&". The JDBC URL needs an "&"
character in it, but it seems that this string is being passed through
to the shell unescaped, so the author of this blueprint made it "\\&"
so that it would pass through the shell.

The problem is that the Tomcat entity *does* do shell escaping,
meaning that to work on Tomcat, the blueprint must be modified like
this:

    java.sysprops:
      brooklyn.example.db.url: >
        $brooklyn:formatString("jdbc:%s%s?user=%s&password=%s",
        component("db").attributeWhenReady("datastore.url"),
        "visitors", "brooklyn", "br00k11n")

There's two problems with this:

1 - the JBoss and Tomcat entities should handle this property in the
same way. They should be offering the same external interface for
common functionality, so that they are interchangable.

2 - the first version requires that the user know about shell
escaping. This is a leaky abstraction: the user should *not* have to
know or think about how these strings are processed internally.

Therefore, I believe that the correct form of the blueprint is the second one.

I've looked at the code to see how to fix this. The JBoss behaviour is
not JBoss-specific; rather it is the behaviour of
JavaSoftwareProcessSshDriver. This overrides the getShellEnvironment()
to set the JAVA_OPTS environment variable, but it does *not* escape
the contents of JAVA_OPTS (it is explicitly documented that it will
not do this). So this is where our abstraction leaks - the user is
required to do shell-escaping on all system properties.

The Tomcat7SshDriver does something different: for Tomcat, the
properties are not passed in JAVA_OPTS but in CATALINA_OPTS.
Therefore, Tomcat7SshDriver overrides getShellEnvironment() and
removes JAVA_OPTS from the map; it then adds CATALINA_OPTS but it
*does* make a shell-safe escape method call. Therefore it is
Tomcat7SshDriver that breaks the pattern to operate in a more sensible
way.

So, certainly the two methods should operate the same way, and really
we want to avoid a leaky abstraction that requires the user to do
shell-escaping.

This would require changing JavaSoftwareProcess base class to properly
escape Java options. However this could potentially impact all the
subclasses of JavaSoftwareProcess - 21 of them at current count. Each
would need to be checked for potential unwanted behaviour following
this change.

Any comments? Should I proceed with this course of action?

Richard.