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

Proposed by Casey Marshall
Status: Work in progress
Proposed branch: lp:~cmars/charms/precise/haproxy/trunk
Merge into: lp:charms/haproxy
Diff against target: 217 lines (+106/-6)
3 files modified
config.yaml (+48/-0)
hooks/charmhelpers/fetch/__init__.py (+19/-2)
hooks/hooks.py (+39/-4)
To merge this branch: bzr merge lp:~cmars/charms/precise/haproxy/trunk
Reviewer Review Type Date Requested Status
Benjamin Saller (community) Needs Fixing
Casey Marshall (community) Needs Information
Marco Ceppi (community) Needs Fixing
Review via email: mp+201458@code.launchpad.net

Description of the change

Added config support alternate installation sources & keys.
Added config for SSL certificate to support reverse proxy SSL termination. Note that this option is only valid if you install a recent development build of haproxy 1.5. I have packaged haxproxy 1.5-dev in ppa:cloud-green/ppa if you're interested. However, if you don't specify this option & leave default (empty string), older haproxy packages should still work ok.

To post a comment you must log in.
Revision history for this message
Andrew Glen-Young (aglenyoung) wrote :

To prevent charm deployers attempting to use the 'ssl_cert' config option with Haproxy < 1.5 perhaps adding a caveat to the option's description would help?

Example:

ssl_cert:
    default: ""
    type: string
    description: |
        This option is only supported in Haproxy >= 1.5

        Use this SSL certificate for frontend SSL termination, if specified.
        This should be a concatenation of:
        [...]

79. By Casey Marshall

Add Haproxy 1.5 caveat to ssl_cert description.

Revision history for this message
Marco Ceppi (marcoceppi) wrote :

Hi Casey, unfortunately this currently breaks `make build` for the charm. Since tests aren't passing post merge I'm unable to approve this. FWIW, your changes seem good - the tests will simply need to be updated to reflect your changes.

--

Once you're able to get the tests passing and wish for another review, make sure to "Request another review" from "charmers" to have this added back to the queue again. Thanks for the work so far!

review: Needs Fixing
80. By Casey Marshall

Only configure_sources() if install_sources & install_keys is set,
otherwise it will raise an exception.

Revision history for this message
Casey Marshall (cmars) wrote :

I'm having a hard time with the Python mock library. I need it to have hooks.config_get return a string, either "" or the contents of a test certificate. However, the mock objects are evaluating to true, breaking the 'if ssl_cert:' logic I've added to hooks.

How can I get the mocked 'hooks.config_get' to provide a valid ssl_cert config value?

review: Needs Information
81. By Casey Marshall

apt-add-repository wasn't add repos by http: URL properly.
Fixed charmhelpers.fetch.add_source for this case.

82. By Casey Marshall

Split out SSL certificate, private key and ca cert chain options per hloeung suggestion.

83. By Casey Marshall

Append http:// repo sources to a distinct file in /etc/apt/sources.list.d/
instead of mucking up the main sources.list.

Revision history for this message
Benjamin Saller (bcsaller) wrote :

Thanks for this. I was unable to get the tests to run proper (as you indicated) which means we can't continue.

As far as helping with the (admittedly tricky) mocking issue you're seeing a series of changes like

    @patch.dict(os.environ, {"JUJU_UNIT_NAME": "haproxy/2"})
    @patch('hooks.config_get')
    def test_creates_a_listen_stanza_with_tuple_entries(self, mconfig):
        mconfig.side_effect = [None, None]

Will result in those tests passing.

Again, thank you.

review: Needs Fixing

Unmerged revisions

83. By Casey Marshall

Append http:// repo sources to a distinct file in /etc/apt/sources.list.d/
instead of mucking up the main sources.list.

82. By Casey Marshall

Split out SSL certificate, private key and ca cert chain options per hloeung suggestion.

81. By Casey Marshall

apt-add-repository wasn't add repos by http: URL properly.
Fixed charmhelpers.fetch.add_source for this case.

80. By Casey Marshall

Only configure_sources() if install_sources & install_keys is set,
otherwise it will raise an exception.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'config.yaml'
2--- config.yaml 2014-01-07 18:44:15 +0000
3+++ config.yaml 2014-01-24 21:27:36 +0000
4@@ -112,6 +112,33 @@
5 before the first variable, service_name, as above. Service options is a
6 comma separated list, server options will be appended as a string to
7 the individual server lines for a given listen stanza.
8+ ssl_cert:
9+ default: ""
10+ type: string
11+ description: |
12+ This option is only supported in Haproxy >= 1.5.
13+
14+ SSL public certificate for frontend SSL termination, if specified.
15+ When configured along with 'ssl_key', all bind stanzas will use this
16+ certificate.
17+ ssl_key:
18+ default: ""
19+ type: string
20+ description: |
21+ This option is only supported in Haproxy >= 1.5.
22+
23+ SSL private key for frontend SSL termination, if specified.
24+ When configured along with 'ssl_cert', all bind stanzas will use this key.
25+ The 'haproxy' user on the deployed unit will have read-access to the
26+ private key.
27+ ssl_chain:
28+ default: ""
29+ type: string
30+ description:
31+ This option is only supported in Haproxy >= 1.5.
32+
33+ SSL CA certificate chain for frontend SSL termination, if specified.
34+ Optional to enable SSL. Necessary when ssl_cert issued by an intermediate CA.
35 sysctl:
36 default: ""
37 type: string
38@@ -128,3 +155,24 @@
39 juju-postgresql-0
40 If you're running multiple environments with the same services in them
41 this allows you to differentiate between them.
42+ install_sources:
43+ default: ""
44+ type: string
45+ description: |
46+ YAML list of additional installation sources, as a string. The number of
47+ install_sources must match the number of install_keys. For example:
48+ install_sources: |
49+ - ppa:project1/ppa
50+ - ppa:project2/ppa
51+ install_keys:
52+ default: ""
53+ type: string
54+ description: |
55+ YAML list of GPG keys for installation sources, as a string. For apt repository
56+ URLs, use the public key ID used to verify package signatures. For
57+ other sources such as PPA, use empty string. This list must have the
58+ same number of elements as install_sources, even if the key items are
59+ all empty string. An example to go with the above for install_sources:
60+ install_keys: |
61+ - ""
62+ - ""
63
64=== modified file 'hooks/charmhelpers/fetch/__init__.py'
65--- hooks/charmhelpers/fetch/__init__.py 2013-08-21 19:19:29 +0000
66+++ hooks/charmhelpers/fetch/__init__.py 2014-01-24 21:27:36 +0000
67@@ -7,6 +7,7 @@
68 urlparse,
69 urlunparse,
70 )
71+import os
72 import subprocess
73 from charmhelpers.core.hookenv import (
74 config,
75@@ -21,6 +22,9 @@
76 deb http://archive.ubuntu.com/ubuntu {}-proposed main universe multiverse restricted
77 """
78
79+# APT repositories appended to a list by this charm
80+CONFIGURED_CHARM_SOURCES = "/etc/apt/sources.list.d/configured-charm-sources.list"
81+
82
83 def filter_installed_packages(packages):
84 """Returns a list of packages that require installation"""
85@@ -79,10 +83,23 @@
86 subprocess.call(cmd)
87
88
89+def contains_source(path, source):
90+ if not os.path.exists(path):
91+ return False
92+ with open(path, 'r') as apt:
93+ for line in apt.readlines():
94+ if line.endswith(source):
95+ return True
96+ return False
97+
98+
99 def add_source(source, key=None):
100- if ((source.startswith('ppa:') or
101- source.startswith('http:'))):
102+ if source.startswith('ppa:'):
103 subprocess.check_call(['add-apt-repository', '--yes', source])
104+ elif source.startswith('http:'):
105+ if not contains_source(CONFIGURED_CHARM_SOURCES, source):
106+ with open(CONFIGURED_CHARM_SOURCES, 'a+') as apt:
107+ apt.write("deb %s\n" % (source))
108 elif source.startswith('cloud:'):
109 apt_install(filter_installed_packages(['ubuntu-cloud-keyring']),
110 fatal=True)
111
112=== modified file 'hooks/hooks.py'
113--- hooks/hooks.py 2014-01-21 15:46:29 +0000
114+++ hooks/hooks.py 2014-01-24 21:27:36 +0000
115@@ -3,6 +3,7 @@
116 import base64
117 import glob
118 import os
119+import pwd
120 import re
121 import socket
122 import shutil
123@@ -25,7 +26,7 @@
124 close_port,
125 unit_get,
126 )
127-from charmhelpers.fetch import apt_install
128+from charmhelpers.fetch import apt_install, configure_sources
129 from charmhelpers.contrib.charmsupport import nrpe
130
131
132@@ -36,8 +37,10 @@
133 default_haproxy_config = "%s/haproxy.cfg" % default_haproxy_config_dir
134 default_haproxy_service_config_dir = "/var/run/haproxy"
135 default_haproxy_lib_dir = "/var/lib/haproxy"
136+default_haproxy_certkey="%s/certkey.pem" % (default_haproxy_config_dir)
137 service_affecting_packages = ['haproxy']
138
139+
140 dupe_options = [
141 "mode tcp",
142 "option tcplog",
143@@ -201,7 +204,7 @@
144 "listen\s+([^\s]+)\s+([^:]+):(.*)",
145 haproxy_config)
146 bind_stanzas = re.findall(
147- "\s+bind\s+([^:]+):(\d+)\s*\n\s+default_backend\s+([^\s]+)",
148+ "\s+bind\s+([^:]+):(\d+)\s?.*\n\s+default_backend\s+([^\s]+)",
149 haproxy_config, re.M)
150 return (tuple(((service, addr, int(port))
151 for service, addr, port in listen_stanzas)) +
152@@ -278,11 +281,18 @@
153 for out, cond, result in results:
154 out.extend(option for option, match in result
155 if match is cond and option not in out)
156+
157+ bind_extra = ""
158+ ssl_cert = config_get("ssl_cert")
159+ ssl_key = config_get("ssl_key")
160+ if ssl_cert and ssl_key:
161+ bind_extra = " ssl crt %s" % (default_haproxy_certkey)
162+
163 service_config = []
164 unit_name = os.environ["JUJU_UNIT_NAME"].replace("/", "-")
165 service_config.append("frontend %s-%s" % (unit_name, service_port))
166- service_config.append(" bind %s:%s" %
167- (service_ip, service_port))
168+ service_config.append(" bind %s:%s%s" %
169+ (service_ip, service_port, bind_extra))
170 service_config.append(" default_backend %s" % (service_name,))
171 service_config.extend(" %s" % service_option.strip()
172 for service_option in fe_options)
173@@ -736,12 +746,36 @@
174 if not os.path.exists(default_haproxy_service_config_dir):
175 os.mkdir(default_haproxy_service_config_dir, 0600)
176
177+ if config_get('install_sources') and config_get('install_keys'):
178+ configure_sources(True)
179+
180 apt_install('haproxy', fatal=True)
181 ensure_package_status(service_affecting_packages,
182 config_get('package_status'))
183 enable_haproxy()
184
185
186+#------------------------------------------------------------------------------
187+# update_ssl_cert: Update the haproxy SSL certificate from service config.
188+#------------------------------------------------------------------------------
189+def update_ssl_cert(config_data):
190+ ssl_cert = config_data.get('ssl_cert')
191+ ssl_key = config_data.get('ssl_key')
192+ ssl_chain = config_data.get('ssl_chain')
193+ if ssl_cert:
194+ with open(default_haproxy_certkey, "w") as f:
195+ f.write(ssl_cert)
196+ f.write(ssl_chain)
197+ f.write(ssl_key)
198+ os.chmod(default_haproxy_certkey, 0600)
199+ # See <pwd.h> struct passwd for more info on the pwd tuple positions.
200+ pwd_info = pwd.getpwnam(config_data['global_user'])
201+ os.chown(default_haproxy_certkey, pwd_info[2], pwd_info[3])
202+ else:
203+ if os.path.exists(default_haproxy_certkey):
204+ os.unlink(default_haproxy_certkey)
205+
206+
207 def config_changed():
208 config_data = config_get()
209
210@@ -761,6 +795,7 @@
211 sys.exit()
212 haproxy_services = load_services()
213 update_sysctl(config_data)
214+ update_ssl_cert(config_data)
215 construct_haproxy_config(haproxy_globals,
216 haproxy_defaults,
217 haproxy_monitoring,

Subscribers

People subscribed via source and target branches