Merge lp:~vbocek/ubuntu-system-image/support_http into lp:~registry/ubuntu-system-image/client

[Vojtech Bocek]

* Add support for "disabled" value in http_port and https_port config options, which disables that protocol.
* Support overriding of [system] section from channel.ini

- The tests are incomplete.

[Barry Warsaw (barry)]

Just a quick comment as I'm going through your branch.

=== modified file 'systemimage/config.py'
--- systemimage/config.py 2014-02-13 18:16:17 +0000
+++ systemimage/config.py 2014-02-25 22:24:24 +0000
> @@ -64,32 +78,39 @@
> if files_read != [path]:
> raise FileNotFoundError(path)
> self.config_file = path
> - self.service = Bag(converters=dict(http_port=int,
> - https_port=int,
> + self.service.update(converters=dict(http_port=port_value_converter,
> + https_port=port_value_converter,
> build_number=int),
> **parser['service'])
> + if (self.service.http_port is DISABLED_PROTOCOL and
> + self.service.https_port is DISABLED_PROTOCOL):
> + raise ValueError('both http and https ports were disabled')
> # Construct the HTTP and HTTPS base urls, which most applications will
> # actually use.
> if self.service.http_port == 80:
> self.service['http_base'] = 'http://{}'.format(self.service.base)
> - else:
> + elif self.service.http_port is not DISABLED_PROTOCOL:
> self.service['http_base'] = 'http://{}:{}'.format(
> self.service.base, self.service.http_port)
> if self.service.https_port == 443:
> self.service['https_base'] = 'https://{}'.format(self.service.base)
> - else:
> + elif self.service.http_port is not DISABLED_PROTOCOL:
> self.service['https_base'] = 'https://{}:{}'.format(
> self.service.base, self.service.https_port)
> - # Short-circuit, since we're loading a channel.ini file.
> - self._override = override
> - if override:
> - return
> - self.system = Bag(converters=dict(timeout=as_timedelta,
> + if self.service.http_port is DISABLED_PROTOCOL:
> + self.service['http_base'] = self.service['https_base']
> + elif self.service.https_port is DISABLED_PROTOCOL:
> + self.service['https_base'] = self.service['http_base']
> + self.system.update(converters=dict(timeout=as_timedelta,
> build_file=expand_path,
> loglevel=as_loglevel,
> settings_db=expand_path,
> tempdir=expand_path),
> **parser['system'])

The changes above are problematic because currently, Bags have the semantic
that their values cannot change after the initial setting. So updating as is
done here raises a ValueError. It's worse too because the test infrastructure
requires a Configuration object before any tests run (for unfortunate reasons
having to do with the D-Bus tests).

I'm not exactly sure of the best way to resolve this. I should probably just
remove this read-only restriction, since it's not really serving a useful
purpose.

[Barry Warsaw (barry)]

On Feb 26, 2014, at 12:03 AM, Barry Warsaw wrote:

>I'm not exactly sure of the best way to resolve this. I should probably just
>remove this read-only restriction, since it's not really serving a useful
>purpose.

nm, I know how to resolve this.

[Vojtech Bocek (vbocek)]

It didn't raise any error while I was testing it, the exception in __setitem__ affects only external changes via [] operator.

>>> from bag import Bag
>>> b=Bag(a=4, b=8)
>>> b.update(a=0, b=2)
>>> b['a']
0
>>> b['b']
2

[Barry Warsaw (barry)]

Thanks very much for this branch. I'm going to merge it to trunk after some
tweaks, and fleshing out of the test suite.

I ended up making these changes to your branch:

  * Renamed DISABLED_PROTOCOL to DISABLED
  * Some minor code style issues.
  * Allow for the [system] section to be missing from a channel.ini file.
  * Bag class API change: Setting an attribute via the setitem API no longer
    makes that item read-only. IOW, only keywords set in the constructor are
    read-only.
  * Flesh out tests.
  * Tweak the manpage.

Once I've merged this to trunk, you can take a look at the specific diff
there. Please let me know if I've missed anything or if you have any
questions or comments.

[Barry Warsaw (barry)]

Approved, with tweaks. See lp:~barry/ubuntu-system-image/lp1278589

review: Approve

=== modified file 'ini-manpage.rst'
--- ini-manpage.rst	2014-02-13 18:16:17 +0000
+++ ini-manpage.rst	2014-02-25 22:24:24 +0000
@@ -49,10 +49,12 @@
     provide both HTTP and HTTPS services.
 
 http_port
-    The port for HTTP connections.
+    The port for HTTP connections. This is an integer or string ``disabled``,
+    if you wish to disable all HTTP connections and use only HTTPS.
 
 https_port
-    The port for HTTPS connections.
+    The port for HTTPS connections. This is an integer or string ``disabled``,
+    if you wish to disable all HTTPS connections and use only HTTP.
 
 channel
     The upgrade channel.

=== modified file 'systemimage/bag.py'
--- systemimage/bag.py	2014-02-13 18:16:17 +0000
+++ systemimage/bag.py	2014-02-25 22:24:24 +0000
@@ -45,6 +45,14 @@
         self._converters = make_converter(converters)
         self.__original__ = {}
         self.__untranslated__ = {}
+        self._load_items(kws)
+
+    def update(self, *, converters=None, **kws):
+        if converters is not None:
+            self._converters.update(converters)
+        self._load_items(kws)
+
+    def _load_items(self, kws):
         for key, value in kws.items():
             self.__original__[key] = value
             safe_key, converted_value = self._normalize_key_value(key, value)

=== modified file 'systemimage/config.py'
--- systemimage/config.py	2014-02-13 18:16:17 +0000
+++ systemimage/config.py	2014-02-25 22:24:24 +0000
@@ -18,6 +18,7 @@
 __all__ = [
     'Configuration',
     'config',
+    'DISABLED_PROTOCOL'
     ]
 
 
@@ -31,14 +32,27 @@
     Bag, as_loglevel, as_object, as_timedelta, makedirs, temporary_directory)
 
 
+DISABLED_PROTOCOL = object()
+
+
 def expand_path(path):
     return os.path.abspath(os.path.expanduser(path))
 
+def port_value_converter(value):
+    if value == 'disabled' or value == 'disable':
+        return DISABLED_PROTOCOL
+    else:
+        res = int(value)
+        if res < 0:
+            raise ValueError
+        return res
 
 class Configuration:
     def __init__(self):
         # Defaults.
         self.config_file = None
+        self.service = Bag()
+        self.system = Bag()
         ini_path = resource_filename('systemimage.data', 'client.ini')
         self.load(ini_path)
         self._override = False
@@ -64,32 +78,39 @@
         if files_read != [path]:
             raise FileNotFoundError(path)
         self.config_file = path
-        self.service = Bag(converters=dict(http_port=int,
-                                           https_port=int,
+        self.service.update(converters=dict(http_port=port_value_converter,
+                                           https_port=port_value_converter,
                                            build_number=int),
                            **parser['service'])
+        if (self.service.http_port is DISABLED_PROTOCOL and
+            self.service.https_port is DISABLED_PROTOCOL):
+            raise ValueError('both http and https ports were disabled')
         # Construct the HTTP and HTTPS base urls, which most applications will
         # actually use.
         if self.service.http_port == 80:
             self.service['http_base'] = 'http://{}'.format(self.service.base)
-        else:
+        elif self.service.http_port is not DISABLED_PROTOCOL:
             self.service['http_base'] = 'http://{}:{}'.format(
                 self.service.base, self.service.http_port)
         if self.service.https_port == 443:
             self.service['https_base'] = 'https://{}'.format(self.service.base)
-        else:
+        elif self.service.http_port is not DISABLED_PROTOCOL:
             self.service['https_base'] = 'https://{}:{}'.format(
                 self.service.base, self.service.https_port)
-        # Short-circuit, since we're loading a channel.ini file.
-        self._override = override
-        if override:
-            return
-        self.system = Bag(converters=dict(timeout=as_timedelta,
+        if self.service.http_port is DISABLED_PROTOCOL:
+            self.service['http_base'] = self.service['https_base']
+        elif self.service.https_port is DISABLED_PROTOCOL:
+            self.service['https_base'] = self.service['http_base']
+        self.system.update(converters=dict(timeout=as_timedelta,
                                           build_file=expand_path,
                                           loglevel=as_loglevel,
                                           settings_db=expand_path,
                                           tempdir=expand_path),
                           **parser['system'])
+        # Short-circuit, since we're loading a channel.ini file.
+        self._override = override
+        if override:
+            return
         self.gpg = Bag(**parser['gpg'])
         self.updater = Bag(**parser['updater'])
         self.hooks = Bag(converters=dict(device=as_object,

=== added file 'systemimage/tests/data/config_05.ini'
--- systemimage/tests/data/config_05.ini	1970-01-01 00:00:00 +0000
+++ systemimage/tests/data/config_05.ini	2014-02-25 22:24:24 +0000
@@ -0,0 +1,36 @@
+# Configuration file for specifying relatively static information about the
+# upgrade resolution process.
+
+[service]
+base: phablet.example.com
+# Disabled port
+http_port: disabled
+https_port: 80443
+channel: stable
+build_number: 0
+
+[system]
+timeout: 10s
+build_file: /etc/ubuntu-build
+tempdir: /tmp
+logfile: /var/log/system-image/client.log
+loglevel: error
+settings_db: /var/lib/phablet/settings.db
+
+[gpg]
+archive_master: /etc/phablet/archive-master.tar.xz
+image_master: /etc/phablet/image-master.tar.xz
+image_signing: /var/lib/phablet/image-signing.tar.xz
+device_signing: /var/lib/phablet/device-signing.tar.xz
+
+[updater]
+cache_partition: /android/cache
+data_partition: /var/lib/phablet/updater
+
+[hooks]
+device: systemimage.device.SystemProperty
+scorer: systemimage.scores.WeightedScorer
+reboot: systemimage.reboot.Reboot
+
+[dbus]
+lifetime: 3s

=== added file 'systemimage/tests/data/config_06.ini'
--- systemimage/tests/data/config_06.ini	1970-01-01 00:00:00 +0000
+++ systemimage/tests/data/config_06.ini	2014-02-25 22:24:24 +0000
@@ -0,0 +1,36 @@
+# Configuration file for specifying relatively static information about the
+# upgrade resolution process.
+
+[service]
+base: phablet.example.com
+# Disabled port
+http_port: 80
+https_port: disabled
+channel: stable
+build_number: 0
+
+[system]
+timeout: 10s
+build_file: /etc/ubuntu-build
+tempdir: /tmp
+logfile: /var/log/system-image/client.log
+loglevel: error
+settings_db: /var/lib/phablet/settings.db
+
+[gpg]
+archive_master: /etc/phablet/archive-master.tar.xz
+image_master: /etc/phablet/image-master.tar.xz
+image_signing: /var/lib/phablet/image-signing.tar.xz
+device_signing: /var/lib/phablet/device-signing.tar.xz
+
+[updater]
+cache_partition: /android/cache
+data_partition: /var/lib/phablet/updater
+
+[hooks]
+device: systemimage.device.SystemProperty
+scorer: systemimage.scores.WeightedScorer
+reboot: systemimage.reboot.Reboot
+
+[dbus]
+lifetime: 3s

=== added file 'systemimage/tests/data/config_07.ini'
--- systemimage/tests/data/config_07.ini	1970-01-01 00:00:00 +0000
+++ systemimage/tests/data/config_07.ini	2014-02-25 22:24:24 +0000
@@ -0,0 +1,36 @@
+# Configuration file for specifying relatively static information about the
+# upgrade resolution process.
+
+[service]
+base: phablet.example.com
+# Both ports disabled
+http_port: disabled
+https_port: disabled
+channel: stable
+build_number: 0
+
+[system]
+timeout: 10s
+build_file: /etc/ubuntu-build
+tempdir: /tmp
+logfile: /var/log/system-image/client.log
+loglevel: error
+settings_db: /var/lib/phablet/settings.db
+
+[gpg]
+archive_master: /etc/phablet/archive-master.tar.xz
+image_master: /etc/phablet/image-master.tar.xz
+image_signing: /var/lib/phablet/image-signing.tar.xz
+device_signing: /var/lib/phablet/device-signing.tar.xz
+
+[updater]
+cache_partition: /android/cache
+data_partition: /var/lib/phablet/updater
+
+[hooks]
+device: systemimage.device.SystemProperty
+scorer: systemimage.scores.WeightedScorer
+reboot: systemimage.reboot.Reboot
+
+[dbus]
+lifetime: 3s

=== modified file 'systemimage/tests/test_config.py'
--- systemimage/tests/test_config.py	2014-02-13 18:16:17 +0000
+++ systemimage/tests/test_config.py	2014-02-25 22:24:24 +0000
@@ -136,6 +136,36 @@
         self.assertEqual(config.service.https_base,
                          'https://phablet.example.com:80443')
 
+    def test_disabled_http_port(self):
+        # config_05.ini has http port disabled and non-standard https port.
+        ini_file = data_path('config_05.ini')
+        config = Configuration()
+        config.load(ini_file)
+        self.assertEqual(config.service.base, 'phablet.example.com')
+        self.assertEqual(config.service.http_base,
+                         'https://phablet.example.com:80443')
+        self.assertEqual(config.service.https_base,
+                         'https://phablet.example.com:80443')
+
+    def test_disabled_https_port(self):
+        # config_06.ini has https port disabled and standard http port.
+        ini_file = data_path('config_06.ini')
+        config = Configuration()
+        config.load(ini_file)
+        self.assertEqual(config.service.base, 'phablet.example.com')
+        self.assertEqual(config.service.http_base,
+                         'http://phablet.example.com')
+        self.assertEqual(config.service.https_base,
+                         'http://phablet.example.com')
+
+    def test_both_ports_disabled(self):
+        # config_07.ini has both http and https ports disabled
+        ini_file = data_path('config_07.ini')
+        config = Configuration()
+        with self.assertRaises(ValueError) as ex:
+            config.load(ini_file)
+        self.assertEqual(str(ex.exception), 'both http and https ports were disabled')
+
     @configuration
     def test_get_build_number(self, ini_file):
         # The current build number is stored in a file specified in the