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

Update load.epp to avoid loading a module already loaded #2483

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

Conversation

mz4r
Copy link

@mz4r mz4r commented Oct 11, 2023

I encountered a problem during an Apache update. The scriplet executed during updates set up the default configuration of module loads. If the node with puppet agent no longer has the puppet configuration active then there is a duplicate problem in the loading of modules, example of warning returned:
[so:warn] [pid 15619] AH01574: module dav_module is already loaded, skipping

The commit therefore makes it possible to load the module if it is not already loaded and thus avoid warnings on duplicates.

avoid loading a module already loaded if default file module are put during Apache upgrade via the scriplet
@mz4r mz4r requested review from bastelfreak, ekohl, smortex and a team as code owners October 11, 2023 08:50
@CLAassistant
Copy link

CLAassistant commented Oct 11, 2023

CLA assistant check
All committers have signed the CLA.

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.

Can you share which OS + version you ran into this? I'd guess some EL based system since RPM can't deal with removed files (unlike DPKG for Debian), but then it would be useful to know which files. I think it's better to lay out files in a way that they overwrite native file locations instead of removing some file and creating it elsewhere.

@@ -4,4 +4,6 @@ LoadFile <%= $loadfile %>
<% } -%>

<% } -%>
<IfModule <%= $_id %> >
Copy link
Collaborator

Choose a reason for hiding this comment

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

https://httpd.apache.org/docs/2.4/mod/core.html#ifmodule suggests this is executed if $_id is present, but shouldn't it be the other way around?

Suggested change
<IfModule <%= $_id %> >
<IfModule !<%= $_id %> >

Fix : Forgot "!"
@mz4r
Copy link
Author

mz4r commented Oct 11, 2023

Can you share which OS + version you ran into this? I'd guess some EL based system since RPM can't deal with removed files (unlike DPKG for Debian), but then it would be useful to know which files. I think it's better to lay out files in a way that they overwrite native file locations instead of removing some file and creating it elsewhere.

OS : Oracle Linux 8 => 5.15.0-104.119.4.2.el8uek.x86_64

Examples files put by scriplet :
01-cgi.conf
00-base.conf
00-dav.conf
00-lua.conf

Apache module configure "loadModule" in <module_name>.load ( example "ssl.load"), but apache scriplet set in "00-ssl.conf".
File list set by apache scriplet depend that installed modules.

"I think it's better to lay out files in a way that they overwrite native file locations instead of removing some file and creating it elsewhere."

Of course, i totaly agree, but the modules management is different. It would be necessary to modify the entire module management in Apache puppet module .

Scriplet apache can put many "LoadModule" in one file.
example /etc/httpd/conf.modules.d/00-proxy.conf :
LoadModule proxy_module modules/mod_proxy.so
LoadModule lbmethod_bybusyness_module modules/mod_lbmethod_bybusyness.so
LoadModule lbmethod_byrequests_module modules/mod_lbmethod_byrequests.so
LoadModule lbmethod_bytraffic_module modules/mod_lbmethod_bytraffic.so
LoadModule lbmethod_heartbeat_module modules/mod_lbmethod_heartbeat.so
LoadModule proxy_ajp_module modules/mod_proxy_ajp.so
LoadModule proxy_balancer_module modules/mod_proxy_balancer.so
LoadModule proxy_connect_module modules/mod_proxy_connect.so
LoadModule proxy_express_module modules/mod_proxy_express.so
LoadModule proxy_fcgi_module modules/mod_proxy_fcgi.so
LoadModule proxy_fdpass_module modules/mod_proxy_fdpass.so
LoadModule proxy_ftp_module modules/mod_proxy_ftp.so
LoadModule proxy_http_module modules/mod_proxy_http.so
LoadModule proxy_hcheck_module modules/mod_proxy_hcheck.so
LoadModule proxy_scgi_module modules/mod_proxy_scgi.so
LoadModule proxy_uwsgi_module modules/mod_proxy_uwsgi.so
LoadModule proxy_wstunnel_module modules/mod_proxy_wstunnel.so

But Apache module set in different file (proxy_ajp.load,proxy_balancer.load,proxy_connect.load ...)

Other one, the file 00-proxyhtml.conf :
LoadModule xml2enc_module modules/mod_xml2enc.so
LoadModule proxy_html_module modules/mod_proxy_html.so

Apache module set then in xml2enc.load and proxy_html.load

@ekohl
Copy link
Collaborator

ekohl commented Oct 11, 2023

So that's pretty much what I suspected. I think it's something that we should tackle properly. This now leads to 2 service restarts: once when the package is updated and once when the config is rewritten. At least the second time is downtime that shouldn't be needed.

I think an acceptance test that does the follow will catch it:

  • Apply some manifest
  • Verify it's idempotent
  • Run dnf reinstall httpd (possibly any separate module packages)
  • Apply manifest again and verify there are no changes.

Another way is to use rpm -qV httpd and verify there are no missing files in /etc/httpd.

@mz4r
Copy link
Author

mz4r commented Oct 13, 2023

Yes, so the easiest way to avoid this problem is to:

  • Check if configuration files for modules are not missing with: rpm -qV httpd
  • If the "purge_configs" variable is true, we create a file with empty content to avoid the duplicate problem and the implementation of default configuration not managed by the Apache module

So we avoid restarting Apache service multiple times.

If you agree, I can do the patch on my side and reopen a pull request.

Tell me if it suits you.

@ekohl
Copy link
Collaborator

ekohl commented Oct 13, 2023

Check if configuration files for modules are not missing with: rpm -qV httpd

As a regression test, it would be good to have this in acceptance tests. That makes adding a new OS version (like EL 10) easier.

If the "purge_configs" variable is true, we create a file with empty content to avoid the duplicate problem and the implementation of default configuration not managed by the Apache module

bf9f0d0 is a historical precedent for this. That time it affected mod_ssl on Red Hat. Anything we missed I'd consider a bug.

So we avoid restarting Apache service multiple times.

I'd like that a lot.

If you agree, I can do the patch on my side and reopen a pull request.

Tell me if it suits you.

It does suit me. Please do.

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.

4 participants