[Swan-dev] PATCH: auto=add + up vs auto=start difference

[Paul Wouters]

I looked into the difference between auto=start and auto=add + ipsec
auto --up. Output captured using: grep command pluto.log

auto=start results in:

Feb  5 21:07:23.461322: | running updown command "ipsec _updown" for verb prepare
Feb  5 21:07:23.461326: | command executing prepare-client
Feb  5 21:07:23.470194: | running updown command "ipsec _updown" for verb route
Feb  5 21:07:23.470214: | command executing route-client
Feb  5 21:07:23.819757: | running updown command "ipsec _updown" for verb up
Feb  5 21:07:23.819771: | command executing up-client

This results in missing routes for a modecfgclient

auto=ignore, --add and --up results in (all work happens on --up)

Feb  5 21:08:45.538574: | running updown command "ipsec _updown" for verb up
Feb  5 21:08:45.538583: | command executing up-client
Feb  5 21:08:45.831816: | running updown command "ipsec _updown" for verb prepare
Feb  5 21:08:45.831822: | command executing prepare-client
Feb  5 21:08:45.846359: | running updown command "ipsec _updown" for verb route
Feb  5 21:08:45.846384: | command executing route-client

Looking at updown.netkey:
- prepare-client only adds VTI interface
- route-client runs addvti and uproute
- up-client runs updateresolvconf and addsource


While i would think prepare -> route -> up is the proper way, it seems
instead the "working" way is up -> prepare -> route.

Since prepare is a no-op without VTI, we are only looking at the order
of route and up. addconn for auto=start calls "add" then "route" then
"up". Possibly that should be "up" then "route", although not very
intuitive. I think the shell verb scripts might have been changed from their
original meaning when things did not work (possibly after addconn work)


So the addconn started by pluto does:

         for each conn:
 		if auto is add/ondemand/start:
 			resolve_defaultroute(conn);
 			starter_whack_add_conn(cfg, conn);

 	/*
 	 * We loaded all connections. Now tell pluto to listen,
 	 * then route the conns and resolve default route.
 	 */
 	starter_whack_listen(cfg);

 	for each conn:
 		if auto is add/ondemand/start
 			# paul: why again, we gave it to pluto, 'conn'
 			# is our local starterconn object
 			resolve_defaultroute(conn);
 		if auto is ondemand/start
 			starter_whack_route_conn(cfg, conn);

 	for each conn:
 		if auto is start
 			starter_whack_initiate_conn(cfg, conn);


So I think the problem is that (probably me when I wrote this code) it
was assumed that auto=start is auto=route+initiate, but probably it is
really just initiate and when done do route. I think because "route"
was already called after "up" the "route" is not called again.

I think we really are talking about two "route" tasks here:

1) setup any halfroutes or sourceip routes before tunnel goes up
2) do required routing once tunnel is up

I think 1) was called verb "route" and 2) was called as part of
verb "up", but it got confusing because we needed routing in up.

Based on that, this is the addconn.c patch that might do more what we
want. It fixes the routes for me when using auto=start and the grep
shows:

Feb  5 22:10:09.779477: | running updown command "ipsec _updown" for verb up 
Feb  5 22:10:09.779483: | command executing up-client
Feb  5 22:10:10.074110: | running updown command "ipsec _updown" for verb prepare 
Feb  5 22:10:10.074117: | command executing prepare-client
Feb  5 22:10:10.083360: | running updown command "ipsec _updown" for verb route 
Feb  5 22:10:10.083380: | command executing route-client

If this also fixes Tuomo's setup, then I think we are ready to push this
change and observe a full testrun to see if anything else happens.
Possibly, some auto=start tests would work better?

Paul
-------------- next part --------------
diff --git a/programs/addconn/addconn.c b/programs/addconn/addconn.c
index ae569727b..e818e0e35 100644
--- a/programs/addconn/addconn.c
+++ b/programs/addconn/addconn.c
@@ -416,12 +416,11 @@ int main(int argc, char *argv[])
 		if (verbose)
 			printf("  Pass #1: Loading auto=add, auto=route and auto=start connections\n");
 
-		for (conn = cfg->conns.tqh_first;
-			conn != NULL;
-			conn = conn->link.tqe_next) {
+		for (conn = cfg->conns.tqh_first; conn != NULL; conn = conn->link.tqe_next) {
 			if (conn->desired_state == STARTUP_ADD ||
 				conn->desired_state == STARTUP_ONDEMAND ||
-				conn->desired_state == STARTUP_START) {
+				conn->desired_state == STARTUP_START)
+			{
 				if (verbose)
 					printf(" %s", conn->name);
 				resolve_defaultroute(conn);
@@ -436,30 +435,22 @@ int main(int argc, char *argv[])
 		starter_whack_listen(cfg);
 
 		if (verbose)
-			printf("  Pass #2: Routing auto=route and auto=start connections\n");
+			printf("  Pass #2: Routing auto=route connections\n");
 
-		for (conn = cfg->conns.tqh_first;
-			conn != NULL;
-			conn = conn->link.tqe_next) {
-			if (conn->desired_state == STARTUP_ADD ||
-				conn->desired_state == STARTUP_ONDEMAND ||
-				conn->desired_state == STARTUP_START) {
+		for (conn = cfg->conns.tqh_first; conn != NULL; conn = conn->link.tqe_next) {
+			if (conn->desired_state == STARTUP_ONDEMAND)
+			{
 				if (verbose)
 					printf(" %s", conn->name);
-				resolve_defaultroute(conn);
-				if (conn->desired_state == STARTUP_ONDEMAND ||
-				    conn->desired_state == STARTUP_START) {
+				if (conn->desired_state == STARTUP_ONDEMAND)
 					starter_whack_route_conn(cfg, conn);
-				}
 			}
 		}
 
 		if (verbose)
 			printf("  Pass #3: Initiating auto=start connections\n");
 
-		for (conn = cfg->conns.tqh_first;
-			conn != NULL;
-			conn = conn->link.tqe_next) {
+		for (conn = cfg->conns.tqh_first; conn != NULL; conn = conn->link.tqe_next) {
 			if (conn->desired_state == STARTUP_START) {
 				if (verbose)
 					printf(" %s", conn->name);