Merge lp:~zyga/checkbox/fix-1297746 into lp:checkbox

Proposed by Zygmunt Krynicki on 2014-03-28
Status: Merged
Approved by: Daniel Manrique 🚗 on 2014-03-31
Approved revision: 2852
Merged at revision: 2854
Proposed branch: lp:~zyga/checkbox/fix-1297746
Merge into: lp:checkbox
Diff against target: 505 lines (+153/-53)
6 files modified
plainbox/plainbox/impl/job.py (+35/-20)
plainbox/plainbox/impl/secure/providers/test_v1.py (+10/-7)
plainbox/plainbox/impl/secure/providers/v1.py (+55/-10)
plainbox/plainbox/impl/test_job.py (+16/-8)
plainbox/plainbox/provider_manager.py (+13/-1)
plainbox/plainbox/test_provider_manager.py (+24/-7)
To merge this branch: bzr merge lp:~zyga/checkbox/fix-1297746
Reviewer Review Type Date Requested Status
Daniel Manrique 🚗 2014-03-28 Approve on 2014-03-31
Review via email: mp+213341@code.launchpad.net

Description of the change

5a7415f plainbox:job: don't require the id field
3a845c8 plainbox:job: validate the presence of job.partial_id
eddc7e0 plainbox:job: drop validator_cls argument
17f4314 plainbox:job: add Problem.deprecated
ad51337 plainbox:job: add strictness control to job validator
3f05db4 plainbox:secure:providers: drop useless import
c1974b4 plainbox:secure:providers: expose control for job validation
b2e6e20 plainbox:provider_manager: enforce strong validation in `manage.py validate`
dd9036e plainbox:provider_manager: handle deprecated fields

To post a comment you must log in.
lp:~zyga/checkbox/fix-1297746 updated on 2014-03-28
2848. By Zygmunt Krynicki on 2014-03-28

plainbox:job: add strictness control to job validator

This patch adds two optional arguments to the validator, strict, which
effectively runs the validator at maximum strictness and deprecated
which makes it find deprecated fields or values.

The default settings will ignore harmless errors (thus making the
validator do no evil) while still reporting (and preventing) serious
problems that would definitely fail at job runtime.

Signed-off-by: Zygmunt Krynicki <email address hidden>

2849. By Zygmunt Krynicki on 2014-03-28

plainbox:secure:providers: drop useless import

Signed-off-by: Zygmunt Krynicki <email address hidden>

2850. By Zygmunt Krynicki on 2014-03-28

plainbox:secure:providers: expose control for job validation

This patch allows two settings (validate and validation_kwargs) to propagate
from Provider1 to JobDefinitionPlugIn. By default validation is enabled
but no validation keywords are supplied so only serious problems are
"reported" (as in preventing a job definition from being loaded).
Those can be changed by passing the two new keyword-only arguments.

They are passed from Provider1.from_definition(),
through Provider1.__init__() and to JobDefinitionPlugIn where they control
the validation block. The second argument (validation_kwargs) is passed
directly to the validator to control its behavior.

Some tests were altered not to overly validate everything where it is
obvious we don't care about valid jobs, just about getting some dummy
data across.

Signed-off-by: Zygmunt Krynicki <email address hidden>

2851. By Zygmunt Krynicki on 2014-03-28

plainbox:provider_manager: enforce strong validation in `manage.py validate`

This patch ironically allows the validate command to load broken job
definitions and inspect them manually inside the command core.

Signed-off-by: Zygmunt Krynicki <email address hidden>

2852. By Zygmunt Krynicki on 2014-03-28

plainbox:provider_manager: handle deprecated fields

This patch makes the `manage.py validate` command support the
Problem.deprecated problem which is used to report 'name' being used
inside job definitions.

Signed-off-by: Zygmunt Krynicki <email address hidden>

Daniel Manrique 🚗 (roadmr) wrote :

Awesome, maybe next we can plug this validation into the tarmac tests to protect against borked jobs?

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plainbox/plainbox/impl/job.py'
2--- plainbox/plainbox/impl/job.py 2014-03-27 13:57:14 +0000
3+++ plainbox/plainbox/impl/job.py 2014-03-28 22:08:58 +0000
4@@ -50,6 +50,7 @@
5 missing = 'missing'
6 wrong = 'wrong'
7 useless = 'useless'
8+ deprecated = 'deprecated'
9
10
11 class ValidationError(ValueError):
12@@ -75,15 +76,27 @@
13 """
14
15 @staticmethod
16- def validate(job):
17+ def validate(job, strict=False, deprecated=False):
18 """
19 Validate the specified job
20+
21+ :param strict:
22+ Enforce strict validation. Non-conforming jobs will be rejected.
23+ This is off by default to ensure that non-critical errors don't
24+ prevent jobs from running.
25+ :param deprecated:
26+ Enforce deprecation validation. Jobs having deprecated fields will
27+ be rejected. This is off by default to allow backwards compatible
28+ jobs to be used without any changes.
29 """
30- # Check if id is empty
31- if job.id is None:
32+ # Check if name is still being used, if running in strict mode
33+ if deprecated and job.name is not None:
34+ raise ValidationError(job.fields.name, Problem.deprecated)
35+ # Check if the partial_id field is empty
36+ if job.partial_id is None:
37 raise ValidationError(job.fields.id, Problem.missing)
38- # Check if summary is empty
39- if job.summary is None:
40+ # Check if summary is empty, if running in strict mode
41+ if strict and job.summary is None:
42 raise ValidationError(job.fields.summary, Problem.missing)
43 # Check if plugin is empty
44 if job.plugin is None:
45@@ -91,11 +104,13 @@
46 # Check if plugin has a good value
47 if job.plugin not in JobDefinition.plugin.get_all_symbols():
48 raise ValidationError(job.fields.plugin, Problem.wrong)
49- # Check if user is given without a command to run
50- if job.user is not None and job.command is None:
51+ # Check if user is given without a command to run, if running in strict
52+ # mode
53+ if strict and job.user is not None and job.command is None:
54 raise ValidationError(job.fields.user, Problem.useless)
55- # Check if environ is given without a command to run
56- if job.environ is not None and job.command is None:
57+ # Check if environ is given without a command to run, if running in
58+ # strict mode
59+ if strict and job.environ is not None and job.command is None:
60 raise ValidationError(job.fields.environ, Problem.useless)
61 # Verify that command is present on a job within the subset that should
62 # really have them (shell, local, resource, attachment, user-verify and
63@@ -121,8 +136,9 @@
64 if job.description is None:
65 raise ValidationError(
66 job.fields.description, Problem.missing)
67- # Ensure that manual jobs don't have command
68- if job.command is not None:
69+ # Ensure that manual jobs don't have command, if running in strict
70+ # mode
71+ if strict and job.command is not None:
72 raise ValidationError(job.fields.command, Problem.useless)
73
74
75@@ -481,16 +497,12 @@
76 @classmethod
77 def from_rfc822_record(cls, record):
78 """
79- Create a JobDefinition instance from rfc822 record
80+ Create a JobDefinition instance from rfc822 record. The resulting
81+ instance may not be valid but will always be created. Only valid jobs
82+ should be executed.
83
84 The record must be a RFC822Record instance.
85-
86- Only the 'id' and 'plugin' keys are required.
87- All other data is stored as is and is entirely optional.
88 """
89- if 'id' not in record.data and 'name' not in record.data:
90- # TRANSLATORS: don't translate id or translate it as 'id field'
91- raise ValueError(_("Cannot create job without an id"))
92 # Strip the trailing newlines form all the raw values coming from the
93 # RFC822 parser. We don't need them and they don't match gettext keys
94 # (xgettext strips out those newlines)
95@@ -498,14 +510,17 @@
96 key: value.rstrip('\n')
97 for key, value in record.raw_data.items()})
98
99- def validate(self, validator_cls=CheckBoxJobValidator):
100+ def validate(self, **validation_kwargs):
101 """
102 Validate this job definition with the specified validator
103
104+ :param validation_kwargs:
105+ Keyword arguments to pass to the
106+ :meth:`CheckBoxJobValidator.validate()`
107 :raises ValidationError:
108 If the job has any problems that make it unsuitable for execution.
109 """
110- validator_cls.validate(self)
111+ CheckBoxJobValidator.validate(self, **validation_kwargs)
112
113 def create_child_job_from_record(self, record):
114 """
115
116=== modified file 'plainbox/plainbox/impl/secure/providers/test_v1.py'
117--- plainbox/plainbox/impl/secure/providers/test_v1.py 2014-03-28 13:02:24 +0000
118+++ plainbox/plainbox/impl/secure/providers/test_v1.py 2014-03-28 22:08:58 +0000
119@@ -25,7 +25,6 @@
120 """
121
122 from unittest import TestCase
123-import os
124
125 from plainbox.impl.job import JobDefinition
126 from plainbox.impl.secure.config import Unset
127@@ -649,7 +648,10 @@
128 self.provider = Provider1(
129 self.NAME, self.VERSION, self.DESCRIPTION, self.SECURE,
130 self.GETTEXT_DOMAIN, self.JOBS_DIR, self.WHITELISTS_DIR,
131- self.DATA_DIR, self.BIN_DIR, self.LOCALE_DIR, self.BASE_DIR)
132+ self.DATA_DIR, self.BIN_DIR, self.LOCALE_DIR, self.BASE_DIR,
133+ # We are using dummy job definitions so let's not shout about those
134+ # being invalid in each test
135+ validate=False)
136
137 def test_repr(self):
138 self.assertEqual(
139@@ -792,11 +794,11 @@
140 JobDefinitionPlugIn("/path/to/jobs1.txt", (
141 "id: a2\n"
142 "\n"
143- "id: a1\n"), self.provider),
144+ "id: a1\n"), self.provider, validate=False),
145 JobDefinitionPlugIn("/path/to/jobs2.txt", (
146 "id: a3\n"
147 "\n"
148- "id: a4\n"), self.provider)
149+ "id: a4\n"), self.provider, validate=False)
150 ]
151 with self.provider._job_collection.fake_plugins(fake_plugins):
152 job_list = self.provider.get_builtin_jobs()
153@@ -840,11 +842,11 @@
154 JobDefinitionPlugIn("/path/to/jobs1.txt", (
155 "id: a2\n"
156 "\n"
157- "id: a1\n"), self.provider),
158+ "id: a1\n"), self.provider, validate=False),
159 JobDefinitionPlugIn("/path/to/jobs2.txt", (
160 "id: a3\n"
161 "\n"
162- "id: a4\n"), self.provider)
163+ "id: a4\n"), self.provider, validate=False)
164 ]
165 with self.provider._job_collection.fake_plugins(fake_plugins):
166 job_list, problem_list = self.provider.load_all_jobs()
167@@ -862,7 +864,8 @@
168 """
169 fake_plugins = [
170 JobDefinitionPlugIn(
171- "/path/to/jobs1.txt", "id: working\n", self.provider)
172+ "/path/to/jobs1.txt", "id: working\n", self.provider,
173+ validate=False)
174 ]
175 fake_problems = [
176 PlugInError("some problem"),
177
178=== modified file 'plainbox/plainbox/impl/secure/providers/v1.py'
179--- plainbox/plainbox/impl/secure/providers/v1.py 2014-03-28 13:02:24 +0000
180+++ plainbox/plainbox/impl/secure/providers/v1.py 2014-03-28 22:08:58 +0000
181@@ -31,6 +31,7 @@
182 from plainbox.abc import IProvider1, IProviderBackend1
183 from plainbox.i18n import gettext as _
184 from plainbox.impl.job import JobDefinition
185+from plainbox.impl.job import ValidationError
186 from plainbox.impl.secure.config import Config, Variable
187 from plainbox.impl.secure.config import IValidator
188 from plainbox.impl.secure.config import NotEmptyValidator
189@@ -87,12 +88,30 @@
190 list of :class:`plainbox.impl.job.JobDefinition` instances from a file.
191 """
192
193- def __init__(self, filename, text, provider):
194+ def __init__(self, filename, text, provider, *,
195+ validate=True, validation_kwargs=None):
196 """
197 Initialize the plug-in with the specified name text
198+
199+ :param filename:
200+ Name of the file with job definitions
201+ :param text:
202+ Full text of the file with job definitions
203+ :param provider:
204+ A provider object to which those jobs belong to
205+ :param validate:
206+ Enable job validation. Incorrect job definitions will not be loaded
207+ and will abort the process of loading of the remainder of the jobs.
208+ This is ON by default to prevent broken job definitions from being
209+ used. This is a keyword-only argument.
210+ :param validation_kwargs:
211+ Keyword arguments to pass to the JobDefinition.validate(). Note,
212+ this is a single argument. This is a keyword-only argument.
213 """
214 self._filename = filename
215 self._job_list = []
216+ if validation_kwargs is None:
217+ validation_kwargs = {}
218 logger.debug(_("Loading jobs definitions from %r..."), filename)
219 try:
220 records = load_rfc822_records(
221@@ -108,10 +127,16 @@
222 raise PlugInError(
223 _("Cannot define job from record {!r}: {}").format(
224 record, exc))
225- else:
226- job._provider = provider
227- self._job_list.append(job)
228- logger.debug(_("Loaded %r"), job)
229+ job._provider = provider
230+ if validate:
231+ try:
232+ job.validate(**validation_kwargs)
233+ except ValidationError as exc:
234+ raise PlugInError(
235+ _("Problem in job definition, field {}: {}").format(
236+ exc.field, exc.problem))
237+ self._job_list.append(job)
238+ logger.debug(_("Loaded %r"), job)
239
240 @property
241 def plugin_name(self):
242@@ -142,7 +167,7 @@
243
244 def __init__(self, name, version, description, secure, gettext_domain,
245 jobs_dir, whitelists_dir, data_dir, bin_dir, locale_dir,
246- base_dir):
247+ base_dir, *, validate=True, validation_kwargs=None):
248 """
249 Initialize a provider with a set of meta-data and directories.
250
251@@ -187,6 +212,16 @@
252 path of the directory with (perhaps) all of jobs_dir,
253 whitelist_dir, data_dir, bin_dir, locale_dir. This may be None.
254 This is also the effective value of $CHECKBOX_SHARE
255+
256+ :param validate:
257+ Enable job validation. Incorrect job definitions will not be loaded
258+ and will abort the process of loading of the remainder of the jobs.
259+ This is ON by default to prevent broken job definitions from being
260+ used. This is a keyword-only argument.
261+
262+ :param validation_kwargs:
263+ Keyword arguments to pass to the JobDefinition.validate(). Note,
264+ this is a single argument. This is a keyword-only argument.
265 """
266 # Meta-data
267 self._name = name
268@@ -215,22 +250,31 @@
269 jobs_dir_list = []
270 self._job_collection = FsPlugInCollection(
271 jobs_dir_list, ext=(".txt", ".txt.in"),
272- wrapper=JobDefinitionPlugIn, provider=self)
273+ wrapper=JobDefinitionPlugIn, provider=self,
274+ validate=validate, validation_kwargs=validation_kwargs)
275 # Setup translations
276 if gettext_domain and locale_dir:
277 gettext.bindtextdomain(self._gettext_domain, self._locale_dir)
278
279 @classmethod
280- def from_definition(cls, definition, secure):
281+ def from_definition(cls, definition, secure, *, validate=True,
282+ validation_kwargs=None):
283 """
284 Initialize a provider from Provider1Definition object
285
286 :param definition:
287 A Provider1Definition object to use as reference
288-
289 :param secure:
290 Value of the secure flag. This cannot be expressed by a definition
291 object.
292+ :param validate:
293+ Enable job validation. Incorrect job definitions will not be loaded
294+ and will abort the process of loading of the remainder of the jobs.
295+ This is ON by default to prevent broken job definitions from being
296+ used. This is a keyword-only argument.
297+ :param validation_kwargs:
298+ Keyword arguments to pass to the JobDefinition.validate(). Note,
299+ this is a single argument. This is a keyword-only argument.
300
301 This method simplifies initialization of a Provider1 object where the
302 caller already has a Provider1Definition object. Depending on the value
303@@ -248,7 +292,8 @@
304 secure, definition.effective_gettext_domain,
305 definition.effective_jobs_dir, definition.effective_whitelists_dir,
306 definition.effective_data_dir, definition.effective_bin_dir,
307- definition.effective_locale_dir, definition.location or None)
308+ definition.effective_locale_dir, definition.location or None,
309+ validate=validate, validation_kwargs=validation_kwargs)
310
311 def __repr__(self):
312 return "<{} name:{!r}>".format(self.__class__.__name__, self.name)
313
314=== modified file 'plainbox/plainbox/impl/test_job.py'
315--- plainbox/plainbox/impl/test_job.py 2014-03-27 13:57:24 +0000
316+++ plainbox/plainbox/impl/test_job.py 2014-03-28 22:08:58 +0000
317@@ -42,6 +42,19 @@
318
319 class CheckBoxJobValidatorTests(TestCase):
320
321+ def test_validate_checks_for_deprecated_name(self):
322+ """
323+ verify that validate() checks if jobs have a value for the 'id'
324+ field.
325+ """
326+ job = JobDefinition({
327+ 'name': 'name'
328+ })
329+ with self.assertRaises(ValidationError) as boom:
330+ CheckBoxJobValidator.validate(job, deprecated=True)
331+ self.assertEqual(boom.exception.field, JobDefinition.fields.name)
332+ self.assertEqual(boom.exception.problem, Problem.deprecated)
333+
334 def test_validate_checks_for_missing_id(self):
335 """
336 verify that validate() checks if jobs have a value for the 'id'
337@@ -91,7 +104,7 @@
338 'user': 'root'
339 })
340 with self.assertRaises(ValidationError) as boom:
341- CheckBoxJobValidator.validate(job)
342+ CheckBoxJobValidator.validate(job, strict=True)
343 self.assertEqual(boom.exception.field, JobDefinition.fields.user)
344 self.assertEqual(boom.exception.problem, Problem.useless)
345
346@@ -106,7 +119,7 @@
347 'environ': 'VAR_NAME'
348 })
349 with self.assertRaises(ValidationError) as boom:
350- CheckBoxJobValidator.validate(job)
351+ CheckBoxJobValidator.validate(job, strict=True)
352 self.assertEqual(boom.exception.field, JobDefinition.fields.environ)
353 self.assertEqual(boom.exception.problem, Problem.useless)
354
355@@ -137,7 +150,7 @@
356 'command': 'run_some_test'
357 })
358 with self.assertRaises(ValidationError) as boom:
359- CheckBoxJobValidator.validate(job)
360+ CheckBoxJobValidator.validate(job, strict=True)
361 self.assertEqual(boom.exception.field, JobDefinition.fields.command)
362 self.assertEqual(boom.exception.problem, Problem.useless)
363
364@@ -254,11 +267,6 @@
365 self.assertEqual(job.command, None)
366 self.assertEqual(job.description, None)
367
368- def test_from_rfc822_record_missing_id(self):
369- record = RFC822Record({'plugin': 'plugin'})
370- with self.assertRaises(ValueError):
371- JobDefinition.from_rfc822_record(record)
372-
373 def test_str(self):
374 job = JobDefinition(self._min_record.data)
375 self.assertEqual(str(job), "id")
376
377=== modified file 'plainbox/plainbox/provider_manager.py'
378--- plainbox/plainbox/provider_manager.py 2014-03-28 13:22:04 +0000
379+++ plainbox/plainbox/provider_manager.py 2014-03-28 22:08:58 +0000
380@@ -623,7 +623,7 @@
381 problem_list = []
382 for job in job_list:
383 try:
384- job.validate()
385+ job.validate(strict=True, deprecated=True)
386 except JobValidationError as exc:
387 problem_list.append((job, exc))
388 return problem_list
389@@ -636,6 +636,7 @@
390 Problem.missing: _("missing definition of required field"),
391 Problem.wrong: _("incorrect value supplied"),
392 Problem.useless: _("useless field in this context"),
393+ Problem.deprecated: _("usage of deprecated field"),
394 }
395 for job, error in problem_list:
396 if isinstance(error, JobValidationError):
397@@ -664,6 +665,17 @@
398 else:
399 print(_("All jobs seem to be valid"))
400
401+ def get_provider(self):
402+ """
403+ Get a Provider1 that describes the current provider
404+
405+ This version disables all validation so that we can see totally broken
406+ providers and let us validate them and handle the errors explicitly.
407+ """
408+ return Provider1.from_definition(
409+ # NOTE: don't validate, we want to validate manually
410+ self.definition, secure=False, validate=False)
411+
412
413 @docstring(
414 # TRANSLATORS: please leave various options (both long and short forms),
415
416=== modified file 'plainbox/plainbox/test_provider_manager.py'
417--- plainbox/plainbox/test_provider_manager.py 2014-03-28 13:22:04 +0000
418+++ plainbox/plainbox/test_provider_manager.py 2014-03-28 22:08:58 +0000
419@@ -120,7 +120,7 @@
420 self.tmpdir + os.path.join("/foo", "lib", "plainbox-providers-1",
421 "2014.com.example:test", "jobs",
422 "jobs.txt"),
423- "name: dummy\nplugin: shell\ncommand: true\n")
424+ "id: dummy\nplugin: shell\ncommand: true\n")
425
426 def test_install__flat_partial(self):
427 """
428@@ -155,7 +155,7 @@
429 self.assertFileContent(
430 self.tmpdir + os.path.join(
431 prefix, "share", "2014.com.example:test", "jobs", "jobs.txt"),
432- "name: dummy\nplugin: shell\ncommand: true\n")
433+ "id: dummy\nplugin: shell\ncommand: true\n")
434 self.assertFileContent(
435 self.tmpdir + os.path.join(
436 prefix, "share", "2014.com.example:test", "whitelists",
437@@ -198,7 +198,7 @@
438 self.tmpdir, "dist", "2014.com.example.test-1.0.tar.gz")
439 self.assertTarballContent(
440 tarball, "2014.com.example.test-1.0/jobs/jobs.txt",
441- "name: dummy\nplugin: shell\ncommand: true\n")
442+ "id: dummy\nplugin: shell\ncommand: true\n")
443 self.assert_common_sdist(tarball)
444
445 def test_sdist__partial(self):
446@@ -287,7 +287,7 @@
447 """
448 filename = os.path.join(self.tmpdir, "jobs", "broken.txt")
449 with open(filename, "wt", encoding='UTF-8') as stream:
450- print("name: broken", file=stream)
451+ print("id: broken", file=stream)
452 print("plugin: shell", file=stream)
453 with TestIO() as test_io:
454 self.tool.main(["validate"])
455@@ -303,7 +303,7 @@
456 """
457 filename = os.path.join(self.tmpdir, "jobs", "broken.txt")
458 with open(filename, "wt", encoding='UTF-8') as stream:
459- print("name: broken", file=stream)
460+ print("id: broken", file=stream)
461 print("plugin: magic", file=stream)
462 with TestIO() as test_io:
463 self.tool.main(["validate"])
464@@ -321,7 +321,7 @@
465 """
466 filename = os.path.join(self.tmpdir, "jobs", "broken.txt")
467 with open(filename, "wt", encoding='UTF-8') as stream:
468- print("name: broken", file=stream)
469+ print("id: broken", file=stream)
470 print("plugin: manual", file=stream)
471 print("description: broken job definition", file=stream)
472 print("command: true", file=stream)
473@@ -332,6 +332,23 @@
474 "jobs/broken.txt:1-4: job '2014.com.example::broken', field 'command': "
475 "useless field in this context\n"))
476
477+ def test_validate__broken_deprecated_field(self):
478+ """
479+ verify that ./manage.py validate shows information about deprecated fields
480+ """
481+ filename = os.path.join(self.tmpdir, "jobs", "broken.txt")
482+ with open(filename, "wt", encoding='UTF-8') as stream:
483+ print("name: broken", file=stream)
484+ print("plugin: manual", file=stream)
485+ print("description: broken job definition", file=stream)
486+ print("command: true", file=stream)
487+ with TestIO() as test_io:
488+ self.tool.main(["validate"])
489+ self.assertEqual(
490+ test_io.stdout, (
491+ "jobs/broken.txt:1-4: job '2014.com.example::broken', field 'name': "
492+ "usage of deprecated field\n"))
493+
494 def test_info(self):
495 """
496 verify that ./manage.py info shows basic provider information
497@@ -359,7 +376,7 @@
498 os.mkdir(os.path.join(tmpdir, "jobs"))
499 filename = os.path.join(tmpdir, "jobs", "jobs.txt")
500 with open(filename, "wt", encoding='UTF-8') as stream:
501- print("name: dummy", file=stream)
502+ print("id: dummy", file=stream)
503 print("plugin: shell", file=stream)
504 print("command: true", file=stream)
505 os.mkdir(os.path.join(tmpdir, "whitelists"))

Subscribers

People subscribed via source and target branches