Merge lp:~raharper/curtin/trunk.apt-config-validate into lp:~curtin-dev/curtin/trunk

[Ryan Harper]

curtin.apt-config: Add some type checking for common input mistakes

The curtin apt-config format requires some specific types. Assert those
types while processing and raise exceptions if found.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:419
https://jenkins.ubuntu.com/server/job/curtin-ci/242/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=amd64/242
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=ppc64el/242

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/242/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

some comments, generally agree with the idea.

[Ryan Harper (raharper)]

On Thu, Aug 18, 2016 at 9:18 AM, Scott Moser <email address hidden> wrote:

> some comments, generally agree with the idea.
>
> Diff comments:
>
> > === modified file 'curtin/commands/apt_config.py'
> > --- curtin/commands/apt_config.py 2016-08-04 16:53:56 +0000
> > +++ curtin/commands/apt_config.py 2016-08-18 13:48:54 +0000
> > @@ -457,14 +459,21 @@
> > """out of a list of potential mirror configurations select
> > and return the one matching the architecture (or default)"""
> > # select the mirror specification (if-any)
> > - mirror_cfg_list = cfg.get(mirrortype, None)
> > - if mirror_cfg_list is None:
> > - return None
> > + mirror_cfg_list = cfg.get(mirrortype, [])
> > + if not isinstance(mirror_cfg_list, list):
> > + raise ValueError(
> > + ('apt[\'%s\'] mirror value must be a list' % mirrortype))
> >
> > # select the specification matching the target arch
> > default = None
> > for mirror_cfg_elem in mirror_cfg_list:
> > - arches = mirror_cfg_elem.get("arches")
> > + if not isinstance(mirror_cfg_elem, dict):
> > + LOG.debug('Skipping invalid mirror element: %s',
> mirror_cfg_elem)
>
> why do we raise ValueError if arches is not a list below, but just log
> debug here? Ie, why is one invalid thing a debug log and the other a
> valueerror.
>

it should raise as well.

>
> > + continue
> > + arches = mirror_cfg_elem.get("arches", {})
> > + if not isinstance(arches, list):
> > + raise ValueError(
> > + ("apt mirror 'arches' value must be a list: [%s]" %
> arches))
> > if arch in arches:
> > return mirror_cfg_elem
> > if "default" in arches:
> > @@ -488,6 +497,10 @@
> > # list of mirrors to try to resolve
> > mirror = search_for_mirror(mcfg.get("search", None))
> >
> > + if not isinstance(mirror, str):
> > + raise ValueError("apt mirror 'uri' value must be a string:
> [%s]" %
>
> this i think fits on one line.
>

OK

>
> > + mirror)
> > +
> > return mirror
> >
> >
> > @@ -564,8 +582,8 @@
> > apt_cfg = cfg.get("apt")
> > # if no apt config section is available, do nothing
> > if apt_cfg is not None:
> > - LOG.debug("Handling apt to target %s with config %s",
> > - target, apt_cfg)
> > + LOG.info("Configuring apt in target %s with config %s",
>
> you can probably join this line break and be < 80 chars.
>

OK

>
> > + target, apt_cfg)
> > try:
> > with util.ChrootableTarget(target, sys_resolvconf=True):
> > handle_apt(apt_cfg, target)
> >
> > === modified file 'tests/unittests/test_apt_source.py'
> > --- tests/unittests/test_apt_source.py 2016-08-05 23:32:42 +0000
> > +++ tests/unittests/test_apt_source.py 2016-08-18 13:48:54 +0000
> > @@ -569,6 +569,29 @@
> > self.assertEqual(mirrors['SECURITY'],
> > smir)
> >
> > + def test_mirror_not_list(self):
> > + """test_mirror_not_list - Test for exception on invalid mirror
> cfg"""
> > + pmir = "htt...

[Scott Moser (smoser)]

one other comment

[Ryan Harper (raharper)]

Currently when you run curtin apt-config, you see *nothing*. I don't think
that's right.

w.r.t the output format, I'm happy to change the message (and demote the
dict dump to debug).

On Thu, Aug 18, 2016 at 10:00 AM, Scott Moser <email address hidden> wrote:

> one other comment
>
> Diff comments:
>
> > === modified file 'curtin/commands/apt_config.py'
> > --- curtin/commands/apt_config.py 2016-08-04 16:53:56 +0000
> > +++ curtin/commands/apt_config.py 2016-08-18 13:48:54 +0000
> > @@ -564,8 +582,8 @@
> > apt_cfg = cfg.get("apt")
> > # if no apt config section is available, do nothing
> > if apt_cfg is not None:
> > - LOG.debug("Handling apt to target %s with config %s",
> > - target, apt_cfg)
> > + LOG.info("Configuring apt in target %s with config %s",
>
> other thing... LOG.info seems loud to me for something that is going to
> dump a string representation of a large dictionary.
> we dont have really good policy in place at all anywhere, but "info" to me
> seems like something that is reasonably shown to a user, and string
> formated dictionaries dumped to users dont' seem reasonable to me.
>
> this is why i almost always use DEBUG.
>
> > + target, apt_cfg)
> > try:
> > with util.ChrootableTarget(target, sys_resolvconf=True):
> > handle_apt(apt_cfg, target)
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.apt-
> config-validate/+merge/303267
> You are the owner of lp:~raharper/curtin/trunk.apt-config-validate.
>

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:419
https://jenkins.ubuntu.com/server/job/curtin-ci/398/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/398
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/398
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/398
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/398
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/398

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/398/rebuild

review: Approve (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:419
https://jenkins.ubuntu.com/server/job/curtin-ci/415/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/415
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/415
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/415
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/415
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=vm-i386/415

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/415/rebuild

review: Approve (continuous-integration)

Unmerged revisions

419. By Ryan Harper on 2016-08-15

fix formatting

418. By Ryan Harper on 2016-08-15

curtin.apt_config: configure log for subcommand usage

417. By Ryan Harper on 2016-08-15

curtin.apt-config: Add some type checking for common input mistakes

The curtin apt-config format requires some specific types. Assert those
types while processing and raise exceptions if found.

=== modified file 'curtin/commands/apt_config.py'
--- curtin/commands/apt_config.py	2016-08-04 16:53:56 +0000
+++ curtin/commands/apt_config.py	2016-08-18 13:48:54 +0000
@@ -26,11 +26,13 @@
 import sys
 import yaml
 
-from curtin.log import LOG
+from .. import log
 from curtin import (config, util, gpg)
 
 from . import populate_one_subcmd
 
+LOG = log.LOG
+
 # this will match 'XXX:YYY' (ie, 'cloud-archive:foo' or 'ppa:bar')
 ADD_APT_REPO_MATCH = r"^[\w-]+:\w"
 
@@ -457,14 +459,21 @@
     """out of a list of potential mirror configurations select
        and return the one matching the architecture (or default)"""
     # select the mirror specification (if-any)
-    mirror_cfg_list = cfg.get(mirrortype, None)
-    if mirror_cfg_list is None:
-        return None
+    mirror_cfg_list = cfg.get(mirrortype, [])
+    if not isinstance(mirror_cfg_list, list):
+        raise ValueError(
+            ('apt[\'%s\'] mirror value must be a list' % mirrortype))
 
     # select the specification matching the target arch
     default = None
     for mirror_cfg_elem in mirror_cfg_list:
-        arches = mirror_cfg_elem.get("arches")
+        if not isinstance(mirror_cfg_elem, dict):
+            LOG.debug('Skipping invalid mirror element: %s', mirror_cfg_elem)
+            continue
+        arches = mirror_cfg_elem.get("arches", {})
+        if not isinstance(arches, list):
+            raise ValueError(
+                ("apt mirror 'arches' value must be a list: [%s]" % arches))
         if arch in arches:
             return mirror_cfg_elem
         if "default" in arches:
@@ -488,6 +497,10 @@
         # list of mirrors to try to resolve
         mirror = search_for_mirror(mcfg.get("search", None))
 
+    if not isinstance(mirror, str):
+        raise ValueError("apt mirror 'uri' value must be a string: [%s]" %
+                         mirror)
+
     return mirror
 
 
@@ -550,6 +563,11 @@
         """
     cfg = config.load_command_config(args, {})
 
+    log.basicConfig(stream=args.log_file, verbosity=1)
+
+    if not isinstance(cfg, dict):
+        raise TypeError("apt config is not a dict.")
+
     if args.target is not None:
         target = args.target
     else:
@@ -564,8 +582,8 @@
     apt_cfg = cfg.get("apt")
     # if no apt config section is available, do nothing
     if apt_cfg is not None:
-        LOG.debug("Handling apt to target %s with config %s",
-                  target, apt_cfg)
+        LOG.info("Configuring apt in target %s with config %s",
+                 target, apt_cfg)
         try:
             with util.ChrootableTarget(target, sys_resolvconf=True):
                 handle_apt(apt_cfg, target)

=== modified file 'tests/unittests/test_apt_source.py'
--- tests/unittests/test_apt_source.py	2016-08-05 23:32:42 +0000
+++ tests/unittests/test_apt_source.py	2016-08-18 13:48:54 +0000
@@ -569,6 +569,29 @@
         self.assertEqual(mirrors['SECURITY'],
                          smir)
 
+    def test_mirror_not_list(self):
+        """test_mirror_not_list - Test for exception on invalid mirror cfg"""
+        pmir = "http://us.archive.ubuntu.com/ubuntu/"
+        smir = "http://security.ubuntu.com/ubuntu/"
+        cfg_not_list = {"primary": {"arches": ["default"],
+                                    "uri": pmir},
+                        "security": {"arches": ["default"],
+                                     "uri": smir}}
+
+        with self.assertRaises(ValueError):
+            apt_config.find_apt_mirror_info(cfg_not_list, 'amd64')
+
+    def test_mirror_not_list_of_dicts(self):
+        """test_mirror_arches_type - Test for exception on invld mirror cfg"""
+        pmir = "http://us.archive.ubuntu.com/ubuntu/"
+        smir = "http://security.ubuntu.com/ubuntu/"
+        cfg_not_list = {"primary": [{"arches": "default",
+                                    "uri": pmir}],
+                        "security": {"arches": "default",
+                                     "uri": smir}}
+        with self.assertRaises(ValueError):
+            apt_config.find_apt_mirror_info(cfg_not_list, 'amd64')
+
     def test_mirror_default(self):
         """test_mirror_default - Test without defining a mirror"""
         arch = util.get_architecture()
@@ -584,6 +607,12 @@
         self.assertEqual(mirrors['SECURITY'],
                          smir)
 
+    def test_mirror_uri_not_none_not_string(self):
+        cfg = {"primary": [{"arches": "default", "uri": {'foobar': 1}}],
+               "security": {"arches": "default", "uri": ["None"]}}
+        with self.assertRaises(ValueError):
+            apt_config.get_mirror(cfg, "primary", 'amd64')
+
     def test_mirror_arches(self):
         """test_mirror_arches - Test arches selection of mirror"""
         pmir = "http://my-primary.ubuntu.com/ubuntu/"