Re: [Kimchi-devel] [PATCHv5 1/2] Fix: Avoid passing unexpected config params to repository manager

On 09/25/2014 08:06 AM, lvroyce(a)linux.vnet.ibm.com wrote: > From: Royce Lv<lvroyce(a)linux.vnet.ibm.com> > > We allow config params for both yum and apt in API.json, > while we need to filter out parmas not proper for each kind. > So add this logic to repository model. > > Signed-off-by: Royce Lv<lvroyce(a)linux.vnet.ibm.com> > --- > src/kimchi/API.json | 2 +- > src/kimchi/i18n.py | 1 + > src/kimchi/repositories.py | 15 ++++++++++++++- > tests/test_model.py | 13 +++++++++++-- > 4 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/src/kimchi/API.json b/src/kimchi/API.json > index 5b752dc..d115c3f 100644 > --- a/src/kimchi/API.json > +++ b/src/kimchi/API.json > @@ -707,7 +707,7 @@ > "error": "KCHREPOS0007E" > }, > "gpgcheck": { > - "description": "Indicates if a GPG signature check on the packages gotten from repository should be performed.", > + "description": "Indicates if a GPG signature check on the packages got from repository should be performed.", I think the previous message is right.

> "type": "boolean", > "error": "KCHREPOS0009E" > }, > diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py > index 4466d23..17d94f9 100644 > --- a/src/kimchi/i18n.py > +++ b/src/kimchi/i18n.py > @@ -287,4 +287,5 @@ messages = { > "KCHREPOS0025E": _("Unable to retrieve repository information. Details: '%(err)s'"), > "KCHREPOS0026E": _("Unable to add repository. Details: '%(err)s'"), > "KCHREPOS0027E": _("Unable to remove repository. Details: '%(err)s'"), > + "KCHREPOS0028E": _("Configuration items: '%(items)s' are not supported by repository manager"), > } > diff --git a/src/kimchi/repositories.py b/src/kimchi/repositories.py > index 1d19fb7..b5a4ee1 100644 > --- a/src/kimchi/repositories.py > +++ b/src/kimchi/repositories.py > @@ -25,7 +25,7 @@ from ConfigParser import ConfigParser > > from kimchi.basemodel import Singleton > from kimchi.config import kimchiLock > -from kimchi.exception import InvalidOperation > +from kimchi.exception import InvalidOperation, InvalidParameter > from kimchi.exception import OperationFailed, NotFoundError, MissingParameter > from kimchi.utils import validate_repo_url > > @@ -104,6 +104,7 @@ class YumRepo(object): > """ > TYPE = 'yum' > DEFAULT_CONF_DIR = "/etc/yum.repos.d" > + CONFIG_ENTRY = ('repo_name', 'mirrorlist') > > def __init__(self): > self._yb = getattr(__import__('yum'), 'YumBase') > @@ -174,6 +175,11 @@ class YumRepo(object): > baseurl = params.get('baseurl', '') > config = params.get('config', {}) > + extra_keys = list( > + set(config.keys()).difference(set(self.CONFIG_ENTRY))) > + if len(extra_keys) > 0: > + raise InvalidParameter("KCHREPOS0028E", > + {'items': ",".join(extra_keys)}) To avoid duplicating code on YumRepo and AptRepo I suggest moving this verification to Repositories().addRepository - there you already know what type of repo you are dealing with. Something like: config = params.get('config', {}) extra_keys = set(config.keys()).difference(set(*self._pkg_mnger,CONFIG_ENTRY*))) if len(extra_keys) > 0: raise

> mirrorlist = config.get('mirrorlist', '') > if not baseurl and not mirrorlist: > raise MissingParameter("KCHREPOS0013E") > @@ -318,6 +324,7 @@ class AptRepo(object): > """ > TYPE = 'deb' > KIMCHI_LIST = "kimchi-source.list" > + CONFIG_ENTRY = ('dist', 'comps') > > def __init__(self): > getattr(__import__('apt_pkg'), 'init_config')() > @@ -413,6 +420,12 @@ class AptRepo(object): > # To create a APT repository the dist is a required parameter > # (in addition to baseurl, verified on controller through API.json) > config = params.get('config', None) > + extra_keys = list( > + set(config.keys()).difference(set(self.CONFIG_ENTRY))) > + if len(extra_keys) > 0: > + raise InvalidParameter("KCHREPOS0028E", > + {'items': ",".join(extra_keys)}) > + > if config is None: > raise MissingParameter("KCHREPOS0019E") > > diff --git a/tests/test_model.py b/tests/test_model.py > index 9c70c5d..fa0491d 100644 > --- a/tests/test_model.py > +++ b/tests/test_model.py > @@ -1310,8 +1310,7 @@ class ModelTests(unittest.TestCase): > 'baseurl': 'http://www.fedora.org'}, > {'repo_id': 'fedora-updates-fake', > 'config': > - {'mirrorlist': 'http://www.fedoraproject.org', > - 'gpgkey': 'file:///tmp/KEY-fedora-updates-fake-19'}}] > + {'mirrorlist': 'http://www.fedoraproject.org'}}] > > deb_repos = [{'baseurl': 'http://archive.ubuntu.com/ubuntu/', > 'config': {'dist': 'quantal'}}, > @@ -1326,12 +1325,22 @@ class ModelTests(unittest.TestCase): > wrong_mirrorlist = {'repo_id': 'wrong-id', > 'baseurl': 'www.example.com', > 'config': {'mirrorlist': url}} > + wrong_config_item = { > + 'repo_id': 'wrong-id', > + 'baseurl': 'www.example.com', > + 'config': { > + 'gpgkey': 'file:///tmp/KEY-fedora-updates-fake-19'}} > > yum_invalid_repos.append(wrong_baseurl) > yum_invalid_repos.append(wrong_mirrorlist) > + yum_invalid_repos.append(wrong_config_item) > > wrong_baseurl['config'] = {'dist': 'tasty'} > + wrong_config = {'baseurl': deb_repos[0]['baseurl'], > + 'config': { > + 'unsupported_item': "a_unsupported_item"}} > deb_invalid_repos.append(wrong_baseurl) > + deb_invalid_repos.append(wrong_config) > > repo_type = inst.capabilities_lookup()['repo_mngt_tool'] > if repo_type == 'yum':