[Kimchi-devel] [PATCH v3 1/3] Github #329: kimchid script changes

[Aline Manera]

On 04/10/2014 05:44 PM, Daniel Barboza wrote:
> From: Daniel Henrique Barboza <danielhb at linux.vnet.ibm.com>
>
> The kimchid script is now launching a reverse proxy (nginx)
> that will forward all requests made to the kimchi port to
> the kimchid process running as root in a different port,
> which can be made inacessible to the outside using
> firewall rules.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb at linux.vnet.ibm.com>
> ---
>   src/kimchid.in | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 102 insertions(+), 9 deletions(-)
>
> diff --git a/src/kimchid.in b/src/kimchid.in
> index 8b63b57..fd755eb 100644
> --- a/src/kimchid.in
> +++ b/src/kimchid.in
> @@ -16,13 +16,18 @@
>   #
>   # You should have received a copy of the GNU Lesser General Public
>   # License along with this library; if not, write to the Free Software
> -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> +# MA  02110-1301  USA

You don't need to break line here. Just remove the extra space 
"02110-1301 USA" to achieve the 80 characters.

>
>   import logging
>   import os
> +import signal
> +import subprocess
>   import sys
>   sys.path.insert(1, '@pythondir@')
>
> +from string import Template
> +
>   import kimchi.server
>   import kimchi.config
>
> @@ -35,7 +40,63 @@ if not paths.installed:
>   ACCESS_LOG = "kimchi-access.log"
>   ERROR_LOG = "kimchi-error.log"
>
> +
> +def create_proxy_config(p_port, k_port,
> +                        p_ssl_port, k_ssl_port):
> +    """Create nginx configuration file based on current ports config
> +
> +    To allow flexibility in which port kimchi runs, we need the same
> +    flexibility with the nginx proxy. This method creates the config
> +    file dynamically by using 'nginx.conf.in' as a template, creating
> +    the file 'nginx_kimchi.config' which will be used to launch the
> +    proxy.
> +
> +    Arguments:
> +    p_port - proxy port
> +    kimchid_port - kimchid port
> +    p_ssl_port - proxy SSL port
> +    kimchid_ssl_port - kimchid SSL port
> +    """
> +
> +    # get SSL paths to be used by the proxy
> +    config_dir = paths.conf_dir
> +    cert = '%s/kimchi-cert.pem' % config_dir
> +    key = '%s/kimchi-key.pem' % config_dir
> +


> +    with open(os.path.join(config_dir, "nginx.conf.in")) as template:
> +        data = template.read()

This patch uses "nginx.conf.in" but it is not in this patch.
You should merge this patch with patch 2 to have a consistent commit.

> +
> +    data = Template(data)
> +    data = data.safe_substitute(proxy_port=p_port,
> +                                kimchid_port=k_port,
> +                                proxy_ssl_port=p_ssl_port,
> +                                kimchid_ssl_port=k_ssl_port,
> +                                cert_pem=cert, cert_key=key)
> +
> +    config_file = open(os.path.join(config_dir, "nginx_kimchi.conf"), "w")
> +    config_file.write(data)
> +    config_file.close()
> +
> +
> +def start_proxy():
> +    """Start nginx reverse proxy."""
> +    config_dir = paths.conf_dir
> +    config_file = "%s/nginx_kimchi.conf" % config_dir
> +    cmd = ['nginx', '-c', config_file]
> +    subprocess.call(cmd)
> +
> +
> +def terminate_proxy():
> +    """Stop nginx process."""
> +    term_proxy_cmd = ['nginx', '-s', 'stop']
> +    subprocess.call(term_proxy_cmd)
> +
> +
>   def main(options):
> +    # Script must run as root or with sudo.
> +    if not os.geteuid() == 0:
> +        sys.exit("\nMust be root to run this script. Exiting ...\n")
> +
>       host = config.get("server", "host")
>       port = config.get("server", "port")
>       ssl_port = config.get("server", "ssl_port")
> @@ -44,20 +105,52 @@ def main(options):
>       logLevel = config.get("logging", "log_level")
>
>       parser = OptionParser()
> -    parser.add_option('--host', type="string", default=host, help="Hostname to listen on")
> -    parser.add_option('--port', type="int", default=port, help="Port to listen on")
> -    parser.add_option('--ssl-port', type="int", default=ssl_port, help="Enable a SSL server on the given port")
> -    parser.add_option('--log-level', default=logLevel, help="Logging level")
> -    parser.add_option('--access-log', default=os.path.join(logDir,ACCESS_LOG), help="Access log file")
> -    parser.add_option('--error-log', default=os.path.join(logDir,ERROR_LOG), help="Error log file")
> -    parser.add_option('--environment', default=runningEnv, help="Running environment of kimchi server")
> -    parser.add_option('--test', action='store_true', help="Run server in mock model")
> +    parser.add_option('--host', type="string", default=host,
> +                      help="Hostname to listen on")
> +    parser.add_option('--port', type="int", default=port,
> +                      help="Port to listen on")
> +    parser.add_option('--ssl-port', type="int", default=ssl_port,
> +                      help="Enable a SSL server on the given port")
> +    parser.add_option('--log-level', default=logLevel,
> +                      help="Logging level")
> +    parser.add_option('--access-log',
> +                      default=os.path.join(logDir, ACCESS_LOG),
> +                      help="Access log file")
> +    parser.add_option('--error-log',
> +                      default=os.path.join(logDir, ERROR_LOG),
> +                      help="Error log file")
> +    parser.add_option('--environment', default=runningEnv,
> +                      help="Running environment of kimchi server")
> +    parser.add_option('--test', action='store_true',
> +                      help="Run server in mock model")
>       (options, args) = parser.parse_args()
>
>       # Add non-option arguments
>       setattr(options, 'ssl_cert', config.get('server', 'ssl_cert'))
>       setattr(options, 'ssl_key', config.get('server', 'ssl_key'))


> +    # The following method is used when the process receives a
> +    # SIGINT signal (CTRL+C).
> +    def terminate_kimchi(signal, frame):
> +        terminate_proxy()
> +        sys.exit(0)
> +    signal.signal(signal.SIGINT, terminate_kimchi)

I think the better way to do it is subscribe to the cherrypy exit 
function in server.py

cherrypy.engine.subscribe('exit', <function to kill proxy>)

> +
> +    proxy_port = options.port
> +    proxy_ssl_port = options.ssl_port
> +



> +    # The max value between port and proxy_port
> +    # will be used to calculate kimchid ports.
> +    next_port = max(proxy_port, proxy_ssl_port) + 1
> +    options.port = next_port
> +    next_port += 1
> +    options.ssl_port = next_port
> +

Automatically discover the ports can be an easy point of errors.
The ports can be in used by other application.
I think we should add more options to set the proxy ports and set a 
default value for
it in the kimchi.conf file

So I expect:

kimchid --proxy-port=XXXX --proxy-ssl-port=YYYY

> +    # Launch reverse proxy: create config file and start.
> +    create_proxy_config(proxy_port, options.port,
> +                        proxy_ssl_port, options.ssl_port)
> +    start_proxy()
> +
>       kimchi.server.main(options)
>
>   if __name__ == '__main__':