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

Add support for mod_md #2090

Merged
merged 5 commits into from
Dec 14, 2020
Merged

Add support for mod_md #2090

merged 5 commits into from
Dec 14, 2020

Conversation

smortex
Copy link
Collaborator

@smortex smortex commented Nov 16, 2020

This PR add support for mod_md, a module for managing domains across virtual hosts and certificate provisioning via the ACME protocol.

What is implemented

Each of these points have been added as a separate commit for clarity:

  • Add support for mod_watchdog — it's a dependency of mod_md;
  • Add support for mod_md — enable the module and configure it;
  • Connect mod_md with apache::vhost to make it usable;
  • Add basic integration test when the module is enabled.

It brings full support for mod_md into the Apache module.

What is not supported

Support for the <MDomain> sections is not part of this PR. <MDomain> allows to override general configuration when requesting certificates from multiple providers. This use case is rather advanced so I guess support for it can be skipped as of today. Future contributors might be able to add support for this by allowing to pass an Enum to $apache::vhost::mdomain with all custom parameters.

Basic use-case

Lines marked with <<< are the one added to the basic configuration thanks to this module:

class { 'apache':
}

class { 'apache::mod::md':                        # <<<
  md_certificate_agreement => 'accepted',         # <<<
  md_contact_email         => '[email protected]', # <<<
}                                                 # <<<

apache::vhost { 'example.com':
  docroot => '/var/www/example.com',
  port    => 443,
  ssl     => true,
  mdomain => true,                                # <<<
}

@puppet-community-rangefinder
Copy link

apache::mod::watchdog 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.

@codecov-io
Copy link

codecov-io commented Nov 16, 2020

Codecov Report

❗ No coverage uploaded for pull request base (main@c3fc5a7). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2090   +/-   ##
=======================================
  Coverage        ?   57.40%           
=======================================
  Files           ?       12           
  Lines           ?      216           
  Branches        ?        0           
=======================================
  Hits            ?      124           
  Misses          ?       92           
  Partials        ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3fc5a7...b082294. Read the comment docs.

@smortex smortex force-pushed the mod_md branch 4 times, most recently from e99405c to 3eb6671 Compare November 17, 2020 01:12
@smortex smortex marked this pull request as ready for review November 17, 2020 02:01
@smortex smortex requested a review from a team as a code owner November 17, 2020 02:01
@sanfrancrisko sanfrancrisko self-assigned this Nov 23, 2020
Copy link
Contributor

@sanfrancrisko sanfrancrisko left a comment

Choose a reason for hiding this comment

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

Thank you very much @smortex for this excellent enhancement. We really appreciate the atomic commits, test coverage and detail you've put in to the description 👍

I'd be happy to merge this as is, but I do wonder if you would be up for adding a basic acceptance test to ensure that the MOD installs OK on a platform? I was having a conversation with another contributor on their PR about this too.

Fully aware that trying to support Apache MODs on all versions of all Linux distros is somewhat of a nightmare (and even if it works now, it may not do so in a future release!).

We recently added functionality to limit the test execution to platforms we only wanted to support - you can read about that here.

If you wanted to identify the platform(s) you're interested in and add a basic acceptance test similar to what I did in TigerKriika#1, then it would allow us to catch any potential future regressions caused by a tweak in package name. This is a very common problem with Apache MODs between new versions of OSs.

Thanks again for the excellent contribution - will look forward to getting it over the line very soon. Give me a shout if I can be of any help regarding a basic acceptance test.

@smortex smortex force-pushed the mod_md branch 2 times, most recently from 15c96d4 to 5936e18 Compare November 24, 2020 00:03
@smortex smortex marked this pull request as draft November 24, 2020 00:08
@smortex smortex force-pushed the mod_md branch 5 times, most recently from 88686a8 to 47549c9 Compare November 24, 2020 07:18
Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

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

🙋🏻‍♀️

templates/vhost/_ssl.erb Show resolved Hide resolved
@smortex smortex force-pushed the mod_md branch 5 times, most recently from d076cb8 to 82ad1b7 Compare November 24, 2020 23:05
This apache module defines programmatic hooks for other apache modules
to periodically run tasks.  It is a dependency for some apache modules
not yet supported by the puppet module:

  * mod_heartbeat
  * mod_heartmonitor
  * mod_md
  * mod_proxy_hcheck
Allow configuring all parameters provided by the module.
Add a $mdomain parameter to apache::vhost.  When set to true, the
certifcate configuration is automatically managed by mod_md.  It is also
possible to use an explicit String to fully control the Subject
Alternative Names of the requested certificate.
Copy link
Collaborator Author

@smortex smortex left a comment

Choose a reason for hiding this comment

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

Thanks for suggesting adding some acceptance test @sanfrancrisko, it helped spot an issue that affected Debian 👍

I fixed this issue by amending the commit, and added the acceptance test as a new commit.

manifests/mod/watchdog.pp Show resolved Hide resolved
@smortex smortex marked this pull request as ready for review November 25, 2020 01:43
Copy link
Contributor

@igalic igalic left a comment

Choose a reason for hiding this comment

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

i was just looking at my own config and how i handle that stuff there, and I use apache::vhost::custom to generate the following:

MDomain igalic.co auto

<VirtualHost *:80>
  ServerName igalic.co
  DocumentRoot '/var/empty'
</VirtualHost>

<VirtualHost *:443>
  ServerName igalic.co
  DocumentRoot /srv/igalic.co/site
  SSLEngine On
</VirtualHost>

That being said:

templates/vhost/_file_header.erb Show resolved Hide resolved
Copy link
Contributor

@sanfrancrisko sanfrancrisko left a comment

Choose a reason for hiding this comment

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

This looks good @smortex - thanks again. I'll leave it open for another 48 hours or so to give @igalic a chance to approve / confirm all her queries were addressed. I believe that is the case, but just want to be 100% sure.

@smortex smortex requested a review from igalic December 7, 2020 16:43
@david22swan
Copy link
Member

@smortex
This looks good to me and already has two approves on it so I'm gonna go ahead and merge.
Thanks for the work you put in :)

@david22swan david22swan merged commit ffac0c6 into puppetlabs:main Dec 14, 2020
@smortex smortex deleted the mod_md branch December 14, 2020 15:50
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.

5 participants