Add support for new feature: port ranges #1325

00ssh · 2019-04-18T12:40:15Z

Pull Request (PR) description

Updated listen_port data type from Integer to Variant[String, Integer] in 'nginx::resource::server' and 'nginx::resource::streamhost' as newer versions of Nginx support port range at listen port.
Please check the release notes from 26th of March:

Changes with nginx 1.15.10                                       26 Mar 2019

    *) Change: when using a hostname in the "listen" directive nginx now
       creates listening sockets for all addresses the hostname resolves to
       (previously, only the first address was used).

    *) Feature: port ranges in the "listen" directive.
...

Updated listen_port data type from Integer to Variant[String, Integer] as newer versions of Nginx support port range at listen port.

Updated listen_port data type from Integer to Variant[String, Integer] as newer versions of Nginx support port range at listen port. Please check the release notes from 26th of March: - http://nginx.org/en/CHANGES - https://www.nginx.com/blog/nginx-plus-r18-released#port-ranges

puppet-community-rangefinder · 2019-04-18T12:40:28Z

nginx::resource::server is a type

The enclosing module is declared in 9 of 575 indexed public Puppetfiles.

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

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

4n0m4l0u5-configure_nginx
nginx::resource::streamhost is a type

The enclosing module is declared in 9 of 575 indexed public Puppetfiles.

that may have no external impact to Forge modules.

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 think it makes sense to introduce a Nginx::PortRange type since it's used in multiple places. Then it should also be simple to use a Pattern. Probably something like Pattern[/\d+-\d+] but you can easily write some tests that verifies it.

00ssh · 2019-05-29T13:30:05Z

Hi,
Can someone point to me, please, what is wrong with my travis tests? The error seems to be:
An error occurred while loading ./spec/acceptance/class_spec.rb. Failure/Error: install_module_dependencies_on(hosts) Beaker::Host::CommandFailure: Host 'debian9-64-1' exited with 1 running: puppet module install puppetlabs-concat -v 5.3.0 Last 10 lines of output were: Notice: Preparing to install into /etc/puppetlabs/code/environments/production/modules ... Notice: Downloading from https://forgeapi.puppet.com ... Error: Could not install module 'puppetlabs-concat' (???) No version of 'puppetlabs-concat' can satisfy all dependencies Use puppet module install --ignore-dependencies to install only this module

Rebase from master

00ssh · 2019-06-03T14:54:39Z

It might have been a forge issue. Now all checks have passed. Can you please take a look at my changes and let me know if it can be merged?

00ssh · 2019-06-04T14:48:16Z

@ekohl @dhoppe can you please give me feedback on this change? we are eager to have this implemented. Thanks!

dhoppe · 2019-06-05T05:44:43Z

I wonder if we should check the version of Nginx somewhere before configuring port ranges. We already did that for some templates, based on the fact nginx_version.

dhoppe · 2019-06-05T12:14:27Z

manifests/resource/server.pp

+      $ipv6_port = $ipv6_listen_port
+    }
+  }
+


@00ssh I think your code is to complicated and does not need to throw an error message. Maybe something like this could work:

# If port range is defined, ignore any other $listen_port defined if versioncmp($facts['nginx_version'], '1.15.10') < 0 { $port = $listen_port if $ipv6_enable == true { $ipv6_port = $ipv6_listen_port } } else { $port = pick_default($listen_port_range, $listen_port) if $ipv6_enable == true { $ipv6_port = pick_default($ipv6_listen_port_range, $ipv6_listen_port) } }

https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/REFERENCE.md#pick_default

@dhoppe Hi, Thanks for your suggestions. I've just tested with pick_default and it works just fine. However, I still have to figure out why the travis pipeline is failing (I don't have an environment connected to the internet right now). It looks like "fact('nginx_version')" doesn't return anything.

@00ssh Maybe, for that reason I used $facts['nginx_version'] at the example from above.

@dhoppe Thanks. I've updated accordingly. Waiting for tests to pass this time!

dhoppe · 2019-06-06T07:40:09Z

spec/defines/resource_server_spec.rb

@@ -125,6 +125,18 @@
              value: 45,
              match: %r{\s+listen\s+\[::\]:45 default ipv6only=on;}
            },
+            {
+              title: 'should set the IPv6 listen port',


You might want to change the title to 'should set the IPv6 listen port range'.

Updated. Thanks for pointing it out!

dhoppe · 2019-06-06T08:19:31Z

@ekohl I would like to merge this pull request. Any objections?

bastelfreak · 2019-06-10T10:46:46Z

manifests/resource/server.pp

+  if versioncmp($facts['nginx_version'], '1.15.10') < 0 {
+    $port = $listen_port
+
+    if $ipv6_enable == true {


You don't need to update the code, but the following would work as well and is shorter:

Suggested change

if $ipv6_enable == true {

if $ipv6_enable {

00ssh · 2019-06-20T14:18:21Z

Hi guys! Are there any other pending issues with this pull request? We would like to start using it from Forge as soon as possible.

Thanks!

00ssh · 2020-01-06T13:44:15Z

@ekohl any updates on my pull request, please?

ekohl

This still feels like it has a lot of edge cases where it might fail but I also don't have a lot of time nowadays to properly review.

ekohl · 2020-01-06T13:56:19Z

manifests/resource/server.pp

@@ -5,7 +5,8 @@
 # Parameters:
 #   [*ensure*]                     - Enables or disables the specified server (present|absent)
 #   [*listen_ip*]                  - Default IP Address for NGINX to listen with this server on. Defaults to all interfaces (*)
-#   [*listen_port*]                - Default IP Port for NGINX to listen with this server on. Defaults to TCP 80
+#   [*listen_port*]                - Default IP Port for NGINX to listen with this server on. Defaults to TCP 80. It can be a port or a port range (eg. '8081-8085').


The comment here is incorrect. It's forced to be an integer so it can't be a range. Can't we change listen_port to a type Variant[Stdlib::Port, Nginx::PortRange] and handle that in the code? I dislike having two parameters for the same thing.

ekohl · 2020-01-06T13:58:14Z

manifests/resource/server.pp

  # Try to error in the case where the user sets ssl_port == listen_port but
  # doesn't set ssl = true
-  if !$ssl and $ssl_port == $listen_port {
+  if !$ssl and $ssl_port == $port {


This can be an issue when port is an array. This repeats below.

ekohl · 2020-01-06T13:59:43Z

types/portrange.pp

@@ -0,0 +1 @@
+type Nginx::PortRange = Pattern[/^\d+-\d+?$/]


Why the question mark at the end? Could you add some tests for the type alias? See https://rspec-puppet.com/documentation/type_aliases/ as well.

ekohl · 2020-01-06T14:00:14Z

spec/defines/resource_server_spec.rb

@@ -446,13 +458,6 @@
              )
            end

-            context 'without a value for the nginx_version fact do' do


Why do you delete this test?

vox-pupuli-tasks · 2020-09-30T23:25:32Z

Dear @00ssh, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

00ssh added 2 commits April 18, 2019 15:33

Update streamhost.pp

b40a4a1

Updated listen_port data type from Integer to Variant[String, Integer] as newer versions of Nginx support port range at listen port.

ekohl requested changes Apr 23, 2019

View reviewed changes

00ssh added 15 commits May 29, 2019 14:47

Update server.pp

efc321e

Create portrange.pp

d61fb86

Updated variable name listen_port -> port

b727650

Updated variable name port -> ipv6_port

b416b8b

Changed variable name from listen_port -> port

9dbf0a6

added support for port range

be02ef8

Added listen_port_range to spec attributes

dd1afc4

Added listen_port_range and ipv6_port_range

513d75d

Added support for port ranges and ipv6 port ranges

d0df89c

fixed value type

fe09594

fixed value type

ec6a201

removed new line trailing spaces

e09f5b4

removed new line trailing spaces

366bc45

remove whitespace

dc8b14a

fixed missing declared resource variable

6a02424

00ssh added 3 commits May 29, 2019 21:48

Updated variable description

addd3c6

Merge pull request #1 from voxpupuli/master

d4021eb

Rebase from master

Update streamhost.pp

bcad22d

dhoppe added the enhancement New feature or request label Jun 5, 2019

00ssh added 3 commits June 5, 2019 11:33

Added nginx version checks for port range support

ffaa006

Added nginx version checks for port range support

d9292c6

Updated specs for port range using nginx_version fact

f36f469

Updated spec for stream for port range support with nginx_version fact

01692cf

dhoppe reviewed Jun 5, 2019

View reviewed changes

ay61su and others added 5 commits June 5, 2019 16:38

simplified code, updated spec, changed from fact function to $fact

558b32b

updated specs for nginx_version in server resource

8907ee4

updated specs for nginx_version in server resource

bf23d6b

Added nginx_version fact to streamhost spec

4045958

removed context "without a value for nginx_version fact"

f96006a

dhoppe reviewed Jun 6, 2019

View reviewed changes

updated specs with correct titles

0c7e28a

dhoppe requested a review from ekohl June 6, 2019 08:19

bastelfreak reviewed Jun 10, 2019

View reviewed changes

bastelfreak changed the title ~~Added support for new feature: port ranges~~ Add support for new feature: port ranges Jun 10, 2019

bastelfreak mentioned this pull request Jun 10, 2019

Check dependency constraints for puppet modules voxpupuli/vox-pupuli-tasks#9

Open

ekohl reviewed Jan 6, 2020

View reviewed changes

vox-pupuli-tasks bot added the merge-conflicts label Sep 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for new feature: port ranges #1325

Add support for new feature: port ranges #1325

00ssh commented Apr 18, 2019

puppet-community-rangefinder bot commented Apr 18, 2019

ekohl left a comment

00ssh commented May 29, 2019

00ssh commented Jun 3, 2019 •

edited

Loading

00ssh commented Jun 4, 2019

dhoppe commented Jun 5, 2019

dhoppe Jun 5, 2019 •

edited

Loading

00ssh Jun 5, 2019

dhoppe Jun 5, 2019

00ssh Jun 5, 2019

dhoppe Jun 6, 2019

00ssh Jun 6, 2019

dhoppe commented Jun 6, 2019

bastelfreak Jun 10, 2019

00ssh commented Jun 20, 2019

00ssh commented Jan 6, 2020

ekohl left a comment

ekohl Jan 6, 2020

ekohl Jan 6, 2020

ekohl Jan 6, 2020

ekohl Jan 6, 2020

vox-pupuli-tasks bot commented Sep 30, 2020

Add support for new feature: port ranges #1325

Are you sure you want to change the base?

Add support for new feature: port ranges #1325

Conversation

00ssh commented Apr 18, 2019

Pull Request (PR) description

puppet-community-rangefinder bot commented Apr 18, 2019

nginx::resource::server is a type

ekohl left a comment

Choose a reason for hiding this comment

00ssh commented May 29, 2019

00ssh commented Jun 3, 2019 • edited Loading

00ssh commented Jun 4, 2019

dhoppe commented Jun 5, 2019

dhoppe Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoppe commented Jun 6, 2019

Choose a reason for hiding this comment

00ssh commented Jun 20, 2019

00ssh commented Jan 6, 2020

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vox-pupuli-tasks bot commented Sep 30, 2020

00ssh commented Jun 3, 2019 •

edited

Loading

dhoppe Jun 5, 2019 •

edited

Loading