Setting environment via template considered harmful

[Ganneff]

Hi,

the module sets the environment to upload the facts to within the variable $fact_upload_params, see

log4jscanner/manifests/init.pp

Line 76 in 53d702d

$fact_upload_params = "facts upload --environment ${environment}"

While this is simple, it has an unfortunate (and annoying) side effect. Due to the notify later on:

log4jscanner/manifests/init.pp

Line 222 in 53d702d

notify => Exec[$generate_scan_data_exec],

It means that a simple environment change will trigger a full log4jscanner run, as puppet will rewrite the script. Which can take ages and is wasteful.
If one is using environment based setups, and switches often between them, testing a single change in ones puppet can mean a ten minute run of log4jscanner.

Thats less than optimal.

I suggest adjusting to either use a
puppet config print
or, better,
awk -F ': ' '/^environment:/ {print $2}' $(puppet config print lastrunreport)
to get at the value.

The latter would cover setups, where people do not use puppets configfile to set the actual environment a machine runs in, by just taking what the last puppet run used.

[binford2k]

There are a couple of problems with this approach, the most obvious being that it won't work on Windows nodes, making the behavior inconsistent across platforms. If we ported the script to a language that can parse the report directly, perhaps that would work.

I'm more interested in hearing your use case though. What's the need for many environment changes?

[Ganneff]

The use for quickly changing is not disturbing the other machines, while some change gets tested running on a selected number of machines first.
I use branches in git, both for long living branches and seperation, as well as the above, having r10k prepare me the trees on the puppet masters.

Works nicely.

Quickly testing something before having it run on all nodes is good.

Windows: Yah, well, I'm not burdened with that +#+#+# thing here, thats why I didn't touch it's code.

[binford2k]

Essentially you're saying that you drop a selection of your nodes into a testing environment before promoting changes to production. 👍

Can you remove the log4jscanner classification from your testing environment? Doing this won't remove the job, so reports will still be submitted. It just won't actively manage them. (to remove it, ensure => absent). Would that work?

[Ganneff]

Essentially you're saying that you drop a selection of your nodes into a testing environment before promoting changes to production. +1

Yup.

Can you remove the log4jscanner classification from your testing environment? Doing this won't remove the job, so reports will still be submitted. It just won't actively manage them. (to remove it, ensure => absent). Would that work?

Seems suboptimal. Would need a change so that the hiera classes lookup only gets log4jscanner back on production. You are right, this will not remove anything there, but it has the side effect of NOT installing it for machines that start their life in one such environment, until they finally "move" over to production. That does happen often enough (its the norm for any machine for a new functionality), that I prefer not going this route.

Well. If you do not want this change, close the issue and the PR - I'm just going to maintain it locally for me then. Our setup allows us to have local branches for modules with changes that aren't going upstream, so if you don't take it, it's not a problem for me. Just thought others may be annoyed by the behaviour too.

[binford2k]

But the original problematic behavior only affects you when changing environments. So you'd see it once on provision, then once when you promoted to prod. Unless you change between a bunch of staging environments, of course.

What if instead we added the option to skip the initial scan? That way changing the environment would prompt the scan script to be updated, but would skip the scan itself.

[Ganneff]

Yep, sounds good. This only adds a few lines of output in the env change run, not an ages long run of a tool.
And can be defaulted to true in hiera easy enough, and false in prod.