Skip to content
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

Bug in ipv6 template #30

Closed
guzmanbraso opened this issue Oct 3, 2012 · 5 comments
Closed

Bug in ipv6 template #30

guzmanbraso opened this issue Oct 3, 2012 · 5 comments

Comments

@guzmanbraso
Copy link
Contributor

Hi James,

I'm working on a pull where I fix a missing $listen_port on templates and introduce a new $listen_options to be able to set directives like 'default' from resource call.

However, when checking ipv6 header it has a fixed default in the listen directive, this means that if you setup more than one vhost with the same listen ip in ipv6 you will have a broken config.

It's very easy to fix, but it's impossible to be backwards compatible on this. Except.... setting on IPV6 new $ipv6_listen_option = 'default', but if we go this way to be backwards compatible, we should warn users to always set listen_options to empty values in their arguments if working with more than one vhost on the same ipv6.

What do you suggest?

@jfryman
Copy link
Contributor

jfryman commented Oct 4, 2012

Can you give me an example of this usage in how you intend to use it? There might be another way to model this in code.

@guzmanbraso
Copy link
Contributor Author

Hi!

Do you mean the bug on ipv6 or the listen_options feature ? I'll explain both just in case.

  • Bug in IPV6

Here I don't think there is any option, the template had the 'default' option for listen directive hardcoded, which forces nginx to believe each virtual host is the default for a request that does not match any other virtual host.

Suppose you have the IPV6 "2001:0db8:85a3:0042:0000:8a2e:0370:7334" and three domains on that IP, then you create four virtual hosts, one for each domain and one to capture the traffic that does not match any virtualhost.

Because template have 'default' option for listen directive hardcoded, every one of the virtualhost will have a listen like this:
listen [2001:0db8:85a3:0042:0000:8a2e:0370:7334]:80 default ipv6only=on
listen [2001:0db8:85a3:0042:0000:8a2e:0370:7334]:80 default ipv6only=on
listen [2001:0db8:85a3:0042:0000:8a2e:0370:7334]:80 default ipv6only=on
listen [2001:0db8:85a3:0042:0000:8a2e:0370:7334]:80 default ipv6only=on

This will cause nginx to decide which of the sites will be the default based on the order it read the files. Can't remember right now if it choose the first or the last one but it won't do what user wants. 'default' option should be set only on 1 listen per IP.

  • New features listen_options and ipv6_listen_options

The need for this is because nginx changed a lot in recent versions and added tens of options to listen directive for fine tunning of servers, in fact in latest version it's non longer 'default' but 'defaut_server', but many many more were added. From nginx manual, options for listen directive: [default_server] [setfib=number] [backlog=number] [rcvbuf=size] [sndbuf=size] [accept_filter=filter] [deferred] [bind] [ipv6only=on|off] [ssl] [so_keepalive=on|off|[keepidle].

I see only three options here:

  1. Add one argument per listen option to the vhost resource.
  2. Add a hash with listen options, one key per option and with a nice default value
  3. Add a variable listen_options where listen options directive can be set as string.

From those three I discarded the first one because it will add a lot of options that likely most users won't ever set or need. Then discarded the second because most options are only for recent versions and it would need a lot of work to properly set default values according to which nginx version is running. At the end only the third was left and choosed it due to lack of options and simplicity.

There is a fourth?

Cheers!

Guzman

@3flex
Copy link
Contributor

3flex commented Nov 22, 2013

@guzmanbraso I know this is old, but this isn't an issue anymore (you can set listen_options per vhost - it defaults to default but is not hard coded):

Would you consider closing?

@3flex
Copy link
Contributor

3flex commented Nov 22, 2013

@jfryman while you're checking into issues, this can be closed. There are a couple of others that can be closed too, I'll ping you on those as well.

@jfryman jfryman closed this as completed Nov 22, 2013
@fadenb
Copy link

fadenb commented Jan 27, 2014

solved since merge of #249

nginx: [emerg] duplicate listen options for [::]:80 in /etc/nginx/sites-enabled/eol.CHANGED.com.conf:4

config generated by current code:

root@webserver:/etc/nginx/sites-enabled# grep -ir listen *
REMOVED.com.conf:  listen                192.168.122.153:80;
REMOVED.com.conf:  listen [::]:80  ipv6only=on;
eol.CHANGED.com.conf:  listen                192.168.122.153:80;
eol.CHANGED.com.conf:  listen [::]:80  ipv6only=on;
SOMETHING.COMPANY.de.conf:  listen                192.168.122.153:80;
SOMETHING.COMPANY.de.conf:  listen [::]:80  ipv6only=on;

This is not caused by a duplicate default listen option but multiple ipv6only=on options. Those are currently hardcoded in https://github.com/jfryman/puppet-nginx/blob/master/templates/vhost/vhost_ssl_header.erb#L4 and https://github.com/jfryman/puppet-nginx/blob/v0.0.6/templates/vhost/vhost_header.erb#L5 .

Would it be reasonable to use the ipv6_listen_options parameter with a default of default ipv6only=on and remove the hardcoded part? If so I will happily create a patch for templates and if required tests.

fadenb referenced this issue in fadenb/puppet-nginx Jan 27, 2014
fadenb referenced this issue in fadenb/puppet-nginx Feb 3, 2014
grooverdan referenced this issue in openquery/puppet-nginx Mar 4, 2014
dcarley added a commit to dcarley/myvps-puppet that referenced this issue Jun 18, 2014
Turn off `default` on all vhosts but the default one. Disable `ip6only` for
all vhosts, because it's now the default:

http://trac.nginx.org/nginx/ticket/345
voxpupuli/puppet-nginx#30
anarcat added a commit to anarcat/puppet-nginx that referenced this issue Aug 30, 2022
This was refering to issue voxpupuli#30 which was closed by changing the
behavior for *not* adding the default in the template. Instead, the
default includes the setting. We do not mention the default here
because it's visible in the generated `REFERENCE.md` file and is not
mentioned in other parameter documentation anyways.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants