Juju Charms Collection

Bug #1551133
Comment #3

Comment 3 for bug 1551133

Revision history for this message

Cory Johns (johnsca) wrote on 2016-03-10:

Matan,

Thank you for your submission to the charm ecosystem! It looks like a great start, but I do have some issues that need to be addressed.

When downloading the RLEC payload from S3, you're not doing any checksum verification to ensure that the file you get is the file you expect. Because we consider this very important for security and data integrity reasons, we have some helpers available in the charmhelpers.fetch package to make this easy. You can use either the high level install_remote [1] function, or the lower-level ArchiveUrlFetchHandler.download_and_validate [2] method (I would recommend the former).

[1]: https://pythonhosted.org/charmhelpers/api/charmhelpers.fetch.html#charmhelpers.fetch.install_remote
[2]: https://pythonhosted.org/charmhelpers/api/charmhelpers.fetch.html#charmhelpers.fetch.archiveurl.ArchiveUrlFetchHandler.download_and_validate

You only handle the "is_leader" case in the install hook, but it is possible that the leader can change. For instance, if the original leader dies or is removed using remove-unit, a new leader will need to be elected to take its place. It would be better if you moved that bit of code from the install hook to the leader-elected hook. Note that that will only be called on the newly elected leader, and is guaranteed to be called at least once.

Your node-relation-departed hook posts to a web service to remove the departing node, but I don't see anywhere that you post to register the node. Is that something that happens implicitly? I do notice that your node-relation-changed hook has the leader put all of the peers in to the leadership settings, but then that setting is never used. I wonder if that was intended to be the registration step?

Your config-change hook seems to be left-over boilerplate from the `charm create` template. It doesn't seem to do anything to update the service configuration based on the new config values, meaning the charm will not respond to the user changing those values after deployment (something we call "immutable config"). It seems that it should update the config file and possibly restart the service. Also, because of the boilerplate, it is currently generating a "dummy" Upstart job, which is not something you want, so I'd also recommend removing the templates directory and the hooks/services.py file.

Finally, your charm doesn't have any relations to enable it to connect to other services. I would recommend that you add support to your charm for the "redis" interface so that it can provide this service to all of the other existing charms that require Redis [3]. It looks like all you would need to provide on that relation is the master_node_ip, port, and password.

[3]: https://jujucharms.com/requires/redis

Again, thank you for this contribution. I look forward to seeing your updates to address these items!

Matan,

Thank you for your submission to the charm ecosystem!  It looks like a great start, but I do have some issues that need to be addressed.

When downloading the RLEC payload from S3, you're not doing any checksum verification to ensure that the file you get is the file you expect.  Because we consider this very important for security and data integrity reasons, we have some helpers available in the charmhelpers.fetch package to make this easy.  You can use either the high level install_remote [1] function, or the lower-level ArchiveUrlFetchHandler.download_and_validate [2] method (I would recommend the former).

You only handle the "is_leader" case in the install hook, but it is possible that the leader can change.  For instance, if the original leader dies or is removed using remove-unit,  a new leader will need to be elected to take its place.  It would be better if you moved that bit of code from the install hook to the leader-elected hook.  Note that that will only be called on the newly elected leader, and is guaranteed to be called at least once.

Your node-relation-departed hook posts to a web service to remove the departing node, but I don't see anywhere that you post to register the node.  Is that something that happens implicitly?  I do notice that your node-relation-changed  hook has the leader put all of the peers in to the leadership settings, but then that setting is never used.  I wonder if that was intended to be the registration step?

Your config-change hook seems to be left-over boilerplate from the `charm create` template.  It doesn't seem to do anything to update the service configuration based on the new config values, meaning the charm will not respond to the user changing those values after deployment (something we call "immutable config").  It seems that it should update the config file and possibly restart the service.  Also, because of the boilerplate, it is currently generating a "dummy" Upstart job, which is not something you want, so I'd also recommend removing the templates directory and the hooks/services.py file.

Finally, your charm doesn't have any relations to enable it to connect to other services.  I would recommend that you add support to your charm for the "redis" interface so that it can provide this  service to all of the other existing charms that require Redis [3].  It looks like all you would need to provide on that relation is the master_node_ip, port, and password.

[3]: https://jujucharms.com/requires/redis

Again, thank you for this contribution.  I look forward to seeing your updates to address these items!