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

Finish conversion to EPP templates #2545

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Finish conversion to EPP templates #2545

wants to merge 8 commits into from

Conversation

smortex
Copy link
Collaborator

@smortex smortex commented May 4, 2024

A lot of work was done to convert the module templates form ERB to EPP, but a few templates where still to be converted.

Along with various benefits, EPP templates offer better detection for access to undefined variables. This refactoring therefore fix a few issues that where reported while converting. Also a bunch of outdated comments about which template use which variable where removed no that this usage is explicit.

The extensive test suite helped ensure the conversion was not introducing regressions.

String $template = 'apache/mod/php.conf.erb',
String $template = 'apache/mod/php.conf.epp',
Copy link
Collaborator Author

@smortex smortex May 5, 2024

Choose a reason for hiding this comment

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

This change should be backwards-compatible. See related commit 675fb82 for details.

@smortex smortex force-pushed the epp branch 2 times, most recently from a77dcd9 to 29b5284 Compare May 5, 2024 20:17
@smortex smortex marked this pull request as ready for review May 5, 2024 21:27
@smortex smortex requested review from bastelfreak, ekohl and a team as code owners May 5, 2024 21:27
bastelfreak
bastelfreak previously approved these changes May 6, 2024
Copy link

@Dan33l Dan33l left a comment

Choose a reason for hiding this comment

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

Huge PR. I tried to add my eyes, and added an inline comment.

templates/mod/status.conf.erb Show resolved Hide resolved
@binford2k
Copy link
Contributor

Hallo @smortex! Fill out this form if you'd like to claim your May the Source Be With You sticker! https://forms.office.com/r/Cn55uJmWMH

Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I wish we had more type enforcement in templates and EPP allows that. If this is backwards incompatible anyway, it may be a good moment to do so.

'dos_burst_time_slice' => $dos_burst_time_slice,
'dos_counter_threshold' => $dos_counter_threshold,
'dos_block_timeout' => $dos_block_timeout,
'_secdefaultaction' => $_secdefaultaction,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we shouldn't pass arguments prefixed with _, so would prefer:

Suggested change
'_secdefaultaction' => $_secdefaultaction,
'secdefaultaction' => $_secdefaultaction,

I know other places do this wrong now, so I'd be OK with leaving that for later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided to go this way in order to limit the changes to just ERB -> EPP. IMHO, we should pass secdefaultaction => $secdefaultaction and move the code that does the transformation into the template, but it would be hard to track the move with all other changes, and as you say this kind of fix would be welcome in other places.

So for now, my proposal is to keep it that way.

@@ -40,11 +40,17 @@
$requires_defaults = 'ip 127.0.0.1 ::1'

# Template uses $extended_status, $status_path
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we already have a hash, the comment is redundant

Suggested change
# Template uses $extended_status, $status_path

@@ -1,5 +1,5 @@
<% $_requires = if $requires { %>$requires<% } else {%>$requires_defaults<%} %>
<% if type($_requires, 'generalized') == String { %>
<% $_requires = if $requires { $requires } else { $requires_defaults} -%>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this equal to:

Suggested change
<% $_requires = if $requires { $requires } else { $requires_defaults} -%>
<% $_requires = pick($requires, $requires_defaults) -%>

But then I'd prefer to have that on a higher level (where epp() is called).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unrelated, but wtf is String(type($_requires, 'generalized')).index('Array') == 0? Isn't that equal to $_requires =~ Array?

Would it make sense to also use native types to enforce a structure? I think the resulting template is easier to understand:

<%- | Variant[
        String[0],
        Array[String[1]],
        Struct[
          {
            enforce  => Optional[Enum['all', 'none', 'any']],
            requires => Array[String[1]],
          },
        ],
      ] $requires,
| -%>
<% if $requires.downcase in ['', 'unmanaged'] { -%>
<% elsif $requires =~ String { -%>
    Require <%= $requires %>
<% } elsif $requires =~ Array -%>
  <%- $requires.each |$req| { -%>
    Require <%= $req %>
  <%- } -%>
<% } $requires =~ Hash { -%>
  <%- if $requires['enforce'] { -%>
    <%- $enforce_str = "Require${requires['enforce'].capitalize}>\n" -%>
    <%- $enforce_open = "    <${enforce_str}" -%>
    <%- $enforce_close = "    </${enforce_str}" -%>
    <%- $indentation = '    ' -%>
  <%- } else { -%>
    <%- $enforce_open = '' -%>
    <%- $enforce_close = '' -%>
    <%- $indentation = '' -%>
  <%- } -%>
<%# %><%= $enforce_open -%>
      <%- $requires['requires'].each |$req| { -%>
<%# %>    <%= $indentation -%>Require <%= $req %>
      <%- } -%>
<%# %><%= $enforce_close -%>
<% } -%>

I'd even be tempted to rewrite the last bit to avoid variables:

<% } $requires =~ Hash { -%>
  <%- if $requires['enforce'] { -%>
    <Require${requires['enforce'].capitalize}>
      <%- $requires['requires'].each |$req| { -%>
        Require <%= $req %>
      <%- } -%>
    </Require${requires['enforce'].capitalize}>
  <%- } else { -%>
      <%- $requires['requires'].each |$req| { -%>
    Require <%= $req %>
      <%- } -%>
  <%- } -%>
<% } -%>

<%# with: -%>
<%# "scope.call_function('template', ["apache/mod/<Template>"])" -%>
<%= epp("apache/mod/_require.epp") -%>
<%= epp("apache/mod/_require.epp", { 'requires' => $requires, 'requires_defaults' => $requires_defaults }) -%>
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this would become:

Suggested change
<%= epp("apache/mod/_require.epp", { 'requires' => $requires, 'requires_defaults' => $requires_defaults }) -%>
<%= epp("apache/mod/_require.epp", { 'requires' => pick($requires, $requires_defaults) }) -%>

@@ -0,0 +1,16 @@
<% if $php_values and !$php_values.empty { -%>
<%- $php_values.stdlib::sort_by |$m| { $m[0] }.each |$key, $value| { -%>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't hash sorting always by key anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, the sort function does not accept Hash, only Array or String. I though a second about adding this to Puppet, but that would require new versions of Puppet and might be an issue…

Comment on lines +13 to +14
<%- if $flag.downcase =~ /true|yes|on|1/ { $_flag = 'on' } else { $_flag = 'off' } -%>
php_flag <%= $key %> <%= $_flag %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can avoid the additional variable with little loss of readability here

Suggested change
<%- if $flag.downcase =~ /true|yes|on|1/ { $_flag = 'on' } else { $_flag = 'off' } -%>
php_flag <%= $key %> <%= $_flag %>
php_flag <%= $key %> <%= if $flag.downcase =~ /true|yes|on|1/ { 'on' } else { 'off' } %>

<% if $php_admin_flags and !$php_admin_flags.empty { -%>
<%- $php_admin_flags.stdlib::sort_by |$m| { $m[0] }.each |$key, $flag| { -%>
<%# normalize flag -%>
<%- if $flag.downcase =~ /true|yes|on|1/ { $_flag = 'on' } else { $_flag = 'off' } -%>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given the repetition, perhaps a helper would be useful? There's a lot of overlap with $_php.epp anyway so other comments I had there apply here too.

@@ -0,0 +1,17 @@
<% unless $setenv.empty { -%>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move the empty() check to vhost.pp and only include the concat fragment if it's non-empty? In fact, we already have $use_env_mod = !empty($setenv) so you can reuse that variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I saw that but kept it as it was before. I will add a commit to avoid the double-test.

@@ -0,0 +1,68 @@
<% if $ssl { -%>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like _setenv.epp, I'd prefer to keep the if in the Puppet code and avoid rendering the whole template.

This code section is weird.  My guess is someone unexpectedly pressed
`J` in vi and joined multiple lines.
The template in templates/vhost/_error_document.erb is only functionnal
if we pass an array of hashes.  This is going to be simplified in a
future commit so only accept values that produce working configuration
and reject configuration that is invalid and ignored.
`apache::mod::php` allows to pass an ERB template, switching the default
template to EPP will require us to change the default value of the
`template` parameter which is generally a breaking change.

Users who rely on this parameter to provide a custom template are
currently using an ERB template, so we must preserve the legacy behavior
for them, and detect if the template should be processed as ERB or EPP.

For this purpose, we check the file extension in a conservative way (any
template whose filename does not end with `.epp` is assumbed to be an
ERB template).

As a result, this change is backwards-compatible for end-users.
A lot of work was done to convert the module templates form ERB to EPP,
but a few templates where still to be converted.

Along with various benefits, EPP templates offer better detection for
access to undefined variables.  This refactoring therefore fix a few
issues that where reported while converting.  Also a bunch of outdated
comments about which template use which variable where removed no that
this usage is explicit.

The extensive test suite helped ensure the conversion was not
introducing regressions.
A required variable in the template does not exist in the class
parameters.  Maybe nobody use this and we can just ignore this for the
next few weeks RedHat 7 (the only OS exercising this code path) is
supported?
The typing system allows us to build more straightforward type checking,
while here replace some conditional constructs to non-conditional ones
as we usualy do nowadays.
'enable_dos_protection' => $enable_dos_protection,
'dos_burst_time_slice' => $dos_burst_time_slice,
'dos_counter_threshold' => $dos_counter_threshold,
'dos_block_timeout' => $dos_block_timeout,
Copy link

@sbrowne-godaddy sbrowne-godaddy Aug 13, 2024

Choose a reason for hiding this comment

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

For CRS2, the $security_crs_parameters also needs a anomaly_score_blocking defined (oddly, as shown in the diff of the security_crs.conf.epp).

The following additional line in this block fixed it for me.

      'anomaly_score_blocking'     => downcase($modsec_secruleengine),

The rule is only ever looking for the lowercase string of on, so the defaults pass through cleanly and any other value is safely ignored.

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.

7 participants