-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
(MODULES-8731) Allow CIDRs for proxy_ips/internal_proxy in remoteip #1891
Conversation
The recent addition of data types for this module [1] introduced an issue, where CIDRs are no longer allowed. This allows those sort of values. [1] puppetlabs@1503035#diff-c2ea3c67760696a0d67bab9fb81757c6
The template and module data types for the internal_proxy parameter didn't match. This makes it so.
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.
LGTM
Looks good to me. Thanks! |
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.
There is one extra space between $internal_proxy
, $proxy_ips
and them types :)
@iglov could you comment on the code? I'm not sure what space you're referring to. |
Ah, that was hard to see. Yes, |
I was using two characters while one was enough.
Optional[Stdlib::Absolutepath] $trusted_proxy_list = undef, | ||
Optional[String] $apache_version = undef, | ||
String $header = 'X-Forwarded-For', | ||
Optional[Array[Variant[Stdlib::Host,Stdlib::IP::Address]]] $internal_proxy = undef, |
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.
Would Variant[Stdlib::Host,Stdlib::IP::Address::V4::CIDR,Stdlib::IP::Address::V6::CIDR]
better show intent?
Stdlib::Host
already allows IP addresses (oddly via Stdlib::Compat::Ip_address
instead of Stdlib::IP::Address::Nosubnet
). The overlap doesn't really matter, and someone should probably submit a Stdlib::IP::Address::CIDR
type.
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.
Would you prefer that I change this to your suggestion? Or should we leave this as-is?
Given the current limitations of the Stdlib types, I think this is a perfectly acceptable fix in this module. |
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.
Now looks nice, thank u 👍
Wowsa, that a lot of teamwork. Great work to all involved. 👏 |
…uppetlabs#1891) * (MODULES-8731) Allow CIDRs for proxy_ips/internal_proxy in remoteip The recent addition of data types for this module [1] introduced an issue, where CIDRs are no longer allowed. This allows those sort of values. [1] puppetlabs@1503035#diff-c2ea3c67760696a0d67bab9fb81757c6 * Make internal_proxy data type in template match module The template and module data types for the internal_proxy parameter didn't match. This makes it so.
This is the same fix as in puppetlabs#1891
…uppetlabs#1891) * (MODULES-8731) Allow CIDRs for proxy_ips/internal_proxy in remoteip The recent addition of data types for this module [1] introduced an issue, where CIDRs are no longer allowed. This allows those sort of values. [1] puppetlabs@1503035#diff-c2ea3c67760696a0d67bab9fb81757c6 * Make internal_proxy data type in template match module The template and module data types for the internal_proxy parameter didn't match. This makes it so.
This is the same fix as in puppetlabs#1891
This is the same fix as in puppetlabs#1891
The recent addition of data types for this module [1] introduced an
issue, where CIDRs are no longer allowed. This allows those sort of
values.
This addresses https://tickets.puppetlabs.com/browse/MODULES-8731
[1] 1503035#diff-c2ea3c67760696a0d67bab9fb81757c6