diff options
Diffstat (limited to 'src/ansiblelint/rules')
93 files changed, 9775 insertions, 0 deletions
diff --git a/src/ansiblelint/rules/__init__.py b/src/ansiblelint/rules/__init__.py new file mode 100644 index 0000000..acb7df1 --- /dev/null +++ b/src/ansiblelint/rules/__init__.py @@ -0,0 +1,560 @@ +"""All internal ansible-lint rules.""" +from __future__ import annotations + +import copy +import inspect +import logging +import re +import sys +from collections import defaultdict +from collections.abc import Iterable, Iterator, MutableMapping, MutableSequence +from importlib import import_module +from pathlib import Path +from typing import TYPE_CHECKING, Any, cast + +import ansiblelint.skip_utils +import ansiblelint.utils +import ansiblelint.yaml_utils +from ansiblelint._internal.rules import ( + AnsibleParserErrorRule, + BaseRule, + LoadingFailureRule, + RuntimeErrorRule, + WarningRule, +) +from ansiblelint.app import App, get_app +from ansiblelint.config import PROFILES, Options, get_rule_config +from ansiblelint.config import options as default_options +from ansiblelint.constants import LINE_NUMBER_KEY, RULE_DOC_URL, SKIPPED_RULES_KEY +from ansiblelint.errors import MatchError +from ansiblelint.file_utils import Lintable, expand_paths_vars + +if TYPE_CHECKING: + from ruamel.yaml.comments import CommentedMap, CommentedSeq + +_logger = logging.getLogger(__name__) + +match_types = { + "matchlines": "line", + "match": "line", # called by matchlines + "matchtasks": "task", + "matchtask": "task", # called by matchtasks + "matchyaml": "yaml", + "matchplay": "play", # called by matchyaml + "matchdir": "dir", +} + + +class AnsibleLintRule(BaseRule): + """AnsibleLintRule should be used as base for writing new rules.""" + + @property + def url(self) -> str: + """Return rule documentation url.""" + return RULE_DOC_URL + self.id + "/" + + @property + def rule_config(self) -> dict[str, Any]: + """Retrieve rule specific configuration.""" + return get_rule_config(self.id) + + def get_config(self, key: str) -> Any: + """Return a configured value for given key string.""" + return self.rule_config.get(key, None) + + @staticmethod + def unjinja(text: str) -> str: + """Remove jinja2 bits from a string.""" + text = re.sub(r"{{.+?}}", "JINJA_EXPRESSION", text) + text = re.sub(r"{%.+?%}", "JINJA_STATEMENT", text) + text = re.sub(r"{#.+?#}", "JINJA_COMMENT", text) + return text + + # pylint: disable=too-many-arguments + def create_matcherror( + self, + message: str = "", + lineno: int = 1, + details: str = "", + filename: Lintable | None = None, + tag: str = "", + ) -> MatchError: + """Instantiate a new MatchError.""" + match = MatchError( + message=message, + lineno=lineno, + details=details, + lintable=filename or Lintable(""), + rule=copy.copy(self), + tag=tag, + ) + # search through callers to find one of the match* methods + frame = inspect.currentframe() + match_type: str | None = None + while not match_type and frame is not None: + func_name = frame.f_code.co_name + match_type = match_types.get(func_name, None) + if match_type: + # add the match_type to the match + match.match_type = match_type + break + frame = frame.f_back # get the parent frame for the next iteration + return match + + @staticmethod + def _enrich_matcherror_with_task_details( + match: MatchError, + task: ansiblelint.utils.Task, + ) -> None: + match.task = task + if not match.details: + match.details = "Task/Handler: " + ansiblelint.utils.task_to_str(task) + if match.lineno < task[LINE_NUMBER_KEY]: + match.lineno = task[LINE_NUMBER_KEY] + + def matchlines(self, file: Lintable) -> list[MatchError]: + matches: list[MatchError] = [] + # arrays are 0-based, line numbers are 1-based + # so use prev_line_no as the counter + for prev_line_no, line in enumerate(file.content.split("\n")): + if line.lstrip().startswith("#"): + continue + + rule_id_list = ansiblelint.skip_utils.get_rule_skips_from_line( + line, + lintable=file, + ) + if self.id in rule_id_list: + continue + + result = self.match(line) + if not result: + continue + message = "" + if isinstance(result, str): + message = result + matcherror = self.create_matcherror( + message=message, + lineno=prev_line_no + 1, + details=line, + filename=file, + ) + matches.append(matcherror) + return matches + + def matchtasks(self, file: Lintable) -> list[MatchError]: + """Call matchtask for each task inside file and return aggregate results. + + Most rules will never need to override matchtasks because its main + purpose is to call matchtask for each task/handlers in the same file, + and to aggregate the results. + """ + matches: list[MatchError] = [] + if ( + file.kind not in ["handlers", "tasks", "playbook"] + or str(file.base_kind) != "text/yaml" + ): + return matches + + for task in ansiblelint.utils.task_in_list( + data=file.data, + kind=file.kind, + file=file, + ): + if task.error is not None: + # normalize_task converts AnsibleParserError to MatchError + return [task.error] + + if ( + self.id in task.skip_tags + or ("action" not in task.normalized_task) + or "skip_ansible_lint" in task.normalized_task.get("tags", []) + ): + continue + + if self.needs_raw_task: + task.normalized_task["__raw_task__"] = task.raw_task + + result = self.matchtask(task, file=file) + if not result: + continue + + if isinstance(result, Iterable) and not isinstance( + result, + str, + ): # list[MatchError] + # https://github.com/PyCQA/pylint/issues/6044 + # pylint: disable=not-an-iterable + for match in result: + if match.tag in task.skip_tags: + continue + self._enrich_matcherror_with_task_details( + match, + task, + ) + matches.append(match) + continue + if isinstance(result, MatchError): + if result.tag in task.skip_tags: + continue + match = result + else: # bool or string + message = "" + if isinstance(result, str): + message = result + match = self.create_matcherror( + message=message, + lineno=task.normalized_task[LINE_NUMBER_KEY], + filename=file, + ) + + self._enrich_matcherror_with_task_details(match, task) + matches.append(match) + return matches + + def matchyaml(self, file: Lintable) -> list[MatchError]: + matches: list[MatchError] = [] + if str(file.base_kind) != "text/yaml": + return matches + + yaml = file.data + # yaml returned can be an AnsibleUnicode (a string) when the yaml + # file contains a single string. YAML spec allows this but we consider + # this an fatal error. + if isinstance(yaml, str): + if yaml.startswith("$ANSIBLE_VAULT"): + return [] + return [MatchError(lintable=file, rule=LoadingFailureRule())] + if not yaml: + return matches + + if isinstance(yaml, dict): + yaml = [yaml] + + for play in yaml: + # Bug #849 + if play is None: + continue + + if self.id in play.get(SKIPPED_RULES_KEY, ()): + continue + + if "skip_ansible_lint" in play.get("tags", []): + continue + + matches.extend(self.matchplay(file, play)) + + return matches + + +class TransformMixin: + """A mixin for AnsibleLintRule to enable transforming files. + + If ansible-lint is started with the ``--write`` option, then the ``Transformer`` + will call the ``transform()`` method for every MatchError identified if the rule + that identified it subclasses this ``TransformMixin``. Only the rule that identified + a MatchError can do transforms to fix that match. + """ + + def transform( + self, + match: MatchError, + lintable: Lintable, + data: CommentedMap | CommentedSeq | str, + ) -> None: + """Transform ``data`` to try to fix the MatchError identified by this rule. + + The ``match`` was generated by this rule in the ``lintable`` file. + When ``transform()`` is called on a rule, the rule should either fix the + issue, if possible, or make modifications that make it easier to fix manually. + + The transform must set ``match.fixed = True`` when data has been transformed to + fix the error. + + For YAML files, ``data`` is an editable YAML dict/array that preserves + any comments that were in the original file. + + .. code:: python + + data[0]["tasks"][0]["when"] = False + + This is easier with the ``seek()`` utility method: + + .. code :: python + + target_task = self.seek(match.yaml_path, data) + target_task["when"] = False + + For any files that aren't YAML, ``data`` is the loaded file's content as a string. + To edit non-YAML files, save the updated contents in ``lintable.content``: + + .. code:: python + + new_data = self.do_something_to_fix_the_match(data) + lintable.content = new_data + """ + + @staticmethod + def seek( + yaml_path: list[int | str], + data: MutableMapping[str, Any] | MutableSequence[Any] | str, + ) -> Any: + """Get the element identified by ``yaml_path`` in ``data``. + + Rules that work with YAML need to seek, or descend, into nested YAML data + structures to perform the relevant transforms. For example: + + .. code:: python + + def transform(self, match, lintable, data): + target_task = self.seek(match.yaml_path, data) + # transform target_task + """ + if isinstance(data, str): + # can't descend into a string + return data + target = data + for segment in yaml_path: + # The cast() calls tell mypy what types we expect. + # Essentially this does: + if isinstance(segment, str): + target = cast(MutableMapping[str, Any], target)[segment] + elif isinstance(segment, int): + target = cast(MutableSequence[Any], target)[segment] + return target + + +# pylint: disable=too-many-nested-blocks +def load_plugins( + dirs: list[str], +) -> Iterator[AnsibleLintRule]: + """Yield a rule class.""" + + def all_subclasses(cls: type) -> set[type]: + return set(cls.__subclasses__()).union( + [s for c in cls.__subclasses__() for s in all_subclasses(c)], + ) + + orig_sys_path = sys.path.copy() + + for directory in dirs: + if directory not in sys.path: + sys.path.append(str(directory)) + + # load all modules in the directory + for f in Path(directory).glob("*.py"): + if "__" not in f.stem and f.stem not in "conftest": + import_module(f"{f.stem}") + # restore sys.path + sys.path = orig_sys_path + + rules: dict[str, BaseRule] = {} + for rule in all_subclasses(BaseRule): + # we do not return the rules that are not loaded from passed 'directory' + # or rules that do not have a valid id. For example, during testing + # python may load other rule classes, some outside the tested rule + # directories. + if ( + rule.id # type: ignore[attr-defined] + and Path(inspect.getfile(rule)).parent.absolute() + in [Path(x).absolute() for x in dirs] + and issubclass(rule, BaseRule) + and rule.id not in rules + ): + rules[rule.id] = rule() + for rule in rules.values(): # type: ignore[assignment] + if isinstance(rule, AnsibleLintRule) and bool(rule.id): + yield rule + + +class RulesCollection: + """Container for a collection of rules.""" + + def __init__( + self, + rulesdirs: list[str] | list[Path] | None = None, + options: Options | None = None, + profile_name: str | None = None, + *, + conditional: bool = True, + app: App | None = None, + ) -> None: + """Initialize a RulesCollection instance.""" + if options is None: + self.options = copy.deepcopy(default_options) + # When initialized without options argument we want it to always + # be offline as this is done only during testing. + self.options.offline = True + else: + self.options = options + self.profile = [] + self.app = app or get_app(offline=True) + + if profile_name: + self.profile = PROFILES[profile_name] + rulesdirs_str = [] if rulesdirs is None else [str(r) for r in rulesdirs] + self.rulesdirs = expand_paths_vars(rulesdirs_str) + self.rules: list[BaseRule] = [] + # internal rules included in order to expose them for docs as they are + # not directly loaded by our rule loader. + self.rules.extend( + [ + RuntimeErrorRule(), + AnsibleParserErrorRule(), + LoadingFailureRule(), + WarningRule(), + ], + ) + for rule in load_plugins(rulesdirs_str): + self.register(rule, conditional=conditional) + self.rules = sorted(self.rules) + + # When we have a profile we unload some of the rules + # But we do include all rules when listing all rules or tags + if profile_name and not (self.options.list_rules or self.options.list_tags): + filter_rules_with_profile(self.rules, profile_name) + + def register(self, obj: AnsibleLintRule, *, conditional: bool = False) -> None: + """Register a rule.""" + # We skip opt-in rules which were not manually enabled. + # But we do include opt-in rules when listing all rules or tags + obj._collection = self # pylint: disable=protected-access # noqa: SLF001 + if any( + [ + not conditional, + self.profile, # when profile is used we load all rules and filter later + "opt-in" not in obj.tags, + obj.id in self.options.enable_list, + self.options.list_rules, + self.options.list_tags, + ], + ): + self.rules.append(obj) + + def __iter__(self) -> Iterator[BaseRule]: + """Return the iterator over the rules in the RulesCollection.""" + return iter(sorted(self.rules)) + + def alphabetical(self) -> Iterator[BaseRule]: + """Return an iterator over the rules in the RulesCollection in alphabetical order.""" + return iter(sorted(self.rules, key=lambda x: x.id)) + + def __len__(self) -> int: + """Return the length of the RulesCollection data.""" + return len(self.rules) + + def extend(self, more: list[AnsibleLintRule]) -> None: + """Combine rules.""" + self.rules.extend(more) + + def run( + self, + file: Lintable, + tags: set[str] | None = None, + skip_list: list[str] | None = None, + ) -> list[MatchError]: + """Run all the rules against the given lintable.""" + matches: list[MatchError] = [] + if tags is None: + tags = set() + if skip_list is None: + skip_list = [] + + if not file.path.is_dir(): + try: + if file.content is not None: # loads the file content + pass + except (OSError, UnicodeDecodeError) as exc: + return [ + MatchError( + message=str(exc), + lintable=file, + rule=LoadingFailureRule(), + tag=f"{LoadingFailureRule.id}[{exc.__class__.__name__.lower()}]", + ), + ] + + for rule in self.rules: + if rule.id == "syntax-check": + continue + if ( + not tags + or rule.has_dynamic_tags + or not set(rule.tags).union([rule.id]).isdisjoint(tags) + ): + rule_definition = set(rule.tags) + rule_definition.add(rule.id) + if set(rule_definition).isdisjoint(skip_list): + matches.extend(rule.getmatches(file)) + + # some rules can produce matches with tags that are inside our + # skip_list, so we need to cleanse the matches + matches = [m for m in matches if m.tag not in skip_list] + + return matches + + def __repr__(self) -> str: + """Return a RulesCollection instance representation.""" + return "\n".join( + [rule.verbose() for rule in sorted(self.rules, key=lambda x: x.id)], + ) + + def list_tags(self) -> str: + """Return a string with all the tags in the RulesCollection.""" + tag_desc = { + "command-shell": "Specific to use of command and shell modules", + "core": "Related to internal implementation of the linter", + "deprecations": "Indicate use of features that are removed from Ansible", + "experimental": "Newly introduced rules, by default triggering only warnings", + "formatting": "Related to code-style", + "idempotency": "Possible indication that consequent runs would produce different results", + "idiom": "Anti-pattern detected, likely to cause undesired behavior", + "metadata": "Invalid metadata, likely related to galaxy, collections or roles", + "opt-in": "Rules that are not used unless manually added to `enable_list`", + "security": "Rules related o potentially security issues, like exposing credentials", + "syntax": "Related to wrong or deprecated syntax", + "unpredictability": "Warn about code that might not work in a predictable way", + "unskippable": "Indicate a fatal error that cannot be ignored or disabled", + "yaml": "External linter which will also produce its own rule codes", + } + + tags = defaultdict(list) + for rule in self.rules: + # Fail early if a rule does not have any of our required tags + if not set(rule.tags).intersection(tag_desc.keys()): + msg = f"Rule {rule} does not have any of the required tags: {', '.join(tag_desc.keys())}" + raise RuntimeError(msg) + for tag in rule.tags: + for id_ in rule.ids(): + tags[tag].append(id_) + result = "# List of tags and rules they cover\n" + for tag in sorted(tags): + desc = tag_desc.get(tag, None) + if desc: + result += f"{tag}: # {desc}\n" + else: + result += f"{tag}:\n" + for name in sorted(tags[tag]): + result += f" - {name}\n" + return result + + +def filter_rules_with_profile(rule_col: list[BaseRule], profile: str) -> None: + """Unload rules that are not part of the specified profile.""" + included = set() + extends = profile + total_rules = len(rule_col) + while extends: + for rule in PROFILES[extends]["rules"]: + _logger.debug("Activating rule `%s` due to profile `%s`", rule, extends) + included.add(rule) + extends = PROFILES[extends].get("extends", None) + for rule in rule_col.copy(): + if rule.id not in included: + _logger.debug( + "Unloading %s rule due to not being part of %s profile.", + rule.id, + profile, + ) + rule_col.remove(rule) + _logger.debug("%s/%s rules included in the profile", len(rule_col), total_rules) diff --git a/src/ansiblelint/rules/args.md b/src/ansiblelint/rules/args.md new file mode 100644 index 0000000..567d0fd --- /dev/null +++ b/src/ansiblelint/rules/args.md @@ -0,0 +1,91 @@ +# args + +This rule validates if the task arguments conform with the plugin documentation. + +The rule validation will check if the option name is valid and has the correct +value along with conditionals on the options like `mutually_exclusive`, +`required_together`, `required_one_of` and so on. + +For more information see the +[argument spec validator](https://docs.ansible.com/ansible/latest/reference_appendices/module_utils.html#argumentspecvalidator) +topic in the Ansible module utility documentation. + +Possible messages: + +- `args[module]` - missing required arguments: ... +- `args[module]` - missing parameter(s) required by ... + +## Problematic Code + +```yaml +--- +- name: Fixture to validate module options failure scenarios + hosts: localhost + tasks: + - name: Clone content repository + ansible.builtin.git: # <- Required option `repo` is missing. + dest: /home/www + accept_hostkey: true + version: master + update: false + + - name: Enable service httpd and ensure it is not masked + ansible.builtin.systemd: # <- Missing 'name' parameter required by 'enabled'. + enabled: true + masked: false + + - name: Use quiet to avoid verbose output + ansible.builtin.assert: + test: + - my_param <= 100 + - my_param >= 0 + quiet: invalid # <- Value for option `quiet` is invalid. +``` + +## Correct Code + +```yaml +--- +- name: Fixture to validate module options pass scenario + hosts: localhost + tasks: + - name: Clone content repository + ansible.builtin.git: # <- Contains required option `repo`. + repo: https://github.com/ansible/ansible-examples + dest: /home/www + accept_hostkey: true + version: master + update: false + + - name: Enable service httpd and ensure it is not masked + ansible.builtin.systemd: # <- Contains 'name' parameter required by 'enabled'. + name: httpd + enabled: false + masked: false + + - name: Use quiet to avoid verbose output + ansible.builtin.assert: + that: + - my_param <= 100 + - my_param >= 0 + quiet: True # <- Has correct type value for option `quiet` which is boolean. +``` + +## Special cases + +In some complex cases where you are using jinja expressions, the linter may not +able to fully validate all the possible values and report a false positive. The +example below would usually report +`parameters are mutually exclusive: data|file|keyserver|url` but because we +added `# noqa: args[module]` it will just pass. + +```yaml +- name: Add apt keys # noqa: args[module] + become: true + ansible.builtin.apt_key: + url: "{{ zj_item['url'] | default(omit) }}" + data: "{{ zj_item['data'] | default(omit) }}" + loop: "{{ repositories_keys }}" + loop_control: + loop_var: zj_item +``` diff --git a/src/ansiblelint/rules/args.py b/src/ansiblelint/rules/args.py new file mode 100644 index 0000000..2acf32e --- /dev/null +++ b/src/ansiblelint/rules/args.py @@ -0,0 +1,310 @@ +"""Rule definition to validate task options.""" +from __future__ import annotations + +import contextlib +import importlib.util +import io +import json +import logging +import re +import sys +from functools import lru_cache +from typing import TYPE_CHECKING, Any + +# pylint: disable=preferred-module +from unittest import mock +from unittest.mock import patch + +# pylint: disable=reimported +import ansible.module_utils.basic as mock_ansible_module +from ansible.module_utils import basic +from ansible.plugins.loader import PluginLoadContext, module_loader + +from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.rules import AnsibleLintRule, RulesCollection +from ansiblelint.text import has_jinja +from ansiblelint.yaml_utils import clean_json + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +_logger = logging.getLogger(__name__) + +ignored_re = re.compile( + "|".join( # noqa: FLY002 + [ + r"^parameters are mutually exclusive:", + # https://github.com/ansible/ansible-lint/issues/3128 as strings can be jinja + # Do not remove unless you manually test if the original example + # from the bug does not trigger the rule anymore. We were not able + # to add a regression test because it would involve installing this + # collection. Attempts to reproduce same bug with other collections + # failed, even if the message originates from Ansible core. + r"^unable to evaluate string as dictionary$", + ], + ), + flags=re.MULTILINE | re.DOTALL, +) + +workarounds_drop_map = { + # https://github.com/ansible/ansible-lint/issues/3110 + "ansible.builtin.copy": ["decrypt"], + # https://github.com/ansible/ansible-lint/issues/2824#issuecomment-1354337466 + # https://github.com/ansible/ansible-lint/issues/3138 + "ansible.builtin.service": ["daemon_reload", "use"], + # Avoid: Unsupported parameters for (basic.py) module: cmd. Supported parameters include: _raw_params, _uses_shell, argv, chdir, creates, executable, removes, stdin, stdin_add_newline, strip_empty_ends. + "ansible.builtin.command": ["cmd"], + # https://github.com/ansible/ansible-lint/issues/3152 + "ansible.posix.synchronize": ["use_ssh_args"], +} +workarounds_inject_map = { + # https://github.com/ansible/ansible-lint/issues/2824 + "ansible.builtin.async_status": {"_async_dir": "/tmp/ansible-async"}, +} + + +@lru_cache +def load_module(module_name: str) -> PluginLoadContext: + """Load plugin from module name and cache it.""" + return module_loader.find_plugin_with_context(module_name) + + +class ValidationPassedError(Exception): + """Exception to be raised when validation passes.""" + + +class CustomAnsibleModule(basic.AnsibleModule): # type: ignore[misc] + """Mock AnsibleModule class.""" + + def __init__(self, *args: str, **kwargs: str) -> None: + """Initialize AnsibleModule mock.""" + super().__init__(*args, **kwargs) + raise ValidationPassedError + + +class ArgsRule(AnsibleLintRule): + """Validating module arguments.""" + + id = "args" + severity = "HIGH" + description = "Check whether tasks are using correct module options." + tags = ["syntax", "experimental"] + version_added = "v6.10.0" + module_aliases: dict[str, str] = {"block/always/rescue": "block/always/rescue"} + _ids = { + "args[module]": description, + } + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + # pylint: disable=too-many-locals,too-many-return-statements + results: list[MatchError] = [] + module_name = task["action"]["__ansible_module_original__"] + failed_msg = None + + if module_name in self.module_aliases: + return [] + + loaded_module = load_module(module_name) + + # https://github.com/ansible/ansible-lint/issues/3200 + # since "ps1" modules cannot be executed on POSIX platforms, we will + # avoid running this rule for such modules + if isinstance( + loaded_module.plugin_resolved_path, + str, + ) and loaded_module.plugin_resolved_path.endswith(".ps1"): + return [] + + module_args = { + key: value + for key, value in task["action"].items() + if not key.startswith("__") + } + + # Return if 'args' is jinja string + # https://github.com/ansible/ansible-lint/issues/3199 + if ( + "args" in task.raw_task + and isinstance(task.raw_task["args"], str) + and has_jinja(task.raw_task["args"]) + ): + return [] + + if loaded_module.resolved_fqcn in workarounds_inject_map: + module_args.update(workarounds_inject_map[loaded_module.resolved_fqcn]) + if loaded_module.resolved_fqcn in workarounds_drop_map: + for key in workarounds_drop_map[loaded_module.resolved_fqcn]: + if key in module_args: + del module_args[key] + + with mock.patch.object( + mock_ansible_module, + "AnsibleModule", + CustomAnsibleModule, + ): + spec = importlib.util.spec_from_file_location( + name=loaded_module.resolved_fqcn, + location=loaded_module.plugin_resolved_path, + ) + if spec: + assert spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + else: + assert file is not None + _logger.warning( + "Unable to load module %s at %s:%s for options validation", + module_name, + file.filename, + task[LINE_NUMBER_KEY], + ) + return [] + + try: + if not hasattr(module, "main"): + # skip validation for module options that are implemented as action plugin + # as the option values can be changed in action plugin and are not passed + # through `ArgumentSpecValidator` class as in case of modules. + return [] + + with patch.object( + sys, + "argv", + ["", json.dumps({"ANSIBLE_MODULE_ARGS": clean_json(module_args)})], + ): + fio = io.StringIO() + failed_msg = "" + # Warning: avoid running anything while stdout is redirected + # as what happens may be very hard to debug. + with contextlib.redirect_stdout(fio): + # pylint: disable=protected-access + basic._ANSIBLE_ARGS = None # noqa: SLF001 + try: + module.main() + except SystemExit: + failed_msg = fio.getvalue() + if failed_msg: + results.extend( + self._parse_failed_msg(failed_msg, task, module_name, file), + ) + + sanitized_results = self._sanitize_results(results, module_name) + return sanitized_results + except ValidationPassedError: + return [] + + # pylint: disable=unused-argument + def _sanitize_results( + self, + results: list[MatchError], + module_name: str, + ) -> list[MatchError]: + """Remove results that are false positive.""" + sanitized_results = [] + for result in results: + result_msg = result.message + if ignored_re.match(result_msg): + continue + sanitized_results.append(result) + + return sanitized_results + + def _parse_failed_msg( + self, + failed_msg: str, + task: dict[str, Any], + module_name: str, + file: Lintable | None = None, + ) -> list[MatchError]: + """Parse failed message and return list of MatchError.""" + results: list[MatchError] = [] + try: + failed_obj = json.loads(failed_msg) + error_message = failed_obj["msg"] + except json.decoder.JSONDecodeError: + error_message = failed_msg + + option_type_check_error = re.search( + r"argument '(?P<name>.*)' is of type", + error_message, + ) + if option_type_check_error: + # ignore options with templated variable value with type check errors + option_key = option_type_check_error.group("name") + option_value = task["action"][option_key] + if has_jinja(option_value): + _logger.debug( + "Type checking ignored for '%s' option in task '%s' at line %s.", + option_key, + module_name, + task[LINE_NUMBER_KEY], + ) + return results + + value_not_in_choices_error = re.search( + r"value of (?P<name>.*) must be one of:", + error_message, + ) + if value_not_in_choices_error: + # ignore templated value not in allowed choices + choice_key = value_not_in_choices_error.group("name") + choice_value = task["action"][choice_key] + if has_jinja(choice_value): + _logger.debug( + "Value checking ignored for '%s' option in task '%s' at line %s.", + choice_key, + module_name, + task[LINE_NUMBER_KEY], + ) + return results + + results.append( + self.create_matcherror( + message=error_message, + lineno=task[LINE_NUMBER_KEY], + tag="args[module]", + filename=file, + ), + ) + return results + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + import pytest # noqa: TCH002 + + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + def test_args_module_fail(default_rules_collection: RulesCollection) -> None: + """Test rule invalid module options.""" + success = "examples/playbooks/rule-args-module-fail.yml" + results = Runner(success, rules=default_rules_collection).run() + assert len(results) == 5 + assert results[0].tag == "args[module]" + assert "missing required arguments" in results[0].message + assert results[1].tag == "args[module]" + assert "missing parameter(s) required by " in results[1].message + assert results[2].tag == "args[module]" + assert "Unsupported parameters for" in results[2].message + assert results[3].tag == "args[module]" + assert "Unsupported parameters for" in results[3].message + assert results[4].tag == "args[module]" + assert "value of state must be one of" in results[4].message + + def test_args_module_pass( + default_rules_collection: RulesCollection, + caplog: pytest.LogCaptureFixture, + ) -> None: + """Test rule valid module options.""" + success = "examples/playbooks/rule-args-module-pass.yml" + with caplog.at_level(logging.WARNING): + results = Runner(success, rules=default_rules_collection).run() + assert len(results) == 0, results + assert len(caplog.records) == 0, caplog.records diff --git a/src/ansiblelint/rules/avoid_implicit.md b/src/ansiblelint/rules/avoid_implicit.md new file mode 100644 index 0000000..4c3d781 --- /dev/null +++ b/src/ansiblelint/rules/avoid_implicit.md @@ -0,0 +1,37 @@ +# avoid-implicit + +This rule identifies the use of dangerous implicit behaviors, often also +undocumented. + +This rule will produce the following type of error messages: + +- `avoid-implicit[copy-content]` is not a string as [copy](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/copy_module.html#synopsis) + modules also accept these, but without documenting them. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Write file content + ansible.builtin.copy: + content: { "foo": "bar" } # <-- should use explicit jinja template + dest: /tmp/foo.txt +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Write file content + vars: + content: { "foo": "bar" } + ansible.builtin.copy: + content: "{{ content | to_json }}" # explicit better than implicit! + dest: /tmp/foo.txt +``` diff --git a/src/ansiblelint/rules/avoid_implicit.py b/src/ansiblelint/rules/avoid_implicit.py new file mode 100644 index 0000000..8d1fe26 --- /dev/null +++ b/src/ansiblelint/rules/avoid_implicit.py @@ -0,0 +1,61 @@ +"""Implementation of avoid-implicit rule.""" +# https://github.com/ansible/ansible-lint/issues/2501 +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class AvoidImplicitRule(AnsibleLintRule): + """Rule that identifies use of undocumented or discouraged implicit behaviors.""" + + id = "avoid-implicit" + shortdesc = "Avoid implicit behaviors" + description = ( + "Items which are templated should use ``template`` instead of " + "``copy`` with ``content`` to ensure correctness." + ) + severity = "MEDIUM" + tags = ["unpredictability"] + version_added = "v6.8.0" + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + """Confirm if current rule is matching a specific task.""" + if task["action"]["__ansible_module__"] == "copy": + content = task["action"].get("content", "") + if not isinstance(content, str): + return True + return False + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + def test_template_instead_of_copy_positive() -> None: + """Positive test for avoid-implicit.""" + collection = RulesCollection() + collection.register(AvoidImplicitRule()) + success = "examples/playbooks/rule-avoid-implicit-pass.yml" + good_runner = Runner(success, rules=collection) + assert [] == good_runner.run() + + def test_template_instead_of_copy_negative() -> None: + """Negative test for avoid-implicit.""" + collection = RulesCollection() + collection.register(AvoidImplicitRule()) + failure = "examples/playbooks/rule-avoid-implicit-fail.yml" + bad_runner = Runner(failure, rules=collection) + errs = bad_runner.run() + assert len(errs) == 1 diff --git a/src/ansiblelint/rules/command_instead_of_module.md b/src/ansiblelint/rules/command_instead_of_module.md new file mode 100644 index 0000000..a4e69b0 --- /dev/null +++ b/src/ansiblelint/rules/command_instead_of_module.md @@ -0,0 +1,35 @@ +# command-instead-of-module + +This rule will recommend you to use a specific ansible module instead for tasks +that are better served by a module, as these are more reliable, provide better +messaging and usually have additional features like the ability to retry. + +In the unlikely case that the rule triggers false positives, you can disable it +by adding a comment like `# noqa: command-instead-of-module` to the same line. + +You can check the [source](https://github.com/ansible/ansible-lint/blob/main/src/ansiblelint/rules/command_instead_of_module.py) +of the rule for all the known commands that trigger the rule and their allowed +list arguments of exceptions and raise a pull request to improve them. + +## Problematic Code + +```yaml +--- +- name: Update apt cache + hosts: all + tasks: + - name: Run apt-get update + ansible.builtin.command: apt-get update # <-- better to use ansible.builtin.apt module +``` + +## Correct Code + +```yaml +--- +- name: Update apt cache + hosts: all + tasks: + - name: Run apt-get update + ansible.builtin.apt: + update_cache: true +``` diff --git a/src/ansiblelint/rules/command_instead_of_module.py b/src/ansiblelint/rules/command_instead_of_module.py new file mode 100644 index 0000000..068e430 --- /dev/null +++ b/src/ansiblelint/rules/command_instead_of_module.py @@ -0,0 +1,139 @@ +"""Implementation of command-instead-of-module rule.""" +# Copyright (c) 2013-2014 Will Thames <will@thames.id.au> +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. +from __future__ import annotations + +import sys +from pathlib import Path +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.utils import convert_to_boolean, get_first_cmd_arg, get_second_cmd_arg + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class CommandsInsteadOfModulesRule(AnsibleLintRule): + """Using command rather than module.""" + + id = "command-instead-of-module" + description = ( + "Executing a command when there is an Ansible module is generally a bad idea" + ) + severity = "HIGH" + tags = ["command-shell", "idiom"] + version_added = "historic" + + _commands = ["command", "shell"] + _modules = { + "apt-get": "apt-get", + "chkconfig": "service", + "curl": "get_url or uri", + "git": "git", + "hg": "hg", + "letsencrypt": "acme_certificate", + "mktemp": "tempfile", + "mount": "mount", + "patch": "patch", + "rpm": "yum or rpm_key", + "rsync": "synchronize", + "sed": "template, replace or lineinfile", + "service": "service", + "supervisorctl": "supervisorctl", + "svn": "subversion", + "systemctl": "systemd", + "tar": "unarchive", + "unzip": "unarchive", + "wget": "get_url or uri", + "yum": "yum", + } + + _executable_options = { + "git": ["branch", "log", "lfs"], + "systemctl": ["--version", "kill", "set-default", "show-environment", "status"], + "yum": ["clean"], + "rpm": ["--nodeps"], + } + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + if task["action"]["__ansible_module__"] not in self._commands: + return False + + first_cmd_arg = get_first_cmd_arg(task) + second_cmd_arg = get_second_cmd_arg(task) + + if not first_cmd_arg: + return False + + executable = Path(first_cmd_arg).name + + if ( + second_cmd_arg + and executable in self._executable_options + and second_cmd_arg in self._executable_options[executable] + ): + return False + + if executable in self._modules and convert_to_boolean( + task["action"].get("warn", True), + ): + message = "{0} used in place of {1} module" + return message.format(executable, self._modules[executable]) + return False + + +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("file", "expected"), + ( + pytest.param( + "examples/playbooks/rule-command-instead-of-module-pass.yml", + 0, + id="pass", + ), + pytest.param( + "examples/playbooks/rule-command-instead-of-module-fail.yml", + 3, + id="fail", + ), + ), + ) + def test_command_instead_of_module( + default_rules_collection: RulesCollection, + file: str, + expected: int, + ) -> None: + """Validate that rule works as intended.""" + results = Runner(file, rules=default_rules_collection).run() + + for result in results: + assert result.rule.id == CommandsInsteadOfModulesRule.id, result + assert len(results) == expected diff --git a/src/ansiblelint/rules/command_instead_of_shell.md b/src/ansiblelint/rules/command_instead_of_shell.md new file mode 100644 index 0000000..0abf69d --- /dev/null +++ b/src/ansiblelint/rules/command_instead_of_shell.md @@ -0,0 +1,30 @@ +# command-instead-of-shell + +This rule identifies uses of `shell` modules instead of a `command` one when +this is not really needed. Shell is considerably slower than command and should +be avoided unless there is a special need for using shell features, like +environment variable expansion or chaining multiple commands using pipes. + +## Problematic Code + +```yaml +--- +- name: Problematic example + hosts: localhost + tasks: + - name: Echo a message + ansible.builtin.shell: echo hello # <-- command is better in this case + changed_when: false +``` + +## Correct Code + +```yaml +--- +- name: Correct example + hosts: localhost + tasks: + - name: Echo a message + ansible.builtin.command: echo hello + changed_when: false +``` diff --git a/src/ansiblelint/rules/command_instead_of_shell.py b/src/ansiblelint/rules/command_instead_of_shell.py new file mode 100644 index 0000000..346a071 --- /dev/null +++ b/src/ansiblelint/rules/command_instead_of_shell.py @@ -0,0 +1,97 @@ +"""Implementation of command-instead-of-shell rule.""" +# Copyright (c) 2016 Will Thames <will@thames.id.au> +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.utils import get_cmd_args + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class UseCommandInsteadOfShellRule(AnsibleLintRule): + """Use shell only when shell functionality is required.""" + + id = "command-instead-of-shell" + description = ( + "Shell should only be used when piping, redirecting " + "or chaining commands (and Ansible would be preferred " + "for some of those!)" + ) + severity = "HIGH" + tags = ["command-shell", "idiom"] + version_added = "historic" + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + # Use unjinja so that we don't match on jinja filters + # rather than pipes + if task["action"]["__ansible_module__"] in ["shell", "ansible.builtin.shell"]: + # Since Ansible 2.4, the `command` module does not accept setting + # the `executable`. If the user needs to set it, they have to use + # the `shell` module. + if "executable" in task["action"]: + return False + + jinja_stripped_cmd = self.unjinja(get_cmd_args(task)) + return not any(ch in jinja_stripped_cmd for ch in "&|<>;$\n*[]{}?`") + return False + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("file", "expected"), + ( + pytest.param( + "examples/playbooks/rule-command-instead-of-shell-pass.yml", + 0, + id="good", + ), + pytest.param( + "examples/playbooks/rule-command-instead-of-shell-fail.yml", + 3, + id="bad", + ), + ), + ) + def test_rule_command_instead_of_shell( + default_rules_collection: RulesCollection, + file: str, + expected: int, + ) -> None: + """Validate that rule works as intended.""" + results = Runner(file, rules=default_rules_collection).run() + for result in results: + assert result.rule.id == UseCommandInsteadOfShellRule.id, result + assert len(results) == expected diff --git a/src/ansiblelint/rules/conftest.py b/src/ansiblelint/rules/conftest.py new file mode 100644 index 0000000..f4df7a5 --- /dev/null +++ b/src/ansiblelint/rules/conftest.py @@ -0,0 +1,3 @@ +"""Makes pytest fixtures available.""" +# pylint: disable=wildcard-import,unused-wildcard-import +from ansiblelint.testing.fixtures import * # noqa: F403 diff --git a/src/ansiblelint/rules/custom/__init__.py b/src/ansiblelint/rules/custom/__init__.py new file mode 100644 index 0000000..8c3e048 --- /dev/null +++ b/src/ansiblelint/rules/custom/__init__.py @@ -0,0 +1 @@ +"""A placeholder package for putting custom rules under this dir.""" diff --git a/src/ansiblelint/rules/deprecated_bare_vars.md b/src/ansiblelint/rules/deprecated_bare_vars.md new file mode 100644 index 0000000..9e2f15b --- /dev/null +++ b/src/ansiblelint/rules/deprecated_bare_vars.md @@ -0,0 +1,32 @@ +# deprecated-bare-vars + +This rule identifies possible confusing expressions where it is not clear if +a variable or string is to be used and asks for clarification. + +You should either use the full variable syntax ('{{{{ {0} }}}}') or, whenever +possible, convert it to a list of strings. + +## Problematic code + +```yaml +--- +- ansible.builtin.debug: + msg: "{{ item }}" + with_items: foo # <-- deprecated-bare-vars +``` + +## Correct code + +```yaml +--- +# if foo is not really a variable: +- ansible.builtin.debug: + msg: "{{ item }}" + with_items: + - foo + +# if foo is a variable: +- ansible.builtin.debug: + msg: "{{ item }}" + with_items: "{{ foo }}" +``` diff --git a/src/ansiblelint/rules/deprecated_bare_vars.py b/src/ansiblelint/rules/deprecated_bare_vars.py new file mode 100644 index 0000000..1756e92 --- /dev/null +++ b/src/ansiblelint/rules/deprecated_bare_vars.py @@ -0,0 +1,124 @@ +"""Implementation of deprecated-bare-vars rule.""" + +# Copyright (c) 2013-2014 Will Thames <will@thames.id.au> +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. + +from __future__ import annotations + +import os +import sys +from typing import TYPE_CHECKING, Any + +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.text import has_glob, has_jinja + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class UsingBareVariablesIsDeprecatedRule(AnsibleLintRule): + """Using bare variables is deprecated.""" + + id = "deprecated-bare-vars" + description = ( + "Using bare variables is deprecated. Update your " + "playbooks so that the environment value uses the full variable " + "syntax ``{{ your_variable }}``" + ) + severity = "VERY_HIGH" + tags = ["deprecations"] + version_added = "historic" + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + loop_type = next((key for key in task if key.startswith("with_")), None) + if loop_type: + if loop_type in [ + "with_nested", + "with_together", + "with_flattened", + "with_filetree", + "with_community.general.filetree", + ]: + # These loops can either take a list defined directly in the task + # or a variable that is a list itself. When a single variable is used + # we just need to check that one variable, and not iterate over it like + # it's a list. Otherwise, loop through and check all items. + items = task[loop_type] + if not isinstance(items, (list, tuple)): + items = [items] + for var in items: + return self._matchvar(var, task, loop_type) + elif loop_type == "with_subelements": + return self._matchvar(task[loop_type][0], task, loop_type) + elif loop_type in ["with_sequence", "with_ini", "with_inventory_hostnames"]: + pass + else: + return self._matchvar(task[loop_type], task, loop_type) + return False + + def _matchvar( + self, + varstring: str, + task: dict[str, Any], + loop_type: str, + ) -> bool | str: + if isinstance(varstring, str) and not has_jinja(varstring): + valid = loop_type == "with_fileglob" and bool( + has_jinja(varstring) or has_glob(varstring), + ) + + valid |= loop_type == "with_filetree" and bool( + has_jinja(varstring) or varstring.endswith(os.sep), + ) + if not valid: + message = "Possible bare variable '{0}' used in a '{1}' loop. You should use the full variable syntax ('{{{{ {0} }}}}') or convert it to a list if that is not really a variable." + return message.format(task[loop_type], loop_type) + return False + + +if "pytest" in sys.modules: + import pytest + + # pylint: disable=ungrouped-imports + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner + + @pytest.mark.filterwarnings("ignore::ansible_compat.runtime.AnsibleWarning") + def test_use_bare_positive() -> None: + """Positive test for deprecated-bare-vars.""" + collection = RulesCollection() + collection.register(UsingBareVariablesIsDeprecatedRule()) + success = "examples/playbooks/rule-deprecated-bare-vars-pass.yml" + good_runner = Runner(success, rules=collection) + assert [] == good_runner.run() + + def test_use_bare_negative() -> None: + """Negative test for deprecated-bare-vars.""" + collection = RulesCollection() + collection.register(UsingBareVariablesIsDeprecatedRule()) + failure = "examples/playbooks/rule-deprecated-bare-vars-fail.yml" + bad_runner = Runner(failure, rules=collection) + errs = bad_runner.run() + assert len(errs) == 12 diff --git a/src/ansiblelint/rules/deprecated_local_action.md b/src/ansiblelint/rules/deprecated_local_action.md new file mode 100644 index 0000000..c52eb9d --- /dev/null +++ b/src/ansiblelint/rules/deprecated_local_action.md @@ -0,0 +1,21 @@ +# deprecated-local-action + +This rule recommends using `delegate_to: localhost` instead of the +`local_action`. + +## Problematic Code + +```yaml +--- +- name: Task example + local_action: # <-- this is deprecated + module: ansible.builtin.debug +``` + +## Correct Code + +```yaml +- name: Task example + ansible.builtin.debug: + delegate_to: localhost # <-- recommended way to run on localhost +``` diff --git a/src/ansiblelint/rules/deprecated_local_action.py b/src/ansiblelint/rules/deprecated_local_action.py new file mode 100644 index 0000000..fc3e4ff --- /dev/null +++ b/src/ansiblelint/rules/deprecated_local_action.py @@ -0,0 +1,52 @@ +"""Implementation for deprecated-local-action rule.""" +# Copyright (c) 2016, Tsukinowa Inc. <info@tsukinowa.jp> +# Copyright (c) 2018, Ansible Project +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class TaskNoLocalAction(AnsibleLintRule): + """Do not use 'local_action', use 'delegate_to: localhost'.""" + + id = "deprecated-local-action" + description = "Do not use ``local_action``, use ``delegate_to: localhost``" + needs_raw_task = True + severity = "MEDIUM" + tags = ["deprecations"] + version_added = "v4.0.0" + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + """Return matches for a task.""" + raw_task = task["__raw_task__"] + if "local_action" in raw_task: + return True + + return False + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + def test_local_action(default_rules_collection: RulesCollection) -> None: + """Positive test deprecated_local_action.""" + results = Runner( + "examples/playbooks/rule-deprecated-local-action-fail.yml", + rules=default_rules_collection, + ).run() + + assert len(results) == 1 + assert results[0].tag == "deprecated-local-action" diff --git a/src/ansiblelint/rules/deprecated_module.md b/src/ansiblelint/rules/deprecated_module.md new file mode 100644 index 0000000..c05d641 --- /dev/null +++ b/src/ansiblelint/rules/deprecated_module.md @@ -0,0 +1,32 @@ +# deprecated-module + +This rule identifies deprecated modules in playbooks. +You should avoid using deprecated modules because they are not maintained, which can pose a security risk. +Additionally when a module is deprecated it is available temporarily with a plan for future removal. + +Refer to the [Ansible module index](https://docs.ansible.com/ansible/latest/collections/index_module.html) for information about replacements and removal dates for deprecated modules. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Configure VLAN ID + ansible.netcommon.net_vlan: # <- Uses a deprecated module. + vlan_id: 20 +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Configure VLAN ID + dellemc.enterprise_sonic.sonic_vlans: # <- Uses a platform specific module. + config: + - vlan_id: 20 +``` diff --git a/src/ansiblelint/rules/deprecated_module.py b/src/ansiblelint/rules/deprecated_module.py new file mode 100644 index 0000000..03c9361 --- /dev/null +++ b/src/ansiblelint/rules/deprecated_module.py @@ -0,0 +1,78 @@ +"""Implementation of deprecated-module rule.""" +# Copyright (c) 2018, Ansible Project + +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + + +class DeprecatedModuleRule(AnsibleLintRule): + """Deprecated module.""" + + id = "deprecated-module" + description = ( + "These are deprecated modules, some modules are kept " + "temporarily for backwards compatibility but usage is discouraged." + ) + link = "https://docs.ansible.com/ansible/latest/collections/index_module.html" + severity = "HIGH" + tags = ["deprecations"] + version_added = "v4.0.0" + + _modules = [ + "accelerate", + "aos_asn_pool", + "aos_blueprint", + "aos_blueprint_param", + "aos_blueprint_virtnet", + "aos_device", + "aos_external_router", + "aos_ip_pool", + "aos_logical_device", + "aos_logical_device_map", + "aos_login", + "aos_rack_type", + "aos_template", + "azure", + "cl_bond", + "cl_bridge", + "cl_img_install", + "cl_interface", + "cl_interface_policy", + "cl_license", + "cl_ports", + "cs_nic", + "docker", + "ec2_ami_find", + "ec2_ami_search", + "ec2_remote_facts", + "ec2_vpc", + "kubernetes", + "netscaler", + "nxos_ip_interface", + "nxos_mtu", + "nxos_portchannel", + "nxos_switchport", + "oc", + "panos_nat_policy", + "panos_security_policy", + "vsphere_guest", + "win_msi", + "include", + ] + + def matchtask( + self, + task: dict[str, Any], + file: Lintable | None = None, + ) -> bool | str: + module = task["action"]["__ansible_module__"] + if module in self._modules: + message = "{0} {1}" + return message.format(self.shortdesc, module) + return False diff --git a/src/ansiblelint/rules/empty_string_compare.md b/src/ansiblelint/rules/empty_string_compare.md new file mode 100644 index 0000000..c20bc51 --- /dev/null +++ b/src/ansiblelint/rules/empty_string_compare.md @@ -0,0 +1,44 @@ +# empty-string-compare + +This rule checks for empty string comparison in playbooks. +To ensure code clarity you should avoid using empty strings in conditional statements with the `when` clause. + +- Use `when: var | length > 0` instead of `when: var != ""`. +- Use `when: var | length == 0` instead of `when: var == ""`. + +This is an opt-in rule. +You must enable it in your Ansible-lint configuration as follows: + +```yaml +enable_list: + - empty-string-compare +``` + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Shut down + ansible.builtin.command: /sbin/shutdown -t now + when: ansible_os_family == "" # <- Compares with an empty string. + - name: Shut down + ansible.builtin.command: /sbin/shutdown -t now + when: ansible_os_family !="" # <- Compares with an empty string. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Shut down + ansible.builtin.shell: | + /sbin/shutdown -t now + echo $var == + when: ansible_os_family +``` diff --git a/src/ansiblelint/rules/empty_string_compare.py b/src/ansiblelint/rules/empty_string_compare.py new file mode 100644 index 0000000..5c7cafc --- /dev/null +++ b/src/ansiblelint/rules/empty_string_compare.py @@ -0,0 +1,80 @@ +"""Implementation of empty-string-compare rule.""" +# Copyright (c) 2016, Will Thames and contributors +# Copyright (c) 2018, Ansible Project + +from __future__ import annotations + +import re +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.yaml_utils import nested_items_path + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class ComparisonToEmptyStringRule(AnsibleLintRule): + """Don't compare to empty string.""" + + id = "empty-string-compare" + description = ( + 'Use ``when: var|length > 0`` rather than ``when: var != ""`` (or ' + 'conversely ``when: var|length == 0`` rather than ``when: var == ""``)' + ) + severity = "HIGH" + tags = ["idiom", "opt-in"] + version_added = "v4.0.0" + + empty_string_compare = re.compile("[=!]= ?(\"{2}|'{2})") + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + for k, v, _ in nested_items_path(task): + if k == "when": + if isinstance(v, str): + if self.empty_string_compare.search(v): + return True + elif isinstance(v, bool): + pass + else: + for item in v: + if isinstance(item, str) and self.empty_string_compare.search( + item, + ): + return True + + return False + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + def test_rule_empty_string_compare_fail() -> None: + """Test rule matches.""" + rules = RulesCollection() + rules.register(ComparisonToEmptyStringRule()) + results = Runner( + "examples/playbooks/rule-empty-string-compare-fail.yml", + rules=rules, + ).run() + assert len(results) == 3 + for result in results: + assert result.message == ComparisonToEmptyStringRule().shortdesc + + def test_rule_empty_string_compare_pass() -> None: + """Test rule matches.""" + rules = RulesCollection() + rules.register(ComparisonToEmptyStringRule()) + results = Runner( + "examples/playbooks/rule-empty-string-compare-pass.yml", + rules=rules, + ).run() + assert len(results) == 0, results diff --git a/src/ansiblelint/rules/fqcn.md b/src/ansiblelint/rules/fqcn.md new file mode 100644 index 0000000..0165477 --- /dev/null +++ b/src/ansiblelint/rules/fqcn.md @@ -0,0 +1,89 @@ +# fqcn + +This rule checks for fully-qualified collection names (FQCN) in Ansible content. + +Declaring an FQCN ensures that an action uses code from the correct namespace. +This avoids ambiguity and conflicts that can cause operations to fail or produce +unexpected results. + +The `fqcn` rule has the following checks: + +- `fqcn[action]` - Use FQCN for module actions, such ... +- `fqcn[action-core]` - Checks for FQCNs from the `ansible.legacy` or + `ansible.builtin` collection. +- `fqcn[canonical]` - You should use canonical module name ... instead of ... +- [`fqcn[deep]`](#deep-modules) - Checks for deep/nested plugins directory + inside collections. +- `fqcn[keyword]` - Avoid `collections` keyword by using FQCN for all plugins, + modules, roles and playbooks. + +!!! note + + In most cases you should declare the `ansible.builtin` collection for internal Ansible actions. + You should declare the `ansible.legacy` collection if you use local overrides with actions, such with as the ``shell`` module. + +!!! warning + + This rule does not take [`collections` keyword](https://docs.ansible.com/ansible/latest/collections_guide/collections_using_playbooks.html#simplifying-module-names-with-the-collections-keyword) into consideration for resolving content. + The `collections` keyword provided a temporary mechanism transitioning to Ansible 2.9. + You should rewrite any content that uses the `collections:` key and avoid it where possible. + +## Canonical module names + +Canonical module names are also known as **resolved module names** and they are +to be preferred for most cases. Many Ansible modules have multiple aliases and +redirects, as these were created over time while the content was refactored. +Still, all of them do finally resolve to the same module name, but not without +adding some performance overhead. As very old aliases are at some point removed, +it makes to just refresh the content to make it point to the current canonical +name. + +The only exception for using a canonical name is if your code still needs to be +compatible with a very old version of Ansible, one that does not know how to +resolve that name. If you find yourself in such a situation, feel free to add +this rule to the ignored list. + +## Deep modules + +When writing modules, you should avoid nesting them in deep directories, even if +Ansible allows you to do so. Since early 2023, the official guidance, backed by +the core team, is to use a flat directory structure for modules. This ensures +optimal performance. + +Existing collections that still use deep directories can migrate to the flat +structure in a backward-compatible way by adding redirects like in +[this example](https://github.com/ansible-collections/community.general/blob/main/meta/runtime.yml#L227-L233). + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Create an SSH connection + shell: ssh ssh_user@{{ ansible_ssh_host }} # <- Does not use the FQCN for the shell module. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook (1st solution) + hosts: all + tasks: + - name: Create an SSH connection + # Use the FQCN for the legacy shell module and allow local overrides. + ansible.legacy.shell: + ssh ssh_user@{{ ansible_ssh_host }} -o IdentityFile=path/to/my_rsa +``` + +```yaml +--- +- name: Example playbook (2nd solution) + hosts: all + tasks: + - name: Create an SSH connection + # Use the FQCN for the builtin shell module. + ansible.builtin.shell: ssh ssh_user@{{ ansible_ssh_host }} +``` diff --git a/src/ansiblelint/rules/fqcn.py b/src/ansiblelint/rules/fqcn.py new file mode 100644 index 0000000..768fb9e --- /dev/null +++ b/src/ansiblelint/rules/fqcn.py @@ -0,0 +1,284 @@ +"""Rule definition for usage of fully qualified collection names for builtins.""" +from __future__ import annotations + +import logging +import sys +from typing import TYPE_CHECKING, Any + +from ansible.plugins.loader import module_loader + +from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.rules import AnsibleLintRule, TransformMixin + +if TYPE_CHECKING: + from ruamel.yaml.comments import CommentedMap, CommentedSeq + + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +_logger = logging.getLogger(__name__) + +builtins = [ + "add_host", + "apt", + "apt_key", + "apt_repository", + "assemble", + "assert", + "async_status", + "blockinfile", + "command", + "copy", + "cron", + "debconf", + "debug", + "dnf", + "dpkg_selections", + "expect", + "fail", + "fetch", + "file", + "find", + "gather_facts", + "get_url", + "getent", + "git", + "group", + "group_by", + "hostname", + "import_playbook", + "import_role", + "import_tasks", + "include", + "include_role", + "include_tasks", + "include_vars", + "iptables", + "known_hosts", + "lineinfile", + "meta", + "package", + "package_facts", + "pause", + "ping", + "pip", + "raw", + "reboot", + "replace", + "rpm_key", + "script", + "service", + "service_facts", + "set_fact", + "set_stats", + "setup", + "shell", + "slurp", + "stat", + "subversion", + "systemd", + "sysvinit", + "tempfile", + "template", + "unarchive", + "uri", + "user", + "wait_for", + "wait_for_connection", + "yum", + "yum_repository", +] + + +class FQCNBuiltinsRule(AnsibleLintRule, TransformMixin): + """Use FQCN for builtin actions.""" + + id = "fqcn" + severity = "MEDIUM" + description = ( + "Check whether actions are using using full qualified collection names." + ) + tags = ["formatting"] + version_added = "v6.8.0" + module_aliases: dict[str, str] = {"block/always/rescue": "block/always/rescue"} + _ids = { + "fqcn[action-core]": "Use FQCN for builtin module actions", + "fqcn[action]": "Use FQCN for module actions", + "fqcn[canonical]": "You should use canonical module name", + } + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + result = [] + module = task["action"]["__ansible_module_original__"] + + if module not in self.module_aliases: + loaded_module = module_loader.find_plugin_with_context(module) + target = loaded_module.resolved_fqcn + self.module_aliases[module] = target + if target is None: + _logger.warning("Unable to resolve FQCN for module %s", module) + self.module_aliases[module] = module + return [] + if target not in self.module_aliases: + self.module_aliases[target] = target + + if module != self.module_aliases[module]: + module_alias = self.module_aliases[module] + if module_alias.startswith("ansible.builtin"): + legacy_module = module_alias.replace( + "ansible.builtin.", + "ansible.legacy.", + 1, + ) + if module != legacy_module: + result.append( + self.create_matcherror( + message=f"Use FQCN for builtin module actions ({module}).", + details=f"Use `{module_alias}` or `{legacy_module}` instead.", + filename=file, + lineno=task["__line__"], + tag="fqcn[action-core]", + ), + ) + else: + if module.count(".") < 2: + result.append( + self.create_matcherror( + message=f"Use FQCN for module actions, such `{self.module_aliases[module]}`.", + details=f"Action `{module}` is not FQCN.", + filename=file, + lineno=task["__line__"], + tag="fqcn[action]", + ), + ) + # TODO(ssbarnea): Remove the c.g. and c.n. exceptions from here once # noqa: FIX002 + # community team is flattening these. + # https://github.com/ansible-community/community-topics/issues/147 + elif not module.startswith("community.general.") or module.startswith( + "community.network.", + ): + result.append( + self.create_matcherror( + message=f"You should use canonical module name `{self.module_aliases[module]}` instead of `{module}`.", + filename=file, + lineno=task["__line__"], + tag="fqcn[canonical]", + ), + ) + return result + + def matchyaml(self, file: Lintable) -> list[MatchError]: + """Return matches found for a specific YAML text.""" + result = [] + if file.kind == "plugin": + i = file.path.resolve().parts.index("plugins") + plugin_type = file.path.resolve().parts[i : i + 2] + short_path = file.path.resolve().parts[i + 2 :] + if len(short_path) > 1: + result.append( + self.create_matcherror( + message=f"Deep plugins directory is discouraged. Move '{file.path}' directly under '{'/'.join(plugin_type)}' folder.", + tag="fqcn[deep]", + filename=file, + ), + ) + elif file.kind == "playbook": + for play in file.data: + if play is None: + continue + + result.extend(self.matchplay(file, play)) + return result + + def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + if file.kind != "playbook": + return [] + if "collections" in data: + return [ + self.create_matcherror( + message="Avoid `collections` keyword by using FQCN for all plugins, modules, roles and playbooks.", + lineno=data[LINE_NUMBER_KEY], + tag="fqcn[keyword]", + filename=file, + ), + ] + return [] + + def transform( + self, + match: MatchError, + lintable: Lintable, + data: CommentedMap | CommentedSeq | str, + ) -> None: + if match.tag in self.ids(): + target_task = self.seek(match.yaml_path, data) + # Unfortunately, a lot of data about Ansible content gets lost here, you only get a simple dict. + # For now, just parse the error messages for the data about action names etc. and fix this later. + if match.tag == "fqcn[action-core]": + # split at the first bracket, cut off the last bracket and dot + current_action = match.message.split("(")[1][:-2] + # This will always replace builtin modules with "ansible.builtin" versions, not "ansible.legacy". + # The latter is technically more correct in what ansible has executed so far, the former is most likely better understood and more robust. + new_action = match.details.split("`")[1] + elif match.tag == "fqcn[action]": + current_action = match.details.split("`")[1] + new_action = match.message.split("`")[1] + elif match.tag == "fqcn[canonical]": + current_action = match.message.split("`")[3] + new_action = match.message.split("`")[1] + for _ in range(len(target_task)): + k, v = target_task.popitem(False) + target_task[new_action if k == current_action else k] = v + match.fixed = True + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + def test_fqcn_builtin_fail() -> None: + """Test rule matches.""" + collection = RulesCollection() + collection.register(FQCNBuiltinsRule()) + success = "examples/playbooks/rule-fqcn-fail.yml" + results = Runner(success, rules=collection).run() + assert len(results) == 3 + assert results[0].tag == "fqcn[keyword]" + assert "Avoid `collections` keyword" in results[0].message + assert results[1].tag == "fqcn[action-core]" + assert "Use FQCN for builtin module actions" in results[1].message + assert results[2].tag == "fqcn[action]" + assert "Use FQCN for module actions, such" in results[2].message + + def test_fqcn_builtin_pass() -> None: + """Test rule does not match.""" + collection = RulesCollection() + collection.register(FQCNBuiltinsRule()) + success = "examples/playbooks/rule-fqcn-pass.yml" + results = Runner(success, rules=collection).run() + assert len(results) == 0, results + + def test_fqcn_deep_fail() -> None: + """Test rule matches.""" + collection = RulesCollection() + collection.register(FQCNBuiltinsRule()) + failure = "examples/collection/plugins/modules/deep/beta.py" + results = Runner(failure, rules=collection).run() + assert len(results) == 1 + assert results[0].tag == "fqcn[deep]" + assert "Deep plugins directory is discouraged" in results[0].message + + def test_fqcn_deep_pass() -> None: + """Test rule does not match.""" + collection = RulesCollection() + collection.register(FQCNBuiltinsRule()) + success = "examples/collection/plugins/modules/alpha.py" + results = Runner(success, rules=collection).run() + assert len(results) == 0 diff --git a/src/ansiblelint/rules/galaxy.md b/src/ansiblelint/rules/galaxy.md new file mode 100644 index 0000000..61fc5c5 --- /dev/null +++ b/src/ansiblelint/rules/galaxy.md @@ -0,0 +1,111 @@ +# galaxy + +This rule identifies if the collection version mentioned in galaxy.yml is ideal +in terms of the version number being greater than or equal to `1.0.0`. + +This rule looks for a changelog file in expected locations, detailed below in +the Changelog Details section. + +This rule checks to see if the `galaxy.yml` file includes one of the required +tags for certification on Automation Hub. Additional custom tags can be added, +but one or more of these tags must be present for certification. + +The tag list is as follows: `application`, `cloud`,`database`, `infrastructure`, +`linux`, `monitoring`, `networking`, `security`,`storage`, `tools`, `windows`. + +This rule can produce messages such: + +- `galaxy[version-missing]` - `galaxy.yaml` should have version tag. +- `galaxy[version-incorrect]` - collection version should be greater than or + equal to `1.0.0` +- `galaxy[no-changelog]` - collection is missing a changelog file in expected + locations. +- `galaxy[no-runtime]` - Please add a + [meta/runtime.yml](https://docs.ansible.com/ansible/latest/dev_guide/developing_collections_structure.html#meta-directory-and-runtime-yml) + file. +- `galaxy[tags]` - `galaxy.yaml` must have one of the required tags: + `application`, `cloud`, `database`, `infrastructure`, `linux`, `monitoring`, + `networking`, `security`, `storage`, `tools`, `windows`. + +If you want to ignore some of the messages above, you can add any of them to the +`ignore_list`. + +## Problematic code + +```yaml +# galaxy.yml +--- +name: foo +namespace: bar +version: 0.2.3 # <-- collection version should be >= 1.0.0 +authors: + - John +readme: ../README.md +description: "..." +``` + +## Correct code + +```yaml +# galaxy.yml +--- +name: foo +namespace: bar +version: 1.0.0 +authors: + - John +readme: ../README.md +description: "..." +``` + +# Changelog Details + +This rule expects a `CHANGELOG.md` or `.rst` file in the collection root or a +`changelogs/changelog.yaml` file. + +If a `changelogs/changelog.yaml` file exists, the schema will be checked. + +## Minimum required changelog.yaml file + +```yaml +# changelog.yaml +--- +releases: {} +``` + +# Required Tag Details + +## Problematic code + +```yaml +# galaxy.yml +--- +namespace: bar +name: foo +version: 1.0.0 +authors: + - John +readme: ../README.md +description: "..." +license: + - Apache-2.0 +repository: https://github.com/ORG/REPO_NAME +``` + +## Correct code + +```yaml +# galaxy.yml +--- +namespace: bar +name: foo +version: 1.0.0 +authors: + - John +readme: ../README.md +description: "..." +license: + - Apache-2.0 +repository: https://github.com/ORG/REPO_NAME +tags: [networking, test_tag, test_tag_2] +``` diff --git a/src/ansiblelint/rules/galaxy.py b/src/ansiblelint/rules/galaxy.py new file mode 100644 index 0000000..2f627f5 --- /dev/null +++ b/src/ansiblelint/rules/galaxy.py @@ -0,0 +1,251 @@ +"""Implementation of GalaxyRule.""" +from __future__ import annotations + +import sys +from functools import total_ordering +from typing import TYPE_CHECKING, Any + +from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + + +class GalaxyRule(AnsibleLintRule): + """Rule for checking collection version is greater than 1.0.0 and checking for changelog.""" + + id = "galaxy" + description = "Confirm via galaxy.yml file if collection version is greater than or equal to 1.0.0 and check for changelog." + severity = "MEDIUM" + tags = ["metadata"] + version_added = "v6.11.0 (last update)" + _ids = { + "galaxy[tags]": "galaxy.yaml must have one of the required tags", + "galaxy[no-changelog]": "No changelog found. Please add a changelog file. Refer to the galaxy.md file for more info.", + "galaxy[version-missing]": "galaxy.yaml should have version tag.", + "galaxy[version-incorrect]": "collection version should be greater than or equal to 1.0.0", + "galaxy[no-runtime]": "meta/runtime.yml file not found.", + } + + def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + """Return matches found for a specific play (entry in playbook).""" + if file.kind != "galaxy": # type: ignore[comparison-overlap] + return [] + + # Defined by Automation Hub Team and Partner Engineering + required_tag_list = [ + "application", + "cloud", + "database", + "infrastructure", + "linux", + "monitoring", + "networking", + "security", + "storage", + "tools", + "windows", + ] + + results = [] + + base_path = file.path.parent.resolve() + changelog_found = 0 + changelog_paths = [ + base_path / "changelogs" / "changelog.yaml", + base_path / "CHANGELOG.rst", + base_path / "CHANGELOG.md", + ] + + for path in changelog_paths: + if path.is_file(): + changelog_found = 1 + + galaxy_tag_list = data.get("tags", None) + + # Changelog Check - building off Galaxy rule as there is no current way to check + # for a nonexistent file + if not changelog_found: + results.append( + self.create_matcherror( + message="No changelog found. Please add a changelog file. Refer to the galaxy.md file for more info.", + tag="galaxy[no-changelog]", + filename=file, + ), + ) + + # Checking if galaxy.yml contains one or more required tags for certification + if not galaxy_tag_list or not any( + tag in required_tag_list for tag in galaxy_tag_list + ): + results.append( + self.create_matcherror( + message=( + f"galaxy.yaml must have one of the required tags: {required_tag_list}" + ), + tag="galaxy[tags]", + filename=file, + ), + ) + + if "version" not in data: + results.append( + self.create_matcherror( + message="galaxy.yaml should have version tag.", + lineno=data[LINE_NUMBER_KEY], + tag="galaxy[version-missing]", + filename=file, + ), + ) + return results + # returning here as it does not make sense + # to continue for version check below + + version = data.get("version") + if Version(version) < Version("1.0.0"): + results.append( + self.create_matcherror( + message="collection version should be greater than or equal to 1.0.0", + # pylint: disable=protected-access + lineno=version._line_number, # noqa: SLF001 + tag="galaxy[version-incorrect]", + filename=file, + ), + ) + + if not (base_path / "meta" / "runtime.yml").is_file(): + results.append( + self.create_matcherror( + message="meta/runtime.yml file not found.", + tag="galaxy[no-runtime]", + filename=file, + ), + ) + + return results + + +@total_ordering +class Version: + """Simple class to compare arbitrary versions.""" + + def __init__(self, version_string: str): + """Construct a Version object.""" + self.components = version_string.split(".") + + def __eq__(self, other: object) -> bool: + """Implement equality comparison.""" + try: + other = _coerce(other) + except NotImplementedError: + return NotImplemented + + return self.components == other.components + + def __lt__(self, other: Version) -> bool: + """Implement lower-than operation.""" + other = _coerce(other) + + return self.components < other.components + + +def _coerce(other: object) -> Version: + if isinstance(other, str): + other = Version(other) + if isinstance(other, (int, float)): + other = Version(str(other)) + if isinstance(other, Version): + return other + msg = f"Unable to coerce object type {type(other)} to Version" + raise NotImplementedError(msg) + + +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner + + def test_galaxy_collection_version_positive() -> None: + """Positive test for collection version in galaxy.""" + collection = RulesCollection() + collection.register(GalaxyRule()) + success = "examples/collection/galaxy.yml" + good_runner = Runner(success, rules=collection) + assert [] == good_runner.run() + + def test_galaxy_collection_version_negative() -> None: + """Negative test for collection version in galaxy.""" + collection = RulesCollection() + collection.register(GalaxyRule()) + failure = "examples/meta/galaxy.yml" + bad_runner = Runner(failure, rules=collection) + errs = bad_runner.run() + assert len(errs) == 1 + + def test_galaxy_no_collection_version() -> None: + """Test for no collection version in galaxy.""" + collection = RulesCollection() + collection.register(GalaxyRule()) + failure = "examples/no_collection_version/galaxy.yml" + bad_runner = Runner(failure, rules=collection) + errs = bad_runner.run() + assert len(errs) == 1 + + def test_version_class() -> None: + """Test for version class.""" + v = Version("1.0.0") + assert v == Version("1.0.0") + assert v != NotImplemented + + def test_coerce() -> None: + """Test for _coerce function.""" + assert _coerce("1.0") == Version("1.0") + assert _coerce(1.0) == Version("1.0") + expected = "Unable to coerce object type" + with pytest.raises(NotImplementedError, match=expected): + _coerce(type(Version)) + + @pytest.mark.parametrize( + ("file", "expected"), + ( + pytest.param( + "examples/galaxy_no_required_tags/fail/galaxy.yml", + ["galaxy[tags]"], + id="tags", + ), + pytest.param( + "examples/galaxy_no_required_tags/pass/galaxy.yml", + [], + id="pass", + ), + pytest.param( + "examples/collection/galaxy.yml", + ["schema[galaxy]"], + id="schema", + ), + pytest.param( + "examples/no_changelog/galaxy.yml", + ["galaxy[no-changelog]"], + id="no-changelog", + ), + pytest.param( + "examples/no_collection_version/galaxy.yml", + ["schema[galaxy]", "galaxy[version-missing]"], + id="no-collection-version", + ), + ), + ) + def test_galaxy_rule( + default_rules_collection: RulesCollection, + file: str, + expected: list[str], + ) -> None: + """Validate that rule works as intended.""" + results = Runner(file, rules=default_rules_collection).run() + + assert len(results) == len(expected) + for index, result in enumerate(results): + assert result.tag == expected[index] diff --git a/src/ansiblelint/rules/ignore_errors.md b/src/ansiblelint/rules/ignore_errors.md new file mode 100644 index 0000000..cb17774 --- /dev/null +++ b/src/ansiblelint/rules/ignore_errors.md @@ -0,0 +1,61 @@ +# ignore-errors + +This rule checks that playbooks do not use the `ignore_errors` directive to ignore all errors. +Ignoring all errors in a playbook hides actual failures, incorrectly mark tasks as failed, and result in unexpected side effects and behavior. + +Instead of using the `ignore_errors: true` directive, you should do the following: + +- Ignore errors only when using the `{{ ansible_check_mode }}` variable. +- Use `register` to register errors. +- Use `failed_when:` and specify acceptable error conditions. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Run apt-get update + ansible.builtin.command: apt-get update + ignore_errors: true # <- Ignores all errors, including important failures. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Run apt-get update + ansible.builtin.command: apt-get update + ignore_errors: "{{ ansible_check_mode }}" # <- Ignores errors in check mode. +``` + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Run apt-get update + ansible.builtin.command: apt-get update + ignore_errors: true + register: ignore_errors_register # <- Stores errors and failures for evaluation. +``` + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Disable apport + become: "yes" + lineinfile: + line: "enabled=0" + dest: /etc/default/apport + mode: 0644 + state: present + register: default_apport + failed_when: default_apport.rc !=0 and not default_apport.rc == 257 # <- Defines conditions that constitute a failure. +``` diff --git a/src/ansiblelint/rules/ignore_errors.py b/src/ansiblelint/rules/ignore_errors.py new file mode 100644 index 0000000..4144f2d --- /dev/null +++ b/src/ansiblelint/rules/ignore_errors.py @@ -0,0 +1,144 @@ +"""IgnoreErrorsRule used with ansible-lint.""" +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class IgnoreErrorsRule(AnsibleLintRule): + """Use failed_when and specify error conditions instead of using ignore_errors.""" + + id = "ignore-errors" + description = ( + "Instead of ignoring all errors, ignore the errors only when using ``{{ ansible_check_mode }}``, " + "register the errors using ``register``, " + "or use ``failed_when:`` and specify acceptable error conditions " + "to reduce the risk of ignoring important failures." + ) + severity = "LOW" + tags = ["unpredictability"] + version_added = "v5.0.7" + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + if ( + task.get("ignore_errors") + and task.get("ignore_errors") != "{{ ansible_check_mode }}" + and not task.get("register") + ): + return True + + return False + + +if "pytest" in sys.modules: + import pytest + + if TYPE_CHECKING: + from ansiblelint.testing import RunFromText # pylint: disable=ungrouped-imports + + IGNORE_ERRORS_TRUE = """ +- hosts: all + tasks: + - name: Run apt-get update + command: apt-get update + ignore_errors: true +""" + + IGNORE_ERRORS_FALSE = """ +- hosts: all + tasks: + - name: Run apt-get update + command: apt-get update + ignore_errors: false +""" + + IGNORE_ERRORS_CHECK_MODE = """ +- hosts: all + tasks: + - name: Run apt-get update + command: apt-get update + ignore_errors: "{{ ansible_check_mode }}" +""" + + IGNORE_ERRORS_REGISTER = """ +- hosts: all + tasks: + - name: Run apt-get update + command: apt-get update + ignore_errors: true + register: ignore_errors_register +""" + + FAILED_WHEN = """ +- hosts: all + tasks: + - name: Disable apport + become: 'yes' + lineinfile: + line: "enabled=0" + dest: /etc/default/apport + mode: 0644 + state: present + register: default_apport + failed_when: default_apport.rc !=0 and not default_apport.rc == 257 +""" + + @pytest.mark.parametrize( + "rule_runner", + (IgnoreErrorsRule,), + indirect=["rule_runner"], + ) + def test_ignore_errors_true(rule_runner: RunFromText) -> None: + """The task uses ignore_errors.""" + results = rule_runner.run_playbook(IGNORE_ERRORS_TRUE) + assert len(results) == 1 + + @pytest.mark.parametrize( + "rule_runner", + (IgnoreErrorsRule,), + indirect=["rule_runner"], + ) + def test_ignore_errors_false(rule_runner: RunFromText) -> None: + """The task uses ignore_errors: false, oddly enough.""" + results = rule_runner.run_playbook(IGNORE_ERRORS_FALSE) + assert len(results) == 0 + + @pytest.mark.parametrize( + "rule_runner", + (IgnoreErrorsRule,), + indirect=["rule_runner"], + ) + def test_ignore_errors_check_mode(rule_runner: RunFromText) -> None: + """The task uses ignore_errors: "{{ ansible_check_mode }}".""" + results = rule_runner.run_playbook(IGNORE_ERRORS_CHECK_MODE) + assert len(results) == 0 + + @pytest.mark.parametrize( + "rule_runner", + (IgnoreErrorsRule,), + indirect=["rule_runner"], + ) + def test_ignore_errors_register(rule_runner: RunFromText) -> None: + """The task uses ignore_errors: but output is registered and managed.""" + results = rule_runner.run_playbook(IGNORE_ERRORS_REGISTER) + assert len(results) == 0 + + @pytest.mark.parametrize( + "rule_runner", + (IgnoreErrorsRule,), + indirect=["rule_runner"], + ) + def test_failed_when(rule_runner: RunFromText) -> None: + """Instead of ignore_errors, this task uses failed_when.""" + results = rule_runner.run_playbook(FAILED_WHEN) + assert len(results) == 0 diff --git a/src/ansiblelint/rules/inline_env_var.md b/src/ansiblelint/rules/inline_env_var.md new file mode 100644 index 0000000..bc83f7e --- /dev/null +++ b/src/ansiblelint/rules/inline_env_var.md @@ -0,0 +1,38 @@ +# inline-env-var + +This rule checks that playbooks do not set environment variables in the `ansible.builtin.command` module. + +You should set environment variables with the `ansible.builtin.shell` module or the `environment` keyword. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Set environment variable + ansible.builtin.command: MY_ENV_VAR=my_value # <- Sets an environment variable in the command module. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Set environment variable + ansible.builtin.shell: echo $MY_ENV_VAR + environment: + MY_ENV_VAR: my_value # <- Sets an environment variable with the environment keyword. +``` + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Set environment variable + ansible.builtin.shell: MY_ENV_VAR=my_value # <- Sets an environment variable with the shell module. +``` diff --git a/src/ansiblelint/rules/inline_env_var.py b/src/ansiblelint/rules/inline_env_var.py new file mode 100644 index 0000000..f578fb7 --- /dev/null +++ b/src/ansiblelint/rules/inline_env_var.py @@ -0,0 +1,76 @@ +"""Implementation of inside-env-var rule.""" +# Copyright (c) 2016 Will Thames <will@thames.id.au> +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. +from __future__ import annotations + +from typing import TYPE_CHECKING + +from ansiblelint.constants import FILENAME_KEY, LINE_NUMBER_KEY +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.utils import Task, get_first_cmd_arg + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + + +class EnvVarsInCommandRule(AnsibleLintRule): + """Command module does not accept setting environment variables inline.""" + + id = "inline-env-var" + description = ( + "Use ``environment:`` to set environment variables " + "or use ``shell`` module which accepts both" + ) + severity = "VERY_HIGH" + tags = ["command-shell", "idiom"] + version_added = "historic" + + expected_args = [ + "chdir", + "creates", + "executable", + "removes", + "stdin", + "warn", + "stdin_add_newline", + "strip_empty_ends", + "cmd", + "__ansible_module__", + "__ansible_module_original__", + "_raw_params", + LINE_NUMBER_KEY, + FILENAME_KEY, + ] + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + if task["action"]["__ansible_module__"] in ["command"]: + first_cmd_arg = get_first_cmd_arg(task) + if not first_cmd_arg: + return False + + return any( + [arg not in self.expected_args for arg in task["action"]] + + ["=" in first_cmd_arg], + ) + return False diff --git a/src/ansiblelint/rules/jinja.md b/src/ansiblelint/rules/jinja.md new file mode 100644 index 0000000..8e1732e --- /dev/null +++ b/src/ansiblelint/rules/jinja.md @@ -0,0 +1,55 @@ +# jinja + +This rule can report problems related to jinja2 string templates. The current +version can report: + +- `jinja[spacing]` when there are no spaces between variables + and operators, including filters, like `{{ var_name | filter }}`. This + improves readability and makes it less likely to introduce typos. +- `jinja[invalid]` when the jinja2 template is invalid, like `{{ {{ '1' }} }}`, + which would result in a runtime error if you try to use it with Ansible, even + if it does pass the Ansible syntax check. + +As jinja2 syntax is closely following Python one we aim to follow +[black](https://black.readthedocs.io/en/stable/) formatting rules. If you are +curious how black would reformat a small sniped feel free to visit +[online black formatter](https://black.vercel.app/) site. Keep in mind to not +include the entire jinja2 template, so instead of `{{ 1+2==3 }}`, do paste +only `1+2==3`. + +In ansible, `changed_when`, `failed_when`, `until`, `when` are considered to +use implicit jinja2 templating, meaning that they do not require `{{ }}`. Our +rule will suggest the removal of the braces for these fields. + +## Problematic code + +```yaml +--- +- name: Some task + vars: + foo: "{{some|dict2items}}" # <-- jinja[spacing] + bar: "{{ & }}" # <-- jinja[invalid] + when: "{{ foo | bool }}" # <-- jinja[spacing] - 'when' has implicit templating +``` + +## Correct code + +```yaml +--- +- name: Some task + vars: + foo: "{{ some | dict2items }}" + bar: "{{ '&' }}" + when: foo | bool +``` + +## Current limitations + +In its current form, this rule presents the following limitations: + +- Jinja2 blocks that have newlines in them will not be reformatted because we + consider that the user deliberately wanted to format them in a particular way. +- Jinja2 blocks that use tilde as a binary operation are ignored because black + does not support tilde as a binary operator. Example: `{{ a ~ b }}`. +- Jinja2 blocks that use dot notation with numbers are ignored because python + and black do not allow it. Example: `{{ foo.0.bar }}` diff --git a/src/ansiblelint/rules/jinja.py b/src/ansiblelint/rules/jinja.py new file mode 100644 index 0000000..08254bc --- /dev/null +++ b/src/ansiblelint/rules/jinja.py @@ -0,0 +1,740 @@ +"""Rule for checking content of jinja template strings.""" +from __future__ import annotations + +import logging +import re +import sys +from collections import namedtuple +from pathlib import Path +from typing import TYPE_CHECKING, Any + +import black +import jinja2 +from ansible.errors import AnsibleError, AnsibleParserError +from ansible.parsing.yaml.objects import AnsibleUnicode +from jinja2.exceptions import TemplateSyntaxError + +from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.file_utils import Lintable +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.skip_utils import get_rule_skips_from_line +from ansiblelint.text import has_jinja +from ansiblelint.utils import parse_yaml_from_file, template +from ansiblelint.yaml_utils import deannotate, nested_items_path + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.utils import Task + + +_logger = logging.getLogger(__package__) +KEYWORDS_WITH_IMPLICIT_TEMPLATE = ("changed_when", "failed_when", "until", "when") + +Token = namedtuple("Token", "lineno token_type value") + +ignored_re = re.compile( + "|".join( # noqa: FLY002 + [ + r"^Object of type method is not JSON serializable", + r"^Unexpected templating type error occurred on", + r"^obj must be a list of dicts or a nested dict$", + r"^the template file (.*) could not be found for the lookup$", + r"could not locate file in lookup", + r"unable to locate collection", + r"^Error in (.*)is undefined$", + r"^Mandatory variable (.*) not defined.$", + r"is undefined", + r"Unrecognized type <<class 'ansible.template.AnsibleUndefined'>> for (.*) filter <value>$", + # https://github.com/ansible/ansible-lint/issues/3155 + r"^The '(.*)' test expects a dictionary$", + ], + ), + flags=re.MULTILINE | re.DOTALL, +) + + +class JinjaRule(AnsibleLintRule): + """Rule that looks inside jinja2 templates.""" + + id = "jinja" + severity = "LOW" + tags = ["formatting"] + version_added = "v6.5.0" + _ansible_error_re = re.compile( + r"^(?P<error>.*): (?P<detail>.*)\. String: (?P<string>.*)$", + flags=re.MULTILINE, + ) + + env = jinja2.Environment(trim_blocks=False) + _tag2msg = { + "invalid": "Syntax error in jinja2 template: {value}", + "spacing": "Jinja2 spacing could be improved: {value} -> {reformatted}", + } + _ids = { + "jinja[invalid]": "Invalid jinja2 syntax", + "jinja[spacing]": "Jinja2 spacing could be improved", + } + + def _msg(self, tag: str, value: str, reformatted: str) -> str: + """Generate error message.""" + return self._tag2msg[tag].format(value=value, reformatted=reformatted) + + # pylint: disable=too-many-locals + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + result = [] + try: + for key, v, path in nested_items_path( + task, + ignored_keys=("block", "ansible.builtin.block", "ansible.legacy.block"), + ): + if isinstance(v, str): + try: + template( + basedir=file.path.parent if file else Path("."), + value=v, + variables=deannotate(task.get("vars", {})), + fail_on_error=True, # we later decide which ones to ignore or not + ) + # ValueError RepresenterError + except AnsibleError as exc: + bypass = False + orig_exc = ( + exc.orig_exc if getattr(exc, "orig_exc", None) else exc + ) + orig_exc_message = getattr(orig_exc, "message", str(orig_exc)) + match = self._ansible_error_re.match( + getattr(orig_exc, "message", str(orig_exc)), + ) + if ignored_re.search(orig_exc_message) or isinstance( + orig_exc, + AnsibleParserError, + ): + # An unhandled exception occurred while running the lookup plugin 'template'. Error was a <class 'ansible.errors.AnsibleError'>, original message: the template file ... could not be found for the lookup. the template file ... could not be found for the lookup + + # ansible@devel (2.14) new behavior: + # AnsibleError(TemplateSyntaxError): template error while templating string: Could not load "ipwrap": 'Invalid plugin FQCN (ansible.netcommon.ipwrap): unable to locate collection ansible.netcommon'. String: Foo {{ buildset_registry.host | ipwrap }}. Could not load "ipwrap": 'Invalid plugin FQCN (ansible.netcommon.ipwrap): unable to locate collection ansible.netcommon' + bypass = True + elif ( + isinstance(orig_exc, (AnsibleError, TemplateSyntaxError)) + and match + ): + error = match.group("error") + detail = match.group("detail") + if error.startswith( + "template error while templating string", + ): + bypass = False + elif detail.startswith("unable to locate collection"): + _logger.debug("Ignored AnsibleError: %s", exc) + bypass = True + else: + bypass = False + elif re.match(r"^lookup plugin (.*) not found$", exc.message): + # lookup plugin 'template' not found + bypass = True + + # AnsibleError: template error while templating string: expected token ':', got '}'. String: {{ {{ '1' }} }} + # AnsibleError: template error while templating string: unable to locate collection ansible.netcommon. String: Foo {{ buildset_registry.host | ipwrap }} + if not bypass: + result.append( + self.create_matcherror( + message=str(exc), + lineno=_get_error_line(task, path), + filename=file, + tag=f"{self.id}[invalid]", + ), + ) + continue + reformatted, details, tag = self.check_whitespace( + v, + key=key, + lintable=file, + ) + if reformatted != v: + result.append( + self.create_matcherror( + message=self._msg( + tag=tag, + value=v, + reformatted=reformatted, + ), + lineno=_get_error_line(task, path), + details=details, + filename=file, + tag=f"{self.id}[{tag}]", + ), + ) + except Exception as exc: + _logger.info("Exception in JinjaRule.matchtask: %s", exc) + raise + return result + + def matchyaml(self, file: Lintable) -> list[MatchError]: + """Return matches for variables defined in vars files.""" + data: dict[str, Any] = {} + raw_results: list[MatchError] = [] + results: list[MatchError] = [] + + if str(file.kind) == "vars": + data = parse_yaml_from_file(str(file.path)) + # pylint: disable=unused-variable + for key, v, _path in nested_items_path(data): + if isinstance(v, AnsibleUnicode): + reformatted, details, tag = self.check_whitespace( + v, + key=key, + lintable=file, + ) + if reformatted != v: + results.append( + self.create_matcherror( + message=self._msg( + tag=tag, + value=v, + reformatted=reformatted, + ), + lineno=v.ansible_pos[1], + details=details, + filename=file, + tag=f"{self.id}[{tag}]", + ), + ) + if raw_results: + lines = file.content.splitlines() + for match in raw_results: + # lineno starts with 1, not zero + skip_list = get_rule_skips_from_line( + line=lines[match.lineno - 1], + lintable=file, + ) + if match.rule.id not in skip_list and match.tag not in skip_list: + results.append(match) + else: + results.extend(super().matchyaml(file)) + return results + + def lex(self, text: str) -> list[Token]: + """Parse jinja template.""" + # https://github.com/pallets/jinja/issues/1711 + self.env.keep_trailing_newline = True + + self.env.lstrip_blocks = False + self.env.trim_blocks = False + self.env.autoescape = True + self.env.newline_sequence = "\n" + tokens = [ + Token(lineno=t[0], token_type=t[1], value=t[2]) for t in self.env.lex(text) + ] + new_text = self.unlex(tokens) + if text != new_text: + _logger.debug( + "Unable to perform full roundtrip lex-unlex on jinja template (expected when '-' modifier is used): {text} -> {new_text}", + ) + return tokens + + def unlex(self, tokens: list[Token]) -> str: + """Return original text by compiling the lex output.""" + result = "" + last_lineno = 1 + last_value = "" + for lineno, _, value in tokens: + if lineno > last_lineno and "\n" not in last_value: + result += "\n" + result += value + last_lineno = lineno + last_value = value + return result + + # pylint: disable=too-many-statements,too-many-locals + def check_whitespace( + self, + text: str, + key: str, + lintable: Lintable | None = None, + ) -> tuple[str, str, str]: + """Check spacing inside given jinja2 template string. + + We aim to match Python Black formatting rules. + :raises NotImplementedError: On few cases where valid jinja is not valid Python. + + :returns: (string, string, string) reformatted text, detailed error, error tag + """ + + def cook(value: str, *, implicit: bool = False) -> str: + """Prepare an implicit string for jinja parsing when needed.""" + if not implicit: + return value + if value.startswith("{{") and value.endswith("}}"): + # maybe we should make this an error? + return value + return f"{{{{ {value} }}}}" + + def uncook(value: str, *, implicit: bool = False) -> str: + """Restore an string to original form when it was an implicit one.""" + if not implicit: + return value + return value[3:-3] + + tokens = [] + details = "" + begin_types = ("variable_begin", "comment_begin", "block_begin") + end_types = ("variable_end", "comment_end", "block_end") + implicit = False + + # implicit templates do not have the {{ }} wrapping + if ( + key in KEYWORDS_WITH_IMPLICIT_TEMPLATE + and lintable + and lintable.kind + in ( + "playbook", + "task", + ) + ): + implicit = True + text = cook(text, implicit=implicit) + + # don't try to lex strings that have no jinja inside them + if not has_jinja(text): + return text, "", "spacing" + + expr_str = None + expr_type = None + verb_skipped = True + lineno = 1 + try: + for token in self.lex(text): + if ( + expr_type + and expr_type.startswith("{%") + and token.token_type in ("name", "whitespace") + and not verb_skipped + ): + # on {% blocks we do not take first word as part of the expression + tokens.append(token) + if token.token_type != "whitespace": + verb_skipped = True + elif token.token_type in begin_types: + tokens.append(token) + expr_type = token.value # such {#, {{, {% + expr_str = "" + verb_skipped = False + elif token.token_type in end_types and expr_str is not None: + # process expression + # pylint: disable=unsupported-membership-test + if isinstance(expr_str, str) and "\n" in expr_str: + raise NotImplementedError + leading_spaces = " " * (len(expr_str) - len(expr_str.lstrip())) + expr_str = leading_spaces + blacken(expr_str.lstrip()) + if tokens[ + -1 + ].token_type != "whitespace" and not expr_str.startswith(" "): + expr_str = " " + expr_str + if not expr_str.endswith(" "): + expr_str += " " + tokens.append(Token(lineno, "data", expr_str)) + tokens.append(token) + expr_str = None + expr_type = None + elif expr_str is not None: + expr_str += token.value + else: + tokens.append(token) + lineno = token.lineno + + except jinja2.exceptions.TemplateSyntaxError as exc: + return "", str(exc.message), "invalid" + # https://github.com/PyCQA/pylint/issues/7433 - py311 only + # pylint: disable=c-extension-no-member + except (NotImplementedError, black.parsing.InvalidInput) as exc: + # black is not able to recognize all valid jinja2 templates, so we + # just ignore InvalidInput errors. + # NotImplementedError is raised internally for expressions with + # newlines, as we decided to not touch them yet. + # These both are documented as known limitations. + _logger.debug("Ignored jinja internal error %s", exc) + return uncook(text, implicit=implicit), "", "spacing" + + # finalize + reformatted = self.unlex(tokens) + failed = reformatted != text + reformatted = uncook(reformatted, implicit=implicit) + details = ( + f"Jinja2 template rewrite recommendation: `{reformatted}`." + if failed + else "" + ) + return reformatted, details, "spacing" + + +def blacken(text: str) -> str: + """Format Jinja2 template using black.""" + return black.format_str( + text, + mode=black.FileMode(line_length=sys.maxsize, string_normalization=False), + ).rstrip("\n") + + +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.fixture(name="error_expected_lines") + def fixture_error_expected_lines() -> list[int]: + """Return list of expected error lines.""" + return [33, 36, 39, 42, 45, 48, 74] + + # 21 68 + @pytest.fixture(name="lint_error_lines") + def fixture_lint_error_lines() -> list[int]: + """Get VarHasSpacesRules linting results on test_playbook.""" + collection = RulesCollection() + collection.register(JinjaRule()) + lintable = Lintable("examples/playbooks/jinja-spacing.yml") + results = Runner(lintable, rules=collection).run() + return [item.lineno for item in results] + + def test_jinja_spacing_playbook( + error_expected_lines: list[int], + lint_error_lines: list[int], + ) -> None: + """Ensure that expected error lines are matching found linting error lines.""" + # list unexpected error lines or non-matching error lines + error_lines_difference = list( + set(error_expected_lines).symmetric_difference(set(lint_error_lines)), + ) + assert len(error_lines_difference) == 0 + + def test_jinja_spacing_vars() -> None: + """Ensure that expected error details are matching found linting error details.""" + collection = RulesCollection() + collection.register(JinjaRule()) + lintable = Lintable("examples/playbooks/vars/jinja-spacing.yml") + results = Runner(lintable, rules=collection).run() + + error_expected_lineno = [14, 15, 16, 17, 18, 19, 32] + assert len(results) == len(error_expected_lineno) + for idx, err in enumerate(results): + assert err.lineno == error_expected_lineno[idx] + + @pytest.mark.parametrize( + ("text", "expected", "tag"), + ( + pytest.param( + "{{-x}}{#a#}{%1%}", + "{{- x }}{# a #}{% 1 %}", + "spacing", + id="add-missing-space", + ), + pytest.param("", "", "spacing", id="1"), + pytest.param("foo", "foo", "spacing", id="2"), + pytest.param("{##}", "{# #}", "spacing", id="3"), + # we want to keep leading spaces as they might be needed for complex multiline jinja files + pytest.param("{# #}", "{# #}", "spacing", id="4"), + pytest.param( + "{{-aaa|xx }}foo\nbar{#some#}\n{%%}", + "{{- aaa | xx }}foo\nbar{# some #}\n{% %}", + "spacing", + id="5", + ), + pytest.param( + "Shell with jinja filter", + "Shell with jinja filter", + "spacing", + id="6", + ), + pytest.param( + "{{{'dummy_2':1}|true}}", + "{{ {'dummy_2': 1} | true }}", + "spacing", + id="7", + ), + pytest.param("{{{foo:{}}}}", "{{ {foo: {}} }}", "spacing", id="8"), + pytest.param( + "{{ {'test': {'subtest': variable}} }}", + "{{ {'test': {'subtest': variable}} }}", + "spacing", + id="9", + ), + pytest.param( + "http://foo.com/{{\n case1 }}", + "http://foo.com/{{\n case1 }}", + "spacing", + id="10", + ), + pytest.param("{{foo(123)}}", "{{ foo(123) }}", "spacing", id="11"), + pytest.param("{{ foo(a.b.c) }}", "{{ foo(a.b.c) }}", "spacing", id="12"), + # pytest.param( + # "spacing", + # ), + pytest.param( + "{{foo(x =['server_options'])}}", + "{{ foo(x=['server_options']) }}", + "spacing", + id="14", + ), + pytest.param( + '{{ [ "host", "NA"] }}', + '{{ ["host", "NA"] }}', + "spacing", + id="15", + ), + pytest.param( + "{{ {'dummy_2': {'nested_dummy_1': value_1,\n 'nested_dummy_2': value_2}} |\ncombine(dummy_1) }}", + "{{ {'dummy_2': {'nested_dummy_1': value_1,\n 'nested_dummy_2': value_2}} |\ncombine(dummy_1) }}", + "spacing", + id="17", + ), + pytest.param("{{ & }}", "", "invalid", id="18"), + pytest.param( + "{{ good_format }}/\n{{- good_format }}\n{{- good_format -}}\n", + "{{ good_format }}/\n{{- good_format }}\n{{- good_format -}}\n", + "spacing", + id="19", + ), + pytest.param( + "{{ {'a': {'b': 'x', 'c': y}} }}", + "{{ {'a': {'b': 'x', 'c': y}} }}", + "spacing", + id="20", + ), + pytest.param( + "2*(1+(3-1)) is {{ 2 * {{ 1 + {{ 3 - 1 }}}} }}", + "2*(1+(3-1)) is {{ 2 * {{1 + {{3 - 1}}}} }}", + "spacing", + id="21", + ), + pytest.param( + '{{ "absent"\nif (v is version("2.8.0", ">=")\nelse "present" }}', + "", + "invalid", + id="22", + ), + pytest.param( + '{{lookup("x",y+"/foo/"+z+".txt")}}', + '{{ lookup("x", y + "/foo/" + z + ".txt") }}', + "spacing", + id="23", + ), + pytest.param( + "{{ x | map(attribute='value') }}", + "{{ x | map(attribute='value') }}", + "spacing", + id="24", + ), + pytest.param( + "{{ r(a= 1,b= True,c= 0.0,d= '') }}", + "{{ r(a=1, b=True, c=0.0, d='') }}", + "spacing", + id="25", + ), + pytest.param("{{ r(1,[]) }}", "{{ r(1, []) }}", "spacing", id="26"), + pytest.param( + "{{ lookup([ddd ]) }}", + "{{ lookup([ddd]) }}", + "spacing", + id="27", + ), + pytest.param( + "{{ [ x ] if x is string else x }}", + "{{ [x] if x is string else x }}", + "spacing", + id="28", + ), + pytest.param( + "{% if a|int <= 8 -%} iptables {%- else -%} iptables-nft {%- endif %}", + "{% if a | int <= 8 -%} iptables{%- else -%} iptables-nft{%- endif %}", + "spacing", + id="29", + ), + pytest.param( + # "- 2" -> "-2", minus does not get separated when there is no left side + "{{ - 2 }}", + "{{ -2 }}", + "spacing", + id="30", + ), + pytest.param( + # "-2" -> "-2", minus does get an undesired spacing + "{{ -2 }}", + "{{ -2 }}", + "spacing", + id="31", + ), + pytest.param( + # array ranges do not have space added + "{{ foo[2:4] }}", + "{{ foo[2:4] }}", + "spacing", + id="32", + ), + pytest.param( + # array ranges have the extra space removed + "{{ foo[2: 4] }}", + "{{ foo[2:4] }}", + "spacing", + id="33", + ), + pytest.param( + # negative array index + "{{ foo[-1] }}", + "{{ foo[-1] }}", + "spacing", + id="34", + ), + pytest.param( + # negative array index, repair + "{{ foo[- 1] }}", + "{{ foo[-1] }}", + "spacing", + id="35", + ), + pytest.param("{{ a +~'b' }}", "{{ a + ~'b' }}", "spacing", id="36"), + pytest.param( + "{{ (a[: -4] *~ b) }}", + "{{ (a[:-4] * ~b) }}", + "spacing", + id="37", + ), + pytest.param("{{ [a,~ b] }}", "{{ [a, ~b] }}", "spacing", id="38"), + # Not supported yet due to being accepted by black: + pytest.param("{{ item.0.user }}", "{{ item.0.user }}", "spacing", id="39"), + # Not supported by back, while jinja allows ~ to be binary operator: + pytest.param("{{ a ~ b }}", "{{ a ~ b }}", "spacing", id="40"), + pytest.param( + "--format='{{'{{'}}.Size{{'}}'}}'", + "--format='{{ '{{' }}.Size{{ '}}' }}'", + "spacing", + id="41", + ), + pytest.param( + "{{ list_one + {{ list_two | max }} }}", + "{{ list_one + {{list_two | max}} }}", + "spacing", + id="42", + ), + pytest.param( + "{{ lookup('file' , '/tmp/non-existent', errors='ignore') }}", + "{{ lookup('file', '/tmp/non-existent', errors='ignore') }}", + "spacing", + id="43", + ), + # https://github.com/ansible/ansible-lint/pull/3057 + # since jinja 3.0.0, \r is converted to \n if the string has jinja in it + pytest.param( + "{{ 'foo' }}\r{{ 'bar' }}", + "{{ 'foo' }}\n{{ 'bar' }}", + "spacing", + id="44", + ), + # if we do not have any jinja constructs, we should keep original \r + # to match ansible behavior + pytest.param( + "foo\rbar", + "foo\rbar", + "spacing", + id="45", + ), + ), + ) + def test_jinja(text: str, expected: str, tag: str) -> None: + """Tests our ability to spot spacing errors inside jinja2 templates.""" + rule = JinjaRule() + + reformatted, details, returned_tag = rule.check_whitespace( + text, + key="name", + lintable=Lintable("playbook.yml"), + ) + assert tag == returned_tag, details + assert expected == reformatted + + @pytest.mark.parametrize( + ("text", "expected", "tag"), + ( + pytest.param( + "1+2", + "1 + 2", + "spacing", + id="0", + ), + pytest.param( + "- 1", + "-1", + "spacing", + id="1", + ), + # Ensure that we do not choke with double templating on implicit + # and instead we remove them braces. + pytest.param("{{ o | bool }}", "o | bool", "spacing", id="2"), + ), + ) + def test_jinja_implicit(text: str, expected: str, tag: str) -> None: + """Tests our ability to spot spacing errors implicit jinja2 templates.""" + rule = JinjaRule() + # implicit jinja2 are working only inside playbooks and tasks + lintable = Lintable(name="playbook.yml", kind="playbook") + reformatted, details, returned_tag = rule.check_whitespace( + text, + key="when", + lintable=lintable, + ) + assert tag == returned_tag, details + assert expected == reformatted + + @pytest.mark.parametrize( + ("lintable", "matches"), + (pytest.param("examples/playbooks/vars/rule_jinja_vars.yml", 0, id="0"),), + ) + def test_jinja_file(lintable: str, matches: int) -> None: + """Tests our ability to process var filesspot spacing errors.""" + collection = RulesCollection() + collection.register(JinjaRule()) + errs = Runner(lintable, rules=collection).run() + assert len(errs) == matches + for err in errs: + assert isinstance(err, JinjaRule) + assert errs[0].tag == "jinja[invalid]" + assert errs[0].rule.id == "jinja" + + def test_jinja_invalid() -> None: + """Tests our ability to spot spacing errors inside jinja2 templates.""" + collection = RulesCollection() + collection.register(JinjaRule()) + success = "examples/playbooks/rule-jinja-fail.yml" + errs = Runner(success, rules=collection).run() + assert len(errs) == 2 + assert errs[0].tag == "jinja[spacing]" + assert errs[0].rule.id == "jinja" + assert errs[0].lineno == 9 + assert errs[1].tag == "jinja[invalid]" + assert errs[1].rule.id == "jinja" + assert errs[1].lineno == 9 + + def test_jinja_valid() -> None: + """Tests our ability to parse jinja, even when variables may not be defined.""" + collection = RulesCollection() + collection.register(JinjaRule()) + success = "examples/playbooks/rule-jinja-pass.yml" + errs = Runner(success, rules=collection).run() + assert len(errs) == 0 + + +def _get_error_line(task: dict[str, Any], path: list[str | int]) -> int: + """Return error line number.""" + line = task[LINE_NUMBER_KEY] + ctx = task + for _ in path: + ctx = ctx[_] + if LINE_NUMBER_KEY in ctx: + line = ctx[LINE_NUMBER_KEY] + if not isinstance(line, int): + msg = "Line number is not an integer" + raise RuntimeError(msg) + return line diff --git a/src/ansiblelint/rules/key_order.md b/src/ansiblelint/rules/key_order.md new file mode 100644 index 0000000..378d8a5 --- /dev/null +++ b/src/ansiblelint/rules/key_order.md @@ -0,0 +1,63 @@ +# key-order + +This rule recommends reordering key names in ansible content to make +code easier to maintain and less prone to errors. + +Here are some examples of common ordering checks done for tasks and handlers: + +- `name` must always be the first key for plays, tasks and handlers +- on tasks, the `block`, `rescue` and `always` keys must be the last keys, + as this would avoid accidental miss-indentation errors between the last task + and the parent level. + +## Problematic code + +```yaml +--- +- hosts: localhost + name: This is a playbook # <-- name key should be the first one + tasks: + - name: A block + block: + - name: Display a message + debug: + msg: "Hello world!" + when: true # <-- when key should be before block +``` + +## Correct code + +```yaml +--- +- name: This is a playbook + hosts: localhost + tasks: + - name: A block + when: true + block: + - name: Display a message + debug: + msg: "Hello world!" +``` + +## Reasoning + +Making decisions about the optimal order of keys for ansible tasks or plays is +no easy task, as we had a huge number of combinations to consider. This is also +the reason why we started with a minimal sorting rule (name to be the first), +and aimed to gradually add more fields later, and only when we find the proofs +that one approach is likely better than the other. + +### Why I no longer can put `when` after a `block`? + +Try to remember that in real life, `block/rescue/always` have the habit to +grow due to the number of tasks they host inside, making them exceed what a single screen. This would move the `when` task further away from the rest of the task properties. A `when` from the last task inside the block can +easily be confused as being at the block level, or the reverse. When tasks are +moved from one location to another, there is a real risk of moving the block +level when with it. + +By putting the `when` before the `block`, we avoid that kind of risk. The same risk applies to any simple property at the task level, so that is why +we concluded that the block keys must be the last ones. + +Another common practice was to put `tags` as the last property. Still, for the +same reasons, we decided that they should not be put after block keys either. diff --git a/src/ansiblelint/rules/key_order.py b/src/ansiblelint/rules/key_order.py new file mode 100644 index 0000000..897da64 --- /dev/null +++ b/src/ansiblelint/rules/key_order.py @@ -0,0 +1,151 @@ +"""All tasks should be have name come first.""" +from __future__ import annotations + +import functools +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +SORTER_TASKS = ( + "name", + # "__module__", + # "action", + # "args", + None, # <-- None include all modules that not using action and * + # "when", + # "notify", + # "tags", + "block", + "rescue", + "always", +) + + +def get_property_sort_index(name: str) -> int: + """Return the index of the property in the sorter.""" + a_index = -1 + for i, v in enumerate(SORTER_TASKS): + if v == name: + return i + if v is None: + a_index = i + return a_index + + +def task_property_sorter(property1: str, property2: str) -> int: + """Sort task properties based on SORTER.""" + v_1 = get_property_sort_index(property1) + v_2 = get_property_sort_index(property2) + return (v_1 > v_2) - (v_1 < v_2) + + +class KeyOrderRule(AnsibleLintRule): + """Ensure specific order of keys in mappings.""" + + id = "key-order" + shortdesc = __doc__ + severity = "LOW" + tags = ["formatting"] + version_added = "v6.6.2" + needs_raw_task = True + _ids = { + "key-order[task]": "You can improve the task key order", + } + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + result = [] + raw_task = task["__raw_task__"] + keys = [key for key in raw_task if not key.startswith("_")] + sorted_keys = sorted(keys, key=functools.cmp_to_key(task_property_sorter)) + if keys != sorted_keys: + result.append( + self.create_matcherror( + f"You can improve the task key order to: {', '.join(sorted_keys)}", + filename=file, + tag="key-order[task]", + ), + ) + return result + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("test_file", "failures"), + ( + pytest.param("examples/playbooks/rule-key-order-pass.yml", 0, id="pass"), + pytest.param("examples/playbooks/rule-key-order-fail.yml", 6, id="fail"), + ), + ) + def test_key_order_rule( + default_rules_collection: RulesCollection, + test_file: str, + failures: int, + ) -> None: + """Test rule matches.""" + results = Runner(test_file, rules=default_rules_collection).run() + assert len(results) == failures + for result in results: + assert result.rule.id == "key-order" + + @pytest.mark.parametrize( + ("properties", "expected"), + ( + pytest.param([], []), + pytest.param(["block", "name"], ["name", "block"]), + pytest.param( + ["block", "name", "action", "..."], + ["name", "action", "...", "block"], + ), + ), + ) + def test_key_order_property_sorter( + properties: list[str], + expected: list[str], + ) -> None: + """Test the task property sorter.""" + result = sorted(properties, key=functools.cmp_to_key(task_property_sorter)) + assert expected == result + + @pytest.mark.parametrize( + ("key", "order"), + ( + pytest.param("name", 0), + pytest.param("action", 1), + pytest.param("foobar", SORTER_TASKS.index(None)), + pytest.param("block", len(SORTER_TASKS) - 3), + pytest.param("rescue", len(SORTER_TASKS) - 2), + pytest.param("always", len(SORTER_TASKS) - 1), + ), + ) + def test_key_order_property_sort_index(key: str, order: int) -> None: + """Test sorting index.""" + assert get_property_sort_index(key) == order + + @pytest.mark.parametrize( + ("prop1", "prop2", "result"), + ( + pytest.param("name", "block", -1), + pytest.param("block", "name", 1), + pytest.param("block", "block", 0), + ), + ) + def test_key_order_property_sortfunc(prop1: str, prop2: str, result: int) -> None: + """Test sorting function.""" + assert task_property_sorter(prop1, prop2) == result diff --git a/src/ansiblelint/rules/latest.md b/src/ansiblelint/rules/latest.md new file mode 100644 index 0000000..1b20432 --- /dev/null +++ b/src/ansiblelint/rules/latest.md @@ -0,0 +1,43 @@ +# latest + +The `latest` rule checks that module arguments like those used for source +control checkout do not have arguments that might generate different results +based on context. + +This more generic rule replaced two older rules named `git-latest` and +`hg-latest`. + +We are aware that there are genuine cases where getting the tip of the main +branch is not accidental. For these cases, just add a comment such as +`# noqa: latest` to the same line to prevent it from triggering. + +## Possible errors messages: + +- `latest[git]` +- `latest[hg]` + +## Problematic code + +```yaml +--- +- name: Example for `latest` rule + hosts: localhost + tasks: + - name: Risky use of git module + ansible.builtin.git: + repo: "https://github.com/ansible/ansible-lint" + version: HEAD # <-- HEAD value is triggering the rule +``` + +## Correct code + +```yaml +--- +- name: Example for `latest` rule + hosts: localhost + tasks: + - name: Safe use of git module + ansible.builtin.git: + repo: "https://github.com/ansible/ansible-lint" + version: abcd1234... # <-- that is safe +``` diff --git a/src/ansiblelint/rules/latest.py b/src/ansiblelint/rules/latest.py new file mode 100644 index 0000000..0838feb --- /dev/null +++ b/src/ansiblelint/rules/latest.py @@ -0,0 +1,46 @@ +"""Implementation of latest rule.""" +from __future__ import annotations + +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class LatestRule(AnsibleLintRule): + """Result of the command may vary on subsequent runs.""" + + id = "latest" + description = ( + "All version control checkouts must point to " + "an explicit commit or tag, not just ``latest``" + ) + severity = "MEDIUM" + tags = ["idempotency"] + version_added = "v6.5.2" + _ids = { + "latest[git]": "Use a commit hash or tag instead of 'latest' for git", + "latest[hg]": "Use a commit hash or tag instead of 'latest' for hg", + } + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str | MatchError: + """Check if module args are safe.""" + if ( + task["action"]["__ansible_module__"] == "git" + and task["action"].get("version", "HEAD") == "HEAD" + ): + return self.create_matcherror(tag="latest[git]", filename=file) + if ( + task["action"]["__ansible_module__"] == "hg" + and task["action"].get("revision", "default") == "default" + ): + return self.create_matcherror(tag="latest[hg]", filename=file) + return False diff --git a/src/ansiblelint/rules/literal_compare.md b/src/ansiblelint/rules/literal_compare.md new file mode 100644 index 0000000..5e25394 --- /dev/null +++ b/src/ansiblelint/rules/literal_compare.md @@ -0,0 +1,32 @@ +# literal-compare + +This rule checks for literal comparison with the `when` clause. +Literal comparison, like `when: var == True`, is unnecessarily complex. +Use `when: var` to keep your playbooks simple. + +Similarly, a check like `when: var != True` or `when: var == False` +should be replaced with `when: not var`. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Print environment variable to stdout + ansible.builtin.command: echo $MY_ENV_VAR + when: ansible_os_family == True # <- Adds complexity to your playbook. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Print environment variable to stdout + ansible.builtin.command: echo $MY_ENV_VAR + when: ansible_os_family # <- Keeps your playbook simple. +``` diff --git a/src/ansiblelint/rules/literal_compare.py b/src/ansiblelint/rules/literal_compare.py new file mode 100644 index 0000000..1129d1d --- /dev/null +++ b/src/ansiblelint/rules/literal_compare.py @@ -0,0 +1,86 @@ +"""Implementation of the literal-compare rule.""" +# Copyright (c) 2016, Will Thames and contributors +# Copyright (c) 2018-2021, Ansible Project + +from __future__ import annotations + +import re +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.yaml_utils import nested_items_path + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class ComparisonToLiteralBoolRule(AnsibleLintRule): + """Don't compare to literal True/False.""" + + id = "literal-compare" + description = ( + "Use ``when: var`` rather than ``when: var == True`` " + "(or conversely ``when: not var``)" + ) + severity = "HIGH" + tags = ["idiom"] + version_added = "v4.0.0" + + literal_bool_compare = re.compile("[=!]= ?(True|true|False|false)") + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + for k, v, _ in nested_items_path(task): + if k == "when": + if isinstance(v, str): + if self.literal_bool_compare.search(v): + return True + elif isinstance(v, bool): + pass + else: + for item in v: + if isinstance(item, str) and self.literal_bool_compare.search( + item, + ): + return True + + return False + + +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("test_file", "failures"), + ( + pytest.param( + "examples/playbooks/rule_literal_compare_fail.yml", + 3, + id="fail", + ), + pytest.param( + "examples/playbooks/rule_literal_compare_pass.yml", + 0, + id="pass", + ), + ), + ) + def test_literal_compare( + default_rules_collection: RulesCollection, + test_file: str, + failures: int, + ) -> None: + """Test rule matches.""" + # Enable checking of loop variable prefixes in roles + results = Runner(test_file, rules=default_rules_collection).run() + for result in results: + assert result.rule.id == "literal-compare" + assert len(results) == failures diff --git a/src/ansiblelint/rules/loop_var_prefix.md b/src/ansiblelint/rules/loop_var_prefix.md new file mode 100644 index 0000000..33adbd7 --- /dev/null +++ b/src/ansiblelint/rules/loop_var_prefix.md @@ -0,0 +1,78 @@ +# loop-var-prefix + +This rule avoids conflicts with nested looping tasks by configuring a variable +prefix with `loop_var`. Ansible sets `item` as the loop variable. You can use +`loop_var` to specify a prefix for loop variables and ensure they are unique to +each task. + +This rule can produce the following messages: + +- `loop-var-prefix[missing]` - Replace any unsafe implicit `item` loop variable + by adding `loop_var: <loop_var_prefix>...`. +- `loop-var-prefix[wrong]` - Ensure loop variables start with + `<loop_var_prefix>`. + +This rule originates from the [Naming parameters section of Ansible Best +Practices guide][cop314]. + +## Settings + +You can change the behavior of this rule by overriding its default regular +expression used to check loop variable naming. Keep in mind that the `{role}` +part is replaced with the inferred role name when applicable. + +```yaml +# .ansible-lint +loop_var_prefix: "^(__|{role}_)" +``` + +This is an opt-in rule. You must enable it in your Ansible-lint configuration as +follows: + +```yaml +enable_list: + - loop-var-prefix +``` + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Does not set a prefix for loop variables. + ansible.builtin.debug: + var: item + loop: + - foo + - bar # <- These items do not have a unique prefix. + - name: Sets a prefix that is not unique. + ansible.builtin.debug: + var: zz_item + loop: + - foo + - bar + loop_control: + loop_var: zz_item # <- This prefix is not unique. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Sets a unique prefix for loop variables. + ansible.builtin.debug: + var: zz_item + loop: + - foo + - bar + loop_control: + loop_var: my_prefix # <- Specifies a unique prefix for loop variables. +``` + +[cop314]: + https://redhat-cop.github.io/automation-good-practices/#_naming_parameters diff --git a/src/ansiblelint/rules/loop_var_prefix.py b/src/ansiblelint/rules/loop_var_prefix.py new file mode 100644 index 0000000..8f1bb56 --- /dev/null +++ b/src/ansiblelint/rules/loop_var_prefix.py @@ -0,0 +1,113 @@ +"""Optional Ansible-lint rule to enforce use of prefix on role loop vars.""" +from __future__ import annotations + +import re +import sys +from typing import TYPE_CHECKING + +from ansiblelint.config import LOOP_VAR_PREFIX, options +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.text import toidentifier + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class RoleLoopVarPrefix(AnsibleLintRule): + """Role loop_var should use configured prefix.""" + + id = "loop-var-prefix" + link = ( + "https://docs.ansible.com/ansible/latest/playbook_guide/" + "playbooks_loops.html#defining-inner-and-outer-variable-names-with-loop-var" + ) + description = """\ +Looping inside roles has the risk of clashing with loops from user-playbooks.\ +""" + + tags = ["idiom"] + prefix = re.compile("") + severity = "MEDIUM" + _ids = { + "loop-var-prefix[wrong]": "Loop variable name does not match regex.", + "loop-var-prefix[missing]": "Replace unsafe implicit `item` loop variable.", + } + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + """Return matches for a task.""" + if not file or not file.role or not options.loop_var_prefix: + return [] + + self.prefix = re.compile( + options.loop_var_prefix.format(role=toidentifier(file.role)), + ) + has_loop = "loop" in task.raw_task + for key in task.raw_task: + if key.startswith("with_"): + has_loop = True + + if has_loop: + loop_control = task.raw_task.get("loop_control", {}) + loop_var = loop_control.get("loop_var", "") + + if loop_var: + if not self.prefix.match(loop_var): + return [ + self.create_matcherror( + message=f"Loop variable name does not match /{options.loop_var_prefix}/ regex, where role={toidentifier(file.role)}.", + filename=file, + tag="loop-var-prefix[wrong]", + ), + ] + else: + return [ + self.create_matcherror( + message=f"Replace unsafe implicit `item` loop variable by adding a `loop_var` that is matching /{options.loop_var_prefix}/ regex.", + filename=file, + tag="loop-var-prefix[missing]", + ), + ] + + return [] + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("test_file", "failures"), + ( + pytest.param( + "examples/playbooks/roles/loop_var_prefix/tasks/pass.yml", + 0, + id="pass", + ), + pytest.param( + "examples/playbooks/roles/loop_var_prefix/tasks/fail.yml", + 6, + id="fail", + ), + ), + ) + def test_loop_var_prefix( + default_rules_collection: RulesCollection, + test_file: str, + failures: int, + ) -> None: + """Test rule matches.""" + # Enable checking of loop variable prefixes in roles + options.loop_var_prefix = LOOP_VAR_PREFIX + results = Runner(test_file, rules=default_rules_collection).run() + for result in results: + assert result.rule.id == RoleLoopVarPrefix().id + assert len(results) == failures diff --git a/src/ansiblelint/rules/meta_incorrect.md b/src/ansiblelint/rules/meta_incorrect.md new file mode 100644 index 0000000..b1e8793 --- /dev/null +++ b/src/ansiblelint/rules/meta_incorrect.md @@ -0,0 +1,32 @@ +# meta-incorrect + +This rule checks role metadata for fields with undefined or default values. +Always set appropriate values for the following metadata fields in the `meta/main.yml` file: + +- `author` +- `description` +- `company` +- `license` + +## Problematic Code + +```yaml +--- +# Metadata fields for the role contain default values. +galaxy_info: + author: your name + description: your role description + company: your company (optional) + license: license (GPL-2.0-or-later, MIT, etc) +``` + +## Correct Code + +```yaml +--- +galaxy_info: + author: Leroy Jenkins + description: This role will set you free. + company: Red Hat + license: Apache +``` diff --git a/src/ansiblelint/rules/meta_incorrect.py b/src/ansiblelint/rules/meta_incorrect.py new file mode 100644 index 0000000..4252254 --- /dev/null +++ b/src/ansiblelint/rules/meta_incorrect.py @@ -0,0 +1,77 @@ +"""Implementation of meta-incorrect rule.""" +# Copyright (c) 2018, Ansible Project +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + + +class MetaChangeFromDefaultRule(AnsibleLintRule): + """meta/main.yml default values should be changed.""" + + id = "meta-incorrect" + field_defaults = [ + ("author", "your name"), + ("description", "your description"), + ("company", "your company (optional)"), + ("license", "license (GPLv2, CC-BY, etc)"), + ("license", "license (GPL-2.0-or-later, MIT, etc)"), + ] + values = ", ".join(sorted({f[0] for f in field_defaults})) + description = ( + f"You should set appropriate values in meta/main.yml for these fields: {values}" + ) + severity = "HIGH" + tags = ["metadata"] + version_added = "v4.0.0" + + def matchyaml(self, file: Lintable) -> list[MatchError]: + if file.kind != "meta" or not file.data: + return [] + + galaxy_info = file.data.get("galaxy_info", None) + if not galaxy_info: + return [] + + results = [] + for field, default in self.field_defaults: + value = galaxy_info.get(field, None) + if value and value == default: + results.append( + self.create_matcherror( + filename=file, + lineno=file.data[LINE_NUMBER_KEY], + message=f"Should change default metadata: {field}", + ), + ) + + return results + + +if "pytest" in sys.modules: + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + def test_default_galaxy_info( + default_rules_collection: RulesCollection, + ) -> None: + """Test for meta-incorrect.""" + results = Runner( + "examples/roles/meta_incorrect_fail", + rules=default_rules_collection, + ).run() + for result in results: + assert result.rule.id == "meta-incorrect" + assert len(results) == 4 + + assert "Should change default metadata: author" in str(results) + assert "Should change default metadata: description" in str(results) + assert "Should change default metadata: company" in str(results) + assert "Should change default metadata: license" in str(results) diff --git a/src/ansiblelint/rules/meta_no_tags.md b/src/ansiblelint/rules/meta_no_tags.md new file mode 100644 index 0000000..9518549 --- /dev/null +++ b/src/ansiblelint/rules/meta_no_tags.md @@ -0,0 +1,22 @@ +# meta-no-tags + +This rule checks role metadata for tags with special characters. +Always use lowercase numbers and letters for tags in the `meta/main.yml` file. + +## Problematic Code + +```yaml +--- +# Metadata tags contain upper-case letters and special characters. +galaxy_info: + galaxy_tags: [MyTag#1, MyTag&^-] +``` + +## Correct Code + +```yaml +--- +# Metadata tags contain only lowercase letters and numbers. +galaxy_info: + galaxy_tags: [mytag1, mytag2] +``` diff --git a/src/ansiblelint/rules/meta_no_tags.py b/src/ansiblelint/rules/meta_no_tags.py new file mode 100644 index 0000000..c27a30e --- /dev/null +++ b/src/ansiblelint/rules/meta_no_tags.py @@ -0,0 +1,159 @@ +"""Implementation of meta-no-tags rule.""" +from __future__ import annotations + +import re +import sys +from pathlib import Path +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule + +# Copyright (c) 2018, Ansible Project + + +if TYPE_CHECKING: + from typing import Any + + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.testing import RunFromText + + +class MetaTagValidRule(AnsibleLintRule): + """Tags must contain lowercase letters and digits only.""" + + id = "meta-no-tags" + description = ( + "Tags must contain lowercase letters and digits only, " + "and ``galaxy_tags`` is expected to be a list" + ) + severity = "HIGH" + tags = ["metadata"] + version_added = "v4.0.0" + + TAG_REGEXP = re.compile("^[a-z0-9]+$") + + def matchyaml(self, file: Lintable) -> list[MatchError]: + """Find violations inside meta files.""" + if file.kind != "meta" or not file.data: + return [] + + galaxy_info = file.data.get("galaxy_info", None) + if not galaxy_info: + return [] + + tags = [] + results = [] + + if "galaxy_tags" in galaxy_info: + if isinstance(galaxy_info["galaxy_tags"], list): + tags += galaxy_info["galaxy_tags"] + else: + results.append( + self.create_matcherror( + "Expected 'galaxy_tags' to be a list", + filename=file, + ), + ) + + if "categories" in galaxy_info: + results.append( + self.create_matcherror( + "Use 'galaxy_tags' rather than 'categories'", + filename=file, + ), + ) + if isinstance(galaxy_info["categories"], list): + tags += galaxy_info["categories"] + else: + results.append( + self.create_matcherror( + "Expected 'categories' to be a list", + filename=file, + ), + ) + + for tag in tags: + msg = self.shortdesc + if not isinstance(tag, str): + results.append( + self.create_matcherror( + f"Tags must be strings: '{tag}'", + filename=file, + ), + ) + continue + if not re.match(self.TAG_REGEXP, tag): + results.append( + self.create_matcherror( + message=f"{msg}, invalid: '{tag}'", + filename=file, + ), + ) + + return results + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + import pytest + + @pytest.mark.parametrize( + "rule_runner", + (MetaTagValidRule,), + indirect=["rule_runner"], + ) + def test_valid_tag_rule(rule_runner: RunFromText) -> None: + """Test rule matches.""" + results = rule_runner.run( + Path("examples/roles/meta_no_tags_valid/meta/main.yml"), + ) + assert "Use 'galaxy_tags' rather than 'categories'" in str(results), results + assert "Expected 'categories' to be a list" in str(results) + assert "invalid: 'my s q l'" in str(results) + assert "invalid: 'MYTAG'" in str(results) + + @pytest.mark.parametrize( + "rule_runner", + (MetaTagValidRule,), + indirect=["rule_runner"], + ) + def test_meta_not_tags(rule_runner: Any) -> None: + """Test rule matches.""" + results = rule_runner.run( + "examples/roles/meta_no_tags_galaxy_info/meta/main.yml", + ) + assert results == [] + + @pytest.mark.parametrize( + "rule_runner", + (MetaTagValidRule,), + indirect=["rule_runner"], + ) + def test_no_galaxy_tags_list(rule_runner: Any) -> None: + """Test rule matches.""" + results = rule_runner.run("examples/roles/meta_tags_no_list/meta/main.yml") + assert "Expected 'galaxy_tags' to be a list" in str(results) + + @pytest.mark.parametrize( + "rule_runner", + (MetaTagValidRule,), + indirect=["rule_runner"], + ) + def test_galaxy_categories_as_list(rule_runner: Any) -> None: + """Test rule matches.""" + results = rule_runner.run( + "examples/roles/meta_categories_as_list/meta/main.yml", + ) + assert "Use 'galaxy_tags' rather than 'categories'" in str(results), results + assert "Expected 'categories' to be a list" not in str(results) + + @pytest.mark.parametrize( + "rule_runner", + (MetaTagValidRule,), + indirect=["rule_runner"], + ) + def test_tags_not_a_string(rule_runner: Any) -> None: + """Test rule matches.""" + results = rule_runner.run("examples/roles/meta_tags_not_a_string/meta/main.yml") + assert "Tags must be strings" in str(results) diff --git a/src/ansiblelint/rules/meta_runtime.md b/src/ansiblelint/rules/meta_runtime.md new file mode 100644 index 0000000..6ed6f17 --- /dev/null +++ b/src/ansiblelint/rules/meta_runtime.md @@ -0,0 +1,46 @@ +# meta-runtime + +This rule checks the meta/runtime.yml `requires_ansible` key against the list of currently supported versions of ansible-core. + +This rule can produce messages such: + +- `requires_ansible` key must be set to a supported version. + +Currently supported versions of ansible-core are: + +- `2.9.10` +- `2.11.x` +- `2.12.x` +- `2.13.x` +- `2.14.x` +- `2.15.x` +- `2.16.x` (in development) + +This rule can produce messages such as: + +- `meta-runtime[unsupported-version]` - `requires_ansible` key must contain a supported version, shown in the list above. +- `meta-runtime[invalid-version]` - `requires_ansible` key must be a valid version identifier. + + +## Problematic code + +```yaml +# runtime.yml +--- +requires_ansible: ">=2.9" +``` + + +```yaml +# runtime.yml +--- +requires_ansible: "2.9" +``` + +## Correct code + +```yaml +# runtime.yml +--- +requires_ansible: ">=2.9.10" +``` diff --git a/src/ansiblelint/rules/meta_runtime.py b/src/ansiblelint/rules/meta_runtime.py new file mode 100644 index 0000000..fed7121 --- /dev/null +++ b/src/ansiblelint/rules/meta_runtime.py @@ -0,0 +1,126 @@ +"""Implementation of meta-runtime rule.""" +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from packaging.specifiers import SpecifierSet + +from ansiblelint.rules import AnsibleLintRule + +# Copyright (c) 2018, Ansible Project + + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + + +class CheckRequiresAnsibleVersion(AnsibleLintRule): + """Required ansible version in meta/runtime.yml must be a supported version.""" + + id = "meta-runtime" + description = ( + "The ``requires_ansible`` key in runtime.yml must specify " + "a supported platform version of ansible-core and be a valid version." + ) + severity = "VERY_HIGH" + tags = ["metadata"] + version_added = "v6.11.0 (last update)" + + # Refer to https://access.redhat.com/support/policy/updates/ansible-automation-platform + # Also add devel to this list + supported_ansible = ["2.9.10", "2.11.", "2.12.", "2.13.", "2.14.", "2.15.", "2.16."] + _ids = { + "meta-runtime[unsupported-version]": "requires_ansible key must be set to a supported version.", + "meta-runtime[invalid-version]": "'requires_ansible' is not a valid requirement specification", + } + + def matchyaml(self, file: Lintable) -> list[MatchError]: + """Find violations inside meta files. + + :param file: Input lintable file that is a match for `meta-runtime` + :returns: List of errors matched to the input file + """ + results = [] + + if file.kind != "meta-runtime": + return [] + + version_required = file.data.get("requires_ansible", None) + + if version_required: + if not any( + version in version_required for version in self.supported_ansible + ): + results.append( + self.create_matcherror( + message="requires_ansible key must be set to a supported version.", + tag="meta-runtime[unsupported-version]", + filename=file, + ), + ) + + try: + SpecifierSet(version_required) + except ValueError: + results.append( + self.create_matcherror( + message="'requires_ansible' is not a valid requirement specification", + tag="meta-runtime[invalid-version]", + filename=file, + ), + ) + + return results + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("test_file", "failures", "tags"), + ( + pytest.param( + "examples/meta_runtime_version_checks/pass/meta/runtime.yml", + 0, + "meta-runtime[unsupported-version]", + id="pass", + ), + pytest.param( + "examples/meta_runtime_version_checks/fail_0/meta/runtime.yml", + 1, + "meta-runtime[unsupported-version]", + id="fail0", + ), + pytest.param( + "examples/meta_runtime_version_checks/fail_1/meta/runtime.yml", + 1, + "meta-runtime[unsupported-version]", + id="fail1", + ), + pytest.param( + "examples/meta_runtime_version_checks/fail_2/meta/runtime.yml", + 1, + "meta-runtime[invalid-version]", + id="fail2", + ), + ), + ) + def test_meta_supported_version( + default_rules_collection: RulesCollection, + test_file: str, + failures: int, + tags: str, + ) -> None: + """Test rule matches.""" + default_rules_collection.register(CheckRequiresAnsibleVersion()) + results = Runner(test_file, rules=default_rules_collection).run() + for result in results: + assert result.rule.id == CheckRequiresAnsibleVersion().id + assert result.tag == tags + assert len(results) == failures diff --git a/src/ansiblelint/rules/meta_video_links.md b/src/ansiblelint/rules/meta_video_links.md new file mode 100644 index 0000000..c3f051b --- /dev/null +++ b/src/ansiblelint/rules/meta_video_links.md @@ -0,0 +1,36 @@ +# meta-video-links + +This rule checks formatting for video links in metadata. Always use dictionaries +for items in the `meta/main.yml` file. + +Items in the `video_links` section must be in a dictionary and use the following +keys: + +- `url` +- `title` + +The value of the `url` key must be a shared link from YouTube, Vimeo, or Google +Drive. + +## Problematic Code + +```yaml +--- +galaxy_info: + video_links: + - https://www.youtube.com/watch?v=aWmRepTSFKs&feature=youtu.be # <- Does not use the url key. + - my_bad_key: https://www.youtube.com/watch?v=aWmRepTSFKs&feature=youtu.be # <- Uses an unsupported key. + title: Incorrect key. + - url: www.acme.com/vid # <- Uses an unsupported url format. + title: Incorrect url format. +``` + +## Correct Code + +```yaml +--- +galaxy_info: + video_links: + - url: https://www.youtube.com/watch?v=aWmRepTSFKs&feature=youtu.be # <- Uses a supported shared link with the url key. + title: Correctly formatted video link. +``` diff --git a/src/ansiblelint/rules/meta_video_links.py b/src/ansiblelint/rules/meta_video_links.py new file mode 100644 index 0000000..5d4941a --- /dev/null +++ b/src/ansiblelint/rules/meta_video_links.py @@ -0,0 +1,122 @@ +"""Implementation of meta-video-links rule.""" +# Copyright (c) 2018, Ansible Project +from __future__ import annotations + +import re +import sys +from typing import TYPE_CHECKING + +from ansiblelint.constants import FILENAME_KEY, LINE_NUMBER_KEY +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from collections.abc import Sequence + + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + + +class MetaVideoLinksRule(AnsibleLintRule): + """meta/main.yml video_links should be formatted correctly.""" + + id = "meta-video-links" + description = ( + "Items in ``video_links`` in meta/main.yml should be " + "dictionaries, and contain only keys ``url`` and ``title``, " + "and have a shared link from a supported provider" + ) + severity = "LOW" + tags = ["metadata"] + version_added = "v4.0.0" + + VIDEO_REGEXP = { + "google": re.compile(r"https://drive\.google\.com.*file/d/([0-9A-Za-z-_]+)/.*"), + "vimeo": re.compile(r"https://vimeo\.com/([0-9]+)"), + "youtube": re.compile(r"https://youtu\.be/([0-9A-Za-z-_]+)"), + } + + def matchyaml(self, file: Lintable) -> list[MatchError]: + if file.kind != "meta" or not file.data: + return [] + + galaxy_info = file.data.get("galaxy_info", None) + if not galaxy_info: + return [] + + video_links = galaxy_info.get("video_links", None) + if not video_links: + return [] + + results = [] + + for video in video_links: + if not isinstance(video, dict): + results.append( + self.create_matcherror( + "Expected item in 'video_links' to be a dictionary", + filename=file, + ), + ) + continue + + if set(video) != {"url", "title", FILENAME_KEY, LINE_NUMBER_KEY}: + results.append( + self.create_matcherror( + "Expected item in 'video_links' to contain " + "only keys 'url' and 'title'", + filename=file, + ), + ) + continue + + for expr in self.VIDEO_REGEXP.values(): + if expr.match(video["url"]): + break + else: + msg = ( + f"URL format '{video['url']}' is not recognized. " + "Expected it be a shared link from Vimeo, YouTube, " + "or Google Drive." + ) + results.append(self.create_matcherror(msg, filename=file)) + + return results + + +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("test_file", "failures"), + ( + pytest.param( + "examples/roles/meta_video_links_fail/meta/main.yml", + ( + "Expected item in 'video_links' to be a dictionary", + "Expected item in 'video_links' to contain only keys 'url' and 'title'", + "URL format 'https://www.youtube.com/watch?v=aWmRepTSFKs&feature=youtu.be' is not recognized. Expected it be a shared link from Vimeo, YouTube, or Google Drive.", + "URL format 'www.acme.com/vid' is not recognized", + ), + id="1", + ), + pytest.param( + "examples/roles/meta_video_links_pass/meta/main.yml", + (), + id="2", + ), + ), + ) + def test_video_links( + default_rules_collection: RulesCollection, + test_file: str, + failures: Sequence[str], + ) -> None: + """Test rule matches.""" + results = Runner(test_file, rules=default_rules_collection).run() + assert len(results) == len(failures) + for index, result in enumerate(results): + assert result.tag == "meta-video-links" + assert failures[index] in result.message diff --git a/src/ansiblelint/rules/name.md b/src/ansiblelint/rules/name.md new file mode 100644 index 0000000..9df4213 --- /dev/null +++ b/src/ansiblelint/rules/name.md @@ -0,0 +1,61 @@ +# name + +This rule identifies several problems related to the naming of tasks and plays. +This is important because these names are the primary way to **identify** and +**document** executed operations on the console, logs or web interface. + +This rule can produce messages as: + +- `name[casing]` - All names should start with an uppercase letter for languages + that support it. +- `name[missing]` - All tasks should be named. +- `name[play]` - All plays should be named. +- `name[prefix]` - Prefix task names in sub-tasks files. (opt-in) +- `name[template]` - Jinja templates should only be at the end of 'name'. This + helps with the identification of tasks inside the source code when they fail. + The use of templating inside `name` keys is discouraged as there are multiple + cases where the rendering of the name template is not possible. + +If you want to ignore some of the messages above, you can add any of them to the +`skip_list`. + +## name[prefix] + +This rule applies only to included task files that are not named `main.yml`. It +suggests adding the stem of the file as a prefix to the task name. + +For example, if you have a task named `Restart server` inside a file named +`tasks/deploy.yml`, this rule suggests renaming it to `deploy | Restart server`, +so it would be easier to identify where it comes from. + +For the moment, this sub-rule is just an **opt-in**, so you need to add it to +your `enable_list` to activate it. + +!!! note + + This rule was designed by [Red Hat Community of Practice](https://redhat-cop.github.io/automation-good-practices/#_prefix_task_names_in_sub_tasks_files_of_roles). The reasoning behind it being + that in a complex roles or playbooks with multiple (sub-)tasks file, it becomes + difficult to understand which task belongs to which file. Adding a prefix, in + combination with the role’s name automatically added by Ansible, makes it a + lot easier to follow and troubleshoot a role play. + +## Problematic code + +```yaml +--- +- hosts: localhost # <-- playbook name[play] + tasks: + - name: create placefolder file # <-- name[casing] due lack of capital letter + ansible.builtin.command: touch /tmp/.placeholder +``` + +## Correct code + +```yaml +--- +- name: Play for creating placeholder + hosts: localhost + tasks: + - name: Create placeholder file + ansible.builtin.command: touch /tmp/.placeholder +``` diff --git a/src/ansiblelint/rules/name.py b/src/ansiblelint/rules/name.py new file mode 100644 index 0000000..41ce5cb --- /dev/null +++ b/src/ansiblelint/rules/name.py @@ -0,0 +1,260 @@ +"""Implementation of NameRule.""" +from __future__ import annotations + +import re +import sys +from copy import deepcopy +from typing import TYPE_CHECKING, Any + +from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.rules import AnsibleLintRule, TransformMixin + +if TYPE_CHECKING: + from ruamel.yaml.comments import CommentedMap, CommentedSeq + + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class NameRule(AnsibleLintRule, TransformMixin): + """Rule for checking task and play names.""" + + id = "name" + description = ( + "All tasks and plays should have a distinct name for readability " + "and for ``--start-at-task`` to work" + ) + severity = "MEDIUM" + tags = ["idiom"] + version_added = "v6.9.1 (last update)" + _re_templated_inside = re.compile(r".*\{\{.*\}\}.*\w.*$") + _ids = { + "name[play]": "All plays should be named.", + "name[missing]": "All tasks should be named.", + "name[prefix]": "Task name should start with a prefix.", + "name[casing]": "All names should start with an uppercase letter.", + "name[template]": "Jinja templates should only be at the end of 'name'", + } + + def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + """Return matches found for a specific play (entry in playbook).""" + results = [] + if file.kind != "playbook": + return [] + if "name" not in data: + return [ + self.create_matcherror( + message="All plays should be named.", + lineno=data[LINE_NUMBER_KEY], + tag="name[play]", + filename=file, + ), + ] + results.extend( + self._check_name( + data["name"], + lintable=file, + lineno=data[LINE_NUMBER_KEY], + ), + ) + return results + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + results = [] + name = task.get("name") + if not name: + results.append( + self.create_matcherror( + message="All tasks should be named.", + lineno=task[LINE_NUMBER_KEY], + tag="name[missing]", + filename=file, + ), + ) + else: + results.extend( + self._prefix_check( + name, + lintable=file, + lineno=task[LINE_NUMBER_KEY], + ), + ) + return results + + def _prefix_check( + self, + name: str, + lintable: Lintable | None, + lineno: int, + ) -> list[MatchError]: + results: list[MatchError] = [] + effective_name = name + if lintable is None: + return [] + + if not results: + results.extend( + self._check_name( + effective_name, + lintable=lintable, + lineno=lineno, + ), + ) + return results + + def _check_name( + self, + name: str, + lintable: Lintable | None, + lineno: int, + ) -> list[MatchError]: + # This rules applies only to languages that do have uppercase and + # lowercase letter, so we ignore anything else. On Unicode isupper() + # is not necessarily the opposite of islower() + results = [] + # stage one check prefix + effective_name = name + if self._collection and lintable: + prefix = self._collection.options.task_name_prefix.format( + stem=lintable.path.stem, + ) + if lintable.kind == "tasks" and lintable.path.stem != "main": + if not name.startswith(prefix): + # For the moment in order to raise errors this rule needs to be + # enabled manually. Still, we do allow use of prefixes even without + # having to enable the rule. + if "name[prefix]" in self._collection.options.enable_list: + results.append( + self.create_matcherror( + message=f"Task name should start with '{prefix}'.", + lineno=lineno, + tag="name[prefix]", + filename=lintable, + ), + ) + return results + else: + effective_name = name[len(prefix) :] + + if ( + effective_name[0].isalpha() + and effective_name[0].islower() + and not effective_name[0].isupper() + ): + results.append( + self.create_matcherror( + message="All names should start with an uppercase letter.", + lineno=lineno, + tag="name[casing]", + filename=lintable, + ), + ) + if self._re_templated_inside.match(name): + results.append( + self.create_matcherror( + message="Jinja templates should only be at the end of 'name'", + lineno=lineno, + tag="name[template]", + filename=lintable, + ), + ) + return results + + def transform( + self, + match: MatchError, + lintable: Lintable, + data: CommentedMap | CommentedSeq | str, + ) -> None: + if match.tag == "name[casing]": + target_task = self.seek(match.yaml_path, data) + # Not using capitalize(), since that rewrites the rest of the name to lower case + target_task[ + "name" + ] = f"{target_task['name'][:1].upper()}{target_task['name'][1:]}" + match.fixed = True + + +if "pytest" in sys.modules: + from ansiblelint.config import options + from ansiblelint.file_utils import Lintable # noqa: F811 + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner + + def test_file_positive() -> None: + """Positive test for name[missing].""" + collection = RulesCollection() + collection.register(NameRule()) + success = "examples/playbooks/rule-name-missing-pass.yml" + good_runner = Runner(success, rules=collection) + assert [] == good_runner.run() + + def test_file_negative() -> None: + """Negative test for name[missing].""" + collection = RulesCollection() + collection.register(NameRule()) + failure = "examples/playbooks/rule-name-missing-fail.yml" + bad_runner = Runner(failure, rules=collection) + errs = bad_runner.run() + assert len(errs) == 5 + + def test_name_prefix_negative() -> None: + """Negative test for name[missing].""" + custom_options = deepcopy(options) + custom_options.enable_list = ["name[prefix]"] + collection = RulesCollection(options=custom_options) + collection.register(NameRule()) + failure = Lintable( + "examples/playbooks/tasks/rule-name-prefix-fail.yml", + kind="tasks", + ) + bad_runner = Runner(failure, rules=collection) + results = bad_runner.run() + assert len(results) == 3 + # , "\n".join(results) + assert results[0].tag == "name[casing]" + assert results[1].tag == "name[prefix]" + assert results[2].tag == "name[prefix]" + + def test_rule_name_lowercase() -> None: + """Negative test for a task that starts with lowercase.""" + collection = RulesCollection() + collection.register(NameRule()) + failure = "examples/playbooks/rule-name-casing.yml" + bad_runner = Runner(failure, rules=collection) + errs = bad_runner.run() + assert len(errs) == 1 + assert errs[0].tag == "name[casing]" + assert errs[0].rule.id == "name" + + def test_name_play() -> None: + """Positive test for name[play].""" + collection = RulesCollection() + collection.register(NameRule()) + success = "examples/playbooks/rule-name-play-fail.yml" + errs = Runner(success, rules=collection).run() + assert len(errs) == 1 + assert errs[0].tag == "name[play]" + assert errs[0].rule.id == "name" + + def test_name_template() -> None: + """Negative test for name[templated].""" + collection = RulesCollection() + collection.register(NameRule()) + failure = "examples/playbooks/rule-name-templated-fail.yml" + bad_runner = Runner(failure, rules=collection) + errs = bad_runner.run() + assert len(errs) == 1 + assert errs[0].tag == "name[template]" + + def test_when_no_lintable() -> None: + """Test when lintable is None.""" + name_rule = NameRule() + # pylint: disable=protected-access + result = name_rule._prefix_check("Foo", None, 1) # noqa: SLF001 + assert len(result) == 0 diff --git a/src/ansiblelint/rules/no_changed_when.md b/src/ansiblelint/rules/no_changed_when.md new file mode 100644 index 0000000..95c1d46 --- /dev/null +++ b/src/ansiblelint/rules/no_changed_when.md @@ -0,0 +1,49 @@ +# no-changed-when + +This rule checks that tasks return changes to results or conditions. Unless +tasks only read information, you should ensure that they return changes in the +following ways: + +- Register results or conditions and use the `changed_when` clause. +- Use the `creates` or `removes` argument. + +You should always use the `changed_when` clause on tasks that do not naturally +detect if a change has occurred or not. Some of the most common examples are +[shell] and [command] modules, which run arbitrary commands. + +One very common workaround is to use a boolean value like `changed_when: false` +if the task never changes anything or `changed_when: true` if it always changes +something, but you can also use any expressions, including ones that use the +registered result of a task, like in our example below. + +This rule also applies to handlers, not only to tasks because they are also +tasks. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Does not handle any output or return codes + ansible.builtin.command: cat {{ my_file | quote }} # <- Does not handle the command output. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Handle shell output with return code + ansible.builtin.command: cat {{ my_file | quote }} + register: my_output # <- Registers the command output. + changed_when: my_output.rc != 0 # <- Uses the return code to define when the task has changed. +``` + +[shell]: + https://docs.ansible.com/ansible/latest/collections/ansible/builtin/shell_module.html +[command]: + https://docs.ansible.com/ansible/latest/collections/ansible/builtin/command_module.html diff --git a/src/ansiblelint/rules/no_changed_when.py b/src/ansiblelint/rules/no_changed_when.py new file mode 100644 index 0000000..28ba427 --- /dev/null +++ b/src/ansiblelint/rules/no_changed_when.py @@ -0,0 +1,106 @@ +"""Implementation of the no-changed-when rule.""" +# Copyright (c) 2016 Will Thames <will@thames.id.au> +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. + +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class CommandHasChangesCheckRule(AnsibleLintRule): + """Commands should not change things if nothing needs doing.""" + + id = "no-changed-when" + severity = "HIGH" + tags = ["command-shell", "idempotency"] + version_added = "historic" + + _commands = [ + "ansible.builtin.command", + "ansible.builtin.shell", + "ansible.builtin.raw", + "ansible.legacy.command", + "ansible.legacy.shell", + "ansible.legacy.raw", + "command", + "shell", + "raw", + ] + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + result = [] + # tasks in a block are "meta" type + if ( + task["__ansible_action_type__"] in ["task", "meta"] + and task["action"]["__ansible_module__"] in self._commands + and ( + "changed_when" not in task.raw_task + and "creates" not in task["action"] + and "removes" not in task["action"] + ) + ): + result.append(self.create_matcherror(filename=file)) + return result + + +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("file", "expected"), + ( + pytest.param( + "examples/playbooks/rule-no-changed-when-pass.yml", + 0, + id="pass", + ), + pytest.param( + "examples/playbooks/rule-no-changed-when-fail.yml", + 3, + id="fail", + ), + ), + ) + def test_rule_no_changed_when( + default_rules_collection: RulesCollection, + file: str, + expected: int, + ) -> None: + """Validate no-changed-when rule.""" + results = Runner(file, rules=default_rules_collection).run() + + for result in results: + assert result.rule.id == CommandHasChangesCheckRule.id, result + assert len(results) == expected diff --git a/src/ansiblelint/rules/no_free_form.md b/src/ansiblelint/rules/no_free_form.md new file mode 100644 index 0000000..0ffc0ac --- /dev/null +++ b/src/ansiblelint/rules/no_free_form.md @@ -0,0 +1,58 @@ +# no-free-form + +This rule identifies any use of +[free-form](https://docs.ansible.com/ansible/2.7/user_guide/playbooks_intro.html#action-shorthand) +module calling syntax and asks for switching to the full syntax. + +**Free-form** syntax, also known as **inline** or **shorthand**, can produce +subtle bugs. It can also prevent editors and IDEs from providing feedback, +autocomplete and validation for the edited line. + +!!! note + + As long you just pass a YAML string that contains a `=` character inside as the + parameter to the action module name, we consider this as using free-form syntax. + Be sure you pass a dictionary to the module, so the free-form parsing is never + triggered. + +As `raw` module only accepts free-form, we trigger `no-free-form[raw]` only if +we detect the presence of `executable=` inside raw calls. We advise the explicit +use of `args:` for configuring the executable to be run. + +This rule can produce messages as: + +- `no-free-form` - Free-form syntax is discouraged. +- `no-free-form[raw-non-string]` - Passing a non-string value to `raw` module is + neither documented nor supported. + +## Problematic code + +```yaml +--- +- name: Example with discouraged free-form syntax + hosts: localhost + tasks: + - name: Create a placefolder file + ansible.builtin.command: chdir=/tmp touch foo # <-- don't use free-form + - name: Use raw to echo + ansible.builtin.raw: executable=/bin/bash echo foo # <-- don't use executable= + changed_when: false +``` + +## Correct code + +```yaml +--- +- name: Example that avoids free-form syntax + hosts: localhost + tasks: + - name: Create a placefolder file + ansible.builtin.command: + cmd: touch foo # <-- ansible will not touch it + chdir: /tmp + - name: Use raw to echo + ansible.builtin.raw: echo foo + args: + executable: /bin/bash # <-- explicit is better + changed_when: false +``` diff --git a/src/ansiblelint/rules/no_free_form.py b/src/ansiblelint/rules/no_free_form.py new file mode 100644 index 0000000..e89333b --- /dev/null +++ b/src/ansiblelint/rules/no_free_form.py @@ -0,0 +1,116 @@ +"""Implementation of NoFreeFormRule.""" +from __future__ import annotations + +import re +import sys +from typing import TYPE_CHECKING + +from ansiblelint.constants import INCLUSION_ACTION_NAMES, LINE_NUMBER_KEY +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class NoFreeFormRule(AnsibleLintRule): + """Rule for detecting discouraged free-form syntax for action modules.""" + + id = "no-free-form" + description = "Avoid free-form inside files as it can produce subtle bugs." + severity = "MEDIUM" + tags = ["syntax", "risk"] + version_added = "v6.8.0" + needs_raw_task = True + cmd_shell_re = re.compile( + r"(chdir|creates|executable|removes|stdin|stdin_add_newline|warn)=", + ) + _ids = { + "no-free-form[raw]": "Avoid embedding `executable=` inside raw calls, use explicit args dictionary instead.", + "no-free-form[raw-non-string]": "Passing a non string value to `raw` module is neither documented or supported.", + } + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + results: list[MatchError] = [] + action = task["action"]["__ansible_module_original__"] + + if action in INCLUSION_ACTION_NAMES: + return results + + action_value = task["__raw_task__"].get(action, None) + if task["action"].get("__ansible_module__", None) == "raw": + if isinstance(action_value, str): + if "executable=" in action_value: + results.append( + self.create_matcherror( + message="Avoid embedding `executable=` inside raw calls, use explicit args dictionary instead.", + lineno=task[LINE_NUMBER_KEY], + filename=file, + tag=f"{self.id}[raw]", + ), + ) + else: + results.append( + self.create_matcherror( + message="Passing a non string value to `raw` module is neither documented or supported.", + lineno=task[LINE_NUMBER_KEY], + filename=file, + tag=f"{self.id}[raw-non-string]", + ), + ) + elif isinstance(action_value, str) and "=" in action_value: + fail = False + if task["action"].get("__ansible_module__") in ( + "ansible.builtin.command", + "ansible.builtin.shell", + "ansible.windows.win_command", + "ansible.windows.win_shell", + "command", + "shell", + "win_command", + "win_shell", + ): + if self.cmd_shell_re.match(action_value): + fail = True + else: + fail = True + if fail: + results.append( + self.create_matcherror( + message=f"Avoid using free-form when calling module actions. ({action})", + lineno=task[LINE_NUMBER_KEY], + filename=file, + ), + ) + return results + + +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("file", "expected"), + ( + pytest.param("examples/playbooks/rule-no-free-form-pass.yml", 0, id="pass"), + pytest.param("examples/playbooks/rule-no-free-form-fail.yml", 3, id="fail"), + ), + ) + def test_rule_no_free_form( + default_rules_collection: RulesCollection, + file: str, + expected: int, + ) -> None: + """Validate that rule works as intended.""" + results = Runner(file, rules=default_rules_collection).run() + + for result in results: + assert result.rule.id == NoFreeFormRule.id, result + assert len(results) == expected diff --git a/src/ansiblelint/rules/no_handler.md b/src/ansiblelint/rules/no_handler.md new file mode 100644 index 0000000..4deccaa --- /dev/null +++ b/src/ansiblelint/rules/no_handler.md @@ -0,0 +1,55 @@ +# no-handler + +This rule checks for the correct handling of changes to results or conditions. + +If a task has a `when: result.changed` condition, it effectively acts as a +[handler](https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_handlers.html#handlers). +The recommended approach is to use `notify` and move tasks to `handlers`. +If necessary you can silence the rule by add a `# noqa: no-handler` comment at the end of the line. + +## Problematic Code + +```yaml +--- +- name: Example of no-handler rule + hosts: localhost + tasks: + - name: Register result of a task + ansible.builtin.copy: + dest: "/tmp/placeholder" + content: "Ansible made this!" + mode: 0600 + register: result # <-- Registers the result of the task. + - name: Second command to run + ansible.builtin.debug: + msg: The placeholder file was modified! + when: result.changed # <-- Triggers the no-handler rule. +``` + +```yaml +--- +# Optionally silences the rule. +when: result.changed # noqa: no-handler +``` + +## Correct Code + +The following code includes the same functionality as the problematic code without recording a `result` variable. + +```yaml +--- +- name: Example of no-handler rule + hosts: localhost + tasks: + - name: Register result of a task + ansible.builtin.copy: + dest: "/tmp/placeholder" + content: "Ansible made this!" + mode: 0600 + notify: + - Second command to run # <-- Handler runs only when the file changes. + handlers: + - name: Second command to run + ansible.builtin.debug: + msg: The placeholder file was modified! +``` diff --git a/src/ansiblelint/rules/no_handler.py b/src/ansiblelint/rules/no_handler.py new file mode 100644 index 0000000..380fd61 --- /dev/null +++ b/src/ansiblelint/rules/no_handler.py @@ -0,0 +1,108 @@ +# Copyright (c) 2016 Will Thames <will@thames.id.au> +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. + +"""UseHandlerRatherThanWhenChangedRule used with ansible-lint.""" +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +def _changed_in_when(item: str) -> bool: + if not isinstance(item, str): + return False + item_list = item.split() + + if {"and", "or", "not"} & set(item_list): + return False + return any( + changed in item + for changed in [ + ".changed", + "|changed", + '["changed"]', + "['changed']", + "is changed", + ] + ) + + +class UseHandlerRatherThanWhenChangedRule(AnsibleLintRule): + """Tasks that run when changed should likely be handlers.""" + + id = "no-handler" + description = ( + "If a task has a ``when: result.changed`` setting, it is effectively " + "acting as a handler. You could use ``notify`` and move that task to " + "``handlers``." + ) + link = "https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_handlers.html#handlers" + severity = "MEDIUM" + tags = ["idiom"] + version_added = "historic" + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + if task["__ansible_action_type__"] != "task": + return False + + when = task.get("when") + + if isinstance(when, list): + if len(when) > 1: + return False + return _changed_in_when(when[0]) + if isinstance(when, str): + return _changed_in_when(when) + return False + + +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("test_file", "failures"), + ( + pytest.param("examples/playbooks/no_handler_fail.yml", 5, id="fail"), + pytest.param("examples/playbooks/no_handler_pass.yml", 0, id="pass"), + ), + ) + def test_no_handler( + default_rules_collection: RulesCollection, + test_file: str, + failures: int, + ) -> None: + """Test rule matches.""" + results = Runner(test_file, rules=default_rules_collection).run() + assert len(results) == failures + for result in results: + assert result.tag == "no-handler" diff --git a/src/ansiblelint/rules/no_jinja_when.md b/src/ansiblelint/rules/no_jinja_when.md new file mode 100644 index 0000000..702e807 --- /dev/null +++ b/src/ansiblelint/rules/no_jinja_when.md @@ -0,0 +1,32 @@ +# no-jinja-when + +This rule checks conditional statements for Jinja expressions in curly brackets `{{ }}`. +Ansible processes conditionals statements that use the `when`, `failed_when`, and `changed_when` clauses as Jinja expressions. + +An Ansible rule is to always use `{{ }}` except with `when` keys. +Using `{{ }}` in conditionals creates a nested expression, which is an Ansible +anti-pattern and does not produce expected results. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Shut down Debian systems + ansible.builtin.command: /sbin/shutdown -t now + when: "{{ ansible_facts['os_family'] == 'Debian' }}" # <- Nests a Jinja expression in a conditional statement. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Shut down Debian systems + ansible.builtin.command: /sbin/shutdown -t now + when: ansible_facts['os_family'] == "Debian" # <- Uses facts in a conditional statement. +``` diff --git a/src/ansiblelint/rules/no_jinja_when.py b/src/ansiblelint/rules/no_jinja_when.py new file mode 100644 index 0000000..807081d --- /dev/null +++ b/src/ansiblelint/rules/no_jinja_when.py @@ -0,0 +1,90 @@ +"""Implementation of no-jinja-when rule.""" +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING, Any + +from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class NoFormattingInWhenRule(AnsibleLintRule): + """No Jinja2 in when.""" + + id = "no-jinja-when" + description = ( + "``when`` is a raw Jinja2 expression, remove redundant {{ }} from variable(s)." + ) + severity = "HIGH" + tags = ["deprecations"] + version_added = "historic" + + @staticmethod + def _is_valid(when: str) -> bool: + if isinstance(when, list): + for item in when: + if ( + isinstance(item, str) + and item.find("{{") != -1 + and item.find("}}") != -1 + ): + return False + return True + if not isinstance(when, str): + return True + return when.find("{{") == -1 and when.find("}}") == -1 + + def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + errors: list[MatchError] = [] + if isinstance(data, dict): + if "roles" not in data or data["roles"] is None: + return errors + for role in data["roles"]: + if ( + isinstance(role, dict) + and "when" in role + and not self._is_valid(role["when"]) + ): + errors.append( + self.create_matcherror( + details=str({"when": role}), + filename=file, + lineno=role[LINE_NUMBER_KEY], + ), + ) + return errors + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + return "when" in task.raw_task and not self._is_valid(task.raw_task["when"]) + + +if "pytest" in sys.modules: + # Tests for no-jinja-when rule. + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner + + def test_jinja_file_positive() -> None: + """Positive test for no-jinja-when.""" + collection = RulesCollection() + collection.register(NoFormattingInWhenRule()) + success = "examples/playbooks/rule-no-jinja-when-pass.yml" + good_runner = Runner(success, rules=collection) + assert [] == good_runner.run() + + def test_jinja_file_negative() -> None: + """Negative test for no-jinja-when.""" + collection = RulesCollection() + collection.register(NoFormattingInWhenRule()) + failure = "examples/playbooks/rule-no-jinja-when-fail.yml" + bad_runner = Runner(failure, rules=collection) + errs = bad_runner.run() + assert len(errs) == 3 diff --git a/src/ansiblelint/rules/no_log_password.md b/src/ansiblelint/rules/no_log_password.md new file mode 100644 index 0000000..579dd11 --- /dev/null +++ b/src/ansiblelint/rules/no_log_password.md @@ -0,0 +1,45 @@ +# no-log-password + +This rule ensures playbooks do not write passwords to logs when using loops. +Always set the `no_log: true` attribute to protect sensitive data. + +While most Ansible modules mask sensitive data, using secrets inside a loop can result in those secrets being logged. +Explicitly adding `no_log: true` prevents accidentally exposing secrets. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Log user passwords + ansible.builtin.user: + name: john_doe + comment: John Doe + uid: 1040 + group: admin + password: "{{ item }}" + with_items: + - wow + no_log: false # <- Sets the no_log attribute to false. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Do not log user passwords + ansible.builtin.user: + name: john_doe + comment: John Doe + uid: 1040 + group: admin + password: "{{ item }}" + with_items: + - wow + no_log: true # <- Sets the no_log attribute to a non-false value. +``` diff --git a/src/ansiblelint/rules/no_log_password.py b/src/ansiblelint/rules/no_log_password.py new file mode 100644 index 0000000..7cc7439 --- /dev/null +++ b/src/ansiblelint/rules/no_log_password.py @@ -0,0 +1,306 @@ +# Copyright 2018, Rackspace US, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""NoLogPasswordsRule used with ansible-lint.""" +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.utils import Task, convert_to_boolean + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + + +class NoLogPasswordsRule(AnsibleLintRule): + """Password should not be logged.""" + + id = "no-log-password" + description = ( + "When passing password argument you should have no_log configured " + "to a non False value to avoid accidental leaking of secrets." + ) + severity = "LOW" + tags = ["opt-in", "security", "experimental"] + version_added = "v5.0.9" + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + if task["action"]["__ansible_module_original__"] == "ansible.builtin.user" and ( + task["action"].get("password_lock") and not task["action"].get("password") + ): + has_password = False + else: + for param in task["action"]: + if "password" in param: + has_password = True + break + else: + has_password = False + + has_loop = [key for key in task if key.startswith("with_") or key == "loop"] + # No no_log and no_log: False behave the same way + # and should return a failure (return True), so we + # need to invert the boolean + no_log = task.get("no_log", False) + + if ( + isinstance(no_log, str) + and no_log.startswith("{{") + and no_log.endswith("}}") + ): + # we cannot really evaluate jinja expressions + return False + + return bool( + has_password and not convert_to_boolean(no_log) and len(has_loop) > 0, + ) + + +if "pytest" in sys.modules: + import pytest + + if TYPE_CHECKING: + from ansiblelint.testing import RunFromText # pylint: disable=ungrouped-imports + + NO_LOG_UNUSED = """ +- name: Test + hosts: all + tasks: + - name: Succeed when no_log is not used but no loop present + ansible.builtin.user: + name: john_doe + password: "wow" + state: absent +""" + + NO_LOG_FALSE = """ +- hosts: all + tasks: + - name: Use of jinja for no_log is valid + user: + name: john_doe + user_password: "{{ item }}" + state: absent + no_log: "{{ False }}" + - name: Fail when no_log is set to False + user: + name: john_doe + user_password: "{{ item }}" + state: absent + with_items: + - wow + - now + no_log: False + - name: Fail when no_log is set to False + ansible.builtin.user: + name: john_doe + user_password: "{{ item }}" + state: absent + with_items: + - wow + - now + no_log: False +""" + + NO_LOG_NO = """ +- hosts: all + tasks: + - name: Fail when no_log is set to no + user: + name: john_doe + password: "{{ item }}" + state: absent + no_log: no + loop: + - wow + - now +""" + + PASSWORD_WITH_LOCK = """ +- hosts: all + tasks: + - name: Fail when password is set and password_lock is true + user: + name: "{{ item }}" + password: "wow" + password_lock: true + with_random_choice: + - ansible + - lint +""" + + NO_LOG_YES = """ +- hosts: all + tasks: + - name: Succeed when no_log is set to yes + with_list: + - name: user + password: wow + - password: now + name: ansible + user: + name: "{{ item.name }}" + password: "{{ item.password }}" + state: absent + no_log: yes +""" + + NO_LOG_TRUE = """ +- hosts: all + tasks: + - name: Succeed when no_log is set to True + user: + name: john_doe + user_password: "{{ item }}" + state: absent + no_log: True + loop: + - wow + - now +""" + + PASSWORD_LOCK_YES = """ +- hosts: all + tasks: + - name: Succeed when only password locking account + user: + name: "{{ item }}" + password_lock: yes + # user_password: "this is a comment, not a password" + with_list: + - ansible + - lint +""" + + PASSWORD_LOCK_YES_BUT_NO_PASSWORD = """ +- hosts: all + tasks: + - name: Succeed when only password locking account + ansible.builtin.user: + name: "{{ item }}" + password_lock: yes + # user_password: "this is a comment, not a password" + with_list: + - ansible + - lint +""" + + PASSWORD_LOCK_FALSE = """ +- hosts: all + tasks: + - name: Succeed when password_lock is false and password is not used + user: + name: lint + password_lock: False +""" + + @pytest.mark.parametrize( + "rule_runner", + (NoLogPasswordsRule,), + indirect=["rule_runner"], + ) + def test_no_log_unused(rule_runner: RunFromText) -> None: + """The task does not use no_log but also no loop.""" + results = rule_runner.run_playbook(NO_LOG_UNUSED) + assert len(results) == 0 + + @pytest.mark.parametrize( + "rule_runner", + (NoLogPasswordsRule,), + indirect=["rule_runner"], + ) + def test_no_log_false(rule_runner: RunFromText) -> None: + """The task sets no_log to false.""" + results = rule_runner.run_playbook(NO_LOG_FALSE) + assert len(results) == 2 + for result in results: + assert result.rule.id == "no-log-password" + + @pytest.mark.parametrize( + "rule_runner", + (NoLogPasswordsRule,), + indirect=["rule_runner"], + ) + def test_no_log_no(rule_runner: RunFromText) -> None: + """The task sets no_log to no.""" + results = rule_runner.run_playbook(NO_LOG_NO) + assert len(results) == 1 + assert results[0].rule.id == "no-log-password" + + @pytest.mark.parametrize( + "rule_runner", + (NoLogPasswordsRule,), + indirect=["rule_runner"], + ) + def test_password_with_lock(rule_runner: RunFromText) -> None: + """The task sets a password but also lock the user.""" + results = rule_runner.run_playbook(PASSWORD_WITH_LOCK) + assert len(results) == 1 + assert results[0].rule.id == "no-log-password" + + @pytest.mark.parametrize( + "rule_runner", + (NoLogPasswordsRule,), + indirect=["rule_runner"], + ) + def test_no_log_yes(rule_runner: RunFromText) -> None: + """The task sets no_log to yes.""" + results = rule_runner.run_playbook(NO_LOG_YES) + assert len(results) == 0 + + @pytest.mark.parametrize( + "rule_runner", + (NoLogPasswordsRule,), + indirect=["rule_runner"], + ) + def test_no_log_true(rule_runner: RunFromText) -> None: + """The task sets no_log to true.""" + results = rule_runner.run_playbook(NO_LOG_TRUE) + assert len(results) == 0 + + @pytest.mark.parametrize( + "rule_runner", + (NoLogPasswordsRule,), + indirect=["rule_runner"], + ) + def test_no_log_password_lock_yes(rule_runner: RunFromText) -> None: + """The task only locks the user.""" + results = rule_runner.run_playbook(PASSWORD_LOCK_YES) + assert len(results) == 0 + + @pytest.mark.parametrize( + "rule_runner", + (NoLogPasswordsRule,), + indirect=["rule_runner"], + ) + def test_no_log_password_lock_yes_but_no_password(rule_runner: RunFromText) -> None: + """The task only locks the user.""" + results = rule_runner.run_playbook(PASSWORD_LOCK_YES_BUT_NO_PASSWORD) + assert len(results) == 0 + + @pytest.mark.parametrize( + "rule_runner", + (NoLogPasswordsRule,), + indirect=["rule_runner"], + ) + def test_password_lock_false(rule_runner: RunFromText) -> None: + """The task does not actually lock the user.""" + results = rule_runner.run_playbook(PASSWORD_LOCK_FALSE) + assert len(results) == 0 diff --git a/src/ansiblelint/rules/no_prompting.md b/src/ansiblelint/rules/no_prompting.md new file mode 100644 index 0000000..7e525c8 --- /dev/null +++ b/src/ansiblelint/rules/no_prompting.md @@ -0,0 +1,35 @@ +# no-prompting + +This rule checks for `vars_prompt` or the `ansible.builtin.pause` module in playbooks. +You should enable this rule to ensure that playbooks can run unattended and in CI/CD pipelines. + +This is an opt-in rule. +You must enable it in your Ansible-lint configuration as follows: + +```yaml +enable_list: + - no-prompting +``` + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: all + vars_prompt: # <- Prompts the user to input credentials. + - name: username + prompt: What is your username? + private: false + + - name: password + prompt: What is your password? + tasks: + - name: Pause for 5 minutes + ansible.builtin.pause: + minutes: 5 # <- Pauses playbook execution for a set period of time. +``` + +## Correct Code + +Correct code for this rule is to omit `vars_prompt` and the `ansible.builtin.pause` module from your playbook. diff --git a/src/ansiblelint/rules/no_prompting.py b/src/ansiblelint/rules/no_prompting.py new file mode 100644 index 0000000..6622771 --- /dev/null +++ b/src/ansiblelint/rules/no_prompting.py @@ -0,0 +1,76 @@ +"""Implementation of no-prompting rule.""" +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING, Any + +from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class NoPromptingRule(AnsibleLintRule): + """Disallow prompting.""" + + id = "no-prompting" + description = ( + "Disallow the use of vars_prompt or ansible.builtin.pause to better" + "accommodate unattended playbook runs and use in CI pipelines." + ) + tags = ["opt-in"] + severity = "VERY_LOW" + version_added = "v6.0.3" + + def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + """Return matches found for a specific playbook.""" + # If the Play uses the 'vars_prompt' section to set variables + + if file.kind != "playbook": # pragma: no cover + return [] + + vars_prompt = data.get("vars_prompt", None) + if not vars_prompt: + return [] + return [ + self.create_matcherror( + message="Play uses vars_prompt", + lineno=vars_prompt[0][LINE_NUMBER_KEY], + filename=file, + ), + ] + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + """Return matches for ansible.builtin.pause tasks.""" + # We do not want to trigger this rule if pause has either seconds or + # minutes defined, as that does not make it blocking. + return task["action"]["__ansible_module_original__"] in [ + "pause", + "ansible.builtin.pause", + ] and not ( + task["action"].get("minutes", None) or task["action"].get("seconds", None) + ) + + +if "pytest" in sys.modules: + from ansiblelint.config import options + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + def test_no_prompting_fail() -> None: + """Negative test for no-prompting.""" + # For testing we want to manually enable opt-in rules + options.enable_list = ["no-prompting"] + rules = RulesCollection(options=options) + rules.register(NoPromptingRule()) + results = Runner("examples/playbooks/rule-no-prompting.yml", rules=rules).run() + assert len(results) == 2 + for result in results: + assert result.rule.id == "no-prompting" diff --git a/src/ansiblelint/rules/no_relative_paths.md b/src/ansiblelint/rules/no_relative_paths.md new file mode 100644 index 0000000..568a145 --- /dev/null +++ b/src/ansiblelint/rules/no_relative_paths.md @@ -0,0 +1,94 @@ +# no-relative-paths + +This rule checks for relative paths in the `ansible.builtin.copy` and +`ansible.builtin.template` modules. + +Relative paths in a task most often direct Ansible to remote files and +directories on managed nodes. In the `ansible.builtin.copy` and +`ansible.builtin.template` modules, the `src` argument refers to local files and +directories on the control node. + +The recommended locations to store files are as follows: + +- Use the `files/` folder in the playbook or role directory for the `copy` + module. +- Use the `templates/` folder in the playbook or role directory for the + `template` module. + +These folders allow you to omit the path or use a sub-folder when specifying +files with the `src` argument. + +!!! note + + If resources are outside your Ansible playbook or role directory you should use an absolute path with the `src` argument. + +!!! warning + + Do not store resources at the same directory level as your Ansible playbook or tasks files. + Doing this can result in disorganized projects and cause user confusion when distinguishing between resources of the same type, such as YAML. + +See +[task paths](https://docs.ansible.com/ansible/latest/playbook_guide/playbook_pathing.html#task-paths) +in the Ansible documentation for more information. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Template a file to /etc/file.conf + ansible.builtin.template: + src: ../my_templates/foo.j2 # <- Uses a relative path in the src argument. + dest: /etc/file.conf + owner: bin + group: wheel + mode: "0644" +``` + +```yaml +- name: Example playbook + hosts: all + vars: + source_path: ../../my_templates/foo.j2 # <- Sets a variable to a relative path. + tasks: + - name: Copy a file to /etc/file.conf + ansible.builtin.copy: + src: "{{ source_path }}" # <- Uses the variable in the src argument. + dest: /etc/foo.conf + owner: foo + group: foo + mode: "0644" +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Template a file to /etc/file.conf + ansible.builtin.template: + src: foo.j2 # <- Uses a path from inside templates/ directory. + dest: /etc/file.conf + owner: bin + group: wheel + mode: "0644" +``` + +```yaml +- name: Example playbook + hosts: all + vars: + source_path: foo.j2 # <- Uses a path from inside files/ directory. + tasks: + - name: Copy a file to /etc/file.conf + ansible.builtin.copy: + src: "{{ source_path }}" # <- Uses the variable in the src argument. + dest: /etc/foo.conf + owner: foo + group: foo + mode: "0644" +``` diff --git a/src/ansiblelint/rules/no_relative_paths.py b/src/ansiblelint/rules/no_relative_paths.py new file mode 100644 index 0000000..470b1b8 --- /dev/null +++ b/src/ansiblelint/rules/no_relative_paths.py @@ -0,0 +1,75 @@ +"""Implementation of no-relative-paths rule.""" +# Copyright (c) 2016, Tsukinowa Inc. <info@tsukinowa.jp> +# Copyright (c) 2018, Ansible Project + +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class RoleRelativePath(AnsibleLintRule): + """The src argument should not use a relative path.""" + + id = "no-relative-paths" + description = "The ``copy`` and ``template`` modules should not use relative path for ``src``." + severity = "HIGH" + tags = ["idiom"] + version_added = "v4.0.0" + + _module_to_path_folder = { + "copy": "files", + "win_copy": "files", + "template": "templates", + "win_template": "win_templates", + } + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + module = task["action"]["__ansible_module__"] + if module not in self._module_to_path_folder: + return False + + if "src" not in task["action"]: + return False + + path_to_check = f"../{self._module_to_path_folder[module]}" + if path_to_check in task["action"]["src"]: + return True + + return False + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("test_file", "failures"), + ( + pytest.param("examples/playbooks/no_relative_paths_fail.yml", 2, id="fail"), + pytest.param("examples/playbooks/no_relative_paths_pass.yml", 0, id="pass"), + ), + ) + def test_no_relative_paths( + default_rules_collection: RulesCollection, + test_file: str, + failures: int, + ) -> None: + """Test rule matches.""" + results = Runner(test_file, rules=default_rules_collection).run() + assert len(results) == failures + for result in results: + assert result.tag == "no-relative-paths" diff --git a/src/ansiblelint/rules/no_same_owner.md b/src/ansiblelint/rules/no_same_owner.md new file mode 100644 index 0000000..350a3d4 --- /dev/null +++ b/src/ansiblelint/rules/no_same_owner.md @@ -0,0 +1,55 @@ +# no-same-owner + +This rule checks that the owner and group do not transfer across hosts. + +In many cases the owner and group on remote hosts do not match the owner and group assigned to source files. +Preserving the owner and group during transfer can result in errors with permissions or leaking sensitive information. + +When you synchronize files, you should avoid transferring the owner and group by setting `owner: false` and `group: false` arguments. +When you unpack archives with the `ansible.builtin.unarchive` module you should set the `--no-same-owner` option. + +This is an opt-in rule. +You must enable it in your Ansible-lint configuration as follows: + +```yaml +enable_list: + - no-same-owner +``` + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Synchronize conf file + ansible.posix.synchronize: + src: /path/conf.yaml + dest: /path/conf.yaml # <- Transfers the owner and group for the file. + - name: Extract tarball to path + ansible.builtin.unarchive: + src: "{{ file }}.tar.gz" + dest: /my/path/ # <- Transfers the owner and group for the file. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Synchronize conf file + ansible.posix.synchronize: + src: /path/conf.yaml + dest: /path/conf.yaml + owner: false + group: false # <- Does not transfer the owner and group for the file. + - name: Extract tarball to path + ansible.builtin.unarchive: + src: "{{ file }}.tar.gz" + dest: /my/path/ + extra_opts: + - --no-same-owner # <- Does not transfer the owner and group for the file. +``` diff --git a/src/ansiblelint/rules/no_same_owner.py b/src/ansiblelint/rules/no_same_owner.py new file mode 100644 index 0000000..021900e --- /dev/null +++ b/src/ansiblelint/rules/no_same_owner.py @@ -0,0 +1,114 @@ +"""Optional rule for avoiding keeping owner/group when transferring files.""" +from __future__ import annotations + +import re +import sys +from typing import TYPE_CHECKING, Any + +from ansible.utils.sentinel import Sentinel + +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class NoSameOwnerRule(AnsibleLintRule): + """Do not preserve the owner and group when transferring files across hosts.""" + + id = "no-same-owner" + description = """ +Optional rule that highlights dangers of assuming that user/group on the remote +machines may not exist on ansible controller or vice versa. Owner and group +should not be preserved when transferring files between them. +""" + severity = "LOW" + tags = ["opt-in"] + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + """Return matches for a task.""" + action = task.get("action") + if not isinstance(action, dict): # pragma: no cover + return False + + module = action["__ansible_module__"] + + if module in ["synchronize", "ansible.posix.synchronize"]: + return self.handle_synchronize(task, action) + + if module in ["unarchive", "ansible.builtin.unarchive"]: + return self.handle_unarchive(task, action) + + return False + + @staticmethod + def handle_synchronize(task: Any, action: dict[str, Any]) -> bool: + """Process a synchronize task.""" + if task.get("delegate_to") != Sentinel: + return False + + archive = action.get("archive", True) + if action.get("owner", archive) or action.get("group", archive): + return True + return False + + @staticmethod + def handle_unarchive(task: Any, action: dict[str, Any]) -> bool: + """Process unarchive task.""" + delegate_to = task.get("delegate_to") + if ( + delegate_to == "localhost" + or delegate_to != "localhost" + and not action.get("remote_src") + ): + src = action.get("src") + if not isinstance(src, str): + return False + + if src.endswith("zip") and "-X" in action.get("extra_opts", []): + return True + if re.search( + r".*\.tar(\.(gz|bz2|xz))?$", + src, + ) and "--no-same-owner" not in action.get("extra_opts", []): + return True + return False + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("test_file", "failures"), + ( + pytest.param( + "examples/roles/role_for_no_same_owner/tasks/fail.yml", + 12, + id="fail", + ), + pytest.param( + "examples/roles/role_for_no_same_owner/tasks/pass.yml", + 0, + id="pass", + ), + ), + ) + def test_no_same_owner_rule( + default_rules_collection: RulesCollection, + test_file: str, + failures: int, + ) -> None: + """Test rule matches.""" + results = Runner(test_file, rules=default_rules_collection).run() + assert len(results) == failures + for result in results: + assert result.message == NoSameOwnerRule().shortdesc diff --git a/src/ansiblelint/rules/no_tabs.md b/src/ansiblelint/rules/no_tabs.md new file mode 100644 index 0000000..7895122 --- /dev/null +++ b/src/ansiblelint/rules/no_tabs.md @@ -0,0 +1,38 @@ +# no-tabs + +This rule checks for the tab character. The `\t` tab character can result in +unexpected display or formatting issues. You should always use spaces instead of +tabs. + +!!! note + + This rule does not trigger alerts for tab characters in the ``ansible.builtin.lineinfile`` module. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Do not trigger the rule + ansible.builtin.lineinfile: + path: some.txt + regexp: '^\t$' + line: 'string with \t inside' + - name: Trigger the rule with a debug message + ansible.builtin.debug: + msg: "Using the \t character can cause formatting issues." # <- Includes the tab character. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Do not trigger the no-tabs rule + ansible.builtin.debug: + msg: "Using space characters avoids formatting issues." +``` diff --git a/src/ansiblelint/rules/no_tabs.py b/src/ansiblelint/rules/no_tabs.py new file mode 100644 index 0000000..c53f1bb --- /dev/null +++ b/src/ansiblelint/rules/no_tabs.py @@ -0,0 +1,67 @@ +"""Implementation of no-tabs rule.""" +# Copyright (c) 2016, Will Thames and contributors +# Copyright (c) 2018, Ansible Project +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.yaml_utils import nested_items_path + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class NoTabsRule(AnsibleLintRule): + """Most files should not contain tabs.""" + + id = "no-tabs" + description = "Tabs can cause unexpected display issues, use spaces" + severity = "LOW" + tags = ["formatting"] + version_added = "v4.0.0" + allow_list = [ + ("lineinfile", "insertafter"), + ("lineinfile", "insertbefore"), + ("lineinfile", "regexp"), + ("lineinfile", "line"), + ("ansible.builtin.lineinfile", "insertafter"), + ("ansible.builtin.lineinfile", "insertbefore"), + ("ansible.builtin.lineinfile", "regexp"), + ("ansible.builtin.lineinfile", "line"), + ("ansible.legacy.lineinfile", "insertafter"), + ("ansible.legacy.lineinfile", "insertbefore"), + ("ansible.legacy.lineinfile", "regexp"), + ("ansible.legacy.lineinfile", "line"), + ] + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + action = task["action"]["__ansible_module__"] + for k, v, _ in nested_items_path(task): + if isinstance(k, str) and "\t" in k: + return True + if isinstance(v, str) and "\t" in v and (action, k) not in self.allow_list: + return True + return False + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + def test_no_tabs_rule(default_rules_collection: RulesCollection) -> None: + """Test rule matches.""" + results = Runner( + "examples/playbooks/rule-no-tabs.yml", + rules=default_rules_collection, + ).run() + assert results[0].lineno == 10 + assert results[0].message == NoTabsRule().shortdesc + assert len(results) == 2 diff --git a/src/ansiblelint/rules/only_builtins.md b/src/ansiblelint/rules/only_builtins.md new file mode 100644 index 0000000..750e194 --- /dev/null +++ b/src/ansiblelint/rules/only_builtins.md @@ -0,0 +1,36 @@ +# only-builtins + +This rule checks that playbooks use actions from the `ansible.builtin` collection only. + +This is an opt-in rule. +You must enable it in your Ansible-lint configuration as follows: + +```yaml +enable_list: + - only-builtins +``` + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: all + tasks: + - name: Deploy a Helm chart for Prometheus + kubernetes.core.helm: # <- Uses a non-builtin collection. + name: test + chart_ref: stable/prometheus + release_namespace: monitoring + create_namespace: true +``` + +## Correct Code + +```yaml +- name: Example playbook + hosts: localhost + tasks: + - name: Run a shell command + ansible.builtin.shell: echo This playbook uses actions from the builtin collection only. +``` diff --git a/src/ansiblelint/rules/only_builtins.py b/src/ansiblelint/rules/only_builtins.py new file mode 100644 index 0000000..78ad93a --- /dev/null +++ b/src/ansiblelint/rules/only_builtins.py @@ -0,0 +1,106 @@ +"""Rule definition for usage of builtin actions only.""" +from __future__ import annotations + +import os +import sys +from typing import TYPE_CHECKING + +from ansiblelint.config import options +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.rules.fqcn import builtins +from ansiblelint.skip_utils import is_nested_task + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class OnlyBuiltinsRule(AnsibleLintRule): + """Use only builtin actions.""" + + id = "only-builtins" + severity = "MEDIUM" + description = "Check whether the playbook uses anything but ``ansible.builtin``" + tags = ["opt-in", "experimental"] + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + module = task["action"]["__ansible_module_original__"] + + allowed_collections = [ + "ansible.builtin", + "ansible.legacy", + *options.only_builtins_allow_collections, + ] + allowed_modules = builtins + options.only_builtins_allow_modules + + is_allowed = ( + any(module.startswith(f"{prefix}.") for prefix in allowed_collections) + or module in allowed_modules + ) + + return not is_allowed and not is_nested_task(task) + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + # pylint: disable=ungrouped-imports + import pytest + + from ansiblelint.constants import RC + from ansiblelint.testing import RunFromText, run_ansible_lint + + SUCCESS_PLAY = """ +- hosts: localhost + tasks: + - name: A block + block: + - name: Shell (fqcn) + ansible.builtin.shell: echo This rule should not get matched by the only-builtins rule + - name: Command with legacy FQCN + ansible.legacy.command: echo This rule should not get matched by the only-builtins rule + """ + + def test_only_builtins_fail() -> None: + """Test rule matches.""" + env = os.environ.copy() + env["NO_COLOR"] = "1" + result = run_ansible_lint( + "--strict", + "--warn-list=", + "--enable-list", + "only-builtins", + "examples/playbooks/rule-only-builtins.yml", + env=env, + ) + assert result.returncode == RC.VIOLATIONS_FOUND + assert "Failed" in result.stderr + assert "warning(s)" in result.stderr + assert "only-builtins: Use only builtin actions" in result.stdout + + def test_only_builtins_allow() -> None: + """Test rule doesn't match.""" + conf_path = "examples/playbooks/.ansible-lint-only-builtins-allow" + result = run_ansible_lint( + f"--config-file={conf_path}", + "--strict", + "--warn-list=", + "--enable-list", + "only-builtins", + "examples/playbooks/rule-only-builtins.yml", + ) + assert "only-builtins" not in result.stdout + assert result.returncode == RC.SUCCESS + + @pytest.mark.parametrize( + "rule_runner", + (OnlyBuiltinsRule,), + indirect=["rule_runner"], + ) + def test_only_builtin_pass(rule_runner: RunFromText) -> None: + """Test rule does not match.""" + results = rule_runner.run_playbook(SUCCESS_PLAY) + assert len(results) == 0, results diff --git a/src/ansiblelint/rules/package_latest.md b/src/ansiblelint/rules/package_latest.md new file mode 100644 index 0000000..c7e0d82 --- /dev/null +++ b/src/ansiblelint/rules/package_latest.md @@ -0,0 +1,71 @@ +# package-latest + +This rule checks that package managers install software in a controlled, safe manner. + +Package manager modules, such as `ansible.builtin.yum`, include a `state` parameter that configures how Ansible installs software. +In production environments, you should set `state` to `present` and specify a target version to ensure that packages are installed to a planned and tested version. + +Setting `state` to `latest` not only installs software, it performs an update and installs additional packages. +This can result in performance degradation or loss of service. +If you do want to update packages to the latest version, you should also set the `update_only` parameter to `true` to avoid installing additional packages. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Install Ansible + ansible.builtin.yum: + name: ansible + state: latest # <- Installs the latest package. + + - name: Install Ansible-lint + ansible.builtin.pip: + name: ansible-lint + args: + state: latest # <- Installs the latest package. + + - name: Install some-package + ansible.builtin.package: + name: some-package + state: latest # <- Installs the latest package. + + - name: Install Ansible with update_only to false + ansible.builtin.yum: + name: sudo + state: latest + update_only: false # <- Updates and installs packages. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Install Ansible + ansible.builtin.yum: + name: ansible-2.12.7.0 + state: present # <- Pins the version to install with yum. + + - name: Install Ansible-lint + ansible.builtin.pip: + name: ansible-lint + args: + state: present + version: 5.4.0 # <- Pins the version to install with pip. + + - name: Install some-package + ansible.builtin.package: + name: some-package + state: present # <- Ensures the package is installed. + + - name: Update Ansible with update_only to true + ansible.builtin.yum: + name: sudo + state: latest + update_only: true # <- Updates but does not install additional packages. +``` diff --git a/src/ansiblelint/rules/package_latest.py b/src/ansiblelint/rules/package_latest.py new file mode 100644 index 0000000..a00a540 --- /dev/null +++ b/src/ansiblelint/rules/package_latest.py @@ -0,0 +1,83 @@ +"""Implementations of the package-latest rule.""" +# Copyright (c) 2016 Will Thames <will@thames.id.au> +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class PackageIsNotLatestRule(AnsibleLintRule): + """Package installs should not use latest.""" + + id = "package-latest" + description = ( + "Package installs should use ``state=present`` with or without a version" + ) + severity = "VERY_LOW" + tags = ["idempotency"] + version_added = "historic" + + _package_managers = [ + "apk", + "apt", + "bower", + "bundler", + "dnf", + "easy_install", + "gem", + "homebrew", + "jenkins_plugin", + "npm", + "openbsd_package", + "openbsd_pkg", + "package", + "pacman", + "pear", + "pip", + "pkg5", + "pkgutil", + "portage", + "slackpkg", + "sorcery", + "swdepot", + "win_chocolatey", + "yarn", + "yum", + "zypper", + ] + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + return ( + task["action"]["__ansible_module__"] in self._package_managers + and not task["action"].get("version") + and not task["action"].get("update_only") + and task["action"].get("state") == "latest" + ) diff --git a/src/ansiblelint/rules/partial_become.md b/src/ansiblelint/rules/partial_become.md new file mode 100644 index 0000000..01f9dae --- /dev/null +++ b/src/ansiblelint/rules/partial_become.md @@ -0,0 +1,42 @@ +# partial-become + +This rule checks that privilege escalation is activated when changing users. + +To perform an action as a different user with the `become_user` directive, you +must set `become: true`. + +!!! warning + + While Ansible inherits have of `become` and `become_user` from upper levels, + like play level or command line, we do not look at these values. This rule + requires you to be explicit and always define both in the same place, mainly + in order to prevent accidents when some tasks are moved from one location to + another one. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Start the httpd service as the apache user + ansible.builtin.service: + name: httpd + state: started + become_user: apache # <- Does not change the user because "become: true" is not set. +``` + +## Correct Code + +```yaml +- name: Example playbook + hosts: localhost + tasks: + - name: Start the httpd service as the apache user + ansible.builtin.service: + name: httpd + state: started + become: true # <- Activates privilege escalation. + become_user: apache # <- Changes the user with the desired privileges. +``` diff --git a/src/ansiblelint/rules/partial_become.py b/src/ansiblelint/rules/partial_become.py new file mode 100644 index 0000000..d14c06f --- /dev/null +++ b/src/ansiblelint/rules/partial_become.py @@ -0,0 +1,138 @@ +"""Implementation of partial-become rule.""" +# Copyright (c) 2016 Will Thames <will@thames.id.au> +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. +from __future__ import annotations + +import sys +from functools import reduce +from typing import TYPE_CHECKING, Any + +from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + + +def _get_subtasks(data: dict[str, Any]) -> list[Any]: + result: list[Any] = [] + block_names = [ + "tasks", + "pre_tasks", + "post_tasks", + "handlers", + "block", + "always", + "rescue", + ] + for name in block_names: + if data and name in data: + result += data[name] or [] + return result + + +def _nested_search(term: str, data: dict[str, Any]) -> Any: + if data and term in data: + return True + return reduce( + (lambda x, y: x or _nested_search(term, y)), + _get_subtasks(data), + False, + ) + + +def _become_user_without_become(becomeuserabove: bool, data: dict[str, Any]) -> Any: + if "become" in data: + # If become is in lineage of tree then correct + return False + if "become_user" in data and _nested_search("become", data): + # If 'become_user' on tree and become somewhere below + # we must check for a case of a second 'become_user' without a + # 'become' in its lineage + subtasks = _get_subtasks(data) + return reduce( + (lambda x, y: x or _become_user_without_become(False, y)), + subtasks, + False, + ) + if _nested_search("become_user", data): + # Keep searching down if 'become_user' exists in the tree below current task + subtasks = _get_subtasks(data) + return len(subtasks) == 0 or reduce( + ( + lambda x, y: x + or _become_user_without_become( + becomeuserabove or "become_user" in data, + y, + ) + ), + subtasks, + False, + ) + # If at bottom of tree, flag up if 'become_user' existed in the lineage of the tree and + # 'become' was not. This is an error if any lineage has a 'become_user' but no become + return becomeuserabove + + +class BecomeUserWithoutBecomeRule(AnsibleLintRule): + """become_user requires become to work as expected.""" + + id = "partial-become" + description = "``become_user`` without ``become`` will not actually change user" + severity = "VERY_HIGH" + tags = ["unpredictability"] + version_added = "historic" + + def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + if file.kind == "playbook": + result = _become_user_without_become(False, data) + if result: + return [ + self.create_matcherror( + message=self.shortdesc, + filename=file, + lineno=data[LINE_NUMBER_KEY], + ), + ] + return [] + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + def test_partial_become_positive() -> None: + """Positive test for partial-become.""" + collection = RulesCollection() + collection.register(BecomeUserWithoutBecomeRule()) + success = "examples/playbooks/rule-partial-become-without-become-pass.yml" + good_runner = Runner(success, rules=collection) + assert [] == good_runner.run() + + def test_partial_become_negative() -> None: + """Negative test for partial-become.""" + collection = RulesCollection() + collection.register(BecomeUserWithoutBecomeRule()) + failure = "examples/playbooks/rule-partial-become-without-become-fail.yml" + bad_runner = Runner(failure, rules=collection) + errs = bad_runner.run() + assert len(errs) == 3 diff --git a/src/ansiblelint/rules/playbook_extension.md b/src/ansiblelint/rules/playbook_extension.md new file mode 100644 index 0000000..dd0e475 --- /dev/null +++ b/src/ansiblelint/rules/playbook_extension.md @@ -0,0 +1,14 @@ +# playbook-extension + +This rule checks the file extension for playbooks is either `.yml` or `.yaml`. +Ansible playbooks are expressed in YAML format with minimal syntax. + +The [YAML syntax](https://docs.ansible.com/ansible/latest/reference_appendices/YAMLSyntax.html#yaml-syntax) reference provides additional detail. + +## Problematic Code + +This rule is triggered if Ansible playbooks do not have a file extension or use an unsupported file extension such as `playbook.json` or `playbook.xml`. + +## Correct Code + +Save Ansible playbooks as valid YAML with the `.yml` or `.yaml` file extension. diff --git a/src/ansiblelint/rules/playbook_extension.py b/src/ansiblelint/rules/playbook_extension.py new file mode 100644 index 0000000..b4ca41c --- /dev/null +++ b/src/ansiblelint/rules/playbook_extension.py @@ -0,0 +1,55 @@ +"""Implementation of playbook-extension rule.""" +# Copyright (c) 2016, Tsukinowa Inc. <info@tsukinowa.jp> +# Copyright (c) 2018, Ansible Project +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from ansiblelint.file_utils import Lintable +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.runner import Runner + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + + +class PlaybookExtensionRule(AnsibleLintRule): + """Use ".yml" or ".yaml" playbook extension.""" + + id = "playbook-extension" + description = 'Playbooks should have the ".yml" or ".yaml" extension' + severity = "MEDIUM" + tags = ["formatting"] + done: list[str] = [] + version_added = "v4.0.0" + + def matchyaml(self, file: Lintable) -> list[MatchError]: + result: list[MatchError] = [] + if file.kind != "playbook": + return result + path = str(file.path) + ext = file.path.suffix + if ext not in [".yml", ".yaml"] and path not in self.done: + self.done.append(path) + result.append(self.create_matcherror(filename=file)) + return result + + +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("file", "expected"), + (pytest.param("examples/playbooks/play-without-extension", 1, id="fail"),), + ) + def test_playbook_extension(file: str, expected: int) -> None: + """The ini_file module does not accept preserve mode.""" + rules = RulesCollection() + rules.register(PlaybookExtensionRule()) + results = Runner(Lintable(file, kind="playbook"), rules=rules).run() + assert len(results) == expected + for result in results: + assert result.tag == "playbook-extension" diff --git a/src/ansiblelint/rules/risky_file_permissions.md b/src/ansiblelint/rules/risky_file_permissions.md new file mode 100644 index 0000000..2a62a6d --- /dev/null +++ b/src/ansiblelint/rules/risky_file_permissions.md @@ -0,0 +1,60 @@ +# risky-file-permissions + +This rule is triggered by various modules that could end up creating new files +on disk with permissions that might be too open, or unpredictable. Please read +the documentation of each module carefully to understand the implications of +using different argument values, as these make the difference between using the +module safely or not. The fix depends on each module and also your particular +situation. + +Some modules have a `create` argument that defaults to `true`. For those you +either need to set `create: false` or provide some permissions like `mode: 0600` +to make the behavior predictable and not dependent on the current system +settings. + +Modules that are checked: + +- [`ansible.builtin.assemble`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/assemble_module.html) +- [`ansible.builtin.copy`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/copy_module.html) +- [`ansible.builtin.file`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/file_module.html) +- [`ansible.builtin.get_url`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/get_url_module.html) +- [`ansible.builtin.replace`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/replace_module.html) +- [`ansible.builtin.template`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/template_module.html) +- [`community.general.archive`](https://docs.ansible.com/ansible/latest/collections/community/general/archive_module.html) +- [`community.general.ini_file`](https://docs.ansible.com/ansible/latest/collections/community/general/ini_file_module.html) + +!!! warning + + This rule does not take [module_defaults](https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_module_defaults.html) configuration into account. + There are currently no plans to implement this feature because changing task location can also change task behavior. + +## Problematic code + +```yaml +--- +- name: Unsafe example of using ini_file + community.general.ini_file: + path: foo + create: true +``` + +## Correct code + +```yaml +--- +- name: Safe example of using ini_file (1st solution) + community.general.ini_file: + path: foo + create: false # prevents creating a file with potentially insecure permissions + +- name: Safe example of using ini_file (2nd solution) + community.general.ini_file: + path: foo + mode: 0600 # explicitly sets the desired permissions, to make the results predictable + +- name: Safe example of using copy (3rd solution) + ansible.builtin.copy: + src: foo + dest: bar + mode: preserve # copy has a special mode that sets the same permissions as the source file +``` diff --git a/src/ansiblelint/rules/risky_file_permissions.py b/src/ansiblelint/rules/risky_file_permissions.py new file mode 100644 index 0000000..f4494eb --- /dev/null +++ b/src/ansiblelint/rules/risky_file_permissions.py @@ -0,0 +1,168 @@ +# Copyright (c) 2020 Sorin Sbarnea <sorin.sbarnea@gmail.com> +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. +"""MissingFilePermissionsRule used with ansible-lint.""" +from __future__ import annotations + +import sys +from pathlib import Path +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +# Despite documentation mentioning 'preserve' only these modules support it: +_modules_with_preserve = ( + "copy", + "template", +) + +_MODULES: set[str] = { + "archive", + "community.general.archive", + "assemble", + "ansible.builtin.assemble", + "copy", # supports preserve + "ansible.builtin.copy", + "file", + "ansible.builtin.file", + "get_url", + "ansible.builtin.get_url", + "replace", # implicit preserve behavior but mode: preserve is invalid + "ansible.builtin.replace", + "template", # supports preserve + "ansible.builtin.template", + # 'unarchive', # disabled because .tar.gz files can have permissions inside +} + +_MODULES_WITH_CREATE: dict[str, bool] = { + "blockinfile": False, + "ansible.builtin.blockinfile": False, + "htpasswd": True, + "community.general.htpasswd": True, + "ini_file": True, + "community.general.ini_file": True, + "lineinfile": False, + "ansible.builtin.lineinfile": False, +} + + +class MissingFilePermissionsRule(AnsibleLintRule): + """File permissions unset or incorrect.""" + + id = "risky-file-permissions" + description = ( + "Missing or unsupported mode parameter can cause unexpected file " + "permissions based " + "on version of Ansible being used. Be explicit, like `mode: 0644` to " + "avoid hitting this rule. Special `preserve` value is accepted " + f"only by {', '.join([f'`{x}`' for x in _modules_with_preserve])} modules." + ) + link = "https://github.com/ansible/ansible/issues/71200" + severity = "VERY_HIGH" + tags = ["unpredictability"] + version_added = "v4.3.0" + + _modules = _MODULES + _modules_with_create = _MODULES_WITH_CREATE + + # pylint: disable=too-many-return-statements + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + module = task["action"]["__ansible_module__"] + mode = task["action"].get("mode", None) + + if not isinstance(task.args, dict): + # We are unable to check args when using jinja templating + return False + + if module not in self._modules and module not in self._modules_with_create: + return False + + if mode == "preserve" and module not in _modules_with_preserve: + return True + + if module in self._modules_with_create: + create = task["action"].get("create", self._modules_with_create[module]) + return create and mode is None + + # A file that doesn't exist cannot have a mode + if task["action"].get("state", None) == "absent": + return False + + # A symlink always has mode 0777 + if task["action"].get("state", None) == "link": + return False + + # Recurse on a directory does not allow for an uniform mode + if task["action"].get("recurse", None): + return False + + # The file module does not create anything when state==file (default) + if module == "file" and task["action"].get("state", "file") == "file": + return False + + # replace module is the only one that has a valid default preserve + # behavior, but we want to trigger rule if user used incorrect + # documentation and put 'preserve', which is not supported. + if module == "replace" and mode is None: + return False + + return mode is None + + +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.testing import RunFromText # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("file", "expected"), + ( + pytest.param( + "examples/playbooks/rule-risky-file-permissions-pass.yml", + 0, + id="pass", + ), + pytest.param( + "examples/playbooks/rule-risky-file-permissions-fail.yml", + 11, + id="fails", + ), + ), + ) + def test_risky_file_permissions( + file: str, + expected: int, + default_rules_collection: RulesCollection, + ) -> None: + """The ini_file module does not accept preserve mode.""" + runner = RunFromText(default_rules_collection) + results = runner.run(Path(file)) + assert len(results) == expected + for result in results: + assert result.tag == "risky-file-permissions" diff --git a/src/ansiblelint/rules/risky_octal.md b/src/ansiblelint/rules/risky_octal.md new file mode 100644 index 0000000..a2f22eb --- /dev/null +++ b/src/ansiblelint/rules/risky_octal.md @@ -0,0 +1,49 @@ +# risky-octal + +This rule checks that octal file permissions are strings that contain a leading +zero or are written in +[symbolic modes](https://www.gnu.org/software/findutils/manual/html_node/find_html/Symbolic-Modes.html), +such as `u+rwx` or `u=rw,g=r,o=r`. + +Using integers or octal values in YAML can result in unexpected behavior. For +example, the YAML loader interprets `0644` as the decimal number `420` but +putting `644` there will produce very different results. + +Modules that are checked: + +- [`ansible.builtin.assemble`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/assemble_module.html) +- [`ansible.builtin.copy`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/copy_module.html) +- [`ansible.builtin.file`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/file_module.html) +- [`ansible.builtin.replace`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/replace_module.html) +- [`ansible.builtin.template`](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/template_module.html) + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Unsafe example of declaring Numeric file permissions + ansible.builtin.file: + path: /etc/foo.conf + owner: foo + group: foo + mode: 644 +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Safe example of declaring Numeric file permissions (1st solution) + ansible.builtin.file: + path: /etc/foo.conf + owner: foo + group: foo + mode: "0644" # <- quoting and the leading zero will prevent surprises + # "0o644" is also a valid alternative. +``` diff --git a/src/ansiblelint/rules/risky_octal.py b/src/ansiblelint/rules/risky_octal.py new file mode 100644 index 0000000..e3651ea --- /dev/null +++ b/src/ansiblelint/rules/risky_octal.py @@ -0,0 +1,196 @@ +"""Implementation of risky-octal rule.""" +# Copyright (c) 2013-2014 Will Thames <will@thames.id.au> +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. + +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule, RulesCollection +from ansiblelint.runner import Runner + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class OctalPermissionsRule(AnsibleLintRule): + """Octal file permissions must contain leading zero or be a string.""" + + id = "risky-octal" + description = ( + "Numeric file permissions without leading zero can behave " + "in unexpected ways." + ) + link = "https://docs.ansible.com/ansible/latest/collections/ansible/builtin/file_module.html" + severity = "VERY_HIGH" + tags = ["formatting"] + version_added = "historic" + + _modules = [ + "assemble", + "copy", + "file", + "ini_file", + "lineinfile", + "replace", + "synchronize", + "template", + "unarchive", + ] + + @staticmethod + def is_invalid_permission(mode: int) -> bool: + """Check if permissions are valid. + + Sensible file permission modes don't have write bit set when read bit + is not set and don't have execute bit set when user execute bit is + not set. + + Also, user permissions are more generous than group permissions and + user and group permissions are more generous than world permissions. + """ + other_write_without_read = ( + mode % 8 and mode % 8 < 4 and not (mode % 8 == 1 and (mode >> 6) % 2 == 1) + ) + group_write_without_read = ( + (mode >> 3) % 8 + and (mode >> 3) % 8 < 4 + and not ((mode >> 3) % 8 == 1 and (mode >> 6) % 2 == 1) + ) + user_write_without_read = ( + (mode >> 6) % 8 and (mode >> 6) % 8 < 4 and (mode >> 6) % 8 != 1 + ) + other_more_generous_than_group = mode % 8 > (mode >> 3) % 8 + other_more_generous_than_user = mode % 8 > (mode >> 6) % 8 + group_more_generous_than_user = (mode >> 3) % 8 > (mode >> 6) % 8 + + return bool( + other_write_without_read + or group_write_without_read + or user_write_without_read + or other_more_generous_than_group + or other_more_generous_than_user + or group_more_generous_than_user, + ) + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + if task["action"]["__ansible_module__"] in self._modules: + mode = task["action"].get("mode", None) + + if isinstance(mode, str): + return False + + if isinstance(mode, int) and self.is_invalid_permission(mode): + return f'`mode: {mode}` should have a string value with leading zero `mode: "0{mode:o}"` or use symbolic mode.' + return False + + +if "pytest" in sys.modules: + import pytest + + VALID_MODES = [ + 0o777, + 0o775, + 0o770, + 0o755, + 0o750, + 0o711, + 0o710, + 0o700, + 0o666, + 0o664, + 0o660, + 0o644, + 0o640, + 0o600, + 0o555, + 0o551, + 0o550, + 0o511, + 0o510, + 0o500, + 0o444, + 0o440, + 0o400, + ] + + INVALID_MODES = [ + 777, + 775, + 770, + 755, + 750, + 711, + 710, + 700, + 666, + 664, + 660, + 644, + 640, + 622, + 620, + 600, + 555, + 551, + 550, # 511 == 0o777, 510 == 0o776, 500 == 0o764 + 444, + 440, + 400, + ] + + @pytest.mark.parametrize( + ("file", "failures"), + ( + pytest.param("examples/playbooks/rule-risky-octal-pass.yml", 0, id="pass"), + pytest.param("examples/playbooks/rule-risky-octal-fail.yml", 4, id="fail"), + ), + ) + def test_octal(file: str, failures: int) -> None: + """Test that octal permissions are valid.""" + collection = RulesCollection() + collection.register(OctalPermissionsRule()) + results = Runner(file, rules=collection).run() + + assert len(results) == failures + for result in results: + assert result.rule.id == "risky-octal" + + def test_octal_valid_modes() -> None: + """Test that octal modes are valid.""" + rule = OctalPermissionsRule() + for mode in VALID_MODES: + assert not rule.is_invalid_permission( + mode, + ), f"0o{mode:o} should be a valid mode" + + def test_octal_invalid_modes() -> None: + """Test that octal modes are invalid.""" + rule = OctalPermissionsRule() + for mode in INVALID_MODES: + assert rule.is_invalid_permission( + mode, + ), f"{mode:d} should be an invalid mode" diff --git a/src/ansiblelint/rules/risky_shell_pipe.md b/src/ansiblelint/rules/risky_shell_pipe.md new file mode 100644 index 0000000..302d0d9 --- /dev/null +++ b/src/ansiblelint/rules/risky_shell_pipe.md @@ -0,0 +1,39 @@ +# risky-shell-pipe + +This rule checks for the bash `pipefail` option with the Ansible `shell` module. + +You should always set `pipefail` when piping output from one command to another. +The return status of a pipeline is the exit status of the command. The +`pipefail` option ensures that tasks fail as expected if the first command +fails. + +As this requirement does apply to PowerShell, for shell commands that have +`pwsh` inside `executable` attribute, this rule will not trigger. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: localhost + tasks: + - name: Pipeline without pipefail + ansible.builtin.shell: false | cat +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: localhost + become: false + tasks: + - name: Pipeline with pipefail + ansible.builtin.shell: set -o pipefail && false | cat + + - name: Pipeline with pipefail, multi-line + ansible.builtin.shell: | + set -o pipefail # <-- adding this will prevent surprises + false | cat +``` diff --git a/src/ansiblelint/rules/risky_shell_pipe.py b/src/ansiblelint/rules/risky_shell_pipe.py new file mode 100644 index 0000000..58a6f5f --- /dev/null +++ b/src/ansiblelint/rules/risky_shell_pipe.py @@ -0,0 +1,93 @@ +"""Implementation of risky-shell-pipe rule.""" +from __future__ import annotations + +import re +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.utils import convert_to_boolean, get_cmd_args + +if TYPE_CHECKING: + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class ShellWithoutPipefail(AnsibleLintRule): + """Shells that use pipes should set the pipefail option.""" + + id = "risky-shell-pipe" + description = ( + "Without the pipefail option set, a shell command that " + "implements a pipeline can fail and still return 0. If " + "any part of the pipeline other than the terminal command " + "fails, the whole pipeline will still return 0, which may " + "be considered a success by Ansible. " + "Pipefail is available in the bash shell." + ) + severity = "MEDIUM" + tags = ["command-shell"] + version_added = "v4.1.0" + + _pipefail_re = re.compile(r"^\s*set.*[+-][A-Za-z]*o\s*pipefail", re.M) + _pipe_re = re.compile(r"(?<!\|)\|(?!\|)") + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str: + if task["__ansible_action_type__"] != "task": + return False + + if task["action"]["__ansible_module__"] != "shell": + return False + + if task.get("ignore_errors"): + return False + + jinja_stripped_cmd = self.unjinja(get_cmd_args(task)) + + # https://github.com/ansible/ansible-lint/issues/3161 + if "pwsh" in task["action"].get("executable", ""): + return False + + return bool( + self._pipe_re.search(jinja_stripped_cmd) + and not self._pipefail_re.search(jinja_stripped_cmd) + and not convert_to_boolean(task["action"].get("ignore_errors", False)), + ) + + +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("file", "expected"), + ( + pytest.param( + "examples/playbooks/rule-risky-shell-pipe-pass.yml", + 0, + id="pass", + ), + pytest.param( + "examples/playbooks/rule-risky-shell-pipe-fail.yml", + 3, + id="fail", + ), + ), + ) + def test_risky_shell_pipe( + default_rules_collection: RulesCollection, + file: str, + expected: int, + ) -> None: + """Validate that rule works as intended.""" + results = Runner(file, rules=default_rules_collection).run() + + for result in results: + assert result.rule.id == ShellWithoutPipefail.id, result + assert len(results) == expected diff --git a/src/ansiblelint/rules/role_name.md b/src/ansiblelint/rules/role_name.md new file mode 100644 index 0000000..28aa8b8 --- /dev/null +++ b/src/ansiblelint/rules/role_name.md @@ -0,0 +1,36 @@ +# role-name + +This rule checks role names to ensure they conform with requirements. + +Role names must contain only lowercase alphanumeric characters and the underscore `_` character. +Role names must also start with an alphabetic character. + +For more information see the [roles directory](https://docs.ansible.com/ansible/devel/dev_guide/developing_collections_structure.html#roles-directory) topic in Ansible documentation. + +`role-name[path]` message tells you to avoid using paths when importing roles. +You should only rely on Ansible's ability to find the role and refer to them +using fully qualified names. + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: localhost + roles: + - 1myrole # <- Does not start with an alphabetic character. + - myrole2[*^ # <- Contains invalid special characters. + - myRole_3 # <- Contains uppercase alphabetic characters. +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: localhost + roles: + - myrole1 # <- Starts with an alphabetic character. + - myrole2 # <- Contains only alphanumeric characters. + - myrole_3 # <- Contains only lowercase alphabetic characters. +``` diff --git a/src/ansiblelint/rules/role_name.py b/src/ansiblelint/rules/role_name.py new file mode 100644 index 0000000..499c086 --- /dev/null +++ b/src/ansiblelint/rules/role_name.py @@ -0,0 +1,170 @@ +"""Implementation of role-name rule.""" +# Copyright (c) 2020 Gael Chamoulaud <gchamoul@redhat.com> +# Copyright (c) 2020 Sorin Sbarnea <ssbarnea@redhat.com> +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in +# all copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +# THE SOFTWARE. +from __future__ import annotations + +import re +import sys +from functools import cache +from typing import TYPE_CHECKING + +from ansiblelint.constants import ROLE_IMPORT_ACTION_NAMES +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.utils import parse_yaml_from_file + +if TYPE_CHECKING: + from pathlib import Path + + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +ROLE_NAME_REGEX = re.compile(r"^[a-z][a-z0-9_]*$") + + +def _remove_prefix(text: str, prefix: str) -> str: + return re.sub(rf"^{re.escape(prefix)}", "", text) + + +@cache +def _match_role_name_regex(role_name: str) -> bool: + return ROLE_NAME_REGEX.match(role_name) is not None + + +class RoleNames(AnsibleLintRule): + """Role name {0} does not match ``^[a-z][a-z0-9_]*$`` pattern.""" + + id = "role-name" + description = ( + "Role names are now limited to contain only lowercase alphanumeric " + "characters, plus underline and start with an alpha character." + ) + link = "https://docs.ansible.com/ansible/devel/dev_guide/developing_collections_structure.html#roles-directory" + severity = "HIGH" + tags = ["deprecations", "metadata"] + version_added = "v6.8.5" + _ids = { + "role-name[path]": "Avoid using paths when importing roles.", + } + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + results = [] + if task["action"]["__ansible_module__"] in ROLE_IMPORT_ACTION_NAMES: + name = task["action"].get("name", "") + if "/" in name: + results.append( + self.create_matcherror( + f"Avoid using paths when importing roles. ({name})", + filename=file, + lineno=task["action"].get("__line__", task["__line__"]), + tag=f"{self.id}[path]", + ), + ) + return results + + def matchdir(self, lintable: Lintable) -> list[MatchError]: + return self.matchyaml(lintable) + + def matchyaml(self, file: Lintable) -> list[MatchError]: + result: list[MatchError] = [] + + if file.kind not in ("meta", "role", "playbook"): + return result + + if file.kind == "playbook": + for play in file.data: + if "roles" in play: + line = play["__line__"] + for role in play["roles"]: + if isinstance(role, dict): + line = role["__line__"] + role_name = role["role"] + elif isinstance(role, str): + role_name = role + if "/" in role_name: + result.append( + self.create_matcherror( + f"Avoid using paths when importing roles. ({role_name})", + filename=file, + lineno=line, + tag=f"{self.id}[path]", + ), + ) + return result + + if file.kind == "role": + role_name = self._infer_role_name( + meta=file.path / "meta" / "main.yml", + default=file.path.name, + ) + else: + role_name = self._infer_role_name( + meta=file.path, + default=file.path.resolve().parents[1].name, + ) + + role_name = _remove_prefix(role_name, "ansible-role-") + if role_name and not _match_role_name_regex(role_name): + result.append( + self.create_matcherror( + filename=file, + message=self.shortdesc.format(role_name), + ), + ) + return result + + @staticmethod + def _infer_role_name(meta: Path, default: str) -> str: + if meta.is_file(): + meta_data = parse_yaml_from_file(str(meta)) + if meta_data: + try: + return str(meta_data["galaxy_info"]["role_name"]) + except KeyError: + pass + return default + + +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("test_file", "failure"), + (pytest.param("examples/playbooks/rule-role-name-path.yml", 3, id="fail"),), + ) + def test_role_name_path( + default_rules_collection: RulesCollection, + test_file: str, + failure: int, + ) -> None: + """Test rule matches.""" + results = Runner(test_file, rules=default_rules_collection).run() + for result in results: + assert result.tag == "role-name[path]" + assert len(results) == failure diff --git a/src/ansiblelint/rules/run_once.md b/src/ansiblelint/rules/run_once.md new file mode 100644 index 0000000..024648b --- /dev/null +++ b/src/ansiblelint/rules/run_once.md @@ -0,0 +1,65 @@ +# run-once + +This rule warns against the use of `run_once` when the `strategy` is set to +`free`. + +This rule can produce the following messages: + +- `run-once[play]`: Play uses `strategy: free`. +- `run-once[task]`: Using `run_once` may behave differently if the `strategy` is + set to `free`. + +For more information see the following topics in Ansible documentation: + +- [free strategy](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/free_strategy.html#free-strategy) +- [selecting a strategy](https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_strategies.html#selecting-a-strategy) +- [run_once(playbook keyword) more info](https://docs.ansible.com/ansible/latest/reference_appendices/playbooks_keywords.html) + +!!! warning + + The reason for the existence of this rule is for reminding users that `run_once` + is not providing any warranty that the task will run only once. + This rule will always trigger regardless of the value configured inside the 'strategy' field. That is because the effective value used at runtime can be different than the value inside the file. For example, ansible command line arguments can alter it. + +It is perfectly fine to add `# noqa: run-once[task]` to mark the warning as +acknowledged and ignored. + +## Problematic Code + +```yaml +--- +- name: "Example with run_once" + hosts: all + strategy: free # <-- avoid use of strategy as free + gather_facts: false + tasks: + - name: Task with run_once + ansible.builtin.debug: + msg: "Test" + run_once: true # <-- avoid use of strategy as free at play level when using run_once at task level +``` + +## Correct Code + +```yaml +- name: "Example without run_once" + hosts: all + gather_facts: false + tasks: + - name: Task without run_once + ansible.builtin.debug: + msg: "Test" +``` + +```yaml +- name: "Example of using run_once with strategy other than free" + hosts: all + strategy: linear + # strategy: free # noqa: run-once[play] (if using strategy: free can skip it this way) + gather_facts: false + tasks: # <-- use noqa to disable rule violations for specific tasks + - name: Task with run_once # noqa: run-once[task] + ansible.builtin.debug: + msg: "Test" + run_once: true +``` diff --git a/src/ansiblelint/rules/run_once.py b/src/ansiblelint/rules/run_once.py new file mode 100644 index 0000000..78968b6 --- /dev/null +++ b/src/ansiblelint/rules/run_once.py @@ -0,0 +1,96 @@ +"""Optional Ansible-lint rule to warn use of run_once with strategy free.""" +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING, Any + +from ansiblelint.constants import LINE_NUMBER_KEY +from ansiblelint.rules import AnsibleLintRule + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + from ansiblelint.utils import Task + + +class RunOnce(AnsibleLintRule): + """Run once should use strategy other than free.""" + + id = "run-once" + link = "https://docs.ansible.com/ansible/latest/reference_appendices/playbooks_keywords.html" + description = "When using run_once, we should avoid using strategy as free." + + tags = ["idiom"] + severity = "MEDIUM" + _ids = { + "run-once[task]": "Using run_once may behave differently if strategy is set to free.", + "run-once[play]": "Play uses strategy: free", + } + + def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + """Return matches found for a specific playbook.""" + # If the Play uses the 'strategy' and it's value is set to free + + if not file or file.kind != "playbook" or not data: + return [] + + strategy = data.get("strategy", None) + run_once = data.get("run_once", False) + if (not strategy and not run_once) or strategy != "free": + return [] + return [ + self.create_matcherror( + message="Play uses strategy: free", + filename=file, + tag=f"{self.id}[play]", + # pylint: disable=protected-access + lineno=strategy._line_number, # noqa: SLF001 + ), + ] + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + """Return matches for a task.""" + if not file or file.kind != "playbook": + return [] + + run_once = task.get("run_once", False) + if not run_once: + return [] + return [ + self.create_matcherror( + message="Using run_once may behave differently if strategy is set to free.", + filename=file, + tag=f"{self.id}[task]", + lineno=task[LINE_NUMBER_KEY], + ), + ] + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("test_file", "failure"), + ( + pytest.param("examples/playbooks/run-once-pass.yml", 0, id="pass"), + pytest.param("examples/playbooks/run-once-fail.yml", 2, id="fail"), + ), + ) + def test_run_once( + default_rules_collection: RulesCollection, + test_file: str, + failure: int, + ) -> None: + """Test rule matches.""" + results = Runner(test_file, rules=default_rules_collection).run() + for result in results: + assert result.rule.id == RunOnce().id + assert len(results) == failure diff --git a/src/ansiblelint/rules/sanity.md b/src/ansiblelint/rules/sanity.md new file mode 100644 index 0000000..5b4f3a4 --- /dev/null +++ b/src/ansiblelint/rules/sanity.md @@ -0,0 +1,54 @@ +# sanity + +This rule checks the `tests/sanity/ignore-x.x.txt` file for disallowed ignores. +This rule is extremely opinionated and enforced by Partner Engineering. The +currently allowed ruleset is subject to change, but is starting at a minimal +number of allowed ignores for maximum test enforcement. Any commented-out ignore +entries are not evaluated. + +This rule can produce messages like: + +- `sanity[cannot-ignore]` - Ignore file contains {test} at line {line_num}, + which is not a permitted ignore. +- `sanity[bad-ignore]` - Ignore file entry at {line_num} is formatted + incorrectly. Please review. + +Currently allowed ignores for all Ansible versions are: + +- `validate-modules:missing-gplv3-license` +- `action-plugin-docs` +- `import-2.6` +- `import-2.6!skip` +- `import-2.7` +- `import-2.7!skip` +- `import-3.5` +- `import-3.5!skip` +- `compile-2.6` +- `compile-2.6!skip` +- `compile-2.7` +- `compile-2.7!skip` +- `compile-3.5` +- `compile-3.5!skip` + +Additionally allowed ignores for Ansible 2.9 are: +- `validate-modules:deprecation-mismatch` +- `validate-modules:invalid-documentation` + +## Problematic code + +``` +# tests/sanity/ignore-x.x.txt +plugins/module_utils/ansible_example_module.py import-3.6!skip +``` + +``` +# tests/sanity/ignore-x.x.txt +plugins/module_utils/ansible_example_module.oops-3.6!skip +``` + +## Correct code + +``` +# tests/sanity/ignore-x.x.txt +plugins/module_utils/ansible_example_module.py import-2.7!skip +``` diff --git a/src/ansiblelint/rules/sanity.py b/src/ansiblelint/rules/sanity.py new file mode 100644 index 0000000..09fe7cc --- /dev/null +++ b/src/ansiblelint/rules/sanity.py @@ -0,0 +1,148 @@ +"""Implementation of sanity rule.""" +from __future__ import annotations + +import sys +from typing import TYPE_CHECKING + +from ansiblelint.rules import AnsibleLintRule + +# Copyright (c) 2018, Ansible Project + + +if TYPE_CHECKING: + from ansiblelint.errors import MatchError + from ansiblelint.file_utils import Lintable + + +class CheckSanityIgnoreFiles(AnsibleLintRule): + """Ignore entries in sanity ignore files must match an allow list.""" + + id = "sanity" + description = ( + "Identifies non-allowed entries in the `tests/sanity/ignore*.txt files." + ) + severity = "MEDIUM" + tags = ["idiom"] + version_added = "v6.14.0" + + # Partner Engineering defines this list. Please contact PE for changes. + + allowed_ignores_v2_9 = [ + "validate-modules:deprecation-mismatch", # Note: 2.9 expects a deprecated key in the METADATA. It was removed in later versions. + "validate-modules:invalid-documentation", # Note: The removed_at_date key in the deprecated section is invalid for 2.9. + ] + + allowed_ignores_all = [ + "validate-modules:missing-gplv3-license", + "action-plugin-docs", # Added for Networking Collections + "import-2.6", + "import-2.6!skip", + "import-2.7", + "import-2.7!skip", + "import-3.5", + "import-3.5!skip", + "compile-2.6", + "compile-2.6!skip", + "compile-2.7", + "compile-2.7!skip", + "compile-3.5", + "compile-3.5!skip", + ] + _ids = { + "sanity[cannot-ignore]": "Ignore file contains ... at line ..., which is not a permitted ignore.", + "sanity[bad-ignore]": "Ignore file entry at ... is formatted incorrectly. Please review.", + } + + def matchyaml(self, file: Lintable) -> list[MatchError]: + """Evaluate sanity ignore lists for disallowed ignores. + + :param file: Input lintable file that is a match for `sanity-ignore-file` + :returns: List of errors matched to the input file + """ + results: list[MatchError] = [] + test = "" + + if file.kind != "sanity-ignore-file": + return [] + + with file.path.open(encoding="utf-8") as ignore_file: + entries = ignore_file.read().splitlines() + + ignores = self.allowed_ignores_all + + # If there is a ignore-2.9.txt file, add the v2_9 list of allowed ignores + if "ignore-2.9.txt" in str(file.abspath): + ignores = self.allowed_ignores_all + self.allowed_ignores_v2_9 + + for line_num, entry in enumerate(entries, 1): + if entry and entry[0] != "#": + try: + if "#" in entry: + entry, _ = entry.split("#") + (_, test) = entry.split() + if test not in ignores: + results.append( + self.create_matcherror( + message=f"Ignore file contains {test} at line {line_num}, which is not a permitted ignore.", + tag="sanity[cannot-ignore]", + lineno=line_num, + filename=file, + ), + ) + + except ValueError: + results.append( + self.create_matcherror( + message=f"Ignore file entry at {line_num} is formatted incorrectly. Please review.", + tag="sanity[bad-ignore]", + lineno=line_num, + filename=file, + ), + ) + + return results + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + import pytest + + from ansiblelint.rules import RulesCollection # pylint: disable=ungrouped-imports + from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports + + @pytest.mark.parametrize( + ("test_file", "failures", "tags"), + ( + pytest.param( + "examples/sanity_ignores/tests/sanity/ignore-2.9.txt", + 0, + "sanity[cannot-ignore]", + id="pass", + ), + pytest.param( + "examples/sanity_ignores/tests/sanity/ignore-2.15.txt", + 1, + "sanity[bad-ignore]", + id="fail0", + ), + pytest.param( + "examples/sanity_ignores/tests/sanity/ignore-2.13.txt", + 1, + "sanity[cannot-ignore]", + id="fail1", + ), + ), + ) + def test_sanity_ignore_files( + default_rules_collection: RulesCollection, + test_file: str, + failures: int, + tags: str, + ) -> None: + """Test rule matches.""" + default_rules_collection.register(CheckSanityIgnoreFiles()) + results = Runner(test_file, rules=default_rules_collection).run() + for result in results: + assert result.rule.id == CheckSanityIgnoreFiles().id + assert result.tag == tags + assert len(results) == failures diff --git a/src/ansiblelint/rules/schema.md b/src/ansiblelint/rules/schema.md new file mode 100644 index 0000000..7c62120 --- /dev/null +++ b/src/ansiblelint/rules/schema.md @@ -0,0 +1,80 @@ +# schema + +The `schema` rule validates Ansible metadata files against JSON schemas. These +schemas ensure the compatibility of Ansible syntax content across versions. + +This `schema` rule is **mandatory**. You cannot use inline `noqa` comments to +ignore it. + +Ansible-lint validates the `schema` rule before processing other rules. This +prevents unexpected syntax from triggering multiple rule violations. + +## Validated schema + +Ansible-lint currently validates several schemas that are maintained in separate +projects and updated independently to ansible-lint. + +> Report bugs related to schema in their respective repository and not in the +> ansible-lint project. + +Maintained in the [ansible-lint](https://github.com/ansible/ansible-lint) +project: + +- `schema[ansible-lint-config]` validates + [ansible-lint configuration](https://github.com/ansible/ansible-lint/blob/main/src/ansiblelint/schemas/ansible-lint-config.json) +- `schema[role-arg-spec]` validates + [role argument specs](https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_reuse_roles.html#specification-format) + which is a little bit different than the module argument spec. +- `schema[execution-environment]` validates + [execution environments](https://docs.ansible.com/automation-controller/latest/html/userguide/execution_environments.html) +- `schema[galaxy]` validates + [collection metadata](https://docs.ansible.com/ansible/latest/dev_guide/collections_galaxy_meta.html). +- `schema[inventory]` validates + [inventory files](https://docs.ansible.com/ansible/latest/inventory_guide/intro_inventory.html) + that match `inventory/*.yml`. +- `schema[meta-runtime]` validates + [runtime information](https://docs.ansible.com/ansible/devel/dev_guide/developing_collections_structure.html#meta-directory-and-runtime-yml) + that matches `meta/runtime.yml` +- `schema[meta]` validates metadata for roles that match `meta/main.yml`. See + [role-dependencies](https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_reuse_roles.html#role-dependencies) + or + [role/metadata.py](https://github.com/ansible/ansible/blob/devel/lib/ansible/playbook/role/metadata.py#L79)) + for details. +- `schema[playbook]` validates Ansible playbooks. +- `schema[requirements]` validates Ansible + [requirements](https://docs.ansible.com/ansible/latest/galaxy/user_guide.html#install-multiple-collections-with-a-requirements-file) + files that match `requirements.yml`. +- `schema[tasks]` validates Ansible task files that match `tasks/**/*.yml`. +- `schema[vars]` validates Ansible + [variables](https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_variables.html) + that match `vars/*.yml` and `defaults/*.yml`. + +Maintained in the +[ansible-navigator](https://github.com/ansible/ansible-navigator) project: + +- `schema[ansible-navigator]` validates + [ansible-navigator configuration](https://github.com/ansible/ansible-navigator/blob/main/src/ansible_navigator/data/ansible-navigator.json) + +## schema[meta] + +For `meta/main.yml` files, Ansible-lint requires a `galaxy_info.standalone` +property that clarifies if a role is an old standalone one or a new one, +collection based: + +```yaml +galaxy_info: + standalone: true # <-- this is a standalone role (not part of a collection) +``` + +Ansible-lint requires the `standalone` key to avoid confusion and provide more +specific error messages. For example, the `meta` schema will require some +properties only for standalone roles or prevent the use of some properties that +are not supported by collections. + +You cannot use an empty `meta/main.yml` file or use only comments in the +`meta/main.yml` file. + +## schema[moves] + +These errors usually look like "foo was moved to bar in 2.10" and indicate +module moves between Ansible versions. diff --git a/src/ansiblelint/rules/schema.py b/src/ansiblelint/rules/schema.py new file mode 100644 index 0000000..32ff2eb --- /dev/null +++ b/src/ansiblelint/rules/schema.py @@ -0,0 +1,371 @@ +"""Rule definition for JSON Schema Validations.""" +from __future__ import annotations + +import logging +import sys +from typing import TYPE_CHECKING, Any + +from ansiblelint.errors import MatchError +from ansiblelint.file_utils import Lintable +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.schemas.__main__ import JSON_SCHEMAS +from ansiblelint.schemas.main import validate_file_schema +from ansiblelint.text import has_jinja + +if TYPE_CHECKING: + from ansiblelint.utils import Task + + +_logger = logging.getLogger(__name__) + + +DESCRIPTION_MD = """ Returned errors will not include exact line numbers, but they will mention +the schema name being used as a tag, like ``schema[playbook]``, +``schema[tasks]``. + +This rule is not skippable and stops further processing of the file. + +If incorrect schema was picked, you might want to either: + +* move the file to standard location, so its file is detected correctly. +* use ``kinds:`` option in linter config to help it pick correct file type. +""" + +pre_checks = { + "task": { + "with_flattened": { + "msg": "with_flattened was moved to with_community.general.flattened in 2.10", + "tag": "moves", + }, + "with_filetree": { + "msg": "with_filetree was moved to with_community.general.filetree in 2.10", + "tag": "moves", + }, + "with_cartesian": { + "msg": "with_cartesian was moved to with_community.general.flattened in 2.10", + "tag": "moves", + }, + }, +} + + +class ValidateSchemaRule(AnsibleLintRule): + """Perform JSON Schema Validation for known lintable kinds.""" + + description = DESCRIPTION_MD + + id = "schema" + severity = "VERY_HIGH" + tags = ["core"] + version_added = "v6.1.0" + _ids = { + "schema[ansible-lint-config]": "", + "schema[ansible-navigator-config]": "", + "schema[changelog]": "", + "schema[execution-environment]": "", + "schema[galaxy]": "", + "schema[inventory]": "", + "schema[meta]": "", + "schema[meta-runtime]": "", + "schema[molecule]": "", + "schema[playbook]": "", + "schema[requirements]": "", + "schema[role-arg-spec]": "", + "schema[rulebook]": "", + "schema[tasks]": "", + "schema[vars]": "", + } + _field_checks: dict[str, list[str]] = {} + + @property + def field_checks(self) -> dict[str, list[str]]: + """Lazy property for returning field checks.""" + if not self._collection: + msg = "Rule was not registered to a RuleCollection." + raise RuntimeError(msg) + if not self._field_checks: + self._field_checks = { + "become_method": sorted( + self._collection.app.runtime.plugins.become.keys(), + ), + } + return self._field_checks + + def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + """Return matches found for a specific playbook.""" + results: list[MatchError] = [] + if not data or file.kind not in ("tasks", "handlers", "playbook"): + return results + # check at play level + results.extend(self._get_field_matches(file=file, data=data)) + return results + + def _get_field_matches( + self, + file: Lintable, + data: dict[str, Any], + ) -> list[MatchError]: + """Retrieve all matches related to fields for the given data block.""" + results = [] + for key, values in self.field_checks.items(): + if key in data: + plugin_value = data[key] + if not has_jinja(plugin_value) and plugin_value not in values: + msg = f"'{key}' must be one of the currently available values: {', '.join(values)}" + results.append( + MatchError( + message=msg, + lineno=data.get("__line__", 1), + lintable=file, + rule=ValidateSchemaRule(), + details=ValidateSchemaRule.description, + tag=f"schema[{file.kind}]", + ), + ) + return results + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> bool | str | MatchError | list[MatchError]: + results = [] + if not file: + file = Lintable("", kind="tasks") + results.extend(self._get_field_matches(file=file, data=task.raw_task)) + for key in pre_checks["task"]: + if key in task.raw_task: + msg = pre_checks["task"][key]["msg"] + tag = pre_checks["task"][key]["tag"] + results.append( + MatchError( + message=msg, + lintable=file, + rule=ValidateSchemaRule(), + details=ValidateSchemaRule.description, + tag=f"schema[{tag}]", + ), + ) + return results + + def matchyaml(self, file: Lintable) -> list[MatchError]: + """Return JSON validation errors found as a list of MatchError(s).""" + result: list[MatchError] = [] + if file.kind not in JSON_SCHEMAS: + return result + + errors = validate_file_schema(file) + if errors: + if errors[0].startswith("Failed to load YAML file"): + _logger.debug( + "Ignored failure to load %s for schema validation, as !vault may cause it.", + file, + ) + return [] + + result.append( + MatchError( + message=errors[0], + lintable=file, + rule=ValidateSchemaRule(), + details=ValidateSchemaRule.description, + tag=f"schema[{file.kind}]", + ), + ) + + if not result: + result = super().matchyaml(file) + return result + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + import pytest + + # pylint: disable=ungrouped-imports + from ansiblelint.config import options + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner + + @pytest.mark.parametrize( + ("file", "expected_kind", "expected"), + ( + pytest.param( + "examples/collection/galaxy.yml", + "galaxy", + ["'GPL' is not one of"], + id="galaxy", + ), + pytest.param( + "examples/roles/invalid_requirements_schema/meta/requirements.yml", + "requirements", + ["{'foo': 'bar'} is not valid under any of the given schemas"], + id="requirements", + ), + pytest.param( + "examples/roles/invalid_meta_schema/meta/main.yml", + "meta", + ["False is not of type 'string'"], + id="meta", + ), + pytest.param( + "examples/playbooks/vars/invalid_vars_schema.yml", + "vars", + ["'123' does not match any of the regexes"], + id="vars", + ), + pytest.param( + "examples/execution-environment.yml", + "execution-environment", + [], + id="execution-environment", + ), + pytest.param( + "examples/ee_broken/execution-environment.yml", + "execution-environment", + ["{'foo': 'bar'} is not valid under any of the given schemas"], + id="execution-environment-broken", + ), + ("examples/meta/runtime.yml", "meta-runtime", []), + pytest.param( + "examples/broken_collection_meta_runtime/meta/runtime.yml", + "meta-runtime", + ["Additional properties are not allowed ('foo' was unexpected)"], + id="meta-runtime-broken", + ), + pytest.param( + "examples/inventory/production.yml", + "inventory", + [], + id="inventory", + ), + pytest.param( + "examples/inventory/broken_dev_inventory.yml", + "inventory", + ["Additional properties are not allowed ('foo' was unexpected)"], + id="inventory-broken", + ), + pytest.param( + ".ansible-lint", + "ansible-lint-config", + [], + id="ansible-lint-config", + ), + pytest.param( + "examples/.config/ansible-lint.yml", + "ansible-lint-config", + [], + id="ansible-lint-config2", + ), + pytest.param( + "examples/broken/.ansible-lint", + "ansible-lint-config", + ["Additional properties are not allowed ('foo' was unexpected)"], + id="ansible-lint-config-broken", + ), + pytest.param( + "examples/ansible-navigator.yml", + "ansible-navigator-config", + [], + id="ansible-navigator-config", + ), + pytest.param( + "examples/broken/ansible-navigator.yml", + "ansible-navigator-config", + ["Additional properties are not allowed ('ansible' was unexpected)"], + id="ansible-navigator-config-broken", + ), + pytest.param( + "examples/roles/hello/meta/argument_specs.yml", + "role-arg-spec", + [], + id="role-arg-spec", + ), + pytest.param( + "examples/roles/broken_argument_specs/meta/argument_specs.yml", + "role-arg-spec", + ["Additional properties are not allowed ('foo' was unexpected)"], + id="role-arg-spec-broken", + ), + pytest.param( + "examples/changelogs/changelog.yaml", + "changelog", + ["Additional properties are not allowed ('foo' was unexpected)"], + id="changelog", + ), + pytest.param( + "examples/rulebooks/rulebook-fail.yml", + "rulebook", + [ + "Additional properties are not allowed ('that_should_not_be_here' was unexpected)", + ], + id="rulebook", + ), + pytest.param( + "examples/rulebooks/rulebook-pass.yml", + "rulebook", + [], + id="rulebook2", + ), + pytest.param( + "examples/playbooks/rule-schema-become-method-pass.yml", + "playbook", + [], + id="playbook", + ), + pytest.param( + "examples/playbooks/rule-schema-become-method-fail.yml", + "playbook", + [ + "'become_method' must be one of the currently available values", + "'become_method' must be one of the currently available values", + ], + id="playbook2", + ), + ), + ) + def test_schema(file: str, expected_kind: str, expected: list[str]) -> None: + """Validate parsing of ansible output.""" + lintable = Lintable(file) + assert lintable.kind == expected_kind + + rules = RulesCollection(options=options) + rules.register(ValidateSchemaRule()) + results = Runner(lintable, rules=rules).run() + + assert len(results) == len(expected), results + for idx, result in enumerate(results): + assert result.filename.endswith(file) + assert expected[idx] in result.message + assert result.tag == f"schema[{expected_kind}]" + + @pytest.mark.parametrize( + ("file", "expected_kind", "expected_tag", "count"), + ( + pytest.param( + "examples/playbooks/rule-syntax-moves.yml", + "playbook", + "schema[moves]", + 3, + id="playbook", + ), + ), + ) + def test_schema_moves( + file: str, + expected_kind: str, + expected_tag: str, + count: int, + ) -> None: + """Validate ability to detect schema[moves].""" + lintable = Lintable(file) + assert lintable.kind == expected_kind + + rules = RulesCollection(options=options) + rules.register(ValidateSchemaRule()) + results = Runner(lintable, rules=rules).run() + + assert len(results) == count, results + for result in results: + assert result.filename.endswith(file) + assert result.tag == expected_tag diff --git a/src/ansiblelint/rules/syntax_check.md b/src/ansiblelint/rules/syntax_check.md new file mode 100644 index 0000000..e8197a5 --- /dev/null +++ b/src/ansiblelint/rules/syntax_check.md @@ -0,0 +1,45 @@ +# syntax-check + +Our linter runs `ansible-playbook --syntax-check` on all playbooks, and if any +of these reports a syntax error, this stops any further processing of these +files. + +This error **cannot be disabled** due to being a prerequisite for other steps. +You can exclude these files from linting, but it is better to make sure they can +be loaded by Ansible. This is often achieved by editing the inventory file +and/or `ansible.cfg` so ansible can load required variables. + +If undefined variables cause the failure, you can use the jinja `default()` +filter to provide fallback values, like in the example below. + +This rule is among the few `unskippable` rules that cannot be added to +`skip_list` or `warn_list`. One possible workaround is to add the entire file to +the `exclude_paths`. This is a valid approach for special cases, like testing +fixtures that are invalid on purpose. + +One of the most common sources of errors is a failure to assert the presence of +various variables at the beginning of the playbook. + +This rule can produce messages like below: + +- `syntax-check[empty-playbook]` is raised when a playbook file has no content. + +## Problematic code + +```yaml +--- +- name: + Bad use of variable inside hosts block (wrong assumption of it being + defined) + hosts: "{{ my_hosts }}" + tasks: [] +``` + +## Correct code + +```yaml +--- +- name: Good use of variable inside hosts, without assumptions + hosts: "{{ my_hosts | default([]) }}" + tasks: [] +``` diff --git a/src/ansiblelint/rules/syntax_check.py b/src/ansiblelint/rules/syntax_check.py new file mode 100644 index 0000000..c6a4c5e --- /dev/null +++ b/src/ansiblelint/rules/syntax_check.py @@ -0,0 +1,58 @@ +"""Rule definition for ansible syntax check.""" +from __future__ import annotations + +import re +from dataclasses import dataclass + +from ansiblelint.rules import AnsibleLintRule + + +@dataclass +class KnownError: + """Class that tracks result of linting.""" + + tag: str + regex: re.Pattern[str] + + +OUTPUT_PATTERNS = ( + KnownError( + tag="missing-file", + regex=re.compile( + # do not use <filename> capture group for this because we want to report original file, not the missing target one + r"(?P<title>Unable to retrieve file contents)\n(?P<details>Could not find or access '(?P<value>.*)'[^\n]*)", + re.MULTILINE | re.S | re.DOTALL, + ), + ), + KnownError( + tag="specific", + regex=re.compile( + r"^ERROR! (?P<title>[^\n]*)\n\nThe error appears to be in '(?P<filename>[\w\/\.\-]+)': line (?P<line>\d+), column (?P<column>\d+)", + re.MULTILINE | re.S | re.DOTALL, + ), + ), + KnownError( + tag="empty-playbook", + regex=re.compile( + "Empty playbook, nothing to do", + re.MULTILINE | re.S | re.DOTALL, + ), + ), + KnownError( + tag="malformed", + regex=re.compile( + "^ERROR! (?P<title>A malformed block was encountered while loading a block[^\n]*)", + re.MULTILINE | re.S | re.DOTALL, + ), + ), +) + + +class AnsibleSyntaxCheckRule(AnsibleLintRule): + """Ansible syntax check failed.""" + + id = "syntax-check" + severity = "VERY_HIGH" + tags = ["core", "unskippable"] + version_added = "v5.0.0" + _order = 0 diff --git a/src/ansiblelint/rules/var_naming.md b/src/ansiblelint/rules/var_naming.md new file mode 100644 index 0000000..3386a0c --- /dev/null +++ b/src/ansiblelint/rules/var_naming.md @@ -0,0 +1,77 @@ +# var-naming + +This rule checks variable names to ensure they conform with requirements. + +Variable names must contain only lowercase alphanumeric characters and the +underscore `_` character. Variable names must also start with either an +alphabetic or underscore `_` character. + +For more information see the [creating valid variable names][var-names] topic in +Ansible documentation and [Naming things (Good Practices for Ansible)][cop]. + +You should also be fully aware of [special variables][magic-vars], also known as +magic variables, especially as most of them can only be read. While Ansible will +just ignore any attempt to set them, the linter will notify the user, so they +would not be confused about a line that does not effectively do anything. + +Possible errors messages: + +- `var-naming[non-string]`: Variables names must be strings. +- `var-naming[non-ascii]`: Variables names must be ASCII. +- `var-naming[no-keyword]`: Variables names must not be Python keywords. +- `var-naming[no-jinja]`: Variables names must not contain jinja2 templating. +- `var-naming[pattern]`: Variables names should match ... regex. +- `var-naming[no-role-prefix]`: Variables names from within roles should use + `role_name_` as a prefix. +- `var-naming[no-reserved]`: Variables names must not be Ansible reserved names. +- `var-naming[read-only]`: This special variable is read-only. + +!!! note + + When using `include_role` or `import_role` with `vars`, vars should start + with included role name prefix. As this role might not be compliant + with this rule yet, you might need to temporarily disable this rule using + a `# noqa: var-naming[no-role-prefix]` comment. + +## Settings + +This rule behavior can be changed by altering the below settings: + +```yaml +# .ansible-lint +var_naming_pattern: "^[a-z_][a-z0-9_]*$" +``` + +## Problematic Code + +```yaml +--- +- name: Example playbook + hosts: localhost + vars: + CamelCase: true # <- Contains a mix of lowercase and uppercase characters. + ALL_CAPS: bar # <- Contains only uppercase characters. + v@r!able: baz # <- Contains special characters. + hosts: [] # <- hosts is an Ansible reserved name + role_name: boo # <-- invalid as being Ansible special magic variable +``` + +## Correct Code + +```yaml +--- +- name: Example playbook + hosts: localhost + vars: + lowercase: true # <- Contains only lowercase characters. + no_caps: bar # <- Does not contains uppercase characters. + variable: baz # <- Does not contain special characters. + my_hosts: [] # <- Does not use a reserved names. + my_role_name: boo +``` + +[cop]: https://redhat-cop.github.io/automation-good-practices/#_naming_things +[var-names]: + https://docs.ansible.com/ansible/latest/playbook_guide/playbooks_variables.html#creating-valid-variable-names +[magic-vars]: + https://docs.ansible.com/ansible/latest/reference_appendices/special_variables.html diff --git a/src/ansiblelint/rules/var_naming.py b/src/ansiblelint/rules/var_naming.py new file mode 100644 index 0000000..389530d --- /dev/null +++ b/src/ansiblelint/rules/var_naming.py @@ -0,0 +1,370 @@ +"""Implementation of var-naming rule.""" +from __future__ import annotations + +import keyword +import re +import sys +from typing import TYPE_CHECKING, Any + +from ansible.parsing.yaml.objects import AnsibleUnicode +from ansible.vars.reserved import get_reserved_names + +from ansiblelint.config import options +from ansiblelint.constants import ANNOTATION_KEYS, LINE_NUMBER_KEY, RC +from ansiblelint.errors import MatchError +from ansiblelint.file_utils import Lintable +from ansiblelint.rules import AnsibleLintRule, RulesCollection +from ansiblelint.runner import Runner +from ansiblelint.skip_utils import get_rule_skips_from_line +from ansiblelint.utils import parse_yaml_from_file + +if TYPE_CHECKING: + from ansiblelint.utils import Task + + +class VariableNamingRule(AnsibleLintRule): + """All variables should be named using only lowercase and underscores.""" + + id = "var-naming" + severity = "MEDIUM" + tags = ["idiom"] + version_added = "v5.0.10" + needs_raw_task = True + re_pattern_str = options.var_naming_pattern or "^[a-z_][a-z0-9_]*$" + re_pattern = re.compile(re_pattern_str) + reserved_names = get_reserved_names() + # List of special variables that should be treated as read-only. This list + # does not include connection variables, which we expect users to tune in + # specific cases. + # https://docs.ansible.com/ansible/latest/reference_appendices/special_variables.html + read_only_names = { + "ansible_check_mode", + "ansible_collection_name", + "ansible_config_file", + "ansible_dependent_role_names", + "ansible_diff_mode", + "ansible_forks", + "ansible_index_var", + "ansible_inventory_sources", + "ansible_limit", + "ansible_local", # special fact + "ansible_loop", + "ansible_loop_var", + "ansible_parent_role_names", + "ansible_parent_role_paths", + "ansible_play_batch", + "ansible_play_hosts", + "ansible_play_hosts_all", + "ansible_play_name", + "ansible_play_role_names", + "ansible_playbook_python", + "ansible_role_name", + "ansible_role_names", + "ansible_run_tags", + "ansible_search_path", + "ansible_skip_tags", + "ansible_verbosity", + "ansible_version", + "group_names", + "groups", + "hostvars", + "inventory_dir", + "inventory_file", + "inventory_hostname", + "inventory_hostname_short", + "omit", + "play_hosts", + "playbook_dir", + "role_name", + "role_names", + "role_path", + } + + # These special variables are used by Ansible but we allow users to set + # them as they might need it in certain cases. + allowed_special_names = { + "ansible_facts", + "ansible_become_user", + "ansible_connection", + "ansible_host", + "ansible_python_interpreter", + "ansible_user", + "ansible_remote_tmp", # no included in docs + } + _ids = { + "var-naming[no-reserved]": "Variables names must not be Ansible reserved names.", + "var-naming[no-jinja]": "Variables names must not contain jinja2 templating.", + "var-naming[pattern]": f"Variables names should match {re_pattern_str} regex.", + } + + # pylint: disable=too-many-return-statements + def get_var_naming_matcherror( + self, + ident: str, + *, + prefix: str = "", + ) -> MatchError | None: + """Return a MatchError if the variable name is not valid, otherwise None.""" + if not isinstance(ident, str): # pragma: no cover + return MatchError( + tag="var-naming[non-string]", + message="Variables names must be strings.", + rule=self, + ) + + if ident in ANNOTATION_KEYS or ident in self.allowed_special_names: + return None + + try: + ident.encode("ascii") + except UnicodeEncodeError: + return MatchError( + tag="var-naming[non-ascii]", + message=f"Variables names must be ASCII. ({ident})", + rule=self, + ) + + if keyword.iskeyword(ident): + return MatchError( + tag="var-naming[no-keyword]", + message=f"Variables names must not be Python keywords. ({ident})", + rule=self, + ) + + if ident in self.reserved_names: + return MatchError( + tag="var-naming[no-reserved]", + message=f"Variables names must not be Ansible reserved names. ({ident})", + rule=self, + ) + + if ident in self.read_only_names: + return MatchError( + tag="var-naming[read-only]", + message=f"This special variable is read-only. ({ident})", + rule=self, + ) + + # We want to allow use of jinja2 templating for variable names + if "{{" in ident: + return MatchError( + tag="var-naming[no-jinja]", + message="Variables names must not contain jinja2 templating.", + rule=self, + ) + + if not bool(self.re_pattern.match(ident)): + return MatchError( + tag="var-naming[pattern]", + message=f"Variables names should match {self.re_pattern_str} regex. ({ident})", + rule=self, + ) + + if prefix and not ident.startswith(f"{prefix}_"): + return MatchError( + tag="var-naming[no-role-prefix]", + message="Variables names from within roles should use role_name_ as a prefix.", + rule=self, + ) + return None + + def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: + """Return matches found for a specific playbook.""" + results: list[MatchError] = [] + raw_results: list[MatchError] = [] + + if not data or file.kind not in ("tasks", "handlers", "playbook", "vars"): + return results + # If the Play uses the 'vars' section to set variables + our_vars = data.get("vars", {}) + for key in our_vars: + match_error = self.get_var_naming_matcherror(key) + if match_error: + match_error.filename = str(file.path) + match_error.lineno = ( + key.ansible_pos[1] + if isinstance(key, AnsibleUnicode) + else our_vars[LINE_NUMBER_KEY] + ) + raw_results.append(match_error) + if raw_results: + lines = file.content.splitlines() + for match in raw_results: + # lineno starts with 1, not zero + skip_list = get_rule_skips_from_line( + line=lines[match.lineno - 1], + lintable=file, + ) + if match.rule.id not in skip_list and match.tag not in skip_list: + results.append(match) + + return results + + def matchtask( + self, + task: Task, + file: Lintable | None = None, + ) -> list[MatchError]: + """Return matches for task based variables.""" + results = [] + prefix = "" + filename = "" if file is None else str(file.path) + if file and file.parent and file.parent.kind == "role": + prefix = file.parent.path.name + ansible_module = task["action"]["__ansible_module__"] + # If the task uses the 'vars' section to set variables + our_vars = task.get("vars", {}) + if ansible_module in ("include_role", "import_role"): + action = task["action"] + if isinstance(action, dict): + role_fqcn = action.get("name", "") + prefix = role_fqcn.split("/" if "/" in role_fqcn else ".")[-1] + else: + prefix = "" + for key in our_vars: + match_error = self.get_var_naming_matcherror(key, prefix=prefix) + if match_error: + match_error.filename = filename + match_error.lineno = our_vars[LINE_NUMBER_KEY] + match_error.message += f" (vars: {key})" + results.append(match_error) + + # If the task uses the 'set_fact' module + if ansible_module == "set_fact": + for key in filter( + lambda x: isinstance(x, str) + and not x.startswith("__") + and x != "cacheable", + task["action"].keys(), + ): + match_error = self.get_var_naming_matcherror(key, prefix=prefix) + if match_error: + match_error.filename = filename + match_error.lineno = task["action"][LINE_NUMBER_KEY] + match_error.message += f" (set_fact: {key})" + results.append(match_error) + + # If the task registers a variable + registered_var = task.get("register", None) + if registered_var: + match_error = self.get_var_naming_matcherror(registered_var, prefix=prefix) + if match_error: + match_error.message += f" (register: {registered_var})" + match_error.filename = filename + match_error.lineno = task[LINE_NUMBER_KEY] + results.append(match_error) + + return results + + def matchyaml(self, file: Lintable) -> list[MatchError]: + """Return matches for variables defined in vars files.""" + results: list[MatchError] = [] + raw_results: list[MatchError] = [] + meta_data: dict[AnsibleUnicode, Any] = {} + filename = "" if file is None else str(file.path) + + if str(file.kind) == "vars" and file.data: + meta_data = parse_yaml_from_file(str(file.path)) + for key in meta_data: + match_error = self.get_var_naming_matcherror(key) + if match_error: + match_error.filename = filename + match_error.lineno = key.ansible_pos[1] + match_error.message += f" (vars: {key})" + raw_results.append(match_error) + if raw_results: + lines = file.content.splitlines() + for match in raw_results: + # lineno starts with 1, not zero + skip_list = get_rule_skips_from_line( + line=lines[match.lineno - 1], + lintable=file, + ) + if match.rule.id not in skip_list and match.tag not in skip_list: + results.append(match) + else: + results.extend(super().matchyaml(file)) + return results + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + import pytest + + from ansiblelint.testing import ( # pylint: disable=ungrouped-imports + run_ansible_lint, + ) + + @pytest.mark.parametrize( + ("file", "expected"), + ( + pytest.param("examples/playbooks/rule-var-naming-fail.yml", 7, id="0"), + pytest.param("examples/Taskfile.yml", 0, id="1"), + ), + ) + def test_invalid_var_name_playbook(file: str, expected: int) -> None: + """Test rule matches.""" + rules = RulesCollection(options=options) + rules.register(VariableNamingRule()) + results = Runner(Lintable(file), rules=rules).run() + assert len(results) == expected + for result in results: + assert result.rule.id == VariableNamingRule.id + # We are not checking line numbers because they can vary between + # different versions of ruamel.yaml (and depending on presence/absence + # of its c-extension) + + def test_invalid_var_name_varsfile( + default_rules_collection: RulesCollection, + ) -> None: + """Test rule matches.""" + results = Runner( + Lintable("examples/playbooks/vars/rule_var_naming_fail.yml"), + rules=default_rules_collection, + ).run() + expected_errors = ( + ("schema[vars]", 1), + ("var-naming[pattern]", 2), + ("var-naming[pattern]", 6), + ("var-naming[no-jinja]", 7), + ("var-naming[no-keyword]", 9), + ("var-naming[non-ascii]", 10), + ("var-naming[no-reserved]", 11), + ("var-naming[read-only]", 12), + ) + assert len(results) == len(expected_errors) + for idx, result in enumerate(results): + assert result.tag == expected_errors[idx][0] + assert result.lineno == expected_errors[idx][1] + + def test_var_naming_with_pattern() -> None: + """Test rule matches.""" + role_path = "examples/roles/var_naming_pattern/tasks/main.yml" + conf_path = "examples/roles/var_naming_pattern/.ansible-lint" + result = run_ansible_lint( + f"--config-file={conf_path}", + role_path, + ) + assert result.returncode == RC.SUCCESS + assert "var-naming" not in result.stdout + + def test_var_naming_with_include_tasks_and_vars() -> None: + """Test with include tasks and vars.""" + role_path = "examples/roles/var_naming_pattern/tasks/include_task_with_vars.yml" + result = run_ansible_lint(role_path) + assert result.returncode == RC.SUCCESS + assert "var-naming" not in result.stdout + + def test_var_naming_with_set_fact_and_cacheable() -> None: + """Test with include tasks and vars.""" + role_path = "examples/roles/var_naming_pattern/tasks/cacheable_set_fact.yml" + result = run_ansible_lint(role_path) + assert result.returncode == RC.SUCCESS + assert "var-naming" not in result.stdout + + def test_var_naming_with_include_role_import_role() -> None: + """Test with include role and import role.""" + role_path = "examples/test_collection/roles/my_role/tasks/main.yml" + result = run_ansible_lint(role_path) + assert result.returncode == RC.SUCCESS + assert "var-naming" not in result.stdout diff --git a/src/ansiblelint/rules/yaml.md b/src/ansiblelint/rules/yaml.md new file mode 100644 index 0000000..8dc56eb --- /dev/null +++ b/src/ansiblelint/rules/yaml.md @@ -0,0 +1,97 @@ +# yaml + +This rule checks YAML syntax and is an implementation of `yamllint`. + +You can disable YAML syntax violations by adding `yaml` to the `skip_list` in +your Ansible-lint configuration as follows: + +```yaml +skip_list: + - yaml +``` + +For more fine-grained control, disable violations for specific rules using tag +identifiers in the `yaml[yamllint_rule]` format as follows: + +```yaml +skip_list: + - yaml[trailing-spaces] + - yaml[indentation] +``` + +If you want Ansible-lint to report YAML syntax violations as warnings, and not +fatal errors, add tag identifiers to the `warn_list` in your configuration, for +example: + +```yaml +warn_list: + - yaml[document-start] +``` + +!!! warning + + You cannot use `tags: [skip_ansible_lint]` to disable this rule but you can + use [yamllint magic comments](https://yamllint.readthedocs.io/en/stable/disable_with_comments.html#disabling-checks-for-all-or-part-of-the-file) for tuning it. + +See the +[list of yamllint rules](https://yamllint.readthedocs.io/en/stable/rules.html) +for more information. + +Some of the detailed error codes that you might see are: + +- `yaml[brackets]` - _too few spaces inside empty brackets_, or _too many spaces + inside brackets_ +- `yaml[colons]` - _too many spaces before colon_, or _too many spaces after + colon_ +- `yaml[commas]` - _too many spaces before comma_, or _too few spaces after + comma_ +- `yaml[comments-indentation]` - _Comment not indented like content_ +- `yaml[comments]` - _Too few spaces before comment_, or _Missing starting space + in comment_ +- `yaml[document-start]` - _missing document start "---"_ or _found forbidden + document start "---"_ +- `yaml[empty-lines]` - _too many blank lines (...> ...)_ +- `yaml[indentation]` - _Wrong indentation: expected ... but found ..._ +- `yaml[key-duplicates]` - _Duplication of key "..." in mapping_ +- `yaml[new-line-at-end-of-file]` - _No new line character at the end of file_ +- `yaml[octal-values]`: forbidden implicit or explicit [octal](#octals) value +- `yaml[syntax]` - YAML syntax is broken +- `yaml[trailing-spaces]` - Spaces are found at the end of lines +- `yaml[truthy]` - _Truthy value should be one of ..._ + +## Octals + +As [YAML specification] regarding octal values changed at least 3 times in +[1.1], [1.2.0] and [1.2.2] we now require users to always add quotes around +octal values, so the YAML loaders will all load them as strings, providing a +consistent behavior. This is also safer as JSON does not support octal values +either. + +By default, yamllint does not check for octals but our custom default ruleset +for it does check these. If for some reason, you do not want to follow our +defaults, you can create a `.yamllint` file in your project and this will take +precedence over our defaults. + +## Problematic code + +```yaml +# Missing YAML document start. +foo: 0777 # <-- yaml[octal-values] +foo2: 0o777 # <-- yaml[octal-values] +foo2: ... # <-- yaml[key-duplicates] +bar: ... # <-- yaml[comments-indentation] +``` + +## Correct code + +```yaml +--- +foo: "0777" # <-- Explicitly quoting octal is less risky. +foo2: "0o777" # <-- Explicitly quoting octal is less risky. +bar: ... # Correct comment indentation. +``` + +[1.1]: https://yaml.org/spec/1.1/ +[1.2.0]: https://yaml.org/spec/1.2.0/ +[1.2.2]: https://yaml.org/spec/1.2.2/ +[yaml specification]: https://yaml.org/ diff --git a/src/ansiblelint/rules/yaml_rule.py b/src/ansiblelint/rules/yaml_rule.py new file mode 100644 index 0000000..4da4d41 --- /dev/null +++ b/src/ansiblelint/rules/yaml_rule.py @@ -0,0 +1,210 @@ +"""Implementation of yaml linting rule (yamllint integration).""" +from __future__ import annotations + +import logging +import sys +from collections.abc import Iterable +from typing import TYPE_CHECKING + +from yamllint.linter import run as run_yamllint + +from ansiblelint.constants import LINE_NUMBER_KEY, SKIPPED_RULES_KEY +from ansiblelint.file_utils import Lintable +from ansiblelint.rules import AnsibleLintRule +from ansiblelint.yaml_utils import load_yamllint_config + +if TYPE_CHECKING: + from typing import Any + + from ansiblelint.errors import MatchError + +_logger = logging.getLogger(__name__) + + +class YamllintRule(AnsibleLintRule): + """Violations reported by yamllint.""" + + id = "yaml" + severity = "VERY_LOW" + tags = ["formatting", "yaml"] + version_added = "v5.0.0" + config = load_yamllint_config() + has_dynamic_tags = True + link = "https://yamllint.readthedocs.io/en/stable/rules.html" + # ensure this rule runs before most of other common rules + _order = 1 + _ids = { + "yaml[anchors]": "", + "yaml[braces]": "", + "yaml[brackets]": "", + "yaml[colons]": "", + "yaml[commas]": "", + "yaml[comments-indentation]": "", + "yaml[comments]": "", + "yaml[document-end]": "", + "yaml[document-start]": "", + "yaml[empty-lines]": "", + "yaml[empty-values]": "", + "yaml[float-values]": "", + "yaml[hyphens]": "", + "yaml[indentation]": "", + "yaml[key-duplicates]": "", + "yaml[key-ordering]": "", + "yaml[line-length]": "", + "yaml[new-line-at-end-of-file]": "", + "yaml[new-lines]": "", + "yaml[octal-values]": "", + "yaml[quoted-strings]": "", + "yaml[trailing-spaces]": "", + "yaml[truthy]": "", + } + + def matchyaml(self, file: Lintable) -> list[MatchError]: + """Return matches found for a specific YAML text.""" + matches: list[MatchError] = [] + if str(file.base_kind) != "text/yaml": + return matches + + for problem in run_yamllint( + file.content, + YamllintRule.config, + filepath=file.path, + ): + self.severity = "VERY_LOW" + if problem.level == "error": + self.severity = "MEDIUM" + matches.append( + self.create_matcherror( + # yamllint does return lower-case sentences + message=problem.desc.capitalize(), + lineno=problem.line, + details="", + filename=file, + tag=f"yaml[{problem.rule}]", + ), + ) + return matches + + +def _combine_skip_rules(data: Any) -> set[str]: + """Return a consolidated list of skipped rules.""" + result = set(data.get(SKIPPED_RULES_KEY, [])) + tags = data.get("tags", []) + if tags and ( + isinstance(tags, Iterable) + and "skip_ansible_lint" in tags + or tags == "skip_ansible_lint" + ): + result.add("skip_ansible_lint") + return result + + +def _fetch_skips(data: Any, collector: dict[int, set[str]]) -> dict[int, set[str]]: + """Retrieve a dictionary with line: skips by looking recursively in given JSON structure.""" + if hasattr(data, "get") and data.get(LINE_NUMBER_KEY): + rules = _combine_skip_rules(data) + if rules: + collector[data.get(LINE_NUMBER_KEY)].update(rules) + if isinstance(data, Iterable) and not isinstance(data, str): + if isinstance(data, dict): + for _entry, value in data.items(): + _fetch_skips(value, collector) + else: # must be some kind of list + for entry in data: + if ( + entry + and hasattr(entry, "get") + and LINE_NUMBER_KEY in entry + and SKIPPED_RULES_KEY in entry + and entry[SKIPPED_RULES_KEY] + ): + collector[entry[LINE_NUMBER_KEY]].update(entry[SKIPPED_RULES_KEY]) + _fetch_skips(entry, collector) + return collector + + +# testing code to be loaded only with pytest or when executed the rule file +if "pytest" in sys.modules: + import pytest + + # pylint: disable=ungrouped-imports + from ansiblelint.config import options + from ansiblelint.rules import RulesCollection + from ansiblelint.runner import Runner + + @pytest.mark.parametrize( + ("file", "expected_kind", "expected"), + ( + pytest.param( + "examples/yamllint/invalid.yml", + "yaml", + [ + 'Missing document start "---"', + 'Duplication of key "foo" in mapping', + "Trailing spaces", + ], + id="invalid", + ), + pytest.param("examples/yamllint/valid.yml", "yaml", [], id="valid"), + pytest.param( + "examples/yamllint/line-length.yml", + "yaml", + ["Line too long (166 > 160 characters)"], + id="line-length", + ), + pytest.param( + "examples/yamllint/multi-document.yaml", + "yaml", + [], + id="multi-document", + ), + pytest.param( + "examples/yamllint/skipped-rule.yml", + "yaml", + [], + id="skipped-rule", + ), + pytest.param( + "examples/playbooks/rule-yaml-fail.yml", + "playbook", + [ + "Truthy value should be one of [false, true]", + "Truthy value should be one of [false, true]", + "Truthy value should be one of [false, true]", + ], + id="rule-yaml-fail", + ), + pytest.param( + "examples/playbooks/rule-yaml-pass.yml", + "playbook", + [], + id="rule-yaml-pass", + ), + ), + ) + @pytest.mark.filterwarnings("ignore::ansible_compat.runtime.AnsibleWarning") + def test_yamllint(file: str, expected_kind: str, expected: list[str]) -> None: + """Validate parsing of ansible output.""" + lintable = Lintable(file) + assert lintable.kind == expected_kind + + rules = RulesCollection(options=options) + rules.register(YamllintRule()) + results = Runner(lintable, rules=rules).run() + + assert len(results) == len(expected), results + for idx, result in enumerate(results): + assert result.filename.endswith(file) + assert expected[idx] in result.message + assert isinstance(result.tag, str) + assert result.tag.startswith("yaml[") + + def test_yamllint_has_help(default_rules_collection: RulesCollection) -> None: + """Asserts that we loaded markdown documentation in help property.""" + for rule in default_rules_collection: + if rule.id == "yaml": + assert rule.help is not None + assert len(rule.help) > 100 + break + else: # pragma: no cover + pytest.fail("No yaml rule found") |