Fixing Streamhost implementation support

[alwiesner]

There were issues with the streamhosts implementation for TCP load balancing. Changes made to support successful implementation of a stream host within NGINX configurations:

1. added "include <%= @conf_dir %>/streams-enabled/.conf;" to stream {} section of nginx.conf.erb. In order to enable nginx to listen on the defined TCP port, the stream{} configuration needs to include the /etc/nginx/streams-enabled/.conf files.

2. Removed 'server_name' from streamhost.erb template. The 'server_name' parameter is not valid for a stream host implementation. Removed line 20: server_name <%= @server_name.join(" ") %>;

3. nginx::resource::streamhost.pp does not ensure that the /etc/nginx/streams-enabled and /etc/nginx/streams-available directories exist before creating the stream host files. Added a file resource to ensure these directories are created, and adjusted the file creation resource to require this resource when creating the files.

[alwiesner]

I believe this build failure is expected, since the server_name parameter was removed from streamhosts.erb, as it is an looks to be an invalid parameter for a tcp load balancing implementation with the stream directive.

Example error from nginx when attempting to start when a conf.stream.d conf file has a server_name parameter defined:

nginx: [emerg] "server_name" directive is not allowed here in /etc/nginx/streams-enabled/sdk.conf:4
nginx: configuration file /etc/nginx/nginx.conf test failed

[astrostl]

Can this get a merge? Was about to develop and submit a similar fix!

[jfryman]

@astrostl If you're willing to pick up the PR and fix up the tests, we can absolutely get this merged in.

There might be a bad test. We should fix that, not merge in code that isn't passing.

[astrostl]

Gotcha. Are passing PRs usually quick-merged?

[3flex]

Just some feedback on this PR - there seems to be several changes unrelated to the original intent of fixing the streamhost support. These should be removed from this PR and added to a new PR as it changes several long-standing behaviours of the module. Thanks!

[bastelfreak]

HI @alwiesner, we recently merged #790 which a bit of the functionality from this PR. Please have a look and rebase.

[wyardley]

#879 did some of this; I also played around with adding some more tests and doing this in a slightly different way in #878, however, as issue #780 points out, it doesn't look as if the streams related directories currently get included.

I got rid of the '$stream' variable in #878, though maybe it should be reintroduced if it's actually going to be implemented as expected (I'd think it should be sufficient to just include the relevant directories, but maybe some other things could drop stuff in there, and that's why there was a $stream?

[wyardley]

@astrostl: are you still interested in working on stream support? As of now, there are some additional changes which should fix some of the problems mentioned here (see above), but not all. However, there have been enough changes since then that I'm not sure if it's easier to start over, or rework this one.

[astrostl]

I'm inactive on that feature and active on other things at work, so I'd pass for now.

[wyardley]

@astrostl, no worries, I've got the same thing going on a lot of the time.
@alwiesner: I think I intended that for you anyway.

For the folks who understand Nginx stream support, do you think there's any reason to have the $stream option that I got rid of recently [edit: guessing this is because it's a module that Nginx is not always built with, so could cause problems if related parameters got included when the module isn't compiled in or loaded as a dynamic module? https://nginx.org/en/docs/stream/ngx_stream_core_module.html)?

Or should we just include the streams directory in /etc/nginx/conf.d always, and have streams exist if someone creates the appropriate resource? To me, having the parameter doesn't seem to add much (if you don't have the module, don't define the resource), but open to suggestions / discussion on that.

Removing server_names from the template seems like something that can, and should be done easily and probably without any tests.

[wyardley]

Replacing with #933
Would love if anyone has time to test 933 + current master, but I think this hits most of the major points.