Merge lp:~jseutter/charms/precise/haproxy/trunk into lp:charms/haproxy

Proposed by Jerry Seutter on 2013-12-11
Status: Merged
Merged at revision: 72
Proposed branch: lp:~jseutter/charms/precise/haproxy/trunk
Merge into: lp:charms/haproxy
Diff against target: 183 lines (+99/-4)
3 files modified
hooks/hooks.py (+27/-3)
hooks/tests/test_helpers.py (+37/-0)
hooks/tests/test_peer_hooks.py (+35/-1)
To merge this branch: bzr merge lp:~jseutter/charms/precise/haproxy/trunk
Reviewer Review Type Date Requested Status
Marco Ceppi 2013-12-11 Approve on 2014-01-03
Jerry Seutter (community) Abstain on 2013-12-13
Chris Glass (community) Approve on 2013-12-13
Adam Collard (community) 2013-12-12 Approve on 2013-12-12
Review via email: mp+198646@code.launchpad.net

Description of the change

This adds support for the backend service to specify errorfiles in the service configuration. If errorfiles are supplied, the haproxy charm will write them to /var/lib/haproxy/<service_name>/<http status>.html and configure haproxy to use them.

Currently only the Landscape charm supplies errorfiles.

To post a comment you must log in.
Adam Collard (adam-collard) wrote :

8 +import base64

Move this up to the first import line so the imports stay in alphabetical order

57 + path = "%s/service_%s" % (default_haproxy_lib_dir, service_name)
58 + if not os.path.exists(path):
59 + os.makedirs(path)
60 + full_path = "%s/%s.html" % (path, errorfile["http_status"])

Use os.path.join() for the path constructions.

121 +

One too many blank lines?

+ self.assertTrue(create_listen_stanza.called)
171 +

move that outside of the with-block - you don't need the patching in place for this

review: Approve
72. By Jerry Seutter on 2013-12-12

Integrate review feedback.

73. By Jerry Seutter on 2013-12-13

Build path using os.path.join()

Jerry Seutter (jseutter) wrote :

> 8 +import base64
>
> Move this up to the first import line so the imports stay in alphabetical
> order

Fixed.

>
>
> 57 + path = "%s/service_%s" % (default_haproxy_lib_dir, service_name)
> 58 + if not os.path.exists(path):
> 59 + os.makedirs(path)
> 60 + full_path = "%s/%s.html" % (path, errorfile["http_status"])
>
> Use os.path.join() for the path constructions.
>

Fixed

> 121 +
>
> One too many blank lines?
>

Fixed

> + self.assertTrue(create_listen_stanza.called)
> 171 +
>
> move that outside of the with-block - you don't need the patching in place for
> this

Fixed. Thanks!

Chris Glass (tribaal) wrote :

Great! +1

The following points are different facets of the same thing:

[0]
+ "%s.html" % errorfile["http_status"])

The HAProxy configuration suggests to call the files *.http instead, since their content is printed to the socket verbatim (it's really a serialized http response, not an HTML file).

[1]
+ errorfiles = [{'http_status': 403, 'path': '/path/403.html',
+ 'content': base64.b64encode('<html></html>')}]

Similar comment, it would probably be good to use complete values to serve as an example, like: http://paste.ubuntu.com/6567575/

[2]
+# content: base 64 content to serve as doc

Maybe rephrase to "base 64 content for HAproxy to print to socket"?

review: Approve
Jerry Seutter (jseutter) :
review: Abstain
74. By Jerry Seutter on 2013-12-13

Error file extension is now .http
The code now reflects the fact that errorfiles is the raw HTTP response,
not just an html document.

Jerry Seutter (jseutter) wrote :

> Great! +1
>
> The following points are different facets of the same thing:
>
> [0]
> + "%s.html" % errorfile["http_status"])
>
> The HAProxy configuration suggests to call the files *.http instead, since
> their content is printed to the socket verbatim (it's really a serialized http
> response, not an HTML file).

Ah, I had read that once but it didn't stick in my brain. Fixed now.

> [1]
> + errorfiles = [{'http_status': 403, 'path': '/path/403.html',
> + 'content': base64.b64encode('<html></html>')}]
>
> Similar comment, it would probably be good to use complete values to serve as
> an example, like: http://paste.ubuntu.com/6567575/

Fixed.

> [2]
> +# content: base 64 content to serve as doc
>
> Maybe rephrase to "base 64 content for HAproxy to print to socket"?

Fixed. Thanks, tribaal!

75. By Jerry Seutter on 2013-12-17

Remove unused 'path' key.

Marco Ceppi (marcoceppi) wrote :

LGTM, +1! Thanks

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'hooks/hooks.py'
2--- hooks/hooks.py 2013-12-11 04:14:19 +0000
3+++ hooks/hooks.py 2013-12-17 16:02:40 +0000
4@@ -1,5 +1,6 @@
5 #!/usr/bin/env python
6
7+import base64
8 import glob
9 import os
10 import re
11@@ -34,6 +35,7 @@
12 default_haproxy_config_dir = "/etc/haproxy"
13 default_haproxy_config = "%s/haproxy.cfg" % default_haproxy_config_dir
14 default_haproxy_service_config_dir = "/var/run/haproxy"
15+default_haproxy_lib_dir = "/var/lib/haproxy"
16 service_affecting_packages = ['haproxy']
17
18 dupe_options = [
19@@ -247,10 +249,14 @@
20 # server_ip
21 # server_port
22 # server_options
23+# errorfiles: List of dicts
24+# http_status: status to handle
25+# content: base 64 content for HAProxy to
26+# write to socket
27 #------------------------------------------------------------------------------
28 def create_listen_stanza(service_name=None, service_ip=None,
29 service_port=None, service_options=None,
30- server_entries=None):
31+ server_entries=None, service_errorfiles=None):
32 if service_name is None or service_ip is None or service_port is None:
33 return None
34 fe_options = []
35@@ -284,6 +290,13 @@
36 service_config.append("backend %s" % (service_name,))
37 service_config.extend(" %s" % service_option.strip()
38 for service_option in be_options)
39+ if service_errorfiles is not None:
40+ for errorfile in service_errorfiles:
41+ path = os.path.join(default_haproxy_lib_dir,
42+ "service_%s" % service_name,
43+ "%s.http" % errorfile["http_status"])
44+ service_config.append(
45+ " errorfile %s %s" % (errorfile["http_status"], path))
46 if isinstance(server_entries, (list, tuple)):
47 for (server_name, server_ip, server_port,
48 server_options) in server_entries:
49@@ -596,6 +609,17 @@
50 log("Service: %s" % service_key)
51 server_entries = service_config.get('servers')
52
53+ errorfiles = service_config.get('errorfiles', [])
54+ for errorfile in errorfiles:
55+ service_name = services_dict[service_key]['service_name']
56+ path = os.path.join(default_haproxy_lib_dir,
57+ "service_%s" % service_name)
58+ if not os.path.exists(path):
59+ os.makedirs(path)
60+ full_path = os.path.join(path, "%s.http" % errorfile["http_status"])
61+ with open(full_path, 'w') as f:
62+ f.write(base64.b64decode(errorfile["content"]))
63+
64 service_name = service_config["service_name"]
65 if not os.path.exists(default_haproxy_service_config_dir):
66 os.mkdir(default_haproxy_service_config_dir, 0600)
67@@ -606,11 +630,11 @@
68 service_config['service_host'],
69 service_config['service_port'],
70 service_config['service_options'],
71- server_entries))
72+ server_entries, errorfiles))
73
74
75 #------------------------------------------------------------------------------
76-# load_services: Convenience function that load the service snippet
77+# load_services: Convenience function that loads the service snippet
78 # configuration from the filesystem.
79 #------------------------------------------------------------------------------
80 def load_services(service_name=None):
81
82=== modified file 'hooks/tests/test_helpers.py'
83--- hooks/tests/test_helpers.py 2013-10-25 21:33:52 +0000
84+++ hooks/tests/test_helpers.py 2013-12-17 16:02:40 +0000
85@@ -1,3 +1,4 @@
86+import base64
87 import os
88
89 from contextlib import contextmanager
90@@ -352,6 +353,42 @@
91
92 self.assertEqual(expected, result)
93
94+ @patch.dict(os.environ, {"JUJU_UNIT_NAME": "haproxy/2"})
95+ def test_creates_a_listen_stanza_with_errorfiles(self):
96+ service_name = 'some-name'
97+ service_ip = '10.11.12.13'
98+ service_port = 1234
99+ service_options = ('foo', 'bar')
100+ server_entries = [
101+ ('name-1', 'ip-1', 'port-1', ('foo1', 'bar1')),
102+ ('name-2', 'ip-2', 'port-2', ('foo2', 'bar2')),
103+ ]
104+ content = ("HTTP/1.0 403 Forbidden\r\n"
105+ "Content-Type: text/html\r\n"
106+ "\r\n"
107+ "<html></html>")
108+ errorfiles = [{'http_status': 403,
109+ 'content': base64.b64encode(content)}]
110+
111+ result = hooks.create_listen_stanza(service_name, service_ip,
112+ service_port, service_options,
113+ server_entries, errorfiles)
114+
115+ expected = '\n'.join((
116+ 'frontend haproxy-2-1234',
117+ ' bind 10.11.12.13:1234',
118+ ' default_backend some-name',
119+ '',
120+ 'backend some-name',
121+ ' foo',
122+ ' bar',
123+ ' errorfile 403 /var/lib/haproxy/service_some-name/403.http',
124+ ' server name-1 ip-1:port-1 foo1 bar1',
125+ ' server name-2 ip-2:port-2 foo2 bar2',
126+ ))
127+
128+ self.assertEqual(expected, result)
129+
130 def test_doesnt_create_listen_stanza_if_args_not_provided(self):
131 self.assertIsNone(hooks.create_listen_stanza())
132
133
134=== modified file 'hooks/tests/test_peer_hooks.py'
135--- hooks/tests/test_peer_hooks.py 2013-09-19 12:45:40 +0000
136+++ hooks/tests/test_peer_hooks.py 2013-12-17 16:02:40 +0000
137@@ -1,3 +1,4 @@
138+import base64
139 import os
140 import yaml
141
142@@ -194,7 +195,40 @@
143 hooks.write_service_config(services_dict)
144
145 create_listen_stanza.assert_called_with(
146- 'bar', 'some-host', 'some-port', 'some-options', (1, 2))
147+ 'bar', 'some-host', 'some-port', 'some-options', (1, 2), [])
148 mock_open.assert_called_with(
149 '/var/run/haproxy/bar.service', 'w')
150 mock_file.write.assert_called_with('some content')
151+
152+ @patch('hooks.create_listen_stanza')
153+ def test_writes_errorfiles(self, create_listen_stanza):
154+ create_listen_stanza.return_value = 'some content'
155+
156+ content = ("HTTP/1.0 403 Forbidden\r\n"
157+ "Content-Type: text/html\r\n"
158+ "\r\n"
159+ "<html></html>")
160+ services_dict = {
161+ 'foo': {
162+ 'service_name': 'bar',
163+ 'service_host': 'some-host',
164+ 'service_port': 'some-port',
165+ 'service_options': 'some-options',
166+ 'servers': (1, 2),
167+ 'errorfiles': [{
168+ 'http_status': 403,
169+ 'content': base64.b64encode(content)
170+ }]
171+ },
172+ }
173+
174+ with patch.object(os.path, "exists") as exists:
175+ exists.return_value = True
176+ with patch_open() as (mock_open, mock_file):
177+ hooks.write_service_config(services_dict)
178+
179+ mock_open.assert_any_call(
180+ '/var/lib/haproxy/service_bar/403.http', 'w')
181+ mock_file.write.assert_any_call(content)
182+ self.assertTrue(create_listen_stanza.called)
183+

Subscribers

People subscribed via source and target branches