Remove log_driver limitations

[timdeluxe]

Current docker versions come with a bunch of new log drivers (see https://docs.docker.com/config/containers/logging/configure/#supported-logging-drivers), most importantly "local", which is not included in the validation of this module.
With this PR i removed the validation for log_driver completely, because one can also integrate other log drivers via plugins, which should be possible.

Fixes #791

[CLAassistant]

All committers have signed the CLA.

[puppet-community-rangefinder]

docker is a class

that may have no external impact to Forge modules.

This module is declared in 6 of 578 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.

[dploeger]

Can this be reviewed please?

[LukasAud]

Closing and reopening PR to rekick automated testing.

[puppet-community-rangefinder]

docker is a class

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

• martyewings-githubactionboard
• puppetlabs-data_entitlement
• simp-simp_docker
• tomkrieger-remediate_install
• samuelson-dockeragent
• maeq-storj
• gennadyvoronkov-dcos
• autostructure-docker_ee
• fervid-docker_ee
• ffquintella-dockerapp
• glorpen-g_docker
• knoppi-jitsi
• dodevops-hanaexpress
• walkamongus-harbor
• mindhive-nvidia_docker_runtime
• puppetlabs-cd4pe
• maany-simple_grid

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

• tsuru-registry
• tsuru-redis_service
• pltraining-selfpaced
• tsuru-router
• c0deaddict-gogs
• flyer-docker_cluster
• maasch-iot_provisioning
• jantman-archlinux_workstation
• nick-rancher
• scottyc-docker_swarm
• justin8-ghost
• autostructure-secure_docker
• jefferyb-kualicoeus
• pltraining-classroom_legacy
• cristifalcas-flannel
• pltraining-classroom
• puppet-gitlab_ci_runner
• Azcender-profile
• pltraining-dockeragent
• praekeltfoundation-seed_stack
• pltraining-puppetfactory
• prozeta-portauthority
• hesco-weave
• vshn-gitlab

This module is declared in 6 of 579 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.

[chelnak]

@timdeluxe Thanks for this suggestion. I'm not 100% sure removing the validation is the right way to go with this.

I wonder if a better approach would be to validate against the log drivers that are currently available?

For example, the following command will return the log drivers:

docker system info --format '{{json .Plugins.Log}}'

["awslogs","fluentd","gcplogs","gelf","journald","json-file","local","logentries","splunk","syslog"]

One could parse the list returned and validate the user-input against it.

[timdeluxe]

@chelnak Hm, yeah, that is an option. It is a bit more complicated as i feel not so familiar with structured ruby facts, which would be required here in my opinion.
But more importantly i see a chicken-egg problem here: Docker would need to be installed to gather the list of valid log drivers to validate to. So when Docker is not installed yet, it would need to allow every log driver and on the next puppet run it would fail (given a not valid log_driver was configured), because then it notices that the given log_driver is not existing.
What do you think about that?

[chelnak]

hmmm, that is an interesting analysis. I can certainly see that happening in the case where an invalid driver is passed. The question is, could we move that validation elsewhere so that it would fail on the first run...

[timdeluxe]

The question is, could we move that validation elsewhere so that it would fail on the first run...

Hit me with ideas!

How about validating it, but just issue a warning instead of a fail, if it does not fit? This way it still would be validated, but it wouldn't fail on the first run. Not perfect but would be at least a compromise...

[timdeluxe]

One additional idea:
On the first run, when command you wrote above can't/doesn't return anything, because docker is not yet installed, it could validate against a hardcoded list, gathered from the current docker version and issue a warning if it doesn't fit. Then (on the next run) when the command returns something validate against that and fail, if it doesn't fit.
This way there would be no error/warning at all on a valid log_driver and a warn and then a fail on a non valid log_driver.

[chelnak]

I think that would actually be a good non destructive option. However if it's incorrect, we should warn and fall back to a known default..

edit: I missed out a crucial word there!

[chelnak]

Then (on the next run) when the command returns something validate against that and fail, if it doesn't fit.

Yeah this is certainly another way to approach it. However I'm not a massive fan of things taking more than one puppet run to apply unless we have no choice.

Warning and falling back to a known default seems more reasonable here to me.

What do you think?

[chelnak]

Hey @timdeluxe , did you get chance to think over my suggestion?

[timdeluxe]

@chelnak sorry for missing feedback, was busy with some actions around my own (round) birthday, please apologize.

I am sorry, but i don't get you at...

Warning and falling back to a known default seems more reasonable here to me.

...what do you mean with "falling back to a known default"? Do you mean the validation should just been done to a known set of log_drivers like done in the current code but the set just will extend with f.e. the currently missing "local" one?
Or do you mean that the log_driver setting itself should be changed by the code if it does not validate against a hardcoded list?
Well, i also don't like the two puppet runs issue, but we have a really crazy case here. When thinking about "known defaults" please always remember, that one may want to use own logging drivers via (self developed maybe) plugins - no "known default" would be ever able to cover this and if a warning would be issued in this case, one has to live with a warning forever - i also do not like that.

In my opinion the only alternatives are:
a) remove the validation at all, like done originally in this PR
b) do as i proposed with warning against a known list and a fail, if the command for the validation gives needful results on later puppet runs

But maybe i did not understand you at some point - in that case, please explain in more detail. Thanks!

[chelnak]

Hey - no problem. 😄

I'm actually leaning towards removing the validation right now. There doesn't seem to be a sensible trade off between value and implementation. I've raised this internally with the team to see what they say.. also if any other community members see this, please feel free to share your opinions.

[timdeluxe]

FYI: Saw that needs-rebase label by david - will do that, once the decision chelnak descibed is made.

[timdeluxe]

@chelnak Any update here?

[chelnak]

Hey @timdeluxe

It's community day today - i'll bring it up in our triage meeting 😄

[chelnak]

Hey @timdeluxe,

Are you able to rebase?

Talked this through and I think this is the right way to go at this moment in time.

I think there might be value in adding something to the README or parameter description to explain that the log driver names must be verified prior to be included.

[timdeluxe]

Hey @timdeluxe,

Are you able to rebase?

Talked this through and I think this is the right way to go at this moment in time.

I think there might be value in adding something to the README or parameter description to explain that the log driver names must be verified prior to be included.

@chelnak Sorry, for the delay. Yes, i am able to rebase and i can include the parameter description (i think that is enough). Will try to do that tomorrow morning (meaning european morning) and update the PR here then.

[chelnak]

@timdeluxe Thanks for getting this updated.

The test failures are not related to this PR so I am happy to get this merged.

[chelnak]

LGTM - thank you!