Fix bug #5328 apache module loading

[michaelgugino]

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

web_infrastructure/apache2_module.py

ANSIBLE VERSION

  lib/ansible/modules/core: (detached HEAD 55b1813ebd) last updated 2016/11/01 14:02:20 (GMT -400)
  lib/ansible/modules/extras: (detached HEAD cf524673e1) last updated 2016/11/01 12:39:51 (GMT -400)
  config file = 
  configured module search path = Default w/o overrides

SUMMARY

Currently, the apache2_module module parses apache configs
for correctness when enabling or disabling apache2 modules.

This behavior introduced a conflict condition when transitioning
between certain modules, such as mpm_worker and mpm_event.

This change only parses apache's configs during check mode,
otherwise it parses the stdout results of attempted to enabled
or disable modules to determine change state.

[robinro]

@michaelgugino Thanks for this PR. I now better understand what you meant in the discussion of #5328.

This changes the behavior quite drastically, since

• it always runs a2enmod (event if the module is already enabled/disabled), not sure whether this has a downside
• it doesn't check the apache config

The latter is used to fix your bug, but in some way this also removes a feature, that one now only gets in check mode.

What do you think about making the config check a flag, like check_config = yes/no, so that one can preserve the current behavior?

@n0trax Please comment/review. btw: do you want to be added as a module maintainer?

[michaelgugino]

@robinro
I think the current behavior of checking the config is incidental, and not part of the intended goals of the commit enabled the behavior.

I think the config behavior would be better captured in it's own module as it's not apache2_module specific, rather it captures config changes that are related to all of apache, not just modules; naturally, I assume that users are going to deploy valid configs which have already been tested elsewhere.

IMO, when a user runs ansible and they are not using 'check_mode' they are implicitly giving permission to ansible to make any changes necessary. Always running a2enmod or a2dismod only makes a change on disk if the current state does not equal the desired state, and if that change is made, it was desired at the time it was made.

In any case, check_config is a fine feature, but I think it's outside the scope of the behavior of the module as currently intended, and outside the scope of the bug fix. It should probably be it's own PR for a new feature.

[robinro]

@michaelgugino
The behavior might be unintended, but it is shipped in 2.2.0.0. Since it is useful, as it provides automatic checking of consistent configuration and forwards apache error messages to the user, it will be in use, so I don't want to drop it if there is an easy way to keep it. This is not a new feature, but an already shipped feature, that is just not explicitly documented.

Please add a check_config flag (or something similar) that runs _module_is_enabled to ensure the module (de)activation works.

In the documentation of that flag one can state the behavior changes like:

check_config: Run a check whether the module was enabled successfully and the apache2 configuration is consistent. C(True) corresponds to the 2.2.0 behavior, while C(False) corresponds to versions before 2.2.0.

[robinro]

Please also add an integration test for your use case in https://github.com/ansible/ansible/blob/devel/test/integration/targets/apache2_module/tasks/actualtest.yml (PR against ansible/ansible, that should be merged after this PR)

It should cover:

• test module fails to remove the last mpm module if check_mode: True or check_config: True
• successful switch of one mpm module vs. another if check_config: False

Maybe you can at the same time also include a test for #5455, which enables proxy_http with both proxy-html and proxy-uwsgi installed. Thanks.

[n0trax]

@robinro yes, thanks. I would be glad if i would be added as maintainer too.

@michaelgugino Thanks for your patch.
For Debian/Ubuntu this patch will work, but on OpenSUSe is a problem. On OpenSUSe a2enmod and a2dismod are not using a correct exit code. The exit code is always 0, even if the module does not exist. Our module then will return Changed=True, but the module is not loaded by the webserver.
Please see PR #2417

[michaelgugino]

This current patch set is really a series of compromises. I have been working with openSUSE locally to test the behavior of apachectl, a2enmod and a2dismod. Very nonstandard behavior for this distro. I have added in to force check the config for openSUSE; I am personally of the opinion this forced check should be removed and we should only perform this check with check_config enabled. There is no good way to verify if a module will actually be loaded or not by apache on openSUSE without parsing the config.

I am happy with the patch as-is, but I'm open to suggestions. Test-case patch forthcoming, please let me know what you think of this patch set.

[michaelgugino]

Created validation PR for patchset: ansible/ansible#18371

[robinro]

SuSE is not really "non-standard", since there is no standard. a2enmod is not an official part of apache2 and only a script that the debian and suse distros provide. Unluckily for us they didn't agree on the implementation.

• platform.linux_distribution does not work reliably (depends on python version and suse version)
• I would like to avoid any explicit distribution checks in the module if possible
  • this means: no use of platform and not checking the return code
• now the check_config behavior depends on the distro: suse always does it, debian only if it's set; thats maximally confusing

In general I would prefer a more minimal patch. Only if check_config is False skip around the module_is_enabled bits and instead parse the output.

[n0trax]

As i understand the apache2 manpage a -D DUMP_MODULES is actually parsing the config and dumps the modules which are loaded on service restart. The same behavior on Debian/Ubuntu and SUSe and RedHat...IMO we should not remove the _module_is_enabled check in the module.

I have done some research in the apache2 sources and figured out a method ap_check_mpm, which is the source of our problem. This method raises an error, if no mpm module is loaded. As far i can see, this check is the only one which causes an failure while dumping the modules. Except for a real error in the configuration...

How about ignoring the AH0054 error when activating any mpm module? This could be done easily and does not change too much in the module.

[michaelgugino]

@n0trax @robinro

Thank you both for the good feedback. I think this current patch set covers our use-cases based on 1) keeping existing behavior in-tact (although source slightly modified) 2) adding check_config bits for disabling the apachectl steps.

I think catching AH0054 approach has some merit, but that will not solve the other edge cases like #5455

In any case, please let me know what you think, I'm sure we can come up with a good patch set that covers most cases.

[gregdek]

Thanks @michaelgugino. To the current maintainers, @robinro, @n0trax please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit', 'needs_revision' or 'close_me' as appropriate.

[This message brought to you by your friendly Ansibull-bot.]

[n0trax]

I'm not really happy with disabling the config check, because we possibly are leaving the webserver in a broken state. Therefore we should not fix #5455, especially as this issue is already fixed by Debian.
At least the check_config flag should be default=True

The other problem is the check of the return code of a2enmod. As I mentioned earlier #5454 (comment), we could have a changed state, even if no module was activated.

IMO #5328 should be fixed by dealing with the specific error and not by disabling the whole check.

@robinro, your opinion?

[n0trax]

needs_revision

[gregdek]

Thanks @michaelgugino for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

[This message brought to you by your friendly Ansibull-bot.]

[michaelgugino]

@n0trax

For the return code 0 in openSUSE, I would say that we 'technically' have a changed state, although it might not be the desired one. using 'a2enmod non_existing_mod' on openSUSE will insert an entry for 'non_existing_mod' in apache's config in /etc/sysconfig. Additionally, using 'a2enmod -l' on openSUSE (which appears to be an openSUSE specific switch) will include 'non_existing_mod' in it's output, although apache will not actually load or complain about the non-existing module.

Specifically handling the mpm specific error code is a small tradeoff as well. It may result in an unbootable apache service if the deployer doesn't ensure that an mpm module is enabled.

Either solution is somewhat of a tradeoff. I have a small inclination towards the existing patch set because it's already written, but I'm happy to make additional changes if needed.

In any case, I think we should come to a consensus, and then execute on that idea.

[robinro]

To me it seems that mpm is really a special case. Especially since apache has a dedicated error for it. So a fix that only checks for that case should be best. I agree with @n0trax.
@michaelgugino Can you please adjust the PR accordingly?

Regarding loading non_existing_mod in openSUSE: We run apache2ctl -M (not a2enmod -l) exactly to cover that case. If the module does not exist or can not be loaded it will not show up in apache2ctl -M and thus make the ansible task fail. The a2enmod -l is a list of modules that should be loaded, but apache2ctl -M is the list of actually loaded modules.

[michaelgugino]

@robinro Sounds good. This latest patch just catches the error for mpm modules. It doesn't exit with any specific code, but parsing stderr seems to work okay.

[robinro]

@michaelgugino did most of the work already, so it should be his PR

I don't have a strong opinion on the name, but I want the default to be the current 2.2.0.0 behavior and ignoring the warnings should only be an option. I'd keep the name general, in case other modules should also be included there.

Another option I had in mind was to use the already existing force option for that, but this would mix two different aspects...

[robinro]

@n0trax which name would you prefer for that option?

[n0trax]

Unfourtunatly i have no nice idea for an other name...But what do you think about using the ignore_configcheck flag not that specific?

[michaelgugino]

@n0trax @robinro

Thanks for the additional feedback. I have merged in the changes from robinro's other PR.

I don't have a specific preference for the name of the additional config. Leaving it generic does allow for more potential workarounds later.

[gregdek]

@michaelgugino A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

[michaelgugino]

ready_for_review

[gregdek]

Thanks @michaelgugino. To the current maintainers, @robinro, @n0trax please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit', 'needs_revision' or 'close_me' as appropriate.

[This message brought to you by your friendly Ansibull-bot.]

[n0trax]

@robinro @michaelgugino

Could you please take a look at my suggestion for a more global ignore_configcheck setting. https://github.com/robinro/ansible-modules-core/pull/1/files

I think it is the last point for this PR...

[robinro]

I'm fine with the latest version.
@n0trax Do you agree? If so we can tell the bot to flag this and ship it before the repo merge.

[n0trax]

shipit

[gregdek]

Thanks again to @michaelgugino for this PR, and thanks @robinro, @n0trax for reviewing. Marking for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

[ansibot]

This repository has been locked. All new issues and pull requests should be filed in https://github.com/ansible/ansible

Please read through the repomerge page in the dev guide. The guide contains links to tools which automatically move your issue or pull request to the ansible/ansible repo.

[robinro]

@michaelgugino Please move this PR to the merged repo using the instructions above (https://prmover.pythonanywhere.com/).
It would be nice to have this in 2.2.1, which is about to be finalized.

[michaelgugino]

New PR created with PR mover:

ansible/ansible#19355

[nitzmahone]

Merged to devel via ansible/ansible#19355, probably will not be cherry-picked to stable-2.2 due to addition of new arg.