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

Finish conversion to EPP templates #2545

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
+1,104 −2,447
Open

Finish conversion to EPP templates #2545

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ae3632a
Add missing line breaks
smortex May 5, 2024
b2b0350
Remove unused file
smortex May 3, 2024
e3320a5
Fix apache::vhost::error_documents data type
smortex May 4, 2024
ce29022
Make sure we still support ERB templates when provided
smortex May 5, 2024
adb5b1a
Switch last templates to EPP
smortex May 4, 2024
14f2189
Fix CI
smortex May 5, 2024
7c48bce
Simplify the way requires are passed to the template
smortex May 17, 2024
6d15dbe
Simplify type checking
smortex May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Switch last templates to EPP 
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.
  • Loading branch information
@smortex @Ramesh7
smortex authored and Ramesh7 committed Jun 6, 2024
commit adb5b1a82faca05bed3a514047815903df2d8bf3
6 changes: 3 additions & 3 deletions manifests/mod/php.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
Optional[String] $path = undef,
Array $extensions = ['.php'],
Optional[String] $content = undef,
String $template = 'apache/mod/php.conf.erb',
String $template = 'apache/mod/php.conf.epp',
Copy link
Collaborator Author

@smortex smortex May 5, 2024 •
edited
Loading

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.

Optional[String] $source = undef,
Optional[String] $root_group = $apache::params::root_group,
Optional[String] $php_version = $apache::params::php_version,
Expand Down Expand Up @@ -63,9 +63,9 @@
fail('apache::mod::php requires apache::mod::prefork or apache::mod::itk; please enable mpm_module => \'prefork\' or mpm_module => \'itk\' on Class[\'apache\']')
}

if $source and ($content or $template != 'apache/mod/php.conf.erb') {
if $source and ($content or $template != 'apache/mod/php.conf.epp') {
warning('source and content or template parameters are provided. source parameter will be used')
} elsif $content and $template != 'apache/mod/php.conf.erb' {
} elsif $content and $template != 'apache/mod/php.conf.epp' {
warning('content and template parameters are provided. content parameter will be used')
}

Expand Down
57 changes: 19 additions & 38 deletions manifests/mod/security.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,49 +316,30 @@
}

if $manage_security_crs {
# Template uses:
# - $_secdefaultaction
# - $critical_anomaly_score
# - $error_anomaly_score
# - $warning_anomaly_score
# - $notice_anomaly_score
# - $inbound_anomaly_threshold
# - $outbound_anomaly_threshold
# - $paranoia_level
# - $executing_paranoia_level
# - $allowed_methods
# - $content_types
# - $restricted_extensions
# - $restricted_headers
# - $secrequestmaxnumargs
# - $enable_dos_protection
# - $dos_burst_time_slice
# - $dos_counter_threshold
# - $dos_block_timeout
$security_crs_parameters = {
'_secdefaultaction' => $_secdefaultaction,
'critical_anomaly_score' => $critical_anomaly_score,
'error_anomaly_score' => $error_anomaly_score,
'warning_anomaly_score' => $warning_anomaly_score,
'notice_anomaly_score' => $notice_anomaly_score,
'inbound_anomaly_threshold' => $inbound_anomaly_threshold,
'outbound_anomaly_threshold' => $outbound_anomaly_threshold,
'secrequestmaxnumargs' => $secrequestmaxnumargs,
'allowed_methods' => $allowed_methods,
'content_types' => $content_types,
'restricted_extensions' => $restricted_extensions,
'restricted_headers' => $restricted_headers,
'paranoia_level' => $paranoia_level,
'executing_paranoia_level' => $executing_paranoia_level,
'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,
'_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.

'critical_anomaly_score' => $critical_anomaly_score,
'error_anomaly_score' => $error_anomaly_score,
'warning_anomaly_score' => $warning_anomaly_score,
'notice_anomaly_score' => $notice_anomaly_score,
'inbound_anomaly_threshold' => $inbound_anomaly_threshold,
'outbound_anomaly_threshold' => $outbound_anomaly_threshold,
'secrequestmaxnumargs' => $secrequestmaxnumargs,
'allowed_methods' => $allowed_methods,
'content_types' => $content_types,
'restricted_extensions' => $restricted_extensions,
'restricted_headers' => $restricted_headers,
'paranoia_level' => $paranoia_level,
'executing_paranoia_level' => $executing_paranoia_level,
'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 •
edited
Loading

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.

}

file { "${modsec_dir}/security_crs.conf":
ensure => file,
content => template('apache/mod/security_crs.conf.erb'),
content => epp('apache/mod/security_crs.conf.epp', $security_crs_parameters),
require => File[$modsec_dir],
notify => Class['apache::service'],
}
Expand Down
9 changes: 7 additions & 2 deletions manifests/mod/status.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,17 @@

$requires_defaults = 'ip 127.0.0.1 ::1'

# Template uses $extended_status, $status_path
$status_params = {
'extended_status' => $extended_status,
'status_path' => $status_path,
'requires' => $requires,
'requires_defaults' => $requires_defaults,
}
file { 'status.conf':
ensure => file,
path => "${apache::mod_dir}/status.conf",
mode => $apache::file_mode,
content => template('apache/mod/status.conf.erb'),
content => epp('apache/mod/status.conf.epp', $status_params),
require => Exec["mkdir ${apache::mod_dir}"],
before => File[$apache::mod_dir],
notify => Class['apache::service'],
Expand Down
Loading