Merge lp:~cameronnemo/upstart/ipv6 into lp:upstart

[Cameron Norman]

Adds the inet6 option for the socket event parameter PROTO and the corresponding man page examples.

[lp:~cameronnemo/upstart/ipv6]

1595. By Cameron Norman on 2014-02-04

remerge from trunk

1596. By Cameron Norman on 2014-02-04

whoopsie in ChangeLog

[James Hunt (jamesodhunt)]

Hi Cameron,

Thanks to you and Kai for undertaking this new feature!

A few comments:

* extra/upstart-socket-bridge.c:
  - epoll_watcher():
    - 'break' has been lost from end of the AF_INET block.
    - buffer should not use nih_local (as that is only for allocated storage) and should really be at the
      top of the 'for' block.
    - AF_INET6 block missing a 'break' at the end.
  - job_add_socket(): '! strcmp (*env, "ADDR")' needs to be updated for IPv6 (using inet_pton() rather than
    inet_aton() maybe?).

I tried testing this using the new IPv6 example from socket-event.7, but I think the last comment above is causing the job to be rejected.

review: Needs Fixing

[lp:~cameronnemo/upstart/ipv6]

1597. By Cameron Norman on 2014-02-05

upstart-socket-bridge.c: epoll_watcher() and job_add_socket() fixes for ipv6

1598. By Cameron Norman on 2014-02-05

upstart-socket-bridge.c: job_add_socket(): pton returns 1 on success, not 0

[James Hunt (jamesodhunt)]

Hi Cameron,

Thanks for updating. There is still a problem with inet_pton(): the test should error if the value returned != 1 since currently all valid IPv6 addresses are rejected. Please could you fix that up and test.

I'm testing this with a job that specifies:

start on socket PROTO=inet6 PORT=80 ADDR=::1
script
echo ok
set
end script

Then running the event monitor:

$ upstart-monitor &

Then, running the bridge in debug mode in the foreground:

$ sudo ./upstart-socket-bridge --debug

And finally connecting to port 80 via IPv6:

$ echo foo | socat 'tcp6-connect:[::1]:80' -

Thanks!

review: Needs Fixing

[lp:~cameronnemo/upstart/ipv6]

1599. By Cameron Norman on 2014-02-06

fixed ptons usage

[Cameron Norman (cameronnemo)]

Thanks for the help and patience James. I am going to try to work on this tonight and tommorow, and will comment here if I reach success, or if I need any help (if that is alright).

[James Hunt (jamesodhunt)]

Hi Cameron - no problem. Feel free to join #upstart on freenode if you have questions.

[Cameron Norman (cameronnemo)]

So I tested it on a Saucy machine and it seems to be working. Would you like to re-review it?

[James Hunt (jamesodhunt)]

Hi Cameron,

Thanks for updating. I've found another issue though: if there are 2 jobs that specify 'start on socket', one for IPv4 the other for IPv6, the bridge will be unable to call bind(2) for each protocol version currently:

$ cat <<EOT | sudo tee /etc/init/test-socket-bridge-ipv4.confj
start on socket PROTO=inet PORT=80 ADDR=127.0.0.1
exec echo ipv4
EOT

$ cat <<EOT | sudo tee /etc/init/test-socket-bridge-ipv6.conf
start on socket PROTO=inet6 PORT=80 ADDR=::1
exec echo ipv6
EOT

$ sudo extra/upstart-socket-bridge --debug
job_add_socket: Found socket
upstart_job_added: Job got added /com/ubuntu/Upstart/jobs/test_2dsocket_2dbridge_2dipv6
job_add_socket: Found socket
upstart-socket-bridge: Failed to bind socket in /com/ubuntu/Upstart/jobs/test_2dsocket_2dbridge_2dipv4: Address already in use

I think we may need some IPV6_V6ONLY setsockopt(2) magic to resolve this?

review: Needs Fixing

[Cameron Norman (cameronnemo)]

I have a question about that. Should we not instead do IPv4 mapped IPv6?

[Cameron Norman (cameronnemo)]

But you would still have to use IPV6_V6ONLY in that scenario. And even then with IPV4 mapped IPv6 you would need to give the job two fd's, which we cannot do with the current socket activation protocol (correct?).

[lp:~cameronnemo/upstart/ipv6]

1600. By Cameron Norman on 2014-02-11

use setsockopt to only bind to ipv6

1601. By Cameron Norman on 2014-02-11

whoops

1602. By Cameron Norman on 2014-02-11

put the setsockopt /after/ the sockfd is created

[Cameron Norman (cameronnemo)]

Ok, so I got it working and tested it with the following job:

start on socket PROTO=inet6 PORT=80 ADDR=::1

exec socat fd:"$UPSTART_FDS" -

[James Hunt (jamesodhunt)]

Hi Cameron,

Yes, this does seem to be working, but if you run 'sudo upstart-socket-bridge --debug' with the 2 test jobs above, an error is raised on the 2nd bind:

$ sudo extra/upstart-socket-bridge --debug
job_add_socket: Found socket
upstart_job_added: Job got added /com/ubuntu/Upstart/jobs/test_2dsocket_2dbridge_2dipv6
job_add_socket: Found socket
upstart-socket-bridge: Failed to bind socket in /com/ubuntu/Upstart/jobs/test_2dsocket_2dbridge_2dipv4: Address already in use

[Cameron Norman (cameronnemo)]

That is strange, because I am not seeing the failure. Are you sure you are using r1602?

[Cameron Norman (cameronnemo)]

Wait, I think I may understand why. Is the system upstart-socket-bridge creating the socket for test-socket-bridge-ipv4.conf at boot? That would explain why the ipv6 one is not binding: the socket bridge you run at boot can not bind to the ipv6 socket. Other than that, I am at a loss as to why it works here, but not there.

[Cameron Norman (cameronnemo)]

why the ipv6 one is not binding
                    ^^^

I meant that it is binding, sorry.

[James Hunt (jamesodhunt)]

Hi Cameron - yes, you were right :-)

Merged - thanks again for your and Kai's work on this!

review: Approve

=== modified file 'ChangeLog'
--- ChangeLog	2014-02-04 11:32:56 +0000
+++ ChangeLog	2014-02-11 02:35:11 +0000
@@ -1,3 +1,10 @@
[phone]   Cameron Norman  <[e-mail]>
+
+	* extra/upstart-socket-bridge.c:
+	  - epoll_watcher(): Fix missing breaks in a switch, move buffer and
+	                     remove its nih_local usage.
+	  - job_add_socket(): added case for name_len being a AF_INET6 address.
+
 2014-02-04  James Hunt  <[e-mail]>
 
 	* extra/man/socket-event.7: Environment variable is
@@ -9,6 +16,15 @@
 	  stealing the console in an container: fopen(3) may not specify
 	  O_NOCTTY and Upstart should not own the console (LP: #1263738).
 
[phone]  Cameron Norman  <[e-mail]>
+
+	* extra/upstart-socket-bridge.c: Fixed indentation, used nih_local.
+	* extra/man/socket-event.7: Added inet6 example.
+
[phone]  Kai Mast <[e-mail]>
+
+	* extra/upstart-socket-bridge.c: Added IPv6 support (LP: #942955).
+
 2014-01-17  James Hunt  <[e-mail]>
 
 	* init/conf.c:

=== modified file 'extra/man/socket-event.7'
--- extra/man/socket-event.7	2014-02-04 11:32:56 +0000
+++ extra/man/socket-event.7	2014-02-11 02:35:11 +0000
@@ -39,7 +39,7 @@
 .\"
 .SH EXAMPLES
 .\"
-.SS Internet socket
+.SS Internet (IPv4) socket
 Start web server when first client connects from localhost:
 .RS
 .nf
@@ -48,6 +48,15 @@
 .fi
 .RE
 .\"
+.SS Internet (IPv6) socket
+Start job when a client connects from localhost:
+.RS
+.nf
+
+start on socket PROTO=inet6 PORT=80 ADDR=::1
+.fi
+.RE
+.\"
 .SS Local socket
 .P
 .RS

=== modified file 'extra/upstart-socket-bridge.c'
--- extra/upstart-socket-bridge.c	2013-10-23 16:33:09 +0000
+++ extra/upstart-socket-bridge.c	2014-02-11 02:35:11 +0000
@@ -67,9 +67,10 @@
 	NihList entry;
 
 	union {
-		struct sockaddr        addr;
-		struct sockaddr_in sin_addr;
-		struct sockaddr_un sun_addr;
+		struct sockaddr         addr;
+		struct sockaddr_in  sin_addr;
+		struct sockaddr_in6 sin6_addr;
+		struct sockaddr_un  sun_addr;
 	};
 	socklen_t addrlen;
 
@@ -295,6 +296,7 @@
 		size_t env_len = 0;
 		char *var;
 		DBusPendingCall *pending_call;
+		char buffer[INET6_ADDRSTRLEN];
 
 		if (event[i].events & EPOLLIN)
 			nih_debug ("%p EPOLLIN", sock);
@@ -308,18 +310,35 @@
 		switch (sock->addr.sa_family) {
 		case AF_INET:
 			NIH_MUST (nih_str_array_add (&env, NULL, &env_len,
-						     "PROTO=inet"));
-
-			var = NIH_MUST (nih_sprintf (NULL, "PORT=%d",
-						     ntohs (sock->sin_addr.sin_port)));
-			NIH_MUST (nih_str_array_addp (&env, NULL, &env_len,
-						      var));
-			nih_discard (var);
-
-			var = NIH_MUST (nih_sprintf (NULL, "ADDR=%s",
-						     inet_ntoa (sock->sin_addr.sin_addr)));
-			NIH_MUST (nih_str_array_addp (&env, NULL, &env_len,
-						      var));
+							"PROTO=inet"));
+
+			var = NIH_MUST (nih_sprintf (NULL, "PORT=%d",
+							ntohs (sock->sin_addr.sin_port)));
+			NIH_MUST (nih_str_array_addp (&env, NULL, &env_len,
+							var));
+			nih_discard (var);
+
+			var = NIH_MUST (nih_sprintf (NULL, "ADDR=%s",
+							inet_ntoa (sock->sin_addr.sin_addr)));
+			NIH_MUST (nih_str_array_addp (&env, NULL, &env_len,
+							var));
+			nih_discard (var);
+			break;
+ 		case AF_INET6:
+			NIH_MUST (nih_str_array_add (&env, NULL, &env_len,
+							"PROTO=inet6"));
+
+			var = NIH_MUST (nih_sprintf (NULL, "PORT=%d",
+							ntohs (sock->sin6_addr.sin6_port)));
+			NIH_MUST (nih_str_array_addp (&env, NULL, &env_len,
+							var));
+			nih_discard (var);
+
+			var = NIH_MUST (nih_sprintf (NULL, "ADDR=%s",
+							inet_ntop(AF_INET6, &sock->sin6_addr.sin6_addr, buffer, INET6_ADDRSTRLEN)));
+
+			NIH_MUST (nih_str_array_addp (&env, NULL, &env_len,
+							var));
 			nih_discard (var);
 			break;
 		case AF_UNIX:
@@ -504,6 +523,11 @@
 				sock->sin_addr.sin_family = AF_INET;
 				sock->sin_addr.sin_addr.s_addr = INADDR_ANY;
 				components = 1;
+			} else if (! strcmp (val, "inet6")) {
+				sock->addrlen = sizeof sock->sin6_addr;
+				sock->sin6_addr.sin6_family = AF_INET6;
+				sock->sin6_addr.sin6_addr = in6addr_any;
+				components = 1;
 			} else if (! strcmp (val, "unix")) {
 				sock->addrlen = sizeof sock->sun_addr;
 				sock->sun_addr.sun_family = AF_UNIX;
@@ -515,22 +539,31 @@
 			}
 
 		} else if (! strncmp (*env, "PORT", name_len)
-			   && (sock->sin_addr.sin_family == AF_INET)) {
+		           && (sock->sin_addr.sin_family == AF_INET)) {
 			sock->sin_addr.sin_port = htons (atoi (val));
 			components--;
-
+		} else if (! strncmp (*env, "PORT", name_len)
+		           && (sock->sin6_addr.sin6_family == AF_INET6)) {
+			sock->sin6_addr.sin6_port = htons (atoi (val));
+			components--;
 		} else if (! strncmp (*env, "ADDR", name_len)
-			   && (sock->sin_addr.sin_family == AF_INET)) {
+		           && (sock->sin_addr.sin_family == AF_INET)) {
 			if (inet_aton (val, &(sock->sin_addr.sin_addr)) == 0) {
 				nih_warn ("Ignored socket event with invalid ADDR=%s in %s",
-					  val, job->path);
-				goto error;
-			}
-
+				          val, job->path);
+				goto error;
+			}
+		} else if (! strncmp (*env, "ADDR", name_len)
+		           && (sock->sin6_addr.sin6_family == AF_INET6)) {
+			if (inet_pton (AF_INET6, val, &(sock->sin6_addr.sin6_addr)) != 1) {
+				nih_warn ("Ignored socket event with invalid ADDR=%s in %s",
+				          val, job->path);
+				goto error;
+			}
 		} else if (! strncmp (*env, "PATH", name_len)
-			   && (sock->sun_addr.sun_family == AF_UNIX)) {
+		           && (sock->sun_addr.sun_family == AF_UNIX)) {
 			strncpy (sock->sun_addr.sun_path, val,
-				 sizeof sock->sun_addr.sun_path);
+			         sizeof sock->sun_addr.sun_path);
 
 			if (sock->sun_addr.sun_path[0] == '@')
 				sock->sun_addr.sun_path[0] = '\0';
@@ -567,6 +600,15 @@
 		goto error;
 	}
 
+	/* If socket is ipv6, need to set IPV6_V6ONLY option */
+	if (sock->sin6_addr.sin6_family == AF_INET6 && 
+	    setsockopt (sock->sock, SOL_IPV6, IPV6_V6ONLY,
+	                &opt, sizeof opt) < 0) {
+		nih_warn ("Failed to set IPV6 only option in %s: %s",
+		          job->path, strerror (errno));
+		goto error;
+	}
+
 	if (bind (sock->sock, &sock->addr, sock->addrlen) < 0) {
 		nih_warn ("Failed to bind socket in %s: %s",
 			  job->path, strerror (errno));