[PATCH 0/2 V2] bug fix: Properly set max body size to nginx proxy
V1 -> V2: - Close file descriptors in test cases - Set max body size unit on nginx config file. For it add {} around Kimchi variables Aline Manera (2): Identify Kimchi variables from nginx config variables in nginx.conf.in file bug fix: Properly set max body size to nginx proxy src/kimchi/proxy.py | 25 +++++++------------ src/nginx.conf.in | 18 ++++++++------ tests/test_rest.py | 71 ++++++++++++++++++++++++++++++++++++++++------------- 3 files changed, 73 insertions(+), 41 deletions(-) -- 1.9.3
Use {} around Kimchi variables in nginx.conf.in file so we can easily identify which ones are placeholders and which ones is from nginx config. It also alllows us to append string after a Kimchi variable (ie, ${max_body_size}k) Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/nginx.conf.in | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/nginx.conf.in b/src/nginx.conf.in index 1d1a398..9d11770 100644 --- a/src/nginx.conf.in +++ b/src/nginx.conf.in @@ -20,7 +20,7 @@ # This is a template file to be used to generate a nginx # proxy config file at kimchid script. -user $user; +user ${user}; worker_processes 1; error_log /var/log/nginx/error.log; @@ -46,10 +46,10 @@ http { send_timeout 600; server { - listen $proxy_ssl_port ssl; + listen ${proxy_ssl_port} ssl; - ssl_certificate $cert_pem; - ssl_certificate_key $cert_key; + ssl_certificate ${cert_pem}; + ssl_certificate_key ${cert_key}; add_header Strict-Transport-Security "max-age=31536000; includeSubdomains;"; add_header X-Frame-Options DENY; @@ -57,16 +57,16 @@ http { add_header X-XSS-Protection "1; mode=block"; location / { - proxy_pass http://127.0.0.1:$kimchid_port; + proxy_pass http://127.0.0.1:${kimchid_port}; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; - proxy_redirect http://127.0.0.1:$kimchid_port/ https://$host:$proxy_ssl_port/; + proxy_redirect http://127.0.0.1:${kimchid_port}/ https://$host:${proxy_ssl_port}/; } } server { - listen $proxy_port; - rewrite ^/(.*)$ https://$host:$proxy_ssl_port/$1 redirect; + listen ${proxy_port}; + rewrite ^/(.*)$ https://$host:${proxy_ssl_port}/$1 redirect; } } -- 1.9.3
To support uploading file to a storage pool, the max body size must be larger enough to allow uploading a ISO or image file. It was properly set on cherrypy server but as we use nginx as a reverse proxy it also needs to have the same config. Otherwise, nginx will block request with body size larger than 1M (the default value). Also add test cases to cover those scenarios. Signed-off-by: Aline Manera <alinefm@linux.vnet.ibm.com> --- src/kimchi/proxy.py | 25 +++++++------------ src/nginx.conf.in | 2 ++ tests/test_rest.py | 71 ++++++++++++++++++++++++++++++++++++++++------------- 3 files changed, 65 insertions(+), 33 deletions(-) diff --git a/src/kimchi/proxy.py b/src/kimchi/proxy.py index d924ac8..9238147 100644 --- a/src/kimchi/proxy.py +++ b/src/kimchi/proxy.py @@ -31,7 +31,7 @@ from kimchi import sslcert from kimchi.config import paths -def _create_proxy_config(p_port, k_port, p_ssl_port, cert, key): +def _create_proxy_config(options): """Create nginx configuration file based on current ports config To allow flexibility in which port kimchi runs, we need the same @@ -41,11 +41,7 @@ def _create_proxy_config(p_port, k_port, p_ssl_port, cert, key): proxy. Arguments: - p_port - proxy port - k_port - kimchid port - p_ssl_port - proxy SSL port - cert - cert file specified by user config - key - key file specified by user config + options - OptionParser object with Kimchi config options """ # User that will run the worker process of the proxy. Fedora, @@ -59,7 +55,7 @@ def _create_proxy_config(p_port, k_port, p_ssl_port, cert, key): config_dir = paths.conf_dir # No certificates specified by the user - if not cert or not key: + if not options.ssl_cert or not options.ssl_key: cert = '%s/kimchi-cert.pem' % config_dir key = '%s/kimchi-key.pem' % config_dir # create cert files if they don't exist @@ -76,10 +72,11 @@ def _create_proxy_config(p_port, k_port, p_ssl_port, cert, key): data = template.read() data = Template(data) data = data.safe_substitute(user=user_proxy, - proxy_port=p_port, - kimchid_port=k_port, - proxy_ssl_port=p_ssl_port, - cert_pem=cert, cert_key=key) + proxy_port=options.port, + kimchid_port=options.cherrypy_port, + proxy_ssl_port=options.ssl_port, + cert_pem=cert, cert_key=key, + max_body_size=eval(options.max_body_size)) # Write file to be used for nginx. config_file = open(os.path.join(config_dir, "nginx_kimchi.conf"), "w") @@ -89,11 +86,7 @@ def _create_proxy_config(p_port, k_port, p_ssl_port, cert, key): def start_proxy(options): """Start nginx reverse proxy.""" - _create_proxy_config(options.port, - options.cherrypy_port, - options.ssl_port, - options.ssl_cert, - options.ssl_key) + _create_proxy_config(options) config_dir = paths.conf_dir config_file = "%s/nginx_kimchi.conf" % config_dir cmd = ['nginx', '-c', config_file] diff --git a/src/nginx.conf.in b/src/nginx.conf.in index 9d11770..b5d207f 100644 --- a/src/nginx.conf.in +++ b/src/nginx.conf.in @@ -38,6 +38,8 @@ http { access_log /var/log/nginx/access.log main; sendfile on; + client_max_body_size ${max_body_size}k; + # Timeout set to 10 minutes to avoid the 504 Gateway Timeout # when Kimchi is processing a request. proxy_connect_timeout 600; diff --git a/tests/test_rest.py b/tests/test_rest.py index ae1c971..b63e990 100644 --- a/tests/test_rest.py +++ b/tests/test_rest.py @@ -35,6 +35,7 @@ from functools import partial import iso_gen import kimchi.mockmodel import kimchi.server +from kimchi.config import paths from kimchi.rollbackcontext import RollbackContext from utils import get_free_port, patch_auth, request from utils import run_server @@ -1907,26 +1908,62 @@ class RestTests(unittest.TestCase): return headers with RollbackContext() as rollback: - upload_path = '/tmp/vol' - f = open(upload_path, 'w') - f.write('abcd') - f.close() - rollback.prependDefer(os.remove, upload_path) - - vol = open(upload_path, 'rb') - url = "https://%s:%s/storagepools/default/storagevolumes" %\ + vol_name = 'COPYING' + vol_path = os.path.join(paths.get_prefix(), vol_name) + url = "https://%s:%s/storagepools/default/storagevolumes" % \ (host, ssl_port) - r = requests.post(url, - files={'file': vol}, - data={'name': 'new_vol2'}, - verify=False, - headers=fake_auth_header()) + with open(vol_path, 'rb') as fd: + r = requests.post(url, + files={'file': fd}, + data={'name': vol_name}, + verify=False, + headers=fake_auth_header()) + self.assertEquals(r.status_code, 202) - time.sleep(1) - vol_list = json.loads( - self.request('/storagepools/default/storagevolumes').read()) - self.assertEquals('new_vol2', vol_list[0]['name']) + self._wait_task(r.json()['id']) + resp = self.request('/storagepools/default/storagevolumes/%s' % + vol_name) + self.assertEquals(200, resp.status) + + # Create a file with 3M to upload + vol_name = '3m-file' + vol_path = os.path.join('/tmp', vol_name) + with open(vol_path, 'wb') as fd: + fd.seek(3*1024*1024-1) + fd.write("\0") + rollback.prependDefer(os.remove, vol_path) + + with open(vol_path, 'rb') as fd: + r = requests.post(url, + files={'file': fd}, + data={'name': vol_name}, + verify=False, + headers=fake_auth_header()) + + self.assertEquals(r.status_code, 202) + self._wait_task(r.json()['id']) + resp = self.request('/storagepools/default/storagevolumes/%s' % + vol_name) + self.assertEquals(200, resp.status) + + # Create a file with 5M to upload + # Max body size is set to 4M so the upload will fail with 413 + vol_name = '5m-file' + vol_path = os.path.join('/tmp', vol_name) + with open(vol_path, 'wb') as fd: + fd.seek(5*1024*1024-1) + fd.write("\0") + rollback.prependDefer(os.remove, vol_path) + + with open(vol_path, 'rb') as fd: + r = requests.post(url, + files={'file': fd}, + data={'name': vol_name}, + verify=False, + headers=fake_auth_header()) + + self.assertEquals(r.status_code, 413) class HttpsRestTests(RestTests): -- 1.9.3
Reviewed-by: Crístian Viana <vianac@linux.vnet.ibm.com> On 10-09-2014 14:39, Aline Manera wrote:
V1 -> V2: - Close file descriptors in test cases - Set max body size unit on nginx config file. For it add {} around Kimchi variables
Aline Manera (2): Identify Kimchi variables from nginx config variables in nginx.conf.in file bug fix: Properly set max body size to nginx proxy
src/kimchi/proxy.py | 25 +++++++------------ src/nginx.conf.in | 18 ++++++++------ tests/test_rest.py | 71 ++++++++++++++++++++++++++++++++++++++++------------- 3 files changed, 73 insertions(+), 41 deletions(-)
Applied. Thanks. Regards, Aline Manera
participants (2)
-
Aline Manera -
Crístian Viana