Fix default IPv6 listen_options clash - #1703
Conversation
When not explicicly set, `$ipv6_listen_options` defaults to `$listen_options` with `"ipv6only=on"` appended to it. The nginx documentation gives two important pieces of information regarding the `ipv6only` parameter: 1. "This parameter is turned on by default"; 2. "It can only be set once on start". Source: https://nginx.org/en/docs/http/ngx_http_core_module.html#listen Because we explicitly set this parameter on each Virtual Host, this leads to broken configuration if multiple virtual hosts are configured. While one could previously explicitly set `ipv6_listen_options => ''` to avoid this, the data type was changed in 4ad7bc0e4and an empty String is not allowed anymore, making the module unusable in such a situation. This commit adjust the module to not add `ipv6only=on` by default anymore, relying on the default behavior of nginx to achieve the same result. It also allows to pass an explicit empty String as `$ipv6_listen_options` to avoid the default value which match `$listen_options`. Last, it adjust the templates to avoid unexpeted spaces in the generated config.
|
Looks like a breaking change 🤔 |
I really consider this a bugfix: the removed parameter was set to its default value, but that value can only be set a single time for a host/port in the configuration. It was bad (and has been bad for a long time), but a workaround was possible until recently, and we cannot use this workaround anymore. So a proper fix was required 😁 But maybe I am missing something, and I don't mind to do a major release after merging this if it makes sense. Can you elaborate about what makes you think this should be considered a breaking change @jay7x ? |
|
I'm looking at changes in test files and that looks like behaviour is changed. I had no look at the rest of the code around though.. does it affect existing setups with default settings? |
Yes, it fix them* 😉 When updating to the latest release, I had to remove the workaround As the removed config is the default one, and cannot be repeated, it feels better to me to not explicitly add it: if the users need to set it to a specific value, they will set it to that value. *: I do have a bunch of nginx servers with really basic configuration, in reality I believe that this change is very safe, but my use case are quite simple and for this reason I cannot be 100% affirmative that it will not cause any kind of trouble with fancy configurations. |
|
I see.. actually, existing installations was my main concern. Read it as "shall we release this change as major or minor release" :) |
3 participants
When not explicicly set,
$ipv6_listen_optionsdefaults to$listen_optionswith"ipv6only=on"appended to it.The nginx documentation gives two important pieces of information regarding the
ipv6onlyparameter:Source: https://nginx.org/en/docs/http/ngx_http_core_module.html#listen
Because we explicitly set this parameter on each Virtual Host, this leads to broken configuration if multiple virtual hosts are configured.
While one could previously explicitly set
ipv6_listen_options => ''to avoid this, the data type was changed in 4ad7bc0 an empty String is not allowed anymore, making the module unusable in such a situation.This commit adjust the module to not add
ipv6only=onby default anymore, relying on the default behavior of nginx to achieve the same result.It also allows to pass an explicit empty String as
$ipv6_listen_optionsto avoid the default value which match$listen_options.Last, it adjust the templates to avoid unexpeted spaces in the generated config.
Fixes: #1293
Fixes: #1700
Overcomes: #1701