From 7cd5b13e34270dd5be79269a0b88c8c408c18663 Mon Sep 17 00:00:00 2001 From: James Cammarata Date: Thu, 30 Oct 2014 16:04:34 -0500 Subject: [PATCH] Make sure v2 playbook classes validate attributes Also removing cruft code from earlier iteration on task.py and fixing a bug where 'shell' was not removed from the task ds after munge() cleaned things up --- v2/ansible/playbook/base.py | 26 +++- v2/ansible/playbook/role/metadata.py | 12 +- v2/ansible/playbook/task.py | 180 +-------------------------- 3 files changed, 23 insertions(+), 195 deletions(-) diff --git a/v2/ansible/playbook/base.py b/v2/ansible/playbook/base.py index e2b96c8cc25..c1632403639 100644 --- a/v2/ansible/playbook/base.py +++ b/v2/ansible/playbook/base.py @@ -24,6 +24,7 @@ from io import FileIO from six import iteritems, string_types +from ansible.errors import AnsibleParserError from ansible.playbook.attribute import Attribute, FieldAttribute from ansible.parsing.yaml import DataLoader @@ -73,14 +74,18 @@ class Base: if isinstance(ds, string_types) or isinstance(ds, FileIO): ds = self._loader.load(ds) - # we currently don't do anything with private attributes but may - # later decide to filter them out of 'ds' here. - + # call the munge() function to massage the data into something + # we can more easily parse, and then call the validation function + # on it to ensure there are no incorrect key values ds = self.munge(ds) + self._validate_attributes(ds) + + # Walk all attributes in the class. + # + # FIXME: we currently don't do anything with private attributes but + # may later decide to filter them out of 'ds' here. - # walk all attributes in the class for (name, attribute) in iteritems(self._get_base_attributes()): - # copy the value over unless a _load_field method is defined if name in ds: method = getattr(self, '_load_%s' % name, None) @@ -96,6 +101,17 @@ class Base: def get_loader(self): return self._loader + def _validate_attributes(self, ds): + ''' + Ensures that there are no keys in the datastructure which do + not map to attributes for this object. + ''' + + valid_attrs = [name for (name, attribute) in iteritems(self._get_base_attributes())] + for key in ds: + if key not in valid_attrs: + raise AnsibleParserError("'%s' is not a valid attribute for a %s" % (key, self.__class__), obj=ds) + def validate(self): ''' validation that is done at parse time, not load time ''' diff --git a/v2/ansible/playbook/role/metadata.py b/v2/ansible/playbook/role/metadata.py index 9e732d6eeaa..485e3da59f2 100644 --- a/v2/ansible/playbook/role/metadata.py +++ b/v2/ansible/playbook/role/metadata.py @@ -21,7 +21,7 @@ __metaclass__ = type from six import iteritems, string_types -from ansible.errors import AnsibleError, AnsibleParserError +from ansible.errors import AnsibleParserError from ansible.playbook.attribute import Attribute, FieldAttribute from ansible.playbook.base import Base from ansible.playbook.role.include import RoleInclude @@ -56,16 +56,6 @@ class RoleMetadata(Base): m = RoleMetadata().load_data(data, loader=loader) return m - def munge(self, ds): - # make sure there are no keys in the datastructure which - # do not map to attributes for this object - valid_attrs = [name for (name, attribute) in iteritems(self._get_base_attributes())] - for name in ds: - if name not in valid_attrs: - print("'%s' is not a valid attribute" % name) - raise AnsibleParserError("'%s' is not a valid attribute" % name, obj=ds) - return ds - def _load_dependencies(self, attr, ds): ''' This is a helper loading function for the dependencis list, diff --git a/v2/ansible/playbook/task.py b/v2/ansible/playbook/task.py index 422668148ba..97f7b06eb62 100644 --- a/v2/ansible/playbook/task.py +++ b/v2/ansible/playbook/task.py @@ -160,7 +160,7 @@ class Task(Base): new_ds['delegate_to'] = delegate_to for (k,v) in ds.iteritems(): - if k in ('action', 'local_action', 'args', 'delegate_to') or k == action: + if k in ('action', 'local_action', 'args', 'delegate_to') or k == action or k == 'shell': # we don't want to re-assign these values, which were # determined by the ModuleArgsParser() above continue @@ -171,181 +171,3 @@ class Task(Base): return new_ds - - # ================================================================================== - # BELOW THIS LINE - # info below this line is "old" and is before the attempt to build Attributes - # use as reference but plan to replace and radically simplify - # ================================================================================== - -LEGACY = """ - - def _load_action(self, ds, k, v): - ''' validate/transmogrify/assign the module and parameters if used in 'action/local_action' format ''' - - results = dict() - module_name, params = v.strip().split(' ', 1) - if module_name not in module_finder: - raise AnsibleError("the specified module '%s' could not be found, check your module path" % module_name) - results['_module_name'] = module_name - results['_parameters'] = parse_kv(params) - - if k == 'local_action': - if 'delegate_to' in ds: - raise AnsibleError("delegate_to cannot be specified with local_action in task: %s" % ds.get('name', v)) - results['_delegate_to'] = '127.0.0.1' - if not 'transport' in ds and not 'connection' in ds: - results['_transport'] = 'local' - return results - - def _load_module(self, ds, k, v): - ''' validate/transmogrify/assign the module and parameters if used in 'module:' format ''' - - results = dict() - if self._module_name: - raise AnsibleError("the module name (%s) was already specified, '%s' is a duplicate" % (self._module_name, k)) - elif 'action' in ds: - raise AnsibleError("multiple actions specified in task: '%s' and '%s'" % (k, ds.get('name', ds['action']))) - results['_module_name'] = k - if isinstance(v, dict) and 'args' in ds: - raise AnsibleError("can't combine args: and a dict for %s: in task %s" % (k, ds.get('name', "%s: %s" % (k, v)))) - results['_parameters'] = self._load_parameters(v) - return results - - def _load_loop(self, ds, k, v): - ''' validate/transmogrify/assign the module any loop directives that have valid action plugins as names ''' - - results = dict() - if isinstance(v, basestring): - param = v.strip() - if (param.startswith('{{') and param.find('}}') == len(ds[x]) - 2 and param.find('|') == -1): - utils.warning("It is unnecessary to use '{{' in loops, leave variables in loop expressions bare.") - plugin_name = k.replace("with_","") - if plugin_name in utils.plugins.lookup_loader: - results['_lookup_plugin'] = plugin_name - results['_lookup_terms'] = v - else: - raise errors.AnsibleError("cannot find lookup plugin named %s for usage in with_%s" % (plugin_name, plugin_name)) - return results - - def _load_legacy_when(self, ds, k, v): - ''' yell about old when syntax being used still ''' - - utils.deprecated("The 'when_' conditional has been removed. Switch to using the regular unified 'when' statements as described on docs.ansible.com.","1.5", removed=True) - if self._when: - raise errors.AnsibleError("multiple when_* statements specified in task %s" % (ds.get('name', ds.get('action')))) - when_name = k.replace("when_","") - return dict(_when = "%s %s" % (when_name, v)) - - def _load_when(self, ds, k, v): - ''' validate/transmogrify/assign a conditional ''' - - conditionals = self._when.copy() - conditionals.push(v) - return dict(_when=conditionals) - - def _load_changed_when(self, ds, k, v): - ''' validate/transmogrify/assign a changed_when conditional ''' - - conditionals = self._changed_when.copy() - conditionals.push(v) - return dict(_changed_when=conditionals) - - def _load_failed_when(self, ds, k, v): - ''' validate/transmogrify/assign a failed_when conditional ''' - - conditionals = self._failed_when.copy() - conditionals.push(v) - return dict(_failed_when=conditionals) - - # FIXME: move to BaseObject - def _load_tags(self, ds, k, v): - ''' validate/transmogrify/assign any tags ''' - - new_tags = self.tags.copy() - tags = v - if isinstance(v, basestring): - tags = v.split(',') - new_tags.push(v) - return dict(_tags=v) - - def _load_invalid_key(self, ds, k, v): - ''' handle any key we do not recognize ''' - - raise AnsibleError("%s is not a legal parameter in an Ansible task or handler" % k) - - def _load_other_valid_key(self, ds, k, v): - ''' handle any other attribute we DO recognize ''' - - results = dict() - k = "_%s" % k - results[k] = v - return results - - def _loader_for_key(self, k): - ''' based on the name of a datastructure element, find the code to handle it ''' - - if k in ('action', 'local_action'): - return self._load_action - elif k in utils.plugins.module_finder: - return self._load_module - elif k.startswith('with_'): - return self._load_loop - elif k == 'changed_when': - return self._load_changed_when - elif k == 'failed_when': - return self._load_failed_when - elif k == 'when': - return self._load_when - elif k == 'tags': - return self._load_tags - elif k not in self.VALID_KEYS: - return self._load_invalid_key - else: - return self._load_other_valid_key - - # ================================================================================== - # PRE-VALIDATION - expected to be uncommonly used, this checks for arguments that - # are aliases of each other. Most everything else should be in the LOAD block - # or the POST-VALIDATE block. - - def _pre_validate(self, ds): - ''' rarely used function to see if the datastructure has items that mean the same thing ''' - - if 'action' in ds and 'local_action' in ds: - raise AnsibleError("the 'action' and 'local_action' attributes can not be used together") - - # ================================================================================= - # POST-VALIDATION: checks for internal inconsistency between fields - # validation can result in an error but also corrections - - def _post_validate(self): - ''' is the loaded datastructure sane? ''' - - if not self._name: - self._name = self._post_validate_fixed_name() - - # incompatible items - self._validate_conflicting_su_and_sudo() - self._validate_conflicting_first_available_file_and_loookup() - - def _post_validate_fixed_name(self): - '' construct a name for the task if no name was specified ''' - - flat_params = " ".join(["%s=%s" % (k,v) for k,v in self._parameters.iteritems()]) - return = "%s %s" % (self._module_name, flat_params) - - def _post_validate_conflicting_su_and_sudo(self): - ''' make sure su/sudo usage doesn't conflict ''' - - conflicting = (self._sudo or self._sudo_user or self._sudo_pass) and (self._su or self._su_user or self._su_pass): - if conflicting: - raise AnsibleError('sudo params ("sudo", "sudo_user", "sudo_pass") and su params ("su", "su_user", "su_pass") cannot be used together') - - def _post_validate_conflicting_first_available_file_and_lookup(self): - ''' first_available_file (deprecated) predates lookup plugins, and cannot be used with those kinds of loops ''' - - if self._first_available_file and self._lookup_plugin: - raise AnsibleError("with_(plugin), and first_available_file are mutually incompatible in a single task") - -"""