Warn if $ssl=false but $ssl_port == $listen_port (#1015) #1022
Conversation
manifests/resource/server.pp
Outdated
| # Try to error in the case where the user sets ssl_port == listen_port but | ||
| # doesn't set ssl = true | ||
| if (!($ssl == true) and ($ssl_port == $listen_port)) { | ||
| fail('nginx: ssl must be true if listen_port is the same as ssl_port') |
There was a problem hiding this comment.
I'd support a warning here, but a failure seems over the top.
There was a problem hiding this comment.
Yeah, I agree. It's now a warning.
There was a problem hiding this comment.
The failure is still here :/
|
Is this the correct fix? Setting $ssl_port to $listen_port and $ssl = true sounds like valid configuration, so perhaps the module behaviour should be fixed instead? |
|
I think this might be at least tangentially related to #1029 as well? |
|
@oranenj I am not sure if this should fail either, but, just to be clear, it's !($ssl = true), not $ssl = true. |
|
I updated it to just warn, and rebased. Still need to look into what the rspec syntax is to test for a warning, and fix the test |
wyardley
changed the title
|
maybe @oranenj can review this again? |
|
Do folks think this is a reasonable way to handle the inability to test warnings in this context for now? Basically, I left the test there as a pending test, and the message string is still in there for reference. |
|
@wyardley: I have no idea how to test for warnings, unfortunately. All I know of rspec is what I've seen in other specs. Anyway, the code I see still has a fail() call. It might also be possible to enforce $ssl = true (maybe with a warning) if $ssl_port is set. Would that be surprising behaviour? |
manifests/resource/server.pp
Outdated
| # Try to error in the case where the user sets ssl_port == listen_port but | ||
| # doesn't set ssl = true | ||
| if (!($ssl == true) and ($ssl_port == $listen_port)) { | ||
| fail('nginx: ssl must be true if listen_port is the same as ssl_port') |
There was a problem hiding this comment.
The failure is still here :/
|
@oranenj D'oh, sorry about that. Updated. Take a look. It might also be possible to enforce $ssl = true (maybe with a warning) if $ssl_port is set. Would that be surprising behaviour? I think there are valid use cases for this; it's only when
Edit: I see what you're saying, warn if |
|
Yeah, these changes look fine to me. |
alexjfisher
changed the title
warn if $ssl=false but $ssl_port == $listen_port (voxpupuli#1015)
warn if $ssl=false but $ssl_port == $listen_port (voxpupuli#1015)
See #1015
I would appreciate review on this... I'm not totally sure why these tests exist, or why there appear to be 2 sets of identical tests there:
https://github.com/voxpupuli/puppet-nginx/blob/master/spec/defines/resource_server_spec.rb#L1019-L1057
vs, say, https://github.com/voxpupuli/puppet-nginx/blob/master/spec/defines/resource_server_spec.rb#L1071-L1086
or what the use case is for having
ssl_port==listen_portwhensslis _not_true, but I think this PR might make some sense.