Add stream log support

[ardrigh]

Pull Request (PR) description

Following on work from previous PRs to add stream log changes to the module.

Updated changes to fix file conflicts and newer changes in v4.1.0

The configuration in Hiera:

nginx::stream_access_log: '/var/log/nginx/nginx-stream-access.log'
nginx::stream_custom_format_log: 'basic'
nginx::stream_log_format:
  basic: '$remote_addr [$time_local] $protocol $status $bytes_sent $bytes_received $session_time $server_addr:$server_port $upstream_addr'

Adds the following lines to nginx.conf:

+  log_format basic '$remote_addr [$time_local] $protocol $status $bytes_sent $bytes_received $session_time $server_addr:$server_port $upstream_addr';
 
+  access_log /var/log/nginx/nginx-stream-access.log basic;

If stream_custom_format_log is not set, nginx will use the built-in combined log format for the stream access log.

This Pull Request (PR) fixes the following issue

Closes: #1439

[puppet-community-rangefinder]

nginx::config is a class

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

• vshn-uhosting
• halyard-boxen

nginx is a class

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

• dhollinger-devopsdays
• eelcomaljaars-friendica
• kalfa-sites
• samuelson-custom_webapp
• akisakye-matomo
• othalla-nextcloud
• jonhallettuob-existdb
• puppet-hyperglass
• pgassmann-letsencrypt_nginx
• stackstorm-st2
• cristifalcas-kibana
• geoffwilliams-r_profile
• covata-safeshare
• ploperations-webhook_proxy
• landcareresearch-ckan

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

• jmkeyes-netbox
• klynton-testing
• lboynton-gitlab
• nicopaez-dockerserver
• pltraining-selfpaced
• jbussdieker-graphite_web
• dmexe-deploy
• firm1-zds
• aursu-grayloginstall
• 4n0m4l0u5-configure_nginx
• ramiel-owncloud
• danieldreier-puppet_installer
• qroac-isp3node
• adcade-statsd
• compass-learninglocker
• compass-examdb
• vormetriclabs-meteor
• chedi-django
• nicopaez-alfred
• nicopaez-triage
• tracywebtech-appdeploy
• puppetfinland-patchwork
• ploperations-puppet
• mightp-librenms
• vshn-uhosting
• qtechnologies-psgi
• maestrodev-maestro_nodes
• puppetfinland-freight
• oris-appserver
• SchnWalter-happydev
• groupbuddies-gb
• halyard-boxen
• mtulio-profiles
• braiins-wordpressnginx

nginx::params is a class

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

• braiins-wordpressnginx

This module is declared in 9 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.

[bastelfreak]

Please enforce the datatype here as well. I guess something like this should work:

Suggested change

	$stream_log_format = {},
	Hash[String[1], String[1]] $stream_log_format = {},

[bastelfreak]

can you test if the if statement is required? The variable always exists, by default it's just empty. I think we can directly start with the .sort_by call

[ekohl]

What could make sense is

<% end -%>
<% unless @stream_log_format.empty? -%>

<% @stream_log_format.sort_by{|k,v| k}.each do |key,value| -%> 

With this you can prevent the empty lines in the generated config. Because right now it generates (roughly):

  include .../streams-enabled/*;


  access_log ...

(note it's 2 empty lines instead of 1)

[ardrigh]

I am working on making updates to fix this PR. I need to check it works with what we have deployed.

I think the tests are failing because they are in the wrong location in the test file. I think the test lines might be better in a context for stream specifically.

Would it work adding a "dynamic module" test similar to geoip module, checking the stream content lines for the access_log and log_format values?

geoip test example:

puppet-nginx/spec/classes/nginx_spec.rb

Line 1162 in fffe7a3

context 'when dynamic_modules is "[\'ngx_http_geoip_module\']" ' do

[ardrigh]

Updated with suggested changes

[ekohl]

From a quick look this looks good, but the tests fail. The CentOS 7 tests failing looks unrelated to me, but the unit tests do.

[ardrigh]

I am working on making updates to fix this PR. I need to check it works with what we have deployed.

I think the tests are failing because they are in the wrong location in the test file. I think the test lines might be better in a context for stream specifically.

Still the same issue with the unit tests. Which isn't surprising since I did a quick copy & paste to get started.

I don't see anything that tests

<% if @stream -%>

And without that being set, it's not expected the lines get written to the config file, and the unit tests correctly fail.

These tests might not cleanly fit within the existing template iterator. I will have another look

[ardrigh]

@ekohl I have added a basic set of tests to cover the main use case for the stream access log.

Appreciate if you could give another review

[ekohl]

Thanks!