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.

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

config options templatable #79244

Draft
wants to merge 23 commits into
base: devel
Choose a base branch
from
Draft

Conversation

bcoca
Copy link
Member

@bcoca bcoca commented Oct 27, 2022

if plugin has templar available, use it to handle config options
todo: make sure most plugins (where it makes sense) have appropriate templar setup

fixes #49946, #73268, #58281

alt for #58288

ISSUE TYPE
  • Bugfix Pull Request
  • Feature Pull Request
COMPONENT NAME

config manager/plugins

@ansibot ansibot added affects_2.15 bug This issue/PR relates to a bug. needs_triage Needs a first human triage before being processed. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 27, 2022
@felixfontein
Copy link
Contributor

The CI failures show a problem with this approach: there are some config parameters that look like (parts of) templates, but should not be evaluated. Examples are the variable_start_string and variable_end_string parameters of the template lookup (which is where CI fails). It could also be that there is a lookup plugin which expects a template string as an input that is then templated with some extra variables provided by the lookup - when that template is evaluated before its original value reaches the plugin, the result could be completely wrong, or templating could fail due to missing variables.

@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 27, 2022
@mattclay mattclay removed the needs_triage Needs a first human triage before being processed. label Oct 27, 2022
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Oct 27, 2022
@bcoca
Copy link
Member Author

bcoca commented Oct 27, 2022

@felixfontein added protection for 'non templates', as for the evaluation, this should only affect get_option calls, in general the lookups should only be trying to template the _terms that are passed in as non keyed input. Still, i'll look to see if there are any that look to template other terms and find a way to mark options as 'static' or not.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 27, 2022
@bcoca bcoca marked this pull request as draft October 27, 2022 23:48
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Oct 27, 2022
@ansible ansible deleted a comment from ansibot Oct 28, 2022
@bcoca bcoca force-pushed the template_conf_options branch from 5e07112 to d753648 Compare October 28, 2022 00:39
@felixfontein
Copy link
Contributor

@felixfontein added protection for 'non templates', as for the evaluation, this should only affect get_option calls, in general the lookups should only be trying to template the _terms that are passed in as non keyed input. Still, i'll look to see if there are any that look to template other terms and find a way to mark options as 'static' or not.

One interesting place are probably inventory plugins, these sometimes already explicitly template some of the options (to allow using lookups to be used for credentials, for example). In c.g the proxmox and linode inventories do that, and in community.dns and community.hrobot also have code for handling that.

@bcoca
Copy link
Member Author

bcoca commented Oct 28, 2022

yes, but many of those 'templatable fields' do not require {{ }} which would then not pass the is_temlpate test , for the others that do their own templating I'll have to look at options as i mentioned above, since inventory options is one of the ones we specifically want to enable.

@bcoca
Copy link
Member Author

bcoca commented Oct 28, 2022

checked proxmox, it uses is_template so the only issue is reusing proxmox_url setting as a variable, which can still be done by having the plugin set_option + update self._templar variables or as it currently works, silently failing at the configmanager and letting the templar at proxmox do the work. The linnode one is even simpler.

@bcoca
Copy link
Member Author

bcoca commented Oct 28, 2022

Thinking about a new property: template: static|vault|restricted|full|implicit:

  • static: no templating (default if we want strict backwards compat)
  • vault: only unvault
  • restricted: (the proposed default) template but no lookups (how constructed keywords currently work)
  • full: template normally (alt default?) (works like loop:)
  • implicit: 'always template, assume {{ }}' (works like when:)

note: that plugin field should always be static, the auto plugin normally evaluates this

@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 Nov 5, 2022
@ansibot ansibot removed 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 Nov 15, 2022
@bcoca bcoca force-pushed the template_conf_options branch from 7cb0b1d to d9f7f34 Compare November 15, 2022 14:48
@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 Nov 23, 2022
@bcoca bcoca force-pushed the template_conf_options branch from c73119e to a190283 Compare December 2, 2022 15:49
@ansibot ansibot removed 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 Dec 2, 2022
@bcoca bcoca marked this pull request as ready for review December 2, 2022 16:20
@bcoca bcoca force-pushed the template_conf_options branch from 42e41d8 to d9e0c27 Compare October 10, 2023 20:29
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Oct 10, 2023
@ansible ansible deleted a comment from ansibot Oct 11, 2023
@ansible ansible deleted a comment from ansibot Oct 11, 2023
@ansibot
Copy link
Contributor

ansibot commented Oct 11, 2023

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

lib/ansible/config/manager.py:563:17: E125: continuation line with same indent as next logical line

click here for bot help

@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 26, 2023
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Nov 21, 2023
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.15 bug This issue/PR relates to a bug. feature This issue/PR relates to a feature request. has_issue needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html 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. stale_review Updates were made after the last review and the last review is more than 7 days old. WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow vaulted configuration for plugins that only support ansible.cfg configs
7 participants