Charm Needed: wildfly

Greetings Saurabh,

Thank you very much for the submission of the wildfly charm! I am excited to have another Java charm in the charm store!

This is a +1 review from me, and someone else from ~charmers should be along to complete the review and promulgate this charm if there are no other issues. We appreciate your submission and look forward to the next round of review!

Charm Proof:

I saw no errors, warnings or information when running “juju charm proof”. That means your charm is high quality! Thank you for taking the time to address all the issues in charm proof before submitting.

README:

The README is in text file which is fine, but many charms use markdown format (README.md) because it allows more formatting options. Please consider releasing a markdown formatted version as it will look nicer in the charm store. If you have charm tools installed we have a command that saves you time by generating a README in markdown format. From the wildfly/ directory run the command: juju charm add readme and a README.ex template file is created with the charm store format.

The “Usage” section should include: juju expose wildfly command. The web console will not be accessible from juju otherwise.

Review comments:

I deployed this charm on local provider and hpcloud (OpenStack environment). I was not able to access the web console (after making wildfly exposed) on hpcloud. I saw no errors in “juju status” so I looked into this a little further. I was able to open the web console on the locally deployed wildfly but the hpcloud instance was still not visible using the http://<ip-address>:9990. I believe this is because you do not open the port through juju (open-port http-port) in your hooks. The local provider does not have a firewall but the other providers do.

According to the Charm Store Policy (https://juju.ubuntu.com/docs/authors-charm-policy.html) we should not use default passwords for charms. I would suggest making the administrator's password configurable by an option in config.yaml. Then users could set their own passwords with: juju set wildfly admin_password=”password” after deployment.

The start hook could be run multiple times, so it should be idempotent. I see the start hook is creating the admin user, and not sure what would happen if the admin user logs in to the web console and changes their password. If the start hook were to run again (for whatever reason). Would the admin user's password be reset in this case? One way to help with idempotency is to check if something (user) exists before creating it.

The install hook uses wget to download the wildfly tar file. Since you are not using apt-get the Charm Store Policy strongly suggests that you should verify the file contents cryptographically before installing it. I would recommend using sha1sum compared to a known value to prevent tampering or an incomplete download.

I moved this bug to incomplete as a result of the review. Once you have addressed the issues with code changes or comments and wish to have another review move the bug back to “New” or “Fix committed” to have it added back to the queue.

Again thanks for your work on this charm!

Hi Matt,

Thank you for taking out your valuable time to review my charm. As per you suggestions I have revised my charm to fix those issues.

Issues fixed:
   - Included the juju expose charm in readme.
   - Used `open-port` to open the 9990 port.
   - made the admin user and password configurable via config.yaml
   - start hook is idempotent now.

Existing issue:
   - verify the file contents cryptographically before installing;
      The reason behind this issue is that there is no cryptographic key provided with the wildfly download. I have raised this issue with wildfly people. Hope they fix it soon.

Saurabh.

Thank you so much for the update on the wildfly charm. It is progressing nicely but there are still a few outstanding issues (that I have outlined below) which need to be resolved prior to accepting this charm in the charm store.

Cryptographic signature verification:

The Charm Store Policy (https://juju.ubuntu.com/docs/authors-charm-policy.html) states that charm code should verify the cryptographic signature of the files that are downloaded (in this case wildfly-8.0.0.CR1.tar.gz). You noted that the original web site does not publish the signatures for the files so I would suggest generating a sha1sum from a file that you have verified has worked.

Here is an example of how to get the sha1sum:
$ sha1sum wildfly-8.0.0.CR1.tar.gz
5f7289a4e8f05e3772808675342aaa8735bad704 wildfly-8.0.0.CR1.tar.gz

Store a valid value for that version in a variable and compare the sha1sum of the file the charm downloads to verify the file is complete and has not changed since the charm was created.

README.md

I saw the README was converted from text to markdown format. My tool (markdown available from apt-get install markdown) was not able to convert README.md to HTML correctly. It looks like there are too many spaces and too few spaces in other places. There are markdown validation tools available on-line (http://daringfireball.net/projects/markdown/dingus) that will help get the format correct.

Errors in the hooks:

I was watching the logs while the wildfly charm installed, and noticed a few errors involving the jboss.pid file:

2014-02-07 20:53:14 INFO juju-log Stopping wildfly apllication server
2014-02-07 20:53:14 INFO config-changed Notfound
2014-02-07 20:53:14 INFO config-changed cat: /opt/wildfly/bin/jboss.pid: No such file or directory
2014-02-07 20:53:14 INFO config-changed kill: usage: kill [-s sigspec | -n signum | -sigspec] pid | jobspec ... or kill -l [sigspec]
2014-02-07 20:53:14 INFO juju-log wildfly stopped successfully

This looks like a problem with the wildfly.sh file that was created.

Open ports:

The charm must open all ports that are needed for operation.

I deployed the wildfly charm to the HP cloud which is an OpenStack environment. I am now able to get to the wildfly administration console on http://ip:9990/. However that is the only port open. I installed a sample war file and was not able to view the webapp context on port 8080. It looks like port 8080 needs to be opened for the webapps to be visible.

You might not have seen this problem because the local provider does not have any firewall on the instances, but providers such as AWS, and HP cloud will have firewalls that Juju handles for you with the open-port, and close-port commands.

It does not look like the code relates the configuration value http-port to any wildfly configuration. For example when I called: juju set wildfly http-port=9991 I was not able to access the admin console on port 9991. Normally the config-changed hook would set some value in wildfly configuration when the port changes. If the ports are not configurable then remove http-port from config.yaml and open the default ports in the install hook.

Thanks again for submi...

I've submitted some fixes for the README: https://code.launchpad.net/~jorge/charms/precise/wildfly/fix-readme/+merge/205797

Dear Matt,

I have submitted a revision of the wildfly charm.

Below are the fixes done:

    - Cryptographic signature verification- using md5 verifivation for the packages not downloaded from apt.
    - Error in the startup script
    - All necessary ports opened
    - Removed the unconfigurable port from config.yaml.

New features:

    - Added mysql relation feature using mysql connector.
    - New datasource gets created when mysql relation is joined
    - The datasaource is removed once the relation is destroyed.

I have tried my best to take care of idempotency.

Kindly have a look at this revision and let me know if some thing is still missing or not upto the mark.

Thanks.

Saurabh,

Thanks for your continued work on this wildfly charm! You addressed all the concerns I noted. The wildfly charm deploys cleanly in Juju on the HP cloud and I was able to load a WAR file to test the functionality of wildfly. I tested access the webapp context and verified that JSP and Servlets were working!

The hooks log appropriately which is very nice for debugging. They verify all downloads, and use templates for configuration files. The README converts to markdown format, and looks very nice.

This latest version of the wildfly looks great, and passes my initial cursory review. According to our process someone else will have to review it before the charm can be accepted to the Charm Store. A Charmer will be by shortly for a final review prior to promulgation.

Great job on this charm. Thanks so much for working on the revisions to make this a great charm!

- Matt

Hi Saurabh,

Thanks for you patience in the review process. Unfortunately, it's against store policy to pre-set sensative details, such as passwords, in the config.yaml. These should be empty and require a user to set the password for their deployment. Leaving a default password for deployments opens a potential attack vector on that deployment.

There are various ways to handle this, most charms leave the option blank and won't configure the service until a password is set. Others randomly generate a password if the password option is blank - then change it when the user supplies one. However you choose to implement this make sure you document the process and behavior in your README file so users know what to expect and how to change this behavior.

Finally, the http interface needs both a hostname and a port. It seems the service runs on port 8080, so you'll also need to relation-set port=8080 in the website-relation-joined hook

relation-set hostname=`unit-get private-address` port=8080

Other than that everything else seems to work within the charm. We appreciate your work in getting this charm ready for the store! Once the above items are reviewed and updated move this bug back into either a New or Fix Committed state and a charmer will be by to promulgate the charm in to the charm store!

Dear Marco,

Thanks for your review.
I have taken care of the issue that you pointed out. I have now left the default password blank and modified the README,md file on how to add a password to the wildfly server.

Also I have set the port 9990 in website-relation-joined hook. This port is used to open the management web console of wildfly AS.
I have pushed the changes please have a look at this revision.

Regards,
Saurabh

Saurabh,

Thank you for the quick turn around on the fix and your patience with the review process. I've reviewed the changes and they look good to me. I'm acknowledging this as a +1 review and will ping a charmer to re-review the charm for promulgation.

All the best!