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

Parse deferred templates non deferred first #1425

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

traylenator
Copy link
Contributor

@traylenator traylenator commented Apr 15, 2024

Summary

Parse templates with deferred values twice so complex data types can be used.

Additional Context

Currently it is not possible to have a template file.epp

<%- |
Stdlib::Port $port,
String[1] $password,
| %>
port <%= $port %>
password <%= $password %>

and run

file{'/tmp/junk':
  content => stdlib::deferrable_epp(
      'module/file.epp',
      { 'port' => '1234', pass => Deferred('secrets::get',['mysecret'])}
   ),
}

since the deferred template substitution will fail:

Error: Failed to apply catalog: Evaluation Error: Resource type not found: Stdlib::Port (file: inlined-epp-text, line: 2, column: 3)

due to Stdlib::Port not being available on the agent node.

This change now parses the EPP twice. The first non deferred parse will reduce the template to:

port = 1234
password <%= $password %>

and this simpler template will be parsed in deferred mode.

Note the original template type for password must accept the intermediate generated value of <%= $password %> which is typically case for a secret password. If the deferred value is more complicated then the argument preparse can be
set false to not attempt the epp substitution of non deferred values.

Provide a detailed description of all the changes present in this pull request.

Related Issues (if any)

Fixes voxpupuli/puppet-redis#513

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.
  • Manually verified. Verified on a separate master and agent with puppet-redis module as per bug.

@traylenator
Copy link
Contributor Author

traylenator commented Apr 15, 2024

@alexjfisher I removed the commented out tests since puppetlabs/rspec-puppet#24 is merged and
it seems to pass now ... bit confused by though tests - think they need some more lines now I expect.

@traylenator
Copy link
Contributor Author

Very open to idea that this new feature should be behind a parameter:

file{'/tmp/junk':
  content => stdlib::deferrable_epp(
      'module/file.epp',
      { 'port' => '1234', pass => Deferred('secrets::get',['mysecret'])},
      prerender => true,
   ),
}

or something?

@traylenator traylenator changed the title Parse deferred templates twice Parse deferred templates first non deferred Apr 15, 2024
@traylenator traylenator changed the title Parse deferred templates first non deferred Parse deferred templates non deferred first Apr 15, 2024
@alexjfisher
Copy link
Collaborator

Very open to idea that this new feature should be behind a parameter:

file{'/tmp/junk':
  content => stdlib::deferrable_epp(
      'module/file.epp',
      { 'port' => '1234', pass => Deferred('secrets::get',['mysecret'])},
      prerender => true,
   ),
}

or something?

Because of

Note the original template type for password must accept the intermediate generated value of <%= $password %> which is typically case for a secret password.

this might be necessary, but maybe the default can be the new behaviour?

@traylenator traylenator force-pushed the nestedepp branch 2 times, most recently from cfcbea5 to 55a003f Compare April 15, 2024 11:24
@traylenator
Copy link
Contributor Author

traylenator commented Apr 15, 2024

this might be necessary, but maybe the default can be the new behaviour?

Yes someone using a secret array type, while rare I expect, will have to do something. Set new preparse=>false or
update their template to accept a string also.

Have added preparse => true as a default.

Currently it is not possible to have a template file.epp

```puppet
<%- |
Stdlib::Port $port,
String[1] $password,
| %>
port <%= $port %>
password <%= $password %>

```

and run

```puppet
file{'/tmp/junk':
  content => stdlib::deferrable_epp('module/file.epp', { 'port' => '1234', pass => Deferred('secrets::get',['mysecret'])}),
}
```
since the deferred template substitution  will fail:
```
Error: Failed to apply catalog: Evaluation Error: Resource type not found: Stdlib::Port (file: inlined-epp-text, line: 2, column: 3)
```
due to Stdlib::Port not being available on the agent node.

This change now parses the EPP twice. The first pass will reduce the
template to:

```puppet
port = 1234
password <%= $password %>
```

and this simpler template will be passed in deferred mode.

Note the original template type for password must accept the
intermediate generated value of `<%= $password %>` which is typically
case for a secret password.
@alexjfisher
Copy link
Collaborator

this might be necessary, but maybe the default can be the new behaviour?

Yes someone using a secret array type, while rare I expect, will have to do something. Set new preparse=>false or update their template to accept a string also.

Have added preparse => true as a default.

That works for me. (I think more likely than a non string type is someone using Pattern for cases where passwords eg. don't accept special characters.)

@traylenator
Copy link
Contributor Author

As this is it's backwards incompatible.
Changing to default false it would be compatible.

I think true is better.

Depends a bit how near we are to a major bump.

traylenator added a commit to traylenator/puppet-redis that referenced this pull request Apr 16, 2024
Process the configuration file `redis.conf` template twice if
some of the templates values are deferred.

Currently with deferred values the resulting deferred
template cannot be processed since it contains
complex datatypes from stdlib in particular.

This is a redis specific solution that may arrive genraly one
day in puppetlabs/puppetlabs-stdlib#1425
Comment on lines 19 to +21
if $variables.stdlib::nested_values.any |$value| { $value.is_a(Deferred) } {
if $preparse {
$_variables_escaped = $variables.map | $_var , $_value | {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like voxpupuli/puppet-redis#515 (comment) this has the problem that nested_values does recurse into hashes while $variables.map doesn't. I think the inner part is essentially a Puppet native version of Ruby's Hash#transform_values. Perhaps what's needed is a deep_transform_values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So:

stdlib::deferrable_epp(template, { 'data' => { 'a' => Deferred(....) } })

Used to be supported and now it would not be with this patch as it is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes. It would be good to somehow cover this in a test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even assuming deep_transform_values can be made available (there is one in rails I see) exactly how you then handle the above case in the the intermediate template somewhat makes my head hurt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Which is I'd think that Puppet should have a native way. Isn't there a .resolve function like there is .unwrap for Sensitive?

traylenator added a commit to traylenator/puppet-redis that referenced this pull request Apr 23, 2024
Process the configuration file `redis.conf` template twice if
some of the templates values are deferred.

Currently with deferred values the resulting deferred
template cannot be processed since it contains
complex datatypes from stdlib in particular.

This is a redis specific solution that may arrive genraly one
day in puppetlabs/puppetlabs-stdlib#1425
traylenator added a commit to traylenator/puppet-redis that referenced this pull request Apr 23, 2024
Process the configuration file `redis.conf` template twice if
some of the templates values are deferred.

Currently with deferred values the resulting deferred
template cannot be processed since it contains
complex datatypes from stdlib in particular.

This is a redis specific solution that may arrive genraly one
day in puppetlabs/puppetlabs-stdlib#1425
traylenator added a commit to traylenator/puppet-redis that referenced this pull request Apr 26, 2024
Process the configuration file `redis.conf` template twice if
some of the templates values are deferred.

Currently with deferred values the resulting deferred
template cannot be processed since it contains
complex datatypes from stdlib in particular.

This is a redis specific solution that may arrive genraly one
day in puppetlabs/puppetlabs-stdlib#1425
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of deferred function fails due to complex types in epp
4 participants