Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix incorrect ansible_managed formatting #79129

Merged
merged 1 commit into from Aug 14, 2023
Merged

fix incorrect ansible_managed formatting #79129

merged 1 commit into from Aug 14, 2023

Conversation

doc-sheet
Copy link
Contributor

@doc-sheet doc-sheet commented Oct 13, 2022 •
edited

SUMMARY

Ansible breaks configs if file name contains % chars.
Especially %n[ame%] which is a line break pattern according to strftime (3)

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

generate_ansible_template_vars()

ADDITIONAL INFO

default value for ansible_managed is

ansible_managed = Ansible managed: {file} modified on %Y-%m-%d %H:%M:%S by {uid} on {host}

if you put {{ansible_managed}} in jinja template and save in file named like /etc/whatever/%name%.conf it will split string to multiple lines and possibly break syntax

Simple test case:

>>> a = 'haha %name% haha %Y-%m-%d'
>>> time.strftime(a)
'haha \name% haha 2022-10-13'

@ansibot ansibot added affects_2.15 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. small_patch labels Oct 13, 2022
@ansibot
Copy link
Contributor

ansibot commented Oct 13, 2022

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/template/__init__.py:106:53: E231: missing whitespace after ','

click here for bot help

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 13, 2022
@nitzmahone nitzmahone removed the needs_triage Needs a first human triage before being processed. label Oct 13, 2022
@nitzmahone
Copy link
Member

This is probably mergeable with a changelog fragment and a test

@doc-sheet
Copy link
Contributor Author

doc-sheet commented Oct 15, 2022 •
edited

Added changelog and test.
Also fixed config load for #39531 test

@doc-sheet
Copy link
Contributor Author

ready_for_review

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 28, 2022
@doc-sheet
Copy link
Contributor Author

What should I do next?

@doc-sheet
Copy link
Contributor Author

bot_status

@ansibot
Copy link
Contributor

ansibot commented Feb 24, 2023

Components

changelogs/fragments/79129-ansible-managed-filename-format.yaml
maintainers:

lib/ansible/template/init.py
maintainers:

test/integration/targets/template/ansible_managed_79129.yml
maintainers: ansible

test/integration/targets/template/runme.sh
maintainers: ansible

[test/integration/targets/template/templates/%necho Onii-chan help Im stuck;exit 1%n.j2](https://github.com/ansible/ansible/blob/devel/test/integration/targets/template/templates/%necho Onii-chan help Im stuck;exit 1%n.j2)
maintainers: ansible

Metadata

waiting_on: ansible
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: clean
ci_status: success
maintainer_shipits (module maintainers): 0
community_shipits (namespace maintainers): 0
ansible_shipits (core team members): 0
shipit_actors (maintainer or core team member): []
shipit_actors_other: []
automerge: automerge shipit test failed

click here for bot help

@doc-sheet
Copy link
Contributor Author

ready_for_review

bcoca
bcoca requested changes Jul 19, 2023
View reviewed changes
lib/ansible/template/__init__.py
@@ -103,7 +103,7 @@ def generate_ansible_template_vars(path, fullpath=None, dest_path=None):
managed_str = managed_default.format(
host=temp_vars['template_host'],
uid=temp_vars['template_uid'],
file=temp_vars['template_path'],
file=temp_vars['template_path'].replace('%', '%%'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix only addresses the %, it should escape any/all characters that would cause issues with templating

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to docs only percent characters could cause unexpected result. I guess it's enough to replace them in in file path to sanitize input.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{{ .. strftime is not the only 'templating' that happens

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh. right

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!
I've updated PR and added test for this.

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 19, 2023
@doc-sheet
fix incorrect ansible_managed formatting
3f40f5a 
Ansible breaks configs if file name could be parsed as jinja template
or contains `%` chars.
Especially `%n`[ame%] which is a line break pattern according to [strftime (3)](https://manpages.debian.org/bullseye/manpages-dev/strftime.3.en.html)
@ansibot ansibot added stale_review Updates were made after the last review and the last review is more than 7 days old. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Aug 9, 2023
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed stale_review Updates were made after the last review and the last review is more than 7 days old. labels Aug 9, 2023
@doc-sheet
Copy link
Contributor Author

ready_for_review

@bcoca bcoca merged commit bd3ffbe into ansible:devel Aug 14, 2023
78 checks passed
@ansible ansible locked and limited conversation to collaborators Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bcoca bcoca bcoca approved these changes

Assignees
No one assigned
Labels
affects_2.15 bug This issue/PR relates to a bug. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@doc-sheet @ansibot @nitzmahone @bcoca