Juju Charms Collection
haproxy package

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

  1. Precise Pangolin (12.04)
  2. trunk
  3. Merge into trunk
Proposed by Jerry Seutter
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 (community) Approve
Jerry Seutter (community) Abstain
Chris Glass (community) Approve
Adam Collard (community) Approve
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.
Revision history for this message
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
lp:~jseutter/charms/precise/haproxy/trunk updated
72. By Jerry Seutter

Integrate review feedback.

73. By Jerry Seutter

Build path using os.path.join()

Revision history for this message
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!

Revision history for this message
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
Revision history for this message
Jerry Seutter (jseutter) :
review: Abstain
lp:~jseutter/charms/precise/haproxy/trunk updated
74. By Jerry Seutter

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

Revision history for this message
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!

lp:~jseutter/charms/precise/haproxy/trunk updated
75. By Jerry Seutter

Remove unused 'path' key.

Revision history for this message
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