From 069c5c773e4cf54dac2e359d526ad968a6369c18 Mon Sep 17 00:00:00 2001 From: Lukas Holecek Date: Dec 12 2019 19:13:24 +0000 Subject: [PATCH 1/2] Fix loading RemoteRule attribute in on-demand rules Signed-off-by: Lukas Holecek --- diff --git a/greenwave/policies.py b/greenwave/policies.py index c4633c2..e7dc800 100644 --- a/greenwave/policies.py +++ b/greenwave/policies.py @@ -413,7 +413,9 @@ class Rule(SafeYAMLObject): processed_rules = [] for rule in rules: if rule['type'] == 'RemoteRule': - processed_rules.append(RemoteRule()) + temp_rule = RemoteRule() + temp_rule.required = rule.get('required', False) + processed_rules.append(temp_rule) elif rule['type'] == 'PassingTestCaseRule': temp_rule = PassingTestCaseRule() temp_rule.test_case_name = rule['test_case_name'] diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py index c8d3d09..492b82d 100644 --- a/greenwave/tests/test_policies.py +++ b/greenwave/tests/test_policies.py @@ -12,6 +12,7 @@ from greenwave.policies import ( summarize_answers, Policy, RemotePolicy, + RemoteRule, RuleSatisfied, TestResultMissing, TestResultFailed, @@ -1074,6 +1075,47 @@ def test_on_demand_policy_match(two_rules): assert isinstance(decision[0], RuleSatisfied) +@pytest.mark.parametrize('namespace', ["rpms", ""]) +def test_remote_rule_policy_on_demand_policy_required(namespace): + """ Testing the RemoteRule with the koji interaction when on_demand policy is given. + In this case we are just mocking koji """ + + nvr = 'nethack-1.2.3-1.el9000' + subject = create_test_subject('koji_build', nvr) + + serverside_json = { + 'product_version': 'fedora-26', + 'id': 'taskotron_release_critical_tasks_with_remoterule', + 'subject_type': 'koji_build', + 'subject_identifier': nvr, + 'rules': [ + { + 'type': 'RemoteRule', + 'required': True + }, + ], + } + + app = create_app('greenwave.config.TestingConfig') + with app.app_context(): + with mock.patch('greenwave.resources.retrieve_scm_from_koji') as scm: + scm.return_value = (namespace, 'nethack', 'c3c47a08a66451cb9686c49f040776ed35a0d1bb') + with mock.patch('greenwave.resources.retrieve_yaml_remote_rule') as f: + f.return_value = None + + policy = OnDemandPolicy.create_from_json(serverside_json) + assert len(policy.rules) == 1 + assert isinstance(policy.rules[0], RemoteRule) + assert policy.rules[0].required + + results = DummyResultsRetriever() + decision = policy.check('fedora-26', subject, results) + assert len(decision) == 1 + assert isinstance(decision[0], MissingGatingYaml) + assert not decision[0].is_satisfied + assert decision[0].subject.identifier == subject.identifier + + def test_two_rules_no_duplicate(tmpdir): nvr = 'nethack-1.2.3-1.el9000' subject = create_test_subject('koji_build', nvr) From b4042b1426be5d501cf6e8d0c8473131caa3e6fb Mon Sep 17 00:00:00 2001 From: Lukas Holecek Date: Dec 12 2019 19:14:07 +0000 Subject: [PATCH 2/2] Make on-demand policies loading more generic Signed-off-by: Lukas Holecek --- diff --git a/functional-tests/test_api_v1.py b/functional-tests/test_api_v1.py index 30115c2..5890291 100644 --- a/functional-tests/test_api_v1.py +++ b/functional-tests/test_api_v1.py @@ -1387,7 +1387,10 @@ def test_cannot_make_decision_without_required_rule_type( } r = requests_session.post(greenwave_server + 'api/v1.0/decision', json=data) assert r.status_code == 400 - assert ('Key \'type\' is required for every rule') == r.json()['message'] + assert ( + "Failed to parse on demand policy: Attribute 'rules': " + "Key 'type' is required for each list item" + ) == r.json()['message'] @pytest.mark.smoke @@ -1409,7 +1412,10 @@ def test_cannot_make_decision_without_required_rule_testcase_name( } r = requests_session.post(greenwave_server + 'api/v1.0/decision', json=data) assert r.status_code == 400 - assert ('Key \'test_case_name\' is required if not a RemoteRule') == r.json()['message'] + assert ( + "Failed to parse on demand policy: Attribute 'rules': " + "Attribute 'test_case_name' is required" + ) == r.json()['message'] def test_make_a_decision_with_verbose_flag_on_demand_policy( diff --git a/greenwave/policies.py b/greenwave/policies.py index e7dc800..8165d56 100644 --- a/greenwave/policies.py +++ b/greenwave/policies.py @@ -667,12 +667,15 @@ class Policy(SafeYAMLObject): if product_version and not self.matches_product_version(product_version): return False - subject = attributes.get('subject') - if subject and subject.type != self.subject_type: + if not self.matches_subject_type(**attributes): return False return not self.rules or any(rule.matches(self, **attributes) for rule in self.rules) + def matches_subject_type(self, **attributes): + subject = attributes.get('subject') + return not subject or subject.type == self.subject_type + @remove_duplicates def check( self, @@ -713,46 +716,25 @@ class Policy(SafeYAMLObject): class OnDemandPolicy(Policy): - root_yaml_tag = '!Policy' - safe_yaml_attributes = {} - - def __init__(self): - self.id = None - self.product_versions = None - self.subject_type = None - self.rules = None - self.blacklist = None - self.excluded_packages = None - self.packages = None - self.relevance_key = None + root_yaml_tag = None @classmethod - def create_from_json(cls, data_dict): - policy = cls() - policy.id = data_dict.get('id') - policy.product_versions = [data_dict['product_version']] - policy.subject_type = data_dict['subject_type'] - policy.rules = Rule.process_on_demand_rules(data_dict['rules']) - policy.blacklist = data_dict.get('blacklist', []) - policy.excluded_packages = data_dict.get('excluded_packages', []) - policy.packages = data_dict.get('packages', []) - policy.relevance_key = data_dict.get('relevance_key') - - # Validate the data before processing. - policy.__validate_attributes() # pylint: disable=protected-access - return policy - - def __validate_attributes(self): - """ Validates types of the attributes. """ - list_attributes = ['product_versions', 'rules', 'excluded_packages', 'packages'] - for attribute in self.__dict__.keys(): - if attribute in list_attributes and not isinstance( - getattr(self, attribute, None), list): - raise TypeError('{} should be a list.'.format(attribute)) - elif attribute not in list_attributes: - if getattr(self, attribute, None) and not isinstance( - getattr(self, attribute, None), str): - raise TypeError('{} should be a string.'.format(attribute)) + def create_from_json(cls, data): + try: + data2 = { + 'id': 'on-demand-policy', + 'product_versions': [data['product_version']], + 'decision_context': 'on-demand-policy', + 'subject_type': 'unused', + } + data2.update(data) + result = cls.from_value(data2) + return result + except SafeYAMLError as e: + raise BadRequest('Failed to parse on demand policy: {}'.format(e)) + + def matches_subject_type(self, **attributes): + return True class RemotePolicy(Policy): diff --git a/greenwave/safe_yaml.py b/greenwave/safe_yaml.py index 429e4a9..6a65bfb 100644 --- a/greenwave/safe_yaml.py +++ b/greenwave/safe_yaml.py @@ -4,6 +4,8 @@ Provides a way of defining type-safe YAML parsing. """ import yaml +safe_yaml_tag_to_class = {} + class SafeYAMLError(RuntimeError): """ @@ -22,6 +24,9 @@ class SafeYAMLAttribute(object): def from_yaml(self, loader, node): raise NotImplementedError() + def from_value(self, value): + raise NotImplementedError() + def to_json(self, value): raise NotImplementedError() @@ -39,6 +44,9 @@ class SafeYAMLDict(SafeYAMLAttribute): def from_yaml(self, loader, node): value = loader.construct_mapping(node) + return self.from_value(value) + + def from_value(self, value): if isinstance(value, dict): return value @@ -63,6 +71,9 @@ class SafeYAMLBool(SafeYAMLAttribute): def from_yaml(self, loader, node): value = loader.construct_scalar(node) value = yaml.safe_load(value) + return self.from_value(value) + + def from_value(self, value): if isinstance(value, bool): return value @@ -86,6 +97,9 @@ class SafeYAMLString(SafeYAMLAttribute): def from_yaml(self, loader, node): value = loader.construct_scalar(node) + return self.from_value(value) + + def from_value(self, value): return str(value) def to_json(self, value): @@ -109,6 +123,29 @@ class SafeYAMLList(SafeYAMLAttribute): def from_yaml(self, loader, node): values = loader.construct_sequence(node) + return self._from_value(values) + + def from_value(self, value): + results = [] + for item in value: + if not isinstance(item, dict): + results.append(item) + continue + + item_type = item.get('type') + if not item_type: + raise SafeYAMLError("Key 'type' is required for each list item") + + cls = safe_yaml_tag_to_class.get(item_type) + if cls is None: + raise SafeYAMLError( + "Key 'type' for an list item is not valid: {}".format(item_type)) + + results.append(cls.from_value(item)) + + return self._from_value(results) + + def _from_value(self, values): for value in values: if not isinstance(value, self.item_type): raise SafeYAMLError( @@ -137,23 +174,28 @@ class SafeYAMLObjectMetaclass(yaml.YAMLObjectMetaclass): def __init__(cls, name, bases, kwds): super().__init__(name, bases, kwds) - tag = getattr(cls, 'root_yaml_tag', None) - if tag: + root_yaml_tag = getattr(cls, 'root_yaml_tag', None) + if root_yaml_tag: class Loader(cls.yaml_loader): def get_node(self): node = super().get_node() - if node.tag != tag: - raise SafeYAMLError('Missing {} tag'.format(tag)) + if node.tag != root_yaml_tag: + raise SafeYAMLError('Missing {} tag'.format(root_yaml_tag)) if not isinstance(node, yaml.MappingNode): - raise SafeYAMLError('Expected mapping for {} tagged object'.format(tag)) + raise SafeYAMLError( + 'Expected mapping for {} tagged object'.format(root_yaml_tag)) return node - Loader.add_constructor(tag, cls.from_yaml) + Loader.add_constructor(root_yaml_tag, cls.from_yaml) cls.yaml_loader = Loader + yaml_tag = getattr(cls, 'yaml_tag', None) + if yaml_tag: + safe_yaml_tag_to_class[yaml_tag.lstrip('!')] = cls + class SafeYAMLObject(yaml.YAMLObject, metaclass=SafeYAMLObjectMetaclass): """ @@ -228,6 +270,28 @@ class SafeYAMLObject(yaml.YAMLObject, metaclass=SafeYAMLObjectMetaclass): return values + @classmethod + def from_value(cls, data): + result = cls() + + for attribute_name, yaml_attribute in cls.safe_yaml_attributes.items(): + value = data.get(attribute_name) + if value is None: + if not yaml_attribute.optional: + msg = 'Attribute {!r} is required'.format(attribute_name) + raise SafeYAMLError(msg) + value = yaml_attribute.default_value + else: + try: + value = yaml_attribute.from_value(value) + except SafeYAMLError as e: + msg = 'Attribute {!r}: {}'.format(attribute_name, str(e)) + raise SafeYAMLError(msg) + + setattr(result, attribute_name, value) + + return result + @property def safe_yaml_label(self): return 'YAML object {}'.format(self.yaml_tag) diff --git a/greenwave/tests/test_policies.py b/greenwave/tests/test_policies.py index 492b82d..11845d4 100644 --- a/greenwave/tests/test_policies.py +++ b/greenwave/tests/test_policies.py @@ -1075,8 +1075,7 @@ def test_on_demand_policy_match(two_rules): assert isinstance(decision[0], RuleSatisfied) -@pytest.mark.parametrize('namespace', ["rpms", ""]) -def test_remote_rule_policy_on_demand_policy_required(namespace): +def test_remote_rule_policy_on_demand_policy_required(): """ Testing the RemoteRule with the koji interaction when on_demand policy is given. In this case we are just mocking koji """ @@ -1086,8 +1085,7 @@ def test_remote_rule_policy_on_demand_policy_required(namespace): serverside_json = { 'product_version': 'fedora-26', 'id': 'taskotron_release_critical_tasks_with_remoterule', - 'subject_type': 'koji_build', - 'subject_identifier': nvr, + 'subject': [{'item': nvr, 'type': 'koji_build'}], 'rules': [ { 'type': 'RemoteRule', @@ -1099,7 +1097,7 @@ def test_remote_rule_policy_on_demand_policy_required(namespace): app = create_app('greenwave.config.TestingConfig') with app.app_context(): with mock.patch('greenwave.resources.retrieve_scm_from_koji') as scm: - scm.return_value = (namespace, 'nethack', 'c3c47a08a66451cb9686c49f040776ed35a0d1bb') + scm.return_value = ('rpms', 'nethack', 'c3c47a08a66451cb9686c49f040776ed35a0d1bb') with mock.patch('greenwave.resources.retrieve_yaml_remote_rule') as f: f.return_value = None