Build all MPMs as shared modules

[tianon]

Closes #11
Closes #63

[yosifkit]

Alpine needs this too. 😉

[tianon]

Updated!

[mnloff]

Thanks for the configuration fix, @oskapt - this unexpected change took us offline as well.

[tianon]

Arg whoops, thanks @oskapt -- will look into a fix ASAP.

[tianon]

Hmm, this appears to already be there in the image-provided configuration: 😕

$ docker pull httpd
Using default tag: latest
latest: Pulling from library/httpd
Digest: sha256:b77059f169e57fb4046813127cf4bc75b5a67ed6021ad85f1abf318e0324fcfe
Status: Image is up to date for httpd:latest

$ docker run --rm httpd grep mod_mpm conf/httpd.conf
LoadModule mpm_event_module modules/mod_mpm_event.so
#LoadModule mpm_prefork_module modules/mod_mpm_prefork.so
#LoadModule mpm_worker_module modules/mod_mpm_worker.so

😞

[jdmcfad]

Anyone who's running with that configuration modified will be probably impacted by this tag rerelease.
None of your consumers can be expected to be on the lookout for functional changes in example configuration files in already-tagged-and-released Docker images.

[tianon]

It's certainly not unprecedented for an official image to have a change like this in an already-released image tag. Folks looking for maximum stability image references should probably be using image digests instead of tags (https://github.com/docker-library/repo-info is probably useful here), since by their nature tags are mutable.

Point taken that this is an unfortunate change though. What would you recommend the fix be? By now, we've got quite a few consumers who've updated their configuration with the appropriate LoadModule line, so if we revert, they'll be broken again, and even more frustrated.

It seems like our best option now is to soldier through, and recommend that folks use the opportunity to update their CI/CD pipeline to at least fire up the container temporarily and verify that it runs and responds to connections before deploying it.

[oskapt]

@tianon agreed that you shouldn't revert now. what's done is done. i don't know what the best course of action might have been - with the foresight to see the potential impact, perhaps no change at all?

if i install apache from source, i expect it to have a sane set of defaults that i can modify with my own config. if apache changes the defaults, they'll do it on a release, possibly add some error checking (a little sed love and a big fat deprecation warning to stderr) to make sure that the change is backwards-compatible, and then give it some time to bake before removing the sed munging a few releases later.

it's normal for a child image to modify httpd.conf and add/remove modules/includes as necessary, so it's very likely that this will affect anyone not running the stock image with all defaults.

regarding the "not unprecedented" comment and suggestion that consumers use specific digest instead of a tag, you might be right...but i don't think it's a realistic expectation. by using the 2.4 tag i understand that i'll get the 2.4 source with all its defaults, not the 2.4 source plus some other stuff that someone i don't know thinks is better than what apache ships with on its own. apache has 20+ years of reliability, and (for me at least) the httpd docker image should also be reliable. that's the point of being the official image.

the change to make mpms shared was not required for anything other than convenience. if i want to change the defaults from upstream, i'll copy your Dockerfile and roll my own image.

i know i sound snarky, and i don't mean to. it's an uncomfortable situation where both of us have merit in our arguments. you thought you were doing the right thing, and maybe i shouldn't trust your image in the way that i did.

to that end, perhaps you can take my earlier suggestion and update the image with some sed to make sure that the appropriate LoadModule line gets inserted automatically.

[tianon]

If you look at the contents of this PR, we didn't modify the configuration at all. All we did was adjust the flags used to compile Apache, and it was upstream's code which changed that default configuration. We intentionally do not supply any modifications from upstream's default-supplied configurations except to point logs to stdout/stderr by default, and let Apache upstream's defaults ride straight through.

It's definitely unfortunate that changing upstream's build flags will change the default configuration that they generate, but that's where we're at.

To be clear, you're suggesting that we add an additional entrypoint to the image which modifies a user-supplied configuration file in order to add a LoadModule directive for mpm_event_module if it doesn't exist?

Honestly, that seems pretty complicated to get right, and not accidentally break someone who's intentionally loading some other MPM, which might not even happen in their httpd.conf thanks to Include. 😕

[oskapt]

Upstream is smart. It dynamically generates config files so that they work. If you have --enable-mpms-shared=all in your configure arguments, then it inserts a LoadModule line for mpm_event into httpd.conf. If you do not have this configure option, it does not insert that line.

(I built your image twice from source, changing only that line in Dockerfile, so I'm 100% sure that I am correct in this statement.)

This means that by changing your compile-time flags, you changed httpd.conf in a way that is not backwards compatible. You broke every image on the Internet that is derived from yours and was not using the default-supplied httpd.conf. You did this for convenience, not for anything more important than that, and saying, "we don't modify upstream's configuration" is a red herring. By changing the compile-time flags you did modify the configuration.

But let's stop discussing the why and wherefore, because it doesn't solve anything. Let's look at how you can fix it.

You already have an entrypoint that's a shell script. It's trivial for you to modify it. You can run apachectl -t and check the exit status. If the MPM is missing, it tells you. No guessing required! You can check if the LoadModule statements are in httpd.conf. You can check if there's an Include before that, and if not, modify it.

Maybe you don't want to modify a user's config - that makes sense. In that case run the checks and output some useful data to stderr like, "Hey - sorry for the inconvenience, but you're missing a LoadModule statement that is required after January 25, 2018 - see <url> for more information on what to change." You could even tell them the default line to insert and where to put it, so that the time to recovery is short.

At least then when people check the container logs, they're not baffled by why their MPM suddenly went for a walk.

[tianon]

I love the suggestion of a warning to help guide users to the appropriate fix to their configuration -- do you have some example code which does that in a clean way? A PR would be really great, but otherwise just a blurb of code which does the appropriate detection would be really helpful.

[mnloff]

FWIW, thanks for maintaining these official containers. Even with today's hiccup, your work is greatly appreciated by a lot of developers.

[oskapt]

@tianon i'll work on this today.

[oskapt]

The following will test the apache config. If the MPM issue is present, it will report how to solve it. If a different error is present, it will report that instead. It will then sleep for X seconds before exiting. If there are no issues with the test, it will start the container.

The message output is configurable in the heredocs at the top - I have the MPM message pointing to this URL, but it should probably point to an issue or a snippet or something that explains what happened.

I removed set -e because it would cause premature failure when the tests fail. It didn't add any value anyway (and actually forced you to use -f in the rm of httpd.pid).

I changed the rm -f to a test and removed the -f from the rm. Unilateral usage of force is not recommended.

#!/bin/bash

# Message for addition of shared MPM modules
read -r -d '' msg_mpm <<'EOF'
ERROR: Recent changes now require that you specify
an MPM module to load in httpd.conf. If you wish to use
the default module (mpm_event), you can place the following
line at the top of your LoadModule directives:

  LoadModule mpm_event_module modules/mod_mpm_event.so

For more information, see the following URL:

  https://github.com/docker-library/httpd/pull/87

EOF

# Generic error message
read -r -d '' msg_normal <<'EOF'
ERROR: There are errors in your apache configuration.

EOF

# seconds to sleep before exiting the container on failure
sleep_time=5

# function to output data to stderr
function errecho() {
  if [[ ! -z "$1" ]]; then
    >&2 echo "$1"
  fi
}

# test if apache will start
response=$(apachectl -t 2>&1)

if [ $? -ne 0 ]; then
  # test failed
  if [[ $response =~ "AH00534" ]]; then
    errecho "$msg_mpm"
  else
    errecho "$msg_normal"
    errecho "$response"
  fi

  sleep $sleep_time
  exit 1
fi

# Apache gets grumpy about PID files pre-existing
if [[ -e /usr/local/apache2/logs/httpd.pid ]]; then
  rm /usr/local/apache2/logs/httpd.pid
fi

exec httpd -DFOREGROUND

[t3chris]

I want to add that also here builds and continuous integration broke. The result was that several teams have been searching for hours for the root cause.

I suggest that in such cases the tag of the docker image is modified, e.g. to "2.4.29.1" whereas the last digit would indicate changes to the Docker image's infrastructure, but not to the containerized application.

Has there been a decision on the way forward?