-
-
Notifications
You must be signed in to change notification settings - Fork 882
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for new feature: port ranges #1325
base: master
Are you sure you want to change the base?
Conversation
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
nginx::resource::server is a typeThe 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): 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Hi, |
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? |
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 |
$ipv6_port = $ipv6_listen_port | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@00ssh Maybe, for that reason I used $facts['nginx_version']
at the example from above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhoppe Thanks. I've updated accordingly. Waiting for tests to pass this time!
spec/defines/resource_server_spec.rb
Outdated
@@ -125,6 +125,18 @@ | |||
value: 45, | |||
match: %r{\s+listen\s+\[::\]:45 default ipv6only=on;} | |||
}, | |||
{ | |||
title: 'should set the IPv6 listen port', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to change the title to 'should set the IPv6 listen port range'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated. Thanks for pointing it out!
@ekohl I would like to merge this pull request. Any objections? |
if versioncmp($facts['nginx_version'], '1.15.10') < 0 { | ||
$port = $listen_port | ||
|
||
if $ipv6_enable == true { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to update the code, but the following would work as well and is shorter:
if $ipv6_enable == true { | |
if $ipv6_enable { |
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! |
@ekohl any updates on my pull request, please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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'). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be an issue when port is an array. This repeats below.
@@ -0,0 +1 @@ | |||
type Nginx::PortRange = Pattern[/^\d+-\d+?$/] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -446,13 +458,6 @@ | |||
) | |||
end | |||
|
|||
context 'without a value for the nginx_version fact do' do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you delete this test?
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 |
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: