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

On 09/22/2014 06:32 AM, lvroyce(a)linux.vnet.ibm.com wrote: > From: Royce Lv <lvroyce(a)linux.vnet.ibm.com&gt; > > 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. > Also fix gpgkey handling for yum. > > Signed-off-by: Royce Lv <lvroyce(a)linux.vnet.ibm.com&gt; > --- > docs/API.md | 2 ++ > src/kimchi/API.json | 12 +++++++++++- > src/kimchi/i18n.py | 1 + > src/kimchi/repositories.py | 20 +++++++++++++++----- > tests/test_model.py | 22 ++++++++++++++++++---- > 5 files changed, 47 insertions(+), 10 deletions(-) > > diff --git a/docs/API.md b/docs/API.md > index b679ce7..a6a2bcc 100644 > --- a/docs/API.md > +++ b/docs/API.md > @@ -963,6 +963,8 @@ http://, ftp:// or file:// URL. > list of baseurls for YUM repository > * dist: Distribution to DEB repository > * comps *(optional)*: List of components to DEB repository > + * gpgcheck *(optional)*: Indicates if a GPG signature check on the > packages got from repository should be performed. > + * gpgkey *(optional)*: URL pointing to the ASCII-armored GPG key > file for the repository. > > ### Resource: Repository > > diff --git a/src/kimchi/API.json b/src/kimchi/API.json > index b8604d2..43d731e 100644 > --- a/src/kimchi/API.json > +++ b/src/kimchi/API.json > @@ -666,6 +666,16 @@ > "description": "URL to a file containing a list of baseurls", > "type": "string", > "error": "KCHREPOS0007E" > + }, > + "gpgcheck": { > + "description": "Indicates if a GPG signature check on the packages > got from repository should be performed.", > + "type": "boolean", > + "error": "KCHREPOS0009E" > + }, > + "gpgkey": { > + "description": "URL pointing to the ASCII-armored GPG key file for > the repository.", > + "type": "string", > + "error": "KCHREPOS0010E" > } > } > } > @@ -713,7 +723,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.", > "type": "boolean", > "error": "KCHREPOS0009E" > }, > diff --git a/src/kimchi/i18n.py b/src/kimchi/i18n.py > index 9e66c68..e83843c 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 676b7c2..3ac0e9c 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', 'gpgcheck', 'gpgkey') > > def __init__(self): > self._yb = getattr(__import__('yum'), 'YumBase') > @@ -172,8 +173,11 @@ class YumRepo(object): > """ > # At least one base url, or one mirror, must be given. > 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)}) > mirrorlist = config.get('mirrorlist', '') > if not baseurl and not mirrorlist: > raise MissingParameter("KCHREPOS0013E") > @@ -195,9 +199,11 @@ class YumRepo(object): > raise InvalidOperation("KCHREPOS0022E", {'repo_id': repo_id}) > > repo_name = config.get('repo_name', repo_id) > + gpgcheck = config.get('gpgcheck', 1) From jsonschema, gpgcheck is boolean, ie, true or false, and the yum repo config expects 1 or 0 > + gpgkey = config.get('gpgkey', list()) From jsonschema, gpgkey is a string and the yum repo config expects a list I know I have commented that gpgcheck and gpgkey were missing on docs/API.md but they are supposed to be like that as during the repo creation we assume the defaut values and they can be changed on PUT request

> repo = {'baseurl': baseurl, 'mirrorlist': mirrorlist, > - 'name': repo_name, 'gpgcheck': 1, > - 'gpgkey': [], 'enabled': 1} > + 'name': repo_name, 'gpgcheck': gpgcheck, > + 'gpgkey': gpgkey, 'enabled': 1} > > # write a repo file in the system with repo{} information. > parser = ConfigParser() > @@ -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,12 +420,15 @@ 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") > > if 'dist' not in config.keys(): > raise MissingParameter("KCHREPOS0019E") > - > uri = params['baseurl'] > dist = config['dist'] > comps = config.get('comps', []) > diff --git a/tests/test_model.py b/tests/test_model.py > index ceedc6f..56a8e12 100644 > --- a/tests/test_model.py > +++ b/tests/test_model.py > @@ -1295,11 +1295,21 @@ class ModelTests(unittest.TestCase): > {'baseurl': 'http://br.archive.kimchi.com/ubuntu/fake', > 'config': {'dist': 'quantal', 'comps': ['main']}}] > > + invalid_deb_repo = {'baseurl': 'http://br.archive.ubuntu.com/ubuntu/', > + 'config': {'gpgkey': 'file:///fake-key-file'}} > + > + invalid_yum_repo = { > + 'repo_id': 'fedora-fake2', > + 'baseurl': 'http://www.fedora.org', > + 'config': {'dist': 'quantal', 'comps': ['main']}} > + > repo_type = inst.capabilities_lookup()['repo_mngt_tool'] > if repo_type == 'yum': > test_repos = yum_repos > + invalid_repo = invalid_deb_repo > elif repo_type == 'deb': > test_repos = deb_repos > + invalid_repo = invalid_yum_repo > else: > # repository management tool was not recognized by Kimchi > # skip test case > @@ -1317,11 +1327,15 @@ class ModelTests(unittest.TestCase): > 'config': {'dist': 'quantal'}} > self.assertRaises(InvalidParameter, inst.repositories_create, repo) > > + # Create repo with invalid config > + self.assertRaises(InvalidParameter, inst.repositories_create, repo) > + > # create repositories with invalid mirrorlist > - for url in invalid_urls: > - repo = {'repo_id': 'repo_fake', > - 'config': {'mirrorlist': url, 'dist': 'quantal'}} > - self.assertRaises(InvalidParameter, inst.repositories_create, repo) > + if repo_type == 'yum': > + for url in invalid_urls: > + repo = {'repo_id': 'repo_fake', > + 'config': {'mirrorlist': url, 'dist': 'quantal'}} > + self.assertRaises(InvalidParameter, inst.repositories_create, repo) You are restricting the test for yum repos. You should create the repo params according to repo type to test deb and yum for url in invalid_urls: if repo_type == 'yum': repo = {} elif repo_type == 'deb': repo = {} self.assertRaises(...) > for repo in test_repos: > system_host_repos = len(inst.repositories_get_list())