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
=== modified file 'hooks/hooks.py'
--- hooks/hooks.py 2013-12-11 04:14:19 +0000
+++ hooks/hooks.py 2013-12-17 16:02:40 +0000
@@ -1,5 +1,6 @@
1#!/usr/bin/env python1#!/usr/bin/env python
22
3import base64
3import glob4import glob
4import os5import os
5import re6import re
@@ -34,6 +35,7 @@
34default_haproxy_config_dir = "/etc/haproxy"35default_haproxy_config_dir = "/etc/haproxy"
35default_haproxy_config = "%s/haproxy.cfg" % default_haproxy_config_dir36default_haproxy_config = "%s/haproxy.cfg" % default_haproxy_config_dir
36default_haproxy_service_config_dir = "/var/run/haproxy"37default_haproxy_service_config_dir = "/var/run/haproxy"
38default_haproxy_lib_dir = "/var/lib/haproxy"
37service_affecting_packages = ['haproxy']39service_affecting_packages = ['haproxy']
3840
39dupe_options = [41dupe_options = [
@@ -247,10 +249,14 @@
247# server_ip249# server_ip
248# server_port250# server_port
249# server_options251# server_options
252# errorfiles: List of dicts
253# http_status: status to handle
254# content: base 64 content for HAProxy to
255# write to socket
250#------------------------------------------------------------------------------256#------------------------------------------------------------------------------
251def create_listen_stanza(service_name=None, service_ip=None,257def create_listen_stanza(service_name=None, service_ip=None,
252 service_port=None, service_options=None,258 service_port=None, service_options=None,
253 server_entries=None):259 server_entries=None, service_errorfiles=None):
254 if service_name is None or service_ip is None or service_port is None:260 if service_name is None or service_ip is None or service_port is None:
255 return None261 return None
256 fe_options = []262 fe_options = []
@@ -284,6 +290,13 @@
284 service_config.append("backend %s" % (service_name,))290 service_config.append("backend %s" % (service_name,))
285 service_config.extend(" %s" % service_option.strip()291 service_config.extend(" %s" % service_option.strip()
286 for service_option in be_options)292 for service_option in be_options)
293 if service_errorfiles is not None:
294 for errorfile in service_errorfiles:
295 path = os.path.join(default_haproxy_lib_dir,
296 "service_%s" % service_name,
297 "%s.http" % errorfile["http_status"])
298 service_config.append(
299 " errorfile %s %s" % (errorfile["http_status"], path))
287 if isinstance(server_entries, (list, tuple)):300 if isinstance(server_entries, (list, tuple)):
288 for (server_name, server_ip, server_port,301 for (server_name, server_ip, server_port,
289 server_options) in server_entries:302 server_options) in server_entries:
@@ -596,6 +609,17 @@
596 log("Service: %s" % service_key)609 log("Service: %s" % service_key)
597 server_entries = service_config.get('servers')610 server_entries = service_config.get('servers')
598611
612 errorfiles = service_config.get('errorfiles', [])
613 for errorfile in errorfiles:
614 service_name = services_dict[service_key]['service_name']
615 path = os.path.join(default_haproxy_lib_dir,
616 "service_%s" % service_name)
617 if not os.path.exists(path):
618 os.makedirs(path)
619 full_path = os.path.join(path, "%s.http" % errorfile["http_status"])
620 with open(full_path, 'w') as f:
621 f.write(base64.b64decode(errorfile["content"]))
622
599 service_name = service_config["service_name"]623 service_name = service_config["service_name"]
600 if not os.path.exists(default_haproxy_service_config_dir):624 if not os.path.exists(default_haproxy_service_config_dir):
601 os.mkdir(default_haproxy_service_config_dir, 0600)625 os.mkdir(default_haproxy_service_config_dir, 0600)
@@ -606,11 +630,11 @@
606 service_config['service_host'],630 service_config['service_host'],
607 service_config['service_port'],631 service_config['service_port'],
608 service_config['service_options'],632 service_config['service_options'],
609 server_entries))633 server_entries, errorfiles))
610634
611635
612#------------------------------------------------------------------------------636#------------------------------------------------------------------------------
613# load_services: Convenience function that load the service snippet637# load_services: Convenience function that loads the service snippet
614# configuration from the filesystem.638# configuration from the filesystem.
615#------------------------------------------------------------------------------639#------------------------------------------------------------------------------
616def load_services(service_name=None):640def load_services(service_name=None):
617641
=== modified file 'hooks/tests/test_helpers.py'
--- hooks/tests/test_helpers.py 2013-10-25 21:33:52 +0000
+++ hooks/tests/test_helpers.py 2013-12-17 16:02:40 +0000
@@ -1,3 +1,4 @@
1import base64
1import os2import os
23
3from contextlib import contextmanager4from contextlib import contextmanager
@@ -352,6 +353,42 @@
352353
353 self.assertEqual(expected, result)354 self.assertEqual(expected, result)
354355
356 @patch.dict(os.environ, {"JUJU_UNIT_NAME": "haproxy/2"})
357 def test_creates_a_listen_stanza_with_errorfiles(self):
358 service_name = 'some-name'
359 service_ip = '10.11.12.13'
360 service_port = 1234
361 service_options = ('foo', 'bar')
362 server_entries = [
363 ('name-1', 'ip-1', 'port-1', ('foo1', 'bar1')),
364 ('name-2', 'ip-2', 'port-2', ('foo2', 'bar2')),
365 ]
366 content = ("HTTP/1.0 403 Forbidden\r\n"
367 "Content-Type: text/html\r\n"
368 "\r\n"
369 "<html></html>")
370 errorfiles = [{'http_status': 403,
371 'content': base64.b64encode(content)}]
372
373 result = hooks.create_listen_stanza(service_name, service_ip,
374 service_port, service_options,
375 server_entries, errorfiles)
376
377 expected = '\n'.join((
378 'frontend haproxy-2-1234',
379 ' bind 10.11.12.13:1234',
380 ' default_backend some-name',
381 '',
382 'backend some-name',
383 ' foo',
384 ' bar',
385 ' errorfile 403 /var/lib/haproxy/service_some-name/403.http',
386 ' server name-1 ip-1:port-1 foo1 bar1',
387 ' server name-2 ip-2:port-2 foo2 bar2',
388 ))
389
390 self.assertEqual(expected, result)
391
355 def test_doesnt_create_listen_stanza_if_args_not_provided(self):392 def test_doesnt_create_listen_stanza_if_args_not_provided(self):
356 self.assertIsNone(hooks.create_listen_stanza())393 self.assertIsNone(hooks.create_listen_stanza())
357394
358395
=== modified file 'hooks/tests/test_peer_hooks.py'
--- hooks/tests/test_peer_hooks.py 2013-09-19 12:45:40 +0000
+++ hooks/tests/test_peer_hooks.py 2013-12-17 16:02:40 +0000
@@ -1,3 +1,4 @@
1import base64
1import os2import os
2import yaml3import yaml
34
@@ -194,7 +195,40 @@
194 hooks.write_service_config(services_dict)195 hooks.write_service_config(services_dict)
195196
196 create_listen_stanza.assert_called_with(197 create_listen_stanza.assert_called_with(
197 'bar', 'some-host', 'some-port', 'some-options', (1, 2))198 'bar', 'some-host', 'some-port', 'some-options', (1, 2), [])
198 mock_open.assert_called_with(199 mock_open.assert_called_with(
199 '/var/run/haproxy/bar.service', 'w')200 '/var/run/haproxy/bar.service', 'w')
200 mock_file.write.assert_called_with('some content')201 mock_file.write.assert_called_with('some content')
202
203 @patch('hooks.create_listen_stanza')
204 def test_writes_errorfiles(self, create_listen_stanza):
205 create_listen_stanza.return_value = 'some content'
206
207 content = ("HTTP/1.0 403 Forbidden\r\n"
208 "Content-Type: text/html\r\n"
209 "\r\n"
210 "<html></html>")
211 services_dict = {
212 'foo': {
213 'service_name': 'bar',
214 'service_host': 'some-host',
215 'service_port': 'some-port',
216 'service_options': 'some-options',
217 'servers': (1, 2),
218 'errorfiles': [{
219 'http_status': 403,
220 'content': base64.b64encode(content)
221 }]
222 },
223 }
224
225 with patch.object(os.path, "exists") as exists:
226 exists.return_value = True
227 with patch_open() as (mock_open, mock_file):
228 hooks.write_service_config(services_dict)
229
230 mock_open.assert_any_call(
231 '/var/lib/haproxy/service_bar/403.http', 'w')
232 mock_file.write.assert_any_call(content)
233 self.assertTrue(create_listen_stanza.called)
234

Subscribers

People subscribed via source and target branches