From 0e6cdefdd0c0c59ecbad1970c4f05c3ba153e24e Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Thu, 29 Sep 2022 21:38:37 +0100 Subject: [PATCH] Replace template-instead-of-copy with avoid-implicit[copy-content] rule (#2512) Fixes: #2501 --- ...ccess.yml => rule-avoid-implicit-fail.yml} | 4 +- ...ilure.yml => rule-avoid-implicit-pass.yml} | 2 +- src/ansiblelint/config.py | 5 ++- src/ansiblelint/data/profiles.yml | 2 +- src/ansiblelint/rules/avoid_implicit.md | 37 +++++++++++++++++++ ...e_instead_of_copy.py => avoid_implicit.py} | 34 ++++++++--------- .../rules/template-instead-of-copy.md | 34 ----------------- 7 files changed, 59 insertions(+), 59 deletions(-) rename examples/playbooks/{rule-template-instead-of-copy-success.yml => rule-avoid-implicit-fail.yml} (58%) rename examples/playbooks/{rule-template-instead-of-copy-failure.yml => rule-avoid-implicit-pass.yml} (72%) create mode 100644 src/ansiblelint/rules/avoid_implicit.md rename src/ansiblelint/rules/{template_instead_of_copy.py => avoid_implicit.py} (57%) delete mode 100644 src/ansiblelint/rules/template-instead-of-copy.md diff --git a/examples/playbooks/rule-template-instead-of-copy-success.yml b/examples/playbooks/rule-avoid-implicit-fail.yml similarity index 58% rename from examples/playbooks/rule-template-instead-of-copy-success.yml rename to examples/playbooks/rule-avoid-implicit-fail.yml index ac39d534d6..4a63454ccb 100644 --- a/examples/playbooks/rule-template-instead-of-copy-success.yml +++ b/examples/playbooks/rule-avoid-implicit-fail.yml @@ -3,7 +3,7 @@ hosts: localhost tasks: - name: Write file content - ansible.builtin.template: - src: foo.txt.j2 + ansible.builtin.copy: + content: { "foo": "bar" } # <-- avoid-implicit[copy-content] dest: /tmp/foo.txt mode: 0600 diff --git a/examples/playbooks/rule-template-instead-of-copy-failure.yml b/examples/playbooks/rule-avoid-implicit-pass.yml similarity index 72% rename from examples/playbooks/rule-template-instead-of-copy-failure.yml rename to examples/playbooks/rule-avoid-implicit-pass.yml index aed029bda8..fa282c21ce 100644 --- a/examples/playbooks/rule-template-instead-of-copy-failure.yml +++ b/examples/playbooks/rule-avoid-implicit-pass.yml @@ -4,6 +4,6 @@ tasks: - name: Write file content ansible.builtin.copy: - content: "Some {{ foo }}" # <-- this is the problem + content: "Some {{ foo }}" dest: /tmp/foo.txt mode: 0600 diff --git a/src/ansiblelint/config.py b/src/ansiblelint/config.py index 1f1cbf1d07..86d753e292 100644 --- a/src/ansiblelint/config.py +++ b/src/ansiblelint/config.py @@ -11,13 +11,14 @@ from ansiblelint.loaders import yaml_from_file DEFAULT_WARN_LIST = [ + "avoid-implicit", "experimental", + "fqcn[action]", + "fqcn[redirect]", "jinja[spacing]", # warning until we resolve all reported false-positives "name[casing]", "name[play]", "role-name", - "fqcn[action]", - "fqcn[redirect]", ] DEFAULT_KINDS = [ diff --git a/src/ansiblelint/data/profiles.yml b/src/ansiblelint/data/profiles.yml index 9204f28c6b..eec7843421 100644 --- a/src/ansiblelint/data/profiles.yml +++ b/src/ansiblelint/data/profiles.yml @@ -63,12 +63,12 @@ safety: a non-determinant outcomes and cause security issues in some cases. extends: moderate rules: + avoid-implicit: latest: package-latest: risky-file-permissions: risky-octal: risky-shell-pipe: - template-instead-of-copy: shared: extends: safety description: > diff --git a/src/ansiblelint/rules/avoid_implicit.md b/src/ansiblelint/rules/avoid_implicit.md new file mode 100644 index 0000000000..4c3d781c13 --- /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/template_instead_of_copy.py b/src/ansiblelint/rules/avoid_implicit.py similarity index 57% rename from src/ansiblelint/rules/template_instead_of_copy.py rename to src/ansiblelint/rules/avoid_implicit.py index 97a359957b..60c1102d64 100644 --- a/src/ansiblelint/rules/template_instead_of_copy.py +++ b/src/ansiblelint/rules/avoid_implicit.py @@ -1,9 +1,5 @@ -"""Implementation of template-instead-of-copy rule.""" -# cspell:disable-next-line -# Copyright (c) 2022, Alexander Skiba -# references -# - github discussion of issue: https://github.com/ansible/ansible/issues/50580 -# - ansible docs with warning: https://docs.ansible.com/ansible/latest/collections/ansible/builtin/copy_module.html#synopsis +"""Implementation of avoid-implicit rule.""" +# https://github.com/ansible/ansible-lint/issues/2501 from __future__ import annotations import sys @@ -15,18 +11,18 @@ from ansiblelint.file_utils import Lintable -class UseTemplateInsteadOfCopyRule(AnsibleLintRule): - """Rule that identifies improper use copy instead of template.""" +class AvoidImplicitRule(AnsibleLintRule): + """Rule that identifies use of undocumented or discouraged implicit behaviors.""" - id = "template-instead-of-copy" - shortdesc = "Templated files should use template instead of copy" + 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 = "custom" + tags = ["unpredictability", "experimental"] + version_added = "v6.8.0" def matchtask( self, task: dict[str, Any], file: Lintable | None = None @@ -34,7 +30,7 @@ def matchtask( """Confirm if current rule is matching a specific task.""" if task["action"]["__ansible_module__"] == "copy": content = task["action"].get("content", "") - if "{{" in content: + if not isinstance(content, str): return True return False @@ -46,18 +42,18 @@ def matchtask( from ansiblelint.runner import Runner # pylint: disable=ungrouped-imports def test_template_instead_of_copy_positive() -> None: - """Positive test for partial-become.""" + """Positive test for avoid-implicit.""" collection = RulesCollection() - collection.register(UseTemplateInsteadOfCopyRule()) - success = "examples/playbooks/rule-template-instead-of-copy-success.yml" + 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 partial-become.""" + """Negative test for avoid-implicit.""" collection = RulesCollection() - collection.register(UseTemplateInsteadOfCopyRule()) - failure = "examples/playbooks/rule-template-instead-of-copy-failure.yml" + 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/template-instead-of-copy.md b/src/ansiblelint/rules/template-instead-of-copy.md deleted file mode 100644 index 2d4631a8d1..0000000000 --- a/src/ansiblelint/rules/template-instead-of-copy.md +++ /dev/null @@ -1,34 +0,0 @@ -# template-instead-of-copy - -This rule identifies the presence of variable interpolation inside `copy` -module and recommends instead the use of the `template` module. This is based on -the official [recommendation](https://docs.ansible.com/ansible/latest/collections/ansible/builtin/copy_module.html#synopsis) -from Ansible documentation. - -## Problematic Code - -```yaml ---- -- name: Example playbook - hosts: localhost - tasks: - - name: Write file content - ansible.builtin.copy: - content: "Some {{ foo }}" # <-- should use template instead - dest: /tmp/foo.txt -``` - -## Correct Code - -```yaml ---- -- name: Example playbook - hosts: localhost - tasks: - - name: Write file content - ansible.builtin.template: - src: foo.txt.j2 - dest: /tmp/foo.txt -``` - -Also, create a file `templates/foo.txt.j2` with `Some {{ foo }}` as its content.