Fix dav_svn for Debian 10 #2135

martijndegouw · 2021-03-01T10:31:56Z

As far as I can tell Debian 10 still needs this work-around.

puppet-community-rangefinder · 2021-03-01T10:32:04Z

apache::mod::dav_svn is a class

that may have no external impact to Forge modules.

This module is declared in 175 of 576 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.

ekohl · 2021-03-01T10:36:49Z

manifests/mod/dav_svn.pp

  ::apache::mod { 'dav_svn': }

-  if $::osfamily == 'Debian' and ! ($::operatingsystemmajrelease in ['6', '9', '16.04', '18.04']) {
+  if $::osfamily == 'Debian' and ! ($::operatingsystemmajrelease in ['6', '9', '10', '16.04', '18.04']) {


This feels like a workaround on some older Debian versions while the newer ones behave as you would expect. Perhaps it should be checked which versions need this. I'd expect Ubuntu 20.04 behaves the same as Debian 10.

Then you would get to something like:

Suggested change

if $::osfamily == 'Debian' and ! ($::operatingsystemmajrelease in ['6', '9', '10', '16.04', '18.04']) {

if $::osfamily == 'Debian' and $::operatingsystemmajrelease in ['7', '8', '14.04'] {

Untested, but I think you get the idea.

Yeah I got it. I also did found some patches from Debian for the dav_svn module which should fix this in the package itself, but the issue still reproduces on my Debian 10 testing machine.

I just checked. The Debian 10 package has pretty similar word-around in mods-available/dav_svn.load, but that file is overwritten by this puppet module.

I must admit I haven't looked at the dav_svn module for ages. Perhaps you can write an acceptance test for the module so we know it really works rather than guessing? That also gives the best chance against regressions.

I will give a shot, It's not hard to reproduce since apache2 simply won't start when including apache::mod::dav_svn :D

Exactly, those are the things that are fairly easy to test.

Given the result of the acceptance test and looking at how Debian implemented their work-around it's safe to say that the whole version checking part of that if statement can be removed. Agreed?

I think that makes sense but it's probably good to check with the actual maintainers.

Thanks for the fix @martijndegouw and the assistance during the PR @ekohl . The test is great - allows us to check we haven't inadvertently stomp over some logic put in there for the older versions, as you said @ekohl .

codecov-io · 2021-03-01T11:11:29Z

Codecov Report

Merging #2135 (08fdb9b) into main (13f55bd) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2135   +/-   ##
=======================================
  Coverage   56.36%   56.36%           
=======================================
  Files          12       12           
  Lines         220      220           
=======================================
  Hits          124      124           
  Misses         96       96

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 8a67aa4...08fdb9b. Read the comment docs.

ekohl · 2021-03-02T15:34:33Z

spec/acceptance/mod_dav_svn_spec.rb

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


Could you add an idempotency check to this?

I’m not sure what you mean by this. Do you want to run the apply_manifest twice? To check if it fails/passes 2 times in a row?

Copied from another module:

it 'applies with no errors' do apply_manifest(pp, catch_failures: true) end it 'applies a second time without changes' do apply_manifest(pp, catch_changes: true) end

martijndegouw · 2021-03-04T10:06:25Z

Great, SLES 15 test is failing, on the exact same issue I have on my SLES server with the puppetlabs repo :/.
`Note: Signing data enables the recipient to verify that no modifications occurred after the data were signed. Accepting data with no, wrong or unknown signature can lead to a corrupted system and in extreme cases even to a system compromise.
Note: File 'repomd.xml' is the repositories master index file. It ensures the integrity of the whole repo.

Warning: This file was modified after it has been signed. This may have been a malicious change, so it might not be trustworthy anymore! You should not continue unless you know it's safe.

Signature verification failed for file 'repomd.xml' from repository 'Puppet 6 Nightly Repository sles 15 - x86_64'
`

sanfrancrisko · 2021-03-08T10:05:09Z

Great, SLES 15 test is failing, on the exact same issue I have on my SLES server with the puppetlabs repo :/.
`Note: Signing data enables the recipient to verify that no modifications occurred after the data were signed. Accepting data with no, wrong or unknown signature can lead to a corrupted system and in extreme cases even to a system compromise.
Note: File 'repomd.xml' is the repositories master index file. It ensures the integrity of the whole repo.

Warning: This file was modified after it has been signed. This may have been a malicious change, so it might not be trustworthy anymore! You should not continue unless you know it's safe.

Signature verification failed for file 'repomd.xml' from repository 'Puppet 6 Nightly Repository sles 15 - x86_64'
`

😞 Would this be a consistently reproducible error? Re-running seems to have gotten me past that issue now, but, if you're experiencing that pretty consistently, I'd like to get the issue raised with our Agent team to look in to.

sanfrancrisko · 2021-03-08T10:26:44Z

PR looks good, but there seems to be a unit test failure. I can't see why we wouldn't still generate a catalog with this File resource when your change is applied. Will have another look at it but just highlighting it here in case yourself or @ekohl figure it out before me.

The work-around that distros apply for dav_svn/authz_svn module loading issue is overwritten by this module. Always apply the work-around.

martijndegouw · 2021-03-08T10:43:17Z

PR looks good, but there seems to be a unit test failure. I can't see why we wouldn't still generate a catalog with this File resource when your change is applied. Will have another look at it but just highlighting it here in case yourself or @ekohl figure it out before me.

Sorry, I misted that one. I've updated the unit test as well.

david22swan · 2021-03-15T11:21:40Z

@martijndegouw This PR looks good to me, thanks for putting in the work.

martijndegouw requested a review from a team as a code owner March 1, 2021 10:31

ekohl reviewed Mar 1, 2021

View reviewed changes

ekohl reviewed Mar 2, 2021

View reviewed changes

martijndegouw added 2 commits March 8, 2021 11:42

Always apply dav_svn/authz_svn work-around

183489e

The work-around that distros apply for dav_svn/authz_svn module loading issue is overwritten by this module. Always apply the work-around.

Add testcase for the work-around in apache::mod:dav_svn

1498149

david22swan merged commit c2d7b9d into puppetlabs:main Mar 15, 2021

david22swan added the bugfix label Mar 15, 2021

	if $::osfamily == 'Debian' and ! ($::operatingsystemmajrelease in ['6', '9', '10', '16.04', '18.04']) {
	if $::osfamily == 'Debian' and $::operatingsystemmajrelease in ['7', '8', '14.04'] {

Fix dav_svn for Debian 10 #2135

Fix dav_svn for Debian 10 #2135

Uh oh!

Conversation

martijndegouw commented Mar 1, 2021

Uh oh!

puppet-community-rangefinder bot commented Mar 1, 2021

apache::mod::dav_svn is a class

This module is declared in 175 of 576 indexed public Puppetfiles.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Mar 1, 2021

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martijndegouw commented Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanfrancrisko commented Mar 8, 2021

Uh oh!

sanfrancrisko commented Mar 8, 2021

Uh oh!

martijndegouw commented Mar 8, 2021

Uh oh!

david22swan commented Mar 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

This module is declared in 175 of 576 indexed public `Puppetfiles`.

martijndegouw commented Mar 4, 2021 •

edited

Loading