Skip to content

Conversation

@TigerKirika
Copy link
Contributor

The LoadModule ID is currently wrong created.

The module creates the .load-File with

LoadModule apreq2_module modules/mod_apreq2.so

instead of

LoadModule apreq_module modules/mod_apreq2.so

http://httpd.apache.org/apreq/docs/libapreq2/group__mod__apreq2.html

@TigerKirika TigerKirika requested a review from a team as a code owner November 5, 2020 12:38
@puppet-community-rangefinder
Copy link

apache::mod::apreq2 is a class

that may have no external impact to Forge modules.

This module is declared in 174 of 575 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@sanfrancrisko sanfrancrisko self-assigned this Nov 9, 2020
@sanfrancrisko
Copy link
Contributor

Thanks for the new feature to add support for the apreq2_module. From what you're describing, you seem to be inferring that with the current behaviour, there is a .load file being created with the module name set to apreq2_module? I'm not sure I see that behaviour currently - could you provide a manifest that will trigger the load of the apreq2_module module?

This change looks good, but it would need an acceptance test to verify that the module is indeed being loaded. Let me know if I can be of any help in that regard

@TigerKirika
Copy link
Contributor Author

I used the following Manifest on a Debian 10 (buster) machine:

class profile::web {
	class { 'apache' :
		default_charset   => 'utf-8',
		keepalive_timeout => 5,
		timeout           => 300,
		server_signature  => 'Off',
		trace_enable      => 'Off',
		mpm_module        => false,
		log_formats       => {
			combined       => '%a %l %u %t \"%r\" %>s %O \"%{Referer}i\" \"%{User-Agent}i\"',
			vhost_combined => '%v:%p %a %l %u %t \"%r\" %>s %O \"%{Referer}i\" \"%{User-Agent}i\"',
			common         => '%a %l %u %t \"%r\" %>s %O',
			referer        => '%{Referer}i -> %U',
			agent          => '%{User-agent}i'
		},
	}

	class { 'apache::mod::apreq2': }
	class { 'apache::mod::expires': }
	class { 'apache::mod::headers': }
	class { 'apache::mod::perl': }
	class { 'apache::mod::prefork': }
	class { 'apache::mod::rewrite': }

}

This installs the Apache and a apreq2.load File is created with the following content

LoadModule apreq2_module modules/mod_apreq2.so

The Apache can't start with this load File since the module name is wrong. You will get the following error-message:

apache2: Syntax error on line 41 of /etc/apache2/apache2.conf: Syntax error on line 1 of /etc/apache2/mods-enabled/apreq2.load: Can't locate API module structure `apreq2_module' in file /usr/lib/apache2/modules/mod_apreq2.so: /usr/lib/apache2/modules/mod_apreq2.so: undefined symbol: apreq2_module

If I install the apache and the libapache2-mod-apreq2 package manually it creates a apreq2.load File with the following content and the apache starts

LoadModule apreq_module modules/mod_apreq2.so

This change looks good, but it would need an acceptance test to verify that the module is indeed being loaded. Let me know if I can be of any help in that regard

I just started to work with puppet so I would appreciate your help regarding a acceptance test.

@sanfrancrisko
Copy link
Contributor

sanfrancrisko commented Nov 23, 2020

Thanks for the detailed explanation @TigerKriika

Firstly, regarding the acceptance tests, I would just keep it quite simple to begin with. If you create a new file under spec/acceptance/mod_apreq2_spec.rb, then paste this basic test in:

require 'spec_helper_acceptance'
apache_hash = apache_settings_hash

describe 'apache::mod::apreq2' do
  pp = <<-MANIFEST
    class { 'apache' : }
    class { 'apache::mod::apreq2': }
  MANIFEST

  it 'succeeds in installing the mod_authnz_apreq2 module' do
    apply_manifest(pp, catch_failures: true)
  end
end

This will reproduce the current issue you are encountering.

You'll need to implement a few more steps to add support for this MOD. The part missing is the package installation of the necessary MODs for the OS for this to be successful.

The way this is typically done is by this not so great method of updating the $mod_packages parameter in params.pp, here.

Typically, the Apache MOD support is going to be difficult to maintain across all Linux distros and versions. You may want to have a read at this blog post I made a while back.

It's also documented here

So I would:

  • Identify what platform(s) you want to get this MOD working on
  • Annotate the apache::mod::apreq2 class def in manifests/mod/apreq2.pp with the # @note Unsupported platforms: tag
  • Add the if: mod_supported_on_platform('apache::mod::apache::mod::apreq2') RSpec filtering condition
  • Enhance the params.pp to install the libapache2-mod-apreq2 package:
$mod_packages = {
        'apreq2'                => 'libapache2-mod-apreq2',
        ...

This commit ensures that the required package for the `apreq2` MOD
is installed on Debian 9 and 10.

Also adds a basic acceptance test for this MOD on those platforms.
@sanfrancrisko
Copy link
Contributor

@TigerKriika I've created a PR to your fork with the above changes: TigerKirika#1

If you want to expand on the basic acceptance test and/or increase the amount of platforms the MOD is supported on, feel free to do so.

…cements

(feat) Add package deps and tests for mod_apreq2 on Debian 9, 10
@TigerKirika
Copy link
Contributor Author

@sanfrancrisko
Thanks for your help. I merged your PR.

@sanfrancrisko sanfrancrisko changed the title Fix apreq2 LoadModule (feat) Add support for apreq2 MOD Nov 26, 2020
@sanfrancrisko sanfrancrisko changed the title (feat) Add support for apreq2 MOD (feat) Add support for apreq2 MOD on Debian 9, 10 Nov 26, 2020
@sanfrancrisko sanfrancrisko merged commit 2d3d81e into puppetlabs:master Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants