new charm - auth-proxy

[Paul Czarkowski]

subordinate service to provides apache reverse proxy, ssl, and auth services to a web service that has none of these things.

[Juan L. Negron (negronjl)]

Hi Paul:

Thanks for the charm.

Reviewing this now.

-Juan

[Juan L. Negron (negronjl)]

Hi Paul:

Here is my feedback on auth-proxy:
- install
--- As a subordinate, you should be careful when replacing files that could also be used by the main charm. I see that you are replacing default and default-ssl which may already have been modified and used by the main charm. At a minimum, you should determine if the file has been modified before replacing it. Arbitrarily replacing such a common ( and maybe essential ) file could be dangerous.

- stop
--- I see a similar problem with stopping apache2 altogether here ... As a subordinate charm, you don't know if apache is already being used by the main charm. Stopping it could stop the main charm as well.

- config-changed
--- I also see some of that here as well ....

All in all, I believe that the changes applied in this charm have to be applied very carefully and should be very targeted. A possible option would be to not assume that default and default-ssl are being used and allow the user to specify the name of the site being used ( you could default to default and default-ssl ). This could give you some indication of the site being used by the main charm.

I am marking the charm as Incomplete while you address the issues mentioned above. I would be happy to review the charm again when you're ready.

Please feel free to reach out to any of us in #juju on freenet ... we are here to help in any way we can.

Thanks,

Juan

[Clint Byrum (clint-fewbar)]

Excerpts from Juan L. Negron's message of 2012-11-29 18:43:47 UTC:
> Hi Paul:
>
> Here is my feedback on auth-proxy:
> - install
> --- As a subordinate, you should be careful when replacing files that could also be used by the main charm. I see that you are replacing default and default-ssl which may already have been modified and used by the main charm. At a minimum, you should determine if the file has been modified before replacing it. Arbitrarily replacing such a common ( and maybe essential ) file could be dangerous.
>

The way to handle this is to not use 'juju-info' as your required
subordinate interface. Instead, require a 'local-apache' interface which
will allow the sub to talk to charms that understand it about things
like where config files are and what conflicting settings are used. This
makes sense so that other aspects of apache can be put into subordinates,
such as rewrite rules for old sites, special logging configurations,
and probably the biggest one, tuning.

juju-info is for things that wouldn't normally interfere with a primary
charm, such as syslogging or monitoring agents.

[Paul Czarkowski (paulcz)]

Thanks for the feedback. Probably a little short sighted of me ... but I never envisioned running this to auth-proxy for an apache server ... I wrote it specifically with Kibana in mind which is a ruby/sinatra app and was thinking of other apps I've done similar things for ( random glassfish and tomcat, and other apps ).

I'll spin some more cycles on this and see if I can come up with a sane way to handle this ... I really wanted to try and keep it transparent to the app that it was going to protect, hence using juju-info for the require.

[Launchpad Janitor (janitor)]

[Expired for Juju Charms Collection because there has been no activity for 60 days.]

[amir sanjar (asanjar)]

initial review:
1) Metadata is missing categories.
2) No icon.svg file.

[amir sanjar (asanjar)]

1) Metadata is missing categories.
2) No icon.svg file.

[Matt Bruzek (mbruzek)]

Hello Paul,

First and foremost thanks for the submission of this charm. It is available on the charm store in your personal namespace. People can download your charm using:

    charm get cs:~paulcz/charms/precise/auth-proxy

To get in the charm store requires following the current charm policy. This charm is missing some information that required by our tooling and process. Please add "categories" key in the metadata.yaml file. The valid option for this charm would be "misc". So simply adding categories: ["misc"] would resovle the metadata problem.

To address the icon warning you can either make an icon using inkscape following these instruction: https://juju.ubuntu.com/docs/authors-charm-icon.html or if there is no icon/image for this service you can grab the "juju-placeholder-a.svg" icon from our placeholder icons here: https://github.com/juju-solutions/generic-charm-icons (the convention we are using is the first letter of the charm name as the generic icon). That said I would highly recommend making a unique icon.

I also noticed that your README instructs the user to set an empty string value. This worked on older versions of juju but setting empty string values is not working on the latest juju. I opened a bug against this in juju-core that is currently working on this issue. This bug is currently affecting only one person. So if you want to add yourself to the bug it might get released faster.

https://bugs.launchpad.net/juju-core/1.20/+bug/1348829

Thanks again for your time writing this charm! Set the bug back to "new" or "fix committed" once you have fixed these issues and we will give it another review.

[Matt Bruzek (mbruzek)]

Paul,

The charm URI that I put in the last comment is wrong. It should have been:
charm get cs:~paulcz/precise/auth-proxy

Users will have to have charm-tools installed. https://juju.ubuntu.com/docs/tools-charm-tools.html for this to work.

Alternately someone could create the charm directory with a bzr command:

bzr branch lp:~paulcz/charms/precise/auth-proxy/trunk auth-proxy

Thanks.

[Launchpad Janitor (janitor)]

[Expired for Juju Charms Collection because there has been no activity for 60 days.]