Improve/silence warning from pull-request #2154 #2160

sircubbi · 2021-06-22T14:49:23Z

Hi,

pull-request #2154 did try to silence the warnings about servernames and filenames to cases where a problem actually exists.

However, there are two problems with that:

there is a check between "name" and "normalized_servername", when it actually should check for differences between "servername" and "normalized_servername".
even if that check is silenced, there is still the second warning about "use_port_for_filenames" not enabled. That warning should only be issued if "use_servername_for_filenames" is enabled.

fixes intention of pull-request #2154

…filename silences warnings to "real" cases, as it was intended with pull-request 2154

CLAassistant · 2021-06-22T14:49:28Z

All committers have signed the CLA.

puppet-community-rangefinder · 2021-06-22T14:49:29Z

apache::vhost is a type

Breaking changes to this file WILL impact these 128 modules (exact match):

Breaking changes to this file MAY impact these 34 modules (near match):

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

I must admit in reviewing this that I'm not entirely sure the whole deprecation for version 8 is that good of an idea. For example, I'm not looking forward to it. It's really a mess when upgrading.

For us the impact of removing use_port_for_filenames would be a lot less when it would not use the port if it's default (80 for http, 443 for https).

ekohl · 2021-06-22T14:53:54Z

manifests/vhost.pp

@@ -2111,7 +2111,7 @@
    false => $name,
  }

-  if ! $use_servername_for_filenames and $name != $normalized_servername {
+  if ! $use_servername_for_filenames and $servername != $normalized_servername {


This looks correct to me. I guess in my testing I didnt' specify a servername so they happened to match.

Btw, I'm reading this again and what I based it on was this part:

puppetlabs-apache/manifests/vhost.pp

Lines 2106 to 2112 in 337e8b0

$filename = $use_servername_for_filenames ? {

true => $use_port_for_filenames ? {

true => regsubst("${normalized_servername}-${port}", ' ', '_', 'G'),

false => regsubst($normalized_servername, ' ', '_', 'G'),

},

false => $name,

}

So right now it uses $name for $filename, but after it will use $normalized_servername which is a change. Maybe I should have done it otherwise and determined $v8_filename instead and compared that.

Sorry, as I see now the check between $name/$normalized_servername and between $servername/$normalized_servername are both bogus in that case.

While I see no problem at all to use a normalized-servername as the filename for the apache-configsnipped, I understand the reasoning in rolling that change in stages and issue warnings.

However, I really would like to have a possibility to "defuse" these warnings on a global level. While still messy, maybe we could lookup the $use_servername_for_filenames and $use_port_for_filenames from hiera? In that case it would be at least possible to enable both parameters on one place.

ekohl · 2021-06-22T14:56:51Z

manifests/vhost.pp

@@ -2122,7 +2122,7 @@
    module, the $use_servername_for_filenames will be removed and log/config file names will be derived from the
    sanitized $servername parameter when not explicitly defined.'
    warning($use_servername_for_filenames_warn_msg)
-  } elsif ! $use_port_for_filenames {
+  } elsif $use_servername_for_filenames and ! $use_port_for_filenames {


This doesn't look correct to me. I think the intention is to warn users ahead of time that they'll run into problems in the future.

I understand. However, I am not sure about the correct usage then. Most vhosts-definitions I have here are actually two definitions for the same servername (the non-ssl version, and the ssl-version).
As for now every puppet run fills the log with that warnings, and the only way to get rid of them is by enabling the $use_port_for_filenames parameter. However I am not really sure if it is a good idea to enable that parameter, since it is scheduled to be deprecated with the next releases.

What would be a proper way to declare two vhosts for the same servername without getting the warning?

Well, I think you have the same problem as I started with. Personally I'd like the whole deprecation not to happen and instead remain compatible as much as possible.

I think the best way for that is to only include the port if it's non-default. I tried to come up with a patch and I think #2161 might do that.

As already mentioned on #2161 that would not help if you need the same servername for a ssl and a non-ssl version. At least one filename has to be prefixed or suffixed with the protocol. But then using the portname seems to be much more appropriate.

elofu17 · 2021-09-23T18:35:42Z

I'm annoyed by the logged message, logged over and over, from every puppet node with apache, for each puppet run.
All nodes are configured as standard as possible, so I don't think I need to see the warnings all the time.

  class { 'apache':
  }
  apache::vhost { $::fqdn:
    port    => 80,
    docroot => "/var/www/${::fqdn}",
  }

This result in e.g.:

2021-09-23 18:41:07,692 WARN  [puppetserver] Scope(Apache::Vhost[foobar.infra.gazonk.com])
    It is possible for multiple virtual hosts to be configured using the same $servername but a different port. When
    using $use_servername_for_filenames, this can lead to duplicate resource declarations.
    When $use_port_for_filenames = true, the $servername and $port parameters, sanitized, are used to construct log and
    config file names.

    From version v7.0.0 of the puppetlabs-apache module, this parameter will default to true. From version v8.0.0 of the
    module, the $use_port_for_filenames will be removed and log/config file names will be derived from the
    sanitized $servername parameter when not explicitly defined.

With only one port, there's no risk for duplicate declarations.
With my standard code, I should see no warnings/instructions at all -- they just confuse me.

The current defaults in v6 should be enough.
The defaults in v7 and v8 should be also be enough.
That is,. I should not need to do any changes to my code during this transition, neither add
use_port_for_filenames = true nor remove it in v8.
Therefore I should not be spammed with annoying warnings/instructions.

zilchms · 2022-01-10T15:32:57Z

Any updates on this? Filling the puppet server log with these warnings, while the underlying "space in name" and deprecation problem is not present in the configured resources, seems a bit much. As i get from the discussion between @ekohl and @sircubbi there seems to be no "nice" way to fix this, is there? Can i help with anything there (would need some time to get into the code)?

github-actions · 2022-04-26T02:12:51Z

This PR has been marked as stale because it has been open for a while and has had no recent activity. If this PR is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days.

sircubbi added 2 commits June 22, 2021 16:39

compare servername (not ressource-name) with normalized-version

fb5d465

fixes intention of pull-request #2154

only warn about port not being used if servername is already used as …

6cfdfcc

…filename silences warnings to "real" cases, as it was intended with pull-request 2154

sircubbi requested a review from a team as a code owner June 22, 2021 14:49

ekohl reviewed Jun 22, 2021

View reviewed changes

chelnak added the community label Feb 24, 2022

github-actions bot added the stale label Apr 26, 2022

github-actions bot closed this May 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve/silence warning from pull-request #2154 #2160

Improve/silence warning from pull-request #2154 #2160

sircubbi commented Jun 22, 2021

CLAassistant commented Jun 22, 2021 •

edited

puppet-community-rangefinder bot commented Jun 22, 2021

ekohl left a comment

ekohl Jun 22, 2021

ekohl Jun 22, 2021

sircubbi Jun 22, 2021

ekohl Jun 22, 2021

sircubbi Jun 22, 2021 •

edited

ekohl Jun 22, 2021

sircubbi Jun 22, 2021

elofu17 commented Sep 23, 2021 •

edited

zilchms commented Jan 10, 2022

github-actions bot commented Apr 26, 2022

	$filename = $use_servername_for_filenames ? {
	true => $use_port_for_filenames ? {
	true => regsubst("${normalized_servername}-${port}", ' ', '_', 'G'),
	false => regsubst($normalized_servername, ' ', '_', 'G'),
	},
	false => $name,
	}

Improve/silence warning from pull-request #2154 #2160

Improve/silence warning from pull-request #2154 #2160

Conversation

sircubbi commented Jun 22, 2021

CLAassistant commented Jun 22, 2021 • edited

puppet-community-rangefinder bot commented Jun 22, 2021

apache::vhost is a type

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

ekohl left a comment

Choose a reason for hiding this comment

ekohl Jun 22, 2021

Choose a reason for hiding this comment

ekohl Jun 22, 2021

Choose a reason for hiding this comment

sircubbi Jun 22, 2021

Choose a reason for hiding this comment

ekohl Jun 22, 2021

Choose a reason for hiding this comment

sircubbi Jun 22, 2021 • edited

Choose a reason for hiding this comment

ekohl Jun 22, 2021

Choose a reason for hiding this comment

sircubbi Jun 22, 2021

Choose a reason for hiding this comment

elofu17 commented Sep 23, 2021 • edited

zilchms commented Jan 10, 2022

github-actions bot commented Apr 26, 2022

CLAassistant commented Jun 22, 2021 •

edited

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

sircubbi Jun 22, 2021 •

edited

elofu17 commented Sep 23, 2021 •

edited