Merge lp:~evarlast/charms/trusty/apache2/trunk into lp:charms/trusty/apache2

[Jay R. Wren]

Add apt config options to allow apache2 to be installed from a PPA. This enabled use of http2 supporting apache.

[lp:~evarlast/charms/trusty/apache2/trunk]

69. By Jay R. Wren on 2015-11-20

do not enable http2 by default

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/1486/

review: Needs Fixing (automated testing)

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/1469/

review: Needs Fixing (automated testing)

[Andrew McLeod (admcleod)]

Hi Jay,

In order to confirm your changes are fully functional, I've attempted to run bundletester (bundletester -e $AWS -l DEBUG -vF) on this.

It appears that there are initial failures related to apt-source, but because of the 'F' I see that there are also older issues related to assert_called_once (see: https://bugs.launchpad.net/charms/+source/apache2/+bug/1511474)

Unfortunately because tests are failing (as noted by the automated testing also) I can't approve this - however, perhaps they're passing for you? If so can you tell me more about your environment so I can try to replicate it - I'm using trusty.

Thanks!
Andrew

review: Needs Fixing

[Andrew McLeod (admcleod)]

Jay,

I've just seen another RQ item from you where you've fixed some of those assert_called_once errors, but still failing on the following:

http://pastebin.ubuntu.com/13898257/

I've put this same pastebin in your other RQ item.

Andrew

review: Needs Fixing

[Adam Israel (aisrael)]

Hi Jay,

Just following up from our conversation on IRC yesterday. There are some failing tests here that are in the upstream, but new failing tests are introduced with this merge. Specifically:

======================================================================
ERROR: tests.test_balancer_hook.HooksTest.test_installs_hook
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/tmp/bundletester-ylCDXc/apache2/.venv/local/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "/tmp/bundletester-ylCDXc/apache2/hooks/tests/test_balancer_hook.py", line 512, in test_installs_hook
    result = hooks.install_hook()
  File "/tmp/bundletester-ylCDXc/apache2/hooks/hooks.py", line 294, in install_hook
    open('config.apt-source', 'w').write(apt_source)
TypeError: expected a character buffer object

======================================================================
ERROR: tests.test_config_changed.ConfigChangedTest.test_config_changed_ensure_empty_site_dir
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/tmp/bundletester-ylCDXc/apache2/.venv/local/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "/tmp/bundletester-ylCDXc/apache2/hooks/tests/test_config_changed.py", line 76, in test_config_changed_ensure_empty_site_dir
    hooks.config_changed()
  File "/tmp/bundletester-ylCDXc/apache2/hooks/hooks.py", line 602, in config_changed
    apt_source = config_data['apt-source']
KeyError: 'apt-source'

review: Needs Fixing

[lp:~evarlast/charms/trusty/apache2/trunk]

70. By Jay R. Wren on 2016-01-16

fix tests which I broke

[Jay R. Wren (evarlast)]

Thanks for this. I think I have fixed the required tests.

Please take another look.

[Haw Loeung (hloeung)]

These are pretty bad defaults. We're adding the http://ppa.launchpad.net/ondrej/apache2/ubuntu PPA and keys?

I think the defaults for apt-key-id and apt-source should be empty.

[Jay R. Wren (evarlast)]

Indeed! I intended them to be empty. Thank Haw.

[lp:~evarlast/charms/trusty/apache2/trunk]

71. By Jay R. Wren on 2016-01-19

default apt-key-id and apt-source to empty string

[Andrew McLeod (admcleod)]

Hi Jay,

There are still tests failing (http://pastebin.ubuntu.com/15017349/) and some of these are the ones Adam pasted further up in this thread. I'm trying to recall if we discussed similar failing tests in another thread. I ran these tests in a charmbox on AWS, perhaps this is unique to my environment?

review: Needs Fixing

[Jay R. Wren (evarlast)]

Maybe someone can help me out with what changes in this MR cause these
tests to fail?

On Thu, Feb 11, 2016 at 6:01 PM, Andrew McLeod
<email address hidden> wrote:
> Review: Needs Fixing
>
> Hi Jay,
>
> There are still tests failing (http://pastebin.ubuntu.com/15017349/) and some of these are the ones Adam pasted further up in this thread. I'm trying to recall if we discussed similar failing tests in another thread. I ran these tests in a charmbox on AWS, perhaps this is unique to my environment?
> --
> https://code.launchpad.net/~evarlast/charms/trusty/apache2/trunk/+merge/278220
> You are the owner of lp:~evarlast/charms/trusty/apache2/trunk.

[Kevin W Monroe (kwmonroe)]

Hey Jay - it's likely that nothing you did in this MP caused these failures. In fact, I'm quite certain that the 'assert_called_once' failures are happening because 'make test' is pulling in a newer version of mock (v1.3.0) that is more strict about mock'd methods [0].

That said, I agree with Andrew's desire to get tests passing before committing updates to a charm as important as apache2. If you have ideas on how to fix these test issues (even though you didn't introduce them), that would be much appreciated. If not, please at least hit the 'affects me too' on bug [0] to raise the heat.

Meanwhile, I'll try to poke on a test refactor to get around the assert_called_once errors, but we'll still need to dig into the other failures Andrew found in his latest pastebin.

[0] - https://bugs.launchpad.net/charms/+source/apache2/+bug/1511474

[Kevin W Monroe (kwmonroe)]

Hey Jay! I didn't realize when I pointed you at bug 1511474 that you already fixed it way back in November (which made it in with your add-logs-interface merge to apache2 today [0]).

That clears up all the apache2 errors that weren't your fault, but we're not out of the woods yet :) 'make test' with these changes merged into the new apache2 trunk branch yields:

======================================================================
ERROR: tests.test_config_changed.ConfigChangedTest.test_config_changed_ensure_empty_site_dir
----------------------------------------------------------------------
_StringException: Traceback (most recent call last):
  File "/tmp/bundletester-0mKWLv/apache2/.venv/local/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
    return func(*args, **keywargs)
  File "/tmp/bundletester-0mKWLv/apache2/hooks/tests/test_config_changed.py", line 81, in test_config_changed_ensure_empty_site_dir
    hooks.config_changed()
  File "/tmp/bundletester-0mKWLv/apache2/hooks/hooks.py", line 604, in config_changed
    old_apt_source = open('config.apt-source', 'r').read()
IOError: [Errno 2] No such file or directory: 'config.apt-source'

This happens because install_hook() is the only place config.apt-source is written, and we're not invoking that in test_config_changed.py. You could mock a bunch of stuff out to handle that (like open and whatever subprocess stuff happens in config_changed), but but do you think it would be easier to alter your diff with a try/except during config_changed() in hooks.py?

...
     relationship_data = {}
     config_data = config_get()

+ apt_source = config_data['apt-source']
+ try:
+ old_apt_source = open('config.apt-source', 'r').read()
+ except IOError:
+ # File may not exist if install hook hasn't run (unlikely) or during
+ # ./hooks/tests/test-config-changed.py. Either way, it is safe to
+ # assume if this file DNE, our old apt source == current apt config.
+ old_apt_source = apt_source
...

In the same method, do you need to write the new repo config to the config.apt-source file? Altering your diff like:

+ if old_apt_source != apt_source:
+ subprocess.check_call(['add-apt-repository', '--yes', '-r',
+ old_apt_source])
+ add_source(apt_source, config_data['apt-key-id'])
+ open('config.apt-source', 'w+').write(apt_source)

Otherwise, I don't think your "old_apt_source != apt_source" test will be accurate after changing that config.

One last morsel of awesome, test_config_changed.py needs to close the hookenv.Config with a paren:

- }
+ "apt-source": "",
+ })

I know this MP has been a long time coming, and I *really* appreciate the fixes to apache2/trunk that you didn't cause. I think we're close with this one, but I want to get your input on these last few concerns of mine. Thanks!!

[0] - https://code.launchpad.net/~evarlast/charms/trusty/apache2/add-logs-interface/+merge/278222

[Kevin W Monroe (kwmonroe)]

review: Needs Information

[lp:~evarlast/charms/trusty/apache2/trunk]

72. By Jay R. Wren on 2016-02-24

fix config changed handling of apt-source

[Jay R. Wren (evarlast)]

Great catch @kevin.

I've updated to include suggested fixes.

review: Needs Resubmitting

[lp:~evarlast/charms/trusty/apache2/trunk]

73. By Jay R. Wren on 2016-02-24

merge parent

74. By Jay R. Wren on 2016-02-25

i knew that was bug

[Kevin W Monroe (kwmonroe)]

Thanks for the updates Jay! Tests pass and I verified apt config settings are honored during both install and config-changed.

This has been merged into trusty/apache2. An updated charm should hit the store in about an hour. Thanks again for your patience during this review!

review: Approve

=== modified file 'Makefile'
--- Makefile	2015-11-23 19:12:12 +0000
+++ Makefile	2016-02-25 16:17:14 +0000
@@ -17,8 +17,16 @@
 	@(charm proof $(PWD) || [ $$? -eq 100 ]) && echo OK
 	@test `cat revision` = 0 && rm revision
 
-.venv:
-	sudo apt-get install -y python-apt python-virtualenv python-jinja2 python-openssl
+/usr/bin/apt:
+	sudo apt-get install -y python-apt
+
+/usr/bin/virtualenv:
+	sudo apt-get install -y python-virtualenv
+
+/usr/lib/python2.7/dist-packages/jinja2:
+	sudo apt-get install -y python-jinja2
+
+.venv: /usr/bin/apt /usr/bin/virtualenv /usr/lib/python2.7/dist-packages/jinja2
 	virtualenv .venv --system-site-packages
 	.venv/bin/pip install -I nose testtools mock pyyaml
 
@@ -42,4 +50,7 @@
 		-d hooks/charmhelpers
 	@echo Do not forget to commit the updated files if any.
 
-.PHONY: revision proof .venv test lint sourcedeps charm-payload
+clean:
+	rm -rf .venv
+
+.PHONY: revision proof test lint sourcedeps charm-payload
 
=== modified file 'config.yaml'
--- config.yaml	2015-08-11 19:06:25 +0000
+++ config.yaml	2016-02-25 16:17:14 +0000
@@ -193,3 +193,16 @@
     type: string
     description: Comma seperated list of OpenID providers for authentication.
     default: ""
+  apt-key-id:
+    type: string
+    default: ""
+    description: A PGP key id.  This is used with PPA and the source
+      option to import a PGP public key for verifying repository signatures.
+      This value must match the PPA for apt-source.
+  apt-source:
+    type: string
+    default: ""
+    description: From where to install packages. This is the PPA source line.
+      Note that due to a bug in software-properties add-apt-repository cannot
+        add the ondrej/apache2 ppa, so the default value here is a full
+        sources line.
 
=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2016-02-18 18:08:34 +0000
+++ hooks/hooks.py	2016-02-25 16:17:14 +0000
@@ -25,7 +25,7 @@
     unit_get
 )
 from charmhelpers.contrib.charmsupport import nrpe
-from charmhelpers.fetch import apt_update
+from charmhelpers.fetch import apt_update, add_source
 
 ###############################################################################
 # Global variables
@@ -289,6 +289,12 @@
 
 
 def install_hook():
+    apt_source = config_get('apt-source') or ''
+    apt_key_id = config_get('apt-key-id') or False
+    open('config.apt-source', 'w').write(apt_source)
+    if apt_source and apt_key_id:
+        print apt_source + " and " + apt_key_id
+        add_source(apt_source, apt_key_id)
     if not os.path.exists(default_apache2_service_config_dir):
         os.mkdir(default_apache2_service_config_dir, 0600)
     apt_update(fatal=True)
@@ -301,6 +307,15 @@
         ensure_package_status(service_affecting_packages,
                               config_get('package_status'))
     ensure_extra_packages()
+    # the apache2 deb does not yet have http2 module in mods-available. Add it.
+    open('/etc/apache2/mods-available/http2.load', 'w').write(
+        'LoadModule http2_module /usr/lib/apache2/modules/mod_http2.so')
+    open('/etc/apache2/mods-available/http2.conf', 'w').write(
+        '''<IfModule http2_module>
+  ProtocolsHonorOrder On
+  Protocols h2 http/1.1
+</IfModule>
+''')
     return install_status
 
 
@@ -585,6 +600,18 @@
     relationship_data = {}
     config_data = config_get()
 
+    apt_source = config_data['apt-source']
+    old_apt_source = ''
+    try:
+        old_apt_source = open('config.apt-source', 'r').read()
+    except:
+        pass
+    if old_apt_source != apt_source:
+        subprocess.check_call(['add-apt-repository', '--yes', '-r',
+                               old_apt_source])
+        add_source(apt_source, config_data['apt-key-id'])
+        open('config.apt-source', 'w').write(apt_source)
+
     ensure_package_status(service_affecting_packages,
                           config_data['package_status'])
     ensure_extra_packages()
 
=== modified file 'hooks/tests/test_balancer_hook.py'
--- hooks/tests/test_balancer_hook.py	2014-12-29 22:59:20 +0000
+++ hooks/tests/test_balancer_hook.py	2016-02-25 16:17:14 +0000
@@ -497,6 +497,7 @@
         if os.path.exists(self.log_path):
             os.remove(self.log_path)
 
+    @patch('hooks.open')
     @patch('hooks.config_get')
     @patch('os.path.exists')
     @patch('os.mkdir')
@@ -504,7 +505,8 @@
     @patch('hooks.log', MagicMock())
     @patch('hooks.apt_update')
     def test_installs_hook(
-            self, apt_update, apt_get_install, mkdir, exists, config_get):
+            self, apt_update, apt_get_install, mkdir, exists, config_get,
+            open):
         exists.return_value = self.not_a_dir
         config_get.return_value = None
         apt_get_install.return_value = 'some result'
@@ -523,19 +525,22 @@
             call('apache2'),
         ])
 
-    @patch('hooks.config_get')
+    @patch('hooks.open')
     @patch('os.path.exists')
     @patch('os.mkdir')
     @patch('hooks.apt_get_install')
     @patch('hooks.log', MagicMock())
     @patch('hooks.apt_update')
     def test_install_hook_installs_extra_packages(
-            self, apt_update, apt_get_install, mkdir, exists, config_get):
+            self, apt_update, apt_get_install, mkdir, exists,
+            open):
         exists.return_value = self.dir_exists
-        config_get.return_value = "extra"
+        c = {'extra_packages': 'extra', 'apt-source': '', 'apt-key-id': ''}
+        config_get = MagicMock(wraps=c.get)
         apt_get_install.return_value = 'some result'
 
-        result = hooks.install_hook()
+        with patch('hooks.config_get', config_get):
+            result = hooks.install_hook()
 
         self.assertEqual(result, 'some result')
         apt_get_install.assert_has_calls([
@@ -547,6 +552,7 @@
             call('extra'),
         ])
 
+    @patch('hooks.open')
     @patch('hooks.config_get')
     @patch('os.path.exists')
     @patch('os.mkdir')
@@ -554,7 +560,8 @@
     @patch('hooks.log', MagicMock())
     @patch('hooks.apt_update')
     def test_doesnt_create_dir_to_install_hooks_if_not_needed(
-            self, apt_update, apt_get_install, mkdir, exists, config_get):
+            self, apt_update, apt_get_install, mkdir, exists, config_get,
+            open):
         exists.return_value = self.dir_exists
         config_get.return_value = None
         apt_get_install.return_value = 'some result'
 
=== modified file 'hooks/tests/test_config_changed.py'
--- hooks/tests/test_config_changed.py	2015-11-23 19:12:12 +0000
+++ hooks/tests/test_config_changed.py	2016-02-25 16:17:14 +0000
@@ -67,6 +67,7 @@
             "servername": "foobar",
             "vhost_http_template": "",
             "vhost_https_template": "",
+            "apt-source": "",
         })
         base = patch.object(hooks, 'default_apache_base_dir', self.dirname)
         config22 = patch.object(hooks, 'default_apache22_config_dir',