Differential D811

Add dist-git style tasks namespace check
ClosedPublic
Actions

Authored by mkrizek on Apr 13 2016, 1:23 PM.

Details

Reviewers
kparal
tflink
Maniphest Tasks
T751: Add additional dist-git style tasks namespace check to libtaskotron
Commits
rLTRNda14c4bba6ea: Add dist-git style tasks namespace check
Test Plan

Plenty of unittests added. Works for me.

Diff Detail

Repository
rLTRN libtaskotron
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mkrizek retitled this revision from to Add dist-git style tasks namespace check.Apr 13 2016, 1:23 PM
mkrizek updated this object.
mkrizek edited the test plan for this revision. (Show Details)
mkrizek added reviewers: tflink, kparal.
kparal requested changes to this revision.Apr 14 2016, 11:50 AM
kparal added inline comments.
libtaskotron/directives/resultsdb_directive.py
32–35

This should be amended to cover additional cases when we raise this.

258–261

The process() method is getting really long, could this be separated into a standalone method?

263

Please add a debug printout before breaking.

281

Please add a debug printout before breaking.

290

This should be pkg.%s. (a dot in the end), right?

293

Can you add explanation where is the KeyError coming from?

294–296

I don't like this very much. We're claiming here it is about namespace permissions, even when the original message is "Could not find task's git" (a completely different problem), or when the task tries to use a namespace we don't know about (also a different problem). Could we throw those errors right away, with specific error messages?

testing/test_resultsdb_directive.py
339

Why is this needed, isn't self.ref_arg_data regenerated before each test method start?

This revision now requires changes to proceed.Apr 14 2016, 11:50 AM
mkrizek updated this revision to Diff 2077.Apr 15 2016, 7:24 AM

Namespace existance check and refactoring

kparal accepted this revision.Apr 15 2016, 10:31 AM

Thanks, looks good.

testing/test_resultsdb_directive.py
339

I still think you can save this one line from all your tests.

This revision is now accepted and ready to land.Apr 15 2016, 10:31 AM
Closed by commit rLTRNda14c4bba6ea: Add dist-git style tasks namespace check (authored by mkrizek). · Explain WhyApr 15 2016, 12:21 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

Diff 2078

View Options

conf/namespaces.yaml.example

1# A list of namespaces that any task is allowed to post a result into 1​# A list of namespaces that any task is allowed to post a result into
2namespaces_safe: 2​namespaces_safe:
3 - scratch 3​ - scratch
4 4​
5# A list of git repos that are allowed to post a result into a particular namespace 5​# A list of git repos that are allowed to post a result into a particular namespace
6namespaces_whitelist: 6​namespaces_whitelist:
7 qa: 7​ qa:
8 - git@bitbucket.org:fedoraqa/task-rpmlint.git 8​ - git@bitbucket.org:fedoraqa/task-rpmlint.git
9 - git@bitbucket.org:fedoraqa/task-depcheck.git 9​ - git@bitbucket.org:fedoraqa/task-depcheck.git
10 - git@bitbucket.org:fedoraqa/task-upgradepath.git 10​ - git@bitbucket.org:fedoraqa/task-upgradepath.git
11 pkg: 11​ pkg:
12 - git://pkgs.fedoraproject.org/tasks/ 12​ - git://pkgs.fedoraproject.org/rpms-checks/
View Options

libtaskotron/directives/resultsdb_directive.py

Show All 23 Lines22 results:
24 description: The output of the check in the ResultYAML format 24​ description: The output of the check in the ResultYAML format
25 type: str 25​ type: str
26returns: | 26​returns: |
27 YAML formatted output as taken from input, enhanced with result-id's for each 27​ YAML formatted output as taken from input, enhanced with result-id's for each
28 successfully stored result. The result-id information then can be used by 28​ successfully stored result. The result-id information then can be used by
29 other directives. If reporting to ResultsDB is disabled, the returned YAML 29​ other directives. If reporting to ResultsDB is disabled, the returned YAML
30 does not contain this extra field. 30​ does not contain this extra field.
31raises: | 31​raises: |
32 * :class:`.TaskotronDirectiveError`: if ``results`` is not in a valid YAML 32​ * :class:`.TaskotronDirectiveError`: if ``results`` is not in a valid YAML
33 format, or when ``item`` or ``type`` are missing in the YAML's result data 33​ format, or when ``item`` or ``type`` are missing in the YAML's result data,
34​ or when results are not allowed to be posted into a given namespace, or
35​ the namespace does not exist.
kparalUnsubmitted
Not Done

This should be amended to cover additional cases when we raise this.

34 * :class:`resultsdb_api.ResultsDBapiException`: for any errors coming from 36​ * :class:`resultsdb_api.ResultsDBapiException`: for any errors coming from
35 the ResultsDB server 37​ the ResultsDB server
36version_added: 0.4 38​version_added: 0.4
37""" 39​"""
38 40​
39EXAMPLES = """ 41​EXAMPLES = """
40These two actions first run ``run_rpmlint.py`` and export its YAML output to 42​These two actions first run ``run_rpmlint.py`` and export its YAML output to
41``${rpmlint_output}``, and then feed this YAML output to ``resultsdb`` 43​``${rpmlint_output}``, and then feed this YAML output to ``resultsdb``
Show All 37 Lines
79 81​
80from libtaskotron.directives import BaseDirective 82​from libtaskotron.directives import BaseDirective
81 83​
82from libtaskotron import check 84​from libtaskotron import check
83from libtaskotron import config 85​from libtaskotron import config
84from libtaskotron import buildbot_utils 86​from libtaskotron import buildbot_utils
85from libtaskotron.exceptions import TaskotronDirectiveError, TaskotronValueError 87​from libtaskotron.exceptions import TaskotronDirectiveError, TaskotronValueError
86from libtaskotron.logger import log 88​from libtaskotron.logger import log
89​from libtaskotron.ext.fedora import rpm_utils
87 90​
88import resultsdb_api 91​import resultsdb_api
89 92​
90directive_class = 'ResultsdbDirective' 93​directive_class = 'ResultsdbDirective'
91 94​
92class ResultsdbDirective(BaseDirective): 95​class ResultsdbDirective(BaseDirective):
93 96​
94 def __init__(self, resultsdb = None): 97​ def __init__(self, resultsdb = None):
▲ Show 20 Lines • Show All 81 Lines • ▼ Show 20 Line(s)177 if not os.path.exists(artifact_path):
176 return None 179​ return None
177 elif not artifact_path.startswith(artifactsdir): 180​ elif not artifact_path.startswith(artifactsdir):
178 log.warn('Artifact %r is placed outside of artifacts directory %r, ignoring', 181​ log.warn('Artifact %r is placed outside of artifacts directory %r, ignoring',
179 artifact_path, artifactsdir) 182​ artifact_path, artifactsdir)
180 return None 183​ return None
181 184​
182 return os.path.relpath(artifact_path, start=artifactsdir) 185​ return os.path.relpath(artifact_path, start=artifactsdir)
183 186​
187​ def check_namespace(self, checkname, arg_data):
188​ conf_ns = config.load_namespaces_config()
189​ repos = None
190​
191​ # check if the namespace exists
192​ for ns in conf_ns['namespaces_safe']:
193​ if checkname.startswith(ns+'.'):
194​ log.debug('Namespace %s found in the safe namespaces.', ns)
195​ return
196​ else:
197​ for ns, rs in conf_ns['namespaces_whitelist'].iteritems():
198​ if checkname.startswith(ns+'.'):
199​ log.debug('Namespace %s found in the namespace whitelist.', ns)
200​ repos = rs
201​ break
202​ else:
203​ raise TaskotronDirectiveError('Namespace %s does not exist.')
204​
205​ try:
206​ taskdir = os.path.dirname(os.path.abspath(arg_data['task']))
207​ gitconfig_filename = os.path.join(taskdir, '.git/config')
208​
209​ gitconfig = configparser.ConfigParser()
210​ gitconfig.read(gitconfig_filename)
211​ repo_url = gitconfig['remote "origin"']['url']
212​ except TypeError, e:
213​ log.exception(e)
214​ raise TaskotronDirectiveError("Could not find task's git remote 'origin' url"
215​ "in %s" % gitconfig_filename)
216​
217​ try:
218​ if not [repo for repo in repos if repo.startswith(repo_url.strip())]:
219​ raise TaskotronDirectiveError
220​
221​ log.debug('Repo %s found in the whitelist', repo_url)
222​ # dist-git style tasks
223​ # here we need to not only check the root namespace but also second level ns,
224​ # e.g. a firefox check can post results into pkg.firefox
225​ if checkname.startswith('pkg.'):
226​ pkg_name = rpm_utils.rpmformat(arg_data['item'], fmt='n')
227​ pkg_ns = 'pkg.%s.' % pkg_name
228​ if not checkname.startswith(pkg_ns):
229​ raise TaskotronDirectiveError
230​ except TaskotronDirectiveError:
231​ raise TaskotronDirectiveError("%s is not allowed to post results into %s "
232​ "namespace. Not posting results." %
233​ (arg_data['checkname'], arg_data['namespace']))
234​
184 def process(self, params, arg_data): 235​ def process(self, params, arg_data):
185 # checking if reporting is enabled is done after importing yaml which 236​ # checking if reporting is enabled is done after importing yaml which
186 # serves as validation of input results 237​ # serves as validation of input results
187 try: 238​ try:
188 check_details = check.import_YAML(params['results']) 239​ check_details = check.import_YAML(params['results'])
189 log.debug("YAML output parsed OK.") 240​ log.debug("YAML output parsed OK.")
190 except TaskotronValueError as e: 241​ except TaskotronValueError as e:
191 raise TaskotronDirectiveError("Failed to load 'results': %s" % e.message) 242​ raise TaskotronDirectiveError("Failed to load 'results': %s" % e.message)
192 243​
193 for detail in check_details: 244​ for detail in check_details:
194 if not (detail.item and detail.report_type): 245​ if not (detail.item and detail.report_type):
195 raise TaskotronDirectiveError( 246​ raise TaskotronDirectiveError(
196 "The resultsdb directive requires " 247​ "The resultsdb directive requires "
197 "'item' and 'type' to be present in the YAML data.") 248​ "'item' and 'type' to be present in the YAML data.")
198 249​
199 conf = config.get_config() 250​ conf = config.get_config()
200 if not conf.report_to_resultsdb: 251​ if not conf.report_to_resultsdb:
201 log.info("Reporting to ResultsDB is disabled. Once enabled, the " 252​ log.info("Reporting to ResultsDB is disabled. Once enabled, the "
202 "following would get reported:\n%s" % params['results']) 253​ "following would get reported:\n%s" % params['results'])
203 return check.export_YAML(check_details) 254​ return check.export_YAML(check_details)
204 255​
205 # find out if the task is allowed to post results into the namespace 256​ checkname = '%s.%s' % (arg_data['namespace'], arg_data['checkname'])
206 conf_ns = config.load_namespaces_config() ​
207 if (conf.profile == config.ProfileName.PRODUCTION and ​
208 arg_data['namespace'] not in conf_ns['namespaces_safe']): ​
209 ​
210 try: ​
211 taskdir = os.path.dirname(os.path.abspath(arg_data['task'])) ​
212 gitconfig_filename = os.path.join(taskdir, '.git/config') ​
213 ​
214 gitconfig = configparser.ConfigParser() ​
215 gitconfig.read(gitconfig_filename) ​
216 repo_url = gitconfig['remote "origin"']['url'] ​
217 except TypeError, e: ​
218 log.exception(e) ​
219 raise TaskotronDirectiveError("Could not find task's git remote 'origin' url in %s" ​
220 % gitconfig_filename) ​
221 257​
222 try: 258​ # find out if the task is allowed to post results into the namespace
223 ns_whitelist = conf_ns['namespaces_whitelist'][arg_data['namespace']] 259​ if config.get_config().profile == config.ProfileName.PRODUCTION:
224 if not [ns for ns in ns_whitelist if ns.startswith(repo_url.strip())]: 260​ self.check_namespace(checkname, arg_data)
225 raise TaskotronDirectiveError ​
226 except (KeyError, TaskotronDirectiveError): ​
227 raise TaskotronDirectiveError("%s is not allowed to post results into %s " ​
228 "namespace. Not posting results." % ​
229 (arg_data['checkname'], arg_data['namespace'])) ​
230 261​
kparalUnsubmitted
Not Done

The process() method is getting really long, could this be separated into a standalone method?

231 # for now, we're creating the resultsdb job at reporting time 262​ # for now, we're creating the resultsdb job at reporting time
232 # the job needs to be 'RUNNING' in order to append any results 263​ # the job needs to be 'RUNNING' in order to append any results
kparalUnsubmitted
Not Done

Please add a debug printout before breaking.

233 job_url, log_url = buildbot_utils.get_urls(arg_data['jobid'], 264​ job_url, log_url = buildbot_utils.get_urls(arg_data['jobid'],
234 self.masterurl, 265​ self.masterurl,
235 self.task_stepname) 266​ self.task_stepname)
236 267​
237 checkname = '%s.%s' % (arg_data['namespace'], arg_data['checkname']) ​
238 job_data = self.create_resultsdb_job(checkname, uuid=arg_data['uuid']) 268​ job_data = self.create_resultsdb_job(checkname, uuid=arg_data['uuid'])
239 269​
240 log.info('Posting %s results to ResultsDB...' % len(check_details)) 270​ log.info('Posting %s results to ResultsDB...' % len(check_details))
241 for detail in check_details: 271​ for detail in check_details:
242 checkname = '%s.%s' % (arg_data['namespace'], 272​ checkname = '%s.%s' % (arg_data['namespace'],
243 detail.checkname or arg_data['checkname']) 273​ detail.checkname or arg_data['checkname'])
244 274​
245 self.ensure_testcase_exists(checkname) 275​ self.ensure_testcase_exists(checkname)
246 result_log_url = log_url 276​ result_log_url = log_url
247 if detail.artifact: 277​ if detail.artifact:
248 artifact_path = self.get_artifact_path( 278​ artifact_path = self.get_artifact_path(
249 arg_data['artifactsdir'], 279​ arg_data['artifactsdir'],
250 detail.artifact 280​ detail.artifact
251 ) 281​ )
kparalUnsubmitted
Not Done

Please add a debug printout before breaking.

252 if artifact_path: 282​ if artifact_path:
253 result_log_url = "%s/all/%s/task_output/%s" % ( 283​ result_log_url = "%s/all/%s/task_output/%s" % (
254 self.artifacts_baseurl, 284​ self.artifacts_baseurl,
255 arg_data['uuid'], 285​ arg_data['uuid'],
256 artifact_path, 286​ artifact_path,
257 ) 287​ )
258 try: 288​ try:
259 result = self.resultsdb.create_result( 289​ result = self.resultsdb.create_result(
260 job_id=job_data['id'], 290​ job_id=job_data['id'],
kparalUnsubmitted
Not Done

This should be pkg.%s. (a dot in the end), right?

261 testcase_name=checkname, 291​ testcase_name=checkname,
262 outcome=detail.outcome, 292​ outcome=detail.outcome,
263 summary=detail.note or None, 293​ summary=detail.note or None,
kparalUnsubmitted
Not Done

Can you add explanation where is the KeyError coming from?

264 log_url=result_log_url, 294​ log_url=result_log_url,
265 item=detail.item, 295​ item=detail.item,
266 type=detail.report_type, 296​ type=detail.report_type,
kparalUnsubmitted
Not Done

I don't like this very much. We're claiming here it is about namespace permissions, even when the original message is "Could not find task's git" (a completely different problem), or when the task tries to use a namespace we don't know about (also a different problem). Could we throw those errors right away, with specific error messages?

267 **detail.keyvals) 297​ **detail.keyvals)
268 detail._internal['resultsdb_result_id'] = result['id'] 298​ detail._internal['resultsdb_result_id'] = result['id']
269 299​
270 except resultsdb_api.ResultsDBapiException, e: 300​ except resultsdb_api.ResultsDBapiException, e:
271 log.error(e) 301​ log.error(e)
272 log.error("Failed to store ResulsDB: `%s` `%s` `%s`", 302​ log.error("Failed to store ResulsDB: `%s` `%s` `%s`",
273 detail.item, checkname, detail.outcome) 303​ detail.item, checkname, detail.outcome)
274 304​
275 # move job to completed 305​ # move job to completed
276 self.complete_resultsdb_job(job_data['id']) 306​ self.complete_resultsdb_job(job_data['id'])
277 307​
278 return check.export_YAML(check_details) 308​ return check.export_YAML(check_details)
View Options

testing/test_resultsdb_directive.py

Show First 20 Lines • Show All 48 Lines • ▼ Show 20 Line(s)35 def setup(self, monkeypatch):
49 self.ref_input = {'results': self.yaml} 49​ self.ref_input = {'results': self.yaml}
50 self.ref_arg_data = { 50​ self.ref_arg_data = {
51 'resultsdb_job_id': 1, 51​ 'resultsdb_job_id': 1,
52 'checkname': 'test_resultsdb_report', 52​ 'checkname': 'test_resultsdb_report',
53 'namespace': 'qa', 53​ 'namespace': 'qa',
54 'jobid': 'all/123', 54​ 'jobid': 'all/123',
55 'uuid': 'c25237a4-b6b3-11e4-b98a-3c970e018701', 55​ 'uuid': 'c25237a4-b6b3-11e4-b98a-3c970e018701',
56 'artifactsdir': '/some/directory/', 56​ 'artifactsdir': '/some/directory/',
57 'task': '/taskdir' 57​ 'task': '/taskdir',
58​ 'item': 'firefox-45.0.2-1.fc23'
58 } 59​ }
59 60​
60 self.ref_resultdata = {u'id': 1234} 61​ self.ref_resultdata = {u'id': 1234}
61 62​
62 self.ref_jobid = 1234 63​ self.ref_jobid = 1234
63 self.ref_refurl = u'http://example.com/%s' % self.ref_arg_data['checkname'] 64​ self.ref_refurl = u'http://example.com/%s' % self.ref_arg_data['checkname']
64 self.ref_jobdata = {u'end_time': None, 65​ self.ref_jobdata = {u'end_time': None,
65 u'href': u'http://127.0.0.1/api/v1.0/jobs/%d' % self.ref_jobid, 66​ u'href': u'http://127.0.0.1/api/v1.0/jobs/%d' % self.ref_jobid,
▲ Show 20 Lines • Show All 260 Lines • ▼ Show 20 Line(s)
326 327​
327 def test_safe_namespace(self, monkeypatch): 328​ def test_safe_namespace(self, monkeypatch):
328 ref_ns = {'namespaces_safe': ['qa']} 329​ ref_ns = {'namespaces_safe': ['qa']}
329 monkeypatch.setattr(config, 'load_namespaces_config', lambda: ref_ns) 330​ monkeypatch.setattr(config, 'load_namespaces_config', lambda: ref_ns)
330 self.conf.profile = config_defaults.ProfileName.PRODUCTION 331​ self.conf.profile = config_defaults.ProfileName.PRODUCTION
331 332​
332 self.test_rdb.process(self.ref_input, self.ref_arg_data) 333​ self.test_rdb.process(self.ref_input, self.ref_arg_data)
333 # did not raise, pass 334​ # did not raise, pass
335​
336​ def test_safe_namespace_multilevel(self, monkeypatch):
337​ ref_ns = {'namespaces_safe': ['qa']}
338​ monkeypatch.setattr(config, 'load_namespaces_config', lambda: ref_ns)
339​ self.ref_arg_data['namespace'] = 'qa.firefox'
kparalUnsubmitted
Not Done

Why is this needed, isn't self.ref_arg_data regenerated before each test method start?

kparalUnsubmitted
Not Done

I still think you can save this one line from all your tests.

340​ self.conf.profile = config_defaults.ProfileName.PRODUCTION
341​
342​ self.test_rdb.process(self.ref_input, self.ref_arg_data)
343​ # did not raise, pass
344​
345​ def test_allowed_namespace_pkg(self, monkeypatch):
346​ ref_ns = {'namespaces_safe': [], 'namespaces_whitelist': {'pkg': ['giturl']}}
347​ monkeypatch.setattr(config, 'load_namespaces_config', lambda: ref_ns)
348​ self.ref_arg_data['namespace'] = 'pkg.firefox'
349​ self.conf.profile = config_defaults.ProfileName.PRODUCTION
350​
351​ self.test_rdb.process(self.ref_input, self.ref_arg_data)
352​ # did not raise, pass
353​
354​ def test_not_allowed_namespace_pkg(self, monkeypatch):
355​ ref_ns = {'namespaces_safe': [], 'namespaces_whitelist': {'pkg': ['giturl']}}
356​ monkeypatch.setattr(config, 'load_namespaces_config', lambda: ref_ns)
357​ self.ref_arg_data['namespace'] = 'pkg.not_firefox'
358​ self.conf.profile = config_defaults.ProfileName.PRODUCTION
359​
360​ with pytest.raises(TaskotronDirectiveError):
361​ self.test_rdb.process(self.ref_input, self.ref_arg_data)
362​
363​ def test_not_allowed_namespace_prefix(self, monkeypatch):
364​ ref_ns = {'namespaces_safe': [], 'namespaces_whitelist': {'qa': ['giturl']}}
365​ monkeypatch.setattr(config, 'load_namespaces_config', lambda: ref_ns)
366​ self.ref_arg_data['namespace'] = 'qa1'
367​ self.conf.profile = config_defaults.ProfileName.PRODUCTION
368​
369​ with pytest.raises(TaskotronDirectiveError):
370​ self.test_rdb.process(self.ref_input, self.ref_arg_data)
371​
372​ def test_namespace_not_exist(self, monkeypatch):
373​ ref_ns = {'namespaces_safe': ['scratch'], 'namespaces_whitelist': {'qa': ['giturl']}}
374​ monkeypatch.setattr(config, 'load_namespaces_config', lambda: ref_ns)
375​ self.ref_arg_data['namespace'] = 'qa1'
376​ self.conf.profile = config_defaults.ProfileName.PRODUCTION
377​
378​ with pytest.raises(TaskotronDirectiveError):
379​ self.test_rdb.process(self.ref_input, self.ref_arg_data)